diff --git a/backend/.sqlx/query-255ba68caa78d0e814ea817693a319f769907025f9c7d1150d45e8ecb1bff4ab.json b/backend/.sqlx/query-255ba68caa78d0e814ea817693a319f769907025f9c7d1150d45e8ecb1bff4ab.json deleted file mode 100644 index 245c65a7a8..0000000000 --- a/backend/.sqlx/query-255ba68caa78d0e814ea817693a319f769907025f9c7d1150d45e8ecb1bff4ab.json +++ /dev/null @@ -1,15 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "UPDATE workspace_settings SET auto_invite = $1 WHERE workspace_id = $2", - "describe": { - "columns": [], - "parameters": { - "Left": [ - "Jsonb", - "Text" - ] - }, - "nullable": [] - }, - "hash": "255ba68caa78d0e814ea817693a319f769907025f9c7d1150d45e8ecb1bff4ab" -} diff --git a/backend/.sqlx/query-a1bcf143135ecb9f32709a56a353d3246dc238920c3e155caa9cf767246fa6fb.json b/backend/.sqlx/query-a1bcf143135ecb9f32709a56a353d3246dc238920c3e155caa9cf767246fa6fb.json new file mode 100644 index 0000000000..18ee22083e --- /dev/null +++ b/backend/.sqlx/query-a1bcf143135ecb9f32709a56a353d3246dc238920c3e155caa9cf767246fa6fb.json @@ -0,0 +1,15 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE workspace_settings SET auto_invite = (COALESCE(auto_invite, '{}'::jsonb) - 'domain') || $1::jsonb WHERE workspace_id = $2", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Jsonb", + "Text" + ] + }, + "nullable": [] + }, + "hash": "a1bcf143135ecb9f32709a56a353d3246dc238920c3e155caa9cf767246fa6fb" +} diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index cc50737a1d..3d9d49412a 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -7e338e4dabf91689bfd7fb0333c6534040b17b59 +f2fced19fcae81de7f6dac545010ce404c052e1b diff --git a/backend/windmill-api-integration-tests/tests/workspaces.rs b/backend/windmill-api-integration-tests/tests/workspaces.rs index 25312cae27..ff603269e7 100644 --- a/backend/windmill-api-integration-tests/tests/workspaces.rs +++ b/backend/windmill-api-integration-tests/tests/workspaces.rs @@ -1166,3 +1166,47 @@ async fn test_create_service_account_drops_orphaned_group_memberships( Ok(()) } + +#[cfg(feature = "private")] +#[sqlx::test(migrations = "../migrations", fixtures("base"))] +async fn test_edit_auto_invite_preserves_instance_groups(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + + sqlx::query( + r#"UPDATE workspace_settings + SET auto_invite = '{"instance_groups": ["eng"], "instance_groups_roles": {"eng": "developer"}}' + WHERE workspace_id = 'test-workspace'"#, + ) + .execute(&db) + .await?; + + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let base = format!("http://localhost:{port}/api/w/test-workspace/workspaces"); + + // enable, then disable + for body in [ + json!({"operator": false, "invite_all": true, "auto_add": false}), + json!({}), + ] { + let resp = authed(client().post(format!("{base}/edit_auto_invite"))) + .json(&body) + .send() + .await?; + assert_eq!(resp.status(), 200, "{body}: {}", resp.text().await?); + + let auto_invite: serde_json::Value = sqlx::query_scalar( + "SELECT auto_invite FROM workspace_settings WHERE workspace_id = 'test-workspace'", + ) + .fetch_one(&db) + .await?; + assert_eq!(auto_invite["instance_groups"], json!(["eng"]), "{body}"); + assert_eq!( + auto_invite["instance_groups_roles"], + json!({"eng": "developer"}), + "{body}" + ); + } + + Ok(()) +} diff --git a/cli/src/commands/sync/sync.ts b/cli/src/commands/sync/sync.ts index 138a346594..6040afc747 100644 --- a/cli/src/commands/sync/sync.ts +++ b/cli/src/commands/sync/sync.ts @@ -2546,9 +2546,9 @@ export function preservePendingScriptLocks( } // `sync push` never applies the workspace's display name from settings.yaml and -// applies its color only when the local file carries one (see -// pushWorkspaceSettings), so on a push the fields it would not apply must -// compare equal, or the row is listed on every run. +// applies its color and auto_invite.instance_groups only when the local file +// carries them (see pushWorkspaceSettings), so on a push the fields it would not +// apply must compare equal, or the row is listed on every run. const isWorkspaceSettingsFile = (p: string) => /^settings(\.[^./\\]+)?\.(yaml|json)$/.test(p); function stripUnappliedSettingsFields(local: any, remote: any) { @@ -2558,6 +2558,20 @@ function stripUnappliedSettingsFields(local: any, remote: any) { delete local?.color; delete remote?.color; } + // push reads a missing auto_invite as {} on both sides + if (local) local.auto_invite ??= {}; + if (remote) remote.auto_invite ??= {}; + const localInvite = local?.auto_invite; + const remoteInvite = remote?.auto_invite; + if (localInvite?.instance_groups == null) { + for (const invite of [localInvite, remoteInvite]) { + delete invite?.instance_groups; + delete invite?.instance_groups_roles; + } + } else { + localInvite.instance_groups_roles ??= {}; + if (remoteInvite) remoteInvite.instance_groups_roles ??= {}; + } } export async function compareDynFSElement( diff --git a/cli/src/core/settings.ts b/cli/src/core/settings.ts index 4174ec0358..6a9358b468 100644 --- a/cli/src/core/settings.ts +++ b/cli/src/core/settings.ts @@ -239,8 +239,19 @@ export async function pushWorkspaceSettings( }); } - // Handle auto_invite using grouped format - if (!deepEqual(localSettings.auto_invite, settings.auto_invite)) { + // Handle auto_invite using grouped format. The domain invite and the instance groups + // are applied by separate endpoints, each rewriting only its own keys. + const { + instance_groups: localGroups, + instance_groups_roles: localGroupRoles, + ...localDomainInvite + } = localSettings.auto_invite ?? {}; + const { + instance_groups: remoteGroups, + instance_groups_roles: remoteGroupRoles, + ...remoteDomainInvite + } = settings.auto_invite ?? {}; + if (!deepEqual(localDomainInvite, remoteDomainInvite)) { log.debug(`Updating auto invite...`); const localAutoInvite = localSettings.auto_invite; @@ -278,6 +289,20 @@ export async function pushWorkspaceSettings( } } + // Only when settings.yaml declares instance_groups: clearing a group removes the + // workspace members it granted, so an absent key must never clear it. + if ( + localGroups != undefined && + (!deepEqual(localGroups, remoteGroups) || + !deepEqual(localGroupRoles ?? {}, remoteGroupRoles ?? {})) + ) { + log.debug(`Updating instance groups...`); + await wmill.editInstanceGroups({ + workspace, + requestBody: { groups: localGroups, roles: localGroupRoles ?? {} }, + }); + } + if (!deepEqual(localSettings.ai_config, settings.ai_config)) { log.debug(`Updating copilot settings...`); await wmill.editCopilotConfig({ diff --git a/cli/test/push_diff_convergence_unit.test.ts b/cli/test/push_diff_convergence_unit.test.ts index 67f6d5b4dd..a6f2c132c0 100644 --- a/cli/test/push_diff_convergence_unit.test.ts +++ b/cli/test/push_diff_convergence_unit.test.ts @@ -320,3 +320,29 @@ test("push: settings.yaml differing only by name or an unset color is not a chan }); expect(await diff(otherColor, remote, skips)).toEqual(["edited settings.yaml"]); }); + +// A push applies auto_invite.instance_groups only when the local file declares +// them (see pushWorkspaceSettings). +test("push: settings.yaml without instance_groups is not a change", async () => { + const remote = local({ + "settings.yaml": + "name: prod\nauto_invite:\n enabled: false\n instance_groups:\n - eng\n instance_groups_roles:\n eng: developer\n", + }); + const undeclared = local({ + "settings.yaml": "name: prod\nauto_invite:\n enabled: false\n", + }); + const skips = { includeSettings: true }; + expect(await diff(undeclared, remote, skips)).toEqual([]); + + const groupsOnlyRemote = local({ + "settings.yaml": "name: prod\nauto_invite:\n instance_groups:\n - eng\n", + }); + const noAutoInvite = local({ "settings.yaml": "name: prod\n" }); + expect(await diff(noAutoInvite, groupsOnlyRemote, skips)).toEqual([]); + + const otherGroups = local({ + "settings.yaml": + "name: prod\nauto_invite:\n enabled: false\n instance_groups: []\n", + }); + expect(await diff(otherGroups, remote, skips)).toEqual(["edited settings.yaml"]); +}); diff --git a/cli/test/push_workspace_settings_auto_invite_unit.test.ts b/cli/test/push_workspace_settings_auto_invite_unit.test.ts new file mode 100644 index 0000000000..b04d895056 --- /dev/null +++ b/cli/test/push_workspace_settings_auto_invite_unit.test.ts @@ -0,0 +1,82 @@ +/** + * Regression guard: `sync push` (pushWorkspaceSettings) applies the domain invite and + * the instance groups of `auto_invite` through their own endpoints, and never clears + * instance groups that settings.yaml does not declare. + */ + +import { expect, test, describe, beforeEach, mock } from "bun:test"; + +let editAutoInviteCalls: unknown[] = []; +let editInstanceGroupsCalls: unknown[] = []; +const remoteAutoInvite = { + enabled: true, + domain: "*", + operator: false, + mode: "invite", + instance_groups: ["eng"], + instance_groups_roles: { eng: "developer" }, +}; + +// Every wmill.* call reachable from pushWorkspaceSettings is stubbed: bun shares one +// mocked module across test files, and names missing from whichever mock loads first +// stay missing for the others. +mock.module("../gen/services.gen.ts", () => ({ + getSettings: async () => ({ auto_invite: remoteAutoInvite }), + getWorkspaceName: async () => "phoenix", + changeWorkspaceName: async () => {}, + changeWorkspaceColor: async () => {}, + editWebhook: async () => {}, + editAutoInvite: async (a: unknown) => { + editAutoInviteCalls.push(a); + }, + editInstanceGroups: async (a: unknown) => { + editInstanceGroupsCalls.push(a); + }, + editErrorHandler: async () => {}, + editSuccessHandler: async () => {}, + editCopilotConfig: async () => {}, + editLargeFileStorageConfig: async () => {}, + editWorkspaceGitSyncConfig: async () => {}, + editWorkspaceDefaultApp: async () => {}, + editDefaultScripts: async () => {}, + workspaceMuteCriticalAlertsUi: async () => {}, + updateOperatorSettings: async () => {}, + editDataTableConfig: async () => {}, + editSlackCommand: async () => {}, + setWorkspaceSlackOauthConfig: async () => {}, + deleteWorkspaceSlackOauthConfig: async () => {}, +})); + +const { pushWorkspaceSettings } = await import("../src/core/settings.ts"); + +describe("pushWorkspaceSettings auto_invite", () => { + beforeEach(() => { + editAutoInviteCalls = []; + editInstanceGroupsCalls = []; + }); + + test("an instance-group-only change updates the groups and leaves the domain invite", async () => { + await pushWorkspaceSettings("phoenix", "settings", undefined, { + name: "phoenix", + auto_invite: { ...remoteAutoInvite, instance_groups_roles: { eng: "admin" } }, + }); + expect(editAutoInviteCalls.length).toBe(0); + expect(editInstanceGroupsCalls).toEqual([ + { + workspace: "phoenix", + requestBody: { groups: ["eng"], roles: { eng: "admin" } }, + }, + ]); + }); + + test("a settings.yaml without instance_groups does not clear them", async () => { + const { instance_groups: _g, instance_groups_roles: _r, ...domainInvite } = + remoteAutoInvite; + await pushWorkspaceSettings("phoenix", "settings", undefined, { + name: "phoenix", + auto_invite: { ...domainInvite, operator: true }, + }); + expect(editAutoInviteCalls.length).toBe(1); + expect(editInstanceGroupsCalls.length).toBe(0); + }); +}); diff --git a/cli/test/push_workspace_settings_identity_unit.test.ts b/cli/test/push_workspace_settings_identity_unit.test.ts index c583a53282..5dcf9815ef 100644 --- a/cli/test/push_workspace_settings_identity_unit.test.ts +++ b/cli/test/push_workspace_settings_identity_unit.test.ts @@ -31,6 +31,7 @@ mock.module("../gen/services.gen.ts", () => ({ editWebhookCalls.push(a); }, editAutoInvite: async () => {}, + editInstanceGroups: async () => {}, editErrorHandler: async () => {}, editSuccessHandler: async () => {}, editCopilotConfig: async () => {},