fix(cas): enhance CAS login popup handling and add tests#40196
Open
soumajitgh wants to merge 5 commits into
Open
fix(cas): enhance CAS login popup handling and add tests#40196soumajitgh wants to merge 5 commits into
soumajitgh wants to merge 5 commits into
Conversation
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a postMessage-based completion signal, a 120s timeout, and centralized cleanup to the CAS popup login flow; server middleware now posts a completion message from the popup. Tests cover message-based success, token/origin validation, popup-close resolution, and timeout rejection. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (openCASLoginPopup)
participant Popup as Popup Window
participant CAS as CAS Server
Client->>Popup: window.open(CAS URL)
Popup->>CAS: Redirect to CAS auth
CAS->>Popup: Authenticate & redirect to callback
Popup->>Client: postMessage({type: 'cas-login-complete', credentialToken}) -- restricted origin
Client->>Client: Validate origin & token
Client->>Popup: focus()
Client->>Popup: close()
alt No postMessage but popup closes
Client->>Client: Poll popup.closed -> resolve
else No postMessage and popup remains open
Client->>Client: 120s timeout -> close() -> reject
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
🧹 Nitpick comments (1)
apps/meteor/client/lib/openCASLoginPopup.ts (1)
113-120: Consider routing the interval resolution throughresolveLoginfor symmetry.The popup-closed branch calls
cleanup()+resolve()directly, bypassing thesettledguard used byresolveLogin/rejectLogin. It's currently safe becausecleanup()clears the timeout and removes the message listener before they can fire again, andclosePopupis intentionally skipped since the popup is already closed. Still, funneling all terminal paths through a single helper makes the settled-flag invariant harder to accidentally break in future edits.♻️ Proposed refactor
- const resolveLogin = () => { + const resolveLogin = (shouldClosePopup = true) => { if (settled) { return; } settled = true; cleanup(); - closePopup(popup); + if (shouldClosePopup) { + closePopup(popup); + } resolve(); }; @@ const checkPopupOpen = setInterval(() => { if (popup.closed || popup.closed === undefined) { - cleanup(); - resolve(); + resolveLogin(false); } }, CAS_LOGIN_POPUP_POLL_INTERVAL_MS);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/lib/openCASLoginPopup.ts` around lines 113 - 120, The popup-closed branch in the checkPopupOpen interval bypasses the settled-guarded path by calling cleanup() + resolve() directly; instead, route that branch through the existing resolveLogin helper so the settled flag and common teardown (resolveLogin/rejectLogin) remain authoritative. Update the checkPopupOpen callback to call resolveLogin() (and avoid calling closePopup since popup is already closed), remove the direct resolve() there, and ensure resolveLogin still performs cleanup() and any needed closePopup logic only when appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/client/lib/openCASLoginPopup.ts`:
- Around line 113-120: The popup-closed branch in the checkPopupOpen interval
bypasses the settled-guarded path by calling cleanup() + resolve() directly;
instead, route that branch through the existing resolveLogin helper so the
settled flag and common teardown (resolveLogin/rejectLogin) remain
authoritative. Update the checkPopupOpen callback to call resolveLogin() (and
avoid calling closePopup since popup is already closed), remove the direct
resolve() there, and ensure resolveLogin still performs cleanup() and any needed
closePopup logic only when appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dd7ebcdb-cbd2-4c88-a204-727f47983f0c
📒 Files selected for processing (3)
apps/meteor/client/lib/openCASLoginPopup.spec.tsapps/meteor/client/lib/openCASLoginPopup.tsapps/meteor/server/lib/cas/middleware.ts
📜 Review details
⏰ 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). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/server/lib/cas/middleware.tsapps/meteor/client/lib/openCASLoginPopup.tsapps/meteor/client/lib/openCASLoginPopup.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
apps/meteor/client/lib/openCASLoginPopup.spec.ts
🧠 Learnings (20)
📓 Common learnings
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
📚 Learning: 2026-03-14T14:58:58.834Z
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Applied to files:
apps/meteor/server/lib/cas/middleware.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/server/lib/cas/middleware.tsapps/meteor/client/lib/openCASLoginPopup.tsapps/meteor/client/lib/openCASLoginPopup.spec.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/server/lib/cas/middleware.tsapps/meteor/client/lib/openCASLoginPopup.tsapps/meteor/client/lib/openCASLoginPopup.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `page.waitFor()` with specific conditions instead of hardcoded timeouts in Playwright tests
Applied to files:
apps/meteor/client/lib/openCASLoginPopup.tsapps/meteor/client/lib/openCASLoginPopup.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Implement proper wait strategies for dynamic content in Playwright tests
Applied to files:
apps/meteor/client/lib/openCASLoginPopup.tsapps/meteor/client/lib/openCASLoginPopup.spec.ts
📚 Learning: 2026-02-10T16:32:42.586Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38528
File: apps/meteor/client/startup/roles.ts:14-14
Timestamp: 2026-02-10T16:32:42.586Z
Learning: In Rocket.Chat's Meteor client code, DDP streams use EJSON and Date fields arrive as Date objects; do not manually construct new Date() in stream handlers (for example, in sdk.stream()). Only REST API responses return plain JSON where dates are strings, so implement explicit conversion there if needed. Apply this guidance to all TypeScript files under apps/meteor/client to ensure consistent date handling in DDP streams and REST responses.
Applied to files:
apps/meteor/client/lib/openCASLoginPopup.tsapps/meteor/client/lib/openCASLoginPopup.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/client/lib/openCASLoginPopup.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/client/lib/openCASLoginPopup.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/client/lib/openCASLoginPopup.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/client/lib/openCASLoginPopup.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/client/lib/openCASLoginPopup.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
apps/meteor/client/lib/openCASLoginPopup.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Prefer web-first assertions (`toBeVisible`, `toHaveText`, etc.) in Playwright tests
Applied to files:
apps/meteor/client/lib/openCASLoginPopup.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/client/lib/openCASLoginPopup.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure clean state for each test execution in Playwright tests
Applied to files:
apps/meteor/client/lib/openCASLoginPopup.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/client/lib/openCASLoginPopup.spec.ts
📚 Learning: 2025-12-16T17:29:45.163Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37834
File: apps/meteor/tests/e2e/page-objects/fragments/admin-flextab-emoji.ts:12-22
Timestamp: 2025-12-16T17:29:45.163Z
Learning: In page object files under `apps/meteor/tests/e2e/page-objects/`, always import `expect` from `../../utils/test` (Playwright's async expect), not from Jest. Jest's `expect` has a synchronous signature and will cause TypeScript errors when used with web-first assertions like `toBeVisible()`.
Applied to files:
apps/meteor/client/lib/openCASLoginPopup.spec.ts
📚 Learning: 2026-02-24T19:22:48.358Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/omnichannel/omnichannel-send-pdf-transcript.spec.ts:66-67
Timestamp: 2026-02-24T19:22:48.358Z
Learning: In Playwright end-to-end tests (e.g., under apps/meteor/tests/e2e/...), prefer locating elements by translated text (getByText) and ARIA roles (getByRole) over data-qa attributes. If translation values change, update the corresponding test locators accordingly. Never use data-qa locators. This guideline applies to all Playwright e2e test specs in the repository and helps keep tests robust to UI text changes and accessible semantics.
Applied to files:
apps/meteor/client/lib/openCASLoginPopup.spec.ts
📚 Learning: 2026-03-06T18:10:15.268Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:15.268Z
Learning: In tests (especially those using testing-library/dom/jsdom) for Rocket.Chat components, the HTML <code> element has an implicit ARIA role of 'code'. Therefore, screen.getByRole('code') or screen.findByRole('code') will locate <code> elements even without a role attribute. Do not flag findByRole('code') as invalid in reviews; prefer using the implicit role instead of adding role="code" unless necessary for accessibility.
Applied to files:
apps/meteor/client/lib/openCASLoginPopup.spec.ts
🔇 Additional comments (2)
apps/meteor/server/lib/cas/middleware.ts (1)
13-22: Completion signal wiring looks correct.The inline script posts
{ type: 'cas-login-complete', credentialToken }to the opener withwindow.location.originas the target origin before callingwindow.close(), which matches the client-side listener contract inopenCASLoginPopup.ts. Since the popup is served from the app origin, the opener'sevent.origin === appOrigincheck will pass. If a strict CSP blocks the inline script, the existingpopup.closedpolling fallback on the client still covers the completion path.apps/meteor/client/lib/openCASLoginPopup.spec.ts (1)
1-120: Solid coverage of the new completion paths.All four branches (matching message resolve, mismatched token ignored + popup-closed resolve, timeout reject, foreign-origin ignored) are exercised with clean isolation via
beforeEach/afterEach, fake timers, and a controllable popup mock. Test names are descriptive and assertions correctly distinguish the close-invocation differences between the message-resolve path and the interval-resolve path.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
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.
Proposed changes (including videos or screenshots)
This PR resolves a long-standing reliability issue where the CAS login process could hang indefinitely if the authentication popup failed to close automatically.
Key Changes:
apps/meteor/client/lib/openCASLoginPopup.ts. If the login doesn't settle within this window, the promise rejects, preventing the UI from hanging forever.postMessageCommunication: Added a robust message-passing mechanism. The CAS callback page (apps/meteor/server/lib/cas/middleware.ts) now explicitly signals completion to the opener window before attempting to close.credentialTokento prevent cross-session interference.popup.closedpolling as a fallback for environments wherepostMessagemight be restricted.cleanuproutine to clear intervals, timeouts, and event listeners as soon as the login settles (resolves or rejects).Issue(s)
Closes #40153
Steps to test or reproduce
While local automated execution was partially blocked by environment issues (see "Further comments"), the logic was manually verified after the final cleanup. To test:
postMessage).Further comments
postMessagevalidation must check against the app origin, not the CAS server origin, because the popup sends the message from the redirected callback page hosted on the Rocket.Chat instance.@rocket.chat/jest-presets/client.resolver/tsconfigworkspace errors.tscfails on pre-existing unrelated files.apps/meteor/client/lib/openCASLoginPopup.spec.tswith targeted test cases for success, origin/token validation, and timeout rejection to ensure long-term coverage.Since you're dealing with those
@rocket.chat/jest-presets/clientresolution errors, would you like me to help you debug yourpackage.jsonortsconfigsettings to get those tests running properly?Summary by CodeRabbit
Bug Fixes
Tests