stop using Display/FromStr for database representations of per-resour…

…ce roles (#1080)

nexus/Cargo.toml

@@ -127,6 +127,7 @@ regex = "1.5.5"
 subprocess = "0.2.9"
 term = "0.7"
 httptest = "0.15.4"
+strum = { version = "0.23", features = [ "derive" ] }
 
 [dev-dependencies.openapi-lint]
 git = "https://github.com/oxidecomputer/openapi-lint"

nexus/src/authz/api_resources.rs

@@ -36,9 +36,11 @@ use super::Action;
 use super::{actor::AuthenticatedActor, Authz};
 use crate::authn;
 use crate::context::OpContext;
+use crate::db;
 use crate::db::fixed_data::FLEET_ID;
 use crate::db::model::UpdateArtifactKind;
 use crate::db::DataStore;
+use anyhow::anyhow;
 use authz_macros::authz_resource;
 use futures::future::BoxFuture;
 use futures::FutureExt;
@@ -48,6 +50,8 @@ use parse_display::Display;
 use parse_display::FromStr;
 use schemars::JsonSchema;
 use serde::{Deserialize, Serialize};
+#[cfg(test)]
+use strum::EnumIter;
 use uuid::Uuid;
 
 /// Describes an authz resource that corresponds to an API resource that has a
@@ -83,9 +87,9 @@ pub trait ApiResourceWithRoles: ApiResource {
 
 /// Describes the specific roles for an `ApiResourceWithRoles`
 pub trait ApiResourceWithRolesType: ApiResourceWithRoles {
-    type AllowedRoles: std::fmt::Display
-        + serde::Serialize
-        + serde::de::DeserializeOwned;
+    type AllowedRoles: serde::Serialize
+        + serde::de::DeserializeOwned
+        + db::model::DatabaseString;
 }
 
 impl<T: ApiResource + oso::ToPolar + Clone> AuthorizedResource for T {
@@ -202,18 +206,9 @@ impl ApiResourceWithRolesType for Fleet {
 }
 
 #[derive(
-    Clone,
-    Copy,
-    Debug,
-    Deserialize,
-    Display,
-    Eq,
-    FromStr,
-    PartialEq,
-    Serialize,
-    JsonSchema,
+    Clone, Copy, Debug, Deserialize, Eq, PartialEq, Serialize, JsonSchema,
 )]
-#[display(style = "kebab-case")]
+#[cfg_attr(test, derive(EnumIter))]
 #[serde(rename_all = "snake_case")]
 pub enum FleetRoles {
     Admin,
@@ -223,6 +218,27 @@ pub enum FleetRoles {
     // they do not show up in this enum.
 }
 
+impl db::model::DatabaseString for FleetRoles {
+    type Error = anyhow::Error;
+
+    fn to_database_string(&self) -> &str {
+        match self {
+            FleetRoles::Admin => "admin",
+            FleetRoles::Collaborator => "collaborator",
+            FleetRoles::Viewer => "viewer",
+        }
+    }
+
+    fn from_database_string(s: &str) -> Result<Self, Self::Error> {
+        match s {
+            "admin" => Ok(FleetRoles::Admin),
+            "collaborator" => Ok(FleetRoles::Collaborator),
+            "viewer" => Ok(FleetRoles::Viewer),
+            _ => Err(anyhow!("unsupported Fleet role from database: {:?}", s)),
+        }
+    }
+}
+
 /// ConsoleSessionList is a synthetic resource used for modeling who has access
 /// to create sessions.
 #[derive(Clone, Copy, Debug, Eq, PartialEq)]
@@ -369,13 +385,36 @@ impl ApiResourceWithRolesType for Organization {
     Serialize,
     JsonSchema,
 )]
+#[cfg_attr(test, derive(EnumIter))]
 #[display(style = "kebab-case")]
 #[serde(rename_all = "snake_case")]
 pub enum OrganizationRoles {
     Admin,
     Collaborator,
 }
 
+impl db::model::DatabaseString for OrganizationRoles {
+    type Error = anyhow::Error;
+
+    fn to_database_string(&self) -> &str {
+        match self {
+            OrganizationRoles::Admin => "admin",
+            OrganizationRoles::Collaborator => "collaborator",
+        }
+    }
+
+    fn from_database_string(s: &str) -> Result<Self, Self::Error> {
+        match s {
+            "admin" => Ok(OrganizationRoles::Admin),
+            "collaborator" => Ok(OrganizationRoles::Collaborator),
+            _ => Err(anyhow!(
+                "unsupported Organization role from database: {:?}",
+                s
+            )),
+        }
+    }
+}
+
 authz_resource! {
     name = "Project",
     parent = "Organization",
@@ -400,6 +439,7 @@ impl ApiResourceWithRolesType for Project {
     Serialize,
     JsonSchema,
 )]
+#[cfg_attr(test, derive(EnumIter))]
 #[display(style = "kebab-case")]
 #[serde(rename_all = "snake_case")]
 pub enum ProjectRoles {
@@ -408,6 +448,29 @@ pub enum ProjectRoles {
     Viewer,
 }
 
+impl db::model::DatabaseString for ProjectRoles {
+    type Error = anyhow::Error;
+
+    fn to_database_string(&self) -> &str {
+        match self {
+            ProjectRoles::Admin => "admin",
+            ProjectRoles::Collaborator => "collaborator",
+            ProjectRoles::Viewer => "viewer",
+        }
+    }
+
+    fn from_database_string(s: &str) -> Result<Self, Self::Error> {
+        match s {
+            "admin" => Ok(ProjectRoles::Admin),
+            "collaborator" => Ok(ProjectRoles::Collaborator),
+            "viewer" => Ok(ProjectRoles::Viewer),
+            _ => {
+                Err(anyhow!("unsupported Project role from database: {:?}", s))
+            }
+        }
+    }
+}
+
 authz_resource! {
     name = "Disk",
     parent = "Project",
@@ -522,6 +585,7 @@ impl ApiResourceWithRolesType for Silo {
     Serialize,
     JsonSchema,
 )]
+#[cfg_attr(test, derive(EnumIter))]
 #[display(style = "kebab-case")]
 #[serde(rename_all = "snake_case")]
 pub enum SiloRoles {
@@ -530,6 +594,27 @@ pub enum SiloRoles {
     Viewer,
 }
 
+impl db::model::DatabaseString for SiloRoles {
+    type Error = anyhow::Error;
+
+    fn to_database_string(&self) -> &str {
+        match self {
+            SiloRoles::Admin => "admin",
+            SiloRoles::Collaborator => "collaborator",
+            SiloRoles::Viewer => "viewer",
+        }
+    }
+
+    fn from_database_string(s: &str) -> Result<Self, Self::Error> {
+        match s {
+            "admin" => Ok(SiloRoles::Admin),
+            "collaborator" => Ok(SiloRoles::Collaborator),
+            "viewer" => Ok(SiloRoles::Viewer),
+            _ => Err(anyhow!("unsupported Silo role from database: {:?}", s)),
+        }
+    }
+}
+
 authz_resource! {
     name = "SiloUser",
     parent = "Silo",
@@ -569,3 +654,28 @@ authz_resource! {
     roles_allowed = false,
     polar_snippet = FleetChild,
 }
+
+#[cfg(test)]
+mod test {
+    use super::FleetRoles;
+    use super::OrganizationRoles;
+    use super::ProjectRoles;
+    use super::SiloRoles;
+    use crate::db::model::test_database_string_impl;
+
+    #[test]
+    fn test_roles_database_strings() {
+        test_database_string_impl::<FleetRoles, _>(
+            "tests/output/authz-roles-fleet.txt",
+        );
+        test_database_string_impl::<SiloRoles, _>(
+            "tests/output/authz-roles-silo.txt",
+        );
+        test_database_string_impl::<OrganizationRoles, _>(
+            "tests/output/authz-roles-organization.txt",
+        );
+        test_database_string_impl::<ProjectRoles, _>(
+            "tests/output/authz-roles-project.txt",
+        );
+    }
+}

nexus/src/db/datastore.rs

@@ -32,6 +32,7 @@ use crate::db::fixed_data::role_assignment::BUILTIN_ROLE_ASSIGNMENTS;
 use crate::db::fixed_data::role_builtin::BUILTIN_ROLES;
 use crate::db::fixed_data::silo::{DEFAULT_SILO, SILO_ID};
 use crate::db::lookup::LookupPath;
+use crate::db::model::DatabaseString;
 use crate::db::{
     self,
     error::{public_error_from_diesel_pool, ErrorHandler, TransactionError},
@@ -2983,14 +2984,15 @@ impl DataStore {
     // tricky without first-classing the Policy in the database.  The impact is
     // mitigated because we cap the number of role assignments per resource
     // pretty tightly.
-    pub async fn role_assignment_replace_visible<
-        T: authz::ApiResourceWithRolesType + Clone,
-    >(
+    pub async fn role_assignment_replace_visible<T>(
         &self,
         opctx: &OpContext,
         authz_resource: &T,
         new_assignments: &[shared::RoleAssignment<T::AllowedRoles>],
-    ) -> ListResultVec<db::model::RoleAssignment> {
+    ) -> ListResultVec<db::model::RoleAssignment>
+    where
+        T: authz::ApiResourceWithRolesType + Clone,
+    {
         // TODO-security We should carefully review what permissions are
         // required for modifying the policy of a resource.
         opctx.authorize(authz::Action::ModifyPolicy, authz_resource).await?;
@@ -3013,7 +3015,7 @@ impl DataStore {
                     r.identity_id,
                     resource_type,
                     resource_id,
-                    &r.role_name.to_string(),
+                    &r.role_name.to_database_string(),
                 )
             })
             .collect::<Vec<_>>();

nexus/src/db/model/mod.rs

@@ -218,6 +218,95 @@ macro_rules! impl_enum_type {
 
 pub(crate) use impl_enum_type;
 
+/// Describes a type that's represented in the database using a String
+///
+/// If you're reaching for this type, consider whether it'd be better to use an
+/// enum in the database, along with `impl_enum_wrapper!` to define a
+/// corresponding Rust type.  This trait is really intended for cases where
+/// that's not possible (e.g., because the set of allowed values isn't the same
+/// for all rows in the table).
+///
+/// For a given type, the impl of `DatabaseString` is potentially different than
+/// the impl of `Serialize` (which is usually how the type appears in an API)
+/// and the impl of `Display` (if the type even has one).  We could piggy-back
+/// on `Display`, but it'd be a footgun in that someone might change `Display`
+/// thinking it wouldn't have much impact, not realizing that it'd be breaking
+/// an on-disk interface.
+pub trait DatabaseString: Sized {
+    type Error: std::fmt::Display;
+
+    fn to_database_string(&self) -> &str;
+    fn from_database_string(s: &str) -> Result<Self, Self::Error>;
+}
+
+/// Does some basic smoke checks on an impl of `DatabaseString`
+///
+/// This tests:
+///
+/// - that for every variant, if we serialize it and deserialize the result, we
+///   get back the original variant
+/// - that if we attempt to deserialize some _other_ input, we get back an error
+/// - that the serialized form for each variant matches what's found in
+///   `expected_output_file`.  This output file is generated by this test using
+///   expectorate.
+///
+/// This cannot completely test the correctness of the implementation, but it
+/// can catch some basic copy/paste errors and accidental compatibility
+/// breakage.
+#[cfg(test)]
+pub fn test_database_string_impl<T, P>(expected_output_file: P)
+where
+    T: std::fmt::Debug + PartialEq + DatabaseString + strum::IntoEnumIterator,
+    P: std::convert::AsRef<std::path::Path>,
+{
+    let mut output = String::new();
+    let mut maxlen: Option<usize> = None;
+
+    for variant in T::iter() {
+        // Serialize the variant.  Verify that we can deserialize the thing we
+        // just got back.
+        let serialized = variant.to_database_string();
+        let deserialized =
+            T::from_database_string(serialized).unwrap_or_else(|_| {
+                panic!(
+                    "failed to deserialize the string {:?}, which we \
+                    got by serializing {:?}",
+                    serialized, variant
+                )
+            });
+        assert_eq!(variant, deserialized);
+
+        // Put the serialized form into "output".  At the end, we'll compare
+        // this to the expected output.  This will fail if somebody has
+        // incompatibly changed things.
+        output.push_str(&format!(
+            "variant {:?}: serialized form = {}\n",
+            variant, serialized
+        ));
+
+        // Keep track of the maximum length of the serialized strings.  We'll
+        // use this to construct an input to `from_database_string()` that was
+        // not emitted by any of the variants' `to_database_string()` functions.
+        match (maxlen, serialized.len()) {
+            (None, newlen) => maxlen = Some(newlen),
+            (Some(oldlen), newlen) if newlen > oldlen => maxlen = Some(newlen),
+            _ => (),
+        }
+    }
+
+    // Check that `from_database_string()` fails when given input that doesn't
+    // match any of the variants' serialized forms.  We construct this input by
+    // providing a string that's longer than all strings emitted by
+    // `to_database_string()`.
+    if let Some(maxlen) = maxlen {
+        let input = String::from_utf8(vec![b'-'; maxlen + 1]).unwrap();
+        T::from_database_string(&input)
+            .expect_err("expected failure to deserialize unknown string");
+    }
+
+    expectorate::assert_contents(expected_output_file, &output);
+}
+
 #[cfg(test)]
 mod tests {
     use super::VpcSubnet;