diff --git a/nexus/db-queries/src/db/datastore/role.rs b/nexus/db-queries/src/db/datastore/role.rs index 18e9f5910dc..cc4a1f9c9ed 100644 --- a/nexus/db-queries/src/db/datastore/role.rs +++ b/nexus/db-queries/src/db/datastore/role.rs @@ -16,6 +16,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 +26,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 +105,29 @@ impl DataStore { .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) } + /// 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( + &self, + conn: &async_bb8_diesel::Connection, + identity_type: IdentityType, + identity_id: Uuid, + ) -> 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?; + + Ok(()) + } + /// 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/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 cae0cb53931..3b4b71de2c8 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,6 +562,41 @@ impl DataStore { "deleted {} password hashes for silo {}", updated_rows, id ); + // delete role assignments held by the silo's users + let updated_rows = + diesel::delete(role_assignment::dsl::role_assignment) + .filter( + role_assignment::dsl::identity_type + .eq(db::model::IdentityType::SiloUser), + ) + .filter( + 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), + ), + ) + .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 + ); + 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()) @@ -607,6 +643,41 @@ impl DataStore { "deleted {} silo group memberships for silo {}", updated_rows, id ); + // delete role assignments held by the silo's groups + let updated_rows = + diesel::delete(role_assignment::dsl::role_assignment) + .filter( + role_assignment::dsl::identity_type + .eq(db::model::IdentityType::SiloGroup), + ) + .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) + })?; + + debug!( + opctx.log, + "deleted {} silo group role assignments for 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_group.rs b/nexus/db-queries/src/db/datastore/silo_group.rs index 812b739a9a1..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,71 +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?; - - 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, diff --git a/nexus/db-queries/src/db/datastore/silo_user.rs b/nexus/db-queries/src/db/datastore/silo_user.rs index b6ba755b01a..42463add2f1 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_assignment_delete_by_identity( + &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/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) { diff --git a/nexus/tests/integration_tests/silos.rs b/nexus/tests/integration_tests/silos.rs index c7564ebc63e..a4416f665c2 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,285 @@ 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. 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 + .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) {