diff --git a/frontend/src/lib/components/CompareDrafts.svelte b/frontend/src/lib/components/CompareDrafts.svelte index 88107f8bba..55dbde52e1 100644 --- a/frontend/src/lib/components/CompareDrafts.svelte +++ b/frontend/src/lib/components/CompareDrafts.svelte @@ -304,9 +304,9 @@ let selectedItems = $state([]) let deploying = $state(false) - // Whether the user may deploy drafts into this workspace — fills the - // `RestrictDeployToDeployers` (+ operator) gap via the shared util, same as the - // fork compare page and the session review drawer. Fail-open while resolving. + // Whether the user may deploy drafts into this workspace, via the shared util — + // same as the fork compare page and the session review drawer. Fail-open while + // resolving. let deployPerm = $state({ ok: true }) $effect(() => { const ws = currentWorkspaceId diff --git a/frontend/src/lib/components/CompareWorkspaces.svelte b/frontend/src/lib/components/CompareWorkspaces.svelte index 875eda25ce..01f2c811d0 100644 --- a/frontend/src/lib/components/CompareWorkspaces.svelte +++ b/frontend/src/lib/components/CompareWorkspaces.svelte @@ -972,10 +972,9 @@ fetchPermissions() }) - // Can the user actually deploy into the target workspace? Fills the frontend - // gap for the `RestrictDeployToDeployers` rule (+ operator), shared with the - // session review drawer via the same checkDeployPermission util. Cached per - // workspace; `deployPerm` tracks whichever side the current direction targets. + // Can the user actually deploy into the target workspace? Shared with the session + // review drawer via the same checkDeployPermission util. Cached per workspace; + // `deployPerm` tracks whichever side the current direction targets. let deployPerms = $state>({}) const deployPermFetched = new Set() $effect(() => { diff --git a/frontend/src/lib/components/sessions/sessionDeployModel.svelte.ts b/frontend/src/lib/components/sessions/sessionDeployModel.svelte.ts index 8332782cdf..16de19794b 100644 --- a/frontend/src/lib/components/sessions/sessionDeployModel.svelte.ts +++ b/frontend/src/lib/components/sessions/sessionDeployModel.svelte.ts @@ -208,9 +208,9 @@ export function useSessionDeployModel(getArgs: () => SessionDeployModelArgs) { }) // ── Deploy permission ──────────────────────────────────────────────────── - // Preflight the shared checkDeployPermission (operator / RestrictDeployToDeployers) - // for the session workspace so the button disables with a reason instead of - // failing on click. `ok` defaults true while resolving (fail-open). + // Preflight the shared checkDeployPermission for the session workspace so the + // button disables with a reason instead of failing on click. `ok` defaults + // true while resolving (fail-open). let deployPerm = $state({ ok: true }) let deployPermFetchedFor = '' $effect(() => { diff --git a/frontend/src/lib/utils_workspace_deploy.test.ts b/frontend/src/lib/utils_workspace_deploy.test.ts index 08a877d012..771de2e3a7 100644 --- a/frontend/src/lib/utils_workspace_deploy.test.ts +++ b/frontend/src/lib/utils_workspace_deploy.test.ts @@ -1,11 +1,21 @@ -import { describe, it, expect } from 'vitest' +import { describe, it, expect, vi } from 'vitest' +import type { ProtectionRuleset, ProtectionRuleKind, User } from './gen' import { + checkDeployPermission, checkPathWritePermission, diffActionableInDirection, diffCreatesInTarget, diffRemovesInTarget } from './utils_workspace_deploy' +// Only the rules fetch is stubbed: the bypass logic it feeds is the code under test here, +// so it has to stay real. +let rulesets: ProtectionRuleset[] = [] +vi.mock('$lib/workspaceProtectionRules.svelte', async (importOriginal) => ({ + ...((await importOriginal()) as object), + fetchProtectionRulesForWorkspace: async () => rulesets +})) + /** The row shape the fork comparison returns for an item the parent has and the * fork does not: one write on the fork side, whatever that write was. */ const parentOnly = { ahead: 1, behind: 0, exists_in_source: true, exists_in_fork: false } @@ -116,3 +126,72 @@ describe('per-item write permission in the deploy target', () => { }) }) }) + +describe('workspace-level deploy permission', () => { + const ruleset = (name: string, rules: ProtectionRuleKind[]): ProtectionRuleset => ({ + name, + rules, + bypass_users: [], + bypass_groups: [] + }) + const member: User = { + email: 'alice@windmill.dev', + username: 'alice', + is_admin: false, + is_super_admin: false, + operator: false, + disabled: false, + created_at: '2024-01-01T00:00:00Z', + groups: [], + folders: [], + folders_read: [], + folders_owners: [] + } + + const permission = (me: User, rules: ProtectionRuleset[]) => { + rulesets = rules + return checkDeployPermission('prod', me) + } + + it('refuses a member when direct deployment is disabled', async () => { + const res = await permission(member, [ruleset('lock', ['DisableDirectDeployment'])]) + expect(res.ok).toBe(false) + expect(res.reason).toContain('prod') + }) + + // wm_deployers is an implicit pass on RestrictDeployToDeployers only. Letting it + // short-circuit the whole check — as an "is this user a deployer?" early return would — + // walks a deployer straight through a deploy-locked workspace. + it('still refuses a wm_deployers member when direct deployment is disabled', async () => { + const deployer = { ...member, groups: ['wm_deployers'] } + expect((await permission(deployer, [ruleset('lock', ['DisableDirectDeployment'])])).ok).toBe( + false + ) + expect((await permission(deployer, [ruleset('gate', ['RestrictDeployToDeployers'])])).ok).toBe( + true + ) + }) + + // whoami reports is_admin and is_super_admin separately; the server sees them merged. + it('lets a superadmin who is a plain member deploy', async () => { + const su = { ...member, is_super_admin: true } + expect((await permission(su, [ruleset('lock', ['DisableDirectDeployment'])])).ok).toBe(true) + }) + + it('reports the direct-deployment refusal when both rules block', async () => { + const res = await permission(member, [ + ruleset('lock', ['DisableDirectDeployment', 'RestrictDeployToDeployers']) + ]) + expect(res.reason).toContain('Direct deployment') + }) + + // The server refuses an operator in the item handler regardless of their global role: a + // superadmin who is an operator in the workspace still gets 401 creating a script. So the + // operator term has to stay above the admin/superadmin short-circuit — hoisting the admin + // checks would offer a deploy the server refuses. + it('refuses an operator who is also a superadmin', async () => { + const res = await permission({ ...member, operator: true, is_super_admin: true }, []) + expect(res.ok).toBe(false) + expect(res.reason).toContain('operator') + }) +}) diff --git a/frontend/src/lib/utils_workspace_deploy.ts b/frontend/src/lib/utils_workspace_deploy.ts index c330a2210b..e86e62a9aa 100644 --- a/frontend/src/lib/utils_workspace_deploy.ts +++ b/frontend/src/lib/utils_workspace_deploy.ts @@ -706,13 +706,16 @@ export async function createFolderIfAbsent( export type DeployPermission = { ok: boolean; reason?: string } /** - * Whether the current user may deploy into `workspace`. Mirrors the server-side - * deploy authorization (`check_user_against_rule` in windmill-common) so the UI - * can disable the action with a reason instead of letting the click 403: - * - operators can never deploy; - * - when the `RestrictDeployToDeployers` protection rule is active, only - * admins, `wm_deployers` members (implicitly), and per-ruleset bypass - * users/groups may deploy. + * Whether the current user may deploy into `workspace`. Mirrors `check_deploy_rules` in + * windmill-common so the UI can disable the action with a reason instead of letting the + * click 403: `DisableDirectDeployment` is evaluated before `RestrictDeployToDeployers`, so + * the same message wins here as on the server when both block; admins and superadmins bypass + * both rules, while `wm_deployers` members bypass only the latter. + * + * The operator refusal is not part of that mirror. The server refuses operators in the item + * handlers instead, and for fewer kinds, so refusing them for everything here is deliberately + * stricter than the server rather than a faithful copy of it. + * * Fails open on any error — the server still enforces on the actual deploy. * Shared by the session dock and the compare page so both gate identically. */ @@ -726,17 +729,32 @@ export async function checkDeployPermission( if (me.operator) { return { ok: false, reason: "You're an operator in this workspace — operators can't deploy" } } - // Admins and wm_deployers members always satisfy RestrictDeployToDeployers - // (the backend allows wm_deployers implicitly, so check it before the - // per-ruleset bypass_users/bypass_groups fallback). - const isDeployer = me.is_admin || (me.groups ?? []).includes('wm_deployers') - if (!isDeployer) { + const userInfo = { + is_admin: !!me.is_admin, + is_super_admin: !!me.is_super_admin, + username: me.username, + groups: me.groups ?? [] + } + // A superadmin who is only a plain member of the workspace still bypasses every rule, + // so dropping either term here refuses a deploy the server accepts. + if (!userInfo.is_admin && !userInfo.is_super_admin) { const rulesets = await fetchProtectionRulesForWorkspace(workspace) - const userInfo = { is_admin: !!me.is_admin, username: me.username, groups: me.groups ?? [] } - if (!canUserBypassRuleKindInRulesets(rulesets, 'RestrictDeployToDeployers', userInfo)) { + if (!canUserBypassRuleKindInRulesets(rulesets, 'DisableDirectDeployment', userInfo)) { return { ok: false, - reason: 'Only workspace admins and members of wm_deployers can deploy here' + reason: `Direct deployment to ${workspace} is disabled — fork the workspace or open a pull request` + } + } + // `wm_deployers` membership is an implicit pass on this rule only, so it cannot + // short-circuit the fetch above the way admin does. + const isDeployer = userInfo.groups.includes('wm_deployers') + if ( + !isDeployer && + !canUserBypassRuleKindInRulesets(rulesets, 'RestrictDeployToDeployers', userInfo) + ) { + return { + ok: false, + reason: `Only workspace admins and members of wm_deployers can deploy to ${workspace}` } } } diff --git a/frontend/src/lib/workspaceProtectionRules.svelte.ts b/frontend/src/lib/workspaceProtectionRules.svelte.ts index 40eeee719b..01d122794d 100644 --- a/frontend/src/lib/workspaceProtectionRules.svelte.ts +++ b/frontend/src/lib/workspaceProtectionRules.svelte.ts @@ -4,7 +4,7 @@ import type { UserExt } from './stores' // The slice of the user identity the bypass checks read — structural, so // callers can pass a whoami response (normalised groups) as well as the // UserExt store value. -export type RuleBypassUser = Pick +export type RuleBypassUser = Pick // Mirrors DEV_WORKSPACE_LOCK_RULE_NAME in windmill-common. The pairing owns this rule by name: // attaching a dev workspace creates it, detaching deletes it. The API refuses to create or delete @@ -102,11 +102,12 @@ export async function fetchProtectionRulesForWorkspace( * Checks if a user can bypass a specific ruleset * @param ruleset The protection ruleset to check * @param userInfo The user information - * @returns true if user can bypass (is admin, in bypass_users, or has group in bypass_groups) + * @returns true if user can bypass (is admin or superadmin, in bypass_users, or has group in bypass_groups) */ export function canUserBypassRule(ruleset: ProtectionRuleset, userInfo: RuleBypassUser): boolean { - // Admin always bypasses - if (userInfo.is_admin) { + // The server bypasses on `ApiAuthed.is_admin`, which it builds as `usr.is_admin || + // super_admin`, so testing `is_admin` alone understates a superadmin who is a plain member. + if (userInfo.is_admin || userInfo.is_super_admin) { return true }