diff --git a/nexus/test-utils/src/resource_helpers.rs b/nexus/test-utils/src/resource_helpers.rs index ace0d2f265a..79fca34f574 100644 --- a/nexus/test-utils/src/resource_helpers.rs +++ b/nexus/test-utils/src/resource_helpers.rs @@ -602,6 +602,11 @@ pub async fn create_silo( .await } +pub async fn delete_silo(client: &ClientTestContext, silo_name_or_id: &str) { + let url = format!("/v1/system/silos/{silo_name_or_id}"); + object_delete(client, &url).await; +} + pub async fn create_silo_with_admin_group_name( client: &ClientTestContext, silo_name: &str, diff --git a/nexus/tests/integration_tests/password_login.rs b/nexus/tests/integration_tests/password_login.rs index 27c82c8aec5..c37f4fb9a10 100644 --- a/nexus/tests/integration_tests/password_login.rs +++ b/nexus/tests/integration_tests/password_login.rs @@ -7,7 +7,9 @@ use http::{StatusCode, header, method::Method}; use nexus_test_utils::http_testing::{AuthnMode, NexusRequest, RequestBuilder}; use nexus_test_utils::resource_helpers::grant_iam; use nexus_test_utils::resource_helpers::test_params; -use nexus_test_utils::resource_helpers::{create_local_user, create_silo}; +use nexus_test_utils::resource_helpers::{ + create_local_user, create_silo, delete_silo, +}; use nexus_test_utils_macros::nexus_test; use nexus_types::external_api::policy::SiloRole; use nexus_types::external_api::silo; @@ -20,9 +22,6 @@ use std::str::FromStr; type ControlPlaneTestContext = nexus_test_utils::ControlPlaneTestContext; -// TODO-coverage verify that deleting a Silo deletes all the users and their -// password hashes - // TODO-coverage A more rigorous test to verify there are no timing attack // vulnerabilities here might be to construct a few kinds of logins (attempt for // nonexistent user, attempt for user with no password set, successful attempt, @@ -44,14 +43,7 @@ async fn test_local_users(cptestctx: &ControlPlaneTestContext) { .await; test_local_user_basic(client, &silo).await; test_local_user_with_no_initial_password(client, &silo).await; - NexusRequest::object_delete( - client, - &format!("/v1/system/silos/{}", silo_name), - ) - .authn_as(AuthnMode::PrivilegedUser) - .execute() - .await - .unwrap(); + delete_silo(client, silo_name.as_str()).await; } async fn test_local_user_basic(client: &ClientTestContext, silo: &silo::Silo) { diff --git a/nexus/tests/integration_tests/silos.rs b/nexus/tests/integration_tests/silos.rs index b5f8de7cd39..9318f77cee1 100644 --- a/nexus/tests/integration_tests/silos.rs +++ b/nexus/tests/integration_tests/silos.rs @@ -19,7 +19,7 @@ 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, + create_subnet_pool, delete_silo, grant_iam, link_ip_pool, link_subnet_pool, object_create, object_delete, objects_list_page_authz, projects_list, test_params, }; @@ -265,11 +265,7 @@ async fn test_silos(cptestctx: &ControlPlaneTestContext) { .expect("failed to make request"); // Verify silo DELETE now works - NexusRequest::object_delete(&client, &discoverable_url) - .authn_as(AuthnMode::PrivilegedUser) - .execute() - .await - .expect("failed to make request"); + delete_silo(client, "discoverable").await; // Verify the DNS name was removed. verify_silo_dns_name(cptestctx, "discoverable", false).await; @@ -529,14 +525,7 @@ async fn test_deleting_a_silo_deletes_the_idp( .await; // Delete the silo - NexusRequest::object_delete( - &client, - &format!("/v1/system/silos/{}", SILO_NAME), - ) - .authn_as(AuthnMode::PrivilegedUser) - .execute() - .await - .expect("failed to make request"); + delete_silo(client, SILO_NAME).await; // Expect that the silo is gone let nexus = &cptestctx.server.server_context().nexus; @@ -869,11 +858,7 @@ async fn test_silo_user_provision_types(cptestctx: &ControlPlaneTestContext) { assert!(existing_silo_user.is_err()); } - NexusRequest::object_delete(&client, &"/v1/system/silos/test-silo") - .authn_as(AuthnMode::PrivilegedUser) - .execute() - .await - .expect("failed to make request"); + delete_silo(client, "test-silo").await; } } @@ -1418,6 +1403,126 @@ async fn test_silo_groups_remove_from_both_groups( assert!(group_names.contains(&"c-group".to_string())); } +// Test that silo delete cleans up associated users and password hashes +#[nexus_test] +async fn test_silo_delete_cleans_up_users_and_password_hashes( + cptestctx: &ControlPlaneTestContext, +) { + let client = &cptestctx.external_client; + let nexus = &cptestctx.server.server_context().nexus; + let opctx = nexus.opctx_external_authn(); + let datastore = nexus.datastore(); + + const SILO_NAME: &str = "test-silo"; + let silo = + create_silo(client, SILO_NAME, true, silo::SiloIdentityMode::LocalOnly) + .await; + let (authz_silo, _) = LookupPath::new(opctx, datastore) + .silo_id(silo.identity.id) + .fetch() + .await + .unwrap(); + + let mut silo_users = Vec::new(); + + // Create two users with passwords and one without. + for (username, password, expect_password_hash) in [ + ( + "homer-simpson", + test_params::UserPassword::Password( + "he card read good".to_string(), + ), + true, + ), + ( + "marge-simpson", + test_params::UserPassword::Password( + "i just think they're neat".to_string(), + ), + true, + ), + ("lisa-simpson", test_params::UserPassword::LoginDisallowed, false), + ] { + let created_user = create_local_user( + client, + &silo, + &UserId::from_str(username).unwrap(), + password, + ) + .await; + + // Verify users and password hash state before deleting the silo. + let (_, authz_silo_user, _) = LookupPath::new(opctx, datastore) + .silo_user_id(created_user.id) + .fetch() + .await + .unwrap(); + assert_eq!( + datastore + .silo_user_password_hash_fetch(opctx, &authz_silo_user) + .await + .unwrap() + .is_some(), + expect_password_hash, + "unexpected password hash state for user {username}", + ); + + silo_users.push((created_user.id, authz_silo_user)); + } + + let authz_silo_user_list = authz::SiloUserList::new(authz_silo.clone()); + + // Verify that listing silo users also reports the 3 new users. + assert_eq!( + datastore + .silo_users_list( + opctx, + &authz_silo_user_list, + &DataPageParams::max_page(), + ) + .await + .unwrap() + .len(), + 3, + "unexpected silo user count before deleting silo", + ); + + delete_silo(client, SILO_NAME).await; + + // Verify that users and password hashes no longer exist after deleting + // the silo. We first check the path for listing silo users and then check + // direct lookup for each created user. + assert!( + datastore + .silo_users_list( + opctx, + &authz_silo_user_list, + &DataPageParams::max_page(), + ) + .await + .unwrap() + .is_empty(), + "unexpected silo users after deleting silo", + ); + + for (silo_user_id, authz_silo_user) in silo_users { + LookupPath::new(opctx, datastore) + .silo_user_id(silo_user_id) + .fetch() + .await + .expect_err("unexpected silo user after deleting silo"); + + assert!( + datastore + .silo_user_password_hash_fetch(opctx, &authz_silo_user) + .await + .unwrap() + .is_none(), + "unexpected password hash after deleting silo", + ); + } +} + // Test that silo delete cleans up associated groups #[nexus_test] async fn test_silo_delete_clean_up_groups(cptestctx: &ControlPlaneTestContext) { @@ -1460,11 +1565,7 @@ async fn test_silo_delete_clean_up_groups(cptestctx: &ControlPlaneTestContext) { .expect("silo_user_from_authenticated_subject"); // Delete the silo - NexusRequest::object_delete(&client, &"/v1/system/silos/test-silo") - .authn_as(AuthnMode::PrivilegedUser) - .execute() - .await - .expect("failed to make request"); + delete_silo(client, "test-silo").await; // Expect the group is gone assert!( @@ -2689,8 +2790,7 @@ async fn test_silo_delete_cleans_up_ip_pool_links( assert_eq!(links.items.len(), 1); // Delete the silo - let url = format!("/v1/system/silos/{}", silo1.identity.id); - object_delete(client, &url).await; + delete_silo(client, &silo1.identity.id.to_string()).await; // Now make sure the links are gone let url = "/v1/system/ip-pools/pool1/silos"; @@ -2773,8 +2873,7 @@ async fn test_silo_delete_cleans_up_subnet_pool_links( .await; assert_eq!(links.items.len(), 1); - let url = format!("/v1/system/silos/{}", silo1.identity.id); - object_delete(client, &url).await; + delete_silo(client, &silo1.identity.id.to_string()).await; let url = "/v1/system/subnet-pools/pool1/silos"; let links =