diff --git a/cli/TESTING.md b/cli/TESTING.md index b7a1e701b8..df0311677d 100644 --- a/cli/TESTING.md +++ b/cli/TESTING.md @@ -35,6 +35,28 @@ binary and starts a shared backend instance. Examples: `sync_pull_push`, `dev_server`, `standalone_commands` +## Module mocks + +`mock.module` replaces a module for the **whole process**, and it does reach modules that +were already imported — a stub one file installs lands on a consumer an earlier file +loaded. + +Handing the module back in `afterAll` is not a reliable undo. Files do run one at a time +(a root-level `afterAll` completes before the next file's body evaluates), so it looks +like it should be — but stubbing `bundle.ts` and restoring it that way still left +`raw_app_svelte_plugin_unit.test.ts` asserting against an empty bundle, green on Linux +and red on Windows, where the `readdir` file order differs. Treat a stub as permanent for +the run. + +So the rule is about what you stub, not how you clean up: **stub only a module no other +in-process suite imports.** Check with `grep -rl "" test/` before reaching +for one. A suite that drives the CLI through a spawned process is out of reach of a +module mock and doesn't count. + +`raw_app_push_policy_unit.test.ts` is the worked example: it stubs `gen/services.gen.ts`, +which passes the rule because nothing else in `test/` imports the three API functions it +replaces, and deliberately does not stub `bundle.ts`, which failed it. + ## AI Benchmark Caveats The repo-level benchmark CLI lives under `ai_evals/`, but it currently depends on diff --git a/cli/src/commands/app/app.ts b/cli/src/commands/app/app.ts index b35efc02a3..0d4ad634c8 100644 --- a/cli/src/commands/app/app.ts +++ b/cli/src/commands/app/app.ts @@ -119,9 +119,10 @@ export function isExecutionModeAnonymous(app: any) { export function isExecutionModeGuest(app: any) { return app?.["policy"]?.["execution_mode"] == "guest"; } -export type AppExecutionMode = "anonymous" | "guest" | "publisher"; +export type AppExecutionMode = "anonymous" | "guest" | "publisher" | "viewer"; /** The access mode is the one policy field a tracked app keeps, as `public` (anonymous) - * or `guests` (guest); the rest of the policy is regenerated on push. */ + * or `guests` (guest); the rest of the policy is preserved from the deployed app on + * push (see `generatingPolicy`). */ export function markAccessFromPolicy(app: any) { if (isExecutionModeAnonymous(app)) { app.public = true; @@ -129,14 +130,35 @@ export function markAccessFromPolicy(app: any) { app.guests = true; } } -export function executionModeFromAppFile(app: any): AppExecutionMode { +/** The mode the tracked file states, or `undefined` when it states none — the + * normal case, since a pull writes only the two open-access markers. `viewer` + * and `publisher` have no marker of their own, so a file can only name them + * through a policy block it was hand-written with. */ +function statedExecutionMode(app: any): AppExecutionMode | undefined { if (app?.["public"] ?? isExecutionModeAnonymous(app)) { return "anonymous"; } if (app?.["guests"] ?? isExecutionModeGuest(app)) { return "guest"; } - return "publisher"; + const mode = app?.["policy"]?.["execution_mode"]; + return mode === "viewer" || mode === "publisher" ? mode : undefined; +} + +/** The mode this push deploys under. A file that states one is authoritative, in + * both directions. Otherwise the two open-access markers are all it says, so + * their absence closes a deployed `anonymous`/`guest` app back down to + * `publisher` — while a deployed `viewer` is not a grant those markers revoke, + * so it carries over rather than widening to `publisher`. */ +export function executionModeForPush( + localApp: any, + deployedPolicy: Policy | undefined, +): AppExecutionMode { + const stated = statedExecutionMode(localApp); + if (stated) { + return stated; + } + return deployedPolicy?.execution_mode === "viewer" ? "viewer" : "publisher"; } export async function pushApp( workspace: string, @@ -161,12 +183,9 @@ export async function pushApp( //ignore } - let remoteOnBehalfOf: string | undefined; - let remoteOnBehalfOfEmail: string | undefined; - if (app?.policy) { - remoteOnBehalfOf = app.policy.on_behalf_of; - remoteOnBehalfOfEmail = app.policy.on_behalf_of_email; - } + // `app.policy` is cleared a few lines down, so capture it first: it is the + // base the regenerated policy is built on. + const deployedPolicy: Policy | undefined = app?.policy; markAccessFromPolicy(app); // console.log(app); @@ -181,20 +200,18 @@ export async function pushApp( const localApp = (await yamlParseFile(path)) as AppFile; replaceInlineScripts(localApp.value, localPath, true); - await generatingPolicy(localApp, remotePath, executionModeFromAppFile(localApp)); - - const preserveFields: { preserve_on_behalf_of?: boolean } = {}; - if (permissionedAsContext?.userIsAdminOrDeployer) { - if (app) { - if (localApp.policy && remoteOnBehalfOf) { - (localApp.policy as any).on_behalf_of = remoteOnBehalfOf; - (localApp.policy as any).on_behalf_of_email = remoteOnBehalfOfEmail; - preserveFields.preserve_on_behalf_of = true; - log.info(`Preserving ${remoteOnBehalfOfEmail ?? remoteOnBehalfOf} as permissioned_as for app ${remotePath}`); - } - } - // On create: backend applies folder defaults - } + // On create the backend applies folder defaults, so there is nothing to preserve. + const preserveFields = preserveOnBehalfOfFields( + remotePath, + deployedPolicy, + permissionedAsContext + ); + await generatingPolicy( + localApp, + remotePath, + executionModeForPush(localApp, deployedPolicy), + basePolicy(localApp, deployedPolicy, !!preserveFields.preserve_on_behalf_of) + ); // extra_perms goes through /acls/* — strip from the body so a perms-only // edit never bumps the app version (see applyExtraPermsDiff for details). @@ -251,18 +268,76 @@ export async function pushApp( export async function generatingPolicy( app: any, path: string, - executionMode: AppExecutionMode + executionMode: AppExecutionMode, + base: Policy | undefined ) { log.info(colors.gray(`Generating fresh policy for app ${path}...`)); try { - app.policy = await windmillUtils.updatePolicy(app.value, undefined); - app.policy.execution_mode = executionMode; + app.policy = await windmillUtils.updatePolicy(app.value, base); + finalizeDerivedPolicy(app.policy, executionMode); } catch (e) { log.error(colors.red(`Error generating policy for app ${path}: ${e}`)); throw e; } } +/** What the regenerated policy starts from: the deployed one, so a push keeps + * settings the tracked file doesn't record; on a first push, whatever the file + * states. The run identity rides along only when `claimsOnBehalfOf` — never + * from the file, never from a pusher who may not preserve one, since `wmill` + * is regularly pointed at servers older than the rewrite that would fix it. */ +export function basePolicy( + localApp: any, + deployedPolicy: Policy | undefined, + claimsOnBehalfOf: boolean +): Policy | undefined { + const stated = deployedPolicy ?? (localApp?.policy as Policy | undefined); + if (!stated || claimsOnBehalfOf) { + return stated; + } + const base: Policy = { ...stated }; + delete base.on_behalf_of; + delete base.on_behalf_of_email; + return base; +} + +/** Claim the run-as identity the regenerated policy carries over from the + * deployed app. Only a deployed identity may be claimed, never one the tracked + * file states — a repo doesn't get to pick who an app runs as. Without the flag + * the backend rewrites `on_behalf_of` to whoever is pushing, and it only honors + * the flag for an admin or a `wm_deployers` member, so a caller who is neither + * doesn't get to claim it here either. */ +export function preserveOnBehalfOfFields( + remotePath: string, + deployedPolicy: Policy | undefined, + permissionedAsContext: PermissionedAsContext | undefined +): { preserve_on_behalf_of?: boolean } { + const onBehalfOf = deployedPolicy?.on_behalf_of; + if (!permissionedAsContext?.userIsAdminOrDeployer || !onBehalfOf) { + return {}; + } + log.info( + `Preserving ${deployedPolicy?.on_behalf_of_email ?? onBehalfOf} as permissioned_as for app ${remotePath}` + ); + return { preserve_on_behalf_of: true }; +} + +/** The policy is written wholesale by the deploy, so the fields it does not + * derive from the tracked sources have to survive the trip. The policy builder + * has already recomputed what it can — the triggerables on both paths, plus the + * S3 rules on the low-code one, which `updateRawAppPolicy` has no equivalent of + * and so carries over. This sets the two left: the access mode, and the legacy + * `triggerables`, which still grant execution (the backend folds them into + * `triggerables_v2` at run time) and so are dropped rather than carried, or a + * deployed app would keep being able to run runnables this push removed. */ +export function finalizeDerivedPolicy( + policy: Policy, + executionMode: AppExecutionMode +) { + policy.triggerables = undefined; + policy.execution_mode = executionMode; +} + async function list(opts: GlobalOptions & { includeDraftOnly?: boolean; json?: boolean }) { const workspace = await resolveWorkspace(opts); await requireLogin(opts); @@ -425,14 +500,16 @@ async function push( if (isRawAppByName || hasRawAppYaml) { const { pushRawApp } = await import("./raw_apps.ts"); const merged = await mergeConfigWithConfigFile(opts); - // Raw-app ownership preservation is not implemented on either push - // path: sync push hands pushRawApp no context either. await pushRawApp( workspace.workspaceId, remotePath, absoluteFilePath, undefined, merged.defaultTs, + await buildPermissionedAsContext( + workspace.workspaceId, + await readEffectiveSyncBehavior(opts, workspace), + ), ); log.info(colors.bold.underline.green("Raw app pushed")); } else { diff --git a/cli/src/commands/app/raw_apps.ts b/cli/src/commands/app/raw_apps.ts index 901a47593f..8d82edb9df 100644 --- a/cli/src/commands/app/raw_apps.ts +++ b/cli/src/commands/app/raw_apps.ts @@ -1,6 +1,9 @@ import { requireLogin } from "../../core/auth.ts"; import { resolveWorkspace, validatePath } from "../../core/context.ts"; -import { mergeConfigWithConfigFile } from "../../core/conf.ts"; +import { + mergeConfigWithConfigFile, + readEffectiveSyncBehavior, +} from "../../core/conf.ts"; import { colors } from "@cliffy/ansi/colors"; import * as log from "../../core/log.ts"; import { sep as SEP } from "node:path"; @@ -17,13 +20,22 @@ import { deepEqual, readTextFile } from "../../utils/utils.ts"; import { type AppExecutionMode, - executionModeFromAppFile, + basePolicy, + executionModeForPush, + finalizeDerivedPolicy, markAccessFromPolicy, + preserveOnBehalfOfFields, replaceInlineScripts, repopulateFields, } from "./app.ts"; +import type { PermissionedAsContext } from "../../core/permissioned_as.ts"; +import { buildPermissionedAsContext } from "../../core/permissioned_as.ts"; import { createBundle, detectFrameworks } from "./bundle.ts"; import { APP_BACKEND_FOLDER, RECORDINGS_FOLDER } from "./app_metadata.ts"; +import { + NEVER_DEPLOYED_DIRS, + NEVER_DEPLOYED_FILES, +} from "../../utils/app_files.ts"; import { writeIfChanged } from "../../utils/utils.ts"; import { yamlOptions } from "../sync/sync.ts"; import { applyExtraPermsDiff } from "../../core/extra_perms.ts"; @@ -316,13 +328,11 @@ async function collectAppFiles( const relativePath = basePath + entry.name; if (entry.isDirectory()) { - // Skip the runnables, node_modules, and sql_to_apply subfolders + // The backend folder deploys as `value.runnables`, not as a bundled + // file; the rest reach the server through no channel at all. if ( entry.name === APP_BACKEND_FOLDER || - entry.name === "node_modules" || - entry.name === "dist" || - entry.name === ".claude" || - entry.name === "sql_to_apply" + NEVER_DEPLOYED_DIRS.has(entry.name) ) { continue; } @@ -334,13 +344,11 @@ async function collectAppFiles( } await readDirRecursive(fullPath + SEP, relativePath + "/"); } else if (entry.isFile()) { - // Skip generated/metadata files that shouldn't be part of the app + // `raw_app.yaml` deploys as the request's metadata rather than as a + // bundled file; the rest reach the server through no channel at all. if ( entry.name === "raw_app.yaml" || - entry.name === "package-lock.json" || - entry.name === "DATATABLES.md" || - entry.name === "AGENTS.md" || - entry.name === "wmill.d.ts" + NEVER_DEPLOYED_FILES.has(entry.name) ) { continue; } @@ -360,6 +368,7 @@ export async function pushRawApp( localPath: string, message?: string, defaultTs: "bun" | "deno" = "bun", + permissionedAsContext?: PermissionedAsContext, ): Promise { if (alreadySynced.includes(localPath)) { return; @@ -376,6 +385,10 @@ export async function pushRawApp( } catch { //ignore } + // `app.policy` is cleared a few lines down, so capture it first. `raw_app.yaml` + // records none of the policy, so anything the deploy drawer set is only here. + const deployedPolicy: Policy | undefined = app?.policy; + markAccessFromPolicy(app); // console.log(app); if (app) { @@ -424,10 +437,21 @@ export async function pushRawApp( // Create a temporary app object for policy generation const appForPolicy = { ...localApp, runnables }; + // On create the backend applies folder defaults, so there is nothing to preserve. + const preserveFields = preserveOnBehalfOfFields( + remotePath, + deployedPolicy, + permissionedAsContext, + ); await generatingPolicy( appForPolicy, remotePath, - executionModeFromAppFile(localApp), + executionModeForPush(localApp, deployedPolicy), + basePolicy( + localApp, + deployedPolicy, + !!preserveFields.preserve_on_behalf_of, + ), ); const files = await collectAppFiles(localPath); @@ -482,6 +506,7 @@ export async function pushRawApp( path: remotePath, summary: localApp.summary, policy: appForPolicy.policy, + ...preserveFields, deployment_message: message, // Preserve any user draft at this path (see backend skip_draft_deletion). skip_draft_deletion: true, @@ -532,14 +557,12 @@ export async function generatingPolicy( app: any, path: string, executionMode: AppExecutionMode, + base: Policy | undefined, ) { log.info(colors.gray(`Generating fresh policy for app ${path}...`)); try { - app.policy = await windmillUtils.updateRawAppPolicy( - app.runnables, - app.policy, - ); - app.policy.execution_mode = executionMode; + app.policy = await windmillUtils.updateRawAppPolicy(app.runnables, base); + finalizeDerivedPolicy(app.policy, executionMode); } catch (e) { log.error(colors.red(`Error generating policy for app ${path}: ${e}`)); throw e; @@ -564,6 +587,10 @@ async function pushRawAppCommand( filePath, undefined, merged.defaultTs, + await buildPermissionedAsContext( + workspace.workspaceId, + await readEffectiveSyncBehavior(opts, workspace), + ), ); log.info(colors.bold.underline.green("Raw app pushed")); } diff --git a/cli/src/commands/sync/sync.ts b/cli/src/commands/sync/sync.ts index 99a098f848..2159ea2539 100644 --- a/cli/src/commands/sync/sync.ts +++ b/cli/src/commands/sync/sync.ts @@ -152,6 +152,7 @@ import { generateAppLocksInternal, RECORDINGS_FOLDER, } from "../app/app_metadata.ts"; +import { deploysWithRawApp } from "../../utils/app_files.ts"; import { isFlowPath, isAppPath, @@ -2018,20 +2019,18 @@ export async function elementsToMap( } if (isRawAppFile(path)) { - // FSFSElement builds paths with the platform separator, while the checks - // below are written with "/": without normalizing, none of them match on - // Windows and the push collector's own exclusions become perpetual diffs. + // FSFSElement builds paths with the platform separator, while + // `deploysWithRawApp` is written with "/": without normalizing it matches + // nothing on Windows and the push collector's own exclusions become + // perpetual diffs. const suffix = path .split(getFolderSuffix("raw_app") + SEP) .pop() ?.replaceAll(SEP, "/"); - if ( - suffix?.startsWith("dist/") || - suffix?.startsWith(RECORDINGS_FOLDER + "/") || - suffix == "wmill.d.ts" || - suffix == "package-lock.json" || - suffix == "DATATABLES.md" - ) { + // A file no push sends is not a change to track. Listing it leaves it + // pending forever — nothing ever uploads it — and pushing it redeploys + // the whole app, reassigning its run-as user, to ship nothing. + if (suffix && !deploysWithRawApp(suffix)) { continue; } } @@ -6142,7 +6141,7 @@ export async function push( undefined, opts.plainSecrets ?? false, alreadySynced, - { message: opts.message }, + { message: opts.message, permissionedAsContext }, ); } else { // Flow folder doesn't exist locally — delete on server @@ -6187,7 +6186,7 @@ export async function push( undefined, opts.plainSecrets ?? false, alreadySynced, - { message: opts.message }, + { message: opts.message, permissionedAsContext }, ); } else { // App folder doesn't exist locally — delete on server @@ -6233,7 +6232,11 @@ export async function push( undefined, opts.plainSecrets ?? false, alreadySynced, - { message: opts.message, defaultTs: opts.defaultTs }, + { + message: opts.message, + defaultTs: opts.defaultTs, + permissionedAsContext, + }, ); } else { // The entire raw app folder was deleted locally, diff --git a/cli/src/core/permissioned_as.ts b/cli/src/core/permissioned_as.ts index 57570b3549..d0301373da 100644 --- a/cli/src/core/permissioned_as.ts +++ b/cli/src/core/permissioned_as.ts @@ -3,6 +3,12 @@ import * as log from "./log.ts"; import { colors } from "@cliffy/ansi/colors"; import { Confirm } from "@cliffy/prompt/confirm"; import { getTypeStrFromPath } from "../types.ts"; +import { + extractFolderPath, + isAppFolderMetadataFile, + isRawAppFolderMetadataFile, +} from "../utils/resource_folders.ts"; +import { deploysWithRawApp } from "../utils/app_files.ts"; import { parseSyncBehavior } from "./conf.ts"; export interface PermissionedAsContext { @@ -89,6 +95,45 @@ function contentHasOnBehalfOf(content: string, typeStr: string): boolean { return false; } +type AppTypeStr = "app" | "raw_app"; + +function isAppTypeStr(typeStr: string): typeStr is AppTypeStr { + return typeStr === "app" || typeStr === "raw_app"; +} + +/** The app folder a file belongs to. `isAppFolderMetadataFile` and its raw twin + * match a literal `/`, unlike `extractFolderPath` — so normalize before either, + * or a Windows path takes a different branch from the same file on Linux. */ +function appFolderOf(path: string, typeStr: AppTypeStr): string { + return extractFolderPath(path, typeStr) ?? path; +} + +function toPosix(path: string): string { + return path.replaceAll("\\", "/"); +} + +/** App folders whose own metadata file is being added or deleted, which is how a + * whole app arrives or goes rather than being redeployed. Neither takes an owner + * over: a create has none yet, and a delete leaves none behind. */ +function appsArrivingOrLeaving(changes: Change[]): Set { + const folders = new Set(); + for (const change of changes) { + if (change.name === "edited") continue; + const path = toPosix(change.path); + if (!isAppFolderMetadataFile(path) && !isRawAppFolderMetadataFile(path)) { + continue; + } + let typeStr: string; + try { + typeStr = getTypeStrFromPath(path); + } catch { + continue; + } + if (isAppTypeStr(typeStr)) folders.add(appFolderOf(path, typeStr)); + } + return folders; +} + export async function preCheckPermissionedAs( changes: Change[], userEmail: string, @@ -101,6 +146,12 @@ export async function preCheckPermissionedAs( if (userIsAdminOrDeployer) return; const wouldChangeItems: { path: string; currentOwner: string }[] = []; + const addItem = (item: { path: string; currentOwner: string }) => { + if (!wouldChangeItems.some((i) => i.path === item.path)) { + wouldChangeItems.push(item); + } + }; + const arrivingOrLeaving = appsArrivingOrLeaving(changes); for (const change of changes) { let typeStr: string; @@ -110,6 +161,22 @@ export async function preCheckPermissionedAs( continue; } + // An app is redeployed whole by any change to any of the files it actually + // sends — added, edited or deleted alike — so its policy is rewritten + // regardless of what the file holds. Settled here, before the content the + // other kinds parse to find their owner, which an app has none of to parse. + if (isAppTypeStr(typeStr)) { + const path = toPosix(change.path); + const folder = appFolderOf(path, typeStr); + if ( + !arrivingOrLeaving.has(folder) && + (typeStr === "app" || deploysWithRawApp(path.slice(folder.length))) + ) { + addItem({ path: folder, currentOwner: "(app policy owner)" }); + } + continue; + } + if (change.name === "added") { const content = change.content; if (!content) continue; @@ -130,11 +197,6 @@ export async function preCheckPermissionedAs( const label = typeStr === "script" ? "(script owner)" : "(flow owner)"; wouldChangeItems.push({ path: change.path, currentOwner: label }); - } else if (typeStr === "app") { - wouldChangeItems.push({ - path: change.path, - currentOwner: "(app policy owner)", - }); } continue; } @@ -177,12 +239,6 @@ export async function preCheckPermissionedAs( } } continue; - } else if (typeStr === "app") { - wouldChangeItems.push({ - path: change.path, - currentOwner: "(app policy owner)", - }); - continue; } else if (typeStr === "schedule") { const match = beforeContent.match( /email:\s*["']?([^\s"']+)["']?/ diff --git a/cli/src/types.ts b/cli/src/types.ts index 9c517ff3c7..5713d06390 100644 --- a/cli/src/types.ts +++ b/cli/src/types.ts @@ -234,7 +234,7 @@ export async function pushObj( if (!rawAppName) { throw new Error(`Could not extract raw app name from path: ${p}`); } - await pushRawApp(workspace, rawAppName, buildFolderPath(rawAppName, "raw_app"), message, defaultTs); + await pushRawApp(workspace, rawAppName, buildFolderPath(rawAppName, "raw_app"), message, defaultTs, permissionedAsContext); } else if (typeEnding === "folder") { await pushFolder(workspace, p, befObj, newObj); } else if (typeEnding === "variable") { diff --git a/cli/src/utils/app_files.ts b/cli/src/utils/app_files.ts new file mode 100644 index 0000000000..ca1be1de0e --- /dev/null +++ b/cli/src/utils/app_files.ts @@ -0,0 +1,48 @@ +import { + APP_BACKEND_FOLDER, + RECORDINGS_FOLDER, +} from "../commands/app/app_metadata.ts"; + +/** Directories under a raw app that no push sends. */ +const NEVER_DEPLOYED_DIRS = new Set([ + "node_modules", + "dist", + ".claude", + "sql_to_apply", +]); + +/** Files under a raw app that no push sends. */ +const NEVER_DEPLOYED_FILES = new Set([ + "package-lock.json", + "DATATABLES.md", + "AGENTS.md", + "wmill.d.ts", +]); + +/** + * Whether an app-root-relative path (`/` separators, leading slash optional) + * reaches the server through any of a push's three channels: `raw_app.yaml` as + * metadata, the backend folder as runnables, the rest bundled by + * `collectAppFiles`. A path this rejects deploys nothing, so changing it is not + * a change to the app however much the sync diff lists it. `collectAppFiles` + * must not drift from this — it reads the same two sets. + */ +export function deploysWithRawApp(relativePath: string): boolean { + const segments = relativePath.split("/").filter(Boolean); + if (segments.length === 0) return false; + const name = segments[segments.length - 1]; + const dirs = segments.slice(0, -1); + // The sets below describe the bundle, which never walks into the backend + // folder — applying them there would strip a runnable whose file shares a + // name (`backend/wmill.d.ts` is the runnable `wmill.d`). Depth 1 because + // `loadRunnablesFromBackend` reads that folder's top level only. + if (dirs[0] === APP_BACKEND_FOLDER) return dirs.length === 1; + if (NEVER_DEPLOYED_FILES.has(name)) return false; + if (dirs.some((d) => NEVER_DEPLOYED_DIRS.has(d))) return false; + // Session recordings are written at the app root only, so an app with a + // `recordings/` component folder of its own still ships it. + if (dirs[0] === RECORDINGS_FOLDER) return false; + return true; +} + +export { NEVER_DEPLOYED_DIRS, NEVER_DEPLOYED_FILES }; diff --git a/cli/test/app_access_mode_unit.test.ts b/cli/test/app_access_mode_unit.test.ts index 12185d6d99..42a3fd082a 100644 --- a/cli/test/app_access_mode_unit.test.ts +++ b/cli/test/app_access_mode_unit.test.ts @@ -1,6 +1,6 @@ import { expect, test } from "bun:test"; import { - executionModeFromAppFile, + executionModeForPush, generatingPolicy, markAccessFromPolicy, } from "../src/commands/app/app.ts"; @@ -13,16 +13,39 @@ test("the access mode survives the app.yaml round trip", async () => { guest.policy = undefined; expect(guest.guests).toBe(true); expect(guest.public).toBeUndefined(); - expect(executionModeFromAppFile(guest)).toBe("guest"); - await generatingPolicy(guest, "u/test/app", executionModeFromAppFile(guest)); + expect(executionModeForPush(guest, undefined)).toBe("guest"); + await generatingPolicy( + guest, + "u/test/app", + executionModeForPush(guest, undefined), + undefined, + ); expect(guest.policy.execution_mode).toBe("guest"); const anonymous: any = { policy: { execution_mode: "anonymous" }, value: {} }; markAccessFromPolicy(anonymous); anonymous.policy = undefined; expect(anonymous.public).toBe(true); - expect(executionModeFromAppFile(anonymous)).toBe("anonymous"); + expect(executionModeForPush(anonymous, undefined)).toBe("anonymous"); - expect(executionModeFromAppFile({ policy: { execution_mode: "publisher" } })).toBe("publisher"); - expect(executionModeFromAppFile({})).toBe("publisher"); + expect(executionModeForPush({ policy: { execution_mode: "publisher" } }, undefined)).toBe("publisher"); + expect(executionModeForPush({}, undefined)).toBe("publisher"); +}); + +// `viewer` is the narrowest mode — each runnable runs as the viewer, not as the +// app's identity — and the only one with no marker in the file, so both ways it +// can reach a push must survive rather than widen to `publisher`. +test("viewer is never widened to publisher by a push", () => { + // Carried over from the deployed app: a pull writes no marker for it. + expect(executionModeForPush({}, { execution_mode: "viewer" })).toBe("viewer"); + // Stated by the file, which is all a first push has to go on. + expect(executionModeForPush({ policy: { execution_mode: "viewer" } }, undefined)).toBe("viewer"); + // The open-access markers still win, in either direction. + expect(executionModeForPush({ public: true }, { execution_mode: "viewer" })).toBe("anonymous"); + expect(executionModeForPush({}, { execution_mode: "anonymous" })).toBe("publisher"); + // A stated mode is authoritative both ways: the carry-over is for a file that + // says nothing, so it must not pin a deployed app to `viewer` forever. + expect( + executionModeForPush({ policy: { execution_mode: "publisher" } }, { execution_mode: "viewer" }) + ).toBe("publisher"); }); diff --git a/cli/test/precheck_permissioned_as_apps_unit.test.ts b/cli/test/precheck_permissioned_as_apps_unit.test.ts new file mode 100644 index 0000000000..4a224a43c0 --- /dev/null +++ b/cli/test/precheck_permissioned_as_apps_unit.test.ts @@ -0,0 +1,147 @@ +/** + * The pre-check is what stops a push from silently reassigning an item's run-as + * user. Raw apps were missing from it, so the one kind whose file records no + * policy at all was also the one that changed owner without a word. + */ + +import { expect, test } from "bun:test"; +import { preCheckPermissionedAs } from "../src/core/permissioned_as.ts"; + +/** Non-interactive and without the override flag, the pre-check exits rather + * than reassigning silently — so a thrown exit is the signal it fired. */ +type Shape = "edited" | "added" | "deleted"; + +function change(path: string, name: Shape = "edited") { + return { name, path, before: "summary: x\n", content: "summary: x\n" }; +} + +async function precheck( + changes: ReturnType[], +): Promise { + const exit = process.exit; + let code: number | undefined; + (process as any).exit = (c?: number) => { + code = c; + throw new Error(`exit:${c}`); + }; + const logged: string[] = []; + const err = console.error; + console.error = (...a: unknown[]) => void logged.push(a.join(" ")); + try { + await preCheckPermissionedAs(changes, "pusher@corp", false, false, false); + } catch (e) { + if (!String(e).startsWith("Error: exit:")) throw e; + } finally { + (process as any).exit = exit; + console.error = err; + } + return code === undefined ? undefined : logged.join("\n"); +} + +test("a raw-app push warns the non-deployer it will take over the run-as user", async () => { + const message = await precheck([change("f/test/myapp.raw_app/index.tsx")]); + + expect(message).toBeDefined(); + expect(message).toContain("f/test/myapp.raw_app"); + expect(message).toContain("pusher@corp"); +}); + +// Deleting one file re-pushes the whole app rather than deleting it, so the +// takeover happens there too. +test("deleting one of an app's files warns like editing one", async () => { + const message = await precheck([ + change("f/test/myapp.raw_app/gone.tsx", "deleted"), + ]); + + expect(message).toContain("f/test/myapp.raw_app"); +}); + +// The metadata file going with it means the app itself is created or removed — +// neither takes an owner over. +test("an app arriving or leaving whole is not a takeover", async () => { + const created = await precheck([ + change("f/test/new.raw_app/raw_app.yaml", "added"), + change("f/test/new.raw_app/index.tsx", "added"), + ]); + const removed = await precheck([ + change("f/test/old.raw_app/raw_app.yaml", "deleted"), + change("f/test/old.raw_app/index.tsx", "deleted"), + ]); + + expect(created).toBeUndefined(); + expect(removed).toBeUndefined(); +}); + +// An app carries no owner in its files, so nothing about it depends on their +// content — an empty one redeploys it exactly like any other. +test("an empty file still counts as a change to the app", async () => { + const added = await precheck([ + { name: "added", path: "f/test/myapp.raw_app/blank.ts", content: "" }, + ]); + const edited = await precheck([ + { name: "edited", path: "f/test/myapp.raw_app/blank.ts", before: "" }, + ]); + + expect(added).toContain("f/test/myapp.raw_app"); + expect(edited).toContain("f/test/myapp.raw_app"); +}); + +// `extractFolderPath` normalizes separators but the metadata predicates match a +// literal `/`, so a Windows path must not take a different branch. +test("a Windows path classifies the same as its posix twin", async () => { + const created = await precheck([ + change("f\\test\\new.raw_app\\raw_app.yaml", "added"), + change("f\\test\\new.raw_app\\index.tsx", "added"), + ]); + const edited = await precheck([ + change("f\\test\\myapp.raw_app\\index.tsx"), + ]); + + expect(created).toBeUndefined(); + expect(edited).toContain("f/test/myapp.raw_app"); +}); + +// `collectAppFiles` never sends these, and the sync diff never stops listing +// them (nothing uploads them, so they stay "added" forever) — so warning on one +// would gate every push of a scaffolded app on the override flag. +test("a file the push never sends is not a change to the app", async () => { + const artifacts = await precheck([ + change("f/test/myapp.raw_app/AGENTS.md", "added"), + change("f/test/myapp.raw_app/sql_to_apply/a.sql", "added"), + change("f/test/myapp.raw_app/node_modules/dep/index.js", "added"), + change("f/test/myapp.raw_app/recordings/r.json", "added"), + change("f/test/myapp.raw_app/package-lock.json"), + change("f/test/myapp.raw_app/wmill.d.ts"), + // Only the backend folder's *top level* is a runnable; nothing reads deeper, + // so the depth limit is what keeps a `backend/node_modules/` from becoming + // the perpetual diff this predicate exists to remove. + change("f/test/myapp.raw_app/backend/node_modules/dep/index.js", "added"), + ]); + // The three channels a push does send through: bundled file, metadata, runnable. + const sent = await precheck([change("f/test/myapp.raw_app/index.tsx")]); + const meta = await precheck([change("f/test/myapp.raw_app/raw_app.yaml")]); + const runnable = await precheck([change("f/test/myapp.raw_app/backend/a.ts")]); + // The runnable channel is not the bundle: the bundle's name exclusions don't + // reach into it, so a runnable file sharing one of those names still deploys. + const namesake = await precheck([ + change("f/test/myapp.raw_app/backend/wmill.d.ts"), + ]); + + expect(artifacts).toBeUndefined(); + expect(sent).toContain("f/test/myapp.raw_app"); + expect(meta).toContain("f/test/myapp.raw_app"); + expect(runnable).toContain("f/test/myapp.raw_app"); + expect(namesake).toContain("f/test/myapp.raw_app"); +}); + +test("an app is listed once however many of its files changed", async () => { + const message = await precheck([ + change("f/test/myapp.raw_app/index.tsx"), + change("f/test/myapp.raw_app/raw_app.yaml"), + change("f/test/myapp.raw_app/backend/a.ts"), + change("f/test/low.app/app.yaml"), + change("f/test/low.app/inline.ts"), + ]); + + expect(message).toContain("2 item(s)"); +}); diff --git a/cli/test/raw_app_push_policy_unit.test.ts b/cli/test/raw_app_push_policy_unit.test.ts new file mode 100644 index 0000000000..fbbc7b55c0 --- /dev/null +++ b/cli/test/raw_app_push_policy_unit.test.ts @@ -0,0 +1,142 @@ +/** + * `raw_app.yaml` records none of the policy but the access-mode markers, so a + * push that regenerated the whole policy reset the deploy drawer's settings — + * run-as identity, sandbox isolation — to the pushing user's. Pin that the + * deployed policy is carried over, that a first push still starts from what the + * file states, and that the markers still close a deployed open app back down. + */ + +import { afterAll, beforeEach, expect, mock, test } from "bun:test"; +import { mkdtemp, symlink, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +let calls: any[] = []; +let deployedPolicy: any; +/** No app deployed at the path: `getAppByPath` 404s and the push creates one. */ +let deployed = true; + +// Stub only what no other in-process suite imports, and treat a stub as +// permanent for the run (see "Module mocks" in cli/TESTING.md). These three API +// functions qualify — nothing else in `test/` imports them. `bundle.ts` did not: +// stubbing it left `raw_app_svelte_plugin_unit.test.ts` asserting against an +// empty bundle, which an `afterAll` hand-back did not prevent. So the real +// bundler runs instead, on the app each push writes below. +const realServices = await import("../gen/services.gen.ts"); + +mock.module("../gen/services.gen.ts", () => ({ + ...realServices, + getAppByPath: async () => { + if (!deployed) throw new Error("not found"); + return { + path: "f/test/raw", + summary: "raw", + value: { files: {}, runnables: {} }, + policy: deployedPolicy, + }; + }, + updateAppRaw: async (a: unknown) => { + calls.push(a); + }, + createAppRaw: async (a: unknown) => { + calls.push(a); + }, +})); + +// Belt and braces: nothing else in-process calls these, and a hand-back is not +// what makes that safe. +afterAll(() => { + mock.module("../gen/services.gen.ts", () => realServices); +}); + +const { pushRawApp } = await import("../src/commands/app/raw_apps.ts"); + +const ADMIN = { + userCache: new Map(), + userIsAdminOrDeployer: true, + userEmail: "deployer@windmill.dev", +}; + +async function push(yamlTail: string, admin = true): Promise { + calls = []; + const dir = await mkdtemp(join(tmpdir(), "windmill_raw_push_")); + await writeFile( + join(dir, "raw_app.yaml"), + `summary: raw\nrunnables: {}\n${yamlTail}`, + "utf-8", + ); + // Any file the remote doesn't have, so the push isn't short-circuited as + // up to date. It is also the bundler's entry point, so it has to compile. + await writeFile(join(dir, "index.tsx"), "export default 1\n", "utf-8"); + await writeFile( + join(dir, "package.json"), + JSON.stringify({ name: "app", private: true }), + "utf-8", + ); + // `ensureNodeModules` only checks the directory is there; borrowing the CLI's + // own skips an npm install per push. + await symlink(join(process.cwd(), "node_modules"), join(dir, "node_modules")); + await pushRawApp("w", "f/test/raw", dir, undefined, "bun", admin ? ADMIN : undefined); + expect(calls).toHaveLength(1); + return calls[0].formData.app; +} + +beforeEach(() => { + deployed = true; + deployedPolicy = { + on_behalf_of: "u/svc", + on_behalf_of_email: "svc@corp", + sandbox: true, + frontend_sdk_scopes: ["jobs:run"], + execution_mode: "anonymous", + // Legacy v1 grants: the backend folds them into v2 at run time, so keeping + // them would keep granting runnables a push has removed. + triggerables: { "script/f/test/gone": {} }, + triggerables_v2: { "a:script/f/test/gone": {} }, + }; +}); + +test("a raw-app push keeps the deployed run-as and sandbox settings", async () => { + const body = await push("public: true\n"); + + expect(body.policy.on_behalf_of).toBe("u/svc"); + expect(body.policy.on_behalf_of_email).toBe("svc@corp"); + expect(body.preserve_on_behalf_of).toBe(true); + expect(body.policy.sandbox).toBe(true); + expect(body.policy.frontend_sdk_scopes).toEqual(["jobs:run"]); + expect(body.policy.execution_mode).toBe("anonymous"); + expect(body.policy.triggerables).toBeUndefined(); + expect(body.policy.triggerables_v2).toEqual({}); +}); + +test("a raw-app push without the marker closes an anonymous app back down", async () => { + const body = await push(""); + + expect(body.policy.execution_mode).toBe("publisher"); +}); + +test("a push that may not claim the deployed identity doesn't send it", async () => { + const body = await push("", false); + + expect(body.preserve_on_behalf_of).toBeUndefined(); + // Not just the flag: the identity itself stays off the wire, so no server can + // deploy this push under it. + expect(body.policy.on_behalf_of).toBeUndefined(); + expect(body.policy.on_behalf_of_email).toBeUndefined(); + // Everything the pusher is entitled to carry over still comes along. + expect(body.policy.sandbox).toBe(true); +}); + +test("a first raw-app push deploys the policy its file states", async () => { + deployed = false; + const body = await push( + "policy:\n sandbox: true\n on_behalf_of: u/impostor\n on_behalf_of_email: impostor@corp\n", + ); + + expect(body.policy.sandbox).toBe(true); + // A repo doesn't get to pick who an app runs as: the identity never reaches + // the wire, so no server can be talked into deploying under it. + expect(body.policy.on_behalf_of).toBeUndefined(); + expect(body.policy.on_behalf_of_email).toBeUndefined(); + expect(body.preserve_on_behalf_of).toBeUndefined(); +});