MALIO-DEV/Coltura
3
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(users) : corrige l'affichage et l'ecrasement des sites sur le drawer RBAC

Browse Source 
Le drawer RBAC de /admin/users initialisait l'etat des sites a partir du payload
/api/users (groupe user:list) qui n'expose pas la collection sites. Consequence :
la section "Sites autorises" affichait toujours 0 case cochee, et la sauvegarde
ecrasait silencieusement les sites existants en BDD.

- Ajout d'une operation GET /users/{id}/rbac (groupe user:rbac:read) dediee au
  chargement du detail pour l'edition : payload list reste leger, detail riche
  sur une URI symetrique au PATCH existant.
- Drawer charge desormais GET /users/{id}/rbac pour initialiser sites, roles
  et directPermissions ; UserListItem ne contient plus sites (inutilise).
- Colonne "Sites" retiree de la table /admin/users : l'info est consultee via
  le drawer, pas la liste (evite aussi la fuite cross-site pour les users avec
  core.users.view mais sans sites.bypass_scope).
- Garde anti-ecrasement dans UserRbacProcessor : respect de la semantique
  merge-patch+json (cle absente = preservee, cle = [] = vidage explicite).
  Restaure les collections ManyToMany absentes du payload a partir du snapshot
  Doctrine. Couvre roles, directPermissions et sites.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Matthieu
2026-04-22 11:17:40 +02:00
parent 6db955f65c
commit 617ee314b3
6 changed files with 168 additions and 45 deletions
Download Patch File Download Diff File Expand all files Collapse all files

49
frontend/modules/core/components/UserRbacDrawer.vue
View File

@@ -112,7 +112,7 @@
</template>
<script setup lang="ts">
import type { Permission, Role, UserListItem, EffectivePermission } from '~/shared/types/rbac'
import type { Permission, Role, UserListItem, UserRbacDetail, EffectivePermission } from '~/shared/types/rbac'
import type { Site } from '~/shared/types/sites'
interface PermissionModule {
@@ -206,39 +206,44 @@ const effectivePermissions = computed<EffectivePermission[]>(() => {
.sort((a, b) => a.code.localeCompare(b.code))
})
// Charger roles, permissions et sites en parallele pour minimiser le TTFB
// a l'ouverture du drawer.
async function loadData() {
const [rolesData, permsData, sitesData] = await Promise.all([
// Charger les referentiels (roles, permissions, sites) + le detail RBAC du user
// en parallele pour minimiser le TTFB a l'ouverture du drawer.
// 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<UserRbacDetail>(`/users/${userId}/rbac`, {}, { toast: false }),
])
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))
}
// Remplir le formulaire quand le user change
watch(() => props.user, (user) => {
if (user) {
form.value.isAdmin = user.isAdmin
selectedRoleIds.value = new Set(user.roles.map(iriToId))
selectedDirectPermissionIds.value = new Set(user.directPermissions.map(iriToId))
selectedSiteIds.value = new Set((user.sites ?? []).map(iriToId))
} else {
form.value.isAdmin = false
selectedRoleIds.value = new Set()
selectedDirectPermissionIds.value = new Set()
selectedSiteIds.value = new Set()
function resetForm() {
form.value.isAdmin = false
selectedRoleIds.value = new Set()
selectedDirectPermissionIds.value = new Set()
selectedSiteIds.value = new Set()
}
// Recharger a l'ouverture OU quand le user change pendant que le drawer est ouvert.
// Le watch combine evite un double fetch si les deux changent dans le meme tick.
watch([() => props.modelValue, () => props.user?.id], ([open, userId]) => {
if (open && userId) {
loadData(userId)
} else if (!open) {
resetForm()
}
}, { immediate: true })
// Charger les donnees quand le drawer s'ouvre
watch(() => props.modelValue, (open) => {
if (open) loadData()
})
function toggleRole(id: number, selected: boolean) {
const ids = new Set(selectedRoleIds.value)
if (selected) ids.add(id)

27
frontend/modules/core/pages/admin/users.vue
View File

@@ -38,7 +38,6 @@
<script setup lang="ts">
import type { UserListItem } from '~/shared/types/rbac'
import type { Site } from '~/shared/types/sites'
const { t } = useI18n()
const api = useApi()
@@ -49,24 +48,21 @@ useHead({ title: t('admin.users.title') })
const canManage = computed(() => can('core.users.manage'))
const users = ref<UserListItem[]>([])
const sitesById = ref(new Map<number, Site>())
const loading = ref(false)
const drawerOpen = ref(false)
const selectedUser = ref<UserListItem | null>(null)
// La colonne "Sites" n'est plus affichee dans la liste : le detail des sites
// rattaches est consulte/edite via le drawer (GET /users/{id}/rbac). Garder
// un payload leger sur /api/users facilite la pagination et evite de fuiter
// l'info cross-site aux users partageant juste un site avec l'appelant.
const columns = [
{ key: 'username', label: t('admin.users.table.username') },
{ key: 'admin', label: t('admin.users.table.admin') },
{ key: 'roles', label: t('admin.users.table.roles') },
{ key: 'directPermissions', label: t('admin.users.table.directPermissions') },
{ key: 'sites', label: t('admin.users.table.sites') },
]
// Extraire l'id numerique depuis une IRI API Platform type `/api/sites/3`.
function iriToId(iri: string): number {
return Number(iri.split('/').pop())
}
const userItems = computed(() =>
users.value.map(user => ({
id: user.id,
@@ -74,27 +70,14 @@ const userItems = computed(() =>
admin: user.isAdmin,
roles: user.roles.length,
directPermissions: user.directPermissions.length,
// Affichage : liste des noms de sites separes par virgule. Les IRIs
// du payload /api/users (groupe user:list) sont resolues via la Map
// construite en parallele depuis /api/sites.
sites: (user.sites ?? [])
.map(iri => sitesById.value.get(iriToId(iri))?.name)
.filter((name): name is string => Boolean(name))
.join(', '),
})),
)
async function loadUsers() {
loading.value = true
try {
// Chargement parallele : les sites alimentent la Map de resolution
// IRI→name pour la colonne "Sites" de la table.
const [usersData, sitesData] = await Promise.all([
api.get<{ member: UserListItem[] }>('/users', {}, { toast: false }),
api.get<{ member: Site[] }>('/sites', { itemsPerPage: 999 }, { toast: false }),
])
const usersData = await api.get<{ member: UserListItem[] }>('/users', {}, { toast: false })
users.value = usersData.member
sitesById.value = new Map(sitesData.member.map(s => [s.id, s]))
} finally {
loading.value = false
}

14
frontend/shared/types/rbac.ts
View File

@@ -21,7 +21,19 @@ export interface UserListItem {
isAdmin: boolean
roles: string[]
directPermissions: string[]
/** IRIs des sites autorises (ticket 2 module Sites). */
}
/**
* Detail RBAC d'un user, renvoye par GET /api/users/{id}/rbac (groupe user:rbac:read).
* Utilise par UserRbacDrawer pour initialiser son formulaire avec l'etat complet
* (sites inclus). Le endpoint de liste /api/users reste volontairement leger et
* n'expose pas ces champs.
*/
export interface UserRbacDetail {
id: number
isAdmin: boolean
roles: string[]
directPermissions: string[]
sites: string[]
}

10
src/Module/Core/Domain/Entity/User.php
View File

@@ -51,6 +51,16 @@ use Symfony\Component\Serializer\Attribute\SerializedName;
),
new Post(security: "is_granted('core.users.manage')", processor: UserPasswordHasherProcessor::class),
new Patch(security: "is_granted('core.users.manage')", processor: UserPasswordHasherProcessor::class),
// Lecture dediee au drawer d'edition RBAC : meme URI que le PATCH pour une
// API symetrique, groupe `user:rbac:read` qui expose sites/roles/directPermissions.
// Garde `core.users.manage` (pas `.view`) car c'est l'endpoint de detail prevu
// pour l'edition, pas la consultation generale (elle passe par GET /users/{id}).
new Get(
name: 'user_rbac_get',
uriTemplate: '/users/{id}/rbac',
security: "is_granted('core.users.manage')",
normalizationContext: ['groups' => ['user:rbac:read']],
),
new Patch(
name: 'user_rbac_patch',
uriTemplate: '/users/{id}/rbac',

103
src/Module/Core/Infrastructure/ApiPlatform/State/Processor/UserRbacProcessor.php
View File

@@ -9,11 +9,13 @@ use ApiPlatform\State\ProcessorInterface;
use App\Module\Core\Domain\Entity\User;
use App\Module\Core\Domain\Exception\LastAdminProtectionException;
use App\Module\Core\Domain\Security\AdminHeadcountGuardInterface;
use Doctrine\Common\Collections\Collection;
use Doctrine\ORM\EntityManagerInterface;
use Doctrine\ORM\PersistentCollection;
use LogicException;
use Symfony\Bundle\SecurityBundle\Security;
use Symfony\Component\DependencyInjection\Attribute\Autowire;
use Symfony\Component\HttpFoundation\RequestStack;
use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
use Symfony\Component\HttpKernel\Exception\BadRequestHttpException;
@@ -51,12 +53,31 @@ use Symfony\Component\HttpKernel\Exception\BadRequestHttpException;
*/
final class UserRbacProcessor implements ProcessorInterface
{
/**
* Mapping cle-payload → (property-path PHP, accesseur, setter utilise pour
* reattacher les items lors de la restauration). Permet au gardefou
* anti-ecrasement de savoir quelles collections restaurer si elles sont
* absentes du payload JSON.
*
* Note : la cle JSON "roles" correspond a la propriete PHP `rbacRoles`
* (renommee via #[SerializedName] pour eviter la collision avec
* UserInterface::getRoles()).
*
* @var array<string, array{getter: string, remover: string, adder: string}>
*/
private const array COLLECTION_MAP = [
'roles' => ['getter' => 'getRbacRoles', 'remover' => 'removeRbacRole', 'adder' => 'addRbacRole'],
'directPermissions' => ['getter' => 'getDirectPermissions', 'remover' => 'removeDirectPermission', 'adder' => 'addDirectPermission'],
'sites' => ['getter' => 'getSites', 'remover' => 'removeSite', 'adder' => 'addSite'],
];
public function __construct(
#[Autowire(service: 'api_platform.doctrine.orm.state.persist_processor')]
private readonly ProcessorInterface $persistProcessor,
private readonly EntityManagerInterface $entityManager,
private readonly Security $security,
private readonly AdminHeadcountGuardInterface $adminHeadcountGuard,
private readonly RequestStack $requestStack,
) {}
public function process(mixed $data, Operation $operation, array $uriVariables = [], array $context = []): mixed
@@ -72,6 +93,19 @@ final class UserRbacProcessor implements ProcessorInterface
));
}
// Garde anti-ecrasement (defense in depth) : PATCH merge-patch+json impose
// que les cles absentes du payload ne mutent PAS les proprietes
// correspondantes. La denormalisation API Platform ne respecte pas cet
// invariant pour les collections ManyToMany — elle reinstancie une
// ArrayCollection vide des que la cle n'est pas presente. Sans cette
// garde, un client qui PATCHe juste `{ "isAdmin": true }` verrait toutes
// ses roles/directPermissions/sites detruits.
//
// On lit le body brut de la requete pour connaitre les cles envoyees,
// puis on restaure les collections absentes a partir de l'etat d'origine
// charge par Doctrine (snapshot des PersistentCollection).
$this->restoreAbsentCollections($data);
$currentUser = $this->security->getUser();
// Calcul partage entre les deux gardes : l'user perdait-il le flag admin ?
@@ -180,4 +214,73 @@ final class UserRbacProcessor implements ProcessorInterface
$this->entityManager->flush();
}
}
/**
* Pour chaque collection RBAC (roles, directPermissions, sites) absente du
* payload JSON, restaure l'etat d'origine a partir du snapshot Doctrine et
* marque la collection comme non-dirty. Idempotent : si la cle est presente
* dans le payload, no-op (la denormalisation fait foi).
*
* Cas d'usage : un client qui PATCHe partiellement (`{ "isAdmin": true }`)
* ne doit pas voir ses autres collections reinitialisees. API Platform
* reinstancie par defaut une collection vide pour les cles absentes, ce
* qui casse la semantique de merge-patch+json.
*
* Pas de fallback si la collection d'origine n'est pas une PersistentCollection
* (ex: User fraichement construit) : dans ce cas aucune restauration n'est
* possible puisqu'il n'y a pas d'etat persiste a restaurer.
*/
private function restoreAbsentCollections(User $user): void
{
$request = $this->requestStack->getCurrentRequest();
if (null === $request) {
return;
}
$rawBody = $request->getContent();
if ('' === $rawBody) {
return;
}
/** @var null|array<string, mixed> $payload */
$payload = json_decode($rawBody, true);
if (!is_array($payload)) {
return;
}
foreach (self::COLLECTION_MAP as $jsonKey => $accessors) {
if (array_key_exists($jsonKey, $payload)) {
continue;
}
/** @var Collection<int, object> $currentCollection */
$currentCollection = $user->{$accessors['getter']}();
if (!$currentCollection instanceof PersistentCollection) {
continue;
}
// Snapshot = etat charge depuis la BDD avant denormalisation.
// On restaure en retirant les items actuels et en ajoutant les
// originaux via l'adder/remover pour que les collections inverses
// (ex: Site::users) restent coherentes.
$snapshot = $currentCollection->getSnapshot();
foreach ($currentCollection->toArray() as $currentItem) {
if (!in_array($currentItem, $snapshot, true)) {
$user->{$accessors['remover']}($currentItem);
}
}
foreach ($snapshot as $originalItem) {
if (!$currentCollection->contains($originalItem)) {
$user->{$accessors['adder']}($originalItem);
}
}
// Marquer comme non-dirty pour que Doctrine ne detecte pas de diff
// et n'emette pas de requete UPDATE inutile sur la table de jointure.
$currentCollection->takeSnapshot();
}
}
}

10
tests/Module/Core/Infrastructure/ApiPlatform/State/Processor/UserRbacProcessorTest.php
View File

@@ -21,6 +21,8 @@ use PHPUnit\Framework\TestCase;
use ReflectionClass;
use stdClass;
use Symfony\Bundle\SecurityBundle\Security;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\RequestStack;
use Symfony\Component\HttpKernel\Exception\BadRequestHttpException;
/**
@@ -38,6 +40,7 @@ final class UserRbacProcessorTest extends TestCase
private MockObject&UnitOfWork $unitOfWork;
private MockObject&Security $security;
private AdminHeadcountGuardInterface&MockObject $adminHeadcountGuard;
private RequestStack $requestStack;
private UserRbacProcessor $processor;
protected function setUp(): void
@@ -48,6 +51,12 @@ final class UserRbacProcessorTest extends TestCase
$this->security = $this->createMock(Security::class);
$this->adminHeadcountGuard = $this->createMock(AdminHeadcountGuardInterface::class);
// Request vide par defaut pour les tests existants : la garde
// anti-ecrasement (restoreAbsentCollections) no-op quand le body est ''
// donc elle n'interfere pas avec les assertions deja en place.
$this->requestStack = new RequestStack();
$this->requestStack->push(new Request());
$this->entityManager->method('getUnitOfWork')->willReturn($this->unitOfWork);
// wrapInTransaction doit executer reellement la closure pour que le
@@ -63,6 +72,7 @@ final class UserRbacProcessorTest extends TestCase
$this->entityManager,
$this->security,
$this->adminHeadcountGuard,
$this->requestStack,
);
}