diff --git a/.changeset/quiet-org-lookup-denial.md b/.changeset/quiet-org-lookup-denial.md new file mode 100644 index 00000000000..54d5ed06cd9 --- /dev/null +++ b/.changeset/quiet-org-lookup-denial.md @@ -0,0 +1,13 @@ +--- +"@agent-native/core": patch +--- + +Reject unavailable organization scope instead of treating failed membership reads as Personal access during credential resolution, MCP identity setup, or background task creation. Generic credential reads also reject non-transient membership failures before returning a stored Personal credential whose organization priority could not be checked. Preserve the narrow first-boot missing-membership-table exception for action dispatch. + +Keep revoked or mismatched delegated organization access a definitive authorization refusal, including MCP authentication and credential lookup wrappers. Resolve generic credentials' organization scope before provider policy checks and report failed policy or role reads as credential-store unavailability with the original cause. + +Keep failed non-transient credential lookups distinct from missing credentials in the secret and secret-pair wrappers, preserving the original failure. Propagate unavailable membership reads through event-based organization resolution and session backfill for ordinary and delegated requests instead of returning Personal scope. Keep authorization error classification independent of sharing database initialization. + +Keep the engine catalog available when current-engine discovery cannot read a provider credential, reporting an unavailable current selection with its error alongside per-provider diagnostics. Runtime engine detection and definitive authorization refusals still reject. + +Record a scoped failed background automation attempt when creator-membership or organization-admin alert lookup is unreadable. Preserve the typed failure and original cause, stop before credential or agent execution, and keep a proven former creator's same-organization admin fallback. diff --git a/packages/core/src/credentials/index.org-resolution.spec.ts b/packages/core/src/credentials/index.org-resolution.spec.ts new file mode 100644 index 00000000000..66c2f3d4241 --- /dev/null +++ b/packages/core/src/credentials/index.org-resolution.spec.ts @@ -0,0 +1,312 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +const fixture = vi.hoisted(() => ({ + email: "owner@example.test", + orgId: "credential-org", + membershipError: null as Error | null, + membershipIds: ["credential-org"], + role: "owner", + roleError: null as Error | null, + restricted: false, + personalSelected: false, + personalValue: "fake-personal-credential" as string | null, +})); + +vi.mock("../db/client.js", async (importOriginal) => ({ + ...(await importOriginal()), + getDbExec: () => ({ + execute: async ({ sql }: { sql: string }) => { + if (sql.includes("SELECT org_id FROM org_members")) { + if (fixture.membershipError) throw fixture.membershipError; + return { rows: fixture.membershipIds.map((org_id) => ({ org_id })) }; + } + if (sql.includes("SELECT role FROM org_members")) { + if (fixture.roleError) throw fixture.roleError; + return { rows: fixture.role ? [{ role: fixture.role }] : [] }; + } + throw new Error(`Unexpected query: ${sql}`); + }, + }), +})); + +vi.mock("../settings/store.js", async (importOriginal) => ({ + ...(await importOriginal()), + getSetting: async (key: string) => { + if (key === `u:${fixture.email}:active-org-id`) { + return fixture.personalSelected ? { orgId: null } : null; + } + if (key === `o:${fixture.orgId}:restrict-personal-provider-keys`) { + return { restricted: fixture.restricted }; + } + if ( + key === `u:${fixture.email}:credential:GENERIC_SERVICE_TOKEN` || + key === `u:${fixture.email}:credential:ANTHROPIC_API_KEY` + ) { + return fixture.personalValue ? { value: fixture.personalValue } : null; + } + throw new Error(`Unexpected setting: ${key}`); + }, +})); + +vi.mock("../secrets/storage.js", () => ({ + readAppSecret: async (ref: { key: string; scope: string; scopeId: string }) => + ["GENERIC_SERVICE_TOKEN", "ANTHROPIC_API_KEY"].includes(ref.key) && + ref.scope === "org" && + ref.scopeId === fixture.orgId + ? { value: "fake-org-credential" } + : null, +})); + +import { CredentialStoreUnavailableError } from "../server/credential-provider.js"; +import { + resolveSecretDetailed, + resolveBuilderGatewayAuth, +} from "../server/credential-provider.js"; +import { runWithRequestContext } from "../server/request-context.js"; +import { ForbiddenError } from "../sharing/access.js"; +import { resolveCredentialDetailed } from "./index.js"; + +function resolveGenericCredential() { + return runWithRequestContext({ userEmail: fixture.email }, () => + resolveCredentialDetailed("GENERIC_SERVICE_TOKEN", { + userEmail: fixture.email, + }), + ); +} + +describe("generic credentials with real organization resolution", () => { + beforeEach(() => { + fixture.membershipError = null; + fixture.membershipIds = [fixture.orgId]; + fixture.role = "owner"; + fixture.roleError = null; + fixture.restricted = false; + fixture.personalSelected = false; + fixture.personalValue = "fake-personal-credential"; + }); + + it.each([ + ["GENERIC_SERVICE_TOKEN", undefined], + ["ANTHROPIC_API_KEY", undefined], + ["GENERIC_SERVICE_TOKEN", "credential-org"], + ["ANTHROPIC_API_KEY", "credential-org"], + ])( + "preserves revoked delegation as forbidden for %s with explicit org %s", + async (key, orgId) => { + fixture.role = ""; + await expect( + runWithRequestContext( + { + userEmail: fixture.email, + orgId: fixture.orgId, + a2aIdentityAssurance: "org-member", + }, + () => + resolveCredentialDetailed(key!, { + userEmail: fixture.email, + orgId, + }), + ), + ).rejects.toBeInstanceOf(ForbiddenError); + }, + ); + + it("preserves delegated refusal through the detailed provider resolver", async () => { + await expect( + runWithRequestContext( + { userEmail: fixture.email, a2aIdentityAssurance: "org-member" }, + () => resolveSecretDetailed("GENERIC_SERVICE_TOKEN"), + ), + ).rejects.toBeInstanceOf(ForbiddenError); + }); + + it("preserves delegated refusal through Builder gateway authentication", async () => { + await expect( + runWithRequestContext( + { userEmail: fixture.email, a2aIdentityAssurance: "org-member" }, + () => resolveBuilderGatewayAuth(), + ), + ).rejects.toBeInstanceOf(ForbiddenError); + }); + + it("refuses an explicit organization outside the valid delegation", async () => { + await expect( + runWithRequestContext( + { + userEmail: fixture.email, + orgId: fixture.orgId, + a2aIdentityAssurance: "org-member", + }, + () => + resolveCredentialDetailed("GENERIC_SERVICE_TOKEN", { + userEmail: fixture.email, + orgId: "other-org", + credentialScope: "org", + }), + ), + ).rejects.toBeInstanceOf(ForbiddenError); + }); + + it("resolves the explicit organization for a valid delegation", async () => { + await expect( + runWithRequestContext( + { + userEmail: fixture.email, + orgId: fixture.orgId, + a2aIdentityAssurance: "org-member", + }, + () => + resolveCredentialDetailed("GENERIC_SERVICE_TOKEN", { + userEmail: fixture.email, + orgId: fixture.orgId, + credentialScope: "org", + }), + ), + ).resolves.toMatchObject({ value: "fake-org-credential", scope: "org" }); + }); + + it.each(["42501", "08006"])( + "types provider-policy membership failures with %s as unavailable", + async (code) => { + fixture.membershipError = Object.assign( + new Error("fake membership lookup failure"), + { code }, + ); + const lookup = runWithRequestContext({ userEmail: fixture.email }, () => + resolveCredentialDetailed("ANTHROPIC_API_KEY", { + userEmail: fixture.email, + }), + ); + await expect(lookup).rejects.toBeInstanceOf( + CredentialStoreUnavailableError, + ); + await expect(lookup).rejects.toMatchObject({ + cause: fixture.membershipError, + retryable: true, + }); + }, + ); + + it.each(["42501", "08006"])( + "types provider-policy role lookup failures with %s as unavailable", + async (code) => { + fixture.restricted = true; + fixture.roleError = Object.assign(new Error("fake role lookup failure"), { + code, + }); + const lookup = runWithRequestContext({ userEmail: fixture.email }, () => + resolveCredentialDetailed("ANTHROPIC_API_KEY", { + userEmail: fixture.email, + }), + ); + await expect(lookup).rejects.toBeInstanceOf( + CredentialStoreUnavailableError, + ); + await expect(lookup).rejects.toMatchObject({ + cause: fixture.roleError, + retryable: true, + }); + }, + ); + + it.each([ + ["personal", "member", true, "fake-personal-credential", "user"], + ["organization", "member", true, "fake-org-credential", "org"], + ["organization", "member", false, "fake-personal-credential", "user"], + ["organization", "owner", true, "fake-org-credential", "org"], + ["organization", "admin", true, "fake-org-credential", "org"], + ])( + "preserves provider policy for %s %s restricted=%s", + async (scope, role, restricted, value, resultScope) => { + fixture.membershipIds = scope === "personal" ? [] : [fixture.orgId]; + fixture.role = role; + fixture.restricted = restricted; + await expect( + runWithRequestContext({ userEmail: fixture.email }, () => + resolveCredentialDetailed("ANTHROPIC_API_KEY", { + userEmail: fixture.email, + }), + ), + ).resolves.toMatchObject({ value, scope: resultScope }); + }, + ); + + it("keeps org-only provider lookup bound to the explicit organization", async () => { + fixture.membershipError = new Error("must not infer scope"); + await expect( + resolveCredentialDetailed("ANTHROPIC_API_KEY", { + userEmail: fixture.email, + orgId: fixture.orgId, + credentialScope: "org", + }), + ).resolves.toMatchObject({ value: "fake-org-credential", scope: "org" }); + await expect( + resolveCredentialDetailed("ANTHROPIC_API_KEY", { + userEmail: fixture.email, + credentialScope: "org", + }), + ).resolves.toBeUndefined(); + }); + + it.each(["42501", "08006"])( + "rejects a readable personal credential when membership lookup fails with %s", + async (code) => { + fixture.membershipError = Object.assign( + new Error( + code === "42501" + ? "permission denied for table org_members" + : "connection reset", + ), + { code }, + ); + + const lookup = resolveGenericCredential(); + await expect(lookup).rejects.toBeInstanceOf( + CredentialStoreUnavailableError, + ); + await expect(lookup).rejects.toMatchObject({ + cause: fixture.membershipError, + retryable: true, + }); + }, + ); + + it("returns a readable personal credential when membership confirms no organization", async () => { + fixture.membershipIds = []; + + await expect(resolveGenericCredential()).resolves.toEqual({ + value: fixture.personalValue, + scope: "user", + scopeId: fixture.email, + }); + }); + + it("keeps an explicit Personal selection even when the user has an organization", async () => { + fixture.personalSelected = true; + + await expect(resolveGenericCredential()).resolves.toMatchObject({ + value: fixture.personalValue, + scope: "user", + }); + }); + + it("preserves genuine credential absence in Personal scope", async () => { + fixture.membershipIds = []; + fixture.personalValue = null; + + await expect(resolveGenericCredential()).resolves.toBeUndefined(); + }); + + it.each([ + ["owner", "fake-org-credential", "org"], + ["admin", "fake-org-credential", "org"], + ["member", "fake-personal-credential", "user"], + ])("preserves %s credential priority", async (role, value, scope) => { + fixture.role = role; + + await expect(resolveGenericCredential()).resolves.toMatchObject({ + value, + scope, + }); + }); +}); diff --git a/packages/core/src/credentials/index.ts b/packages/core/src/credentials/index.ts index 57d2f929dc3..7197818fe95 100644 --- a/packages/core/src/credentials/index.ts +++ b/packages/core/src/credentials/index.ts @@ -10,7 +10,7 @@ import { readAppSecret, type SecretRef, } from "../secrets/storage.js"; -import { assertCredentialStoreReadable } from "../server/credential-provider.js"; +import { CredentialStoreUnavailableError } from "../server/credential-provider.js"; import { isPersonalCredentialScope, readsOrgCredentialFirst, @@ -22,6 +22,7 @@ import { } from "../server/personal-provider-key-policy.js"; import { getRequestContext } from "../server/request-context.js"; import { getSetting, putSetting, deleteSetting } from "../settings/store.js"; +import { ForbiddenError } from "../sharing/errors.js"; const SETTING_PREFIX = "credential:"; @@ -193,14 +194,22 @@ export async function resolveCredentialForScope( async function resolveEffectiveOrgId( ctx: CredentialContext, ): Promise<{ orgId: string | null; lookupFailed: boolean; cause?: unknown }> { - if (ctx.orgId) return { orgId: ctx.orgId, lookupFailed: false }; try { + const delegated = + getRequestContext()?.a2aIdentityAssurance === "org-member"; + if (ctx.orgId && !delegated) + return { orgId: ctx.orgId, lookupFailed: false }; const { resolveOrgIdForEmail } = await import("../org/context.js"); + const orgId = await resolveOrgIdForEmail(ctx.userEmail); + if (delegated && ctx.orgId && ctx.orgId !== orgId) { + throw new ForbiddenError(); + } return { - orgId: await resolveOrgIdForEmail(ctx.userEmail), + orgId, lookupFailed: false, }; } catch (cause) { + if (cause instanceof ForbiddenError) throw cause; return { orgId: null, lookupFailed: true, cause }; } } @@ -236,15 +245,26 @@ export async function resolveCredentialDetailed( ): Promise { if (!ctx?.userEmail) return undefined; + if (ctx.credentialScope === "org" && !ctx.orgId) return undefined; + const orgLookup = await resolveEffectiveOrgId(ctx); + if (orgLookup.lookupFailed) { + throw new CredentialStoreUnavailableError(orgLookup.cause); + } + const { orgId } = orgLookup; + // Stored but unused while the org restricts a member's provider keys. - const personalRestricted = - ctx.credentialScope !== "org" && - isPersonalProviderPolicyKey(key) && - (await isPersonalProviderKeyUseRestricted( - ctx.orgId - ? { email: ctx.userEmail, orgId: ctx.orgId } - : { email: ctx.userEmail }, - )); + let personalRestricted = false; + if (ctx.credentialScope !== "org" && isPersonalProviderPolicyKey(key)) { + try { + personalRestricted = await isPersonalProviderKeyUseRestricted({ + email: ctx.userEmail, + orgId, + }); + } catch (cause) { + if (cause instanceof ForbiddenError) throw cause; + throw new CredentialStoreUnavailableError(cause); + } + } let personal: ResolvedCredential | undefined; if (ctx.credentialScope !== "org" && !personalRestricted) { @@ -254,11 +274,6 @@ export async function resolveCredentialDetailed( if (value) personal = { value, scope: "user", scopeId: ctx.userEmail }; } - if (ctx.credentialScope === "org" && !ctx.orgId) return undefined; - const orgLookup = await resolveEffectiveOrgId(ctx); - // Before the personal answer: without the org, an owner's role is unknown. - assertCredentialStoreReadable(orgLookup); - const { orgId } = orgLookup; // An owner or admin runs on the organization's credential ahead of their own. if (personal && !(await readsOrgCredentialFirst(orgId, ctx.userEmail))) { return personal; diff --git a/packages/core/src/credentials/personal-key-policy.spec.ts b/packages/core/src/credentials/personal-key-policy.spec.ts index 8eb046a4f23..582e706d653 100644 --- a/packages/core/src/credentials/personal-key-policy.spec.ts +++ b/packages/core/src/credentials/personal-key-policy.spec.ts @@ -28,6 +28,7 @@ vi.mock( }), ); +import { CredentialStoreUnavailableError } from "../server/credential-provider.js"; import { resolveCredential } from "./index.js"; const EMAIL = "member@example.com"; @@ -82,7 +83,10 @@ describe("resolveCredential with personal API keys restricted", () => { await expect( resolveCredential("OPENAI_API_KEY", { userEmail: EMAIL }), ).resolves.toBeUndefined(); - expect(mocks.isRestricted).toHaveBeenCalledWith({ email: EMAIL }); + expect(mocks.isRestricted).toHaveBeenCalledWith({ + email: EMAIL, + orgId: "org-1", + }); }); it("leaves other credentials and unrestricted callers alone", async () => { @@ -101,10 +105,13 @@ describe("resolveCredential with personal API keys restricted", () => { }); it("fails loudly when the restriction can't be read", async () => { - mocks.isRestricted.mockRejectedValue(new Error("settings unavailable")); + const cause = new Error("settings unavailable"); + mocks.isRestricted.mockRejectedValue(cause); rows({ [`user:${EMAIL}:ANTHROPIC_API_KEY`]: "sk-ant-personal" }); - await expect( - resolveCredential("ANTHROPIC_API_KEY", { userEmail: EMAIL }), - ).rejects.toThrow("settings unavailable"); + const lookup = resolveCredential("ANTHROPIC_API_KEY", { userEmail: EMAIL }); + await expect(lookup).rejects.toBeInstanceOf( + CredentialStoreUnavailableError, + ); + await expect(lookup).rejects.toMatchObject({ cause, retryable: true }); }); }); diff --git a/packages/core/src/jobs/background-automation-runner.spec.ts b/packages/core/src/jobs/background-automation-runner.spec.ts index 3a251d3d57f..e7a49f3c30c 100644 --- a/packages/core/src/jobs/background-automation-runner.spec.ts +++ b/packages/core/src/jobs/background-automation-runner.spec.ts @@ -2334,6 +2334,107 @@ describe("runBackgroundAutomation — preconditions fail before any thread or ru return history.notification_email; } + it.each(["membership", "admin"] as const)( + "records a failed attempt when the %s recipient lookup is unreadable", + async (deniedAt) => { + await seedOrg([["bob@agent-native.test", "owner"]]); + const { ensureTable } = await import("./run-history.js"); + await ensureTable(); + const { resolveOwnerEngineApiKey } = + await import("../agent/production-agent.js"); + const { runAgentLoopDirectWithSoftTimeout } = + await import("../agent/run-loop-with-resume.js"); + const name = `recipient-denied-${deniedAt}`; + const automation = precondition(name, { + runAs: "shared", + orgId: "acme", + createdBy: "alice@agent-native.test", + }); + automation.resource.owner = "__organization__:acme"; + const denied = Object.assign( + new Error("permission denied for table org_members"), + { + code: "42501", + }, + ); + const execute = rawClient.execute.getMockImplementation()!; + const queries: string[] = []; + rawClient.execute.mockImplementation(async (input) => { + if (typeof input !== "string" && /FROM org_members/.test(input.sql)) { + queries.push(input.sql); + if ( + deniedAt === "membership" || + input.sql.includes("role IN ('owner', 'admin')") + ) + throw denied; + } + return execute(input); + }); + createThreadMock.mockClear(); + vi.mocked(resolveOwnerEngineApiKey).mockClear(); + vi.mocked(runAgentLoopDirectWithSoftTimeout).mockClear(); + const errors = vi.spyOn(console, "error").mockImplementation(() => {}); + try { + const result = await runBackgroundAutomation( + runOptions(automation, { + ownerEmail: "__organization__:acme", + orgId: "acme", + }), + { ...standardDeps, appId: "calendar" }, + ).then( + () => ({ error: undefined }), + (error: unknown) => ({ error }), + ); + const history = await pglite + .prepare( + `SELECT owner, scope, org_id, app_id, path, status, error, error_code, + notification_email, run_id, thread_id, started_at, finished_at + FROM automation_runs WHERE automation = ?`, + ) + .all(name); + expect(history).toHaveLength(1); + const errorCode = + deniedAt === "membership" + ? "owner_unverifiable" + : "automation_notification_recipient_unavailable"; + expect(history[0]).toMatchObject({ + owner: "__organization__:acme", + scope: "organization", + org_id: "acme", + app_id: "calendar", + path: `jobs/${name}.md`, + status: "error", + error_code: errorCode, + error: expect.stringContaining( + "permission denied for table org_members", + ), + notification_email: null, + run_id: null, + thread_id: null, + started_at: expect.any(Number), + finished_at: expect.any(Number), + }); + expect(Number(history[0].finished_at)).toBeGreaterThanOrEqual( + Number(history[0].started_at), + ); + expect(result.error).toMatchObject({ errorCode, cause: denied }); + expect( + errors.mock.calls.some((call) => call.includes(result.error)), + ).toBe(true); + expect(createThreadMock).not.toHaveBeenCalled(); + expect(resolveOwnerEngineApiKey).not.toHaveBeenCalled(); + expect(runAgentLoopDirectWithSoftTimeout).not.toHaveBeenCalled(); + await expect(countRowsWithPrefix(`job-${name}`)).resolves.toBe(0); + if (deniedAt === "membership") { + expect(queries).toHaveLength(1); + } + } finally { + rawClient.execute.mockImplementation(execute); + errors.mockRestore(); + } + }, + ); + it("alerts an org owner, not a creator who has left the organization", async () => { await seedOrg([["bob@agent-native.test", "owner"]]); expect(await orgJobAlertRecipient("org-creator-left")).toBe( diff --git a/packages/core/src/jobs/background-automation-runner.ts b/packages/core/src/jobs/background-automation-runner.ts index 7bf22001192..cd218638691 100644 --- a/packages/core/src/jobs/background-automation-runner.ts +++ b/packages/core/src/jobs/background-automation-runner.ts @@ -154,8 +154,13 @@ export const BACKGROUND_RUN_HARD_TIMEOUT_MS = 10 * 60_000; export class BackgroundAutomationRunError extends Error { readonly errorCode: string; readonly deliveryNote?: string; - constructor(message: string, errorCode: string, deliveryNote?: string) { - super(message); + constructor( + message: string, + errorCode: string, + deliveryNote?: string, + options?: ErrorOptions, + ) { + super(message, options); this.name = "BackgroundAutomationRunError"; this.errorCode = errorCode; this.deliveryNote = deliveryNote; @@ -242,7 +247,12 @@ export type AutomationIdentityFailureCode = export type AutomationIdentityValidation = | { ok: true } - | { ok: false; reason: string; code: AutomationIdentityFailureCode }; + | { + ok: false; + reason: string; + code: AutomationIdentityFailureCode; + cause?: unknown; + }; export type BackgroundAutomationIdentityResult = | { ok: true; identity: AutomationExecutionIdentity } @@ -289,6 +299,7 @@ export async function validateAutomationRunIdentity( ok: false, reason: `could not verify user "${ownerEmail}" for this run`, code: "owner_unverifiable", + cause: error, }; } @@ -302,13 +313,6 @@ export async function validateAutomationRunIdentity( LIMIT 1`, args: [orgId, ownerEmail], }); - if (memberRows === null) { - return { - ok: false, - reason: `could not verify membership in org "${orgId}"`, - code: "owner_unverifiable", - }; - } if (memberRows.length === 0) { return { ok: false, @@ -317,12 +321,13 @@ export async function validateAutomationRunIdentity( }; } return { ok: true }; - } catch { + } catch (cause) { // coercion-ok: reported as `owner_unverifiable`, which the scheduler retries instead of treating the owner as gone. return { ok: false, reason: `could not verify membership in org "${orgId}"`, code: "owner_unverifiable", + cause, }; } } @@ -542,12 +547,17 @@ async function notificationEmailFor( return owner; } const creator = createdBy?.trim(); - if ( - creator?.includes("@") && - automationOwnerKind(creator) === "user" && - (await validateAutomationRunIdentity(creator, orgId)).ok - ) { - return creator; + if (creator?.includes("@") && automationOwnerKind(creator) === "user") { + const validity = await validateAutomationRunIdentity(creator, orgId); + if (validity.ok) return creator; + if (validity.code === "owner_unverifiable") { + throw new BackgroundAutomationRunError( + `${validity.reason}${validity.cause instanceof Error ? `: ${validity.cause.message}` : ""}`, + validity.code, + undefined, + { cause: validity.cause }, + ); + } } const admin = orgId ? await orgAdminEmail(orgId) : undefined; if (!admin) { @@ -567,12 +577,6 @@ async function orgAdminEmail(orgId: string): Promise { LIMIT 1`, args: [orgId], }); - if (rows === null) { - console.error( - `[automations] automation_alert_no_recipient: could not read the owners of org "${orgId}".`, - ); - return undefined; - } const email = rows[0]?.email; return typeof email === "string" && email ? email : undefined; } @@ -589,21 +593,55 @@ export async function startBackgroundAutomationHistory( appId?: string, options?: StartAutomationRunOptions, ): Promise { - return startAutomationRun( + let notificationEmail: string | undefined; + let lookupFailure: BackgroundAutomationRunError | undefined; + try { + notificationEmail = await notificationEmailFor( + automation.name, + ownerEmail, + automation.meta.createdBy, + orgId, + ); + } catch (cause) { + lookupFailure = + cause instanceof BackgroundAutomationRunError + ? cause + : new BackgroundAutomationRunError( + `Could not resolve the alert recipient for automation "${automation.name}"${orgId ? ` in org "${orgId}"` : ""}: ${cause instanceof Error ? cause.message : String(cause)}`, + "automation_notification_recipient_unavailable", + undefined, + { cause }, + ); + } + const historyId = await startAutomationRun( { ...automationRunOwnership(automation.resource.owner, ownerEmail, orgId), automation: automation.name, path: automation.resource.path, appId, - notificationEmail: await notificationEmailFor( - automation.name, - ownerEmail, - automation.meta.createdBy, - orgId, - ), + notificationEmail, }, options, ); + if (lookupFailure) { + await finishAutomationRun( + historyId, + "error", + lookupFailure.message, + lookupFailure.errorCode, + { + notify: false, + requirePersisted: true, + expectedRunId: null, + }, + ); + console.error( + `[automations] Could not verify the alert recipient for "${automation.name}" (history "${historyId}", org "${orgId ?? "none"}"):`, + lookupFailure, + ); + throw lookupFailure; + } + return historyId; } export async function runBackgroundAutomation( @@ -623,6 +661,7 @@ export async function runBackgroundAutomation( deps.appId, ); } catch (err) { + if (err instanceof BackgroundAutomationRunError) throw err; console.error( `[automations] Could not open a history record for "${automation.name}"; running anyway:`, err, diff --git a/packages/core/src/mcp/build-server.ts b/packages/core/src/mcp/build-server.ts index ce4ab533548..afdd1d555c6 100644 --- a/packages/core/src/mcp/build-server.ts +++ b/packages/core/src/mcp/build-server.ts @@ -74,6 +74,7 @@ import { type McpDirectoryWidgetReadArgument, renewMcpDirectoryWidgetCapabilityScope, } from "../shared/embed-auth.js"; +import { ForbiddenError } from "../sharing/errors.js"; import { type McpAnalyticsContext, describeMcpError, @@ -4473,7 +4474,10 @@ export async function resolveMcpIdentityOrgId( try { const { resolveOrgIdForEmail } = await import("../org/context.js"); return (await resolveOrgIdForEmail(userEmail)) ?? undefined; - } catch { - return undefined; + } catch (cause) { + if (cause instanceof ForbiddenError) { + throw new McpIdentityVerificationRefusedError(); + } + throw new McpIdentityVerificationUnavailableError(cause); } } diff --git a/packages/core/src/mcp/build-server.verify-auth.spec.ts b/packages/core/src/mcp/build-server.verify-auth.spec.ts index d88668912d9..f61b523492e 100644 --- a/packages/core/src/mcp/build-server.verify-auth.spec.ts +++ b/packages/core/src/mcp/build-server.verify-auth.spec.ts @@ -1668,6 +1668,8 @@ describe("verifyAuth — a credential for an address an email change retired", ( }); describe("resolveMcpIdentityOrgId", () => { + afterEach(() => vi.restoreAllMocks()); + beforeEach(() => { vi.clearAllMocks(); lookupOrgByDomainMock.mockResolvedValue({ status: "not-found" }); @@ -1767,4 +1769,45 @@ describe("resolveMcpIdentityOrgId", () => { expect(resolveOrgIdForEmailMock).toHaveBeenCalledWith("alice@example.com"); }); + + it.each([ + ['relation "org_members" does not exist', "42P01"], + ["permission denied for table org_members", "42501"], + ["membership lookup failed", undefined], + ] as const)( + "keeps real email membership failures unavailable: %s", + async (message, code) => { + const org = + await vi.importActual( + "../org/context.js", + ); + resolveOrgIdForEmailMock.mockImplementation(org.resolveOrgIdForEmail); + const db = await import("../db/client.js"); + const execute = vi + .fn() + .mockRejectedValue(Object.assign(new Error(message), { code })); + vi.spyOn(db, "getDbExec").mockReturnValue({ execute } as any); + + await expect( + resolveMcpIdentityOrgId({ userEmail: "alice@example.test" }), + ).rejects.toBeInstanceOf(McpIdentityVerificationUnavailableError); + expect(execute).toHaveBeenCalledOnce(); + }, + ); + + it("preserves Personal after a successful real membership read without rows", async () => { + const org = + await vi.importActual( + "../org/context.js", + ); + resolveOrgIdForEmailMock.mockImplementation(org.resolveOrgIdForEmail); + const db = await import("../db/client.js"); + const execute = vi.fn().mockResolvedValue({ rows: [] }); + vi.spyOn(db, "getDbExec").mockReturnValue({ execute } as any); + + await expect( + resolveMcpIdentityOrgId({ userEmail: "alice@example.test" }), + ).resolves.toBeUndefined(); + expect(execute).toHaveBeenCalledOnce(); + }); }); diff --git a/packages/core/src/mcp/connector-catalog.spec.ts b/packages/core/src/mcp/connector-catalog.spec.ts index 0f4401141db..9fe2ec88f90 100644 --- a/packages/core/src/mcp/connector-catalog.spec.ts +++ b/packages/core/src/mcp/connector-catalog.spec.ts @@ -13,6 +13,10 @@ const issuedConnectTokens = vi.hoisted( >(), ); +const resolveOrgIdForEmail = vi.hoisted(() => + vi.fn(async (_email: string): Promise => null), +); + vi.mock("./builtin-tools.js", () => ({ getBuiltinCrossAppTools: () => ({ list_apps: { @@ -79,6 +83,7 @@ vi.mock("./builtin-tools.js", () => ({ vi.mock("../org/context.js", () => ({ resolveOrgByDomain: vi.fn(async () => null), getA2ASecretByDomain: vi.fn(async () => null), + resolveOrgIdForEmail, })); vi.mock("../server/better-auth-instance.js", async (importOriginal) => ({ @@ -102,6 +107,7 @@ vi.mock("./connect-store.js", () => ({ beforeEach(() => { issuedConnectTokens.clear(); + resolveOrgIdForEmail.mockReset().mockResolvedValue(null); }); vi.mock("../server/embed-session.js", () => ({ @@ -980,10 +986,40 @@ describe("connector-catalog tier", () => { const names: string[] = out.result.tools.map((t: any) => t.name); expect(names).toContain("db-exec"); expect(names).toContain("seed-kitchen-sink"); + expect(resolveOrgIdForEmail).toHaveBeenCalledExactlyOnceWith( + "alice@example.com", + ); } finally { delete process.env.BETTER_AUTH_SECRET; } }); + + it("keeps a full-catalog OAuth scope lookup failure retryable", async () => { + process.env.BETTER_AUTH_SECRET = OAUTH_SECRET; + resolveOrgIdForEmail.mockRejectedValueOnce( + Object.assign(new Error("permission denied for table org_members"), { + code: "42501", + }), + ); + const token = await signOAuthToken({ + catalogScope: "full", + clientId: "catalog-test-client", + }); + const event = makeEvent({ + headers: { authorization: `Bearer ${token}` }, + body: { jsonrpc: "2.0", id: 8, method: "tools/list", params: {} }, + }); + + const result = await handleMcpRequest(event, connectorConfig as any); + + expect(result).toMatchObject({ error: "Service Unavailable" }); + expect(event._status).toBe(503); + expect(event._responseHeaders?.["retry-after"]).toBe("5"); + expect(event._responseHeaders?.["www-authenticate"]).toBeUndefined(); + expect(resolveOrgIdForEmail).toHaveBeenCalledExactlyOnceWith( + "alice@example.com", + ); + }); }); describe("AGENT_NATIVE_MCP_FULL_CATALOG=1 overrides connector tier", () => { diff --git a/packages/core/src/mcp/server.spec.ts b/packages/core/src/mcp/server.spec.ts index 9093db8895e..032b007e83c 100644 --- a/packages/core/src/mcp/server.spec.ts +++ b/packages/core/src/mcp/server.spec.ts @@ -24,6 +24,7 @@ import { listResourceSuggestions } from "../review/suggestions/actions.js"; import { loadActionsFromStaticRegistry } from "../server/action-discovery.js"; import { registerErrorCaptureProvider } from "../server/capture-error.js"; import { filesystemActionEntry } from "../server/filesystem-action-entry.js"; +import { runWithRequestContext } from "../server/request-context.js"; import { isMcpDirectoryWidgetReadCapabilityScope, isMcpDirectoryWidgetWriteCapabilityScope, @@ -10195,6 +10196,135 @@ describe("handleMcpRequest — web-standard runtime fallback (no Node req/res)", expect(result).toMatchObject({ error: "Service Unavailable" }); }); + it.each([ + "revoked", + "email-mismatch", + "missing-org", + "valid", + "unavailable", + ] as const)( + "preserves delegated organization resolution over HTTP: %s", + async (scope) => { + process.env.BETTER_AUTH_SECRET = + "oauth-secret-at-least-32-characters-long"; + const { signMcpOAuthAccessToken } = await import("./oauth-token.js"); + const token = await signMcpOAuthAccessToken({ + ownerEmail: "delegate@example.test", + clientId: "client-123", + scope: "mcp:read", + resource: "https://mail.agent-native.com/mcp", + issuer: "https://mail.agent-native.com", + }); + const org = + await vi.importActual( + "../org/context.js", + ); + resolveOrgIdForEmailMock.mockImplementation(org.resolveOrgIdForEmail); + const db = await import("../db/client.js"); + const realDb = db.getDbExec(); + const dbSpy = vi.spyOn(db, "getDbExec").mockReturnValue({ + execute: async (query: { sql: string; args: unknown[] }) => { + if (!query.sql.includes("SELECT role FROM org_members")) + return realDb.execute(query); + if (scope === "unavailable") + throw Object.assign(new Error("fake membership database failure"), { + code: "42501", + }); + return { rows: scope === "revoked" ? [] : [{ role: "member" }] }; + }, + } as any); + const event = makeWebEvent({ + method: "POST", + body: { jsonrpc: "2.0", id: 15, method: "tools/list", params: {} }, + headers: { authorization: `Bearer ${token}` }, + }); + try { + const result = await runWithRequestContext( + { + userEmail: + scope === "email-mismatch" + ? "other@example.test" + : "delegate@example.test", + orgId: scope === "missing-org" ? undefined : "delegated-org", + a2aIdentityAssurance: "org-member", + }, + () => handleMcpRequest(event, config as any), + ); + expect(result instanceof Response ? result.status : event._status).toBe( + scope === "valid" ? 200 : scope === "unavailable" ? 503 : 401, + ); + if (scope === "unavailable") { + expect(event._responseHeaders?.["retry-after"]).toBe("5"); + expect(event._responseHeaders?.["www-authenticate"]).toBeUndefined(); + expect(result).toMatchObject({ error: "Service Unavailable" }); + } else if (scope !== "valid") { + expect(event._responseHeaders?.["retry-after"]).toBeUndefined(); + expect(event._responseHeaders?.["www-authenticate"]).toContain( + 'error="invalid_token"', + ); + expect(result).toMatchObject({ reason: "invalid" }); + } + } finally { + dbSpy.mockRestore(); + } + }, + ); + + it.each(["unavailable", "personal"] as const)( + "preserves the email-only MCP scope result over HTTP: %s", + async (scope) => { + process.env.BETTER_AUTH_SECRET = + "oauth-secret-at-least-32-characters-long"; + const { signMcpOAuthAccessToken } = await import("./oauth-token.js"); + const token = await signMcpOAuthAccessToken({ + ownerEmail: "email-only@example.test", + clientId: "client-123", + scope: "mcp:read", + resource: "https://mail.agent-native.com/mcp", + issuer: "https://mail.agent-native.com", + }); + const org = + await vi.importActual( + "../org/context.js", + ); + resolveOrgIdForEmailMock.mockImplementation(org.resolveOrgIdForEmail); + const db = await import("../db/client.js"); + const realDb = db.getDbExec(); + const execute = vi.fn(async (query: { sql: string; args: unknown[] }) => { + if (!query.sql.includes("SELECT org_id FROM org_members")) + return realDb.execute(query); + if (scope === "unavailable") + throw Object.assign( + new Error("permission denied for table org_members"), + { code: "42501" }, + ); + return { rows: [] }; + }); + const dbSpy = vi + .spyOn(db, "getDbExec") + .mockReturnValue({ execute } as any); + const event = makeWebEvent({ + method: "POST", + body: { jsonrpc: "2.0", id: 15, method: "tools/list", params: {} }, + headers: { authorization: `Bearer ${token}` }, + }); + try { + const result = await handleMcpRequest(event, config as any); + expect(result instanceof Response ? result.status : event._status).toBe( + scope === "unavailable" ? 503 : 200, + ); + expect(event._responseHeaders?.["www-authenticate"]).toBeUndefined(); + if (scope === "unavailable") { + expect(event._responseHeaders?.["retry-after"]).toBe("5"); + expect(result).toMatchObject({ error: "Service Unavailable" }); + } + expect(resolveOrgIdForEmailMock).toHaveBeenCalledOnce(); + } finally { + dbSpy.mockRestore(); + } + }, + ); + it.each([ ["unknown", { status: "not-found" }], ["ambiguous", { status: "ambiguous" }], diff --git a/packages/core/src/mcp/server.ts b/packages/core/src/mcp/server.ts index dd70032e7c8..ff45e58d776 100644 --- a/packages/core/src/mcp/server.ts +++ b/packages/core/src/mcp/server.ts @@ -423,14 +423,16 @@ async function handleMcpRequestInternal( } if ( - authResult.identity?.orgId === undefined && - authResult.identity?.orgDomain !== undefined + authResult.identity && + authResult.identity.orgId === undefined && + (authResult.identity.orgDomain !== undefined || + Boolean(authResult.identity.userEmail)) ) { try { const orgId = await resolveMcpIdentityOrgId(authResult.identity); authResult = { ...authResult, - identity: { ...authResult.identity, orgId }, + identity: { ...authResult.identity, orgId: orgId ?? null }, }; } catch (error) { if (error instanceof McpIdentityVerificationUnavailableError) { diff --git a/packages/core/src/org/context.spec.ts b/packages/core/src/org/context.spec.ts index 945ac873ec8..85da1953702 100644 --- a/packages/core/src/org/context.spec.ts +++ b/packages/core/src/org/context.spec.ts @@ -30,6 +30,7 @@ vi.mock("../application-state/store.js", () => ({ import { markVerifiedServiceIdentityForEvent } from "../server/request-context.js"; import { runWithRequestContext } from "../server/request-context.js"; +import { ForbiddenError } from "../sharing/access.js"; import { setActiveOrgId } from "./active-org.js"; import { __resetDomainMatchCacheForTests } from "./auto-join-domain.js"; import { @@ -107,17 +108,13 @@ describe("getOrgContext", () => { expect(mockExecute).not.toHaveBeenCalled(); }); - it("marks a swallowed membership read failure as unavailable org scope", async () => { + it("rejects a missing membership table instead of marking Personal scope", async () => { mockGetSession.mockResolvedValue({ email: "a@b.com" }); - mockExecute.mockRejectedValueOnce( - new Error('relation "org_members" does not exist'), - ); + const cause = new Error('relation "org_members" does not exist'); + mockExecute.mockRejectedValueOnce(cause); - await expect(getOrgContext(EVENT)).resolves.toMatchObject({ - email: "a@b.com", - orgId: null, - }); - expect(hasOrgScopeResolutionUnavailable(EVENT)).toBe(true); + await expect(getOrgContext(EVENT)).rejects.toBe(cause); + expect(hasOrgScopeResolutionUnavailable(EVENT)).toBe(false); }); it("resolves a verified service identity for an existing unlinked org", async () => { @@ -270,7 +267,7 @@ describe("getOrgContext", () => { orgRole: "owner", }); - const ctx = await runWithRequestContext( + const lookup = runWithRequestContext( { a2aIdentityAssurance: "org-member", userEmail: "owner@example.test", @@ -279,12 +276,7 @@ describe("getOrgContext", () => { () => getOrgContext(EVENT), ); - expect(ctx).toEqual({ - email: "other@example.test", - orgId: null, - orgName: null, - role: null, - }); + await expect(lookup).rejects.toBeInstanceOf(ForbiddenError); expect(mockExecute).not.toHaveBeenCalled(); }); @@ -721,17 +713,27 @@ describe("getOrgContext", () => { }); describe("membership-lookup failure (tables missing before migrations)", () => { - it("returns the session orgId when present", async () => { + it("rejects the lookup even when a session orgId is present", async () => { + mockGetSession.mockResolvedValue({ + email: "a@b.com", + orgId: "sess-org", + orgRole: "admin", + }); + const cause = new Error('relation "org_members" does not exist'); + mockExecute.mockRejectedValueOnce(cause); + await expect(getOrgContext(EVENT)).rejects.toBe(cause); + }); + + it("preserves a claimed session org during a transient lookup failure", async () => { mockGetSession.mockResolvedValue({ email: "a@b.com", orgId: "sess-org", orgRole: "admin", }); mockExecute.mockRejectedValueOnce( - new Error('relation "org_members" does not exist'), + Object.assign(new Error("db query timed out"), { code: "57014" }), ); - const ctx = await getOrgContext(EVENT); - expect(ctx).toEqual({ + await expect(getOrgContext(EVENT)).resolves.toEqual({ email: "a@b.com", orgId: "sess-org", orgName: null, @@ -739,18 +741,11 @@ describe("getOrgContext", () => { }); }); - it("returns a null-org context when there is no session orgId", async () => { + it("rejects the lookup when there is no session orgId", async () => { mockGetSession.mockResolvedValue({ email: "a@b.com" }); - mockExecute.mockRejectedValueOnce( - new Error('relation "org_members" does not exist'), - ); - const ctx = await getOrgContext(EVENT); - expect(ctx).toEqual({ - email: "a@b.com", - orgId: null, - orgName: null, - role: null, - }); + const cause = new Error('relation "org_members" does not exist'); + mockExecute.mockRejectedValueOnce(cause); + await expect(getOrgContext(EVENT)).rejects.toBe(cause); }); }); @@ -1235,6 +1230,88 @@ describe("getOrgContext", () => { }); }); +describe("resolveOrgIdForEmailViaEvent membership", () => { + beforeEach(() => { + vi.clearAllMocks(); + mockExecute.mockResolvedValue({ rows: [] }); + mockGetUserSetting.mockResolvedValue(null); + }); + + const resolveDelegated = (event: ReturnType) => + runWithRequestContext( + { + userEmail: "delegate@example.test", + orgId: "delegated-org", + a2aIdentityAssurance: "org-member", + }, + () => resolveOrgIdForEmailViaEvent(event, "delegate@example.test"), + ); + + it.each(["42501", "08006"])( + "rejects unavailable delegated membership (%s) instead of returning Personal", + async (code) => { + const cause = Object.assign(new Error("membership read failed"), { + code, + }); + mockExecute.mockRejectedValueOnce(cause); + const lookup = resolveDelegated(makeEvent()); + await expect(lookup).rejects.toThrow(); + await expect(lookup).rejects.not.toBeInstanceOf(ForbiddenError); + await expect(lookup).rejects.toBe(cause); + }, + ); + + it("preserves definitive refusal when membership is absent", async () => { + await expect(resolveDelegated(makeEvent())).rejects.toBeInstanceOf( + ForbiddenError, + ); + }); + + it("returns a healthy delegated organization", async () => { + queueSelect([ + { orgId: "delegated-org", role: "member", orgName: "Example" }, + ]); + await expect(resolveDelegated(makeEvent())).resolves.toBe("delegated-org"); + }); + + it.each(["42501", "08006", "42P01"])( + "rejects unavailable ordinary membership (%s) without caching Personal scope", + async (code) => { + const cause = Object.assign(new Error("membership read failed"), { + code, + }); + mockExecute.mockRejectedValueOnce(cause); + const event = makeEvent(); + const resolve = () => + runWithRequestContext({ userEmail: "member@example.test" }, () => + resolveOrgIdForEmailViaEvent(event, "member@example.test"), + ); + await expect(resolve()).rejects.toBe(cause); + queueSelect([ + { orgId: "ordinary-org", role: "member", orgName: "Example" }, + ]); + await expect(resolve()).resolves.toBe("ordinary-org"); + }, + ); + + it("returns Personal when the ordinary membership read succeeds without rows", async () => { + await expect( + resolveOrgIdForEmailViaEvent(makeEvent(), "personal@example.test"), + ).resolves.toBeNull(); + }); + + it("keeps an explicit Personal scope org-less without a membership read", async () => { + await expect( + runWithRequestContext( + { userEmail: "personal@example.test", orgScope: "personal" }, + () => + resolveOrgIdForEmailViaEvent(makeEvent(), "personal@example.test"), + ), + ).resolves.toBeNull(); + expect(mockExecute).not.toHaveBeenCalled(); + }); +}); + describe("resolveOrgIdForEmail", () => { beforeEach(() => { vi.clearAllMocks(); @@ -1247,6 +1324,72 @@ describe("resolveOrgIdForEmail", () => { expect(await resolveOrgIdForEmail("nobody@b.com")).toBeNull(); }); + it.each(["missing-member", "email-mismatch", "missing-org"])( + "refuses delegated %s instead of reporting unavailable or Personal scope", + async (scenario) => { + const lookup = runWithRequestContext( + { + userEmail: "delegate@example.test", + orgId: scenario === "missing-org" ? undefined : "delegated-org", + a2aIdentityAssurance: "org-member", + }, + () => + resolveOrgIdForEmail( + scenario === "email-mismatch" + ? "other@example.test" + : "delegate@example.test", + ), + ); + await expect(lookup).rejects.toBeInstanceOf(ForbiddenError); + await expect(lookup).rejects.toMatchObject({ statusCode: 403 }); + }, + ); + + it("refuses a revoked delegation at the scope result boundary", async () => { + await expect( + runWithRequestContext( + { + userEmail: "delegate@example.test", + orgId: "delegated-org", + a2aIdentityAssurance: "org-member", + }, + () => resolveOrgScopeForEmail("delegate@example.test"), + ), + ).rejects.toBeInstanceOf(ForbiddenError); + }); + + it("keeps a valid delegation pinned to its organization", async () => { + queueSelect([{ role: "owner" }]); + await expect( + runWithRequestContext( + { + userEmail: "delegate@example.test", + orgId: "delegated-org", + a2aIdentityAssurance: "org-member", + }, + () => resolveOrgIdForEmail("delegate@example.test"), + ), + ).resolves.toBe("delegated-org"); + }); + + it("preserves the actual database failure while checking delegation", async () => { + const cause = Object.assign( + new Error("permission denied for table org_members"), + { code: "42501" }, + ); + mockExecute.mockRejectedValueOnce(cause); + await expect( + runWithRequestContext( + { + userEmail: "delegate@example.test", + orgId: "delegated-org", + a2aIdentityAssurance: "org-member", + }, + () => resolveOrgIdForEmail("delegate@example.test"), + ), + ).rejects.toBe(cause); + }); + it("confirms personal scope when the membership query succeeds without rows", async () => { queueSelect([]); await expect(resolveOrgScopeForEmail("nobody@b.com")).resolves.toEqual({ @@ -1255,11 +1398,11 @@ describe("resolveOrgIdForEmail", () => { }); it("keeps scope unknown when the membership query is unavailable", async () => { - mockExecute.mockRejectedValueOnce( - new Error('relation "org_members" does not exist'), - ); + const cause = new Error('relation "org_members" does not exist'); + mockExecute.mockRejectedValueOnce(cause); await expect(resolveOrgScopeForEmail("nobody@b.com")).resolves.toEqual({ status: "unknown", + cause, }); }); @@ -1303,11 +1446,14 @@ describe("resolveOrgIdForEmail", () => { expect(mockExecute.mock.calls[0][0].args).toEqual(["mixed@case.com"]); }); - it("returns null on a DB error (missing tables) rather than throwing", async () => { - mockExecute.mockRejectedValueOnce( - new Error('relation "org_members" does not exist'), - ); - expect(await resolveOrgIdForEmail("a@b.com")).toBeNull(); + it.each([ + new Error('relation "org_members" does not exist'), + Object.assign(new Error("permission denied for table org_members"), { + code: "42501", + }), + ])("throws when organization scope is unavailable: %s", async (error) => { + mockExecute.mockRejectedValueOnce(error); + await expect(resolveOrgIdForEmail("a@b.com")).rejects.toBe(error); }); it("throws when org_members is unreadable, so callers cannot mistake it for 'no org'", async () => { diff --git a/packages/core/src/org/context.ts b/packages/core/src/org/context.ts index 7163847b379..73643aaf885 100644 --- a/packages/core/src/org/context.ts +++ b/packages/core/src/org/context.ts @@ -21,6 +21,7 @@ import { import { getSetting } from "../settings/store.js"; import { getUserSetting } from "../settings/user-settings.js"; import { FIRST_RUN_ONBOARDING_ELIGIBLE_KEY } from "../shared/first-run-onboarding.js"; +import { ForbiddenError } from "../sharing/errors.js"; import { setActiveOrgId } from "./active-org.js"; import { autoJoinDomainMatchingOrgs } from "./auto-join-domain.js"; import { isFreeEmailProvider } from "./free-email-providers.js"; @@ -63,14 +64,9 @@ function markOrgScopeResolutionUnavailable(event: H3Event): void { export async function queryOrgMembers(query: { sql: string; args: unknown[]; -}): Promise[] | null> { - try { - const { rows } = await getDbExec().execute(query); - return rows as Record[]; - } catch (err) { - if (isTransientDatabaseError(err)) throw err; - return null; - } +}): Promise[]> { + const { rows } = await getDbExec().execute(query); + return rows as Record[]; } function normalizeOrgRole(value: unknown): OrgRole | null { @@ -79,9 +75,7 @@ function normalizeOrgRole(value: unknown): OrgRole | null { : null; } -function delegatedOrgMemberScope( - email?: string | null, -): string | null | undefined { +function delegatedOrgMemberScope(email?: string | null): string | undefined { const requestContext = getRequestContext(); if (requestContext?.a2aIdentityAssurance !== "org-member") return undefined; const requestEmail = requestContext.userEmail?.trim().toLowerCase(); @@ -91,7 +85,7 @@ function delegatedOrgMemberScope( !requestOrgId || (email && email.trim().toLowerCase() !== requestEmail) ) { - return null; + throw new ForbiddenError(); } return requestOrgId; } @@ -102,12 +96,13 @@ function scopeMembershipsForDelegatedIdentity( ): MembershipRow[] | null { const scopedOrgId = delegatedOrgMemberScope(email); if (scopedOrgId === undefined || memberships === null) return memberships; - if (!scopedOrgId) return []; - return memberships.flatMap((membership) => { + const scopedMemberships = memberships.flatMap((membership) => { if (membership.orgId !== scopedOrgId) return []; const role = effectiveOrgMemberRole(membership.role, scopedOrgId, email); return role ? [{ ...membership, role }] : []; }); + if (scopedMemberships.length === 0) throw new ForbiddenError(); + return scopedMemberships; } function emailDomainOf(email: string): string | null { @@ -280,7 +275,6 @@ function loadMembershipsForEvent( email: string, ): Promise { const scopedOrgId = delegatedOrgMemberScope(email); - if (scopedOrgId === null) return Promise.resolve([]); const ctx = event.context as Record; const cache = ((ctx[MEMBERSHIPS_CACHE_KEY] as | Map> @@ -326,9 +320,6 @@ async function resolveOrgContextUncached(event: H3Event): Promise { const isDelegatedOrgMember = requestContext?.a2aIdentityAssurance === "org-member"; const delegatedOrgId = delegatedOrgMemberScope(email); - if (isDelegatedOrgMember && !delegatedOrgId) { - return { email, orgId: null, orgName: null, role: null }; - } if (isAnonymousWaitlistSessionEmail(email)) { return { email, orgId: null, orgName: null, role: null }; } @@ -462,6 +453,7 @@ async function resolveOrgContextUncached(event: H3Event): Promise { selectedOrgId && !memberships.some((membership) => membership.orgId === selectedOrgId) ) { + if (isDelegatedOrgMember) throw new ForbiddenError(); return { email, orgId: null, orgName: null, role: null }; } if (selectedOrgId && memberships.some((m) => m.orgId === selectedOrgId)) { @@ -682,34 +674,36 @@ async function loadMembershipsUncached( export type OrgScopeResolution = | { status: "organization"; orgId: string } | { status: "personal" } - | { status: "unknown" }; + | { status: "unknown"; cause?: unknown }; export async function resolveOrgScopeForEmail( email: string, ): Promise { const delegatedOrgId = delegatedOrgMemberScope(email); if (delegatedOrgId !== undefined) { - if (!delegatedOrgId) return { status: "unknown" }; const role = await readOrgMemberRole(delegatedOrgId, email); - return role - ? { status: "organization", orgId: delegatedOrgId } - : { status: "unknown" }; + if (!role) throw new ForbiddenError(); + return { status: "organization", orgId: delegatedOrgId }; } if (getRequestContext()?.orgScope === "personal") { return { status: "personal" }; } - const idsPromise = requestMemberOrgIds(email, async () => { - const rows = await queryOrgMembers({ - sql: `SELECT org_id FROM org_members + let ids: string[]; + try { + ids = await requestMemberOrgIds(email, async () => { + const { rows } = await getDbExec().execute({ + sql: `SELECT org_id FROM org_members WHERE LOWER(email) = ? AND federation_removal_pending_at IS NULL ${MEMBERSHIP_FALLBACK_ORDER_BY}`, - args: [email.toLowerCase()], + args: [email.toLowerCase()], + }); + return rows.map((r: any) => String(r.org_id)); }); - return rows?.map((r: any) => String(r.org_id)) ?? null; - }); - const ids = await idsPromise; - if (ids === null) return { status: "unknown" }; + } catch (cause) { + if (isTransientDatabaseError(cause)) throw cause; + return { status: "unknown", cause }; + } if (ids.length === 0) return { status: "personal" }; const activeOrgSetting = (await getUserSetting( @@ -727,6 +721,10 @@ export async function resolveOrgIdForEmail( email: string, ): Promise { const resolution = await resolveOrgScopeForEmail(email); + if (resolution.status === "unknown") { + if (resolution.cause !== undefined) throw resolution.cause; + throw new Error("Organization scope is unavailable"); + } return resolution.status === "organization" ? resolution.orgId : null; } @@ -738,9 +736,9 @@ export async function resolveActiveOrgIdForStorageCredential( const delegatedOrgId = delegatedOrgMemberScope(normalizedEmail); if (delegatedOrgId !== undefined) { - if (!delegatedOrgId) return null; const role = await readOrgMemberRole(delegatedOrgId, normalizedEmail); - return role ? delegatedOrgId : null; + if (!role) throw new ForbiddenError(); + return delegatedOrgId; } const { rows } = await getDbExec().execute({ @@ -773,10 +771,9 @@ export async function resolveOrgIdForEmailViaEvent( ): Promise { const delegatedOrgId = delegatedOrgMemberScope(email); if (delegatedOrgId !== undefined) { - if (!delegatedOrgId) return null; const memberships = await loadMembershipsForEvent(event, email); - if (!memberships || !memberships.some((m) => m.orgId === delegatedOrgId)) { - return null; + if (memberships === null) { + throw new Error("Organization scope is unavailable"); } const refreshed = await refreshFederatedMemberships( event, @@ -787,9 +784,10 @@ export async function resolveOrgIdForEmailViaEvent( if (refreshed !== memberships) { updateMembershipsForEvent(event, email, refreshed); } - return refreshed.some((m) => m.orgId === delegatedOrgId) - ? delegatedOrgId - : null; + if (!refreshed.some((m) => m.orgId === delegatedOrgId)) { + throw new ForbiddenError(); + } + return delegatedOrgId; } if (getRequestContext()?.orgScope === "personal") return null; const settingPromise = loadActiveOrgSettingForEvent(event, email); diff --git a/packages/core/src/org/request-org-cache.spec.ts b/packages/core/src/org/request-org-cache.spec.ts index 0cfef6f898e..8dda800309f 100644 --- a/packages/core/src/org/request-org-cache.spec.ts +++ b/packages/core/src/org/request-org-cache.spec.ts @@ -131,25 +131,28 @@ describe("per-request org membership memo", () => { expect(results).toEqual(["org1", "org2"]); }); - it("evicts a transient failure instead of memoizing it", async () => { - mockExecute.mockRejectedValueOnce( - Object.assign(new Error("connection failure"), { code: "08006" }), - ); + it.each(["08006", "42501", undefined])( + "evicts a failed read instead of memoizing it: %s", + async (code) => { + mockExecute.mockRejectedValueOnce( + Object.assign(new Error("membership lookup failed"), { code }), + ); - const result = await runWithRequestContext( - { userEmail: "alice@builder.io" }, - async () => { - await expect( - resolveOrgIdForEmail("alice@builder.io"), - ).rejects.toThrow(); - mockExecute.mockResolvedValue({ rows: [{ org_id: "org1" }] }); - return resolveOrgIdForEmail("alice@builder.io"); - }, - ); + const result = await runWithRequestContext( + { userEmail: "alice@builder.io" }, + async () => { + await expect( + resolveOrgIdForEmail("alice@builder.io"), + ).rejects.toThrow(); + mockExecute.mockResolvedValue({ rows: [{ org_id: "org1" }] }); + return resolveOrgIdForEmail("alice@builder.io"); + }, + ); - expect(result).toBe("org1"); - expect(memberRowQueries()).toHaveLength(2); - }); + expect(result).toBe("org1"); + expect(memberRowQueries()).toHaveLength(2); + }, + ); }); describe("cross-request membership cache", () => { diff --git a/packages/core/src/org/request-org-cache.ts b/packages/core/src/org/request-org-cache.ts index 8790c29cdad..774efe3a5c3 100644 --- a/packages/core/src/org/request-org-cache.ts +++ b/packages/core/src/org/request-org-cache.ts @@ -20,14 +20,11 @@ import { createTtlCache } from "../shared/ttl-cache.js"; * run owner or credential subject), so a context-only key would answer one * identity with another's memberships. */ -const requestOrgIds = new WeakMap< - object, - Map> ->(); +const requestOrgIds = new WeakMap>>(); function cacheForRequest( create: boolean, -): Map> | null { +): Map> | null { const ctx = getRequestContext(); if (!ctx || typeof ctx !== "object") return null; let cache = requestOrgIds.get(ctx); @@ -40,8 +37,8 @@ function cacheForRequest( export function requestMemberOrgIds( email: string, - load: () => Promise, -): Promise { + load: () => Promise, +): Promise { const cache = cacheForRequest(true); if (!cache) return load(); const key = email.trim().toLowerCase(); diff --git a/packages/core/src/scripts/agent-engines/list-agent-engines.spec.ts b/packages/core/src/scripts/agent-engines/list-agent-engines.spec.ts index 2b2233aadbe..3f02f8157f5 100644 --- a/packages/core/src/scripts/agent-engines/list-agent-engines.spec.ts +++ b/packages/core/src/scripts/agent-engines/list-agent-engines.spec.ts @@ -12,6 +12,7 @@ describe("list-agent-engines", () => { let defaultAuthority: { allowed: boolean }; beforeEach(() => { + vi.doUnmock("../../agent/engine/index.js"); vi.doUnmock("../../server/credential-provider.js"); vi.resetModules(); vi.doUnmock("../../settings/store.js"); @@ -305,6 +306,48 @@ describe("list-agent-engines", () => { state: "default", scope: "org", }); + expect(byName("ai-sdk:google")).toMatchObject({ + configuredError: expect.stringContaining("Could not read"), + }); + expect(byName("ai-sdk:google")?.configured).toBeUndefined(); + expect(byName("ai-sdk:openai")?.configured).toBe(false); + expect(byName("ai-sdk:openai")?.configuredError).toBeUndefined(); + expect(result.current).toBeNull(); + expect(result.currentError).toContain("Could not read"); + }); + + it.each(["forbidden", "unexpected"])( + "preserves a %s failure from current-engine discovery", + async (kind) => { + const { ForbiddenError } = await import("../../sharing/access.js"); + const cause = + kind === "forbidden" + ? new ForbiddenError() + : new Error("discovery failed"); + vi.doMock("../../agent/engine/index.js", async (importOriginal) => ({ + ...(await importOriginal< + typeof import("../../agent/engine/index.js") + >()), + detectEngineFromUserSecrets: async () => { + throw cause; + }, + })); + const { run } = await import("./list-agent-engines.js"); + await expect(run()).rejects.toBe(cause); + }, + ); + + it("preserves a definitive refusal during per-engine readiness checks", async () => { + const { ForbiddenError } = await import("../../sharing/access.js"); + const cause = new ForbiddenError(); + vi.doMock("../../agent/engine/index.js", async (importOriginal) => ({ + ...(await importOriginal()), + isStoredEngineUsableForRequest: async () => { + throw cause; + }, + })); + const { run } = await import("./list-agent-engines.js"); + await expect(run()).rejects.toBe(cause); }); it("does not report AGENT_ENGINE as current when its optional package is missing", async () => { diff --git a/packages/core/src/scripts/agent-engines/list-agent-engines.ts b/packages/core/src/scripts/agent-engines/list-agent-engines.ts index 05f09e90873..dfb539efa3c 100644 --- a/packages/core/src/scripts/agent-engines/list-agent-engines.ts +++ b/packages/core/src/scripts/agent-engines/list-agent-engines.ts @@ -29,12 +29,14 @@ import { getAppConfig } from "../../app-config/index.js"; import { CHATGPT_SUBSCRIPTION_LAB } from "../../labs/core-labs.js"; import { getUserLabEnabled } from "../../labs/store.js"; import { + CredentialStoreUnavailableError, prefetchSecrets, readProviderCredentialRejections, resolveSecretDetailed, type ProviderCredentialRejection, } from "../../server/credential-provider.js"; import { getRequestUserEmail } from "../../server/request-context.js"; +import { ForbiddenError } from "../../sharing/errors.js"; export const tool: ActionTool = { description: @@ -167,73 +169,94 @@ export async function run(args: Record = {}): Promise { ? (defaultSetting.value as { engine?: string; model?: string }) : null; - const storedEntry = - typeof current?.engine === "string" ? engineFor(current.engine) : undefined; - const storedUsable = - !!storedEntry && - (await isStoredEngineUsableForRequest(current, storedEntry)); - const appDefault = await getAgentAppModelDefaultForCurrentRequest(args.appId); - const appDefaultEntry = - typeof appDefault?.engine === "string" - ? engineFor(appDefault.engine) - : undefined; - const appDefaultUsable = - !!appDefault && - !!appDefaultEntry && - (await isStoredEngineUsableForRequest(appDefault, appDefaultEntry)); - const detectedFromUser = await detectEngineFromUserSecrets(); - const configuredEngine = getAppConfig().agent.engine; - const envEntry = configuredEngine ? engineFor(configuredEngine) : undefined; - const envUsable = - !!envEntry && - (await isStoredEngineUsableForRequest({ engine: envEntry.name }, envEntry)); - const envUnavailable = !!envEntry && !envUsable; - const detectedFromEnv = detectEngineFromEnv(); - const envSelectedEntry = envUsable ? envEntry : undefined; + let currentEngine: { engine: string; model: string | undefined } | null = + null; + let currentError: string | undefined; + try { + const storedEntry = + typeof current?.engine === "string" + ? engineFor(current.engine) + : undefined; + const storedUsable = + !!storedEntry && + (await isStoredEngineUsableForRequest(current, storedEntry)); + const appDefault = await getAgentAppModelDefaultForCurrentRequest( + args.appId, + ); + const appDefaultEntry = + typeof appDefault?.engine === "string" + ? engineFor(appDefault.engine) + : undefined; + const appDefaultUsable = + !!appDefault && + !!appDefaultEntry && + (await isStoredEngineUsableForRequest(appDefault, appDefaultEntry)); + const detectedFromUser = await detectEngineFromUserSecrets(); + const configuredEngine = getAppConfig().agent.engine; + const envEntry = configuredEngine ? engineFor(configuredEngine) : undefined; + const envUsable = + !!envEntry && + (await isStoredEngineUsableForRequest( + { engine: envEntry.name }, + envEntry, + )); + const envUnavailable = !!envEntry && !envUsable; + const detectedFromEnv = detectEngineFromEnv(); + const envSelectedEntry = envUsable ? envEntry : undefined; - const currentEntry = envUnavailable - ? undefined - : (envSelectedEntry ?? - (appDefaultUsable ? appDefaultEntry : undefined) ?? - (storedUsable ? storedEntry : undefined) ?? - detectedFromUser ?? - detectedFromEnv ?? - getAgentEngineEntry("anthropic")); - const configuredModel = getAppConfig().agent.model; - const currentModelCandidate = - configuredModel && configuredModel !== "auto" - ? configuredModel - : appDefaultUsable && currentEntry?.name === appDefault?.engine - ? appDefault?.model - : storedUsable && currentEntry?.name === current?.engine - ? current?.model - : undefined; - const acceptsCustomModels = currentEntry - ? await resolveEngineAcceptsCustomModels(currentEntry) - : false; - const preserveCustomModels = currentEntry - ? await resolveEnginePreservesCustomModels(currentEntry) - : false; - // Mirrors the runtime: an engine default nobody picked yields to the first - // checked model once it's unchecked. - const currentSelection = - currentEntry && !currentModelCandidate - ? await selectionFor(currentEntry.name) - : null; - const checkedDefault = - currentSelection?.state === "selected" && - currentEntry && - !currentSelection.models.includes(currentEntry.defaultModel) - ? currentSelection.models[0] - : undefined; - const currentModel = - currentEntry && !envUnavailable - ? normalizeModelForEngine( - currentEntry, - currentModelCandidate ?? checkedDefault ?? currentEntry.defaultModel, - { acceptsCustomModels, preserveCustomModels }, - ) - : undefined; + const currentEntry = envUnavailable + ? undefined + : (envSelectedEntry ?? + (appDefaultUsable ? appDefaultEntry : undefined) ?? + (storedUsable ? storedEntry : undefined) ?? + detectedFromUser ?? + detectedFromEnv ?? + getAgentEngineEntry("anthropic")); + const configuredModel = getAppConfig().agent.model; + const currentModelCandidate = + configuredModel && configuredModel !== "auto" + ? configuredModel + : appDefaultUsable && currentEntry?.name === appDefault?.engine + ? appDefault?.model + : storedUsable && currentEntry?.name === current?.engine + ? current?.model + : undefined; + const acceptsCustomModels = currentEntry + ? await resolveEngineAcceptsCustomModels(currentEntry) + : false; + const preserveCustomModels = currentEntry + ? await resolveEnginePreservesCustomModels(currentEntry) + : false; + // Mirrors the runtime: an engine default nobody picked yields to the first + // checked model once it's unchecked. + const currentSelection = + currentEntry && !currentModelCandidate + ? await selectionFor(currentEntry.name) + : null; + const checkedDefault = + currentSelection?.state === "selected" && + currentEntry && + !currentSelection.models.includes(currentEntry.defaultModel) + ? currentSelection.models[0] + : undefined; + const currentModel = + currentEntry && !envUnavailable + ? normalizeModelForEngine( + currentEntry, + currentModelCandidate ?? + checkedDefault ?? + currentEntry.defaultModel, + { acceptsCustomModels, preserveCustomModels }, + ) + : undefined; + currentEngine = + !currentEntry || envUnavailable + ? null + : { engine: currentEntry.name, model: currentModel }; + } catch (error) { + if (!(error instanceof CredentialStoreUnavailableError)) throw error; + currentError = error.message; + } const engineEntries = await Promise.all( engines.map(async (e) => { // Resolved per engine, not across the set: one provider whose credential @@ -253,6 +276,7 @@ export async function run(args: Record = {}): Promise { e, ); } catch (error) { + if (error instanceof ForbiddenError) throw error; configuredError = error instanceof Error ? error.message : String(error); } @@ -318,13 +342,8 @@ export async function run(args: Record = {}): Promise { ); const result = { engines: engineEntries, - current: - !currentEntry || envUnavailable - ? null - : { - engine: currentEntry.name, - model: currentModel, - }, + current: currentEngine, + ...(currentError ? { currentError } : {}), canUpdateDefault: defaultAuthority.allowed, defaultSource: defaultSetting.source, }; diff --git a/packages/core/src/server/action-routes.spec.ts b/packages/core/src/server/action-routes.spec.ts index 065cefde274..c6a0043299c 100644 --- a/packages/core/src/server/action-routes.spec.ts +++ b/packages/core/src/server/action-routes.spec.ts @@ -6074,42 +6074,55 @@ describe("mountActionRoutes", () => { expect(received.orgId).toBeNull(); }); - it.each(['relation "org_members" does not exist'])( - "suppresses the verified first-boot missing org table error: %s", - async (message) => { + it.each([ + ["first boot", 'relation "org_members" does not exist', "42P01", true], + ["denied", "permission denied for table org_members", "42501", false], + ["unknown", "membership lookup failed", undefined, false], + ["other table", 'relation "settings" does not exist', "42P01", false], + ] as const)( + "dispatches only the real first-boot resolver exception: %s", + async (_label, message, code, allowed) => { + const org = + await vi.importActual( + "../org/context.js", + ); + mockResolveOrgIdForEmail.mockImplementation(org.resolveOrgIdForEmail); + mockResolveOrgScopeForEmail.mockImplementation( + org.resolveOrgScopeForEmail, + ); + const db = await import("../db/client.js"); + const failure = Object.assign(new Error(message), { code }); + const execute = vi.fn().mockRejectedValue(failure); + vi.spyOn(db, "getDbExec").mockReturnValue({ execute } as any); const { mountActionRoutes } = await import("./action-routes.js"); - mockResolveOrgIdForEmail.mockRejectedValue(new Error(message)); - const mounted: Array<{ path: string; handler: any }> = []; - const nitroApp = { - use: vi.fn((path: string, handler: any) => - mounted.push({ path, handler }), - ), - }; - let received: any; + const mounted: Array<{ handler: any }> = []; + const run = vi.fn(async () => getRequestContext()); mountActionRoutes( - nitroApp, - { - "do-thing": { - run: vi.fn(async (_params, ctx) => { - received = ctx; - return { ok: true }; - }), - } as any, - }, + { use: (_path: string, handler: any) => mounted.push({ handler }) }, + { "do-thing": { run } as any }, { - getOwnerFromEvent: async () => "steve@example.com", + getOwnerFromEvent: async () => "first-boot@example.test", resolveOrgId: async () => null, }, ); + const response = mounted[0]!.handler({ + _method: "POST", + _headers: { "x-agent-native-frontend": "1" }, + context: {}, + req: { json: async () => ({}) }, + }); - await expect( - mounted[0].handler({ - _method: "POST", - _headers: { "x-agent-native-frontend": "1" }, - req: { json: async () => ({}) }, - }), - ).resolves.toEqual({ ok: true }); - expect(received.orgId).toBeNull(); + if (allowed) { + await expect(response).resolves.toMatchObject({ + userEmail: "first-boot@example.test", + }); + expect(await response).not.toHaveProperty("orgScope", "personal"); + expect(run).toHaveBeenCalledOnce(); + } else { + await expect(response).rejects.toBe(failure); + expect(run).not.toHaveBeenCalled(); + } + expect(execute).toHaveBeenCalled(); }, ); diff --git a/packages/core/src/server/agent-run-context.spec.ts b/packages/core/src/server/agent-run-context.spec.ts index b2822556698..4649c8a3384 100644 --- a/packages/core/src/server/agent-run-context.spec.ts +++ b/packages/core/src/server/agent-run-context.spec.ts @@ -36,6 +36,7 @@ import { AGENT_CHAT_BROWSER_SESSION_ID_FIELD, AGENT_CHAT_REQUEST_CONTEXT_FIELD, } from "../agent/durable-background.js"; +import { ForbiddenError } from "../sharing/access.js"; import { resolveAgentRunOrgId, resolveAgentRunOrgScope, @@ -211,6 +212,17 @@ describe("server/agent-run-context", () => { ).resolves.toEqual({ orgScope: "personal" }); }); + it("preserves a definitive organization refusal during background scope resolution", async () => { + const refusal = new ForbiddenError(); + resolveOrgScopeForEmailMock.mockRejectedValue(refusal); + await expect( + resolveAgentRunOrgScope({ + event: makeEvent(), + ownerContext: { owner: "delegate@example.test", anonymous: false }, + }), + ).rejects.toBe(refusal); + }); + it("keeps organization scope unknown when an organization lookup fails", async () => { getSessionMock.mockRejectedValue(new Error("session unavailable")); getOrgContextMock.mockRejectedValue(new Error("org store unavailable")); diff --git a/packages/core/src/server/agent-run-context.ts b/packages/core/src/server/agent-run-context.ts index 69c54ae3fc0..34c3e3dda1c 100644 --- a/packages/core/src/server/agent-run-context.ts +++ b/packages/core/src/server/agent-run-context.ts @@ -13,6 +13,7 @@ import { SYNTHETIC_TRAFFIC_HEADER, isSyntheticTrafficValue, } from "../shared/test-traffic.js"; +import { ForbiddenError } from "../sharing/errors.js"; import { CREDENTIAL_MEMBERSHIP_UNAVAILABLE_MESSAGE, isCredentialMembershipUnavailable, @@ -313,7 +314,8 @@ export async function resolveAgentRunOrgScope(options: { const { getSession } = await import("./auth.js"); const session = await getSession(options.event); resolvedOrgId = normalizeId(session?.orgId); - } catch { + } catch (error) { + if (error instanceof ForbiddenError) throw error; orgLookupFailed = true; } @@ -336,7 +338,8 @@ export async function resolveAgentRunOrgScope(options: { ) { return { orgScope: "personal" }; } - } catch { + } catch (error) { + if (error instanceof ForbiddenError) throw error; orgLookupFailed = true; } } @@ -352,7 +355,8 @@ export async function resolveAgentRunOrgScope(options: { } else if (!orgLookupFailed) { return { orgScope: "personal" }; } - } catch { + } catch (error) { + if (error instanceof ForbiddenError) throw error; orgLookupFailed = true; } } diff --git a/packages/core/src/server/agent-teams-delegation-depth.spec.ts b/packages/core/src/server/agent-teams-delegation-depth.spec.ts index fb2f2882db9..60ed3b2398f 100644 --- a/packages/core/src/server/agent-teams-delegation-depth.spec.ts +++ b/packages/core/src/server/agent-teams-delegation-depth.spec.ts @@ -58,7 +58,8 @@ vi.mock("../progress/registry.js", () => ({ completeRun: vi.fn(async () => ({})), })); -vi.mock("./request-context.js", () => ({ +vi.mock("./request-context.js", async (importOriginal) => ({ + ...(await importOriginal()), getRequestOrgId: () => requestContextState.orgId, getRequestRunContext: () => ({ allowedActionNames: requestContextState.allowedActionNames, @@ -98,6 +99,7 @@ describe("agent-teams delegation-depth guardrail", () => { }); afterEach(() => { + vi.restoreAllMocks(); delete process.env.AGENT_NATIVE_MAX_SUBAGENT_DEPTH; }); @@ -188,16 +190,158 @@ describe("agent-teams delegation-depth guardrail", () => { }); it("preserves an explicitly org-less parent request", async () => { + const request = await import("./request-context.js"); + const actualRequest = await vi.importActual( + "./request-context.js", + ); + vi.spyOn(request, "hasRequestContext").mockImplementation( + actualRequest.hasRequestContext, + ); + vi.spyOn(request, "getRequestOrgId").mockImplementation( + actualRequest.getRequestOrgId, + ); + const org = + await vi.importActual( + "../org/context.js", + ); + resolveOrgIdForEmailMock.mockImplementation(() => + org.resolveOrgIdForEmail(OWNER), + ); + const db = await import("../db/client.js"); + const execute = vi + .fn() + .mockRejectedValue( + new Error("Personal scope must not query memberships"), + ); + vi.spyOn(db, "getDbExec").mockReturnValue({ execute } as any); const { spawnTask } = await import("./agent-teams.js"); - requestContextState.active = true; - requestContextState.orgId = undefined; - await spawnTask(baseSpawnOptions()); + await actualRequest.runWithRequestContext( + { userEmail: OWNER, orgScope: "personal" }, + () => spawnTask(baseSpawnOptions()), + ); expect(enqueueAgentTeamRunMock).toHaveBeenCalledWith( expect.objectContaining({ orgId: null }), ); - expect(resolveOrgIdForEmailMock).not.toHaveBeenCalled(); + expect(execute).not.toHaveBeenCalled(); + }); + + it("rejects an unresolved real parent request before creating resources", async () => { + const request = await import("./request-context.js"); + const actualRequest = await vi.importActual( + "./request-context.js", + ); + vi.spyOn(request, "hasRequestContext").mockImplementation( + actualRequest.hasRequestContext, + ); + vi.spyOn(request, "getRequestOrgId").mockImplementation( + actualRequest.getRequestOrgId, + ); + const org = + await vi.importActual( + "../org/context.js", + ); + resolveOrgIdForEmailMock.mockImplementation(() => + org.resolveOrgIdForEmail(OWNER), + ); + const db = await import("../db/client.js"); + const execute = vi.fn().mockRejectedValue( + Object.assign(new Error("permission denied for table org_members"), { + code: "42501", + }), + ); + vi.spyOn(db, "getDbExec").mockReturnValue({ execute } as any); + const { spawnTask } = await import("./agent-teams.js"); + const parentSend = vi.fn(); + const [result] = await Promise.allSettled([ + actualRequest.runWithRequestContext({ userEmail: OWNER }, () => + spawnTask({ ...baseSpawnOptions(), parentSend }), + ), + ]); + expect({ + result: result.status, + threads: createThreadMock.mock.calls.length, + state: appState.size, + queued: enqueueAgentTeamRunMock.mock.calls.length, + dispatched: fireInternalDispatchMock.mock.calls.length, + notified: parentSend.mock.calls.length, + }).toEqual({ + result: "rejected", + threads: 0, + state: 0, + queued: 0, + dispatched: 0, + notified: 0, + }); + expect(execute).toHaveBeenCalledOnce(); + }); + + it.each([ + ['relation "org_members" does not exist', "42P01"], + ["permission denied for table org_members", "42501"], + ["membership lookup failed", undefined], + ] as const)( + "rejects real background org lookup failure before creating resources: %s", + async (message, code) => { + const org = + await vi.importActual( + "../org/context.js", + ); + resolveOrgIdForEmailMock.mockImplementation(() => + org.resolveOrgIdForEmail(OWNER), + ); + const db = await import("../db/client.js"); + const execute = vi + .fn() + .mockRejectedValue(Object.assign(new Error(message), { code })); + vi.spyOn(db, "getDbExec").mockReturnValue({ execute } as any); + const { spawnTask } = await import("./agent-teams.js"); + const parentSend = vi.fn(); + const [result] = await Promise.allSettled([ + spawnTask({ ...baseSpawnOptions(), parentSend }), + ]); + + expect({ + result: result.status, + threads: createThreadMock.mock.calls.length, + state: appState.size, + queued: enqueueAgentTeamRunMock.mock.calls.length, + dispatched: fireInternalDispatchMock.mock.calls.length, + notified: parentSend.mock.calls.length, + }).toEqual({ + result: "rejected", + threads: 0, + state: 0, + queued: 0, + dispatched: 0, + notified: 0, + }); + expect(execute).toHaveBeenCalledOnce(); + }, + ); + + it("persists Personal after a successful real background membership read without rows", async () => { + const org = + await vi.importActual( + "../org/context.js", + ); + resolveOrgIdForEmailMock.mockImplementation(() => + org.resolveOrgIdForEmail(OWNER), + ); + const db = await import("../db/client.js"); + const execute = vi.fn().mockResolvedValue({ rows: [] }); + vi.spyOn(db, "getDbExec").mockReturnValue({ execute } as any); + const { spawnTask } = await import("./agent-teams.js"); + + const task = await spawnTask(baseSpawnOptions()); + + expect(task.orgId).toBeNull(); + expect(appState.get(`agent-task:${task.taskId}`)?.orgId).toBeNull(); + expect(enqueueAgentTeamRunMock).toHaveBeenCalledWith( + expect.objectContaining({ orgId: null }), + ); + expect(execute).toHaveBeenCalledOnce(); }); it("scopes a spawned thread to the parent app", async () => { diff --git a/packages/core/src/server/agent-teams.ts b/packages/core/src/server/agent-teams.ts index df800487efa..0cfe62b2bb2 100644 --- a/packages/core/src/server/agent-teams.ts +++ b/packages/core/src/server/agent-teams.ts @@ -1864,6 +1864,8 @@ export async function spawnTask(opts: SpawnTaskOptions): Promise { throw new SubagentDelegationDepthError(decision); } const childDepth = decision.childDepth; + const requestOrgId = hasRequestContext() ? getRequestOrgId() : undefined; + const orgId = requestOrgId ?? (await resolveOrgIdForEmail(opts.ownerEmail)); const taskId = generateTaskId(); const userMsgId = `msg-${taskId}-user`; @@ -1895,16 +1897,6 @@ export async function spawnTask(opts: SpawnTaskOptions): Promise { const runId = taskRunId(taskId); const createdAt = Date.now(); - let orgId: string | null; - if (hasRequestContext()) { - orgId = getRequestOrgId() ?? null; - } else { - try { - orgId = await resolveOrgIdForEmail(opts.ownerEmail); - } catch { - orgId = null; - } - } const task: AgentTask = { taskId, threadId: thread.id, diff --git a/packages/core/src/server/auth.spec.ts b/packages/core/src/server/auth.spec.ts index 3ea19bf9e5f..9e0dbd91c92 100644 --- a/packages/core/src/server/auth.spec.ts +++ b/packages/core/src/server/auth.spec.ts @@ -3629,7 +3629,10 @@ describe("server/auth", () => { const app = createMockApp(); await autoMountAuth(app, { - getSession: async () => ({ email: "member@example.com" }), + getSession: async () => ({ + email: "member@example.com", + orgId: "fixture-org", + }), }); const guard = app.use.mock.calls @@ -8976,6 +8979,47 @@ describe("server/auth", () => { expect(sessionLookups).toHaveBeenCalledTimes(2); }); + it.each(["42501", "08006", "forbidden"])( + "preserves an org backfill failure instead of returning an org-less session (%s)", + async (code) => { + vi.stubEnv("NODE_ENV", "production"); + delete process.env.ACCESS_TOKEN; + delete process.env.ACCESS_TOKENS; + const { ForbiddenError } = await import("../sharing/access.js"); + const cause = + code === "forbidden" + ? new ForbiddenError() + : Object.assign(new Error("membership read failed"), { code }); + vi.doMock("./better-auth-instance.js", () => ({ + getBetterAuth: vi.fn(async () => undefined), + getBetterAuthSync: () => ({ + api: { + getSession: async () => ({ + user: { id: "fake-auth-user", email: "member@example.test" }, + session: { token: "fake-session-token" }, + }), + }, + }), + })); + const execute = vi.fn(async ({ sql }: { sql: string }) => { + if (sql.includes("FROM org_members")) throw cause; + return { rows: [] }; + }); + vi.doMock("../db/client.js", async (importOriginal) => ({ + ...(await importOriginal()), + getDbExec: () => ({ execute }), + })); + const { getSession } = await import("./auth.js"); + await expect( + getSession( + createMockEvent({ + headers: { cookie: "an_session=fake-session-token" }, + }), + ), + ).rejects.toBe(cause); + }, + ); + it("records identity resolution start before asynchronous credential validation", async () => { vi.stubEnv("NODE_ENV", "production"); delete process.env.ACCESS_TOKEN; diff --git a/packages/core/src/server/auth.ts b/packages/core/src/server/auth.ts index 426f913fdb4..acc59069494 100644 --- a/packages/core/src/server/auth.ts +++ b/packages/core/src/server/auth.ts @@ -4758,9 +4758,7 @@ async function backfillSessionOrg( ): Promise { if (session.orgId || hasExplicitPersonalOrgScope(event)) return session; const { resolveOrgIdForEmailViaEvent } = await import("../org/context.js"); - const orgId = await resolveOrgIdForEmailViaEvent(event, session.email).catch( - () => null, - ); + const orgId = await resolveOrgIdForEmailViaEvent(event, session.email); return orgId ? { ...session, orgId } : session; } diff --git a/packages/core/src/server/credential-provider-org-resolution.spec.ts b/packages/core/src/server/credential-provider-org-resolution.spec.ts new file mode 100644 index 00000000000..7b80432bb26 --- /dev/null +++ b/packages/core/src/server/credential-provider-org-resolution.spec.ts @@ -0,0 +1,206 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +const fixture = vi.hoisted(() => ({ + membershipUnavailable: false, + membershipMissing: false, + personalAbsent: false, + storageFailure: undefined as Error | undefined, + membershipFailure: Object.assign( + new Error("permission denied for table org_members"), + { code: "42501" }, + ), + restricted: true, + email: "member@example.test", + orgId: "restricted-org", + personalKey: "fake-personal-provider-key", +})); + +vi.mock("../db/client.js", async (importOriginal) => ({ + ...(await importOriginal()), + getDbExec: () => ({ + execute: async ({ sql }: { sql: string }) => { + if (sql.includes("SELECT org_id FROM org_members")) { + if (fixture.membershipUnavailable) { + throw fixture.membershipFailure; + } + return { + rows: fixture.membershipMissing ? [] : [{ org_id: fixture.orgId }], + }; + } + if (sql.includes("SELECT role FROM org_members")) { + return { rows: [{ role: "member" }] }; + } + throw new Error(`Unexpected query: ${sql}`); + }, + }), +})); + +vi.mock("../settings/store.js", async (importOriginal) => ({ + ...(await importOriginal()), + getSetting: async (key: string) => { + if (key === `u:${fixture.email}:active-org-id`) return null; + if (key === `o:${fixture.orgId}:restrict-personal-provider-keys`) { + return { restricted: fixture.restricted }; + } + throw new Error(`Unexpected setting: ${key}`); + }, +})); + +vi.mock("../secrets/storage.js", () => { + const readAppSecret = async (ref: { + key: string; + scope: string; + scopeId: string; + }) => { + if (fixture.storageFailure) throw fixture.storageFailure; + return !fixture.personalAbsent && + ref.scope === "user" && + ref.scopeId === fixture.email + ? { + value: + ref.key === "ANTHROPIC_API_KEY" + ? fixture.personalKey + : `fake-${ref.key}`, + } + : null; + }; + return { + readAppSecret, + readAppSecrets: async (ref: { + keys: readonly string[]; + scope: string; + scopeId: string; + }) => { + const rows = new Map(); + for (const key of ref.keys) { + const row = await readAppSecret({ ...ref, key }); + if (row) rows.set(key, row); + } + return rows; + }, + }; +}); + +import { ForbiddenError } from "../sharing/access.js"; +import { + CredentialStoreUnavailableError, + resolveSecret, + resolveSecretDetailed, + resolveSecretPairs, +} from "./credential-provider.js"; +import { runWithRequestContext } from "./request-context.js"; + +function resolvePersonalProviderKey() { + return runWithRequestContext({ userEmail: fixture.email }, () => + resolveSecretDetailed("ANTHROPIC_API_KEY"), + ); +} + +beforeEach(() => { + fixture.membershipUnavailable = false; + fixture.membershipMissing = false; + fixture.personalAbsent = false; + fixture.storageFailure = undefined; + fixture.restricted = true; + vi.stubEnv("ANTHROPIC_API_KEY", ""); + vi.stubEnv("AGENT_VAULT_ORG_ID", ""); +}); + +afterEach(() => vi.unstubAllEnvs()); + +describe("credential resolution with real org scope and personal-key policy", () => { + it("reports a failed lookup without returning a personal key when membership access is denied", async () => { + fixture.membershipUnavailable = true; + + await expect(resolvePersonalProviderKey()).resolves.toMatchObject({ + value: null, + lookupFailed: true, + }); + }); + + it("skips the restricted member's readable personal key when membership is readable", async () => { + await expect(resolvePersonalProviderKey()).resolves.toMatchObject({ + value: null, + lookupFailed: false, + }); + }); + + it("returns the same readable personal key when the organization allows it", async () => { + fixture.restricted = false; + + await expect(resolvePersonalProviderKey()).resolves.toMatchObject({ + value: fixture.personalKey, + lookupFailed: false, + source: "user", + }); + }); +}); + +describe.each([ + { + name: "resolveSecret", + resolve: () => resolveSecret("ANTHROPIC_API_KEY"), + personal: fixture.personalKey, + }, + { + name: "resolveSecretPairs", + resolve: () => + resolveSecretPairs([["EXAMPLE_CLIENT_ID", "EXAMPLE_CLIENT_SECRET"]]), + personal: ["fake-EXAMPLE_CLIENT_ID", "fake-EXAMPLE_CLIENT_SECRET"], + }, +])("$name with real organization resolution", ({ resolve, personal }) => { + it("throws with the original nontransient membership lookup failure", async () => { + fixture.membershipUnavailable = true; + const lookup = runWithRequestContext({ userEmail: fixture.email }, resolve); + await expect(lookup).rejects.toBeInstanceOf( + CredentialStoreUnavailableError, + ); + await expect(lookup.catch((error: Error) => error.cause)).resolves.toBe( + fixture.membershipFailure, + ); + }); + + it("preserves a definitive refusal's identity", async () => { + const refusal = new ForbiddenError(); + fixture.storageFailure = refusal; + await expect( + runWithRequestContext({ userEmail: fixture.email }, resolve), + ).rejects.toBe(refusal); + }); + + it("refuses an incomplete delegated identity", async () => { + await expect( + runWithRequestContext( + { + userEmail: fixture.email, + a2aIdentityAssurance: "org-member", + }, + resolve, + ), + ).rejects.toBeInstanceOf(ForbiddenError); + }); + + it("returns null when readable Personal storage has no credential", async () => { + fixture.personalAbsent = true; + await expect( + runWithRequestContext( + { userEmail: fixture.email, orgScope: "personal" }, + resolve, + ), + ).resolves.toBeNull(); + }); + + it("returns the readable Personal credential without an organization", async () => { + fixture.membershipMissing = true; + await expect( + runWithRequestContext({ userEmail: fixture.email }, resolve), + ).resolves.toEqual(personal); + }); + + it("returns the readable credential when the organization permits it", async () => { + fixture.restricted = false; + await expect( + runWithRequestContext({ userEmail: fixture.email }, resolve), + ).resolves.toEqual(personal); + }); +}); diff --git a/packages/core/src/server/credential-provider.ts b/packages/core/src/server/credential-provider.ts index 006c8b32be8..295f46f49dc 100644 --- a/packages/core/src/server/credential-provider.ts +++ b/packages/core/src/server/credential-provider.ts @@ -5,12 +5,9 @@ import { GATEWAY_UNAVAILABLE_VISITOR_MESSAGE, } from "../agent/engine/credential-errors.js"; import { getAppConfig } from "../app-config/index.js"; -import { - getDbExec, - isLocalDatabase, - isTransientDatabaseError, -} from "../db/client.js"; +import { getDbExec, isLocalDatabase } from "../db/client.js"; import { getOrgSetting } from "../settings/org-settings.js"; +import { ForbiddenError } from "../sharing/errors.js"; import { BUILDER_CREDENTIAL_KEYS } from "./builder-credential-keys.js"; import { BuilderOAuthScopeError, @@ -154,7 +151,8 @@ export function assertCredentialStoreReadable(result: { lookupFailed: boolean; cause?: unknown; }): void { - if (result.lookupFailed && isTransientDatabaseError(result.cause)) { + if (result.cause instanceof ForbiddenError) throw result.cause; + if (result.lookupFailed) { throw new CredentialStoreUnavailableError(result.cause); } } @@ -443,6 +441,7 @@ async function resolveOrgIdForRequestEmail( const { resolveOrgIdForEmail } = await import("../org/context.js"); return { orgId: await resolveOrgIdForEmail(email) }; } catch (err) { + if (err instanceof ForbiddenError) throw err; return { orgId: null, cause: err }; } } @@ -704,6 +703,7 @@ async function resolveScopedBuilderCredential( ); } } catch (err) { + if (err instanceof ForbiddenError) throw err; if (traceLookup) { console.log( `[builder-credential] key=${key} email=${email} scope=${scopeAttempted} error=${(err as Error)?.message ?? err}`, @@ -887,6 +887,7 @@ async function resolveScopedBuilderCredentials( } } } catch (err) { + if (err instanceof ForbiddenError) throw err; if (traceLookup) { console.log( `[builder-credential] email=${email} scope=${scopeAttempted} credentials error=${(err as Error)?.message ?? err}`, @@ -1311,6 +1312,7 @@ export async function resolveBuilderGatewayAuth( ? await hasBuilderOAuthSession(ownerEmail, orgId, { orgOnly: true }) : await hasBuilderOAuthSession(ownerEmail, orgId); } catch (error) { + if (error instanceof ForbiddenError) throw error; throw new BuilderCredentialLookupError(error); } } @@ -1329,6 +1331,7 @@ export async function resolveBuilderGatewayAuth( } : null; } catch (error) { + if (error instanceof ForbiddenError) throw error; if (error instanceof BuilderOAuthScopeError) return null; throw new BuilderCredentialLookupError(error); } @@ -2044,8 +2047,7 @@ export async function resolveSecretPairs( if (lookupFailed) { const environmentPair = readEnvironmentPairs(); if (environmentPair) return environmentPair; - assertCredentialStoreReadable({ lookupFailed, cause }); - return null; + throw cause; } if (personal && !(await readsOrgCredentialFirst(orgId, email))) { @@ -2104,6 +2106,7 @@ export async function resolveSecretPairs( } } } catch (error) { + if (error instanceof ForbiddenError) throw error; lookupFailed = true; cause = error; } @@ -2312,6 +2315,7 @@ export async function resolveSecretDetailed( } } } catch (err) { + if (err instanceof ForbiddenError) throw err; if (traceLookup) { console.log( `[resolve-secret] key=${key} email=${email} scope=error err=${(err as Error)?.message ?? err}`, diff --git a/packages/core/src/sharing/access.ts b/packages/core/src/sharing/access.ts index 197b6a71175..7872714662b 100644 --- a/packages/core/src/sharing/access.ts +++ b/packages/core/src/sharing/access.ts @@ -16,11 +16,13 @@ import { workspaceUserGroupsIncludeUser, workspaceUserGroupsTable, } from "../workspace-connections/groups.js"; +import { ForbiddenError } from "./errors.js"; import { listShareableResources, requireShareableResource, type ShareableResourceRegistration, } from "./registry.js"; +export { ForbiddenError } from "./errors.js"; import { ROLE_RANK, type ShareRole, type Visibility } from "./schema.js"; function findRegistrationByTable( @@ -32,14 +34,6 @@ function findRegistrationByTable( return undefined; } -export class ForbiddenError extends Error { - statusCode = 403; - constructor(message = "Forbidden") { - super(message); - this.name = "ForbiddenError"; - } -} - export interface AccessContext { transaction?: DbExec; userEmail?: string; diff --git a/packages/core/src/sharing/errors.ts b/packages/core/src/sharing/errors.ts new file mode 100644 index 00000000000..fd769e388bb --- /dev/null +++ b/packages/core/src/sharing/errors.ts @@ -0,0 +1,7 @@ +export class ForbiddenError extends Error { + statusCode = 403; + constructor(message = "Forbidden") { + super(message); + this.name = "ForbiddenError"; + } +} diff --git a/templates/clips/actions/delete-comment.test.ts b/templates/clips/actions/delete-comment.test.ts index f4d2175c05c..9fc221fce97 100644 --- a/templates/clips/actions/delete-comment.test.ts +++ b/templates/clips/actions/delete-comment.test.ts @@ -96,7 +96,10 @@ import action from "./delete-comment"; beforeEach(() => { vi.resetAllMocks(); - mockAssertAccess.mockResolvedValue({ role: "viewer", resource: {} }); + mockAssertAccess.mockResolvedValue({ + role: "viewer", + resource: { enableComments: true }, + }); mockGetUserEmail.mockReturnValue("author@example.com"); state.rows = [ { @@ -158,47 +161,94 @@ describe("delete-comment", () => { }, ); - it.each([true, false])( - "preserves owner cleanup in Trash with enableComments=%s", - async (enableComments) => { - mockGetUserEmail.mockReturnValue("owner@example.com"); - mockAssertAccess.mockResolvedValue({ - role: "owner", - resource: { enableComments, trashedAt: "2026-01-01T00:00:00Z" }, - }); - const result = await action.run(action.schema.parse({ id: "comment-1" })); - expect(result.deletedCommentIds).toEqual([ - "comment-1", - "comment-1-reply", - "comment-1-reply-2", - ]); - }, - ); + it("preserves owner cleanup in Trash with comments enabled", async () => { + mockGetUserEmail.mockReturnValue("owner@example.com"); + mockAssertAccess.mockResolvedValue({ + role: "owner", + resource: { enableComments: true, trashedAt: "2026-01-01T00:00:00Z" }, + }); + const result = await action.run(action.schema.parse({ id: "comment-1" })); + expect(result.deletedCommentIds).toEqual([ + "comment-1", + "comment-1-reply", + "comment-1-reply-2", + ]); + }); + + it("deletes a comment and descendants with comments enabled", async () => { + mockAssertAccess.mockResolvedValue({ + role: "viewer", + resource: { enableComments: true, trashedAt: null }, + }); + const result = await action.run(action.schema.parse({ id: "comment-1" })); - it.each([true, false])( - "deletes a comment and descendants with enableComments=%s", - async (enableComments) => { + expect(result).toEqual({ + id: "comment-1", + deletedCommentIds: ["comment-1", "comment-1-reply", "comment-1-reply-2"], + }); + expect(state.rows.map((row) => row.id)).toEqual([ + "comment-2", + "comment-1-cross-record-reply", + ]); + expect(mockWriteAppState).toHaveBeenCalledWith("refresh-signal", { + ts: expect.any(Number), + }); + }); + + it.each(["viewer", "editor", "owner"])( + "refuses deletion with comments disabled for %s", + async (role) => { mockAssertAccess.mockResolvedValue({ - role: "viewer", - resource: { enableComments, trashedAt: null }, - }); - const result = await action.run(action.schema.parse({ id: "comment-1" })); - - expect(result).toEqual({ - id: "comment-1", - deletedCommentIds: [ - "comment-1", - "comment-1-reply", - "comment-1-reply-2", - ], - }); - expect(state.rows.map((row) => row.id)).toEqual([ - "comment-2", - "comment-1-cross-record-reply", - ]); - expect(mockWriteAppState).toHaveBeenCalledWith("refresh-signal", { - ts: expect.any(Number), + role, + resource: { enableComments: false }, }); + const originalRows = [...state.rows]; + await expect(action.run({ id: "comment-1" })).rejects.toBeInstanceOf( + ForbiddenError, + ); + expect(state.rows).toEqual(originalRows); + expect(mockWriteAppState).not.toHaveBeenCalled(); }, ); + + it("recognizes the author regardless of email case", async () => { + state.rows[0].authorEmail = "Author@Example.com"; + mockGetUserEmail.mockReturnValue("author@example.COM"); + mockAssertAccess.mockImplementation(async (_type, _id, role) => { + if (role === "editor") throw new ForbiddenError(); + return { role: "viewer", resource: { enableComments: true } }; + }); + expect(await action.run({ id: "comment-1" })).toMatchObject({ + id: "comment-1", + }); + expect(state.rows.map((row) => row.id)).not.toContain("comment-1"); + }); + + it("still refuses deletion by an unrelated viewer", async () => { + mockGetUserEmail.mockReturnValue("unrelated@example.com"); + mockAssertAccess.mockImplementation(async (_type, _id, role) => { + if (role === "editor") throw new ForbiddenError(); + return { role: "viewer", resource: { enableComments: true } }; + }); + const originalRows = [...state.rows]; + await expect(action.run({ id: "comment-1" })).rejects.toBeInstanceOf( + ForbiddenError, + ); + expect(state.rows).toEqual(originalRows); + }); + + it("preserves deletion by a recording editor with comments enabled", async () => { + mockGetUserEmail.mockReturnValue("editor@example.com"); + mockAssertAccess.mockResolvedValue({ + role: "editor", + resource: { enableComments: true }, + }); + expect(await action.run({ id: "comment-1" })).toMatchObject({ + id: "comment-1", + }); + expect(state.rows.map((row) => row.id)).toEqual([ + "comment-2", + "comment-1-cross-record-reply", + ]); + }); }); diff --git a/templates/clips/actions/delete-comment.ts b/templates/clips/actions/delete-comment.ts index a28ff6b931b..10bcba12dc6 100644 --- a/templates/clips/actions/delete-comment.ts +++ b/templates/clips/actions/delete-comment.ts @@ -6,10 +6,8 @@ import { and, eq, inArray } from "drizzle-orm"; import { z } from "zod"; import { getDb, schema } from "../server/db/index.js"; -import { - isRecordingExpiredForViewer, - isRecordingTrashedForViewer, -} from "../server/lib/recording-page-access.js"; +import { assertCommentAccess } from "../server/lib/comment-access.js"; +import { sameOwnerEmail } from "../server/lib/recordings.js"; export default defineAction({ description: @@ -27,29 +25,9 @@ export default defineAction({ if (!existing) throw new Error(`Comment not found: ${args.id}`); const userEmail = getRequestUserEmail(); - const isAuthor = !!userEmail && existing.authorEmail === userEmail; + const isAuthor = sameOwnerEmail(existing.authorEmail, userEmail); - const access = await assertAccess( - "recording", - existing.recordingId, - "viewer", - ); - if ( - isRecordingTrashedForViewer({ - trashedAt: (access.resource as { trashedAt?: string | null }).trashedAt, - viewerIsOwner: access.role === "owner", - }) - ) { - throw new ForbiddenError(); - } - if ( - isRecordingExpiredForViewer({ - expiresAt: (access.resource as { expiresAt?: string }).expiresAt, - viewerIsOwner: access.role === "owner", - }) - ) { - throw new ForbiddenError("Recording has expired"); - } + await assertCommentAccess(existing.recordingId); if (!isAuthor) { try { diff --git a/templates/clips/actions/list-comments.test.ts b/templates/clips/actions/list-comments.test.ts index db3590d4dab..0116fc7924a 100644 --- a/templates/clips/actions/list-comments.test.ts +++ b/templates/clips/actions/list-comments.test.ts @@ -42,6 +42,22 @@ beforeEach(() => { describe("list-comments recording policy", () => { it.each([ { role: "viewer", resource: { enableComments: false } }, + { + role: "viewer", + resource: { + enableComments: true, + visibility: "public", + password: "fake-share-password", + }, + }, + { + role: "editor", + resource: { + enableComments: true, + visibility: "public", + password: "fake-share-password", + }, + }, { role: "viewer", resource: { enableComments: true, trashedAt: "2026-10-22T12:00:00.000Z" }, @@ -67,6 +83,14 @@ describe("list-comments recording policy", () => { it.each([ { role: "owner", resource: { enableComments: false } }, + { + role: "owner", + resource: { + enableComments: false, + visibility: "public", + password: "fake-share-password", + }, + }, { role: "owner", resource: { enableComments: true, trashedAt: "2026-10-22T12:00:00.000Z" }, diff --git a/templates/clips/changelog/2026-10-23-comment-access-and-deletion-follow-recording-permissions-and.md b/templates/clips/changelog/2026-10-23-comment-access-and-deletion-follow-recording-permissions-and.md new file mode 100644 index 00000000000..756366d9e6f --- /dev/null +++ b/templates/clips/changelog/2026-10-23-comment-access-and-deletion-follow-recording-permissions-and.md @@ -0,0 +1,6 @@ +--- +type: fixed +date: 2026-10-23 +--- + +Comment access and deletion follow recording permissions and settings consistently. diff --git a/templates/clips/server/lib/comment-access.ts b/templates/clips/server/lib/comment-access.ts index 371281c5383..f2884c55516 100644 --- a/templates/clips/server/lib/comment-access.ts +++ b/templates/clips/server/lib/comment-access.ts @@ -13,6 +13,7 @@ export async function assertCommentAccess( const access = await assertAccess("recording", recordingId, "viewer"); const recording = access.resource as { enableComments: boolean; + password?: string | null; expiresAt?: string | null; trashedAt?: string | null; }; @@ -36,7 +37,8 @@ export async function assertCommentAccess( } if ( operation === "read" - ? !canReadRecordingComments({ + ? (Boolean(recording.password) && access.role !== "owner") || + !canReadRecordingComments({ enableComments: recording.enableComments, viewerIsOwner: access.role === "owner", }) diff --git a/templates/content/actions/bind-content-database-source-field.db.test.ts b/templates/content/actions/bind-content-database-source-field.db.test.ts index 5cabeed660a..64916514f41 100644 --- a/templates/content/actions/bind-content-database-source-field.db.test.ts +++ b/templates/content/actions/bind-content-database-source-field.db.test.ts @@ -58,7 +58,7 @@ afterAll(() => { let counter = 0; async function asOwner(fn: () => Promise): Promise { - return runWithRequestContext({ userEmail: OWNER }, fn); + return runWithRequestContext({ userEmail: OWNER, orgScope: "personal" }, fn); } async function seedRowUnion() { diff --git a/templates/content/actions/database-setup-mcp.db.test.ts b/templates/content/actions/database-setup-mcp.db.test.ts index 54b0388a5a7..32bd44d27bf 100644 --- a/templates/content/actions/database-setup-mcp.db.test.ts +++ b/templates/content/actions/database-setup-mcp.db.test.ts @@ -51,6 +51,7 @@ let outsiderClient: MCPClient; function callerIdentity(userEmail: string): MCPCallerIdentity { return { userEmail, + orgId: null, orgDomain: undefined, oauthScopes: ["mcp:read", "mcp:write"], }; diff --git a/templates/content/actions/document-description-mcp.db.test.ts b/templates/content/actions/document-description-mcp.db.test.ts index 6ddf2bdf61d..834d113d757 100644 --- a/templates/content/actions/document-description-mcp.db.test.ts +++ b/templates/content/actions/document-description-mcp.db.test.ts @@ -65,6 +65,7 @@ async function connect(userEmail: string, writes = true) { }, { userEmail, + orgId: null, orgDomain: undefined, oauthScopes: writes ? ["mcp:read", "mcp:write"] : ["mcp:read"], }, diff --git a/templates/content/actions/document-trash-mcp.db.test.ts b/templates/content/actions/document-trash-mcp.db.test.ts index 08e4111122f..4188f5d5ad1 100644 --- a/templates/content/actions/document-trash-mcp.db.test.ts +++ b/templates/content/actions/document-trash-mcp.db.test.ts @@ -50,6 +50,7 @@ let outsiderClient: MCPClient; function callerIdentity(userEmail: string): MCPCallerIdentity { return { userEmail, + orgId: null, orgDomain: undefined, oauthScopes: ["mcp:read", "mcp:write"], };