From 680c172e539361c79f322c22ff85c901a6796f05 Mon Sep 17 00:00:00 2001 From: mindsers Date: Tue, 25 Aug 2026 03:43:56 +0200 Subject: [PATCH 1/2] =?UTF-8?q?chore(auth):=20drop=20direct=20user-permiss?= =?UTF-8?q?ion=20grants=20=E2=80=94=20roles=20only?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the role-based permission epic (#142, phase 7 / #149). A permission reached a UserAccount three ways: a direct CongregationUserPermission row, a role on the account, or a role on its linked Member. The first path is now gone, so the settings role screens are the single place access is granted. Migration 20260826000000 turns every direct grant into a role assignment, then drops the table. Each (congregation, permission) pair that had at least one grant gets one auto-role granting exactly that permission, keyed can-- per AUTO_ROLE_KEY_BY_PERMISSION. They are ordinary custom roles afterwards — renameable, deletable — with a null name so the label resolves from the message catalogue rather than pinning a language into the database. A congregation that already owned a role under one of those keys keeps it untouched; the auto-role lands on -migrated instead. Adopting the existing role would have silently widened it for everyone assigned to it. Behaviour preservation is pinned by an integration test that captures each account's effective permission set before the backfill and asserts it is identical after, across an admin held only directly, a user holding the same permission both directly and via a role, a member-bound role that must not be touched, an account with nothing, and the collision case. Also: - resolveEffectivePermissions and accountsWithPermissionFilter walk roles only; the three-path comment is now two. - setupFirstAccount and registerCongregation hand the first account admin through ensureAdminRole, inside the scoped transaction — the role tables are RLS-scoped, unlike the direct grant they replace. - updateAccount edits identity only. Its last-admin pre-check goes with the permission arm; setUserCustomRoleAssignments already guards role removal. - The archive importer keeps reading congregation-user-permissions.ndjson (and the pre-#152 congregation-user-roles.ndjson) and lands each grant on the matching auto-role, so older backups still restore everyone's access. Exports no longer emit the file. - The GDPR account export reports role-mediated permissions; anonymize drops the role assignments, which is what now revokes access. - The "Autorisations supplémentaires" card and the direct-permission count column are removed from the user screens. The control plane was migrated off this table first (Unitae/unitae-platform), since its CD applies this migration before rolling its own image. --- CLAUDE.md | 2 +- .../migration.sql | 131 +++++++ ...irect-user-permissions.integration.test.ts | 319 ++++++++++++++++++ app/database/schema.prisma | 31 +- app/database/seed-marketing.ts | 63 ++-- .../register-congregation.server.test.ts | 20 +- .../server/register-congregation.server.ts | 25 +- .../server/setup-first-account.server.test.ts | 24 +- .../server/setup-first-account.server.ts | 24 +- .../congregation/routes/roles/edit-role.tsx | 2 +- .../settings/routes/users/edit-user.tsx | 37 +- .../settings/routes/users/user-list.tsx | 14 - app/features/settings/schemas/user.schema.ts | 5 - .../server/anonymize-account.server.test.ts | 9 +- .../server/anonymize-account.server.ts | 5 +- .../server/anonymize.integration.test.ts | 22 +- .../server/data-transfer.integration.test.ts | 219 ++++++++++-- .../server/export-account-data.server.test.ts | 5 +- .../server/export-account-data.server.ts | 18 +- .../server/export-congregation.server.ts | 16 +- .../server/import-user-accounts.server.ts | 74 +++- .../update-account.server.integration.test.ts | 77 ++--- .../server/update-account.server.test.ts | 48 +-- .../settings/server/update-account.server.ts | 36 +- app/i18n/messages/en.json | 29 +- app/i18n/messages/fr.json | 29 +- .../permissions.server.integration.test.ts | 33 +- app/shared/auth/permissions.server.test.ts | 31 +- app/shared/auth/permissions.server.ts | 32 +- app/shared/domain/audit.server.ts | 4 + .../domain/roles.server.integration.test.ts | 1 - app/shared/domain/setup.server.ts | 40 +++ app/shared/types/permission.ts | 44 +++ app/shared/types/role.ts | 43 ++- docs/development/architecture.md | 10 +- docs/development/data-transfer.md | 4 +- docs/development/notifications.md | 2 +- docs/development/permissions-and-roles.md | 35 +- docs/development/row-level-security.md | 2 +- scripts/check-tenant-scoping.ts | 1 - 40 files changed, 1183 insertions(+), 383 deletions(-) create mode 100644 app/database/migrations/20260826000000_drop_direct_user_permissions/migration.sql create mode 100644 app/database/migrations/drop-direct-user-permissions.integration.test.ts diff --git a/CLAUDE.md b/CLAUDE.md index bbae4d90..dbcd2368 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -279,7 +279,7 @@ const permissions = context.get(permissionsContext) if (!permissions.has(Permission.TerritoriesManager)) throw redirect('/dashboard') ``` -**Auth model:** `Permission` (24 entries, in `app/shared/types/permission.ts`) is the unit of access. **Roles** (DB table) bundle permissions and are assigned to users — built-in roles plus custom roles a Roles Manager creates. `requireAuth()` runs `resolveEffectivePermissions` and stores the user's full granted set in `permissionsContext`; the legacy `_required` parameter is retained for call-site compatibility but no longer filters anything. See `docs/development/permissions-and-roles.md`. +**Auth model:** `Permission` (24 entries, in `app/shared/types/permission.ts`) is the unit of access. **Roles** (DB table) bundle permissions and are assigned to users — built-in roles plus custom roles a Roles Manager creates. A role is the *only* way a permission reaches an account: the direct `CongregationUserPermission` edge was migrated into auto-roles and dropped in #149, so never reintroduce a user→permission write. `requireAuth()` runs `resolveEffectivePermissions` and stores the user's full granted set in `permissionsContext`; the legacy `_required` parameter is retained for call-site compatibility but no longer filters anything. See `docs/development/permissions-and-roles.md`. **Two-factor (TOTP):** opt-in per user. `UserAccount.twoFactorSecret` stores the AES-256-GCM ciphertext of the base32 seed (`null` = never enrolled) and `twoFactorEnabledAt` stays `null` while enrollment is pending. An enrolled user's login lands on `authentication/routes/two-factor-challenge.tsx` before the session is issued — anything that changes the login path must keep that hop intact. Server side: `totp.server.ts`, `start-two-factor-enrollment.server.ts`, `verify-two-factor-challenge.server.ts`. diff --git a/app/database/migrations/20260826000000_drop_direct_user_permissions/migration.sql b/app/database/migrations/20260826000000_drop_direct_user_permissions/migration.sql new file mode 100644 index 00000000..bcb996f1 --- /dev/null +++ b/app/database/migrations/20260826000000_drop_direct_user_permissions/migration.sql @@ -0,0 +1,131 @@ +-- Completes the "replace" half of the role-based permission epic (#142, phase 7 / #149). +-- +-- Until now a permission could reach a UserAccount three ways: a direct +-- CongregationUserPermission row, a role on the account, or a role on its linked +-- Member. This migration removes the first path by turning every direct grant +-- into a role assignment, then dropping the table. After it, resolveEffectivePermissions +-- walks roles only, and the settings screens are the single place a permission is granted. +-- +-- The contract is *no change in who can do what*. Each (congregation, permission) +-- pair that has at least one direct grant gets one auto-role granting exactly that +-- permission, and each grant becomes a UserRoleAssignment to it. +-- +-- Auto-roles are ordinary custom roles (isBuiltIn = false), so admins can rename, +-- re-scope or delete them afterwards. Their name and description stay NULL: that is +-- the convention built-in Role rows already use, and it means no French or English +-- text is pinned into the database here — getRoleDisplayName resolves the label from +-- the message catalogue for the reader's locale, and an admin who renames one stores +-- their own name, which then wins. +-- +-- Every INSERT is ON CONFLICT DO NOTHING so the file is safe to re-run, and safe for +-- a user who already holds the same permission through a role. +-- +-- RLS note: Role, RolePermission, UserRoleAssignment and AuditLog all FORCE row level +-- security, but every policy short-circuits to `true` when app.congregation_id is unset +-- (the CASE WHEN NULLIF(...) IS NULL THEN true form). A migration runs with it unset, +-- so these cross-tenant writes are permitted. + +-- The permission -> auto-role key mapping is materialized into a temp table rather than +-- recomputed per statement. The collision CASE below reads "Role", so re-evaluating it +-- after the roles are inserted would return a different key and the later joins would +-- miss. Resolve once, reuse three times. +CREATE TEMP TABLE "_direct_grant_role" ( + "congregationId" INTEGER NOT NULL, + "permissionId" INTEGER NOT NULL, + "roleKey" TEXT NOT NULL, + PRIMARY KEY ("congregationId", "permissionId") +); + +-- A congregation may already own a custom role slugified to one of these keys — an +-- admin is free to create a role called "Peut tout faire". Reusing it would silently +-- grant that permission to everyone already assigned to it, so a taken key falls back +-- to "-migrated", and then to "-migrated-". +INSERT INTO "_direct_grant_role" ("congregationId", "permissionId", "roleKey") +SELECT n.cid, + n.pid, + CASE + WHEN NOT EXISTS (SELECT 1 FROM "Role" r WHERE r."congregationId" = n.cid AND r."key" = n.base) + THEN n.base + WHEN NOT EXISTS (SELECT 1 FROM "Role" r WHERE r."congregationId" = n.cid AND r."key" = n.base || '-migrated') + THEN n.base || '-migrated' + ELSE n.base || '-migrated-' || n.pid::text + END +FROM ( + SELECT DISTINCT cup."congregationId" AS cid, cup."permissionId" AS pid, m.role_key AS base + FROM "CongregationUserPermission" cup + JOIN "Permission" p ON p."id" = cup."permissionId" + JOIN (VALUES + ('admin', 'can-do-anything'), + ('board-viewer', 'can-view-board-documents'), + ('board-uploader', 'can-upload-board-documents'), + ('board-validator', 'can-validate-board-documents'), + ('territories-viewer', 'can-view-territories'), + ('territories-manager', 'can-edit-territories'), + ('prospection-viewer', 'can-view-prospection'), + ('prospection-manager', 'can-edit-prospection'), + ('publisher-viewer', 'can-view-publishers'), + ('publisher-manager', 'can-edit-publishers'), + ('emergency-info-viewer', 'can-view-emergency-info'), + ('emergency-info-manager', 'can-edit-emergency-info'), + ('activity-viewer', 'can-view-activities'), + ('activity-manager', 'can-edit-activities'), + ('pioneer-goal-manager', 'can-manage-pioneer-goals'), + ('program-viewer', 'can-view-programs'), + ('program-manager', 'can-edit-programs'), + ('absence-viewer', 'can-view-absences'), + ('external-speaker-viewer', 'can-view-external-speakers'), + ('external-speaker-manager', 'can-edit-external-speakers'), + ('settings-user-manager', 'can-manage-users'), + ('roles-viewer', 'can-view-roles'), + ('roles-manager', 'can-manage-roles'), + ('permissions-manager', 'can-manage-permissions') + ) AS m(permission_key, role_key) ON m.permission_key = p."key" +) n; + +-- 1. One auto-role per (congregation, permission) that anybody holds directly. +INSERT INTO "Role" ("key", "name", "description", "isBuiltIn", "congregationId", "createdAt", "updatedAt") +SELECT m."roleKey", NULL, NULL, false, m."congregationId", CURRENT_TIMESTAMP, CURRENT_TIMESTAMP +FROM "_direct_grant_role" m +ON CONFLICT ("key", "congregationId") DO NOTHING; + +-- 2. Each auto-role grants exactly the one permission it was created for. +INSERT INTO "RolePermission" ("roleId", "permissionId", "congregationId") +SELECT r."id", m."permissionId", m."congregationId" +FROM "_direct_grant_role" m +JOIN "Role" r ON r."congregationId" = m."congregationId" AND r."key" = m."roleKey" +ON CONFLICT ("roleId", "permissionId") DO NOTHING; + +-- 3. Every direct grant becomes a membership in the matching auto-role. A user who +-- already reached the permission through some other role just gains a second, +-- redundant path — the union is unchanged, which is the point. +INSERT INTO "UserRoleAssignment" ("userId", "roleId", "congregationId") +SELECT cup."userId", r."id", cup."congregationId" +FROM "CongregationUserPermission" cup +JOIN "_direct_grant_role" m + ON m."congregationId" = cup."congregationId" AND m."permissionId" = cup."permissionId" +JOIN "Role" r ON r."congregationId" = m."congregationId" AND r."key" = m."roleKey" +ON CONFLICT ("userId", "roleId") DO NOTHING; + +-- 4. One bulk audit event per affected congregation, so an admin who later wonders +-- where these roles came from can find the answer in the audit log. actorId is NULL: +-- nobody performed this, the deploy did. +INSERT INTO "AuditLog" ("action", "entityType", "entityId", "actorId", "actorEmail", "metadata", "congregationId", "createdAt") +SELECT 'permission.direct_grants_migrated', + 'Congregation', + cup."congregationId", + NULL, + NULL, + json_build_object( + 'grants', COUNT(*), + 'roles', COUNT(DISTINCT cup."permissionId"), + 'users', COUNT(DISTINCT cup."userId") + )::text, + cup."congregationId", + CURRENT_TIMESTAMP +FROM "CongregationUserPermission" cup +GROUP BY cup."congregationId"; + +DROP TABLE "_direct_grant_role"; + +-- The direct edge is gone. Its RLS policy, indexes and foreign keys go with the table. +DROP TABLE "CongregationUserPermission"; diff --git a/app/database/migrations/drop-direct-user-permissions.integration.test.ts b/app/database/migrations/drop-direct-user-permissions.integration.test.ts new file mode 100644 index 00000000..f9e7c07a --- /dev/null +++ b/app/database/migrations/drop-direct-user-permissions.integration.test.ts @@ -0,0 +1,319 @@ +import { readFileSync } from 'node:fs' +import { resolve } from 'node:path' +import { PrismaPg } from '@prisma/adapter-pg' +import { afterAll, describe, expect, it } from 'vitest' +import { PrismaClient } from '~/database/generated/client' + +// Runs against DB_URL rather than DB_RUNTIME_URL: a migration executes as the +// schema owner, and this one drops a table, which the RLS-bound runtime role +// cannot do. +const adapter = new PrismaPg({ + connectionString: process.env.DB_URL, + max: 3, + connectionTimeoutMillis: 5000, +}) +const testDb = new PrismaClient({ adapter }) + +const MIGRATION_SQL = resolve(import.meta.dirname, '20260826000000_drop_direct_user_permissions', 'migration.sql') + +const DROP_STATEMENT = 'DROP TABLE "CongregationUserPermission"' + +/** + * The real migration file, split into statements. + * + * Reading the shipped artifact rather than a paraphrase is the point: a test + * that re-typed the SQL would keep passing after someone edited the file. + */ +function migrationStatements(): string[] { + return readFileSync(MIGRATION_SQL, 'utf8') + .split('\n') + .filter(line => !line.trimStart().startsWith('--')) + .join('\n') + .split(';') + .map(statement => statement.trim()) + .filter(statement => statement.length > 0) +} + +/** + * Everything the migration does except the final `DROP TABLE`. + * + * The drop is withheld deliberately. Dropping a table takes ACCESS EXCLUSIVE not + * only on it but on every table its foreign keys reference — `UserAccount`, + * `Permission`, `Congregation` — and this fixture holds that lock until it rolls + * back. Integration files run in parallel against one database, so executing it + * here deadlocks unrelated suites at random. + * + * Withholding it costs nothing in coverage: the backfill above is the part that + * can be wrong, and `drops the direct-grant table` below still pins that the + * statement ships. + */ +function backfillStatements(): string[] { + return migrationStatements().filter(statement => !statement.startsWith(DROP_STATEMENT)) +} + +/** Thrown to roll the fixture back; every assertion runs on captured values. */ +class Rollback extends Error {} + +afterAll(async () => { + await testDb.$disconnect() +}) + +type Tx = Parameters[0]>[0] + +/** + * The permission set an account effectively holds, under the pre-migration + * rule: direct grants unioned with everything its account-bound and + * member-bound roles grant. + * + * Deliberately re-derived here from the three tables rather than calling + * `resolveEffectivePermissions`, which binds to its own module-level client + * and so cannot see this transaction's uncommitted fixture. + */ +async function effectiveBefore(tx: Tx, userId: number, congregationId: number): Promise { + const direct = await tx.$queryRaw<{ key: string }[]>` + SELECT p."key" FROM "CongregationUserPermission" cup + JOIN "Permission" p ON p."id" = cup."permissionId" + WHERE cup."userId" = ${userId} AND cup."congregationId" = ${congregationId} + ` + return [...new Set([...direct.map(r => r.key), ...(await effectiveAfter(tx, userId, congregationId))])].sort() +} + +/** The same question under the post-migration rule: roles only. */ +async function effectiveAfter(tx: Tx, userId: number, congregationId: number): Promise { + const rows = await tx.rolePermission.findMany({ + where: { + congregationId, + role: { + OR: [ + { members: { some: { userId } } }, + { memberAssignments: { some: { member: { account: { id: userId } } } } }, + ], + }, + }, + select: { permission: { select: { key: true } } }, + }) + return [...new Set(rows.map(r => r.permission.key))].sort() +} + +interface AccountSnapshot { + label: string + before: string[] + after: string[] +} + +interface Captured { + accounts: AccountSnapshot[] + auditActions: string[] + auditCongregationIds: number[] + /** Permission keys still granted by congregation B's pre-existing `can-do-anything` role. */ + collidingRoleGrants: string[] + /** Keys of the roles the migration created in congregation B. */ + createdRoleKeysInB: string[] +} + +let fixtureRun: Promise | undefined + +/** Memoized: the fixture is expensive and every assertion reads the same snapshot. */ +function runMigrationOverFixture(): Promise { + fixtureRun ??= executeMigrationOverFixture() + return fixtureRun +} + +async function executeMigrationOverFixture(): Promise { + let captured: Captured | undefined + + try { + await testDb.$transaction( + async tx => { + const stamp = `dropdirect-${process.pid}-${globalThis.performance.now().toString().replace('.', '')}` + + const permissionId = async (key: string) => { + const row = await tx.permission.findUnique({ where: { key }, select: { id: true } }) + if (!row) throw new Error(`Permission "${key}" is not seeded in this database`) + return row.id + } + const [adminPid, territoriesPid, programPid, boardValidatorPid] = await Promise.all([ + permissionId('admin'), + permissionId('territories-manager'), + permissionId('program-viewer'), + permissionId('board-validator'), + ]) + + const congA = await tx.congregation.create({ data: { name: `${stamp}-a`, slug: `${stamp}-a`, active: true } }) + const congB = await tx.congregation.create({ data: { name: `${stamp}-b`, slug: `${stamp}-b`, active: true } }) + + const account = async (congregationId: number, tag: string) => + tx.userAccount.create({ + data: { email: `${stamp}-${tag}@test.invalid`, password: 'hashed', active: true, congregationId }, + }) + + const directGrant = (userId: number, pid: number, congregationId: number) => + tx.$executeRaw` + INSERT INTO "CongregationUserPermission" ("userId", "permissionId", "congregationId") + VALUES (${userId}, ${pid}, ${congregationId}) + ` + + // --- Congregation A --------------------------------------------- + // Admin held only as a direct grant: the case the whole migration exists for. + const aAdmin = await account(congA.id, 'a-admin') + await directGrant(aAdmin.id, adminPid, congA.id) + + // Holds territories-manager BOTH directly and through a role. The + // migration must not double-create, and the set must not change. + const aMixed = await account(congA.id, 'a-mixed') + const aRole = await tx.role.create({ + data: { key: `${stamp}-existing`, name: 'Déjà en place', isBuiltIn: false, congregationId: congA.id }, + }) + await tx.rolePermission.createMany({ + data: [ + { roleId: aRole.id, permissionId: territoriesPid, congregationId: congA.id }, + { roleId: aRole.id, permissionId: programPid, congregationId: congA.id }, + ], + }) + await tx.userRoleAssignment.create({ data: { userId: aMixed.id, roleId: aRole.id, congregationId: congA.id } }) + await directGrant(aMixed.id, territoriesPid, congA.id) + + // Permission arrives through the linked Member's role. Nothing here is + // a direct grant, so the migration must leave it completely alone. + const aViaMember = await account(congA.id, 'a-member') + const member = await tx.member.create({ + data: { firstname: 'Marc', lastname: 'Dupont', congregationId: congA.id }, + }) + await tx.userAccount.update({ where: { id: aViaMember.id }, data: { memberId: member.id } }) + const memberRole = await tx.role.create({ + data: { key: `${stamp}-member-role`, name: 'Rôle membre', isBuiltIn: false, congregationId: congA.id }, + }) + await tx.rolePermission.create({ + data: { roleId: memberRole.id, permissionId: boardValidatorPid, congregationId: congA.id }, + }) + await tx.memberRoleAssignment.create({ + data: { memberId: member.id, roleId: memberRole.id, congregationId: congA.id }, + }) + + // No grants at all — must stay empty, and must not get an audit row. + const aNone = await account(congA.id, 'a-none') + + // --- Congregation B: key collision ------------------------------ + // This congregation already owns a role slugified to `can-do-anything` + // that grants only program-viewer. Reusing it would silently hand + // admin to everyone already assigned to it. + const bExisting = await tx.role.create({ + data: { key: 'can-do-anything', name: 'Peut tout faire', isBuiltIn: false, congregationId: congB.id }, + }) + await tx.rolePermission.create({ + data: { roleId: bExisting.id, permissionId: programPid, congregationId: congB.id }, + }) + const bBystander = await account(congB.id, 'b-bystander') + await tx.userRoleAssignment.create({ + data: { userId: bBystander.id, roleId: bExisting.id, congregationId: congB.id }, + }) + + const bAdmin = await account(congB.id, 'b-admin') + await directGrant(bAdmin.id, adminPid, congB.id) + + const subjects: { label: string; id: number; congregationId: number }[] = [ + { label: 'a-admin', id: aAdmin.id, congregationId: congA.id }, + { label: 'a-mixed', id: aMixed.id, congregationId: congA.id }, + { label: 'a-member', id: aViaMember.id, congregationId: congA.id }, + { label: 'a-none', id: aNone.id, congregationId: congA.id }, + { label: 'b-bystander', id: bBystander.id, congregationId: congB.id }, + { label: 'b-admin', id: bAdmin.id, congregationId: congB.id }, + ] + + const before = new Map() + for (const s of subjects) before.set(s.label, await effectiveBefore(tx, s.id, s.congregationId)) + + for (const statement of backfillStatements()) { + await tx.$executeRawUnsafe(statement) + } + + const accounts: AccountSnapshot[] = [] + for (const s of subjects) { + accounts.push({ + label: s.label, + before: before.get(s.label) ?? [], + after: await effectiveAfter(tx, s.id, s.congregationId), + }) + } + + const auditRows = await tx.auditLog.findMany({ + where: { congregationId: { in: [congA.id, congB.id] } }, + select: { action: true, congregationId: true }, + }) + + const rolesInB = await tx.role.findMany({ + where: { congregationId: congB.id, isBuiltIn: false }, + select: { key: true }, + }) + + const collidingGrants = await tx.rolePermission.findMany({ + where: { roleId: bExisting.id }, + select: { permission: { select: { key: true } } }, + }) + + captured = { + accounts, + auditActions: auditRows.map(r => r.action), + auditCongregationIds: auditRows.map(r => r.congregationId).sort((a, b) => a - b), + collidingRoleGrants: collidingGrants.map(g => g.permission.key).sort(), + createdRoleKeysInB: rolesInB.map(r => r.key).sort(), + } + + // Everything above unwinds here. + throw new Rollback() + }, + { timeout: 25_000 }, + ) + } catch (error) { + if (!(error instanceof Rollback)) throw error + } + + if (!captured) throw new Error('fixture never ran') + return captured +} + +describe('20260826000000_drop_direct_user_permissions', () => { + it('leaves every account with exactly the permissions it had before', async () => { + const result = await runMigrationOverFixture() + + // The acceptance criterion of the whole change. Asserted per account so a + // failure names which one drifted rather than just "sets differ". + for (const account of result.accounts) { + expect(`${account.label}: ${account.after.join(',')}`).toBe(`${account.label}: ${account.before.join(',')}`) + } + + // Sanity: the fixture actually exercised something. Without this the loop + // above passes just as happily against six empty sets. + const admin = result.accounts.find(a => a.label === 'a-admin') + expect(admin?.after).toEqual(['admin']) + const mixed = result.accounts.find(a => a.label === 'a-mixed') + expect(mixed?.after).toEqual(['program-viewer', 'territories-manager']) + const viaMember = result.accounts.find(a => a.label === 'a-member') + expect(viaMember?.after).toEqual(['board-validator']) + expect(result.accounts.find(a => a.label === 'a-none')?.after).toEqual([]) + }) + + it('never widens a role a congregation already owned under the auto-role key', async () => { + const result = await runMigrationOverFixture() + + // Congregation B's own `can-do-anything` must still grant program-viewer + // and nothing else — in particular not admin. + expect(result.collidingRoleGrants).toEqual(['program-viewer']) + // ...and the admin grant landed on a distinct, suffixed role instead. + expect(result.createdRoleKeysInB).toEqual(['can-do-anything', 'can-do-anything-migrated']) + }) + + it('records one bulk audit event per migrated congregation', async () => { + const result = await runMigrationOverFixture() + + expect(result.auditActions).toEqual(['permission.direct_grants_migrated', 'permission.direct_grants_migrated']) + expect(new Set(result.auditCongregationIds).size).toBe(2) + }) + + it('drops the direct-grant table', () => { + // Asserted against the shipped SQL rather than executed — see + // `backfillStatements` for why the drop is withheld from the fixture. + const statements = migrationStatements() + expect(statements.at(-1)).toBe(DROP_STATEMENT) + }) +}) diff --git a/app/database/schema.prisma b/app/database/schema.prisma index 3d14a080..eaac51d1 100644 --- a/app/database/schema.prisma +++ b/app/database/schema.prisma @@ -101,7 +101,6 @@ model Congregation { boardDynamicDocumentSettings BoardDynamicDocumentSettings[] events Event[] settings Setting[] - userPermissions CongregationUserPermission[] roles Role[] rolePermissions RolePermission[] userRoleAssignments UserRoleAssignment[] @@ -292,13 +291,12 @@ model UserAccount { twoFactorEnabledAt DateTime? // Account-bound relations - congregationPermissions CongregationUserPermission[] roleAssignments UserRoleAssignment[] // management/access roles - eventsCreated Event[] @relation("createdBy") - documentsViewed BoardDocument[] @relation("viewedBy") - documentVersionUploads BoardDocumentVersion[] @relation("boardDocumentVersionUploader") + eventsCreated Event[] @relation("createdBy") + documentsViewed BoardDocument[] @relation("viewedBy") + documentVersionUploads BoardDocumentVersion[] @relation("boardDocumentVersionUploader") dynamicDocumentViews BoardDynamicDocumentView[] - templateResponsibilities TemplateResponsible[] @relation("templateResponsible") + templateResponsibilities TemplateResponsible[] @relation("templateResponsible") passwordResetTokens PasswordResetToken[] emailVerificationTokens EmailVerificationToken[] calendarFeedTokens CalendarFeedToken[] @@ -312,10 +310,9 @@ model UserAccount { } model Permission { - id Int @id @default(autoincrement()) - key String @unique - congregationPermissions CongregationUserPermission[] - rolePermissions RolePermission[] + id Int @id @default(autoincrement()) + key String @unique + rolePermissions RolePermission[] } model Role { @@ -394,20 +391,6 @@ model MemberRoleAssignment { @@index([roleId]) } -model CongregationUserPermission { - id Int @id @default(autoincrement()) - user UserAccount @relation(fields: [userId], references: [id], onDelete: Cascade) - userId Int - permission Permission @relation(fields: [permissionId], references: [id], onDelete: Cascade) - permissionId Int - congregation Congregation @relation(fields: [congregationId], references: [id], onDelete: Cascade) - congregationId Int - - @@unique([userId, permissionId, congregationId]) - @@unique([id, congregationId]) - @@index([permissionId]) -} - model BoardSection { id Int @id @default(autoincrement()) name String diff --git a/app/database/seed-marketing.ts b/app/database/seed-marketing.ts index 736c0a3b..a2f9473f 100644 --- a/app/database/seed-marketing.ts +++ b/app/database/seed-marketing.ts @@ -24,7 +24,7 @@ import { TerritoryKindKey } from '../features/territories/model/territory-kind.t import { seedBuiltInTerritoryKinds } from '../features/territories/server/territory-kinds.server' import { syncBuiltInRoleAssignments } from '../shared/domain/built-in-roles.server' import { seedBuiltInRoles } from '../shared/domain/setup.server' -import { Permission } from '../shared/types/permission' +import { autoRoleKeyForPermission, Permission } from '../shared/types/permission' import { PublisherType } from '../shared/types/publisher-type' import { stripDiacritics } from '../shared/utils/strip-diacritics' import { PrismaClient } from './generated/client' @@ -563,7 +563,9 @@ async function cleanCongregationData(congregationId: number) { await prisma.buildingEntrance.deleteMany({ where: { congregationId } }) await prisma.building.deleteMany({ where: { congregationId } }) await prisma.territory.deleteMany({ where: { congregationId } }) - await prisma.congregationUserPermission.deleteMany({ where: { congregationId } }) + await prisma.userRoleAssignment.deleteMany({ where: { congregationId } }) + await prisma.rolePermission.deleteMany({ where: { congregationId } }) + await prisma.role.deleteMany({ where: { congregationId, isBuiltIn: false } }) await prisma.member.updateMany({ where: { congregationId }, data: { publisherGroupId: null }, @@ -765,40 +767,41 @@ async function main() { settingsUserManagerRole, ].filter(Boolean) - for (const role of rolesToAssign) { - if (!role) continue - await prisma.congregationUserPermission.upsert({ - where: { - userId_permissionId_congregationId: { - userId: mainAdmin.accountId, - permissionId: role.id, - congregationId: congId, - }, - }, + // Permissions travel through roles only. The demo tenant therefore gets the + // same auto-roles the #149 backfill mints for a real congregation, so what a + // demo admin sees on the roles screen matches production. + async function grantViaAutoRole(accountId: number, permission: { id: number; key: string }) { + const key = autoRoleKeyForPermission(permission.key) + if (!key) return + + const role = await prisma.role.upsert({ + where: { key_congregationId: { key, congregationId: congId } }, + update: {}, + create: { key, isBuiltIn: false, congregationId: congId }, + select: { id: true }, + }) + await prisma.rolePermission.upsert({ + where: { roleId_permissionId: { roleId: role.id, permissionId: permission.id } }, + update: {}, + create: { roleId: role.id, permissionId: permission.id, congregationId: congId }, + }) + await prisma.userRoleAssignment.upsert({ + where: { userId_roleId: { userId: accountId, roleId: role.id } }, update: {}, - create: { userId: mainAdmin.accountId, permissionId: role.id, congregationId: congId }, + create: { userId: accountId, roleId: role.id, congregationId: congId }, }) } + for (const permission of rolesToAssign) { + if (!permission) continue + await grantViaAutoRole(mainAdmin.accountId, permission) + } + // Other elders get territory viewer + board validator for (let i = 1; i < 5; i++) { - for (const role of [terrViewerRole, boardValidatorRole]) { - if (!role) continue - await prisma.congregationUserPermission.upsert({ - where: { - userId_permissionId_congregationId: { - userId: createdUsers[i].accountId, - permissionId: role.id, - congregationId: congId, - }, - }, - update: {}, - create: { - userId: createdUsers[i].accountId, - permissionId: role.id, - congregationId: congId, - }, - }) + for (const permission of [terrViewerRole, boardValidatorRole]) { + if (!permission) continue + await grantViaAutoRole(createdUsers[i].accountId, permission) } } diff --git a/app/features/authentication/server/register-congregation.server.test.ts b/app/features/authentication/server/register-congregation.server.test.ts index 6086327e..12b7015a 100644 --- a/app/features/authentication/server/register-congregation.server.test.ts +++ b/app/features/authentication/server/register-congregation.server.test.ts @@ -6,6 +6,7 @@ const scopedDb = { // Seeding the default templates also seeds the part presets they link to. partPreset: { findFirst: vi.fn(), create: vi.fn(), findMany: vi.fn() }, role: { upsert: vi.fn(), findUnique: vi.fn() }, + userRoleAssignment: { create: vi.fn() }, // Setup also seeds the built-in territory kinds. territoryKind: { upsert: vi.fn() }, permission: { findUnique: vi.fn() }, @@ -17,7 +18,6 @@ vi.mock('~/shared/infra/db.server', () => ({ congregation: { findUnique: vi.fn(), create: vi.fn() }, userAccount: { findUnique: vi.fn(), create: vi.fn() }, permission: { findUnique: vi.fn(), upsert: vi.fn() }, - congregationUserPermission: { create: vi.fn() }, consentRecord: { create: vi.fn() }, }, withScope: vi.fn((_id: number, fn: (db: unknown) => Promise) => fn(scopedDb)), @@ -53,7 +53,10 @@ beforeEach(() => { })) as never) vi.mocked(db.userAccount.create).mockResolvedValue({ id: 10 } as never) vi.mocked(db.permission.findUnique).mockResolvedValue({ id: 5, key: 'admin' } as never) - vi.mocked(db.congregationUserPermission.create).mockResolvedValue({} as never) + scopedDb.permission.findUnique.mockResolvedValue({ id: 5 } as never) + scopedDb.role.upsert.mockResolvedValue({ id: 77 } as never) + scopedDb.rolePermission.upsert.mockResolvedValue({} as never) + scopedDb.userRoleAssignment.create.mockResolvedValue({} as never) scopedDb.eventKind.upsert.mockResolvedValue({} as never) scopedDb.eventTemplate.findFirst.mockResolvedValue(null as never) scopedDb.eventTemplate.create.mockResolvedValue({} as never) @@ -72,6 +75,19 @@ describe('registerCongregation', () => { expect('congregationSlug' in result && result.congregationSlug).not.toBe('test-congre') }) + it('donne les droits admin au premier compte via un rôle, pas un octroi direct', async () => { + await registerCongregation('Ma Congrégation', 'test-congre', 'admin@test.com', 'motdepasse', 'fr') + + // Depuis #149 l'arête directe utilisateur->permission n'existe plus : le + // compte créé à l'inscription ne peut devenir admin qu'en passant par un rôle. + expect(scopedDb.role.upsert).toHaveBeenCalledWith( + expect.objectContaining({ create: { key: 'can-do-anything', isBuiltIn: false, congregationId: 1 } }), + ) + expect(scopedDb.userRoleAssignment.create).toHaveBeenCalledWith({ + data: { userId: 10, roleId: 77, congregationId: 1 }, + }) + }) + it('génère des slugs différents pour deux inscriptions du même nom', async () => { const first = await registerCongregation('Ma Congrégation', 'test-congre', 'a@test.com', 'motdepasse', 'fr') const second = await registerCongregation('Ma Congrégation', 'test-congre', 'b@test.com', 'motdepasse', 'fr') diff --git a/app/features/authentication/server/register-congregation.server.ts b/app/features/authentication/server/register-congregation.server.ts index f0276637..8d208831 100644 --- a/app/features/authentication/server/register-congregation.server.ts +++ b/app/features/authentication/server/register-congregation.server.ts @@ -6,7 +6,7 @@ import type { locales } from '~/i18n/paraglide/runtime' import { hash } from '~/shared/auth/crypto.server' import { syncBuiltInRoleAssignments } from '~/shared/domain/built-in-roles.server' import { ConsentPurpose, recordConsentUnscoped } from '~/shared/domain/consent.server' -import { seedCongregationDefaults, seedPermissions } from '~/shared/domain/setup.server' +import { ensureAdminRole, seedCongregationDefaults, seedPermissions } from '~/shared/domain/setup.server' import { createLogger } from '~/shared/infra/logger.server' type Locale = (typeof locales)[number] @@ -72,23 +72,20 @@ export async function registerCongregation( }, }) - // Assign admin permission - const adminPermission = await db.permission.findUnique({ where: { key: 'admin' } }) - if (adminPermission) { - await db.congregationUserPermission.create({ - data: { - userId: user.id, - permissionId: adminPermission.id, - congregationId: congregation.id, - }, - }) - } - // Create default programme templates (including the system day-off and // freeform templates) inside a scoped transaction so PostgreSQL RLS allows - // the inserts. + // the inserts. The admin role belongs here too: Role, RolePermission and + // UserRoleAssignment are all RLS-scoped, unlike the direct grant this replaced. await withScope(congregation.id, async scopedDb => { await seedCongregationDefaults(scopedDb, congregation.id, locale, seedDefaultTemplates, seedBuiltInTerritoryKinds) + + const adminRoleId = await ensureAdminRole(scopedDb, congregation.id) + if (adminRoleId != null) { + await scopedDb.userRoleAssignment.create({ + data: { userId: user.id, roleId: adminRoleId, congregationId: congregation.id }, + }) + } + await syncBuiltInRoleAssignments(scopedDb, user.id, congregation.id, user.id) }) diff --git a/app/features/authentication/server/setup-first-account.server.test.ts b/app/features/authentication/server/setup-first-account.server.test.ts index 4f24f773..b830c19a 100644 --- a/app/features/authentication/server/setup-first-account.server.test.ts +++ b/app/features/authentication/server/setup-first-account.server.test.ts @@ -6,6 +6,7 @@ const scopedDb = { // Seeding the default templates also seeds the part presets they link to. partPreset: { findFirst: vi.fn(), create: vi.fn(), findMany: vi.fn() }, role: { upsert: vi.fn(), findUnique: vi.fn() }, + userRoleAssignment: { create: vi.fn() }, // Setup also seeds the built-in territory kinds. territoryKind: { upsert: vi.fn() }, permission: { findUnique: vi.fn() }, @@ -17,7 +18,6 @@ vi.mock('~/shared/infra/db.server', () => ({ congregation: { findFirst: vi.fn(), create: vi.fn() }, userAccount: { create: vi.fn() }, permission: { findUnique: vi.fn(), upsert: vi.fn() }, - congregationUserPermission: { create: vi.fn() }, consentRecord: { create: vi.fn() }, }, withScope: vi.fn((_id: number, fn: (db: unknown) => Promise) => fn(scopedDb)), @@ -41,7 +41,10 @@ beforeEach(() => { vi.mocked(db.congregation.create).mockResolvedValue({ id: 1, slug: 'test' } as never) vi.mocked(db.userAccount.create).mockResolvedValue({ id: 42 } as never) vi.mocked(db.permission.findUnique).mockResolvedValue({ id: 5, key: 'admin' } as never) - vi.mocked(db.congregationUserPermission.create).mockResolvedValue({} as never) + scopedDb.permission.findUnique.mockResolvedValue({ id: 5 } as never) + scopedDb.role.upsert.mockResolvedValue({ id: 77 } as never) + scopedDb.rolePermission.upsert.mockResolvedValue({} as never) + scopedDb.userRoleAssignment.create.mockResolvedValue({} as never) scopedDb.eventKind.upsert.mockResolvedValue({} as never) scopedDb.eventTemplate.findFirst.mockResolvedValue(null as never) scopedDb.eventTemplate.create.mockResolvedValue({} as never) @@ -56,10 +59,23 @@ describe('setupFirstAccount', () => { expect(result).toBe(42) }) - it("fonctionne même si le rôle admin n'existe pas", async () => { - vi.mocked(db.permission.findUnique).mockResolvedValue(null as never) + it("fonctionne même si la permission admin n'existe pas", async () => { + scopedDb.permission.findUnique.mockResolvedValue(null as never) const result = await setupFirstAccount('admin@test.com', 'motdepasse', 'Ma Congré', 'ma-congre', 'fr') expect(result).toBe(42) }) + + it('donne les droits admin au premier compte via un rôle, pas un octroi direct', async () => { + await setupFirstAccount('admin@test.com', 'motdepasse', 'Ma Congré', 'ma-congre', 'fr') + + // Depuis #149 l'arête directe utilisateur->permission n'existe plus : le + // premier compte ne peut devenir admin qu'en passant par un rôle. + expect(scopedDb.role.upsert).toHaveBeenCalledWith( + expect.objectContaining({ create: { key: 'can-do-anything', isBuiltIn: false, congregationId: 1 } }), + ) + expect(scopedDb.userRoleAssignment.create).toHaveBeenCalledWith({ + data: { userId: 42, roleId: 77, congregationId: 1 }, + }) + }) }) diff --git a/app/features/authentication/server/setup-first-account.server.ts b/app/features/authentication/server/setup-first-account.server.ts index ee33b052..e4ef84ab 100644 --- a/app/features/authentication/server/setup-first-account.server.ts +++ b/app/features/authentication/server/setup-first-account.server.ts @@ -4,7 +4,7 @@ import type { locales } from '~/i18n/paraglide/runtime' import { hash } from '~/shared/auth/crypto.server' import { syncBuiltInRoleAssignments } from '~/shared/domain/built-in-roles.server' import { ConsentPurpose, recordConsentUnscoped } from '~/shared/domain/consent.server' -import { seedCongregationDefaults, seedPermissions } from '~/shared/domain/setup.server' +import { ensureAdminRole, seedCongregationDefaults, seedPermissions } from '~/shared/domain/setup.server' type Locale = (typeof locales)[number] @@ -43,22 +43,20 @@ export async function setupFirstAccount( }, }) - const adminPermission = await db.permission.findUnique({ where: { key: 'admin' } }) - if (adminPermission) { - await db.congregationUserPermission.create({ - data: { - userId: user.id, - permissionId: adminPermission.id, - congregationId: congregation.id, - }, - }) - } - // Create default programme templates (including the system day-off and // freeform templates) inside a scoped transaction so PostgreSQL RLS allows - // the inserts. + // the inserts. The admin role belongs here too: Role, RolePermission and + // UserRoleAssignment are all RLS-scoped, unlike the direct grant this replaced. await withScope(congregation.id, async scopedDb => { await seedCongregationDefaults(scopedDb, congregation.id, locale, seedDefaultTemplates, seedBuiltInTerritoryKinds) + + const adminRoleId = await ensureAdminRole(scopedDb, congregation.id) + if (adminRoleId != null) { + await scopedDb.userRoleAssignment.create({ + data: { userId: user.id, roleId: adminRoleId, congregationId: congregation.id }, + }) + } + await syncBuiltInRoleAssignments(scopedDb, user.id, congregation.id, user.id) }) diff --git a/app/features/congregation/routes/roles/edit-role.tsx b/app/features/congregation/routes/roles/edit-role.tsx index 54a9241b..d3385d5c 100644 --- a/app/features/congregation/routes/roles/edit-role.tsx +++ b/app/features/congregation/routes/roles/edit-role.tsx @@ -88,7 +88,7 @@ export default function EditRolePage({ loaderData, actionData }: Route.Component {fields.name.errors &&

{fields.name.errors}

} diff --git a/app/features/settings/routes/users/edit-user.tsx b/app/features/settings/routes/users/edit-user.tsx index e715b348..8274fa8b 100644 --- a/app/features/settings/routes/users/edit-user.tsx +++ b/app/features/settings/routes/users/edit-user.tsx @@ -4,7 +4,6 @@ import { Download, IdCard, ShieldAlert, UserPlus } from 'lucide-react' import { data, Form, Link, redirect, useSubmit } from 'react-router' import { editUserSchema } from '~/features/settings/schemas/user.schema' import { updateAccount } from '~/features/settings/server/update-account.server' -import { RolePermissionPicker } from '~/features/settings/ui/RolePermissionPicker' import * as m from '~/i18n/paraglide/messages' import { currentAccountContext, permissionsContext, withScopeFromContext } from '~/shared/auth/route-context.server' import { setUserCustomRoleAssignments } from '~/shared/domain/roles.server' @@ -24,7 +23,7 @@ import { AlertDialogTrigger, } from '~/shared/ui/alert-dialog' import { Button } from '~/shared/ui/button' -import { Card, CardContent, CardDescription, CardHeader, CardTitle } from '~/shared/ui/card' +import { Card, CardContent, CardHeader, CardTitle } from '~/shared/ui/card' import { Checkbox } from '~/shared/ui/checkbox' import { FormActions } from '~/shared/ui/FormActions' import { useFocusError } from '~/shared/ui/hooks/use-focus-error' @@ -64,7 +63,6 @@ export function loader({ params, context }: Route.LoaderArgs) { }, include: { member: { select: { id: true, firstname: true, lastname: true, isPublisher: true, anonymizedAt: true } }, - congregationPermissions: { include: { permission: true } }, // Identity-role assignments for the matrix come from the linked Member }, }) @@ -82,7 +80,6 @@ export function loader({ params, context }: Route.LoaderArgs) { select: { roleId: true }, }) - const permissionList = await db.permission.findMany() const allRoles = await db.role.findMany({ where: { congregationId: currentUser.congregationId }, orderBy: [{ isBuiltIn: 'desc' }, { name: 'asc' }, { key: 'asc' }], @@ -98,8 +95,6 @@ export function loader({ params, context }: Route.LoaderArgs) { active: user.active, firstname: user.member?.firstname ?? user.firstname, lastname: user.member?.lastname ?? user.lastname, - permissions: user.congregationPermissions.map(cp => cp.permission), - permissionList, builtInRoles: allRoles .filter(r => r.isBuiltIn) .map(r => ({ @@ -130,8 +125,7 @@ export function loader({ params, context }: Route.LoaderArgs) { // biome-ignore lint/complexity/noExcessiveCognitiveComplexity: large edit page with multiple optional sections (custom roles, danger-zone, anonymized banner) export default function SettingsLayout({ loaderData, actionData }: Route.ComponentProps) { - const { permissionList, builtInRoles, customRoles, canManageRoles, isAdmin, canAnonymize, anonymizedAt, ...user } = - loaderData + const { builtInRoles, customRoles, canManageRoles, canAnonymize, anonymizedAt, ...user } = loaderData const { blocker, markDirty } = useUnsavedChanges() const submit = useSubmit() @@ -341,29 +335,17 @@ export default function SettingsLayout({ loaderData, actionData }: Route.Compone )} - - - {m.settings_user_edit_rights_title()} - {m.settings_user_edit_rights_subtitle()} - - - {publisherNotUser ? ( + {publisherNotUser && ( + +

{m.settings_user_edit_publisher_only_notice()}
{m.settings_user_edit_publisher_only_hint()}

- ) : ( - p.key)} - name="permissions" - showHeader={false} - disabledKeys={isAdmin ? [] : [Permission.Admin]} - /> - )} - - + + + )} {m.settings_user_edit_submit()} @@ -457,7 +439,7 @@ export async function action({ request, params, context }: Route.ActionArgs) { return data(submission.reply(), { status: 400 }) } - const { firstname, lastname, email, active, permissions: selectedPermissions, customRoleIds } = submission.value + const { firstname, lastname, email, active, customRoleIds } = submission.value return withScopeFromContext(context, async db => { try { @@ -466,7 +448,6 @@ export async function action({ request, params, context }: Route.ActionArgs) { lastname, email, active, - permissions: selectedPermissions, }) await setUserCustomRoleAssignments(db, accountId, currentUser.congregationId, currentUser.id, customRoleIds) diff --git a/app/features/settings/routes/users/user-list.tsx b/app/features/settings/routes/users/user-list.tsx index 277076ba..5ae9bd97 100644 --- a/app/features/settings/routes/users/user-list.tsx +++ b/app/features/settings/routes/users/user-list.tsx @@ -62,11 +62,6 @@ export function loader({ request, context }: Route.LoaderArgs) { member: { select: { firstname: true, lastname: true, isPublisher: true, leftAt: true } }, // UserRoleAssignment holds management/custom roles only (post-split). roleAssignments: { select: { roleId: true } }, - _count: { - select: { - congregationPermissions: true, - }, - }, }, orderBy: [ { @@ -102,7 +97,6 @@ export function loader({ request, context }: Route.LoaderArgs) { isPublisher: account.member?.isPublisher ?? false, builtInRoleCount: account.memberId ? (builtInCountByMember.get(account.memberId) ?? 0) : 0, customRoleCount: account.roleAssignments.length, - directPermissionCount: account._count.congregationPermissions, })), roles: { canViewPublishers, @@ -140,7 +134,6 @@ export default function UserListPage({ loaderData }: Route.ComponentProps) { {m.settings_users_table_email()} {m.settings_users_table_publisher()} {m.settings_users_table_roles()} - {m.settings_users_table_custom_permissions()} {m.settings_users_table_actions_sr()} @@ -205,13 +198,6 @@ export default function UserListPage({ loaderData }: Route.ComponentProps) { )} - - {user.directPermissionCount > 0 ? ( - {user.directPermissionCount} - ) : ( - 0 - )} -