refactor(oauth): consolidate authorization flow primitives - #14157
refactor(oauth): consolidate authorization flow primitives#14157wenxi-onyx wants to merge 1 commit into
Conversation
Greptile SummaryThe PR consolidates reusable OAuth authorization primitives while preserving the connector and MCP authorization flows.
Confidence Score: 5/5The PR appears safe to merge, with no concrete blocking or independently actionable non-blocking issues identified. The consolidated validators and fingerprint helpers preserve reachable behavior for current OAuth callers, and the changed imports do not introduce a circular dependency or external initialization at import time. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
Request[OAuth connect request] --> Validate[Validate SafeOAuthReturnPath]
Validate --> Params[Reject conflicting protocol parameters]
Params --> Snapshot[Fingerprint OAuth configuration]
Snapshot --> Store[Store tenant-scoped authorization attempt]
Store --> Provider[Redirect to OAuth provider]
Provider --> Callback[OAuth callback]
Callback --> Consume[Consume and validate attempt]
Consume --> Return[Redirect to validated internal return path]
Reviews (1): Last reviewed commit: "refactor(oauth): consolidate authorizati..." | Re-trigger Greptile |
|
Full-stack Preview (frontend + backend)
Sign in with GitHub as an |
There was a problem hiding this comment.
1 issue found across 8 files
Confidence score: 3/5
backend/onyx/oauth/authorization_attempt.pypasseslist[tuple[str, str]]tomcp_oauth_connection_headers_fingerprint, which is outside Pydantic’sJsonValuetype and causes strictty checkto fail; convert the header pairs to aJsonValue-compatible structure or adjust the typing.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/onyx/oauth/authorization_attempt.py">
<violation number="1" location="backend/onyx/oauth/authorization_attempt.py:24">
P2: `mcp_oauth_connection_headers_fingerprint` passes `list[tuple[str, str]]`, which is not included in Pydantic's `JsonValue` type, so the strict `ty check` fails on this call. Either convert the header pairs to JSON arrays before calling this helper or include that explicitly typed tuple-list shape in the helper's accepted input type.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| MAX_AUTHORIZATION_ATTEMPT_TTL_SECONDS = 10 * 60 | ||
|
|
||
|
|
||
| def canonical_json_fingerprint(value: JsonValue) -> str: |
There was a problem hiding this comment.
P2: mcp_oauth_connection_headers_fingerprint passes list[tuple[str, str]], which is not included in Pydantic's JsonValue type, so the strict ty check fails on this call. Either convert the header pairs to JSON arrays before calling this helper or include that explicitly typed tuple-list shape in the helper's accepted input type.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/onyx/oauth/authorization_attempt.py, line 24:
<comment>`mcp_oauth_connection_headers_fingerprint` passes `list[tuple[str, str]]`, which is not included in Pydantic's `JsonValue` type, so the strict `ty check` fails on this call. Either convert the header pairs to JSON arrays before calling this helper or include that explicitly typed tuple-list shape in the helper's accepted input type.</comment>
<file context>
@@ -20,6 +21,18 @@
MAX_AUTHORIZATION_ATTEMPT_TTL_SECONDS = 10 * 60
+def canonical_json_fingerprint(value: JsonValue) -> str:
+ """Fingerprint configuration captured by a pending authorization attempt."""
+ serialized = json.dumps(
</file context>
| def canonical_json_fingerprint(value: JsonValue) -> str: | |
| def canonical_json_fingerprint( | |
| value: JsonValue | list[tuple[str, str]], | |
| ) -> str: |
2a8380d to
f32f450
Compare
f32f450 to
c033d98
Compare
🖼️ Visual Regression Report
|
Description
OAuth connection flows now share one authorization-attempt lifecycle, but callers still repeated security-sensitive setup: tenant-aware cache lookup, the ten-minute TTL, payload field constraints, stable configuration fingerprints, protected authorization parameters, and internal return-path validation. Small differences in these rules can make callbacks accept stale configuration or allow provider options to replace fields owned by the OAuth flow.
This base change gives those rules one implementation. AuthorizationAttemptStore can now be declared once with a namespace and payload type; it resolves the current tenant cache for each operation, uses the standard TTL by default, and still accepts an injected cache for isolated tests. Canonical configuration fingerprints live with the attempt store, while shared OAuth models define fingerprint, PKCE-verifier, and safe return-path constraints. Protected authorization parameters remain beside authorization URL construction.
The scope remains intentionally narrow. Domain payload fields, configuration inputs, provider behavior, persistence, and callback side effects stay in their owning features. The next PRs can therefore declare one store and supply their own payload without rebuilding the common lifecycle or introducing a universal OAuth framework.
How Has This Been Tested?
Additional Options