diff --git a/src/routes/v2/pages/Editor/components/ArgumentRow/ArgumentRow.tsx b/src/routes/v2/pages/Editor/components/ArgumentRow/ArgumentRow.tsx index 46d9788a4c..57151d59f9 100644 --- a/src/routes/v2/pages/Editor/components/ArgumentRow/ArgumentRow.tsx +++ b/src/routes/v2/pages/Editor/components/ArgumentRow/ArgumentRow.tsx @@ -3,6 +3,7 @@ import { observer } from "mobx-react-lite"; import { useEffect, useRef, useState } from "react"; import { getDynamicDataDisplayInfo } from "@/components/shared/ReactFlow/FlowCanvas/TaskNode/ArgumentsEditor/dynamicDataUtils"; +import { FieldMessage } from "@/components/ui/field-message"; import { InlineStack } from "@/components/ui/layout"; import { Text } from "@/components/ui/typography"; import type { @@ -13,8 +14,12 @@ import type { } from "@/models/componentSpec"; import { useAnalytics } from "@/providers/AnalyticsProvider"; import { useIOActions } from "@/routes/v2/pages/Editor/store/actions/useIOActions"; +import { useEditorSession } from "@/routes/v2/pages/Editor/store/EditorSessionContext"; import { useSharedStores } from "@/routes/v2/shared/store/SharedStoreContext"; -import type { DynamicDataArgument } from "@/utils/componentSpec"; +import { + type DynamicDataArgument, + isSecretArgument, +} from "@/utils/componentSpec"; import { canResetArgument, @@ -63,6 +68,8 @@ export const ArgumentRow = observer(function ArgumentRow({ }: ArgumentRowProps) { const { track } = useAnalytics(); const { editor } = useSharedStores(); + const { pipelineFile } = useEditorSession(); + const secretsOwner = pipelineFile.activePipelineFile?.secretsOwner; const { setArgument, removeArgument, @@ -79,6 +86,9 @@ export const ArgumentRow = observer(function ArgumentRow({ const isBound = binding !== undefined; const isDynamic = isDynamicDataValue(currentValue); + const secretsLocked = + !!secretsOwner && + task.arguments.some((argument) => isSecretArgument(argument.value)); const dynamicDisplayInfo = isDynamic ? getDynamicDataDisplayInfo(currentValue.dynamicData) : null; @@ -98,7 +108,7 @@ export const ArgumentRow = observer(function ArgumentRow({ }, [isFocused, externalEditor]); const handleClick = () => { - if (isDynamic) return; + if (isDynamic || secretsLocked) return; track("v2.pipeline_editor.task_arguments.argument_row.edit_started"); onSelectionChanged?.(inputSpec.name); if (externalEditor) { @@ -198,6 +208,8 @@ export const ArgumentRow = observer(function ArgumentRow({ inputType={inputSpec.type} canReset={canReset} canUnset={canUnset} + disabled={secretsLocked} + secretsOwner={secretsOwner} excludeEntityIds={[task.$id]} taskAnnotations={taskAnnotations} onResetToDefault={handleResetToDefault} @@ -219,9 +231,15 @@ export const ArgumentRow = observer(function ArgumentRow({ task={task} inputSpec={inputSpec} currentValue={currentValue} + readOnly={secretsLocked} onChangeComplete={handleChangeComplete} onBlur={handleBlur} /> + {secretsLocked && isSecretArgument(currentValue) && ( + + Only {secretsOwner} can change tasks with secrets. + + )} ); }); diff --git a/src/routes/v2/pages/Editor/components/ArgumentRow/components/ArgumentValueDisplay.tsx b/src/routes/v2/pages/Editor/components/ArgumentRow/components/ArgumentValueDisplay.tsx index 678e0cf6b2..61f43d7365 100644 --- a/src/routes/v2/pages/Editor/components/ArgumentRow/components/ArgumentValueDisplay.tsx +++ b/src/routes/v2/pages/Editor/components/ArgumentRow/components/ArgumentValueDisplay.tsx @@ -21,6 +21,7 @@ interface ArgumentValueDisplayProps { task: Task; inputSpec: InputSpec; currentValue: unknown; + readOnly?: boolean; onChangeComplete: (value: string) => void; onBlur: () => void; } @@ -36,6 +37,7 @@ export function ArgumentValueDisplay({ task, inputSpec, currentValue, + readOnly, onChangeComplete, onBlur, }: ArgumentValueDisplayProps) { @@ -70,8 +72,9 @@ export function ArgumentValueDisplay({ key={`${task.$id}-${inputSpec.name}-${String(currentValue ?? "")}`} defaultValue={typeof currentValue === "string" ? currentValue : ""} expandDialogTitle={`Value for ${inputSpec.name}`} - onChangeComplete={onChangeComplete} + onChangeComplete={readOnly ? undefined : onChangeComplete} onBlur={onBlur} + readOnly={readOnly} highlightSyntax placeholder={isBound ? bindingLabel || "" : placeholder} className={cn( diff --git a/src/routes/v2/pages/Editor/components/ArgumentRow/components/ThunderMenu/ThunderMenu.tsx b/src/routes/v2/pages/Editor/components/ArgumentRow/components/ThunderMenu/ThunderMenu.tsx index e2a16db966..bdfc84a8b3 100644 --- a/src/routes/v2/pages/Editor/components/ArgumentRow/components/ThunderMenu/ThunderMenu.tsx +++ b/src/routes/v2/pages/Editor/components/ArgumentRow/components/ThunderMenu/ThunderMenu.tsx @@ -34,6 +34,7 @@ interface ThunderMenuProps { canReset: boolean; canUnset: boolean; disabled?: boolean; + secretsOwner?: string; excludeEntityIds: string[]; taskAnnotations?: TaskAnnotations; onResetToDefault: () => void; @@ -48,6 +49,7 @@ export const ThunderMenu = observer(function ThunderMenu({ canReset, canUnset, disabled = false, + secretsOwner, excludeEntityIds, taskAnnotations, onResetToDefault, @@ -144,6 +146,7 @@ export const ThunderMenu = observer(function ThunderMenu({ diff --git a/src/routes/v2/pages/Editor/components/ArgumentRow/components/ThunderMenu/components/DynamicDataSubmenu.tsx b/src/routes/v2/pages/Editor/components/ArgumentRow/components/ThunderMenu/components/DynamicDataSubmenu.tsx index 3b3ca6103d..bb1a612752 100644 --- a/src/routes/v2/pages/Editor/components/ArgumentRow/components/ThunderMenu/components/DynamicDataSubmenu.tsx +++ b/src/routes/v2/pages/Editor/components/ArgumentRow/components/ThunderMenu/components/DynamicDataSubmenu.tsx @@ -9,19 +9,22 @@ import { DropdownMenuSubTrigger, } from "@/components/ui/dropdown-menu"; import { Icon } from "@/components/ui/icon"; -import { InlineStack } from "@/components/ui/layout"; +import { BlockStack, InlineStack } from "@/components/ui/layout"; +import { Text } from "@/components/ui/typography"; import { tracking } from "@/utils/tracking"; type DynamicDataGroup = ReturnType[number]; interface DynamicDataSubmenuProps { groups: DynamicDataGroup[]; + secretsOwner?: string; onOpenSecretDialog: () => void; onSelectSystemData: (key: string) => void; } export function DynamicDataSubmenu({ groups, + secretsOwner, onOpenSecretDialog, onSelectSystemData, }: DynamicDataSubmenuProps) { @@ -44,7 +47,19 @@ export function DynamicDataSubmenu({ {group.title} - {group.requiresDialog ? ( + {group.requiresDialog && secretsOwner ? ( + + + + + Select Secret... + + + + Ask {secretsOwner} to add secrets. + + + ) : group.requiresDialog ? ( { expect(body).not.toHaveProperty("versioning_mode"); }); - it("rejects updates to another owner's pipeline before issuing a write", async () => { - fetchMock.mockResolvedValueOnce(accountResponse()); + it("saves another owner's pipeline by id so the backend can check edit access", async () => { + const shared = pipeline({ user_id: "another-owner@example.com" }); + saveResponses(shared); - await expect( - writeCloudPipeline( - { - filePath: FILE_PATH, - componentSpec: localSpec, - existingPipeline: pipeline({ user_id: "another-owner@example.com" }), - }, - connection, - ), - ).rejects.toThrow("Only the owner"); - expect(fetchMock).toHaveBeenCalledTimes(1); + await writeCloudPipeline( + { + filePath: FILE_PATH, + componentSpec: localSpec, + existingPipeline: shared, + }, + connection, + ); + + const request = requestAt(1); + expect(request.method).toBe("PUT"); + expect(new URL(request.url).pathname).toBe(`/api/pipelines/${shared.id}`); }); it("creates a copy under the viewer's account when no existing pipeline is supplied", async () => { diff --git a/src/services/cloudPipelineService.ts b/src/services/cloudPipelineService.ts index 7a827c4200..ec6820d9c1 100644 --- a/src/services/cloudPipelineService.ts +++ b/src/services/cloudPipelineService.ts @@ -14,6 +14,8 @@ import { isRecord } from "@/utils/typeGuards"; const USER_PIPELINES_PATH = "/api/users/me/pipelines"; const PIPELINE_BY_ID_PATH = "/api/pipelines/{pipeline_id}"; +const PIPELINE_SCOPES_PATH = + "/api/access_control/pipeline/{pipeline_id}/my_scopes"; const accountSchema = z.object({ id: z.string().min(1), @@ -175,6 +177,22 @@ export async function getCloudPipeline( return pipeline; } +export async function getCloudPipelineScopes( + pipelineId: string, + connection: CloudConnection, +): Promise { + const result = await client.get({ + ...requestOptions(connection), + url: PIPELINE_SCOPES_PATH, + path: { pipeline_id: pipelineId }, + }); + if (!result.response.ok) return []; + const scopes = z + .object({ my_scopes: z.array(z.string()) }) + .safeParse(result.data); + return scopes.success ? scopes.data.my_scopes : []; +} + export async function writeCloudPipeline( { filePath, @@ -186,26 +204,36 @@ export async function writeCloudPipeline( ): Promise { const account = connection.account ?? (await getCloudPipelineAccount(connection)); - requireWritePermission(account, existingPipeline); + const owner = existingPipeline?.user_id ?? account.id; + requireWritePermission(account); if (existingPipeline && existingPipeline.file_path !== filePath) { throw new Error("A remote pipeline's storage path cannot change."); } const template = existingPipeline ?? sourcePipeline; const task = componentSpecToCloudTask(componentSpec, template); - const result = await client.put({ - ...requestOptions(connection), - url: USER_PIPELINES_PATH, - query: { file_path: filePath }, - body: { - root_pipeline_task: task, - pipeline_run_annotations: template?.pipeline_run_annotations ?? {}, - }, - }); + const body = { + root_pipeline_task: task, + pipeline_run_annotations: template?.pipeline_run_annotations ?? {}, + }; + const result = + existingPipeline && owner !== account.id + ? await client.put({ + ...requestOptions(connection), + url: PIPELINE_BY_ID_PATH, + path: { pipeline_id: existingPipeline.id }, + body, + }) + : await client.put({ + ...requestOptions(connection), + url: USER_PIPELINES_PATH, + query: { file_path: filePath }, + body, + }); requireSuccessfulResponse(result); const savedPipeline = pipelineSchema.parse(result.data); if ( - savedPipeline.user_id !== account.id || + savedPipeline.user_id !== owner || savedPipeline.file_path !== filePath || (existingPipeline && savedPipeline.id !== existingPipeline.id) ) { diff --git a/src/services/pipelineStorage/PipelineFile.ts b/src/services/pipelineStorage/PipelineFile.ts index e7c417ec4f..1c6c54b1b5 100644 --- a/src/services/pipelineStorage/PipelineFile.ts +++ b/src/services/pipelineStorage/PipelineFile.ts @@ -53,6 +53,9 @@ export class PipelineFile { get savedTaskArguments(): Record { return this.redirectedFile?.savedTaskArguments ?? {}; } + get secretsOwner(): string | undefined { + return this.redirectedFile?.secretsOwner; + } async retry(): Promise { await this.redirectedFile?.retry(); diff --git a/src/services/pipelineStorage/RemotePipelineFile.ts b/src/services/pipelineStorage/RemotePipelineFile.ts index 59f19fb02d..216e7c81d4 100644 --- a/src/services/pipelineStorage/RemotePipelineFile.ts +++ b/src/services/pipelineStorage/RemotePipelineFile.ts @@ -19,6 +19,7 @@ import type { RemotePipelineStore } from "./RemotePipelineStore"; export class RemotePipelineFile extends PipelineFile { @observable.ref accessor recovery: RemotePipelineRecovery; + @observable.ref accessor sharedScopes: string[] = []; private pendingWrites = observable.box(0); constructor( @@ -45,12 +46,21 @@ export class RemotePipelineFile extends PipelineFile { const account = this.store.account; return pipeline ? account - ? account.id === pipeline.user_id && - account.permissions.includes("write") + ? account.permissions.includes("write") && + (account.id === pipeline.user_id || + this.sharedScopes.includes("pipeline:update")) : this.recovery.ownerId === pipeline.user_id : !account || account.permissions.includes("write"); } + override get secretsOwner(): string | undefined { + const pipeline = this.recovery.pipeline ?? this.summary; + const account = this.store.account; + return pipeline && account && account.id !== pipeline.user_id + ? pipeline.user_id + : undefined; + } + override get displayName(): string { return this.recovery.displayName; } diff --git a/src/services/pipelineStorage/RemotePipelineStore.test.ts b/src/services/pipelineStorage/RemotePipelineStore.test.ts index b32e0ca987..85b7efec07 100644 --- a/src/services/pipelineStorage/RemotePipelineStore.test.ts +++ b/src/services/pipelineStorage/RemotePipelineStore.test.ts @@ -34,6 +34,7 @@ vi.mock("@/services/cloudPipelineService", () => ({ getCloudPipelineSavedTaskArguments: vi.fn(), getCloudPipeline: vi.fn(), getCloudPipelineAccount: vi.fn(), + getCloudPipelineScopes: vi.fn(async () => []), listCloudPipelinePage: vi.fn(), listCloudPipelines: vi.fn(), writeCloudPipeline: vi.fn(), diff --git a/src/services/pipelineStorage/RemotePipelineStore.ts b/src/services/pipelineStorage/RemotePipelineStore.ts index 8588904bff..a40afa2830 100644 --- a/src/services/pipelineStorage/RemotePipelineStore.ts +++ b/src/services/pipelineStorage/RemotePipelineStore.ts @@ -242,7 +242,11 @@ export class RemotePipelineStore { ); // Other owners can use the same file path; never attach our draft to their pipeline. const ownRecord = record?.pipeline?.id === pipeline.id ? record : undefined; - return this.fromSummary( + const sharedScopes = + pipeline.user_id === this.account?.id + ? [] + : await this.driver.scopes(pipeline); + const file = this.fromSummary( pipeline, ownRecord ? { ...ownRecord, pipeline } @@ -262,6 +266,8 @@ export class RemotePipelineStore { : undefined, }, ); + file.sharedScopes = sharedScopes; + return file; } async resolveLocal(fileId: string): Promise { diff --git a/src/services/pipelineStorage/drivers/RemotePipelineStorageDriver.ts b/src/services/pipelineStorage/drivers/RemotePipelineStorageDriver.ts index c4b0912a45..ad8d11df50 100644 --- a/src/services/pipelineStorage/drivers/RemotePipelineStorageDriver.ts +++ b/src/services/pipelineStorage/drivers/RemotePipelineStorageDriver.ts @@ -10,6 +10,7 @@ import { deleteCloudPipeline, getCloudPipeline, getCloudPipelineAccount, + getCloudPipelineScopes, listCloudPipelinePage, listCloudPipelines, writeCloudPipeline, @@ -121,6 +122,10 @@ export class RemotePipelineStorageDriver implements PipelineStorageDriver { + return getCloudPipelineScopes(pipeline.id, await this.connect()); + } + async write( storageKey: string, content: string, diff --git a/tests/e2e/remote-pipelines.spec.ts b/tests/e2e/remote-pipelines.spec.ts index 24d8168de2..8a8f5e181d 100644 --- a/tests/e2e/remote-pipelines.spec.ts +++ b/tests/e2e/remote-pipelines.spec.ts @@ -78,6 +78,8 @@ async function mockBackend(page: Page, initial = [pipeline()]) { runs: [] as ReturnType[], runSubmissions: [] as BodyCreateApiPipelineRunsPost[], writeOrder: [] as string[], + scopes: {} as Record, + sharedWrites: [] as ReturnType["root_pipeline_task"][], }; const context = page.context(); await context.addInitScript(() => { @@ -137,6 +139,29 @@ async function mockBackend(page: Page, initial = [pipeline()]) { }, }); } + if (url.pathname.startsWith("/api/access_control/pipeline/")) { + return route.fulfill({ + json: { my_scopes: state.scopes[url.pathname.split("/")[4]] ?? [] }, + }); + } + if ( + url.pathname.startsWith("/api/pipelines/") && + request.method() === "PUT" + ) { + const body = request.postDataJSON(); + const item = state.pipelines.find( + (item) => item.id === url.pathname.split("/").at(-1), + ); + if (!item) + return route.fulfill({ status: 404, json: { detail: "Not found" } }); + state.sharedWrites.push(body.root_pipeline_task); + const saved = { ...item, ...body }; + state.pipelines = [ + ...state.pipelines.filter((entry) => entry.id !== item.id), + saved, + ]; + return route.fulfill({ json: saved }); + } if (url.pathname.startsWith("/api/pipelines/")) { state.reads.push(url.pathname); const item = state.pipelines.find( @@ -1197,3 +1222,120 @@ test("opens another owner's pipeline read-only and clones into the viewer's acco state.pipelines.find((item) => item.id === viewerPipelineId)?.pipeline_name, ).toBe("Shared report"); }); + +const sharedOwner = "ada@example.com"; + +function containerTask( + name: string, + inputs: string[], + args: Record, + position: { x: number; y: number }, +) { + return { + componentRef: { + spec: { + name, + inputs: inputs.map((input) => ({ + name: input, + type: "String", + optional: input === "api_key", + })), + implementation: { + container: { image: "python:3.12", command: ["python", "-c", name] }, + }, + }, + }, + arguments: args, + annotations: { "editor.position": JSON.stringify(position) }, + }; +} + +function sharedPipelineWithSecret() { + const item = pipeline(viewerPipelineId, sharedOwner, "Churn model"); + item.root_pipeline_task.componentRef.spec.implementation.graph.tasks = { + "Fetch data": containerTask( + "Fetch data", + ["token", "table"], + { + token: { dynamicData: { secret: { name: "WAREHOUSE_TOKEN" } } }, + table: "orders", + }, + { x: 80, y: 120 }, + ), + "Train model": containerTask( + "Train model", + ["epochs", "api_key"], + { epochs: "10" }, + { x: 480, y: 120 }, + ), + } as never; + return item; +} + +async function openTask(page: Page, name: string) { + await page.locator(".react-flow__node").filter({ hasText: name }).click(); +} + +test("an editor changes a shared pipeline around its secrets", async ({ + page, +}) => { + const state = await mockBackend(page, [sharedPipelineWithSecret()]); + state.scopes[viewerPipelineId] = ["pipeline:read", "pipeline:update"]; + await page.goto(editorUrl(viewerPipelineId)); + await expect(page.locator("[data-editor-ready]")).toBeVisible(); + await expect(page.getByTestId("auto-save-button")).toBeEnabled(); + + await openTask(page, "Fetch data"); + await expect( + page.getByText(`Only ${sharedOwner} can change tasks with secrets.`), + ).toBeVisible(); + await page.screenshot({ path: "test-results/editor-tips/secret-task.png" }); + + await openTask(page, "Train model"); + const row = page.locator('[data-argument-name="api_key"]'); + await row.hover(); + await row.getByTestId("thunder-menu-trigger").click(); + await page + .locator('[data-tour="thunder-menu-content"]') + .getByText(/dynamic data/i) + .click(); + const submenu = page.locator('[data-tour="thunder-menu-submenu-content"]'); + await expect( + submenu.getByText(`Ask ${sharedOwner} to add secrets.`), + ).toBeVisible(); + await submenu.evaluate((element) => + Promise.all( + element + .getAnimations({ subtree: true }) + .map((animation) => animation.finished), + ), + ); + await page.screenshot({ path: "test-results/editor-tips/add-secret.png" }); + await page.keyboard.press("Escape"); + await page.keyboard.press("Escape"); + await openTask(page, "Train model"); + + const epochs = page + .locator('[data-argument-name="epochs"]') + .getByTestId("argument-input"); + await epochs.click(); + await epochs.fill("20"); + await epochs.blur(); + await expect.poll(() => state.sharedWrites.length).toBeGreaterThan(0); + const saved = state.sharedWrites.at(-1) as unknown as { + componentRef: { + spec: { + implementation: { + graph: { tasks: Record }; + }; + }; + }; + }; + const tasks = saved.componentRef.spec.implementation.graph.tasks; + expect(tasks["Train model"].arguments).toEqual({ epochs: "20" }); + expect(tasks["Fetch data"].arguments).toEqual({ + token: { dynamicData: { secret: { name: "WAREHOUSE_TOKEN" } } }, + table: "orders", + }); + expect(state.writes).toHaveLength(0); +});