From dee9ec0092c6f01c65dbc39190280d70c72d334e Mon Sep 17 00:00:00 2001 From: Alexander Petric Date: Sun, 2 Aug 2026 10:26:01 +0000 Subject: [PATCH] fix(groups): clean up workspace refs when instance groups are deleted --- ...9d448cd8916e58c788492c4373e367234a2a3.json | 22 ++ ...2243894e40a081fd2e7f95b466612aed054de.json | 22 ++ ...479ac854ee79b57198ffac4712fa8daa16ce6.json | 14 + ...dcc9d963cc033582bf2e945e8bf3a301b4247.json | 22 ++ ...361495016b1e768212e40a13746a31218c38a.json | 23 ++ ...d402c7b7c93ff9fd89364afd41470a92a5e2d.json | 22 ++ ...7d3c59f52a164a8e81ba3c55adce3978b2502.json | 24 ++ ...cc4416ced3a4f543607fd716669512e76bc8a.json | 28 ++ backend/ee-repo-ref.txt | 2 +- backend/windmill-api-groups/src/groups.rs | 221 ++++++++++++- .../tests/groups.rs | 295 ++++++++++++++++++ 11 files changed, 687 insertions(+), 8 deletions(-) create mode 100644 backend/.sqlx/query-327468e87ef27806076abe555199d448cd8916e58c788492c4373e367234a2a3.json create mode 100644 backend/.sqlx/query-374863f06094e404523e700df832243894e40a081fd2e7f95b466612aed054de.json create mode 100644 backend/.sqlx/query-50a658c5244e8acd17131023ffc479ac854ee79b57198ffac4712fa8daa16ce6.json create mode 100644 backend/.sqlx/query-a06e1d9f6f95e4c4c2b98310ebddcc9d963cc033582bf2e945e8bf3a301b4247.json create mode 100644 backend/.sqlx/query-a1f41deb3571289c7e1fba2f282361495016b1e768212e40a13746a31218c38a.json create mode 100644 backend/.sqlx/query-bbc2638aae4fb3556c8d876e4efd402c7b7c93ff9fd89364afd41470a92a5e2d.json create mode 100644 backend/.sqlx/query-d4a7a93f98b38f75167428d4ab67d3c59f52a164a8e81ba3c55adce3978b2502.json create mode 100644 backend/.sqlx/query-f007fe55c47cdd45a9b14e93852cc4416ced3a4f543607fd716669512e76bc8a.json diff --git a/backend/.sqlx/query-327468e87ef27806076abe555199d448cd8916e58c788492c4373e367234a2a3.json b/backend/.sqlx/query-327468e87ef27806076abe555199d448cd8916e58c788492c4373e367234a2a3.json new file mode 100644 index 0000000000..ab0e4a5ab6 --- /dev/null +++ b/backend/.sqlx/query-327468e87ef27806076abe555199d448cd8916e58c788492c4373e367234a2a3.json @@ -0,0 +1,22 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT DISTINCT email FROM email_to_igroup WHERE igroup = ANY($1)", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "email", + "type_info": "Varchar" + } + ], + "parameters": { + "Left": [ + "TextArray" + ] + }, + "nullable": [ + false + ] + }, + "hash": "327468e87ef27806076abe555199d448cd8916e58c788492c4373e367234a2a3" +} diff --git a/backend/.sqlx/query-374863f06094e404523e700df832243894e40a081fd2e7f95b466612aed054de.json b/backend/.sqlx/query-374863f06094e404523e700df832243894e40a081fd2e7f95b466612aed054de.json new file mode 100644 index 0000000000..f8b19be3c5 --- /dev/null +++ b/backend/.sqlx/query-374863f06094e404523e700df832243894e40a081fd2e7f95b466612aed054de.json @@ -0,0 +1,22 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT workspace_id FROM workspace_settings WHERE auto_invite->'instance_groups' ?| $1", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "workspace_id", + "type_info": "Varchar" + } + ], + "parameters": { + "Left": [ + "TextArray" + ] + }, + "nullable": [ + false + ] + }, + "hash": "374863f06094e404523e700df832243894e40a081fd2e7f95b466612aed054de" +} diff --git a/backend/.sqlx/query-50a658c5244e8acd17131023ffc479ac854ee79b57198ffac4712fa8daa16ce6.json b/backend/.sqlx/query-50a658c5244e8acd17131023ffc479ac854ee79b57198ffac4712fa8daa16ce6.json new file mode 100644 index 0000000000..c42bc7be1e --- /dev/null +++ b/backend/.sqlx/query-50a658c5244e8acd17131023ffc479ac854ee79b57198ffac4712fa8daa16ce6.json @@ -0,0 +1,14 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE workspace_settings SET\n auto_invite = jsonb_set(\n jsonb_set(\n COALESCE(auto_invite, '{}'::jsonb),\n '{instance_groups}',\n (SELECT COALESCE(jsonb_agg(elem), '[]'::jsonb)\n FROM jsonb_array_elements(COALESCE(auto_invite->'instance_groups', '[]'::jsonb)) elem\n WHERE elem #>> '{}' <> ALL($1))\n ),\n '{instance_groups_roles}',\n COALESCE(auto_invite->'instance_groups_roles', '{}'::jsonb) - $1::text[]\n )\n WHERE auto_invite->'instance_groups' ?| $1", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "TextArray" + ] + }, + "nullable": [] + }, + "hash": "50a658c5244e8acd17131023ffc479ac854ee79b57198ffac4712fa8daa16ce6" +} diff --git a/backend/.sqlx/query-a06e1d9f6f95e4c4c2b98310ebddcc9d963cc033582bf2e945e8bf3a301b4247.json b/backend/.sqlx/query-a06e1d9f6f95e4c4c2b98310ebddcc9d963cc033582bf2e945e8bf3a301b4247.json new file mode 100644 index 0000000000..909e6ad42d --- /dev/null +++ b/backend/.sqlx/query-a06e1d9f6f95e4c4c2b98310ebddcc9d963cc033582bf2e945e8bf3a301b4247.json @@ -0,0 +1,22 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT pg_advisory_xact_lock($1)", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "pg_advisory_xact_lock", + "type_info": "Void" + } + ], + "parameters": { + "Left": [ + "Int8" + ] + }, + "nullable": [ + null + ] + }, + "hash": "a06e1d9f6f95e4c4c2b98310ebddcc9d963cc033582bf2e945e8bf3a301b4247" +} diff --git a/backend/.sqlx/query-a1f41deb3571289c7e1fba2f282361495016b1e768212e40a13746a31218c38a.json b/backend/.sqlx/query-a1f41deb3571289c7e1fba2f282361495016b1e768212e40a13746a31218c38a.json new file mode 100644 index 0000000000..e55934c561 --- /dev/null +++ b/backend/.sqlx/query-a1f41deb3571289c7e1fba2f282361495016b1e768212e40a13746a31218c38a.json @@ -0,0 +1,23 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT 1 FROM usr WHERE workspace_id = $1 AND email = $2 FOR UPDATE", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "?column?", + "type_info": "Int4" + } + ], + "parameters": { + "Left": [ + "Text", + "Text" + ] + }, + "nullable": [ + null + ] + }, + "hash": "a1f41deb3571289c7e1fba2f282361495016b1e768212e40a13746a31218c38a" +} diff --git a/backend/.sqlx/query-bbc2638aae4fb3556c8d876e4efd402c7b7c93ff9fd89364afd41470a92a5e2d.json b/backend/.sqlx/query-bbc2638aae4fb3556c8d876e4efd402c7b7c93ff9fd89364afd41470a92a5e2d.json new file mode 100644 index 0000000000..e59183f51a --- /dev/null +++ b/backend/.sqlx/query-bbc2638aae4fb3556c8d876e4efd402c7b7c93ff9fd89364afd41470a92a5e2d.json @@ -0,0 +1,22 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT name FROM instance_group WHERE name <> ALL($1)", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "name", + "type_info": "Varchar" + } + ], + "parameters": { + "Left": [ + "TextArray" + ] + }, + "nullable": [ + false + ] + }, + "hash": "bbc2638aae4fb3556c8d876e4efd402c7b7c93ff9fd89364afd41470a92a5e2d" +} diff --git a/backend/.sqlx/query-d4a7a93f98b38f75167428d4ab67d3c59f52a164a8e81ba3c55adce3978b2502.json b/backend/.sqlx/query-d4a7a93f98b38f75167428d4ab67d3c59f52a164a8e81ba3c55adce3978b2502.json new file mode 100644 index 0000000000..c729beb2ad --- /dev/null +++ b/backend/.sqlx/query-d4a7a93f98b38f75167428d4ab67d3c59f52a164a8e81ba3c55adce3978b2502.json @@ -0,0 +1,24 @@ +{ + "db_name": "PostgreSQL", + "query": "\n SELECT EXISTS (\n SELECT 1\n FROM email_to_igroup e\n JOIN workspace_settings ws ON ws.workspace_id = $2\n WHERE e.email = $1\n AND e.igroup <> ALL($3)\n AND ws.auto_invite->'instance_groups' ? e.igroup\n )\n ", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "exists", + "type_info": "Bool" + } + ], + "parameters": { + "Left": [ + "Text", + "Text", + "TextArray" + ] + }, + "nullable": [ + null + ] + }, + "hash": "d4a7a93f98b38f75167428d4ab67d3c59f52a164a8e81ba3c55adce3978b2502" +} diff --git a/backend/.sqlx/query-f007fe55c47cdd45a9b14e93852cc4416ced3a4f543607fd716669512e76bc8a.json b/backend/.sqlx/query-f007fe55c47cdd45a9b14e93852cc4416ced3a4f543607fd716669512e76bc8a.json new file mode 100644 index 0000000000..0bff233c06 --- /dev/null +++ b/backend/.sqlx/query-f007fe55c47cdd45a9b14e93852cc4416ced3a4f543607fd716669512e76bc8a.json @@ -0,0 +1,28 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT auto_invite->'instance_groups' as \"groups: serde_json::Value\",\n auto_invite->'instance_groups_roles' as \"roles: serde_json::Value\"\n FROM workspace_settings WHERE workspace_id = $1", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "groups: serde_json::Value", + "type_info": "Jsonb" + }, + { + "ordinal": 1, + "name": "roles: serde_json::Value", + "type_info": "Jsonb" + } + ], + "parameters": { + "Left": [ + "Text" + ] + }, + "nullable": [ + null, + null + ] + }, + "hash": "f007fe55c47cdd45a9b14e93852cc4416ced3a4f543607fd716669512e76bc8a" +} diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index 589a683dba..0d710c6aec 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -2ba6a2a75b6fc97858b306b2c98ada481e363c10 +dd8e7b847cc2fe43bef85e2f0c49011bb61c7457 \ No newline at end of file diff --git a/backend/windmill-api-groups/src/groups.rs b/backend/windmill-api-groups/src/groups.rs index 263daad624..eba818d45d 100644 --- a/backend/windmill-api-groups/src/groups.rs +++ b/backend/windmill-api-groups/src/groups.rs @@ -513,6 +513,130 @@ async fn update_igroup( Ok(format!("Updated group {}", name)) } +/// Serializes instance-group mutations against each other. +/// +/// `delete_igroup`, `overwrite_igroups` and `remove_user_igroup` touch `usr` and +/// `email_to_igroup` in whichever order each one's correctness requires, so they do not share a +/// single lock hierarchy. Taking this first makes deadlock between them impossible instead. +const INSTANCE_GROUP_LOCK: i64 = 0x776d_6967; // "wmig" + +pub async fn lock_instance_group_mutations(tx: &mut Transaction<'_, Postgres>) -> Result<()> { + sqlx::query_scalar!("SELECT pg_advisory_xact_lock($1)", INSTANCE_GROUP_LOCK) + .fetch_one(&mut **tx) + .await?; + Ok(()) +} + +/// Workspaces whose auto-assignment config references any of `groups`. +/// +/// Reads `workspace_settings` across the whole instance without checking the caller's rights. +/// Callers must have established superadmin beforehand; the result leaks which workspaces are +/// configured with a given instance group. +#[cfg(feature = "private")] +pub async fn workspaces_referencing_instance_groups( + groups: &[String], + tx: &mut Transaction<'_, Postgres>, +) -> Result> { + if groups.is_empty() { + return Ok(vec![]); + } + + let workspaces = sqlx::query_scalar!( + "SELECT workspace_id FROM workspace_settings WHERE auto_invite->'instance_groups' ?| $1", + groups + ) + .fetch_all(&mut **tx) + .await?; + + Ok(workspaces) +} + +/// Re-derive membership for each workspace from the instance groups it still references. +/// +/// `cleanup_removed_instance_groups` drops a user whenever `added_via.group` names the removed +/// group, but that field only records their highest-precedence group. Without this pass, losing +/// their top group also revokes access they still hold through a lower-precedence one. +/// +/// Grants workspace access and roles without checking the caller's rights, and takes the +/// workspace list from the caller. Callers must have established superadmin, or admin on every +/// workspace passed, beforehand. `authed` is audit attribution only, not an authorization check. +#[cfg(feature = "private")] +pub async fn reprocess_workspace_instance_groups( + workspaces: &[String], + tx: &mut Transaction<'_, Postgres>, + authed: &impl windmill_audit::audit_oss::AuditAuthorable, +) -> Result<()> { + use windmill_api_workspaces::workspaces_ee::process_instance_group_auto_adds; + + for w_id in workspaces { + let settings = sqlx::query!( + r#"SELECT auto_invite->'instance_groups' as "groups: serde_json::Value", + auto_invite->'instance_groups_roles' as "roles: serde_json::Value" + FROM workspace_settings WHERE workspace_id = $1"#, + w_id + ) + .fetch_optional(&mut **tx) + .await?; + + let Some(settings) = settings else { continue }; + + let groups: Vec = settings + .groups + .and_then(|v| serde_json::from_value(v).ok()) + .unwrap_or_default(); + if groups.is_empty() { + continue; + } + + let roles = settings.roles.unwrap_or_else(|| serde_json::json!({})); + process_instance_group_auto_adds(w_id, &groups, &roles, tx, authed).await?; + } + + Ok(()) +} + +/// Drop `groups` from every workspace's instance-group auto-assignment config. +/// +/// Workspaces reference instance groups by name in `workspace_settings.auto_invite`, and +/// nothing in the schema ties those references to `instance_group` rows. A deleted group whose +/// name is left behind here silently re-acquires its members if a group of the same name is +/// created later. +/// +/// Mutates every workspace's settings, so callers must have established superadmin first. +pub(crate) async fn remove_instance_groups_from_workspace_settings( + groups: &[String], + tx: &mut Transaction<'_, Postgres>, +) -> Result<()> { + if groups.is_empty() { + return Ok(()); + } + + // Row filter must stay `?|`: it yields false on a JSON `null` instance_groups, where + // jsonb_array_elements_text would instead raise and abort the whole transaction. It is not + // index-backed — the GIN index covers the auto_invite column, not this expression — which + // is acceptable since workspace_settings holds one row per workspace. + sqlx::query!( + r#"UPDATE workspace_settings SET + auto_invite = jsonb_set( + jsonb_set( + COALESCE(auto_invite, '{}'::jsonb), + '{instance_groups}', + (SELECT COALESCE(jsonb_agg(elem), '[]'::jsonb) + FROM jsonb_array_elements(COALESCE(auto_invite->'instance_groups', '[]'::jsonb)) elem + WHERE elem #>> '{}' <> ALL($1)) + ), + '{instance_groups_roles}', + COALESCE(auto_invite->'instance_groups_roles', '{}'::jsonb) - $1::text[] + ) + WHERE auto_invite->'instance_groups' ?| $1"#, + groups + ) + .execute(&mut **tx) + .await?; + + Ok(()) +} + async fn delete_igroup( authed: ApiAuthed, Extension(db): Extension, @@ -521,6 +645,8 @@ async fn delete_igroup( require_super_admin(&db, &authed.email).await?; let mut tx: Transaction<'_, Postgres> = db.begin().await?; + lock_instance_group_mutations(&mut tx).await?; + // Fetch group's instance_role and members before deletion let group_role = sqlx::query_scalar!( "SELECT instance_role FROM instance_group WHERE name = $1", @@ -538,6 +664,25 @@ async fn delete_igroup( vec![] }; + // Captured before the settings update strips the group from them. + #[cfg(feature = "private")] + let affected_workspaces = + workspaces_referencing_instance_groups(std::slice::from_ref(&name), &mut tx).await?; + + remove_instance_groups_from_workspace_settings(std::slice::from_ref(&name), &mut tx).await?; + + // Gated on `private` alone, matching the auto-add path this reverses + // (workspaces::edit_instance_groups) — CE builds include `private` without `enterprise`. + // Runs after the settings update so both take workspace_settings before usr, as + // edit_instance_groups does; the reverse order deadlocks against a concurrent edit. + // Both must precede the deletes below, which remove the rows membership is resolved from. + #[cfg(feature = "private")] + { + use windmill_api_workspaces::workspaces_ee::cleanup_removed_instance_groups; + cleanup_removed_instance_groups(std::slice::from_ref(&name), &[], &mut tx).await?; + reprocess_workspace_instance_groups(&affected_workspaces, &mut tx, &authed).await?; + } + sqlx::query!("DELETE FROM email_to_igroup WHERE igroup = $1", name) .execute(&mut *tx) .await?; @@ -1099,12 +1244,34 @@ async fn remove_user_igroup( require_super_admin(&db, &authed.email).await?; let mut tx = db.begin().await?; + lock_instance_group_mutations(&mut tx).await?; + let group_opt = sqlx::query_scalar!("SELECT name FROM instance_group WHERE name = $1", name,) .fetch_optional(&mut *tx) .await?; not_found_if_none(group_opt, "IGroup", &name)?; + #[cfg(feature = "private")] + let affected_workspaces = + workspaces_referencing_instance_groups(std::slice::from_ref(&name), &mut tx).await?; + + // Drop the workspace memberships this group granted before deleting the membership row, so + // this path takes usr before email_to_igroup like delete_igroup does — the reverse order + // deadlocks the two against each other. Correct on this side of the delete because `name` + // is passed as a removed group and so cannot satisfy the "still qualifies" test. + #[cfg(feature = "private")] + { + use windmill_api_workspaces::workspaces_ee::remove_users_from_instance_group_workspaces; + remove_users_from_instance_group_workspaces( + &email, + &name, + std::slice::from_ref(&name), + &mut tx, + ) + .await?; + } + sqlx::query!( "DELETE FROM email_to_igroup WHERE email = $1 AND igroup = $2", email, @@ -1113,6 +1280,11 @@ async fn remove_user_igroup( .execute(&mut *tx) .await?; + // After the membership row is gone, so surviving members are re-roled from the groups they + // actually still belong to. + #[cfg(feature = "private")] + reprocess_workspace_instance_groups(&affected_workspaces, &mut tx, &authed).await?; + audit_log( &mut *tx, &authed, @@ -1124,13 +1296,6 @@ async fn remove_user_igroup( ) .await?; - // Remove user from workspaces where they were added via this instance group - #[cfg(all(feature = "private", feature = "enterprise"))] - { - use windmill_api_workspaces::workspaces_ee::remove_users_from_instance_group_workspaces; - remove_users_from_instance_group_workspaces(&email, &name, &mut tx).await?; - } - // Recompute instance-level role after group removal let effective_role = compute_effective_instance_role(&email, &mut tx).await?; apply_instance_role(&email, effective_role.as_deref(), &mut tx).await?; @@ -1264,6 +1429,33 @@ async fn overwrite_igroups( require_super_admin(&db, &authed.email).await?; let mut tx = db.begin().await?; + lock_instance_group_mutations(&mut tx).await?; + + let imported_names: Vec = igroups.iter().map(|g| g.name.clone()).collect(); + let previous_names: Vec = sqlx::query_scalar!( + "SELECT name FROM instance_group WHERE name <> ALL($1)", + &imported_names + ) + .fetch_all(&mut *tx) + .await?; + + // Captured before the settings update strips the dropped groups from them. + #[cfg(feature = "private")] + let affected_workspaces = + workspaces_referencing_instance_groups(&previous_names, &mut tx).await?; + + // Captured before the wipe below: afterwards the dropped groups no longer exist, so their + // former members can no longer be resolved. + #[cfg(feature = "private")] + let dropped_members: Vec = sqlx::query_scalar!( + "SELECT DISTINCT email FROM email_to_igroup WHERE igroup = ANY($1)", + &previous_names + ) + .fetch_all(&mut *tx) + .await?; + + remove_instance_groups_from_workspace_settings(&previous_names, &mut tx).await?; + sqlx::query!("DELETE FROM email_to_igroup") .execute(&mut *tx) .await?; @@ -1299,6 +1491,21 @@ async fn overwrite_igroups( } } + // Both run after the re-insert so they see the imported membership. Doing the removal + // earlier would judge a member who moved from a dropped group to a retained one against + // their pre-import groups, delete them, and re-add them stripped of their workspace data. + #[cfg(feature = "private")] + { + use windmill_api_workspaces::workspaces_ee::remove_users_from_instance_group_workspaces; + for email in &dropped_members { + for group in &previous_names { + remove_users_from_instance_group_workspaces(email, group, &previous_names, &mut tx) + .await?; + } + } + reprocess_workspace_instance_groups(&affected_workspaces, &mut tx, &authed).await?; + } + // Propagate instance roles for all groups that have one for igroup in igroups.iter() { if igroup.instance_role.is_some() { diff --git a/backend/windmill-api-integration-tests/tests/groups.rs b/backend/windmill-api-integration-tests/tests/groups.rs index 0fd8ee7e08..50c1f045f0 100644 --- a/backend/windmill-api-integration-tests/tests/groups.rs +++ b/backend/windmill-api-integration-tests/tests/groups.rs @@ -297,3 +297,298 @@ async fn test_group_endpoints(db: Pool) -> anyhow::Result<()> { Ok(()) } + +/// Deleting an instance group must not revoke workspace access a member still holds through +/// another configured group. +/// +/// `cleanup_removed_instance_groups` drops a workspace user whenever `added_via.group` names the +/// deleted group, and that field records only their highest-precedence group — so without a +/// reprocessing pass, deleting the top group also evicts members who still qualify via a lower one. +#[cfg(feature = "private")] +#[sqlx::test(migrations = "../migrations", fixtures("base"))] +async fn test_delete_instance_group_preserves_access_via_other_group( + db: Pool, +) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let global_base = format!("http://localhost:{port}/api/groups"); + let ws_base = format!("http://localhost:{port}/api/w/test-workspace/workspaces"); + + for g in ["igroup_a", "igroup_b"] { + let resp = authed(client().post(format!("{global_base}/create"))) + .json(&json!({ "name": g, "summary": g })) + .send() + .await?; + assert_eq!(resp.status(), 200, "create {g}"); + } + + // multi@ belongs to both groups; only_a@ only to the group that gets deleted. + for (g, email) in [ + ("igroup_a", "multi@example.com"), + ("igroup_b", "multi@example.com"), + ("igroup_a", "only_a@example.com"), + ] { + let resp = authed(client().post(format!("{global_base}/adduser/{g}"))) + .json(&json!({ "email": email })) + .send() + .await?; + assert_eq!(resp.status(), 200, "adduser {g}/{email}"); + } + + // igroup_a grants the higher-precedence role, so added_via lands on it. + let resp = authed(client().post(format!("{ws_base}/edit_instance_groups"))) + .json(&json!({ + "groups": ["igroup_a", "igroup_b"], + "roles": { "igroup_a": "admin", "igroup_b": "developer" } + })) + .send() + .await?; + assert_eq!( + resp.status(), + 200, + "edit_instance_groups: {}", + resp.text().await? + ); + + let (is_admin, via): (bool, Option) = sqlx::query_as( + "SELECT is_admin, added_via->>'group' FROM usr + WHERE workspace_id = 'test-workspace' AND email = 'multi@example.com'", + ) + .fetch_one(&db) + .await?; + assert!(is_admin, "multi@ should start as admin via igroup_a"); + assert_eq!(via.as_deref(), Some("igroup_a")); + + // Workspace state that must survive the group removal. `delete_workspace_user_internal` + // drops all of this, so a delete-and-re-add of a still-qualifying member loses it silently. + let username: String = sqlx::query_scalar( + "SELECT username FROM usr WHERE workspace_id = 'test-workspace' AND email = 'multi@example.com'", + ) + .fetch_one(&db) + .await?; + sqlx::query( + "INSERT INTO favorite (workspace_id, usr, path, favorite_kind) + VALUES ('test-workspace', $1, 'f/keep/me', 'script')", + ) + .bind(&username) + .execute(&db) + .await?; + sqlx::query( + "INSERT INTO draft (workspace_id, path, typ, value) + VALUES ('test-workspace', 'u/' || $1 || '/keep', 'script', '{}'::jsonb)", + ) + .bind(&username) + .execute(&db) + .await?; + + let resp = authed(client().delete(format!("{global_base}/delete/igroup_a"))) + .send() + .await?; + assert_eq!(resp.status(), 200, "delete igroup_a: {}", resp.text().await?); + + // Still a member, downgraded to igroup_b's role rather than evicted. + let (is_admin, is_operator, via): (bool, bool, Option) = sqlx::query_as( + "SELECT is_admin, operator, added_via->>'group' FROM usr + WHERE workspace_id = 'test-workspace' AND email = 'multi@example.com'", + ) + .fetch_one(&db) + .await?; + assert!(!is_admin, "multi@ should lose admin with igroup_a gone"); + assert!(!is_operator, "igroup_b grants developer, not operator"); + assert_eq!( + via.as_deref(), + Some("igroup_b"), + "added_via should re-point at the surviving group" + ); + + // Their workspace state is intact: they were never deleted and re-added. + let favorites: i64 = sqlx::query_scalar( + "SELECT count(*) FROM favorite WHERE workspace_id = 'test-workspace' AND path = 'f/keep/me'", + ) + .fetch_one(&db) + .await?; + assert_eq!(favorites, 1, "favorite must survive losing a non-sole group"); + let drafts: i64 = sqlx::query_scalar( + "SELECT count(*) FROM draft WHERE workspace_id = 'test-workspace' AND path LIKE 'u/%/keep'", + ) + .fetch_one(&db) + .await?; + assert_eq!(drafts, 1, "draft must survive losing a non-sole group"); + + // igroup_a was only_a@'s sole path in, so they are removed. + let remaining: i64 = sqlx::query_scalar( + "SELECT count(*) FROM usr + WHERE workspace_id = 'test-workspace' AND email = 'only_a@example.com'", + ) + .fetch_one(&db) + .await?; + assert_eq!(remaining, 0, "only_a@ should be removed with igroup_a"); + + // The deleted group leaves no dangling reference in either auto_invite field. + let (groups, roles): (serde_json::Value, serde_json::Value) = sqlx::query_as( + "SELECT auto_invite->'instance_groups', auto_invite->'instance_groups_roles' + FROM workspace_settings WHERE workspace_id = 'test-workspace'", + ) + .fetch_one(&db) + .await?; + assert_eq!(groups, json!(["igroup_b"]), "igroup_a should be stripped"); + assert_eq!( + roles, + json!({ "igroup_b": "developer" }), + "igroup_a's role entry should be stripped" + ); + + Ok(()) +} + +/// Removing a member from one instance group must re-derive their role from the groups they +/// still belong to, not leave the privileges the removed group granted. +/// +/// Regression: once still-qualifying members stopped being deleted and re-added, the removal +/// paths had to recompute roles explicitly — otherwise a member dropped from an admin group +/// kept `is_admin` through their stale row. +#[cfg(feature = "private")] +#[sqlx::test(migrations = "../migrations", fixtures("base"))] +async fn test_remove_user_from_instance_group_rederives_role( + db: Pool, +) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let global_base = format!("http://localhost:{port}/api/groups"); + let ws_base = format!("http://localhost:{port}/api/w/test-workspace/workspaces"); + + for g in ["role_a", "role_b"] { + let resp = authed(client().post(format!("{global_base}/create"))) + .json(&json!({ "name": g })) + .send() + .await?; + assert_eq!(resp.status(), 200, "create {g}"); + let resp = authed(client().post(format!("{global_base}/adduser/{g}"))) + .json(&json!({ "email": "demoted@example.com" })) + .send() + .await?; + assert_eq!(resp.status(), 200, "adduser {g}"); + } + + let resp = authed(client().post(format!("{ws_base}/edit_instance_groups"))) + .json(&json!({ + "groups": ["role_a", "role_b"], + "roles": { "role_a": "admin", "role_b": "developer" } + })) + .send() + .await?; + assert_eq!(resp.status(), 200, "edit: {}", resp.text().await?); + + let is_admin: bool = sqlx::query_scalar( + "SELECT is_admin FROM usr WHERE workspace_id = 'test-workspace' AND email = 'demoted@example.com'", + ) + .fetch_one(&db) + .await?; + assert!(is_admin, "should start admin via role_a"); + + // Drop them from the admin group only. + let resp = authed(client().post(format!("{global_base}/removeuser/role_a"))) + .json(&json!({ "email": "demoted@example.com" })) + .send() + .await?; + assert_eq!(resp.status(), 200, "removeuser: {}", resp.text().await?); + + let (is_admin, is_operator, via): (bool, bool, Option) = sqlx::query_as( + "SELECT is_admin, operator, added_via->>'group' FROM usr + WHERE workspace_id = 'test-workspace' AND email = 'demoted@example.com'", + ) + .fetch_one(&db) + .await?; + assert!( + !is_admin, + "admin granted by role_a must not survive removal from role_a" + ); + assert!(!is_operator, "role_b grants developer"); + assert_eq!(via.as_deref(), Some("role_b")); + + Ok(()) +} + +/// An overwrite import that moves a member from a dropped group to a retained one must keep +/// their workspace data. +/// +/// Regression: the qualification check ran before the imported memberships were installed, so +/// the member's new group was not yet visible, they were deleted, and the reprocess pass +/// re-added them stripped of everything workspace-scoped. +#[cfg(feature = "private")] +#[sqlx::test(migrations = "../migrations", fixtures("base"))] +async fn test_overwrite_igroups_preserves_moved_member_data( + db: Pool, +) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let global_base = format!("http://localhost:{port}/api/groups"); + let ws_base = format!("http://localhost:{port}/api/w/test-workspace/workspaces"); + + for g in ["move_from", "move_to"] { + let resp = authed(client().post(format!("{global_base}/create"))) + .json(&json!({ "name": g })) + .send() + .await?; + assert_eq!(resp.status(), 200, "create {g}"); + } + // Member starts only in move_from. + let resp = authed(client().post(format!("{global_base}/adduser/move_from"))) + .json(&json!({ "email": "mover@example.com" })) + .send() + .await?; + assert_eq!(resp.status(), 200); + + let resp = authed(client().post(format!("{ws_base}/edit_instance_groups"))) + .json(&json!({ + "groups": ["move_from", "move_to"], + "roles": { "move_from": "developer", "move_to": "developer" } + })) + .send() + .await?; + assert_eq!(resp.status(), 200, "edit: {}", resp.text().await?); + + let username: String = sqlx::query_scalar( + "SELECT username FROM usr WHERE workspace_id = 'test-workspace' AND email = 'mover@example.com'", + ) + .fetch_one(&db) + .await?; + sqlx::query( + "INSERT INTO favorite (workspace_id, usr, path, favorite_kind) + VALUES ('test-workspace', $1, 'f/moved/keep', 'script')", + ) + .bind(&username) + .execute(&db) + .await?; + + // Import drops move_from entirely and puts the member in move_to instead. + let resp = authed(client().post(format!("{global_base}/overwrite"))) + .json(&json!([ + { "name": "move_to", "emails": ["mover@example.com"] } + ])) + .send() + .await?; + assert_eq!(resp.status(), 200, "overwrite: {}", resp.text().await?); + + let remaining: i64 = sqlx::query_scalar( + "SELECT count(*) FROM usr WHERE workspace_id = 'test-workspace' AND email = 'mover@example.com'", + ) + .fetch_one(&db) + .await?; + assert_eq!(remaining, 1, "member should still be in the workspace via move_to"); + + let favorites: i64 = sqlx::query_scalar( + "SELECT count(*) FROM favorite WHERE workspace_id = 'test-workspace' AND path = 'f/moved/keep'", + ) + .fetch_one(&db) + .await?; + assert_eq!( + favorites, 1, + "moving between groups in one import must not destroy workspace data" + ); + + Ok(()) +}