code review feedback

Author: davepacheco

nexus/src/authz/api_resources.rs

@@ -20,7 +20,7 @@
 // parts we need for authorization.
 
 use super::actor::AnyActor;
-use super::context::Authorize;
+use super::context::AuthorizedResource;
 use super::roles::{
     load_roles_for_resource, load_roles_for_resource_tree, RoleSet,
 };
@@ -37,9 +37,7 @@ use uuid::Uuid;
 
 /// Describes an authz resource that corresponds to an API resource that has a
 /// corresponding ResourceType and is stored in the database
-///
-/// This is a helper trait used to impl [`AuthzResource`].
-pub trait AuthzApiResource: Clone + Send + Sync + 'static {
+pub trait ApiResource: Clone + Send + Sync + 'static {
     /// If roles can be assigned to this resource, return the type and id of the
     /// database record describing this resource
     ///
@@ -49,14 +47,14 @@ pub trait AuthzApiResource: Clone + Send + Sync + 'static {
     /// If this resource has a parent in the API hierarchy whose assigned roles
     /// can affect access to this resource, return the parent resource.
     /// Otherwise, returns `None`.
-    fn parent(&self) -> Option<&dyn Authorize>;
+    fn parent(&self) -> Option<&dyn AuthorizedResource>;
 
     /// Returns an error as though this resource were not found, suitable for
     /// use when an actor should not be able to see that this resource exists
     fn not_found(&self) -> Error;
 }
 
-impl<T: AuthzApiResource + oso::PolarClass> Authorize for T {
+impl<T: ApiResource + oso::PolarClass> AuthorizedResource for T {
     fn load_roles<'a, 'b, 'c, 'd, 'e, 'f>(
         &'a self,
         opctx: &'b OpContext,
@@ -81,21 +79,21 @@ impl<T: AuthzApiResource + oso::PolarClass> Authorize for T {
         error: Error,
         actor: AnyActor,
         action: Action,
-    ) -> Result<(), Error> {
+    ) -> Error {
         if action == Action::Read {
-            return Err(self.not_found());
+            return self.not_found();
         }
 
         // If the user failed an authz check, and they can't even read this
         // resource, then we should produce a 404 rather than a 401/403.
         match authz.is_allowed(&actor, Action::Read, self) {
-            Err(error) => Err(Error::internal_error(&format!(
+            Err(error) => Error::internal_error(&format!(
                 "failed to compute read authorization to determine visibility: \
                 {:#}",
                 error
-            ))),
-            Ok(false) => Err(self.not_found()),
-            Ok(true) => Err(error),
+            )),
+            Ok(false) => self.not_found(),
+            Ok(true) => error,
         }
     }
 }
@@ -163,13 +161,13 @@ impl oso::PolarClass for Fleet {
     }
 }
 
-impl Authorize for Fleet {
+impl AuthorizedResource for Fleet {
     fn load_roles<'a, 'b, 'c, 'd, 'e, 'f>(
         &'a self,
-        opctx: &'b crate::context::OpContext,
-        datastore: &'c crate::db::DataStore,
-        authn: &'d crate::authn::Context,
-        roleset: &'e mut super::roles::RoleSet,
+        opctx: &'b OpContext,
+        datastore: &'c DataStore,
+        authn: &'d authn::Context,
+        roleset: &'e mut RoleSet,
     ) -> futures::future::BoxFuture<'f, Result<(), Error>>
     where
         'a: 'f,
@@ -195,8 +193,8 @@ impl Authorize for Fleet {
         error: Error,
         _: AnyActor,
         _: Action,
-    ) -> Result<(), Error> {
-        Err(error)
+    ) -> Error {
+        error
     }
 }
 
@@ -232,12 +230,12 @@ impl oso::PolarClass for FleetChild {
     }
 }
 
-impl AuthzApiResource for FleetChild {
+impl ApiResource for FleetChild {
     fn db_resource(&self) -> Option<(ResourceType, Uuid)> {
         None
     }
 
-    fn parent(&self) -> Option<&dyn Authorize> {
+    fn parent(&self) -> Option<&dyn AuthorizedResource> {
         Some(&FLEET)
     }
 
@@ -303,12 +301,12 @@ impl oso::PolarClass for Organization {
     }
 }
 
-impl AuthzApiResource for Organization {
+impl ApiResource for Organization {
     fn db_resource(&self) -> Option<(ResourceType, Uuid)> {
         Some((ResourceType::Organization, self.organization_id))
     }
 
-    fn parent(&self) -> Option<&dyn Authorize> {
+    fn parent(&self) -> Option<&dyn AuthorizedResource> {
         Some(&FLEET)
     }
 
@@ -382,12 +380,12 @@ impl oso::PolarClass for Project {
     }
 }
 
-impl AuthzApiResource for Project {
+impl ApiResource for Project {
     fn db_resource(&self) -> Option<(ResourceType, Uuid)> {
         Some((ResourceType::Project, self.project_id))
     }
 
-    fn parent(&self) -> Option<&dyn Authorize> {
+    fn parent(&self) -> Option<&dyn AuthorizedResource> {
         Some(&self.parent)
     }
 
@@ -438,13 +436,13 @@ impl oso::PolarClass for ProjectChild {
     }
 }
 
-impl AuthzApiResource for ProjectChild {
+impl ApiResource for ProjectChild {
     fn db_resource(&self) -> Option<(ResourceType, Uuid)> {
         // We do not support assigning roles to children of Projects.
         None
     }
 
-    fn parent(&self) -> Option<&dyn Authorize> {
+    fn parent(&self) -> Option<&dyn AuthorizedResource> {
         Some(&self.parent)
     }
 

nexus/src/authz/context.rs

@@ -77,7 +77,7 @@ impl Context {
         resource: Resource,
     ) -> Result<(), Error>
     where
-        Resource: oso::ToPolar + Authorize + Clone,
+        Resource: AuthorizedResource + Clone,
     {
         let mut roles = RoleSet::new();
         resource
@@ -105,13 +105,13 @@ impl Context {
                     }
                 };
 
-                resource.on_unauthorized(&self.authz, error, actor, action)
+                Err(resource.on_unauthorized(&self.authz, error, actor, action))
             }
         }
     }
 }
 
-pub trait Authorize: Send + Sync + 'static {
+pub trait AuthorizedResource: oso::ToPolar + Send + Sync + 'static {
     /// Find all roles for the user described in `authn` that might be used to
     /// make an authorization decision on `self` (a resource)
     ///
@@ -148,7 +148,7 @@ pub trait Authorize: Send + Sync + 'static {
         error: Error,
         actor: AnyActor,
         action: Action,
-    ) -> Result<(), Error>;
+    ) -> Error;
 }
 
 #[cfg(test)]

nexus/src/authz/mod.rs

@@ -171,7 +171,7 @@ pub use api_resources::ProjectChild;
 pub use api_resources::FLEET;
 
 mod context;
-pub use context::Authorize;
+pub use context::AuthorizedResource;
 pub use context::Authz;
 pub use context::Context;
 

nexus/src/authz/oso_generic.rs

@@ -11,7 +11,7 @@ use super::api_resources::FleetChild;
 use super::api_resources::Organization;
 use super::api_resources::Project;
 use super::api_resources::ProjectChild;
-use super::context::Authorize;
+use super::context::AuthorizedResource;
 use super::roles::RoleSet;
 use super::Authz;
 use crate::authn;
@@ -150,7 +150,7 @@ impl oso::PolarClass for Database {
     }
 }
 
-impl Authorize for Database {
+impl AuthorizedResource for Database {
     fn load_roles<'a, 'b, 'c, 'd, 'e, 'f>(
         &'a self,
         _: &'b OpContext,
@@ -183,7 +183,7 @@ impl Authorize for Database {
         error: Error,
         _: AnyActor,
         _: Action,
-    ) -> Result<(), Error> {
-        Err(error)
+    ) -> Error {
+        error
     }
 }

nexus/src/authz/roles.rs

@@ -17,8 +17,7 @@
 //! its parent Organization or the parent Fleet.  This isn't as large as it
 //! might sound, since the hierarchy is not _that_ deep, but we do have to make
 //! multiple database queries to load all this.  This is all done by
-//! [`AuthzResource::fetch_all_related_roles_for_user()`].  This is really done
-//! by the impl of [`AuthzResource`] for [`AuthzApiResource`].
+//! [`load_roles_for_resource_tree`].
 //!
 //! Once we've got the complete list of roles, we include that in the data
 //! structure we pass into Oso.   When it asks whether an actor has a role on a
@@ -35,7 +34,7 @@
 //! request, and we don't want that thread to block while we hit the database.
 //! Both of these issues could be addressed with considerably more work.
 
-use super::api_resources::AuthzApiResource;
+use super::api_resources::ApiResource;
 use crate::authn;
 use crate::context::OpContext;
 use crate::db::DataStore;
@@ -93,7 +92,7 @@ pub async fn load_roles_for_resource_tree<R>(
     roleset: &mut RoleSet,
 ) -> Result<(), Error>
 where
-    R: AuthzApiResource,
+    R: ApiResource,
 {
     // If roles can be assigned directly on this resource, load them.
     if let Some((resource_type, resource_id)) = resource.db_resource() {

nexus/src/context.rs

@@ -12,7 +12,7 @@ use super::db;
 use super::Nexus;
 use crate::authn::external::session_cookie::{Session, SessionStore};
 use crate::authn::Actor;
-use crate::authz::Authorize;
+use crate::authz::AuthorizedResource;
 use crate::db::model::ConsoleSession;
 use crate::db::DataStore;
 use async_trait::async_trait;
@@ -311,7 +311,7 @@ impl OpContext {
         resource: &Resource,
     ) -> Result<(), Error>
     where
-        Resource: oso::ToPolar + Authorize + Debug + Clone,
+        Resource: AuthorizedResource + Debug + Clone,
     {
         /*
          * TODO-cleanup In an ideal world, Oso would consume &Action and