From 69c7ae30d2d5377073cebc000ea2e28d16fadf9b Mon Sep 17 00:00:00 2001 From: joshunrau Date: Tue, 3 Nov 2026 20:54:26 -0500 Subject: [PATCH 1/5] feat(api): add permission sets A permission set is a named list of rules an administrator defines once and links users to, instead of rebuilding the same additional permissions for each user. Each rule applies to every group or to the holder's groups, resolved at sign-in like a group manager's own rules. A set is linked, not copied: editing it changes every holder from their next sign-in, and deleting it removes it from its holders. A set and custom additional permissions are mutually exclusive. PUT /v1/users/:id/permissions now takes { kind: 'permissionSet', permissionSetId } or { kind: 'custom', permissions }, and each write empties the other. A set may not grant manage all, an administrator never holds one, and every /v1/permission-sets route is admin-only, since editing a set is a grant to each holder. Co-Authored-By: Claude Opus 5.5 --- .../docs/architecture/auth-and-permissions.md | 36 ++- apps/api/AGENTS.md | 21 +- apps/api/prisma/schema.prisma | 31 ++ .../__tests__/assignments.service.test.ts | 15 +- .../auth/__tests__/ability.factory.test.ts | 247 ++++++++++----- .../src/auth/__tests__/auth.service.test.ts | 49 ++- apps/api/src/auth/ability.factory.ts | 43 ++- apps/api/src/auth/auth.service.ts | 7 +- apps/api/src/core/prisma.ts | 1 + .../__tests__/groups.controller.test.ts | 27 +- .../instrument-records.service.test.ts | 3 +- .../__tests__/instruments.controller.test.ts | 23 +- .../__tests__/instruments.service.test.ts | 13 +- apps/api/src/main.ts | 2 + .../permission-sets.controller.test.ts | 108 +++++++ .../__tests__/permission-sets.service.test.ts | 134 +++++++++ .../permission-sets.controller.ts | 56 ++++ .../permission-sets/permission-sets.module.ts | 11 + .../permission-sets.service.ts | 75 +++++ .../__tests__/subjects.service.test.ts | 23 +- .../users/__tests__/users.controller.test.ts | 44 ++- .../src/users/__tests__/users.service.test.ts | 119 ++++++-- apps/api/src/users/users.controller.ts | 4 +- apps/api/src/users/users.module.ts | 3 +- apps/api/src/users/users.service.ts | 54 +++- .../test/suites/02-user-permissions.suite.ts | 4 +- .../suites/03-record-file-deletion.suite.ts | 1 + .../test/suites/04-permission-sets.suite.ts | 282 ++++++++++++++++++ packages/schemas/AGENTS.md | 8 +- packages/schemas/package.json | 1 + .../__tests__/permission-set.test.ts | 125 ++++++++ .../src/permission-set/permission-set.ts | 107 +++++++ .../schemas/src/setup/__tests__/setup.test.ts | 13 +- packages/schemas/src/setup/setup.ts | 4 +- .../schemas/src/user/__tests__/user.test.ts | 46 ++- packages/schemas/src/user/user.ts | 74 +++-- 36 files changed, 1589 insertions(+), 225 deletions(-) create mode 100644 apps/api/src/permission-sets/__tests__/permission-sets.controller.test.ts create mode 100644 apps/api/src/permission-sets/__tests__/permission-sets.service.test.ts create mode 100644 apps/api/src/permission-sets/permission-sets.controller.ts create mode 100644 apps/api/src/permission-sets/permission-sets.module.ts create mode 100644 apps/api/src/permission-sets/permission-sets.service.ts create mode 100644 apps/api/test/suites/04-permission-sets.suite.ts create mode 100644 packages/schemas/src/permission-set/__tests__/permission-set.test.ts create mode 100644 packages/schemas/src/permission-set/permission-set.ts diff --git a/.agents/docs/architecture/auth-and-permissions.md b/.agents/docs/architecture/auth-and-permissions.md index 9400ca59d..1e85701a1 100644 --- a/.agents/docs/architecture/auth-and-permissions.md +++ b/.agents/docs/architecture/auth-and-permissions.md @@ -42,6 +42,7 @@ The full inventory of non-ordinary access declarations, current as of writing: | `PATCH /v1/users/self-update/:id` | `{ action: 'read', subject: 'User' }` | Deliberately weak; `UsersService.updateSelfById` throws `ForbiddenException` unless `id === currentUser.id`. The controller carries a comment saying so. | | `PUT /v1/users/:id/permissions` | `ADMIN_ONLY` | `ADMIN` alone. An `update User` grant is one of the things this route hands out, so it must not be enough to reach it, or the holder could grant themselves `manage all`. `$UpdateUserData` no longer carries the field either. | | `POST /v1/users`, `PATCH /v1/users/:id`, `PATCH /v1/users/:id/archive`, `PATCH /v1/users/:id/unarchive` | `ADMIN_ONLY` | `ADMIN` alone, whatever `User` action a grant names. Level, groups and password are what the rest of a user's access derives from, so a grantee could otherwise promote themselves, join every group, or log in as an admin whose password they set. `UsersService` also refuses an admin archiving, unarchiving, disabling or demoting their own account, so the last one cannot lock every admin-only route. | +| `GET`, `POST /v1/permission-sets`, `GET`, `PATCH`, `DELETE /v1/permission-sets/:id` | `ADMIN_ONLY` | `ADMIN` alone, reads included. A set's rules are applied to every holder at sign-in, so editing one is a grant to each of them, and only an administrator may grant; holders and group managers have no use for reading it. `PermissionSet` is deliberately absent from `$AppSubjectName`, so no grant can name it either (see below). | | `GET /v1/summary` | five-element array (`read` on `Instrument`, `InstrumentRecord`, `Session`, `Subject`, `User`) | The only use of the multi-element array form; all five must pass. | Adding a fourth `'public'` route, or a second `[]`, is a security decision — raise it rather than @@ -113,11 +114,15 @@ Prisma results already carry `__modelName` — it is a computed field added by ## Where an ability comes from 1. `AuthService.login` builds one with `AbilityFactory.createForPayload`, switching on - `basePermissionLevel` (`ADMIN` / `GROUP_MANAGER` / `STANDARD`) and then applying the user's - `additionalPermissions` on top. A stored rule carrying a `groupId` is applied with that model's - group condition from `GROUP_SCOPED_CONDITIONS` (`src/auth/ability.factory.ts`), so it reaches - only that group's rows; a `null` `groupId` is an unconditional rule, which is what every rule - written before the field existed reads back as. + `basePermissionLevel` (`ADMIN` / `GROUP_MANAGER` / `STANDARD`) and then applying, on top, the + rules of the user's permission set and the user's `additionalPermissions`. A stored rule + carrying a `groupId` is applied with that model's group condition from `GROUP_SCOPED_CONDITIONS` + (`src/auth/ability.factory.ts`), so it reaches only that group's rows; a `null` `groupId` is an + unconditional rule, which is what every rule written before the field existed reads back as. + A permission set rule names no group: `HOLDER_GROUPS` gets the same condition over all of the + holder's groups, the way a group manager's own rules do, and `ALL_GROUPS` is unconditional. + `login` reads the set through `UsersService.findByUsername` and passes its rules as + `createForPayload`'s second, required, parameter; the set itself is not put on the payload. 2. The **serialized rules** (`ability.rules`) are signed into the JWT alongside the payload, `expiresIn: '1h'`. 3. On every request `JwtStrategy.validate` rebuilds the ability from those rules with @@ -125,8 +130,9 @@ Prisma results already carry `__modelName` — it is a computed field added by Consequences worth knowing: -- **Permissions are frozen into the token.** Changing a user's groups, base level or additional - permissions has no effect until they obtain a new token. There is no refresh endpoint. +- **Permissions are frozen into the token.** Changing a user's groups, base level, additional + permissions or permission set — or editing the set itself — has no effect until they obtain a new + token. There is no refresh endpoint. - **`basePermissionLevel` is nullable** and the `switch` has no `default`, so a user with `null` gets only whatever `additionalPermissions` grants — usually nothing. - **JWT expiry is ignored in development.** `ignoreExpiration: configService.getOrThrow('NODE_ENV') === 'development'` @@ -167,11 +173,13 @@ hand-written: **These two currently agree with each other but not with the derived list.** They contain `all`, `Assignment`, `Group`, `Instrument`, `InstrumentRecord`, `InstrumentRepo`, `Session`, `Subject`, -`User`. The Prisma schema also defines `AuditLog`, `InstrumentRecordFile`, `InstrumentRepoSyncLog` -and `SetupState`, which -are therefore valid in `@RouteAccess` and in `AbilityFactory` rules (`InstrumentRecordFile` is used -in both) but cannot be granted as an `additionalPermission`. Treat that as the existing state, not -as a licence to widen it silently. +`User`. The Prisma schema also defines `AuditLog`, `InstrumentRecordFile`, `InstrumentRepoSyncLog`, +`PermissionSet` and `SetupState`, which are therefore valid in `@RouteAccess` and in +`AbilityFactory` rules (`InstrumentRecordFile` is used in both) but cannot be granted as an +`additionalPermission` or as a permission set rule, whose `subject` uses the same enum. Treat that +as the existing state, not as a licence to widen it silently. `PermissionSet` in particular must +stay out: a grant on it would let its holder edit a set they hold, and so grant themselves anything +a set may contain. A third list, `$GroupScopableSubjectName` in the same schemas file, says which of these a per-user permission may be confined to one group on. It is derived from `$AppSubjectName` by excluding `all` @@ -185,7 +193,9 @@ any other subject at the boundary, and the factory throws on one it meets anyway **A pair can be storable and still do nothing.** Every route that writes a user is `ADMIN_ONLY`, so a grant of any `User` action but `read` reaches nothing. `isGrantablePermission` (`packages/schemas/src/core/core.ts`) is that rule: `$UpdateUserPermissionsData` refuses what it -refuses, and the web permissions editor offers only what it allows. `$UserPermission` itself does +refuses, and the web permissions editor offers only what it allows. A permission set applies +`isPermissionSetGrantable` (`packages/schemas/src/permission-set/permission-set.ts`) instead, which +also refuses `manage all`: one edit to the set would otherwise make every holder an administrator. `$UserPermission` itself does not apply it, because it is also the read model and grants stored before the rule still have to parse; the editor marks those "No effect" and leaves them out of its next save. A new admin-only route that a grantable pair used to reach belongs in the same predicate. diff --git a/apps/api/AGENTS.md b/apps/api/AGENTS.md index d60832522..c9c9c2dcd 100644 --- a/apps/api/AGENTS.md +++ b/apps/api/AGENTS.md @@ -121,10 +121,11 @@ CASL subjects are derived automatically from the Prisma `TypeMap`, so every mode without you doing anything. But the subjects a user can be _granted_ through `additionalPermissions` come from **two hand-maintained lists that must agree with each other**: `enum AppSubject` in `prisma/schema.prisma` and `$AppSubjectName` in `packages/schemas/src/core/core.ts`. Both currently -list the same eight models plus `all`. Four models — `AuditLog`, `InstrumentRecordFile`, -`InstrumentRepoSyncLog` and `SetupState` — appear in neither, so no user can be granted a permission naming them; they are -reachable only through rules the ability factory writes itself. Adding a model that users should be -able to hold a permission on means editing both lists. +list the same eight models plus `all`. Five models — `AuditLog`, `InstrumentRecordFile`, +`InstrumentRepoSyncLog`, `PermissionSet` and `SetupState` — appear in neither, so no user can be +granted a permission naming them; they are reachable only through rules the ability factory writes +itself. `PermissionSet` must stay that way: a grant on it would let its holder edit a set they hold. +Adding a model that users should be able to hold a permission on means editing both lists. A granted permission may be confined to one group. `AuthRule.groupId` names it (`null`, which is what every rule written before the field existed reads back as, means every group), and @@ -137,6 +138,18 @@ on a `User` action; the route inventory in the architecture doc below says why. `UsersService.updatePermissions` accepts a scoped grant only for a group the user belongs to, and `updateById` drops the grants scoped to a group the user is removed from. +A user may instead hold a **permission set** (`src/permission-sets/`): an admin-defined, named list +of rules, linked through `User.permissionSetId`, whose rules `createForPayload` applies alongside the +base level. It takes them as a **required** second parameter, so the one caller, `AuthService.login`, +cannot drop them; login reads the set through `UsersService.findByUsername`'s include and never puts +it on the token payload. A rule's scope is relative — `HOLDER_GROUPS` is the holder's groups at +sign-in, through the same `GROUP_SCOPED_CONDITIONS` — so a set names no group. A set and custom +`additionalPermissions` are mutually exclusive: `PUT /v1/users/:id/permissions` takes +`{ kind: 'permissionSet' }` or `{ kind: 'custom' }` and each write empties the other. An `ADMIN` never +holds a set — `POST /v1/users` and the permissions route refuse one, and `updateById` disconnects it +on promotion — and deleting a set clears every holder through `onDelete: SetNull`. Every +`/v1/permission-sets` route is `ADMIN_ONLY`, because editing a set is a grant to each holder. + Background: `.agents/docs/architecture/auth-and-permissions.md`. ## Validation diff --git a/apps/api/prisma/schema.prisma b/apps/api/prisma/schema.prisma index c23295192..082856166 100644 --- a/apps/api/prisma/schema.prisma +++ b/apps/api/prisma/schema.prisma @@ -349,6 +349,32 @@ type AuthRule { groupId String? @db.ObjectId } +enum PermissionSetScope { + ALL_GROUPS + HOLDER_GROUPS +} + +/// A rule in a permission set. Its scope is relative to the holder rather than naming a group, so one +/// set can serve users in different groups; it is resolved against the holder's groups at sign-in. +type PermissionSetRule { + action AppAction + subject AppSubject + scope PermissionSetScope +} + +/// A named list of rules an administrator defines once and links users to. Not in `enum AppSubject` +/// on purpose: a grant on it would let its holder edit a set they hold, and so grant themselves more. +model PermissionSet { + createdAt DateTime @default(now()) @db.Date + updatedAt DateTime @updatedAt @db.Date + id String @id @default(auto()) @map("_id") @db.ObjectId + name String @unique + rules PermissionSetRule[] + users User[] + + @@map("PermissionSetModel") +} + model User { createdAt DateTime @default(now()) @db.Date updatedAt DateTime @updatedAt @db.Date @@ -373,6 +399,11 @@ model User { email String? disabled Boolean? archivedAt DateTime? @db.Date + /// The permission set this user holds, if any. Mutually exclusive with a non-empty + /// `additionalPermissions`: `PUT /v1/users/:id/permissions` is the one writer of both, and always + /// writes them together. Optional, so users created before this field existed read back as none. + permissionSetId String? @db.ObjectId + permissionSet PermissionSet? @relation(fields: [permissionSetId], references: [id], onDelete: SetNull) @@unique([username]) @@map("UserModel") diff --git a/apps/api/src/assignments/__tests__/assignments.service.test.ts b/apps/api/src/assignments/__tests__/assignments.service.test.ts index 5d37c3d9e..64c8153d3 100644 --- a/apps/api/src/assignments/__tests__/assignments.service.test.ts +++ b/apps/api/src/assignments/__tests__/assignments.service.test.ts @@ -39,12 +39,15 @@ const userAt = ( additionalPermissions: Permissions = [] ) => { return { - ability: new AbilityFactory({ verbose: vi.fn() } as any).createForPayload({ - additionalPermissions, - basePermissionLevel, - groups: [{ id: groupId }], - id: 'user-1' - } as any), + ability: new AbilityFactory({ verbose: vi.fn() } as any).createForPayload( + { + additionalPermissions, + basePermissionLevel, + groups: [{ id: groupId }], + id: 'user-1' + } as any, + [] + ), id: 'user-1' } as any; }; diff --git a/apps/api/src/auth/__tests__/ability.factory.test.ts b/apps/api/src/auth/__tests__/ability.factory.test.ts index 8edeb185b..751c88c12 100644 --- a/apps/api/src/auth/__tests__/ability.factory.test.ts +++ b/apps/api/src/auth/__tests__/ability.factory.test.ts @@ -43,22 +43,25 @@ describe('AbilityFactory', () => { username: 'admin-user' }; - const ability = abilityFactory.createForPayload(payload as any); + const ability = abilityFactory.createForPayload(payload as any, []); expect(ability.can('manage', 'all')).toBe(true); }); it('should reduce a user owing a password reset to reading themselves, whatever their permission level', () => { - const ability = abilityFactory.createForPayload({ - additionalPermissions: [{ action: 'read', groupId: null, subject: 'Subject' }], - basePermissionLevel: 'ADMIN', - firstName: 'Test', - groups: [{ id: 'group-1' }], - id: 'user-1', - lastName: 'User', - mustResetPassword: true, - username: 'admin-user' - } as any); + const ability = abilityFactory.createForPayload( + { + additionalPermissions: [{ action: 'read', groupId: null, subject: 'Subject' }], + basePermissionLevel: 'ADMIN', + firstName: 'Test', + groups: [{ id: 'group-1' }], + id: 'user-1', + lastName: 'User', + mustResetPassword: true, + username: 'admin-user' + } as any, + [] + ); // Allowed, because `updateSelfById` is gated on it and is the only way out of the reset. expect(ability.can('read', subject('User', { id: 'user-1' }) as any)).toBe(true); @@ -83,7 +86,7 @@ describe('AbilityFactory', () => { username: 'admin-user' }; - const ability = abilityFactory.createForPayload(payload as any); + const ability = abilityFactory.createForPayload(payload as any, []); expect(ability.can('manage', subject('Group', { id: 'group-1' }) as any)).toBe(true); expect(ability.can('manage', subject('Group', { id: 'group-2' }) as any)).toBe(false); @@ -99,29 +102,35 @@ describe('AbilityFactory', () => { ['InstrumentRecord', [], { groupId: { in: [] } }], ['Subject', [], { groupIds: { hasSome: [] } }] ] as const)("should confine a group manager's %s query to the groups %j", (modelName, groupIds, condition) => { - const ability = abilityFactory.createForPayload({ - additionalPermissions: undefined, - basePermissionLevel: 'GROUP_MANAGER', - firstName: 'Test', - groups: groupIds.map((id) => ({ id })), - id: 'user-1', - lastName: 'User', - username: 'manager-user' - } as any); + const ability = abilityFactory.createForPayload( + { + additionalPermissions: undefined, + basePermissionLevel: 'GROUP_MANAGER', + firstName: 'Test', + groups: groupIds.map((id) => ({ id })), + id: 'user-1', + lastName: 'User', + username: 'manager-user' + } as any, + [] + ); expect(accessibleQuery(ability, 'read', modelName)).toStrictEqual({ OR: [condition] }); }); it('should restrict series deletion to the manager group', () => { - const ability = abilityFactory.createForPayload({ - additionalPermissions: undefined, - basePermissionLevel: 'GROUP_MANAGER', - firstName: 'Test', - groups: [{ id: 'group-1' }], - id: 'user-1', - lastName: 'User', - username: 'manager-user' - } as any); + const ability = abilityFactory.createForPayload( + { + additionalPermissions: undefined, + basePermissionLevel: 'GROUP_MANAGER', + firstName: 'Test', + groups: [{ id: 'group-1' }], + id: 'user-1', + lastName: 'User', + username: 'manager-user' + } as any, + [] + ); expect(ability.can('delete', subject('Instrument', { seriesGroupId: 'group-1' }) as any)).toBe(true); expect(ability.can('delete', subject('Instrument', { seriesGroupId: 'group-2' }) as any)).toBe(false); @@ -141,7 +150,7 @@ describe('AbilityFactory', () => { username: 'standard-user' }; - const ability = abilityFactory.createForPayload(payload as any); + const ability = abilityFactory.createForPayload(payload as any, []); expect(ability.can('read', subject('User', { id: 'user-1' }) as any)).toBe(true); expect(ability.can('read', subject('User', { id: 'user-2' }) as any)).toBe(false); @@ -159,7 +168,7 @@ describe('AbilityFactory', () => { username: 'standard-user' }; - const ability = abilityFactory.createForPayload(payload as any); + const ability = abilityFactory.createForPayload(payload as any, []); expect(ability.can('create', subject('InstrumentRecordFile', { groupId: 'group-1' }) as any)).toBe(true); expect(ability.can('create', subject('InstrumentRecordFile', { groupId: 'group-2' }) as any)).toBe(false); @@ -178,7 +187,7 @@ describe('AbilityFactory', () => { username: 'manager-user' }; - const ability = abilityFactory.createForPayload(payload as any); + const ability = abilityFactory.createForPayload(payload as any, []); expect(ability.can('create', subject('InstrumentRecordFile', { groupId: 'group-1' }) as any)).toBe(true); expect(ability.can('create', subject('InstrumentRecordFile', { groupId: 'group-2' }) as any)).toBe(false); @@ -187,15 +196,18 @@ describe('AbilityFactory', () => { it.each(Object.entries(GROUP_SCOPED_ROWS))( 'should confine a granted %s permission to the group it names', (subjectName, { inGroup, outOfGroup }) => { - const ability = abilityFactory.createForPayload({ - additionalPermissions: [{ action: 'read', groupId: 'group-1', subject: subjectName }], - basePermissionLevel: null, - firstName: 'Test', - groups: [], - id: 'user-1', - lastName: 'User', - username: 'granted-user' - } as any); + const ability = abilityFactory.createForPayload( + { + additionalPermissions: [{ action: 'read', groupId: 'group-1', subject: subjectName }], + basePermissionLevel: null, + firstName: 'Test', + groups: [], + id: 'user-1', + lastName: 'User', + username: 'granted-user' + } as any, + [] + ); expect(ability.can('read', subject(subjectName, inGroup) as any)).toBe(true); expect(ability.can('read', subject(subjectName, outOfGroup) as any)).toBe(false); @@ -203,15 +215,18 @@ describe('AbilityFactory', () => { ); it('should apply an unscoped grant to every group, as every grant was before scoping existed', () => { - const ability = abilityFactory.createForPayload({ - additionalPermissions: [{ action: 'read', groupId: null, subject: 'Subject' }], - basePermissionLevel: null, - firstName: 'Test', - groups: [], - id: 'user-1', - lastName: 'User', - username: 'granted-user' - } as any); + const ability = abilityFactory.createForPayload( + { + additionalPermissions: [{ action: 'read', groupId: null, subject: 'Subject' }], + basePermissionLevel: null, + firstName: 'Test', + groups: [], + id: 'user-1', + lastName: 'User', + username: 'granted-user' + } as any, + [] + ); expect(ability.can('read', subject('Subject', { groupIds: ['group-1'] }) as any)).toBe(true); expect(ability.can('read', subject('Subject', { groupIds: ['group-2'] }) as any)).toBe(true); @@ -219,15 +234,18 @@ describe('AbilityFactory', () => { }); it('should add a scoped grant on top of the base level without widening anything else', () => { - const ability = abilityFactory.createForPayload({ - additionalPermissions: [{ action: 'read', groupId: 'group-1', subject: 'InstrumentRecord' }], - basePermissionLevel: 'STANDARD', - firstName: 'Test', - groups: [{ id: 'group-1' }], - id: 'user-1', - lastName: 'User', - username: 'standard-user' - } as any); + const ability = abilityFactory.createForPayload( + { + additionalPermissions: [{ action: 'read', groupId: 'group-1', subject: 'InstrumentRecord' }], + basePermissionLevel: 'STANDARD', + firstName: 'Test', + groups: [{ id: 'group-1' }], + id: 'user-1', + lastName: 'User', + username: 'standard-user' + } as any, + [] + ); expect(ability.can('read', subject('InstrumentRecord', { groupId: 'group-1' }) as any)).toBe(true); expect(ability.can('read', subject('Subject', { groupIds: ['group-1'] }) as any)).toBe(true); @@ -237,15 +255,18 @@ describe('AbilityFactory', () => { it('should refuse to build an ability from a stored grant confined to a group on a resource that cannot be', () => { const build = () => { - return abilityFactory.createForPayload({ - additionalPermissions: [{ action: 'read', groupId: 'group-1', subject: 'Instrument' }], - basePermissionLevel: null, - firstName: 'Test', - groups: [], - id: 'user-1', - lastName: 'User', - username: 'granted-user' - } as any); + return abilityFactory.createForPayload( + { + additionalPermissions: [{ action: 'read', groupId: 'group-1', subject: 'Instrument' }], + basePermissionLevel: null, + firstName: 'Test', + groups: [], + id: 'user-1', + lastName: 'User', + username: 'granted-user' + } as any, + [] + ); }; expect(build).toThrow("Cannot confine permission 'read Instrument' to group 'group-1'"); @@ -269,9 +290,95 @@ describe('AbilityFactory', () => { username: 'some-user' }; - const ability = abilityFactory.createForPayload(payload as any); + const ability = abilityFactory.createForPayload(payload as any, []); expect(accessibleQuery(ability, 'read', 'Session')).toBeDefined(); } ); + describe('permission set rules', () => { + const holderPayload = (groupIds: string[], overrides: object = {}) => ({ + additionalPermissions: [], + basePermissionLevel: 'STANDARD', + firstName: 'Test', + groups: groupIds.map((id) => ({ id })), + id: 'user-1', + lastName: 'User', + username: 'holder', + ...overrides + }); + + it.each(Object.entries(GROUP_SCOPED_ROWS))( + "should confine a %s rule scoped to the holder's groups to those groups", + (subjectName, { inGroup, outOfGroup }) => { + const ability = abilityFactory.createForPayload(holderPayload(['group-1']) as any, [ + { action: 'read', scope: 'HOLDER_GROUPS', subject: subjectName as GroupScopableSubjectName } + ]); + + expect(ability.can('read', subject(subjectName, inGroup) as any)).toBe(true); + expect(ability.can('read', subject(subjectName, outOfGroup) as any)).toBe(false); + } + ); + + it("should reach every one of the holder's groups, not only the first", () => { + const ability = abilityFactory.createForPayload(holderPayload(['group-1', 'group-2']) as any, [ + { action: 'read', scope: 'HOLDER_GROUPS', subject: 'InstrumentRecord' } + ]); + + expect(accessibleQuery(ability, 'read', 'InstrumentRecord')).toStrictEqual({ + OR: [{ groupId: { in: ['group-1', 'group-2'] } }] + }); + }); + + it('should apply a rule scoped to every group to rows in groups the holder is not in', () => { + const ability = abilityFactory.createForPayload(holderPayload(['group-1']) as any, [ + { action: 'read', scope: 'ALL_GROUPS', subject: 'InstrumentRecord' } + ]); + + expect(ability.can('read', subject('InstrumentRecord', { groupId: 'group-1' }) as any)).toBe(true); + expect(ability.can('read', subject('InstrumentRecord', { groupId: 'group-2' }) as any)).toBe(true); + expect(ability.can('update', subject('InstrumentRecord', { groupId: 'group-1' }) as any)).toBe(false); + }); + + it("should match nothing for a holder in no group when a rule is scoped to the holder's groups", () => { + const ability = abilityFactory.createForPayload(holderPayload([], { basePermissionLevel: null }) as any, [ + { action: 'read', scope: 'HOLDER_GROUPS', subject: 'InstrumentRecord' } + ]); + + expect(ability.can('read', subject('InstrumentRecord', { groupId: 'group-1' }) as any)).toBe(false); + expect(accessibleQuery(ability, 'read', 'InstrumentRecord')).toStrictEqual({ OR: [{ groupId: { in: [] } }] }); + }); + + it("should refuse to build an ability from a stored rule scoped to the holder's groups on a resource no group owns", () => { + const build = () => { + return abilityFactory.createForPayload(holderPayload(['group-1']) as any, [ + { action: 'read', scope: 'HOLDER_GROUPS', subject: 'Instrument' } + ]); + }; + + expect(build).toThrow("Cannot confine permission set rule 'read Instrument' to the holder's groups"); + }); + + it('should apply the set alongside custom grants and the base level, widening nothing else', () => { + const ability = abilityFactory.createForPayload( + holderPayload(['group-1'], { + additionalPermissions: [{ action: 'read', groupId: 'group-1', subject: 'Assignment' }] + }) as any, + [{ action: 'read', scope: 'HOLDER_GROUPS', subject: 'InstrumentRecord' }] + ); + + expect(ability.can('read', subject('InstrumentRecord', { groupId: 'group-1' }) as any)).toBe(true); + expect(ability.can('read', subject('Assignment', { groupId: 'group-1' }) as any)).toBe(true); + expect(ability.can('read', subject('Subject', { groupIds: ['group-1'] }) as any)).toBe(true); + expect(ability.can('manage', 'all')).toBe(false); + }); + + it('should ignore the set for a holder owing a password reset, who may only read themselves', () => { + const ability = abilityFactory.createForPayload(holderPayload(['group-1'], { mustResetPassword: true }) as any, [ + { action: 'read', scope: 'ALL_GROUPS', subject: 'Subject' } + ]); + + expect(ability.can('read', 'Subject')).toBe(false); + expect(ability.can('read', subject('User', { id: 'user-1' }) as any)).toBe(true); + }); + }); }); diff --git a/apps/api/src/auth/__tests__/auth.service.test.ts b/apps/api/src/auth/__tests__/auth.service.test.ts index 74c58ab86..7198eaa71 100644 --- a/apps/api/src/auth/__tests__/auth.service.test.ts +++ b/apps/api/src/auth/__tests__/auth.service.test.ts @@ -37,7 +37,7 @@ describe('AuthService', () => { let usersService: MockedInstance; const requestUserFor = (basePermissionLevel: 'ADMIN' | 'GROUP_MANAGER' | 'STANDARD'): RequestUser => { - const ability = abilityFactory.createForPayload({ ...BASE_PAYLOAD, basePermissionLevel } as any); + const ability = abilityFactory.createForPayload({ ...BASE_PAYLOAD, basePermissionLevel } as any, []); return { ...BASE_PAYLOAD, ability, basePermissionLevel } as unknown as RequestUser; }; @@ -82,6 +82,8 @@ describe('AuthService', () => { id: 'user-1', lastName: 'User', mustResetPassword: null, + permissionSet: null, + permissionSetId: null, phoneNumber: null, sex: null, updatedAt: new Date('2026-01-01T00:00:00.000Z'), @@ -139,6 +141,51 @@ describe('AuthService', () => { }); }); + it("should sign the rules of the user's permission set, confined to the user's groups where the set says so", async () => { + usersService.findByUsername.mockResolvedValue( + storedUser({ + groups: [{ id: 'group-1' } as StoredUser['groups'][number]], + permissionSet: { + __modelName: 'PermissionSet', + createdAt: new Date('2026-01-01T00:00:00.000Z'), + id: 'set-1', + name: 'Research Assistant', + rules: [{ action: 'read', scope: 'HOLDER_GROUPS', subject: 'InstrumentRecord' }], + updatedAt: new Date('2026-01-01T00:00:00.000Z') + }, + permissionSetId: 'set-1' + }) + ); + cryptoService.comparePassword.mockResolvedValue(true); + await authService.login(credentials); + expect(jwtService.signAsync.mock.lastCall?.[0]).toMatchObject({ + permissions: expect.arrayContaining([ + { action: 'read', conditions: { groupId: { in: ['group-1'] } }, subject: 'InstrumentRecord' } + ]) + }); + }); + + it('should leave the permission set itself off the signed payload, so it does not grow the token', async () => { + usersService.findByUsername.mockResolvedValue( + storedUser({ + permissionSet: { + __modelName: 'PermissionSet', + createdAt: new Date('2026-01-01T00:00:00.000Z'), + id: 'set-1', + name: 'Research Assistant', + rules: [{ action: 'read', scope: 'ALL_GROUPS', subject: 'Subject' }], + updatedAt: new Date('2026-01-01T00:00:00.000Z') + }, + permissionSetId: 'set-1' + }) + ); + cryptoService.comparePassword.mockResolvedValue(true); + await authService.login(credentials); + const signed = jwtService.signAsync.mock.lastCall?.[0] as object; + expect(signed).not.toHaveProperty('permissionSet'); + expect(signed).not.toHaveProperty('permissionSetId'); + }); + it('should treat an account with no reset flag as not needing a reset, since older accounts never stored one', async () => { usersService.findByUsername.mockResolvedValue(storedUser()); cryptoService.comparePassword.mockResolvedValue(true); diff --git a/apps/api/src/auth/ability.factory.ts b/apps/api/src/auth/ability.factory.ts index ce7111664..294234d17 100644 --- a/apps/api/src/auth/ability.factory.ts +++ b/apps/api/src/auth/ability.factory.ts @@ -5,6 +5,7 @@ import { Injectable } from '@nestjs/common'; import type { TokenPayload } from '@opendatacapture/schemas/auth'; import { isGroupScopableSubject } from '@opendatacapture/schemas/core'; import type { GroupScopableSubjectName } from '@opendatacapture/schemas/core'; +import type { PermissionSetRule } from '@opendatacapture/schemas/permission-set'; import type { PrismaModelWhereInputMap } from '@/core/prisma'; @@ -13,25 +14,30 @@ import { createAppAbility, detectAppSubject } from './ability.utils'; import type { AppAbility, Permission } from './auth.types'; /** - * The condition confining a per-user permission to one group, per model that can carry one. The - * fields are the same ones the group manager rules below use. Typed over the schema's list, so a - * subject made scopable there fails to compile here until its group field is named. + * The condition confining a granted permission to some groups, per model that can carry one: one + * group for a per-user grant, the holder's groups for a permission set rule. The fields are the same + * ones the group manager rules below use. Typed over the schema's list, so a subject made scopable + * there fails to compile here until its group field is named. */ const GROUP_SCOPED_CONDITIONS = { - Assignment: (groupId) => ({ groupId: { in: [groupId] } }), - Group: (groupId) => ({ id: { in: [groupId] } }), - InstrumentRecord: (groupId) => ({ groupId: { in: [groupId] } }), - InstrumentRepo: (groupId) => ({ groupIds: { hasSome: [groupId] } }), - Session: (groupId) => ({ groupId: { in: [groupId] } }), - Subject: (groupId) => ({ groupIds: { hasSome: [groupId] } }), - User: (groupId) => ({ groupIds: { hasSome: [groupId] } }) -} satisfies { [K in GroupScopableSubjectName]: (groupId: string) => NonNullable }; + Assignment: (groupIds) => ({ groupId: { in: groupIds } }), + Group: (groupIds) => ({ id: { in: groupIds } }), + InstrumentRecord: (groupIds) => ({ groupId: { in: groupIds } }), + InstrumentRepo: (groupIds) => ({ groupIds: { hasSome: groupIds } }), + Session: (groupIds) => ({ groupId: { in: groupIds } }), + Subject: (groupIds) => ({ groupIds: { hasSome: groupIds } }), + User: (groupIds) => ({ groupIds: { hasSome: groupIds } }) +} satisfies { [K in GroupScopableSubjectName]: (groupIds: string[]) => NonNullable }; @Injectable() export class AbilityFactory { constructor(private readonly loggingService: LoggingService) {} - createForPayload(payload: Omit): AppAbility { + /** + * `permissionSetRules` are the rules of the set the user holds, read at sign-in rather than carried + * on the payload. Required, so the caller cannot leave a holder with only their base level. + */ + createForPayload(payload: Omit, permissionSetRules: PermissionSetRule[]): AppAbility { this.loggingService.verbose({ message: 'Creating Ability From Payload', payload @@ -87,11 +93,22 @@ export class AbilityFactory { ability.can('read', 'User', { id: payload.id }); break; } + permissionSetRules.forEach(({ action, scope, subject }) => { + if (scope === 'ALL_GROUPS') { + ability.can(action, subject); + } else if (isGroupScopableSubject(subject)) { + ability.can(action, subject, GROUP_SCOPED_CONDITIONS[subject](groupIds)); + } else { + // As below: the schema refuses this shape at the boundary, and neither reading of a stored one + // is what the set's author asked for. + throw new Error(`Cannot confine permission set rule '${action} ${subject}' to the holder's groups`); + } + }); payload.additionalPermissions?.forEach(({ action, groupId, subject }) => { if (groupId === null) { ability.can(action, subject); } else if (isGroupScopableSubject(subject)) { - ability.can(action, subject, GROUP_SCOPED_CONDITIONS[subject](groupId)); + ability.can(action, subject, GROUP_SCOPED_CONDITIONS[subject]([groupId])); } else { // The schema refuses this shape at the boundary; a stored row that has it anyway is applied // under neither reading, since either one silently changes what the grant means. diff --git a/apps/api/src/auth/auth.service.ts b/apps/api/src/auth/auth.service.ts index cb0ad4ff2..c29843f90 100644 --- a/apps/api/src/auth/auth.service.ts +++ b/apps/api/src/auth/auth.service.ts @@ -3,7 +3,7 @@ import type { RequestUser } from '@douglasneuroinformatics/libnest'; import { ForbiddenException, Injectable, NotFoundException, UnauthorizedException } from '@nestjs/common'; import { JwtService } from '@nestjs/jwt'; import type { $LoginCredentials, TokenPayload } from '@opendatacapture/schemas/auth'; -import type { Group, User } from '@prisma/client'; +import type { Group, PermissionSet, User } from '@prisma/client'; import { AuditLogger } from '@/audit/audit.logger'; import { UsersService } from '@/users/users.service'; @@ -41,6 +41,7 @@ export class AuthService { async login(credentials: $LoginCredentials): Promise<{ accessToken: string }> { let user: User & { groups: Group[]; + permissionSet: null | PermissionSet; }; try { user = await this.usersService.findByUsername(credentials.username, { includeHashedPassword: true }); @@ -77,7 +78,9 @@ export class AuthService { username: user.username }; - const ability = this.abilityFactory.createForPayload(tokenPayload); + // The set's rules are resolved here rather than carried on the payload: what they grant is signed + // into `permissions` below, so the token grows only as much as equivalent custom grants would. + const ability = this.abilityFactory.createForPayload(tokenPayload, user.permissionSet?.rules ?? []); const accessToken = await this.jwtService.signAsync( { ...tokenPayload, permissions: ability.rules }, diff --git a/apps/api/src/core/prisma.ts b/apps/api/src/core/prisma.ts index fa55d8f48..aaf22d2a4 100644 --- a/apps/api/src/core/prisma.ts +++ b/apps/api/src/core/prisma.ts @@ -72,6 +72,7 @@ const DATABASE_INDEXES: DatabaseIndex[] = [ { collection: 'InstrumentRepoModel', fields: ['url'], unique: true }, { collection: 'InstrumentRepoSyncLogModel', fields: ['createdAt'] }, { collection: 'InstrumentRepoSyncLogModel', fields: ['repoId', 'createdAt'] }, + { collection: 'PermissionSetModel', fields: ['name'], unique: true }, { collection: 'SessionModel', fields: ['groupId', 'date'] }, { collection: 'SessionModel', fields: ['subjectId'] }, { collection: 'SubjectModel', fields: ['groupIds'] }, diff --git a/apps/api/src/groups/__tests__/groups.controller.test.ts b/apps/api/src/groups/__tests__/groups.controller.test.ts index fb573e46d..7f1723695 100644 --- a/apps/api/src/groups/__tests__/groups.controller.test.ts +++ b/apps/api/src/groups/__tests__/groups.controller.test.ts @@ -60,18 +60,21 @@ describe('GroupsController', () => { // `create Group` declaration admitted every group manager (#1468). describe('route access for create', () => { const abilityFor = (basePermissionLevel: BasePermissionLevel) => { - return new AbilityFactory(MockFactory.createMock(LoggingService) as unknown as LoggingService).createForPayload({ - basePermissionLevel, - firstName: 'Test', - groups: [{ id: 'group-1' }] as Group[], - id: 'user-1', - kind: 'login', - lastName: 'User', - // This suite is testing basePermissionLevel-driven access, not the forced-reset ability, - // which would short-circuit every case below to `read User self` regardless of the role. - mustResetPassword: false, - username: 'test-user' - }); + return new AbilityFactory(MockFactory.createMock(LoggingService) as unknown as LoggingService).createForPayload( + { + basePermissionLevel, + firstName: 'Test', + groups: [{ id: 'group-1' }] as Group[], + id: 'user-1', + kind: 'login', + lastName: 'User', + // This suite is testing basePermissionLevel-driven access, not the forced-reset ability, + // which would short-circuit every case below to `read User self` regardless of the role. + mustResetPassword: false, + username: 'test-user' + }, + [] + ); }; // `@RouteAccess` stores its metadata on the handler itself, which is what the guard reads diff --git a/apps/api/src/instrument-records/__tests__/instrument-records.service.test.ts b/apps/api/src/instrument-records/__tests__/instrument-records.service.test.ts index 39c5062d6..c64e3a2fa 100644 --- a/apps/api/src/instrument-records/__tests__/instrument-records.service.test.ts +++ b/apps/api/src/instrument-records/__tests__/instrument-records.service.test.ts @@ -909,7 +909,8 @@ describe('InstrumentRecordsService', () => { id: 'user-1', lastName: 'User', username: 'test-user' - } as any + } as any, + [] ); }; diff --git a/apps/api/src/instruments/__tests__/instruments.controller.test.ts b/apps/api/src/instruments/__tests__/instruments.controller.test.ts index afcf27c20..b18ebacb3 100644 --- a/apps/api/src/instruments/__tests__/instruments.controller.test.ts +++ b/apps/api/src/instruments/__tests__/instruments.controller.test.ts @@ -79,16 +79,19 @@ describe('InstrumentsController', () => { // and delete their own group's series, which the guard would not tell apart from any other. describe.each(['findSeriesOverview', 'updateSeriesArchive'] as const)('route access for %s', (handlerName) => { const abilityFor = (basePermissionLevel: BasePermissionLevel) => { - return new AbilityFactory(MockFactory.createMock(LoggingService) as unknown as LoggingService).createForPayload({ - basePermissionLevel, - firstName: 'Test', - groups: [{ id: 'group-1' }] as Group[], - id: 'user-1', - kind: 'login', - lastName: 'User', - mustResetPassword: false, - username: 'test-user' - }); + return new AbilityFactory(MockFactory.createMock(LoggingService) as unknown as LoggingService).createForPayload( + { + basePermissionLevel, + firstName: 'Test', + groups: [{ id: 'group-1' }] as Group[], + id: 'user-1', + kind: 'login', + lastName: 'User', + mustResetPassword: false, + username: 'test-user' + }, + [] + ); }; const { action, subject } = new Reflector().get( ROUTE_ACCESS_METADATA_KEY, diff --git a/apps/api/src/instruments/__tests__/instruments.service.test.ts b/apps/api/src/instruments/__tests__/instruments.service.test.ts index 8502319e7..84ae054e8 100644 --- a/apps/api/src/instruments/__tests__/instruments.service.test.ts +++ b/apps/api/src/instruments/__tests__/instruments.service.test.ts @@ -1215,11 +1215,14 @@ describe('InstrumentsService', () => { // `create` but not `read`, and this route is gated on `read Instrument`, which they do hold. it('should resolve for a caller who may read no records, rather than failing the request', async () => { const abilityFactory = new AbilityFactory(MockFactory.createMock(LoggingService) as unknown as LoggingService); - const ability = abilityFactory.createForPayload({ - basePermissionLevel: 'STANDARD', - groups: [{ id: 'group-1' }], - id: 'user-1' - } as any); + const ability = abilityFactory.createForPayload( + { + basePermissionLevel: 'STANDARD', + groups: [{ id: 'group-1' }], + id: 'user-1' + } as any, + [] + ); await expect(instrumentsService.find({ subjectId: 'subject-1' }, { ability })).resolves.toStrictEqual([]); diff --git a/apps/api/src/main.ts b/apps/api/src/main.ts index baff0327c..37a3b49e9 100644 --- a/apps/api/src/main.ts +++ b/apps/api/src/main.ts @@ -11,6 +11,7 @@ import { InstrumentRecordsModule } from './instrument-records/instrument-records import { InstrumentReposModule } from './instrument-repos/instrument-repos.module'; import { InstrumentsModule } from './instruments/instruments.module'; import { MailModule } from './mail/mail.module'; +import { PermissionSetsModule } from './permission-sets/permission-sets.module'; import { SessionsModule } from './sessions/sessions.module'; import { SetupModule } from './setup/setup.module'; import { StorageModule } from './storage/storage.module'; @@ -47,6 +48,7 @@ export default AppFactory.create({ InstrumentReposModule, InstrumentsModule, MailModule, + PermissionSetsModule, PrismaModule.forRootAsync({ useClass: PrismaModuleOptionsFactory }), diff --git a/apps/api/src/permission-sets/__tests__/permission-sets.controller.test.ts b/apps/api/src/permission-sets/__tests__/permission-sets.controller.test.ts new file mode 100644 index 000000000..1d5436b81 --- /dev/null +++ b/apps/api/src/permission-sets/__tests__/permission-sets.controller.test.ts @@ -0,0 +1,108 @@ +import { LoggingService } from '@douglasneuroinformatics/libnest'; +import { MockFactory } from '@douglasneuroinformatics/libnest/testing'; +import type { MockedInstance } from '@douglasneuroinformatics/libnest/testing'; +import { Reflector } from '@nestjs/core'; +import { Test } from '@nestjs/testing'; +import type { Permissions } from '@opendatacapture/schemas/core'; +import type { Group } from '@opendatacapture/schemas/group'; +import type { PermissionSetRule } from '@opendatacapture/schemas/permission-set'; +import type { BasePermissionLevel } from '@opendatacapture/schemas/user'; +import { beforeEach, describe, expect, it } from 'vitest'; + +import { AbilityFactory } from '@/auth/ability.factory'; +import { createAppAbility } from '@/auth/ability.utils'; +import { ROUTE_ACCESS_METADATA_KEY } from '@/core/decorators/route-access.decorator'; +import type { ProtectedRoutePermissionSet } from '@/core/decorators/route-access.decorator'; + +import { PermissionSetsController } from '../permission-sets.controller'; +import { PermissionSetsService } from '../permission-sets.service'; + +const ability = createAppAbility([{ action: 'manage', subject: 'all' }]); + +const RULES: PermissionSetRule[] = [{ action: 'read', scope: 'HOLDER_GROUPS', subject: 'Subject' }]; + +describe('PermissionSetsController', () => { + let permissionSetsController: PermissionSetsController; + let permissionSetsService: MockedInstance; + + beforeEach(async () => { + const moduleRef = await Test.createTestingModule({ + controllers: [PermissionSetsController], + providers: [MockFactory.createForService(PermissionSetsService)] + }).compile(); + permissionSetsController = moduleRef.get(PermissionSetsController); + permissionSetsService = moduleRef.get(PermissionSetsService); + }); + + it('should pass a new set to the service as received, since the body is already validated', async () => { + await permissionSetsController.create({ name: 'Research Assistant', rules: RULES }); + expect(permissionSetsService.create).toHaveBeenCalledWith({ name: 'Research Assistant', rules: RULES }); + }); + + it('should forward the caller ability when listing sets, so the query is scoped', async () => { + await permissionSetsController.findAll(ability); + expect(permissionSetsService.findAll).toHaveBeenCalledWith({ ability }); + }); + + it('should forward the caller ability when reading a set, so the query is scoped', async () => { + await permissionSetsController.findById('set-1', ability); + expect(permissionSetsService.findById).toHaveBeenCalledWith('set-1', { ability }); + }); + + it('should forward the caller ability when updating a set, so the write is scoped', async () => { + await permissionSetsController.updateById('set-1', { name: 'Renamed' }, ability); + expect(permissionSetsService.updateById).toHaveBeenCalledWith('set-1', { name: 'Renamed' }, { ability }); + }); + + it('should forward the caller ability when deleting a set, so the write is scoped', async () => { + await permissionSetsController.deleteById('set-1', ability); + expect(permissionSetsService.deleteById).toHaveBeenCalledWith('set-1', { ability }); + }); + + // What `JwtAuthGuard` itself computes for each route: the declared access, evaluated against a whole + // ability. Editing a set is a grant to each of its holders, so nothing short of an administrator may. + describe('route access', () => { + const HANDLERS = ['create', 'deleteById', 'findAll', 'findById', 'updateById'] as const; + + const abilityFor = (basePermissionLevel: BasePermissionLevel, additionalPermissions: Permissions = []) => { + return new AbilityFactory(MockFactory.createMock(LoggingService) as unknown as LoggingService).createForPayload( + { + additionalPermissions, + basePermissionLevel, + firstName: 'Test', + groups: [{ id: 'group-1' }] as Group[], + id: 'user-1', + kind: 'login', + lastName: 'User', + mustResetPassword: false, + username: 'test-user' + }, + [] + ); + }; + + const declaredAccess = (handler: (typeof HANDLERS)[number]) => { + return new Reflector().get( + ROUTE_ACCESS_METADATA_KEY, + Object.getOwnPropertyDescriptor(PermissionSetsController.prototype, handler)!.value + ); + }; + + it.each(HANDLERS)( + 'should refuse %s to a group manager granted every action but manage on everything, which a set may hold', + (handler) => { + const { action, subject } = declaredAccess(handler); + const grantee = abilityFor( + 'GROUP_MANAGER', + (['create', 'delete', 'read', 'update'] as const).map((action) => ({ action, groupId: null, subject: 'all' })) + ); + expect(grantee.can(action, subject)).toBe(false); + } + ); + + it.each(HANDLERS)('should allow %s to an administrator', (handler) => { + const { action, subject } = declaredAccess(handler); + expect(abilityFor('ADMIN').can(action, subject)).toBe(true); + }); + }); +}); diff --git a/apps/api/src/permission-sets/__tests__/permission-sets.service.test.ts b/apps/api/src/permission-sets/__tests__/permission-sets.service.test.ts new file mode 100644 index 000000000..fdecb733a --- /dev/null +++ b/apps/api/src/permission-sets/__tests__/permission-sets.service.test.ts @@ -0,0 +1,134 @@ +import type { Model } from '@douglasneuroinformatics/libnest'; +import { getModelToken } from '@douglasneuroinformatics/libnest'; +import { MockFactory } from '@douglasneuroinformatics/libnest/testing'; +import type { MockedInstance } from '@douglasneuroinformatics/libnest/testing'; +import { ConflictException, NotFoundException } from '@nestjs/common'; +import { Test } from '@nestjs/testing'; +import type { PermissionSetRule } from '@opendatacapture/schemas/permission-set'; +import { beforeEach, describe, expect, it } from 'vitest'; + +import { accessibleQuery, createAppAbility } from '@/auth/ability.utils'; + +import { PermissionSetsService } from '../permission-sets.service'; + +const ability = createAppAbility([{ action: 'manage', subject: 'all' }]); + +const RULES: PermissionSetRule[] = [{ action: 'read', scope: 'HOLDER_GROUPS', subject: 'Subject' }]; + +const STORED_SET = { _count: { users: 3 }, id: 'set-1', name: 'Research Assistant', rules: RULES }; + +describe('PermissionSetsService', () => { + let permissionSetsService: PermissionSetsService; + let permissionSetModel: MockedInstance>; + + beforeEach(async () => { + const moduleRef = await Test.createTestingModule({ + providers: [PermissionSetsService, MockFactory.createForModelToken(getModelToken('PermissionSet'))] + }).compile(); + permissionSetModel = moduleRef.get(getModelToken('PermissionSet')); + permissionSetsService = moduleRef.get(PermissionSetsService); + permissionSetModel.exists.mockResolvedValue(false); + permissionSetModel.findFirst.mockResolvedValue(STORED_SET); + }); + + describe('create', () => { + it('should store the name and rules', async () => { + await permissionSetsService.create({ name: 'Research Assistant', rules: RULES }); + expect(permissionSetModel.create.mock.lastCall?.[0]).toEqual({ + data: { name: 'Research Assistant', rules: RULES } + }); + }); + + it('should refuse a name another set already has, before writing anything', async () => { + permissionSetModel.exists.mockResolvedValue(true); + await expect(permissionSetsService.create({ name: 'Research Assistant', rules: RULES })).rejects.toThrow( + ConflictException + ); + expect(permissionSetModel.create).not.toHaveBeenCalled(); + }); + }); + + describe('findAll', () => { + it('should return each set with the number of users holding it', async () => { + permissionSetModel.findMany.mockResolvedValue([STORED_SET]); + await expect(permissionSetsService.findAll({ ability })).resolves.toEqual([ + { holderCount: 3, id: 'set-1', name: 'Research Assistant', rules: RULES } + ]); + }); + + it('should scope the listing to the caller ability', async () => { + permissionSetModel.findMany.mockResolvedValue([]); + await permissionSetsService.findAll({ ability }); + expect(permissionSetModel.findMany.mock.lastCall?.[0]).toMatchObject({ + where: accessibleQuery(ability, 'read', 'PermissionSet') + }); + }); + }); + + describe('findById', () => { + it('should return the set with the number of users holding it, found within the caller read scope', async () => { + await expect(permissionSetsService.findById('set-1', { ability })).resolves.toEqual({ + holderCount: 3, + id: 'set-1', + name: 'Research Assistant', + rules: RULES + }); + expect(permissionSetModel.findFirst.mock.lastCall?.[0]).toMatchObject({ + where: { AND: [accessibleQuery(ability, 'read', 'PermissionSet')], id: 'set-1' } + }); + }); + + it('should throw when the set cannot be found', async () => { + permissionSetModel.findFirst.mockResolvedValue(null); + await expect(permissionSetsService.findById('set-1')).rejects.toThrow(NotFoundException); + }); + }); + + describe('updateById', () => { + it('should replace the rules wholesale, as a composite list must be on MongoDB', async () => { + await permissionSetsService.updateById('set-1', { rules: RULES }, { ability }); + expect(permissionSetModel.update.mock.lastCall?.[0]).toMatchObject({ + data: { rules: { set: RULES } }, + where: { AND: [accessibleQuery(ability, 'update', 'PermissionSet')], id: 'set-1' } + }); + }); + + it('should leave the rules alone when they are not being changed', async () => { + await permissionSetsService.updateById('set-1', { name: 'Renamed' }); + expect(permissionSetModel.update.mock.lastCall?.[0].data).toEqual({ name: 'Renamed', rules: undefined }); + }); + + it('should refuse a rename to a name another set already has', async () => { + permissionSetModel.exists.mockResolvedValue(true); + await expect(permissionSetsService.updateById('set-1', { name: 'Taken' })).rejects.toThrow(ConflictException); + expect(permissionSetModel.update).not.toHaveBeenCalled(); + }); + + it('should not look for a collision when the name is unchanged, since it would always find this set', async () => { + await permissionSetsService.updateById('set-1', { name: 'Research Assistant' }); + expect(permissionSetModel.exists).not.toHaveBeenCalled(); + expect(permissionSetModel.update).toHaveBeenCalledOnce(); + }); + + it('should throw when the set cannot be found, before writing anything', async () => { + permissionSetModel.findFirst.mockResolvedValue(null); + await expect(permissionSetsService.updateById('set-1', { name: 'Renamed' })).rejects.toThrow(NotFoundException); + expect(permissionSetModel.update).not.toHaveBeenCalled(); + }); + }); + + describe('deleteById', () => { + it('should delete the set within the caller delete scope', async () => { + await permissionSetsService.deleteById('set-1', { ability }); + expect(permissionSetModel.delete.mock.lastCall?.[0]).toEqual({ + where: { AND: [accessibleQuery(ability, 'delete', 'PermissionSet')], id: 'set-1' } + }); + }); + + it('should throw when the set cannot be found, rather than failing inside the delete', async () => { + permissionSetModel.findFirst.mockResolvedValue(null); + await expect(permissionSetsService.deleteById('set-1')).rejects.toThrow(NotFoundException); + expect(permissionSetModel.delete).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/apps/api/src/permission-sets/permission-sets.controller.ts b/apps/api/src/permission-sets/permission-sets.controller.ts new file mode 100644 index 000000000..93df6a4a0 --- /dev/null +++ b/apps/api/src/permission-sets/permission-sets.controller.ts @@ -0,0 +1,56 @@ +import { ApiOperation, CurrentUser, ValidObjectIdPipe } from '@douglasneuroinformatics/libnest'; +import { Body, Controller, Delete, Get, Param, Patch, Post } from '@nestjs/common'; +import { $CreatePermissionSetData, $UpdatePermissionSetData } from '@opendatacapture/schemas/permission-set'; + +import type { AppAbility } from '@/auth/auth.types'; +import { ADMIN_ONLY, RouteAccess } from '@/core/decorators/route-access.decorator'; + +import { PermissionSetsService } from './permission-sets.service'; + +/** + * Every route is admin-only, reads included. Editing a set changes what each of its holders may do, + * so a write is a grant, and only an administrator may grant; holders have no use for reading it. + */ +@Controller('permission-sets') +export class PermissionSetsController { + constructor(private readonly permissionSetsService: PermissionSetsService) {} + + @ApiOperation({ summary: 'Create Permission Set' }) + @Post() + @RouteAccess(ADMIN_ONLY) + create(@Body() data: $CreatePermissionSetData) { + return this.permissionSetsService.create(data); + } + + @ApiOperation({ summary: 'Delete Permission Set' }) + @Delete(':id') + @RouteAccess(ADMIN_ONLY) + deleteById(@Param('id', ValidObjectIdPipe) id: string, @CurrentUser('ability') ability: AppAbility) { + return this.permissionSetsService.deleteById(id, { ability }); + } + + @ApiOperation({ summary: 'Get All Permission Sets' }) + @Get() + @RouteAccess(ADMIN_ONLY) + findAll(@CurrentUser('ability') ability: AppAbility) { + return this.permissionSetsService.findAll({ ability }); + } + + @ApiOperation({ summary: 'Get Permission Set' }) + @Get(':id') + @RouteAccess(ADMIN_ONLY) + findById(@Param('id', ValidObjectIdPipe) id: string, @CurrentUser('ability') ability: AppAbility) { + return this.permissionSetsService.findById(id, { ability }); + } + + @ApiOperation({ summary: 'Update Permission Set' }) + @Patch(':id') + @RouteAccess(ADMIN_ONLY) + updateById( + @Param('id', ValidObjectIdPipe) id: string, + @Body() update: $UpdatePermissionSetData, + @CurrentUser('ability') ability: AppAbility + ) { + return this.permissionSetsService.updateById(id, update, { ability }); + } +} diff --git a/apps/api/src/permission-sets/permission-sets.module.ts b/apps/api/src/permission-sets/permission-sets.module.ts new file mode 100644 index 000000000..756d38498 --- /dev/null +++ b/apps/api/src/permission-sets/permission-sets.module.ts @@ -0,0 +1,11 @@ +import { Module } from '@nestjs/common'; + +import { PermissionSetsController } from './permission-sets.controller'; +import { PermissionSetsService } from './permission-sets.service'; + +@Module({ + controllers: [PermissionSetsController], + exports: [PermissionSetsService], + providers: [PermissionSetsService] +}) +export class PermissionSetsModule {} diff --git a/apps/api/src/permission-sets/permission-sets.service.ts b/apps/api/src/permission-sets/permission-sets.service.ts new file mode 100644 index 000000000..35fcee5e2 --- /dev/null +++ b/apps/api/src/permission-sets/permission-sets.service.ts @@ -0,0 +1,75 @@ +import { InjectModel } from '@douglasneuroinformatics/libnest'; +import type { Model } from '@douglasneuroinformatics/libnest'; +import { ConflictException, Injectable, NotFoundException } from '@nestjs/common'; +import { $CreatePermissionSetData, $UpdatePermissionSetData } from '@opendatacapture/schemas/permission-set'; + +import { accessibleQuery } from '@/auth/ability.utils'; +import type { EntityOperationOptions } from '@/core/types'; + +const INCLUDE_HOLDER_COUNT = { _count: { select: { users: true } } } as const; + +function withHolderCount({ + _count, + ...permissionSet +}: TPermissionSet) { + return { ...permissionSet, holderCount: _count.users }; +} + +@Injectable() +export class PermissionSetsService { + constructor(@InjectModel('PermissionSet') private readonly permissionSetModel: Model<'PermissionSet'>) {} + + async create({ name, rules }: $CreatePermissionSetData) { + await this.assertNameIsFree(name); + return this.permissionSetModel.create({ + data: { name, rules } + }); + } + + /** Every holder's `permissionSetId` is cleared by the relation's `onDelete: SetNull`. */ + async deleteById(id: string, { ability }: EntityOperationOptions = {}) { + await this.findById(id, { ability }); + return this.permissionSetModel.delete({ + where: { AND: [accessibleQuery(ability, 'delete', 'PermissionSet')], id } + }); + } + + async findAll({ ability }: EntityOperationOptions = {}) { + const permissionSets = await this.permissionSetModel.findMany({ + include: INCLUDE_HOLDER_COUNT, + orderBy: { name: 'asc' }, + where: accessibleQuery(ability, 'read', 'PermissionSet') + }); + return permissionSets.map(withHolderCount); + } + + async findById(id: string, { ability }: EntityOperationOptions = {}) { + const permissionSet = await this.permissionSetModel.findFirst({ + include: INCLUDE_HOLDER_COUNT, + where: { AND: [accessibleQuery(ability, 'read', 'PermissionSet')], id } + }); + if (!permissionSet) { + throw new NotFoundException(`Failed to find permission set with ID: ${id}`); + } + return withHolderCount(permissionSet); + } + + async updateById(id: string, { name, rules }: $UpdatePermissionSetData, { ability }: EntityOperationOptions = {}) { + const permissionSet = await this.findById(id, { ability }); + if (name !== undefined && name !== permissionSet.name) { + await this.assertNameIsFree(name); + } + return this.permissionSetModel.update({ + // Composite list fields must be replaced via `set` in the MongoDB connector. + data: { name, rules: rules ? { set: rules } : undefined }, + where: { AND: [accessibleQuery(ability, 'update', 'PermissionSet')], id } + }); + } + + /** Unscoped on purpose: a name must be unique across every set, not only those the caller can read. */ + private async assertNameIsFree(name: string) { + if (await this.permissionSetModel.exists({ name })) { + throw new ConflictException(`Permission set with name '${name}' already exists!`); + } + } +} diff --git a/apps/api/src/subjects/__tests__/subjects.service.test.ts b/apps/api/src/subjects/__tests__/subjects.service.test.ts index b48033bb4..15f2c5b88 100644 --- a/apps/api/src/subjects/__tests__/subjects.service.test.ts +++ b/apps/api/src/subjects/__tests__/subjects.service.test.ts @@ -248,16 +248,19 @@ describe('SubjectsService', () => { // for the subject at all, which escapes as a 500 rather than the empty list they should see. it('should return an empty list for a caller who may read no records, rather than throwing', async () => { const abilityFactory = new AbilityFactory(MockFactory.createMock(LoggingService) as unknown as LoggingService); - const ability = abilityFactory.createForPayload({ - basePermissionLevel: 'STANDARD', - firstName: null, - groups: [], - id: 'user-1', - kind: 'login', - lastName: null, - mustResetPassword: false, - username: 'standard-user' - }); + const ability = abilityFactory.createForPayload( + { + basePermissionLevel: 'STANDARD', + firstName: null, + groups: [], + id: 'user-1', + kind: 'login', + lastName: null, + mustResetPassword: false, + username: 'standard-user' + }, + [] + ); subjectModel.findMany.mockResolvedValueOnce([]); await expect(subjectsService.find({ hasRecord: true }, { ability })).resolves.toStrictEqual([]); diff --git a/apps/api/src/users/__tests__/users.controller.test.ts b/apps/api/src/users/__tests__/users.controller.test.ts index 8ec067db5..4cbcf66ba 100644 --- a/apps/api/src/users/__tests__/users.controller.test.ts +++ b/apps/api/src/users/__tests__/users.controller.test.ts @@ -219,10 +219,23 @@ describe('UsersController', () => { }); describe('updatePermissions', () => { - it('should forward the caller ability, so the write is scoped', async () => { + it('should forward custom grants with the caller ability, so the write is scoped', async () => { const permissions: Permissions = [{ action: 'read', groupId: 'group-1', subject: 'Subject' }]; - await usersController.updatePermissions('user-1', { permissions }, ability); - expect(usersService.updatePermissions).toHaveBeenCalledWith('user-1', permissions, { ability }); + await usersController.updatePermissions('user-1', { kind: 'custom', permissions }, ability); + expect(usersService.updatePermissions).toHaveBeenCalledWith( + 'user-1', + { kind: 'custom', permissions }, + { ability } + ); + }); + + it('should forward a permission set with the caller ability, so the write is scoped', async () => { + await usersController.updatePermissions('user-1', { kind: 'permissionSet', permissionSetId: 'set-1' }, ability); + expect(usersService.updatePermissions).toHaveBeenCalledWith( + 'user-1', + { kind: 'permissionSet', permissionSetId: 'set-1' }, + { ability } + ); }); }); @@ -233,17 +246,20 @@ describe('UsersController', () => { const WRITE_HANDLERS = ['archiveById', 'create', 'unarchiveById', 'updateById', 'updatePermissions'] as const; const abilityFor = (basePermissionLevel: BasePermissionLevel, additionalPermissions: Permissions = []) => { - return new AbilityFactory(MockFactory.createMock(LoggingService) as unknown as LoggingService).createForPayload({ - additionalPermissions, - basePermissionLevel, - firstName: 'Test', - groups: [{ id: 'group-1' }] as Group[], - id: 'user-1', - kind: 'login', - lastName: 'User', - mustResetPassword: false, - username: 'test-user' - }); + return new AbilityFactory(MockFactory.createMock(LoggingService) as unknown as LoggingService).createForPayload( + { + additionalPermissions, + basePermissionLevel, + firstName: 'Test', + groups: [{ id: 'group-1' }] as Group[], + id: 'user-1', + kind: 'login', + lastName: 'User', + mustResetPassword: false, + username: 'test-user' + }, + [] + ); }; const declaredAccess = (handler: (typeof WRITE_HANDLERS)[number]) => { diff --git a/apps/api/src/users/__tests__/users.service.test.ts b/apps/api/src/users/__tests__/users.service.test.ts index 78062ad45..7ab09402d 100644 --- a/apps/api/src/users/__tests__/users.service.test.ts +++ b/apps/api/src/users/__tests__/users.service.test.ts @@ -6,6 +6,7 @@ import { estimatePasswordStrength } from '@douglasneuroinformatics/libpasswd'; import { BadRequestException, ConflictException, ForbiddenException, NotFoundException } from '@nestjs/common'; import { Test } from '@nestjs/testing'; import type { Permissions } from '@opendatacapture/schemas/core'; +import type { $UpdateUserPermissionsData } from '@opendatacapture/schemas/user'; import { pwnedPassword } from 'hibp'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import type { Mock } from 'vitest'; @@ -14,6 +15,7 @@ import { AuditLogger } from '@/audit/audit.logger'; import { accessibleQuery, createAppAbility } from '@/auth/ability.utils'; import { GroupsService } from '../../groups/groups.service'; +import { PermissionSetsService } from '../../permission-sets/permission-sets.service'; import { UsersService } from '../users.service'; vi.mock('hibp', () => ({ pwnedPassword: vi.fn() })); @@ -38,6 +40,7 @@ describe('UsersService', () => { let userModel: MockedInstance>; let cryptoService: MockedInstance; let groupsService: MockedInstance; + let permissionSetsService: MockedInstance; beforeEach(async () => { vi.clearAllMocks(); @@ -47,13 +50,15 @@ describe('UsersService', () => { MockFactory.createForModelToken(getModelToken('User')), MockFactory.createForService(AuditLogger), MockFactory.createForService(CryptoService), - MockFactory.createForService(GroupsService) + MockFactory.createForService(GroupsService), + MockFactory.createForService(PermissionSetsService) ] }).compile(); userModel = moduleRef.get(getModelToken('User')); usersService = moduleRef.get(UsersService); cryptoService = moduleRef.get(CryptoService); groupsService = moduleRef.get(GroupsService); + permissionSetsService = moduleRef.get(PermissionSetsService); userModel.exists.mockResolvedValue(false); userModel.create.mockResolvedValue({}); @@ -143,6 +148,24 @@ describe('UsersService', () => { await usersService.create({ ...baseUser }); expect(userModel.create.mock.lastCall?.[0].data.additionalPermissions).toEqual([]); }); + + it('should start the user with no permission set when none is requested', async () => { + await usersService.create({ ...baseUser }); + expect(userModel.create.mock.lastCall?.[0].data.permissionSet).toBeUndefined(); + expect(permissionSetsService.findById).not.toHaveBeenCalled(); + }); + + it('should link the requested permission set, looked up with the caller options', async () => { + await usersService.create({ ...baseUser, permissionSetId: 'set-1' }, { ability: admin.ability }); + expect(permissionSetsService.findById).toHaveBeenCalledWith('set-1', { ability: admin.ability }); + expect(userModel.create.mock.lastCall?.[0].data.permissionSet).toEqual({ connect: { id: 'set-1' } }); + }); + + it('should propagate a permission set lookup that finds nothing, so the user is never created', async () => { + permissionSetsService.findById.mockRejectedValue(new NotFoundException()); + await expect(usersService.create({ ...baseUser, permissionSetId: 'set-1' })).rejects.toThrow(NotFoundException); + expect(userModel.create).not.toHaveBeenCalled(); + }); }); describe('find', () => { @@ -313,6 +336,19 @@ describe('UsersService', () => { expect(userModel.findFirst).not.toHaveBeenCalled(); }); + it('should remove the permission set of a user promoted to administrator, who already holds every permission', async () => { + await usersService.updateById('user-1', { basePermissionLevel: 'ADMIN' }, admin); + expect(userModel.update.mock.lastCall?.[0].data.permissionSet).toEqual({ disconnect: true }); + }); + + it.each(['GROUP_MANAGER', 'STANDARD', undefined] as const)( + 'should leave the permission set alone when the level becomes %s', + async (basePermissionLevel) => { + await usersService.updateById('user-1', { basePermissionLevel, firstName: 'Janet' }, admin); + expect(userModel.update.mock.lastCall?.[0].data.permissionSet).toBeUndefined(); + } + ); + it('should let an administrator disable and demote another user', async () => { await usersService.updateById('user-1', { basePermissionLevel: 'STANDARD', disabled: true }, admin); expect(userModel.update.mock.lastCall?.[0].data).toMatchObject({ @@ -357,42 +393,85 @@ describe('UsersService', () => { describe('updatePermissions', () => { beforeEach(() => { - userModel.findFirst.mockResolvedValue({ groupIds: ['group-1'], id: 'user-1' }); + userModel.findFirst.mockResolvedValue({ basePermissionLevel: 'STANDARD', groupIds: ['group-1'], id: 'user-1' }); userModel.update.mockResolvedValue({}); }); - it('should write a grant confined to a group the user belongs to', async () => { - const permissions: Permissions = [{ action: 'read', groupId: 'group-1', subject: 'Subject' }]; - await usersService.updatePermissions('user-1', permissions); - expect(userModel.update.mock.lastCall?.[0]).toMatchObject({ - data: { additionalPermissions: permissions }, - where: { id: 'user-1' } + describe('custom grants', () => { + it('should write a grant confined to a group the user belongs to', async () => { + const permissions: Permissions = [{ action: 'read', groupId: 'group-1', subject: 'Subject' }]; + await usersService.updatePermissions('user-1', { kind: 'custom', permissions }); + expect(userModel.update.mock.lastCall?.[0]).toMatchObject({ + data: { additionalPermissions: permissions }, + where: { id: 'user-1' } + }); }); - }); - it('should reject a grant confined to a group the user does not belong to, before writing anything', async () => { - const permissions: Permissions = [{ action: 'read', groupId: 'group-2', subject: 'Subject' }]; - await expect(usersService.updatePermissions('user-1', permissions)).rejects.toThrow(BadRequestException); - expect(userModel.update).not.toHaveBeenCalled(); + it('should remove the permission set in the same write, so the user never holds both', async () => { + await usersService.updatePermissions('user-1', { kind: 'custom', permissions: [] }); + expect(userModel.update.mock.lastCall?.[0].data.permissionSet).toEqual({ disconnect: true }); + }); + + it('should reject a grant confined to a group the user does not belong to, before writing anything', async () => { + const permissions: Permissions = [{ action: 'read', groupId: 'group-2', subject: 'Subject' }]; + await expect(usersService.updatePermissions('user-1', { kind: 'custom', permissions })).rejects.toThrow( + BadRequestException + ); + expect(userModel.update).not.toHaveBeenCalled(); + }); + + it("should accept an unscoped grant whatever the user's groups", async () => { + const permissions: Permissions = [{ action: 'create', groupId: null, subject: 'Instrument' }]; + await usersService.updatePermissions('user-1', { kind: 'custom', permissions }); + expect(userModel.update.mock.lastCall?.[0]).toMatchObject({ data: { additionalPermissions: permissions } }); + }); }); - it("should accept an unscoped grant whatever the user's groups", async () => { - const permissions: Permissions = [{ action: 'create', groupId: null, subject: 'Instrument' }]; - await usersService.updatePermissions('user-1', permissions); - expect(userModel.update.mock.lastCall?.[0]).toMatchObject({ data: { additionalPermissions: permissions } }); + describe('a permission set', () => { + const data = { kind: 'permissionSet', permissionSetId: 'set-1' } as const; + + it('should link the set and empty the custom grants in the same write, so the user never holds both', async () => { + await usersService.updatePermissions('user-1', data); + expect(userModel.update.mock.lastCall?.[0]).toMatchObject({ + data: { additionalPermissions: [], permissionSet: { connect: { id: 'set-1' } } }, + where: { id: 'user-1' } + }); + }); + + it('should look up the set with the caller ability', async () => { + await usersService.updatePermissions('user-1', data, { ability: admin.ability }); + expect(permissionSetsService.findById).toHaveBeenCalledWith('set-1', { ability: admin.ability }); + }); + + it('should propagate a set lookup that finds nothing, before writing anything', async () => { + permissionSetsService.findById.mockRejectedValue(new NotFoundException()); + await expect(usersService.updatePermissions('user-1', data)).rejects.toThrow(NotFoundException); + expect(userModel.update).not.toHaveBeenCalled(); + }); + + it('should refuse a set for an administrator, who already holds every permission', async () => { + userModel.findFirst.mockResolvedValue({ basePermissionLevel: 'ADMIN', groupIds: [], id: 'user-1' }); + await expect(usersService.updatePermissions('user-1', data)).rejects.toThrow(BadRequestException); + expect(userModel.update).not.toHaveBeenCalled(); + }); }); it('should throw when the user cannot be found, rather than creating one', async () => { userModel.findFirst.mockResolvedValue(null); - await expect(usersService.updatePermissions('user-1', [])).rejects.toThrow(NotFoundException); + await expect(usersService.updatePermissions('user-1', { kind: 'custom', permissions: [] })).rejects.toThrow( + NotFoundException + ); expect(userModel.update).not.toHaveBeenCalled(); }); - it('should scope the write to the caller ability', async () => { + it.each<$UpdateUserPermissionsData>([ + { kind: 'custom', permissions: [] }, + { kind: 'permissionSet', permissionSetId: 'set-1' } + ])('should scope a $kind write to the caller ability', async (data) => { const ability = createAppAbility([ { action: 'manage', conditions: { groupIds: { hasSome: ['group-1'] } }, subject: 'User' } ]); - await usersService.updatePermissions('user-1', [], { ability }); + await usersService.updatePermissions('user-1', data, { ability }); expect(userModel.update.mock.lastCall?.[0]).toMatchObject({ where: { AND: [accessibleQuery(ability, 'update', 'User')], id: 'user-1' } }); diff --git a/apps/api/src/users/users.controller.ts b/apps/api/src/users/users.controller.ts index 8f91529a8..0e0287b40 100644 --- a/apps/api/src/users/users.controller.ts +++ b/apps/api/src/users/users.controller.ts @@ -104,10 +104,10 @@ export class UsersController { @RouteAccess(ADMIN_ONLY) updatePermissions( @Param('id') id: string, - @Body() { permissions }: $UpdateUserPermissionsData, + @Body() data: $UpdateUserPermissionsData, @CurrentUser('ability') ability: AppAbility ) { - return this.usersService.updatePermissions(id, permissions, { ability }); + return this.usersService.updatePermissions(id, data, { ability }); } @ApiOperation({ summary: 'Self Update User' }) diff --git a/apps/api/src/users/users.module.ts b/apps/api/src/users/users.module.ts index ca53f69da..2fbf8195d 100644 --- a/apps/api/src/users/users.module.ts +++ b/apps/api/src/users/users.module.ts @@ -2,6 +2,7 @@ import { Module } from '@nestjs/common'; import { GroupsModule } from '@/groups/groups.module'; import { MailModule } from '@/mail/mail.module'; +import { PermissionSetsModule } from '@/permission-sets/permission-sets.module'; import { UsersController } from './users.controller'; import { UsersService } from './users.service'; @@ -9,7 +10,7 @@ import { UsersService } from './users.service'; @Module({ controllers: [UsersController], exports: [UsersService], - imports: [GroupsModule, MailModule], + imports: [GroupsModule, MailModule, PermissionSetsModule], providers: [UsersService] }) export class UsersModule {} diff --git a/apps/api/src/users/users.service.ts b/apps/api/src/users/users.service.ts index 735e6368a..ff696c8ea 100644 --- a/apps/api/src/users/users.service.ts +++ b/apps/api/src/users/users.service.ts @@ -9,8 +9,12 @@ import { Logger, NotFoundException } from '@nestjs/common'; -import type { Permissions } from '@opendatacapture/schemas/core'; -import { $CreateUserData, $SelfUpdateUserData, $UpdateUserData } from '@opendatacapture/schemas/user'; +import { + $CreateUserData, + $SelfUpdateUserData, + $UpdateUserData, + $UpdateUserPermissionsData +} from '@opendatacapture/schemas/user'; import type { PasswordErrorCode } from '@opendatacapture/schemas/user'; import { pwnedPassword } from 'hibp'; @@ -18,6 +22,7 @@ import { AuditLogger } from '@/audit/audit.logger'; import { accessibleQuery } from '@/auth/ability.utils'; import type { EntityOperationOptions } from '@/core/types'; import { GroupsService } from '@/groups/groups.service'; +import { PermissionSetsService } from '@/permission-sets/permission-sets.service'; @Injectable() export class UsersService { @@ -27,7 +32,8 @@ export class UsersService { @InjectModel('User') private readonly userModel: Model<'User'>, private readonly auditLogger: AuditLogger, private readonly cryptoService: CryptoService, - private readonly groupsService: GroupsService + private readonly groupsService: GroupsService, + private readonly permissionSetsService: PermissionSetsService ) {} async archiveById(id: string, currentUser: RequestUser) { @@ -72,7 +78,7 @@ export class UsersService { }); } - /** Adds a new user to the database with default permissions, verifying the provided groups exist */ + /** Adds a new user to the database with default permissions, verifying the provided groups and permission set exist */ async create( { basePermissionLevel, @@ -84,6 +90,7 @@ export class UsersService { lastName, mustResetPassword, password, + permissionSetId, phoneNumber, sex, username @@ -100,6 +107,9 @@ export class UsersService { for (const id of groupIds) { await this.groupsService.findById(id, options); } + if (permissionSetId !== undefined) { + await this.permissionSetsService.findById(permissionSetId, options); + } const hashedPassword = await this.cryptoService.hashPassword(password); @@ -117,6 +127,7 @@ export class UsersService { hashedPassword, lastName, mustResetPassword, + permissionSet: permissionSetId === undefined ? undefined : { connect: { id: permissionSetId } }, phoneNumber, sex, username @@ -156,7 +167,7 @@ export class UsersService { { ability, includeHashedPassword }: EntityOperationOptions & { includeHashedPassword?: boolean } = {} ) { const user = await this.userModel.findFirst({ - include: { groups: true }, + include: { groups: true, permissionSet: true }, omit: { hashedPassword: !includeHashedPassword }, @@ -208,7 +219,10 @@ export class UsersService { groups: { set: groupIds?.map((id) => ({ id })) }, - hashedPassword + hashedPassword, + // An administrator already holds every permission, so a set they kept would only inflate its + // holder count. Relation syntax because `groups` above is one, and Prisma will not mix the two. + permissionSet: data.basePermissionLevel === 'ADMIN' ? { disconnect: true } : undefined }, omit: { hashedPassword: true @@ -218,12 +232,28 @@ export class UsersService { } /** - * Replaces the user's additional permissions. A grant confined to a group is accepted only when the - * user belongs to that group, so what the web client offers and what this accepts are one rule. + * Replaces where the user's permissions beyond their base level come from: a permission set, or + * custom grants. Each write empties the other, which is what keeps a user from holding both. A + * custom grant confined to a group is accepted only when the user belongs to that group, so what + * the web client offers and what this accepts are one rule. */ - async updatePermissions(id: string, permissions: Permissions, { ability }: EntityOperationOptions = {}) { + async updatePermissions(id: string, data: $UpdateUserPermissionsData, { ability }: EntityOperationOptions = {}) { const user = await this.findById(id, { ability }); - const foreignGroupIds = permissions + const where = { AND: [accessibleQuery(ability, 'update', 'User')], id }; + if (data.kind === 'permissionSet') { + if (user.basePermissionLevel === 'ADMIN') { + throw new BadRequestException('Administrators hold every permission, so they cannot hold a permission set'); + } + await this.permissionSetsService.findById(data.permissionSetId, { ability }); + return this.userModel.update({ + data: { additionalPermissions: [], permissionSet: { connect: { id: data.permissionSetId } } }, + omit: { + hashedPassword: true + }, + where + }); + } + const foreignGroupIds = data.permissions .map(({ groupId }) => groupId) .filter((groupId): groupId is string => groupId !== null && !user.groupIds.includes(groupId)); if (foreignGroupIds.length) { @@ -232,11 +262,11 @@ export class UsersService { ); } return this.userModel.update({ - data: { additionalPermissions: permissions }, + data: { additionalPermissions: data.permissions, permissionSet: { disconnect: true } }, omit: { hashedPassword: true }, - where: { AND: [accessibleQuery(ability, 'update', 'User')], id } + where }); } diff --git a/apps/api/test/suites/02-user-permissions.suite.ts b/apps/api/test/suites/02-user-permissions.suite.ts index 2f55d8a9a..e2c6167f6 100644 --- a/apps/api/test/suites/02-user-permissions.suite.ts +++ b/apps/api/test/suites/02-user-permissions.suite.ts @@ -79,7 +79,7 @@ export default defineSuite('user permissions', function () { const response = await this.app.inject({ headers: adminHeaders, method: 'PUT', - payload: { permissions: [{ action: 'read', groupId: otherGroup.id, subject: 'Subject' }] }, + payload: { kind: 'custom', permissions: [{ action: 'read', groupId: otherGroup.id, subject: 'Subject' }] }, url: `/v1/users/${user.id}/permissions` }); expect(response.statusCode).toBe(400); @@ -89,7 +89,7 @@ export default defineSuite('user permissions', function () { const response = await this.app.inject({ headers: adminHeaders, method: 'PUT', - payload: { permissions: [{ action: 'read', groupId: group.id, subject: 'Subject' }] }, + payload: { kind: 'custom', permissions: [{ action: 'read', groupId: group.id, subject: 'Subject' }] }, url: `/v1/users/${user.id}/permissions` }); expect(response.statusCode).toBe(200); diff --git a/apps/api/test/suites/03-record-file-deletion.suite.ts b/apps/api/test/suites/03-record-file-deletion.suite.ts index 1a6cda61f..0c4f2ee66 100644 --- a/apps/api/test/suites/03-record-file-deletion.suite.ts +++ b/apps/api/test/suites/03-record-file-deletion.suite.ts @@ -91,6 +91,7 @@ export default defineSuite('record file deletion', function () { headers: adminHeaders, method: 'PUT', payload: { + kind: 'custom', permissions: ['InstrumentRecord', 'Session', 'Subject'].map((subject) => ({ action: 'delete', groupId, diff --git a/apps/api/test/suites/04-permission-sets.suite.ts b/apps/api/test/suites/04-permission-sets.suite.ts new file mode 100644 index 000000000..979b4c118 --- /dev/null +++ b/apps/api/test/suites/04-permission-sets.suite.ts @@ -0,0 +1,282 @@ +import { randomUUID } from 'node:crypto'; + +import type { Group } from '@opendatacapture/schemas/group'; +import type { PermissionSet, PermissionSetSummary } from '@opendatacapture/schemas/permission-set'; +import type { User } from '@opendatacapture/schemas/user'; +import { beforeAll, describe, expect, it } from 'vitest'; + +import { defineSuite } from '../helpers'; + +const PASSWORD = 'DataCapture2025_Test'; + +type SignedRule = { action: string; conditions?: unknown; subject: string }; + +/** + * Runs after `02-user-permissions`, whose setup created the administrator. Everything here is proven + * against the real database: holder counts, the `SetNull` that deleting a set relies on, and the + * rules a holder's token ends up carrying. + */ +export default defineSuite('permission sets', function () { + let adminHeaders: { authorization: string }; + let group: Group; + + const login = async (username: string): Promise => { + const response = await this.app.inject({ + method: 'POST', + payload: { password: PASSWORD, username }, + url: '/v1/auth/login' + }); + expect(response.statusCode).toBe(200); + return response.json<{ accessToken: string }>().accessToken; + }; + + const signedRules = (accessToken: string): SignedRule[] => { + const payload = Buffer.from(accessToken.split('.')[1]!, 'base64url').toString(); + return (JSON.parse(payload) as { permissions: SignedRule[] }).permissions; + }; + + const createPermissionSet = async (): Promise => { + const response = await this.app.inject({ + headers: adminHeaders, + method: 'POST', + payload: { + name: `Set ${randomUUID()}`, + rules: [ + { action: 'read', scope: 'HOLDER_GROUPS', subject: 'InstrumentRecord' }, + { action: 'read', scope: 'ALL_GROUPS', subject: 'Assignment' } + ] + }, + url: '/v1/permission-sets' + }); + expect(response.statusCode).toBe(201); + return response.json(); + }; + + const createUser = async (payload: object = {}) => { + return this.app.inject({ + headers: adminHeaders, + method: 'POST', + payload: { + basePermissionLevel: 'STANDARD', + firstName: 'Set', + groupIds: [group.id], + lastName: 'Holder', + password: PASSWORD, + username: `holder-${randomUUID()}`, + ...payload + }, + url: '/v1/users' + }); + }; + + const findPermissionSet = async (id: string): Promise => { + const response = await this.app.inject({ headers: adminHeaders, method: 'GET', url: `/v1/permission-sets/${id}` }); + expect(response.statusCode).toBe(200); + return response.json(); + }; + + const findUser = async (id: string): Promise => { + const response = await this.app.inject({ headers: adminHeaders, method: 'GET', url: `/v1/users/${id}` }); + expect(response.statusCode).toBe(200); + return response.json(); + }; + + const setPermissions = (id: string, payload: object) => { + return this.app.inject({ headers: adminHeaders, method: 'PUT', payload, url: `/v1/users/${id}/permissions` }); + }; + + beforeAll(async () => { + adminHeaders = { authorization: `Bearer ${await login('admin')}` }; + const response = await this.app.inject({ + headers: adminHeaders, + method: 'POST', + payload: { name: 'Permission Set Group', type: 'RESEARCH' }, + url: '/v1/groups' + }); + expect(response.statusCode).toBe(201); + group = response.json(); + }); + + describe('POST /v1/permission-sets', () => { + it('should refuse a name another set already has', async () => { + const { name } = await createPermissionSet(); + const response = await this.app.inject({ + headers: adminHeaders, + method: 'POST', + payload: { name, rules: [{ action: 'read', scope: 'ALL_GROUPS', subject: 'Subject' }] }, + url: '/v1/permission-sets' + }); + expect(response.statusCode).toBe(409); + }); + + it('should refuse a set granting manage all, which would make every holder an administrator', async () => { + const response = await this.app.inject({ + headers: adminHeaders, + method: 'POST', + payload: { name: `Set ${randomUUID()}`, rules: [{ action: 'manage', scope: 'ALL_GROUPS', subject: 'all' }] }, + url: '/v1/permission-sets' + }); + expect(response.statusCode).toBe(400); + }); + + it('should refuse a user below administrator, even one whose set reaches every other resource', async () => { + const response = await this.app.inject({ + headers: adminHeaders, + method: 'POST', + payload: { + name: `Set ${randomUUID()}`, + rules: (['create', 'delete', 'read', 'update'] as const).map((action) => ({ + action, + scope: 'ALL_GROUPS', + subject: 'all' + })) + }, + url: '/v1/permission-sets' + }); + expect(response.statusCode).toBe(201); + const holder = await createUser({ permissionSetId: response.json().id }); + expect(holder.statusCode).toBe(201); + const headers = { authorization: `Bearer ${await login(holder.json().username)}` }; + const listing = await this.app.inject({ headers, method: 'GET', url: '/v1/permission-sets' }); + expect(listing.statusCode).toBe(403); + }); + }); + + describe('assigning a set', () => { + it('should link a set when the user is created, and count them as a holder', async () => { + const permissionSet = await createPermissionSet(); + const response = await createUser({ permissionSetId: permissionSet.id }); + expect(response.statusCode).toBe(201); + expect(response.json().permissionSetId).toBe(permissionSet.id); + expect((await findPermissionSet(permissionSet.id)).holderCount).toBe(1); + }); + + it('should refuse a set for an administrator being created', async () => { + const permissionSet = await createPermissionSet(); + const response = await createUser({ basePermissionLevel: 'ADMIN', permissionSetId: permissionSet.id }); + expect(response.statusCode).toBe(400); + }); + + it('should refuse a set that does not exist', async () => { + const response = await createUser({ permissionSetId: '000000000000000000000000' }); + expect(response.statusCode).toBe(404); + }); + + it("should sign the set's rules into the holder's token, confined to the holder's groups where the set says so", async () => { + const permissionSet = await createPermissionSet(); + const user = (await createUser()).json(); + const response = await setPermissions(user.id, { kind: 'permissionSet', permissionSetId: permissionSet.id }); + expect(response.statusCode).toBe(200); + const rules = signedRules(await login(user.username)); + expect(rules).toContainEqual({ + action: 'read', + conditions: { groupId: { in: [group.id] } }, + subject: 'InstrumentRecord' + }); + expect(rules).toContainEqual({ action: 'read', subject: 'Assignment' }); + }); + + it('should switch between a set and custom grants, each write emptying the other', async () => { + const permissionSet = await createPermissionSet(); + const user = (await createUser()).json(); + const custom = { kind: 'custom', permissions: [{ action: 'read', groupId: null, subject: 'Session' }] }; + + expect((await setPermissions(user.id, custom)).statusCode).toBe(200); + expect( + (await setPermissions(user.id, { kind: 'permissionSet', permissionSetId: permissionSet.id })).statusCode + ).toBe(200); + expect(await findUser(user.id)).toMatchObject({ additionalPermissions: [], permissionSetId: permissionSet.id }); + + expect((await setPermissions(user.id, custom)).statusCode).toBe(200); + expect(await findUser(user.id)).toMatchObject({ + additionalPermissions: custom.permissions, + permissionSetId: null + }); + expect((await findPermissionSet(permissionSet.id)).holderCount).toBe(0); + }); + + it('should refuse a set for an existing administrator', async () => { + const permissionSet = await createPermissionSet(); + const admin = (await createUser({ basePermissionLevel: 'ADMIN', groupIds: [] })).json(); + const response = await setPermissions(admin.id, { kind: 'permissionSet', permissionSetId: permissionSet.id }); + expect(response.statusCode).toBe(400); + }); + + it('should remove the set from a user promoted to administrator', async () => { + const permissionSet = await createPermissionSet(); + const user = (await createUser({ permissionSetId: permissionSet.id })).json(); + const response = await this.app.inject({ + headers: adminHeaders, + method: 'PATCH', + payload: { basePermissionLevel: 'ADMIN' }, + url: `/v1/users/${user.id}` + }); + expect(response.statusCode).toBe(200); + expect((await findUser(user.id)).permissionSetId).toBeNull(); + }); + + it('should promote a user who never held a set', async () => { + const user = (await createUser()).json(); + const response = await this.app.inject({ + headers: adminHeaders, + method: 'PATCH', + payload: { basePermissionLevel: 'ADMIN' }, + url: `/v1/users/${user.id}` + }); + expect(response.statusCode).toBe(200); + }); + }); + + describe('PATCH /v1/permission-sets/:id', () => { + it("should change what a holder's next token carries", async () => { + const permissionSet = await createPermissionSet(); + const user = (await createUser({ permissionSetId: permissionSet.id })).json(); + const response = await this.app.inject({ + headers: adminHeaders, + method: 'PATCH', + payload: { rules: [{ action: 'read', scope: 'ALL_GROUPS', subject: 'Session' }] }, + url: `/v1/permission-sets/${permissionSet.id}` + }); + expect(response.statusCode).toBe(200); + const rules = signedRules(await login(user.username)); + expect(rules).toContainEqual({ action: 'read', subject: 'Session' }); + expect(rules).not.toContainEqual(expect.objectContaining({ subject: 'Assignment' })); + }); + }); + + describe('DELETE /v1/permission-sets/:id', () => { + it('should remove the set from every holder, leaving them their base level alone', async () => { + const permissionSet = await createPermissionSet(); + const holders = [ + (await createUser({ permissionSetId: permissionSet.id })).json(), + (await createUser({ permissionSetId: permissionSet.id })).json() + ]; + expect((await findPermissionSet(permissionSet.id)).holderCount).toBe(2); + + const response = await this.app.inject({ + headers: adminHeaders, + method: 'DELETE', + url: `/v1/permission-sets/${permissionSet.id}` + }); + expect(response.statusCode).toBe(200); + + for (const holder of holders) { + expect((await findUser(holder.id)).permissionSetId).toBeNull(); + expect(signedRules(await login(holder.username))).not.toContainEqual( + expect.objectContaining({ subject: 'Assignment' }) + ); + } + const listing = await this.app.inject({ headers: adminHeaders, method: 'GET', url: '/v1/permission-sets' }); + expect(listing.json().map(({ id }) => id)).not.toContain(permissionSet.id); + }); + + it('should answer a set that does not exist with not found', async () => { + const response = await this.app.inject({ + headers: adminHeaders, + method: 'DELETE', + url: '/v1/permission-sets/000000000000000000000000' + }); + expect(response.statusCode).toBe(404); + }); + }); +}); diff --git a/packages/schemas/AGENTS.md b/packages/schemas/AGENTS.md index 3cbb753f0..a19950165 100644 --- a/packages/schemas/AGENTS.md +++ b/packages/schemas/AGENTS.md @@ -64,9 +64,15 @@ body is parsed against it; `AuthPayload` and `TokenPayload` are plain types beca parses them. Do not add a schema for a shape that only crosses a typed boundary. Variants are composed with `.extend()` / `.pick()` / `.omit()` / `.partial()` off an existing shape, -never rewritten — `$UpdateUserData = $CreateUserData.partial().extend({...})`. Rewriting is how the +never rewritten — `$UpdateUserData = $BaseUserData.partial().extend({...})`. Rewriting is how the two drift. +**Derive only from an object without a `.check`.** The vendored zod silently drops a `.check` when +you `.extend()`, `.pick()`, `.omit()` or `.partial()` the object carrying it, and newer zod releases +throw on some of these instead. Where a schema needs a rule relating its fields, keep the unchecked +shape as its own schema and derive every variant from that — `$BaseUserData` is the shape +`$CreateUserData` checks and `$UpdateUserData` makes partial. + ## src/instrument The one barrel in the package (`instrument.ts` re-exports the six sibling files), and the only place diff --git a/packages/schemas/package.json b/packages/schemas/package.json index a619e8c5e..a063d1dbf 100644 --- a/packages/schemas/package.json +++ b/packages/schemas/package.json @@ -15,6 +15,7 @@ "./instrument-records": "./src/instrument-records/instrument-records.ts", "./instrument-repo": "./src/instrument-repo/instrument-repo.ts", "./mail": "./src/mail/mail.ts", + "./permission-set": "./src/permission-set/permission-set.ts", "./session": "./src/session/session.ts", "./setup": "./src/setup/setup.ts", "./storage": "./src/storage/storage.ts", diff --git a/packages/schemas/src/permission-set/__tests__/permission-set.test.ts b/packages/schemas/src/permission-set/__tests__/permission-set.test.ts new file mode 100644 index 000000000..c563040ad --- /dev/null +++ b/packages/schemas/src/permission-set/__tests__/permission-set.test.ts @@ -0,0 +1,125 @@ +import { describe, expect, it } from 'vitest'; + +import { + $CreatePermissionSetData, + $PermissionSet, + $PermissionSetRule, + $PermissionSetSummary, + $UpdatePermissionSetData, + isPermissionSetGrantable +} from '../permission-set.js'; + +const STORED_SET = { + createdAt: new Date(), + id: 'set-1', + name: 'Research Assistant', + rules: [{ action: 'read', scope: 'HOLDER_GROUPS', subject: 'Subject' }], + updatedAt: new Date() +}; + +describe('$PermissionSetRule', () => { + it('should accept a rule confined to the holder groups on a resource that belongs to a group', () => { + expect($PermissionSetRule.safeParse({ action: 'read', scope: 'HOLDER_GROUPS', subject: 'Subject' }).success).toBe( + true + ); + }); + + it('should reject a rule confined to the holder groups on a resource no group owns', () => { + const result = $PermissionSetRule.safeParse({ action: 'read', scope: 'HOLDER_GROUPS', subject: 'Instrument' }); + expect(result.error?.issues[0]?.path).toEqual(['scope']); + }); + + it('should accept a rule on a resource no group owns when it applies to every group', () => { + expect($PermissionSetRule.safeParse({ action: 'read', scope: 'ALL_GROUPS', subject: 'Instrument' }).success).toBe( + true + ); + }); +}); + +describe('$PermissionSet', () => { + it('should accept a stored rule a set may no longer contain, so a set saved before still parses', () => { + const rules = [{ action: 'manage', scope: 'ALL_GROUPS', subject: 'all' }]; + expect($PermissionSet.safeParse({ ...STORED_SET, rules }).success).toBe(true); + }); +}); + +describe('$PermissionSetSummary', () => { + it('should reject a negative holder count', () => { + expect($PermissionSetSummary.safeParse({ ...STORED_SET, holderCount: -1 }).success).toBe(false); + }); + + it('should accept a set with no holders', () => { + expect($PermissionSetSummary.safeParse({ ...STORED_SET, holderCount: 0 }).success).toBe(true); + }); +}); + +describe('isPermissionSetGrantable', () => { + it('should refuse manage all, which would make every holder an administrator', () => { + expect(isPermissionSetGrantable({ action: 'manage', subject: 'all' })).toBe(false); + }); + + it('should refuse a user write, which only an administrator can make', () => { + expect(isPermissionSetGrantable({ action: 'update', subject: 'User' })).toBe(false); + }); + + it('should allow every other action on every resource, as a per-user grant would', () => { + expect(isPermissionSetGrantable({ action: 'read', subject: 'all' })).toBe(true); + expect(isPermissionSetGrantable({ action: 'manage', subject: 'Subject' })).toBe(true); + }); +}); + +describe('$CreatePermissionSetData', () => { + const data = { name: 'Research Assistant', rules: [{ action: 'read', scope: 'HOLDER_GROUPS', subject: 'Subject' }] }; + + it('should accept a named set with one rule', () => { + expect($CreatePermissionSetData.safeParse(data).success).toBe(true); + }); + + it('should reject a set with no rules, which would grant nothing', () => { + expect($CreatePermissionSetData.safeParse({ ...data, rules: [] }).success).toBe(false); + }); + + it('should reject a blank name, which would leave the set unidentifiable in the list', () => { + expect($CreatePermissionSetData.safeParse({ ...data, name: '' }).success).toBe(false); + }); + + it('should reject manage all, naming the offending rule', () => { + const rules = [...data.rules, { action: 'manage', scope: 'ALL_GROUPS', subject: 'all' }]; + const result = $CreatePermissionSetData.safeParse({ ...data, rules }); + expect(result.error?.issues).toMatchObject([ + { message: 'A permission set cannot make its holders administrators', path: ['rules', 1] } + ]); + }); + + it('should reject a user write, naming the offending rule', () => { + const rules = [{ action: 'update', scope: 'HOLDER_GROUPS', subject: 'User' }]; + const result = $CreatePermissionSetData.safeParse({ ...data, rules }); + expect(result.error?.issues).toMatchObject([ + { + message: 'Only an administrator can create, modify or delete users, so this permission would do nothing', + path: ['rules', 0] + } + ]); + }); + + it('should reject a rule repeated in the set, naming the repeat', () => { + const result = $CreatePermissionSetData.safeParse({ ...data, rules: [...data.rules, ...data.rules] }); + expect(result.error?.issues).toMatchObject([{ path: ['rules', 1] }]); + }); + + it('should accept the same rule at two scopes, since they grant different rows', () => { + const rules = [...data.rules, { action: 'read', scope: 'ALL_GROUPS', subject: 'Subject' }]; + expect($CreatePermissionSetData.safeParse({ ...data, rules }).success).toBe(true); + }); +}); + +describe('$UpdatePermissionSetData', () => { + it('should accept a rename alone, leaving the rules as they are', () => { + expect($UpdatePermissionSetData.safeParse({ name: 'Renamed' }).success).toBe(true); + }); + + it('should apply the same rule check as a create when the rules are replaced', () => { + const rules = [{ action: 'manage', scope: 'ALL_GROUPS', subject: 'all' }]; + expect($UpdatePermissionSetData.safeParse({ rules }).success).toBe(false); + }); +}); diff --git a/packages/schemas/src/permission-set/permission-set.ts b/packages/schemas/src/permission-set/permission-set.ts new file mode 100644 index 000000000..04233f79e --- /dev/null +++ b/packages/schemas/src/permission-set/permission-set.ts @@ -0,0 +1,107 @@ +import { z } from 'zod/v4'; + +import { + $AppAction, + $AppSubjectName, + $BaseModel, + isGrantablePermission, + isGroupScopableSubject +} from '../core/core.js'; + +import type { UserPermission } from '../core/core.js'; + +/** + * Where a rule in a permission set applies, relative to whoever holds the set: the groups the holder + * belongs to when they sign in, or every group. A set names no group itself, so one set can serve + * users in different groups. + */ +const $PermissionSetScope = z.enum(['ALL_GROUPS', 'HOLDER_GROUPS']); + +/** + * One rule of a permission set, as stored. This is the read model, so like `$UserPermission` it does + * not apply {@link isPermissionSetGrantable}: a set stored before a rule was tightened must still parse. + */ +const $PermissionSetRule = z + .object({ + action: $AppAction, + scope: $PermissionSetScope, + subject: $AppSubjectName + }) + .check((ctx) => { + if (ctx.value.scope === 'HOLDER_GROUPS' && !isGroupScopableSubject(ctx.value.subject)) { + ctx.issues.push({ + code: 'custom', + input: ctx.value.scope, + message: 'This resource cannot be scoped to a group', + path: ['scope'] + }); + } + }); + +/** + * Whether a permission set may contain a rule. Everything a per-user grant may name, except + * `manage all`: that makes its holder an administrator, so a single edit to the set would promote + * every holder at once. The `ADMIN` permission level exists for that. + */ +const isPermissionSetGrantable = ({ action, subject }: Pick): boolean => { + return isGrantablePermission({ action, subject }) && !(action === 'manage' && subject === 'all'); +}; + +/** The rules a write may store: at least one, none repeated, and each one a set may contain. */ +const $GrantablePermissionSetRules = z + .array($PermissionSetRule) + .min(1) + .check((ctx) => { + const seen = new Set(); + ctx.value.forEach((rule, index) => { + if (!isPermissionSetGrantable(rule)) { + ctx.issues.push({ + code: 'custom', + input: rule, + message: isGrantablePermission(rule) + ? 'A permission set cannot make its holders administrators' + : 'Only an administrator can create, modify or delete users, so this permission would do nothing', + path: [index] + }); + } + const key = [rule.action, rule.subject, rule.scope].join(' '); + if (seen.has(key)) { + ctx.issues.push({ code: 'custom', input: rule, message: 'This rule is already in the set', path: [index] }); + } + seen.add(key); + }); + }); + +const $PermissionSet = $BaseModel.extend({ + name: z.string().min(1), + rules: $PermissionSetRule.array() +}); + +/** What the permission set routes return: the set, and how many users hold it. */ +const $PermissionSetSummary = $PermissionSet.extend({ + holderCount: z.number().int().nonnegative() +}); + +const $CreatePermissionSetData = z.object({ + name: z.string().min(1), + rules: $GrantablePermissionSetRules +}); + +const $UpdatePermissionSetData = $CreatePermissionSetData.partial(); + +export type $CreatePermissionSetData = z.infer; +export type PermissionSet = z.infer; +export type PermissionSetRule = z.infer; +export type PermissionSetScope = z.infer; +export type PermissionSetSummary = z.infer; +export type $UpdatePermissionSetData = z.infer; + +export { + $CreatePermissionSetData, + $PermissionSet, + $PermissionSetRule, + $PermissionSetScope, + $PermissionSetSummary, + $UpdatePermissionSetData, + isPermissionSetGrantable +}; diff --git a/packages/schemas/src/setup/__tests__/setup.test.ts b/packages/schemas/src/setup/__tests__/setup.test.ts index 62de08d9d..180d76249 100644 --- a/packages/schemas/src/setup/__tests__/setup.test.ts +++ b/packages/schemas/src/setup/__tests__/setup.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from 'vitest'; -import { $BrandingConfig, $UpdateSetupStateData } from '../setup.js'; +import { $BrandingConfig, $InitAppOptions, $UpdateSetupStateData } from '../setup.js'; describe('$BrandingConfig', () => { describe('customLogoSrc', () => { @@ -107,3 +107,14 @@ describe('$BrandingConfig', () => { }); }); }); + +describe('$InitAppOptions', () => { + it('should drop a permission set sent for the first administrator, who holds every permission already', () => { + const result = $InitAppOptions.safeParse({ + admin: { firstName: 'Ada', lastName: 'Admin', password: 'password', permissionSetId: 'set-1', username: 'admin' }, + enableExperimentalFeatures: false, + initDemo: false + }); + expect(result.data?.admin).not.toHaveProperty('permissionSetId'); + }); +}); diff --git a/packages/schemas/src/setup/setup.ts b/packages/schemas/src/setup/setup.ts index 1a4a7e829..89968d155 100644 --- a/packages/schemas/src/setup/setup.ts +++ b/packages/schemas/src/setup/setup.ts @@ -1,7 +1,7 @@ import { z } from 'zod/v4'; import { $ActiveLanguages } from '../core/core.js'; -import { $CreateUserData } from '../user/user.js'; +import { $BaseUserData } from '../user/user.js'; import type { Language } from '../core/core.js'; @@ -229,7 +229,7 @@ const $UpdateSetupStateData = z.object({ tableColorMode: $TableColorMode.optional() }); -const $CreateAdminData = $CreateUserData.omit({ +const $CreateAdminData = $BaseUserData.omit({ basePermissionLevel: true, groupIds: true }); diff --git a/packages/schemas/src/user/__tests__/user.test.ts b/packages/schemas/src/user/__tests__/user.test.ts index 886cb0c6f..23ab88b22 100644 --- a/packages/schemas/src/user/__tests__/user.test.ts +++ b/packages/schemas/src/user/__tests__/user.test.ts @@ -18,16 +18,26 @@ describe('$UpdateUserData', () => { expect(result.success).toBe(true); expect(result.data).not.toHaveProperty('additionalPermissions'); }); + + it('should strip permissionSetId, so a profile update cannot assign a permission set', () => { + const result = $UpdateUserData.safeParse({ permissionSetId: 'set-1' }); + expect(result.success).toBe(true); + expect(result.data).not.toHaveProperty('permissionSetId'); + }); + + it('should accept a promotion to administrator, which the create-time permission set rule does not reach', () => { + expect($UpdateUserData.safeParse({ basePermissionLevel: 'ADMIN' }).success).toBe(true); + }); }); describe('$UpdateUserPermissionsData', () => { - it('should accept an empty set, which is how every grant is revoked', () => { - expect($UpdateUserPermissionsData.safeParse({ permissions: [] }).success).toBe(true); + it('should accept an empty set of custom grants, which is how every grant is revoked', () => { + expect($UpdateUserPermissionsData.safeParse({ kind: 'custom', permissions: [] }).success).toBe(true); }); it('should reject a grant confined to a group on a resource that cannot be', () => { const permissions = [{ action: 'read', groupId: 'group-1', subject: 'Instrument' }]; - expect($UpdateUserPermissionsData.safeParse({ permissions }).success).toBe(false); + expect($UpdateUserPermissionsData.safeParse({ kind: 'custom', permissions }).success).toBe(false); }); it('should reject a grant that writes users, naming the offending entry, since it would do nothing', () => { @@ -35,10 +45,29 @@ describe('$UpdateUserPermissionsData', () => { { action: 'read', groupId: null, subject: 'Subject' }, { action: 'update', groupId: null, subject: 'User' } ]; - const result = $UpdateUserPermissionsData.safeParse({ permissions }); + const result = $UpdateUserPermissionsData.safeParse({ kind: 'custom', permissions }); expect(result.success).toBe(false); expect(result.error?.issues[0]?.path).toEqual(['permissions', 1]); }); + + it('should accept a permission set by its id', () => { + expect($UpdateUserPermissionsData.safeParse({ kind: 'permissionSet', permissionSetId: 'set-1' }).success).toBe( + true + ); + }); + + it('should reject a body without a kind, the shape every write took before permission sets', () => { + expect($UpdateUserPermissionsData.safeParse({ permissions: [] }).success).toBe(false); + }); + + it('should drop custom grants sent beside a permission set, so a user can never hold both', () => { + const result = $UpdateUserPermissionsData.safeParse({ + kind: 'permissionSet', + permissions: [{ action: 'read', groupId: null, subject: 'Subject' }], + permissionSetId: 'set-1' + }); + expect(result.data).toStrictEqual({ kind: 'permissionSet', permissionSetId: 'set-1' }); + }); }); describe('$SelfUpdateUserData', () => { @@ -72,6 +101,15 @@ describe('$CreateUserData', () => { it('should accept a well-formed phone number', () => { expect($CreateUserData.safeParse({ ...data, phoneNumber: '(514) 555-1234' }).success).toBe(true); }); + + it('should accept a permission set for a user below administrator', () => { + expect($CreateUserData.safeParse({ ...data, permissionSetId: 'set-1' }).success).toBe(true); + }); + + it('should reject a permission set for an administrator, who already holds every permission', () => { + const result = $CreateUserData.safeParse({ ...data, basePermissionLevel: 'ADMIN', permissionSetId: 'set-1' }); + expect(result.error?.issues[0]?.path).toEqual(['permissionSetId']); + }); }); describe('$PhoneNumber', () => { diff --git a/packages/schemas/src/user/user.ts b/packages/schemas/src/user/user.ts index d0ba7d38f..50536799d 100644 --- a/packages/schemas/src/user/user.ts +++ b/packages/schemas/src/user/user.ts @@ -77,6 +77,11 @@ export const $User = $BaseModel.extend({ * themselves. Nullish because it postdates the users already in the database. */ mustResetPassword: z.boolean().nullish(), + /** + * The permission set this user holds, if any. Never alongside custom `additionalPermissions`: + * {@link $UpdateUserPermissionsData} writes the two together, one of them always empty. + */ + permissionSetId: z.string().nullish(), // Not `$PhoneNumber`: this is the read model, which must still parse numbers stored before the // digit minimum existed. The rule is enforced on the write schemas below. phoneNumber: z.string().nullish(), @@ -88,8 +93,12 @@ export const $User = $BaseModel.extend({ export type CreateUserResponse = z.infer; export const $CreateUserResponse = $User.extend({ welcomeEmail: $EmailDeliveryResult }); -export type $CreateUserData = z.infer; -export const $CreateUserData = $User +/** + * The account fields an administrator writes, with none of the rules relating one to another. Every + * user write schema derives from this rather than from another write schema, because deriving from + * an object drops its `.check` without a word. + */ +export const $BaseUserData = $User .pick({ basePermissionLevel: true, firstName: true, @@ -107,33 +116,60 @@ export const $CreateUserData = $User sex: $Sex.optional() }); +export type $CreateUserData = z.infer; +export const $CreateUserData = $BaseUserData + .extend({ + permissionSetId: z.string().optional() + }) + .check((ctx) => { + if (ctx.value.basePermissionLevel === 'ADMIN' && ctx.value.permissionSetId !== undefined) { + ctx.issues.push({ + code: 'custom', + input: ctx.value.permissionSetId, + message: 'Administrators hold every permission, so they cannot hold a permission set', + path: ['permissionSetId'] + }); + } + }); + /** * Optional contact details are nullable here, and only here, so an update can clear one. - * `additionalPermissions` is deliberately absent: it is written only through - * {@link $UpdateUserPermissionsData}, whose route is gated more tightly than a profile update. + * `additionalPermissions` and `permissionSetId` are deliberately absent: they are written only + * through {@link $UpdateUserPermissionsData}, whose route is gated more tightly than a profile update. */ export type $UpdateUserData = z.infer; -export const $UpdateUserData = $CreateUserData.partial().extend({ +export const $UpdateUserData = $BaseUserData.partial().extend({ email: z.email().nullish(), phoneNumber: $PhoneNumber.nullish() }); -/** The complete set of a user's additional permissions; a write replaces what is stored. */ +/** + * Where a user's permissions beyond their base level come from: a permission set, or their own + * custom grants. A write replaces both, so a body naming one empties the other and no user can hold + * the two at once. + */ export type $UpdateUserPermissionsData = z.infer; -export const $UpdateUserPermissionsData = z.object({ - permissions: $Permissions.check((ctx) => { - ctx.value.forEach((permission, index) => { - if (!isGrantablePermission(permission)) { - ctx.issues.push({ - code: 'custom', - input: permission, - message: 'Only an administrator can create, modify or delete users, so this permission would do nothing', - path: [index] - }); - } - }); +export const $UpdateUserPermissionsData = z.discriminatedUnion('kind', [ + z.object({ + kind: z.literal('permissionSet'), + permissionSetId: z.string() + }), + z.object({ + kind: z.literal('custom'), + permissions: $Permissions.check((ctx) => { + ctx.value.forEach((permission, index) => { + if (!isGrantablePermission(permission)) { + ctx.issues.push({ + code: 'custom', + input: permission, + message: 'Only an administrator can create, modify or delete users, so this permission would do nothing', + path: [index] + }); + } + }); + }) }) -}); +]); export type $SelfUpdateUserData = z.infer; export const $SelfUpdateUserData = $UpdateUserData From 2df4f89b93fac71fea8ebe7855914f3f5da51919 Mon Sep 17 00:00:00 2001 From: joshunrau Date: Tue, 3 Nov 2026 20:54:31 -0500 Subject: [PATCH 2/5] feat(web): manage permission sets and assign them to users Adds the admin pages to list, create, edit and delete permission sets, with the holder count stated before a delete. The user page's permissions card switches between a permission set and custom permissions, warning when choosing a set discards saved custom grants, and the create-user form can link a set to any user below administrator. Co-Authored-By: Claude Opus 5.5 --- .../src/components/CellSelect/CellSelect.tsx | 31 ++ .../CellSelect/__tests__/CellSelect.test.tsx | 45 ++ apps/web/src/components/CellSelect/index.ts | 1 + .../PermissionSetForm/PermissionSetForm.tsx | 103 ++++ .../__tests__/PermissionSetForm.test.tsx | 116 ++++ .../src/components/PermissionSetForm/index.ts | 1 + .../PermissionSetRulesEditor.stories.tsx | 62 +++ .../PermissionSetRulesEditor.tsx | 221 ++++++++ .../PermissionSetRulesEditor.test.tsx | 155 ++++++ .../PermissionSetRulesEditor/index.ts | 1 + .../PermissionSetChoice.tsx | 104 ++++ .../UserPermissionsEditor.tsx | 499 +++++++++--------- .../__tests__/UserPermissionsEditor.test.tsx | 112 +++- .../useConfirmPermissionSetDeletion.test.tsx | 69 +++ .../useCreatePermissionSetMutation.test.ts | 63 +++ .../__tests__/useCreateUserMutation.test.ts | 6 + .../useDeletePermissionSetMutation.test.ts | 47 ++ .../src/hooks/__tests__/useNavItems.test.ts | 10 + .../__tests__/usePermissionLabels.test.ts | 32 ++ .../__tests__/usePermissionSetQuery.test.ts | 67 +++ .../__tests__/usePermissionSetsQuery.test.ts | 69 +++ .../useUpdatePermissionSetMutation.test.ts | 58 ++ .../__tests__/useUpdateUserMutation.test.ts | 26 +- .../hooks/useConfirmPermissionSetDeletion.ts | 58 ++ .../hooks/useCreatePermissionSetMutation.ts | 23 + .../hooks/useDeletePermissionSetMutation.ts | 20 + apps/web/src/hooks/useNavItems.ts | 10 + apps/web/src/hooks/usePermissionLabels.ts | 39 ++ apps/web/src/hooks/usePermissionSetQuery.ts | 20 + apps/web/src/hooks/usePermissionSetsQuery.ts | 19 + .../hooks/useUpdatePermissionSetMutation.ts | 22 + apps/web/src/hooks/useUpdateUserMutation.ts | 14 +- apps/web/src/route-tree.ts | 67 +++ .../permission-sets/$permissionSetId.tsx | 87 +++ .../__tests__/$permissionSetId.test.tsx | 191 +++++++ .../permission-sets/__tests__/create.test.tsx | 89 ++++ .../permission-sets/__tests__/index.test.tsx | 141 +++++ .../_app/admin/permission-sets/create.tsx | 52 ++ .../_app/admin/permission-sets/index.tsx | 111 ++++ .../src/routes/_app/admin/users/$userId.tsx | 55 +- .../admin/users/__tests__/$userId.test.tsx | 100 +++- .../admin/users/__tests__/create.test.tsx | 86 ++- .../src/routes/_app/admin/users/create.tsx | 51 +- .../__tests__/permission-set-rules.test.ts | 64 +++ .../src/utils/__tests__/permissions.test.ts | 9 + apps/web/src/utils/permission-set-rules.ts | 45 ++ apps/web/src/utils/permissions.ts | 21 +- 47 files changed, 2997 insertions(+), 295 deletions(-) create mode 100644 apps/web/src/components/CellSelect/CellSelect.tsx create mode 100644 apps/web/src/components/CellSelect/__tests__/CellSelect.test.tsx create mode 100644 apps/web/src/components/CellSelect/index.ts create mode 100644 apps/web/src/components/PermissionSetForm/PermissionSetForm.tsx create mode 100644 apps/web/src/components/PermissionSetForm/__tests__/PermissionSetForm.test.tsx create mode 100644 apps/web/src/components/PermissionSetForm/index.ts create mode 100644 apps/web/src/components/PermissionSetRulesEditor/PermissionSetRulesEditor.stories.tsx create mode 100644 apps/web/src/components/PermissionSetRulesEditor/PermissionSetRulesEditor.tsx create mode 100644 apps/web/src/components/PermissionSetRulesEditor/__tests__/PermissionSetRulesEditor.test.tsx create mode 100644 apps/web/src/components/PermissionSetRulesEditor/index.ts create mode 100644 apps/web/src/components/UserPermissionsEditor/PermissionSetChoice.tsx create mode 100644 apps/web/src/hooks/__tests__/useConfirmPermissionSetDeletion.test.tsx create mode 100644 apps/web/src/hooks/__tests__/useCreatePermissionSetMutation.test.ts create mode 100644 apps/web/src/hooks/__tests__/useDeletePermissionSetMutation.test.ts create mode 100644 apps/web/src/hooks/__tests__/usePermissionLabels.test.ts create mode 100644 apps/web/src/hooks/__tests__/usePermissionSetQuery.test.ts create mode 100644 apps/web/src/hooks/__tests__/usePermissionSetsQuery.test.ts create mode 100644 apps/web/src/hooks/__tests__/useUpdatePermissionSetMutation.test.ts create mode 100644 apps/web/src/hooks/useConfirmPermissionSetDeletion.ts create mode 100644 apps/web/src/hooks/useCreatePermissionSetMutation.ts create mode 100644 apps/web/src/hooks/useDeletePermissionSetMutation.ts create mode 100644 apps/web/src/hooks/usePermissionLabels.ts create mode 100644 apps/web/src/hooks/usePermissionSetQuery.ts create mode 100644 apps/web/src/hooks/usePermissionSetsQuery.ts create mode 100644 apps/web/src/hooks/useUpdatePermissionSetMutation.ts create mode 100644 apps/web/src/routes/_app/admin/permission-sets/$permissionSetId.tsx create mode 100644 apps/web/src/routes/_app/admin/permission-sets/__tests__/$permissionSetId.test.tsx create mode 100644 apps/web/src/routes/_app/admin/permission-sets/__tests__/create.test.tsx create mode 100644 apps/web/src/routes/_app/admin/permission-sets/__tests__/index.test.tsx create mode 100644 apps/web/src/routes/_app/admin/permission-sets/create.tsx create mode 100644 apps/web/src/routes/_app/admin/permission-sets/index.tsx create mode 100644 apps/web/src/utils/__tests__/permission-set-rules.test.ts create mode 100644 apps/web/src/utils/permission-set-rules.ts diff --git a/apps/web/src/components/CellSelect/CellSelect.tsx b/apps/web/src/components/CellSelect/CellSelect.tsx new file mode 100644 index 000000000..fefdbd88e --- /dev/null +++ b/apps/web/src/components/CellSelect/CellSelect.tsx @@ -0,0 +1,31 @@ +import { Select } from '@douglasneuroinformatics/libui/components'; + +type CellSelectProps = { + disabled?: boolean; + label: string; + name: string; + onValueChange: (value: string) => void; + options: { [key: string]: string }; + placeholder: string; + value: string | undefined; +}; + +/** A select drawn flat into a table cell. Its test ids follow libui's form select, keyed on `name`. */ +export const CellSelect = ({ disabled, label, name, onValueChange, options, placeholder, value }: CellSelectProps) => ( + +); diff --git a/apps/web/src/components/CellSelect/__tests__/CellSelect.test.tsx b/apps/web/src/components/CellSelect/__tests__/CellSelect.test.tsx new file mode 100644 index 000000000..311caf094 --- /dev/null +++ b/apps/web/src/components/CellSelect/__tests__/CellSelect.test.tsx @@ -0,0 +1,45 @@ +import { cleanup, fireEvent, render, screen } from '@testing-library/react'; +import { afterEach, describe, expect, it, vi } from 'vitest'; + +import { CellSelect } from '@/components/CellSelect/CellSelect'; + +import '@/services/i18n'; + +const onValueChange = vi.fn(); + +const renderSelect = (value?: string) => { + render( + + ); +}; + +describe('CellSelect', () => { + afterEach(() => { + cleanup(); + vi.clearAllMocks(); + }); + + it('should show the placeholder until a value is chosen', () => { + renderSelect(); + expect(screen.getByTestId('action-select-trigger').textContent).toBe('Choose…'); + }); + + it('should report the option chosen, keyed by its value', () => { + renderSelect(); + fireEvent.keyDown(screen.getByTestId('action-select-trigger'), { key: 'Enter' }); + fireEvent.click(screen.getByTestId('action-select-item-read')); + expect(onValueChange).toHaveBeenCalledWith('read'); + }); + + it('should name the trigger by its label, since a table cell has no visible one', () => { + renderSelect('create'); + expect(screen.getByRole('combobox', { name: 'Action' }).textContent).toBe('Create'); + }); +}); diff --git a/apps/web/src/components/CellSelect/index.ts b/apps/web/src/components/CellSelect/index.ts new file mode 100644 index 000000000..ede8e193c --- /dev/null +++ b/apps/web/src/components/CellSelect/index.ts @@ -0,0 +1 @@ +export * from './CellSelect'; diff --git a/apps/web/src/components/PermissionSetForm/PermissionSetForm.tsx b/apps/web/src/components/PermissionSetForm/PermissionSetForm.tsx new file mode 100644 index 000000000..2d760fd3c --- /dev/null +++ b/apps/web/src/components/PermissionSetForm/PermissionSetForm.tsx @@ -0,0 +1,103 @@ +import { useState } from 'react'; +import type { ReactNode } from 'react'; + +import { Form } from '@douglasneuroinformatics/libui/components'; +import { useTranslation } from '@douglasneuroinformatics/libui/hooks'; +import { $CreatePermissionSetData } from '@opendatacapture/schemas/permission-set'; +import type { PermissionSet } from '@opendatacapture/schemas/permission-set'; + +import { PermissionSetRulesEditor } from '@/components/PermissionSetRulesEditor'; +import { createRuleDraft, isIncompleteRuleDraft, withRuleDrafts } from '@/utils/permission-set-rules'; +import type { PermissionSetRuleDraft } from '@/utils/permission-set-rules'; + +type PermissionSetFormProps = { + /** The set being edited; omitted when creating one. */ + initialValues?: Pick; + /** Resolves to a message to show on the form when the save fails, or nothing when it succeeds. */ + onSubmit: (data: $CreatePermissionSetData) => Promise; + /** A button rendered beside the submit button, such as one deleting the set being edited. */ + secondaryAction?: ReactNode; + submitLabel: string; +}; + +/** The name and rules of a permission set, for creating one or editing it. */ +export const PermissionSetForm = ({ + initialValues, + onSubmit, + secondaryAction, + submitLabel +}: PermissionSetFormProps) => { + const { t } = useTranslation(); + const [rules, setRules] = useState(initialValues?.rules ?? []); + const [drafts, setDrafts] = useState([createRuleDraft()]); + const [highlightIncomplete, setHighlightIncomplete] = useState(false); + + return ( +
+

{t({ en: 'Rules', es: 'Reglas', fr: 'Règles' })}

+

+ {t({ + en: "Each rule adds to a holder's base permission level from their next sign-in. Holder's groups means the groups they belong to at that time.", + es: 'Cada regla se suma al nivel de permisos de base del titular a partir de su próximo inicio de sesión. Grupos del titular son los grupos a los que pertenece en ese momento.', + fr: "Chaque règle s'ajoute au niveau de base du titulaire à partir de sa prochaine connexion. Les groupes du titulaire sont ceux auxquels il appartient à ce moment-là." + })} +

+ + + } + initialValues={{ name: initialValues?.name }} + submitBtnLabel={submitLabel} + validationSchema={$CreatePermissionSetData.pick({ name: true })} + onSubmit={async ({ name }) => { + if (drafts.some(isIncompleteRuleDraft)) { + setHighlightIncomplete(true); + return { + errorMessage: t({ + en: 'A rule row is incomplete. Choose its action and resource, or remove it.', + es: 'Una fila de reglas está incompleta. Elija su acción y su recurso, o elimínela.', + fr: 'Une ligne de règle est incomplète. Choisissez son action et sa ressource, ou retirez-la.' + }), + success: false + }; + } + setHighlightIncomplete(false); + const nextRules = withRuleDrafts(rules, drafts); + if (nextRules.length === 0) { + return { + errorMessage: t({ + en: 'Add at least one rule. A permission set without rules grants nothing.', + es: 'Agregue al menos una regla. Un conjunto de permisos sin reglas no concede nada.', + fr: "Ajoutez au moins une règle. Un ensemble d'autorisations sans règle n'accorde rien." + }), + success: false + }; + } + const result = await onSubmit({ name, rules: nextRules }); + if (!result) { + setRules(nextRules); + setDrafts([createRuleDraft()]); + } + return result; + }} + /> + ); +}; diff --git a/apps/web/src/components/PermissionSetForm/__tests__/PermissionSetForm.test.tsx b/apps/web/src/components/PermissionSetForm/__tests__/PermissionSetForm.test.tsx new file mode 100644 index 000000000..11e6e296a --- /dev/null +++ b/apps/web/src/components/PermissionSetForm/__tests__/PermissionSetForm.test.tsx @@ -0,0 +1,116 @@ +import type { PermissionSetRule } from '@opendatacapture/schemas/permission-set'; +import { cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { PermissionSetForm } from '@/components/PermissionSetForm/PermissionSetForm'; + +import '@/services/i18n'; + +const onSubmit = vi.fn(); + +const RULE: PermissionSetRule = { action: 'read', scope: 'HOLDER_GROUPS', subject: 'Subject' }; + +const field = (name: string) => document.querySelector(`[name="${name}"]`)!; + +const type = (name: string, value: string) => fireEvent.change(field(name), { target: { value } }); + +const choose = (field: 'action' | 'subject', value: string) => { + fireEvent.keyDown(screen.getAllByTestId(`rule-${field}-select-trigger`).at(-1)!, { key: 'Enter' }); + fireEvent.click(screen.getByTestId(`rule-${field}-select-item-${value}`)); +}; + +const submit = () => fireEvent.click(screen.getByLabelText('Submit')); + +const errorMessages = () => screen.queryAllByTestId('error-message-text').map((element) => element.textContent); + +describe('PermissionSetForm', () => { + beforeEach(() => { + onSubmit.mockResolvedValue(undefined); + }); + + afterEach(() => { + cleanup(); + vi.clearAllMocks(); + }); + + it('should submit the name and the rule rows filled in, without needing each row added first', async () => { + render(); + type('name', 'Research Assistant'); + choose('action', 'read'); + choose('subject', 'Subject'); + submit(); + await waitFor(() => { + expect(onSubmit).toHaveBeenCalledWith({ name: 'Research Assistant', rules: [RULE] }); + }); + }); + + it('should refuse a set with no rules, which would grant nothing', async () => { + render(); + type('name', 'Empty'); + submit(); + await waitFor(() => expect(errorMessages().join()).toContain('Add at least one rule')); + expect(onSubmit).not.toHaveBeenCalled(); + }); + + it('should refuse a half-filled rule row and highlight it, rather than silently dropping it', async () => { + render(); + choose('action', 'create'); + submit(); + await waitFor(() => expect(errorMessages().join()).toContain('A rule row is incomplete')); + expect(screen.getByTestId('add-rule-row').getAttribute('aria-invalid')).toBe('true'); + expect(onSubmit).not.toHaveBeenCalled(); + }); + + it('should refuse a set without a name', async () => { + render(); + choose('action', 'read'); + choose('subject', 'Subject'); + submit(); + await waitFor(() => expect(errorMessages().length).toBeGreaterThan(0)); + expect(onSubmit).not.toHaveBeenCalled(); + }); + + it('should start from the set being edited', () => { + render( + + ); + expect(field('name').value).toBe('Research Assistant'); + expect(screen.getAllByTestId('permission-set-rule-row')).toHaveLength(1); + }); + + it('should render a secondary action beside the submit button', () => { + render( + } + submitLabel="Save" + onSubmit={onSubmit} + /> + ); + expect(screen.getByTestId('secondary-action').parentElement).toBe(screen.getByLabelText('Submit').parentElement); + }); + + it('should keep the saved rules listed and offer a fresh row once a save succeeds', async () => { + render(); + choose('action', 'create'); + choose('subject', 'Session'); + submit(); + await waitFor(() => expect(screen.getAllByTestId('permission-set-rule-row')).toHaveLength(2)); + expect(screen.getByTestId('rule-action-select-trigger').textContent).toBe('Choose…'); + expect(field('name').value).toBe('Set'); + }); + + it('should show why a save failed and keep the unsaved row, so nothing entered is lost', async () => { + onSubmit.mockResolvedValue({ errorMessage: "Permission set with name 'Set' already exists!", success: false }); + render(); + choose('action', 'create'); + choose('subject', 'Session'); + submit(); + expect(await screen.findByText("Permission set with name 'Set' already exists!")).toBeTruthy(); + expect(screen.getAllByTestId('permission-set-rule-row')).toHaveLength(1); + expect(screen.getByTestId('rule-action-select-trigger').textContent).toBe('Create'); + }); +}); diff --git a/apps/web/src/components/PermissionSetForm/index.ts b/apps/web/src/components/PermissionSetForm/index.ts new file mode 100644 index 000000000..81a96303d --- /dev/null +++ b/apps/web/src/components/PermissionSetForm/index.ts @@ -0,0 +1 @@ +export * from './PermissionSetForm'; diff --git a/apps/web/src/components/PermissionSetRulesEditor/PermissionSetRulesEditor.stories.tsx b/apps/web/src/components/PermissionSetRulesEditor/PermissionSetRulesEditor.stories.tsx new file mode 100644 index 000000000..2becf4ef4 --- /dev/null +++ b/apps/web/src/components/PermissionSetRulesEditor/PermissionSetRulesEditor.stories.tsx @@ -0,0 +1,62 @@ +import { useState } from 'react'; + +import type { PermissionSetRule } from '@opendatacapture/schemas/permission-set'; +import type { Meta, StoryObj } from '@storybook/react-vite'; + +import { createRuleDraft } from '@/utils/permission-set-rules'; +import type { PermissionSetRuleDraft } from '@/utils/permission-set-rules'; + +import { PermissionSetRulesEditor } from './PermissionSetRulesEditor'; + +type Story = StoryObj; + +const RULES: PermissionSetRule[] = [ + { action: 'read', scope: 'HOLDER_GROUPS', subject: 'Subject' }, + { action: 'read', scope: 'HOLDER_GROUPS', subject: 'InstrumentRecord' }, + { action: 'read', scope: 'ALL_GROUPS', subject: 'Instrument' } +]; + +const EditableRules = ({ rules }: { rules: PermissionSetRule[] }) => { + const [currentRules, setRules] = useState(rules); + const [drafts, setDrafts] = useState([createRuleDraft()]); + return ( + + ); +}; + +export default { + component: PermissionSetRulesEditor, + decorators: [ + (Story) => ( +
+ +
+ ) + ], + parameters: { + layout: 'centered' + } +} as Meta; + +/** A new set: nothing saved yet, one row ready to fill in. */ +export const Empty: Story = { + render: () => +}; + +/** A set being edited, with rules on both scopes. */ +export const Editing: Story = { + render: () => +}; + +/** How a user's page shows the set they hold. */ +export const ReadOnly: Story = { + args: { + readOnly: true, + rules: RULES + } +}; diff --git a/apps/web/src/components/PermissionSetRulesEditor/PermissionSetRulesEditor.tsx b/apps/web/src/components/PermissionSetRulesEditor/PermissionSetRulesEditor.tsx new file mode 100644 index 000000000..dbb448966 --- /dev/null +++ b/apps/web/src/components/PermissionSetRulesEditor/PermissionSetRulesEditor.tsx @@ -0,0 +1,221 @@ +import { Button, Table } from '@douglasneuroinformatics/libui/components'; +import { useTranslation } from '@douglasneuroinformatics/libui/hooks'; +import { cn } from '@douglasneuroinformatics/libui/utils'; +import { $AppAction, $AppSubjectName, isGroupScopableSubject } from '@opendatacapture/schemas/core'; +import { $PermissionSetScope, isPermissionSetGrantable } from '@opendatacapture/schemas/permission-set'; +import type { PermissionSetRule, PermissionSetScope } from '@opendatacapture/schemas/permission-set'; +import { GlobeIcon, PlusIcon, Trash2Icon, UsersIcon } from 'lucide-react'; + +import { CellSelect } from '@/components/CellSelect'; +import { usePermissionLabels } from '@/hooks/usePermissionLabels'; +import { createRuleDraft, isIncompleteRuleDraft } from '@/utils/permission-set-rules'; +import type { PermissionSetRuleDraft } from '@/utils/permission-set-rules'; +import { grantableActions, grantableSubjects, withoutPermission } from '@/utils/permissions'; + +type PermissionSetRulesEditorProps = + | { + drafts: PermissionSetRuleDraft[]; + highlightIncomplete?: boolean; + onDraftsChange: (drafts: PermissionSetRuleDraft[]) => void; + onRulesChange: (rules: PermissionSetRule[]) => void; + readOnly?: false; + rules: PermissionSetRule[]; + } + | { + readOnly: true; + rules: PermissionSetRule[]; + }; + +/** + * The rules of a permission set: the saved ones above, with rows for adding more beneath them. Only + * what a set may contain is offered, so `manage all` never is. Read-only, it lists the rules alone. + */ +export const PermissionSetRulesEditor = (props: PermissionSetRulesEditorProps) => { + const { t } = useTranslation(); + const { actionLabels, columnLabels, subjectLabels } = usePermissionLabels(); + const { rules } = props; + const editor = props.readOnly ? null : props; + + const scopeLabels: { [K in PermissionSetScope]: string } = { + ALL_GROUPS: t({ en: 'Every group', es: 'Todos los grupos', fr: 'Tous les groupes' }), + HOLDER_GROUPS: t({ en: "Holder's groups", es: 'Grupos del titular', fr: 'Groupes du titulaire' }) + }; + const placeholder = t({ en: 'Choose…', es: 'Elija…', fr: 'Choisir…' }); + const removeLabel = t({ en: 'Remove rule', es: 'Retirar la regla', fr: 'Retirer la règle' }); + const addLabel = t({ en: 'Add Rule', es: 'Agregar una regla', fr: 'Ajouter une règle' }); + + const updateDraft = (index: number, changes: Partial) => { + editor?.onDraftsChange( + editor.drafts.map((draft, draftIndex) => (draftIndex === index ? { ...draft, ...changes } : draft)) + ); + }; + + return ( +
+
+ + + + {columnLabels.action} + {columnLabels.subject} + {columnLabels.scope} + {editor && } + + + + {rules.map((rule, index) => ( + + {actionLabels[rule.action]} + {subjectLabels[rule.subject]} + + + {rule.scope === 'ALL_GROUPS' ? ( + + ) : ( + + )} + {scopeLabels[rule.scope]} + + + {editor && ( + + + + )} + + ))} + + {editor && ( + + {editor.drafts.map((draft, index) => { + const { action, scope, subject } = draft; + const isFlagged = editor.highlightIncomplete && isIncompleteRuleDraft(draft); + return ( + + + [ + option, + actionLabels[option] + ]) + )} + placeholder={placeholder} + value={action} + onValueChange={(value) => updateDraft(index, { action: $AppAction.parse(value) })} + /> + + + [ + option, + subjectLabels[option] + ]) + )} + placeholder={placeholder} + value={subject} + onValueChange={(value) => updateDraft(index, { subject: $AppSubjectName.parse(value) })} + /> + + + {subject !== undefined && + (isGroupScopableSubject(subject) ? ( + updateDraft(index, { scope: $PermissionSetScope.parse(value) })} + /> + ) : ( + + {scopeLabels.ALL_GROUPS} + + ))} + + + + + + + ); + })} + {editor.drafts.length === 0 && ( + + + + + + )} + + )} +
+
+ {editor?.highlightIncomplete && editor.drafts.some(isIncompleteRuleDraft) && ( +

+ {t({ + en: 'Complete or remove the highlighted rule rows before saving.', + es: 'Complete o elimine las filas de reglas resaltadas antes de guardar.', + fr: 'Complétez ou retirez les lignes de règle en surbrillance avant d’enregistrer.' + })} +

+ )} +
+ ); +}; diff --git a/apps/web/src/components/PermissionSetRulesEditor/__tests__/PermissionSetRulesEditor.test.tsx b/apps/web/src/components/PermissionSetRulesEditor/__tests__/PermissionSetRulesEditor.test.tsx new file mode 100644 index 000000000..5f1d82f81 --- /dev/null +++ b/apps/web/src/components/PermissionSetRulesEditor/__tests__/PermissionSetRulesEditor.test.tsx @@ -0,0 +1,155 @@ +import { useState } from 'react'; + +import type { PermissionSetRule } from '@opendatacapture/schemas/permission-set'; +import { cleanup, fireEvent, render, screen } from '@testing-library/react'; +import { afterEach, describe, expect, it, vi } from 'vitest'; + +import { PermissionSetRulesEditor } from '@/components/PermissionSetRulesEditor/PermissionSetRulesEditor'; +import { createRuleDraft } from '@/utils/permission-set-rules'; +import type { PermissionSetRuleDraft } from '@/utils/permission-set-rules'; + +import '@/services/i18n'; + +const onRulesChange = vi.fn(); +const onDraftsChange = vi.fn(); + +const RULES: PermissionSetRule[] = [ + { action: 'read', scope: 'HOLDER_GROUPS', subject: 'Subject' }, + { action: 'read', scope: 'ALL_GROUPS', subject: 'Instrument' } +]; + +const Editor = ({ + highlightIncomplete, + rules = RULES +}: { + highlightIncomplete?: boolean; + rules?: PermissionSetRule[]; +}) => { + const [currentRules, setRules] = useState(rules); + const [drafts, setDrafts] = useState([createRuleDraft()]); + return ( + { + setDrafts(drafts); + onDraftsChange(drafts); + }} + onRulesChange={(rules) => { + setRules(rules); + onRulesChange(rules); + }} + /> + ); +}; + +/** Opens one of the add-row selects from the keyboard, since happy-dom dispatches no pointer capture. */ +const choose = (field: 'action' | 'scope' | 'subject', value: string, rowIndex = 0) => { + fireEvent.keyDown(screen.getAllByTestId(`rule-${field}-select-trigger`)[rowIndex]!, { key: 'Enter' }); + fireEvent.click(screen.getByTestId(`rule-${field}-select-item-${value}`)); +}; + +/** The options an add-row select offers, read off its open list. */ +const optionsOf = (field: 'action' | 'subject') => { + fireEvent.keyDown(screen.getByTestId(`rule-${field}-select-trigger`), { key: 'Enter' }); + return screen + .getAllByTestId(new RegExp(`^rule-${field}-select-item-`)) + .map((item) => item.dataset.testid!.replace(`rule-${field}-select-item-`, '')); +}; + +describe('PermissionSetRulesEditor', () => { + afterEach(() => { + cleanup(); + vi.clearAllMocks(); + }); + + it('should list every rule with where it applies', () => { + render(); + expect(screen.getAllByTestId('permission-set-rule-row')).toHaveLength(2); + expect(screen.getAllByTestId('permission-set-rule-scope').map((cell) => cell.textContent)).toEqual([ + "Holder's groups", + 'Every group' + ]); + }); + + it('should stage the removal of a rule without touching the others', () => { + render(); + fireEvent.click(screen.getAllByTestId('permission-set-rule-remove')[0]!); + expect(onRulesChange).toHaveBeenCalledWith([RULES[1]]); + }); + + it("should stage a complete row confined to the holder's groups unless another scope is chosen", () => { + render(); + choose('action', 'read'); + choose('subject', 'Session'); + expect(onDraftsChange.mock.lastCall?.[0]).toEqual([{ action: 'read', scope: 'HOLDER_GROUPS', subject: 'Session' }]); + choose('scope', 'ALL_GROUPS'); + expect(onDraftsChange.mock.lastCall?.[0]).toEqual([{ action: 'read', scope: 'ALL_GROUPS', subject: 'Session' }]); + }); + + it('should offer no scope until a resource is chosen', () => { + render(); + expect(screen.queryByTestId('rule-scope-select-trigger')).toBeNull(); + }); + + it('should fix the scope to every group for a resource no group owns', () => { + render(); + choose('subject', 'Instrument'); + expect(screen.queryByTestId('rule-scope-select-trigger')).toBeNull(); + expect(screen.getByTestId('rule-scope-fixed').textContent).toBe('Every group'); + }); + + it('should never offer manage on all, which would make every holder an administrator', () => { + render(); + choose('action', 'manage'); + expect(optionsOf('subject')).not.toContain('all'); + }); + + it('should offer only read once User is chosen, since every route that writes a user is admin-only', () => { + render(); + choose('subject', 'User'); + expect(optionsOf('action')).toEqual(['read']); + }); + + it('should leave the other rows untouched when one changes', () => { + render(); + fireEvent.click(screen.getByTestId('rule-draft-add')); + choose('action', 'create', 1); + expect(onDraftsChange.mock.lastCall?.[0]).toEqual([ + { scope: 'HOLDER_GROUPS' }, + { action: 'create', scope: 'HOLDER_GROUPS' } + ]); + }); + + it('should offer a fresh row once the last one is removed', () => { + render(); + fireEvent.click(screen.getByTestId('rule-draft-remove')); + expect(screen.queryAllByTestId('add-rule-row')).toHaveLength(0); + fireEvent.click(screen.getByTestId('rule-draft-add')); + expect(screen.getAllByTestId('add-rule-row')).toHaveLength(1); + expect(onRulesChange).not.toHaveBeenCalled(); + }); + + it('should flag a started but unfinished row once a save has been refused', () => { + render(); + expect(screen.queryByTestId('rule-drafts-incomplete')).toBeNull(); + choose('action', 'read'); + expect(screen.getByTestId('add-rule-row').getAttribute('aria-invalid')).toBe('true'); + expect(screen.getByTestId('rule-drafts-incomplete')).toBeTruthy(); + }); + + it('should not flag an unfinished row before any save is attempted', () => { + render(); + choose('action', 'read'); + expect(screen.getByTestId('add-rule-row').getAttribute('aria-invalid')).toBeNull(); + }); + + it('should list the rules alone when read-only, with nothing to add or remove', () => { + render(); + expect(screen.getAllByTestId('permission-set-rule-row')).toHaveLength(2); + expect(screen.queryByTestId('permission-set-rule-remove')).toBeNull(); + expect(screen.queryByTestId('add-rule-row')).toBeNull(); + expect(screen.queryByTestId('rule-draft-add')).toBeNull(); + }); +}); diff --git a/apps/web/src/components/PermissionSetRulesEditor/index.ts b/apps/web/src/components/PermissionSetRulesEditor/index.ts new file mode 100644 index 000000000..9eb42a089 --- /dev/null +++ b/apps/web/src/components/PermissionSetRulesEditor/index.ts @@ -0,0 +1 @@ +export * from './PermissionSetRulesEditor'; diff --git a/apps/web/src/components/UserPermissionsEditor/PermissionSetChoice.tsx b/apps/web/src/components/UserPermissionsEditor/PermissionSetChoice.tsx new file mode 100644 index 000000000..ff0a661e3 --- /dev/null +++ b/apps/web/src/components/UserPermissionsEditor/PermissionSetChoice.tsx @@ -0,0 +1,104 @@ +import { Label, Select } from '@douglasneuroinformatics/libui/components'; +import { useTranslation } from '@douglasneuroinformatics/libui/hooks'; +import type { PermissionSetSummary } from '@opendatacapture/schemas/permission-set'; +import { TriangleAlertIcon } from 'lucide-react'; + +import { PermissionSetRulesEditor } from '@/components/PermissionSetRulesEditor'; + +type PermissionSetChoiceProps = { + /** Whether the user holds saved custom grants, which assigning a set removes. */ + discardsCustomGrants: boolean; + highlightIncomplete?: boolean; + isSaving?: boolean; + onPermissionSetIdChange: (permissionSetId: string) => void; + permissionSetId: string | undefined; + permissionSets: PermissionSetSummary[]; +}; + +/** The permission set a user holds, chosen from every set, with what the chosen one grants beneath it. */ +export const PermissionSetChoice = ({ + discardsCustomGrants, + highlightIncomplete, + isSaving, + onPermissionSetIdChange, + permissionSetId, + permissionSets +}: PermissionSetChoiceProps) => { + const { t } = useTranslation(); + const chosen = permissionSets.find((permissionSet) => permissionSet.id === permissionSetId); + + if (permissionSets.length === 0) { + return ( +

+ {t({ + en: 'No permission sets exist yet. An administrator can create one under Manage Permission Sets.', + es: 'Todavía no existe ningún conjunto de permisos. Un administrador puede crear uno en Gestionar conjuntos de permisos.', + fr: "Aucun ensemble d'autorisations n'existe encore. Un administrateur peut en créer un sous Gérer les ensembles d'autorisations." + })} +

+ ); + } + + return ( +
+
+ + +
+ {highlightIncomplete && !chosen && ( +

+ {t({ + en: 'Choose a permission set, or switch to custom permissions.', + es: 'Elija un conjunto de permisos o cambie a permisos personalizados.', + fr: "Choisissez un ensemble d'autorisations ou passez aux autorisations personnalisées." + })} +

+ )} + {chosen && ( +
+ +
+ )} + {discardsCustomGrants && ( +
+ +

+ {t({ + en: "This user holds custom grants. Saving with a permission set removes them, since a user's permissions come from one or the other.", + es: 'Este usuario tiene permisos personalizados. Guardar con un conjunto de permisos los elimina, ya que los permisos de un usuario provienen de uno u otro.', + fr: "Cet utilisateur détient des autorisations personnalisées. Enregistrer avec un ensemble d'autorisations les retire, puisque les autorisations d'un utilisateur proviennent de l'un ou de l'autre." + })} +

+
+ )} +
+ ); +}; diff --git a/apps/web/src/components/UserPermissionsEditor/UserPermissionsEditor.tsx b/apps/web/src/components/UserPermissionsEditor/UserPermissionsEditor.tsx index 79ddc55a0..3897640c0 100644 --- a/apps/web/src/components/UserPermissionsEditor/UserPermissionsEditor.tsx +++ b/apps/web/src/components/UserPermissionsEditor/UserPermissionsEditor.tsx @@ -1,4 +1,4 @@ -import { Button, Card, Select, Table } from '@douglasneuroinformatics/libui/components'; +import { Button, Card, Table, Tabs } from '@douglasneuroinformatics/libui/components'; import { useTheme, useTranslation } from '@douglasneuroinformatics/libui/hooks'; import { cn } from '@douglasneuroinformatics/libui/utils'; import { @@ -7,13 +7,16 @@ import { isGrantablePermission, isGroupScopableSubject } from '@opendatacapture/schemas/core'; -import type { AppAction, AppSubjectName, Permissions } from '@opendatacapture/schemas/core'; +import type { Permissions } from '@opendatacapture/schemas/core'; import type { Group } from '@opendatacapture/schemas/group'; -import type { User } from '@opendatacapture/schemas/user'; +import type { PermissionSetSummary } from '@opendatacapture/schemas/permission-set'; +import type { $UpdateUserPermissionsData, User } from '@opendatacapture/schemas/user'; import { PlusIcon, Trash2Icon, TriangleAlertIcon } from 'lucide-react'; +import { CellSelect } from '@/components/CellSelect'; import { Chip } from '@/components/Chip'; import { ColorTagCell } from '@/components/ColorTagCell'; +import { usePermissionLabels } from '@/hooks/usePermissionLabels'; import { resolveTheme } from '@/utils/chart-theme'; import { ALL_GROUPS, @@ -25,34 +28,10 @@ import { import type { PermissionDraft } from '@/utils/permissions'; import { getAllGroupsScopeColor } from '@/utils/tag-colors'; -type CellSelectProps = { - disabled?: boolean; - label: string; - name: string; - onValueChange: (value: string) => void; - options: { [key: string]: string }; - placeholder: string; - value: string | undefined; -}; +import { PermissionSetChoice } from './PermissionSetChoice'; -const CellSelect = ({ disabled, label, name, onValueChange, options, placeholder, value }: CellSelectProps) => ( - -); +/** Where the user's permissions beyond their base level come from: a permission set, or their own grants. */ +type PermissionSource = $UpdateUserPermissionsData['kind']; type UserPermissionsEditorProps = { drafts: PermissionDraft[]; @@ -61,7 +40,12 @@ type UserPermissionsEditorProps = { isSaving?: boolean; onDraftsChange: (drafts: PermissionDraft[]) => void; onPermissionsChange: (permissions: Permissions) => void; + onPermissionSetIdChange: (permissionSetId: string) => void; + onSourceChange: (source: PermissionSource) => void; permissions: Permissions; + permissionSetId: string | undefined; + permissionSets: PermissionSetSummary[]; + source: PermissionSource; user: User; }; @@ -72,44 +56,20 @@ export const UserPermissionsEditor = ({ isSaving, onDraftsChange, onPermissionsChange, + onPermissionSetIdChange, + onSourceChange, permissions, + permissionSetId, + permissionSets, + source, user }: UserPermissionsEditorProps) => { const { t } = useTranslation(); + const { actionLabels, columnLabels, subjectLabels } = usePermissionLabels(); const userGroups = groups.filter((group) => user.groupIds.includes(group.id)); const defaultScope = userGroups.length === 1 ? userGroups[0]?.id : undefined; - const actionLabels: { [K in AppAction]: string } = { - create: t({ en: 'Create', es: 'Crear', fr: 'Créer' }), - delete: t({ en: 'Delete', es: 'Eliminar', fr: 'Supprimer' }), - manage: t({ en: 'Manage (All)', es: 'Gestionar (todo)', fr: 'Gérer (Tout)' }), - read: t({ en: 'Read', es: 'Leer', fr: 'Lire' }), - update: t({ en: 'Update', es: 'Actualizar', fr: 'Modifier' }) - }; - - const subjectLabels: { [K in AppSubjectName]: string } = { - all: t({ en: 'All', es: 'Todo', fr: 'Tous' }), - Assignment: t({ en: 'Assignment', es: 'Asignación', fr: 'Assignation' }), - Group: t({ en: 'Group', es: 'Grupo', fr: 'Groupe' }), - Instrument: t({ en: 'Instrument', es: 'Instrumento', fr: 'Instrument' }), - InstrumentRecord: t({ - en: 'Instrument Record', - es: 'Registro de instrumento', - fr: "Enregistrement de l'instrument" - }), - InstrumentRepo: t({ en: 'Instrument Repository', es: 'Repositorio de instrumentos', fr: "Dépôt d'instruments" }), - Session: t({ en: 'Session', es: 'Sesión', fr: 'Session' }), - Subject: t({ en: 'Subject', es: 'Sujeto', fr: 'Client' }), - User: t({ en: 'User', es: 'Usuario', fr: 'Utilisateur' }) - }; - - const columnLabels = { - action: t({ en: 'Action', es: 'Acción', fr: 'Action' }), - scope: t({ en: 'Scope', es: 'Alcance', fr: 'Portée' }), - subject: t({ en: 'Resource', es: 'Recurso', fr: 'Ressource' }) - }; - const [theme] = useTheme(); const allGroupsLabel = t({ en: 'All Groups', es: 'Todos los grupos', fr: 'Tous les groupes' }); const placeholder = t({ en: 'Choose…', es: 'Elija…', fr: 'Choisir…' }); @@ -149,210 +109,243 @@ export const UserPermissionsEditor = ({ {t({ en: 'Permissions', es: 'Permisos', fr: 'Autorisations' })} {t({ - en: 'Grants added here come on top of the base permission level. Changes take effect the next time this user signs in.', - es: 'Los permisos concedidos aquí se suman al nivel de permisos de base. Los cambios se aplican la próxima vez que este usuario inicie sesión.', - fr: "Les autorisations accordées ici s'ajoutent au niveau de base. Les modifications prennent effet à la prochaine connexion de cet utilisateur." + en: 'A permission set or custom grants come on top of the base permission level, never both. Changes take effect the next time this user signs in.', + es: 'Un conjunto de permisos o permisos personalizados se suman al nivel de permisos de base, nunca ambos. Los cambios se aplican la próxima vez que este usuario inicie sesión.', + fr: "Un ensemble d'autorisations ou des autorisations personnalisées s'ajoutent au niveau de base, jamais les deux. Les modifications prennent effet à la prochaine connexion de cet utilisateur." })} - {hasIneffectiveGrants && ( -

- {t({ - en: 'Only an administrator can create, modify or delete users, so grants marked "No effect" do nothing. They are removed the next time this user\'s permissions are changed.', - es: 'Solo un administrador puede crear, modificar o eliminar usuarios, por lo que los permisos marcados como «Sin efecto» no hacen nada. Se retiran la próxima vez que se modifiquen los permisos de este usuario.', - fr: 'Seul un administrateur peut créer, modifier ou supprimer des utilisateurs : les autorisations marquées « Sans effet » ne font donc rien. Elles sont retirées à la prochaine modification des autorisations de cet utilisateur.' - })} -

- )} -
- - - - {columnLabels.action} - {columnLabels.subject} - {columnLabels.scope} - - - - - {permissions.map((permission, index) => ( - - {actionLabels[permission.action]} - -
- {subjectLabels[permission.subject]} - {!isGrantablePermission(permission) && ( - - {t({ en: 'No effect', es: 'Sin efecto', fr: 'Sans effet' })} - - )} -
- - - {permission.groupId === null ? ( - - ) : ( - (groups.find((group) => group.id === permission.groupId)?.name ?? permission.groupId) - )} - - -
+ + + {columnLabels.action} + {columnLabels.subject} + {columnLabels.scope} + + + + + {permissions.map((permission, index) => ( + - - - - - ))} - - - {drafts.map((draft, index) => { - const { action, scope, subject } = draft; - const isFlagged = highlightIncomplete && isIncompleteDraft(draft); - return ( - - - [option, actionLabels[option]]) - )} - placeholder={placeholder} - value={action} - onValueChange={(value) => updateDraft(index, { action: $AppAction.parse(value) })} - /> - - - [option, subjectLabels[option]]) + {actionLabels[permission.action]} + +
+ {subjectLabels[permission.subject]} + {!isGrantablePermission(permission) && ( + + {t({ en: 'No effect', es: 'Sin efecto', fr: 'Sans effet' })} + + )} +
+ + + {permission.groupId === null ? ( + + ) : ( + (groups.find((group) => group.id === permission.groupId)?.name ?? permission.groupId) )} - placeholder={placeholder} - value={subject} - onValueChange={(value) => updateDraft(index, { subject: $AppSubjectName.parse(value) })} - /> - - - {subject !== undefined && isGroupScopableSubject(subject) && ( - + + - + + + ))} + + + {drafts.map((draft, index) => { + const { action, scope, subject } = draft; + const isFlagged = highlightIncomplete && isIncompleteDraft(draft); + return ( + - - - - - ); - })} - {drafts.length === 0 && ( - - - - - - )} - -
-
- {highlightIncomplete && drafts.some(isIncompleteDraft) && ( -

- {t({ - en: 'Complete or remove the highlighted permission rows before saving.', - es: 'Complete o elimine las filas de permisos resaltadas antes de guardar.', - fr: "Complétez ou retirez les lignes d'autorisation en surbrillance avant d'enregistrer." - })} -

- )} - {isManageAll && ( -
- -

- {t({ - en: "Manage (All) makes this user an administrator: they can read and change every group's data, manage every user's account and permissions, and create instruments, which can run code on the server.", - es: 'Gestionar (todo) convierte a este usuario en administrador: puede consultar y modificar los datos de todos los grupos, gestionar la cuenta y los permisos de cada usuario, y crear instrumentos, los cuales pueden ejecutar código en el servidor.', - fr: 'Gérer (Tout) fait de cet utilisateur un administrateur, qui peut consulter et modifier les données de tous les groupes, gérer le compte et les autorisations de chaque utilisateur, et créer des instruments, lesquels peuvent exécuter du code sur le serveur.' - })} -

+ + [option, actionLabels[option]]) + )} + placeholder={placeholder} + value={action} + onValueChange={(value) => updateDraft(index, { action: $AppAction.parse(value) })} + /> + + + [option, subjectLabels[option]]) + )} + placeholder={placeholder} + value={subject} + onValueChange={(value) => updateDraft(index, { subject: $AppSubjectName.parse(value) })} + /> + + + {subject !== undefined && isGroupScopableSubject(subject) && ( + updateDraft(index, { scope })} + /> + )} + + + + + + + ); + })} + {drafts.length === 0 && ( + + + + + + )} + + +
+ {highlightIncomplete && drafts.some(isIncompleteDraft) && ( +

+ {t({ + en: 'Complete or remove the highlighted permission rows before saving.', + es: 'Complete o elimine las filas de permisos resaltadas antes de guardar.', + fr: "Complétez ou retirez les lignes d'autorisation en surbrillance avant d'enregistrer." + })} +

+ )} + {isManageAll && ( +
+ +

+ {t({ + en: "Manage (All) makes this user an administrator: they can read and change every group's data, manage every user's account and permissions, and create instruments, which can run code on the server.", + es: 'Gestionar (todo) convierte a este usuario en administrador: puede consultar y modificar los datos de todos los grupos, gestionar la cuenta y los permisos de cada usuario, y crear instrumentos, los cuales pueden ejecutar código en el servidor.', + fr: 'Gérer (Tout) fait de cet utilisateur un administrateur, qui peut consulter et modifier les données de tous les groupes, gérer le compte et les autorisations de chaque utilisateur, et créer des instruments, lesquels peuvent exécuter du code sur le serveur.' + })} +

+
+ )} )} ); }; + +export type { PermissionSource }; diff --git a/apps/web/src/components/UserPermissionsEditor/__tests__/UserPermissionsEditor.test.tsx b/apps/web/src/components/UserPermissionsEditor/__tests__/UserPermissionsEditor.test.tsx index 8650b5a4c..675fa6a3c 100644 --- a/apps/web/src/components/UserPermissionsEditor/__tests__/UserPermissionsEditor.test.tsx +++ b/apps/web/src/components/UserPermissionsEditor/__tests__/UserPermissionsEditor.test.tsx @@ -2,11 +2,13 @@ import { useState } from 'react'; import type { ComponentProps } from 'react'; import type { Permissions } from '@opendatacapture/schemas/core'; +import type { PermissionSetSummary } from '@opendatacapture/schemas/permission-set'; import type { User } from '@opendatacapture/schemas/user'; import { cleanup, fireEvent, render, screen, within } from '@testing-library/react'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import { UserPermissionsEditor } from '@/components/UserPermissionsEditor/UserPermissionsEditor'; +import type { PermissionSource } from '@/components/UserPermissionsEditor/UserPermissionsEditor'; import type { PermissionDraft } from '@/utils/permissions'; import '@/services/i18n'; @@ -15,22 +17,54 @@ vi.mock('@/hooks/useSetupStateQuery', () => ({ useSetupStateQuery: () => ({ data const onPermissionsChange = vi.fn(); const onDraftsChange = vi.fn(); +const onPermissionSetIdChange = vi.fn(); +const onSourceChange = vi.fn(); + +const PERMISSION_SETS: PermissionSetSummary[] = [ + { + createdAt: new Date('2026-01-01'), + holderCount: 3, + id: 'set-1', + name: 'Research Assistant', + rules: [ + { action: 'read', scope: 'HOLDER_GROUPS', subject: 'Subject' }, + { action: 'read', scope: 'ALL_GROUPS', subject: 'Instrument' } + ], + updatedAt: new Date('2026-01-01') + }, + { + createdAt: new Date('2026-01-01'), + holderCount: 0, + id: 'set-2', + name: 'Coordinator', + rules: [{ action: 'create', scope: 'HOLDER_GROUPS', subject: 'Session' }], + updatedAt: new Date('2026-01-01') + } +]; const Editor = ({ groups, highlightIncomplete, + permissionSets = PERMISSION_SETS, user -}: Pick, 'groups' | 'highlightIncomplete' | 'user'>) => { +}: Pick, 'groups' | 'highlightIncomplete' | 'user'> & { + permissionSets?: PermissionSetSummary[]; +}) => { const [permissions, setPermissions] = useState(user.additionalPermissions); const [drafts, setDrafts] = useState([ { scope: user.groupIds.length === 1 ? user.groupIds[0] : undefined } ]); + const [source, setSource] = useState(user.permissionSetId ? 'permissionSet' : 'custom'); + const [permissionSetId, setPermissionSetId] = useState(user.permissionSetId ?? undefined); return ( { setDrafts(drafts); @@ -40,10 +74,28 @@ const Editor = ({ setPermissions(permissions); onPermissionsChange(permissions); }} + onPermissionSetIdChange={(permissionSetId) => { + setPermissionSetId(permissionSetId); + onPermissionSetIdChange(permissionSetId); + }} + onSourceChange={(source) => { + setSource(source); + onSourceChange(source); + }} /> ); }; +/** Radix tabs activate on a primary-button mouse down, which happy-dom passes through. */ +const chooseSource = (source: PermissionSource) => { + fireEvent.mouseDown(screen.getByTestId(`user-permissions-source-${source}`), { button: 0, ctrlKey: false }); +}; + +const choosePermissionSet = (id: string) => { + fireEvent.keyDown(screen.getByTestId('permission-set-select-trigger'), { key: 'Enter' }); + fireEvent.click(screen.getByTestId(`permission-set-select-item-${id}`)); +}; + /** Opens one of the add-row selects from the keyboard, since happy-dom dispatches no pointer capture. */ const choose = (field: 'action' | 'scope' | 'subject', value: string, rowIndex = 0) => { fireEvent.keyDown(screen.getAllByTestId(`${field}-select-trigger`)[rowIndex]!, { key: 'Enter' }); @@ -228,5 +280,63 @@ describe('UserPermissionsEditor', () => { expect(screen.getByTestId('user-permissions-admin-notice')).toBeTruthy(); expect(screen.queryByTestId('user-permissions-table')).toBeNull(); expect(screen.queryByTestId('add-permission-row')).toBeNull(); + expect(screen.queryByTestId('user-permissions-source')).toBeNull(); + }); + + describe('permission source', () => { + const custom: User = { ...user, additionalPermissions: [] }; + + it('should switch to choosing a permission set, hiding the custom grants', () => { + render(); + expect(screen.getByTestId('user-permissions-custom')).toBeTruthy(); + chooseSource('permissionSet'); + expect(onSourceChange).toHaveBeenCalledWith('permissionSet'); + expect(screen.queryByTestId('user-permissions-custom')).toBeNull(); + expect(screen.getByTestId('permission-set-select-trigger')).toBeTruthy(); + }); + + it('should switch back to the custom grants', () => { + render(); + chooseSource('custom'); + expect(onSourceChange).toHaveBeenCalledWith('custom'); + expect(screen.getByTestId('user-permissions-custom')).toBeTruthy(); + }); + + it('should show what the chosen permission set grants, read-only', () => { + render(); + chooseSource('permissionSet'); + choosePermissionSet('set-1'); + expect(onPermissionSetIdChange).toHaveBeenCalledWith('set-1'); + expect(screen.getAllByTestId('permission-set-rule-row')).toHaveLength(2); + expect(screen.queryByTestId('permission-set-rule-remove')).toBeNull(); + }); + + it('should warn that saving with a set discards the custom grants the user holds', () => { + render(); + chooseSource('permissionSet'); + expect(screen.getByTestId('user-permissions-discard-warning')).toBeTruthy(); + }); + + it('should not warn about discarding grants for a user who holds none', () => { + render(); + chooseSource('permissionSet'); + expect(screen.queryByTestId('user-permissions-discard-warning')).toBeNull(); + }); + + it('should say so when no permission set exists to choose from', () => { + render(); + chooseSource('permissionSet'); + expect(screen.getByTestId('user-permission-set-none')).toBeTruthy(); + expect(screen.queryByTestId('permission-set-select-trigger')).toBeNull(); + }); + + it('should flag a missing choice once a save has been refused', () => { + render(); + chooseSource('permissionSet'); + expect(screen.getByTestId('permission-set-required')).toBeTruthy(); + expect(screen.getByTestId('permission-set-select-trigger').getAttribute('aria-invalid')).toBe('true'); + choosePermissionSet('set-2'); + expect(screen.queryByTestId('permission-set-required')).toBeNull(); + }); }); }); diff --git a/apps/web/src/hooks/__tests__/useConfirmPermissionSetDeletion.test.tsx b/apps/web/src/hooks/__tests__/useConfirmPermissionSetDeletion.test.tsx new file mode 100644 index 000000000..01ca8baeb --- /dev/null +++ b/apps/web/src/hooks/__tests__/useConfirmPermissionSetDeletion.test.tsx @@ -0,0 +1,69 @@ +import type { PropsWithChildren } from 'react'; + +import { CoreProvider } from '@douglasneuroinformatics/libui/providers'; +import { act, cleanup, fireEvent, renderHook, screen, within } from '@testing-library/react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { useConfirmPermissionSetDeletion } from '../useConfirmPermissionSetDeletion'; + +import '@/services/i18n'; + +const deleteMutate = vi.hoisted(() => vi.fn()); + +vi.mock('@/hooks/useDeletePermissionSetMutation', () => ({ + useDeletePermissionSetMutation: () => ({ mutate: deleteMutate }) +})); + +const PERMISSION_SET = { holderCount: 3, id: 'set-1', name: 'Research Assistant' }; + +const renderConfirm = () => { + const wrapper = ({ children }: PropsWithChildren) => {children}; + return renderHook(() => useConfirmPermissionSetDeletion(), { wrapper }).result; +}; + +const confirmation = () => screen.getByTestId('destructive-action-dialog'); + +describe('useConfirmPermissionSetDeletion', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + afterEach(() => { + // libui queues confirmations in a module-level store, so one left open would greet the next test. + const dialog = screen.queryByTestId('destructive-action-dialog'); + if (dialog) { + fireEvent.click(within(dialog).getByRole('button', { name: 'No' })); + } + cleanup(); + }); + + it.each([ + [0, 'No user holds this permission set, so deleting it changes no one’s permissions.'], + [1, '1 user holds this permission set. Deleting it removes it from them'], + [3, '3 users hold this permission set. Deleting it removes it from them'] + ])( + 'should tell the admin how %i holders are affected, since deleting takes the set from each', + (holderCount, wording) => { + const result = renderConfirm(); + act(() => result.current({ ...PERMISSION_SET, holderCount })); + expect(confirmation().textContent).toContain('Delete the permission set "Research Assistant"?'); + expect(confirmation().textContent).toContain(wording); + expect(deleteMutate).not.toHaveBeenCalled(); + } + ); + + it('should delete the set once confirmed, then report it deleted', () => { + const onDeleted = vi.fn(); + const result = renderConfirm(); + act(() => result.current(PERMISSION_SET, { onDeleted })); + fireEvent.click(within(confirmation()).getByRole('button', { name: 'Yes' })); + expect(deleteMutate).toHaveBeenCalledWith({ id: 'set-1' }, { onSuccess: onDeleted }); + }); + + it('should leave the set alone when the admin declines', () => { + const result = renderConfirm(); + act(() => result.current(PERMISSION_SET)); + fireEvent.click(within(confirmation()).getByRole('button', { name: 'No' })); + expect(deleteMutate).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/web/src/hooks/__tests__/useCreatePermissionSetMutation.test.ts b/apps/web/src/hooks/__tests__/useCreatePermissionSetMutation.test.ts new file mode 100644 index 000000000..d5edebd6c --- /dev/null +++ b/apps/web/src/hooks/__tests__/useCreatePermissionSetMutation.test.ts @@ -0,0 +1,63 @@ +import type { PropsWithChildren } from 'react'; +import { createElement } from 'react'; + +import type { $CreatePermissionSetData } from '@opendatacapture/schemas/permission-set'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { act, renderHook, waitFor } from '@testing-library/react'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { useCreatePermissionSetMutation } from '../useCreatePermissionSetMutation'; + +const mockAxios = vi.hoisted(() => ({ post: vi.fn() })); +const addNotification = vi.hoisted(() => vi.fn()); + +vi.mock('axios', () => ({ default: mockAxios })); + +vi.mock('@douglasneuroinformatics/libui/hooks', () => ({ + useNotificationsStore: vi.fn((selector) => selector({ addNotification })) +})); + +const DATA: $CreatePermissionSetData = { + name: 'Research Assistant', + rules: [{ action: 'read', scope: 'HOLDER_GROUPS', subject: 'Subject' }] +}; + +function renderCreateMutation() { + // The app's own default, so a rejection that is not kept away from the error boundary throws. + const queryClient = new QueryClient({ defaultOptions: { mutations: { retry: false, throwOnError: true } } }); + const invalidateQueries = vi.spyOn(queryClient, 'invalidateQueries'); + const wrapper = ({ children }: PropsWithChildren) => { + return createElement(QueryClientProvider, { children, client: queryClient }); + }; + return { ...renderHook(() => useCreatePermissionSetMutation(), { wrapper }), invalidateQueries }; +} + +describe('useCreatePermissionSetMutation', () => { + beforeEach(() => { + vi.clearAllMocks(); + mockAxios.post.mockResolvedValue({ data: {} }); + }); + + it('should send the set as the request body, without the default error toast the form replaces', async () => { + const { result } = renderCreateMutation(); + await result.current.mutateAsync({ data: DATA }); + expect(mockAxios.post).toHaveBeenCalledWith('/v1/permission-sets', DATA, { + meta: { disableDefaultErrorNotification: true } + }); + }); + + it('should announce success and refresh the sets, so the new set can be assigned straight away', async () => { + const { invalidateQueries, result } = renderCreateMutation(); + await result.current.mutateAsync({ data: DATA }); + expect(addNotification).toHaveBeenCalledWith({ type: 'success' }); + expect(invalidateQueries).toHaveBeenCalledWith({ queryKey: ['permission-sets'] }); + }); + + it('should keep a refused set out of the router error boundary, so the form keeps what was entered', async () => { + mockAxios.post.mockRejectedValue(new Error('Conflict')); + const { result } = renderCreateMutation(); + act(() => result.current.mutate({ data: DATA })); + await waitFor(() => expect(result.current.isError).toBe(true)); + expect(addNotification).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/web/src/hooks/__tests__/useCreateUserMutation.test.ts b/apps/web/src/hooks/__tests__/useCreateUserMutation.test.ts index b4b1ada9f..3ad86da95 100644 --- a/apps/web/src/hooks/__tests__/useCreateUserMutation.test.ts +++ b/apps/web/src/hooks/__tests__/useCreateUserMutation.test.ts @@ -60,6 +60,12 @@ describe('useCreateUserMutation', () => { }); }); + it('posts the permission set chosen for the user along with the rest of the account', async () => { + const { result } = renderCreateMutation(); + await result.current.mutateAsync({ data: { ...createUserData, permissionSetId: 'set-1' } }); + expect(mockAxios.post.mock.lastCall?.[1]).toMatchObject({ permissionSetId: 'set-1' }); + }); + it('parses the created user together with the welcome-email outcome', async () => { const { result } = renderCreateMutation(); const response = await result.current.mutateAsync({ data: createUserData }); diff --git a/apps/web/src/hooks/__tests__/useDeletePermissionSetMutation.test.ts b/apps/web/src/hooks/__tests__/useDeletePermissionSetMutation.test.ts new file mode 100644 index 000000000..1e0ccc00d --- /dev/null +++ b/apps/web/src/hooks/__tests__/useDeletePermissionSetMutation.test.ts @@ -0,0 +1,47 @@ +import type { PropsWithChildren } from 'react'; +import { createElement } from 'react'; + +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { renderHook } from '@testing-library/react'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { useDeletePermissionSetMutation } from '../useDeletePermissionSetMutation'; + +const mockAxios = vi.hoisted(() => ({ delete: vi.fn() })); +const addNotification = vi.hoisted(() => vi.fn()); + +vi.mock('axios', () => ({ default: mockAxios })); + +vi.mock('@douglasneuroinformatics/libui/hooks', () => ({ + useNotificationsStore: vi.fn((selector) => selector({ addNotification })) +})); + +function renderDeleteMutation() { + const queryClient = new QueryClient({ defaultOptions: { mutations: { retry: false } } }); + const invalidateQueries = vi.spyOn(queryClient, 'invalidateQueries'); + const wrapper = ({ children }: PropsWithChildren) => { + return createElement(QueryClientProvider, { children, client: queryClient }); + }; + return { ...renderHook(() => useDeletePermissionSetMutation(), { wrapper }), invalidateQueries }; +} + +describe('useDeletePermissionSetMutation', () => { + beforeEach(() => { + vi.clearAllMocks(); + mockAxios.delete.mockResolvedValue({ data: {} }); + }); + + it('should delete the set named by its id and announce it', async () => { + const { result } = renderDeleteMutation(); + await result.current.mutateAsync({ id: 'set-1' }); + expect(mockAxios.delete).toHaveBeenCalledWith('/v1/permission-sets/set-1'); + expect(addNotification).toHaveBeenCalledWith({ type: 'success' }); + }); + + it('should refresh the sets and the users, since every holder of the set has lost it', async () => { + const { invalidateQueries, result } = renderDeleteMutation(); + await result.current.mutateAsync({ id: 'set-1' }); + expect(invalidateQueries).toHaveBeenCalledWith({ queryKey: ['permission-sets'] }); + expect(invalidateQueries).toHaveBeenCalledWith({ queryKey: ['users'] }); + }); +}); diff --git a/apps/web/src/hooks/__tests__/useNavItems.test.ts b/apps/web/src/hooks/__tests__/useNavItems.test.ts index 9057c65dd..498e4b43e 100644 --- a/apps/web/src/hooks/__tests__/useNavItems.test.ts +++ b/apps/web/src/hooks/__tests__/useNavItems.test.ts @@ -201,6 +201,16 @@ describe('useNavItems', () => { expect(navUrls()).toContain('/upload'); }); + it('should offer the permission sets beside the users to an administrator', () => { + const urls = navUrls(); + expect(urls.indexOf('/admin/permission-sets')).toBe(urls.indexOf('/admin/users') + 1); + }); + + it('should hide the permission sets from anyone short of an administrator', () => { + mocks.can.mockImplementation((action, subject) => !(action === 'manage' && subject === 'all')); + expect(navUrls()).not.toContain('/admin/permission-sets'); + }); + it('should offer no navigation to a user without any permissions', () => { mocks.can.mockReturnValue(false); expect(renderHook(() => useNavItems()).result.current).toEqual([]); diff --git a/apps/web/src/hooks/__tests__/usePermissionLabels.test.ts b/apps/web/src/hooks/__tests__/usePermissionLabels.test.ts new file mode 100644 index 000000000..2eb53e5f6 --- /dev/null +++ b/apps/web/src/hooks/__tests__/usePermissionLabels.test.ts @@ -0,0 +1,32 @@ +import { i18n } from '@douglasneuroinformatics/libui/i18n'; +import { $AppAction, $AppSubjectName } from '@opendatacapture/schemas/core'; +import { cleanup, renderHook } from '@testing-library/react'; +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; + +import { usePermissionLabels } from '../usePermissionLabels'; + +import '@/services/i18n'; + +describe('usePermissionLabels', () => { + beforeEach(() => { + i18n.changeLanguage('en'); + }); + + afterEach(cleanup); + + it('should name every action and resource, so none reaches the screen as its stored value', () => { + const { result } = renderHook(() => usePermissionLabels()); + expect(Object.keys(result.current.actionLabels).toSorted()).toEqual($AppAction.options.toSorted()); + expect(Object.keys(result.current.subjectLabels).toSorted()).toEqual($AppSubjectName.options.toSorted()); + expect(result.current.subjectLabels.InstrumentRecord).toBe('Instrument Record'); + }); + + // The per-user grants and the rules of a permission set are both drawn from this map, so the same + // resource cannot be called one thing on a user's page and another in the set it holds. + it('should follow the interface language', () => { + i18n.changeLanguage('fr'); + const { result } = renderHook(() => usePermissionLabels()); + expect(result.current.actionLabels.read).toBe('Lire'); + expect(result.current.columnLabels.subject).toBe('Ressource'); + }); +}); diff --git a/apps/web/src/hooks/__tests__/usePermissionSetQuery.test.ts b/apps/web/src/hooks/__tests__/usePermissionSetQuery.test.ts new file mode 100644 index 000000000..072453307 --- /dev/null +++ b/apps/web/src/hooks/__tests__/usePermissionSetQuery.test.ts @@ -0,0 +1,67 @@ +import type { PropsWithChildren } from 'react'; +import { createElement, Suspense } from 'react'; + +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { renderHook, waitFor } from '@testing-library/react'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { z } from 'zod/v4'; + +import { permissionSetQueryOptions, usePermissionSetQuery } from '../usePermissionSetQuery'; +import { PERMISSION_SETS_QUERY_KEY } from '../usePermissionSetsQuery'; + +const mockAxios = vi.hoisted(() => ({ get: vi.fn() })); + +vi.mock('axios', () => ({ default: mockAxios })); + +const PERMISSION_SET = { + createdAt: '2026-01-01T00:00:00.000Z', + holderCount: 0, + id: 'set-1', + name: 'Research Assistant', + rules: [{ action: 'read', scope: 'HOLDER_GROUPS', subject: 'Subject' }], + updatedAt: '2026-01-02T00:00:00.000Z' +}; + +function createQueryClient() { + return new QueryClient({ defaultOptions: { queries: { retry: false } } }); +} + +describe('permissionSetQueryOptions', () => { + beforeEach(() => { + vi.clearAllMocks(); + mockAxios.get.mockResolvedValue({ data: PERMISSION_SET }); + }); + + it('should request the set named by its id', async () => { + const permissionSet = await createQueryClient().fetchQuery(permissionSetQueryOptions('set-1')); + expect(mockAxios.get).toHaveBeenCalledWith('/v1/permission-sets/set-1'); + expect(permissionSet.updatedAt).toEqual(new Date(PERMISSION_SET.updatedAt)); + }); + + it('should reject a response that is not a set', async () => { + mockAxios.get.mockResolvedValue({ data: { id: 'set-1' } }); + await expect(createQueryClient().fetchQuery(permissionSetQueryOptions('set-1'))).rejects.toThrow(z.ZodError); + }); + + it('should key each set under the list key, so invalidating the list refreshes it too', () => { + expect(permissionSetQueryOptions('set-1').queryKey).toEqual([PERMISSION_SETS_QUERY_KEY, 'set-1']); + expect(permissionSetQueryOptions('set-2').queryKey).not.toEqual(permissionSetQueryOptions('set-1').queryKey); + }); +}); + +describe('usePermissionSetQuery', () => { + it('should resolve to the set once the suspended request settles', async () => { + mockAxios.get.mockResolvedValue({ data: PERMISSION_SET }); + const queryClient = createQueryClient(); + const wrapper = ({ children }: PropsWithChildren) => { + return createElement( + QueryClientProvider, + { client: queryClient }, + createElement(Suspense, { fallback: null }, children) + ); + }; + const { result } = renderHook(() => usePermissionSetQuery('set-1'), { wrapper }); + await waitFor(() => expect(result.current).toBeTruthy()); + expect(result.current.data.name).toBe('Research Assistant'); + }); +}); diff --git a/apps/web/src/hooks/__tests__/usePermissionSetsQuery.test.ts b/apps/web/src/hooks/__tests__/usePermissionSetsQuery.test.ts new file mode 100644 index 000000000..a31ebc48d --- /dev/null +++ b/apps/web/src/hooks/__tests__/usePermissionSetsQuery.test.ts @@ -0,0 +1,69 @@ +import type { PropsWithChildren } from 'react'; +import { createElement, Suspense } from 'react'; + +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { renderHook, waitFor } from '@testing-library/react'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { z } from 'zod/v4'; + +import { + PERMISSION_SETS_QUERY_KEY, + permissionSetsQueryOptions, + usePermissionSetsQuery +} from '../usePermissionSetsQuery'; + +const mockAxios = vi.hoisted(() => ({ get: vi.fn() })); + +vi.mock('axios', () => ({ default: mockAxios })); + +const PERMISSION_SET = { + createdAt: '2026-01-01T00:00:00.000Z', + holderCount: 2, + id: 'set-1', + name: 'Research Assistant', + rules: [{ action: 'read', scope: 'HOLDER_GROUPS', subject: 'Subject' }], + updatedAt: '2026-01-02T00:00:00.000Z' +}; + +function createQueryClient() { + return new QueryClient({ defaultOptions: { queries: { retry: false } } }); +} + +describe('permissionSetsQueryOptions', () => { + beforeEach(() => { + vi.clearAllMocks(); + mockAxios.get.mockResolvedValue({ data: [PERMISSION_SET] }); + }); + + it('should parse the sets so dates arrive as Date objects rather than strings', async () => { + const permissionSets = await createQueryClient().fetchQuery(permissionSetsQueryOptions()); + expect(mockAxios.get).toHaveBeenCalledWith('/v1/permission-sets'); + expect(permissionSets[0]!.createdAt).toEqual(new Date(PERMISSION_SET.createdAt)); + }); + + it('should reject a set without its holder count, which the list and the delete warning both show', async () => { + mockAxios.get.mockResolvedValue({ data: [{ ...PERMISSION_SET, holderCount: undefined }] }); + await expect(createQueryClient().fetchQuery(permissionSetsQueryOptions())).rejects.toThrow(z.ZodError); + }); + + it('should cache under the shared key, so mutations can invalidate it', () => { + expect(permissionSetsQueryOptions().queryKey).toEqual([PERMISSION_SETS_QUERY_KEY]); + }); +}); + +describe('usePermissionSetsQuery', () => { + it('should resolve to the sets once the suspended request settles', async () => { + mockAxios.get.mockResolvedValue({ data: [PERMISSION_SET] }); + const queryClient = createQueryClient(); + const wrapper = ({ children }: PropsWithChildren) => { + return createElement( + QueryClientProvider, + { client: queryClient }, + createElement(Suspense, { fallback: null }, children) + ); + }; + const { result } = renderHook(() => usePermissionSetsQuery(), { wrapper }); + await waitFor(() => expect(result.current).toBeTruthy()); + expect(result.current.data.map((permissionSet) => permissionSet.id)).toEqual(['set-1']); + }); +}); diff --git a/apps/web/src/hooks/__tests__/useUpdatePermissionSetMutation.test.ts b/apps/web/src/hooks/__tests__/useUpdatePermissionSetMutation.test.ts new file mode 100644 index 000000000..1065fe334 --- /dev/null +++ b/apps/web/src/hooks/__tests__/useUpdatePermissionSetMutation.test.ts @@ -0,0 +1,58 @@ +import type { PropsWithChildren } from 'react'; +import { createElement } from 'react'; + +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { act, renderHook, waitFor } from '@testing-library/react'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { useUpdatePermissionSetMutation } from '../useUpdatePermissionSetMutation'; + +const mockAxios = vi.hoisted(() => ({ patch: vi.fn() })); +const addNotification = vi.hoisted(() => vi.fn()); + +vi.mock('axios', () => ({ default: mockAxios })); + +vi.mock('@douglasneuroinformatics/libui/hooks', () => ({ + useNotificationsStore: vi.fn((selector) => selector({ addNotification })) +})); + +function renderUpdateMutation() { + const queryClient = new QueryClient({ defaultOptions: { mutations: { retry: false, throwOnError: true } } }); + const invalidateQueries = vi.spyOn(queryClient, 'invalidateQueries'); + const wrapper = ({ children }: PropsWithChildren) => { + return createElement(QueryClientProvider, { children, client: queryClient }); + }; + return { ...renderHook(() => useUpdatePermissionSetMutation(), { wrapper }), invalidateQueries }; +} + +describe('useUpdatePermissionSetMutation', () => { + beforeEach(() => { + vi.clearAllMocks(); + mockAxios.patch.mockResolvedValue({ data: {} }); + }); + + it('should patch the set named by its id, without the default error toast the form replaces', async () => { + const { result } = renderUpdateMutation(); + await result.current.mutateAsync({ data: { name: 'Renamed' }, id: 'set-1' }); + expect(mockAxios.patch).toHaveBeenCalledWith( + '/v1/permission-sets/set-1', + { name: 'Renamed' }, + { meta: { disableDefaultErrorNotification: true } } + ); + }); + + it('should announce success and refresh the sets, so every page showing the set sees the change', async () => { + const { invalidateQueries, result } = renderUpdateMutation(); + await result.current.mutateAsync({ data: { name: 'Renamed' }, id: 'set-1' }); + expect(addNotification).toHaveBeenCalledWith({ type: 'success' }); + expect(invalidateQueries).toHaveBeenCalledWith({ queryKey: ['permission-sets'] }); + }); + + it('should keep a refused update out of the router error boundary, so the form keeps the edits', async () => { + mockAxios.patch.mockRejectedValue(new Error('Conflict')); + const { result } = renderUpdateMutation(); + act(() => result.current.mutate({ data: { name: 'Taken' }, id: 'set-1' })); + await waitFor(() => expect(result.current.isError).toBe(true)); + expect(addNotification).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/web/src/hooks/__tests__/useUpdateUserMutation.test.ts b/apps/web/src/hooks/__tests__/useUpdateUserMutation.test.ts index d40858cec..a1ae22b5c 100644 --- a/apps/web/src/hooks/__tests__/useUpdateUserMutation.test.ts +++ b/apps/web/src/hooks/__tests__/useUpdateUserMutation.test.ts @@ -1,7 +1,7 @@ import type { PropsWithChildren } from 'react'; import { createElement } from 'react'; -import type { Permissions } from '@opendatacapture/schemas/core'; +import type { $UpdateUserPermissionsData } from '@opendatacapture/schemas/user'; import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; import { cleanup, renderHook, waitFor } from '@testing-library/react'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; @@ -16,7 +16,10 @@ vi.mock('@douglasneuroinformatics/libui/hooks', () => ({ } })); -const permissions: Permissions = [{ action: 'read', groupId: 'group-1', subject: 'Subject' }]; +const permissions: $UpdateUserPermissionsData = { + kind: 'custom', + permissions: [{ action: 'read', groupId: 'group-1', subject: 'Subject' }] +}; function renderUpdateMutation() { const queryClient = new QueryClient({ defaultOptions: { mutations: { retry: false, throwOnError: true } } }); @@ -39,7 +42,7 @@ describe('useUpdateUserMutation', () => { const invalidate = vi.spyOn(queryClient, 'invalidateQueries'); await result.current.mutateAsync({ data: { email: 'jane@example.org' }, id: 'user-1', permissions }); expect(mocks.patch).toHaveBeenCalledWith('/v1/users/user-1', { email: 'jane@example.org' }, expect.anything()); - expect(mocks.put).toHaveBeenCalledWith('/v1/users/user-1/permissions', { permissions }, expect.anything()); + expect(mocks.put).toHaveBeenCalledWith('/v1/users/user-1/permissions', permissions, expect.anything()); expect(mocks.addNotification).toHaveBeenCalledExactlyOnceWith({ type: 'success' }); expect(invalidate).toHaveBeenCalledWith({ queryKey: ['users'] }); }); @@ -66,8 +69,19 @@ describe('useUpdateUserMutation', () => { it('should clear permissions when the last row was removed', async () => { const { result } = renderUpdateMutation(); - await result.current.mutateAsync({ data: {}, id: 'user-1', permissions: [] }); - expect(mocks.put).toHaveBeenCalledWith('/v1/users/user-1/permissions', { permissions: [] }, expect.anything()); + await result.current.mutateAsync({ data: {}, id: 'user-1', permissions: { kind: 'custom', permissions: [] } }); + expect(mocks.put).toHaveBeenCalledWith( + '/v1/users/user-1/permissions', + { kind: 'custom', permissions: [] }, + expect.anything() + ); + }); + + it('should send a permission set as it was given, so the server replaces any custom grants with it', async () => { + const { result } = renderUpdateMutation(); + const permissionSet: $UpdateUserPermissionsData = { kind: 'permissionSet', permissionSetId: 'set-1' }; + await result.current.mutateAsync({ data: {}, id: 'user-1', permissions: permissionSet }); + expect(mocks.put).toHaveBeenCalledWith('/v1/users/user-1/permissions', permissionSet, expect.anything()); }); it('should show no success confirmation when permissions fail to save', async () => { @@ -84,7 +98,7 @@ describe('useUpdateUserMutation', () => { await result.current.mutateAsync({ data: {}, id: 'user-1', permissions }); const config = { meta: { disableDefaultErrorNotification: true } }; expect(mocks.patch).toHaveBeenCalledWith('/v1/users/user-1', {}, config); - expect(mocks.put).toHaveBeenCalledWith('/v1/users/user-1/permissions', { permissions }, config); + expect(mocks.put).toHaveBeenCalledWith('/v1/users/user-1/permissions', permissions, config); }); it('should leave a rejection to its caller without escaping to the route error boundary', async () => { diff --git a/apps/web/src/hooks/useConfirmPermissionSetDeletion.ts b/apps/web/src/hooks/useConfirmPermissionSetDeletion.ts new file mode 100644 index 000000000..7d35aedfa --- /dev/null +++ b/apps/web/src/hooks/useConfirmPermissionSetDeletion.ts @@ -0,0 +1,58 @@ +import { useDestructiveAction, useTranslation } from '@douglasneuroinformatics/libui/hooks'; +import type { PermissionSetSummary } from '@opendatacapture/schemas/permission-set'; + +import { useDeletePermissionSetMutation } from './useDeletePermissionSetMutation'; + +type PermissionSetDeletionOptions = { + onDeleted?: () => void; +}; + +/** + * Deletes a permission set once the admin confirms, in a dialog stating how many users hold it: + * deleting a set takes it from every holder, which is otherwise visible nowhere on the page. + */ +export function useConfirmPermissionSetDeletion() { + const { t } = useTranslation(); + const confirm = useDestructiveAction(); + const deletePermissionSetMutation = useDeletePermissionSetMutation(); + + const describeHolders = (holderCount: number) => { + if (holderCount === 0) { + return t({ + en: 'No user holds this permission set, so deleting it changes no one’s permissions.', + es: 'Ningún usuario tiene este conjunto de permisos, por lo que eliminarlo no cambia los permisos de nadie.', + fr: "Aucun utilisateur ne détient cet ensemble d'autorisations : le supprimer ne change les autorisations de personne." + }); + } else if (holderCount === 1) { + return t({ + en: '1 user holds this permission set. Deleting it removes it from them; from their next sign-in they keep only their base permissions.', + es: '1 usuario tiene este conjunto de permisos. Eliminarlo se lo retira; a partir de su próximo inicio de sesión conservará solo sus permisos de base.', + fr: "1 utilisateur détient cet ensemble d'autorisations. Le supprimer le lui retire ; dès sa prochaine connexion, il ne conservera que ses autorisations de base." + }); + } + return t({ + en: `${holderCount} users hold this permission set. Deleting it removes it from them; from their next sign-in they keep only their base permissions.`, + es: `${holderCount} usuarios tienen este conjunto de permisos. Eliminarlo se lo retira; a partir de su próximo inicio de sesión conservarán solo sus permisos de base.`, + fr: `${holderCount} utilisateurs détiennent cet ensemble d'autorisations. Le supprimer le leur retire ; dès leur prochaine connexion, ils ne conserveront que leurs autorisations de base.` + }); + }; + + return ( + { holderCount, id, name }: Pick, + { onDeleted }: PermissionSetDeletionOptions = {} + ) => { + confirm( + () => { + deletePermissionSetMutation.mutate({ id }, { onSuccess: onDeleted }); + }, + { + description: describeHolders(holderCount), + title: t({ + en: `Delete the permission set "${name}"?`, + es: `¿Eliminar el conjunto de permisos «${name}»?`, + fr: `Supprimer l'ensemble d'autorisations « ${name} » ?` + }) + } + ); + }; +} diff --git a/apps/web/src/hooks/useCreatePermissionSetMutation.ts b/apps/web/src/hooks/useCreatePermissionSetMutation.ts new file mode 100644 index 000000000..21c6c08f7 --- /dev/null +++ b/apps/web/src/hooks/useCreatePermissionSetMutation.ts @@ -0,0 +1,23 @@ +import { useNotificationsStore } from '@douglasneuroinformatics/libui/hooks'; +import type { $CreatePermissionSetData } from '@opendatacapture/schemas/permission-set'; +import { useMutation, useQueryClient } from '@tanstack/react-query'; +import axios from 'axios'; + +import { PERMISSION_SETS_QUERY_KEY } from './usePermissionSetsQuery'; + +export function useCreatePermissionSetMutation() { + const queryClient = useQueryClient(); + const addNotification = useNotificationsStore((store) => store.addNotification); + return useMutation({ + mutationFn: ({ data }: { data: $CreatePermissionSetData }) => { + return axios.post('/v1/permission-sets', data, { meta: { disableDefaultErrorNotification: true } }); + }, + onSuccess() { + addNotification({ type: 'success' }); + void queryClient.invalidateQueries({ queryKey: [PERMISSION_SETS_QUERY_KEY] }); + }, + // The create page catches the rejection to report it on the form, which keeps what the admin + // entered -- a name already taken, most often -- so it must not also escape to the error boundary. + throwOnError: false + }); +} diff --git a/apps/web/src/hooks/useDeletePermissionSetMutation.ts b/apps/web/src/hooks/useDeletePermissionSetMutation.ts new file mode 100644 index 000000000..b36bb805a --- /dev/null +++ b/apps/web/src/hooks/useDeletePermissionSetMutation.ts @@ -0,0 +1,20 @@ +import { useNotificationsStore } from '@douglasneuroinformatics/libui/hooks'; +import { useMutation, useQueryClient } from '@tanstack/react-query'; +import axios from 'axios'; + +import { PERMISSION_SETS_QUERY_KEY } from './usePermissionSetsQuery'; +import { USERS_QUERY_KEY } from './useUsersQuery'; + +export function useDeletePermissionSetMutation() { + const queryClient = useQueryClient(); + const addNotification = useNotificationsStore((store) => store.addNotification); + return useMutation({ + mutationFn: ({ id }: { id: string }) => axios.delete(`/v1/permission-sets/${id}`), + onSuccess() { + addNotification({ type: 'success' }); + void queryClient.invalidateQueries({ queryKey: [PERMISSION_SETS_QUERY_KEY] }); + // Deleting a set takes it from every holder, so any cached user still naming it is stale. + void queryClient.invalidateQueries({ queryKey: [USERS_QUERY_KEY] }); + } + }); +} diff --git a/apps/web/src/hooks/useNavItems.ts b/apps/web/src/hooks/useNavItems.ts index 2b2da3062..9c8c80080 100644 --- a/apps/web/src/hooks/useNavItems.ts +++ b/apps/web/src/hooks/useNavItems.ts @@ -10,6 +10,7 @@ import { DatabaseIcon, EyeIcon, FileTextIcon, + KeyRoundIcon, LayersIcon, LogsIcon, MailIcon, @@ -184,6 +185,15 @@ export function useNavItems() { }), url: '/admin/users' }); + globalItems.push({ + icon: KeyRoundIcon, + label: t({ + en: 'Manage Permission Sets', + es: 'Gestionar conjuntos de permisos', + fr: "Gérer les ensembles d'autorisations" + }), + url: '/admin/permission-sets' + }); } const adminItems: NavItem[] = []; diff --git a/apps/web/src/hooks/usePermissionLabels.ts b/apps/web/src/hooks/usePermissionLabels.ts new file mode 100644 index 000000000..2ce7bffee --- /dev/null +++ b/apps/web/src/hooks/usePermissionLabels.ts @@ -0,0 +1,39 @@ +import { useTranslation } from '@douglasneuroinformatics/libui/hooks'; +import type { AppAction, AppSubjectName } from '@opendatacapture/schemas/core'; + +/** What each action and resource a permission can name is called in the interface language. */ +export function usePermissionLabels() { + const { t } = useTranslation(); + + const actionLabels: { [K in AppAction]: string } = { + create: t({ en: 'Create', es: 'Crear', fr: 'Créer' }), + delete: t({ en: 'Delete', es: 'Eliminar', fr: 'Supprimer' }), + manage: t({ en: 'Manage (All)', es: 'Gestionar (todo)', fr: 'Gérer (Tout)' }), + read: t({ en: 'Read', es: 'Leer', fr: 'Lire' }), + update: t({ en: 'Update', es: 'Actualizar', fr: 'Modifier' }) + }; + + const subjectLabels: { [K in AppSubjectName]: string } = { + all: t({ en: 'All', es: 'Todo', fr: 'Tous' }), + Assignment: t({ en: 'Assignment', es: 'Asignación', fr: 'Assignation' }), + Group: t({ en: 'Group', es: 'Grupo', fr: 'Groupe' }), + Instrument: t({ en: 'Instrument', es: 'Instrumento', fr: 'Instrument' }), + InstrumentRecord: t({ + en: 'Instrument Record', + es: 'Registro de instrumento', + fr: "Enregistrement de l'instrument" + }), + InstrumentRepo: t({ en: 'Instrument Repository', es: 'Repositorio de instrumentos', fr: "Dépôt d'instruments" }), + Session: t({ en: 'Session', es: 'Sesión', fr: 'Session' }), + Subject: t({ en: 'Subject', es: 'Sujeto', fr: 'Client' }), + User: t({ en: 'User', es: 'Usuario', fr: 'Utilisateur' }) + }; + + const columnLabels = { + action: t({ en: 'Action', es: 'Acción', fr: 'Action' }), + scope: t({ en: 'Scope', es: 'Alcance', fr: 'Portée' }), + subject: t({ en: 'Resource', es: 'Recurso', fr: 'Ressource' }) + }; + + return { actionLabels, columnLabels, subjectLabels }; +} diff --git a/apps/web/src/hooks/usePermissionSetQuery.ts b/apps/web/src/hooks/usePermissionSetQuery.ts new file mode 100644 index 000000000..4c9720c63 --- /dev/null +++ b/apps/web/src/hooks/usePermissionSetQuery.ts @@ -0,0 +1,20 @@ +import { $PermissionSetSummary } from '@opendatacapture/schemas/permission-set'; +import { queryOptions, useSuspenseQuery } from '@tanstack/react-query'; +import axios from 'axios'; + +import { PERMISSION_SETS_QUERY_KEY } from './usePermissionSetsQuery'; + +/** Keyed under the list's key, so whatever invalidates the list refreshes the set as well. */ +export const permissionSetQueryOptions = (id: string) => { + return queryOptions({ + queryFn: async () => { + const response = await axios.get(`/v1/permission-sets/${id}`); + return $PermissionSetSummary.parse(response.data); + }, + queryKey: [PERMISSION_SETS_QUERY_KEY, id] + }); +}; + +export function usePermissionSetQuery(id: string) { + return useSuspenseQuery(permissionSetQueryOptions(id)); +} diff --git a/apps/web/src/hooks/usePermissionSetsQuery.ts b/apps/web/src/hooks/usePermissionSetsQuery.ts new file mode 100644 index 000000000..b90e0ae83 --- /dev/null +++ b/apps/web/src/hooks/usePermissionSetsQuery.ts @@ -0,0 +1,19 @@ +import { $PermissionSetSummary } from '@opendatacapture/schemas/permission-set'; +import { queryOptions, useSuspenseQuery } from '@tanstack/react-query'; +import axios from 'axios'; + +export const PERMISSION_SETS_QUERY_KEY = 'permission-sets'; + +export const permissionSetsQueryOptions = () => { + return queryOptions({ + queryFn: async () => { + const response = await axios.get('/v1/permission-sets'); + return $PermissionSetSummary.array().parse(response.data); + }, + queryKey: [PERMISSION_SETS_QUERY_KEY] + }); +}; + +export function usePermissionSetsQuery() { + return useSuspenseQuery(permissionSetsQueryOptions()); +} diff --git a/apps/web/src/hooks/useUpdatePermissionSetMutation.ts b/apps/web/src/hooks/useUpdatePermissionSetMutation.ts new file mode 100644 index 000000000..5dc2fcd32 --- /dev/null +++ b/apps/web/src/hooks/useUpdatePermissionSetMutation.ts @@ -0,0 +1,22 @@ +import { useNotificationsStore } from '@douglasneuroinformatics/libui/hooks'; +import type { $UpdatePermissionSetData } from '@opendatacapture/schemas/permission-set'; +import { useMutation, useQueryClient } from '@tanstack/react-query'; +import axios from 'axios'; + +import { PERMISSION_SETS_QUERY_KEY } from './usePermissionSetsQuery'; + +export function useUpdatePermissionSetMutation() { + const queryClient = useQueryClient(); + const addNotification = useNotificationsStore((store) => store.addNotification); + return useMutation({ + mutationFn: ({ data, id }: { data: $UpdatePermissionSetData; id: string }) => { + return axios.patch(`/v1/permission-sets/${id}`, data, { meta: { disableDefaultErrorNotification: true } }); + }, + onSuccess() { + addNotification({ type: 'success' }); + void queryClient.invalidateQueries({ queryKey: [PERMISSION_SETS_QUERY_KEY] }); + }, + // The edit page catches the rejection to report it on the form, which keeps the admin's edits. + throwOnError: false + }); +} diff --git a/apps/web/src/hooks/useUpdateUserMutation.ts b/apps/web/src/hooks/useUpdateUserMutation.ts index 384dd2852..b76a7cba9 100644 --- a/apps/web/src/hooks/useUpdateUserMutation.ts +++ b/apps/web/src/hooks/useUpdateUserMutation.ts @@ -1,5 +1,4 @@ import { useNotificationsStore } from '@douglasneuroinformatics/libui/hooks'; -import type { Permissions } from '@opendatacapture/schemas/core'; import type { $UpdateUserData, $UpdateUserPermissionsData } from '@opendatacapture/schemas/user'; import { useMutation, useQueryClient } from '@tanstack/react-query'; import axios from 'axios'; @@ -10,11 +9,18 @@ export function useUpdateUserMutation() { const queryClient = useQueryClient(); const addNotification = useNotificationsStore((store) => store.addNotification); return useMutation({ - mutationFn: async ({ data, id, permissions }: { data: $UpdateUserData; id: string; permissions?: Permissions }) => { + mutationFn: async ({ + data, + id, + permissions + }: { + data: $UpdateUserData; + id: string; + permissions?: $UpdateUserPermissionsData; + }) => { await axios.patch(`/v1/users/${id}`, data, { meta: { disableDefaultErrorNotification: true } }); if (permissions !== undefined) { - const permissionsData: $UpdateUserPermissionsData = { permissions }; - await axios.put(`/v1/users/${id}/permissions`, permissionsData, { + await axios.put(`/v1/users/${id}/permissions`, permissions, { meta: { disableDefaultErrorNotification: true } }); } diff --git a/apps/web/src/route-tree.ts b/apps/web/src/route-tree.ts index f30e021b1..9adea1af1 100644 --- a/apps/web/src/route-tree.ts +++ b/apps/web/src/route-tree.ts @@ -36,6 +36,9 @@ import { Route as AppAdminBrandingLoginPageRouteImport } from './routes/_app/adm import { Route as AppAdminGroupsIndexRouteImport } from './routes/_app/admin/groups/index' import { Route as AppAdminGroupsCreateRouteImport } from './routes/_app/admin/groups/create' import { Route as AppAdminInstrumentReposIndexRouteImport } from './routes/_app/admin/instrument-repos/index' +import { Route as AppAdminPermissionSetsIndexRouteImport } from './routes/_app/admin/permission-sets/index' +import { Route as AppAdminPermissionSetsPermissionSetIdRouteImport } from './routes/_app/admin/permission-sets/$permissionSetId' +import { Route as AppAdminPermissionSetsCreateRouteImport } from './routes/_app/admin/permission-sets/create' import { Route as AppAdminUsersIndexRouteImport } from './routes/_app/admin/users/index' import { Route as AppAdminUsersUserIdRouteImport } from './routes/_app/admin/users/$userId' import { Route as AppAdminUsersCreateRouteImport } from './routes/_app/admin/users/create' @@ -191,6 +194,24 @@ const AppAdminInstrumentReposIndexRoute = path: '/admin/instrument-repos/', getParentRoute: () => AppRouteRoute, } as any) +const AppAdminPermissionSetsIndexRoute = + AppAdminPermissionSetsIndexRouteImport.update({ + id: '/admin/permission-sets/', + path: '/admin/permission-sets/', + getParentRoute: () => AppRouteRoute, + } as any) +const AppAdminPermissionSetsPermissionSetIdRoute = + AppAdminPermissionSetsPermissionSetIdRouteImport.update({ + id: '/admin/permission-sets/$permissionSetId', + path: '/admin/permission-sets/$permissionSetId', + getParentRoute: () => AppRouteRoute, + } as any) +const AppAdminPermissionSetsCreateRoute = + AppAdminPermissionSetsCreateRouteImport.update({ + id: '/admin/permission-sets/create', + path: '/admin/permission-sets/create', + getParentRoute: () => AppRouteRoute, + } as any) const AppAdminUsersIndexRoute = AppAdminUsersIndexRouteImport.update({ id: '/admin/users/', path: '/admin/users/', @@ -303,12 +324,15 @@ export interface FileRoutesByFullPath { '/admin/audit/logs': typeof AppAdminAuditLogsRoute '/admin/branding/login-page': typeof AppAdminBrandingLoginPageRoute '/admin/groups/create': typeof AppAdminGroupsCreateRoute + '/admin/permission-sets/$permissionSetId': typeof AppAdminPermissionSetsPermissionSetIdRoute + '/admin/permission-sets/create': typeof AppAdminPermissionSetsCreateRoute '/admin/users/$userId': typeof AppAdminUsersUserIdRoute '/admin/users/create': typeof AppAdminUsersCreateRoute '/instruments/render/$id': typeof AppInstrumentsRenderIdRoute '/admin/branding/': typeof AppAdminBrandingIndexRoute '/admin/groups/': typeof AppAdminGroupsIndexRoute '/admin/instrument-repos/': typeof AppAdminInstrumentReposIndexRoute + '/admin/permission-sets/': typeof AppAdminPermissionSetsIndexRoute '/admin/users/': typeof AppAdminUsersIndexRoute '/datahub/instruments/': typeof AppDatahubInstrumentsIndexRoute '/datahub/subjects/': typeof AppDatahubSubjectsIndexRoute @@ -345,12 +369,15 @@ export interface FileRoutesByTo { '/admin/audit/logs': typeof AppAdminAuditLogsRoute '/admin/branding/login-page': typeof AppAdminBrandingLoginPageRoute '/admin/groups/create': typeof AppAdminGroupsCreateRoute + '/admin/permission-sets/$permissionSetId': typeof AppAdminPermissionSetsPermissionSetIdRoute + '/admin/permission-sets/create': typeof AppAdminPermissionSetsCreateRoute '/admin/users/$userId': typeof AppAdminUsersUserIdRoute '/admin/users/create': typeof AppAdminUsersCreateRoute '/instruments/render/$id': typeof AppInstrumentsRenderIdRoute '/admin/branding': typeof AppAdminBrandingIndexRoute '/admin/groups': typeof AppAdminGroupsIndexRoute '/admin/instrument-repos': typeof AppAdminInstrumentReposIndexRoute + '/admin/permission-sets': typeof AppAdminPermissionSetsIndexRoute '/admin/users': typeof AppAdminUsersIndexRoute '/datahub/instruments': typeof AppDatahubInstrumentsIndexRoute '/datahub/subjects': typeof AppDatahubSubjectsIndexRoute @@ -390,12 +417,15 @@ export interface FileRoutesById { '/_app/admin/audit/logs': typeof AppAdminAuditLogsRoute '/_app/admin/branding/login-page': typeof AppAdminBrandingLoginPageRoute '/_app/admin/groups/create': typeof AppAdminGroupsCreateRoute + '/_app/admin/permission-sets/$permissionSetId': typeof AppAdminPermissionSetsPermissionSetIdRoute + '/_app/admin/permission-sets/create': typeof AppAdminPermissionSetsCreateRoute '/_app/admin/users/$userId': typeof AppAdminUsersUserIdRoute '/_app/admin/users/create': typeof AppAdminUsersCreateRoute '/_app/instruments/render/$id': typeof AppInstrumentsRenderIdRoute '/_app/admin/branding/': typeof AppAdminBrandingIndexRoute '/_app/admin/groups/': typeof AppAdminGroupsIndexRoute '/_app/admin/instrument-repos/': typeof AppAdminInstrumentReposIndexRoute + '/_app/admin/permission-sets/': typeof AppAdminPermissionSetsIndexRoute '/_app/admin/users/': typeof AppAdminUsersIndexRoute '/_app/datahub/instruments/': typeof AppDatahubInstrumentsIndexRoute '/_app/datahub/subjects/': typeof AppDatahubSubjectsIndexRoute @@ -435,12 +465,15 @@ export interface FileRouteTypes { | '/admin/audit/logs' | '/admin/branding/login-page' | '/admin/groups/create' + | '/admin/permission-sets/$permissionSetId' + | '/admin/permission-sets/create' | '/admin/users/$userId' | '/admin/users/create' | '/instruments/render/$id' | '/admin/branding/' | '/admin/groups/' | '/admin/instrument-repos/' + | '/admin/permission-sets/' | '/admin/users/' | '/datahub/instruments/' | '/datahub/subjects/' @@ -477,12 +510,15 @@ export interface FileRouteTypes { | '/admin/audit/logs' | '/admin/branding/login-page' | '/admin/groups/create' + | '/admin/permission-sets/$permissionSetId' + | '/admin/permission-sets/create' | '/admin/users/$userId' | '/admin/users/create' | '/instruments/render/$id' | '/admin/branding' | '/admin/groups' | '/admin/instrument-repos' + | '/admin/permission-sets' | '/admin/users' | '/datahub/instruments' | '/datahub/subjects' @@ -521,12 +557,15 @@ export interface FileRouteTypes { | '/_app/admin/audit/logs' | '/_app/admin/branding/login-page' | '/_app/admin/groups/create' + | '/_app/admin/permission-sets/$permissionSetId' + | '/_app/admin/permission-sets/create' | '/_app/admin/users/$userId' | '/_app/admin/users/create' | '/_app/instruments/render/$id' | '/_app/admin/branding/' | '/_app/admin/groups/' | '/_app/admin/instrument-repos/' + | '/_app/admin/permission-sets/' | '/_app/admin/users/' | '/_app/datahub/instruments/' | '/_app/datahub/subjects/' @@ -737,6 +776,27 @@ declare module '@tanstack/react-router' { preLoaderRoute: typeof AppAdminInstrumentReposIndexRouteImport parentRoute: typeof AppRouteRoute } + '/_app/admin/permission-sets/': { + id: '/_app/admin/permission-sets/' + path: '/admin/permission-sets' + fullPath: '/admin/permission-sets/' + preLoaderRoute: typeof AppAdminPermissionSetsIndexRouteImport + parentRoute: typeof AppRouteRoute + } + '/_app/admin/permission-sets/$permissionSetId': { + id: '/_app/admin/permission-sets/$permissionSetId' + path: '/admin/permission-sets/$permissionSetId' + fullPath: '/admin/permission-sets/$permissionSetId' + preLoaderRoute: typeof AppAdminPermissionSetsPermissionSetIdRouteImport + parentRoute: typeof AppRouteRoute + } + '/_app/admin/permission-sets/create': { + id: '/_app/admin/permission-sets/create' + path: '/admin/permission-sets/create' + fullPath: '/admin/permission-sets/create' + preLoaderRoute: typeof AppAdminPermissionSetsCreateRouteImport + parentRoute: typeof AppRouteRoute + } '/_app/admin/users/': { id: '/_app/admin/users/' path: '/admin/users' @@ -913,12 +973,15 @@ interface AppRouteRouteChildren { AppAdminAuditLogsRoute: typeof AppAdminAuditLogsRoute AppAdminBrandingLoginPageRoute: typeof AppAdminBrandingLoginPageRoute AppAdminGroupsCreateRoute: typeof AppAdminGroupsCreateRoute + AppAdminPermissionSetsPermissionSetIdRoute: typeof AppAdminPermissionSetsPermissionSetIdRoute + AppAdminPermissionSetsCreateRoute: typeof AppAdminPermissionSetsCreateRoute AppAdminUsersUserIdRoute: typeof AppAdminUsersUserIdRoute AppAdminUsersCreateRoute: typeof AppAdminUsersCreateRoute AppInstrumentsRenderIdRoute: typeof AppInstrumentsRenderIdRoute AppAdminBrandingIndexRoute: typeof AppAdminBrandingIndexRoute AppAdminGroupsIndexRoute: typeof AppAdminGroupsIndexRoute AppAdminInstrumentReposIndexRoute: typeof AppAdminInstrumentReposIndexRoute + AppAdminPermissionSetsIndexRoute: typeof AppAdminPermissionSetsIndexRoute AppAdminUsersIndexRoute: typeof AppAdminUsersIndexRoute AppDatahubInstrumentsIndexRoute: typeof AppDatahubInstrumentsIndexRoute AppDatahubSubjectsIndexRoute: typeof AppDatahubSubjectsIndexRoute @@ -950,12 +1013,16 @@ const AppRouteRouteChildren: AppRouteRouteChildren = { AppAdminAuditLogsRoute: AppAdminAuditLogsRoute, AppAdminBrandingLoginPageRoute: AppAdminBrandingLoginPageRoute, AppAdminGroupsCreateRoute: AppAdminGroupsCreateRoute, + AppAdminPermissionSetsPermissionSetIdRoute: + AppAdminPermissionSetsPermissionSetIdRoute, + AppAdminPermissionSetsCreateRoute: AppAdminPermissionSetsCreateRoute, AppAdminUsersUserIdRoute: AppAdminUsersUserIdRoute, AppAdminUsersCreateRoute: AppAdminUsersCreateRoute, AppInstrumentsRenderIdRoute: AppInstrumentsRenderIdRoute, AppAdminBrandingIndexRoute: AppAdminBrandingIndexRoute, AppAdminGroupsIndexRoute: AppAdminGroupsIndexRoute, AppAdminInstrumentReposIndexRoute: AppAdminInstrumentReposIndexRoute, + AppAdminPermissionSetsIndexRoute: AppAdminPermissionSetsIndexRoute, AppAdminUsersIndexRoute: AppAdminUsersIndexRoute, AppDatahubInstrumentsIndexRoute: AppDatahubInstrumentsIndexRoute, AppDatahubSubjectsIndexRoute: AppDatahubSubjectsIndexRoute, diff --git a/apps/web/src/routes/_app/admin/permission-sets/$permissionSetId.tsx b/apps/web/src/routes/_app/admin/permission-sets/$permissionSetId.tsx new file mode 100644 index 000000000..a7f69af3c --- /dev/null +++ b/apps/web/src/routes/_app/admin/permission-sets/$permissionSetId.tsx @@ -0,0 +1,87 @@ +import { Button, Heading } from '@douglasneuroinformatics/libui/components'; +import { useTranslation } from '@douglasneuroinformatics/libui/hooks'; +import { ChevronLeftIcon } from '@heroicons/react/24/solid'; +import { createFileRoute, Link, useNavigate } from '@tanstack/react-router'; + +import { PageHeader } from '@/components/PageHeader'; +import { PermissionSetForm } from '@/components/PermissionSetForm'; +import { useConfirmPermissionSetDeletion } from '@/hooks/useConfirmPermissionSetDeletion'; +import { permissionSetQueryOptions, usePermissionSetQuery } from '@/hooks/usePermissionSetQuery'; +import { useUpdatePermissionSetMutation } from '@/hooks/useUpdatePermissionSetMutation'; +import { getApiErrorMessage } from '@/utils/error'; + +const PermissionSetEditor = ({ permissionSetId }: { permissionSetId: string }) => { + const { t } = useTranslation(); + const navigate = useNavigate(); + const permissionSetQuery = usePermissionSetQuery(permissionSetId); + const updatePermissionSetMutation = useUpdatePermissionSetMutation(); + const confirmDeletion = useConfirmPermissionSetDeletion(); + const permissionSet = permissionSetQuery.data; + + return ( +
+ + + {t({ + en: 'Manage Permission Set', + es: 'Gestionar el conjunto de permisos', + fr: "Gérer l'ensemble d'autorisations" + })} + + +
+ + + {t({ en: 'Return', es: 'Volver', fr: 'Retour' })} + + confirmDeletion(permissionSet, { onDeleted: () => void navigate({ to: '..' }) })} + > + {t('core.delete')} + + } + submitLabel={t({ en: 'Save Changes', es: 'Guardar los cambios', fr: 'Enregistrer les modifications' })} + onSubmit={async (data) => { + try { + await updatePermissionSetMutation.mutateAsync({ data, id: permissionSet.id }); + } catch (err) { + const errorMessage = getApiErrorMessage( + err, + t({ + en: 'Failed to save permission set', + es: 'Error al guardar el conjunto de permisos', + fr: "Échec de l'enregistrement de l'ensemble d'autorisations" + }) + ); + return { errorMessage, success: false }; + } + return undefined; + }} + /> +
+
+ ); +}; + +const RouteComponent = () => { + const { permissionSetId } = Route.useParams(); + return ; +}; + +export const Route = createFileRoute('/_app/admin/permission-sets/$permissionSetId')({ + component: RouteComponent, + loader: async ({ context, params }) => { + await context.queryClient.ensureQueryData(permissionSetQueryOptions(params.permissionSetId)); + } +}); diff --git a/apps/web/src/routes/_app/admin/permission-sets/__tests__/$permissionSetId.test.tsx b/apps/web/src/routes/_app/admin/permission-sets/__tests__/$permissionSetId.test.tsx new file mode 100644 index 000000000..7debdd4de --- /dev/null +++ b/apps/web/src/routes/_app/admin/permission-sets/__tests__/$permissionSetId.test.tsx @@ -0,0 +1,191 @@ +import type { PropsWithChildren } from 'react'; + +import { CoreProvider } from '@douglasneuroinformatics/libui/providers'; +import type { PermissionSetSummary } from '@opendatacapture/schemas/permission-set'; +import { QueryClient } from '@tanstack/react-query'; +import { act, cleanup, fireEvent, render, screen, waitFor, within } from '@testing-library/react'; +import { AxiosError, AxiosHeaders } from 'axios'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { Route } from '@/routes/_app/admin/permission-sets/$permissionSetId'; + +import '@/services/i18n'; + +type MutateOptions = { onSuccess: () => void }; + +const mocks = vi.hoisted(() => ({ + deleteMutate: vi.fn<(variables: { id: string }, options: MutateOptions) => void>(), + navigate: vi.fn(), + permissionSet: null as unknown as PermissionSetSummary, + permissionSetQueryOptions: vi.fn((id: string) => ({ queryKey: ['permission-sets', id] })), + updateMutateAsync: vi.fn(), + usePermissionSetQuery: vi.fn() +})); + +vi.mock('@tanstack/react-router', async (importOriginal) => ({ + ...(await importOriginal()), + Link: ({ children, to, ...props }: PropsWithChildren<{ 'data-testid'?: string; to: string }>) => ( + + {children} + + ), + useNavigate: () => mocks.navigate +})); +vi.mock('@/hooks/usePermissionSetQuery', () => ({ + permissionSetQueryOptions: mocks.permissionSetQueryOptions, + usePermissionSetQuery: (id: string) => { + mocks.usePermissionSetQuery(id); + return { data: mocks.permissionSet }; + } +})); +vi.mock('@/hooks/useUpdatePermissionSetMutation', () => ({ + useUpdatePermissionSetMutation: () => ({ mutateAsync: mocks.updateMutateAsync }) +})); +vi.mock('@/hooks/useDeletePermissionSetMutation', () => ({ + useDeletePermissionSetMutation: () => ({ mutate: mocks.deleteMutate }) +})); + +const permissionSetFixture = (overrides: Partial = {}): PermissionSetSummary => ({ + createdAt: new Date('2026-01-01'), + holderCount: 3, + id: 'set-1', + name: 'Research Assistant', + rules: [{ action: 'read', scope: 'HOLDER_GROUPS', subject: 'Subject' }], + updatedAt: new Date('2026-01-01'), + ...overrides +}); + +const renderPage = () => { + const Component = Route.options.component!; + return render( + + + + ); +}; + +const field = (name: string) => document.querySelector(`[name="${name}"]`)!; + +const submit = () => fireEvent.click(screen.getByLabelText('Submit')); + +const runLoader = (queryClient: QueryClient) => { + const { loader } = Route.options; + if (typeof loader !== 'function') { + throw new Error('Expected the route to define its loader as a function'); + } + return loader({ + context: { queryClient }, + params: { permissionSetId: 'set-1' } + } as Parameters[0]); +}; + +describe('Route', () => { + beforeEach(() => { + vi.clearAllMocks(); + vi.spyOn(Route, 'useParams').mockReturnValue({ permissionSetId: 'set-1' }); + mocks.permissionSet = permissionSetFixture(); + mocks.updateMutateAsync.mockResolvedValue({ data: {} }); + }); + + afterEach(() => { + cleanup(); + vi.restoreAllMocks(); + }); + + it('should prefetch the set in the loader, so the form renders without a waterfall', async () => { + const queryClient = new QueryClient(); + const ensureQueryData = vi.spyOn(queryClient, 'ensureQueryData').mockResolvedValue(undefined); + await runLoader(queryClient); + expect(ensureQueryData).toHaveBeenCalledWith({ queryKey: ['permission-sets', 'set-1'] }); + }); + + it('should load the set named in the url and fill the form from it', () => { + renderPage(); + expect(mocks.usePermissionSetQuery).toHaveBeenCalledWith('set-1'); + expect(field('name').value).toBe('Research Assistant'); + expect(screen.getAllByTestId('permission-set-rule-row')).toHaveLength(1); + }); + + it('should link back to the list, so an admin can leave without saving or deleting', () => { + renderPage(); + expect(screen.getByTestId('admin-permission-set-back').getAttribute('href')).toBe('/admin/permission-sets'); + }); + + describe('saving', () => { + it('should save the edited name and rules against the set', async () => { + renderPage(); + fireEvent.change(field('name'), { target: { value: 'Renamed' } }); + submit(); + await waitFor(() => { + expect(mocks.updateMutateAsync).toHaveBeenCalledWith({ + data: { + name: 'Renamed', + rules: [{ action: 'read', scope: 'HOLDER_GROUPS', subject: 'Subject' }] + }, + id: 'set-1' + }); + }); + }); + + it('should say why a save failed and stay on the page', async () => { + mocks.updateMutateAsync.mockRejectedValue( + new AxiosError('Request failed with status code 409', 'ERR_BAD_REQUEST', undefined, undefined, { + config: { headers: new AxiosHeaders() }, + data: { message: "Permission set with name 'Coordinator' already exists!" }, + headers: {}, + status: 409, + statusText: 'Conflict' + }) + ); + renderPage(); + submit(); + expect(await screen.findByText("Permission set with name 'Coordinator' already exists!")).toBeTruthy(); + expect(mocks.navigate).not.toHaveBeenCalled(); + }); + + it('should fall back to a generic message when the failure carries no reason', async () => { + mocks.updateMutateAsync.mockRejectedValue(new Error('Network Error')); + renderPage(); + submit(); + expect(await screen.findByText('Failed to save permission set')).toBeTruthy(); + }); + }); + + describe('deleting', () => { + const confirmation = () => screen.getByTestId('destructive-action-dialog'); + + it('should offer delete beside save, so the page needs no separate delete section', () => { + renderPage(); + expect(screen.getByTestId('delete-permission-set').parentElement).toBe( + screen.getByLabelText('Submit').parentElement + ); + }); + + it('should state who holds the set in the confirmation alone, not on the page', () => { + renderPage(); + expect(document.body.textContent).not.toContain('3 users hold this permission set'); + fireEvent.click(screen.getByTestId('delete-permission-set')); + expect(confirmation().textContent).toContain('3 users hold this permission set'); + expect(mocks.deleteMutate).not.toHaveBeenCalled(); + // libui queues confirmations in a module-level store, so one left open would greet the next test. + fireEvent.click(within(confirmation()).getByRole('button', { name: 'No' })); + }); + + it('should delete the set once confirmed, then return to the list', async () => { + renderPage(); + fireEvent.click(screen.getByTestId('delete-permission-set')); + fireEvent.click(within(confirmation()).getByRole('button', { name: 'Yes' })); + await waitFor(() => expect(mocks.deleteMutate).toHaveBeenCalledWith({ id: 'set-1' }, expect.anything())); + expect(mocks.navigate).not.toHaveBeenCalled(); + act(() => mocks.deleteMutate.mock.lastCall![1].onSuccess()); + expect(mocks.navigate).toHaveBeenCalledWith({ to: '..' }); + }); + + it('should leave the set alone when the admin declines', () => { + renderPage(); + fireEvent.click(screen.getByTestId('delete-permission-set')); + fireEvent.click(within(confirmation()).getByRole('button', { name: 'No' })); + expect(mocks.deleteMutate).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/apps/web/src/routes/_app/admin/permission-sets/__tests__/create.test.tsx b/apps/web/src/routes/_app/admin/permission-sets/__tests__/create.test.tsx new file mode 100644 index 000000000..0b26a5f92 --- /dev/null +++ b/apps/web/src/routes/_app/admin/permission-sets/__tests__/create.test.tsx @@ -0,0 +1,89 @@ +import { cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { AxiosError, AxiosHeaders } from 'axios'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { Route } from '@/routes/_app/admin/permission-sets/create'; + +import '@/services/i18n'; + +const mocks = vi.hoisted(() => ({ + mutateAsync: vi.fn(), + navigate: vi.fn() +})); + +vi.mock('@tanstack/react-router', async (importOriginal) => ({ + ...(await importOriginal()), + useNavigate: () => mocks.navigate +})); +vi.mock('@/hooks/useCreatePermissionSetMutation', () => ({ + useCreatePermissionSetMutation: () => ({ mutateAsync: mocks.mutateAsync }) +})); + +const CreatePermissionSetPage = Route.options.component!; + +const field = (name: string) => document.querySelector(`[name="${name}"]`)!; + +const fillAndSubmit = (name: string) => { + render(); + fireEvent.change(field('name'), { target: { value: name } }); + fireEvent.keyDown(screen.getByTestId('rule-action-select-trigger'), { key: 'Enter' }); + fireEvent.click(screen.getByTestId('rule-action-select-item-read')); + fireEvent.keyDown(screen.getByTestId('rule-subject-select-trigger'), { key: 'Enter' }); + fireEvent.click(screen.getByTestId('rule-subject-select-item-Subject')); + fireEvent.click(screen.getByLabelText('Submit')); +}; + +const conflict = (message: string) => { + return new AxiosError('Request failed with status code 409', 'ERR_BAD_REQUEST', undefined, undefined, { + config: { headers: new AxiosHeaders() }, + data: { message }, + headers: {}, + status: 409, + statusText: 'Conflict' + }); +}; + +describe('Route', () => { + beforeEach(() => { + vi.clearAllMocks(); + mocks.mutateAsync.mockResolvedValue({ data: {} }); + }); + + afterEach(cleanup); + + it('should title the page as adding a permission set', () => { + render(); + expect(screen.getByRole('heading', { level: 2, name: 'Add Permission Set' })).toBeTruthy(); + }); + + it('should create the set with its name and rules', async () => { + fillAndSubmit('Research Assistant'); + await waitFor(() => { + expect(mocks.mutateAsync).toHaveBeenCalledWith({ + data: { + name: 'Research Assistant', + rules: [{ action: 'read', scope: 'HOLDER_GROUPS', subject: 'Subject' }] + } + }); + }); + }); + + it('should return to the list once the set is created', async () => { + fillAndSubmit('Research Assistant'); + await waitFor(() => expect(mocks.navigate).toHaveBeenCalledWith({ to: '..' })); + }); + + it('should stay on the form and say why when the name is taken', async () => { + mocks.mutateAsync.mockRejectedValue(conflict("Permission set with name 'Research Assistant' already exists!")); + fillAndSubmit('Research Assistant'); + expect(await screen.findByText("Permission set with name 'Research Assistant' already exists!")).toBeTruthy(); + expect(mocks.navigate).not.toHaveBeenCalled(); + expect(field('name').value).toBe('Research Assistant'); + }); + + it('should fall back to a generic message when the failure carries no reason', async () => { + mocks.mutateAsync.mockRejectedValue(new Error('Network Error')); + fillAndSubmit('Research Assistant'); + expect(await screen.findByText('Failed to create permission set')).toBeTruthy(); + }); +}); diff --git a/apps/web/src/routes/_app/admin/permission-sets/__tests__/index.test.tsx b/apps/web/src/routes/_app/admin/permission-sets/__tests__/index.test.tsx new file mode 100644 index 000000000..13380a53a --- /dev/null +++ b/apps/web/src/routes/_app/admin/permission-sets/__tests__/index.test.tsx @@ -0,0 +1,141 @@ +import type { PropsWithChildren } from 'react'; + +import { CoreProvider } from '@douglasneuroinformatics/libui/providers'; +import type { PermissionSetSummary } from '@opendatacapture/schemas/permission-set'; +import { QueryClient } from '@tanstack/react-query'; +import { cleanup, fireEvent, render, screen, within } from '@testing-library/react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { Route } from '@/routes/_app/admin/permission-sets/index'; + +import '@/services/i18n'; + +const mocks = vi.hoisted(() => ({ + deleteMutate: vi.fn(), + navigate: vi.fn(), + permissionSets: [] as PermissionSetSummary[], + permissionSetsQueryOptions: vi.fn(() => ({ queryKey: ['permission-sets'] })) +})); + +vi.mock('@tanstack/react-router', async (importOriginal) => ({ + ...(await importOriginal()), + Link: ({ children, to, ...props }: PropsWithChildren<{ 'data-testid'?: string; to: string }>) => ( + + {children} + + ), + useNavigate: () => mocks.navigate +})); +vi.mock('@/hooks/useDeletePermissionSetMutation', () => ({ + useDeletePermissionSetMutation: () => ({ mutate: mocks.deleteMutate }) +})); +vi.mock('@/hooks/usePermissionSetsQuery', () => ({ + permissionSetsQueryOptions: mocks.permissionSetsQueryOptions, + usePermissionSetsQuery: () => ({ data: mocks.permissionSets }) +})); + +const permissionSetFixture = (overrides: Partial = {}): PermissionSetSummary => ({ + createdAt: new Date('2026-01-01'), + holderCount: 3, + id: 'set-1', + name: 'Research Assistant', + rules: [ + { action: 'read', scope: 'HOLDER_GROUPS', subject: 'Subject' }, + { action: 'read', scope: 'ALL_GROUPS', subject: 'Instrument' } + ], + updatedAt: new Date('2026-01-01'), + ...overrides +}); + +const renderPage = (permissionSets: PermissionSetSummary[]) => { + mocks.permissionSets = permissionSets; + const Component = Route.options.component!; + render( + + + + ); +}; + +const rows = () => screen.getAllByTestId('data-table-row'); + +const cellTexts = (row: HTMLElement) => Array.from(row.children, (cell) => cell.textContent); + +const runLoader = (queryClient: QueryClient) => { + const { loader } = Route.options; + if (typeof loader !== 'function') { + throw new Error('Expected the route to define its loader as a function'); + } + return loader({ context: { queryClient } } as Parameters[0]); +}; + +describe('Route', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + afterEach(cleanup); + + it('should prefetch the permission sets in the loader, so the table renders without a waterfall', async () => { + const queryClient = new QueryClient(); + const ensureQueryData = vi.spyOn(queryClient, 'ensureQueryData').mockResolvedValue([]); + await runLoader(queryClient); + expect(ensureQueryData).toHaveBeenCalledWith({ queryKey: ['permission-sets'] }); + }); + + it('should list each set with how many rules it has and how many users hold it', () => { + renderPage([permissionSetFixture()]); + expect(cellTexts(rows()[0]!).slice(0, 3)).toEqual(['Research Assistant', '2', '3']); + }); + + it('should highlight a clicked row without leaving the list, as the other admin tables do', () => { + renderPage([permissionSetFixture()]); + fireEvent.click(rows()[0]!); + expect(rows()[0]!.querySelector('[data-row-selected="true"]')).toBeTruthy(); + expect(mocks.navigate).not.toHaveBeenCalled(); + }); + + it('should open a set when its row is double-clicked', () => { + renderPage([permissionSetFixture()]); + fireEvent.doubleClick(rows()[0]!); + expect(mocks.navigate).toHaveBeenCalledWith({ + params: { permissionSetId: 'set-1' }, + to: '/admin/permission-sets/$permissionSetId' + }); + }); + + describe('row actions', () => { + const openRowAction = (label: string) => { + fireEvent.keyDown(within(rows()[0]!).getByTestId('row-actions-trigger'), { key: 'Enter' }); + fireEvent.click(within(screen.getByTestId('row-actions-dropdown')).getByRole('menuitem', { name: label })); + }; + + it('should open a set from its manage action', () => { + renderPage([permissionSetFixture()]); + openRowAction('Manage'); + expect(mocks.navigate).toHaveBeenCalledWith({ + params: { permissionSetId: 'set-1' }, + to: '/admin/permission-sets/$permissionSetId' + }); + }); + + it('should confirm a delete from its row action, naming how many users hold the set', () => { + renderPage([permissionSetFixture()]); + openRowAction('Delete'); + const confirmation = screen.getByTestId('destructive-action-dialog'); + expect(confirmation.textContent).toContain('3 users hold this permission set'); + fireEvent.click(within(confirmation).getByRole('button', { name: 'Yes' })); + expect(mocks.deleteMutate).toHaveBeenCalledWith({ id: 'set-1' }, { onSuccess: undefined }); + }); + }); + + it('should link to adding a set', () => { + renderPage([]); + expect(screen.getByTestId('create-permission-set').getAttribute('href')).toBe('/admin/permission-sets/create'); + }); + + it('should explain what a permission set is while none exists', () => { + renderPage([]); + expect(screen.getByTestId('data-table-empty-state').textContent).toContain('No permission sets yet'); + }); +}); diff --git a/apps/web/src/routes/_app/admin/permission-sets/create.tsx b/apps/web/src/routes/_app/admin/permission-sets/create.tsx new file mode 100644 index 000000000..5a5463d0c --- /dev/null +++ b/apps/web/src/routes/_app/admin/permission-sets/create.tsx @@ -0,0 +1,52 @@ +import { Heading } from '@douglasneuroinformatics/libui/components'; +import { useTranslation } from '@douglasneuroinformatics/libui/hooks'; +import { createFileRoute, useNavigate } from '@tanstack/react-router'; + +import { PageHeader } from '@/components/PageHeader'; +import { PermissionSetForm } from '@/components/PermissionSetForm'; +import { useCreatePermissionSetMutation } from '@/hooks/useCreatePermissionSetMutation'; +import { getApiErrorMessage } from '@/utils/error'; + +const RouteComponent = () => { + const { t } = useTranslation(); + const navigate = useNavigate(); + const createPermissionSetMutation = useCreatePermissionSetMutation(); + + return ( +
+ + + {t({ + en: 'Add Permission Set', + es: 'Agregar un conjunto de permisos', + fr: "Ajouter un ensemble d'autorisations" + })} + + + { + try { + await createPermissionSetMutation.mutateAsync({ data }); + } catch (err) { + const errorMessage = getApiErrorMessage( + err, + t({ + en: 'Failed to create permission set', + es: 'Error al crear el conjunto de permisos', + fr: "Échec de la création de l'ensemble d'autorisations" + }) + ); + return { errorMessage, success: false }; + } + void navigate({ to: '..' }); + return undefined; + }} + /> +
+ ); +}; + +export const Route = createFileRoute('/_app/admin/permission-sets/create')({ + component: RouteComponent +}); diff --git a/apps/web/src/routes/_app/admin/permission-sets/index.tsx b/apps/web/src/routes/_app/admin/permission-sets/index.tsx new file mode 100644 index 000000000..71cdd598a --- /dev/null +++ b/apps/web/src/routes/_app/admin/permission-sets/index.tsx @@ -0,0 +1,111 @@ +import React, { useState } from 'react'; + +import { Button, DataTable, Heading } from '@douglasneuroinformatics/libui/components'; +import { useTranslation } from '@douglasneuroinformatics/libui/hooks'; +import type { PermissionSetSummary } from '@opendatacapture/schemas/permission-set'; +import { createFileRoute, Link, useNavigate } from '@tanstack/react-router'; + +import { PageHeader } from '@/components/PageHeader'; +import { useConfirmPermissionSetDeletion } from '@/hooks/useConfirmPermissionSetDeletion'; +import { permissionSetsQueryOptions, usePermissionSetsQuery } from '@/hooks/usePermissionSetsQuery'; + +const RouteComponent = () => { + const { t } = useTranslation(); + const navigate = useNavigate(); + const permissionSetsQuery = usePermissionSetsQuery(); + const confirmDeletion = useConfirmPermissionSetDeletion(); + const [highlightedRowId, setHighlightedRowId] = useState(null); + + const handleOpenPermissionSet = (permissionSet: PermissionSetSummary) => { + setHighlightedRowId(permissionSet.id); + void navigate({ params: { permissionSetId: permissionSet.id }, to: '/admin/permission-sets/$permissionSetId' }); + }; + + return ( + + + + {t({ + en: 'Manage Permission Sets', + es: 'Gestionar conjuntos de permisos', + fr: "Gérer les ensembles d'autorisations" + })} + + + { + const permissionSet = ctx.row.original; + return ( + + {permissionSet.name} + + + ); + }, + header: t({ en: 'Name', es: 'Nombre', fr: 'Nom' }) + }, + { + accessorFn: (permissionSet) => permissionSet.rules.length, + header: t({ en: 'Rules', es: 'Reglas', fr: 'Règles' }), + id: 'ruleCount' + }, + { + accessorKey: 'holderCount', + cell: ({ row }) => {row.original.holderCount}, + header: t({ en: 'Holders', es: 'Titulares', fr: 'Titulaires' }) + } + ]} + data={permissionSetsQuery.data} + data-testid="admin-permission-sets-table" + emptyStateProps={{ + description: t({ + en: 'A permission set bundles grants an administrator can then give users in one step.', + es: 'Un conjunto de permisos agrupa permisos que un administrador puede luego dar a los usuarios en un solo paso.', + fr: "Un ensemble d'autorisations regroupe des autorisations qu'un administrateur peut ensuite donner aux utilisateurs en une seule étape." + }), + title: t({ + en: 'No permission sets yet', + es: 'Todavía no hay conjuntos de permisos', + fr: "Aucun ensemble d'autorisations pour l'instant" + }) + }} + rowActions={[ + { + label: t('common.manage'), + onSelect: handleOpenPermissionSet + }, + { + label: t('core.delete'), + onSelect: (permissionSet) => confirmDeletion(permissionSet) + } + ]} + togglesComponent={() => ( + + )} + onRowClick={(permissionSet) => setHighlightedRowId(permissionSet.id)} + onRowDoubleClick={handleOpenPermissionSet} + /> + + ); +}; + +export const Route = createFileRoute('/_app/admin/permission-sets/')({ + component: RouteComponent, + loader: async ({ context }) => { + await context.queryClient.ensureQueryData(permissionSetsQueryOptions()); + } +}); diff --git a/apps/web/src/routes/_app/admin/users/$userId.tsx b/apps/web/src/routes/_app/admin/users/$userId.tsx index 69d4b35a3..e341785c4 100644 --- a/apps/web/src/routes/_app/admin/users/$userId.tsx +++ b/apps/web/src/routes/_app/admin/users/$userId.tsx @@ -6,6 +6,7 @@ import { useTranslation } from '@douglasneuroinformatics/libui/hooks'; import { ArchiveBoxArrowDownIcon, ArchiveBoxXMarkIcon, ChevronLeftIcon } from '@heroicons/react/24/solid'; import { isGrantablePermission } from '@opendatacapture/schemas/core'; import type { Permissions } from '@opendatacapture/schemas/core'; +import type { $UpdateUserPermissionsData } from '@opendatacapture/schemas/user'; import { createFileRoute, Link } from '@tanstack/react-router'; import { Chip } from '@/components/Chip'; @@ -14,9 +15,11 @@ import { UpdateUserForm } from '@/components/UpdateUserForm'; import type { UpdateUserFormInputData } from '@/components/UpdateUserForm'; import { UserIcon } from '@/components/UserIcon'; import { UserPermissionsEditor } from '@/components/UserPermissionsEditor'; +import type { PermissionSource } from '@/components/UserPermissionsEditor'; import { useArchiveUserMutation } from '@/hooks/useArchiveUserMutation'; import { useFindUserQuery, useFindUserQueryOptions } from '@/hooks/useFindUserQuery'; import { groupsQueryOptions, useGroupsQuery } from '@/hooks/useGroupsQuery'; +import { permissionSetsQueryOptions, usePermissionSetsQuery } from '@/hooks/usePermissionSetsQuery'; import { useUnarchiveUserMutation } from '@/hooks/useUnarchiveUserMutation'; import { useUpdateUserMutation } from '@/hooks/useUpdateUserMutation'; import { useAppStore } from '@/store'; @@ -28,6 +31,7 @@ const UserEditor = ({ userId }: { userId: string }) => { const currentUser = useAppStore((store) => store.currentUser); const { t } = useTranslation(); const groupsQuery = useGroupsQuery(); + const permissionSetsQuery = usePermissionSetsQuery(); const userQuery = useFindUserQuery(userId); const archiveUserMutation = useArchiveUserMutation(); const unarchiveUserMutation = useUnarchiveUserMutation(); @@ -42,6 +46,10 @@ const UserEditor = ({ userId }: { userId: string }) => { const user = userQuery.data; const [localPermissions, setLocalPermissions] = useState(user.additionalPermissions); + const [permissionSource, setPermissionSource] = useState( + user.permissionSetId ? 'permissionSet' : 'custom' + ); + const [permissionSetId, setPermissionSetId] = useState(user.permissionSetId ?? undefined); const [permissionDrafts, setPermissionDrafts] = useState([ { scope: user.groupIds.length === 1 ? user.groupIds[0] : undefined } @@ -209,7 +217,18 @@ const UserEditor = ({ userId }: { userId: string }) => { if (isSaving) { return undefined; } - if (permissionDrafts.some(isIncompleteDraft)) { + const isPermissionSetMissing = permissionSource === 'permissionSet' && permissionSetId === undefined; + if (user.basePermissionLevel !== 'ADMIN' && isPermissionSetMissing) { + setHighlightIncompleteDrafts(true); + const errorMessage = t({ + en: 'No permission set is chosen. Choose one, or switch to custom permissions.', + es: 'No se ha elegido ningún conjunto de permisos. Elija uno o cambie a permisos personalizados.', + fr: "Aucun ensemble d'autorisations n'est choisi. Choisissez-en un ou passez aux autorisations personnalisées." + }); + setSubmitErrorMessage(errorMessage); + return { errorMessage, success: false }; + } + if (permissionSource === 'custom' && permissionDrafts.some(isIncompleteDraft)) { setHighlightIncompleteDrafts(true); const errorMessage = t({ en: 'A permission row is incomplete. Choose its action, resource and scope, or remove it.', @@ -228,17 +247,27 @@ const UserEditor = ({ userId }: { userId: string }) => { groupIds: Array.from(groupIds), phoneNumber: omittedIfUnchanged(phoneNumber, user.phoneNumber) }; + const customPermissions = withPermissionDrafts(localPermissions, permissionDrafts) + .filter(isGrantablePermission) + .filter((permission) => permission.groupId === null || groupIds.has(permission.groupId)); + let permissions: $UpdateUserPermissionsData | undefined; + if (user.basePermissionLevel === 'ADMIN') { + permissions = undefined; + } else if (permissionSource === 'permissionSet' && permissionSetId !== undefined) { + permissions = { kind: 'permissionSet', permissionSetId }; + } else { + permissions = { kind: 'custom', permissions: customPermissions }; + } try { - const permissions = withPermissionDrafts(localPermissions, permissionDrafts) - .filter(isGrantablePermission) - .filter((permission) => permission.groupId === null || groupIds.has(permission.groupId)); - await updateUserMutation.mutateAsync({ - data: accountData, - id: user.id, - permissions: user.basePermissionLevel === 'ADMIN' ? undefined : permissions - }); + await updateUserMutation.mutateAsync({ data: accountData, id: user.id, permissions }); await userQuery.refetch(); - setLocalPermissions(permissions); + // Each kind of write empties the other on the server, so the editor follows suit. + if (permissions?.kind === 'custom') { + setLocalPermissions(permissions.permissions); + setPermissionSetId(undefined); + } else if (permissions?.kind === 'permissionSet') { + setLocalPermissions([]); + } setPermissionDrafts([{ scope: groupIds.size === 1 ? Array.from(groupIds)[0] : undefined }]); setSavedProfileCount((count) => count + 1); return undefined; @@ -263,9 +292,14 @@ const UserEditor = ({ userId }: { userId: string }) => { highlightIncomplete={highlightIncompleteDrafts} isSaving={isSaving} permissions={localPermissions} + permissionSetId={permissionSetId} + permissionSets={permissionSetsQuery.data} + source={permissionSource} user={user} onDraftsChange={setPermissionDrafts} onPermissionsChange={setLocalPermissions} + onPermissionSetIdChange={setPermissionSetId} + onSourceChange={setPermissionSource} /> @@ -169,7 +169,7 @@ export const PermissionSetRulesEditor = (props: PermissionSetRulesEditorProps) = size="icon" type="button" variant="ghost" - onClick={() => editor.onDraftsChange(editor.drafts.filter((_, i) => i !== index))} + onClick={() => editor.onDraftsChange(withoutIndex(editor.drafts, index))} > diff --git a/apps/web/src/components/UserPermissionsEditor/PermissionSetChoice.tsx b/apps/web/src/components/UserPermissionsEditor/PermissionSetChoice.tsx index ff0a661e3..c56c0916e 100644 --- a/apps/web/src/components/UserPermissionsEditor/PermissionSetChoice.tsx +++ b/apps/web/src/components/UserPermissionsEditor/PermissionSetChoice.tsx @@ -1,10 +1,11 @@ import { Label, Select } from '@douglasneuroinformatics/libui/components'; import { useTranslation } from '@douglasneuroinformatics/libui/hooks'; import type { PermissionSetSummary } from '@opendatacapture/schemas/permission-set'; -import { TriangleAlertIcon } from 'lucide-react'; import { PermissionSetRulesEditor } from '@/components/PermissionSetRulesEditor'; +import { WarningCallout } from './WarningCallout'; + type PermissionSetChoiceProps = { /** Whether the user holds saved custom grants, which assigning a set removes. */ discardsCustomGrants: boolean; @@ -84,20 +85,13 @@ export const PermissionSetChoice = ({ )} {discardsCustomGrants && ( -
- -

- {t({ - en: "This user holds custom grants. Saving with a permission set removes them, since a user's permissions come from one or the other.", - es: 'Este usuario tiene permisos personalizados. Guardar con un conjunto de permisos los elimina, ya que los permisos de un usuario provienen de uno u otro.', - fr: "Cet utilisateur détient des autorisations personnalisées. Enregistrer avec un ensemble d'autorisations les retire, puisque les autorisations d'un utilisateur proviennent de l'un ou de l'autre." - })} -

-
+ + {t({ + en: "This user holds custom grants. Saving with a permission set removes them, since a user's permissions come from one or the other.", + es: 'Este usuario tiene permisos personalizados. Guardar con un conjunto de permisos los elimina, ya que los permisos de un usuario provienen de uno u otro.', + fr: "Cet utilisateur détient des autorisations personnalisées. Enregistrer avec un ensemble d'autorisations les retire, puisque les autorisations d'un utilisateur proviennent de l'un ou de l'autre." + })} + )} ); diff --git a/apps/web/src/components/UserPermissionsEditor/UserPermissionsEditor.tsx b/apps/web/src/components/UserPermissionsEditor/UserPermissionsEditor.tsx index 3897640c0..83e4dc242 100644 --- a/apps/web/src/components/UserPermissionsEditor/UserPermissionsEditor.tsx +++ b/apps/web/src/components/UserPermissionsEditor/UserPermissionsEditor.tsx @@ -11,24 +11,19 @@ import type { Permissions } from '@opendatacapture/schemas/core'; import type { Group } from '@opendatacapture/schemas/group'; import type { PermissionSetSummary } from '@opendatacapture/schemas/permission-set'; import type { $UpdateUserPermissionsData, User } from '@opendatacapture/schemas/user'; -import { PlusIcon, Trash2Icon, TriangleAlertIcon } from 'lucide-react'; +import { PlusIcon, Trash2Icon } from 'lucide-react'; import { CellSelect } from '@/components/CellSelect'; import { Chip } from '@/components/Chip'; import { ColorTagCell } from '@/components/ColorTagCell'; import { usePermissionLabels } from '@/hooks/usePermissionLabels'; import { resolveTheme } from '@/utils/chart-theme'; -import { - ALL_GROUPS, - grantableActions, - grantableSubjects, - isIncompleteDraft, - withoutPermission -} from '@/utils/permissions'; +import { ALL_GROUPS, grantableActions, grantableSubjects, isIncompleteDraft, withoutIndex } from '@/utils/permissions'; import type { PermissionDraft } from '@/utils/permissions'; import { getAllGroupsScopeColor } from '@/utils/tag-colors'; import { PermissionSetChoice } from './PermissionSetChoice'; +import { WarningCallout } from './WarningCallout'; /** Where the user's permissions beyond their base level come from: a permission set, or their own grants. */ type PermissionSource = $UpdateUserPermissionsData['kind']; @@ -197,7 +192,7 @@ export const UserPermissionsEditor = ({ size="icon" type="button" variant="ghost" - onClick={() => onPermissionsChange(withoutPermission(permissions, index))} + onClick={() => onPermissionsChange(withoutIndex(permissions, index))} > @@ -267,7 +262,7 @@ export const UserPermissionsEditor = ({ size="icon" type="button" variant="ghost" - onClick={() => onDraftsChange(drafts.filter((_, draftIndex) => draftIndex !== index))} + onClick={() => onDraftsChange(withoutIndex(drafts, index))} > @@ -326,20 +321,13 @@ export const UserPermissionsEditor = ({

)} {isManageAll && ( -
- -

- {t({ - en: "Manage (All) makes this user an administrator: they can read and change every group's data, manage every user's account and permissions, and create instruments, which can run code on the server.", - es: 'Gestionar (todo) convierte a este usuario en administrador: puede consultar y modificar los datos de todos los grupos, gestionar la cuenta y los permisos de cada usuario, y crear instrumentos, los cuales pueden ejecutar código en el servidor.', - fr: 'Gérer (Tout) fait de cet utilisateur un administrateur, qui peut consulter et modifier les données de tous les groupes, gérer le compte et les autorisations de chaque utilisateur, et créer des instruments, lesquels peuvent exécuter du code sur le serveur.' - })} -

-
+ + {t({ + en: "Manage (All) makes this user an administrator: they can read and change every group's data, manage every user's account and permissions, and create instruments, which can run code on the server.", + es: 'Gestionar (todo) convierte a este usuario en administrador: puede consultar y modificar los datos de todos los grupos, gestionar la cuenta y los permisos de cada usuario, y crear instrumentos, los cuales pueden ejecutar código en el servidor.', + fr: 'Gérer (Tout) fait de cet utilisateur un administrateur, qui peut consulter et modifier les données de tous les groupes, gérer le compte et les autorisations de chaque utilisateur, et créer des instruments, lesquels peuvent exécuter du code sur le serveur.' + })} + )} )} diff --git a/apps/web/src/components/UserPermissionsEditor/WarningCallout.tsx b/apps/web/src/components/UserPermissionsEditor/WarningCallout.tsx new file mode 100644 index 000000000..745927e59 --- /dev/null +++ b/apps/web/src/components/UserPermissionsEditor/WarningCallout.tsx @@ -0,0 +1,25 @@ +import type React from 'react'; + +import { cn } from '@douglasneuroinformatics/libui/utils'; +import { TriangleAlertIcon } from 'lucide-react'; + +type WarningCalloutProps = { + children: React.ReactNode; + className?: string; + 'data-testid': string; +}; + +/** A warning about what saving the permissions as they stand will do, announced as soon as it appears. */ +export const WarningCallout = ({ children, className, 'data-testid': testId }: WarningCalloutProps) => ( +
+ +

{children}

+
+); diff --git a/apps/web/src/hooks/__tests__/useUpdateUserMutation.test.ts b/apps/web/src/hooks/__tests__/useUpdateUserMutation.test.ts index a1ae22b5c..8145ff071 100644 --- a/apps/web/src/hooks/__tests__/useUpdateUserMutation.test.ts +++ b/apps/web/src/hooks/__tests__/useUpdateUserMutation.test.ts @@ -16,7 +16,7 @@ vi.mock('@douglasneuroinformatics/libui/hooks', () => ({ } })); -const permissions: $UpdateUserPermissionsData = { +const permissionsUpdate: $UpdateUserPermissionsData = { kind: 'custom', permissions: [{ action: 'read', groupId: 'group-1', subject: 'Subject' }] }; @@ -40,9 +40,9 @@ describe('useUpdateUserMutation', () => { it('should show one confirmation after both account and permissions save', async () => { const { queryClient, result } = renderUpdateMutation(); const invalidate = vi.spyOn(queryClient, 'invalidateQueries'); - await result.current.mutateAsync({ data: { email: 'jane@example.org' }, id: 'user-1', permissions }); + await result.current.mutateAsync({ data: { email: 'jane@example.org' }, id: 'user-1', permissionsUpdate }); expect(mocks.patch).toHaveBeenCalledWith('/v1/users/user-1', { email: 'jane@example.org' }, expect.anything()); - expect(mocks.put).toHaveBeenCalledWith('/v1/users/user-1/permissions', permissions, expect.anything()); + expect(mocks.put).toHaveBeenCalledWith('/v1/users/user-1/permissions', permissionsUpdate, expect.anything()); expect(mocks.addNotification).toHaveBeenCalledExactlyOnceWith({ type: 'success' }); expect(invalidate).toHaveBeenCalledWith({ queryKey: ['users'] }); }); @@ -51,7 +51,7 @@ describe('useUpdateUserMutation', () => { const accountSave = Promise.withResolvers(); mocks.patch.mockReturnValueOnce(accountSave.promise); const { result } = renderUpdateMutation(); - const save = result.current.mutateAsync({ data: { groupIds: ['group-1'] }, id: 'user-1', permissions }); + const save = result.current.mutateAsync({ data: { groupIds: ['group-1'] }, id: 'user-1', permissionsUpdate }); await vi.waitFor(() => expect(mocks.patch).toHaveBeenCalled()); expect(mocks.put).not.toHaveBeenCalled(); expect(mocks.addNotification).not.toHaveBeenCalled(); @@ -69,7 +69,11 @@ describe('useUpdateUserMutation', () => { it('should clear permissions when the last row was removed', async () => { const { result } = renderUpdateMutation(); - await result.current.mutateAsync({ data: {}, id: 'user-1', permissions: { kind: 'custom', permissions: [] } }); + await result.current.mutateAsync({ + data: {}, + id: 'user-1', + permissionsUpdate: { kind: 'custom', permissions: [] } + }); expect(mocks.put).toHaveBeenCalledWith( '/v1/users/user-1/permissions', { kind: 'custom', permissions: [] }, @@ -80,14 +84,14 @@ describe('useUpdateUserMutation', () => { it('should send a permission set as it was given, so the server replaces any custom grants with it', async () => { const { result } = renderUpdateMutation(); const permissionSet: $UpdateUserPermissionsData = { kind: 'permissionSet', permissionSetId: 'set-1' }; - await result.current.mutateAsync({ data: {}, id: 'user-1', permissions: permissionSet }); + await result.current.mutateAsync({ data: {}, id: 'user-1', permissionsUpdate: permissionSet }); expect(mocks.put).toHaveBeenCalledWith('/v1/users/user-1/permissions', permissionSet, expect.anything()); }); it('should show no success confirmation when permissions fail to save', async () => { mocks.put.mockRejectedValueOnce(new Error('permission save failed')); const { result } = renderUpdateMutation(); - await expect(result.current.mutateAsync({ data: {}, id: 'user-1', permissions })).rejects.toThrow( + await expect(result.current.mutateAsync({ data: {}, id: 'user-1', permissionsUpdate })).rejects.toThrow( 'permission save failed' ); expect(mocks.addNotification).not.toHaveBeenCalled(); @@ -95,10 +99,10 @@ describe('useUpdateUserMutation', () => { it('should suppress the default error toast on both requests, since the caller shows its own message', async () => { const { result } = renderUpdateMutation(); - await result.current.mutateAsync({ data: {}, id: 'user-1', permissions }); + await result.current.mutateAsync({ data: {}, id: 'user-1', permissionsUpdate }); const config = { meta: { disableDefaultErrorNotification: true } }; expect(mocks.patch).toHaveBeenCalledWith('/v1/users/user-1', {}, config); - expect(mocks.put).toHaveBeenCalledWith('/v1/users/user-1/permissions', permissions, config); + expect(mocks.put).toHaveBeenCalledWith('/v1/users/user-1/permissions', permissionsUpdate, config); }); it('should leave a rejection to its caller without escaping to the route error boundary', async () => { diff --git a/apps/web/src/hooks/useUpdateUserMutation.ts b/apps/web/src/hooks/useUpdateUserMutation.ts index b76a7cba9..0dbd9025c 100644 --- a/apps/web/src/hooks/useUpdateUserMutation.ts +++ b/apps/web/src/hooks/useUpdateUserMutation.ts @@ -12,15 +12,15 @@ export function useUpdateUserMutation() { mutationFn: async ({ data, id, - permissions + permissionsUpdate }: { data: $UpdateUserData; id: string; - permissions?: $UpdateUserPermissionsData; + permissionsUpdate?: $UpdateUserPermissionsData; }) => { await axios.patch(`/v1/users/${id}`, data, { meta: { disableDefaultErrorNotification: true } }); - if (permissions !== undefined) { - await axios.put(`/v1/users/${id}/permissions`, permissions, { + if (permissionsUpdate !== undefined) { + await axios.put(`/v1/users/${id}/permissions`, permissionsUpdate, { meta: { disableDefaultErrorNotification: true } }); } diff --git a/apps/web/src/routes/_app/admin/permission-sets/__tests__/index.test.tsx b/apps/web/src/routes/_app/admin/permission-sets/__tests__/index.test.tsx index 13380a53a..ac409efb3 100644 --- a/apps/web/src/routes/_app/admin/permission-sets/__tests__/index.test.tsx +++ b/apps/web/src/routes/_app/admin/permission-sets/__tests__/index.test.tsx @@ -88,6 +88,12 @@ describe('Route', () => { expect(cellTexts(rows()[0]!).slice(0, 3)).toEqual(['Research Assistant', '2', '3']); }); + it('should clip a long name to one line with the full name on hover, so its row stays one line tall', () => { + const name = 'Research Assistant With A Name Long Enough To Outgrow Its Column'; + renderPage([permissionSetFixture({ name })]); + expect(within(rows()[0]!).getByTitle(name).className).toContain('whitespace-nowrap'); + }); + it('should highlight a clicked row without leaving the list, as the other admin tables do', () => { renderPage([permissionSetFixture()]); fireEvent.click(rows()[0]!); diff --git a/apps/web/src/routes/_app/admin/permission-sets/index.tsx b/apps/web/src/routes/_app/admin/permission-sets/index.tsx index 71cdd598a..b1781556f 100644 --- a/apps/web/src/routes/_app/admin/permission-sets/index.tsx +++ b/apps/web/src/routes/_app/admin/permission-sets/index.tsx @@ -6,6 +6,7 @@ import type { PermissionSetSummary } from '@opendatacapture/schemas/permission-s import { createFileRoute, Link, useNavigate } from '@tanstack/react-router'; import { PageHeader } from '@/components/PageHeader'; +import { TruncatedCell } from '@/components/TruncatedCell'; import { useConfirmPermissionSetDeletion } from '@/hooks/useConfirmPermissionSetDeletion'; import { permissionSetsQueryOptions, usePermissionSetsQuery } from '@/hooks/usePermissionSetsQuery'; @@ -39,8 +40,12 @@ const RouteComponent = () => { cell: (ctx) => { const permissionSet = ctx.row.original; return ( - - {permissionSet.name} + + { } ]} data={permissionSetsQuery.data} - data-testid="admin-permission-sets-table" emptyStateProps={{ description: t({ en: 'A permission set bundles grants an administrator can then give users in one step.', diff --git a/apps/web/src/routes/_app/admin/users/$userId.tsx b/apps/web/src/routes/_app/admin/users/$userId.tsx index e341785c4..6bf7ec8a5 100644 --- a/apps/web/src/routes/_app/admin/users/$userId.tsx +++ b/apps/web/src/routes/_app/admin/users/$userId.tsx @@ -250,22 +250,22 @@ const UserEditor = ({ userId }: { userId: string }) => { const customPermissions = withPermissionDrafts(localPermissions, permissionDrafts) .filter(isGrantablePermission) .filter((permission) => permission.groupId === null || groupIds.has(permission.groupId)); - let permissions: $UpdateUserPermissionsData | undefined; + let permissionsUpdate: $UpdateUserPermissionsData | undefined; if (user.basePermissionLevel === 'ADMIN') { - permissions = undefined; + permissionsUpdate = undefined; } else if (permissionSource === 'permissionSet' && permissionSetId !== undefined) { - permissions = { kind: 'permissionSet', permissionSetId }; + permissionsUpdate = { kind: 'permissionSet', permissionSetId }; } else { - permissions = { kind: 'custom', permissions: customPermissions }; + permissionsUpdate = { kind: 'custom', permissions: customPermissions }; } try { - await updateUserMutation.mutateAsync({ data: accountData, id: user.id, permissions }); + await updateUserMutation.mutateAsync({ data: accountData, id: user.id, permissionsUpdate }); await userQuery.refetch(); // Each kind of write empties the other on the server, so the editor follows suit. - if (permissions?.kind === 'custom') { - setLocalPermissions(permissions.permissions); + if (permissionsUpdate?.kind === 'custom') { + setLocalPermissions(permissionsUpdate.permissions); setPermissionSetId(undefined); - } else if (permissions?.kind === 'permissionSet') { + } else if (permissionsUpdate?.kind === 'permissionSet') { setLocalPermissions([]); } setPermissionDrafts([{ scope: groupIds.size === 1 ? Array.from(groupIds)[0] : undefined }]); diff --git a/apps/web/src/routes/_app/admin/users/__tests__/$userId.test.tsx b/apps/web/src/routes/_app/admin/users/__tests__/$userId.test.tsx index 77acdfd17..464fbf2c8 100644 --- a/apps/web/src/routes/_app/admin/users/__tests__/$userId.test.tsx +++ b/apps/web/src/routes/_app/admin/users/__tests__/$userId.test.tsx @@ -356,7 +356,7 @@ describe('Route', () => { act(() => editorProps().onPermissionSetIdChange('set-1')); await submit(); expect(mocks.mutateAsync.mock.lastCall?.[0]).toMatchObject({ - permissions: { kind: 'permissionSet', permissionSetId: 'set-1' } + permissionsUpdate: { kind: 'permissionSet', permissionSetId: 'set-1' } }); expect(editorProps().permissions).toEqual([]); }); @@ -383,7 +383,9 @@ describe('Route', () => { renderPage(); act(() => editorProps().onSourceChange('custom')); await submit(); - expect(mocks.mutateAsync.mock.lastCall?.[0]).toMatchObject({ permissions: { kind: 'custom', permissions: [] } }); + expect(mocks.mutateAsync.mock.lastCall?.[0]).toMatchObject({ + permissionsUpdate: { kind: 'custom', permissions: [] } + }); expect(editorProps().permissionSetId).toBeUndefined(); }); @@ -392,7 +394,7 @@ describe('Route', () => { renderPage(); act(() => editorProps().onSourceChange('permissionSet')); await submit(); - expect(mocks.mutateAsync.mock.lastCall?.[0]).toMatchObject({ permissions: undefined }); + expect(mocks.mutateAsync.mock.lastCall?.[0]).toMatchObject({ permissionsUpdate: undefined }); }); }); @@ -429,7 +431,7 @@ describe('Route', () => { phoneNumber: undefined }, id: 'user-1', - permissions: { kind: 'custom', permissions: [] } + permissionsUpdate: { kind: 'custom', permissions: [] } }); }); @@ -439,7 +441,7 @@ describe('Route', () => { act(() => editorProps().onDraftsChange([{ action: 'create', scope: 'group-1', subject: 'Subject' }])); await submit(); expect(mocks.mutateAsync.mock.lastCall?.[0]).toMatchObject({ - permissions: { + permissionsUpdate: { kind: 'custom', permissions: [ { action: 'read', groupId: 'group-1', subject: 'Subject' }, @@ -458,7 +460,7 @@ describe('Route', () => { renderPage(); await submit(); expect(mocks.mutateAsync.mock.lastCall?.[0]).toMatchObject({ - permissions: { kind: 'custom', permissions: [{ action: 'read', groupId: null, subject: 'Instrument' }] } + permissionsUpdate: { kind: 'custom', permissions: [{ action: 'read', groupId: null, subject: 'Instrument' }] } }); }); @@ -466,7 +468,7 @@ describe('Route', () => { mocks.state.user.basePermissionLevel = 'ADMIN'; renderPage(); await submit(); - expect(mocks.mutateAsync.mock.lastCall?.[0]).toMatchObject({ permissions: undefined }); + expect(mocks.mutateAsync.mock.lastCall?.[0]).toMatchObject({ permissionsUpdate: undefined }); }); it('should reload the user and report success once the save lands', async () => { diff --git a/apps/web/src/utils/__tests__/permissions.test.ts b/apps/web/src/utils/__tests__/permissions.test.ts index 8e2b308c4..9303673c8 100644 --- a/apps/web/src/utils/__tests__/permissions.test.ts +++ b/apps/web/src/utils/__tests__/permissions.test.ts @@ -9,7 +9,7 @@ import { grantableSubjects, isIncompleteDraft, toUserPermission, - withoutPermission, + withoutIndex, withPermission, withPermissionDrafts } from '../permissions'; @@ -100,13 +100,13 @@ describe('withPermission', () => { }); }); -describe('withoutPermission', () => { - it('should remove only the grant at the given index', () => { +describe('withoutIndex', () => { + it('should remove only the row at the given index', () => { const permissions: Permissions = [ { action: 'read', groupId: 'group-1', subject: 'Subject' }, { action: 'read', groupId: null, subject: 'Subject' } ]; - expect(withoutPermission(permissions, 0)).toEqual([permissions[1]]); + expect(withoutIndex(permissions, 0)).toEqual([permissions[1]]); }); }); diff --git a/apps/web/src/utils/permissions.ts b/apps/web/src/utils/permissions.ts index c13c092f1..e325ca991 100644 --- a/apps/web/src/utils/permissions.ts +++ b/apps/web/src/utils/permissions.ts @@ -89,8 +89,8 @@ const withPermissionDrafts = (permissions: Permissions, drafts: PermissionDraft[ }, permissions); }; -const withoutPermission = (permissions: TPermission[], index: number): TPermission[] => { - return permissions.filter((_, i) => i !== index); +const withoutIndex = (items: TItem[], index: number): TItem[] => { + return items.filter((_, itemIndex) => itemIndex !== index); }; export { @@ -100,7 +100,7 @@ export { grantableSubjects, isIncompleteDraft, toUserPermission, - withoutPermission, + withoutIndex, withPermission, withPermissionDrafts }; diff --git a/packages/schemas/AGENTS.md b/packages/schemas/AGENTS.md index a19950165..b52c6624c 100644 --- a/packages/schemas/AGENTS.md +++ b/packages/schemas/AGENTS.md @@ -94,7 +94,7 @@ shape. Two constraints on anything you write there: publish JSON Schema for every kind × language. This is why `$InstrumentValidationSchema` is `z.any().refine(...)` rather than `z.custom()`, which cannot be converted. -## Three security-relevant exports +## Four security-relevant exports `$AppSubjectName` in `src/core/core.ts` is the hand-maintained list of CASL subject names. It is not derived from `schema.prisma` and does not mirror it. A model that needs permission checks has to be @@ -110,6 +110,11 @@ newly scopable subject's group field is named. applied by `$UpdateUserPermissionsData` and not by `$UserPermission`, which must still parse grants stored before it — `.agents/docs/architecture/auth-and-permissions.md` says why. +`isPermissionSetGrantable`, in `src/permission-set/permission-set.ts`, is the same rule for a +permission set, and also refuses `manage all`: one edit to a set would otherwise make every holder an +administrator. `$CreatePermissionSetData` and `$UpdatePermissionSetData` apply it, and +`$PermissionSetRule`, the read model, does not, for the same reason. + ## Tests `pnpm exec vitest --project schemas`. There is a `vitest.config.ts`; no setup files, no environment diff --git a/packages/schemas/src/permission-set/permission-set.ts b/packages/schemas/src/permission-set/permission-set.ts index 04233f79e..afc982ed0 100644 --- a/packages/schemas/src/permission-set/permission-set.ts +++ b/packages/schemas/src/permission-set/permission-set.ts @@ -82,8 +82,7 @@ const $PermissionSetSummary = $PermissionSet.extend({ holderCount: z.number().int().nonnegative() }); -const $CreatePermissionSetData = z.object({ - name: z.string().min(1), +const $CreatePermissionSetData = $PermissionSet.pick({ name: true }).extend({ rules: $GrantablePermissionSetRules }); diff --git a/testing/src/pages/_app/admin/permission-sets/index.page.ts b/testing/src/pages/_app/admin/permission-sets/index.page.ts index dbc191e6e..94a442dff 100644 --- a/testing/src/pages/_app/admin/permission-sets/index.page.ts +++ b/testing/src/pages/_app/admin/permission-sets/index.page.ts @@ -9,7 +9,7 @@ export class PermissionSetsPage extends AppPage { constructor(page: Page) { super(page); - // libui's DataTable sets its own testid and ignores the one the route passes it. + // libui's DataTable sets its own testid and takes none from the route. this.table = page.getByTestId('data-table'); this.createLink = page.getByTestId('create-permission-set'); this.destructiveActionDialog = page.getByTestId('destructive-action-dialog'); @@ -21,6 +21,10 @@ export class PermissionSetsPage extends AppPage { await this.$ref.getByTestId('row-actions-dropdown').getByRole('menuitem', { name: action }).click(); } + nameCell(name: string): Locator { + return this.row(name).getByTestId('permission-set-name'); + } + row(name: string): Locator { return this.table.getByTestId('data-table-row').filter({ hasText: name }); } diff --git a/testing/src/specs/permission-sets.spec.ts b/testing/src/specs/permission-sets.spec.ts index 16b886f7a..d1586a81a 100644 --- a/testing/src/specs/permission-sets.spec.ts +++ b/testing/src/specs/permission-sets.spec.ts @@ -1,6 +1,4 @@ -import type { Group } from '@opendatacapture/schemas/group'; import type { PermissionSetRule } from '@opendatacapture/schemas/permission-set'; -import type { User } from '@opendatacapture/schemas/user'; import type { APIRequestContext } from '@playwright/test'; import { PermissionSetPage } from '../pages/_app/admin/permission-sets/$permissionSetId.page'; @@ -23,12 +21,12 @@ const RULES: PermissionSetRule[] = [ /** The users and groups a holder can read, with a token minted now, since permissions are frozen into it at login. */ async function readAccess(request: APIRequestContext, credentials: { password: string; username: string }) { const token = await ApiClient.login(request, credentials); - const read = async (path: string) => { + const readIds = async (path: 'groups' | 'users') => { const response = await request.get(`${API}/${path}`, { headers: { Authorization: `Bearer ${token}` } }); expect(response.ok()).toBe(true); - return ((await response.json()) as T[]).map(({ id }) => id); + return ((await response.json()) as { id: string }[]).map(({ id }) => id); }; - return { groupIds: await read('groups'), userIds: await read('users') }; + return { groupIds: await readIds('groups'), userIds: await readIds('users') }; } test.describe('permission sets', () => { @@ -182,6 +180,26 @@ test.describe('permission sets', () => { expect((await api.findUserById(user.id)).permissionSetId).toBeNull(); }); + test('should clip a long permission set name to one line and name it in full on hover', async ({ + api, + getPageModel, + uniqueId + }) => { + const name = `Assistants de recherche cliniques des laboratoires partenaires du programme ${uniqueId}`; + await api.createPermissionSet(RULES, { name }); + + const listPage = await getPageModel('/admin/permission-sets'); + const cell = listPage.nameCell(name); + + await expect(cell).toHaveAttribute('title', name); + const layout = await cell.evaluate((element) => ({ + clipped: element.scrollWidth > element.clientWidth, + lines: Math.round(element.getBoundingClientRect().height / parseFloat(getComputedStyle(element).lineHeight)) + })); + expect(layout.clipped).toBe(true); + expect(layout.lines).toBe(1); + }); + test('should offer no permission set for an administrator', async ({ api, authenticateAs, getPageModel, page }) => { await api.createPermissionSet(RULES);