fix: Show iframe auth when login with token fails #36919
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: 9af54ce The changes in this PR will be included in the next version bump. This PR includes changesets to release 39 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughIntroduces callback-capable loginWithToken across UI context and provider, forwards callbacks from iframe hook, and adds end-to-end iframe authentication tests with a fixture page. Includes a changeset to patch-bump @rocket.chat/ui-contexts and @rocket.chat/meteor. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User (Browser)
participant App as Meteor Client (App)
participant Prov as AuthenticationProvider
participant S as Meteor Server
participant I as Iframe Login Page
U->>App: Load Login
App->>I: Render iframe (postMessage listener)
I-->>App: postMessage login-with-token {loginToken}
App->>Prov: loginWithToken(token, callback?)
Prov->>S: Meteor.loginWithToken(token)
alt Token valid
S-->>Prov: Success
Prov-->>App: Promise resolved
App-->>U: Logged in UI
else Token invalid/error
S-->>Prov: Error
Prov-->>App: Promise rejected + callback(error)
App->>I: postMessage login-error
I-->>U: Show "Login failed"
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #36919 +/- ##
===========================================
+ Coverage 66.54% 66.57% +0.02%
===========================================
Files 3344 3344
Lines 114629 114631 +2
Branches 21094 21110 +16
===========================================
+ Hits 76281 76312 +31
+ Misses 35658 35626 -32
- Partials 2690 2693 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/meteor/tests/e2e/fixtures/files/iframe-login.html (2)
7-21: Consider improving iframe communication security.The script uses
window.parent.postMessage(..., '*')which allows any parent window to receive the message. For better security, consider specifying the expected parent origin instead of using the wildcard'*'.- window.parent.postMessage({ - event: 'login-with-token', - loginToken: 'REPLACE_WITH_TOKEN', - }, '*'); + // Replace '*' with the expected parent origin in production + window.parent.postMessage({ + event: 'login-with-token', + loginToken: 'REPLACE_WITH_TOKEN', + }, window.location.ancestorOrigins?.[0] || '*');
31-33: Fix formatting issue in button element.There's an inconsistent line break in the button element that affects code readability.
- <button id="submit" type="button" - - onclick="login()">Login</button> + <button id="submit" type="button" onclick="login()">Login</button>apps/meteor/tests/e2e/iframe-authentication.spec.ts (1)
39-41: Consider using path.join for better cross-platform compatibility.The current file path resolution works but could be improved for better cross-platform support.
- const htmlContent = fs - .readFileSync(path.resolve(__dirname, 'fixtures/files/iframe-login.html'), 'utf-8') - .replace('REPLACE_WITH_TOKEN', Users.user1.data.loginToken); + const htmlContent = fs + .readFileSync(path.join(__dirname, 'fixtures', 'files', 'iframe-login.html'), 'utf-8') + .replace('REPLACE_WITH_TOKEN', Users.user1.data.loginToken);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.changeset/rich-parrots-lie.md(1 hunks)apps/meteor/client/hooks/iframe/useIframe.ts(1 hunks)apps/meteor/client/providers/AuthenticationProvider/AuthenticationProvider.tsx(1 hunks)apps/meteor/tests/e2e/fixtures/files/iframe-login.html(1 hunks)apps/meteor/tests/e2e/iframe-authentication.spec.ts(1 hunks)apps/meteor/tests/e2e/page-objects/auth.ts(2 hunks)packages/ui-contexts/src/AuthenticationContext.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/meteor/client/hooks/iframe/useIframe.ts (1)
packages/livechat/src/widget.ts (1)
callback(507-509)
apps/meteor/tests/e2e/iframe-authentication.spec.ts (2)
apps/meteor/tests/e2e/page-objects/auth.ts (1)
Registration(3-97)apps/meteor/tests/e2e/page-objects/utils.ts (1)
Utils(3-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 📦 Meteor Build - coverage
🔇 Additional comments (8)
.changeset/rich-parrots-lie.md (1)
1-7: LGTM!The changeset entry correctly documents the patches to both affected packages and provides a clear description of the feature enhancement.
apps/meteor/client/hooks/iframe/useIframe.ts (1)
24-26: LGTM!The change correctly forwards the callback to
tokenLogin, enabling error handling for iframe authentication failures. This aligns with the PR objective to show the iframe auth page when token login fails.packages/ui-contexts/src/AuthenticationContext.ts (1)
12-12: LGTM!The signature extension to include an optional callback parameter is well-defined and maintains backward compatibility. This enables proper error handling in the iframe authentication flow.
apps/meteor/client/providers/AuthenticationProvider/AuthenticationProvider.tsx (1)
44-54: LGTM!The implementation correctly adds callback support while maintaining the existing Promise-based behavior. The error handling appropriately logs the error and invokes the callback before rejecting, which allows for proper iframe authentication fallback.
apps/meteor/tests/e2e/page-objects/auth.ts (1)
82-96: LGTM!The new iframe-related page object getters are well-structured and follow consistent naming conventions. They provide the necessary selectors for testing iframe authentication functionality.
apps/meteor/tests/e2e/iframe-authentication.spec.ts (3)
15-27: LGTM!The test setup properly configures iframe authentication settings in
beforeAlland cleans them up inafterAll. This ensures test isolation and proper cleanup.
52-124: Excellent test coverage!The test suite comprehensively covers all the key scenarios for iframe authentication:
- Basic iframe rendering
- API error fallback
- Successful token login
- Invalid token handling
- End-to-end iframe login flow
- Error display in iframe
The tests are well-structured and validate the expected behavior described in the PR objectives.
135-149: LGTM!The incomplete settings test properly validates that the default login page is shown when iframe configuration is incomplete, ensuring graceful degradation.
MartinSchoeler
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
|
/backport 7.10.2 |
|
7.10.2 already exists in the project |
|
/backport 7.10.3 |
|
Pull request #37291 added to Project: "Patch 7.10.3" |
Proposed changes (including videos or screenshots)
Fixes an issue related to iframe authentication where default login page was loading when an invalid login token was returned from the API.
This was happening because we were not handling token rejection properly in this case of
loginWithToken.Also adds end to end tests for iframe authentication feature
Issue(s)
Steps to test or reproduce
Further comments
CORE-1332
CORE-1346
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores