fix unable to login if authentication is delegated to another CAS ser… #6865
Conversation
|
|
WalkthroughIntroduces a conditional guard in the authentication webview to verify the SAML/CAS redirect URL contains the Rocket.Chat server reference before processing token extraction and payload construction, preventing unintended webview closure on external redirects. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/views/AuthenticationWebView.tsx (1)
112-121: Declare thepayloadvariable before use.The variable
payloadis assigned at lines 115 and 117 but never declared, causing a TypeScript compilation error with strict mode enabled. Addlet payload;before the conditional at line 112.Additionally, while the
isRocketChatServerguard correctly prevents premature webview closure during CAS delegation, ensure the SAML vs CAS payload distinction is tested with both single-factor and delegated authentication flows.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
app/views/AuthenticationWebView.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
app/views/AuthenticationWebView.tsx (1)
app/sagas/login.js (4)
server(86-86)server(230-230)server(299-299)server(375-375)
|
Hey |
|
Hello, and thank you for this PR. if (authType === 'saml' || authType === 'cas') {
const parsedUrl = parse(url, true);
// Only close the webview when redirected back to the Rocket.Chat server
// This prevents premature closure when CAS delegates to another CAS server for MFA
const isRocketChatServer = url.startsWith(server);
// ticket -> cas / validate & saml_idp_credentialToken -> saml
if (isRocketChatServer && (parsedUrl.pathname?.includes('validate') || parsedUrl.query?.ticket || parsedUrl.query?.saml_idp_credentialToken)) {
let payload: ICredentials;
if (authType === 'saml') {
const token = parsedUrl.query?.saml_idp_credentialToken || ssoToken;
const credentialToken = { credentialToken: token };
payload = { ...credentialToken, saml: true };
} else {
payload = { cas: { credentialToken: ssoToken } };
}
debouncedLogin(payload);
}
}We hope that this will be quickly incorporated into a future update of the mobile application 🙏 |
|
@vasusadariya can you add @floriannari changes? |
|
hey @diegolmello thanks! i will add these changes that @floriannari suggest me |
4 participants
Proposed changes
Fixed a critical bug in the CAS authentication flow that prevented multi-factor authentication (MFA) from completing. The issue occurred when using cascaded CAS servers where the first CAS server delegates to a second CAS server for MFA.
Previously, the
AuthenticationWebViewwould close prematurely when detecting any CAS ticket in the URL, including intermediate redirects between CAS servers. This fix adds a check to ensure the webview only closes when the authentication flow redirects back to the Rocket.Chat server itself, allowing the complete MFA flow to finish successfully.Changes:
Issue(s)
Fixes the CAS MFA authentication issue where the app closes the authentication webview before completing the second factor validation.
How to test or reproduce
Setup:
Steps to reproduce the bug (before fix):
Expected behavior (after fix):
Affected screens:
AuthenticationWebView.tsx- CAS/SAML authentication flowScreenshots
N/A - Authentication flow behavior (no visual UI changes)
Types of changes
Checklist
Further comments
This is a targeted fix that addresses the root cause without affecting other authentication flows. The change is minimal and adds a single conditional check to verify the redirect URL matches the Rocket.Chat server before triggering the login process.
Technical details:
url.includes(server)before processing CAS/SAML ticketsThis fix is backward compatible and doesn't affect:
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.