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..f61af7ab --- /dev/null +++ b/app/database/migrations/20260826000000_drop_direct_user_permissions/migration.sql @@ -0,0 +1,173 @@ +-- 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". Adopting it would silently +-- grant that permission to everyone already assigned to it, so a taken key falls back +-- to "-migrated", and then to "-migrated-". +-- +-- "Taken" means taken *by something else*: a key whose role already grants exactly this +-- one permission is a role this migration itself created, so it is reused rather than +-- duplicated. That is what makes the file re-runnable, and it matches the rule +-- `resolveAutoRoleId` applies on the archive-import path. +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 + AND NOT ( + (SELECT COUNT(*) FROM "RolePermission" rp WHERE rp."roleId" = r."id") = 1 + AND EXISTS ( + SELECT 1 FROM "RolePermission" rp + WHERE rp."roleId" = r."id" AND rp."permissionId" = n.pid + ) + ) + ) + THEN n.base + WHEN NOT EXISTS ( + SELECT 1 FROM "Role" r + WHERE r."congregationId" = n.cid AND r."key" = n.base || '-migrated' + AND NOT ( + (SELECT COUNT(*) FROM "RolePermission" rp WHERE rp."roleId" = r."id") = 1 + AND EXISTS ( + SELECT 1 FROM "RolePermission" rp + WHERE rp."roleId" = r."id" AND rp."permissionId" = n.pid + ) + ) + ) + THEN n.base || '-migrated' + ELSE n.base || '-migrated-' || n.pid::text + END +FROM ( + -- LEFT JOIN with a COALESCE fallback, not an inner join. A permission that + -- shipped between this file being written and being run would not be in the + -- VALUES list, and an inner join would drop its grants on the floor — + -- permanently, since the table goes away at the end of this migration. + -- `can-` is a less pretty role name than the curated ones below, and an + -- admin can rename it; silently revoking someone's access is not fixable. + SELECT DISTINCT + cup."congregationId" AS cid, + cup."permissionId" AS pid, + COALESCE(m.role_key, 'can-' || p."key") AS base + FROM "CongregationUserPermission" cup + JOIN "Permission" p ON p."id" = cup."permissionId" + LEFT 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 +WHERE NOT EXISTS ( + -- Unlike the inserts above there is no unique key to conflict on, so the + -- re-run guard has to be explicit: without it a replay would file a second + -- event per congregation and make the trail read as two migrations. + SELECT 1 FROM "AuditLog" a + WHERE a."congregationId" = cup."congregationId" + AND a."action" = 'permission.direct_grants_migrated' +) +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..d98a5487 --- /dev/null +++ b/app/database/migrations/drop-direct-user-permissions.integration.test.ts @@ -0,0 +1,392 @@ +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 table as it stood before this migration dropped it, reduced to the three + * columns the migration actually reads. + * + * CI runs `prisma migrate deploy` before this suite, so by the time the test + * executes the real table is already gone and there would be no "before" to + * migrate. `IF NOT EXISTS` makes this a no-op on a development database where + * the migration has not been applied yet, so the test behaves the same in both. + * + * Deliberately without foreign keys: they are irrelevant to what is under test + * and would make any drop of this table take ACCESS EXCLUSIVE on `UserAccount`, + * `Permission` and `Congregation`. + */ +const RECREATE_DROPPED_TABLE = ` + CREATE TABLE IF NOT EXISTS "CongregationUserPermission" ( + "id" SERIAL PRIMARY KEY, + "userId" INTEGER NOT NULL, + "permissionId" INTEGER NOT NULL, + "congregationId" INTEGER NOT NULL + ) +` + +/** + * 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. On a development database the table still + * carries its foreign keys, so dropping it takes ACCESS EXCLUSIVE on + * `UserAccount`, `Permission` and `Congregation` too, and this fixture holds + * that until it rolls back. Integration files run in parallel against one + * database, so executing it deadlocked 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[] + /** The permission key deliberately left out of the migration's mapping table. */ + unmappedPermissionKey: string + /** Row counts after one backfill, and after running it a second time. */ + countsAfterFirstRun: Record + countsAfterSecondRun: Record +} + +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'), + ]) + + await tx.$executeRawUnsafe(RECREATE_DROPPED_TABLE) + + 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 }, + }) + + // A permission the mapping table has never heard of. Reachable if a + // permission ships between this migration being written and being run. + // Its grants must survive anyway — the table is about to be dropped, so + // anything not carried across is lost for good. + const unmappedKey = `${stamp}-unmapped` + const unmapped = await tx.permission.create({ data: { key: unmappedKey }, select: { id: true } }) + const aUnmapped = await account(congA.id, 'a-unmapped') + await directGrant(aUnmapped.id, unmapped.id, 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-unmapped', id: aUnmapped.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 countRows = async (): Promise> => ({ + roles: await tx.role.count({ where: { congregationId: { in: [congA.id, congB.id] } } }), + rolePermissions: await tx.rolePermission.count({ where: { congregationId: { in: [congA.id, congB.id] } } }), + assignments: await tx.userRoleAssignment.count({ where: { congregationId: { in: [congA.id, congB.id] } } }), + auditRows: await tx.auditLog.count({ where: { congregationId: { in: [congA.id, congB.id] } } }), + }) + + const countsAfterFirstRun = await countRows() + + // The file claims to be re-runnable (every INSERT is ON CONFLICT DO + // NOTHING). A deploy that retries would otherwise duplicate roles and + // double-count the audit metadata. + for (const statement of backfillStatements()) { + await tx.$executeRawUnsafe(statement) + } + const countsAfterSecondRun = await countRows() + + 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), + unmappedPermissionKey: unmappedKey, + countsAfterFirstRun, + countsAfterSecondRun, + 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([]) + + // A permission missing from the mapping table still has to survive: the + // grant table is dropped, so a skipped row is permanently lost access. + const unmapped = result.accounts.find(a => a.label === 'a-unmapped') + expect(unmapped?.after).toEqual([result.unmappedPermissionKey]) + }) + + 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('is re-runnable without duplicating anything', async () => { + const result = await runMigrationOverFixture() + + expect(result.countsAfterSecondRun).toEqual(result.countsAfterFirstRun) + // Guard against the comparison passing on two empty snapshots. + expect(result.countsAfterFirstRun.roles).toBeGreaterThan(0) + expect(result.countsAfterFirstRun.assignments).toBeGreaterThan(0) + }) + + 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 - )} -