fix(groups): recompute member roles when an instance group role is cleared (#11708)

* fix(groups): recompute member roles when an instance group role is cleared

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix: restore sqlx cache entry still used by the scim group delete

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

backend/windmill-api-groups/src/groups.rs

@@ -470,26 +470,16 @@ async fn update_igroup(
     require_super_admin(&db, &authed).await?;
     let mut tx: Transaction<'_, Postgres> = db.begin().await?;
 
-    let exists_opt = sqlx::query("SELECT 1 FROM instance_group WHERE name = $1")
-        .bind(name.clone())
-        .fetch_optional(&mut *tx)
-        .await?;
-    not_found_if_none(exists_opt, "instance_group", name.clone())?;
+    let old_role_opt = sqlx::query_scalar!(
+        "SELECT instance_role FROM instance_group WHERE name = $1 FOR UPDATE",
+        &name
+    )
+    .fetch_optional(&mut *tx)
+    .await?;
+    let old_role = not_found_if_none(old_role_opt, "instance_group", name.clone())?;
 
     let validated_role = validate_instance_role(&igroup_update.instance_role)?;
 
-    // Fetch old role before updating so we can detect changes
-    let old_role = if igroup_update.instance_role.is_some() {
-        sqlx::query_scalar!(
-            "SELECT instance_role FROM instance_group WHERE name = $1",
-            &name
-        )
-        .fetch_one(&mut *tx)
-        .await?
-    } else {
-        None
-    };
-
     sqlx::query("UPDATE instance_group SET summary = $1, instance_role = $2 WHERE name = $3")
         .bind(igroup_update.new_summary)
         .bind(&validated_role)
@@ -497,8 +487,9 @@ async fn update_igroup(
         .execute(&mut *tx)
         .await?;
 
-    // If instance_role actually changed, propagate to all group members
-    if igroup_update.instance_role.is_some() && old_role != validated_role {
+    // An omitted or null instance_role is stored as NULL above, so it revokes like any other
+    // change: members must be recomputed whenever the stored role differs.
+    if old_role != validated_role {
         propagate_instance_group_roles(&name, &mut tx).await?;
     }
 
@@ -664,21 +655,19 @@ async fn delete_igroup(
 
     // FOR UPDATE: the group row is the group-level mutex, taken before the workspace
     // advisory locks (see reconcile_workspace_instance_groups).
-    let group_role = sqlx::query_scalar!(
+    sqlx::query_scalar!(
         "SELECT instance_role FROM instance_group WHERE name = $1 FOR UPDATE",
         &name
     )
     .fetch_optional(&mut *tx)
-    .await?
-    .flatten();
+    .await?;
 
-    let affected_members: Vec<String> = if group_role.is_some() {
+    // Collected whatever the group's current role: a member can still hold a role this group
+    // granted before it was cleared.
+    let affected_members: Vec<String> =
         sqlx::query_scalar!("SELECT email FROM email_to_igroup WHERE igroup = $1", &name)
             .fetch_all(&mut *tx)
-            .await?
-    } else {
-        vec![]
-    };
+            .await?;
 
     // Captured and advisory-locked before the settings update strips the group from them.
     #[cfg(feature = "private")]

backend/windmill-api-integration-tests/tests/groups.rs

@@ -497,6 +497,72 @@ async fn test_remove_user_from_instance_group_rederives_role(
     Ok(())
 }
 
+/// Clearing an instance group's role, or deleting a group whose role was cleared out from under
+/// its members, must take the instance-level role back from them.
+#[sqlx::test(migrations = "../migrations", fixtures("base"))]
+async fn test_instance_group_role_revoked_on_clear_and_delete(
+    db: Pool<Postgres>,
+) -> 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 email = "test2@windmill.dev";
+
+    let is_super_admin = || async {
+        sqlx::query_scalar::<_, bool>("SELECT super_admin FROM password WHERE email = $1")
+            .bind(email)
+            .fetch_one(&db)
+            .await
+    };
+
+    let resp = authed(client().post(format!("{global_base}/create")))
+        .json(&json!({ "name": "admins_grp" }))
+        .send()
+        .await?;
+    assert_eq!(resp.status(), 200, "create: {}", resp.text().await?);
+    let resp = authed(client().post(format!("{global_base}/update/admins_grp")))
+        .json(&json!({ "new_summary": "", "instance_role": "superadmin" }))
+        .send()
+        .await?;
+    assert_eq!(resp.status(), 200, "set role: {}", resp.text().await?);
+    let resp = authed(client().post(format!("{global_base}/adduser/admins_grp")))
+        .json(&json!({ "email": email }))
+        .send()
+        .await?;
+    assert_eq!(resp.status(), 200, "adduser: {}", resp.text().await?);
+    assert!(is_super_admin().await?, "group should grant superadmin");
+
+    let resp = authed(client().post(format!("{global_base}/update/admins_grp")))
+        .json(&json!({ "new_summary": "", "instance_role": null }))
+        .send()
+        .await?;
+    assert_eq!(resp.status(), 200, "update: {}", resp.text().await?);
+    assert!(
+        !is_super_admin().await?,
+        "clearing the group's role must revoke superadmin"
+    );
+
+    // A member left elevated while the group's stored role is already NULL.
+    sqlx::query(
+        "UPDATE password SET super_admin = true, role_source = 'instance_group' WHERE email = $1",
+    )
+    .bind(email)
+    .execute(&db)
+    .await?;
+
+    let resp = authed(client().delete(format!("{global_base}/delete/admins_grp")))
+        .send()
+        .await?;
+    assert_eq!(resp.status(), 200, "delete: {}", resp.text().await?);
+    assert!(
+        !is_super_admin().await?,
+        "deleting a role-cleared group must revoke superadmin"
+    );
+
+    Ok(())
+}
+
 /// An overwrite import that moves a member from a dropped group to a retained one must keep
 /// their workspace data.
 ///