Merge remote-tracking branch 'origin/authz-roles' into authz-role-ass…

…ignments

common/src/api/external/mod.rs

@@ -296,6 +296,90 @@ impl Name {
     }
 }
 
+/**
+ * Name for a built-in role
+ */
+#[derive(
+    Clone,
+    Debug,
+    DeserializeFromStr,
+    Display,
+    Eq,
+    FromStr,
+    Ord,
+    PartialEq,
+    PartialOrd,
+    SerializeDisplay,
+)]
+#[display("{resource_type}.{role_name}")]
+pub struct RoleName {
+    // "resource_type" is generally the String value of one of the
+    // `ResourceType` variants.  We could store the parsed `ResourceType`
+    // instead, but it's useful to be able to represent RoleNames for resource
+    // types that we don't know about.  That could happen if we happen to find
+    // them in the database, for example.
+    #[from_str(regex = "[a-z-]+")]
+    resource_type: String,
+    #[from_str(regex = "[a-z-]+")]
+    role_name: String,
+}
+
+impl RoleName {
+    pub fn new(resource_type: &str, role_name: &str) -> RoleName {
+        RoleName {
+            resource_type: String::from(resource_type),
+            role_name: String::from(role_name),
+        }
+    }
+}
+
+/**
+ * Custom JsonSchema implementation to encode the constraints on Name
+ */
+/* TODO see TODOs on Name above */
+impl JsonSchema for RoleName {
+    fn schema_name() -> String {
+        "RoleName".to_string()
+    }
+    fn json_schema(
+        _gen: &mut schemars::gen::SchemaGenerator,
+    ) -> schemars::schema::Schema {
+        schemars::schema::Schema::Object(schemars::schema::SchemaObject {
+            metadata: Some(Box::new(schemars::schema::Metadata {
+                id: None,
+                title: Some("A name for a built-in role".to_string()),
+                description: Some(
+                    "Role names consist of two string components \
+                     separated by dot (\".\")."
+                        .to_string(),
+                ),
+                default: None,
+                deprecated: false,
+                read_only: false,
+                write_only: false,
+                examples: vec![],
+            })),
+            instance_type: Some(schemars::schema::SingleOrVec::Single(
+                Box::new(schemars::schema::InstanceType::String),
+            )),
+            format: None,
+            enum_values: None,
+            const_value: None,
+            subschemas: None,
+            number: None,
+            string: Some(Box::new(schemars::schema::StringValidation {
+                max_length: Some(63),
+                min_length: None,
+                pattern: Some("[a-z-]+\\.[a-z-]+".to_string()),
+            })),
+            array: None,
+            object: None,
+            reference: None,
+            extensions: BTreeMap::new(),
+        })
+    }
+}
+
 /**
  * A count of bytes, typically used either for memory or storage capacity
  *
@@ -466,15 +550,25 @@ impl TryFrom<i64> for Generation {
  * Identifies a type of API resource
  */
 #[derive(
-    Clone, Copy, Debug, Deserialize, Eq, Ord, PartialEq, PartialOrd, Serialize,
+    Clone,
+    Copy,
+    Debug,
+    DeserializeFromStr,
+    Display,
+    Eq,
+    FromStr,
+    Ord,
+    PartialEq,
+    PartialOrd,
+    SerializeDisplay,
 )]
+#[display(style = "kebab-case")]
 pub enum ResourceType {
     Fleet,
     Organization,
     Project,
     Dataset,
     Disk,
-    DiskAttachment,
     Instance,
     NetworkInterface,
     Rack,
@@ -492,38 +586,6 @@ pub enum ResourceType {
     Zpool,
 }
 
-impl Display for ResourceType {
-    fn fmt(&self, f: &mut Formatter) -> FormatResult {
-        write!(
-            f,
-            "{}",
-            match self {
-                ResourceType::Fleet => "fleet",
-                ResourceType::Organization => "organization",
-                ResourceType::Project => "project",
-                ResourceType::Dataset => "dataset",
-                ResourceType::Disk => "disk",
-                ResourceType::DiskAttachment => "disk attachment",
-                ResourceType::Instance => "instance",
-                ResourceType::NetworkInterface => "network interface",
-                ResourceType::Rack => "rack",
-                ResourceType::Sled => "sled",
-                ResourceType::SagaDbg => "saga_dbg",
-                ResourceType::Vpc => "vpc",
-                ResourceType::VpcFirewallRule => "vpc firewall rule",
-                ResourceType::VpcSubnet => "vpc subnet",
-                ResourceType::VpcRouter => "vpc router",
-                ResourceType::RouterRoute => "vpc router route",
-                ResourceType::Oximeter => "oximeter",
-                ResourceType::MetricProducer => "metric producer",
-                ResourceType::Role => "role",
-                ResourceType::User => "user",
-                ResourceType::Zpool => "zpool",
-            }
-        )
-    }
-}
-
 pub async fn to_list<T, U>(object_stream: ObjectStream<T>) -> Vec<U>
 where
     T: Into<U>,
@@ -873,18 +935,6 @@ impl DiskState {
     }
 }
 
-/**
- * Describes a Disk's attachment to an Instance
- */
-#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]
-#[serde(rename_all = "camelCase")]
-pub struct DiskAttachment {
-    pub instance_id: Uuid,
-    pub disk_id: Uuid,
-    pub disk_name: Name,
-    pub disk_state: DiskState,
-}
-
 /*
  * Sagas
  *
@@ -1861,13 +1911,14 @@ pub struct NetworkInterface {
 #[cfg(test)]
 mod test {
     use super::{
-        ByteCount, L4Port, L4PortRange, Name, NetworkTarget,
+        ByteCount, L4Port, L4PortRange, Name, NetworkTarget, RoleName,
         VpcFirewallRuleAction, VpcFirewallRuleDirection, VpcFirewallRuleFilter,
         VpcFirewallRuleHostFilter, VpcFirewallRulePriority,
         VpcFirewallRuleProtocol, VpcFirewallRuleStatus, VpcFirewallRuleTarget,
         VpcFirewallRuleUpdate, VpcFirewallRuleUpdateParams,
     };
     use crate::api::external::Error;
+    use crate::api::external::ResourceType;
     use std::convert::TryFrom;
     use std::net::IpAddr;
     use std::net::Ipv4Addr;
@@ -1920,6 +1971,83 @@ mod test {
         }
     }
 
+    #[test]
+    fn test_role_name_parse() {
+        // Error cases
+        let bad_inputs = vec![
+            // empty string is always worth testing
+            "",
+            // missing dot
+            "project",
+            // extra dot (or, illegal character in the second component)
+            "project.admin.super",
+            // missing resource type (or, another bogus resource type)
+            ".admin",
+            // missing role name
+            "project.",
+            // illegal characters in role name
+            "project.not_good",
+        ];
+
+        for input in bad_inputs {
+            eprintln!("check name {:?} (expecting error)", input);
+            let result =
+                input.parse::<RoleName>().expect_err("unexpectedly succeeded");
+            eprintln!("(expected) error: {:?}", result);
+        }
+
+        eprintln!("check name \"project.admin\" (expecting success)");
+        let role_name =
+            "project.admin".parse::<RoleName>().expect("failed to parse");
+        assert_eq!(role_name.to_string(), "project.admin");
+        assert_eq!(role_name.resource_type, "project");
+        assert_eq!(role_name.role_name, "admin");
+
+        eprintln!("check name \"barf.admin\" (expecting success)");
+        let role_name =
+            "barf.admin".parse::<RoleName>().expect("failed to parse");
+        assert_eq!(role_name.to_string(), "barf.admin");
+        assert_eq!(role_name.resource_type, "barf");
+        assert_eq!(role_name.role_name, "admin");
+
+        eprintln!("check name \"organization.super-user\" (expecting success)");
+        let role_name = "organization.super-user"
+            .parse::<RoleName>()
+            .expect("failed to parse");
+        assert_eq!(role_name.to_string(), "organization.super-user");
+        assert_eq!(role_name.resource_type, "organization");
+        assert_eq!(role_name.role_name, "super-user");
+    }
+
+    #[test]
+    fn test_resource_name_parse() {
+        let bad_inputs = vec![
+            "bogus",
+            "",
+            "Project",
+            "oRgAnIzAtIoN",
+            "organisation",
+            "vpc subnet",
+            "vpc_subnet",
+        ];
+        for input in bad_inputs {
+            eprintln!("check resource type {:?} (expecting error)", input);
+            let result = input
+                .parse::<ResourceType>()
+                .expect_err("unexpectedly succeeded");
+            eprintln!("(expected) error: {:?}", result);
+        }
+
+        assert_eq!(
+            ResourceType::Project,
+            "project".parse::<ResourceType>().unwrap()
+        );
+        assert_eq!(
+            ResourceType::VpcSubnet,
+            "vpc-subnet".parse::<ResourceType>().unwrap()
+        );
+    }
+
     #[test]
     fn test_name_parse_from_param() {
         let result = Name::from_param(String::from("my-name"), "the_name");

common/src/sql/dbinit.sql

@@ -735,7 +735,7 @@ INSERT INTO omicron.public.user_builtin (
  * other tables.  But they're just different enough that we can't use most of
  * the same abstractions:
  *
- * * "id": We have no need for a uuid because the (role_name, full_name) is
+ * * "id": We have no need for a uuid because the (resource_type, role_name) is
  *   already unique and immutable.
  * * "name": What we call "full name" above could instead be called "name",
  *   which would be consistent with other identity metadata.  But it's not a

nexus/src/db/datastore.rs

@@ -67,15 +67,16 @@ use crate::db::{
         public_error_from_diesel_pool_shouldnt_fail,
     },
     model::{
-        ConsoleSession, Dataset, Disk, DiskAttachment, DiskRuntimeState,
-        Generation, IncompleteNetworkInterface, Instance, InstanceRuntimeState,
-        Name, NetworkInterface, Organization, OrganizationUpdate, OximeterInfo,
+        ConsoleSession, Dataset, Disk, DiskRuntimeState, Generation,
+        IncompleteNetworkInterface, Instance, InstanceRuntimeState, Name,
+        NetworkInterface, Organization, OrganizationUpdate, OximeterInfo,
         ProducerEndpoint, Project, ProjectUpdate, RoleAssignmentBuiltin,
         RoleBuiltin, RouterRoute, RouterRouteUpdate, Sled, UserBuiltin, Vpc,
         VpcFirewallRule, VpcRouter, VpcRouterUpdate, VpcSubnet,
         VpcSubnetUpdate, VpcUpdate, Zpool,
     },
     pagination::paginated,
+    pagination::paginated_multicolumn,
     subnet_allocation::AllocateIpQuery,
     update_and_check::{UpdateAndCheck, UpdateStatus},
 };
@@ -861,7 +862,7 @@ impl DataStore {
         &self,
         instance_id: &Uuid,
         pagparams: &DataPageParams<'_, Name>,
-    ) -> ListResultVec<DiskAttachment> {
+    ) -> ListResultVec<Disk> {
         use db::schema::disk::dsl;
 
         paginated(dsl::disk, dsl::name, &pagparams)
@@ -870,13 +871,6 @@ impl DataStore {
             .select(Disk::as_select())
             .load_async::<Disk>(self.pool())
             .await
-            .map(|disks| {
-                disks
-                    .into_iter()
-                    // Unwrap safety: filtered by instance_id in query.
-                    .map(|disk| disk.attachment().unwrap())
-                    .collect()
-            })
             .map_err(|e| {
                 public_error_from_diesel_pool(
                     e,
@@ -2168,49 +2162,21 @@ impl DataStore {
     ) -> ListResultVec<RoleBuiltin> {
         use db::schema::role_builtin::dsl;
         opctx.authorize(authz::Action::ListChildren, authz::FLEET).await?;
-
-        // TODO-column This is essentially a multi-column version of
-        // `paginated`.  It would be better to turn this into a function.  It's
-        // not obvious how to do that.
-        let mut query =
-            dsl::role_builtin.into_boxed().limit(pagparams.limit.get().into());
-        let query = match pagparams.direction {
-            dropshot::PaginationOrder::Ascending => {
-                if let Some((v1, v2)) = &pagparams.marker {
-                    query = query.filter(
-                        (dsl::resource_type
-                            .eq(v1.clone())
-                            .and(dsl::role_name.gt(v2.clone())))
-                        .or(dsl::resource_type.gt(v1.clone())),
-                    )
-                }
-
-                query.order((dsl::resource_type.asc(), dsl::role_name.asc()))
-            }
-            dropshot::PaginationOrder::Descending => {
-                if let Some((v1, v2)) = &pagparams.marker {
-                    query = query.filter(
-                        (dsl::resource_type
-                            .eq(v1.clone())
-                            .and(dsl::role_name.lt(v2.clone())))
-                        .or(dsl::resource_type.lt(v1.clone())),
-                    )
-                }
-                query.order((dsl::resource_type.desc(), dsl::role_name.desc()))
-            }
-        };
-
-        query
-            .select(RoleBuiltin::as_select())
-            .load_async::<RoleBuiltin>(self.pool_authorized(opctx).await?)
-            .await
-            .map_err(|e| {
-                public_error_from_diesel_pool(
-                    e,
-                    ResourceType::Role,
-                    LookupType::Other("Listing All".to_string()),
-                )
-            })
+        paginated_multicolumn(
+            dsl::role_builtin,
+            (dsl::resource_type, dsl::role_name),
+            pagparams,
+        )
+        .select(RoleBuiltin::as_select())
+        .load_async::<RoleBuiltin>(self.pool_authorized(opctx).await?)
+        .await
+        .map_err(|e| {
+            public_error_from_diesel_pool(
+                e,
+                ResourceType::Role,
+                LookupType::Other("Listing All".to_string()),
+            )
+        })
     }
 
     pub async fn role_builtin_fetch(