feat: add OAuth2ConsentHandler with VK, session, and user identity resolution modes#4507
Conversation
|
|
|
Warning Review limit reached
More reviews will be available in 7 minutes and 16 seconds. Learn how PR review limits work. To continue reviewing without waiting, enable usage-based billing in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a new ChangesOAuth2 Consent Flow Handler
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
transports/bifrost-http/handlers/oauth2_consent.go (2)
335-348: 💤 Low valueConsider handling URL parse failures more explicitly.
If
url.Parsefails, the function returns the base string unchanged without logging or propagating the error. While theredirect_uriis sourced from the database and should have been validated during the authorize request creation, silent failures can hide configuration or data-integrity issues.Optional improvement
func buildRedirectURL(base string, params map[string]string) string { u, err := url.Parse(base) if err != nil { + logger.Warn("failed to parse redirect_uri %q: %v; returning base unchanged", base, err) return base } q := u.Query() for k, v := range params { if v != "" { q.Set(k, v) } } u.RawQuery = q.Encode() return u.String() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transports/bifrost-http/handlers/oauth2_consent.go` around lines 335 - 348, The buildRedirectURL function silently returns the base string when url.Parse fails without logging the error, which can hide configuration or data-integrity issues. Add explicit error logging when the url.Parse call fails in the buildRedirectURL function so that URL parsing failures are properly reported and can be tracked for debugging configuration or data issues.
202-205: ⚡ Quick winConsider logging temp token deletion failures.
The temp token invalidation error is silently ignored. While the primary replay protection is the
consentedstatus in the database (line 194), logging cleanup failures would improve observability and help detect issues with the temp token service.Suggested improvement
// Invalidate the temp token so the consent page cannot be submitted twice. if h.tempTokens != nil { - _, _ = h.tempTokens.DeleteByResourceID(ctx, temptoken.OAuth2ConsentScopeName, flowID) + if _, err := h.tempTokens.DeleteByResourceID(ctx, temptoken.OAuth2ConsentScopeName, flowID); err != nil { + logger.Warn("failed to invalidate oauth2 consent temp token for flow %s: %v", flowID, err) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transports/bifrost-http/handlers/oauth2_consent.go` around lines 202 - 205, The error returned from the h.tempTokens.DeleteByResourceID call is being silently ignored with blank identifiers. Instead of discarding the error, capture it and check if it is not nil, then log the error using an appropriate logger (such as h's logger or a context logger) to improve observability and help detect issues with the temp token service. This logging should still occur even though the primary replay protection is the consented status in the database, as it helps track cleanup failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@transports/bifrost-http/handlers/oauth2_consent.go`:
- Around line 335-348: The buildRedirectURL function silently returns the base
string when url.Parse fails without logging the error, which can hide
configuration or data-integrity issues. Add explicit error logging when the
url.Parse call fails in the buildRedirectURL function so that URL parsing
failures are properly reported and can be tracked for debugging configuration or
data issues.
- Around line 202-205: The error returned from the
h.tempTokens.DeleteByResourceID call is being silently ignored with blank
identifiers. Instead of discarding the error, capture it and check if it is not
nil, then log the error using an appropriate logger (such as h's logger or a
context logger) to improve observability and help detect issues with the temp
token service. This logging should still occur even though the primary replay
protection is the consented status in the database, as it helps track cleanup
failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5019294d-9091-4622-a89e-cc35d7b83b21
📒 Files selected for processing (2)
transports/bifrost-http/handlers/oauth2_consent.gotransports/bifrost-http/server/server.go
a3e6af6 to
8bebc42
Compare
df31ae6 to
6ebebaf
Compare
8bebc42 to
0c48164
Compare
6ebebaf to
7a09a9d
Compare
0c48164 to
4eb43f0
Compare
7a09a9d to
2be7d78
Compare
4eb43f0 to
02df1ae
Compare
2be7d78 to
4f9b35b
Compare
02df1ae to
d37e5e4
Compare
4f9b35b to
76885d3
Compare
76885d3 to
d82df7b
Compare
d37e5e4 to
5117e2b
Compare
d82df7b to
b8e3cc8
Compare
5117e2b to
6f3e346
Compare
b8e3cc8 to
b4ad168
Compare
6f3e346 to
a9ce98e
Compare
b4ad168 to
314e59d
Compare
a9ce98e to
2ca91f0
Compare
314e59d to
2c2f614
Compare

Summary
Introduces the OAuth2 consent flow API, enabling MCP clients to complete the OAuth2 authorization code flow by presenting a consent page where users can select how they want to identify themselves (via virtual key, session, or user identity).
Changes
OAuth2ConsentHandlerwith two endpoints:GET /api/oauth2/consent/flows/{id}— returns flow details, available identity modes, and the currently logged-in user (if a valid session exists)PUT /api/oauth2/consent/flows/{id}— resolves identity, mints an authorization code (storing only its SHA256 hash), marks the flow as consented, invalidates the temp token, and returns the redirect URL with the code and state per RFC 6749 §4.1.2 and RFC 9207OAuth2IdentityResolverinterface as an optional extension point for user identity resolution. When nil, onlyvkandsessionmodes are offered. When provided, ausermode becomes available if the resolver reports it is configured.RegisterAPIRoutes, defaulting to a nil identity resolver if one has not been pre-assigned toBifrostHTTPServer.OAuth2ConsentHandler.Type of change
Affected areas
How to test
go test ./...GET /api/oauth2/consent/flows/{id}with the temp token — verifyclient_name,available_modes,expires_at, and optionallylogged_in_userare returned.PUT /api/oauth2/consent/flows/{id}with{"mode": "vk", "value": "<plaintext_vk>"}— verify the response contains aredirect_urlwithcode,state, andissquery parameters.consentedand the temp token is invalidated (a second PUT should fail).mode: session(whenEnforceAuthOnInferenceis false) andmode: user(when an identity resolver is configured).Screenshots/Recordings
N/A
Breaking changes
Related issues
N/A
Security considerations
Checklist
docs/contributing/README.mdand followed the guidelines