From 84c28175208968262fdb11301475bd6f2b554b85 Mon Sep 17 00:00:00 2001 From: wendrul Date: Tue, 31 Mar 2026 18:16:44 +0200 Subject: [PATCH] fix: hasOnBehalfOf instead of no field on yamls --- cli/src/commands/flow/flow.ts | 7 +- cli/src/commands/script/script.ts | 7 +- cli/src/commands/sync/sync.ts | 7 +- cli/src/core/permissioned_as.ts | 126 ++++++++++++++++++++----- cli/test/permissioned_as.test.ts | 152 ++++++++++++++++++++++++++++++ 5 files changed, 268 insertions(+), 31 deletions(-) diff --git a/cli/src/commands/flow/flow.ts b/cli/src/commands/flow/flow.ts index d35753f98b..29da99b224 100644 --- a/cli/src/commands/flow/flow.ts +++ b/cli/src/commands/flow/flow.ts @@ -40,6 +40,7 @@ export interface FlowFile { value: any; schema?: any; on_behalf_of_email?: string; + has_on_behalf_of?: boolean; } function normalizeOptionalString(value: string | null | undefined): string | undefined { @@ -170,9 +171,13 @@ export async function pushFlow( await replaceInlineScripts([localFlow.value.preprocessor_module], fileReader, log, localPath, SEP); } + // Extract CLI-only field before sending to API + const hasOnBehalfOf = localFlow.has_on_behalf_of ?? !!localFlow.on_behalf_of_email; + delete (localFlow as any).has_on_behalf_of; + // Build preserve flags for permissioned_as const preserveFields: Partial = {}; - if (permissionedAsContext?.userIsAdminOrDeployer) { + if (permissionedAsContext?.userIsAdminOrDeployer && hasOnBehalfOf) { if (flow) { // Updating: preserve the remote's on_behalf_of_email (only if it has one) if (flow.on_behalf_of_email) { diff --git a/cli/src/commands/script/script.ts b/cli/src/commands/script/script.ts index 2d899b4f17..7c7a516259 100644 --- a/cli/src/commands/script/script.ts +++ b/cli/src/commands/script/script.ts @@ -449,8 +449,11 @@ export async function handleFile( modules: modules, }; + // Compute whether original remote had on_behalf_of set + const hasOnBehalfOf = typed?.has_on_behalf_of ?? !!typed?.on_behalf_of_email; + // Add preserve flags for permissioned_as - if (permissionedAsContext?.userIsAdminOrDeployer) { + if (permissionedAsContext?.userIsAdminOrDeployer && hasOnBehalfOf) { if (remote) { // Updating: preserve the remote's on_behalf_of_email (only if it has one) if (remote.on_behalf_of_email) { @@ -506,7 +509,7 @@ export async function handleFile( typed.debounce_key == remote["debounce_key"] && typed.debounce_delay_s == remote["debounce_delay_s"] && typed.codebase == remote.codebase && - typed.on_behalf_of_email == remote.on_behalf_of_email && + (typed.has_on_behalf_of !== undefined ? true : typed.on_behalf_of_email == remote.on_behalf_of_email) && deepEqual(typed.envs, remote.envs) && deepEqual(modules ?? null, remote.modules ?? null)) ) { diff --git a/cli/src/commands/sync/sync.ts b/cli/src/commands/sync/sync.ts index 765eafb9c5..663065aecb 100644 --- a/cli/src/commands/sync/sync.ts +++ b/cli/src/commands/sync/sync.ts @@ -692,6 +692,7 @@ function ZipFSElement( } if (stripOnBehalfOf) { + (flow as any).has_on_behalf_of = !!(flow as any).on_behalf_of_email; delete (flow as any).on_behalf_of_email; } @@ -976,6 +977,7 @@ function ZipFSElement( parsed["codebase"] = undefined; } if (stripOnBehalfOf) { + parsed["has_on_behalf_of"] = !!parsed["on_behalf_of_email"]; delete parsed["on_behalf_of_email"]; } // Modules are stored as files in __mod/ folder, not in metadata @@ -2829,13 +2831,14 @@ export async function push( userIsAdminOrDeployer, }; - // Pre-check: warn non-admin/non-deployer users about permissioned_as changes + // Pre-check: warn about permissioned_as changes await preCheckPermissionedAs( changes, user.email, userIsAdminOrDeployer, opts.acceptOverridingPermissionedAsWithSelf ?? false, - !!process.stdin.isTTY + !!process.stdin.isTTY, + validatedRules ); } diff --git a/cli/src/core/permissioned_as.ts b/cli/src/core/permissioned_as.ts index cf428afa54..dde8a240f8 100644 --- a/cli/src/core/permissioned_as.ts +++ b/cli/src/core/permissioned_as.ts @@ -139,10 +139,40 @@ export interface Change { } /** - * Pre-checks whether items being pushed will have their permissioned_as/email changed - * because the deploying user is not an admin or deployer. + * Extract the remote path (used for rule matching) from a local file path. + */ +function extractRemotePathForRuleCheck(filePath: string, typeStr: string): string | undefined { + if (typeStr === "script") { + const match = filePath.match(/^(.+)\.script\.(yaml|json)$/); + return match ? match[1] : undefined; + } + if (typeStr === "flow") { + // Handle both .flow/ and __flow/ suffixes + const match = filePath.match(/^(.+?)(?:\.flow|__flow)\//); + return match ? match[1] : undefined; + } + return undefined; +} + +/** + * Check if content has on_behalf_of set (new has_on_behalf_of boolean or legacy email). + */ +function contentHasOnBehalfOf(content: string, typeStr: string): boolean { + if (typeStr === "script") { + return !!content.match(/has_on_behalf_of:\s*(true)/) || + !!content.match(/on_behalf_of_email:\s*["']?([^\s"']+)["']?/); + } + if (typeStr === "flow") { + return !!content.match(/has_on_behalf_of:\s*(true)/); + } + return false; +} + +/** + * Pre-checks whether items being pushed will have their permissioned_as/email changed. * - * For admins/deployers, preserve flags will be sent with the API calls, so no warning is needed. + * For admins/deployers editing existing items, preserve flags handle ownership — no warning needed. + * For admins/deployers creating items with has_on_behalf_of but no matching rule, warn/prompt. * For non-admin/non-deployer users, the API will silently overwrite the owner to the deploying user. */ export async function preCheckPermissionedAs( @@ -150,19 +180,12 @@ export async function preCheckPermissionedAs( userEmail: string, userIsAdminOrDeployer: boolean, acceptOverride: boolean, - isInteractive: boolean + isInteractive: boolean, + rules: PermissionedAsRule[] = [] ): Promise { - if (userIsAdminOrDeployer) { - return; - } - const wouldChangeItems: { path: string; currentOwner: string }[] = []; for (const change of changes) { - if (change.name !== "edited") { - continue; - } - let typeStr: string; try { typeStr = getTypeStrFromPath(change.path); @@ -170,6 +193,44 @@ export async function preCheckPermissionedAs( continue; } + // --- "added" changes: new items being created --- + if (change.name === "added") { + const content = change.content; + if (!content) continue; + + const isScriptMeta = typeStr === "script" && + (change.path.endsWith(".script.yaml") || change.path.endsWith(".script.json")); + const isFlowMeta = typeStr === "flow" && + (change.path.endsWith("flow.yaml") || change.path.endsWith("flow.json")); + + if ((isScriptMeta || isFlowMeta) && contentHasOnBehalfOf(content, typeStr)) { + if (userIsAdminOrDeployer) { + // Admins can apply rules — only flag if no rule matches this path + const remotePath = extractRemotePathForRuleCheck(change.path, typeStr); + if (!remotePath || !resolvePermissionedAsRule(remotePath, rules)) { + const label = typeStr === "script" ? "(script owner)" : "(flow owner)"; + wouldChangeItems.push({ path: change.path, currentOwner: label }); + } + } else { + const label = typeStr === "script" ? "(script owner)" : "(flow owner)"; + wouldChangeItems.push({ path: change.path, currentOwner: label }); + } + } else if (typeStr === "app" && !userIsAdminOrDeployer) { + wouldChangeItems.push({ path: change.path, currentOwner: "(app policy owner)" }); + } + continue; + } + + // --- "edited" changes --- + if (change.name !== "edited") { + continue; + } + + // For edits, admins preserve from remote — no warning needed + if (userIsAdminOrDeployer) { + continue; + } + const beforeContent = change.before; if (!beforeContent) continue; @@ -181,20 +242,31 @@ export async function preCheckPermissionedAs( change.path.endsWith(".script.yaml") || change.path.endsWith(".script.json") ) { - const match = beforeContent.match( - /on_behalf_of_email:\s*["']?([^\s"']+)["']?/ - ); - if (match) { - currentOwner = match[1]; + // New format: has_on_behalf_of boolean + const hasOboMatch = beforeContent.match(/has_on_behalf_of:\s*(true)/); + if (hasOboMatch) { + currentOwner = "(script owner)"; + } else { + // Legacy format: on_behalf_of_email directly in file + const emailMatch = beforeContent.match( + /on_behalf_of_email:\s*["']?([^\s"']+)["']?/ + ); + if (emailMatch) { + currentOwner = emailMatch[1]; + } } } } else if (typeStr === "flow") { - // Flow on_behalf_of_email is stripped during sync pull, so we can't - // reliably detect the current owner from local files. Always flag it. - wouldChangeItems.push({ - path: change.path, - currentOwner: "(flow owner)", - }); + // Only flag when has_on_behalf_of: true is present + if (change.path.endsWith("flow.yaml") || change.path.endsWith("flow.json")) { + const hasOboMatch = beforeContent.match(/has_on_behalf_of:\s*(true)/); + if (hasOboMatch) { + wouldChangeItems.push({ + path: change.path, + currentOwner: "(flow owner)", + }); + } + } continue; } else if (typeStr === "app") { // Apps always have on_behalf_of set - any edited app will change owner @@ -233,9 +305,11 @@ export async function preCheckPermissionedAs( .map((item) => ` - ${item.path} (current owner: ${item.currentOwner})`) .join("\n"); - const message = - `You are not an admin or member of 'wm_deployers'. The following ${wouldChangeItems.length} item(s) ` + - `will have their permissioned_as/email changed to your user (${userEmail}):\n${itemList}`; + const message = userIsAdminOrDeployer + ? `The following ${wouldChangeItems.length} item(s) have on_behalf_of set but no matching defaultPermissionedAs rule in wmill.yaml. ` + + `They will be created with your user (${userEmail}) as permissioned_as:\n${itemList}` + : `You are not an admin or member of 'wm_deployers'. The following ${wouldChangeItems.length} item(s) ` + + `will have their permissioned_as/email changed to your user (${userEmail}):\n${itemList}`; if (acceptOverride) { log.warn(colors.yellow(`Warning: ${message}`)); diff --git a/cli/test/permissioned_as.test.ts b/cli/test/permissioned_as.test.ts index 7a868379cf..6f88b3deb2 100644 --- a/cli/test/permissioned_as.test.ts +++ b/cli/test/permissioned_as.test.ts @@ -3,10 +3,13 @@ */ import { expect, test, describe } from "bun:test"; +import { mock } from "bun:test"; import { validatePermissionedAsRules, resolvePermissionedAsRule, + preCheckPermissionedAs, type PermissionedAsRule, + type Change, } from "../src/core/permissioned_as.ts"; // ============================================================================= @@ -269,3 +272,152 @@ describe("resolvePermissionedAsRule", () => { ).toBeUndefined(); }); }); + +// ============================================================================= +// preCheckPermissionedAs — has_on_behalf_of gating +// ============================================================================= + +describe("preCheckPermissionedAs", () => { + const userEmail = "user@example.com"; + + // Helper to check if preCheck would exit (flag items) + async function expectFlagged(fn: () => Promise) { + const originalExit = process.exit; + let exitCalled = false; + process.exit = ((code?: number) => { exitCalled = true; }) as any; + try { + await fn(); + expect(exitCalled).toBe(true); + } finally { + process.exit = originalExit; + } + } + + // Helper: make a script edit change + function scriptEdit(before: string): Change { + return { + name: "edited", + path: "f/my_script.script.yaml", + before, + after: "summary: updated\n", + }; + } + + // Helper: make a script added change + function scriptAdded(content: string, path = "f/my_script.script.yaml"): Change { + return { name: "added", path, content }; + } + + // Helper: make a flow edit change + function flowEdit(before: string): Change { + return { + name: "edited", + path: "f/my_flow.flow/flow.yaml", + before, + after: "summary: updated\n", + }; + } + + // Helper: make a flow added change + function flowAdded(content: string, path = "f/my_flow.flow/flow.yaml"): Change { + return { name: "added", path, content }; + } + + // --- Non-admin, edited changes --- + + test("non-admin: edited script with has_on_behalf_of: true is flagged", async () => { + await expectFlagged(() => + preCheckPermissionedAs([scriptEdit("summary: test\nhas_on_behalf_of: true\n")], userEmail, false, false, false) + ); + }); + + test("non-admin: edited script with has_on_behalf_of: false is not flagged", async () => { + await preCheckPermissionedAs([scriptEdit("summary: test\nhas_on_behalf_of: false\n")], userEmail, false, false, false); + }); + + test("non-admin: edited flow with has_on_behalf_of: true is flagged", async () => { + await expectFlagged(() => + preCheckPermissionedAs([flowEdit("summary: test\nhas_on_behalf_of: true\n")], userEmail, false, false, false) + ); + }); + + test("non-admin: edited flow with has_on_behalf_of: false is not flagged", async () => { + await preCheckPermissionedAs([flowEdit("summary: test\nhas_on_behalf_of: false\n")], userEmail, false, false, false); + }); + + test("non-admin: legacy script with on_behalf_of_email is still flagged", async () => { + await expectFlagged(() => + preCheckPermissionedAs([scriptEdit("summary: test\non_behalf_of_email: foo@bar.com\n")], userEmail, false, false, false) + ); + }); + + test("non-admin: script without obo fields is not flagged", async () => { + await preCheckPermissionedAs([scriptEdit("summary: test\n")], userEmail, false, false, false); + }); + + // --- Non-admin, added changes --- + + test("non-admin: added script with has_on_behalf_of: true is flagged", async () => { + await expectFlagged(() => + preCheckPermissionedAs([scriptAdded("summary: test\nhas_on_behalf_of: true\n")], userEmail, false, false, false) + ); + }); + + test("non-admin: added flow with has_on_behalf_of: true is flagged", async () => { + await expectFlagged(() => + preCheckPermissionedAs([flowAdded("summary: test\nhas_on_behalf_of: true\n")], userEmail, false, false, false) + ); + }); + + test("non-admin: added script with has_on_behalf_of: false is not flagged", async () => { + await preCheckPermissionedAs([scriptAdded("summary: test\nhas_on_behalf_of: false\n")], userEmail, false, false, false); + }); + + // --- Admin, edited changes (preserve handles these — not flagged) --- + + test("admin: edited script with has_on_behalf_of: true is not flagged (preserve handles)", async () => { + await preCheckPermissionedAs([scriptEdit("summary: test\nhas_on_behalf_of: true\n")], userEmail, true, false, false); + }); + + test("admin: edited flow with has_on_behalf_of: true is not flagged (preserve handles)", async () => { + await preCheckPermissionedAs([flowEdit("summary: test\nhas_on_behalf_of: true\n")], userEmail, true, false, false); + }); + + // --- Admin, added changes (no remote to preserve — rule check) --- + + test("admin: added script with has_on_behalf_of: true and no rule is flagged", async () => { + await expectFlagged(() => + preCheckPermissionedAs([scriptAdded("summary: test\nhas_on_behalf_of: true\n")], userEmail, true, false, false, []) + ); + }); + + test("admin: added flow with has_on_behalf_of: true and no rule is flagged", async () => { + await expectFlagged(() => + preCheckPermissionedAs([flowAdded("summary: test\nhas_on_behalf_of: true\n")], userEmail, true, false, false, []) + ); + }); + + test("admin: added script with has_on_behalf_of: true and matching rule is not flagged", async () => { + const rules = [{ email: "admin@co.com", path_pattern: "f/**" }]; + await preCheckPermissionedAs([scriptAdded("summary: test\nhas_on_behalf_of: true\n")], userEmail, true, false, false, rules); + }); + + test("admin: added flow with has_on_behalf_of: true and matching rule is not flagged", async () => { + const rules = [{ email: "admin@co.com", path_pattern: "f/**" }]; + await preCheckPermissionedAs([flowAdded("summary: test\nhas_on_behalf_of: true\n")], userEmail, true, false, false, rules); + }); + + test("admin: added script with has_on_behalf_of: false is not flagged (no obo)", async () => { + await preCheckPermissionedAs([scriptAdded("summary: test\nhas_on_behalf_of: false\n")], userEmail, true, false, false, []); + }); + + // --- acceptOverride flag --- + + test("flagged items with acceptOverride: true logs warning but does not exit", async () => { + // Should return normally (warning logged but no exit) + await preCheckPermissionedAs( + [scriptAdded("summary: test\nhas_on_behalf_of: true\n")], + userEmail, true, true, false, [] + ); + }); +});