From 56b17572f05c7f607a3b9c2b3808d4069cc27bb6 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Tue, 16 Nov 2021 18:43:49 -0800 Subject: [PATCH 01/12] flesh out Oso policy and associated types --- common/src/api/external/mod.rs | 2 +- nexus/src/authz/mod.rs | 1 + nexus/src/authz/omicron.polar | 156 +++++++++++++++++++++++++++--- nexus/src/authz/oso_types.rs | 167 +++++++++++++++++++++++++-------- nexus/src/db/datastore.rs | 2 +- 5 files changed, 275 insertions(+), 53 deletions(-) diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index bf063c0fee6..f6c6a21585e 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -458,7 +458,7 @@ impl TryFrom for Generation { /** * Identifies a type of API resource */ -#[derive(Debug, Deserialize, PartialEq, Serialize)] +#[derive(Clone, Copy, Debug, Deserialize, Eq, PartialEq, Serialize)] pub enum ResourceType { Organization, Project, diff --git a/nexus/src/authz/mod.rs b/nexus/src/authz/mod.rs index 9795c344074..340ae367f4a 100644 --- a/nexus/src/authz/mod.rs +++ b/nexus/src/authz/mod.rs @@ -86,6 +86,7 @@ mod test { /* * These are essentially unit tests for the policy itself. * TODO-coverage This is just a start. + * XXX Flesh these out now that we have a more realistic policy and types * 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 diff --git a/nexus/src/authz/omicron.polar b/nexus/src/authz/omicron.polar index 28ddf99b92b..a47ecb50f7b 100644 --- a/nexus/src/authz/omicron.polar +++ b/nexus/src/authz/omicron.polar @@ -46,22 +46,154 @@ resource Database { # All authenticated users have the "user" role on the database. has_role(_actor: AuthenticatedActor, "user", _resource: Database); +# +# Permissions and predefined roles +# +# For now, we define the following permissions for most resources in the system: +# +# - "create_child": required to create child resources. +# +# - "list_children": required to list children (of all types) of a resources +# +# - "modify": required to modify or delete a resource or any of its children +# +# - "read": required to read a resource +# +# We define the following predefined roles for only a few high-level resources: +# +# - "admin": has all permissions on the resource +# +# - "collaborator": has "list_children" and "create_$child" for all children. +# They'll inherit the "admin" role for any resources that they create. +# +# - "viewer": has "read" and "list_children" on a resource +# +# Below the project level, permissions are granted at the Project level. For +# example, for someone to be able to create, modify, or delete any Instances, +# they must be granted project.collaborator, which means they can create, +# modify, or delete _all_ resources in the Project. +# +# The complete set of predefined roles: +# +# - fleet.admin (superuser for the whole system) +# - fleet.collaborator (can create and own orgs) +# - organization.admin (complete control over an organization) +# - organization.collaborator (can create, modify, and delete projects) +# - project.admin (complete control over a project) +# - project.collaborator (can create, modify, and delete all resources within +# the project, but cannot modify or delete the project +# itself) +# - project.viewer (can see everything in the project, but cannot modify +# anything) +# -# At the top level is the "Fleet" resource. Fleet administrators can create -# Organizations, which essentially gives them permissions to do anything with -# the Fleet. (They're not exactly superusers, though. They only inherit an -# "admin" role on Organizations they create. They cannot necessarily even see -# Organizations created by other Fleet administrators.) +# At the top level is the "Fleet" resource. resource Fleet { - permissions = [ "create_organization" ]; - roles = [ "admin" ]; - "create_organization" if "admin"; + permissions = [ + "list_children", + "modify", + "read", + "create_child", + ]; + + roles = [ "admin", "collaborator" ]; + + # Fleet collaborators can create Organizations and see fleet-wide + # information, including Organizations that they don't have permissions + # on. (They cannot list projects within those organizations, however.) + # They cannot modify fleet-wide information. + "list_children" if "collaborator"; + "read" if "collaborator"; + "create_child" if "collaborator"; + + # Fleet administrators are whole-system superusers. + "collaborator" if "admin"; + "modify" if "admin"; } resource Organization { - permissions = [ "create_project" ]; - roles = [ "admin" ]; + permissions = [ + "list_children", + "modify", + "read", + "create_child", + ]; + roles = [ "admin", "collaborator" ]; + + # Organization collaborators can create Projects and see + # organization-wide information, including Projects that they don't have + # permissions on. (They cannot see anything inside those Projects, + # though.) They cannot modify or delete the organization itself. + "list_children" if "collaborator"; + "read" if "collaborator"; + "create_child" if "collaborator"; + + # Organization administrators can modify and delete the Organization + # itself. They can also see and administer everything in the + # Organization (recursively). + "collaborator" if "admin"; + "modify" if "admin"; + + relations = { parent_fleet: Fleet }; + "admin" if "admin" on "parent_fleet"; +} + +resource Project { + permissions = [ + "list_children", + "modify", + "read", + "create_child", + ]; + roles = [ "admin", "collaborator", "viewer" ]; + + # Project viewers can see everything in the Project. + "list_children" if "viewer"; + "read" if "viewer"; + + # Project collaborators can see, modify, and delete everything inside + # the Project recursively. (This is different from Fleet and + # Organization-level collaborators, who can only modify and delete child + # resources that they have specific permissions on. That's because + # we're not implementing fine-grained permissions within Projects yet.) + # They cannot modify or delete the Project itself. + "viewer" if "collaborator"; + "create_child" if "collaborator"; + + # Project administrators can modify and delete the Project" itself. + "collaborator" if "admin"; + "modify" if "admin"; + + relations = { parent_organization: Organization }; + "admin" if "admin" on "parent_organization"; +} - # Administrator permissions - "create_project" if "admin"; +# For now, we use one generic resource to represent every kind of thing inside +# the Project. That's because they all have the same behavior. +resource ProjectResource { + permissions = [ + "list_children", + "modify", + "read", + "create_child", + ]; + + relations = { parent_project: Project }; + "list_children" if "viewer" on "parent_project"; + "read" if "viewer" on "parent_project"; + + "modify" if "collaborator" on "parent_project"; + "create_child" if "collaborator" on "parent_project"; } + +# Define relationships +has_relation(fleet: Fleet, "parent_fleet", organization: Organization) + if organization.fleet = fleet; +has_relation(organization: Organization, "parent_organization", project: Project) + if project.organization = organization; +has_relation(project: Project, "parent_project", project_resource: ProjectResource) + if project_resource.project = project; + +# Define role relationships +has_role(actor: Actor, role: String, resource: Resource) + if resource.has_role(actor, role); diff --git a/nexus/src/authz/oso_types.rs b/nexus/src/authz/oso_types.rs index 7222011041b..343d5a94bb0 100644 --- a/nexus/src/authz/oso_types.rs +++ b/nexus/src/authz/oso_types.rs @@ -20,9 +20,11 @@ use crate::authn; use crate::db; use crate::db::identity::Resource; use anyhow::Context; +use omicron_common::api::external::ResourceType; use oso::Oso; use oso::PolarClass; use std::fmt; +use uuid::Uuid; /// Polar configuration describing control plane authorization rules pub const OMICRON_AUTHZ_CONFIG: &str = include_str!("omicron.polar"); @@ -38,6 +40,8 @@ pub fn make_omicron_oso() -> Result { Database::get_polar_class(), Fleet::get_polar_class(), Organization::get_polar_class(), + Project::get_polar_class(), + ProjectResource::get_polar_class(), ]; for c in classes { oso.register_class(c).context("registering class")?; @@ -55,15 +59,15 @@ pub fn make_omicron_oso() -> Result { /// Describes an action being authorized /// /// There's currently just one enum of Actions for all of Omicron. We expect -/// most objects to support mosty the same set of actions, except that we use -/// distinct variants for the various "create" actions because it's possible a -/// resource might support more than one different thing that you can create -/// (e.g., a Project will support CreateInstance and CreateDisk). +/// most objects to support mosty the same set of actions. #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub enum Action { Query, // only used for [`Database`] - Read, // general "read" action - CreateOrganization, + Read, + Modify, + Delete, + ListChildren, + CreateChild, } impl oso::PolarClass for Action { @@ -75,7 +79,10 @@ impl oso::PolarClass for Action { match a { Action::Query => Perm::Query, Action::Read => Perm::Read, - Action::CreateOrganization => Perm::CreateOrganization, + Action::Modify => Perm::Modify, + Action::Delete => Perm::Modify, + Action::ListChildren => Perm::ListChildren, + Action::CreateChild => Perm::CreateChild, } .to_string() }) @@ -89,9 +96,11 @@ impl oso::PolarClass for Action { /// even impl [`PolarClass`]. #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub enum Perm { - Query, + Query, // Only for [`Database`] Read, - CreateOrganization, + Modify, + ListChildren, + CreateChild, } impl fmt::Display for Perm { @@ -99,22 +108,21 @@ impl fmt::Display for Perm { // This implementation MUST be kept in sync with the Polar configuration // for Omicron, which uses literal strings for permissions. f.write_str(match self { - Perm::Read => "read", Perm::Query => "query", - Perm::CreateOrganization => "create_organization", + Perm::Read => "read", + Perm::Modify => "modify", + Perm::ListChildren => "list_children", + Perm::CreateChild => "create_child", }) } } /// Represents [`authn::Context`] (which is either an authenticated or /// unauthenticated actor) for Polar -#[derive(Clone, Debug, Eq, PartialEq)] -pub struct AnyActor(bool, Option); -impl From<&authn::Context> for AnyActor { - fn from(authn: &authn::Context) -> Self { - let actor = authn.actor(); - AnyActor(actor.is_some(), actor.map(|a| a.0.to_string())) - } +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub struct AnyActor { + authenticated: bool, + actor_id: Option, } impl oso::PolarClass for AnyActor { @@ -122,35 +130,49 @@ impl oso::PolarClass for AnyActor { oso::Class::builder() .name("AnyActor") .set_equality_check(|a1: &AnyActor, a2: &AnyActor| a1 == a2) - .add_attribute_getter("authenticated", |a: &AnyActor| a.0) + .add_attribute_getter("authenticated", |a: &AnyActor| { + a.authenticated + }) .add_attribute_getter("authn_actor", |a: &AnyActor| { - a.1.as_ref().map(|a| AuthenticatedActor(a.clone())) + a.actor_id.map(|actor_id| AuthenticatedActor { actor_id }) }) } } -/// Represents an authenticated [`authn::Context`] for Polar -#[derive(Clone, Debug, Eq, PartialEq)] -pub struct AuthenticatedActor(String); -impl From<&authn::Actor> for AuthenticatedActor { - fn from(omicron_actor: &authn::Actor) -> Self { - // We store the string rather than the uuid because PolarClass is not - // impl'd for Uuids. (We could instead use a Vec for the raw bytes, - // but is that really better?) - AuthenticatedActor(omicron_actor.0.to_string()) +impl From<&authn::Context> for AnyActor { + fn from(authn: &authn::Context) -> Self { + let actor = authn.actor(); + AnyActor { + authenticated: actor.is_some(), + actor_id: actor.map(|a| a.0), + } } } +/// Represents an authenticated [`authn::Context`] for Polar +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub struct AuthenticatedActor { + actor_id: Uuid, +} + impl oso::PolarClass for AuthenticatedActor { fn get_polar_class_builder() -> oso::ClassBuilder { oso::Class::builder() .add_attribute_getter("is_test_user", |a: &AuthenticatedActor| { - a.0 == authn::TEST_USER_UUID_PRIVILEGED + a.actor_id.to_string() == authn::TEST_USER_UUID_PRIVILEGED }) .set_equality_check( |a1: &AuthenticatedActor, a2: &AuthenticatedActor| a1 == a2, ) - .add_attribute_getter("id", |a: &AuthenticatedActor| a.0.clone()) + .add_attribute_getter("id", |a: &AuthenticatedActor| { + a.actor_id.to_string() + }) + } +} + +impl From<&authn::Actor> for AuthenticatedActor { + fn from(omicron_actor: &authn::Actor) -> Self { + AuthenticatedActor { actor_id: omicron_actor.0 } } } @@ -160,30 +182,97 @@ impl oso::PolarClass for AuthenticatedActor { // See the note above about why we use newtypes and why we don't use // derive(PolarClass). // - /// Represents the whole Oxide fleet to Polar (used essentially to mean /// "global"). See RFD 24. -#[derive(Clone, Debug, Eq, PartialEq, oso::PolarClass)] +#[derive(Clone, Copy, Debug, Eq, PartialEq, oso::PolarClass)] pub struct Fleet; pub const FLEET: Fleet = Fleet; /// Wraps [`db::model::Organization`] for Polar -#[derive(Clone, Debug, Eq, PartialEq)] -pub struct Organization(String); +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub struct Organization { + organization_id: Uuid, +} + +impl oso::PolarClass for Organization { + fn get_polar_class_builder() -> oso::ClassBuilder { + oso::Class::builder() + .set_equality_check(|a1, a2| a1 == a2) + .add_attribute_getter("fleet", |_: &Organization| Fleet {}) + } +} + impl From<&db::model::Organization> for Organization { fn from(omicron_organization: &db::model::Organization) -> Self { - Organization(omicron_organization.id().to_string()) + Organization { organization_id: omicron_organization.id() } } } -impl oso::PolarClass for Organization { +/// Wraps [`db::model::Project`] for Polar +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct Project { + organization_id: Uuid, + project_id: Uuid, +} + +impl oso::PolarClass for Project { fn get_polar_class_builder() -> oso::ClassBuilder { - oso::Class::builder().set_equality_check(|a1, a2| a1 == a2) + oso::Class::builder() + .set_equality_check(|a1, a2| a1 == a2) + .add_attribute_getter("organization", |p: &Project| Organization { + organization_id: p.organization_id, + }) + } +} + +impl From<&db::model::Project> for Project { + fn from(omicron_project: &db::model::Project) -> Self { + Project { + organization_id: omicron_project.organization_id, + project_id: omicron_project.id(), + } + } +} + +impl Project { + fn resource( + &self, + resource_type: ResourceType, + resource_id: Uuid, + ) -> ProjectResource { + ProjectResource { + organization_id: self.organization_id, + project_id: self.project_id, + resource_type, + resource_id, + } + } +} + +/// Wraps any resource that lives inside a Project for Polar +/// +/// This would include [`db::model::Instance`], [`db::model::Disk`], etc. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub struct ProjectResource { + organization_id: Uuid, + project_id: Uuid, + resource_type: ResourceType, + resource_id: Uuid, +} + +impl oso::PolarClass for ProjectResource { + fn get_polar_class_builder() -> oso::ClassBuilder { + oso::Class::builder() + .set_equality_check(|a1, a2| a1 == a2) + .add_attribute_getter("project", |pr: &ProjectResource| Project { + organization_id: pr.organization_id, + project_id: pr.project_id, + }) } } /// Represents the database itself to Polar (so that we can have roles with no /// access to the database at all) -#[derive(Clone, Debug, Eq, PartialEq, oso::PolarClass)] +#[derive(Clone, Copy, Debug, Eq, PartialEq, oso::PolarClass)] pub struct Database; pub const DATABASE: Database = Database; diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index 60f5e4d4ff2..42829892f97 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -151,7 +151,7 @@ impl DataStore { ) -> CreateResult { use db::schema::organization::dsl; - opctx.authorize(authz::Action::CreateOrganization, authz::FLEET)?; + opctx.authorize(authz::Action::CreateChild, authz::FLEET)?; let name = organization.name().as_str().to_string(); diesel::insert_into(dsl::organization) From 75a528de40833f879e09c925b4f223904fc045ab Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Tue, 16 Nov 2021 19:27:02 -0800 Subject: [PATCH 02/12] fix tests --- nexus/src/authz/mod.rs | 4 +- nexus/src/authz/omicron.polar | 12 +---- nexus/src/authz/oso_types.rs | 95 +++++++++++++++++++++++++++++++++-- 3 files changed, 94 insertions(+), 17 deletions(-) diff --git a/nexus/src/authz/mod.rs b/nexus/src/authz/mod.rs index 340ae367f4a..9026d6d1684 100644 --- a/nexus/src/authz/mod.rs +++ b/nexus/src/authz/mod.rs @@ -135,11 +135,11 @@ mod test { fn test_organization() { let authz_privileged = authz_context_for_actor(TEST_USER_UUID_PRIVILEGED); - authz_privileged.authorize(Action::CreateOrganization, FLEET).expect( + authz_privileged.authorize(Action::CreateChild, FLEET).expect( "expected privileged user to be able to create organization", ); let authz_nobody = authz_context_for_actor(TEST_USER_UUID_UNPRIVILEGED); - authz_nobody.authorize(Action::CreateOrganization, FLEET).expect_err( + authz_nobody.authorize(Action::CreateChild, FLEET).expect_err( "expected unprivileged user not to be able to create organization", ); let authz_noauth = authz_context_noauth(); diff --git a/nexus/src/authz/omicron.polar b/nexus/src/authz/omicron.polar index a47ecb50f7b..2b282b579e1 100644 --- a/nexus/src/authz/omicron.polar +++ b/nexus/src/authz/omicron.polar @@ -21,15 +21,6 @@ allow(actor: AnyActor, action: Action, resource) if actor.authenticated and has_permission(actor.authn_actor.unwrap(), action.to_perm(), resource); -# For now, we hardcode some fixed actor ids and what roles they have. -# This will eventually need to be replaced with data that comes from the -# database. -# NOTE: this user uuid is duplicated in authn/mod.rs. -has_role(actor: AuthenticatedActor, _role: String, _resource: Resource) if - actor.is_test_user; - - - # # Resources # @@ -44,6 +35,7 @@ resource Database { } # All authenticated users have the "user" role on the database. +# XXX This rule doesn't seem to get used for some reason. has_role(_actor: AuthenticatedActor, "user", _resource: Database); # @@ -195,5 +187,5 @@ has_relation(project: Project, "parent_project", project_resource: ProjectResour if project_resource.project = project; # Define role relationships -has_role(actor: Actor, role: String, resource: Resource) +has_role(actor: AuthenticatedActor, role: String, resource: Resource) if resource.has_role(actor, role); diff --git a/nexus/src/authz/oso_types.rs b/nexus/src/authz/oso_types.rs index 343d5a94bb0..2bc722e61ec 100644 --- a/nexus/src/authz/oso_types.rs +++ b/nexus/src/authz/oso_types.rs @@ -155,12 +155,42 @@ pub struct AuthenticatedActor { actor_id: Uuid, } +impl AuthenticatedActor { + /** + * Returns whether this actor has the given role for the given resource + */ + fn has_role_resource( + &self, + _resource_type: ResourceType, + _resource_id: Uuid, + _role: &str, + ) -> bool { + /* + * TODO We will probably want to either pre-load the list of roles that + * the actor has for this resource or else at this point we will go + * fetch them from the database. + */ + self.actor_id.to_string() == authn::TEST_USER_UUID_PRIVILEGED + } + + /** + * Returns whether this actor has the given role for a fleet + */ + /* + * XXX This is special-cased only because Fleets don't exist in the database + * and do not have an id. It might be a good idea to put them in the + * database with their own id, though it might mean that lots of authz + * checks need to do an extra database query to load the fleet_id from the + * Organization. + */ + fn has_role_fleet(&self, _fleet: &Fleet, role: &str) -> bool { + self.actor_id.to_string() == authn::TEST_USER_UUID_PRIVILEGED + } +} + impl oso::PolarClass for AuthenticatedActor { fn get_polar_class_builder() -> oso::ClassBuilder { oso::Class::builder() - .add_attribute_getter("is_test_user", |a: &AuthenticatedActor| { - a.actor_id.to_string() == authn::TEST_USER_UUID_PRIVILEGED - }) .set_equality_check( |a1: &AuthenticatedActor, a2: &AuthenticatedActor| a1 == a2, ) @@ -184,10 +214,21 @@ impl From<&authn::Actor> for AuthenticatedActor { // /// Represents the whole Oxide fleet to Polar (used essentially to mean /// "global"). See RFD 24. -#[derive(Clone, Copy, Debug, Eq, PartialEq, oso::PolarClass)] +#[derive(Clone, Copy, Debug, Eq, PartialEq)] pub struct Fleet; pub const FLEET: Fleet = Fleet; +impl oso::PolarClass for Fleet { + fn get_polar_class_builder() -> oso::ClassBuilder { + oso::Class::builder().set_equality_check(|a1, a2| a1 == a2).add_method( + "has_role", + |fleet: &Fleet, actor: AuthenticatedActor, role: String| { + actor.has_role_fleet(fleet, &role) + }, + ) + } +} + /// Wraps [`db::model::Organization`] for Polar #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub struct Organization { @@ -198,6 +239,16 @@ impl oso::PolarClass for Organization { fn get_polar_class_builder() -> oso::ClassBuilder { oso::Class::builder() .set_equality_check(|a1, a2| a1 == a2) + .add_method( + "has_role", + |o: &Organization, actor: AuthenticatedActor, role: String| { + actor.has_role_resource( + ResourceType::Organization, + o.organization_id, + &role, + ) + }, + ) .add_attribute_getter("fleet", |_: &Organization| Fleet {}) } } @@ -219,6 +270,16 @@ impl oso::PolarClass for Project { fn get_polar_class_builder() -> oso::ClassBuilder { oso::Class::builder() .set_equality_check(|a1, a2| a1 == a2) + .add_method( + "has_role", + |p: &Project, actor: AuthenticatedActor, role: String| { + actor.has_role_resource( + ResourceType::Project, + p.project_id, + &role, + ) + }, + ) .add_attribute_getter("organization", |p: &Project| Organization { organization_id: p.organization_id, }) @@ -264,6 +325,18 @@ impl oso::PolarClass for ProjectResource { fn get_polar_class_builder() -> oso::ClassBuilder { oso::Class::builder() .set_equality_check(|a1, a2| a1 == a2) + .add_method( + "has_role", + |pr: &ProjectResource, + actor: AuthenticatedActor, + role: String| { + actor.has_role_resource( + pr.resource_type, + pr.resource_id, + &role, + ) + }, + ) .add_attribute_getter("project", |pr: &ProjectResource| Project { organization_id: pr.organization_id, project_id: pr.project_id, @@ -273,6 +346,18 @@ impl oso::PolarClass for ProjectResource { /// Represents the database itself to Polar (so that we can have roles with no /// access to the database at all) -#[derive(Clone, Copy, Debug, Eq, PartialEq, oso::PolarClass)] +#[derive(Clone, Copy, Debug, Eq, PartialEq)] pub struct Database; pub const DATABASE: Database = Database; + +impl oso::PolarClass for Database { + fn get_polar_class_builder() -> oso::ClassBuilder { + oso::Class::builder().add_method( + "has_role", + |_d: &Database, _actor: AuthenticatedActor, role: String| { + assert_eq!(role, "user"); + true + }, + ) + } +} From 1f1dcf8dfb389171110d5a74f44ec6431722de9c Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Tue, 16 Nov 2021 19:53:58 -0800 Subject: [PATCH 03/12] add authz to list-organizations endpoint --- nexus/src/authz/oso_types.rs | 2 +- nexus/src/db/datastore.rs | 9 ++++++-- nexus/src/external_api/http_entrypoints.rs | 5 +++-- nexus/src/nexus.rs | 6 +++-- nexus/tests/common/resource_helpers.rs | 26 ++++++++++++++++++++++ nexus/tests/test_organizations.rs | 14 ++++++++---- tools/oxapi_demo | 2 +- 7 files changed, 52 insertions(+), 12 deletions(-) diff --git a/nexus/src/authz/oso_types.rs b/nexus/src/authz/oso_types.rs index 2bc722e61ec..63d33970eb5 100644 --- a/nexus/src/authz/oso_types.rs +++ b/nexus/src/authz/oso_types.rs @@ -183,7 +183,7 @@ impl AuthenticatedActor { * checks need to do an extra database query to load the fleet_id from the * Organization. */ - fn has_role_fleet(&self, _fleet: &Fleet, role: &str) -> bool { + fn has_role_fleet(&self, _fleet: &Fleet, _role: &str) -> bool { self.actor_id.to_string() == authn::TEST_USER_UUID_PRIVILEGED } } diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index 42829892f97..1937e78e48b 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -280,13 +280,16 @@ impl DataStore { pub async fn organizations_list_by_id( &self, + opctx: &OpContext, pagparams: &DataPageParams<'_, Uuid>, ) -> ListResultVec { + // XXX working here use db::schema::organization::dsl; + opctx.authorize(authz::Action::ListChildren, authz::FLEET)?; paginated(dsl::organization, dsl::id, pagparams) .filter(dsl::time_deleted.is_null()) .select(Organization::as_select()) - .load_async::(self.pool()) + .load_async::(self.pool_authorized(opctx)?) .await .map_err(|e| { public_error_from_diesel_pool( @@ -299,13 +302,15 @@ impl DataStore { pub async fn organizations_list_by_name( &self, + opctx: &OpContext, pagparams: &DataPageParams<'_, Name>, ) -> ListResultVec { use db::schema::organization::dsl; + opctx.authorize(authz::Action::ListChildren, authz::FLEET)?; paginated(dsl::organization, dsl::name, pagparams) .filter(dsl::time_deleted.is_null()) .select(Organization::as_select()) - .load_async::(self.pool()) + .load_async::(self.pool_authorized(opctx)?) .await .map_err(|e| { public_error_from_diesel_pool( diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 0fa10a6b31c..830e11bdf82 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -196,6 +196,7 @@ async fn organizations_get( let apictx = rqctx.context(); let nexus = &apictx.nexus; let handler = async { + let opctx = OpContext::for_external_api(&rqctx).await?; let query = query_params.into_inner(); let params = ScanByNameOrId::from_query(&query)?; let field = pagination_field_for_scan_params(params); @@ -203,14 +204,14 @@ async fn organizations_get( let organizations = match field { PagField::Id => { let page_selector = data_page_params_nameid_id(&rqctx, &query)?; - nexus.organizations_list_by_id(&page_selector).await? + nexus.organizations_list_by_id(&opctx, &page_selector).await? } PagField::Name => { let page_selector = data_page_params_nameid_name(&rqctx, &query)? .map_name(|n| Name::ref_cast(n)); - nexus.organizations_list_by_name(&page_selector).await? + nexus.organizations_list_by_name(&opctx, &page_selector).await? } } .into_iter() diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index 7a33af71651..849313487a4 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -426,16 +426,18 @@ impl Nexus { pub async fn organizations_list_by_name( &self, + opctx: &OpContext, pagparams: &DataPageParams<'_, Name>, ) -> ListResultVec { - self.db_datastore.organizations_list_by_name(pagparams).await + self.db_datastore.organizations_list_by_name(opctx, pagparams).await } pub async fn organizations_list_by_id( &self, + opctx: &OpContext, pagparams: &DataPageParams<'_, Uuid>, ) -> ListResultVec { - self.db_datastore.organizations_list_by_id(pagparams).await + self.db_datastore.organizations_list_by_id(opctx, pagparams).await } pub async fn organization_delete(&self, name: &Name) -> DeleteResult { diff --git a/nexus/tests/common/resource_helpers.rs b/nexus/tests/common/resource_helpers.rs index 8464920fbf7..d2aace94856 100644 --- a/nexus/tests/common/resource_helpers.rs +++ b/nexus/tests/common/resource_helpers.rs @@ -13,6 +13,32 @@ use omicron_common::api::external::VpcRouterCreateParams; use omicron_nexus::external_api::params; use omicron_nexus::external_api::views::{Organization, Project}; +// XXX This needs to be better abstracted. +// and/or we should do the dropshot RFE for a builder that makes requests. +pub async fn objects_list_page_authz( + client: &ClientTestContext, + path: &str, +) -> dropshot::ResultsPage +where + ItemType: serde::de::DeserializeOwned, +{ + let authn_header = http::HeaderValue::from_static( + omicron_nexus::authn::TEST_USER_UUID_PRIVILEGED, + ); + let uri = client.url(path); + let request = hyper::Request::builder() + .header(HTTP_HEADER_OXIDE_AUTHN_SPOOF, authn_header) + .method(Method::GET) + .uri(uri) + .body("".into()) + .expect("attempted to construct invalid test request"); + let mut response = client + .make_request_with_request(request, StatusCode::OK) + .await + .expect("failed to make request"); + read_json::>(&mut response).await +} + pub async fn create_organization( client: &ClientTestContext, organization_name: &str, diff --git a/nexus/tests/test_organizations.rs b/nexus/tests/test_organizations.rs index ba4e1fb74d3..6af4aa7c56e 100644 --- a/nexus/tests/test_organizations.rs +++ b/nexus/tests/test_organizations.rs @@ -1,9 +1,11 @@ use omicron_nexus::external_api::views::Organization; -use dropshot::test_util::{object_delete, object_get, objects_list_page}; +use dropshot::test_util::{object_delete, object_get}; pub mod common; -use common::resource_helpers::{create_organization, create_project}; +use common::resource_helpers::{ + create_organization, create_project, objects_list_page_authz, +}; use common::test_setup; use http::method::Method; use http::StatusCode; @@ -41,7 +43,9 @@ async fn test_organizations() { // Verify GET /organizations works let organizations = - objects_list_page::(client, "/organizations").await.items; + objects_list_page_authz::(client, "/organizations") + .await + .items; assert_eq!(organizations.len(), 2); // alphabetical order for now assert_eq!(organizations[0].identity.name, o2_name); @@ -58,7 +62,9 @@ async fn test_organizations() { // Verify the org is gone from the organizations list let organizations = - objects_list_page::(client, "/organizations").await.items; + objects_list_page_authz::(client, "/organizations") + .await + .items; assert_eq!(organizations.len(), 1); assert_eq!(organizations[0].identity.name, o2_name); diff --git a/tools/oxapi_demo b/tools/oxapi_demo index fb23270caf9..60d46317e37 100755 --- a/tools/oxapi_demo +++ b/tools/oxapi_demo @@ -160,7 +160,7 @@ function mkjson function cmd_organizations_list { [[ $# != 0 ]] && usage "expected no arguments" - do_curl /organizations + do_curl_authn /organizations } function cmd_organization_create_demo From f5a423406a9fea36af6b7ea5954935fee6bb1302 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Fri, 19 Nov 2021 11:03:36 -0800 Subject: [PATCH 04/12] use new niceness --- nexus/src/db/datastore.rs | 1 - nexus/tests/common/resource_helpers.rs | 22 ++++++---------------- 2 files changed, 6 insertions(+), 17 deletions(-) diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index 1937e78e48b..91cec9d5ea6 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -283,7 +283,6 @@ impl DataStore { opctx: &OpContext, pagparams: &DataPageParams<'_, Uuid>, ) -> ListResultVec { - // XXX working here use db::schema::organization::dsl; opctx.authorize(authz::Action::ListChildren, authz::FLEET)?; paginated(dsl::organization, dsl::id, pagparams) diff --git a/nexus/tests/common/resource_helpers.rs b/nexus/tests/common/resource_helpers.rs index d2aace94856..abfa05fd06a 100644 --- a/nexus/tests/common/resource_helpers.rs +++ b/nexus/tests/common/resource_helpers.rs @@ -13,8 +13,6 @@ use omicron_common::api::external::VpcRouterCreateParams; use omicron_nexus::external_api::params; use omicron_nexus::external_api::views::{Organization, Project}; -// XXX This needs to be better abstracted. -// and/or we should do the dropshot RFE for a builder that makes requests. pub async fn objects_list_page_authz( client: &ClientTestContext, path: &str, @@ -22,21 +20,13 @@ pub async fn objects_list_page_authz( where ItemType: serde::de::DeserializeOwned, { - let authn_header = http::HeaderValue::from_static( - omicron_nexus::authn::TEST_USER_UUID_PRIVILEGED, - ); - let uri = client.url(path); - let request = hyper::Request::builder() - .header(HTTP_HEADER_OXIDE_AUTHN_SPOOF, authn_header) - .method(Method::GET) - .uri(uri) - .body("".into()) - .expect("attempted to construct invalid test request"); - let mut response = client - .make_request_with_request(request, StatusCode::OK) + NexusRequest::object_get(client, path) + .authn_as(AuthnMode::PrivilegedUser) + .execute() .await - .expect("failed to make request"); - read_json::>(&mut response).await + .expect("failed to make request") + .response_body() + .unwrap() } pub async fn create_organization( From 6b9e273d76aa85b20744855f10bfbf474ba9f435 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Fri, 19 Nov 2021 12:43:16 -0800 Subject: [PATCH 05/12] protect "DELETE disk" too as an example of a ProjectResource --- nexus/src/authz/mod.rs | 2 + nexus/src/authz/oso_types.rs | 53 +++++++++++++++------- nexus/src/db/datastore.rs | 12 ++++- nexus/src/external_api/http_entrypoints.rs | 10 +++- nexus/src/nexus.rs | 26 ++++++++--- nexus/tests/common/http_testing.rs | 2 +- nexus/tests/test_disks.rs | 23 +++++++--- 7 files changed, 94 insertions(+), 34 deletions(-) diff --git a/nexus/src/authz/mod.rs b/nexus/src/authz/mod.rs index 9026d6d1684..a58c88c7aa3 100644 --- a/nexus/src/authz/mod.rs +++ b/nexus/src/authz/mod.rs @@ -8,6 +8,8 @@ use std::sync::Arc; mod oso_types; pub use oso_types::Action; pub use oso_types::Organization; +pub use oso_types::Project; +pub use oso_types::ProjectResource; pub use oso_types::DATABASE; pub use oso_types::FLEET; diff --git a/nexus/src/authz/oso_types.rs b/nexus/src/authz/oso_types.rs index 63d33970eb5..80ec3742c91 100644 --- a/nexus/src/authz/oso_types.rs +++ b/nexus/src/authz/oso_types.rs @@ -218,6 +218,12 @@ impl From<&authn::Actor> for AuthenticatedActor { pub struct Fleet; pub const FLEET: Fleet = Fleet; +impl Fleet { + pub fn organization(&self, organization_id: Uuid) -> Organization { + Organization { organization_id } + } +} + impl oso::PolarClass for Fleet { fn get_polar_class_builder() -> oso::ClassBuilder { oso::Class::builder().set_equality_check(|a1, a2| a1 == a2).add_method( @@ -235,6 +241,15 @@ pub struct Organization { organization_id: Uuid, } +impl Organization { + pub fn project(&self, project_id: Uuid) -> Project { + Project { + organization_id: self.organization_id, + project_id, + } + } +} + impl oso::PolarClass for Organization { fn get_polar_class_builder() -> oso::ClassBuilder { oso::Class::builder() @@ -260,12 +275,27 @@ impl From<&db::model::Organization> for Organization { } /// Wraps [`db::model::Project`] for Polar -#[derive(Clone, Debug, Eq, PartialEq)] +#[derive(Clone, Copy, Debug, Eq, PartialEq)] pub struct Project { organization_id: Uuid, project_id: Uuid, } +impl Project { + pub fn resource( + &self, + resource_type: ResourceType, + resource_id: Uuid, + ) -> ProjectResource { + ProjectResource { + organization_id: self.organization_id, + project_id: self.project_id, + resource_type, + resource_id, + } + } +} + impl oso::PolarClass for Project { fn get_polar_class_builder() -> oso::ClassBuilder { oso::Class::builder() @@ -295,21 +325,6 @@ impl From<&db::model::Project> for Project { } } -impl Project { - fn resource( - &self, - resource_type: ResourceType, - resource_id: Uuid, - ) -> ProjectResource { - ProjectResource { - organization_id: self.organization_id, - project_id: self.project_id, - resource_type, - resource_id, - } - } -} - /// Wraps any resource that lives inside a Project for Polar /// /// This would include [`db::model::Instance`], [`db::model::Disk`], etc. @@ -321,6 +336,12 @@ pub struct ProjectResource { resource_id: Uuid, } +impl ProjectResource { + pub fn id(&self) -> &Uuid { + &self.resource_id + } +} + impl oso::PolarClass for ProjectResource { fn get_polar_class_builder() -> oso::ClassBuilder { oso::Class::builder() diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index 91cec9d5ea6..bcde4fca5f2 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -903,10 +903,18 @@ impl DataStore { }) } - pub async fn project_delete_disk(&self, disk_id: &Uuid) -> DeleteResult { + pub async fn project_delete_disk( + &self, + opctx: &OpContext, + disk_authz: authz::ProjectResource, + ) -> DeleteResult { use db::schema::disk::dsl; let now = Utc::now(); + let disk_id = disk_authz.id(); + // XXX TODO-coverage add tests for this + opctx.authorize(authz::Action::Delete, disk_authz)?; + let destroyed = api::external::DiskState::Destroyed.label(); let detached = api::external::DiskState::Detached.label(); let faulted = api::external::DiskState::Faulted.label(); @@ -917,7 +925,7 @@ impl DataStore { .filter(dsl::disk_state.eq_any(vec![detached, faulted])) .set((dsl::disk_state.eq(destroyed), dsl::time_deleted.eq(now))) .check_if_exists::(*disk_id) - .execute_and_check(self.pool()) + .execute_and_check(self.pool_authorized(opctx)?) .await .map_err(|e| { public_error_from_diesel_pool( diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 830e11bdf82..92353e189ca 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -597,7 +597,7 @@ async fn project_disks_get_disk( let project_name = &path.project_name; let disk_name = &path.disk_name; let handler = async { - let disk = nexus + let (disk, _) = nexus .project_lookup_disk(&organization_name, &project_name, &disk_name) .await?; Ok(HttpResponseOk(disk.into())) @@ -623,8 +623,14 @@ async fn project_disks_delete_disk( let project_name = &path.project_name; let disk_name = &path.disk_name; let handler = async { + let opctx = OpContext::for_external_api(&rqctx).await?; nexus - .project_delete_disk(&organization_name, &project_name, &disk_name) + .project_delete_disk( + &opctx, + &organization_name, + &project_name, + &disk_name, + ) .await?; Ok(HttpResponseDeleted()) }; diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index 849313487a4..40810eceeb3 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -640,7 +640,7 @@ impl Nexus { organization_name: &Name, project_name: &Name, disk_name: &Name, - ) -> LookupResult { + ) -> LookupResult<(db::model::Disk, authz::ProjectResource)> { let organization_id = self .db_datastore .organization_lookup_id_by_name(organization_name) @@ -649,16 +649,28 @@ impl Nexus { .db_datastore .project_lookup_id_by_name(&organization_id, project_name) .await?; - self.db_datastore.disk_fetch_by_name(&project_id, disk_name).await + let disk = self + .db_datastore + .disk_fetch_by_name(&project_id, disk_name) + .await?; + let disk_id = disk.id(); + Ok(( + disk, + authz::FLEET + .organization(organization_id) + .project(project_id) + .resource(ResourceType::Disk, disk_id), + )) } pub async fn project_delete_disk( &self, + opctx: &OpContext, organization_name: &Name, project_name: &Name, disk_name: &Name, ) -> DeleteResult { - let disk = self + let (disk, authz_disk) = self .project_lookup_disk(organization_name, project_name, disk_name) .await?; let runtime = disk.runtime(); @@ -688,7 +700,7 @@ impl Nexus { * before actually beginning the attach process. Sagas can maybe * address that. */ - self.db_datastore.project_delete_disk(&disk.id()).await + self.db_datastore.project_delete_disk(opctx, authz_disk).await } /* @@ -1127,7 +1139,7 @@ impl Nexus { .await?; // TODO: This shouldn't be looking up multiple database entries by name, // it should resolve names to IDs first. - let disk = self + let (disk, _) = self .project_lookup_disk(organization_name, project_name, disk_name) .await?; if let Some(instance_id) = disk.runtime_state.attach_instance_id { @@ -1170,7 +1182,7 @@ impl Nexus { .await?; // TODO: This shouldn't be looking up multiple database entries by name, // it should resolve names to IDs first. - let disk = self + let (disk, _) = self .project_lookup_disk(organization_name, project_name, disk_name) .await?; let instance_id = &instance.id(); @@ -1297,7 +1309,7 @@ impl Nexus { .await?; // TODO: This shouldn't be looking up multiple database entries by name, // it should resolve names to IDs first. - let disk = self + let (disk, _) = self .project_lookup_disk(organization_name, project_name, disk_name) .await?; let instance_id = &instance.id(); diff --git a/nexus/tests/common/http_testing.rs b/nexus/tests/common/http_testing.rs index 7770d9c0da9..9b332e74fe4 100644 --- a/nexus/tests/common/http_testing.rs +++ b/nexus/tests/common/http_testing.rs @@ -409,7 +409,7 @@ impl<'a> NexusRequest<'a> { /// Returns a new `NexusRequest` suitable for `DELETE $uri` pub fn object_delete(testctx: &'a ClientTestContext, uri: &str) -> Self { NexusRequest::new( - RequestBuilder::new(testctx, http::Method::GET, uri) + RequestBuilder::new(testctx, http::Method::DELETE, uri) .expect_status(Some(http::StatusCode::NO_CONTENT)), ) } diff --git a/nexus/tests/test_disks.rs b/nexus/tests/test_disks.rs index c6117f670ed..76c832acc48 100644 --- a/nexus/tests/test_disks.rs +++ b/nexus/tests/test_disks.rs @@ -26,6 +26,9 @@ use dropshot::test_util::read_json; use dropshot::test_util::ClientTestContext; pub mod common; +use common::http_testing::AuthnMode; +use common::http_testing::NexusRequest; +use common::http_testing::RequestBuilder; use common::identity_eq; use common::resource_helpers::create_organization; use common::resource_helpers::create_project; @@ -62,9 +65,16 @@ async fn test_disks() { assert_eq!(error.message, "not found: disk with name \"just-rainsticks\""); /* We should also get a 404 if we delete one. */ - let error = client - .make_request_error(Method::DELETE, &disk_url, StatusCode::NOT_FOUND) - .await; + let error = NexusRequest::new( + RequestBuilder::new(client, Method::DELETE, &disk_url) + .expect_status(Some(StatusCode::NOT_FOUND)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("unexpected success") + .response_body::() + .unwrap(); assert_eq!(error.message, "not found: disk with name \"just-rainsticks\""); /* Create a disk. */ @@ -492,10 +502,11 @@ async fn test_disks() { ); /* It's not allowed to delete a disk that's detaching, either. */ - client - .make_request_no_body(Method::DELETE, &disk_url, StatusCode::NO_CONTENT) + NexusRequest::object_delete(client, &disk_url) + .authn_as(AuthnMode::PrivilegedUser) + .execute() .await - .unwrap(); + .expect("failed to delete disk"); /* It should no longer be present in our list of disks. */ assert_eq!(disks_list(&client, &url_disks).await.len(), 0); From cb93be0bb9713291dd8bbba8a9018db89e19cd2e Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Fri, 19 Nov 2021 12:47:36 -0800 Subject: [PATCH 06/12] fix oxapi_demo for disk delete --- tools/oxapi_demo | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/oxapi_demo b/tools/oxapi_demo index 60d46317e37..bae46f78bea 100755 --- a/tools/oxapi_demo +++ b/tools/oxapi_demo @@ -304,7 +304,7 @@ function cmd_disk_get function cmd_disk_delete { [[ $# != 3 ]] && usage "expected ORGANIZATION_NAME PROJECT_NAME DISK_NAME" - do_curl "/organizations/$1/projects/$2/disks/$3" -X DELETE + do_curl_authz "/organizations/$1/projects/$2/disks/$3" -X DELETE } function cmd_vpc_create_demo From 5a0badf42fea604144502635e9768b056db078f4 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Fri, 19 Nov 2021 12:49:03 -0800 Subject: [PATCH 07/12] fix style --- nexus/src/authz/oso_types.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/nexus/src/authz/oso_types.rs b/nexus/src/authz/oso_types.rs index 80ec3742c91..34f46f39896 100644 --- a/nexus/src/authz/oso_types.rs +++ b/nexus/src/authz/oso_types.rs @@ -243,10 +243,7 @@ pub struct Organization { impl Organization { pub fn project(&self, project_id: Uuid) -> Project { - Project { - organization_id: self.organization_id, - project_id, - } + Project { organization_id: self.organization_id, project_id } } } From 65dd0c7657475d1ee93d9f1aeac35cd5395fdb26 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Fri, 19 Nov 2021 18:48:57 -0800 Subject: [PATCH 08/12] test disk authz --- nexus/src/db/datastore.rs | 1 - nexus/tests/test_disks.rs | 20 +++++++++++++++++++- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index bcde4fca5f2..2754b8ee6d6 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -912,7 +912,6 @@ impl DataStore { let now = Utc::now(); let disk_id = disk_authz.id(); - // XXX TODO-coverage add tests for this opctx.authorize(authz::Action::Delete, disk_authz)?; let destroyed = api::external::DiskState::Destroyed.label(); diff --git a/nexus/tests/test_disks.rs b/nexus/tests/test_disks.rs index 76c832acc48..91d1d0cb6ae 100644 --- a/nexus/tests/test_disks.rs +++ b/nexus/tests/test_disks.rs @@ -501,7 +501,25 @@ async fn test_disks() { "disk \"just-rainsticks\" is not attached to instance \"instance2\"" ); - /* It's not allowed to delete a disk that's detaching, either. */ + /* + * If we're not authenticated, or authenticated as an unprivileged user, we + * shouldn't be able to delete this disk. + */ + NexusRequest::new( + RequestBuilder::new(client, Method::DELETE, &disk_url) + .expect_status(Some(StatusCode::UNAUTHORIZED)), + ) + .execute() + .await + .expect("expected request to fail"); + NexusRequest::new( + RequestBuilder::new(client, Method::DELETE, &disk_url) + .expect_status(Some(StatusCode::FORBIDDEN)), + ) + .authn_as(AuthnMode::UnprivilegedUser) + .execute() + .await + .expect("expected request to fail"); NexusRequest::object_delete(client, &disk_url) .authn_as(AuthnMode::PrivilegedUser) .execute() From 5c4c3dac9c7ba4ca8059d579ade6edf6e6ad3074 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Fri, 19 Nov 2021 18:50:37 -0800 Subject: [PATCH 09/12] remove one XXX --- nexus/src/authz/oso_types.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/nexus/src/authz/oso_types.rs b/nexus/src/authz/oso_types.rs index 34f46f39896..acd0dd5c4e0 100644 --- a/nexus/src/authz/oso_types.rs +++ b/nexus/src/authz/oso_types.rs @@ -177,11 +177,11 @@ impl AuthenticatedActor { * Returns whether this actor has the given role for a fleet */ /* - * XXX This is special-cased only because Fleets don't exist in the database - * and do not have an id. It might be a good idea to put them in the - * database with their own id, though it might mean that lots of authz - * checks need to do an extra database query to load the fleet_id from the - * Organization. + * This is special-cased because Fleets don't exist in the database and do + * not have an id. It might be a good idea to put them in the database with + * their own id. But it's not needed for anything right now. (It might + * also mean that many authz checks would need to do an extra database query + * to load the fleet_id from the Organization.) */ fn has_role_fleet(&self, _fleet: &Fleet, _role: &str) -> bool { self.actor_id.to_string() == authn::TEST_USER_UUID_PRIVILEGED From 223567b33eb52e73e1a26048da9ad61ebe301914 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Fri, 19 Nov 2021 19:18:09 -0800 Subject: [PATCH 10/12] remove another todo --- nexus/src/authz/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nexus/src/authz/mod.rs b/nexus/src/authz/mod.rs index a58c88c7aa3..827bf5561c4 100644 --- a/nexus/src/authz/mod.rs +++ b/nexus/src/authz/mod.rs @@ -87,8 +87,8 @@ impl Context { mod test { /* * These are essentially unit tests for the policy itself. - * TODO-coverage This is just a start. - * XXX Flesh these out now that we have a more realistic policy and types + * TODO-coverage This is just a start. But we need roles to do a more + * comprehensive test. * 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 From 50f2bd7d860c01df7060d04dfc956d02e3f532a7 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Mon, 22 Nov 2021 12:36:51 -0800 Subject: [PATCH 11/12] rename ProjectResource --- nexus/src/authz/mod.rs | 2 +- nexus/src/authz/omicron.polar | 6 +++--- nexus/src/authz/oso_types.rs | 16 ++++++++-------- nexus/src/db/datastore.rs | 2 +- nexus/src/nexus.rs | 2 +- 5 files changed, 14 insertions(+), 14 deletions(-) diff --git a/nexus/src/authz/mod.rs b/nexus/src/authz/mod.rs index 827bf5561c4..4ad5f7663ac 100644 --- a/nexus/src/authz/mod.rs +++ b/nexus/src/authz/mod.rs @@ -9,7 +9,7 @@ mod oso_types; pub use oso_types::Action; pub use oso_types::Organization; pub use oso_types::Project; -pub use oso_types::ProjectResource; +pub use oso_types::ProjectChild; pub use oso_types::DATABASE; pub use oso_types::FLEET; diff --git a/nexus/src/authz/omicron.polar b/nexus/src/authz/omicron.polar index 2b282b579e1..bc7f521dc5e 100644 --- a/nexus/src/authz/omicron.polar +++ b/nexus/src/authz/omicron.polar @@ -162,7 +162,7 @@ resource Project { # For now, we use one generic resource to represent every kind of thing inside # the Project. That's because they all have the same behavior. -resource ProjectResource { +resource ProjectChild { permissions = [ "list_children", "modify", @@ -183,8 +183,8 @@ has_relation(fleet: Fleet, "parent_fleet", organization: Organization) if organization.fleet = fleet; has_relation(organization: Organization, "parent_organization", project: Project) if project.organization = organization; -has_relation(project: Project, "parent_project", project_resource: ProjectResource) - if project_resource.project = project; +has_relation(project: Project, "parent_project", project_child: ProjectChild) + if project_child.project = project; # Define role relationships has_role(actor: AuthenticatedActor, role: String, resource: Resource) diff --git a/nexus/src/authz/oso_types.rs b/nexus/src/authz/oso_types.rs index acd0dd5c4e0..452bf56b975 100644 --- a/nexus/src/authz/oso_types.rs +++ b/nexus/src/authz/oso_types.rs @@ -41,7 +41,7 @@ pub fn make_omicron_oso() -> Result { Fleet::get_polar_class(), Organization::get_polar_class(), Project::get_polar_class(), - ProjectResource::get_polar_class(), + ProjectChild::get_polar_class(), ]; for c in classes { oso.register_class(c).context("registering class")?; @@ -283,8 +283,8 @@ impl Project { &self, resource_type: ResourceType, resource_id: Uuid, - ) -> ProjectResource { - ProjectResource { + ) -> ProjectChild { + ProjectChild { organization_id: self.organization_id, project_id: self.project_id, resource_type, @@ -326,26 +326,26 @@ impl From<&db::model::Project> for Project { /// /// This would include [`db::model::Instance`], [`db::model::Disk`], etc. #[derive(Clone, Copy, Debug, Eq, PartialEq)] -pub struct ProjectResource { +pub struct ProjectChild { organization_id: Uuid, project_id: Uuid, resource_type: ResourceType, resource_id: Uuid, } -impl ProjectResource { +impl ProjectChild { pub fn id(&self) -> &Uuid { &self.resource_id } } -impl oso::PolarClass for ProjectResource { +impl oso::PolarClass for ProjectChild { fn get_polar_class_builder() -> oso::ClassBuilder { oso::Class::builder() .set_equality_check(|a1, a2| a1 == a2) .add_method( "has_role", - |pr: &ProjectResource, + |pr: &ProjectChild, actor: AuthenticatedActor, role: String| { actor.has_role_resource( @@ -355,7 +355,7 @@ impl oso::PolarClass for ProjectResource { ) }, ) - .add_attribute_getter("project", |pr: &ProjectResource| Project { + .add_attribute_getter("project", |pr: &ProjectChild| Project { organization_id: pr.organization_id, project_id: pr.project_id, }) diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index 2754b8ee6d6..9458f85a2d2 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -906,7 +906,7 @@ impl DataStore { pub async fn project_delete_disk( &self, opctx: &OpContext, - disk_authz: authz::ProjectResource, + disk_authz: authz::ProjectChild, ) -> DeleteResult { use db::schema::disk::dsl; let now = Utc::now(); diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index 40810eceeb3..2c398f50b37 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -640,7 +640,7 @@ impl Nexus { organization_name: &Name, project_name: &Name, disk_name: &Name, - ) -> LookupResult<(db::model::Disk, authz::ProjectResource)> { + ) -> LookupResult<(db::model::Disk, authz::ProjectChild)> { let organization_id = self .db_datastore .organization_lookup_id_by_name(organization_name) From 65934269d81d8a5df0ad13aea554ce7b983440cc Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Mon, 22 Nov 2021 12:37:14 -0800 Subject: [PATCH 12/12] fix style --- nexus/src/authz/oso_types.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/nexus/src/authz/oso_types.rs b/nexus/src/authz/oso_types.rs index 452bf56b975..0887be2ace8 100644 --- a/nexus/src/authz/oso_types.rs +++ b/nexus/src/authz/oso_types.rs @@ -345,9 +345,7 @@ impl oso::PolarClass for ProjectChild { .set_equality_check(|a1, a2| a1 == a2) .add_method( "has_role", - |pr: &ProjectChild, - actor: AuthenticatedActor, - role: String| { + |pr: &ProjectChild, actor: AuthenticatedActor, role: String| { actor.has_role_resource( pr.resource_type, pr.resource_id,