diff --git a/frontend/src/__tests__/permissions.test.ts b/frontend/src/__tests__/permissions.test.ts index 034de5b75..dc598cb1f 100644 --- a/frontend/src/__tests__/permissions.test.ts +++ b/frontend/src/__tests__/permissions.test.ts @@ -59,12 +59,15 @@ describe('permissions', () => { // Issue #1407 (four-eyes): approve-own is intentionally absent from the // standard user permission set. Self-approval requires an explicit custom // group grant; ownership alone does not confer the right to approve. + // Issue #1401: view:config added (migration 000088) so non-admin users can + // read settings via GET /api/config and GET /api/ri-exchange/config. const perms = getRolePermissions('user'); const expected = [ 'view:recommendations', 'view:plans', 'view:purchases', 'view:history', + 'view:config', // #1401: allows reading settings (not writing) 'create:plans', 'update:plans', 'delete:plans', @@ -81,17 +84,22 @@ describe('permissions', () => { expected.forEach((p) => expect(perms.has(p)).toBe(true)); // approve-own must NOT be in the standard user permission set (issue #1407). expect(perms.has('approve-own:purchases')).toBe(false); + // update:config must NOT be granted; only admins can write settings. + expect(perms.has('update:config')).toBe(false); expect(perms.size).toBe(expected.length); }); - test('readonly role grants only view on recommendations/plans/history', () => { + test('readonly role grants view on recommendations/plans/history/config', () => { + // Issue #1401: view:config added so read-only users can view settings. const perms = getRolePermissions('readonly'); expect(perms.has('view:recommendations')).toBe(true); expect(perms.has('view:plans')).toBe(true); expect(perms.has('view:history')).toBe(true); + expect(perms.has('view:config')).toBe(true); // #1401 addition expect(perms.has('view:purchases')).toBe(false); expect(perms.has('create:plans')).toBe(false); - expect(perms.size).toBe(3); + expect(perms.has('update:config')).toBe(false); // read-only, not write + expect(perms.size).toBe(4); }); test('unknown role grants no permissions', () => { diff --git a/frontend/src/__tests__/riexchange-automation-settings.test.ts b/frontend/src/__tests__/riexchange-automation-settings.test.ts new file mode 100644 index 000000000..514368266 --- /dev/null +++ b/frontend/src/__tests__/riexchange-automation-settings.test.ts @@ -0,0 +1,149 @@ +/** + * loadAutomationSettings permission-gating tests (issue #1413). + * + * GET /api/ri-exchange/config requires view:config. Before migration 000088 + * granted view:config to non-admin groups, the handler returned 403 and + * loadAutomationSettings showed an error paragraph ("Failed to load settings: + * permission denied: requires view on config") at the bottom of the Purchasing + * Policies panel. The page should degrade gracefully instead: clear the + * container and return without showing any error. + * + * After the migration non-admin users have view:config, so the 403 no longer + * occurs in normal operation. The graceful-degradation path remains as defense + * in depth for custom roles that do not include view:config. + */ + +jest.mock('../api', () => ({ + getRIExchangeConfig: jest.fn(), +})); + +jest.mock('../settings', () => ({ + applyReadOnlySettings: jest.fn(), + isPermissionDeniedError: jest.fn(), +})); + +jest.mock('../state', () => ({ + getCurrentUser: jest.fn(), +})); + +import * as api from '../api'; +import * as settings from '../settings'; +import { loadAutomationSettings } from '../riexchange'; + +function makePermissionError(status: number, message: string): Error & { status: number } { + const err = new Error(message) as Error & { status: number }; + err.status = status; + return err; +} + +const setupContainer = () => { + document.body.replaceChildren(); + const container = document.createElement('div'); + container.id = 'ri-exchange-automation-settings'; + // Simulate the loading paragraph loadAutomationSettings prepends. + const loading = document.createElement('p'); + loading.className = 'loading'; + loading.textContent = 'Loading settings...'; + container.appendChild(loading); + document.body.appendChild(container); + return container; +}; + +describe('loadAutomationSettings: graceful degradation on permission errors (issue #1413)', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + test( + '403 permission-denied: container is cleared, no error paragraph shown', + async () => { + setupContainer(); + const permErr = makePermissionError(403, 'permission denied: requires view on config'); + (api.getRIExchangeConfig as jest.Mock).mockRejectedValueOnce(permErr); + // isPermissionDeniedError is imported from settings and must return true + // for the graceful-degradation branch to fire. + (settings.isPermissionDeniedError as jest.Mock).mockReturnValue(true); + + await loadAutomationSettings(); + + const container = document.getElementById('ri-exchange-automation-settings'); + // No error paragraph — the section should be empty (silently cleared). + const errorP = container?.querySelector('.error'); + expect(errorP).toBeNull(); + // The container itself must be present in the DOM (not removed). + expect(container).not.toBeNull(); + // No child nodes: both the loading paragraph and any error content cleared. + expect(container?.childNodes.length).toBe(0); + }, + ); + + test( + '403 permission-denied: no retry button is shown', + async () => { + setupContainer(); + const permErr = makePermissionError(403, 'permission denied: requires view on config'); + (api.getRIExchangeConfig as jest.Mock).mockRejectedValueOnce(permErr); + (settings.isPermissionDeniedError as jest.Mock).mockReturnValue(true); + + await loadAutomationSettings(); + + const container = document.getElementById('ri-exchange-automation-settings'); + const retryBtn = container?.querySelector('button'); + expect(retryBtn).toBeNull(); + }, + ); + + test( + 'non-permission 403 (e.g. IP block): error paragraph IS shown', + async () => { + setupContainer(); + const nonPermErr = makePermissionError(403, 'access blocked by IP policy'); + (api.getRIExchangeConfig as jest.Mock).mockRejectedValueOnce(nonPermErr); + // isPermissionDeniedError returns false for non-permission 403s. + (settings.isPermissionDeniedError as jest.Mock).mockReturnValue(false); + + await loadAutomationSettings(); + + const container = document.getElementById('ri-exchange-automation-settings'); + const errorP = container?.querySelector('.error'); + expect(errorP).not.toBeNull(); + expect(errorP?.textContent).toContain('access blocked by IP policy'); + }, + ); + + test( + 'network error: error paragraph IS shown with message', + async () => { + setupContainer(); + (api.getRIExchangeConfig as jest.Mock).mockRejectedValueOnce( + new Error('Failed to fetch'), + ); + (settings.isPermissionDeniedError as jest.Mock).mockReturnValue(false); + + await loadAutomationSettings(); + + const container = document.getElementById('ri-exchange-automation-settings'); + const errorP = container?.querySelector('.error'); + expect(errorP).not.toBeNull(); + expect(errorP?.textContent).toContain('Failed to fetch'); + }, + ); + + test( + 'network error: retry button IS shown', + async () => { + setupContainer(); + (api.getRIExchangeConfig as jest.Mock).mockRejectedValueOnce( + new Error('Failed to fetch'), + ); + (settings.isPermissionDeniedError as jest.Mock).mockReturnValue(false); + + await loadAutomationSettings(); + + const container = document.getElementById('ri-exchange-automation-settings'); + const retryBtn = container?.querySelector('button'); + expect(retryBtn).not.toBeNull(); + expect(retryBtn?.textContent).toBe('Retry'); + }, + ); +}); diff --git a/frontend/src/__tests__/settings-permissions.test.ts b/frontend/src/__tests__/settings-permissions.test.ts index fe6363701..db9175454 100644 --- a/frontend/src/__tests__/settings-permissions.test.ts +++ b/frontend/src/__tests__/settings-permissions.test.ts @@ -1,5 +1,6 @@ /** - * Settings page permission gating for issue #365, issue #870, and issue #979. + * Settings page permission gating for issue #365, issue #870, issue #979, + * issue #1401, and issue #1410. * * The /admin/general + /admin/purchasing sub-tabs are reachable for * every signed-in role (only /admin/accounts and /admin/users are @@ -14,6 +15,16 @@ * 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. + * + * Issue #1401: after migration 000088 grants view:config to non-admin groups, + * GET /api/config succeeds for those users. The settings form (#global-settings- + * form) must become VISIBLE (not just the section header) after a successful + * load, and all its controls must be read-only (disabled, Save/Reset hidden). + * + * Issue #1410: the Purchasing Policies panel's own inputs (term, payment, + * target coverage) live in #purchasing-panel, not in #global-settings-form. + * applyReadOnlySettings queries both independently; after a successful + * getConfig call those controls must be disabled for non-admin sessions. */ import { loadGlobalSettings, resetConfigCache } from '../settings'; import * as api from '../api'; @@ -61,6 +72,11 @@ interface ControlSpec { buttonType?: 'button' | 'submit'; } +// Controls that live inside #global-settings-form (General sub-tab). +// Issue #1410: term/payment/coverage/lookback belong in #purchasing-panel in the +// real HTML; they are added there in setupDom below. Do NOT put them here to +// avoid duplicate IDs and to correctly test that applyReadOnlySettings disables +// them via the #purchasing-panel path (not the #global-settings-form path). const FORM_CONTROLS: ReadonlyArray = [ { tag: 'input', id: 'provider-aws', type: 'checkbox' }, { tag: 'input', id: 'provider-azure', type: 'checkbox' }, @@ -72,30 +88,8 @@ const FORM_CONTROLS: ReadonlyArray = [ id: 'setting-collection-schedule', options: [{ value: 'daily', label: 'Daily' }], }, - { - tag: 'select', - id: 'setting-default-term', - options: [ - { value: '1', label: '1' }, - { value: '3', label: '3', selected: true }, - ], - }, - { - tag: 'select', - id: 'setting-default-payment', - options: [ - { value: 'no-upfront', label: 'No', selected: true }, - { value: 'all-upfront', label: 'All' }, - ], - }, - { tag: 'input', id: 'setting-default-coverage', type: 'number', value: '80' }, { tag: 'input', id: 'setting-notification-days', type: 'number', value: '3' }, { tag: 'input', id: 'setting-recs-stale-hours', type: 'number', value: '24' }, - { - tag: 'select', - id: 'setting-recs-lookback-days', - options: [{ value: '7', label: '7', selected: true }], - }, { tag: 'button', id: 'reset-settings-btn', buttonType: 'button' }, { tag: 'button', id: 'save-settings-btn', buttonType: 'submit' }, ]; @@ -141,13 +135,56 @@ const setupDom = () => { err.className = 'hidden'; // Purchasing Policies panel: a sibling
outside #global-settings-form. - // Only the grace-period inputs (unique to this panel) and the action buttons - // are included here to avoid duplicate IDs with the form above. In the real - // HTML the term/payment/coverage selects also live in this panel; that does - // not affect the read-only gate logic since applyReadOnlySettings queries - // #purchasing-panel independently of #global-settings-form. + // Matches the real HTML structure: term/payment/coverage/lookback live HERE + // (in #purchasing-global-defaults and #purchasing-recommendations-lookback + // fieldsets inside #purchasing-panel), not in #global-settings-form. + // applyReadOnlySettings queries #purchasing-panel independently so these + // controls are disabled via that path. Adding them here verifies issue #1410. const purchasingPanel = document.createElement('section'); purchasingPanel.id = 'purchasing-panel'; + + // Global Purchase Defaults (in real HTML: fieldset#purchasing-global-defaults) + const termSel = document.createElement('select'); + termSel.id = 'setting-default-term'; + for (const v of ['1', '3']) { + const o = document.createElement('option'); + o.value = v; + o.textContent = v + ' Year'; + if (v === '3') o.selected = true; + termSel.appendChild(o); + } + purchasingPanel.appendChild(termSel); + + const paymentSel = document.createElement('select'); + paymentSel.id = 'setting-default-payment'; + for (const v of ['no-upfront', 'all-upfront']) { + const o = document.createElement('option'); + o.value = v; + o.textContent = v; + if (v === 'no-upfront') o.selected = true; + paymentSel.appendChild(o); + } + purchasingPanel.appendChild(paymentSel); + + const coverageInput = document.createElement('input'); + coverageInput.id = 'setting-default-coverage'; + coverageInput.type = 'number'; + coverageInput.value = '80'; + purchasingPanel.appendChild(coverageInput); + + // Recommendations Lookback (in real HTML: fieldset#purchasing-recommendations-lookback) + const lookbackSel = document.createElement('select'); + lookbackSel.id = 'setting-recs-lookback-days'; + for (const v of ['7', '30', '60']) { + const o = document.createElement('option'); + o.value = v; + o.textContent = v + ' days'; + if (v === '7') o.selected = true; + lookbackSel.appendChild(o); + } + purchasingPanel.appendChild(lookbackSel); + + // Grace period inputs (unique to the purchasing panel) for (const id of ['setting-grace-aws', 'setting-grace-azure', 'setting-grace-gcp']) { const inp = document.createElement('input'); inp.id = id; @@ -350,3 +387,117 @@ describe('Settings config load: graceful degradation on permission errors (issue expect(errorEl?.textContent).toContain('Failed to fetch'); }); }); + +/** + * Issue #1401: after migration 000088 grants view:config to Standard Users and + * Read-Only Users, GET /api/config returns 200 for those groups. The settings + * form (#global-settings-form) must be VISIBLE after a successful load — users + * must see populated fields, not just the section header. + * + * Issue #1410: the Purchasing Policies panel's core inputs (Default Term, + * Default Payment Option, Target Coverage) live in #purchasing-panel in the + * real HTML and must be disabled (read-only) for non-admin sessions. + * applyReadOnlySettings queries #purchasing-panel independently of + * #global-settings-form; this test suite verifies that path. + */ +describe('Non-admin settings visibility after view:config grant (issues #1401 and #1410)', () => { + beforeEach(() => { + jest.clearAllMocks(); + resetConfigCache(); + setupDom(); + }); + + test( + '#1401: form (#global-settings-form) is visible (not just header) after ' + + 'successful getConfig for a standard user', + async () => { + mockUser('user'); + await loadGlobalSettings(); + + const formEl = document.getElementById('global-settings-form'); + // The form must not carry the 'hidden' class. Before migration 000088 + // loadGlobalSettings returned early on 403, so the form stayed hidden + // and only the section header was visible. + expect(formEl?.classList.contains('hidden')).toBe(false); + }, + ); + + test( + '#1401: form is visible and notification-email field is populated for ' + + 'a read-only user', + async () => { + mockUser('readonly'); + await loadGlobalSettings(); + + const formEl = document.getElementById('global-settings-form'); + expect(formEl?.classList.contains('hidden')).toBe(false); + // Field is populated from the mocked getConfig response. + const emailInput = document.getElementById('setting-notification-email') as HTMLInputElement; + expect(emailInput.value).toBe('ops@example.com'); + }, + ); + + test( + '#1410: Default Term select (in #purchasing-panel) is disabled for a ' + + 'standard user without write permission', + async () => { + mockUser('user'); + await loadGlobalSettings(); + + // setting-default-term lives in #purchasing-panel in the real HTML. + // applyReadOnlySettings queries the panel independently; before the fix + // loadGlobalSettings returned early on 403 and never called + // applyReadOnlySettings, leaving this select enabled. + const termSel = document.getElementById('setting-default-term') as HTMLSelectElement; + expect(termSel.disabled).toBe(true); + }, + ); + + test( + '#1410: Default Payment select (in #purchasing-panel) is disabled for a ' + + 'read-only user without write permission', + async () => { + mockUser('readonly'); + await loadGlobalSettings(); + + const paymentSel = document.getElementById('setting-default-payment') as HTMLSelectElement; + expect(paymentSel.disabled).toBe(true); + }, + ); + + test( + '#1410: Target Coverage input (in #purchasing-panel) is disabled and ' + + 'Save/Reset purchasing buttons are hidden for a standard user', + async () => { + mockUser('user'); + await loadGlobalSettings(); + + const coverage = document.getElementById('setting-default-coverage') as HTMLInputElement; + expect(coverage.disabled).toBe(true); + + const savePurchBtn = document.getElementById('save-purchasing-btn') as HTMLButtonElement; + const resetPurchBtn = document.getElementById('reset-purchasing-btn') as HTMLButtonElement; + expect(savePurchBtn.hidden).toBe(true); + expect(resetPurchBtn.hidden).toBe(true); + }, + ); + + test( + '#1410: all #purchasing-panel inputs and selects are disabled for a ' + + 'read-only user (full panel sweep)', + async () => { + mockUser('readonly'); + await loadGlobalSettings(); + + const inputs = document.querySelectorAll( + '#purchasing-panel input, #purchasing-panel select', + ); + expect(inputs.length).toBeGreaterThan(0); + inputs.forEach((el) => + expect(el.disabled).toBe( + true, + ), + ); + }, + ); +}); diff --git a/frontend/src/permissions.generated.ts b/frontend/src/permissions.generated.ts index 142989e97..a0276e5a2 100644 --- a/frontend/src/permissions.generated.ts +++ b/frontend/src/permissions.generated.ts @@ -28,6 +28,7 @@ export const USER_PERMS: ReadonlySet = new Set([ 'sell-own:purchases', 'update:plans', 'update:purchases', + 'view:config', 'view:history', 'view:plans', 'view:purchases', @@ -35,6 +36,7 @@ export const USER_PERMS: ReadonlySet = new Set([ ]); export const READONLY_PERMS: ReadonlySet = new Set([ + 'view:config', 'view:history', 'view:plans', 'view:recommendations', diff --git a/frontend/src/riexchange.ts b/frontend/src/riexchange.ts index a4f0772b4..d2a6be84a 100644 --- a/frontend/src/riexchange.ts +++ b/frontend/src/riexchange.ts @@ -28,7 +28,7 @@ import type { import { openModal, closeModal } from './modal'; import { showSkeletonRows, teardownSkeleton } from './lib/skeleton'; import { canAccess } from './permissions'; -import { applyReadOnlySettings } from './settings'; +import { applyReadOnlySettings, isPermissionDeniedError } from './settings'; import { showToast } from './toast'; import { getCurrentUser } from './state'; @@ -1900,6 +1900,16 @@ export async function loadAutomationSettings(): Promise { const config = await api.getRIExchangeConfig(); renderAutomationSettings(container, config); } catch (error) { + // Issue #1413: when the backend returns 403 permission denied the user + // legitimately lacks access to the Exchange Automation section. Hide it + // silently (same graceful-degradation pattern as loadGlobalSettings for + // issue #979) instead of showing an error paragraph at the bottom of the + // Purchasing Policies panel. Any other failure (network, 5xx) still + // surfaces the error so operators can diagnose the problem. + if (isPermissionDeniedError(error)) { + container.textContent = ''; + return; + } const err = error as Error; container.textContent = ''; const errorP = document.createElement('p'); diff --git a/frontend/src/settings.ts b/frontend/src/settings.ts index c63f30e0f..e07fc6b32 100644 --- a/frontend/src/settings.ts +++ b/frontend/src/settings.ts @@ -84,8 +84,12 @@ 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 { +/** + * Returns true when an API error signals a 403 permission-denied response. + * Exported so sibling modules (riexchange.ts) can apply the same graceful + * degradation without duplicating the predicate. + */ +export function isPermissionDeniedError(error: unknown): boolean { return error instanceof Error && (error as { status?: number }).status === 403 && error.message.startsWith('permission denied'); diff --git a/internal/api/handler_config_test.go b/internal/api/handler_config_test.go index dedb63c4f..688cfb29b 100644 --- a/internal/api/handler_config_test.go +++ b/internal/api/handler_config_test.go @@ -1225,3 +1225,48 @@ func TestHandler_getServiceConfig_ViewConfigPermission_Enforced(t *testing.T) { assert.Equal(t, 403, ce.code, "expected 403, got %d", ce.code) mockStore.AssertNotCalled(t, "GetServiceConfig", mock.Anything, mock.Anything, mock.Anything) } + +// TestHandler_updateConfig_RequiresUpdateConfigPermission verifies that the +// settings write endpoint (PUT /api/config) returns 403 to a caller who holds +// view:config (sufficient for GET) but not update:config. The frontend's +// applyReadOnlySettings already hides Save/Reset for such users, but the +// backend must reject any attempt to write config regardless of the UI gate +// (issues #1401, #1410: non-admin settings RBAC fix). +func TestHandler_updateConfig_RequiresUpdateConfigPermission(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + + // A non-admin user who was granted view:config (by migration 000088) but + // not update:config. This mirrors the Standard-Users permission set after + // the fix: they can read settings (GET) but cannot write them (PUT). + userSession := &Session{ + UserID: "dddddddd-dddd-dddd-dddd-dddddddddddd", + Email: "standard@example.com", + } + mockAuth.On("ValidateSession", ctx, "user-token").Return(userSession, nil) + // Grant view:config so the session itself is valid (GET would succeed). + mockAuth.On("HasPermissionAPI", ctx, userSession.UserID, "view", "config").Return(true, nil).Maybe() + // Deny update:config — this is the permission PUT /api/config checks. + mockAuth.On("HasPermissionAPI", ctx, userSession.UserID, "update", "config").Return(false, nil) + // Deny admin:* so requireAdmin also fails (belt-and-suspenders: the route + // is AuthAdmin-gated, but the handler adds its own requirePermission check). + mockAuth.On("HasPermissionAPI", ctx, userSession.UserID, mock.AnythingOfType("string"), mock.AnythingOfType("string")). + Return(false, nil).Maybe() + + handler := &Handler{config: mockStore, auth: mockAuth} + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer user-token"}, + Body: `{"default_term": 1}`, + } + result, err := handler.updateConfig(ctx, req) + require.Error(t, err, "updateConfig must be rejected for a user who lacks update:config") + assert.Nil(t, result) + ce, ok := IsClientError(err) + require.True(t, ok, "expected ClientError, got %T: %v", err, err) + assert.Equal(t, 403, ce.code, "expected 403, got %d", ce.code) + // The store must never be reached — the permission check fires first. + mockStore.AssertNotCalled(t, "GetGlobalConfig", mock.Anything) + mockStore.AssertNotCalled(t, "SaveGlobalConfig", mock.Anything, mock.Anything) +} diff --git a/internal/auth/service_group_test.go b/internal/auth/service_group_test.go index 7eeae6715..4306d706d 100644 --- a/internal/auth/service_group_test.go +++ b/internal/auth/service_group_test.go @@ -285,8 +285,9 @@ func TestService_GetUserPermissions(t *testing.T) { // + retry-own:purchases (issue #47) // + revoke-own:purchases (issue #290) // + sell-own:purchases (issue #292). + // + view:config (migration 000088, issues #1401/#1410/#1413). // NOTE: approve-own removed (issue #1407, four-eyes). - assert.Len(t, permissions, 12) + assert.Len(t, permissions, 13) mockStore.AssertExpectations(t) }) @@ -311,7 +312,7 @@ func TestService_GetUserPermissions(t *testing.T) { permissions, err := service.GetUserPermissions(ctx, "readonly-123") require.NoError(t, err) - assert.Len(t, permissions, 3) // 3 readonly permissions + assert.Len(t, permissions, 4) // 4 readonly permissions (view:config added by migration 000088) mockStore.AssertExpectations(t) }) @@ -355,11 +356,13 @@ func TestService_GetUserPermissions(t *testing.T) { permissions, err := service.GetUserPermissions(ctx, "user-123") require.NoError(t, err) - // 12 standard-group (incl. delete:plans (PR-A #660) + update:purchases (PR-A #660) + // 13 standard-group (incl. delete:plans (PR-A #660) + update:purchases (PR-A #660) // + cancel-own (#46) + retry-own (#47) - // + revoke-own (#290) + sell-own (#292):purchases; approve-own removed - // per #1407 four-eyes) + 1 group1 + 1 group2 = 14 - assert.Len(t, permissions, 14) + // + revoke-own (#290) + sell-own (#292):purchases + // + view:config (migration 000088, issues #1401/#1410/#1413); + // approve-own removed per #1407 four-eyes) + // + 1 group1 (execute:plans) + 1 group2 (update:config) = 15 + assert.Len(t, permissions, 15) mockStore.AssertExpectations(t) }) @@ -408,8 +411,9 @@ func TestService_GetUserPermissions(t *testing.T) { // + retry-own:purchases (issue #47) // + revoke-own:purchases (issue #290) // + sell-own:purchases (issue #292). + // + view:config (migration 000088, issues #1401/#1410/#1413). // NOTE: approve-own removed (issue #1407, four-eyes). - assert.Len(t, permissions, 12) + assert.Len(t, permissions, 13) mockStore.AssertExpectations(t) }) diff --git a/internal/auth/types.go b/internal/auth/types.go index dbe5d0026..778cfb2d9 100644 --- a/internal/auth/types.go +++ b/internal/auth/types.go @@ -560,6 +560,15 @@ func DefaultUserPermissions() []Permission { // offering_class = 'standard' and the cloud account to match the // row's cloud_account_id before creating the listing. {Action: ActionSellOwn, Resource: ResourcePurchases}, + // view:config — every authenticated user can read the global settings + // (providers, default term/payment/coverage, etc.) as read-only. The + // GET /api/config and GET /api/ri-exchange/config handlers both require + // this permission; without it non-admin users got 403 and the Settings + // page rendered a blank form (issues #1401, #1410, #1413). Writing + // to config still requires update:config, which only admins hold via + // admin:*. The handler also withholds the SourceIdentity field from + // non-admin sessions regardless of this grant (issue #407). + {Action: ActionView, Resource: ResourceConfig}, } } @@ -569,6 +578,9 @@ func DefaultReadOnlyPermissions() []Permission { {Action: ActionView, Resource: ResourceRecommendations}, {Action: ActionView, Resource: ResourcePlans}, {Action: ActionView, Resource: ResourceHistory}, + // view:config — read-only users can view global settings (issue #1401). + // They cannot write config (update:config is admin-only via admin:*). + {Action: ActionView, Resource: ResourceConfig}, } } diff --git a/internal/auth/types_test.go b/internal/auth/types_test.go index 7c222c6ca..80981f492 100644 --- a/internal/auth/types_test.go +++ b/internal/auth/types_test.go @@ -22,8 +22,9 @@ func TestDefaultPermissions(t *testing.T) { // + retry-own:purchases (issue #47) // + revoke-own:purchases (issue #290) // + sell-own:purchases (issue #292) - // NOTE: approve-own was removed (issue #1407, four-eyes) = 12. - assert.Len(t, perms, 12) + // + view:config (migration 000088, issues #1401/#1410/#1413) + // NOTE: approve-own was removed (issue #1407, four-eyes) = 13. + assert.Len(t, perms, 13) actions := make(map[string]bool) for _, p := range perms { @@ -34,6 +35,8 @@ func TestDefaultPermissions(t *testing.T) { assert.True(t, actions[ActionView+":"+ResourcePlans]) assert.True(t, actions[ActionView+":"+ResourcePurchases]) assert.True(t, actions[ActionView+":"+ResourceHistory]) + assert.True(t, actions[ActionView+":"+ResourceConfig], + "view:config must be in DefaultUserPermissions (issues #1401/#1410/#1413)") assert.True(t, actions[ActionCreate+":"+ResourcePlans]) assert.True(t, actions[ActionUpdate+":"+ResourcePlans]) assert.True(t, actions[ActionDelete+":"+ResourcePlans]) @@ -47,16 +50,30 @@ func TestDefaultPermissions(t *testing.T) { "approve-own must not be in DefaultUserPermissions (four-eyes, issue #1407)") // sell-own:purchases must be a default user permission (issue #292). assert.True(t, actions[ActionSellOwn+":"+ResourcePurchases]) + // Write-gating: non-admins may read config but not write it. + assert.False(t, actions[ActionUpdate+":"+ResourceConfig], + "update:config must not be in DefaultUserPermissions") }) t.Run("DefaultReadOnlyPermissions returns readonly access", func(t *testing.T) { perms := DefaultReadOnlyPermissions() - assert.Len(t, perms, 3) + // view:config added by migration 000088 (issues #1401/#1413) = 4. + assert.Len(t, perms, 4) // All should be view actions for _, p := range perms { assert.Equal(t, ActionView, p.Action) } + + actions := make(map[string]bool) + for _, p := range perms { + actions[p.Action+":"+p.Resource] = true + } + assert.True(t, actions[ActionView+":"+ResourceConfig], + "view:config must be in DefaultReadOnlyPermissions (issues #1401/#1413)") + // Read-only must never be able to write config. + assert.False(t, actions[ActionUpdate+":"+ResourceConfig], + "update:config must not be in DefaultReadOnlyPermissions") }) t.Run("DefaultPurchaserPermissions contains carved verbs and view grants", func(t *testing.T) { diff --git a/internal/database/postgres/migrations/000088_grant_view_config_to_nonadmin_groups.down.sql b/internal/database/postgres/migrations/000088_grant_view_config_to_nonadmin_groups.down.sql new file mode 100644 index 000000000..b27e193a2 --- /dev/null +++ b/internal/database/postgres/migrations/000088_grant_view_config_to_nonadmin_groups.down.sql @@ -0,0 +1,25 @@ +-- Reverse: remove view:config from Standard Users and Read-Only Users. +-- +-- After this rollback non-admin users will again receive 403 on +-- GET /api/config and GET /api/ri-exchange/config, restoring the +-- broken behaviour that issues #1401, #1410, and #1413 describe. +-- Apply only when explicitly rolling back 000088. + +UPDATE groups +SET + permissions = ( + SELECT COALESCE( + jsonb_agg(elem ORDER BY elem->>'action', elem->>'resource'), + '[]'::jsonb + ) + FROM jsonb_array_elements(permissions) AS elem + WHERE NOT ( + elem->>'action' = 'view' + AND elem->>'resource' = 'config' + ) + ), + updated_at = NOW() +WHERE id IN ( + '00000000-0000-5000-8000-000000000005', -- Standard Users + '00000000-0000-5000-8000-000000000006' -- Read-Only Users +); diff --git a/internal/database/postgres/migrations/000088_grant_view_config_to_nonadmin_groups.up.sql b/internal/database/postgres/migrations/000088_grant_view_config_to_nonadmin_groups.up.sql new file mode 100644 index 000000000..19b8a9302 --- /dev/null +++ b/internal/database/postgres/migrations/000088_grant_view_config_to_nonadmin_groups.up.sql @@ -0,0 +1,26 @@ +-- Grant view:config to Standard Users and Read-Only Users (issues #1401, #1410, #1413). +-- +-- The GET /api/config and GET /api/ri-exchange/config handlers both require +-- view:config. Without this grant the non-admin groups received 403, causing: +-- #1401: Global Configuration form never rendered (only the section header was visible). +-- #1410: Purchasing Policies inputs remained enabled (applyReadOnlySettings was +-- never called because loadGlobalSettings returned early on 403). +-- #1413: A "Failed to load settings: permission denied" error paragraph appeared +-- inside the Exchange Automation container on the Purchasing Policies page. +-- +-- Writing config still requires update:config (AuthAdmin-gated route + explicit +-- handler check), which admins hold implicitly via admin:*. Non-admin groups are +-- NOT granted update:config here. The GET handler also withholds the SourceIdentity +-- field from non-admin sessions regardless (issue #407). +-- +-- Only inserts the entry when absent; idempotent on repeated apply. + +UPDATE groups +SET + permissions = permissions || '[{"action":"view","resource":"config"}]'::jsonb, + updated_at = NOW() +WHERE id IN ( + '00000000-0000-5000-8000-000000000005', -- Standard Users + '00000000-0000-5000-8000-000000000006' -- Read-Only Users +) + AND NOT (permissions @> '[{"action":"view","resource":"config"}]'); diff --git a/internal/database/postgres/migrations/000088_grant_view_config_to_nonadmin_groups_test.go b/internal/database/postgres/migrations/000088_grant_view_config_to_nonadmin_groups_test.go new file mode 100644 index 000000000..83d69b47e --- /dev/null +++ b/internal/database/postgres/migrations/000088_grant_view_config_to_nonadmin_groups_test.go @@ -0,0 +1,178 @@ +//go:build integration +// +build integration + +package migrations_test + +import ( + "context" + "testing" + + "github.com/LeanerCloud/CUDly/internal/database/postgres/migrations" + "github.com/LeanerCloud/CUDly/internal/database/postgres/testhelpers" + "github.com/jackc/pgx/v5/pgxpool" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// groupHasPermission reports whether the given group's permissions JSONB array +// grants the given (action, resource) pair. +func groupHasPermission(t *testing.T, ctx context.Context, pool *pgxpool.Pool, groupID, action, resource string) bool { + t.Helper() + var has bool + err := pool.QueryRow(ctx, ` + SELECT EXISTS( + SELECT 1 + FROM groups g, jsonb_array_elements(g.permissions) AS elem + WHERE g.id = $1 + AND elem->>'action' = $2 + AND elem->>'resource' = $3 + ) + `, groupID, action, resource).Scan(&has) + require.NoError(t, err, "querying %s:%s on group %s", action, resource, groupID) + return has +} + +// TestMigration_GrantViewConfigToNonAdminGroups covers issues #1401, #1410, and +// #1413: migration 000088 grants view:config to Standard Users and Read-Only +// Users so GET /api/config and GET /api/ri-exchange/config return 200 instead of +// 403 for those groups. The grant must be surgical: no other permissions are +// touched on either group, and the write-gating verb (update:config) is not added. +func TestMigration_GrantViewConfigToNonAdminGroups(t *testing.T) { + ctx := context.Background() + migrationsPath := getMigrationsPath() + + t.Run("view:config granted to Standard Users and Read-Only Users", func(t *testing.T) { + container, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + defer container.Cleanup(ctx) + pool := container.DB.Pool() + + // Pin at 000086, the last migration on main before this change. view:config + // must be absent from both non-admin groups at this point (000088 not applied). + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 86)) + + require.False(t, groupHasPermission(t, ctx, pool, standardUsersGroupIDTest, "view", "config"), + "precondition: Standard Users must NOT hold view:config at v86") + require.False(t, groupHasPermission(t, ctx, pool, readOnlyUsersGroupIDTest, "view", "config"), + "precondition: Read-Only Users must NOT hold view:config at v86") + + // Apply 000088. Pin to this migration's own version so a later + // migration (e.g. 000089) landing on main cannot change what this + // test exercises (feedback_migration_test_pin_version, incident #1436). + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 88)) + + assert.True(t, groupHasPermission(t, ctx, pool, standardUsersGroupIDTest, "view", "config"), + "migration 000088 must add view:config to Standard Users (issue #1401)") + assert.True(t, groupHasPermission(t, ctx, pool, readOnlyUsersGroupIDTest, "view", "config"), + "migration 000088 must add view:config to Read-Only Users (issue #1401)") + }) + + t.Run("update:config is NOT granted (write gate is unaffected)", func(t *testing.T) { + container, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + defer container.Cleanup(ctx) + pool := container.DB.Pool() + + // Pin to 000088 (feedback_migration_test_pin_version, #1436). + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 88)) + + assert.False(t, groupHasPermission(t, ctx, pool, standardUsersGroupIDTest, "update", "config"), + "migration 000088 must NOT grant update:config to Standard Users") + assert.False(t, groupHasPermission(t, ctx, pool, readOnlyUsersGroupIDTest, "update", "config"), + "migration 000088 must NOT grant update:config to Read-Only Users") + }) + + t.Run("sibling permissions on Standard Users are preserved", func(t *testing.T) { + container, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + defer container.Cleanup(ctx) + pool := container.DB.Pool() + + // Pin to 000088 (feedback_migration_test_pin_version, #1436). + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 88)) + + for _, perm := range []struct{ action, resource string }{ + {"view", "recommendations"}, + {"view", "plans"}, + {"view", "purchases"}, + {"view", "history"}, + {"create", "plans"}, + {"update", "plans"}, + {"cancel-own", "purchases"}, + {"retry-own", "purchases"}, + } { + assert.True(t, + groupHasPermission(t, ctx, pool, standardUsersGroupIDTest, perm.action, perm.resource), + "migration 000088 must preserve %s:%s on Standard Users", perm.action, perm.resource) + } + }) + + t.Run("down migration removes view:config from both groups", func(t *testing.T) { + container, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + defer container.Cleanup(ctx) + pool := container.DB.Pool() + + // Pin to 000088 so a later migration on main cannot shift the head + // this rollback targets (feedback_migration_test_pin_version, #1436). + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 88)) + + // Confirm 000088 is applied. + require.True(t, groupHasPermission(t, ctx, pool, standardUsersGroupIDTest, "view", "config"), + "head state: Standard Users must hold view:config after 000088") + require.True(t, groupHasPermission(t, ctx, pool, readOnlyUsersGroupIDTest, "view", "config"), + "head state: Read-Only Users must hold view:config after 000088") + + // Roll back 000088 specifically (migrate down to 000087), not a + // relative "rollback 1 from HEAD" which would undo a later migration. + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 87)) + + assert.False(t, groupHasPermission(t, ctx, pool, standardUsersGroupIDTest, "view", "config"), + "000088 down migration must remove view:config from Standard Users") + assert.False(t, groupHasPermission(t, ctx, pool, readOnlyUsersGroupIDTest, "view", "config"), + "000088 down migration must remove view:config from Read-Only Users") + }) + + t.Run("idempotent: applying to a group already holding view:config is safe", func(t *testing.T) { + container, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + defer container.Cleanup(ctx) + pool := container.DB.Pool() + + // Pin to 000088 (feedback_migration_test_pin_version, #1436). + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 88)) + + // Count entries for view:config before the re-apply. + var count int + err = pool.QueryRow(ctx, ` + SELECT COUNT(*) + FROM groups g, jsonb_array_elements(g.permissions) AS elem + WHERE g.id = $1 + AND elem->>'action' = 'view' + AND elem->>'resource' = 'config' + `, standardUsersGroupIDTest).Scan(&count) + require.NoError(t, err) + require.Equal(t, 1, count, "exactly one view:config entry before re-apply") + + // Run the up SQL again directly. + _, err = pool.Exec(ctx, ` + UPDATE groups + SET permissions = permissions || '[{"action":"view","resource":"config"}]'::jsonb, + updated_at = NOW() + WHERE id IN ($1, $2) + AND NOT (permissions @> '[{"action":"view","resource":"config"}]') + `, standardUsersGroupIDTest, readOnlyUsersGroupIDTest) + require.NoError(t, err) + + // Count must still be 1 (the NOT EXISTS guard prevents duplication). + err = pool.QueryRow(ctx, ` + SELECT COUNT(*) + FROM groups g, jsonb_array_elements(g.permissions) AS elem + WHERE g.id = $1 + AND elem->>'action' = 'view' + AND elem->>'resource' = 'config' + `, standardUsersGroupIDTest).Scan(&count) + require.NoError(t, err) + assert.Equal(t, 1, count, "re-applying must not duplicate the view:config entry") + }) +}