mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-31 22:24:16 +01:00
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 <noreply@paperclip.ing>
This commit is contained in:
1 parent
f832df022f
commit
d02c08c188
10 files changed
+45
-9
No files matched your search
@@ -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.
|
||||
@@ -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`
|
||||
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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 });
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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 }
|
||||
|
||||
@@ -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(),
|
||||
},
|
||||
|
||||
@@ -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" });
|
||||
});
|
||||
|
||||
});
|
||||
@@ -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 <ServicePicker {...props} initial={props.initial ?? (pendingScopes ? googleWorkspaceSelectionForScopes(pendingScopes) : existing ? googleWorkspaceSelectionForScopes(savedScopes) : undefined)} />;
|
||||
|
||||
Reference in new issue
Block a user