add unit test for authz policy (#1123)

nexus/src/authn/mod.rs

@@ -204,15 +204,15 @@ impl Context {
     /// (for testing only)
     #[cfg(test)]
     pub fn unprivileged_test_user() -> Context {
-        Self::test_silo_user(
-            USER_TEST_UNPRIVILEGED.silo_id,
+        Context::for_test_user(
             USER_TEST_UNPRIVILEGED.id(),
+            USER_TEST_UNPRIVILEGED.silo_id,
         )
     }
 
-    /// Returns an authenticated context for a given silo user
+    /// Returns an authenticated context for the specific Silo user.
     #[cfg(test)]
-    pub fn test_silo_user(silo_id: Uuid, silo_user_id: Uuid) -> Context {
+    pub fn for_test_user(silo_user_id: Uuid, silo_id: Uuid) -> Context {
         Context {
             kind: Kind::Authenticated(Details {
                 actor: Actor::SiloUser { silo_user_id, silo_id },

nexus/src/authz/api_resources.rs

@@ -46,6 +46,7 @@ use futures::future::BoxFuture;
 use futures::FutureExt;
 use lazy_static::lazy_static;
 use omicron_common::api::external::{Error, LookupType, ResourceType};
+use oso::PolarClass;
 use parse_display::Display;
 use parse_display::FromStr;
 use schemars::JsonSchema;
@@ -93,7 +94,7 @@ pub trait ApiResourceWithRolesType: ApiResourceWithRoles {
         + Clone;
 }
 
-impl<T: ApiResource + oso::ToPolar + Clone> AuthorizedResource for T {
+impl<T: ApiResource + oso::PolarClass + Clone> AuthorizedResource for T {
     fn load_roles<'a, 'b, 'c, 'd, 'e, 'f>(
         &'a self,
         opctx: &'b OpContext,
@@ -135,6 +136,10 @@ impl<T: ApiResource + oso::ToPolar + Clone> AuthorizedResource for T {
             Ok(true) => error,
         }
     }
+
+    fn polar_class(&self) -> oso::Class {
+        Self::get_polar_class()
+    }
 }
 
 /// Represents the Oxide fleet for authz purposes
@@ -299,6 +304,10 @@ impl AuthorizedResource for ConsoleSessionList {
     ) -> Error {
         error
     }
+
+    fn polar_class(&self) -> oso::Class {
+        Self::get_polar_class()
+    }
 }
 
 #[derive(Clone, Copy, Debug)]
@@ -360,6 +369,10 @@ impl AuthorizedResource for GlobalImageList {
     ) -> Error {
         error
     }
+
+    fn polar_class(&self) -> oso::Class {
+        Self::get_polar_class()
+    }
 }
 
 #[derive(Clone, Copy, Debug)]
@@ -422,6 +435,10 @@ impl AuthorizedResource for IpPoolList {
     ) -> Error {
         error
     }
+
+    fn polar_class(&self) -> oso::Class {
+        Self::get_polar_class()
+    }
 }
 
 #[derive(Clone, Copy, Debug, Eq, PartialEq)]
@@ -476,6 +493,10 @@ impl AuthorizedResource for DeviceAuthRequestList {
     ) -> Error {
         error
     }
+
+    fn polar_class(&self) -> oso::Class {
+        Self::get_polar_class()
+    }
 }
 
 // Main resource hierarchy: Organizations, Projects, and their resources

nexus/src/authz/context.rs

@@ -50,6 +50,11 @@ impl Authz {
     {
         self.oso.is_allowed(actor.clone(), action, resource.clone())
     }
+
+    #[cfg(test)]
+    pub fn into_class_names(self) -> BTreeSet<String> {
+        self.class_names
+    }
 }
 
 /// Operation-specific authorization context
@@ -81,7 +86,7 @@ impl Context {
         resource: Resource,
     ) -> Result<(), Error>
     where
-        Resource: AuthorizedResource + oso::PolarClass + Clone,
+        Resource: AuthorizedResource + Clone,
     {
         // If we're given a resource whose PolarClass was never registered with
         // Oso, then the call to `is_allowed()` below will always return false
@@ -97,7 +102,7 @@ impl Context {
         // of a programmer error than an operational error.  But unlike most
         // programmer errors, the nature of the problem and the blast radius are
         // well understood, so we may as well avoid crashing.)
-        let class_name = &Resource::get_polar_class().name;
+        let class_name = &resource.polar_class().name;
         bail_unless!(
             self.authz.class_names.contains(class_name),
             "attempted authz check on unregistered resource: {:?}",
@@ -181,23 +186,17 @@ pub trait AuthorizedResource: oso::ToPolar + Send + Sync + 'static {
         actor: AnyActor,
         action: Action,
     ) -> Error;
+
+    /// Returns the Polar class that implements this resource
+    fn polar_class(&self) -> oso::Class;
 }
 
 #[cfg(test)]
 mod test {
-    // These are essentially unit tests for the policy itself.
-    // TODO-coverage This is just a start.  But we need better support for role
-    // assignments for non-built-in users to do more here.
-    // TODO If this gets any more complicated, we could consider automatically
-    // generating the test cases.  We could precreate a bunch of resources and
-    // some users with different roles.  Then we could run through a table that
-    // says exactly which users should be able to do what to each resource.
     use crate::authn;
     use crate::authz::Action;
     use crate::authz::Authz;
     use crate::authz::Context;
-    use crate::authz::DATABASE;
-    use crate::authz::FLEET;
     use crate::db::DataStore;
     use nexus_test_utils::db::test_setup_database;
     use omicron_test_utils::dev;
@@ -212,102 +211,6 @@ mod test {
         Context::new(Arc::new(authn), Arc::new(authz), datastore)
     }
 
-    fn authz_context_noauth(
-        log: &slog::Logger,
-        datastore: Arc<DataStore>,
-    ) -> Context {
-        let authn = authn::Context::internal_unauthenticated();
-        let authz = Authz::new(log);
-        Context::new(Arc::new(authn), Arc::new(authz), datastore)
-    }
-
-    #[tokio::test]
-    async fn test_database() {
-        let logctx = dev::test_setup_log("test_database");
-        let mut db = test_setup_database(&logctx.log).await;
-        let (opctx, datastore) =
-            crate::db::datastore::datastore_test(&logctx, &db).await;
-        let authz_privileged = authz_context_for_actor(
-            &logctx.log,
-            authn::Context::privileged_test_user(),
-            Arc::clone(&datastore),
-        );
-        authz_privileged
-            .authorize(&opctx, Action::Query, DATABASE)
-            .await
-            .expect("expected privileged user to be able to query database");
-        let error = authz_privileged
-            .authorize(&opctx, Action::Modify, DATABASE)
-            .await
-            .expect_err(
-                "expected privileged test user not to be able to modify \
-                database",
-            );
-        assert!(matches!(
-            error,
-            omicron_common::api::external::Error::Forbidden
-        ));
-        let authz_nobody = authz_context_for_actor(
-            &logctx.log,
-            authn::Context::unprivileged_test_user(),
-            Arc::clone(&datastore),
-        );
-        authz_nobody
-            .authorize(&opctx, Action::Query, DATABASE)
-            .await
-            .expect("expected unprivileged user to be able to query database");
-        let authz_noauth = authz_context_noauth(&logctx.log, datastore);
-        authz_noauth
-            .authorize(&opctx, Action::Query, DATABASE)
-            .await
-            .expect_err(
-            "expected unauthenticated user not to be able to query database",
-        );
-        db.cleanup().await.unwrap();
-        logctx.cleanup_successful();
-    }
-
-    #[tokio::test]
-    async fn test_organization() {
-        let logctx = dev::test_setup_log("test_organization");
-        let mut db = test_setup_database(&logctx.log).await;
-        let (opctx, datastore) =
-            crate::db::datastore::datastore_test(&logctx, &db).await;
-
-        let authz_privileged = authz_context_for_actor(
-            &logctx.log,
-            authn::Context::privileged_test_user(),
-            Arc::clone(&datastore),
-        );
-        authz_privileged
-            .authorize(&opctx, Action::CreateChild, FLEET)
-            .await
-            .expect(
-                "expected privileged user to be able to create organization",
-            );
-        let authz_nobody = authz_context_for_actor(
-            &logctx.log,
-            authn::Context::unprivileged_test_user(),
-            Arc::clone(&datastore),
-        );
-        authz_nobody
-            .authorize(&opctx, Action::CreateChild, FLEET)
-            .await
-            .expect_err(
-            "expected unprivileged user not to be able to create organization",
-        );
-        let authz_noauth = authz_context_noauth(&logctx.log, datastore);
-        authz_noauth
-            .authorize(&opctx, Action::Query, DATABASE)
-            .await
-            .expect_err(
-                "expected unauthenticated user not to be able \
-            to create organization",
-            );
-        db.cleanup().await.unwrap();
-        logctx.cleanup_successful();
-    }
-
     #[tokio::test]
     async fn test_unregistered_resource() {
         let logctx = dev::test_setup_log("test_unregistered_resource");
@@ -353,6 +256,10 @@ mod test {
                 // authorize() shouldn't get far enough to call this.
                 unimplemented!();
             }
+
+            fn polar_class(&self) -> oso::Class {
+                Self::get_polar_class()
+            }
         }
 
         // Make sure an authz check with this resource fails with a clear

nexus/src/authz/mod.rs

@@ -182,3 +182,6 @@ pub use oso_generic::Action;
 pub use oso_generic::DATABASE;
 
 mod roles;
+
+#[cfg(test)]
+mod policy_test;

nexus/src/authz/omicron.polar

@@ -456,8 +456,6 @@ resource Database {
 # All authenticated users have the "query" permission on the database.
 has_permission(_actor: AuthenticatedActor, "query", _resource: Database);
 
-# The "db-init" user is the only one with the "init" role.
-has_permission(actor: AuthenticatedActor, "modify", _resource: Database)
-	if actor = USER_DB_INIT;
-has_permission(actor: AuthenticatedActor, "create_child", _resource: IpPoolList)
-	if actor = USER_DB_INIT;
+# The "db-init" user is the only one with the "modify" permission.
+has_permission(USER_DB_INIT: AuthenticatedActor, "modify", _resource: Database);
+has_permission(USER_DB_INIT: AuthenticatedActor, "create_child", _resource: IpPoolList);

nexus/src/authz/oso_generic.rs

@@ -156,15 +156,16 @@ pub fn make_omicron_oso(log: &slog::Logger) -> Result<OsoInit, anyhow::Error> {
 /// There's currently just one enum of Actions for all of Omicron.  We expect
 /// most objects to support mostly the same set of actions.
 #[derive(Clone, Copy, Debug, Eq, PartialEq)]
+#[cfg_attr(test, derive(strum::EnumIter))]
 pub enum Action {
     Query, // only used for [`Database`]
     Read,
+    ListChildren,
     ReadPolicy,
     Modify,
     ModifyPolicy,
-    Delete,
-    ListChildren,
     CreateChild,
+    Delete,
     ListIdentityProviders, // only used during [`Nexus::identity_provider_list`]
 }
 
@@ -288,6 +289,10 @@ impl AuthorizedResource for Database {
     ) -> Error {
         error
     }
+
+    fn polar_class(&self) -> oso::Class {
+        Self::get_polar_class()
+    }
 }
 
 #[cfg(test)]