refactor(oauth): share authorization attempts across OAuth flows - #14122
Conversation
Greptile SummaryThe PR adds a tenant-scoped, typed, one-time authorization-attempt store and migrates connector OAuth state and PKCE verifier storage to the configured cache.
Confidence Score: 5/5The PR appears safe to merge with no concrete blocking or non-blocking defects identified. The cache implementations satisfy the expanded interface, preserve tenant isolation and atomic one-time semantics, and the migrated OAuth flow consistently validates ownership, connector scope, expiry, PKCE data, and local return paths. Important Files Changed
Sequence DiagramsequenceDiagram
participant Browser
participant API as Connector OAuth API
participant Cache as Tenant Cache
participant Provider as OAuth Provider
Browser->>API: Authorize(source, local return path)
API->>Cache: SET NX(owner, source, state, payload, TTL)
API-->>Browser: Provider authorization URL
Browser->>Provider: Authorize with state and PKCE challenge
Provider-->>Browser: Callback(code, state)
Browser->>API: Callback(code, state)
API->>Cache: Atomic GETDEL(owner, source, state)
API->>Provider: Exchange code and PKCE verifier
API-->>Browser: Redirect with credentialId
Reviews (1): Last reviewed commit: "refactor(oauth): share connector authori..." | Re-trigger Greptile |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17119bde88
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
All reported issues were addressed across 17 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
🖼️ Visual Regression Report
|
evan-onyx
left a comment
There was a problem hiding this comment.
In general I like the approach. I think for a PR like this we might want more E2E tests to get confidence
edbda6d to
cbe94d6
Compare
|
Preview Deployment
|
cbe94d6 to
235e767
Compare
Description
Why this is needed
OAuth authorization starts in one request and finishes later in a browser callback. During that gap, the server must remember who started the flow, where to return them, and—when PKCE is used—the secret verifier needed to finish safely.
That temporary state must work across API replicas, expire automatically, and be usable only once. The connector flow did not have one shared mechanism with all of those guarantees, which made concurrent callbacks and multi-replica deployments harder to reason about.
How it works
This PR adds a small, shared authorization-attempt store backed by Onyx's configured cache. Each attempt:
The connector OAuth flow is the first consumer. Its PKCE verifier now stays on the server instead of in the browser, and its callback verifies the initiating user before using the attempt. The cache gains the two standard atomic operations needed to support this with either Redis or PostgreSQL.
Why this design
The reusable part of OAuth is the authorization-attempt lifecycle—not provider-specific requests, token mapping, or credential storage. Keeping that boundary narrow gives later MCP, external-app, federated-connector, and action OAuth flows the same safety guarantees without forcing their different domain logic into one large framework.
Starting with connectors keeps this foundation reviewable and proves it with a complete, smaller consumer. PR #14123 builds directly on it for the more involved MCP flow.
How Has This Been Tested?
Additional Options