refactor: move oidc handling to backend and add support for oidc post#923
Open
steveiliop56 wants to merge 27 commits into
Open
refactor: move oidc handling to backend and add support for oidc post#923steveiliop56 wants to merge 27 commits into
steveiliop56 wants to merge 27 commits into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughConsolidates UI language/theme into QuickActions; adds screen-parameter parsing/serialization and login_for routing; migrates frontend auth pages to ticketed OIDC authorize-complete; and refactors backend OAuth/OIDC contracts, services, controllers, and tests. ChangesOIDC Authorization Flow & Quick Actions UI
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Co-Authored-By: Claude <noreply@anthropic.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
dosubot
Bot
added
the
size:XXL
![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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/controller/oidc_controller.go (1)
280-290:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing
json: truefor consistent JSON error response.The
authorizeCompletehandler is designed to return JSON responses for frontend consumption (as noted in the comment on line 212-214). However, the error case at lines 280-290 doesn't setjson: true, which would cause a 302 redirect instead of a JSON response, inconsistent with all other error paths in this handler.Proposed fix
err = controller.oidc.DeleteOldSession(c, sub) if err != nil { controller.authorizeError(c, authorizeErrorParams{ err: err, reason: "Failed to delete old sessions", reasonPublic: "Failed to delete old sessions", callback: authorizeReq.RedirectURI, callbackError: "server_error", state: authorizeReq.State, + json: true, }) return }🤖 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 280 - 290, The error path calls controller.authorizeError with an authorizeErrorParams struct but omits the json flag, causing a redirect instead of the intended JSON response; update the authorizeError invocation in the authorizeComplete handler (the block that constructs authorizeErrorParams using err, reason "Failed to delete old sessions", callback: authorizeReq.RedirectURI, callbackError: "server_error", state: authorizeReq.State) to include json: true so the authorizeError function returns a consistent JSON error response.
🤖 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 `@frontend/src/components/quick-actions/quick-actions.tsx`:
- Around line 112-120: The menu trigger button in QuickActions (the <button>
rendering Avatar or the Settings icon) lacks an accessible name; update the
button to include an explicit accessible label (e.g., add aria-label="Open user
menu" or a dynamic aria-label based on auth state) and include
aria-haspopup="menu" and aria-expanded tied to the menu's open state (e.g.,
isMenuOpen) so assistive tech knows purpose and state; ensure the label text is
descriptive for both authenticated (Avatar) and unauthenticated (Settings) cases
and update any related state variables or props used by the menu toggle
functions (e.g., the click handler that opens the menu).
In `@frontend/src/lib/hooks/screen-params.ts`:
- Around line 29-33: The current serialization in screen-params.ts lets
undefined become the string "undefined" because the filter only excludes null;
update the filter used when building the URLSearchParams (the
Object.entries(params).filter(...) call that feeds p) to exclude both null and
undefined (e.g., use v != null or explicitly check v !== null && v !==
undefined) so only real string values are passed into URLSearchParams before
calling toString().
In `@frontend/src/pages/totp-page.tsx`:
- Around line 68-73: The redirect for unauthenticated users in the totp pending
check is losing screen params: when !totp.pending && !auth.authenticated the
code returns <Navigate to="/login" replace /> instead of preserving
compiledParams; update this branch to use the same login redirect as the
authenticated branch (use loginForUrl which already includes compiledParams or
append compiledParams to "/login") so both paths preserve screen params (refer
to totp.pending, auth.authenticated, and loginForUrl).
In `@internal/controller/proxy_controller_test.go`:
- Around line 80-82: The tests in proxy_controller_test.go use
assert.Contains(t, location, url.QueryEscape("app")) which can match any "app"
substring; update each check to assert the explicit login_for=app pair by either
parsing the redirect URL (use url.Parse(location) and url.ParseQuery(u.RawQuery)
then assert query["login_for"][0] == "app") or by asserting the encoded pair
directly (e.g., assert.Contains(t, location,
"login_for="+url.QueryEscape("app"))); apply this change for the occurrences
around lines 80-82, 95-97, 111-113, 129-131, 146-148, and 164-166 referencing
the same location variable used in the tests.
---
Outside diff comments:
In `@internal/controller/oidc_controller.go`:
- Around line 280-290: The error path calls controller.authorizeError with an
authorizeErrorParams struct but omits the json flag, causing a redirect instead
of the intended JSON response; update the authorizeError invocation in the
authorizeComplete handler (the block that constructs authorizeErrorParams using
err, reason "Failed to delete old sessions", callback: authorizeReq.RedirectURI,
callbackError: "server_error", state: authorizeReq.State) to include json: true
so the authorizeError function returns a consistent JSON error response.
🪄 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: e974aca6-a8fc-4d8b-a434-014d4a8b7555
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (31)
frontend/src/components/language/language.tsxfrontend/src/components/layout/layout.tsxfrontend/src/components/quick-actions/quick-actions.tsxfrontend/src/components/theme-toggle/theme-toggle.tsxfrontend/src/components/ui/scroll-area.tsxfrontend/src/lib/hooks/login-for.tsfrontend/src/lib/hooks/oidc.tsfrontend/src/lib/hooks/redirect-uri.tsfrontend/src/lib/hooks/screen-params.tsfrontend/src/lib/i18n/locales/en-US.jsonfrontend/src/lib/i18n/locales/en.jsonfrontend/src/main.tsxfrontend/src/pages/authorize-page.tsxfrontend/src/pages/continue-page.tsxfrontend/src/pages/forgot-password-page.tsxfrontend/src/pages/login-page.tsxfrontend/src/pages/logout-page.tsxfrontend/src/pages/totp-page.tsxfrontend/src/schemas/oidc-schemas.tsfrontend/vite.config.tsgo.modinternal/bootstrap/router_bootstrap.gointernal/controller/controller.gointernal/controller/oauth_controller.gointernal/controller/oidc_controller.gointernal/controller/oidc_controller_test.gointernal/controller/proxy_controller.gointernal/controller/proxy_controller_test.gointernal/middleware/ui_middleware.gointernal/service/auth_service.gointernal/service/oidc_service.go
💤 Files with no reviewable changes (4)
- frontend/src/lib/hooks/oidc.ts
- frontend/src/components/theme-toggle/theme-toggle.tsx
- frontend/src/schemas/oidc-schemas.ts
- frontend/src/components/language/language.tsx
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/controller/oidc_controller.go (2)
122-141:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle
requestobjects from POST form bodies too.With the new POST support, this branch only checks
requestin the URL query. If a client sendsrequest=<jwt>in the form body, the code falls through tobinding.Form, butservice.AuthorizeRequesthas norequestfield, so the JWT is dropped and validation runs against an empty request. That breaks POST request-object flows.Suggested fix
- // step 1: if we have a request object, decode it and ignore other params. If not, bind the params as usual - if raw := reqQueries.Get("request"); raw != "" { + // step 1: if we have a request object, decode it and ignore other params. If not, bind the params as usual + raw := reqQueries.Get("request") + if raw == "" && c.Request.Method == http.MethodPost { + raw = c.PostForm("request") + } + if raw != "" { requestObject, err := controller.oidc.DecodeAuthorizeJWT(raw) if err != nil {🤖 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 122 - 141, The code only checks reqQueries.Get("request") (query string) and thus ignores a "request" JWT sent in a POST form body; update the branch that handles request objects so it also looks for the "request" parameter in POST form data before falling back to binding. Specifically, inside the authorize handler around reqQueries/Get("request") and before calling DecodeAuthorizeJWT, check for a form "request" value when c.Request.Method == http.MethodPost (e.g., use c.PostForm("request") or ensure c.Request.ParseForm() and inspect c.Request.Form/PostForm), then call controller.oidc.DecodeAuthorizeJWT(raw) and handle errors via controller.authorizeError with authorizeErrorParams as currently done; if decoded, assign to req (the same variable used with c.ShouldBindWith) and skip the binding. This keeps existing logic in DecodeAuthorizeJWT, authorizeError, c.ShouldBindWith, binding.Form intact while supporting POST request-object flows.
280-287:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
/oidc/authorize-completeerrors on the JSON contract.This handler is frontend-facing and already returns JSON on success and on its earlier failures, but these two branches drop back to redirect mode because
json: trueis missing. A backend error here will produce a 302 to the client callback instead of the{status, redirect_uri}payload the caller expects.Suggested fix
err = controller.oidc.DeleteOldSession(c, sub) if err != nil { controller.authorizeError(c, authorizeErrorParams{ err: err, reason: "Failed to delete old sessions", reasonPublic: "Failed to delete old sessions", callback: authorizeReq.RedirectURI, callbackError: "server_error", state: authorizeReq.State, + json: true, }) return } @@ if err != nil { controller.authorizeError(c, authorizeErrorParams{ err: err, reason: "Failed to build query", reasonPublic: "Failed to build query", callback: authorizeReq.RedirectURI, callbackError: "server_error", state: authorizeReq.State, + json: true, }) return }Also applies to: 300-307
🤖 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 280 - 287, The authorize error branches in the authorize-complete handler call controller.authorizeError without json=true, causing redirects instead of the expected JSON {status, redirect_uri} contract; update both calls that construct authorizeErrorParams (the one passing reason "Failed to delete old sessions" and the later similar branch around the authorizeError invocation at the 300-307 range) to include json: true so controller.authorizeError returns a JSON response; locate the calls to controller.authorizeError and add json: true to the authorizeErrorParams passed to it.
🤖 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.
Outside diff comments:
In `@internal/controller/oidc_controller.go`:
- Around line 122-141: The code only checks reqQueries.Get("request") (query
string) and thus ignores a "request" JWT sent in a POST form body; update the
branch that handles request objects so it also looks for the "request" parameter
in POST form data before falling back to binding. Specifically, inside the
authorize handler around reqQueries/Get("request") and before calling
DecodeAuthorizeJWT, check for a form "request" value when c.Request.Method ==
http.MethodPost (e.g., use c.PostForm("request") or ensure c.Request.ParseForm()
and inspect c.Request.Form/PostForm), then call
controller.oidc.DecodeAuthorizeJWT(raw) and handle errors via
controller.authorizeError with authorizeErrorParams as currently done; if
decoded, assign to req (the same variable used with c.ShouldBindWith) and skip
the binding. This keeps existing logic in DecodeAuthorizeJWT, authorizeError,
c.ShouldBindWith, binding.Form intact while supporting POST request-object
flows.
- Around line 280-287: The authorize error branches in the authorize-complete
handler call controller.authorizeError without json=true, causing redirects
instead of the expected JSON {status, redirect_uri} contract; update both calls
that construct authorizeErrorParams (the one passing reason "Failed to delete
old sessions" and the later similar branch around the authorizeError invocation
at the 300-307 range) to include json: true so controller.authorizeError returns
a JSON response; locate the calls to controller.authorizeError and add json:
true to the authorizeErrorParams passed to it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 84353ac8-7966-45fb-b3fa-3b3f782cc748
📒 Files selected for processing (3)
internal/controller/oidc_controller.gointernal/controller/oidc_controller_test.gointernal/service/oidc_service.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/service/oidc_service.go
- internal/controller/oidc_controller_test.go
steveiliop56
commented
Jun 9, 2026
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
Refactors
Localization