diff --git a/frontend/src/__tests__/group-edit-unrepresentable-permissions.test.ts b/frontend/src/__tests__/group-edit-unrepresentable-permissions.test.ts new file mode 100644 index 000000000..90274ba16 --- /dev/null +++ b/frontend/src/__tests__/group-edit-unrepresentable-permissions.test.ts @@ -0,0 +1,286 @@ +/** + * Regression tests for issue #1629: editing a group through the Admin UI + * silently dropped permissions the perm-action + + +
+ + + `; +} + +// Simulates opening a group for edit, then saving after ONLY the +// description changed (the exact scenario from the issue: "an admin +// opens a group, changes only the description, and clicks Save"). +async function openEditDescriptionOnlyAndSave(group: api.APIGroup, newDescription: string): Promise { + (api.getGroup as jest.Mock).mockResolvedValue(group); + await groupModals.openEditGroupModal(group.id); + + (document.getElementById('group-description') as HTMLTextAreaElement).value = newDescription; + + const event = { preventDefault: jest.fn() } as unknown as Event; + await groupModals.saveGroup(event); +} + +describe('regression: group edit no longer drops/widens unrepresentable permissions (#1629)', () => { + beforeEach(() => { + setUpModalDom(); + groupState.setCurrentEditingGroup(null); + jest.clearAllMocks(); + }); + + test('seeded Purchaser permissions survive an edit that only changes the description', async () => { + const group: api.APIGroup = { + id: 'purchaser-group-id', + name: 'Purchaser', + description: 'Old description', + permissions: PURCHASER_PERMISSIONS, + created_at: '2024-01-01T00:00:00Z', + }; + + await openEditDescriptionOnlyAndSave(group, 'New description'); + + expect(api.updateGroup).toHaveBeenCalledWith('purchaser-group-id', { + name: 'Purchaser', + description: 'New description', + // Byte-identical to the input: same 7 permissions, same order, same + // action/resource values. Neither approve-any/retry-any dropped nor + // history widened to '*'. + permissions: PURCHASER_PERMISSIONS, + }); + }); + + test('approve-any:purchases specifically is not dropped (the four-eyes approval verb)', async () => { + const group: api.APIGroup = { + id: 'purchaser-group-id', + name: 'Purchaser', + description: 'Old description', + permissions: PURCHASER_PERMISSIONS, + created_at: '2024-01-01T00:00:00Z', + }; + + await openEditDescriptionOnlyAndSave(group, 'New description'); + + const call = (api.updateGroup as jest.Mock).mock.calls[0]; + const saved = call[1].permissions as api.Permission[]; + expect(saved).toContainEqual({ action: 'approve-any', resource: 'purchases' }); + expect(saved).toContainEqual({ action: 'retry-any', resource: 'purchases' }); + }); + + test('view:history specifically is not widened to view:* (would leak users/groups/config/accounts read access)', async () => { + const group: api.APIGroup = { + id: 'purchaser-group-id', + name: 'Purchaser', + description: 'Old description', + permissions: PURCHASER_PERMISSIONS, + created_at: '2024-01-01T00:00:00Z', + }; + + await openEditDescriptionOnlyAndSave(group, 'New description'); + + const call = (api.updateGroup as jest.Mock).mock.calls[0]; + const saved = call[1].permissions as api.Permission[]; + expect(saved).toContainEqual({ action: 'view', resource: 'history' }); + // No permission was widened to the wildcard resource as a side effect. + expect(saved.some(p => p.action === 'view' && p.resource === '*')).toBe(false); + }); + + test('a genuinely unrecognised action/resource pair round-trips unchanged instead of being dropped or widened', async () => { + // Simulates a future backend verb the frontend's ALL_ACTIONS/ + // ALL_RESOURCES hasn't been taught yet (or legacy/foreign data) -- + // the defense-in-depth path, independent of whether the known-vocabulary + // list is currently complete. + const foreignPermission: api.Permission = { action: 'time-travel', resource: 'flux-capacitor' }; + const group: api.APIGroup = { + id: 'exotic-group-id', + name: 'Exotic', + description: 'Old description', + permissions: [foreignPermission], + created_at: '2024-01-01T00:00:00Z', + }; + + await openEditDescriptionOnlyAndSave(group, 'New description'); + + expect(api.updateGroup).toHaveBeenCalledWith('exotic-group-id', { + name: 'Exotic', + description: 'New description', + permissions: [foreignPermission], + }); + }); + + test('the unrecognised value is visibly flagged in the select, not silently presented as a normal option', async () => { + const foreignPermission: api.Permission = { action: 'time-travel', resource: 'flux-capacitor' }; + const group: api.APIGroup = { + id: 'exotic-group-id', + name: 'Exotic', + description: 'Old description', + permissions: [foreignPermission], + created_at: '2024-01-01T00:00:00Z', + }; + + (api.getGroup as jest.Mock).mockResolvedValue(group); + await groupModals.openEditGroupModal(group.id); + + const actionSelect = document.querySelector('.perm-action') as HTMLSelectElement; + const resourceSelect = document.querySelector('.perm-resource') as HTMLSelectElement; + expect(actionSelect.value).toBe('time-travel'); + expect(resourceSelect.value).toBe('flux-capacitor'); + // The selected option's own label carries a visible warning, not a + // plain/normal-looking label, so an admin scanning the closed +// elements (issue #1629). This list used to be hardcoded to 7 of the 20 +// actions and 9 of the 11 resources the backend can store. A stored +// permission carrying one of the missing values had no matching ']; + for (const action of ALL_ACTIONS) { + const selected = currentValue === action ? ' selected' : ''; + options.push(``); + } + if (currentValue && !(ALL_ACTIONS as readonly string[]).includes(currentValue)) { + options.push(``); + } + return options.join(''); +} + +function buildResourceOptions(currentValue: string | undefined): string { + const options: string[] = []; + for (const resource of ALL_RESOURCES) { + const isDefault = !currentValue && resource === '*'; + const selected = currentValue === resource || isDefault ? ' selected' : ''; + options.push(``); + } + if (currentValue && !(ALL_RESOURCES as readonly string[]).includes(currentValue)) { + options.push(``); + } + return options.join(''); +} + /** * Add a new permission to the form */ @@ -129,27 +208,12 @@ export function addPermission(permission?: Permission): void {
diff --git a/frontend/src/permissions.ts b/frontend/src/permissions.ts index f30481e26..111f07eb2 100644 --- a/frontend/src/permissions.ts +++ b/frontend/src/permissions.ts @@ -82,6 +82,60 @@ export type Resource = | 'ri-exchange' | '*'; +// ALL_ACTIONS / ALL_RESOURCES: runtime enumeration of the Action / Resource +// unions above, for UI surfaces that must be able to represent every +// permission the backend can store -- the group-edit form's action/resource +//