Skip to content

Commit fb736c4

Browse files
committed
fix(auth): grant view:config to non-admin groups, disable settings fields for read-only users
Root cause for issues #1401, #1410, and #1413: GET /api/config and GET /api/ri-exchange/config both require view:config, but Standard Users and Read-Only Users lacked this permission. The cascade: - #1401: only the section header rendered; the config form was never shown because loadGlobalSettings returned early on 403. - #1410: applyReadOnlySettings was never called, so Purchasing Policies inputs stayed enabled for non-admin sessions. - #1413: a "permission denied" error paragraph appeared inside the Exchange Automation container instead of degrading gracefully. Fix: - internal/auth/types.go: add view:config to DefaultUserPermissions() and DefaultReadOnlyPermissions() (write gate update:config is still admin-only). - migration 000088: SQL UPDATE grants view:config to Standard Users (00000000-...-0005) and Read-Only Users (00000000-...-0006) with an idempotent NOT EXISTS guard; down migration removes it via jsonb_agg. - frontend/src/permissions.generated.ts: regenerated to include view:config in USER_PERMS and READONLY_PERMS. - frontend/src/settings.ts: export isPermissionDeniedError so sibling modules can reuse it without duplicating the detection logic. - frontend/src/riexchange.ts: graceful-degrade on permission denied in loadAutomationSettings (clears container, no error paragraph). Tests: - internal/auth/types_test.go + service_group_test.go: updated counts (11 -> 12 for users, 3 -> 4 for read-only) plus positive view:config and negative update:config assertions. - internal/api/handler_config_test.go: new test asserts updateConfig returns 403 for a session that holds view:config but not update:config. - internal/database/postgres/migrations/000088_..._test.go: integration tests cover grant, write-gate exclusion, sibling preservation, down migration, and idempotency. - frontend/__tests__/permissions.test.ts: updated expected permission sets to include view:config; renamed readonly describe to reflect the 4-permission set. - frontend/__tests__/settings-permissions.test.ts: 6 new tests verify the form is visible (not just the header) after getConfig succeeds for standard and read-only users, and that Purchasing Policies inputs are disabled. - frontend/__tests__/riexchange-automation-settings.test.ts: new file with 5 tests covering permission-denied graceful degradation. Closes #1401 Closes #1410 Closes #1413
1 parent 7f0573d commit fb736c4

13 files changed

Lines changed: 670 additions & 48 deletions

‎frontend/src/__tests__/permissions.test.ts‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,12 +59,15 @@ describe('permissions', () => {
5959
// Issue #1407 (four-eyes): approve-own is intentionally absent from the
6060
// standard user permission set. Self-approval requires an explicit custom
6161
// group grant; ownership alone does not confer the right to approve.
62+
// Issue #1401: view:config added (migration 000088) so non-admin users can
63+
// read settings via GET /api/config and GET /api/ri-exchange/config.
6264
const perms = getRolePermissions('user');
6365
const expected = [
6466
'view:recommendations',
6567
'view:plans',
6668
'view:purchases',
6769
'view:history',
70+
'view:config', // #1401: allows reading settings (not writing)
6871
'create:plans',
6972
'update:plans',
7073
'delete:plans',
@@ -78,17 +81,22 @@ describe('permissions', () => {
7881
expected.forEach((p) => expect(perms.has(p)).toBe(true));
7982
// approve-own must NOT be in the standard user permission set (issue #1407).
8083
expect(perms.has('approve-own:purchases')).toBe(false);
84+
// update:config must NOT be granted; only admins can write settings.
85+
expect(perms.has('update:config')).toBe(false);
8186
expect(perms.size).toBe(expected.length);
8287
});
8388

84-
test('readonly role grants only view on recommendations/plans/history', () => {
89+
test('readonly role grants view on recommendations/plans/history/config', () => {
90+
// Issue #1401: view:config added so read-only users can view settings.
8591
const perms = getRolePermissions('readonly');
8692
expect(perms.has('view:recommendations')).toBe(true);
8793
expect(perms.has('view:plans')).toBe(true);
8894
expect(perms.has('view:history')).toBe(true);
95+
expect(perms.has('view:config')).toBe(true); // #1401 addition
8996
expect(perms.has('view:purchases')).toBe(false);
9097
expect(perms.has('create:plans')).toBe(false);
91-
expect(perms.size).toBe(3);
98+
expect(perms.has('update:config')).toBe(false); // read-only, not write
99+
expect(perms.size).toBe(4);
92100
});
93101

94102
test('unknown role grants no permissions', () => {
Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,149 @@
1+
/**
2+
* loadAutomationSettings permission-gating tests (issue #1413).
3+
*
4+
* GET /api/ri-exchange/config requires view:config. Before migration 000088
5+
* granted view:config to non-admin groups, the handler returned 403 and
6+
* loadAutomationSettings showed an error paragraph ("Failed to load settings:
7+
* permission denied: requires view on config") at the bottom of the Purchasing
8+
* Policies panel. The page should degrade gracefully instead: clear the
9+
* container and return without showing any error.
10+
*
11+
* After the migration non-admin users have view:config, so the 403 no longer
12+
* occurs in normal operation. The graceful-degradation path remains as defense
13+
* in depth for custom roles that do not include view:config.
14+
*/
15+
16+
jest.mock('../api', () => ({
17+
getRIExchangeConfig: jest.fn(),
18+
}));
19+
20+
jest.mock('../settings', () => ({
21+
applyReadOnlySettings: jest.fn(),
22+
isPermissionDeniedError: jest.fn(),
23+
}));
24+
25+
jest.mock('../state', () => ({
26+
getCurrentUser: jest.fn(),
27+
}));
28+
29+
import * as api from '../api';
30+
import * as settings from '../settings';
31+
import { loadAutomationSettings } from '../riexchange';
32+
33+
function makePermissionError(status: number, message: string): Error & { status: number } {
34+
const err = new Error(message) as Error & { status: number };
35+
err.status = status;
36+
return err;
37+
}
38+
39+
const setupContainer = () => {
40+
document.body.replaceChildren();
41+
const container = document.createElement('div');
42+
container.id = 'ri-exchange-automation-settings';
43+
// Simulate the loading paragraph loadAutomationSettings prepends.
44+
const loading = document.createElement('p');
45+
loading.className = 'loading';
46+
loading.textContent = 'Loading settings...';
47+
container.appendChild(loading);
48+
document.body.appendChild(container);
49+
return container;
50+
};
51+
52+
describe('loadAutomationSettings: graceful degradation on permission errors (issue #1413)', () => {
53+
beforeEach(() => {
54+
jest.clearAllMocks();
55+
});
56+
57+
test(
58+
'403 permission-denied: container is cleared, no error paragraph shown',
59+
async () => {
60+
setupContainer();
61+
const permErr = makePermissionError(403, 'permission denied: requires view on config');
62+
(api.getRIExchangeConfig as jest.Mock).mockRejectedValueOnce(permErr);
63+
// isPermissionDeniedError is imported from settings and must return true
64+
// for the graceful-degradation branch to fire.
65+
(settings.isPermissionDeniedError as jest.Mock).mockReturnValue(true);
66+
67+
await loadAutomationSettings();
68+
69+
const container = document.getElementById('ri-exchange-automation-settings');
70+
// No error paragraph — the section should be empty (silently cleared).
71+
const errorP = container?.querySelector('.error');
72+
expect(errorP).toBeNull();
73+
// The container itself must be present in the DOM (not removed).
74+
expect(container).not.toBeNull();
75+
// No child nodes: both the loading paragraph and any error content cleared.
76+
expect(container?.childNodes.length).toBe(0);
77+
},
78+
);
79+
80+
test(
81+
'403 permission-denied: no retry button is shown',
82+
async () => {
83+
setupContainer();
84+
const permErr = makePermissionError(403, 'permission denied: requires view on config');
85+
(api.getRIExchangeConfig as jest.Mock).mockRejectedValueOnce(permErr);
86+
(settings.isPermissionDeniedError as jest.Mock).mockReturnValue(true);
87+
88+
await loadAutomationSettings();
89+
90+
const container = document.getElementById('ri-exchange-automation-settings');
91+
const retryBtn = container?.querySelector('button');
92+
expect(retryBtn).toBeNull();
93+
},
94+
);
95+
96+
test(
97+
'non-permission 403 (e.g. IP block): error paragraph IS shown',
98+
async () => {
99+
setupContainer();
100+
const nonPermErr = makePermissionError(403, 'access blocked by IP policy');
101+
(api.getRIExchangeConfig as jest.Mock).mockRejectedValueOnce(nonPermErr);
102+
// isPermissionDeniedError returns false for non-permission 403s.
103+
(settings.isPermissionDeniedError as jest.Mock).mockReturnValue(false);
104+
105+
await loadAutomationSettings();
106+
107+
const container = document.getElementById('ri-exchange-automation-settings');
108+
const errorP = container?.querySelector('.error');
109+
expect(errorP).not.toBeNull();
110+
expect(errorP?.textContent).toContain('access blocked by IP policy');
111+
},
112+
);
113+
114+
test(
115+
'network error: error paragraph IS shown with message',
116+
async () => {
117+
setupContainer();
118+
(api.getRIExchangeConfig as jest.Mock).mockRejectedValueOnce(
119+
new Error('Failed to fetch'),
120+
);
121+
(settings.isPermissionDeniedError as jest.Mock).mockReturnValue(false);
122+
123+
await loadAutomationSettings();
124+
125+
const container = document.getElementById('ri-exchange-automation-settings');
126+
const errorP = container?.querySelector('.error');
127+
expect(errorP).not.toBeNull();
128+
expect(errorP?.textContent).toContain('Failed to fetch');
129+
},
130+
);
131+
132+
test(
133+
'network error: retry button IS shown',
134+
async () => {
135+
setupContainer();
136+
(api.getRIExchangeConfig as jest.Mock).mockRejectedValueOnce(
137+
new Error('Failed to fetch'),
138+
);
139+
(settings.isPermissionDeniedError as jest.Mock).mockReturnValue(false);
140+
141+
await loadAutomationSettings();
142+
143+
const container = document.getElementById('ri-exchange-automation-settings');
144+
const retryBtn = container?.querySelector('button');
145+
expect(retryBtn).not.toBeNull();
146+
expect(retryBtn?.textContent).toBe('Retry');
147+
},
148+
);
149+
});

0 commit comments

Comments
 (0)