feat(supportedVersions): role-targeted expiration messages - #3371
Conversation
Add an optional `roles` field to supportedVersions `Message`. When set, an expiration message is only shown to users whose roles intersect the list; when omitted (or empty) the message is shown to everyone, so older clients and existing payloads are unaffected. To evaluate targeting, capture the logged-in user's roles from the workspace REST API (/api/v1/me) on login and store them on the server state. Any failure leaves roles unset, which falls back to showing the message to all users — the pre-existing behavior. This lets the signed payload restrict version warnings to admins (the only ones who can act on them) without a desktop release for each targeting change.
WalkthroughAdds role-based targeting for server version expiration messages. A new preloader module supports role sourcing from a bridge API (with ChangesRole-based expiration message targeting
Sequence Diagram(s)sequenceDiagram
participant setUserLoggedIn as setUserLoggedIn (preload)
participant userRoles as userRoles.ts
participant localStorage
participant API as Workspace /api/v1/me
participant store as Redux store
participant reducer as servers reducer
participant main as isServerVersionSupported
setUserLoggedIn->>userRoles: updateUserRoles() [on login]
userRoles->>localStorage: read Meteor.loginToken, Meteor.userId
userRoles->>API: GET /api/v1/me (X-Auth-Token, X-User-Id)
API-->>userRoles: { roles: string[] }
userRoles->>store: dispatch WEBVIEW_USER_ROLES_CHANGED({ url, userRoles })
store->>reducer: WEBVIEW_USER_ROLES_CHANGED
reducer-->>store: upsert server.userRoles
main->>main: getExpirationMessage({ messages, userRoles: server.userRoles })
main->>main: messageMatchesUserRoles — filter by role intersection
main-->>main: first eligible message or undefined
setUserLoggedIn->>userRoles: clearUserRoles() [on logout]
userRoles->>store: dispatch WEBVIEW_USER_ROLES_CHANGED({ url, userRoles: undefined })
store->>reducer: WEBVIEW_USER_ROLES_CHANGED
reducer-->>store: upsert server.userRoles to undefined
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/servers/preload/userRoles.ts`:
- Around line 23-24: The comment describing the fallback behavior for unset
roles in the userRoles.ts file incorrectly states that role-targeted messages
fall back to being shown to everyone. However, the actual runtime filtering
logic in messageMatchesUserRoles (located in
src/servers/supportedVersions/main.ts) hides targeted messages when userRoles is
unknown rather than showing them to everyone. Update the comment to accurately
reflect the true behavior: when roles are unset or unknown, role-targeted
messages are hidden from users, not displayed to everyone.
- Around line 26-50: The updateUserRoles function can persist stale role data
when async responses complete after the authentication context changes (logout
or account switch). After the fetch completes in updateUserRoles, re-validate
that the current authToken and userId from localStorage still match the values
used to initiate the fetch. If they do not match, this indicates the user has
logged out or switched accounts, and you should call dispatchUserRoles with an
empty array to clear stale roles instead of persisting the fetched roles.
Additionally, ensure that failed fetch responses (when response.ok is false)
also clear the existing roles by dispatching an empty array rather than silently
returning and leaving stale data in place.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 033ec533-ff9a-4b59-8134-39016d82cea2
📒 Files selected for processing (8)
src/servers/common.tssrc/servers/preload/userLoggedIn.tssrc/servers/preload/userRoles.tssrc/servers/reducers.tssrc/servers/supportedVersions/main.main.spec.tssrc/servers/supportedVersions/main.tssrc/servers/supportedVersions/types.tssrc/ui/actions.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: check (ubuntu-latest)
- GitHub Check: check (macos-latest)
- GitHub Check: check (windows-latest)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use TypeScript for all new code unless explicitly told otherwise
Use optional chaining with fallbacks for platform-specific APIs instead of mocking when possible. Example:const uid = process.getuid?.() ?? 1000;
Files:
src/servers/supportedVersions/types.tssrc/servers/common.tssrc/servers/preload/userLoggedIn.tssrc/servers/reducers.tssrc/servers/supportedVersions/main.main.spec.tssrc/servers/preload/userRoles.tssrc/servers/supportedVersions/main.tssrc/ui/actions.ts
**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{tsx,ts}: MANDATORY: Use Fuselage components for all UI work. Only create custom components when Fuselage doesn't provide what's needed
Import UI components from@rocket.chat/fuselageand checkTheme.d.tsfor valid color tokens
Use React functional components with hooks
Use PascalCase for component file names
Files:
src/servers/supportedVersions/types.tssrc/servers/common.tssrc/servers/preload/userLoggedIn.tssrc/servers/reducers.tssrc/servers/supportedVersions/main.main.spec.tssrc/servers/preload/userRoles.tssrc/servers/supportedVersions/main.tssrc/ui/actions.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Redux actions must follow FSA (Flux Standard Action) pattern
Avoid unnecessary comments — write self-documenting code through clear naming
Always verify libraries by checking official docs and.d.tsfiles innode_modules/. Never assume props, tokens, or APIs work without verification
Avoid subjective descriptors ('smart', 'excellent', 'dumb') in documentation and comments
Use measurable descriptions in code documentation: 'reduced memory usage', 'improved by X%' instead of subjective claims
NEVER invent metrics — don't include estimated time spent or speculated user counts. Only include numbers from actual logs, error messages, or documented sources
Files:
src/servers/supportedVersions/types.tssrc/servers/common.tssrc/servers/preload/userLoggedIn.tssrc/servers/reducers.tssrc/servers/supportedVersions/main.main.spec.tssrc/servers/preload/userRoles.tssrc/servers/supportedVersions/main.tssrc/ui/actions.ts
**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
*.spec.tsfile naming for Renderer process tests
Files:
src/servers/supportedVersions/main.main.spec.ts
**/*.main.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
*.main.spec.tsfile naming for Main process tests
Files:
src/servers/supportedVersions/main.main.spec.ts
**/*.{spec.ts,main.spec.ts}
📄 CodeRabbit inference engine (CLAUDE.md)
Only mock platform-specific APIs when defensive coding isn't possible. Linux-only APIs requiring mocks:
process.getuid(),process.getgid(),process.geteuid(),process.getegid()
Files:
src/servers/supportedVersions/main.main.spec.ts
🔇 Additional comments (8)
src/servers/supportedVersions/main.ts (2)
187-231: LGTM!
386-386: LGTM!Also applies to: 412-412, 433-433
src/servers/supportedVersions/main.main.spec.ts (1)
914-978: LGTM!src/servers/common.ts (1)
20-20: LGTM!src/servers/supportedVersions/types.ts (1)
11-18: LGTM!src/ui/actions.ts (1)
85-85: LGTM!Also applies to: 238-241
src/servers/preload/userLoggedIn.ts (1)
5-5: LGTM!Also applies to: 16-20
src/servers/reducers.ts (1)
19-19: LGTM!Also applies to: 60-60, 200-204
| * roles unset, which makes role-targeted messages fall back to being shown to | ||
| * everyone — the pre-existing behavior. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the fallback-behavior comment to match actual role filtering.
The comment says unset roles make role-targeted messages visible to everyone, but runtime filtering (src/servers/supportedVersions/main.ts, messageMatchesUserRoles) hides targeted messages when userRoles is unknown. This mismatch can lead to incorrect follow-up changes.
🤖 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 `@src/servers/preload/userRoles.ts` around lines 23 - 24, The comment
describing the fallback behavior for unset roles in the userRoles.ts file
incorrectly states that role-targeted messages fall back to being shown to
everyone. However, the actual runtime filtering logic in messageMatchesUserRoles
(located in src/servers/supportedVersions/main.ts) hides targeted messages when
userRoles is unknown rather than showing them to everyone. Update the comment to
accurately reflect the true behavior: when roles are unset or unknown,
role-targeted messages are hidden from users, not displayed to everyone.
| export const updateUserRoles = async (): Promise<void> => { | ||
| try { | ||
| const serverUrl = getServerUrl(); | ||
| if (!serverUrl) return; | ||
|
|
||
| const authToken = localStorage.getItem('Meteor.loginToken'); | ||
| const userId = localStorage.getItem('Meteor.userId'); | ||
| if (!authToken || !userId) return; | ||
|
|
||
| const response = await fetch(buildMeEndpoint(serverUrl), { | ||
| headers: { | ||
| 'X-Auth-Token': authToken, | ||
| 'X-User-Id': userId, | ||
| }, | ||
| }); | ||
| if (!response.ok) return; | ||
|
|
||
| const data = await response.json(); | ||
| const roles: unknown = data?.roles; | ||
| if (!Array.isArray(roles)) return; | ||
|
|
||
| const userRoles = roles.filter( | ||
| (role): role is string => typeof role === 'string' | ||
| ); | ||
| dispatchUserRoles(userRoles); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Prevent stale roles from being persisted across failed or outdated fetches.
updateUserRoles can keep/apply stale role data: failed refresh paths never clear existing roles, and late async responses can overwrite state after auth context changes (logout/account switch). Guard the response against current auth context and clear stale roles before refresh.
💡 Suggested fix
export const updateUserRoles = async (): Promise<void> => {
try {
const serverUrl = getServerUrl();
- if (!serverUrl) return;
+ if (!serverUrl) {
+ dispatchUserRoles(undefined);
+ return;
+ }
const authToken = localStorage.getItem('Meteor.loginToken');
const userId = localStorage.getItem('Meteor.userId');
- if (!authToken || !userId) return;
+ if (!authToken || !userId) {
+ dispatchUserRoles(undefined);
+ return;
+ }
+
+ // Avoid reusing stale roles while refresh is in flight.
+ dispatchUserRoles(undefined);
const response = await fetch(buildMeEndpoint(serverUrl), {
headers: {
'X-Auth-Token': authToken,
'X-User-Id': userId,
},
});
if (!response.ok) return;
const data = await response.json();
const roles: unknown = data?.roles;
if (!Array.isArray(roles)) return;
const userRoles = roles.filter(
(role): role is string => typeof role === 'string'
);
+
+ // Drop stale responses if auth context changed while awaiting network/JSON.
+ if (
+ localStorage.getItem('Meteor.loginToken') !== authToken ||
+ localStorage.getItem('Meteor.userId') !== userId
+ ) {
+ return;
+ }
+
dispatchUserRoles(userRoles);
} catch {🤖 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 `@src/servers/preload/userRoles.ts` around lines 26 - 50, The updateUserRoles
function can persist stale role data when async responses complete after the
authentication context changes (logout or account switch). After the fetch
completes in updateUserRoles, re-validate that the current authToken and userId
from localStorage still match the values used to initiate the fetch. If they do
not match, this indicates the user has logged out or switched accounts, and you
should call dispatchUserRoles with an empty array to clear stale roles instead
of persisting the fetched roles. Additionally, ensure that failed fetch
responses (when response.ok is false) also clear the existing roles by
dispatching an empty array rather than silently returning and leaving stale data
in place.
Make role acquisition hybrid. Expose RocketChatDesktop.setUserRoles so the web client can push the logged-in user's roles reactively (no token handling, propagates on role changes). The /api/v1/me fetch becomes a fallback used only when the bridge hasn't provided roles, covering web clients that predate the bridge call. Bridge-provided roles always take precedence over the fetch.
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 (1)
src/servers/preload/userRoles.ts (1)
65-76: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPreserve bridge precedence after JSON parsing.
Line 67 checks
rolesProvidedByBridgebeforeawait response.json(), but bridge roles can arrive during JSON parsing. Then Line 76 can still dispatch REST roles and overwrite authoritative bridge roles.Suggested fix
- // The bridge may have resolved while the request was in flight; if so, keep - // the authoritative value. - if (rolesProvidedByBridge) return; - - const data = await response.json(); + // The bridge may resolve at any point while this async path is running. + if (rolesProvidedByBridge) return; + const data = await response.json(); + if (rolesProvidedByBridge) return; const roles: unknown = data?.roles; if (!Array.isArray(roles)) return; const userRoles = roles.filter( (role): role is string => typeof role === 'string' ); + if (rolesProvidedByBridge) return; dispatchUserRoles(userRoles);🤖 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 `@src/servers/preload/userRoles.ts` around lines 65 - 76, The rolesProvidedByBridge flag is checked at the beginning before JSON parsing, but since bridge roles can arrive asynchronously during the await response.json() call, the flag can become true before the dispatchUserRoles(userRoles) call at the end. Add a second check for rolesProvidedByBridge right before calling dispatchUserRoles to ensure that if bridge roles arrived during JSON parsing, the REST roles are not dispatched and do not overwrite the authoritative bridge values.
🧹 Nitpick comments (1)
src/servers/preload/userRoles.spec.ts (1)
78-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an explicit in-flight precedence test (fetch started before bridge update).
Current coverage checks pre-fetch bridge precedence, but not the race where bridge roles arrive while
/api/v1/meis still pending. A dedicated deferred-fetch test would lock this contract down.♻️ Suggested test addition
+ it('does not override bridge roles when bridge updates during an in-flight fetch', async () => { + let resolveFetch!: (value: { + ok: boolean; + json: () => Promise<{ roles: unknown }>; + }) => void; + const fetchPromise = new Promise<{ + ok: boolean; + json: () => Promise<{ roles: unknown }>; + }>((resolve) => { + resolveFetch = resolve; + }); + + global.fetch = jest.fn(() => fetchPromise) as unknown as typeof global.fetch; + + const pendingUpdate = updateUserRoles(); + setUserRoles(['admin']); + resolveFetch({ ok: true, json: async () => ({ roles: ['user'] }) }); + await pendingUpdate; + + expect(dispatchMock).toHaveBeenCalledTimes(1); + expect(dispatchMock).toHaveBeenCalledWith({ + type: WEBVIEW_USER_ROLES_CHANGED, + payload: { url: 'https://rocket.chat', userRoles: ['admin'] }, + }); + });🤖 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 `@src/servers/preload/userRoles.spec.ts` around lines 78 - 87, Add a new test case that verifies the race condition scenario where the bridge provides roles while an in-flight fetch request to /api/v1/me is still pending. Create a test that sets up a deferred fetch promise (that doesn't resolve immediately), calls updateUserRoles() to initiate the fetch, then invokes setUserRoles() to simulate the bridge providing roles while the fetch is still pending, and finally verify that dispatch is not called with fetch results and that the pending fetch promise is properly handled. This test complements the existing test which only checks the scenario where bridge roles arrive before fetch is initiated.
🤖 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 `@src/servers/preload/userRoles.ts`:
- Around line 65-76: The rolesProvidedByBridge flag is checked at the beginning
before JSON parsing, but since bridge roles can arrive asynchronously during the
await response.json() call, the flag can become true before the
dispatchUserRoles(userRoles) call at the end. Add a second check for
rolesProvidedByBridge right before calling dispatchUserRoles to ensure that if
bridge roles arrived during JSON parsing, the REST roles are not dispatched and
do not overwrite the authoritative bridge values.
---
Nitpick comments:
In `@src/servers/preload/userRoles.spec.ts`:
- Around line 78-87: Add a new test case that verifies the race condition
scenario where the bridge provides roles while an in-flight fetch request to
/api/v1/me is still pending. Create a test that sets up a deferred fetch promise
(that doesn't resolve immediately), calls updateUserRoles() to initiate the
fetch, then invokes setUserRoles() to simulate the bridge providing roles while
the fetch is still pending, and finally verify that dispatch is not called with
fetch results and that the pending fetch promise is properly handled. This test
complements the existing test which only checks the scenario where bridge roles
arrive before fetch is initiated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5a95b732-8f9d-46bb-b6bb-02fd0a82ea60
📒 Files selected for processing (3)
src/servers/preload/api.tssrc/servers/preload/userRoles.spec.tssrc/servers/preload/userRoles.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: check (windows-latest)
- GitHub Check: check (ubuntu-latest)
- GitHub Check: check (macos-latest)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use TypeScript for all new code unless explicitly told otherwise
Use optional chaining with fallbacks for platform-specific APIs instead of mocking when possible. Example:const uid = process.getuid?.() ?? 1000;
Files:
src/servers/preload/userRoles.spec.tssrc/servers/preload/userRoles.tssrc/servers/preload/api.ts
**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{tsx,ts}: MANDATORY: Use Fuselage components for all UI work. Only create custom components when Fuselage doesn't provide what's needed
Import UI components from@rocket.chat/fuselageand checkTheme.d.tsfor valid color tokens
Use React functional components with hooks
Use PascalCase for component file names
Files:
src/servers/preload/userRoles.spec.tssrc/servers/preload/userRoles.tssrc/servers/preload/api.ts
**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
*.spec.tsfile naming for Renderer process tests
Files:
src/servers/preload/userRoles.spec.ts
**/*.{spec.ts,main.spec.ts}
📄 CodeRabbit inference engine (CLAUDE.md)
Only mock platform-specific APIs when defensive coding isn't possible. Linux-only APIs requiring mocks:
process.getuid(),process.getgid(),process.geteuid(),process.getegid()
Files:
src/servers/preload/userRoles.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Redux actions must follow FSA (Flux Standard Action) pattern
Avoid unnecessary comments — write self-documenting code through clear naming
Always verify libraries by checking official docs and.d.tsfiles innode_modules/. Never assume props, tokens, or APIs work without verification
Avoid subjective descriptors ('smart', 'excellent', 'dumb') in documentation and comments
Use measurable descriptions in code documentation: 'reduced memory usage', 'improved by X%' instead of subjective claims
NEVER invent metrics — don't include estimated time spent or speculated user counts. Only include numbers from actual logs, error messages, or documented sources
Files:
src/servers/preload/userRoles.spec.tssrc/servers/preload/userRoles.tssrc/servers/preload/api.ts
🪛 ast-grep (0.44.0)
src/servers/preload/userRoles.spec.ts
[warning] 30-30: Do not store sensitive data (credentials, tokens, PII) in localStorage or sessionStorage; it is readable by any script and persists on the device.
Context: localStorage.setItem('Meteor.loginToken', 'token')
Note: [CWE-312] Cleartext Storage of Sensitive Information.
(local-storage-sensitive-data-typescript)
🔇 Additional comments (3)
src/servers/preload/userRoles.spec.ts (1)
1-77: LGTM!Also applies to: 89-97
src/servers/preload/userRoles.ts (1)
42-44: Fallback-behavior comments still conflict with runtime behavior.Already raised in prior review; keeping as duplicate to avoid restating.
Also applies to: 78-80
src/servers/preload/api.ts (1)
43-43: LGTM!Also applies to: 63-63, 92-92
|
Reviewed the desktop side end-to-end against current Looks solid
Notes (non-blocking)
Approving. |
2 participants
Jira: ARCH-2192
Proposal
Add an optional
rolesfield to the supportedVersionsMessageso the signed payload can restrict a version-expiration warning to specific roles (e.g. workspace admins), instead of showing it to every logged-in user.Behavior
Message.rolesomitted or empty → message shown to everyone. Keeps current behavior; clients that predate the field ignore it, so existing payloads are unaffected.Message.rolespresent → message shown only to users whose roles intersect the list. If the client doesn't know the user's roles, the message is not shown (honors the restriction).How roles are known (hybrid)
The desktop had no notion of user role. Roles are now acquired two ways, with a clear precedence:
RocketChatDesktop.setUserRoles(roles). The web client observes its own reactive user and pushes roles. No token handling, no REST, and role changes propagate live./api/v1/meusing the session token already present in the webviewlocalStorage. Covers web clients that predate the bridge call. Skipped once the bridge provides roles; bridge values always win.Any failure leaves roles unset, which makes role-targeted messages fall back to being shown to everyone — the pre-existing behavior.
Why
The expiration warning is only actionable by admins. Targeting it via the signed payload lets us tune who sees the warning without shipping a desktop release for each change.
Changes
Message.roles?: string[]+Server.userRoles?: string[]getExpirationMessage(all three branches: exception, supported version, enforcement)WEBVIEW_USER_ROLES_CHANGEDaction + reducer caseRocketChatDesktop.setUserRolesbridge methodservers/preload/userRoles.ts— bridge push +/api/v1/mefallback, cleared on logoutNotes
supportedVersionspayload is a shared contract (also consumed by mobile, produced by cloud). Therolesfield should be agreed centrally so other clients honor it; this PR is the desktop side. ThesetUserRolesbridge call is the matching web-client change.Verification
tsc --noEmitcleaneslintclean on changed filesjest→ 81 passed (incl. 9 new across two specs)