From 88ac0d4f6864c43d3061538e1dc3e940cfcde351 Mon Sep 17 00:00:00 2001 From: Hitesh Sharma Date: Mon, 9 Nov 2026 17:13:57 -0800 Subject: [PATCH 1/5] fix(storage): delete what a principal or account owns with it Inline policies and permissions boundaries are stored by principal name with no foreign key to the principal, and MongoDB and Cassandra link no IAM record to its principal or account. A delete left them behind, and a principal created later with the same name, or an account with the same id, got them and their permissions (#693, #694). - SQLite, PostgreSQL: a user, group or role delete also deletes its inline policies and boundary, in the principal's transaction. - MongoDB: a user delete also deletes its boundary and tags, a group delete its policies and memberships, a role delete its policies, boundary, tags and sessions. Errors are returned, not ignored. - Cassandra: a group delete also deletes its policies and memberships, a user delete its boundary, a role delete its policies, boundary, tags and sessions. - MongoDB, Cassandra: an account delete deletes the account's users, groups, roles and all they own. What a principal or account owns goes before its own record, so a failure leaves less, never more. - Every backend: a user, group or role create, after its own insert, also deletes rows stored under its name, so rows an earlier release left are cleared. MongoDB and Cassandra do the same for an account id. MongoDB now refuses a taken account id on create. - MongoDB, Cassandra: a user, access key, session or role of an account that no longer exists is unknown when signing in or assuming a role. extenddb_storage::iam_leftovers_conformance (feature test-util) checks this against a real database; each backend's suite runs it. Fixes #693. Fixes #694. --- crates/storage-cassandra/src/catalog_store.rs | 21 + .../storage-cassandra/src/credential_store.rs | 25 ++ .../src/management_store/accounts.rs | 27 +- .../src/management_store/dependents.rs | 418 ++++++++++++++++++ .../src/management_store/groups.rs | 23 +- .../src/management_store/mod.rs | 1 + .../src/management_store/roles.rs | 31 +- .../src/management_store/users.rs | 131 +----- .../src/password_hash_store.rs | 8 +- .../tests/direct/iam_leftovers.rs | 101 +++++ .../tests/direct_integration.rs | 2 + .../storage-mongodb/src/credential_store.rs | 24 + .../storage-mongodb/src/management_store.rs | 331 +++++++++++--- .../src/management_store/live_tests.rs | 143 ++++++ .../src/password_hash_store.rs | 10 +- .../src/management_store/groups.rs | 49 +- .../src/management_store/policies.rs | 35 ++ .../src/management_store/roles.rs | 43 +- .../src/management_store/users.rs | 24 +- .../storage-postgres/tests/iam_leftovers.rs | 70 +++ .../src/management_store/groups.rs | 39 +- .../src/management_store/mod.rs | 23 + .../src/management_store/policies.rs | 33 ++ .../src/management_store/roles.rs | 37 +- .../src/management_store/users.rs | 23 +- crates/storage/Cargo.toml | 4 +- .../storage/src/iam_leftovers_conformance.rs | 307 +++++++++++++ crates/storage/src/lib.rs | 3 + devtools/run-mongodb-tests | 14 + 29 files changed, 1761 insertions(+), 239 deletions(-) create mode 100644 crates/storage-cassandra/src/management_store/dependents.rs create mode 100644 crates/storage-cassandra/tests/direct/iam_leftovers.rs create mode 100644 crates/storage-mongodb/src/management_store/live_tests.rs create mode 100644 crates/storage-postgres/tests/iam_leftovers.rs create mode 100644 crates/storage/src/iam_leftovers_conformance.rs diff --git a/crates/storage-cassandra/src/catalog_store.rs b/crates/storage-cassandra/src/catalog_store.rs index 50a3c8206..3994b5177 100644 --- a/crates/storage-cassandra/src/catalog_store.rs +++ b/crates/storage-cassandra/src/catalog_store.rs @@ -240,6 +240,27 @@ impl CassandraCatalogStore { Ok(!rows.is_empty()) } + /// [`Self::account_exists`] read at `LOCAL_QUORUM`, for the console's + /// quorum reads: an account delete, committed at quorum, is seen. + /// + /// Before account deletes removed them, a deleted account's users stayed + /// in the catalog; the console treats a user whose account is gone as + /// absent. + pub(crate) async fn account_exists_quorum(&self, account_id: &str) -> Result { + let query = format!( + "SELECT account_id FROM {}.accounts WHERE account_id = ?", + self.catalog_keyspace() + ); + let rows = crate::cassandra_util::query_rows_quorum( + &self.session, + &query, + cdrs_tokio::query_values!(account_id), + "account_exists_quorum", + ) + .await?; + Ok(!rows.is_empty()) + } + /// Check if a user exists. Used to emulate foreign key checks. pub(crate) async fn user_exists( &self, diff --git a/crates/storage-cassandra/src/credential_store.rs b/crates/storage-cassandra/src/credential_store.rs index 26a00ef9b..4a10eb7a7 100644 --- a/crates/storage-cassandra/src/credential_store.rs +++ b/crates/storage-cassandra/src/credential_store.rs @@ -51,6 +51,25 @@ impl CassandraCredentialStore { fn catalog_keyspace(&self) -> String { format!("{}_catalog", self.keyspace_prefix) } + + /// Whether `account_id` has a row in `accounts`. Before account deletes + /// removed them, a deleted account's access keys and role sessions stayed + /// in the catalog; a credential of an account that no longer exists is + /// unknown. + async fn account_exists(&self, account_id: &str) -> Result { + let query = format!( + "SELECT account_id FROM {}.accounts WHERE account_id = ?", + self.catalog_keyspace() + ); + crate::cassandra_util::query_rows::( + &self.session, + &query, + cdrs_tokio::query_values!(account_id), + "credential account_exists", + ) + .await + .map(|rows| !rows.is_empty()) + } } #[async_trait] @@ -125,6 +144,9 @@ impl CassandraCredentialStore { tracing::error!("Failed to parse is_active: {e}"); DynamoDbError::InternalServerError("Internal error during authentication".to_owned()) })?; + if !self.account_exists(&account_id).await? { + return Ok(None); + } let secret_key = decrypt_secret(&encrypted, &self.encryption_key, access_key_id).map_err(|e| { @@ -206,6 +228,9 @@ impl CassandraCredentialStore { tracing::error!("Failed to parse session_token: {e}"); DynamoDbError::InternalServerError("Internal error during authentication".to_owned()) })?; + if !self.account_exists(&account_id).await? { + return Ok(None); + } let expires_at_ms: i64 = row.get_r_by_name("expires_at").map_err(|e| { tracing::error!("Failed to parse expires_at: {e}"); diff --git a/crates/storage-cassandra/src/management_store/accounts.rs b/crates/storage-cassandra/src/management_store/accounts.rs index 1fd9b2179..f861ffa9c 100644 --- a/crates/storage-cassandra/src/management_store/accounts.rs +++ b/crates/storage-cassandra/src/management_store/accounts.rs @@ -59,7 +59,23 @@ impl CassandraCatalogStore { return Err(OpError::AlreadyExists("Account already exists".to_owned())); } - Ok(()) + // An account deleted before deletes removed them could have left its + // IAM rows under this id. If they cannot be removed, remove the new + // `accounts` row again, so that a retry is not refused while they + // stay; the retry finds the keyspace complete. + let cleared = self.delete_account_iam(account_id).await; + if cleared.is_err() + && let Err(e) = crate::cassandra_util::apply_lwt::( + self.session(), + &format!("DELETE FROM {catalog_keyspace}.accounts WHERE account_id = ? IF EXISTS"), + cdrs_tokio::query_values!(account_id), + "create_account undo", + ) + .await + { + tracing::error!("create_account: remove the new account after a failed clear: {e:?}"); + } + cleared } pub(crate) async fn delete_account_impl(&self, account_id: &str) -> OpResult<()> { @@ -89,6 +105,12 @@ impl CassandraCatalogStore { )); } + // The account's users, groups, roles and everything they own go + // first, as the foreign keys to `accounts` remove them on SQLite and + // PostgreSQL. Left behind, a user could still sign in, and an account + // created later with the same id would get them all. + self.delete_account_iam(account_id).await?; + // Backups outlive their source table, but cannot outlive the account // keyspace that stores their payload. Remove every denormalized and // authoritative catalog row before dropping that keyspace. @@ -142,7 +164,8 @@ impl CassandraCatalogStore { ) .await?; - crate::cassandra_util::apply_lwt( + // Quorum commit, so the console's quorum reads see the account gone. + crate::cassandra_util::apply_lwt_quorum( self.session(), &format!("DELETE FROM {catalog_keyspace}.accounts WHERE account_id = ? IF EXISTS"), cdrs_tokio::query_values!(account_id), diff --git a/crates/storage-cassandra/src/management_store/dependents.rs b/crates/storage-cassandra/src/management_store/dependents.rs new file mode 100644 index 000000000..464ff0559 --- /dev/null +++ b/crates/storage-cassandra/src/management_store/dependents.rs @@ -0,0 +1,418 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +//! The rows a principal or an account owns in the catalog, and their delete. +//! +//! The catalog has no references between tables, so nothing else removes +//! them. A row left behind belongs to the next principal created with the +//! same name: an inline policy or boundary grants it permissions, an access +//! key or a role session signs in as it, and a membership puts it in a group. +//! A deleted account's rows stay as well, and its users could still sign in. +//! +//! Cassandra has no transaction across tables, so each caller deletes what a +//! principal owns before its own row, credentials first: a failure then +//! leaves a principal with less than it had, never a credential or a +//! permission without it, and a retry finds the principal and finishes. +//! +//! Tables written with `IF NOT EXISTS` are deleted with `IF EXISTS`, and the +//! others with a plain `DELETE` (LWT/non-LWT mixing rule, ADR-0021). + +use std::collections::BTreeSet; + +use cdrs_tokio::query::QueryValues; +use cdrs_tokio::query_values; +use extenddb_storage::management_store::{OpError, OpResult}; + +use crate::cassandra_util::{apply_lwt, apply_lwt_quorum, execute, get_column, query_rows}; +use crate::catalog_store::CassandraCatalogStore; + +impl CassandraCatalogStore { + /// The text values of `column` in the rows `query` selects. + async fn select_texts( + &self, + query: &str, + values: QueryValues, + column: &str, + what: &str, + ) -> OpResult> { + query_rows::(self.session(), query, values, what) + .await? + .iter() + .map(|row| get_column(row, column, what)) + .collect() + } + + /// Delete every row a principal owns besides its own row in `iam_users`, + /// `iam_groups` or `iam_roles`. + pub(crate) async fn delete_principal_dependents( + &self, + account_id: &str, + principal_type: &str, + principal_name: &str, + ) -> OpResult<()> { + let ks = self.catalog_keyspace(); + let session = self.session(); + let what = format!("delete {principal_type} {principal_name}'s rows"); + let what = what.as_str(); + + match principal_type { + "user" => { + // access_keys has an IF NOT EXISTS insert path (import). + let key_ids = self + .select_texts( + &format!( + "SELECT access_key_id FROM {ks}.access_keys \ + WHERE account_id = ? AND user_name = ? ALLOW FILTERING" + ), + query_values!(account_id, principal_name), + "access_key_id", + what, + ) + .await?; + for key_id in &key_ids { + apply_lwt::( + session, + &format!("DELETE FROM {ks}.access_keys WHERE access_key_id = ? IF EXISTS"), + query_values!(key_id.as_str()), + what, + ) + .await?; + } + // iam_group_members is written with IF NOT EXISTS. + let groups = self + .select_texts( + &format!( + "SELECT group_name FROM {ks}.iam_group_members \ + WHERE account_id = ? AND user_name = ? ALLOW FILTERING" + ), + query_values!(account_id, principal_name), + "group_name", + what, + ) + .await?; + for group in &groups { + apply_lwt::( + session, + &format!( + "DELETE FROM {ks}.iam_group_members \ + WHERE account_id = ? AND group_name = ? AND user_name = ? IF EXISTS" + ), + query_values!(account_id, group.as_str(), principal_name), + what, + ) + .await?; + } + execute::( + session, + &format!( + "DELETE FROM {ks}.iam_user_tags WHERE account_id = ? AND user_name = ?" + ), + query_values!(account_id, principal_name), + what, + ) + .await?; + } + "group" => { + // iam_group_members is written with IF NOT EXISTS. + let members = self + .select_texts( + &format!( + "SELECT user_name FROM {ks}.iam_group_members \ + WHERE account_id = ? AND group_name = ?" + ), + query_values!(account_id, principal_name), + "user_name", + what, + ) + .await?; + for member in &members { + apply_lwt::( + session, + &format!( + "DELETE FROM {ks}.iam_group_members \ + WHERE account_id = ? AND group_name = ? AND user_name = ? IF EXISTS" + ), + query_values!(account_id, principal_name, member.as_str()), + what, + ) + .await?; + } + } + "role" => { + let tokens = self + .select_texts( + &format!( + "SELECT session_token FROM {ks}.iam_sessions \ + WHERE account_id = ? AND role_name = ? ALLOW FILTERING" + ), + query_values!(account_id, principal_name), + "session_token", + what, + ) + .await?; + for token in &tokens { + execute::( + session, + &format!("DELETE FROM {ks}.iam_sessions WHERE session_token = ?"), + query_values!(token.as_str()), + what, + ) + .await?; + } + execute::( + session, + &format!( + "DELETE FROM {ks}.iam_role_tags WHERE account_id = ? AND role_name = ?" + ), + query_values!(account_id, principal_name), + what, + ) + .await?; + } + other => unreachable!("principal type {other}"), + } + + let policies = self + .select_texts( + &format!( + "SELECT policy_name FROM {ks}.iam_policies \ + WHERE account_id = ? AND principal_type = ? AND principal_name = ?" + ), + query_values!(account_id, principal_type, principal_name), + "policy_name", + what, + ) + .await?; + for policy in &policies { + execute::( + session, + &format!( + "DELETE FROM {ks}.iam_policies WHERE account_id = ? AND principal_type = ? \ + AND principal_name = ? AND policy_name = ?" + ), + query_values!(account_id, principal_type, principal_name, policy.as_str()), + what, + ) + .await?; + } + if principal_type != "group" { + execute::( + session, + &format!( + "DELETE FROM {ks}.iam_permissions_boundaries \ + WHERE account_id = ? AND principal_type = ? AND principal_name = ?" + ), + query_values!(account_id, principal_type, principal_name), + what, + ) + .await?; + } + Ok(()) + } + + /// Delete a principal's own row in `iam_users`, `iam_groups` or + /// `iam_roles`, all written with `IF NOT EXISTS`. A user's delete commits + /// at quorum, so the console's quorum session check sees it. + pub(crate) async fn delete_principal_row( + &self, + account_id: &str, + principal_type: &str, + principal_name: &str, + ) -> OpResult<()> { + let ks = self.catalog_keyspace(); + let what = format!("delete {principal_type} {principal_name}"); + match principal_type { + "user" => apply_lwt_quorum::( + self.session(), + &format!( + "DELETE FROM {ks}.iam_users WHERE account_id = ? AND user_name = ? IF EXISTS" + ), + query_values!(account_id, principal_name), + &what, + ) + .await?, + "group" => apply_lwt::( + self.session(), + &format!( + "DELETE FROM {ks}.iam_groups WHERE account_id = ? AND group_name = ? IF EXISTS" + ), + query_values!(account_id, principal_name), + &what, + ) + .await?, + "role" => apply_lwt::( + self.session(), + &format!( + "DELETE FROM {ks}.iam_roles WHERE account_id = ? AND role_name = ? IF EXISTS" + ), + query_values!(account_id, principal_name), + &what, + ) + .await?, + other => unreachable!("principal type {other}"), + }; + Ok(()) + } + + /// After a principal's row was inserted, delete the rows a delete left + /// behind under its name before deletes removed them. If that fails, + /// delete the new row again, so that a retry of the create is not refused + /// as a duplicate while the rows stay. + pub(crate) async fn clear_new_principal( + &self, + account_id: &str, + principal_type: &str, + principal_name: &str, + ) -> OpResult<()> { + let cleared = self + .delete_principal_dependents(account_id, principal_type, principal_name) + .await; + if cleared.is_err() + && let Err(e) = self + .delete_principal_row(account_id, principal_type, principal_name) + .await + { + tracing::error!( + "create_{principal_type}: remove the new {principal_type} after a failed clear: {e:?}" + ); + } + cleared + } + + /// Delete every IAM row of an account: each user, group and role that has + /// a row in any IAM table, with what it owns, and then its own row. + /// + /// Names come from every table, not only `iam_users`, `iam_groups` and + /// `iam_roles`, so that rows a principal's delete left behind before + /// deletes removed them go too. + pub(crate) async fn delete_account_iam(&self, account_id: &str) -> OpResult<()> { + let ks = self.catalog_keyspace(); + let what = "delete the account's IAM rows"; + let mut principals: BTreeSet<(&'static str, String)> = BTreeSet::new(); + let sources: [(&'static str, String, &str); 13] = [ + ( + "user", + format!("SELECT user_name FROM {ks}.iam_users WHERE account_id = ?"), + "user_name", + ), + ( + "user", + format!( + "SELECT user_name FROM {ks}.access_keys WHERE account_id = ? ALLOW FILTERING" + ), + "user_name", + ), + ( + "user", + format!( + "SELECT user_name FROM {ks}.iam_group_members \ + WHERE account_id = ? ALLOW FILTERING" + ), + "user_name", + ), + ( + "user", + format!( + "SELECT user_name FROM {ks}.iam_user_tags WHERE account_id = ? ALLOW FILTERING" + ), + "user_name", + ), + ( + "user", + format!( + "SELECT principal_name FROM {ks}.iam_policies \ + WHERE account_id = ? AND principal_type = 'user'" + ), + "principal_name", + ), + ( + "user", + format!( + "SELECT principal_name FROM {ks}.iam_permissions_boundaries \ + WHERE account_id = ? AND principal_type = 'user'" + ), + "principal_name", + ), + ( + "group", + format!("SELECT group_name FROM {ks}.iam_groups WHERE account_id = ?"), + "group_name", + ), + ( + "group", + format!( + "SELECT group_name FROM {ks}.iam_group_members \ + WHERE account_id = ? ALLOW FILTERING" + ), + "group_name", + ), + ( + "group", + format!( + "SELECT principal_name FROM {ks}.iam_policies \ + WHERE account_id = ? AND principal_type = 'group'" + ), + "principal_name", + ), + ( + "role", + format!("SELECT role_name FROM {ks}.iam_roles WHERE account_id = ?"), + "role_name", + ), + ( + "role", + format!( + "SELECT role_name FROM {ks}.iam_sessions WHERE account_id = ? ALLOW FILTERING" + ), + "role_name", + ), + ( + "role", + format!( + "SELECT role_name FROM {ks}.iam_role_tags WHERE account_id = ? ALLOW FILTERING" + ), + "role_name", + ), + ( + "role", + format!( + "SELECT principal_name FROM {ks}.iam_policies \ + WHERE account_id = ? AND principal_type = 'role'" + ), + "principal_name", + ), + ]; + for (principal_type, query, column) in &sources { + for name in self + .select_texts(query, query_values!(account_id), column, what) + .await? + { + principals.insert((*principal_type, name)); + } + } + // Role boundaries: the role's own name is enough, and roles with only a + // boundary left are found here. + for name in self + .select_texts( + &format!( + "SELECT principal_name FROM {ks}.iam_permissions_boundaries \ + WHERE account_id = ? AND principal_type = 'role'" + ), + query_values!(account_id), + "principal_name", + what, + ) + .await? + { + principals.insert(("role", name)); + } + + for (principal_type, name) in &principals { + self.delete_principal_dependents(account_id, principal_type, name) + .await?; + self.delete_principal_row(account_id, principal_type, name) + .await?; + } + Ok(()) + } +} diff --git a/crates/storage-cassandra/src/management_store/groups.rs b/crates/storage-cassandra/src/management_store/groups.rs index 47cd15d72..5ee74b6cc 100644 --- a/crates/storage-cassandra/src/management_store/groups.rs +++ b/crates/storage-cassandra/src/management_store/groups.rs @@ -38,8 +38,10 @@ impl CassandraCatalogStore { "IAM group already exists".to_owned(), )); } - - Ok(()) + // A group deleted before deletes removed them could have left rows + // under this name. + self.clear_new_principal(account_id, "group", group_name) + .await } pub(crate) async fn delete_group_impl( @@ -50,18 +52,11 @@ impl CassandraCatalogStore { if !self.group_exists(account_id, group_name).await? { return Err(OpError::NotFound("IAM group not found".to_owned())); } - - let catalog_keyspace = self.catalog_keyspace(); - crate::cassandra_util::apply_lwt( - self.session(), - &format!( - "DELETE FROM {catalog_keyspace}.iam_groups WHERE account_id = ? AND group_name = ? IF EXISTS" - ), - cdrs_tokio::query_values!(account_id, group_name), - "delete_group", - ) - .await?; - Ok(()) + // What the group owns goes first, and its row last (see `dependents`). + self.delete_principal_dependents(account_id, "group", group_name) + .await?; + self.delete_principal_row(account_id, "group", group_name) + .await } pub(crate) async fn list_groups_impl( diff --git a/crates/storage-cassandra/src/management_store/mod.rs b/crates/storage-cassandra/src/management_store/mod.rs index 896ebef37..d7ba82dcb 100755 --- a/crates/storage-cassandra/src/management_store/mod.rs +++ b/crates/storage-cassandra/src/management_store/mod.rs @@ -15,6 +15,7 @@ use super::catalog_store::CassandraCatalogStore; mod access_keys; mod accounts; +mod dependents; mod groups; mod policies; mod roles; diff --git a/crates/storage-cassandra/src/management_store/roles.rs b/crates/storage-cassandra/src/management_store/roles.rs index 30120d584..fb27c75f8 100755 --- a/crates/storage-cassandra/src/management_store/roles.rs +++ b/crates/storage-cassandra/src/management_store/roles.rs @@ -8,7 +8,7 @@ use extenddb_storage::management_store::{OpError, OpResult, RoleDetail}; use time::OffsetDateTime; use crate::cassandra_util::{ - apply_lwt, execute, get_column, get_timestamp, map_rows, query_optional, query_rows, + execute, get_column, get_timestamp, map_rows, query_optional, query_rows, }; use crate::catalog_store::CassandraCatalogStore; @@ -64,27 +64,22 @@ impl CassandraCatalogStore { if !applied { return Err(OpError::AlreadyExists("IAM role already exists".to_owned())); } - - Ok(()) + // A role deleted before deletes removed them could have left rows + // under this name. + self.clear_new_principal(account_id, "role", role_name) + .await } pub(crate) async fn delete_role_impl(&self, account_id: &str, role_name: &str) -> OpResult<()> { if !self.role_exists(account_id, role_name).await? { return Err(OpError::NotFound("IAM role not found".to_owned())); } - - let query = format!( - "DELETE FROM {}.iam_roles WHERE account_id = ? AND role_name = ? IF EXISTS", - self.catalog_keyspace() - ); - apply_lwt( - self.session(), - &query, - query_values!(account_id, role_name), - "delete_role", - ) - .await?; - Ok(()) + // What the role owns goes first, sessions among it, and its row last + // (see `dependents`). + self.delete_principal_dependents(account_id, "role", role_name) + .await?; + self.delete_principal_row(account_id, "role", role_name) + .await } pub(crate) async fn list_roles_impl( @@ -232,6 +227,10 @@ impl CassandraCatalogStore { let Some(row) = row else { return Ok(None); }; + // A role of a deleted account cannot be assumed. + if !self.account_exists(account_id).await? { + return Ok(None); + } let trust_policy_str: String = get_column(&row, "trust_policy", "get_role_trust_policy")?; let trust_policy: serde_json::Value = diff --git a/crates/storage-cassandra/src/management_store/users.rs b/crates/storage-cassandra/src/management_store/users.rs index 0db1e676d..9cadb2542 100644 --- a/crates/storage-cassandra/src/management_store/users.rs +++ b/crates/storage-cassandra/src/management_store/users.rs @@ -64,6 +64,10 @@ impl CassandraCatalogStore { if !applied { return Err(OpError::AlreadyExists("IAM user already exists".to_owned())); } + // A user deleted before deletes removed them could have left rows + // under this name. + self.clear_new_principal(&account_id, "user", &user_name) + .await?; // User was created; now insert the self-service policy. let policy_query = format!( @@ -93,121 +97,13 @@ impl CassandraCatalogStore { if !self.user_exists(account_id, user_name).await? { return Err(OpError::NotFound("IAM user not found".to_owned())); } - - let ks = self.catalog_keyspace(); - let session = self.session(); - - // Gather all keys needed for cascade deletes before building the batch. - let key_ids: Vec = crate::cassandra_util::query_rows( - session, - &format!( - "SELECT access_key_id FROM {ks}.access_keys \ - WHERE account_id = ? AND user_name = ? ALLOW FILTERING" - ), - cdrs_tokio::query_values!(account_id, user_name), - "delete_user access_keys", - ) - .await? - .into_iter() - .map(|row| crate::cassandra_util::get_column(&row, "access_key_id", "delete_user")) - .collect::>()?; - - let policy_names: Vec = crate::cassandra_util::query_rows( - session, - &format!( - "SELECT policy_name FROM {ks}.iam_policies \ - WHERE account_id = ? AND principal_type = 'user' AND principal_name = ?" - ), - cdrs_tokio::query_values!(account_id, user_name), - "delete_user policies", - ) - .await? - .into_iter() - .map(|row| crate::cassandra_util::get_column(&row, "policy_name", "delete_user")) - .collect::>()?; - - let group_names: Vec = crate::cassandra_util::query_rows( - session, - &format!( - "SELECT group_name FROM {ks}.iam_group_members \ - WHERE account_id = ? AND user_name = ? ALLOW FILTERING" - ), - cdrs_tokio::query_values!(account_id, user_name), - "delete_user groups", - ) - .await? - .into_iter() - .map(|row| crate::cassandra_util::get_column(&row, "group_name", "delete_user")) - .collect::>()?; - - // Sequential deletes ordered so that the most security-sensitive rows - // are removed first. access_keys is deleted before iam_users so that - // a partial failure leaves orphaned catalog entries (inert) rather than - // live credentials whose parent user is already gone. iam_users is - // deleted last so the user is only "officially" absent once all - // dependent rows have been cleaned up, and a retry can still find the - // user and re-attempt any failed steps. - // - // Only tables created with IF NOT EXISTS use IF EXISTS on delete - // (LWT/non-LWT mixing rule). Tables with plain INSERT use plain DELETE. - // See ADR-0021. - let session = std::sync::Arc::clone(session); - for key_id in &key_ids { - // access_keys has an IF NOT EXISTS insert path (import) — use IF EXISTS on delete - crate::cassandra_util::apply_lwt( - &session, - &format!("DELETE FROM {ks}.access_keys WHERE access_key_id = ? IF EXISTS"), - cdrs_tokio::query_values!(key_id.as_str()), - "delete_user access_keys", - ) - .await?; - } - for group_name in &group_names { - // iam_group_members uses IF NOT EXISTS — must use IF EXISTS - crate::cassandra_util::apply_lwt( - &session, - &format!( - "DELETE FROM {ks}.iam_group_members \ - WHERE account_id = ? AND group_name = ? AND user_name = ? IF EXISTS" - ), - cdrs_tokio::query_values!(account_id, group_name.as_str(), user_name), - "delete_user iam_group_members", - ) + // What the user owns goes first, and its row last: the user is only + // gone once all of it is, and a retry still finds the user and + // finishes (see `dependents`). + self.delete_principal_dependents(account_id, "user", user_name) .await?; - } - for policy_name in &policy_names { - // iam_policies uses plain INSERT — plain DELETE is safe - crate::cassandra_util::execute( - &session, - &format!( - "DELETE FROM {ks}.iam_policies \ - WHERE account_id = ? AND principal_type = 'user' \ - AND principal_name = ? AND policy_name = ?" - ), - cdrs_tokio::query_values!(account_id, user_name, policy_name.as_str()), - "delete_user iam_policies", - ) - .await?; - } - // iam_user_tags uses plain INSERT — plain DELETE is safe - crate::cassandra_util::execute( - &session, - &format!("DELETE FROM {ks}.iam_user_tags WHERE account_id = ? AND user_name = ?"), - cdrs_tokio::query_values!(account_id, user_name), - "delete_user iam_user_tags", - ) - .await?; - // iam_users last: user is only gone once all dependent rows are cleaned up. - // Quorum commit, so the console's quorum session check sees the deletion. - crate::cassandra_util::apply_lwt_quorum( - &session, - &format!("DELETE FROM {ks}.iam_users WHERE account_id = ? AND user_name = ? IF EXISTS"), - cdrs_tokio::query_values!(account_id, user_name), - "delete_user iam_users", - ) - .await?; - - Ok(()) + self.delete_principal_row(account_id, "user", user_name) + .await } pub(crate) async fn list_users_impl( @@ -394,9 +290,14 @@ impl CassandraCatalogStore { // `None` when the user is absent or has no console password, which // still runs one bcrypt check (see `verify_stored_password`), as a - // wrong password does. + // wrong password does. A user of a deleted account counts as absent. let hash: Option = row.and_then(|row| row.get_by_name("password_hash").ok().flatten()); + let hash = if hash.is_some() && !self.account_exists_quorum(account_id).await? { + None + } else { + hash + }; extenddb_storage::password::verify_stored_password(password.to_owned(), hash).await } diff --git a/crates/storage-cassandra/src/password_hash_store.rs b/crates/storage-cassandra/src/password_hash_store.rs index 7038b2617..a3d5f9658 100644 --- a/crates/storage-cassandra/src/password_hash_store.rs +++ b/crates/storage-cassandra/src/password_hash_store.rs @@ -56,7 +56,13 @@ impl PasswordHashStore for CassandraCatalogStore { "get_user_password_hash", ) .await?; - password_hash_of(rows, "get_user_password_hash") + let hash = password_hash_of(rows, "get_user_password_hash")?; + // A user of a deleted account has no password: its console + // sessions end and it cannot log in. + if hash.is_some() && !self.account_exists_quorum(&account_id).await? { + return Ok(None); + } + Ok(hash) }) } } diff --git a/crates/storage-cassandra/tests/direct/iam_leftovers.rs b/crates/storage-cassandra/tests/direct/iam_leftovers.rs new file mode 100644 index 000000000..9ddcd016b --- /dev/null +++ b/crates/storage-cassandra/tests/direct/iam_leftovers.rs @@ -0,0 +1,101 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +//! A deleted principal or account leaves nothing behind (#693, #694). + +#[cfg(test)] +mod tests { + use crate::helpers::{setup_engine, test_config, unique_test_account}; + use extenddb_storage::management_store::{ManagementStore, PasswordHashStore}; + + #[tokio::test] + async fn deleted_principals_and_accounts_leave_nothing() { + if crate::helpers::skip_without_cassandra() { + return; + } + let engine = setup_engine().await; + let config = test_config(); + let store = crate::helpers::create_catalog_store(&engine, &config); + extenddb_storage::iam_leftovers_conformance::run(&store, &unique_test_account()).await; + } + + /// Before account deletes removed them, a deleted account's users and + /// roles stayed. With only the `accounts` row removed, as such a delete + /// left it, the user cannot sign in to the console and the role cannot be + /// assumed. + #[tokio::test] + async fn a_user_or_role_of_a_deleted_account_grants_nothing() { + if crate::helpers::skip_without_cassandra() { + return; + } + let engine = setup_engine().await; + let config = test_config(); + let store = crate::helpers::create_catalog_store(&engine, &config); + let account = unique_test_account(); + store.create_account(&account, "acct").await.unwrap(); + let hash = bcrypt::hash("correct horse", 4).unwrap(); + store + .create_user(&account, "alice", Some(&hash)) + .await + .unwrap(); + let trust = serde_json::json!({"Version": "2012-10-17", "Statement": []}); + store.create_role(&account, "app", &trust).await.unwrap(); + assert!( + store + .get_user_password_hash(&account, "alice") + .await + .unwrap() + .is_some() + ); + assert!( + store + .verify_iam_user_password(&account, "alice", "correct horse") + .await + .unwrap() + ); + assert!( + store + .get_role_trust_policy(&account, "app") + .await + .unwrap() + .is_some() + ); + + // `accounts` is written with IF NOT EXISTS, so the delete is an LWT. + engine + .session_arc() + .query_with_values( + format!( + "DELETE FROM {}_catalog.accounts WHERE account_id = ? IF EXISTS", + config.keyspace_prefix + ), + cdrs_tokio::query_values!(account.as_str()), + ) + .await + .expect("remove the accounts row only"); + + assert_eq!( + store + .get_user_password_hash(&account, "alice") + .await + .unwrap(), + None + ); + assert!( + !store + .verify_iam_user_password(&account, "alice", "correct horse") + .await + .unwrap() + ); + assert_eq!( + store.get_role_trust_policy(&account, "app").await.unwrap(), + None + ); + + // The account created again with this id starts empty. + store.create_account(&account, "acct-again").await.unwrap(); + assert!(store.list_users(&account).await.unwrap().is_empty()); + assert!(store.list_roles(&account).await.unwrap().is_empty()); + store.delete_account(&account).await.unwrap(); + } +} diff --git a/crates/storage-cassandra/tests/direct_integration.rs b/crates/storage-cassandra/tests/direct_integration.rs index 863bbda69..f35cacdbb 100644 --- a/crates/storage-cassandra/tests/direct_integration.rs +++ b/crates/storage-cassandra/tests/direct_integration.rs @@ -31,6 +31,8 @@ mod concurrent_account_creation; mod delete_item; #[path = "direct/groups.rs"] mod groups; +#[path = "direct/iam_leftovers.rs"] +mod iam_leftovers; #[path = "direct/index.rs"] mod index; #[path = "direct/keyspace_migrations.rs"] diff --git a/crates/storage-mongodb/src/credential_store.rs b/crates/storage-mongodb/src/credential_store.rs index 2d75c2a41..74d318b46 100644 --- a/crates/storage-mongodb/src/credential_store.rs +++ b/crates/storage-mongodb/src/credential_store.rs @@ -36,6 +36,24 @@ impl MongoCredentialStore { self.client.database("extenddb_catalog") } + /// Whether `account_id` has a document in `accounts`. Before account + /// deletes removed them, a deleted account's access keys and role + /// sessions stayed in the catalog; a credential of an account that no + /// longer exists is unknown. + async fn account_exists(&self, account_id: &str) -> Result { + self.catalog_db() + .collection::("accounts") + .find_one(doc! { "account_id": account_id }) + .await + .map(|d| d.is_some()) + .map_err(|e| { + tracing::error!("Account lookup failed for account {account_id}: {e}"); + DynamoDbError::InternalServerError( + "Internal error during authentication".to_owned(), + ) + }) + } + async fn lookup_user_credential( &self, access_key_id: &str, @@ -65,6 +83,9 @@ impl MongoCredentialStore { // a corrupted or partially written access-key record cannot silently // authenticate. let is_active = key_doc.get_bool("is_active").unwrap_or(false); + if !self.account_exists(&account_id).await? { + return Ok(None); + } let secret_key = decrypt_secret(&encrypted, &self.encryption_key, access_key_id).map_err(|e| { @@ -127,6 +148,9 @@ impl MongoCredentialStore { .get_str("session_token") .unwrap_or_default() .to_owned(); + if !self.account_exists(&account_id).await? { + return Ok(None); + } let expires_at = session_doc.get_datetime("expires_at").map_err(|_| { DynamoDbError::InternalServerError("Internal error during authentication".to_owned()) diff --git a/crates/storage-mongodb/src/management_store.rs b/crates/storage-mongodb/src/management_store.rs index 305ffb0fe..c5df12934 100644 --- a/crates/storage-mongodb/src/management_store.rs +++ b/crates/storage-mongodb/src/management_store.rs @@ -35,6 +35,200 @@ fn now_bson() -> BsonDateTime { BsonDateTime::now() } +/// The records a principal owns besides its own document, as +/// `(collection, filter)`: what deleting it must also delete. +/// +/// The catalog has no references between collections, so nothing else +/// removes them. A record left behind would belong to the next principal +/// created with the same name: an inline policy or boundary would grant it +/// permissions, an access key or a role session would sign in as it, and a +/// membership would put it in a group. +fn principal_dependents( + account_id: &str, + principal_type: &str, + principal_name: &str, +) -> Vec<(&'static str, Document)> { + let attached = doc! { + "account_id": account_id, + "principal_type": principal_type, + "principal_name": principal_name, + }; + match principal_type { + "user" => vec![ + ( + "access_keys", + doc! { "account_id": account_id, "user_name": principal_name }, + ), + ("iam_policies", attached.clone()), + ("iam_permissions_boundaries", attached), + ( + "iam_group_members", + doc! { "account_id": account_id, "user_name": principal_name }, + ), + ( + "iam_user_tags", + doc! { "account_id": account_id, "user_name": principal_name }, + ), + ], + "group" => vec![ + ("iam_policies", attached), + ( + "iam_group_members", + doc! { "account_id": account_id, "group_name": principal_name }, + ), + ], + "role" => vec![ + ("iam_policies", attached.clone()), + ("iam_permissions_boundaries", attached), + ( + "iam_role_tags", + doc! { "account_id": account_id, "role_name": principal_name }, + ), + ( + "iam_sessions", + doc! { "account_id": account_id, "role_name": principal_name }, + ), + ], + other => unreachable!("principal type {other}"), + } +} + +/// The filter of a principal's own document: `{account_id, }`. +fn principal_filter(account_id: &str, name_field: &str, principal_name: &str) -> Document { + let mut filter = doc! { "account_id": account_id }; + filter.insert(name_field, principal_name); + filter +} + +/// Every collection that holds an account's IAM records, each record with an +/// `account_id` field. An account's delete removes all of them, as the +/// foreign keys to `accounts` do on SQLite and PostgreSQL. +const ACCOUNT_IAM_COLLECTIONS: &[&str] = &[ + "access_keys", + "iam_sessions", + "iam_policies", + "iam_permissions_boundaries", + "iam_group_members", + "iam_user_tags", + "iam_role_tags", + "iam_users", + "iam_groups", + "iam_roles", +]; + +impl MongoCatalogStore { + /// Whether the account has a document in `accounts`. + /// + /// The catalog has no references between collections, and before account + /// deletes removed them, a deleted account's IAM records stayed. The + /// reads that sign a principal in, or let a role be assumed, check the + /// account through this, so those records grant nothing. + pub(crate) async fn account_exists(&self, account_id: &str) -> OpResult { + self.catalog_db() + .collection::("accounts") + .find_one(doc! { "account_id": account_id }) + .await + .map(|d| d.is_some()) + .map_err(|e| { + tracing::error!("account_exists: {e}"); + OpError::Internal("Database error".to_owned()) + }) + } + + /// Delete every IAM record of an account: its users, groups and roles and + /// all they own. Each principal's own document goes last. + async fn delete_account_iam(&self, what: &str, account_id: &str) -> OpResult<()> { + let records: Vec<(&str, Document)> = ACCOUNT_IAM_COLLECTIONS + .iter() + .map(|collection| (*collection, doc! { "account_id": account_id })) + .collect(); + self.delete_records(what, &records).await + } + + /// Delete each `(collection, filter)` in turn, stopping at the first + /// error. The management store writes without transactions, so callers + /// delete what a principal or an account owns before its own document: + /// a failure then leaves it with less than it had, never its records + /// without it. + async fn delete_records(&self, what: &str, records: &[(&str, Document)]) -> OpResult<()> { + for (collection, filter) in records { + self.catalog_db() + .collection::(collection) + .delete_many(filter.clone()) + .await + .map_err(|e| { + tracing::error!("{what}: delete from {collection}: {e}"); + OpError::Internal("Database error".to_owned()) + })?; + } + Ok(()) + } + + /// Delete a principal's records and then its document in `collection`. + async fn delete_principal( + &self, + collection: &str, + name_field: &str, + account_id: &str, + principal_type: &str, + principal_name: &str, + ) -> OpResult<()> { + let what = format!("delete_{principal_type}"); + self.delete_records( + &what, + &principal_dependents(account_id, principal_type, principal_name), + ) + .await?; + let result = self + .catalog_db() + .collection::(collection) + .delete_one(principal_filter(account_id, name_field, principal_name)) + .await + .map_err(|e| { + tracing::error!("{what} failed: {e}"); + OpError::Internal("Database error".to_owned()) + })?; + if result.deleted_count == 0 { + return Err(OpError::NotFound(format!("IAM {principal_type} not found"))); + } + Ok(()) + } + + /// After a principal's document was inserted, delete the records a delete + /// left behind under its name before deletes removed them. If that fails, + /// delete the new document again, so that a retry of the create is not + /// refused as a duplicate while the records stay. + async fn clear_new_principal( + &self, + collection: &str, + name_field: &str, + account_id: &str, + principal_type: &str, + principal_name: &str, + ) -> OpResult<()> { + let what = format!("create_{principal_type}"); + let cleared = self + .delete_records( + &what, + &principal_dependents(account_id, principal_type, principal_name), + ) + .await; + if cleared.is_err() { + let undo = self + .catalog_db() + .collection::(collection) + .delete_one(principal_filter(account_id, name_field, principal_name)) + .await; + if let Err(e) = undo { + tracing::error!( + "{what}: remove the new {principal_type} after a failed clear: {e}" + ); + } + } + cleared + } +} + // ── ManagementStore ───────────────────────────────────────────────────── impl ManagementStore for MongoCatalogStore { @@ -57,6 +251,18 @@ impl ManagementStore for MongoCatalogStore { let account_name = account_name.to_owned(); Box::pin(async move { let coll = self.catalog_db().collection::("accounts"); + // `accounts` has a unique index on `account_name` only, so refuse a + // taken id here: the clear below must never reach a live account. + let taken = coll + .find_one(doc! { "account_id": &account_id }) + .await + .map_err(|e| { + tracing::error!("create_account check id: {e}"); + OpError::Internal("Database error".to_owned()) + })?; + if taken.is_some() { + return Err(OpError::AlreadyExists("Account already exists".to_owned())); + } let result = coll .insert_one(doc! { "account_id": &account_id, @@ -64,16 +270,27 @@ impl ManagementStore for MongoCatalogStore { "created_at": now_bson(), }) .await; - match result { - Ok(_) => Ok(()), + let inserted = match result { + Ok(inserted) => inserted.inserted_id, Err(e) if is_duplicate_key(&e) => { - Err(OpError::AlreadyExists("Account already exists".to_owned())) + return Err(OpError::AlreadyExists("Account already exists".to_owned())); } Err(e) => { tracing::error!("create_account failed: {e}"); - Err(OpError::Internal("Database error".to_owned())) + return Err(OpError::Internal("Database error".to_owned())); } + }; + // An account deleted before deletes removed them could have left + // its IAM records under this id. If they cannot be removed, remove + // the new account again, so that a retry is not refused while they + // stay. + let cleared = self.delete_account_iam("create_account", &account_id).await; + if cleared.is_err() + && let Err(e) = coll.delete_one(doc! { "_id": inserted }).await + { + tracing::error!("create_account: remove the new account after a failed clear: {e}"); } + cleared }) } @@ -96,9 +313,16 @@ impl ManagementStore for MongoCatalogStore { )); } + // The account's users, groups, roles and everything they own go + // first, as the foreign keys to `accounts` remove them on SQLite + // and PostgreSQL. Left behind, a user could still sign in, and an + // account created later with the same id would get them all. + self.delete_account_iam("delete_account", &account_id) + .await?; + let coll = self.catalog_db().collection::("accounts"); let result = coll - .delete_one(doc! { "account_id": &account_id }) + .delete_many(doc! { "account_id": &account_id }) .await .map_err(|e| { tracing::error!("delete_account: {e}"); @@ -358,6 +582,8 @@ impl ManagementStore for MongoCatalogStore { return Err(OpError::Internal("Database error".to_owned())); } } + self.clear_new_principal("iam_users", "user_name", &account_id, "user", &user_name) + .await?; // Seed default self-service policy. let self_service_policy = serde_json::json!({ @@ -408,33 +634,8 @@ impl ManagementStore for MongoCatalogStore { let account_id = account_id.to_owned(); let user_name = user_name.to_owned(); Box::pin(async move { - let coll = self.catalog_db().collection::("iam_users"); - let result = coll - .delete_one(doc! { "account_id": &account_id, "user_name": &user_name }) + self.delete_principal("iam_users", "user_name", &account_id, "user", &user_name) .await - .map_err(|e| { - tracing::error!("delete_user failed: {e}"); - OpError::Internal("Database error".to_owned()) - })?; - if result.deleted_count == 0 { - return Err(OpError::NotFound("IAM user not found".to_owned())); - } - // Cascade: delete access keys, policies, group memberships - let keys_coll = self.catalog_db().collection::("access_keys"); - let _ = keys_coll - .delete_many(doc! { "account_id": &account_id, "user_name": &user_name }) - .await; - let policies_coll = self.catalog_db().collection::("iam_policies"); - let _ = policies_coll - .delete_many(doc! { "account_id": &account_id, "principal_type": "user", "principal_name": &user_name }) - .await; - let members_coll = self - .catalog_db() - .collection::("iam_group_members"); - let _ = members_coll - .delete_many(doc! { "account_id": &account_id, "user_name": &user_name }) - .await; - Ok(()) }) } @@ -636,8 +837,13 @@ impl ManagementStore for MongoCatalogStore { // `None` when the user is absent or has no console password, which // still runs one bcrypt check (see `verify_stored_password`), as a - // wrong password does. + // wrong password does. A user of a deleted account counts as absent. let hash = doc.and_then(|d| d.get_str("password_hash").ok().map(str::to_owned)); + let hash = if hash.is_some() && !self.account_exists(&account_id).await? { + None + } else { + hash + }; extenddb_storage::password::verify_stored_password(password, hash).await }) } @@ -784,7 +990,16 @@ impl ManagementStore for MongoCatalogStore { }) .await; match result { - Ok(_) => Ok(()), + Ok(_) => { + self.clear_new_principal( + "iam_groups", + "group_name", + &account_id, + "group", + &group_name, + ) + .await + } Err(e) if is_duplicate_key(&e) => Err(OpError::AlreadyExists( "IAM group already exists".to_owned(), )), @@ -800,18 +1015,14 @@ impl ManagementStore for MongoCatalogStore { let account_id = account_id.to_owned(); let group_name = group_name.to_owned(); Box::pin(async move { - let coll = self.catalog_db().collection::("iam_groups"); - let result = coll - .delete_one(doc! { "account_id": &account_id, "group_name": &group_name }) - .await - .map_err(|e| { - tracing::error!("delete_group failed: {e}"); - OpError::Internal("Database error".to_owned()) - })?; - if result.deleted_count == 0 { - return Err(OpError::NotFound("IAM group not found".to_owned())); - } - Ok(()) + self.delete_principal( + "iam_groups", + "group_name", + &account_id, + "group", + &group_name, + ) + .await }) } @@ -1044,7 +1255,16 @@ impl ManagementStore for MongoCatalogStore { }) .await; match result { - Ok(_) => Ok(()), + Ok(_) => { + self.clear_new_principal( + "iam_roles", + "role_name", + &account_id, + "role", + &role_name, + ) + .await + } Err(e) if is_duplicate_key(&e) => { Err(OpError::AlreadyExists("IAM role already exists".to_owned())) } @@ -1060,18 +1280,8 @@ impl ManagementStore for MongoCatalogStore { let account_id = account_id.to_owned(); let role_name = role_name.to_owned(); Box::pin(async move { - let coll = self.catalog_db().collection::("iam_roles"); - let result = coll - .delete_one(doc! { "account_id": &account_id, "role_name": &role_name }) + self.delete_principal("iam_roles", "role_name", &account_id, "role", &role_name) .await - .map_err(|e| { - tracing::error!("delete_role failed: {e}"); - OpError::Internal("Database error".to_owned()) - })?; - if result.deleted_count == 0 { - return Err(OpError::NotFound("IAM role not found".to_owned())); - } - Ok(()) }) } @@ -1216,6 +1426,10 @@ impl ManagementStore for MongoCatalogStore { tracing::error!("get_role_trust_policy: {e}"); OpError::Internal("Database error".to_owned()) })?; + // A role of a deleted account cannot be assumed. + if doc.is_some() && !self.account_exists(&account_id).await? { + return Ok(None); + } Ok(doc.and_then(|d| { d.get("trust_policy") .and_then(|b| bson::from_bson(b.clone()).ok()) @@ -2525,3 +2739,6 @@ fn stored_document(d: &Document) -> StoredDocument { Some(b) => bson::from_bson::(b.clone()).map_err(|e| e.to_string()), } } + +#[cfg(test)] +mod live_tests; diff --git a/crates/storage-mongodb/src/management_store/live_tests.rs b/crates/storage-mongodb/src/management_store/live_tests.rs new file mode 100644 index 000000000..5fdbb949d --- /dev/null +++ b/crates/storage-mongodb/src/management_store/live_tests.rs @@ -0,0 +1,143 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +//! A deleted principal or account leaves nothing behind (#693, #694), against +//! a MongoDB replica set named by `EXTENDDB_TEST_MONGODB_URI`. +//! +//! `cargo test` does not run these: `devtools/run-mongodb-tests --rust-live` +//! runs them with `--ignored`. Each test works in its own pair of databases, +//! created with every migration as `init` creates them, and drops them at the +//! end. + +use std::time::Duration; + +use extenddb_storage::management_store::{ManagementStore, PasswordHashStore}; +use mongodb::bson::{Document, doc}; + +use crate::MongoCatalogStore; +use crate::migrations::{MongoTarget, Timing, lock, migrate_holding}; + +fn uri() -> String { + std::env::var("EXTENDDB_TEST_MONGODB_URI").expect( + "set EXTENDDB_TEST_MONGODB_URI to a MongoDB replica set, e.g. \ + mongodb://127.0.0.1:27741/?directConnection=true", + ) +} + +/// A catalog store on scratch databases that every migration created. +struct Scratch { + client: mongodb::Client, + catalog: String, + data: String, + store: MongoCatalogStore, +} + +impl Scratch { + async fn new() -> Self { + let tag = uuid::Uuid::new_v4().simple().to_string(); + let catalog = format!("t{}_cat", &tag[..12]); + let data = format!("t{}_data", &tag[..12]); + let target = MongoTarget::with_databases(uri(), catalog.clone(), data.clone()); + let (held, target) = lock(&target, true, Duration::from_secs(10), Timing::PRODUCTION) + .await + .expect("lock the scratch deployment"); + migrate_holding(&target, "test", &held) + .await + .expect("create the scratch catalog"); + held.release().await; + let client = mongodb::Client::with_uri_str(uri()).await.expect("client"); + let store = MongoCatalogStore::on_database(client.clone(), client.database(&catalog)); + Self { + client, + catalog, + data, + store, + } + } + + async fn drop(self) { + self.client + .database(&self.catalog) + .drop() + .await + .expect("drop catalog"); + self.client + .database(&self.data) + .drop() + .await + .expect("drop data"); + } +} + +#[tokio::test] +#[ignore = "needs a MongoDB replica set in EXTENDDB_TEST_MONGODB_URI"] +async fn deleted_principals_and_accounts_leave_nothing() { + let s = Scratch::new().await; + extenddb_storage::iam_leftovers_conformance::run(&s.store, "123456789012").await; + s.drop().await; +} + +/// Before account deletes removed them, a deleted account's users and roles +/// stayed. With only the `accounts` document removed, as such a delete left +/// it, the user cannot sign in to the console and the role cannot be assumed. +#[tokio::test] +#[ignore = "needs a MongoDB replica set in EXTENDDB_TEST_MONGODB_URI"] +async fn a_user_or_role_of_a_deleted_account_grants_nothing() { + const ACCOUNT: &str = "123456789012"; + let s = Scratch::new().await; + let store = &s.store; + store.create_account(ACCOUNT, "acct").await.unwrap(); + let hash = bcrypt::hash("correct horse", 4).unwrap(); + store + .create_user(ACCOUNT, "alice", Some(&hash)) + .await + .unwrap(); + let trust = serde_json::json!({"Version": "2012-10-17", "Statement": []}); + store.create_role(ACCOUNT, "app", &trust).await.unwrap(); + assert!( + store + .get_user_password_hash(ACCOUNT, "alice") + .await + .unwrap() + .is_some() + ); + assert!( + store + .verify_iam_user_password(ACCOUNT, "alice", "correct horse") + .await + .unwrap() + ); + assert!( + store + .get_role_trust_policy(ACCOUNT, "app") + .await + .unwrap() + .is_some() + ); + + s.client + .database(&s.catalog) + .collection::("accounts") + .delete_many(doc! { "account_id": ACCOUNT }) + .await + .expect("remove the accounts document only"); + + assert_eq!( + store + .get_user_password_hash(ACCOUNT, "alice") + .await + .unwrap(), + None + ); + assert!( + !store + .verify_iam_user_password(ACCOUNT, "alice", "correct horse") + .await + .unwrap() + ); + assert_eq!( + store.get_role_trust_policy(ACCOUNT, "app").await.unwrap(), + None + ); + s.drop().await; +} diff --git a/crates/storage-mongodb/src/password_hash_store.rs b/crates/storage-mongodb/src/password_hash_store.rs index be35a9504..298e7848f 100644 --- a/crates/storage-mongodb/src/password_hash_store.rs +++ b/crates/storage-mongodb/src/password_hash_store.rs @@ -23,8 +23,16 @@ impl PasswordHashStore for MongoCatalogStore { user_name: &str, ) -> BoxFuture<'_, OpResult>> { let filter = doc! { "account_id": account_id, "user_name": user_name }; + let account_id = account_id.to_owned(); Box::pin(async move { - find_password_hash(self, "iam_users", filter, "get_user_password_hash").await + let hash = + find_password_hash(self, "iam_users", filter, "get_user_password_hash").await?; + // A user of a deleted account has no password: its console + // sessions end and it cannot log in. + if hash.is_some() && !self.account_exists(&account_id).await? { + return Ok(None); + } + Ok(hash) }) } } diff --git a/crates/storage-postgres/src/management_store/groups.rs b/crates/storage-postgres/src/management_store/groups.rs index 570047642..7961ad8c6 100755 --- a/crates/storage-postgres/src/management_store/groups.rs +++ b/crates/storage-postgres/src/management_store/groups.rs @@ -6,6 +6,7 @@ use extenddb_storage::management_store::{GroupDetail, OpError, OpResult}; use super::database_error; +use super::policies::delete_principal_attachments; use crate::catalog_store::PostgresCatalogStore; use crate::pg_util::{is_fk_violation, is_unique_violation}; @@ -16,25 +17,40 @@ impl PostgresCatalogStore { group_name: &str, ) -> OpResult<()> { let group_arn = format!("arn:aws:iam::{account_id}:group/{group_name}"); + let mut tx = self.pool().begin().await.map_err(|e| { + tracing::error!("create_group begin transaction: {e}"); + OpError::Internal("Database error".to_owned()) + })?; let result = sqlx::query( "INSERT INTO iam_groups (account_id, group_name, group_arn) VALUES ($1, $2, $3)", ) .bind(account_id) .bind(group_name) .bind(&group_arn) - .execute(self.pool()) + .execute(&mut *tx) .await; match result { - Ok(_) => Ok(()), - Err(e) if is_unique_violation(&e) => Err(OpError::AlreadyExists( - "IAM group already exists".to_owned(), - )), - Err(e) if is_fk_violation(&e) => Err(OpError::NotFound("Account not found".to_owned())), + Ok(_) => {} + Err(e) if is_unique_violation(&e) => { + return Err(OpError::AlreadyExists( + "IAM group already exists".to_owned(), + )); + } + Err(e) if is_fk_violation(&e) => { + return Err(OpError::NotFound("Account not found".to_owned())); + } Err(e) => { tracing::error!("create_group failed: {e}"); - Err(OpError::Internal("Database error".to_owned())) + return Err(OpError::Internal("Database error".to_owned())); } } + // A group deleted before deletes removed them could have left its + // inline policies under this name. + delete_principal_attachments(&mut tx, account_id, "group", group_name).await?; + tx.commit().await.map_err(|e| { + tracing::error!("create_group commit: {e}"); + OpError::Internal("Database error".to_owned()) + }) } pub(crate) async fn delete_group_impl( @@ -42,22 +58,33 @@ impl PostgresCatalogStore { account_id: &str, group_name: &str, ) -> OpResult<()> { + let mut tx = self.pool().begin().await.map_err(|e| { + tracing::error!("delete_group begin transaction: {e}"); + OpError::Internal("Database error".to_owned()) + })?; + // The foreign keys remove the group's memberships. Its inline + // policies have none. let result = sqlx::query("DELETE FROM iam_groups WHERE account_id = $1 AND group_name = $2") .bind(account_id) .bind(group_name) - .execute(self.pool()) + .execute(&mut *tx) .await; match result { Ok(r) if r.rows_affected() == 0 => { - Err(OpError::NotFound("IAM group not found".to_owned())) + return Err(OpError::NotFound("IAM group not found".to_owned())); } - Ok(_) => Ok(()), + Ok(_) => {} Err(e) => { tracing::error!("delete_group failed: {e}"); - Err(OpError::Internal("Database error".to_owned())) + return Err(OpError::Internal("Database error".to_owned())); } } + delete_principal_attachments(&mut tx, account_id, "group", group_name).await?; + tx.commit().await.map_err(|e| { + tracing::error!("delete_group commit: {e}"); + OpError::Internal("Database error".to_owned()) + }) } pub(crate) async fn list_groups_impl( diff --git a/crates/storage-postgres/src/management_store/policies.rs b/crates/storage-postgres/src/management_store/policies.rs index 771e83350..8098cc5a7 100755 --- a/crates/storage-postgres/src/management_store/policies.rs +++ b/crates/storage-postgres/src/management_store/policies.rs @@ -9,6 +9,41 @@ use super::database_error; use crate::catalog_store::PostgresCatalogStore; use crate::pg_util::is_fk_violation; +/// Delete the inline policies and the permissions boundary stored under a +/// principal's name, inside the caller's transaction. +/// +/// `iam_policies` and `iam_permissions_boundaries` name their principal by +/// `(principal_type, principal_name)` and have no foreign key to it, so the +/// cascades that remove a principal's tags, access keys and memberships do not +/// reach them. A principal's delete calls this so that a later principal with +/// the same name does not get them, and a principal's create calls it so that +/// rows a delete left behind before this existed are removed too. +pub(super) async fn delete_principal_attachments( + conn: &mut sqlx::PgConnection, + account_id: &str, + principal_type: &str, + principal_name: &str, +) -> OpResult<()> { + for table in ["iam_policies", "iam_permissions_boundaries"] { + sqlx::query(&format!( + "DELETE FROM {table} \ + WHERE account_id = $1 AND principal_type = $2 AND principal_name = $3" + )) + .bind(account_id) + .bind(principal_type) + .bind(principal_name) + .execute(&mut *conn) + .await + .map_err(|e| { + database_error( + &format!("delete {principal_type} {principal_name}'s rows in {table}"), + &e, + ) + })?; + } + Ok(()) +} + impl PostgresCatalogStore { // ── Policies ─────────────────────────────────────────────────── diff --git a/crates/storage-postgres/src/management_store/roles.rs b/crates/storage-postgres/src/management_store/roles.rs index 8c231fef1..c6f3079fe 100755 --- a/crates/storage-postgres/src/management_store/roles.rs +++ b/crates/storage-postgres/src/management_store/roles.rs @@ -6,6 +6,7 @@ use extenddb_storage::management_store::{OpError, OpResult, RoleDetail}; use super::database_error; +use super::policies::delete_principal_attachments; use crate::catalog_store::PostgresCatalogStore; use crate::pg_util::{is_fk_violation, is_unique_violation}; @@ -17,6 +18,10 @@ impl PostgresCatalogStore { trust_policy: &serde_json::Value, ) -> OpResult<()> { let role_arn = format!("arn:aws:iam::{account_id}:role/{role_name}"); + let mut tx = self.pool().begin().await.map_err(|e| { + tracing::error!("create_role begin transaction: {e}"); + OpError::Internal("Database error".to_owned()) + })?; let result = sqlx::query( "INSERT INTO iam_roles (account_id, role_name, role_arn, trust_policy) \ VALUES ($1, $2, $3, $4)", @@ -25,37 +30,57 @@ impl PostgresCatalogStore { .bind(role_name) .bind(&role_arn) .bind(trust_policy) - .execute(self.pool()) + .execute(&mut *tx) .await; match result { - Ok(_) => Ok(()), + Ok(_) => {} Err(e) if is_unique_violation(&e) => { - Err(OpError::AlreadyExists("IAM role already exists".to_owned())) + return Err(OpError::AlreadyExists("IAM role already exists".to_owned())); + } + Err(e) if is_fk_violation(&e) => { + return Err(OpError::NotFound("Account not found".to_owned())); } - Err(e) if is_fk_violation(&e) => Err(OpError::NotFound("Account not found".to_owned())), Err(e) => { tracing::error!("create_role failed: {e}"); - Err(OpError::Internal("Database error".to_owned())) + return Err(OpError::Internal("Database error".to_owned())); } } + // A role deleted before deletes removed them could have left its + // inline policies and boundary under this name. + delete_principal_attachments(&mut tx, account_id, "role", role_name).await?; + tx.commit().await.map_err(|e| { + tracing::error!("create_role commit: {e}"); + OpError::Internal("Database error".to_owned()) + }) } pub(crate) async fn delete_role_impl(&self, account_id: &str, role_name: &str) -> OpResult<()> { + let mut tx = self.pool().begin().await.map_err(|e| { + tracing::error!("delete_role begin transaction: {e}"); + OpError::Internal("Database error".to_owned()) + })?; + // The foreign keys remove the role's tags and sessions. Its inline + // policies and boundary have none. let result = sqlx::query("DELETE FROM iam_roles WHERE account_id = $1 AND role_name = $2") .bind(account_id) .bind(role_name) - .execute(self.pool()) + .execute(&mut *tx) .await; match result { Ok(r) if r.rows_affected() == 0 => { - Err(OpError::NotFound("IAM role not found".to_owned())) + return Err(OpError::NotFound("IAM role not found".to_owned())); } - Ok(_) => Ok(()), + Ok(_) => {} Err(e) => { tracing::error!("delete_role failed: {e}"); - Err(OpError::Internal("Database error".to_owned())) + return Err(OpError::Internal("Database error".to_owned())); } } + delete_principal_attachments(&mut tx, account_id, "role", role_name).await?; + tx.commit().await.map_err(|e| { + tracing::error!("delete_role commit: {e}"); + OpError::Internal("Database error".to_owned()) + }) } pub(crate) async fn list_roles_impl( diff --git a/crates/storage-postgres/src/management_store/users.rs b/crates/storage-postgres/src/management_store/users.rs index 341f8142c..1377198b7 100755 --- a/crates/storage-postgres/src/management_store/users.rs +++ b/crates/storage-postgres/src/management_store/users.rs @@ -6,6 +6,7 @@ use extenddb_storage::management_store::{OpError, OpResult, UserDetail}; use super::database_error; +use super::policies::delete_principal_attachments; use crate::catalog_store::PostgresCatalogStore; use crate::pg_util::{is_fk_violation, is_unique_violation}; @@ -48,6 +49,10 @@ impl PostgresCatalogStore { } } + // A user deleted before deletes removed these could have left its + // inline policies and boundary under this name. + delete_principal_attachments(&mut tx, account_id, "user", user_name).await?; + // Seed default self-service policy. let self_service_policy = serde_json::json!({ "Version": "2012-10-17", @@ -86,21 +91,32 @@ impl PostgresCatalogStore { } pub(crate) async fn delete_user_impl(&self, account_id: &str, user_name: &str) -> OpResult<()> { + let mut tx = self.pool().begin().await.map_err(|e| { + tracing::error!("delete_user begin transaction: {e}"); + OpError::Internal("Database error".to_owned()) + })?; + // The foreign keys remove the user's access keys, tags and group + // memberships. Its inline policies and boundary have none. let result = sqlx::query("DELETE FROM iam_users WHERE account_id = $1 AND user_name = $2") .bind(account_id) .bind(user_name) - .execute(self.pool()) + .execute(&mut *tx) .await; match result { Ok(r) if r.rows_affected() == 0 => { - Err(OpError::NotFound("IAM user not found".to_owned())) + return Err(OpError::NotFound("IAM user not found".to_owned())); } - Ok(_) => Ok(()), + Ok(_) => {} Err(e) => { tracing::error!("delete_user failed: {e}"); - Err(OpError::Internal("Database error".to_owned())) + return Err(OpError::Internal("Database error".to_owned())); } } + delete_principal_attachments(&mut tx, account_id, "user", user_name).await?; + tx.commit().await.map_err(|e| { + tracing::error!("delete_user commit: {e}"); + OpError::Internal("Database error".to_owned()) + }) } pub(crate) async fn list_users_impl( diff --git a/crates/storage-postgres/tests/iam_leftovers.rs b/crates/storage-postgres/tests/iam_leftovers.rs new file mode 100644 index 000000000..6633ed5dc --- /dev/null +++ b/crates/storage-postgres/tests/iam_leftovers.rs @@ -0,0 +1,70 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +//! A deleted principal or account leaves nothing for one created later with +//! its name or id (`extenddb_storage::iam_leftovers_conformance`). The test +//! works in a scratch database on the server that +//! `EXTENDDB_TEST_PG_CONNECTION_STRING` names, and drops it at the end. + +use extenddb_storage_postgres::{PostgresCatalogStore, apply_all_migrations}; +use sqlx::postgres::PgPoolOptions; + +/// The server's base URL, or `None` to skip. With `EXTENDDB_TEST_PG_REQUIRED` +/// set to any value but `0`, a job that must reach a server fails here +/// instead. +fn base_conn() -> Option { + let conn = std::env::var("EXTENDDB_TEST_PG_CONNECTION_STRING").unwrap_or_default(); + if conn.is_empty() { + let required = + std::env::var("EXTENDDB_TEST_PG_REQUIRED").is_ok_and(|v| !v.is_empty() && v != "0"); + assert!( + !required, + "EXTENDDB_TEST_PG_CONNECTION_STRING is not set, and EXTENDDB_TEST_PG_REQUIRED says \ + this job must reach a PostgreSQL server" + ); + return None; + } + Some(conn.trim_end_matches('/').to_owned()) +} + +#[tokio::test] +async fn deleted_principals_and_accounts_leave_nothing() { + let Some(base) = base_conn() else { + eprintln!("SKIP: EXTENDDB_TEST_PG_CONNECTION_STRING is not set"); + return; + }; + let db_name = format!( + "eddb_leftovers_{}", + &uuid::Uuid::new_v4().simple().to_string()[..12] + ); + let admin = PgPoolOptions::new() + .max_connections(1) + .connect(&format!("{base}/postgres")) + .await + .expect("connect to the postgres maintenance database"); + sqlx::query(&format!("CREATE DATABASE \"{db_name}\"")) + .execute(&admin) + .await + .expect("create the scratch database"); + let db = PgPoolOptions::new() + .max_connections(2) + .connect(&format!("{base}/{db_name}")) + .await + .expect("connect to the scratch database"); + apply_all_migrations(&db, &db) + .await + .expect("apply the crate's migrations"); + + let store = PostgresCatalogStore::new(db.clone()); + extenddb_storage::iam_leftovers_conformance::run(&store, "123456789012").await; + + drop(store); + db.close().await; + sqlx::query(&format!( + "DROP DATABASE IF EXISTS \"{db_name}\" WITH (FORCE)" + )) + .execute(&admin) + .await + .expect("drop the scratch database"); + admin.close().await; +} diff --git a/crates/storage-sqlite/src/management_store/groups.rs b/crates/storage-sqlite/src/management_store/groups.rs index e653c81a8..8a08714eb 100644 --- a/crates/storage-sqlite/src/management_store/groups.rs +++ b/crates/storage-sqlite/src/management_store/groups.rs @@ -8,6 +8,8 @@ use extenddb_storage::management_store::{GroupDetail, GroupListEntry, OpError, O use crate::catalog_store::SqliteCatalogStore; use crate::sqlite_util::{is_fk_violation, is_unique_violation, parse_timestamp}; +use super::policies::delete_principal_attachments; + impl SqliteCatalogStore { pub(crate) async fn create_group_impl( &self, @@ -17,13 +19,20 @@ impl SqliteCatalogStore { let group_arn = format!("arn:aws:iam::{account_id}:group/{group_name}"); // D1: every writer holds the write lock. let _writer = self.lock_writes().await; + let mut tx = self + .pool() + .begin_with("BEGIN IMMEDIATE") + .await + .map_err(|e| { + tracing::error!("create_group begin: {e}"); + OpError::Internal("Database error".to_owned()) + })?; sqlx::query("INSERT INTO iam_groups (account_id, group_name, group_arn) VALUES (?, ?, ?)") .bind(account_id) .bind(group_name) .bind(&group_arn) - .execute(self.pool()) + .execute(&mut *tx) .await - .map(|_| ()) .map_err(|e| { if is_unique_violation(&e) { OpError::AlreadyExists("IAM group already exists".to_owned()) @@ -33,7 +42,14 @@ impl SqliteCatalogStore { tracing::error!("create_group: {e}"); OpError::Internal("Database error".to_owned()) } - }) + })?; + // A group deleted before deletes removed them could have left its + // inline policies under this name. + delete_principal_attachments(&mut tx, account_id, "group", group_name).await?; + tx.commit().await.map_err(|e| { + tracing::error!("create_group commit: {e}"); + OpError::Internal("Database error".to_owned()) + }) } pub(crate) async fn delete_group_impl( @@ -43,10 +59,20 @@ impl SqliteCatalogStore { ) -> OpResult<()> { // D1: every writer holds the write lock. let _writer = self.lock_writes().await; + let mut tx = self + .pool() + .begin_with("BEGIN IMMEDIATE") + .await + .map_err(|e| { + tracing::error!("delete_group begin: {e}"); + OpError::Internal("Database error".to_owned()) + })?; + // The foreign keys remove the group's memberships. Its inline + // policies have none. let result = sqlx::query("DELETE FROM iam_groups WHERE account_id = ? AND group_name = ?") .bind(account_id) .bind(group_name) - .execute(self.pool()) + .execute(&mut *tx) .await .map_err(|e| { tracing::error!("delete_group: {e}"); @@ -55,6 +81,11 @@ impl SqliteCatalogStore { if result.rows_affected() == 0 { return Err(OpError::NotFound("IAM group not found".to_owned())); } + delete_principal_attachments(&mut tx, account_id, "group", group_name).await?; + tx.commit().await.map_err(|e| { + tracing::error!("delete_group commit: {e}"); + OpError::Internal("Database error".to_owned()) + })?; Ok(()) } diff --git a/crates/storage-sqlite/src/management_store/mod.rs b/crates/storage-sqlite/src/management_store/mod.rs index 563327b41..7e1884a31 100644 --- a/crates/storage-sqlite/src/management_store/mod.rs +++ b/crates/storage-sqlite/src/management_store/mod.rs @@ -966,3 +966,26 @@ mod migration_tests { assert_eq!(names, ["b_tagged", ""]); } } + +#[cfg(test)] +mod leftover_tests { + use super::*; + use sqlx::sqlite::SqlitePoolOptions; + + #[tokio::test] + async fn deleted_principals_and_accounts_leave_nothing() { + // One connection keeps the in-memory database alive for the test. + let pool = SqlitePoolOptions::new() + .max_connections(1) + .connect("sqlite::memory:") + .await + .expect("open in-memory sqlite"); + sqlx::query("PRAGMA foreign_keys = ON") + .execute(&pool) + .await + .unwrap(); + crate::migrations::apply_for_tests(&pool).await; + let store = SqliteCatalogStore::new(pool); + extenddb_storage::iam_leftovers_conformance::run(&store, "111111111111").await; + } +} diff --git a/crates/storage-sqlite/src/management_store/policies.rs b/crates/storage-sqlite/src/management_store/policies.rs index 1cc31e1c4..ead25f428 100644 --- a/crates/storage-sqlite/src/management_store/policies.rs +++ b/crates/storage-sqlite/src/management_store/policies.rs @@ -9,6 +9,39 @@ use time::OffsetDateTime; use crate::catalog_store::SqliteCatalogStore; use crate::sqlite_util::parse_timestamp; +/// Delete the inline policies and the permissions boundary stored under a +/// principal's name, inside the caller's transaction. +/// +/// `iam_policies` and `iam_permissions_boundaries` name their principal by +/// `(principal_type, principal_name)` and have no foreign key to it, so the +/// cascades that remove a principal's tags, access keys and memberships do not +/// reach them. A principal's delete calls this so that a later principal with +/// the same name does not get them, and a principal's create calls it so that +/// rows a delete left behind before this existed are removed too. +pub(super) async fn delete_principal_attachments( + conn: &mut sqlx::SqliteConnection, + account_id: &str, + principal_type: &str, + principal_name: &str, +) -> OpResult<()> { + for table in ["iam_policies", "iam_permissions_boundaries"] { + sqlx::query(&format!( + "DELETE FROM {table} \ + WHERE account_id = ? AND principal_type = ? AND principal_name = ?" + )) + .bind(account_id) + .bind(principal_type) + .bind(principal_name) + .execute(&mut *conn) + .await + .map_err(|e| { + tracing::error!("delete {principal_type} {principal_name}'s rows in {table}: {e}"); + OpError::Internal("Database error".to_owned()) + })?; + } + Ok(()) +} + impl SqliteCatalogStore { pub(crate) async fn put_policy_impl( &self, diff --git a/crates/storage-sqlite/src/management_store/roles.rs b/crates/storage-sqlite/src/management_store/roles.rs index b3d81582b..c94750c81 100644 --- a/crates/storage-sqlite/src/management_store/roles.rs +++ b/crates/storage-sqlite/src/management_store/roles.rs @@ -11,6 +11,8 @@ use extenddb_storage::management_store::{OpError, OpResult, RoleDetail, RoleList use crate::catalog_store::SqliteCatalogStore; use crate::sqlite_util::{is_fk_violation, is_unique_violation, parse_timestamp}; +use super::policies::delete_principal_attachments; + /// Parse a stored JSON-text column into a `serde_json::Value`. fn parse_json(s: &str, ctx: &str) -> OpResult { serde_json::from_str(s).map_err(|e| OpError::Internal(format!("{ctx} parse json: {e}"))) @@ -28,6 +30,14 @@ impl SqliteCatalogStore { .map_err(|e| OpError::Internal(format!("serialize trust policy: {e}")))?; // D1: every writer holds the write lock. let _writer = self.lock_writes().await; + let mut tx = self + .pool() + .begin_with("BEGIN IMMEDIATE") + .await + .map_err(|e| { + tracing::error!("create_role begin: {e}"); + OpError::Internal("Database error".to_owned()) + })?; sqlx::query( "INSERT INTO iam_roles (account_id, role_name, role_arn, trust_policy) \ VALUES (?, ?, ?, ?)", @@ -36,9 +46,8 @@ impl SqliteCatalogStore { .bind(role_name) .bind(&role_arn) .bind(&trust) - .execute(self.pool()) + .execute(&mut *tx) .await - .map(|_| ()) .map_err(|e| { if is_unique_violation(&e) { OpError::AlreadyExists("IAM role already exists".to_owned()) @@ -48,16 +57,33 @@ impl SqliteCatalogStore { tracing::error!("create_role: {e}"); OpError::Internal("Database error".to_owned()) } + })?; + // A role deleted before deletes removed them could have left its + // inline policies and boundary under this name. + delete_principal_attachments(&mut tx, account_id, "role", role_name).await?; + tx.commit().await.map_err(|e| { + tracing::error!("create_role commit: {e}"); + OpError::Internal("Database error".to_owned()) }) } pub(crate) async fn delete_role_impl(&self, account_id: &str, role_name: &str) -> OpResult<()> { // D1: every writer holds the write lock. let _writer = self.lock_writes().await; + let mut tx = self + .pool() + .begin_with("BEGIN IMMEDIATE") + .await + .map_err(|e| { + tracing::error!("delete_role begin: {e}"); + OpError::Internal("Database error".to_owned()) + })?; + // The foreign keys remove the role's tags and sessions. Its inline + // policies and boundary have none. let result = sqlx::query("DELETE FROM iam_roles WHERE account_id = ? AND role_name = ?") .bind(account_id) .bind(role_name) - .execute(self.pool()) + .execute(&mut *tx) .await .map_err(|e| { tracing::error!("delete_role: {e}"); @@ -66,6 +92,11 @@ impl SqliteCatalogStore { if result.rows_affected() == 0 { return Err(OpError::NotFound("IAM role not found".to_owned())); } + delete_principal_attachments(&mut tx, account_id, "role", role_name).await?; + tx.commit().await.map_err(|e| { + tracing::error!("delete_role commit: {e}"); + OpError::Internal("Database error".to_owned()) + })?; Ok(()) } diff --git a/crates/storage-sqlite/src/management_store/users.rs b/crates/storage-sqlite/src/management_store/users.rs index 69c9fd6c7..f892d4a67 100644 --- a/crates/storage-sqlite/src/management_store/users.rs +++ b/crates/storage-sqlite/src/management_store/users.rs @@ -9,6 +9,8 @@ use extenddb_storage::management_store::{OpError, OpResult, UserDetail, UserList use crate::catalog_store::SqliteCatalogStore; use crate::sqlite_util::{is_fk_violation, is_unique_violation, parse_timestamp}; +use super::policies::delete_principal_attachments; + impl SqliteCatalogStore { pub(crate) async fn create_user_impl( &self, @@ -51,6 +53,10 @@ impl SqliteCatalogStore { } })?; + // A user deleted before deletes removed these could have left its + // inline policies and boundary under this name. + delete_principal_attachments(&mut tx, account_id, "user", user_name).await?; + // Seed a default self-service policy so the user can manage its own // access keys and password without an administrator attaching one // (parity with the PostgreSQL backend). @@ -94,10 +100,20 @@ impl SqliteCatalogStore { pub(crate) async fn delete_user_impl(&self, account_id: &str, user_name: &str) -> OpResult<()> { // D1: every writer holds the write lock. let _writer = self.lock_writes().await; + let mut tx = self + .pool() + .begin_with("BEGIN IMMEDIATE") + .await + .map_err(|e| { + tracing::error!("delete_user begin: {e}"); + OpError::Internal("Database error".to_owned()) + })?; + // The foreign keys remove the user's access keys, tags and group + // memberships. Its inline policies and boundary have none. let result = sqlx::query("DELETE FROM iam_users WHERE account_id = ? AND user_name = ?") .bind(account_id) .bind(user_name) - .execute(self.pool()) + .execute(&mut *tx) .await .map_err(|e| { tracing::error!("delete_user: {e}"); @@ -106,6 +122,11 @@ impl SqliteCatalogStore { if result.rows_affected() == 0 { return Err(OpError::NotFound("IAM user not found".to_owned())); } + delete_principal_attachments(&mut tx, account_id, "user", user_name).await?; + tx.commit().await.map_err(|e| { + tracing::error!("delete_user commit: {e}"); + OpError::Internal("Database error".to_owned()) + })?; Ok(()) } diff --git a/crates/storage/Cargo.toml b/crates/storage/Cargo.toml index a024143df..cca2692a9 100755 --- a/crates/storage/Cargo.toml +++ b/crates/storage/Cargo.toml @@ -12,7 +12,9 @@ license.workspace = true # backend crate runs on its migration table, and # `password::count_verifications`, which returns the bcrypt cost of each check # a login ran. It also compiles the counter in `verify_password` that records -# those costs. Production code never enables it. +# those costs, and `iam_leftovers_conformance`, the check each backend's tests +# run that a deleted principal or account leaves nothing behind. Production +# code never enables it. test-util = [] # Compiles the `snapshot_conformance` check that each backend's tests run # against a real database. Production builds do not compile it. diff --git a/crates/storage/src/iam_leftovers_conformance.rs b/crates/storage/src/iam_leftovers_conformance.rs new file mode 100644 index 000000000..fdd5e9401 --- /dev/null +++ b/crates/storage/src/iam_leftovers_conformance.rs @@ -0,0 +1,307 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +//! Backend-independent check that a deleted principal or account leaves +//! nothing for one created later with its name or id (#693, #694), run by +//! each backend's own test suite against a real database. +//! +//! Inline policies and permissions boundaries are stored by principal name. +//! No backend links them to their principal, and MongoDB and Cassandra link +//! no IAM record to its account. The check: +//! +//! - deletes a user, a group and a role that hold policies and boundaries, +//! and reads that none of them is left under its name; +//! - stores policies and boundaries under names that have no principal, as a +//! delete before these fixes left them, creates principals with those +//! names, and reads that each starts empty (a user with its +//! `SelfServicePolicy` only); +//! - creates a principal that already exists, and reads that the refusal +//! left its policies and boundary in place; +//! - deletes an account that holds a user, a group and a role with policies, +//! creates an account with the same id, and reads that it has no +//! principals and no policies under the old names. +//! +//! The store methods do not check that a principal exists before a put +//! (the management API does), which is what lets the check store rows under +//! a name with no principal. + +use serde_json::{Value, json}; + +use crate::management_store::{ManagementStore, OpError}; + +fn allow(action: &str) -> Value { + json!({ + "Version": "2012-10-17", + "Statement": [{"Effect": "Allow", "Action": action, "Resource": "*"}], + }) +} + +fn trust(account_id: &str) -> Value { + json!({ + "Version": "2012-10-17", + "Statement": [{ + "Effect": "Allow", + "Principal": {"AWS": format!("arn:aws:iam::{account_id}:root")}, + "Action": "sts:AssumeRole", + }], + }) +} + +async fn policy_names( + store: &dyn ManagementStore, + account_id: &str, + principal_type: &str, + name: &str, +) -> Vec { + let mut names: Vec = store + .list_policies(account_id, principal_type, name) + .await + .unwrap_or_else(|e| panic!("list {principal_type} {name}'s policies: {e:?}")) + .into_iter() + .map(|(policy_name, _, _)| policy_name) + .collect(); + names.sort(); + names +} + +async fn boundary( + store: &dyn ManagementStore, + account_id: &str, + principal_type: &str, + name: &str, +) -> Option { + match principal_type { + "user" => store.get_user_boundary(account_id, name).await, + "role" => store.get_role_boundary(account_id, name).await, + other => unreachable!("principal type {other}"), + } + .unwrap_or_else(|e| panic!("get {principal_type} {name}'s boundary: {e:?}")) +} + +async fn put(store: &dyn ManagementStore, account_id: &str, principal_type: &str, name: &str) { + store + .put_policy( + account_id, + principal_type, + name, + "old", + &allow("dynamodb:ListTables"), + ) + .await + .unwrap_or_else(|e| panic!("put {principal_type} {name}'s policy: {e:?}")); + let set = match principal_type { + "user" => { + store + .set_user_boundary(account_id, name, &allow("dynamodb:*")) + .await + } + "role" => { + store + .set_role_boundary(account_id, name, &allow("dynamodb:*")) + .await + } + _ => Ok(()), + }; + set.unwrap_or_else(|e| panic!("set {principal_type} {name}'s boundary: {e:?}")); +} + +async fn create(store: &dyn ManagementStore, account_id: &str, principal_type: &str, name: &str) { + let created = match principal_type { + "user" => store.create_user(account_id, name, None).await, + "group" => store.create_group(account_id, name).await, + "role" => { + store + .create_role(account_id, name, &trust(account_id)) + .await + } + other => unreachable!("principal type {other}"), + }; + created.unwrap_or_else(|e| panic!("create {principal_type} {name}: {e:?}")); +} + +async fn delete(store: &dyn ManagementStore, account_id: &str, principal_type: &str, name: &str) { + let deleted = match principal_type { + "user" => store.delete_user(account_id, name).await, + "group" => store.delete_group(account_id, name).await, + "role" => store.delete_role(account_id, name).await, + other => unreachable!("principal type {other}"), + }; + deleted.unwrap_or_else(|e| panic!("delete {principal_type} {name}: {e:?}")); +} + +/// What a principal of `principal_type` has when it starts empty. +fn fresh_policies(principal_type: &str) -> Vec { + if principal_type == "user" { + vec!["SelfServicePolicy".to_owned()] + } else { + Vec::new() + } +} + +/// Assert that `name` has no policies (beyond a new user's) and no boundary. +async fn assert_empty( + store: &dyn ManagementStore, + account_id: &str, + principal_type: &str, + name: &str, + expected_policies: &[String], + when: &str, +) { + assert_eq!( + policy_names(store, account_id, principal_type, name).await, + expected_policies, + "{when}: {principal_type} {name}'s policies" + ); + if principal_type != "group" { + assert_eq!( + boundary(store, account_id, principal_type, name).await, + None, + "{when}: {principal_type} {name}'s boundary" + ); + } +} + +/// Run the check in account `account_id`, which must not exist yet. The +/// account is deleted at the end; a failure leaves it for the caller's +/// cleanup. +/// +/// # Panics +/// +/// Panics on the first store error or wrong answer. +pub async fn run(store: &dyn ManagementStore, account_id: &str) { + store + .create_account(account_id, &format!("leftovers-{account_id}")) + .await + .expect("create the account"); + + for principal_type in ["user", "group", "role"] { + // A delete removes what the principal holds. + let name = format!("deleted-{principal_type}"); + create(store, account_id, principal_type, &name).await; + put(store, account_id, principal_type, &name).await; + delete(store, account_id, principal_type, &name).await; + assert_empty( + store, + account_id, + principal_type, + &name, + &[], + "after its delete", + ) + .await; + + // A create clears rows a delete before these fixes left. + let name = format!("left-{principal_type}"); + put(store, account_id, principal_type, &name).await; + create(store, account_id, principal_type, &name).await; + assert_empty( + store, + account_id, + principal_type, + &name, + &fresh_policies(principal_type), + "created over rows left under its name", + ) + .await; + + // A create of a principal that exists clears nothing. + let name = format!("live-{principal_type}"); + create(store, account_id, principal_type, &name).await; + put(store, account_id, principal_type, &name).await; + let again = match principal_type { + "user" => store.create_user(account_id, &name, None).await, + "group" => store.create_group(account_id, &name).await, + _ => { + store + .create_role(account_id, &name, &trust(account_id)) + .await + } + }; + assert!( + matches!(again, Err(OpError::AlreadyExists(_))), + "create {principal_type} {name} again: {again:?}" + ); + let mut kept = fresh_policies(principal_type); + kept.insert(0, "old".to_owned()); + kept.sort(); + assert_eq!( + policy_names(store, account_id, principal_type, &name).await, + kept, + "{principal_type} {name}'s policies after a refused create" + ); + if principal_type != "group" { + assert!( + boundary(store, account_id, principal_type, &name) + .await + .is_some(), + "{principal_type} {name}'s boundary after a refused create" + ); + } + } + + // An account's delete removes its principals and what they hold. Its + // tables go first; the check creates none. + for principal_type in ["user", "group", "role"] { + let name = format!("in-account-{principal_type}"); + create(store, account_id, principal_type, &name).await; + put(store, account_id, principal_type, &name).await; + } + for (principal_type, name) in [ + ("user", "left-user"), + ("user", "live-user"), + ("group", "left-group"), + ("group", "live-group"), + ("role", "left-role"), + ("role", "live-role"), + ] { + delete(store, account_id, principal_type, name).await; + } + store + .delete_account(account_id) + .await + .expect("delete the account"); + store + .create_account(account_id, &format!("leftovers-again-{account_id}")) + .await + .expect("create the account again"); + assert!( + store + .list_users(account_id) + .await + .expect("list users") + .is_empty(), + "users of the account created again" + ); + assert!( + store + .list_groups(account_id) + .await + .expect("list groups") + .is_empty(), + "groups of the account created again" + ); + assert!( + store + .list_roles(account_id) + .await + .expect("list roles") + .is_empty(), + "roles of the account created again" + ); + for principal_type in ["user", "group", "role"] { + let name = format!("in-account-{principal_type}"); + assert_empty( + store, + account_id, + principal_type, + &name, + &[], + "in the account created again", + ) + .await; + } + store + .delete_account(account_id) + .await + .expect("delete the account again"); +} diff --git a/crates/storage/src/lib.rs b/crates/storage/src/lib.rs index 96bed67b0..a989ec510 100755 --- a/crates/storage/src/lib.rs +++ b/crates/storage/src/lib.rs @@ -17,6 +17,9 @@ pub mod diagnostics; pub mod diagnostics_store; pub mod error; pub mod hooks; +#[doc(hidden)] +#[cfg(feature = "test-util")] +pub mod iam_leftovers_conformance; pub mod management_store; pub mod management_store_factory; pub mod migrations; diff --git a/devtools/run-mongodb-tests b/devtools/run-mongodb-tests index 57eaba215..7baa06182 100755 --- a/devtools/run-mongodb-tests +++ b/devtools/run-mongodb-tests @@ -444,6 +444,20 @@ if $RUST_LIVE; then RC=1 fi + # A deleted principal or account leaves nothing behind (#693, #694): the + # shared `iam_leftovers_conformance` check, and a deleted account's user + # and role granting nothing. Required, like the backup engine's. + echo "" + echo "=== MongoDB IAM delete live Rust tests (storage-mongodb, --ignored) ===" + if EXTENDDB_TEST_MONGODB_REQUIRED=1 \ + cargo test -p extenddb-storage-mongodb --lib -- \ + --ignored --test-threads=1 management_store::live_tests::; then + echo " ✓ MongoDB IAM delete live Rust tests passed" + else + echo " ✗ MongoDB IAM delete live Rust tests failed" + RC=1 + fi + # The restore fence cells need the `test-hooks` feature (the GSI backfill # gate `bulk_load` honours is compiled only under it), and no other job From f97ce9cacbca43fa5406ac1dea22df457515a0d1 Mon Sep 17 00:00:00 2001 From: Hitesh Sharma Date: Mon, 9 Nov 2026 17:13:58 -0800 Subject: [PATCH 2/5] fix(management): refuse a policy or boundary on a missing principal Putting an inline policy or a permissions boundary on a user, group or role that does not exist answered 204 and stored it under the name, so it waited for the next principal created with that name. The management API and the console now answer 404 "IAM not found", as IAM answers NoSuchEntity. A create also clears such rows, which covers a put that races a delete. --- .../server/src/console/pages/policy_pages.rs | 32 +++++++++---- crates/server/src/management/iam_policy.rs | 48 +++++++++++++++++++ crates/server/src/management/mod.rs | 1 + .../src/management/permissions_boundary.rs | 11 +++++ 4 files changed, 82 insertions(+), 10 deletions(-) diff --git a/crates/server/src/console/pages/policy_pages.rs b/crates/server/src/console/pages/policy_pages.rs index 9c7634cf4..a57198f58 100755 --- a/crates/server/src/console/pages/policy_pages.rs +++ b/crates/server/src/console/pages/policy_pages.rs @@ -326,17 +326,29 @@ async fn put_policy( .into_response(); } - match state - .catalog_store - .put_policy( - account_id, - principal_type, - principal_name, - &form.policy_name, - &doc, - ) - .await + let stored = match crate::management::require_principal( + &*state.catalog_store, + account_id, + principal_type, + principal_name, + ) + .await { + Ok(()) => { + state + .catalog_store + .put_policy( + account_id, + principal_type, + principal_name, + &form.policy_name, + &doc, + ) + .await + } + Err(e) => Err(e), + }; + match stored { Ok(()) => { invalidate_policy_caches(state, account_id, principal_type, principal_name).await; Redirect::to(redirect_url).into_response() diff --git a/crates/server/src/management/iam_policy.rs b/crates/server/src/management/iam_policy.rs index 8e5de48c6..b863863a5 100755 --- a/crates/server/src/management/iam_policy.rs +++ b/crates/server/src/management/iam_policy.rs @@ -23,6 +23,43 @@ struct PolicyEntry { created_at: String, } +/// `NotFound` unless the principal exists, as IAM answers `NoSuchEntity` for a +/// policy or boundary put on a principal it does not have. +/// +/// Inline policies and boundaries are stored by principal name, so a put on a +/// name with no principal would wait there and belong to the next principal +/// created with that name. Its create removes such rows too, which covers a +/// put that races a delete. +pub(crate) async fn require_principal( + store: &dyn extenddb_storage::CatalogStore, + account_id: &str, + principal_type: &str, + principal_name: &str, +) -> Result<(), extenddb_storage::management_store::OpError> { + let exists = match principal_type { + "user" => store + .get_user_detail(account_id, principal_name) + .await? + .is_some(), + "group" => store + .get_group_detail(account_id, principal_name) + .await? + .is_some(), + "role" => store + .get_role_detail(account_id, principal_name) + .await? + .is_some(), + other => unreachable!("principal type {other}"), + }; + if exists { + Ok(()) + } else { + Err(extenddb_storage::management_store::OpError::NotFound( + format!("IAM {principal_type} not found"), + )) + } +} + pub async fn put_user_policy( State(state): State>, headers: HeaderMap, @@ -211,6 +248,17 @@ async fn put_policy( } } + if let Err(e) = require_principal( + &*state.catalog_store, + account_id, + principal_type, + principal_name, + ) + .await + { + return op_err_to_response(OpError::from_storage(e)); + } + match state .catalog_store .put_policy( diff --git a/crates/server/src/management/mod.rs b/crates/server/src/management/mod.rs index ec3968766..2e5975da0 100755 --- a/crates/server/src/management/mod.rs +++ b/crates/server/src/management/mod.rs @@ -21,6 +21,7 @@ mod cache_metrics; pub(crate) mod crypto; mod iam_group; mod iam_policy; +pub(crate) use iam_policy::require_principal; mod iam_role; mod iam_user; mod iam_user_self; diff --git a/crates/server/src/management/permissions_boundary.rs b/crates/server/src/management/permissions_boundary.rs index dd34bcad6..6e55b4285 100755 --- a/crates/server/src/management/permissions_boundary.rs +++ b/crates/server/src/management/permissions_boundary.rs @@ -137,6 +137,17 @@ async fn set_boundary( } } + if let Err(e) = super::iam_policy::require_principal( + &*state.catalog_store, + account_id, + principal_type, + principal_name, + ) + .await + { + return op_err_to_response(OpError::from_storage(e)); + } + let result = match principal_type { "user" => { state From a826d9615cbb0ece8877230ff86caa1eb504720c Mon Sep 17 00:00:00 2001 From: Hitesh Sharma Date: Mon, 9 Nov 2026 17:13:58 -0800 Subject: [PATCH 3/5] test: check that a deleted principal or account leaves nothing behind Each test gives a role, user, group or account everything it can own, deletes it, checks that its credentials stop working, creates it again with the same name or id, and checks that it starts empty. Two more check that a policy or boundary put on a missing principal is refused with 404. --- tests/test_iam_delete_leftovers.py | 317 +++++++++++++++++++++++++++++ 1 file changed, 317 insertions(+) create mode 100644 tests/test_iam_delete_leftovers.py diff --git a/tests/test_iam_delete_leftovers.py b/tests/test_iam_delete_leftovers.py new file mode 100644 index 000000000..7911a893c --- /dev/null +++ b/tests/test_iam_delete_leftovers.py @@ -0,0 +1,317 @@ +# Copyright 2026 ExtendDB contributors +# SPDX-License-Identifier: Apache-2.0 + +"""A deleted principal or account leaves nothing for one created later with +its name or id (#693, #694). + +Inline policies and permissions boundaries are stored by principal name, and +on MongoDB and Cassandra nothing links any IAM record to its principal or +account. Each test gives a principal or an account everything it can own, +deletes it, checks that its credentials stop working, creates it again with +the same name or id, and checks that the new one starts empty and is refused. + +The last test checks that a policy or boundary cannot be put on a principal +that does not exist, as IAM answers ``NoSuchEntity``. + +Prerequisites: a running extenddb with ``auth.provider = "builtin"`` on +EXTENDDB_TEST_ENDPOINT, and admin credentials in EXTENDDB_ADMIN_USER / +EXTENDDB_ADMIN_PASSWORD. The tests use the management API, which Amazon +DynamoDB does not have. +""" + +from __future__ import annotations + +import os +import uuid +from typing import Any + +import boto3 +import pytest +import requests +from botocore.config import Config as BotoConfig +from botocore.exceptions import ClientError + +from management_helpers import ManagementClient + + +def _require_auth_env() -> tuple[str, str, str]: + endpoint = os.environ.get("EXTENDDB_TEST_ENDPOINT", "").strip() + admin_user = os.environ.get("EXTENDDB_ADMIN_USER", "").strip() + admin_pass = os.environ.get("EXTENDDB_ADMIN_PASSWORD", "").strip() + if not endpoint or not admin_user or not admin_pass: + pytest.fail( + "MISCONFIGURED: these tests require EXTENDDB_TEST_ENDPOINT, " + "EXTENDDB_ADMIN_USER and EXTENDDB_ADMIN_PASSWORD." + ) + return endpoint, admin_user, admin_pass + + +@pytest.fixture(scope="module") +def endpoint() -> str: + return _require_auth_env()[0] + + +@pytest.fixture(scope="module") +def mgmt() -> ManagementClient: + endpoint, admin_user, admin_pass = _require_auth_env() + return ManagementClient(endpoint, admin_user, admin_pass) + + +@pytest.fixture +def account_id(mgmt): + """A new account, deleted on teardown with whatever is left in it.""" + acct = f"{uuid.uuid4().int % 10**12:012d}" + resp = mgmt.create_account(acct, f"leftovers-{acct}") + assert resp.status_code == 201, resp.text + yield acct + for user in _names(mgmt.list_users(acct), "user_name"): + mgmt.delete_user(acct, user) + for group in _names(mgmt.list_groups(acct), "group_name"): + mgmt.delete_group(acct, group) + for role in _names(mgmt.list_roles(acct), "role_name"): + mgmt.delete_role(acct, role) + mgmt.delete_account(acct) + + +def _names(resp: requests.Response, field: str) -> list[str]: + return [entry[field] for entry in resp.json()] if resp.status_code == 200 else [] + + +def _unique(prefix: str) -> str: + return f"{prefix}-{uuid.uuid4().hex[:8]}" + + +def _allow(*actions: str) -> dict: + return { + "Version": "2012-10-17", + "Statement": [{"Effect": "Allow", "Action": list(actions), "Resource": "*"}], + } + + +def _ddb(endpoint: str, creds: dict) -> Any: + return boto3.client( + "dynamodb", + endpoint_url=endpoint, + aws_access_key_id=creds["access_key_id"], + aws_secret_access_key=creds["secret_access_key"], + aws_session_token=creds.get("session_token"), + config=BotoConfig( + region_name=os.environ.get("AWS_DEFAULT_REGION", "us-east-1"), + signature_version="v4", + retries={"max_attempts": 0, "mode": "standard"}, + ), + verify=False, # local TLS uses a self-signed cert + ) + + +def _error_code(call) -> str: + """The error code `call` raises, or ``OK`` if it succeeds.""" + try: + call() + except ClientError as e: + return e.response["Error"]["Code"] + return "OK" + + +def _unknown_credential(call) -> bool: + """Whether `call` is refused because its credential is unknown.""" + return _error_code(call) in ("UnrecognizedClientException", "InvalidSignatureException") + + +def _access_key(mgmt: ManagementClient, account_id: str, user: str) -> dict: + resp = mgmt.create_access_key(account_id, user) + assert resp.status_code == 201, resp.text + return resp.json() + + +def _trust(account_id: str) -> dict: + return { + "Version": "2012-10-17", + "Statement": [{ + "Effect": "Allow", + "Principal": {"AWS": f"arn:aws:iam::{account_id}:root"}, + "Action": "sts:AssumeRole", + }], + } + + +def _session(mgmt: ManagementClient, account_id: str, role: str) -> dict: + resp = mgmt.assume_role(account_id, role, f"arn:aws:iam::{account_id}:root", _unique("s")) + assert resp.status_code == 201, resp.text + return resp.json() + + +def _tags(mgmt: ManagementClient, account_id: str, kind: str, name: str) -> list: + resp = requests.get( + f"{mgmt.base_url}/accounts/{account_id}/{kind}s/{name}/tags", + auth=mgmt.admin_auth, + timeout=mgmt.timeout, + verify=mgmt.verify, + ) + assert resp.status_code == 200, resp.text + return resp.json() + + +def _policy_names(resp: requests.Response) -> list[str]: + assert resp.status_code == 200, resp.text + return sorted(p["policy_name"] for p in resp.json()) + + +def _ok(resp: requests.Response, *codes: int) -> None: + assert resp.status_code in (codes or (200, 201, 204)), resp.text + + +class TestDeletedPrincipalLeavesNothing: + def test_role(self, endpoint, mgmt, account_id): + role = _unique("role") + _ok(mgmt.create_role(account_id, role, _trust(account_id)), 201) + _ok(mgmt.put_role_policy(account_id, role, "old", _allow("dynamodb:ListTables"))) + _ok(mgmt.set_role_boundary(account_id, role, _allow("dynamodb:*"))) + _ok(mgmt.tag_role(account_id, role, {"team": "old"})) + old_session = _ddb(endpoint, _session(mgmt, account_id, role)) + assert _error_code(old_session.list_tables) == "OK" + + _ok(mgmt.delete_role(account_id, role), 204) + assert _unknown_credential(old_session.list_tables) + + _ok(mgmt.create_role(account_id, role, _trust(account_id)), 201) + assert _policy_names(mgmt.list_role_policies(account_id, role)) == [] + assert mgmt.get_role_boundary(account_id, role).status_code == 404 + assert _tags(mgmt, account_id, "role", role) == [] + assert _unknown_credential(old_session.list_tables) + new_session = _ddb(endpoint, _session(mgmt, account_id, role)) + assert _error_code(new_session.list_tables) == "AccessDeniedException" + + def test_user(self, endpoint, mgmt, account_id): + user, group = _unique("user"), _unique("group") + _ok(mgmt.create_user(account_id, user), 201) + _ok(mgmt.put_user_policy(account_id, user, "old", _allow("dynamodb:ListTables"))) + _ok(mgmt.set_user_boundary(account_id, user, _allow("dynamodb:*"))) + _ok(mgmt.tag_user(account_id, user, {"team": "old"})) + _ok(mgmt.create_group(account_id, group), 201) + _ok(mgmt.put_group_policy(account_id, group, "g", _allow("dynamodb:DescribeLimits"))) + _ok(mgmt.add_group_member(account_id, group, user)) + old_key = _ddb(endpoint, _access_key(mgmt, account_id, user)) + assert _error_code(old_key.list_tables) == "OK" + assert _error_code(old_key.describe_limits) == "OK" + + _ok(mgmt.delete_user(account_id, user), 204) + assert _unknown_credential(old_key.list_tables) + + _ok(mgmt.create_user(account_id, user), 201) + # Every new user gets the self-service policy, and nothing else. + assert _policy_names(mgmt.list_user_policies(account_id, user)) == ["SelfServicePolicy"] + assert mgmt.get_user_boundary(account_id, user).status_code == 404 + assert _tags(mgmt, account_id, "user", user) == [] + resp = mgmt.list_access_keys(account_id, user) + assert resp.status_code == 200 and resp.json() == [], resp.text + assert _unknown_credential(old_key.list_tables) + new_key = _ddb(endpoint, _access_key(mgmt, account_id, user)) + assert _error_code(new_key.list_tables) == "AccessDeniedException" + # The old user's membership is gone with it: the group's policy no + # longer applies. + assert _error_code(new_key.describe_limits) == "AccessDeniedException" + + def test_group(self, endpoint, mgmt, account_id): + group, member = _unique("group"), _unique("member") + _ok(mgmt.create_group(account_id, group), 201) + _ok(mgmt.put_group_policy(account_id, group, "old", _allow("dynamodb:ListTables"))) + _ok(mgmt.create_user(account_id, member), 201) + _ok(mgmt.add_group_member(account_id, group, member)) + key = _ddb(endpoint, _access_key(mgmt, account_id, member)) + assert _error_code(key.list_tables) == "OK" + + _ok(mgmt.delete_group(account_id, group), 204) + assert _error_code(key.list_tables) == "AccessDeniedException" + + _ok(mgmt.create_group(account_id, group), 201) + assert _policy_names(mgmt.list_group_policies(account_id, group)) == [] + # The member is not a member of the new group, and has no policy + # that allows ListTables from it. + assert _error_code(key.list_tables) == "AccessDeniedException" + _ok(mgmt.put_group_policy(account_id, group, "new", _allow("dynamodb:ListTables"))) + assert _error_code(key.list_tables) == "AccessDeniedException" + + +class TestDeletedAccountLeavesNothing: + def test_account(self, endpoint, mgmt): + account_id = f"{uuid.uuid4().int % 10**12:012d}" + user, role, password = _unique("user"), _unique("role"), uuid.uuid4().hex + _ok(mgmt.create_account(account_id, f"leftovers-{account_id}"), 201) + try: + _ok(mgmt.create_user(account_id, user, password), 201) + _ok(mgmt.put_user_policy(account_id, user, "old", _allow("dynamodb:ListTables"))) + key = _ddb(endpoint, _access_key(mgmt, account_id, user)) + _ok(mgmt.create_role(account_id, role, _trust(account_id)), 201) + _ok(mgmt.put_role_policy(account_id, role, "old", _allow("dynamodb:ListTables"))) + session = _ddb(endpoint, _session(mgmt, account_id, role)) + group = _unique("group") + _ok(mgmt.create_group(account_id, group), 201) + assert _error_code(key.list_tables) == "OK" + assert _error_code(session.list_tables) == "OK" + self_service = requests.get( + f"{mgmt.base_url}/accounts/{account_id}/users/{user}/access-keys", + auth=(f"{account_id}/{user}", password), + timeout=mgmt.timeout, + verify=mgmt.verify, + ) + assert self_service.status_code == 200, self_service.text + + _ok(mgmt.delete_account(account_id), 204) + + assert _unknown_credential(key.list_tables) + assert _unknown_credential(session.list_tables) + self_service = requests.get( + f"{mgmt.base_url}/accounts/{account_id}/users/{user}/access-keys", + auth=(f"{account_id}/{user}", password), + timeout=mgmt.timeout, + verify=mgmt.verify, + ) + assert self_service.status_code == 401, self_service.text + login = requests.post( + f"{endpoint.rstrip('/')}/console/login", + data={"username": f"{account_id}/{user}", "password": password}, + allow_redirects=False, + timeout=30, + verify=mgmt.verify, + ) + # A successful login answers 303 to the dashboard. + assert login.status_code != 303, login.headers + + _ok(mgmt.create_account(account_id, f"leftovers-again-{account_id}"), 201) + assert _names(mgmt.list_users(account_id), "user_name") == [] + assert _names(mgmt.list_groups(account_id), "group_name") == [] + assert _names(mgmt.list_roles(account_id), "role_name") == [] + assert _unknown_credential(key.list_tables) + assert _unknown_credential(session.list_tables) + # A principal created again in the new account starts empty too. + _ok(mgmt.create_role(account_id, role, _trust(account_id)), 201) + assert _policy_names(mgmt.list_role_policies(account_id, role)) == [] + _ok(mgmt.create_user(account_id, user), 201) + assert _policy_names(mgmt.list_user_policies(account_id, user)) == [ + "SelfServicePolicy" + ] + finally: + mgmt.delete_user(account_id, user) + mgmt.delete_role(account_id, role) + mgmt.delete_account(account_id) + + +class TestAttachToMissingPrincipal: + @pytest.mark.parametrize("kind", ["user", "group", "role"]) + def test_inline_policy_is_refused(self, mgmt, account_id, kind): + name = _unique(kind) + put = getattr(mgmt, f"put_{kind}_policy") + resp = put(account_id, name, "early", _allow("dynamodb:ListTables")) + assert resp.status_code == 404, resp.text + assert resp.text == f"IAM {kind} not found" + resp = getattr(mgmt, f"list_{kind}_policies")(account_id, name) + assert resp.status_code != 200 or resp.json() == [], resp.text + + @pytest.mark.parametrize("kind", ["user", "role"]) + def test_boundary_is_refused(self, mgmt, account_id, kind): + name = _unique(kind) + resp = getattr(mgmt, f"set_{kind}_boundary")(account_id, name, _allow("dynamodb:*")) + assert resp.status_code == 404, resp.text + assert resp.text == f"IAM {kind} not found" + assert getattr(mgmt, f"get_{kind}_boundary")(account_id, name).status_code == 404 From 88698a4a70e8227ebb42a88aea6879ebeef8f9a9 Mon Sep 17 00:00:00 2001 From: Hitesh Sharma Date: Mon, 9 Nov 2026 17:13:58 -0800 Subject: [PATCH 4/5] docs: say what a delete removes, and the 404 for a missing principal The getting-started and admin guides list what a user, group, role or account delete removes. The upgrade manual has a v0.1.15 entry, the differences document compares the delete and the put with IAM's DeleteConflict and NoSuchEntity, and the storage guide states the cascade rules a backend must follow. --- docs/differences-from-dynamodb.md | 2 + docs/getting-started.md | 10 ++++- docs/manuals/05-admin-guide.md | 11 +++-- docs/manuals/07-upgrade-manual.md | 40 +++++++++++++++++++ docs/manuals/12-extending-extenddb-storage.md | 30 +++++++++++++- 5 files changed, 85 insertions(+), 8 deletions(-) diff --git a/docs/differences-from-dynamodb.md b/docs/differences-from-dynamodb.md index 29a92ce1e..7db86fb48 100755 --- a/docs/differences-from-dynamodb.md +++ b/docs/differences-from-dynamodb.md @@ -31,6 +31,8 @@ from a request sent to that service or backend. | Role chaining | Supported | Not implemented | | SourceIdentity, TransitiveTagKeys | Supported | Not implemented | | Resource policies | Supported | Not implemented (deferred) | +| Deleting a user, group or role that still has inline policies (a user also access keys or group memberships, a group also members) | `DeleteUser`, `DeleteGroup` and `DeleteRole` refuse with `DeleteConflict` until they are removed | The delete succeeds and removes them, and the principal's boundary, tags and sessions, with the principal. A principal created later with the same name starts empty | +| Putting an inline policy or boundary on a principal that does not exist | `NoSuchEntity` | HTTP 404 `IAM user not found` (or `group`, `role`) from the management API | | Table ARN as `TableName` | Accepted; the request acts on that table | Authorized against that table, then the request fails with `ResourceNotFoundException` | | AccessDenied message for an ARN outside this deployment (caller with no policy) | Names the ARN as supplied. A table ARN in `TableName` with another partition, service or region never reaches the IAM decision: it is `ValidationException` ("Invalid AWS partition", "Invalid vendor code", "Invalid AWS region") for every principal (measured) | Names the ARN as supplied. A table ARN in `TableName` with another partition, service or region is authorized as this deployment's table, and a caller with no policy is denied naming the ARN as supplied | | A resource ARN in another account | Refused with the same answer for a caller allowed `dynamodb:*` on `*` and a caller with no policy. When the account exists: `AccessDeniedException` "User: is not authorized to perform: dynamodb: on resource: because no resource-based policy allows the dynamodb: action", except `CreateTable`, the time-to-live, continuous-backup, backup, export and restore operations, which answer `ValidationException` "This action is only supported by accounts that match the resource owner’s account.". When it does not: `AccessDeniedException` "The caller is not authorized to access the specified resource." on `GetItem`, `PutItem`, `UpdateItem`, `Query`, `Scan` and `BatchGetItem`, the same `ValidationException` on `RestoreTableToPointInTime`, and `AccessDeniedException` "Access is denied" on the rest. A batch or transaction with tables in more than one account is `ValidationException` "Invalid request! All tables in the request must be from a single account." (measured) | The same answers, before the IAM decision and in dev mode too, where an account exists when this deployment's catalog holds it. `DeleteTable`, `DeleteItem` and `DeleteBackup` take the answers of `UpdateTable`, `UpdateItem` and `DescribeBackup` (not measured). A table ARN in `TableName` that also has another partition, service or region, or that names an index or backup, gets these answers too. The service answers it with a `ValidationException` (the partition, vendor or region text in the row above, or "Valid ARN format is ..." for an index or backup ARN): on `GetItem` whether or not the account exists, and on `DescribeTable` when it exists (measured; the other operations were not sent) | diff --git a/docs/getting-started.md b/docs/getting-started.md index 06e24b767..147318606 100755 --- a/docs/getting-started.md +++ b/docs/getting-started.md @@ -848,6 +848,8 @@ Account IDs must be 12-digit numeric strings (matching AWS account ID format): Accounts with existing tables cannot be deleted. Delete all tables first. +Deleting an account also deletes its users, groups and roles with everything they hold, so their access keys and sessions stop working and its users cannot log in. An account created later with the same ID starts empty. + ```bash ./target/release/extenddb manage --user admin --password \ delete-account --account-id 123456789012 @@ -874,7 +876,7 @@ A default self-service policy is automatically attached, allowing the user to ma ### Delete an IAM user -Deleting a user also removes their access keys, group memberships, tags, and policies (via CASCADE). +Deleting a user also removes its access keys, group memberships, tags, inline policies and permissions boundary. A user created later with the same name starts with only the default `SelfServicePolicy`. ```bash ./target/release/extenddb manage --user admin --password \ @@ -983,6 +985,8 @@ Every import is logged to syslog (access key ID, account, user — never the sec ### Delete a group +Deleting a group also removes its inline policies and its memberships. A group created later with the same name starts with no policies and no members. + ```bash ./target/release/extenddb manage --user admin --password \ delete-group --account-id 123456789012 --group-name developers @@ -990,7 +994,7 @@ Every import is logged to syslog (access key ID, account, user — never the sec ### Put a user policy -Creates or replaces a named policy on an IAM user. The policy document must be valid JSON with `Version` and `Statement` fields. +Creates or replaces a named policy on an IAM user. The policy document must be valid JSON with `Version` and `Statement` fields. The user must exist; a policy put on a user that does not exist is refused with HTTP 404 `IAM user not found`. The same holds for group and role policies and for permissions boundaries. ```bash ./target/release/extenddb manage --user admin --password \ @@ -1101,6 +1105,8 @@ Creates or replaces a named policy on an IAM user. The policy document must be v ### Delete a role +Deleting a role also removes its inline policies, permissions boundary, tags and sessions: credentials issued by `assume-role` stop working. A role created later with the same name starts with none of them. + ```bash ./target/release/extenddb manage --user admin --password \ delete-role --account-id 123456789012 --role-name data-reader diff --git a/docs/manuals/05-admin-guide.md b/docs/manuals/05-admin-guide.md index 779517d43..1b3bae03f 100755 --- a/docs/manuals/05-admin-guide.md +++ b/docs/manuals/05-admin-guide.md @@ -338,7 +338,7 @@ Account IDs must be 12-digit numeric strings (matching AWS format). If `--accoun ./target/release/extenddb manage --user admin --password \ list-users --account-id 123456789012 -# Delete (cascades: removes keys, memberships, tags, policies) +# Delete (also removes its keys, memberships, tags, policies and boundary) ./target/release/extenddb manage --user admin --password \ delete-user --account-id 123456789012 --user-name alice ``` @@ -385,7 +385,7 @@ Access key prefixes: `AKIAEXTENDDB` (long-lived), `ASIAEXTENDDB` (temporary/Assu remove-group-member --account-id 123456789012 \ --group-name developers --user-name alice -# Delete +# Delete (also removes its policies and memberships) ./target/release/extenddb manage --user admin --password \ delete-group --account-id 123456789012 --group-name developers ``` @@ -413,7 +413,7 @@ Access key prefixes: `AKIAEXTENDDB` (long-lived), `ASIAEXTENDDB` (temporary/Assu --caller-arn arn:aws:iam::123456789012:user/alice \ --session-name test-session -# Delete +# Delete (also removes its policies, boundary, tags and sessions) ./target/release/extenddb manage --user admin --password \ delete-role --account-id 123456789012 --role-name data-reader ``` @@ -476,7 +476,10 @@ What a trust-policy `Deny` does not do: ### Policies -Inline policies can be attached to users, groups, and roles: +Inline policies can be attached to users, groups, and roles. The user, group +or role must exist: a policy or permissions boundary put on a name with no +principal is refused with HTTP 404 (`IAM user not found`, `IAM group not found` +or `IAM role not found`), as IAM answers `NoSuchEntity`. ```bash # User policy diff --git a/docs/manuals/07-upgrade-manual.md b/docs/manuals/07-upgrade-manual.md index 2e45065cf..012e2b18a 100755 --- a/docs/manuals/07-upgrade-manual.md +++ b/docs/manuals/07-upgrade-manual.md @@ -323,6 +323,46 @@ Catalog upgrades change the schema; behavior changes alter how the running serve interprets existing data or configuration. Review these before upgrading a live deployment, even when no catalog migration is required. +### v0.1.15: a deleted principal or account leaves nothing behind + +**What changed.** Deleting a user, group, role or account now removes +everything it owns, on every backend: + +- A user's access keys, group memberships, tags, inline policies and + permissions boundary. Before, SQLite and PostgreSQL kept its inline policies + and boundary, and MongoDB and Cassandra kept its boundary and, on MongoDB, + its tags. +- A group's inline policies and memberships. Before, every backend kept its + inline policies, and MongoDB and Cassandra kept its memberships. +- A role's inline policies, boundary, tags and sessions. Before, every backend + kept its inline policies and boundary, and MongoDB and Cassandra kept its + tags and sessions. +- On MongoDB and Cassandra, an account's users, groups, roles and all they + own. Before, only the account record (and on Cassandra its backups) was + removed, and the rest stayed in the catalog. + +A user, group or role created later with the same name got what the deleted +one left, and so its permissions. Now creating a user, group or role also +removes anything stored under its name, and creating an account on MongoDB or +Cassandra removes anything stored under its id, so records an earlier release +left behind are cleared when the name or id is used again. On MongoDB and +Cassandra a user, access key, session or role of an account that no longer +exists is treated as unknown. + +The management API and the console also refuse an inline policy or +permissions boundary put on a user, group or role that does not exist, with +HTTP 404 and `IAM user not found` (or `group`, `role`), where they answered +204 and stored it under the name. On MongoDB, `create-account` refuses an +account ID that is already in use with `Account already exists`, as the other +backends do; before, it created a second account with that ID. + +**Who is affected.** A script that attaches a policy or boundary before it +creates the principal now gets HTTP 404 and must create the principal first. +A deployment that relied on a deleted principal's policies coming back when it +was created again must attach them again. No catalog migration is required. + +--- + ### v0.1.15: Point-in-time recovery is reported as unsupported (catalog migration required) **What changed.** `UpdateContinuousBackups` no longer accepts an enable request, and `DescribeContinuousBackups` no longer reports point-in-time recovery `ENABLED` with a 35-day window. Point-in-time recovery is reported as unsupported: describe answers `PointInTimeRecoveryStatus: DISABLED`, an enable request returns `ContinuousBackupsUnavailableException`, and `RestoreTableToPointInTime` checks the source table and then returns `PointInTimeRecoveryUnavailableException`. Authorization of `RestoreTableToPointInTime` checks the item actions on the target table before the restore action on the source, as the service does, so a caller with no policy is denied naming `dynamodb:Scan` on the target table, where earlier builds named `dynamodb:RestoreTableToPointInTime` on the source table. A table that does not exist is `TableNotFoundException` on all three operations, where `DescribeContinuousBackups` and `UpdateContinuousBackups` answered `ResourceNotFoundException`, and `UpdateContinuousBackups` without `PointInTimeRecoverySpecification` is a `ValidationException`, where it was taken as disabling. See [Differences from DynamoDB](../differences-from-dynamodb.md) for the full surface. The catalog migration removes the storage of a per-table enable flag that only the description read. diff --git a/docs/manuals/12-extending-extenddb-storage.md b/docs/manuals/12-extending-extenddb-storage.md index 6c54f3344..97991419a 100755 --- a/docs/manuals/12-extending-extenddb-storage.md +++ b/docs/manuals/12-extending-extenddb-storage.md @@ -177,7 +177,7 @@ Defined in `crates/storage/src/management_store/mod.rs`. Covers all IAM CRUD ope | `get_account_summary` | Account summary (user/group/role/policy counts) | Key design decisions: -- Account deletion must cascade to all users, groups, roles, policies, and access keys atomically. +- Deleting an account, user, group or role must remove everything it owns, and creating one must remove what an older delete left under its name or id (see [CASCADE Semantics](#cascade-semantics)). - Access key secrets are encrypted (AES-256-GCM) before storage. - Sessions have expiration enforcement. @@ -392,7 +392,33 @@ Stream sequence numbers must be monotonically increasing within a shard. Your ba ### CASCADE Semantics -Account deletion must cascade to all child resources (users, groups, roles, policies, access keys, sessions) atomically. Your backend must provide equivalent cascade logic. +IAM records name their principal and account by value, and a record left +behind belongs to the next principal or account created with that name or id: +an inline policy or boundary grants it permissions, an access key or role +session signs in as it, and a membership puts it in a group. So: + +- Deleting a user, group or role must also delete everything it owns: a + user's access keys, group memberships, tags, inline policies and boundary; + a group's inline policies and memberships; a role's inline policies, + boundary, tags and sessions. On PostgreSQL and SQLite the foreign keys + cover all of it except inline policies and boundaries, which name their + principal by `(principal_type, principal_name)` and are deleted in the same + transaction as the principal. +- Account deletion must cascade to all child resources (users, groups, roles, + policies, boundaries, access keys, memberships, tags, sessions). +- Creating a user, group or role must delete what is stored under its name + before it exists (rows an older release's delete left behind), and only + after its own insert succeeded, so that a create refused as a duplicate + touches nothing. Creating an account does the same for its id. +- A backend without foreign keys deletes what a principal or account owns + before the principal or account itself, so that a failure leaves less, never + a credential without its owner. The MongoDB and Cassandra backends also treat + a user, access key, session or role of an account that no longer exists as + unknown when signing in or assuming a role. + +`extenddb_storage::iam_leftovers_conformance::run` (feature `test-util`) +checks these properties against a real database; each backend's test suite +runs it. ## PostgreSQL-isms in the Default Backend From 99ff83342465030400093f1b722cf65345dd5f2d Mon Sep 17 00:00:00 2001 From: Hitesh Sharma Date: Mon, 9 Nov 2026 17:40:29 -0800 Subject: [PATCH 5/5] fix(storage-sqlite): roll back a principal write before returning A principal's create or delete returned an error, such as AlreadyExists or NotFound, by dropping its BEGIN IMMEDIATE transaction. Dropping a SQLite transaction only queues its rollback, and a BEGIN IMMEDIATE on the connection before the rollback ran failed with "attempted to call begin_with at non-zero transaction depth", so the next write on that connection answered 500. The creates and deletes now commit or roll back through one helper before they return. --- .../src/management_store/groups.rs | 76 ++++----- .../src/management_store/policies.rs | 40 +++++ .../src/management_store/roles.rs | 106 ++++++------- .../src/management_store/users.rs | 144 ++++++++---------- 4 files changed, 182 insertions(+), 184 deletions(-) diff --git a/crates/storage-sqlite/src/management_store/groups.rs b/crates/storage-sqlite/src/management_store/groups.rs index 8a08714eb..6af061e88 100644 --- a/crates/storage-sqlite/src/management_store/groups.rs +++ b/crates/storage-sqlite/src/management_store/groups.rs @@ -8,7 +8,7 @@ use extenddb_storage::management_store::{GroupDetail, GroupListEntry, OpError, O use crate::catalog_store::SqliteCatalogStore; use crate::sqlite_util::{is_fk_violation, is_unique_violation, parse_timestamp}; -use super::policies::delete_principal_attachments; +use super::policies::{begin_write, delete_principal_attachments, finish}; impl SqliteCatalogStore { pub(crate) async fn create_group_impl( @@ -19,15 +19,11 @@ impl SqliteCatalogStore { let group_arn = format!("arn:aws:iam::{account_id}:group/{group_name}"); // D1: every writer holds the write lock. let _writer = self.lock_writes().await; - let mut tx = self - .pool() - .begin_with("BEGIN IMMEDIATE") - .await - .map_err(|e| { - tracing::error!("create_group begin: {e}"); - OpError::Internal("Database error".to_owned()) - })?; - sqlx::query("INSERT INTO iam_groups (account_id, group_name, group_arn) VALUES (?, ?, ?)") + let mut tx = begin_write(self.pool(), "create_group").await?; + let result: OpResult<()> = async { + sqlx::query( + "INSERT INTO iam_groups (account_id, group_name, group_arn) VALUES (?, ?, ?)", + ) .bind(account_id) .bind(group_name) .bind(&group_arn) @@ -43,13 +39,12 @@ impl SqliteCatalogStore { OpError::Internal("Database error".to_owned()) } })?; - // A group deleted before deletes removed them could have left its - // inline policies under this name. - delete_principal_attachments(&mut tx, account_id, "group", group_name).await?; - tx.commit().await.map_err(|e| { - tracing::error!("create_group commit: {e}"); - OpError::Internal("Database error".to_owned()) - }) + // A group deleted before deletes removed them could have left its + // inline policies under this name. + delete_principal_attachments(&mut tx, account_id, "group", group_name).await + } + .await; + finish(tx, result, "create_group").await } pub(crate) async fn delete_group_impl( @@ -59,34 +54,27 @@ impl SqliteCatalogStore { ) -> OpResult<()> { // D1: every writer holds the write lock. let _writer = self.lock_writes().await; - let mut tx = self - .pool() - .begin_with("BEGIN IMMEDIATE") - .await - .map_err(|e| { - tracing::error!("delete_group begin: {e}"); - OpError::Internal("Database error".to_owned()) - })?; - // The foreign keys remove the group's memberships. Its inline - // policies have none. - let result = sqlx::query("DELETE FROM iam_groups WHERE account_id = ? AND group_name = ?") - .bind(account_id) - .bind(group_name) - .execute(&mut *tx) - .await - .map_err(|e| { - tracing::error!("delete_group: {e}"); - OpError::Internal("Database error".to_owned()) - })?; - if result.rows_affected() == 0 { - return Err(OpError::NotFound("IAM group not found".to_owned())); + let mut tx = begin_write(self.pool(), "delete_group").await?; + let result: OpResult<()> = async { + // The foreign keys remove the group's memberships. Its inline policies + // have none. + let deleted = + sqlx::query("DELETE FROM iam_groups WHERE account_id = ? AND group_name = ?") + .bind(account_id) + .bind(group_name) + .execute(&mut *tx) + .await + .map_err(|e| { + tracing::error!("delete_group: {e}"); + OpError::Internal("Database error".to_owned()) + })?; + if deleted.rows_affected() == 0 { + return Err(OpError::NotFound("IAM group not found".to_owned())); + } + delete_principal_attachments(&mut tx, account_id, "group", group_name).await } - delete_principal_attachments(&mut tx, account_id, "group", group_name).await?; - tx.commit().await.map_err(|e| { - tracing::error!("delete_group commit: {e}"); - OpError::Internal("Database error".to_owned()) - })?; - Ok(()) + .await; + finish(tx, result, "delete_group").await } pub(crate) async fn list_groups_impl(&self, account_id: &str) -> OpResult> { diff --git a/crates/storage-sqlite/src/management_store/policies.rs b/crates/storage-sqlite/src/management_store/policies.rs index ead25f428..cb3a647c9 100644 --- a/crates/storage-sqlite/src/management_store/policies.rs +++ b/crates/storage-sqlite/src/management_store/policies.rs @@ -9,6 +9,46 @@ use time::OffsetDateTime; use crate::catalog_store::SqliteCatalogStore; use crate::sqlite_util::parse_timestamp; +/// Begin the `BEGIN IMMEDIATE` transaction a principal's create or delete +/// runs in. The caller holds the write lock (D1). +pub(super) async fn begin_write<'a>( + pool: &'a sqlx::SqlitePool, + what: &str, +) -> OpResult> { + pool.begin_with("BEGIN IMMEDIATE").await.map_err(|e| { + tracing::error!("{what} begin: {e}"); + OpError::Internal("Database error".to_owned()) + }) +} + +/// Commit `tx` when `result` is `Ok`, and roll it back otherwise, so the +/// connection goes back to the pool with no transaction open. +/// +/// Dropping a SQLite transaction only queues its rollback, and a +/// `BEGIN IMMEDIATE` on that connection before the rollback ran fails with +/// "attempted to call begin_with at non-zero transaction depth". +pub(super) async fn finish( + tx: sqlx::Transaction<'_, sqlx::Sqlite>, + result: OpResult, + what: &str, +) -> OpResult { + match result { + Ok(value) => { + tx.commit().await.map_err(|e| { + tracing::error!("{what} commit: {e}"); + OpError::Internal("Database error".to_owned()) + })?; + Ok(value) + } + Err(e) => { + if let Err(rollback) = tx.rollback().await { + tracing::error!("{what} rollback: {rollback}"); + } + Err(e) + } + } +} + /// Delete the inline policies and the permissions boundary stored under a /// principal's name, inside the caller's transaction. /// diff --git a/crates/storage-sqlite/src/management_store/roles.rs b/crates/storage-sqlite/src/management_store/roles.rs index c94750c81..b197e940c 100644 --- a/crates/storage-sqlite/src/management_store/roles.rs +++ b/crates/storage-sqlite/src/management_store/roles.rs @@ -11,7 +11,7 @@ use extenddb_storage::management_store::{OpError, OpResult, RoleDetail, RoleList use crate::catalog_store::SqliteCatalogStore; use crate::sqlite_util::{is_fk_violation, is_unique_violation, parse_timestamp}; -use super::policies::delete_principal_attachments; +use super::policies::{begin_write, delete_principal_attachments, finish}; /// Parse a stored JSON-text column into a `serde_json::Value`. fn parse_json(s: &str, ctx: &str) -> OpResult { @@ -30,74 +30,60 @@ impl SqliteCatalogStore { .map_err(|e| OpError::Internal(format!("serialize trust policy: {e}")))?; // D1: every writer holds the write lock. let _writer = self.lock_writes().await; - let mut tx = self - .pool() - .begin_with("BEGIN IMMEDIATE") + let mut tx = begin_write(self.pool(), "create_role").await?; + let result: OpResult<()> = async { + sqlx::query( + "INSERT INTO iam_roles (account_id, role_name, role_arn, trust_policy) \ + VALUES (?, ?, ?, ?)", + ) + .bind(account_id) + .bind(role_name) + .bind(&role_arn) + .bind(&trust) + .execute(&mut *tx) .await .map_err(|e| { - tracing::error!("create_role begin: {e}"); - OpError::Internal("Database error".to_owned()) + if is_unique_violation(&e) { + OpError::AlreadyExists("IAM role already exists".to_owned()) + } else if is_fk_violation(&e) { + OpError::NotFound("Account not found".to_owned()) + } else { + tracing::error!("create_role: {e}"); + OpError::Internal("Database error".to_owned()) + } })?; - sqlx::query( - "INSERT INTO iam_roles (account_id, role_name, role_arn, trust_policy) \ - VALUES (?, ?, ?, ?)", - ) - .bind(account_id) - .bind(role_name) - .bind(&role_arn) - .bind(&trust) - .execute(&mut *tx) - .await - .map_err(|e| { - if is_unique_violation(&e) { - OpError::AlreadyExists("IAM role already exists".to_owned()) - } else if is_fk_violation(&e) { - OpError::NotFound("Account not found".to_owned()) - } else { - tracing::error!("create_role: {e}"); - OpError::Internal("Database error".to_owned()) - } - })?; - // A role deleted before deletes removed them could have left its - // inline policies and boundary under this name. - delete_principal_attachments(&mut tx, account_id, "role", role_name).await?; - tx.commit().await.map_err(|e| { - tracing::error!("create_role commit: {e}"); - OpError::Internal("Database error".to_owned()) - }) + // A role deleted before deletes removed them could have left its + // inline policies and boundary under this name. + delete_principal_attachments(&mut tx, account_id, "role", role_name).await + } + .await; + finish(tx, result, "create_role").await } pub(crate) async fn delete_role_impl(&self, account_id: &str, role_name: &str) -> OpResult<()> { // D1: every writer holds the write lock. let _writer = self.lock_writes().await; - let mut tx = self - .pool() - .begin_with("BEGIN IMMEDIATE") - .await - .map_err(|e| { - tracing::error!("delete_role begin: {e}"); - OpError::Internal("Database error".to_owned()) - })?; - // The foreign keys remove the role's tags and sessions. Its inline - // policies and boundary have none. - let result = sqlx::query("DELETE FROM iam_roles WHERE account_id = ? AND role_name = ?") - .bind(account_id) - .bind(role_name) - .execute(&mut *tx) - .await - .map_err(|e| { - tracing::error!("delete_role: {e}"); - OpError::Internal("Database error".to_owned()) - })?; - if result.rows_affected() == 0 { - return Err(OpError::NotFound("IAM role not found".to_owned())); + let mut tx = begin_write(self.pool(), "delete_role").await?; + let result: OpResult<()> = async { + // The foreign keys remove the role's tags and sessions. Its + // inline policies and boundary have none. + let deleted = + sqlx::query("DELETE FROM iam_roles WHERE account_id = ? AND role_name = ?") + .bind(account_id) + .bind(role_name) + .execute(&mut *tx) + .await + .map_err(|e| { + tracing::error!("delete_role: {e}"); + OpError::Internal("Database error".to_owned()) + })?; + if deleted.rows_affected() == 0 { + return Err(OpError::NotFound("IAM role not found".to_owned())); + } + delete_principal_attachments(&mut tx, account_id, "role", role_name).await } - delete_principal_attachments(&mut tx, account_id, "role", role_name).await?; - tx.commit().await.map_err(|e| { - tracing::error!("delete_role commit: {e}"); - OpError::Internal("Database error".to_owned()) - })?; - Ok(()) + .await; + finish(tx, result, "delete_role").await } pub(crate) async fn list_roles_impl(&self, account_id: &str) -> OpResult> { diff --git a/crates/storage-sqlite/src/management_store/users.rs b/crates/storage-sqlite/src/management_store/users.rs index f892d4a67..dd3602ac2 100644 --- a/crates/storage-sqlite/src/management_store/users.rs +++ b/crates/storage-sqlite/src/management_store/users.rs @@ -9,7 +9,7 @@ use extenddb_storage::management_store::{OpError, OpResult, UserDetail, UserList use crate::catalog_store::SqliteCatalogStore; use crate::sqlite_util::{is_fk_violation, is_unique_violation, parse_timestamp}; -use super::policies::delete_principal_attachments; +use super::policies::{begin_write, delete_principal_attachments, finish}; impl SqliteCatalogStore { pub(crate) async fn create_user_impl( @@ -20,43 +20,6 @@ impl SqliteCatalogStore { ) -> OpResult<()> { let user_arn = format!("arn:aws:iam::{account_id}:user/{user_name}"); - // D1: every writer holds the write lock, acquired before the - // transaction begins so the lock never waits on a pool connection. - let _writer = self.lock_writes().await; - let mut tx = self - .pool() - .begin_with("BEGIN IMMEDIATE") - .await - .map_err(|e| { - tracing::error!("create_user begin: {e}"); - OpError::Internal("Database error".to_owned()) - })?; - - sqlx::query( - "INSERT INTO iam_users (account_id, user_name, user_arn, password_hash) \ - VALUES (?, ?, ?, ?)", - ) - .bind(account_id) - .bind(user_name) - .bind(&user_arn) - .bind(password_hash) - .execute(&mut *tx) - .await - .map_err(|e| { - if is_unique_violation(&e) { - OpError::AlreadyExists("IAM user already exists".to_owned()) - } else if is_fk_violation(&e) { - OpError::NotFound("Account not found".to_owned()) - } else { - tracing::error!("create_user: {e}"); - OpError::Internal("Database error".to_owned()) - } - })?; - - // A user deleted before deletes removed these could have left its - // inline policies and boundary under this name. - delete_principal_attachments(&mut tx, account_id, "user", user_name).await?; - // Seed a default self-service policy so the user can manage its own // access keys and password without an administrator attaching one // (parity with the PostgreSQL backend). @@ -70,64 +33,85 @@ impl SqliteCatalogStore { "iam:ListAccessKeys", "iam:ChangePassword" ], - "Resource": user_arn, + "Resource": &user_arn, }] }) .to_string(); - sqlx::query( - "INSERT INTO iam_policies \ - (account_id, principal_type, principal_name, policy_name, policy_document) \ - VALUES (?, 'user', ?, 'SelfServicePolicy', ?) ON CONFLICT DO NOTHING", - ) - .bind(account_id) - .bind(user_name) - .bind(&self_service_policy) - .execute(&mut *tx) - .await - .map_err(|e| { - tracing::error!("seed self-service policy: {e}"); - OpError::Internal("Database error".to_owned()) - })?; - - tx.commit().await.map_err(|e| { - tracing::error!("create_user commit: {e}"); - OpError::Internal("Database error".to_owned()) - })?; - Ok(()) - } - - pub(crate) async fn delete_user_impl(&self, account_id: &str, user_name: &str) -> OpResult<()> { - // D1: every writer holds the write lock. + // D1: every writer holds the write lock, acquired before the + // transaction begins so the lock never waits on a pool connection. let _writer = self.lock_writes().await; - let mut tx = self - .pool() - .begin_with("BEGIN IMMEDIATE") + let mut tx = begin_write(self.pool(), "create_user").await?; + let result: OpResult<()> = async { + sqlx::query( + "INSERT INTO iam_users (account_id, user_name, user_arn, password_hash) \ + VALUES (?, ?, ?, ?)", + ) + .bind(account_id) + .bind(user_name) + .bind(&user_arn) + .bind(password_hash) + .execute(&mut *tx) .await .map_err(|e| { - tracing::error!("delete_user begin: {e}"); - OpError::Internal("Database error".to_owned()) + if is_unique_violation(&e) { + OpError::AlreadyExists("IAM user already exists".to_owned()) + } else if is_fk_violation(&e) { + OpError::NotFound("Account not found".to_owned()) + } else { + tracing::error!("create_user: {e}"); + OpError::Internal("Database error".to_owned()) + } })?; - // The foreign keys remove the user's access keys, tags and group - // memberships. Its inline policies and boundary have none. - let result = sqlx::query("DELETE FROM iam_users WHERE account_id = ? AND user_name = ?") + + // A user deleted before deletes removed these could have left its + // inline policies and boundary under this name. + delete_principal_attachments(&mut tx, account_id, "user", user_name).await?; + + sqlx::query( + "INSERT INTO iam_policies \ + (account_id, principal_type, principal_name, policy_name, policy_document) \ + VALUES (?, 'user', ?, 'SelfServicePolicy', ?) ON CONFLICT DO NOTHING", + ) .bind(account_id) .bind(user_name) + .bind(&self_service_policy) .execute(&mut *tx) .await .map_err(|e| { - tracing::error!("delete_user: {e}"); + tracing::error!("seed self-service policy: {e}"); OpError::Internal("Database error".to_owned()) })?; - if result.rows_affected() == 0 { - return Err(OpError::NotFound("IAM user not found".to_owned())); + Ok(()) } - delete_principal_attachments(&mut tx, account_id, "user", user_name).await?; - tx.commit().await.map_err(|e| { - tracing::error!("delete_user commit: {e}"); - OpError::Internal("Database error".to_owned()) - })?; - Ok(()) + .await; + finish(tx, result, "create_user").await + } + + pub(crate) async fn delete_user_impl(&self, account_id: &str, user_name: &str) -> OpResult<()> { + // D1: every writer holds the write lock. + let _writer = self.lock_writes().await; + let mut tx = begin_write(self.pool(), "delete_user").await?; + let result: OpResult<()> = async { + // The foreign keys remove the user's access keys, tags and group + // memberships. Its inline policies and boundary have none. + let deleted = + sqlx::query("DELETE FROM iam_users WHERE account_id = ? AND user_name = ?") + .bind(account_id) + .bind(user_name) + .execute(&mut *tx) + .await + .map_err(|e| { + tracing::error!("delete_user: {e}"); + OpError::Internal("Database error".to_owned()) + })?; + if deleted.rows_affected() == 0 { + return Err(OpError::NotFound("IAM user not found".to_owned())); + } + delete_principal_attachments(&mut tx, account_id, "user", user_name).await + } + .await; + finish(tx, result, "delete_user").await } pub(crate) async fn list_users_impl(&self, account_id: &str) -> OpResult> {