From e89a55fb06fc54c783e3d1bb9349c39d5440eb5b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 5 Jun 2026 12:51:24 +0200 Subject: [PATCH 1/3] fix(settings): suppress permission-denied banner on Purchasing policies for non-admins (closes #979) When GET /api/config returns HTTP 403 with a "permission denied" error, loadGlobalSettings now returns early instead of rendering the red error banner. The section stays hidden and the page degrades gracefully, matching the existing Exchange Automation hide-for-non-admins pattern. Non-permission 403s, 5xx, and network failures still surface the banner. Four new tests in settings-permissions.test.ts cover: the 403+permission path (no banner), unrelated 403s (banner shown), 500s (banner shown), and network errors (banner shown). --- .../__tests__/settings-permissions.test.ts | 85 ++++++++++++++++++- frontend/src/settings.ts | 13 +++ 2 files changed, 96 insertions(+), 2 deletions(-) diff --git a/frontend/src/__tests__/settings-permissions.test.ts b/frontend/src/__tests__/settings-permissions.test.ts index ee7959ced..fe6363701 100644 --- a/frontend/src/__tests__/settings-permissions.test.ts +++ b/frontend/src/__tests__/settings-permissions.test.ts @@ -1,5 +1,5 @@ /** - * Settings page permission gating for issue #365 and issue #870. + * Settings page permission gating for issue #365, issue #870, and issue #979. * * The /admin/general + /admin/purchasing sub-tabs are reachable for * every signed-in role (only /admin/accounts and /admin/users are @@ -9,8 +9,14 @@ * Issue #870: the Purchasing Policies panel (#purchasing-panel) is a * sibling
outside #global-settings-form. The viewer role must * see all its inputs as disabled and both Save/Reset buttons hidden. + * + * Issue #979: when GET /api/config returns 403 (permission denied), the + * error banner must NOT be shown; the section stays hidden and the page + * degrades gracefully. Non-permission failures (network, 5xx) still + * surface the error banner. */ -import { loadGlobalSettings } from '../settings'; +import { loadGlobalSettings, resetConfigCache } from '../settings'; +import * as api from '../api'; jest.mock('../api', () => ({ getConfig: jest.fn().mockResolvedValue({ @@ -269,3 +275,78 @@ describe('Purchasing Policies panel permission gating (issue #870)', () => { expect(graceGcp.disabled).toBe(true); }); }); + +/** + * Issue #979: graceful degradation when GET /api/config returns a + * permission-denied error for non-admin users on the Purchasing policies page. + */ +describe('Settings config load: graceful degradation on permission errors (issue #979)', () => { + beforeEach(() => { + jest.clearAllMocks(); + setupDom(); + resetConfigCache(); + }); + + function makePermissionError(status: number, message: string): Error & { status: number } { + const err = new Error(message) as Error & { status: number }; + err.status = status; + return err; + } + + test('403 permission-denied error hides loading, keeps form hidden, shows no error banner', async () => { + (api.getConfig as jest.Mock).mockRejectedValueOnce( + makePermissionError(403, 'permission denied: requires view on config'), + ); + + await loadGlobalSettings(); + + const loadingEl = document.getElementById('settings-loading'); + const formEl = document.getElementById('global-settings-form'); + const errorEl = document.getElementById('settings-error'); + + // Loading spinner must be hidden (not spinning forever) + expect(loadingEl?.classList.contains('hidden')).toBe(true); + // Form stays hidden; no partial render + expect(formEl?.classList.contains('hidden')).toBe(true); + // No error banner shown + expect(errorEl?.classList.contains('hidden')).toBe(true); + }); + + test('403 error with message not starting with "permission denied" still shows error banner', async () => { + // A 403 for a different reason (e.g. IP block) should still surface + (api.getConfig as jest.Mock).mockRejectedValueOnce( + makePermissionError(403, 'access blocked by IP policy'), + ); + + await loadGlobalSettings(); + + const errorEl = document.getElementById('settings-error'); + // Banner is shown for non-permission-denied 403s + expect(errorEl?.classList.contains('hidden')).toBe(false); + expect(errorEl?.textContent).toContain('access blocked by IP policy'); + }); + + test('500 server error still shows the error banner', async () => { + (api.getConfig as jest.Mock).mockRejectedValueOnce( + makePermissionError(500, 'internal server error'), + ); + + await loadGlobalSettings(); + + const errorEl = document.getElementById('settings-error'); + expect(errorEl?.classList.contains('hidden')).toBe(false); + expect(errorEl?.textContent).toContain('internal server error'); + }); + + test('network error still shows the error banner', async () => { + (api.getConfig as jest.Mock).mockRejectedValueOnce( + new Error('Failed to fetch'), + ); + + await loadGlobalSettings(); + + const errorEl = document.getElementById('settings-error'); + expect(errorEl?.classList.contains('hidden')).toBe(false); + expect(errorEl?.textContent).toContain('Failed to fetch'); + }); +}); diff --git a/frontend/src/settings.ts b/frontend/src/settings.ts index 3da61a289..7344c84dd 100644 --- a/frontend/src/settings.ts +++ b/frontend/src/settings.ts @@ -2975,6 +2975,19 @@ export async function loadGlobalSettings(): Promise { cachedSourceCloud = data.source_cloud ?? 'aws'; } catch (error) { + // Issue #979: when the backend returns 403 with a "permission denied" + // message the non-admin user already has restricted visibility + // (Exchange Automation etc. are hidden). Silently hide the section + // so the page degrades gracefully instead of showing a red banner. + // All other failures (network errors, 5xx, or unrelated 403s) still + // surface the banner. + const isPermissionDenied = + (error as { status?: number }).status === 403 && + (error instanceof Error && error.message.startsWith('permission denied')); + if (isPermissionDenied) { + if (loadingEl) loadingEl.classList.add('hidden'); + return; + } console.error('Failed to load settings:', error); if (loadingEl) loadingEl.classList.add('hidden'); if (errorEl) { From 9a9f0472a694291685b65b6d6ce349a3cba2b2a6 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 6 Jun 2026 08:01:03 +0200 Subject: [PATCH 2/3] refactor(settings): extract isPermissionDeniedError helper (#981 CR) Pull the inline 403 + "permission denied" check out of the catch block into a small helper so the catch reads as a single intent-revealing line. Behavior unchanged; existing settings-permissions tests cover the contract. --- frontend/src/settings.ts | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/frontend/src/settings.ts b/frontend/src/settings.ts index 7344c84dd..7fa40037e 100644 --- a/frontend/src/settings.ts +++ b/frontend/src/settings.ts @@ -77,6 +77,13 @@ export function setGlobalDefaultsForTest(defaults: { term: number; payment: stri cachedGlobalDefaults = { ...defaults }; } +/** Returns true when an API error signals a 403 permission-denied response. */ +function isPermissionDeniedError(error: unknown): boolean { + return (error as { status?: number }).status === 403 + && error instanceof Error + && error.message.startsWith('permission denied'); +} + // sessionStorage key for the in-progress (unsaved) AWS External ID. Persists // across modal close/reopen cycles within the same session so the operator // can copy the value into AWS, close the modal, and come back to the same @@ -2981,10 +2988,7 @@ export async function loadGlobalSettings(): Promise { // so the page degrades gracefully instead of showing a red banner. // All other failures (network errors, 5xx, or unrelated 403s) still // surface the banner. - const isPermissionDenied = - (error as { status?: number }).status === 403 && - (error instanceof Error && error.message.startsWith('permission denied')); - if (isPermissionDenied) { + if (isPermissionDeniedError(error)) { if (loadingEl) loadingEl.classList.add('hidden'); return; } From ee2ac50e52643283d9549e95daafff6838e2a4cf Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 8 Jun 2026 13:55:25 -0700 Subject: [PATCH 3/3] refactor(settings): narrow Error before reading status in isPermissionDeniedError (#981 CR) Reorder the guard so the `instanceof Error` narrowing precedes the `.status` type assertion, matching the conventional TypeScript idiom (narrow first, then access properties). Behavior is unchanged thanks to short-circuit evaluation; this addresses the CodeRabbit style nitpick. --- frontend/src/settings.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frontend/src/settings.ts b/frontend/src/settings.ts index 7fa40037e..0897bbf72 100644 --- a/frontend/src/settings.ts +++ b/frontend/src/settings.ts @@ -79,8 +79,8 @@ export function setGlobalDefaultsForTest(defaults: { term: number; payment: stri /** Returns true when an API error signals a 403 permission-denied response. */ function isPermissionDeniedError(error: unknown): boolean { - return (error as { status?: number }).status === 403 - && error instanceof Error + return error instanceof Error + && (error as { status?: number }).status === 403 && error.message.startsWith('permission denied'); }