Review fixes for user authorization tab hiding (#3035)

src/Frontend/src/App.vue

@@ -9,16 +9,18 @@ import BackendChecksNotifications from "@/components/BackendChecksNotifications.
 import { storeToRefs } from "pinia";
 import { useAuthStore } from "@/stores/AuthStore";
 import { useUserPermissionsStore } from "@/stores/UserPermissionsStore";
+import { useEnvironmentAndVersionsStore } from "@/stores/EnvironmentAndVersionsStore";
 
 const authStore = useAuthStore();
 const route = useRoute();
 const { isAuthenticated, authEnabled } = storeToRefs(authStore);
 
 const permissionsStore = useUserPermissionsStore();
+const environmentStore = useEnvironmentAndVersionsStore();
 watch(
-  [authEnabled, isAuthenticated],
-  ([enabled, authenticated]) => {
-    if (enabled && authenticated) {
+  [authEnabled, isAuthenticated, () => environmentStore.environment.supportsUserPermissions],
+  ([enabled, authenticated, supported]) => {
+    if (enabled && authenticated && supported) {
       permissionsStore.refresh();
     }
   },

src/Frontend/src/components/PageHeader.vue

@@ -15,22 +15,15 @@ import AuditMenuItem from "./audit/AuditMenuItem.vue";
 import monitoringClient from "@/components/monitoring/monitoringClient";
 import UserProfileMenuItem from "@/components/UserProfileMenuItem.vue";
 import { useAuthStore } from "@/stores/AuthStore";
-import { useUserPermissionsStore, type PermissionsSummary } from "@/stores/UserPermissionsStore";
+import usePermissionGate from "@/composables/usePermissionGate";
 import { storeToRefs } from "pinia";
 
 const isMonitoringEnabled = monitoringClient.isMonitoringEnabled;
 
 const authStore = useAuthStore();
 const { authEnabled, isAuthenticated } = storeToRefs(authStore);
 
-const permissionsStore = useUserPermissionsStore();
-const { summary } = storeToRefs(permissionsStore);
-
-const shouldGate = computed(() => authEnabled.value && isAuthenticated.value && summary.value !== null);
-
-function has(flag: keyof PermissionsSummary): boolean {
-  return !shouldGate.value || summary.value?.[flag] === true;
-}
+const { has } = usePermissionGate();
 
 // prettier-ignore
 const menuItems = computed(
@@ -42,7 +35,7 @@ const menuItems = computed(
   ...(has("failed_messages_read") ? [FailedMessagesMenuItem] : []),
   ...(has("failed_messages_read") ? [CustomChecksMenuItem] : []),
   ...(has("failed_messages_read") ? [EventsMenuItem] : []),
-  ...(has("failed_messages_read") ? [ThroughputMenuItem] : []),
+  ...(has("admin_read") ? [ThroughputMenuItem] : []),
   ConfigurationMenuItem,
   FeedbackButton,
 ]);

src/Frontend/src/components/configuration/UserPermissions.vue

@@ -33,35 +33,35 @@ onMounted(async () => {
                 <tr>
                   <td>Failed Messages</td>
                   <td class="text-center">
-                    <FAIcon :icon="store.summary.failed_messages_read ? faCheck : faTimes" :class="store.summary.failed_messages_read ? 'icon-granted' : 'icon-denied'" />
+                    <FAIcon :icon="store.summary.failed_messages_read ? faCheck : faTimes" :class="store.summary.failed_messages_read ? 'text-success' : 'text-danger'" />
                   </td>
                   <td class="text-center">
-                    <FAIcon :icon="store.summary.failed_messages_write ? faCheck : faTimes" :class="store.summary.failed_messages_write ? 'icon-granted' : 'icon-denied'" />
+                    <FAIcon :icon="store.summary.failed_messages_write ? faCheck : faTimes" :class="store.summary.failed_messages_write ? 'text-success' : 'text-danger'" />
                   </td>
                 </tr>
                 <tr>
                   <td>Auditing</td>
                   <td class="text-center">
-                    <FAIcon :icon="store.summary.auditing_read ? faCheck : faTimes" :class="store.summary.auditing_read ? 'icon-granted' : 'icon-denied'" />
+                    <FAIcon :icon="store.summary.auditing_read ? faCheck : faTimes" :class="store.summary.auditing_read ? 'text-success' : 'text-danger'" />
                   </td>
                   <td class="text-center">—</td>
                 </tr>
                 <tr>
                   <td>Monitoring</td>
                   <td class="text-center">
-                    <FAIcon :icon="store.summary.monitoring_read ? faCheck : faTimes" :class="store.summary.monitoring_read ? 'icon-granted' : 'icon-denied'" />
+                    <FAIcon :icon="store.summary.monitoring_read ? faCheck : faTimes" :class="store.summary.monitoring_read ? 'text-success' : 'text-danger'" />
                   </td>
                   <td class="text-center">
-                    <FAIcon :icon="store.summary.monitoring_write ? faCheck : faTimes" :class="store.summary.monitoring_write ? 'icon-granted' : 'icon-denied'" />
+                    <FAIcon :icon="store.summary.monitoring_write ? faCheck : faTimes" :class="store.summary.monitoring_write ? 'text-success' : 'text-danger'" />
                   </td>
                 </tr>
                 <tr>
                   <td>Admin</td>
                   <td class="text-center">
-                    <FAIcon :icon="store.summary.admin_read ? faCheck : faTimes" :class="store.summary.admin_read ? 'icon-granted' : 'icon-denied'" />
+                    <FAIcon :icon="store.summary.admin_read ? faCheck : faTimes" :class="store.summary.admin_read ? 'text-success' : 'text-danger'" />
                   </td>
                   <td class="text-center">
-                    <FAIcon :icon="store.summary.admin_write ? faCheck : faTimes" :class="store.summary.admin_write ? 'icon-granted' : 'icon-denied'" />
+                    <FAIcon :icon="store.summary.admin_write ? faCheck : faTimes" :class="store.summary.admin_write ? 'text-success' : 'text-danger'" />
                   </td>
                 </tr>
               </tbody>
@@ -92,14 +92,6 @@ onMounted(async () => {
   padding: 10px 16px;
 }
 
-.icon-granted {
-  color: #28a745;
-}
-
-.icon-denied {
-  color: #dc3545;
-}
-
 .user-label {
   color: #666;
   margin-bottom: 12px;

src/Frontend/src/components/serviceControlClient.ts

@@ -29,8 +29,13 @@ class ServiceControlClient {
     }
   }
 
-  public async fetchTypedFromServiceControl<T>(suffix: string, signal?: AbortSignal): Promise<[Response, T]> {
-    const response = await authFetch(`${this.url}${suffix}`, { signal });
+  public fetchTypedFromServiceControl<T>(suffix: string, signal?: AbortSignal): Promise<[Response, T]> {
+    return this.fetchTypedFromUrl<T>(`${this.url}${suffix}`, signal);
+  }
+
+  // Fetch from an absolute URL, e.g. one discovered from the ServiceControl root document.
+  public async fetchTypedFromUrl<T>(url: string, signal?: AbortSignal): Promise<[Response, T]> {
+    const response = await authFetch(url, { signal });
     if (!response.ok) throw new HttpError(response.status, response.statusText ?? "No response");
     const data = await response.json();
 

src/Frontend/src/composables/usePermissionGate.spec.ts

@@ -0,0 +1,75 @@
+import { describe, test, expect, beforeEach } from "vitest";
+import { createTestingPinia } from "@pinia/testing";
+import { setActivePinia } from "pinia";
+import usePermissionGate from "@/composables/usePermissionGate";
+import { useAuthStore } from "@/stores/AuthStore";
+import { useUserPermissionsStore, type PermissionsSummary } from "@/stores/UserPermissionsStore";
+
+const summaryWith = (overrides: Partial<PermissionsSummary> = {}): PermissionsSummary => ({
+  failed_messages_read: false,
+  failed_messages_write: false,
+  auditing_read: false,
+  monitoring_read: false,
+  monitoring_write: false,
+  admin_read: false,
+  admin_write: false,
+  ...overrides,
+});
+
+describe("usePermissionGate", () => {
+  beforeEach(() => {
+    setActivePinia(createTestingPinia({ stubActions: false }));
+  });
+
+  function withState(opts: { authEnabled: boolean; isAuthenticated: boolean; summary: PermissionsSummary | null }) {
+    const auth = useAuthStore();
+    auth.authEnabled = opts.authEnabled;
+    auth.isAuthenticated = opts.isAuthenticated;
+    useUserPermissionsStore().summary = opts.summary;
+    return usePermissionGate();
+  }
+
+  test("fails open (shows everything) when authorization is disabled", () => {
+    const { has, shouldGate } = withState({ authEnabled: false, isAuthenticated: true, summary: summaryWith() });
+
+    expect(shouldGate.value).toBe(false);
+    expect(has("admin_read")).toBe(true);
+    expect(has("failed_messages_read")).toBe(true);
+  });
+
+  test("fails open when the user is not authenticated", () => {
+    const { has, shouldGate } = withState({ authEnabled: true, isAuthenticated: false, summary: summaryWith() });
+
+    expect(shouldGate.value).toBe(false);
+    expect(has("admin_read")).toBe(true);
+  });
+
+  test("fails open while the permission summary has not loaded yet", () => {
+    const { has, shouldGate } = withState({ authEnabled: true, isAuthenticated: true, summary: null });
+
+    expect(shouldGate.value).toBe(false);
+    expect(has("admin_read")).toBe(true);
+  });
+
+  test("gates per flag once enabled, authenticated and loaded", () => {
+    const { has, shouldGate } = withState({ authEnabled: true, isAuthenticated: true, summary: summaryWith({ admin_read: true }) });
+
+    expect(shouldGate.value).toBe(true);
+    expect(has("admin_read")).toBe(true);
+    expect(has("failed_messages_read")).toBe(false);
+  });
+
+  test("reacts to the summary being updated", () => {
+    const auth = useAuthStore();
+    auth.authEnabled = true;
+    auth.isAuthenticated = true;
+    const permissions = useUserPermissionsStore();
+    permissions.summary = summaryWith();
+
+    const { has } = usePermissionGate();
+    expect(has("monitoring_read")).toBe(false);
+
+    permissions.summary = summaryWith({ monitoring_read: true });
+    expect(has("monitoring_read")).toBe(true);
+  });
+});

src/Frontend/src/composables/usePermissionGate.ts

@@ -0,0 +1,22 @@
+import { computed } from "vue";
+import { storeToRefs } from "pinia";
+import { useAuthStore } from "@/stores/AuthStore";
+import { useUserPermissionsStore, type PermissionsSummary } from "@/stores/UserPermissionsStore";
+
+// Centralises the "should this UI element be shown for the current user" decision.
+// Gating only kicks in once authorization is enabled, the user is authenticated and
+// the permission summary has loaded; otherwise everything is shown (fail-open) so the
+// UI is unchanged for unauthenticated/older-ServiceControl setups. Server-side checks
+// remain the real enforcement — this only hides UI the user cannot use anyway.
+export default function usePermissionGate() {
+  const { authEnabled, isAuthenticated } = storeToRefs(useAuthStore());
+  const { summary } = storeToRefs(useUserPermissionsStore());
+
+  const shouldGate = computed(() => authEnabled.value && isAuthenticated.value && summary.value !== null);
+
+  function has(flag: keyof PermissionsSummary): boolean {
+    return !shouldGate.value || summary.value?.[flag] === true;
+  }
+
+  return { has, shouldGate };
+}

src/Frontend/src/stores/EnvironmentAndVersionsStore.ts

@@ -18,6 +18,8 @@ export const useEnvironmentAndVersionsStore = defineStore("EnvironmentAndVersion
     sp_version: window.defaultConfig && window.defaultConfig.version ? window.defaultConfig.version : "1.2.0",
     supportsArchiveGroups: false,
     supportsUserPermissions: false,
+    mypermissions_all_url: "",
+    mypermissions_summary_url: "",
     endpoints_error_url: "",
     known_endpoints_url: "",
     endpoints_message_search_url: "",
@@ -58,6 +60,8 @@ export const useEnvironmentAndVersionsStore = defineStore("EnvironmentAndVersion
     if (scVer) {
       environment.supportsArchiveGroups = !!scVer.archived_groups_url;
       environment.supportsUserPermissions = !!scVer.mypermissions_all && !!scVer.mypermissions_summary;
+      environment.mypermissions_all_url = scVer.mypermissions_all ?? "";
+      environment.mypermissions_summary_url = scVer.mypermissions_summary ?? "";
       environment.is_compatible_with_sc = isSupported(environment.sc_version, environment.minimum_supported_sc_version);
       environment.endpoints_error_url = scVer && scVer.endpoints_error_url;
       environment.known_endpoints_url = scVer && scVer.known_endpoints_url;

src/Frontend/src/stores/UserPermissionsStore.spec.ts

@@ -0,0 +1,89 @@
+import { describe, test, expect, beforeEach, vi } from "vitest";
+import { createTestingPinia } from "@pinia/testing";
+import { setActivePinia } from "pinia";
+
+vi.mock("@/components/serviceControlClient", () => ({
+  default: { fetchTypedFromUrl: vi.fn() },
+}));
+
+import serviceControlClient from "@/components/serviceControlClient";
+import logger from "@/logger";
+import { useUserPermissionsStore } from "@/stores/UserPermissionsStore";
+import { useEnvironmentAndVersionsStore } from "@/stores/EnvironmentAndVersionsStore";
+
+const summaryUrl = "http://localhost/my/permissions";
+const allUrl = "http://localhost/my/permissions/all";
+
+const fetchTypedFromUrl = vi.mocked(serviceControlClient.fetchTypedFromUrl);
+
+const summaryData = { failed_messages_read: true, failed_messages_write: false, auditing_read: true, monitoring_read: false, monitoring_write: false, admin_read: false, admin_write: false };
+const descriptorData = { user: "alice", permissions: ["error:messages:view", "audit:view"] };
+
+function ok<T>(data: T): [Response, T] {
+  return [{} as Response, data];
+}
+
+function setup() {
+  setActivePinia(createTestingPinia({ stubActions: false }));
+  const environment = useEnvironmentAndVersionsStore().environment;
+  environment.mypermissions_summary_url = summaryUrl;
+  environment.mypermissions_all_url = allUrl;
+  return useUserPermissionsStore();
+}
+
+describe("UserPermissionsStore", () => {
+  beforeEach(() => {
+    fetchTypedFromUrl.mockReset();
+  });
+
+  test("refresh fetches the discovered URLs and populates summary and descriptor", async () => {
+    const store = setup();
+    fetchTypedFromUrl.mockImplementation((url: string) => Promise.resolve(url === summaryUrl ? ok(summaryData) : ok(descriptorData)));
+
+    await store.refresh();
+
+    expect(fetchTypedFromUrl).toHaveBeenCalledWith(summaryUrl);
+    expect(fetchTypedFromUrl).toHaveBeenCalledWith(allUrl);
+    expect(store.summary).toEqual(summaryData);
+    expect(store.descriptor).toEqual(descriptorData);
+    expect(store.error).toBeNull();
+    expect(store.loading).toBe(false);
+  });
+
+  test("refresh records an error and logs when a request fails", async () => {
+    const store = setup();
+    const loggerError = vi.spyOn(logger, "error").mockImplementation(() => {});
+    const failure = new Error("boom");
+    fetchTypedFromUrl.mockRejectedValue(failure);
+
+    await store.refresh();
+
+    expect(store.summary).toBeNull();
+    expect(store.error).toBe("Failed to load user permissions");
+    expect(store.loading).toBe(false);
+    expect(loggerError).toHaveBeenCalledWith("Failed to load user permissions", failure);
+    loggerError.mockRestore();
+  });
+
+  test("concurrent refreshes share a single in-flight request", async () => {
+    const store = setup();
+    let resolveSummary: (value: [Response, typeof summaryData]) => void = () => {};
+    const pendingSummary = new Promise<[Response, typeof summaryData]>((resolve) => (resolveSummary = resolve));
+    fetchTypedFromUrl.mockImplementation((url: string) => (url === summaryUrl ? pendingSummary : Promise.resolve(ok(descriptorData))));
+
+    const first = store.refresh();
+    const second = store.refresh();
+
+    // One load in flight => two endpoint calls (summary + descriptor), not four.
+    expect(fetchTypedFromUrl).toHaveBeenCalledTimes(2);
+
+    resolveSummary(ok(summaryData));
+    await Promise.all([first, second]);
+
+    expect(store.summary).toEqual(summaryData);
+
+    // Once settled the in-flight slot is released, so a later refresh fetches again.
+    await store.refresh();
+    expect(fetchTypedFromUrl).toHaveBeenCalledTimes(4);
+  });
+});

src/Frontend/src/stores/UserPermissionsStore.ts

@@ -1,6 +1,8 @@
 import { acceptHMRUpdate, defineStore } from "pinia";
 import { ref } from "vue";
 import serviceControlClient from "@/components/serviceControlClient";
+import { useEnvironmentAndVersionsStore } from "@/stores/EnvironmentAndVersionsStore";
+import logger from "@/logger";
 
 interface PermissionsSummary {
   failed_messages_read: boolean;
@@ -23,17 +25,28 @@ export const useUserPermissionsStore = defineStore("UserPermissionsStore", () =>
   const loading = ref(false);
   const error = ref<string | null>(null);
 
-  async function refresh() {
+  // Multiple callers can request a refresh in the same tick (the App-level watch
+  // and the User Permissions view's onMounted). Share a single in-flight request
+  // so they don't trigger duplicate fetches.
+  let inFlight: Promise<void> | null = null;
+
+  function refresh() {
+    return (inFlight ??= load().finally(() => (inFlight = null)));
+  }
+
+  async function load() {
     loading.value = true;
     error.value = null;
+    const { environment } = useEnvironmentAndVersionsStore();
     try {
       const [summaryResult, descriptorResult] = await Promise.all([
-        serviceControlClient.fetchTypedFromServiceControl<PermissionsSummary>("my/permissions"),
-        serviceControlClient.fetchTypedFromServiceControl<PermissionsDescriptor>("my/permissions/all"),
+        serviceControlClient.fetchTypedFromUrl<PermissionsSummary>(environment.mypermissions_summary_url),
+        serviceControlClient.fetchTypedFromUrl<PermissionsDescriptor>(environment.mypermissions_all_url),
       ]);
       summary.value = summaryResult[1];
       descriptor.value = descriptorResult[1];
-    } catch {
+    } catch (err) {
+      logger.error("Failed to load user permissions", err);
       error.value = "Failed to load user permissions";
     } finally {
       loading.value = false;