feat: do not show oidc consent screen every time#989
Draft
steveiliop56 wants to merge 1 commit into
Draft
Conversation
dosubot
Bot
added
the
size:M
Contributor
📝 WalkthroughWalkthroughChangesOIDC authorization now validates client configuration, signs approved scopes into a per-client cookie, verifies that cookie on later requests, and derives OIDC scope consent flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant OIDCController
participant OIDCService
participant OIDCClient
Browser->>OIDCController: Send authorize request
OIDCController->>OIDCService: Verify approved scope cookie
OIDCService-->>OIDCController: Return verification result
OIDCController->>OIDCClient: Get client configuration
OIDCClient-->>OIDCController: Return client
OIDCController-->>Browser: Return consent response and signed scope cookie
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/service/oidc_service.go (1)
310-328: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winOIDC startup now rejects legacy client configs
internal/service/oidc_service.go:310-328
NewOIDCServicenow hard-fails on any client that doesn't use a UUIDclientIdand a 32+ char secret. The repo still has OIDC fixtures usingsome-client-id/some-client-secret, so add a migration path or keep accepting legacy values with a warning.🤖 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 `@internal/service/oidc_service.go` around lines 310 - 328, Update NewOIDCService’s client validation to preserve backward compatibility with legacy client IDs and secrets used by existing fixtures. Replace the unconditional UUID and 32-character secret failures around uuid.Validate and the client-secret length check with a migration or compatibility path that accepts legacy values while emitting clear warnings, while retaining strict validation for newly configured clients where appropriate.
🤖 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.
Inline comments:
In `@internal/controller/oidc_controller.go`:
- Around line 247-259: Bind the consent cookie to the authenticated user in the
OIDC prompt handling around VerifySignedValue: include the current user's stable
identity, such as the `sub` claim, in the signed payload and validate that same
identity before skipping consent. Update the consent-cookie creation logic to
sign the identical user-bound value, and clear or invalidate the cookie during
logout so it cannot survive account changes.
In `@internal/service/oidc_service.go`:
- Around line 995-997: Replace the regular string comparison in
OIDCService.VerifySignedValue with hmac.Equal, comparing the generated signature
and signedValue as byte slices to ensure constant-time verification.
---
Outside diff comments:
In `@internal/service/oidc_service.go`:
- Around line 310-328: Update NewOIDCService’s client validation to preserve
backward compatibility with legacy client IDs and secrets used by existing
fixtures. Replace the unconditional UUID and 32-character secret failures around
uuid.Validate and the client-secret length check with a migration or
compatibility path that accepts legacy values while emitting clear warnings,
while retaining strict validation for newly configured clients where
appropriate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 90bd38a8-adfa-4101-b24a-c7a3dd3a322e
📒 Files selected for processing (6)
frontend/src/pages/authorize-page.tsxinternal/bootstrap/app_bootstrap.gointernal/controller/oidc_controller.gointernal/model/constants.gointernal/model/runtime.gointernal/service/oidc_service.go
Comment on lines
+247
to
+259
| cookieId := strings.SplitN(client.ClientID, "-", 2)[0] | ||
| cookieName := fmt.Sprintf("%s-%s", controller.runtime.ScopeCookieName, cookieId) | ||
| scopeCookie, err := c.Cookie(cookieName) | ||
|
|
||
| if err == nil { | ||
| scopes := fmt.Sprintf("scopes=%s;", req.Scope) | ||
| if controller.oidc.VerifySignedValue(client.ClientSecret, []byte(scopes), scopeCookie) { | ||
| if values.OIDCPrompt != service.OIDCPromptLogin { | ||
| values.OIDCPrompt = service.OIDCPromptNone | ||
| } | ||
| } | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the OIDC controller and related consent-cookie code.
git ls-files | rg '^internal/controller/oidc_controller\.go$|^internal/.*\.go$' || true
echo "---- outline: internal/controller/oidc_controller.go ----"
ast-grep outline internal/controller/oidc_controller.go --view expanded || true
echo "---- relevant reads/writes ----"
rg -n "ScopeCookieName|VerifySignedValue|CreateSignedValue|OIDCPrompt|Authenticated|logout|Logout|scopeCookie|scopes=" internal/controller/oidc_controller.go internal -S
echo "---- surrounding lines around read site ----"
sed -n '230,280p' internal/controller/oidc_controller.go
echo "---- surrounding lines around write site ----"
sed -n '380,430p' internal/controller/oidc_controller.goRepository: tinyauthapp/tinyauth
Length of output: 20696
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- authorize() around prompt handling ----"
sed -n '150,260p' internal/controller/oidc_controller.go
echo "---- logout handler ----"
sed -n '230,310p' internal/controller/user_controller.go
echo "---- search for scope-cookie deletion/clearing ----"
rg -n "ScopeCookieName|SetCookie\\(|MaxAge: *-1|DeleteCookie|scope cookie|tinyauth-scope" internal -SRepository: tinyauthapp/tinyauth
Length of output: 7322
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- authorizeComplete() and nearby flow ----"
sed -n '280,380p' internal/controller/oidc_controller.go
echo "---- route wiring for OIDC endpoints ----"
rg -n "authorizeComplete|/oidc/authorize|authorize\\(" internal/bootstrap internal/controller internal -S
echo "---- OIDC controller tests mentioning prompt or consent cookies ----"
rg -n "prompt|scope cookie|consent|OIDCPromptNone|authorizeComplete|scopes=" internal/controller/oidc_controller_test.go internal/controller -SRepository: tinyauthapp/tinyauth
Length of output: 8988
Bind consent to the authenticated user The scope cookie is browser-wide and not tied to the logged-in account, so a different user on the same browser can skip consent for the same client/scopes. It also survives logout because nothing clears it. Bind the signed value to the user identity (for example sub) and clear it on logout.
🤖 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 `@internal/controller/oidc_controller.go` around lines 247 - 259, Bind the
consent cookie to the authenticated user in the OIDC prompt handling around
VerifySignedValue: include the current user's stable identity, such as the `sub`
claim, in the signed payload and validate that same identity before skipping
consent. Update the consent-cookie creation logic to sign the identical
user-bound value, and clear or invalidate the cookie during logout so it cannot
survive account changes.
Comment on lines
+995
to
+997
| func (service *OIDCService) VerifySignedValue(key string, data []byte, signedValue string) bool { | ||
| return service.CreateSignedValue(key, data) == signedValue | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file and surrounding context.
sed -n '1,120p' internal/service/oidc_service.go
printf '\n----\n'
sed -n '940,1020p' internal/service/oidc_service.go
printf '\n----\n'
# Find related symbols/usages.
rg -n "VerifySignedValue|CreateSignedValue|hmac\.Equal|Verify.*Signed" internal/service/oidc_service.go internal -SRepository: tinyauthapp/tinyauth
Length of output: 7359
Use constant-time comparison for the signature check. CreateSignedValue(...) == signedValue compares the HMAC with a regular string comparison and leaks timing differences; use hmac.Equal here instead.
🤖 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 `@internal/service/oidc_service.go` around lines 995 - 997, Replace the regular
string comparison in OIDCService.VerifySignedValue with hmac.Equal, comparing
the generated signature and signedValue as byte slices to ensure constant-time
verification.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes