From 4d5a7f8cde5cbfe37352efe10a309965a5e512d7 Mon Sep 17 00:00:00 2001 From: Camiel van Schoonhoven Date: Thu, 22 Oct 2026 11:39:40 -0700 Subject: [PATCH] feat(secrets): scope secret storage to a team Secrets are stored per caller, so a team's secrets are read and written by sending the impersonation header on these four requests. Every function in `secretsStorage` now takes an optional team id and sends the header only when one is given. Every caller still passes none, so they behave exactly as before. - `secretsListQuery(teamId)` builds the cache key and the fetch from the same team id, so the cache can never hold one account's secrets under another's key. The three screens that list secrets use it instead of handing `fetchSecretsList` to React Query directly, which would now receive the query context as its team id. - `SecretsQueryKeys.All` keys the user's secrets as `["secrets", "user"]` and a team's as `["secrets", "team", teamId]`. The unused `Id` key is gone, since it no longer fits that shape. - `addSecret` and `updateSecret` pass `description` and `expires_at`, which the API has always accepted as query params. No form sets them yet. - Failures carry the backend's reason instead of `[object ReadableStream]`. They throw `SecretsApiError` with the status, or `TeamImpersonationError` when the backend refuses to act as the team, so a later screen can tell that apart from an expired session. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../SecretsManagement/SelectSecretDialog.tsx | 9 +- .../components/ReplaceSecretView.tsx | 8 +- .../components/SecretsList.tsx | 9 +- .../SecretsManagement/secretsStorage.test.ts | 236 ++++++++++++++++++ .../SecretsManagement/secretsStorage.ts | 63 ++++- .../shared/SecretsManagement/types.ts | 9 +- 6 files changed, 303 insertions(+), 31 deletions(-) create mode 100644 src/components/shared/SecretsManagement/secretsStorage.test.ts diff --git a/src/components/shared/SecretsManagement/SelectSecretDialog.tsx b/src/components/shared/SecretsManagement/SelectSecretDialog.tsx index 6650e82bbe..389856e01a 100644 --- a/src/components/shared/SecretsManagement/SelectSecretDialog.tsx +++ b/src/components/shared/SecretsManagement/SelectSecretDialog.tsx @@ -22,8 +22,8 @@ import { useTourMockBackend } from "@/providers/TourProvider/tourMockBackend"; import { AddSecretForm } from "./components/AddSecretForm"; import { SecretsBackendUnavailable } from "./components/SecretsBackendUnavailable"; -import { fetchSecretsList } from "./secretsStorage"; -import { type Secret, SecretsQueryKeys } from "./types"; +import { secretsListQuery } from "./secretsStorage"; +import type { Secret } from "./types"; type DialogMode = "select" | "add"; @@ -107,10 +107,7 @@ interface SelectSecretDialogContentProps { function SelectSecretDialogContentInternal({ onSelect, }: SelectSecretDialogContentProps) { - const { data: secrets } = useSuspenseQuery({ - queryKey: SecretsQueryKeys.All(), - queryFn: fetchSecretsList, - }); + const { data: secrets } = useSuspenseQuery(secretsListQuery()); const [mode, setMode] = useState("select"); diff --git a/src/components/shared/SecretsManagement/components/ReplaceSecretView.tsx b/src/components/shared/SecretsManagement/components/ReplaceSecretView.tsx index 70b87d8b58..4c48db4474 100644 --- a/src/components/shared/SecretsManagement/components/ReplaceSecretView.tsx +++ b/src/components/shared/SecretsManagement/components/ReplaceSecretView.tsx @@ -8,8 +8,7 @@ import { Paragraph } from "@/components/ui/typography"; import useToastNotification from "@/hooks/useToastNotification"; import { useAnalytics } from "@/providers/AnalyticsProvider"; -import { fetchSecretsList } from "../secretsStorage"; -import { SecretsQueryKeys } from "../types"; +import { secretsListQuery } from "../secretsStorage"; import { AddSecretForm } from "./AddSecretForm"; import { SecretsBreadcrumbs } from "./SecretsBreadcrumbs"; @@ -19,10 +18,7 @@ export function ReplaceSecretView() { const navigate = useNavigate(); const { track } = useAnalytics(); - const { data: secrets } = useSuspenseQuery({ - queryKey: SecretsQueryKeys.All(), - queryFn: fetchSecretsList, - }); + const { data: secrets } = useSuspenseQuery(secretsListQuery()); const secret = secrets.find((s) => s.id === secretId); diff --git a/src/components/shared/SecretsManagement/components/SecretsList.tsx b/src/components/shared/SecretsManagement/components/SecretsList.tsx index 1c867c5be9..305ab17759 100644 --- a/src/components/shared/SecretsManagement/components/SecretsList.tsx +++ b/src/components/shared/SecretsManagement/components/SecretsList.tsx @@ -12,8 +12,8 @@ import { useTourMockBackend } from "@/providers/TourProvider/tourMockBackend"; import { formatRelativeTime } from "@/utils/date"; import { withSuspenseWrapper } from "../../SuspenseWrapper"; -import { fetchSecretsList } from "../secretsStorage"; -import { type Secret, SecretsQueryKeys } from "../types"; +import { secretsListQuery } from "../secretsStorage"; +import type { Secret } from "../types"; import { RemoveSecretButton } from "./RemoveSecretButton"; import { SecretsBackendUnavailable } from "./SecretsBackendUnavailable"; @@ -26,10 +26,7 @@ function SecretsListInternal({ onRemoveSuccess, onEditSecret, }: SecretsListProps) { - const { data: secrets } = useSuspenseQuery({ - queryKey: SecretsQueryKeys.All(), - queryFn: fetchSecretsList, - }); + const { data: secrets } = useSuspenseQuery(secretsListQuery()); if (secrets.length === 0) { return ( diff --git a/src/components/shared/SecretsManagement/secretsStorage.test.ts b/src/components/shared/SecretsManagement/secretsStorage.test.ts new file mode 100644 index 0000000000..617466cfee --- /dev/null +++ b/src/components/shared/SecretsManagement/secretsStorage.test.ts @@ -0,0 +1,236 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +vi.mock("@/api/sdk.gen", () => ({ + createSecretApiSecretsPost: vi.fn(), + deleteSecretApiSecretsSecretNameDelete: vi.fn(), + listSecretsApiSecretsGet: vi.fn(), + updateSecretApiSecretsSecretNamePut: vi.fn(), +})); + +vi.mock("@/providers/TourProvider/tourMockBackend", () => ({ + isTourMockActive: () => false, + mockAddSecret: vi.fn(), + mockListSecrets: vi.fn(), + mockRemoveSecret: vi.fn(), + mockUpdateSecret: vi.fn(), +})); + +import { + createSecretApiSecretsPost, + deleteSecretApiSecretsSecretNameDelete, + listSecretsApiSecretsGet, + updateSecretApiSecretsSecretNamePut, +} from "@/api/sdk.gen"; +import { TeamImpersonationError } from "@/services/teams/errors"; + +import { + addSecret, + fetchSecretsList, + removeSecret, + SecretsApiError, + secretsListQuery, + updateSecret, +} from "./secretsStorage"; + +const TEAM_ID = "ml-team@project.iam.gserviceaccount.com"; +const HEADER_NAME = "x-tangle-impersonate-team-id"; +const TEAM_HEADERS = { [HEADER_NAME]: TEAM_ID }; + +const sdkResult = (status: number, body: { data?: unknown; error?: unknown }) => + ({ + data: body.data, + error: body.error, + request: new Request("http://localhost/api/secrets/"), + response: new Response(null, { status }), + }) as never; + +describe("secretsStorage", () => { + beforeEach(() => { + vi.clearAllMocks(); + window.__TANGLE_TEAM_IMPERSONATION__ = { headerName: HEADER_NAME }; + }); + + afterEach(() => { + delete window.__TANGLE_TEAM_IMPERSONATION__; + }); + + describe("fetchSecretsList", () => { + beforeEach(() => { + vi.mocked(listSecretsApiSecretsGet).mockResolvedValue( + sdkResult(200, { + data: { + secrets: [ + { + secret_name: "API_KEY", + created_at: "2026-10-01T10:00:00", + updated_at: "2026-10-02T10:00:00Z", + expires_at: "2026-12-31T23:59:59Z", + description: "For the model provider", + }, + ], + }, + }), + ); + }); + + it("lists the user's own secrets without the team header", async () => { + await expect(fetchSecretsList()).resolves.toEqual([ + { + id: "API_KEY", + name: "API_KEY", + createdAt: new Date("2026-10-01T10:00:00Z"), + updatedAt: new Date("2026-10-02T10:00:00Z"), + expiresAt: new Date("2026-12-31T23:59:59Z"), + description: "For the model provider", + }, + ]); + expect(listSecretsApiSecretsGet).toHaveBeenCalledWith({ headers: {} }); + }); + + it("lists a team's secrets while acting as the team", async () => { + await fetchSecretsList(TEAM_ID); + + expect(listSecretsApiSecretsGet).toHaveBeenCalledWith({ + headers: TEAM_HEADERS, + }); + }); + + it("throws a TeamImpersonationError when the backend refuses the team", async () => { + const refusal = `Impersonation error: Unknown team ${TEAM_ID}`; + vi.mocked(listSecretsApiSecretsGet).mockResolvedValue( + sdkResult(401, { error: { detail: refusal } }), + ); + + const failure = fetchSecretsList(TEAM_ID); + + await expect(failure).rejects.toBeInstanceOf(TeamImpersonationError); + await expect(failure).rejects.toMatchObject({ + message: refusal, + status: 401, + }); + }); + }); + + describe("secretsListQuery", () => { + it("keeps a team's secrets apart from the user's in the cache", () => { + expect(secretsListQuery().queryKey).toEqual(["secrets", "user"]); + expect(secretsListQuery(TEAM_ID).queryKey).toEqual([ + "secrets", + "team", + TEAM_ID, + ]); + }); + + it("fetches the secrets of the team its key names", async () => { + vi.mocked(listSecretsApiSecretsGet).mockResolvedValue( + sdkResult(200, { data: { secrets: [] } }), + ); + + await secretsListQuery(TEAM_ID).queryFn(); + + expect(listSecretsApiSecretsGet).toHaveBeenCalledWith({ + headers: TEAM_HEADERS, + }); + }); + }); + + describe("addSecret", () => { + it("sends the description and expiry with the team header", async () => { + vi.mocked(createSecretApiSecretsPost).mockResolvedValue( + sdkResult(200, { data: {} }), + ); + + await addSecret( + { + name: "API_KEY", + value: "s3cret", + description: "For the model provider", + expiresAt: new Date("2026-12-31T23:59:59.999Z"), + }, + TEAM_ID, + ); + + expect(createSecretApiSecretsPost).toHaveBeenCalledWith({ + query: { + secret_name: "API_KEY", + description: "For the model provider", + expires_at: "2026-12-31T23:59:59.999Z", + }, + body: { secret_value: "s3cret" }, + headers: TEAM_HEADERS, + }); + }); + + it("carries the backend's reason when it refuses", async () => { + vi.mocked(createSecretApiSecretsPost).mockResolvedValue( + sdkResult(409, { + error: { detail: "Secret API_KEY already exists" }, + }), + ); + + const failure = addSecret({ name: "API_KEY", value: "s3cret" }); + + await expect(failure).rejects.toBeInstanceOf(SecretsApiError); + await expect(failure).rejects.toMatchObject({ + message: "Failed to add secret: Secret API_KEY already exists", + status: 409, + }); + }); + }); + + describe("updateSecret", () => { + it("replaces the value, description and expiry with the team header", async () => { + vi.mocked(updateSecretApiSecretsSecretNamePut).mockResolvedValue( + sdkResult(200, { data: {} }), + ); + + await updateSecret( + "API_KEY", + { + value: "n3w", + description: "Rotated", + expiresAt: new Date("2027-01-31T23:59:59.999Z"), + }, + TEAM_ID, + ); + + expect(updateSecretApiSecretsSecretNamePut).toHaveBeenCalledWith({ + path: { secret_name: "API_KEY" }, + query: { + description: "Rotated", + expires_at: "2027-01-31T23:59:59.999Z", + }, + body: { secret_value: "n3w" }, + headers: TEAM_HEADERS, + }); + }); + }); + + describe("removeSecret", () => { + it("removes a team's secret with the team header", async () => { + vi.mocked(deleteSecretApiSecretsSecretNameDelete).mockResolvedValue( + sdkResult(200, { data: {} }), + ); + + await removeSecret("API_KEY", TEAM_ID); + + expect(deleteSecretApiSecretsSecretNameDelete).toHaveBeenCalledWith({ + path: { secret_name: "API_KEY" }, + headers: TEAM_HEADERS, + }); + }); + + it("removes the user's own secret without it", async () => { + vi.mocked(deleteSecretApiSecretsSecretNameDelete).mockResolvedValue( + sdkResult(200, { data: {} }), + ); + + await removeSecret("API_KEY"); + + expect(deleteSecretApiSecretsSecretNameDelete).toHaveBeenCalledWith({ + path: { secret_name: "API_KEY" }, + headers: {}, + }); + }); + }); +}); diff --git a/src/components/shared/SecretsManagement/secretsStorage.ts b/src/components/shared/SecretsManagement/secretsStorage.ts index 4842838745..30c0d58333 100644 --- a/src/components/shared/SecretsManagement/secretsStorage.ts +++ b/src/components/shared/SecretsManagement/secretsStorage.ts @@ -11,8 +11,15 @@ import { mockRemoveSecret, mockUpdateSecret, } from "@/providers/TourProvider/tourMockBackend"; +import { ApiError } from "@/services/projects/errors"; +import { describeErrorBody } from "@/services/requestFailureMessage"; +import { + readImpersonationRefusal, + TeamImpersonationError, +} from "@/services/teams/errors"; +import { teamRequestHeaders } from "@/services/teams/teamRequestHeaders"; -import type { Secret } from "./types"; +import { type Secret, SecretsQueryKeys } from "./types"; /** * Parses a date string, assuming UTC if no timezone is specified. @@ -39,15 +46,37 @@ function parseAsUtc(dateString: string): Date { return new Date(dateString + "Z"); } -export async function fetchSecretsList() { +export class SecretsApiError extends ApiError { + constructor(message: string, status: number) { + super("SecretsApiError", message, status); + } +} + +function secretsFailure( + action: string, + result: { error: unknown; response: Response }, +): ApiError { + const status = result.response.status; + const refusal = readImpersonationRefusal(result.error); + if (refusal) { + return new TeamImpersonationError(refusal, status); + } + + const reason = describeErrorBody(result.error); + return new SecretsApiError(reason ? `${action}: ${reason}` : action, status); +} + +export async function fetchSecretsList(teamId?: string) { if (isTourMockActive()) { return mockListSecrets(); } - const response = await listSecretsApiSecretsGet(); + const response = await listSecretsApiSecretsGet({ + headers: teamRequestHeaders(teamId), + }); if (response.response.status !== 200) { - throw new Error(`Failed to fetch secrets: ${response.response.body}`); + throw secretsFailure("Failed to fetch secrets", response); } return ( @@ -67,9 +96,17 @@ export async function fetchSecretsList() { ); } +export function secretsListQuery(teamId?: string) { + return { + queryKey: SecretsQueryKeys.All(teamId), + queryFn: () => fetchSecretsList(teamId), + }; +} + export async function updateSecret( secretId: string, secret: Partial & Pick, + teamId?: string, ) { if (isTourMockActive()) { mockUpdateSecret(secretId, secret.value); @@ -80,13 +117,18 @@ export async function updateSecret( path: { secret_name: secretId, }, + query: { + description: secret.description, + expires_at: secret.expiresAt?.toISOString(), + }, body: { secret_value: secret.value ?? "", }, + headers: teamRequestHeaders(teamId), }); if (response.response.status !== 200) { - throw new Error(`Failed to update secret: ${response.response.body}`); + throw secretsFailure("Failed to update secret", response); } return true; @@ -94,6 +136,7 @@ export async function updateSecret( export async function addSecret( secret: Partial & Pick, + teamId?: string, ) { if (isTourMockActive()) { mockAddSecret(secret.name ?? "", secret.value); @@ -103,20 +146,23 @@ export async function addSecret( const response = await createSecretApiSecretsPost({ query: { secret_name: secret.name ?? "", + description: secret.description, + expires_at: secret.expiresAt?.toISOString(), }, body: { secret_value: secret.value ?? "", }, + headers: teamRequestHeaders(teamId), }); if (response.response.status !== 200) { - throw new Error(`Failed to add secret: ${response.response.body}`); + throw secretsFailure("Failed to add secret", response); } return true; } -export async function removeSecret(secretId: string) { +export async function removeSecret(secretId: string, teamId?: string) { if (isTourMockActive()) { mockRemoveSecret(secretId); return true; @@ -126,10 +172,11 @@ export async function removeSecret(secretId: string) { path: { secret_name: secretId, }, + headers: teamRequestHeaders(teamId), }); if (response.response.status !== 200) { - throw new Error(`Failed to remove secret: ${response.response.body}`); + throw secretsFailure("Failed to remove secret", response); } return true; diff --git a/src/components/shared/SecretsManagement/types.ts b/src/components/shared/SecretsManagement/types.ts index 8e708cedd7..287bcea52a 100644 --- a/src/components/shared/SecretsManagement/types.ts +++ b/src/components/shared/SecretsManagement/types.ts @@ -38,10 +38,9 @@ export function extractSecretName(arg: DynamicDataArgument): string | null { return null; } -/** - * Query keys for React Query. - */ export const SecretsQueryKeys = { - All: () => ["secrets"] as const, - Id: (id: string) => ["secrets", id] as const, + All: (teamId?: string) => + teamId + ? (["secrets", "team", teamId] as const) + : (["secrets", "user"] as const), } as const;