authz: fix several bugs (#1110)

common/src/sql/dbinit.sql

@@ -273,6 +273,7 @@ CREATE TABLE omicron.public.organization (
 );
 
 CREATE UNIQUE INDEX ON omicron.public.organization (
+    silo_id,
     name
 ) WHERE
     time_deleted IS NULL;

nexus/src/app/project.rs

@@ -96,7 +96,7 @@ impl super::Nexus {
     ) -> ListResultVec<db::model::Project> {
         let (.., authz_org) = LookupPath::new(opctx, &self.db_datastore)
             .organization_name(organization_name)
-            .lookup_for(authz::Action::CreateChild)
+            .lookup_for(authz::Action::ListChildren)
             .await?;
         self.db_datastore
             .projects_list_by_name(opctx, &authz_org, pagparams)
@@ -111,7 +111,7 @@ impl super::Nexus {
     ) -> ListResultVec<db::model::Project> {
         let (.., authz_org) = LookupPath::new(opctx, &self.db_datastore)
             .organization_name(organization_name)
-            .lookup_for(authz::Action::CreateChild)
+            .lookup_for(authz::Action::ListChildren)
             .await?;
         self.db_datastore
             .projects_list_by_id(opctx, &authz_org, pagparams)

nexus/src/authz/omicron.polar

@@ -137,23 +137,41 @@ resource Silo {
 	];
 	roles = [ "admin", "collaborator", "viewer" ];
 
+	# permissions granted by this resource's roles
 	"list_children" if "viewer";
 	"read" if "viewer";
+	"create_child" if "collaborator";
+	"modify" if "admin";
 
+	# roles implied by other roles
 	"viewer" if "collaborator";
-	"create_child" if "collaborator";
 	"collaborator" if "admin";
-	"modify" if "admin";
+
+	# roles implied by relationships with the parent fleet
 	relations = { parent_fleet: Fleet };
-	"admin" if "admin" on "parent_fleet";
-	"collaborator" if "collaborator" on "parent_fleet";
+	"admin" if "collaborator" on "parent_fleet";
 	"viewer" if "viewer" on "parent_fleet";
 }
 has_relation(fleet: Fleet, "parent_fleet", silo: Silo)
 	if silo.fleet = fleet;
-has_role(actor: AuthenticatedActor, "viewer", silo: Silo)
+
+# All authenticated users can read their own Silo.  That's not quite the same as
+# having the "viewer" role.  For example, they cannot list Organizations in the
+# Silo.
+#
+# One reason this is necessary is because if an unprivileged user tries to
+# create an Organization using "POST /organizations", they should get back a 403
+# (which implies they're able to see /organizations, which is essentially seeing
+# the Silo itself) rather than a 404.  This behavior isn't a hard constraint
+# (i.e., you could reasonably get a 404 for an API you're not allowed to call).
+# Nor is the implementation (i.e., we could special-case this endpoint somehow).
+# But granting this permission is the simplest way to keep this endpoint's
+# behavior consistent with the rest of the API.
+#
+# It's unclear what else would break if users couldn't see their own Silo.
+has_permission(actor: AuthenticatedActor, "read", silo: Silo)
 	# TODO-security TODO-coverage We should have a test that exercises this
-	# case.
+	# syntax.
 	if silo in actor.silo;
 
 resource Organization {
@@ -180,7 +198,9 @@ resource Organization {
 	"modify" if "admin";
 
 	relations = { parent_silo: Silo };
-	"admin" if "admin" on "parent_silo";
+	"admin" if "collaborator" on "parent_silo";
+	"read" if "viewer" on "parent_silo";
+	"list_children" if "viewer" on "parent_silo";
 }
 has_relation(silo: Silo, "parent_silo", organization: Organization)
 	if organization.silo = silo;
@@ -212,7 +232,9 @@ resource Project {
 	"modify" if "admin";
 
 	relations = { parent_organization: Organization };
-	"admin" if "admin" on "parent_organization";
+	"admin" if "collaborator" on "parent_organization";
+
+	"viewer" if "list_children" on "parent_organization";
 }
 has_relation(organization: Organization, "parent_organization", project: Project)
 	if project.organization = organization;

nexus/src/db/datastore.rs

@@ -676,7 +676,7 @@ impl DataStore {
         pagparams: &DataPageParams<'_, Name>,
     ) -> ListResultVec<Organization> {
         let authz_silo = opctx.authn.silo_required()?;
-        opctx.authorize(authz::Action::ListChildren, &authz::FLEET).await?;
+        opctx.authorize(authz::Action::ListChildren, &authz_silo).await?;
 
         use db::schema::organization::dsl;
         paginated(dsl::organization, dsl::name, pagparams)

nexus/test-utils/src/resource_helpers.rs

@@ -18,6 +18,8 @@ use omicron_common::api::external::Instance;
 use omicron_common::api::external::InstanceCpuCount;
 use omicron_nexus::crucible_agent_client::types::State as RegionState;
 use omicron_nexus::external_api::params;
+use omicron_nexus::external_api::shared;
+use omicron_nexus::external_api::shared::IdentityType;
 use omicron_nexus::external_api::views::{
     Organization, Project, Silo, Vpc, VpcRouter,
 };
@@ -280,6 +282,51 @@ pub async fn create_router(
     .unwrap()
 }
 
+/// Grant a role on a resource to a user
+///
+/// * `grant_resource_url`: URL of the resource we're granting the role on
+/// * `grant_role`: the role we're granting
+/// * `grant_user`: the uuid of the user we're granting the role to
+/// * `run_as`: the user _doing_ the granting
+pub async fn grant_iam<T>(
+    client: &ClientTestContext,
+    grant_resource_url: &str,
+    grant_role: T,
+    grant_user: Uuid,
+    run_as: AuthnMode,
+) where
+    T: serde::Serialize + serde::de::DeserializeOwned,
+{
+    let policy_url = format!("{}/policy", grant_resource_url);
+    let existing_policy: shared::Policy<T> =
+        NexusRequest::object_get(client, &policy_url)
+            .authn_as(run_as.clone())
+            .execute()
+            .await
+            .expect("failed to fetch policy")
+            .parsed_body()
+            .expect("failed to parse policy");
+    let new_role_assignment = shared::RoleAssignment {
+        identity_type: IdentityType::SiloUser,
+        identity_id: grant_user,
+        role_name: grant_role,
+    };
+    let new_role_assignments = existing_policy
+        .role_assignments
+        .into_iter()
+        .chain(std::iter::once(new_role_assignment))
+        .collect();
+
+    let new_policy = shared::Policy { role_assignments: new_role_assignments };
+
+    // TODO-correctness use etag when we have it
+    NexusRequest::object_put(client, &policy_url, Some(&new_policy))
+        .authn_as(run_as)
+        .execute()
+        .await
+        .expect("failed to update policy");
+}
+
 pub async fn project_get(
     client: &ClientTestContext,
     project_url: &str,

nexus/tests/integration_tests/silos.rs

@@ -3,17 +3,20 @@
 // file, You can obtain one at https://mozilla.org/MPL/2.0/.
 
 use nexus_test_utils::http_testing::{AuthnMode, NexusRequest};
-use omicron_nexus::external_api::views::{Organization, Silo};
+use omicron_common::api::external::{IdentityMetadataCreateParams, Name};
+use omicron_nexus::external_api::views::{self, Organization, Silo};
 use omicron_nexus::TestInterfaces as _;
 
 use http::method::Method;
 use http::StatusCode;
 use nexus_test_utils::resource_helpers::{
-    create_organization, create_silo, objects_list_page_authz,
+    create_organization, create_silo, grant_iam, objects_list_page_authz,
 };
 
 use nexus_test_utils::ControlPlaneTestContext;
 use nexus_test_utils_macros::nexus_test;
+use omicron_nexus::authz::SiloRoles;
+use omicron_nexus::external_api::params;
 
 #[nexus_test]
 async fn test_silos(cptestctx: &ControlPlaneTestContext) {
@@ -74,14 +77,62 @@ async fn test_silos(cptestctx: &ControlPlaneTestContext) {
         .await
         .unwrap();
 
+    // Grant the user "admin" privileges on that Silo.
+    grant_iam(
+        client,
+        "/silos/discoverable",
+        SiloRoles::Admin,
+        new_silo_user_id,
+        AuthnMode::PrivilegedUser,
+    )
+    .await;
+
     // TODO-coverage, TODO-security: Add test for Silo-local session
     // when we can use users in another Silo.
 
     let authn_opctx = nexus.opctx_external_authn();
 
     // Create organization with built-in user auth
     // Note: this currently goes to the built-in silo!
-    create_organization(&client, "someorg").await;
+    let org_name: Name = "someorg".parse().unwrap();
+    let new_org_in_default_silo =
+        create_organization(&client, org_name.as_str()).await;
+
+    // Create an Organization of the same name in a different Silo to verify
+    // that's possible.
+    let new_org_in_our_silo = NexusRequest::objects_post(
+        client,
+        "/organizations",
+        &params::OrganizationCreate {
+            identity: IdentityMetadataCreateParams {
+                name: org_name.clone(),
+                description: String::new(),
+            },
+        },
+    )
+    .authn_as(AuthnMode::SiloUser(new_silo_user_id))
+    .execute()
+    .await
+    .expect("failed to create same-named Organization in a different Silo")
+    .parsed_body::<views::Organization>()
+    .expect("failed to parse new Organization");
+    assert_eq!(
+        new_org_in_default_silo.identity.name,
+        new_org_in_our_silo.identity.name
+    );
+    assert_ne!(
+        new_org_in_default_silo.identity.id,
+        new_org_in_our_silo.identity.id
+    );
+    // Delete it so that we can delete the Silo later.
+    NexusRequest::object_delete(
+        client,
+        &format!("/organizations/{}", org_name),
+    )
+    .authn_as(AuthnMode::SiloUser(new_silo_user_id))
+    .execute()
+    .await
+    .expect("failed to delete test Organization");
 
     // Verify GET /organizations works with built-in user auth
     let organizations =