Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/servers/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export type Server = {
failed?: boolean;
webContentsId?: number;
userLoggedIn?: boolean;
userRoles?: string[];
gitCommitHash?: string;
allowedRedirects?: string[];
outlookCredentials?: OutlookCredentials;
Expand Down
7 changes: 7 additions & 0 deletions src/servers/preload/userLoggedIn.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { dispatch } from '../../store';
import { WEBVIEW_USER_LOGGED_IN } from '../../ui/actions';
import type { Server } from '../common';
import { getServerUrl } from './urls';
import { clearUserRoles, updateUserRoles } from './userRoles';

export const setUserLoggedIn = (userLoggedIn: Server['userLoggedIn']): void => {
dispatch({
Expand All @@ -11,4 +12,10 @@ export const setUserLoggedIn = (userLoggedIn: Server['userLoggedIn']): void => {
userLoggedIn,
},
});

if (userLoggedIn) {
void updateUserRoles();
} else {
clearUserRoles();
}
};
59 changes: 59 additions & 0 deletions src/servers/preload/userRoles.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
import { dispatch } from '../../store';
import { WEBVIEW_USER_ROLES_CHANGED } from '../../ui/actions';
import type { Server } from '../common';
import { getServerUrl } from './urls';

const dispatchUserRoles = (userRoles: Server['userRoles']): void => {
dispatch({
type: WEBVIEW_USER_ROLES_CHANGED,
payload: {
url: getServerUrl(),
userRoles,
},
});
};

const buildMeEndpoint = (serverUrl: string): string =>
`${serverUrl.replace(/\/$/, '')}/api/v1/me`;

/**
* Reads the logged-in user's roles from the workspace REST API and stores them
* on the server state. Roles are used to decide which supportedVersions
* messages a user is allowed to see (see Message.roles). Any failure leaves
* roles unset, which makes role-targeted messages fall back to being shown to
* everyone — the pre-existing behavior.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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);
Comment on lines +46 to +76

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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.

} catch {
// Network/parse errors are non-fatal: leave roles unset so role-targeted
// messages default to visible.
}
};

export const clearUserRoles = (): void => {
dispatchUserRoles(undefined);
};
7 changes: 7 additions & 0 deletions src/servers/reducers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
WEBVIEW_TITLE_CHANGED,
WEBVIEW_UNREAD_CHANGED,
WEBVIEW_USER_LOGGED_IN,
WEBVIEW_USER_ROLES_CHANGED,
WEBVIEW_FAVICON_CHANGED,
WEBVIEW_DID_START_LOADING,
WEBVIEW_DID_FAIL_LOAD,
Expand Down Expand Up @@ -56,6 +57,7 @@ type ServersActionTypes =
| ActionOf<typeof WEBVIEW_TITLE_CHANGED>
| ActionOf<typeof WEBVIEW_UNREAD_CHANGED>
| ActionOf<typeof WEBVIEW_USER_LOGGED_IN>
| ActionOf<typeof WEBVIEW_USER_ROLES_CHANGED>
| ActionOf<typeof WEBVIEW_ALLOWED_REDIRECTS_CHANGED>
| ActionOf<typeof WEBVIEW_FAVICON_CHANGED>
| ActionOf<typeof APP_SETTINGS_LOADED>
Expand Down Expand Up @@ -195,6 +197,11 @@ export const servers: Reducer<Server[], ServersActionTypes> = (
return upsert(state, { url, userLoggedIn });
}

case WEBVIEW_USER_ROLES_CHANGED: {
const { url, userRoles } = action.payload;
return upsert(state, { url, userRoles });
}

case WEBVIEW_ALLOWED_REDIRECTS_CHANGED: {
const { url, allowedRedirects } = action.payload;
return upsert(state, { url, allowedRedirects });
Expand Down
66 changes: 66 additions & 0 deletions src/servers/supportedVersions/main.main.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -910,6 +910,72 @@ describe('supportedVersions/main.ts', () => {

expect(result.supported).toBe(false);
});

describe('message role targeting', () => {
const futureDate = new Date(Date.now() + 86400000);
const buildSupportedVersions = (roles?: string[]): SupportedVersions =>
({
enforcementStartDate: new Date(Date.now() + 172800000).toISOString(),
timestamp: new Date().toISOString(),
versions: [
{
version: '5.4.0',
expiration: futureDate,
},
],
messages: [
{
remainingDays: 30,
title: 'targeted',
subtitle: 'sub',
description: 'desc',
type: 'info',
...(roles ? { roles } : {}),
params: {},
link: '',
},
],
i18n: { en: {} },
}) as any;

it('shows a message with no roles to every user', async () => {
const result = await isServerVersionSupported(
{ ...mockServer, userRoles: ['user'] } as any,
buildSupportedVersions()
);

expect(result.message?.title).toBe('targeted');
});

it('shows a role-targeted message when the user has the role', async () => {
const result = await isServerVersionSupported(
{ ...mockServer, userRoles: ['admin', 'user'] } as any,
buildSupportedVersions(['admin'])
);

expect(result.message?.title).toBe('targeted');
});

it('hides a role-targeted message from users without the role', async () => {
const result = await isServerVersionSupported(
{ ...mockServer, userRoles: ['user'] } as any,
buildSupportedVersions(['admin'])
);

expect(result.supported).toBe(true);
expect(result.message).toBeUndefined();
});

it('hides a role-targeted message when user roles are unknown', async () => {
const result = await isServerVersionSupported(
mockServer as any,
buildSupportedVersions(['admin'])
);

expect(result.supported).toBe(true);
expect(result.message).toBeUndefined();
});
});
});

describe('Cache and Retry Integration', () => {
Expand Down
26 changes: 25 additions & 1 deletion src/servers/supportedVersions/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -184,12 +184,30 @@ const getUniqueId = async (
}
};

const messageMatchesUserRoles = (
message: Message,
userRoles?: string[]
): boolean => {
// No targeting set on the message → show to everyone (default behavior).
if (!message.roles?.length) {
return true;
}
// Targeting set but we don't know the user's roles → don't show, to honor
// the intent of restricting the message.
if (!userRoles?.length) {
return false;
}
return message.roles.some((role) => userRoles.includes(role));
};

const getExpirationMessage = ({
messages,
expiration,
userRoles,
}: {
messages?: Message[];
expiration?: Date;
userRoles?: string[];
}): Message | undefined => {
if (
!messages?.length ||
Expand All @@ -199,7 +217,10 @@ const getExpirationMessage = ({
) {
return;
}
const sortedMessages = messages.sort(
const eligibleMessages = messages.filter((message) =>
messageMatchesUserRoles(message, userRoles)
);
const sortedMessages = eligibleMessages.sort(
(a, b) => a.remainingDays - b.remainingDays
);
const message = sortedMessages.find(
Expand Down Expand Up @@ -362,6 +383,7 @@ export const isServerVersionSupported = async (
const selectedExpirationMessage = getExpirationMessage({
messages,
expiration: exception.expiration,
userRoles: server.userRoles,
}) as Message;

return {
Expand All @@ -387,6 +409,7 @@ export const isServerVersionSupported = async (
const selectedExpirationMessage = getExpirationMessage({
messages,
expiration: supportedVersion.expiration,
userRoles: server.userRoles,
}) as Message;

return {
Expand All @@ -407,6 +430,7 @@ export const isServerVersionSupported = async (
const selectedExpirationMessage = getExpirationMessage({
messages: supportedVersionsData.messages,
expiration: enforcementStartDate,
userRoles: server.userRoles,
}) as Message;

return {
Expand Down
8 changes: 8 additions & 0 deletions src/servers/supportedVersions/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,14 @@ export type Message = {
subtitle: string;
description: string;
type: 'primary' | 'warning' | 'danger';
/**
* Roles allowed to see this message. When omitted (or empty), the message is
* shown to every user. When present, the message is only shown to users whose
* roles intersect this list (e.g. `["admin"]` targets workspace admins only).
* Clients that predate this field ignore it and keep showing the message to
* everyone, so the field is backward compatible.
*/
roles?: string[];
params: Record<string, unknown> & {
instance_version?: string;
instance_email?: string;
Expand Down
5 changes: 5 additions & 0 deletions src/ui/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ export const WEBVIEW_TITLE_CHANGED = 'webview/title-changed';
export const WEBVIEW_PAGE_TITLE_CHANGED = 'webview/page-title-changed';
export const WEBVIEW_UNREAD_CHANGED = 'webview/unread-changed';
export const WEBVIEW_USER_LOGGED_IN = 'webview/user-loggedin';
export const WEBVIEW_USER_ROLES_CHANGED = 'webview/user-roles-changed';
export const WEBVIEW_ALLOWED_REDIRECTS_CHANGED =
'webview/allowed-redirects-changed';
export const SETTINGS_SET_REPORT_OPT_IN_CHANGED =
Expand Down Expand Up @@ -234,6 +235,10 @@ export type UiActionTypeToPayloadMap = {
url: Server['url'];
userLoggedIn: Server['userLoggedIn'];
};
[WEBVIEW_USER_ROLES_CHANGED]: {
url: Server['url'];
userRoles: Server['userRoles'];
};
[WEBVIEW_GIT_COMMIT_HASH_CHECK]: {
url: Server['url'];
gitCommitHash: Server['gitCommitHash'];
Expand Down
Loading