diff --git a/frontend/i18n/locales/fr.json b/frontend/i18n/locales/fr.json index d562ed9..5e308e9 100644 --- a/frontend/i18n/locales/fr.json +++ b/frontend/i18n/locales/fr.json @@ -136,7 +136,8 @@ }, "permissions": { "selectAll": "Tout selectionner", - "noPermissions": "Aucune permission disponible" + "noPermissions": "Aucune permission disponible", + "loadFailed": "Impossible de charger le catalogue de permissions. L'enregistrement est désactivé pour éviter tout écrasement accidentel." } }, "users": { @@ -160,7 +161,8 @@ "noEffectivePermissions": "Aucune permission effective", "sourceRole": "via {role}", "sourceDirect": "Direct", - "lastAdminWarning": "Impossible de retirer le statut administrateur du dernier admin" + "lastAdminWarning": "Impossible de retirer le statut administrateur du dernier admin", + "loadFailed": "Impossible de charger les droits de cet utilisateur. L'enregistrement est désactivé pour éviter tout écrasement accidentel." }, "toast": { "updated": "Permissions mises à jour avec succès" diff --git a/frontend/modules/core/components/RoleDrawer.vue b/frontend/modules/core/components/RoleDrawer.vue index 687cc71..cc76aaa 100644 --- a/frontend/modules/core/components/RoleDrawer.vue +++ b/frontend/modules/core/components/RoleDrawer.vue @@ -33,7 +33,15 @@

{{ t('admin.roles.form.permissions') }}

-
+ +
+ {{ t('admin.roles.permissions.loadFailed') }} +
+
{{ t('admin.roles.permissions.noPermissions') }}
@@ -70,7 +78,7 @@
@@ -102,6 +110,11 @@ const emit = defineEmits<{ const saving = ref(false) const allPermissions = ref([]) +// Signale un echec de chargement du catalogue de permissions : on bloque +// alors la sauvegarde pour eviter qu'un drawer ouvert avec zero permission +// visible (cas d'un 403 ou d'une panne reseau) n'ecrase silencieusement +// toutes les permissions du role. +const permissionsLoadFailed = ref(false) const form = ref({ label: '', @@ -129,12 +142,21 @@ const permissionsByModule = computed(() => { // Charger les permissions au montage async function loadPermissions() { - const data = await api.get<{ member: Permission[] }>( - '/permissions', - { 'orphan': false, itemsPerPage: 999 }, - { toast: false }, - ) - allPermissions.value = data.member + permissionsLoadFailed.value = false + try { + const data = await api.get<{ member: Permission[] }>( + '/permissions', + { 'orphan': false, itemsPerPage: 999 }, + // `toast: true` : en cas d'echec (403, reseau, 500), l'utilisateur + // voit l'erreur remonter. Sans ce feedback, un catalogue vide + // ressemblerait a un role sans permissions disponibles. + { toast: true }, + ) + allPermissions.value = data.member + } catch { + allPermissions.value = [] + permissionsLoadFailed.value = true + } } // Remplir le formulaire quand le role change diff --git a/frontend/modules/core/components/UserRbacDrawer.vue b/frontend/modules/core/components/UserRbacDrawer.vue index f4d61df..54b194d 100644 --- a/frontend/modules/core/components/UserRbacDrawer.vue +++ b/frontend/modules/core/components/UserRbacDrawer.vue @@ -6,6 +6,16 @@ @update:model-value="emit('update:modelValue', $event)" >
+ +
+ + {{ t('admin.users.drawer.loadFailed') }} +
+
@@ -138,6 +148,10 @@ const saving = ref(false) const allRoles = ref([]) const allPermissions = ref([]) const allSites = ref([]) +// Signale un echec de chargement des referentiels : on bloque alors la +// sauvegarde pour eviter qu'un drawer ouvert sans donnees (403, reseau) +// n'ecrase silencieusement l'etat RBAC du user (vidage roles/permissions/sites). +const loadFailed = ref(false) const form = ref({ isAdmin: false }) const selectedRoleIds = ref(new Set()) @@ -211,20 +225,32 @@ const effectivePermissions = computed(() => { // Le detail RBAC est la seule source de verite pour l'etat initial du formulaire : // props.user vient de la liste /api/users qui n'expose pas les sites (groupe leger). async function loadData(userId: number) { - const [rolesData, permsData, sitesData, userRbac] = await Promise.all([ - api.get<{ member: Role[] }>('/roles', {}, { toast: false }), - api.get<{ member: Permission[] }>('/permissions', { orphan: false, itemsPerPage: 999 }, { toast: false }), - api.get<{ member: Site[] }>('/sites', { itemsPerPage: 999 }, { toast: false }), - api.get(`/users/${userId}/rbac`, {}, { toast: false }), - ]) - allRoles.value = rolesData.member - allPermissions.value = permsData.member - allSites.value = sitesData.member + loadFailed.value = false + try { + const [rolesData, permsData, sitesData, userRbac] = await Promise.all([ + // `toast: true` : en cas d'echec, l'utilisateur voit un toast + // d'erreur. Sans ce feedback, le drawer s'afficherait vide et la + // sauvegarde ecraserait silencieusement l'etat RBAC du user. + api.get<{ member: Role[] }>('/roles', {}, { toast: true }), + api.get<{ member: Permission[] }>('/permissions', { orphan: false, itemsPerPage: 999 }, { toast: true }), + api.get<{ member: Site[] }>('/sites', { itemsPerPage: 999 }, { toast: true }), + api.get(`/users/${userId}/rbac`, {}, { toast: true }), + ]) + allRoles.value = rolesData.member + allPermissions.value = permsData.member + allSites.value = sitesData.member - form.value.isAdmin = userRbac.isAdmin - selectedRoleIds.value = new Set((userRbac.roles ?? []).map(iriToId)) - selectedDirectPermissionIds.value = new Set((userRbac.directPermissions ?? []).map(iriToId)) - selectedSiteIds.value = new Set((userRbac.sites ?? []).map(iriToId)) + form.value.isAdmin = userRbac.isAdmin + selectedRoleIds.value = new Set((userRbac.roles ?? []).map(iriToId)) + selectedDirectPermissionIds.value = new Set((userRbac.directPermissions ?? []).map(iriToId)) + selectedSiteIds.value = new Set((userRbac.sites ?? []).map(iriToId)) + } catch { + loadFailed.value = true + allRoles.value = [] + allPermissions.value = [] + allSites.value = [] + resetForm() + } } function resetForm() { diff --git a/frontend/modules/core/pages/admin/audit-log.vue b/frontend/modules/core/pages/admin/audit-log.vue index 4a8a7f9..ebb6aa1 100644 --- a/frontend/modules/core/pages/admin/audit-log.vue +++ b/frontend/modules/core/pages/admin/audit-log.vue @@ -153,7 +153,7 @@ import type { AuditLogEntry, AuditLogFilters } from '~/shared/types' const { t } = useI18n() const { can } = usePermissions() -const { fetchLogs, fetchEntityTypes } = useAuditLog() +const { fetchLogsCached, fetchEntityTypes } = useAuditLog() // Protection cote UI : le middleware `modules.global.ts` filtre deja les // routes desactivees, mais si quelqu'un atterit ici sans la permission on @@ -275,7 +275,7 @@ async function loadEntries(): Promise { const token = ++requestToken loading.value = true try { - const data = await fetchLogs({ + const data = await fetchLogsCached({ ...filters, // Convertit datetime-local (YYYY-MM-DDTHH:MM) en ISO pour l'API. performedAtAfter: filters.performedAtAfter ? toIso(filters.performedAtAfter) : undefined, diff --git a/frontend/shared/components/audit/AuditLogDetail.vue b/frontend/shared/components/audit/AuditLogDetail.vue index 7058a7c..0f815fd 100644 --- a/frontend/shared/components/audit/AuditLogDetail.vue +++ b/frontend/shared/components/audit/AuditLogDetail.vue @@ -91,7 +91,9 @@ const collectionDiff = computed new Intl.RelativeTimeFormat(locale.value, { numeric: 'auto' })) function relativeDate(iso: string): string { const diffMs = Date.now() - new Date(iso).getTime() const diffSec = Math.round(diffMs / 1000) const absSec = Math.abs(diffSec) + const fmt = rtf.value - if (absSec < 60) return rtf.format(-Math.sign(diffSec) * Math.abs(diffSec), 'second') - if (absSec < 3600) return rtf.format(-Math.sign(diffSec) * Math.round(absSec / 60), 'minute') - if (absSec < 86400) return rtf.format(-Math.sign(diffSec) * Math.round(absSec / 3600), 'hour') - if (absSec < 604800) return rtf.format(-Math.sign(diffSec) * Math.round(absSec / 86400), 'day') - return rtf.format(-Math.sign(diffSec) * Math.round(absSec / 604800), 'week') + if (absSec < 60) return fmt.format(-Math.sign(diffSec) * Math.abs(diffSec), 'second') + if (absSec < 3600) return fmt.format(-Math.sign(diffSec) * Math.round(absSec / 60), 'minute') + if (absSec < 86400) return fmt.format(-Math.sign(diffSec) * Math.round(absSec / 3600), 'hour') + if (absSec < 604800) return fmt.format(-Math.sign(diffSec) * Math.round(absSec / 86400), 'day') + return fmt.format(-Math.sign(diffSec) * Math.round(absSec / 604800), 'week') } function absoluteDate(iso: string): string { - return new Date(iso).toLocaleString('fr-FR', { + // Meme logique : la locale de formatage suit celle de l'app. + return new Date(iso).toLocaleString(locale.value, { dateStyle: 'medium', timeStyle: 'short', }) @@ -223,7 +227,9 @@ function snapshotSummary(entry: AuditLogEntry): string { function formatValue(value: unknown): string { if (value === null || value === undefined) return '∅' - if (typeof value === 'boolean') return value ? 'oui' : 'non' + // Passe par i18n plutot qu'un hardcode FR : si une autre locale est + // ajoutee, le rendu s'adapte sans nouvelle passe sur ce composant. + if (typeof value === 'boolean') return value ? t('common.yes') : t('common.no') if (typeof value === 'object') return JSON.stringify(value) return String(value) } diff --git a/frontend/shared/composables/useAuditLog.ts b/frontend/shared/composables/useAuditLog.ts index faac0d7..7a34420 100644 --- a/frontend/shared/composables/useAuditLog.ts +++ b/frontend/shared/composables/useAuditLog.ts @@ -128,9 +128,14 @@ export function useAuditLog() { }) } + // API publique : on expose volontairement deux noms distincts pour les + // deux contrats (cache/no-cache). Aliaser `fetchLogs` vers la version + // cachee trompait les appelants : un consommateur qui destructurait + // `{ fetchLogs }` en pensant faire un appel neutre polluait en realite + // `lastCollection`, effet indetectable sans lire l'impl. return { lastCollection, - fetchLogs: fetchLogsCached, + fetchLogsCached, fetchLogById, fetchEntityLogs, fetchEntityTypes, diff --git a/frontend/tests/e2e/permissions/sidebar-visibility.spec.ts b/frontend/tests/e2e/permissions/sidebar-visibility.spec.ts index e7fc8d7..01323df 100644 --- a/frontend/tests/e2e/permissions/sidebar-visibility.spec.ts +++ b/frontend/tests/e2e/permissions/sidebar-visibility.spec.ts @@ -30,13 +30,14 @@ test.describe('Sidebar visibility', () => { await loginAs(context, persona.key) await page.goto('/') - // Attendre que la sidebar soit chargee (le middleware auth fetch /api/sidebar - // apres login). Les liens presents apparaissent alors ; les absents ne - // seront jamais attaches au DOM. - await page.waitForLoadState('networkidle') - const sidebar = new SidebarComponent(page) + // Attente semantique : on ancre sur un lien toujours present pour + // tout user authentifie (Mon compte > Tableau de bord). Remplace + // `networkidle` qui est reconnu instable en CI (SPAs avec polling + // ou HMR peuvent ne jamais quitter cet etat). + await expect(sidebar.accountDashboardLink()).toBeVisible({ timeout: 10000 }) + for (const link of ALL_ADMIN_LINKS) { const locator = sidebar.adminLink(link) const shouldBeVisible = persona.expectedAdminLinks.includes(link) @@ -66,10 +67,11 @@ test.describe('Sidebar visibility', () => { // dessus — ca bloquerait le logout de users sans permissions. await loginAs(context, 'user-nothing') await page.goto('/') - await page.waitForLoadState('networkidle') const sidebar = new SidebarComponent(page) - await expect(sidebar.accountDashboardLink()).toBeVisible() + // Meme strategie que ci-dessus : ancrage semantique plutot que + // `networkidle` pour eviter les faux timeouts en CI. + await expect(sidebar.accountDashboardLink()).toBeVisible({ timeout: 10000 }) await expect(sidebar.logoutLink()).toBeVisible() }) diff --git a/src/Module/Core/Infrastructure/Audit/AuditLogWriter.php b/src/Module/Core/Infrastructure/Audit/AuditLogWriter.php index 012e1de..152f506 100644 --- a/src/Module/Core/Infrastructure/Audit/AuditLogWriter.php +++ b/src/Module/Core/Infrastructure/Audit/AuditLogWriter.php @@ -85,14 +85,25 @@ final class AuditLogWriter * d'update : le listener prefiltre deja mais on garde cette garde * en defense-in-depth si un appelant direct oublie `#[AuditIgnore]`. * + * Recursion : parcourt les sous-tableaux (ex: changes structures + * `{field: {old, new}}`, snapshots avec relations imbriquees, ou + * payload arbitraire pousse par un appelant direct). + * * @param array $data * * @return array */ private function stripSensitive(array $data): array { - foreach (self::SENSITIVE_KEYS as $sensitiveKey) { - unset($data[$sensitiveKey]); + foreach ($data as $key => $value) { + if (in_array($key, self::SENSITIVE_KEYS, true)) { + unset($data[$key]); + + continue; + } + if (is_array($value)) { + $data[$key] = $this->stripSensitive($value); + } } return $data; diff --git a/tests/Module/Core/Api/PermissionApiTest.php b/tests/Module/Core/Api/PermissionApiTest.php index 7d66ffc..18fc34b 100644 --- a/tests/Module/Core/Api/PermissionApiTest.php +++ b/tests/Module/Core/Api/PermissionApiTest.php @@ -226,6 +226,51 @@ final class PermissionApiTest extends AbstractApiTestCase self::assertResponseIsSuccessful(); } + public function testNonAdminWithPermissionViewCanGetItem(): void + { + // Miroir item de testNonAdminWithPermissionViewCanListPermissions : + // la security expression OR est repliquee sur `new Get(...)` et doit + // donc aussi s'appliquer ici. + $permission = $this->getEm()->getRepository(Permission::class) + ->findOneBy(['code' => 'test.core.users.view']) + ; + self::assertNotNull($permission); + + $creds = $this->createUserWithPermission('core.permissions.view'); + $client = $this->authenticatedClient($creds['username'], $creds['password']); + $client->request('GET', '/api/permissions/'.$permission->getId()); + + self::assertResponseIsSuccessful(); + } + + public function testNonAdminWithUsersManageCanGetItem(): void + { + $permission = $this->getEm()->getRepository(Permission::class) + ->findOneBy(['code' => 'test.core.users.view']) + ; + self::assertNotNull($permission); + + $creds = $this->createUserWithPermission('core.users.manage'); + $client = $this->authenticatedClient($creds['username'], $creds['password']); + $client->request('GET', '/api/permissions/'.$permission->getId()); + + self::assertResponseIsSuccessful(); + } + + public function testNonAdminWithRolesManageCanGetItem(): void + { + $permission = $this->getEm()->getRepository(Permission::class) + ->findOneBy(['code' => 'test.core.users.view']) + ; + self::assertNotNull($permission); + + $creds = $this->createUserWithPermission('core.roles.manage'); + $client = $this->authenticatedClient($creds['username'], $creds['password']); + $client->request('GET', '/api/permissions/'.$permission->getId()); + + self::assertResponseIsSuccessful(); + } + private function cleanupTestPermissions(): void { $em = $this->getEm(); diff --git a/tests/Module/Core/Api/UserRbacSitesApiTest.php b/tests/Module/Core/Api/UserRbacSitesApiTest.php index 2595360..fe74668 100644 --- a/tests/Module/Core/Api/UserRbacSitesApiTest.php +++ b/tests/Module/Core/Api/UserRbacSitesApiTest.php @@ -174,6 +174,81 @@ final class UserRbacSitesApiTest extends AbstractApiTestCase self::assertSame('Chatellerault', $reloaded->getSites()->first()->getName()); } + public function testRbacPatchWithoutSitesKeyDoesNotAutoSwitchCurrentSiteWhenNull(): void + { + // Scenario : alice a des sites mais currentSite=null. Un PATCH /rbac + // qui ne touche PAS a la clef `sites` ne doit PAS auto-selectionner + // silencieusement un site via ensureCurrentSiteConsistency. + // + // Sans ce garde, un payload { "isAdmin": false } pourrait, si la + // denormalisation marque la collection `sites` dirty a tort (ou si + // la logique de detection se base sur PersistentCollection::isDirty() + // avant restauration), declencher ensureCurrentSiteConsistency et + // recaler currentSite sur sites->first() — ce qui est un effet de + // bord indetectable par le client. + $em = $this->getEm(); + $alice = $em->getRepository(User::class)->findOneBy(['username' => 'alice']); + $aliceId = $alice->getId(); + $alice->setCurrentSite(null); + $em->flush(); + $em->clear(); + + $client = $this->authenticatedClient('admin', 'admin'); + $client->request('PATCH', '/api/users/'.$aliceId.'/rbac', [ + 'headers' => ['Content-Type' => 'application/merge-patch+json'], + 'json' => ['isAdmin' => false], + ]); + + self::assertResponseIsSuccessful(); + + $em = $this->getEm(); + $em->clear(); + $reloaded = $em->getRepository(User::class)->find($aliceId); + self::assertNotNull($reloaded); + // currentSite doit rester null : PATCH /rbac sans clef `sites` ne doit + // pas muter la selection de site courant de l'user. + self::assertNull( + $reloaded->getCurrentSite(), + 'PATCH /rbac sans clef `sites` ne doit pas auto-selectionner un site.', + ); + // Les sites eux-memes doivent etre preserves. + self::assertCount(1, $reloaded->getSites()); + + $this->restoreAliceSites(); + } + + public function testRbacPatchWithoutSitesKeyDoesNotRequireSitesManagePermission(): void + { + // Scenario Codex : un user non-admin qui porte `core.users.manage` + // mais PAS `sites.manage` doit pouvoir PATCHer /rbac sans clef `sites` + // sans se faire refuser l'acces. + // + // Si sitesWereMutated se base uniquement sur PersistentCollection::isDirty() + // calcule avant restoreAbsentCollections, une denormalisation qui + // marque a tort la collection dirty lorsque la clef est absente du + // payload lancerait un 403 parasite. La source de verite doit etre + // la presence de la clef dans le payload JSON. + $this->skipIfSitesModuleDisabled(); + + $em = $this->getEm(); + $alice = $em->getRepository(User::class)->findOneBy(['username' => 'alice']); + $aliceId = $alice->getId(); + $em->clear(); + + // User jetable : core.users.manage uniquement (pas sites.manage). + $creds = $this->createUserWithPermission('core.users.manage'); + $client = $this->authenticatedClient($creds['username'], $creds['password']); + + $client->request('PATCH', '/api/users/'.$aliceId.'/rbac', [ + 'headers' => ['Content-Type' => 'application/merge-patch+json'], + 'json' => ['isAdmin' => false], + ]); + + // Pas de 403 : la requete ne touche pas aux sites, sites.manage n'est + // pas requis. + self::assertResponseIsSuccessful(); + } + /** * Remet alice dans l'etat des fixtures : un seul site Chatellerault, * currentSite Chatellerault. Evite la pollution inter-tests. diff --git a/tests/Module/Core/Infrastructure/Audit/AuditLogWriterTest.php b/tests/Module/Core/Infrastructure/Audit/AuditLogWriterTest.php index 9932540..627e733 100644 --- a/tests/Module/Core/Infrastructure/Audit/AuditLogWriterTest.php +++ b/tests/Module/Core/Infrastructure/Audit/AuditLogWriterTest.php @@ -111,6 +111,33 @@ final class AuditLogWriterTest extends TestCase $this->assertSame('bob@example.com', $changes['email']); } + public function testStripsSensitiveKeysRecursively(): void + { + // Defense-in-depth : un appelant direct peut passer un payload + // imbrique (ex: relations embarquees). Les cles sensibles doivent + // etre supprimees a tous les niveaux de profondeur. + $security = $this->buildSecurityWithUser('alice'); + $writer = new AuditLogWriter($this->connection, $security, $this->requestStack, $this->requestIdProvider); + + $writer->log('core.User', '1', 'create', [ + 'username' => 'bob', + 'profile' => [ + 'email' => 'bob@example.com', + 'password' => 'leaked_in_nested', + 'nested' => [ + 'token' => 'should_be_stripped', + 'harmless' => 'kept', + ], + ], + ]); + + $changes = $this->capturedInsert[1]['changes']; + $this->assertArrayNotHasKey('password', $changes['profile']); + $this->assertArrayNotHasKey('token', $changes['profile']['nested']); + $this->assertSame('kept', $changes['profile']['nested']['harmless']); + $this->assertSame('bob@example.com', $changes['profile']['email']); + } + public function testCapturesIpAddressWhenRequestPresent(): void { $request = Request::create('/api/users', 'POST');