Redirect to login when external login initialization fails#565
Redirect to login when external login initialization fails#565
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 13 minutes and 43 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client
participant Service as ExtLogin Service
participant Provider as SAML/OAuth Provider
participant Browser as Browser (Redirect)
Client->>Service: initialize_login_with_ext_provider(request)
Service->>Provider: _prepare_external_auth_request(...)
alt provider prepares OK
Provider-->>Service: auth request (redirect data)
Service-->>Browser: 302 Redirect -> provider auth URL
Browser-->>Provider: Browser follows redirect
else provider raises / ExternalLoginError
Provider-->>Service: raises ExternalLoginError
Service-->>Service: log exception, build _error_redirect_response(result=LOGIN_FAILED, ext_login_error=ACCESS_DENIED/...)
Service-->>Browser: 302 Redirect -> login/redirect_uri (delete SSO cookie)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@seacatauth/external_login/authentication/service.py`:
- Around line 110-123: The try/except only catches ExternalLoginError, allowing
provider/library exceptions (e.g., from prepare_for_authenticate) to escape and
produce bare errors; wrap the call to self._prepare_external_auth_request in a
broader exception handler in all initialize paths (login, sign-up, pairing) so
any Exception is caught, logged, and handled by returning
self._error_redirect_response (same signature as the current ExternalLoginError
branch). Specifically update the call sites that invoke
_prepare_external_auth_request (referenced by AuthOperation.LogIn / sign-up /
pairing initialization methods) to catch Exception, log the provider and full
exception details, and return the same fallback redirect
(delete_sso_cookie=True, result="login_failed" or appropriate result for
sign-up/pairing) using _error_redirect_response so provider internals cannot
bypass the redirect flow.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 240aab6e-8eb7-4f06-b062-c2c5ffebfbf1
📒 Files selected for processing (2)
CHANGELOG.mdseacatauth/external_login/authentication/service.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
seacatauth/external_login/authentication/service.py (1)
486-491: UseExtLoginError.REGISTRATION_DISABLEDin this branch too.This is the one remaining signup-failure path still sending a raw string, so it sits outside the enum-based contract the rest of the file now uses.
♻️ Suggested cleanup
return await self._error_redirect_response( self.LoginUri, result=ExtLoginResult.SIGNUP_FAILED, delete_sso_cookie=True, - ext_login_error="registration_disabled", + ext_login_error=ExtLoginError.REGISTRATION_DISABLED, redirect_uri=self._get_final_redirect_uri(state) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@seacatauth/external_login/authentication/service.py` around lines 486 - 491, Replace the raw string "registration_disabled" with the enum constant ExtLoginError.REGISTRATION_DISABLED in the _error_redirect_response call so this signup-failure branch follows the enum-based contract; update the call inside the branch that returns await self._error_redirect_response(..., result=ExtLoginResult.SIGNUP_FAILED, ext_login_error=...) to pass ExtLoginError.REGISTRATION_DISABLED instead of the string, leaving other args (delete_sso_cookie and redirect_uri=self._get_final_redirect_uri(state)) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@seacatauth/external_login/authentication/service.py`:
- Around line 170-183: The pairing-init failure path deletes the active SSO
cookie and can log out an already-authenticated user; in the block that catches
ExternalLoginError after calling _prepare_external_auth_request for
AuthOperation.PairAccount, stop clearing the SSO cookie by passing
delete_sso_cookie=False (or omitting that flag) to _error_redirect_response so
existing sessions remain intact; update the catch in the method that calls
_prepare_external_auth_request (referenced by _prepare_external_auth_request,
AuthOperation.PairAccount, and _error_redirect_response) accordingly.
---
Nitpick comments:
In `@seacatauth/external_login/authentication/service.py`:
- Around line 486-491: Replace the raw string "registration_disabled" with the
enum constant ExtLoginError.REGISTRATION_DISABLED in the
_error_redirect_response call so this signup-failure branch follows the
enum-based contract; update the call inside the branch that returns await
self._error_redirect_response(..., result=ExtLoginResult.SIGNUP_FAILED,
ext_login_error=...) to pass ExtLoginError.REGISTRATION_DISABLED instead of the
string, leaving other args (delete_sso_cookie and
redirect_uri=self._get_final_redirect_uri(state)) unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 454a1bb3-1153-4662-a70b-1732687c7e32
📒 Files selected for processing (2)
seacatauth/external_login/authentication/providers/saml.pyseacatauth/external_login/authentication/service.py
1 participant
Issue
When external login initialization fails, the user gets stuck on an ugly bare error page.
Fix
Catch the error and redirect the user back to the login page.
Summary by CodeRabbit
Bug Fixes
New Features