fix: hasOnBehalfOf instead of no field on yamls

This commit is contained in:
wendrul
2026-03-31 18:16:44 +02:00
parent 07501aadaf
commit 84c2817520
5 changed files with 268 additions and 31 deletions
+6 -1
View File
@@ -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<OpenFlowWPath> = {};
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) {
+5 -2
View File
@@ -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))
) {
+5 -2
View File
@@ -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
);
}
+100 -26
View File
@@ -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<void> {
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}`));
+152
View File
@@ -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<void>) {
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, []
);
});
});