From fe4bc67046edac694385b52cbcb5fa8c64228f0b Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Thu, 5 Nov 2026 14:54:11 -0800 Subject: [PATCH 01/10] Delete role assignments when users or groups are deleted --- nexus/db-queries/src/db/datastore/role.rs | 30 ++ nexus/db-queries/src/db/datastore/silo.rs | 81 +++-- .../db-queries/src/db/datastore/silo_group.rs | 24 +- .../db-queries/src/db/datastore/silo_user.rs | 8 + nexus/tests/integration_tests/silos.rs | 288 +++++++++++++++++- 5 files changed, 402 insertions(+), 29 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/role.rs b/nexus/db-queries/src/db/datastore/role.rs index 18e9f5910dc..2bd2496dc7b 100644 --- a/nexus/db-queries/src/db/datastore/role.rs +++ b/nexus/db-queries/src/db/datastore/role.rs @@ -5,6 +5,7 @@ //! [`DataStore`] methods related to roles. use super::DataStore; +use super::SQL_BATCH_SIZE; use crate::authz; use crate::authz::AuthorizedResource; use crate::context::OpContext; @@ -16,6 +17,7 @@ use crate::db::model::IdentityType; use crate::db::model::RoleAssignment; use async_bb8_diesel::AsyncRunQueryDsl; use diesel::prelude::*; +use diesel::result::Error as DieselError; use nexus_db_errors::ErrorHandler; use nexus_db_errors::TransactionError; use nexus_db_errors::public_error_from_diesel; @@ -25,6 +27,7 @@ use nexus_types::external_api::policy; use omicron_common::api::external::Error; use omicron_common::api::external::ListResultVec; use omicron_common::bail_unless; +use uuid::Uuid; impl DataStore { /// Load role assignments for built-in users and built-in roles into the @@ -103,6 +106,33 @@ impl DataStore { .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) } + /// Delete all role assignments held by the given identities, on any + /// resource. Used when deleting users, groups, and silos so that deleted + /// identities don't linger in policies. + /// + /// Returns the raw diesel error so this can run inside a transaction; + /// callers outside one should map it with `public_error_from_diesel`. + /// + /// Returns the number of rows deleted. + pub(super) async fn role_assignments_delete_for_identities( + &self, + conn: &async_bb8_diesel::Connection, + identity_type: IdentityType, + identity_ids: &[Uuid], + ) -> Result { + use nexus_db_schema::schema::role_assignment::dsl; + + let mut deleted = 0; + for batch in identity_ids.chunks(SQL_BATCH_SIZE.get() as usize) { + deleted += diesel::delete(dsl::role_assignment) + .filter(dsl::identity_type.eq(identity_type.clone())) + .filter(dsl::identity_id.eq_any(batch.to_vec())) + .execute_async(conn) + .await?; + } + Ok(deleted) + } + /// Removes all existing externally-visble role assignments on /// `authz_resource` and adds those specified by `new_assignments` /// diff --git a/nexus/db-queries/src/db/datastore/silo.rs b/nexus/db-queries/src/db/datastore/silo.rs index cae0cb53931..21ce081422f 100644 --- a/nexus/db-queries/src/db/datastore/silo.rs +++ b/nexus/db-queries/src/db/datastore/silo.rs @@ -561,21 +561,44 @@ impl DataStore { "deleted {} password hashes for silo {}", updated_rows, id ); - let updated_rows = diesel::update(silo_user::dsl::silo_user) - .filter(silo_user::dsl::silo_id.eq(to_db_typed_uuid(id))) - .filter(silo_user::dsl::time_deleted.is_null()) - .filter( - silo_user::dsl::user_provision_type - .eq(db_silo.user_provision_type), + let deleted_user_ids: Vec = + diesel::update(silo_user::dsl::silo_user) + .filter(silo_user::dsl::silo_id.eq(to_db_typed_uuid(id))) + .filter(silo_user::dsl::time_deleted.is_null()) + .filter( + silo_user::dsl::user_provision_type + .eq(db_silo.user_provision_type), + ) + .set(silo_user::dsl::time_deleted.eq(now)) + .returning(silo_user::dsl::id) + .get_results_async(&*conn) + .await + .map_err(|e| { + public_error_from_diesel(e, ErrorHandler::Server) + })?; + + debug!( + opctx.log, + "deleted {} silo users for silo {}", + deleted_user_ids.len(), + id + ); + + // delete role assignments held by the silo's users + let updated_rows = self + .role_assignments_delete_for_identities( + &conn, + db::model::IdentityType::SiloUser, + &deleted_user_ids, ) - .set(silo_user::dsl::time_deleted.eq(now)) - .execute_async(&*conn) .await .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; debug!( opctx.log, - "deleted {} silo users for silo {}", updated_rows, id + "deleted {} silo user role assignments for silo {}", + updated_rows, + id ); // delete all silo group memberships @@ -608,21 +631,43 @@ impl DataStore { ); // delete all silo groups - let updated_rows = diesel::update(silo_group::dsl::silo_group) - .filter(silo_group::dsl::silo_id.eq(to_db_typed_uuid(id))) - .filter(silo_group::dsl::time_deleted.is_null()) - .filter( - silo_group::dsl::user_provision_type - .eq(db_silo.user_provision_type), + let deleted_group_ids: Vec = + diesel::update(silo_group::dsl::silo_group) + .filter(silo_group::dsl::silo_id.eq(to_db_typed_uuid(id))) + .filter(silo_group::dsl::time_deleted.is_null()) + .filter( + silo_group::dsl::user_provision_type + .eq(db_silo.user_provision_type), + ) + .set(silo_group::dsl::time_deleted.eq(now)) + .returning(silo_group::dsl::id) + .get_results_async(&*conn) + .await + .map_err(|e| { + public_error_from_diesel(e, ErrorHandler::Server) + })?; + + debug!( + opctx.log, + "deleted {} silo groups for silo {}", + deleted_group_ids.len(), + id + ); + + let updated_rows = self + .role_assignments_delete_for_identities( + &conn, + db::model::IdentityType::SiloGroup, + &deleted_group_ids, ) - .set(silo_group::dsl::time_deleted.eq(now)) - .execute_async(&*conn) .await .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; debug!( opctx.log, - "deleted {} silo groups for silo {}", updated_rows, id + "deleted {} silo group role assignments for silo {}", + updated_rows, + id ); // delete all silo identity providers diff --git a/nexus/db-queries/src/db/datastore/silo_group.rs b/nexus/db-queries/src/db/datastore/silo_group.rs index 812b739a9a1..07450388b17 100644 --- a/nexus/db-queries/src/db/datastore/silo_group.rs +++ b/nexus/db-queries/src/db/datastore/silo_group.rs @@ -721,13 +721,23 @@ impl DataStore { } // Delete silo group - use nexus_db_schema::schema::silo_group::dsl; - diesel::update(dsl::silo_group) - .filter(dsl::id.eq(to_db_typed_uuid(group_id))) - .filter(dsl::time_deleted.is_null()) - .set(dsl::time_deleted.eq(Utc::now())) - .execute_async(&conn) - .await?; + { + use nexus_db_schema::schema::silo_group::dsl; + diesel::update(dsl::silo_group) + .filter(dsl::id.eq(to_db_typed_uuid(group_id))) + .filter(dsl::time_deleted.is_null()) + .set(dsl::time_deleted.eq(Utc::now())) + .execute_async(&conn) + .await?; + } + + // delete group's role assignments + self.role_assignments_delete_for_identities( + &conn, + model::IdentityType::SiloGroup, + &[group_id.into_untyped_uuid()], + ) + .await?; Ok(()) }) diff --git a/nexus/db-queries/src/db/datastore/silo_user.rs b/nexus/db-queries/src/db/datastore/silo_user.rs index b6ba755b01a..af49b49fc79 100644 --- a/nexus/db-queries/src/db/datastore/silo_user.rs +++ b/nexus/db-queries/src/db/datastore/silo_user.rs @@ -550,6 +550,14 @@ impl DataStore { .await?; } + // delete the user's role assignments + self.role_assignments_delete_for_identities( + &conn, + model::IdentityType::SiloUser, + &[authz_silo_user_id.into_untyped_uuid()], + ) + .await?; + // Delete ssh keys. { use nexus_db_schema::schema::ssh_key::dsl; diff --git a/nexus/tests/integration_tests/silos.rs b/nexus/tests/integration_tests/silos.rs index c7564ebc63e..3890010a360 100644 --- a/nexus/tests/integration_tests/silos.rs +++ b/nexus/tests/integration_tests/silos.rs @@ -19,9 +19,9 @@ use nexus_db_queries::db::identity::Asset; use nexus_test_utils::http_testing::{AuthnMode, NexusRequest, RequestBuilder}; use nexus_test_utils::resource_helpers::{ create_ip_pool, create_local_user, create_project, create_silo, - create_subnet_pool, grant_iam, link_ip_pool, link_subnet_pool, - object_create, object_delete, objects_list_page_authz, projects_list, - test_params, + create_subnet_pool, grant_iam, grant_iam_for_group, link_ip_pool, + link_subnet_pool, object_create, object_delete, objects_list_page_authz, + projects_list, test_params, }; use nexus_test_utils_macros::nexus_test; use nexus_types::external_api::certificate; @@ -51,12 +51,15 @@ use hickory_resolver::proto::ProtoErrorKind; use http::StatusCode; use http::method::Method; use httptest::{Expectation, Server, matchers::*, responders::*}; -use nexus_types::external_api::policy::{FleetRole, SiloRole}; +use nexus_types::external_api::policy::{ + self, FleetRole, ProjectRole, SiloRole, +}; use omicron_uuid_kinds::GenericUuid; use omicron_uuid_kinds::SiloUuid; use std::convert::Infallible; use std::net::Ipv4Addr; use std::time::Duration; +use uuid::Uuid; type ControlPlaneTestContext = nexus_test_utils::ControlPlaneTestContext; @@ -1471,6 +1474,283 @@ async fn test_silo_delete_clean_up_groups(cptestctx: &ControlPlaneTestContext) { .expect_err("user found"); } +/// Fetch a resource's policy and return the identity ids it names +async fn policy_identity_ids( + client: &dropshot::test_util::ClientTestContext, + policy_url: &str, + authn_as: AuthnMode, +) -> Vec { + let policy: policy::Policy = + NexusRequest::object_get(client, policy_url) + .authn_as(authn_as) + .execute() + .await + .expect("failed to fetch policy") + .parsed_body() + .expect("failed to parse policy"); + policy.role_assignments.into_iter().map(|r| r.identity_id).collect() +} + +#[nexus_test] +async fn test_silo_user_delete_clean_up_role_assignments( + cptestctx: &ControlPlaneTestContext, +) { + let client = &cptestctx.external_client; + + let silo = + create_silo(client, "test-silo", silo::SiloIdentityMode::LocalOnly) + .await; + let fleet_policy_url = "/v1/system/policy"; + let silo_policy_url = "/v1/system/silos/test-silo/policy"; + + let user = create_local_user( + client, + &silo, + &"role-user".parse().unwrap(), + test_params::UserPassword::LoginDisallowed, + ) + .await; + let user_id = user.id.into_untyped_uuid(); + + // Give the user roles on the silo, on the fleet, and on a project inside + // the silo, so that deletion has to clean up assignments on resources + // both inside and outside the silo. + grant_iam( + client, + "/v1/system/silos/test-silo", + SiloRole::Admin, + user.id, + AuthnMode::PrivilegedUser, + ) + .await; + grant_iam( + client, + "/v1/system", + FleetRole::Viewer, + user.id, + AuthnMode::PrivilegedUser, + ) + .await; + + let project: project::Project = NexusRequest::objects_post( + client, + "/v1/projects", + &project::ProjectCreate { + identity: IdentityMetadataCreateParams { + name: "role-project".parse().unwrap(), + description: String::new(), + }, + defaults: None, + }, + ) + .authn_as(AuthnMode::SiloUser(user.id)) + .execute() + .await + .expect("failed to create project") + .parsed_body() + .unwrap(); + let project_url = format!("/v1/projects/{}", project.identity.name); + let project_policy_url = format!("{project_url}/policy"); + grant_iam( + client, + &project_url, + ProjectRole::Admin, + user.id, + AuthnMode::SiloUser(user.id), + ) + .await; + + // A second silo admin, so we can still read the project's policy after + // the first user is gone, and to confirm their assignments are untouched. + let admin = create_local_user( + client, + &silo, + &"admin-user".parse().unwrap(), + test_params::UserPassword::LoginDisallowed, + ) + .await; + let admin_id = admin.id.into_untyped_uuid(); + grant_iam( + client, + "/v1/system/silos/test-silo", + SiloRole::Admin, + admin.id, + AuthnMode::PrivilegedUser, + ) + .await; + + let fleet_ids = policy_identity_ids::( + client, + fleet_policy_url, + AuthnMode::PrivilegedUser, + ) + .await; + let silo_ids = policy_identity_ids::( + client, + silo_policy_url, + AuthnMode::PrivilegedUser, + ) + .await; + let project_ids = policy_identity_ids::( + client, + &project_policy_url, + AuthnMode::SiloUser(admin.id), + ) + .await; + assert!(fleet_ids.contains(&user_id)); + assert!(silo_ids.contains(&user_id)); + assert!(silo_ids.contains(&admin_id)); + assert!(project_ids.contains(&user_id)); + + object_delete( + client, + &format!( + "/v1/system/identity-providers/local/users/{}?silo=test-silo", + user.id + ), + ) + .await; + + let fleet_ids = policy_identity_ids::( + client, + fleet_policy_url, + AuthnMode::PrivilegedUser, + ) + .await; + let silo_ids = policy_identity_ids::( + client, + silo_policy_url, + AuthnMode::PrivilegedUser, + ) + .await; + let project_ids = policy_identity_ids::( + client, + &project_policy_url, + AuthnMode::SiloUser(admin.id), + ) + .await; + assert!(!fleet_ids.contains(&user_id)); + assert!(!silo_ids.contains(&user_id)); + assert!(!project_ids.contains(&user_id)); + assert!(silo_ids.contains(&admin_id)); +} + +#[nexus_test] +async fn test_silo_delete_clean_up_role_assignments( + cptestctx: &ControlPlaneTestContext, +) { + let client = &cptestctx.external_client; + let nexus = &cptestctx.server.server_context().nexus; + let datastore = nexus.datastore(); + + let silo = + create_silo(client, "test-silo", silo::SiloIdentityMode::SamlJit).await; + + let opctx_external_authn = nexus.opctx_external_authn(); + let opctx = + OpContext::for_tests(cptestctx.logctx.log.new(o!()), datastore.clone()); + + let (authz_silo, db_silo) = LookupPath::new(&opctx, datastore) + .silo_name(&silo.identity.name.into()) + .fetch() + .await + .unwrap(); + + // Add a user with a group membership + let silo_user = nexus + .silo_user_from_authenticated_subject( + &opctx_external_authn, + &authz_silo, + &db_silo, + &AuthenticatedSubject { + external_id: "user@company.com".into(), + groups: vec!["sre".into()], + }, + ) + .await + .expect("silo_user_from_authenticated_subject"); + let silo_group = datastore + .silo_group_optional_lookup( + &opctx_external_authn, + &authz_silo, + &SiloGroupLookup::Jit { external_id: "sre" }, + ) + .await + .expect("silo_group_optional_lookup") + .expect("group should exist"); + let user_id = silo_user.id().into_untyped_uuid(); + let group_id = silo_group.id().into_untyped_uuid(); + + // Give both the user and the group roles on the silo and on the fleet. + // The fleet outlives the silo, so assignments there are the ones that + // would otherwise be left dangling. + let silo_url = "/v1/system/silos/test-silo"; + let fleet_url = "/v1/system"; + grant_iam( + client, + silo_url, + SiloRole::Viewer, + silo_user.id(), + AuthnMode::PrivilegedUser, + ) + .await; + grant_iam_for_group( + client, + silo_url, + SiloRole::Viewer, + silo_group.id(), + AuthnMode::PrivilegedUser, + ) + .await; + grant_iam( + client, + fleet_url, + FleetRole::Viewer, + silo_user.id(), + AuthnMode::PrivilegedUser, + ) + .await; + grant_iam_for_group( + client, + fleet_url, + FleetRole::Viewer, + silo_group.id(), + AuthnMode::PrivilegedUser, + ) + .await; + + let fleet_ids = policy_identity_ids::( + client, + "/v1/system/policy", + AuthnMode::PrivilegedUser, + ) + .await; + assert!(fleet_ids.contains(&user_id)); + assert!(fleet_ids.contains(&group_id)); + + object_delete(client, silo_url).await; + + let fleet_ids = policy_identity_ids::( + client, + "/v1/system/policy", + AuthnMode::PrivilegedUser, + ) + .await; + assert!(!fleet_ids.contains(&user_id)); + assert!(!fleet_ids.contains(&group_id)); + + // The silo's policy can no longer be fetched through the API, so check + // the database directly. + let silo_assignments = datastore + .role_assignment_fetch_visible(&opctx, &authz_silo) + .await + .expect("role_assignment_fetch_visible"); + assert!( + silo_assignments.is_empty(), + "unexpected role assignments: {silo_assignments:?}" + ); +} + // Test ensuring the same group from different users #[nexus_test] async fn test_ensure_same_silo_group(cptestctx: &ControlPlaneTestContext) { From 43386520cf825528913ef266e28823409c4db99a Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Thu, 5 Nov 2026 15:08:46 -0800 Subject: [PATCH 02/10] tweak name --- nexus/db-queries/src/db/datastore/role.rs | 2 +- nexus/db-queries/src/db/datastore/silo.rs | 4 ++-- nexus/db-queries/src/db/datastore/silo_group.rs | 2 +- nexus/db-queries/src/db/datastore/silo_user.rs | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/role.rs b/nexus/db-queries/src/db/datastore/role.rs index 2bd2496dc7b..266b662117b 100644 --- a/nexus/db-queries/src/db/datastore/role.rs +++ b/nexus/db-queries/src/db/datastore/role.rs @@ -114,7 +114,7 @@ impl DataStore { /// callers outside one should map it with `public_error_from_diesel`. /// /// Returns the number of rows deleted. - pub(super) async fn role_assignments_delete_for_identities( + pub(super) async fn role_assignment_delete_by_identity( &self, conn: &async_bb8_diesel::Connection, identity_type: IdentityType, diff --git a/nexus/db-queries/src/db/datastore/silo.rs b/nexus/db-queries/src/db/datastore/silo.rs index 21ce081422f..cd616feede0 100644 --- a/nexus/db-queries/src/db/datastore/silo.rs +++ b/nexus/db-queries/src/db/datastore/silo.rs @@ -586,7 +586,7 @@ impl DataStore { // delete role assignments held by the silo's users let updated_rows = self - .role_assignments_delete_for_identities( + .role_assignment_delete_by_identity( &conn, db::model::IdentityType::SiloUser, &deleted_user_ids, @@ -655,7 +655,7 @@ impl DataStore { ); let updated_rows = self - .role_assignments_delete_for_identities( + .role_assignment_delete_by_identity( &conn, db::model::IdentityType::SiloGroup, &deleted_group_ids, diff --git a/nexus/db-queries/src/db/datastore/silo_group.rs b/nexus/db-queries/src/db/datastore/silo_group.rs index 07450388b17..6c23eab1987 100644 --- a/nexus/db-queries/src/db/datastore/silo_group.rs +++ b/nexus/db-queries/src/db/datastore/silo_group.rs @@ -732,7 +732,7 @@ impl DataStore { } // delete group's role assignments - self.role_assignments_delete_for_identities( + self.role_assignment_delete_by_identity( &conn, model::IdentityType::SiloGroup, &[group_id.into_untyped_uuid()], diff --git a/nexus/db-queries/src/db/datastore/silo_user.rs b/nexus/db-queries/src/db/datastore/silo_user.rs index af49b49fc79..37fdb89d4e9 100644 --- a/nexus/db-queries/src/db/datastore/silo_user.rs +++ b/nexus/db-queries/src/db/datastore/silo_user.rs @@ -551,7 +551,7 @@ impl DataStore { } // delete the user's role assignments - self.role_assignments_delete_for_identities( + self.role_assignment_delete_by_identity( &conn, model::IdentityType::SiloUser, &[authz_silo_user_id.into_untyped_uuid()], From 536003cd623212fc784132041d50d5feb82196c1 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Thu, 5 Nov 2026 15:56:18 -0800 Subject: [PATCH 03/10] Use subquery to delete role assignments --- nexus/db-queries/src/db/datastore/role.rs | 21 ++-- nexus/db-queries/src/db/datastore/silo.rs | 109 +++++++++++------- .../db-queries/src/db/datastore/silo_group.rs | 2 +- .../db-queries/src/db/datastore/silo_user.rs | 2 +- nexus/db-schema/src/schema.rs | 2 + 5 files changed, 81 insertions(+), 55 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/role.rs b/nexus/db-queries/src/db/datastore/role.rs index 266b662117b..e97ab870f0c 100644 --- a/nexus/db-queries/src/db/datastore/role.rs +++ b/nexus/db-queries/src/db/datastore/role.rs @@ -5,7 +5,6 @@ //! [`DataStore`] methods related to roles. use super::DataStore; -use super::SQL_BATCH_SIZE; use crate::authz; use crate::authz::AuthorizedResource; use crate::context::OpContext; @@ -106,8 +105,8 @@ impl DataStore { .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) } - /// Delete all role assignments held by the given identities, on any - /// resource. Used when deleting users, groups, and silos so that deleted + /// Delete all role assignments held by the given identity, on any + /// resource. Used when deleting users and groups so that deleted /// identities don't linger in policies. /// /// Returns the raw diesel error so this can run inside a transaction; @@ -118,19 +117,15 @@ impl DataStore { &self, conn: &async_bb8_diesel::Connection, identity_type: IdentityType, - identity_ids: &[Uuid], + identity_id: Uuid, ) -> Result { use nexus_db_schema::schema::role_assignment::dsl; - let mut deleted = 0; - for batch in identity_ids.chunks(SQL_BATCH_SIZE.get() as usize) { - deleted += diesel::delete(dsl::role_assignment) - .filter(dsl::identity_type.eq(identity_type.clone())) - .filter(dsl::identity_id.eq_any(batch.to_vec())) - .execute_async(conn) - .await?; - } - Ok(deleted) + diesel::delete(dsl::role_assignment) + .filter(dsl::identity_type.eq(identity_type)) + .filter(dsl::identity_id.eq(identity_id)) + .execute_async(conn) + .await } /// Removes all existing externally-visble role assignments on diff --git a/nexus/db-queries/src/db/datastore/silo.rs b/nexus/db-queries/src/db/datastore/silo.rs index cd616feede0..f33ec2a2dd7 100644 --- a/nexus/db-queries/src/db/datastore/silo.rs +++ b/nexus/db-queries/src/db/datastore/silo.rs @@ -452,6 +452,7 @@ impl DataStore { opctx.authorize(authz::Action::Delete, authz_silo).await?; use nexus_db_schema::schema::project; + use nexus_db_schema::schema::role_assignment; use nexus_db_schema::schema::silo; use nexus_db_schema::schema::silo_group; use nexus_db_schema::schema::silo_group_membership; @@ -561,17 +562,31 @@ impl DataStore { "deleted {} password hashes for silo {}", updated_rows, id ); - let deleted_user_ids: Vec = - diesel::update(silo_user::dsl::silo_user) - .filter(silo_user::dsl::silo_id.eq(to_db_typed_uuid(id))) - .filter(silo_user::dsl::time_deleted.is_null()) + // delete role assignments held by the silo's users (on any resource, + // including the fleet, which outlives the silo). This must run before + // the users are marked deleted, since the subquery filters on that. + let updated_rows = + diesel::delete(role_assignment::dsl::role_assignment) + .filter( + role_assignment::dsl::identity_type + .eq(db::model::IdentityType::SiloUser), + ) .filter( - silo_user::dsl::user_provision_type - .eq(db_silo.user_provision_type), + role_assignment::dsl::identity_id.eq_any( + silo_user::dsl::silo_user + .filter( + silo_user::dsl::silo_id + .eq(to_db_typed_uuid(id)), + ) + .filter(silo_user::dsl::time_deleted.is_null()) + .filter( + silo_user::dsl::user_provision_type + .eq(db_silo.user_provision_type), + ) + .select(silo_user::dsl::id), + ), ) - .set(silo_user::dsl::time_deleted.eq(now)) - .returning(silo_user::dsl::id) - .get_results_async(&*conn) + .execute_async(&*conn) .await .map_err(|e| { public_error_from_diesel(e, ErrorHandler::Server) @@ -579,26 +594,26 @@ impl DataStore { debug!( opctx.log, - "deleted {} silo users for silo {}", - deleted_user_ids.len(), + "deleted {} silo user role assignments for silo {}", + updated_rows, id ); - // delete role assignments held by the silo's users - let updated_rows = self - .role_assignment_delete_by_identity( - &conn, - db::model::IdentityType::SiloUser, - &deleted_user_ids, + let updated_rows = diesel::update(silo_user::dsl::silo_user) + .filter(silo_user::dsl::silo_id.eq(to_db_typed_uuid(id))) + .filter(silo_user::dsl::time_deleted.is_null()) + .filter( + silo_user::dsl::user_provision_type + .eq(db_silo.user_provision_type), ) + .set(silo_user::dsl::time_deleted.eq(now)) + .execute_async(&*conn) .await .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; debug!( opctx.log, - "deleted {} silo user role assignments for silo {}", - updated_rows, - id + "deleted {} silo users for silo {}", updated_rows, id ); // delete all silo group memberships @@ -630,18 +645,30 @@ impl DataStore { "deleted {} silo group memberships for silo {}", updated_rows, id ); - // delete all silo groups - let deleted_group_ids: Vec = - diesel::update(silo_group::dsl::silo_group) - .filter(silo_group::dsl::silo_id.eq(to_db_typed_uuid(id))) - .filter(silo_group::dsl::time_deleted.is_null()) + // delete role assignments held by the silo's groups, before the groups + // are marked deleted (same reasoning as for users above) + let updated_rows = + diesel::delete(role_assignment::dsl::role_assignment) .filter( - silo_group::dsl::user_provision_type - .eq(db_silo.user_provision_type), + role_assignment::dsl::identity_type + .eq(db::model::IdentityType::SiloGroup), ) - .set(silo_group::dsl::time_deleted.eq(now)) - .returning(silo_group::dsl::id) - .get_results_async(&*conn) + .filter( + role_assignment::dsl::identity_id.eq_any( + silo_group::dsl::silo_group + .filter( + silo_group::dsl::silo_id + .eq(to_db_typed_uuid(id)), + ) + .filter(silo_group::dsl::time_deleted.is_null()) + .filter( + silo_group::dsl::user_provision_type + .eq(db_silo.user_provision_type), + ) + .select(silo_group::dsl::id), + ), + ) + .execute_async(&*conn) .await .map_err(|e| { public_error_from_diesel(e, ErrorHandler::Server) @@ -649,25 +676,27 @@ impl DataStore { debug!( opctx.log, - "deleted {} silo groups for silo {}", - deleted_group_ids.len(), + "deleted {} silo group role assignments for silo {}", + updated_rows, id ); - let updated_rows = self - .role_assignment_delete_by_identity( - &conn, - db::model::IdentityType::SiloGroup, - &deleted_group_ids, + // delete all silo groups + let updated_rows = diesel::update(silo_group::dsl::silo_group) + .filter(silo_group::dsl::silo_id.eq(to_db_typed_uuid(id))) + .filter(silo_group::dsl::time_deleted.is_null()) + .filter( + silo_group::dsl::user_provision_type + .eq(db_silo.user_provision_type), ) + .set(silo_group::dsl::time_deleted.eq(now)) + .execute_async(&*conn) .await .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; debug!( opctx.log, - "deleted {} silo group role assignments for silo {}", - updated_rows, - id + "deleted {} silo groups for silo {}", updated_rows, id ); // delete all silo identity providers diff --git a/nexus/db-queries/src/db/datastore/silo_group.rs b/nexus/db-queries/src/db/datastore/silo_group.rs index 6c23eab1987..f9a600c0675 100644 --- a/nexus/db-queries/src/db/datastore/silo_group.rs +++ b/nexus/db-queries/src/db/datastore/silo_group.rs @@ -735,7 +735,7 @@ impl DataStore { self.role_assignment_delete_by_identity( &conn, model::IdentityType::SiloGroup, - &[group_id.into_untyped_uuid()], + group_id.into_untyped_uuid(), ) .await?; diff --git a/nexus/db-queries/src/db/datastore/silo_user.rs b/nexus/db-queries/src/db/datastore/silo_user.rs index 37fdb89d4e9..42463add2f1 100644 --- a/nexus/db-queries/src/db/datastore/silo_user.rs +++ b/nexus/db-queries/src/db/datastore/silo_user.rs @@ -554,7 +554,7 @@ impl DataStore { self.role_assignment_delete_by_identity( &conn, model::IdentityType::SiloUser, - &[authz_silo_user_id.into_untyped_uuid()], + authz_silo_user_id.into_untyped_uuid(), ) .await?; diff --git a/nexus/db-schema/src/schema.rs b/nexus/db-schema/src/schema.rs index d17b49a5d90..69438ceecd6 100644 --- a/nexus/db-schema/src/schema.rs +++ b/nexus/db-schema/src/schema.rs @@ -921,6 +921,8 @@ allow_tables_to_appear_in_same_query!( allow_tables_to_appear_in_same_query!(silo_group, silo); allow_tables_to_appear_in_same_query!(silo_user, silo); allow_tables_to_appear_in_same_query!(role_assignment, silo_group_membership); +allow_tables_to_appear_in_same_query!(role_assignment, silo_user); +allow_tables_to_appear_in_same_query!(role_assignment, silo_group); table! { identity_provider (silo_id, id) { From e241b4304be76144638db15265924bd8a7285e52 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Thu, 5 Nov 2026 16:42:19 -0800 Subject: [PATCH 04/10] Remove unused silo_group_delete method --- .../db-queries/src/db/datastore/silo_group.rs | 77 ------------------- 1 file changed, 77 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/silo_group.rs b/nexus/db-queries/src/db/datastore/silo_group.rs index f9a600c0675..56bbd87ebf9 100644 --- a/nexus/db-queries/src/db/datastore/silo_group.rs +++ b/nexus/db-queries/src/db/datastore/silo_group.rs @@ -20,13 +20,11 @@ use chrono::DateTime; use chrono::Utc; use diesel::prelude::*; use nexus_db_errors::ErrorHandler; -use nexus_db_errors::TransactionError; use nexus_db_errors::public_error_from_diesel; use nexus_types::external_api::user; use nexus_types::identity::Asset; use omicron_common::api::external::CreateResult; use omicron_common::api::external::DataPageParams; -use omicron_common::api::external::DeleteResult; use omicron_common::api::external::Error; use omicron_common::api::external::InternalContext; use omicron_common::api::external::ListResultVec; @@ -680,81 +678,6 @@ impl DataStore { .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) } - pub async fn silo_group_delete( - &self, - opctx: &OpContext, - authz_silo_group: &authz::SiloGroup, - ) -> DeleteResult { - opctx.authorize(authz::Action::Delete, authz_silo_group).await?; - - #[derive(Debug, thiserror::Error)] - enum SiloDeleteError { - #[error("group {0} still has memberships")] - GroupStillHasMemberships(SiloGroupUuid), - } - type TxnError = TransactionError; - - let group_id = authz_silo_group.id(); - let conn = self.pool_connection_authorized(opctx).await?; - - // Prefer to use "transaction_retry_wrapper" - self.transaction_non_retry_wrapper("silo_group_delete") - .transaction(&conn, |conn| async move { - use nexus_db_schema::schema::silo_group_membership; - - // Don't delete groups that still have memberships - let group_memberships = - silo_group_membership::dsl::silo_group_membership - .filter( - silo_group_membership::dsl::silo_group_id - .eq(to_db_typed_uuid(group_id)), - ) - .select(SiloGroupMembership::as_returning()) - .limit(1) - .load_async(&conn) - .await?; - - if !group_memberships.is_empty() { - return Err(TxnError::CustomError( - SiloDeleteError::GroupStillHasMemberships(group_id), - )); - } - - // Delete silo group - { - use nexus_db_schema::schema::silo_group::dsl; - diesel::update(dsl::silo_group) - .filter(dsl::id.eq(to_db_typed_uuid(group_id))) - .filter(dsl::time_deleted.is_null()) - .set(dsl::time_deleted.eq(Utc::now())) - .execute_async(&conn) - .await?; - } - - // delete group's role assignments - self.role_assignment_delete_by_identity( - &conn, - model::IdentityType::SiloGroup, - group_id.into_untyped_uuid(), - ) - .await?; - - Ok(()) - }) - .await - .map_err(|e| match e { - TxnError::CustomError( - SiloDeleteError::GroupStillHasMemberships(id), - ) => Error::invalid_request(&format!( - "group {0} still has memberships", - id - )), - TxnError::Database(error) => { - public_error_from_diesel(error, ErrorHandler::Server) - } - }) - } - pub async fn silo_groups_list_by_id( &self, opctx: &OpContext, From dac9c7f09ee908cf906cc58590cac4a72c7f7e86 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Fri, 6 Nov 2026 17:28:58 -0800 Subject: [PATCH 05/10] dedupe by using new helper in SCIM methods --- .../src/db/datastore/scim_provider_store.rs | 32 ++++++++----------- nexus/db-queries/src/db/datastore/silo.rs | 4 +-- 2 files changed, 15 insertions(+), 21 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/scim_provider_store.rs b/nexus/db-queries/src/db/datastore/scim_provider_store.rs index 38c3440a88a..e856b843bc2 100644 --- a/nexus/db-queries/src/db/datastore/scim_provider_store.rs +++ b/nexus/db-queries/src/db/datastore/scim_provider_store.rs @@ -507,15 +507,13 @@ impl<'a> CrdbScimProviderStore<'a> { } // Cleanup role assignment records for the deleted user - { - use nexus_db_schema::schema::role_assignment::dsl; - - diesel::delete(dsl::role_assignment) - .filter(dsl::identity_type.eq(IdentityType::SiloUser)) - .filter(dsl::identity_id.eq(to_db_typed_uuid(user_id))) - .execute_async(conn) - .await?; - } + self.datastore + .role_assignment_delete_by_identity( + conn, + IdentityType::SiloUser, + user_id.into_untyped_uuid(), + ) + .await?; // Finally we should cleanup all auth related things like sessions and // tokens. @@ -1217,15 +1215,13 @@ impl<'a> CrdbScimProviderStore<'a> { } // Cleanup role assignment records for the deleted group - { - use nexus_db_schema::schema::role_assignment::dsl; - - diesel::delete(dsl::role_assignment) - .filter(dsl::identity_id.eq(to_db_typed_uuid(group_id))) - .filter(dsl::identity_type.eq(IdentityType::SiloGroup)) - .execute_async(conn) - .await?; - } + self.datastore + .role_assignment_delete_by_identity( + conn, + IdentityType::SiloGroup, + group_id.into_untyped_uuid(), + ) + .await?; Ok(true) } diff --git a/nexus/db-queries/src/db/datastore/silo.rs b/nexus/db-queries/src/db/datastore/silo.rs index f33ec2a2dd7..357a6c99efd 100644 --- a/nexus/db-queries/src/db/datastore/silo.rs +++ b/nexus/db-queries/src/db/datastore/silo.rs @@ -562,9 +562,7 @@ impl DataStore { "deleted {} password hashes for silo {}", updated_rows, id ); - // delete role assignments held by the silo's users (on any resource, - // including the fleet, which outlives the silo). This must run before - // the users are marked deleted, since the subquery filters on that. + // delete role assignments held by the silo's users let updated_rows = diesel::delete(role_assignment::dsl::role_assignment) .filter( From a78602ec566ca90ecae058d43985121117def2a7 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Fri, 6 Nov 2026 17:41:56 -0800 Subject: [PATCH 06/10] cleanup --- nexus/db-queries/src/db/datastore/role.rs | 8 ++++---- nexus/db-queries/src/db/datastore/silo.rs | 3 +-- nexus/tests/integration_tests/silos.rs | 4 +++- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/role.rs b/nexus/db-queries/src/db/datastore/role.rs index e97ab870f0c..cc4a1f9c9ed 100644 --- a/nexus/db-queries/src/db/datastore/role.rs +++ b/nexus/db-queries/src/db/datastore/role.rs @@ -111,21 +111,21 @@ impl DataStore { /// /// Returns the raw diesel error so this can run inside a transaction; /// callers outside one should map it with `public_error_from_diesel`. - /// - /// Returns the number of rows deleted. pub(super) async fn role_assignment_delete_by_identity( &self, conn: &async_bb8_diesel::Connection, identity_type: IdentityType, identity_id: Uuid, - ) -> Result { + ) -> Result<(), DieselError> { use nexus_db_schema::schema::role_assignment::dsl; diesel::delete(dsl::role_assignment) .filter(dsl::identity_type.eq(identity_type)) .filter(dsl::identity_id.eq(identity_id)) .execute_async(conn) - .await + .await?; + + Ok(()) } /// Removes all existing externally-visble role assignments on diff --git a/nexus/db-queries/src/db/datastore/silo.rs b/nexus/db-queries/src/db/datastore/silo.rs index 357a6c99efd..3b4b71de2c8 100644 --- a/nexus/db-queries/src/db/datastore/silo.rs +++ b/nexus/db-queries/src/db/datastore/silo.rs @@ -643,8 +643,7 @@ impl DataStore { "deleted {} silo group memberships for silo {}", updated_rows, id ); - // delete role assignments held by the silo's groups, before the groups - // are marked deleted (same reasoning as for users above) + // delete role assignments held by the silo's groups let updated_rows = diesel::delete(role_assignment::dsl::role_assignment) .filter( diff --git a/nexus/tests/integration_tests/silos.rs b/nexus/tests/integration_tests/silos.rs index 3890010a360..a4416f665c2 100644 --- a/nexus/tests/integration_tests/silos.rs +++ b/nexus/tests/integration_tests/silos.rs @@ -1740,7 +1740,9 @@ async fn test_silo_delete_clean_up_role_assignments( assert!(!fleet_ids.contains(&group_id)); // The silo's policy can no longer be fetched through the API, so check - // the database directly. + // the database directly. This is empty only because every assignment on + // the silo was held by one of its own users or groups: silo deletion does + // not remove assignments on the silo held by identities from other silos. let silo_assignments = datastore .role_assignment_fetch_visible(&opctx, &authz_silo) .await From 8566efcc50dbc1389cff94436637521c94d4a129 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Mon, 9 Nov 2026 17:19:42 -0800 Subject: [PATCH 07/10] updated helper name; better handle silo deletion --- nexus/db-queries/src/db/datastore/role.rs | 2 +- .../src/db/datastore/scim_provider_store.rs | 4 +-- nexus/db-queries/src/db/datastore/silo.rs | 24 ++++++++++++++++++ .../db-queries/src/db/datastore/silo_user.rs | 2 +- nexus/tests/integration_tests/silos.rs | 25 ++++++++++++++++--- 5 files changed, 50 insertions(+), 7 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/role.rs b/nexus/db-queries/src/db/datastore/role.rs index cc4a1f9c9ed..68c7e714c5d 100644 --- a/nexus/db-queries/src/db/datastore/role.rs +++ b/nexus/db-queries/src/db/datastore/role.rs @@ -111,7 +111,7 @@ impl DataStore { /// /// Returns the raw diesel error so this can run inside a transaction; /// callers outside one should map it with `public_error_from_diesel`. - pub(super) async fn role_assignment_delete_by_identity( + pub(super) async fn role_assignment_delete_by_identity_on_connection( &self, conn: &async_bb8_diesel::Connection, identity_type: IdentityType, diff --git a/nexus/db-queries/src/db/datastore/scim_provider_store.rs b/nexus/db-queries/src/db/datastore/scim_provider_store.rs index e856b843bc2..30dbcfb5e7f 100644 --- a/nexus/db-queries/src/db/datastore/scim_provider_store.rs +++ b/nexus/db-queries/src/db/datastore/scim_provider_store.rs @@ -508,7 +508,7 @@ impl<'a> CrdbScimProviderStore<'a> { // Cleanup role assignment records for the deleted user self.datastore - .role_assignment_delete_by_identity( + .role_assignment_delete_by_identity_on_connection( conn, IdentityType::SiloUser, user_id.into_untyped_uuid(), @@ -1216,7 +1216,7 @@ impl<'a> CrdbScimProviderStore<'a> { // Cleanup role assignment records for the deleted group self.datastore - .role_assignment_delete_by_identity( + .role_assignment_delete_by_identity_on_connection( conn, IdentityType::SiloGroup, group_id.into_untyped_uuid(), diff --git a/nexus/db-queries/src/db/datastore/silo.rs b/nexus/db-queries/src/db/datastore/silo.rs index 3b4b71de2c8..ceaad14d7b0 100644 --- a/nexus/db-queries/src/db/datastore/silo.rs +++ b/nexus/db-queries/src/db/datastore/silo.rs @@ -678,6 +678,30 @@ impl DataStore { id ); + // delete role assignments on the silo itself. The subqueries above + // only cover identities that belong to this silo; users from other + // silos (typically fleet admins) can hold roles on it too. + let updated_rows = + diesel::delete(role_assignment::dsl::role_assignment) + .filter( + role_assignment::dsl::resource_type + .eq(ResourceType::Silo.to_string()), + ) + .filter( + role_assignment::dsl::resource_id + .eq(id.into_untyped_uuid()), + ) + .execute_async(&*conn) + .await + .map_err(|e| { + public_error_from_diesel(e, ErrorHandler::Server) + })?; + + debug!( + opctx.log, + "deleted {} role assignments on silo {}", updated_rows, id + ); + // delete all silo groups let updated_rows = diesel::update(silo_group::dsl::silo_group) .filter(silo_group::dsl::silo_id.eq(to_db_typed_uuid(id))) diff --git a/nexus/db-queries/src/db/datastore/silo_user.rs b/nexus/db-queries/src/db/datastore/silo_user.rs index 42463add2f1..061a98f480f 100644 --- a/nexus/db-queries/src/db/datastore/silo_user.rs +++ b/nexus/db-queries/src/db/datastore/silo_user.rs @@ -551,7 +551,7 @@ impl DataStore { } // delete the user's role assignments - self.role_assignment_delete_by_identity( + self.role_assignment_delete_by_identity_on_connection( &conn, model::IdentityType::SiloUser, authz_silo_user_id.into_untyped_uuid(), diff --git a/nexus/tests/integration_tests/silos.rs b/nexus/tests/integration_tests/silos.rs index a4416f665c2..9e7e8008dd0 100644 --- a/nexus/tests/integration_tests/silos.rs +++ b/nexus/tests/integration_tests/silos.rs @@ -1718,6 +1718,26 @@ async fn test_silo_delete_clean_up_role_assignments( AuthnMode::PrivilegedUser, ) .await; + // Also give a user from a different silo a role on this silo. Deleting + // the silo should remove this too, even though the user survives. + grant_iam( + client, + silo_url, + SiloRole::Admin, + USER_TEST_PRIVILEGED.id(), + AuthnMode::PrivilegedUser, + ) + .await; + + let silo_ids = policy_identity_ids::( + client, + &format!("{silo_url}/policy"), + AuthnMode::PrivilegedUser, + ) + .await; + assert!(silo_ids.contains(&user_id)); + assert!(silo_ids.contains(&group_id)); + assert!(silo_ids.contains(&USER_TEST_PRIVILEGED.id().into_untyped_uuid())); let fleet_ids = policy_identity_ids::( client, @@ -1740,9 +1760,8 @@ async fn test_silo_delete_clean_up_role_assignments( assert!(!fleet_ids.contains(&group_id)); // The silo's policy can no longer be fetched through the API, so check - // the database directly. This is empty only because every assignment on - // the silo was held by one of its own users or groups: silo deletion does - // not remove assignments on the silo held by identities from other silos. + // the database directly. Every assignment on the silo should be gone, + // including the one held by the user from the other silo. let silo_assignments = datastore .role_assignment_fetch_visible(&opctx, &authz_silo) .await From d16c3702b671b5fbf1b7bb5ecbebb3455fbbafa9 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Tue, 10 Nov 2026 12:18:45 -0800 Subject: [PATCH 08/10] delete project role rows on project deletion --- nexus/db-queries/src/db/datastore/project.rs | 6 +++ nexus/db-queries/src/db/datastore/role.rs | 26 +++++++++ nexus/db-queries/src/db/datastore/silo.rs | 19 ++----- nexus/tests/integration_tests/projects.rs | 57 ++++++++++++++++++++ 4 files changed, 93 insertions(+), 15 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/project.rs b/nexus/db-queries/src/db/datastore/project.rs index 3ce905ab062..c8ae91eece4 100644 --- a/nexus/db-queries/src/db/datastore/project.rs +++ b/nexus/db-queries/src/db/datastore/project.rs @@ -296,6 +296,12 @@ impl DataStore { db_project.id().into_untyped_uuid(), ) .await?; + + self.role_assignment_delete_by_resource_on_connection( + &conn, + authz_project, + ) + .await?; Ok(()) } }) diff --git a/nexus/db-queries/src/db/datastore/role.rs b/nexus/db-queries/src/db/datastore/role.rs index 68c7e714c5d..116964c9a8a 100644 --- a/nexus/db-queries/src/db/datastore/role.rs +++ b/nexus/db-queries/src/db/datastore/role.rs @@ -128,6 +128,32 @@ impl DataStore { Ok(()) } + /// Delete all role assignments on the given resource, held by any + /// identity. Used when deleting resources that have policies (silos, + /// projects) so their assignments don't outlive them. + /// + /// Returns the number of deleted rows. Returns the raw diesel error so + /// this can run inside a transaction; callers outside one should map it + /// with `public_error_from_diesel`. + pub(super) async fn role_assignment_delete_by_resource_on_connection< + T: authz::ApiResourceWithRoles, + >( + &self, + conn: &async_bb8_diesel::Connection, + authz_resource: &T, + ) -> Result { + use nexus_db_schema::schema::role_assignment::dsl; + + diesel::delete(dsl::role_assignment) + .filter( + dsl::resource_type + .eq(authz_resource.resource_type().to_string()), + ) + .filter(dsl::resource_id.eq(authz_resource.resource_id())) + .execute_async(conn) + .await + } + /// Removes all existing externally-visble role assignments on /// `authz_resource` and adds those specified by `new_assignments` /// diff --git a/nexus/db-queries/src/db/datastore/silo.rs b/nexus/db-queries/src/db/datastore/silo.rs index ceaad14d7b0..afe78b97760 100644 --- a/nexus/db-queries/src/db/datastore/silo.rs +++ b/nexus/db-queries/src/db/datastore/silo.rs @@ -681,21 +681,10 @@ impl DataStore { // delete role assignments on the silo itself. The subqueries above // only cover identities that belong to this silo; users from other // silos (typically fleet admins) can hold roles on it too. - let updated_rows = - diesel::delete(role_assignment::dsl::role_assignment) - .filter( - role_assignment::dsl::resource_type - .eq(ResourceType::Silo.to_string()), - ) - .filter( - role_assignment::dsl::resource_id - .eq(id.into_untyped_uuid()), - ) - .execute_async(&*conn) - .await - .map_err(|e| { - public_error_from_diesel(e, ErrorHandler::Server) - })?; + let updated_rows = self + .role_assignment_delete_by_resource_on_connection(&conn, authz_silo) + .await + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; debug!( opctx.log, diff --git a/nexus/tests/integration_tests/projects.rs b/nexus/tests/integration_tests/projects.rs index 30877f80041..bba3d807cd6 100644 --- a/nexus/tests/integration_tests/projects.rs +++ b/nexus/tests/integration_tests/projects.rs @@ -6,6 +6,11 @@ use dropshot::HttpErrorResponseBody; use dropshot::test_util::ClientTestContext; use http::StatusCode; use http::method::Method; +use nexus_db_lookup::LookupPath; +use nexus_db_queries::authn::USER_TEST_UNPRIVILEGED; +use nexus_db_queries::authz; +use nexus_db_queries::context::OpContext; +use nexus_db_queries::db::identity::Asset; use nexus_test_utils::http_testing::AuthnMode; use nexus_test_utils::http_testing::NexusRequest; use nexus_test_utils::http_testing::RequestBuilder; @@ -31,6 +36,7 @@ use nexus_types::external_api::floating_ip; use nexus_types::external_api::instance; use nexus_types::external_api::instance::InstanceCpuCount; use nexus_types::external_api::ip_pool; +use nexus_types::external_api::policy::ProjectRole; use nexus_types::external_api::policy::SiloRole; use nexus_types::external_api::project; use nexus_types::external_api::project::Project; @@ -284,6 +290,57 @@ async fn test_project_deletion(cptestctx: &ControlPlaneTestContext) { delete_project(&url, &client).await; } +#[nexus_test] +async fn test_project_deletion_clean_up_role_assignments( + cptestctx: &ControlPlaneTestContext, +) { + let client = &cptestctx.external_client; + let nexus = &cptestctx.server.server_context().nexus; + let datastore = nexus.datastore(); + let opctx = + OpContext::for_tests(cptestctx.logctx.log.clone(), datastore.clone()); + + let name = "role-project"; + let url = format!("/v1/projects/{}", name); + create_project(&client, &name).await; + grant_iam( + client, + &url, + ProjectRole::Viewer, + USER_TEST_UNPRIVILEGED.id(), + AuthnMode::PrivilegedUser, + ) + .await; + + // Look up the project before deleting it, because the lookup fails once + // the project is gone. + let (.., authz_project) = LookupPath::new(&opctx, datastore) + .project_name(&Name::from_str(name).unwrap().into()) + .lookup_for(authz::Action::Read) + .await + .expect("project lookup"); + let assignments = datastore + .role_assignment_fetch_visible(&opctx, &authz_project) + .await + .expect("role_assignment_fetch_visible"); + assert!(!assignments.is_empty()); + + delete_project_default_subnet(&name, &client).await; + delete_project_default_vpc(&name, &client).await; + delete_project(&url, &client).await; + + // The project's policy can no longer be fetched through the API, so + // check the database directly. + let assignments = datastore + .role_assignment_fetch_visible(&opctx, &authz_project) + .await + .expect("role_assignment_fetch_visible"); + assert!( + assignments.is_empty(), + "unexpected role assignments: {assignments:?}" + ); +} + #[nexus_test] async fn test_project_deletion_with_instance( cptestctx: &ControlPlaneTestContext, From d78d6ffb686b60fc2124f18905f04192914fe2b2 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Tue, 10 Nov 2026 14:26:50 -0800 Subject: [PATCH 09/10] delete silo role assignments within the silo deletion flow --- nexus/db-queries/src/db/datastore/role.rs | 13 +++++++------ nexus/db-queries/src/db/datastore/silo.rs | 23 ++++++++++------------- nexus/tests/integration_tests/projects.rs | 7 ++++++- 3 files changed, 23 insertions(+), 20 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/role.rs b/nexus/db-queries/src/db/datastore/role.rs index 116964c9a8a..7d6334c42f9 100644 --- a/nexus/db-queries/src/db/datastore/role.rs +++ b/nexus/db-queries/src/db/datastore/role.rs @@ -132,16 +132,15 @@ impl DataStore { /// identity. Used when deleting resources that have policies (silos, /// projects) so their assignments don't outlive them. /// - /// Returns the number of deleted rows. Returns the raw diesel error so - /// this can run inside a transaction; callers outside one should map it - /// with `public_error_from_diesel`. + /// Returns the raw diesel error so this can run inside a transaction; + /// callers outside one should map it with `public_error_from_diesel`. pub(super) async fn role_assignment_delete_by_resource_on_connection< T: authz::ApiResourceWithRoles, >( &self, conn: &async_bb8_diesel::Connection, authz_resource: &T, - ) -> Result { + ) -> Result<(), DieselError> { use nexus_db_schema::schema::role_assignment::dsl; diesel::delete(dsl::role_assignment) @@ -151,10 +150,12 @@ impl DataStore { ) .filter(dsl::resource_id.eq(authz_resource.resource_id())) .execute_async(conn) - .await + .await?; + + Ok(()) } - /// Removes all existing externally-visble role assignments on + /// Removes all existing externally-visible role assignments on /// `authz_resource` and adds those specified by `new_assignments` /// /// Role assignments for internal identities (e.g., built-in users) are not diff --git a/nexus/db-queries/src/db/datastore/silo.rs b/nexus/db-queries/src/db/datastore/silo.rs index afe78b97760..0f4aad8c2d2 100644 --- a/nexus/db-queries/src/db/datastore/silo.rs +++ b/nexus/db-queries/src/db/datastore/silo.rs @@ -512,6 +512,16 @@ impl DataStore { self.silo_auth_settings_delete(opctx, &conn, &authz_silo) .await?; + // Delete all role assignments on the silo, including those + // held by users from other silos (typically fleet admins). + // The count is bounded by MAX_ROLE_ASSIGNMENTS_PER_RESOURCE, + // so unlike the per-identity cleanup below, this can run in + // the transaction. + self.role_assignment_delete_by_resource_on_connection( + &conn, authz_silo, + ) + .await?; + self.virtual_provisioning_collection_delete_on_connection( &opctx.log, &conn, @@ -678,19 +688,6 @@ impl DataStore { id ); - // delete role assignments on the silo itself. The subqueries above - // only cover identities that belong to this silo; users from other - // silos (typically fleet admins) can hold roles on it too. - let updated_rows = self - .role_assignment_delete_by_resource_on_connection(&conn, authz_silo) - .await - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; - - debug!( - opctx.log, - "deleted {} role assignments on silo {}", updated_rows, id - ); - // delete all silo groups let updated_rows = diesel::update(silo_group::dsl::silo_group) .filter(silo_group::dsl::silo_id.eq(to_db_typed_uuid(id))) diff --git a/nexus/tests/integration_tests/projects.rs b/nexus/tests/integration_tests/projects.rs index bba3d807cd6..b4e5d673819 100644 --- a/nexus/tests/integration_tests/projects.rs +++ b/nexus/tests/integration_tests/projects.rs @@ -48,6 +48,7 @@ use nexus_types_versions::latest::instance::Instance; use omicron_common::api::external::ByteCount; use omicron_common::api::external::IdentityMetadataCreateParams; use omicron_common::api::external::Name; +use omicron_uuid_kinds::GenericUuid; use std::str::FromStr; type ControlPlaneTestContext = @@ -323,7 +324,11 @@ async fn test_project_deletion_clean_up_role_assignments( .role_assignment_fetch_visible(&opctx, &authz_project) .await .expect("role_assignment_fetch_visible"); - assert!(!assignments.is_empty()); + let user_id = USER_TEST_UNPRIVILEGED.id().into_untyped_uuid(); + assert!( + assignments.iter().any(|a| a.identity_id == user_id), + "missing role assignment for {user_id}: {assignments:?}" + ); delete_project_default_subnet(&name, &client).await; delete_project_default_vpc(&name, &client).await; From 45f82a265d36045ded38a8d4fa1d2723aea81e0d Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Wed, 11 Nov 2026 10:38:44 -0800 Subject: [PATCH 10/10] comment cleanup --- nexus/db-queries/src/db/datastore/role.rs | 6 ------ nexus/db-queries/src/db/datastore/silo.rs | 4 +--- nexus/db-queries/src/db/datastore/silo_user.rs | 2 +- 3 files changed, 2 insertions(+), 10 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/role.rs b/nexus/db-queries/src/db/datastore/role.rs index 7d6334c42f9..571c50b7edf 100644 --- a/nexus/db-queries/src/db/datastore/role.rs +++ b/nexus/db-queries/src/db/datastore/role.rs @@ -108,9 +108,6 @@ impl DataStore { /// Delete all role assignments held by the given identity, on any /// resource. Used when deleting users and groups so that deleted /// identities don't linger in policies. - /// - /// Returns the raw diesel error so this can run inside a transaction; - /// callers outside one should map it with `public_error_from_diesel`. pub(super) async fn role_assignment_delete_by_identity_on_connection( &self, conn: &async_bb8_diesel::Connection, @@ -131,9 +128,6 @@ impl DataStore { /// Delete all role assignments on the given resource, held by any /// identity. Used when deleting resources that have policies (silos, /// projects) so their assignments don't outlive them. - /// - /// Returns the raw diesel error so this can run inside a transaction; - /// callers outside one should map it with `public_error_from_diesel`. pub(super) async fn role_assignment_delete_by_resource_on_connection< T: authz::ApiResourceWithRoles, >( diff --git a/nexus/db-queries/src/db/datastore/silo.rs b/nexus/db-queries/src/db/datastore/silo.rs index 0f4aad8c2d2..44b107916c9 100644 --- a/nexus/db-queries/src/db/datastore/silo.rs +++ b/nexus/db-queries/src/db/datastore/silo.rs @@ -514,9 +514,7 @@ impl DataStore { // Delete all role assignments on the silo, including those // held by users from other silos (typically fleet admins). - // The count is bounded by MAX_ROLE_ASSIGNMENTS_PER_RESOURCE, - // so unlike the per-identity cleanup below, this can run in - // the transaction. + // The count is bounded by MAX_ROLE_ASSIGNMENTS_PER_RESOURCE. self.role_assignment_delete_by_resource_on_connection( &conn, authz_silo, ) diff --git a/nexus/db-queries/src/db/datastore/silo_user.rs b/nexus/db-queries/src/db/datastore/silo_user.rs index 061a98f480f..72a80e9c4d4 100644 --- a/nexus/db-queries/src/db/datastore/silo_user.rs +++ b/nexus/db-queries/src/db/datastore/silo_user.rs @@ -550,7 +550,7 @@ impl DataStore { .await?; } - // delete the user's role assignments + // Delete role assignments. self.role_assignment_delete_by_identity_on_connection( &conn, model::IdentityType::SiloUser,