feat(authenticate): map the gate and consent errors on both auth RPCs - #1915
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 SummarySummary by CodeRabbit
WalkthroughAuthentication flows now validate consent before delivery, propagate intent, accepted document IDs, and IP metadata, and map known errors to Connect responses. Tests and RFC text cover enabled and disabled consent behavior. ChangesConsent-aware flow validation
Authentication request propagation
Authentication and callback rejection mapping
Estimated code review effort: 3 (Moderate) | ~30 minutes Merge Risk: 🟡 Moderate · up to This change adds consent-aware authentication validation and error mapping, but login requests carrying consent IDs can still proceed through the core flow and unknown intent values can avoid signup-level completeness checks. These request-shape and consent-validation gaps should be resolved before merge. Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Coverage Report for CI Build 34157510811Coverage increased (+0.2%) to 50.212%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
bb8970c to
8e30f1a
Compare
8e30f1a to
e22800f
Compare
e22800f to
40db3a2
Compare
40db3a2 to
f31c0a5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Team
Run ID: 63039b56-ab17-4458-89bd-e53320731adf
📒 Files selected for processing (8)
core/authenticate/mocks/consent_service.gocore/authenticate/service.gocore/authenticate/service_test.godocs/rfcs/0002-explicit-consent-at-signup.mdinternal/api/v1beta1connect/authenticate.gointernal/api/v1beta1connect/authenticate_test.gointernal/api/v1beta1connect/consent.gointernal/api/v1beta1connect/errors.go
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review.
| default: | ||
| return authenticate.FlowIntentUnspecified |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- authenticate.go conversion and handler ---'
sed -n '70,155p' internal/api/v1beta1connect/authenticate.go
printf '%s\n' '--- consent gate and intent semantics ---'
rg -n -A80 -B10 'func .*gateFlowConsent|FlowIntentSignup|FlowIntentUnspecified|resolve' core/authenticate internal/api/v1beta1connect
printf '%s\n' '--- rejection mapping and conversion tests ---'
rg -n -A35 -B10 'lookupAuthFlowRejection|TestToFlowIntent|FlowIntent\(99\)' internal/api/v1beta1connectRepository: raystack/frontier
Length of output: 50374
🤖 get_repo_knowledge executed:
get_repo_knowledge raystack/frontier /tmp/coderabbit-repo-knowledge/raystack-frontier-36c605d8/learnings
Length of output: 12095
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- changed file ---'
sed -n '70,155p' internal/api/v1beta1connect/authenticate.go
printf '%s\n' '--- consent and intent definitions ---'
rg -n -A90 -B10 'func .*gateFlowConsent|FlowIntentSignup|FlowIntentUnspecified|resolve' core/authenticate internal/api/v1beta1connect
printf '%s\n' '--- rejection mapping and conversion tests ---'
rg -n -A35 -B10 'lookupAuthFlowRejection|TestToFlowIntent|FlowIntent\(99\)' internal/api/v1beta1connectRepository: raystack/frontier
Length of output: 50374
Other (CWE-841)
Reachability: External · Exploitability: Trivial
Reject unknown FlowIntent values.
toFlowIntent maps unknown enum values, such as FlowIntent(99), to FlowIntentUnspecified. This applies known-ID consent validation instead of the signup completeness check, allowing a new account to start without all required documents. Return connect.CodeInvalidArgument before StartFlow, preserve only FLOW_INTENT_UNSPECIFIED, and update TestToFlowIntent.
f31c0a5 to
30b9c17
Compare
30b9c17 to
1f684a9
Compare
1f684a9 to
70630b3
Compare
| // consentEnabled reports whether this deployment asks for consent at all. Boot | ||
| // validation rejects an enabled block with no documents, so an empty set means | ||
| // the feature is off. | ||
| func (h *ConnectHandler) consentEnabled() bool { |
There was a problem hiding this comment.
consentEnabled derives "consent is on" from the length of Documents(), which leans on boot validation to stay true. Can we instead expose Enabled() on the consent service and have the handler ask, so the rule lives with the config it reads?
The two request fields reach StartFlow, and the three rejections become legible to a client instead of arriving as a 500. Authenticate calls sessionutils.ExtractSessionMetadata itself for the IP. Authenticate and AuthCallback are both on the authentication skip list, so nothing puts session metadata on the context. That helper parses the user agent into an OS and a browser family and drops the raw string, so what reaches the consent record is the IP and nothing else, from when the user accepted rather than from the callback. ErrLoginUserNotFound maps to NotFound, ErrSignupUserExists to AlreadyExists and ErrConsentRequired to FailedPrecondition, from both RPCs. All three had to join the errors AuthCallback handles explicitly, which maps a fixed list to a 4xx and everything else to Internal; they keep their own codes rather than the InvalidArgument the rest of that list gets, because FailedPrecondition is what lets a client separate a consent rejection from a bad code or an expired flow. Both RPCs answer with a code rather than a redirect. Frontier serves no route for the callback URL: it points at a page the application hosts, and that page is what calls AuthCallback over connect, so it already holds the rejection and decides where the user goes next. Handing it a location header instead would oblige every such page to be written to read one, and a 2xx carrying that header reads as success to one that is not. The three codes are distinct, so a client tells the rejections apart without a second vocabulary alongside them. StartFlow gains the consent half of the flow start gate, so a rejection lands before an OTP is sent and before the browser leaves for an identity provider. A signup intent runs ResolveAll there; an unspecified intent runs Resolve, which still catches an unknown id before the redirect while completeness waits for user creation; a login intent checks nothing, because it writes no record. Ids sent with a login intent are rejected by the handler as InvalidArgument, since accepting them silently would leave a client believing it recorded a consent that does not exist. With app.consent disabled both Resolve and ResolveAll resolve nothing and reject nothing, so the ids are ignored rather than rejected and one client build works against both kinds of deployment. The handler's own rejection follows that rule: it fires only when documents are configured, since a deployment that records no consent under any intent has nothing an id could refer to, and a client built for a consent deployment must still be able to log in against one that is not. Rendering any of this is a separate frontend change: the sign-in and sign-up views for what Authenticate returns, and the callback page for what AuthCallback returns. Refs docs/rfcs/0002-explicit-consent-at-signup.md, Enforcement. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hf1XuPCWcHZ7QY5u4WBB2G
…ady say The same pass over the error mapping: keep the reasons that are not in the code, and drop what the tests restated from the handler's own doc comments (the FailedPrecondition rationale, the bare-sentinel rule, the unspecified intent default). No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
70630b3 to
127d0b4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Team
Run ID: b45680bf-1653-430a-8b9f-984598b06422
📒 Files selected for processing (2)
core/authenticate/service.gocore/authenticate/service_test.go
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review.
| // identical: a signup has to be complete here, an unset intent only has to | ||
| // name known ids, and a login checks nothing because it writes no record. | ||
| func (s Service) gateFlowConsent(intent FlowIntent, ids []string) error { | ||
| if s.consentService == nil || intent == FlowIntentLogin { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject supplied consent IDs for login flows.
Line 459 returns before it inspects ids. A login request with consent IDs can therefore send an OTP or redirect to the provider. The PR requires this request shape to be rejected when consent is enabled.
Keep the disabled-consent behavior. Add a login validation path for non-empty IDs and update the login test to assert that no flow starts.
3 participants
Summary
ErrLoginUserNotFound→NotFound,ErrSignupUserExists→AlreadyExists,ErrConsentRequired→FailedPrecondition, from bothAuthenticateandAuthCallback. All three had to join the fixed listAuthCallbackhandles explicitly, or they fall through toInternalby construction.FailedPreconditionspecifically, because the other errors on that list are allInvalidArgument— the client sent something wrong and resending will not help. A consent rejection is the opposite: the request was well formed and the client can fix it by asking the user to accept what is missing. That distinction is what a client cannot recover from a message string.AuthCallbackover connect, so it already holds the rejection and decides where the user goes. A redirect was implemented first and withdrawn: nothing follows the location header, and a 2xx carrying it reads as success to a client that is not looking for it.StartFlowgains the consent half of the flow-start gate, so a signup rejection lands before an OTP is sent and before the browser leaves for the provider. An unspecified intent checks only that the ids are known, since completeness is not yet knowable; a login intent checks nothing, because it writes no record.app.consentdisabled ids are ignored rather than rejected, which is content and handled one layer down. Rendering any of this is a separate frontend change.