From d02c08c1889163ec674018c0f0907f39749287ae Mon Sep 17 00:00:00 2001 From: Dotta Date: Mon, 26 Oct 2026 17:30:55 -0500 Subject: [PATCH] Preserve saved service choices across token refresh Track successful OAuth consent independently of refresh writes and qualify the delegated-test authorization fixes. Co-Authored-By: Paperclip --- doc/plans/2026-10-13-connection-api-parity.md | 10 ++++++++++ maps/agent-access-map/app-permissions.md | 12 ++++++------ packages/db/src/schema/tool_access.ts | 2 ++ packages/shared/src/types/tool-access.ts | 2 ++ ...tool-access-oauth-protocol.integration.test.ts | 4 ++++ .../tool-access/connections/organization-grant.ts | 3 ++- .../services/tool-access/oauth/cloud-callback.ts | 1 + .../services/tool-access/oauth/direct-callback.ts | 1 + .../connections/GoogleWorkspaceServices.test.tsx | 15 ++++++++++++++- .../connections/GoogleWorkspaceServices.tsx | 4 +++- 10 files changed, 45 insertions(+), 9 deletions(-) diff --git a/doc/plans/2026-10-13-connection-api-parity.md b/doc/plans/2026-10-13-connection-api-parity.md index d3538b41d2b..54545f56b4c 100644 --- a/doc/plans/2026-10-13-connection-api-parity.md +++ b/doc/plans/2026-10-13-connection-api-parity.md @@ -169,3 +169,13 @@ The implementation was rebased onto `a1f77821b9cd5c9c846fdf7a0fa5cc620ad2b6bb`. - Master advanced to `c7c12dbc79` while the review fixes were being qualified. Its published Copilot v62 identity is preserved byte-for-byte. The combined delegated connection guidance uses v63 with digest `sha256:f29dc04c3217ad74aeaf96b2c466d5a1dc6c55942f5bd4db9c1cf52f2985d19e`; the shared identity generator and **60 Copilot installation tests passed**. The first root-level invocation selected no Runner tests; the package configuration ran the intended suite. Migration 0379 is unchanged by this merge. - A later security review found that activity reads authorized the entire endpoint conversation history before applying the activity limit. The real run-JWT regression measured **200 checks before the fix and one afterward** for the same one-item page. The query now selects only conversations referenced by the bounded activity candidates, deduplicates issue decisions, and reuses the request's run read context. The focused regression is green; a fresh complete suite and current-head CI remain required. + + +### 2026-10-26: test authority and saved Google consent review fixes + +- The exact-head review of `69bf11aef9` found private task results readable through test status and cross-agent test execution without the caller's connection access. Both initial actual-auth regressions failed in `/tmp/parity-test-boundaries-red.log`. Implementation `70b7c973c8` passes the authenticated actor into status reads, rechecks task authorization plus agent/responsible-user privacy, and binds agent reads to their originating principal. +- Cross-agent tests require a matching caller profile and the same credential identity, intersect caller and target action policies, and recheck both before execution including approved continuation. The ordinary human approval covers either Ask first policy; self-approval stays denied. Caller-only and shared rate admissions persist without charging one shared bucket twice. +- Intermediate focused runs exposed an expectation mismatch (normal policy denial returns Off), the need to check profile membership independently of broad `tools:use`, and result classification attempting to capture a target agent with the caller's run. The final implementation preserves the actual run identity while retaining the separate test target. Diagnostic failures remain in `/tmp/parity-test-boundaries-green.log`, `/tmp/parity-test-boundaries-green-2.log`, and `/tmp/parity-test-cross-agent-diagnostic.log`; they are superseded by the 16-case passing focused run `/tmp/parity-test-boundaries-green-3.log`. +- **78 API/OpenAPI tests passed** in fresh PostgreSQL (`parity_review_31`): 51 assembled-server actual-auth scenarios and 27 API contracts. Log: `/tmp/parity-test-final-full.log`. Final server typecheck/build and map validation passed (`/tmp/parity-test-final-types.log`, `/tmp/parity-test-final-build.log`, `/tmp/parity-test-final-maps.log`). These replace the earlier 71-test snapshot; no full local test-sweep pass is claimed. +- Companion review also found that token refresh advances `grant.updatedAt` and can erase a saved pending Google service selection. Both new component cases failed before the fix (`/tmp/parity-google-refresh-ui-red.log`). Successful direct/cloud consent now records additive `providerTenant.oauth.consentedAt` metadata; refresh preserves it. The picker compares the pending request against that consent timestamp, using grant creation for legacy metadata rather than treating refresh as new consent. +- **10 focused Google/OAuth tests passed**: eight service-picker cases (including newer/older consent and refresh/legacy behavior) plus two real local-HTTP OAuth protocol cases proving that successful consent is timestamped and concurrent token refresh preserves it. Log: `/tmp/parity-google-refresh-green.log`. The protocol fixture uses its existing board actor injection and does not count as actual-auth agent evidence. JSON metadata is additive; normal migration generation validates that no SQL change is required. diff --git a/maps/agent-access-map/app-permissions.md b/maps/agent-access-map/app-permissions.md index 6647abc7c71..889ffad2c2a 100644 --- a/maps/agent-access-map/app-permissions.md +++ b/maps/agent-access-map/app-permissions.md @@ -254,19 +254,19 @@ Gaps: This individual operation has source review but no recorded HTTP verificat API: `present` — `POST /api/tool-connections/{connectionId}/test-calls`; JSON REST. Agent access: `available` — agent REST and default native `search_api` / `call_api`. -Policy: `tools:use`, responsible-user credential access and target-agent authority; authenticated task/project policies apply before dispatch and after approval. A successful yielded run may continue only while current user, agent, task, and grant authority remain valid. +Policy: `tools:use`, responsible-user credential access and target-agent authority. Testing another agent also requires the caller’s effective profile to include the tool and both principals to select the same credential identity. Caller and target action policies apply before dispatch and after approval; either Ask first decision requires ordinary human approval. Shared rate buckets consume one slot per test. A successful yielded run may continue only while current user, agent, task, and grant authority remain valid. Evidence: `verified` — [assembled-server integration](../../server/src/__tests__/connection-management.integration.test.ts), actual bearer/session authentication, persisted PostgreSQL state, and deterministic local MCP/provider fixtures. -Verification: `passed` — 25-scenario fixture suite on a fresh isolated PostgreSQL database, including scoped denial, approved continuation, revocation, cancellation and replay. No live provider qualification. +Verification: `passed` — 51-scenario fixture suite on a fresh isolated PostgreSQL database, including denied caller profiles, dedicated credentials, caller-only approvals and rate limits, shared rates, private status reads, approved continuation, revocation, cancellation and replay. No live provider qualification. Gaps: Provider fixtures do not qualify live provider setup or availability. ### `get-tool-connections-test-calls` API: `present` — `GET /api/tool-connections/{connectionId}/test-calls/{actionRequestId}`; JSON REST. Agent access: `available` — agent REST and default native `search_api` / `call_api`. -Policy: `tools:use`; [shared management policy](../../doc/plans/2026-10-13-connection-api-parity.md#authorization-contract). Resolved resources and every affected target are checked; personal ownership remains the responsible user. -Evidence: `source-only` — [route](../../server/src/routes/tool-access.ts); reviewed in the working tree based on the source commit above. -Verification: `not-run` — individual operation not executed in this audit; shared authorization and representative family flows are exercised by the authenticated integration suite. Follow the route schema, inspect persistence/audit, and assert owner, scope and revoked-authority denial. -Gaps: This individual operation has source review but no recorded HTTP verification. Provider availability and live setup remain unqualified. +Policy: `tools:use`; shared management policy, originating-principal binding, and current linked-task read authorization. Agent and responsible-user task privacy checks apply even in shadow mode before exposing saved arguments or provider results. +Evidence: `verified` — [assembled-server integration](../../server/src/__tests__/connection-management.integration.test.ts), actual persistent key/run JWT authentication and saved PostgreSQL state. +Verification: `passed` — own visible status returns 200; moving its invocation to an inaccessible private task causes 403 with both agent authentication paths. This regression was red before the fix. +Gaps: Provider fixtures do not qualify live provider setup or availability. ### `post-companies-tools-action-requests-trust-rule` diff --git a/packages/db/src/schema/tool_access.ts b/packages/db/src/schema/tool_access.ts index 747828734e4..f4aede1b4cd 100644 --- a/packages/db/src/schema/tool_access.ts +++ b/packages/db/src/schema/tool_access.ts @@ -204,6 +204,8 @@ export const connectionGrants = pgTable( * per-grant because two users can authorize the same connection with different scopes. */ requestedScopes?: string[]; + /** Last successful consent; ordinary token refresh preserves this. */ + consentedAt?: string; tokenType?: string; refreshedAt?: string; refreshTokenExpiresAt?: string; diff --git a/packages/shared/src/types/tool-access.ts b/packages/shared/src/types/tool-access.ts index b85ef4b83fd..0fbd422fdad 100644 --- a/packages/shared/src/types/tool-access.ts +++ b/packages/shared/src/types/tool-access.ts @@ -233,6 +233,8 @@ export interface ConnectionGrant { * request, so this is the baseline its response is judged against. */ requestedScopes?: string[]; + /** Last successful consent; ordinary token refresh preserves this. */ + consentedAt?: string; tokenType?: string; refreshTokenExpiresAt?: string; refreshedAt?: string; diff --git a/server/src/__tests__/tool-access-oauth-protocol.integration.test.ts b/server/src/__tests__/tool-access-oauth-protocol.integration.test.ts index 9223c214346..daa783228e6 100644 --- a/server/src/__tests__/tool-access-oauth-protocol.integration.test.ts +++ b/server/src/__tests__/tool-access-oauth-protocol.integration.test.ts @@ -152,6 +152,9 @@ describe("tool OAuth over local HTTP", () => { expect(calls.filter((call) => call.path === "/register")).toHaveLength(1); const [grant] = await db.select().from(connectionGrants).where(eq(connectionGrants.connectionId, connection.id)); + const consentedAt = grant.providerTenant?.oauth?.consentedAt; + expect(consentedAt).toEqual(expect.any(String)); + expect(Number.isFinite(Date.parse(consentedAt!))).toBe(true); await db.update(connectionGrants).set({ providerTenant: { ...grant.providerTenant, oauth: { ...grant.providerTenant?.oauth, accessTokenExpiresAt: new Date(0).toISOString() }, @@ -174,6 +177,7 @@ describe("tool OAuth over local HTTP", () => { expect(exchanges[1].authorization).toBe(tokenCall.authorization); const [persistedGrant] = await db.select().from(connectionGrants).where(eq(connectionGrants.id, grant.id)); expect(persistedGrant.providerTenant?.oauth).not.toHaveProperty("refreshLease"); + expect(persistedGrant.providerTenant?.oauth?.consentedAt).toBe(consentedAt); expect(Date.parse(persistedGrant.providerTenant!.oauth!.accessTokenExpiresAt!)).toBeGreaterThan(Date.now()); expect(JSON.stringify(persistedGrant)).not.toMatch(/fixture-(access|refresh|secret)/); await first.checkHealth(connection.id, { actorType: "user", actorId: userId }); diff --git a/server/src/services/tool-access/connections/organization-grant.ts b/server/src/services/tool-access/connections/organization-grant.ts index 721398dd70d..e95505549a7 100644 --- a/server/src/services/tool-access/connections/organization-grant.ts +++ b/server/src/services/tool-access/connections/organization-grant.ts @@ -19,7 +19,7 @@ export function createOrganizationGrantStore(db: Db) { * so without this the grants API reads back a shared identity whose scope has no source * and no over-grant warning — while the connection record carries both. * - * Deliberately scope fields only. The shared grant takes its credential lifecycle from the + * Scope provenance includes its consent timestamp. The shared grant takes its credential lifecycle from the * connection, and writing `accessTokenExpiresAt` here would make an expired connection * look fresh to the refresh-due check and skip the reconnect prompt. */ @@ -42,6 +42,7 @@ export function createOrganizationGrantStore(db: Db) { scopeSource: oauthProvenance.scopeSource, unrequestedScopes: oauthProvenance.unrequestedScopes, requestedScopes: oauthProvenance.requestedScopes, + consentedAt: new Date().toISOString(), }, } : current; diff --git a/server/src/services/tool-access/oauth/cloud-callback.ts b/server/src/services/tool-access/oauth/cloud-callback.ts index 5c5a3902620..dd3c6feed87 100644 --- a/server/src/services/tool-access/oauth/cloud-callback.ts +++ b/server/src/services/tool-access/oauth/cloud-callback.ts @@ -308,6 +308,7 @@ export function createCloudOAuthCallback(db: Db, dependencies: Dependencies) { accessTokenExpiresAt: credentials.accessTokenExpiresAt, scopes: credentials.scopes, requestedScopes: stateRow.requestedScopes ?? undefined, + consentedAt: now().toISOString(), tokenType: credentials.tokenType, ...(credentials.refreshTokenExpiresAt ? { refreshTokenExpiresAt: credentials.refreshTokenExpiresAt } diff --git a/server/src/services/tool-access/oauth/direct-callback.ts b/server/src/services/tool-access/oauth/direct-callback.ts index a95f49c8a21..b46e6c8fd16 100644 --- a/server/src/services/tool-access/oauth/direct-callback.ts +++ b/server/src/services/tool-access/oauth/direct-callback.ts @@ -270,6 +270,7 @@ export function createDirectOAuthCallback(db: Db, dependencies: Dependencies) { // different scopes. The connection-level list is whichever callback ran last, // so it is the wrong baseline for anyone else's refresh. requestedScopes: authorizedScopes, + consentedAt: connectedAt.toISOString(), tokenType: token.tokenType, refreshedAt: connectedAt.toISOString(), }, diff --git a/ui/src/features/connections/GoogleWorkspaceServices.test.tsx b/ui/src/features/connections/GoogleWorkspaceServices.test.tsx index 64fead06020..86865bd28be 100644 --- a/ui/src/features/connections/GoogleWorkspaceServices.test.tsx +++ b/ui/src/features/connections/GoogleWorkspaceServices.test.tsx @@ -98,7 +98,7 @@ describe("Workspace service consent", () => { it.each([true, false])("uses pending consent only when newer than the saved grant (newer: %s)", async newer => { listGrants.mockResolvedValue({ currentUserId: "me", grants: [{ kind: "user", subjectUserId: "me", updatedAt: "2026-10-26T10:00:00Z", - providerTenant: { oauth: { requestedScopes: googleWorkspaceSelectedScopes({ drive: "write" }) } }, + providerTenant: { oauth: { requestedScopes: googleWorkspaceSelectedScopes({ drive: "write" }), consentedAt: "2026-10-26T10:00:00Z" } }, }] }); await render({ ...existing, config: { oauth: { pendingWorkspaceServiceRequests: { me: { requestedScopes: ["https://www.googleapis.com/auth/gmail.readonly"], requestedAt: newer ? "2026-10-26T11:00:00Z" : "2026-10-26T09:00:00Z" }, @@ -106,4 +106,17 @@ describe("Workspace service consent", () => { await click("Continue"); expect(onContinue).toHaveBeenCalledWith(newer ? { gmail: "read" } : { drive: "write" }); }); + it.each([false, true])("preserves pending service choices through token refresh (legacy grant: %s)", async legacy => { + listGrants.mockResolvedValue({ currentUserId: "me", grants: [{ + kind: "user", subjectUserId: "me", createdAt: "2026-10-26T10:00:00Z", updatedAt: "2026-10-26T12:00:00Z", + providerTenant: { oauth: { requestedScopes: googleWorkspaceSelectedScopes({ drive: "write" }), + ...(legacy ? {} : { consentedAt: "2026-10-26T10:00:00Z" }), refreshedAt: "2026-10-26T12:00:00Z" } }, + }] }); + await render({ ...existing, config: { oauth: { pendingWorkspaceServiceRequests: { + me: { requestedScopes: ["https://www.googleapis.com/auth/gmail.readonly"], requestedAt: "2026-10-26T11:00:00Z" }, + } } } }); + await click("Continue"); + expect(onContinue).toHaveBeenCalledWith({ gmail: "read" }); + }); + }); diff --git a/ui/src/features/connections/GoogleWorkspaceServices.tsx b/ui/src/features/connections/GoogleWorkspaceServices.tsx index 621731e8ca6..b0a9c21e2ba 100644 --- a/ui/src/features/connections/GoogleWorkspaceServices.tsx +++ b/ui/src/features/connections/GoogleWorkspaceServices.tsx @@ -60,7 +60,9 @@ export function GoogleWorkspaceServicesDialog(props: DialogProps) { const savedScopes = grant?.providerTenant?.oauth?.requestedScopes ?? grant?.providerTenant?.oauth?.scopes ?? []; const pending = grants.data?.currentUserId ? pendingRequests?.[grants.data.currentUserId] : undefined; const pendingTime = Date.parse(pending?.requestedAt ?? ""); - const grantTime = grant ? new Date(grant.updatedAt).getTime() : -Infinity; + // Refresh and lease writes update the grant row without changing consent. + const consentTime = grant ? Date.parse(grant.providerTenant?.oauth?.consentedAt ?? String(grant.createdAt)) : NaN; + const grantTime = Number.isFinite(consentTime) ? consentTime : -Infinity; const pendingScopes = Number.isFinite(pendingTime) && pendingTime > grantTime && Array.isArray(pending?.requestedScopes) && pending.requestedScopes.every(scope => typeof scope === "string") ? pending.requestedScopes as string[] : undefined; return ;