From 133f205393791c15aa7e33aaa538569afbde10d3 Mon Sep 17 00:00:00 2001 From: Matthieu Date: Fri, 26 Jun 2026 16:14:11 +0200 Subject: [PATCH] =?UTF-8?q?test(user)=20:=20couvre=20le=20soft-delete=20+?= =?UTF-8?q?=20d=C3=A9sarchivage=20admin=20et=20corrige=20les=20retours=20d?= =?UTF-8?q?e=20review?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - ajoute des tests fonctionnels (archive au DELETE, exclusion de la collection, listing/désarchivage admin, anti-auto-archivage) et un test unitaire du ArchivedUserChecker - expose un filtre BooleanFilter `archived` + bypass admin dans ExcludeArchivedUserExtension pour lister les archivés (?archived=true) - rend `archived` modifiable par un admin (groupe user:write + ApiProperty ROLE_ADMIN) → désarchivage possible via PATCH /api/users/{id} - RestoreMissingUsersCommand : ne compte que les insertions réelles (ON CONFLICT DO NOTHING n'est plus comptabilisé à tort) - relève memory_limit des tests à 512M (boot sérialiseur API Platform) --- phpunit.dist.xml | 4 + src/Command/RestoreMissingUsersCommand.php | 12 +- src/Module/Core/Domain/Entity/User.php | 8 +- .../ExcludeArchivedUserExtension.php | 17 +- .../Module/Core/UserArchiveApiTest.php | 145 ++++++++++++++++++ .../Security/ArchivedUserCheckerTest.php | 43 ++++++ 6 files changed, 224 insertions(+), 5 deletions(-) create mode 100644 tests/Functional/Module/Core/UserArchiveApiTest.php create mode 100644 tests/Unit/Module/Core/Infrastructure/Security/ArchivedUserCheckerTest.php diff --git a/phpunit.dist.xml b/phpunit.dist.xml index 817239b..d90d6fb 100644 --- a/phpunit.dist.xml +++ b/phpunit.dist.xml @@ -13,6 +13,10 @@ + + diff --git a/src/Command/RestoreMissingUsersCommand.php b/src/Command/RestoreMissingUsersCommand.php index 9688392..7a0e3e6 100644 --- a/src/Command/RestoreMissingUsersCommand.php +++ b/src/Command/RestoreMissingUsersCommand.php @@ -102,7 +102,7 @@ final class RestoreMissingUsersCommand extends Command $created = 0; foreach ($missingIds as $id) { - $this->connection->executeStatement( + $inserted = $this->connection->executeStatement( <<<'SQL' INSERT INTO "user" (id, username, first_name, last_name, roles, password, created_at, @@ -122,8 +122,14 @@ final class RestoreMissingUsersCommand extends Command 'password' => $hash, ], ); - ++$created; - $io->writeln(sprintf(' ✓ user #%d recreated (archived)', $id)); + + // ON CONFLICT may have skipped an already-present row — only count real inserts. + if ($inserted > 0) { + ++$created; + $io->writeln(sprintf(' ✓ user #%d recreated (archived)', $id)); + } else { + $io->writeln(sprintf(' • user #%d already present — skipped', $id)); + } } $io->success(sprintf('%d user(s) restored as archived. References are valid again — no data lost.', $created)); diff --git a/src/Module/Core/Domain/Entity/User.php b/src/Module/Core/Domain/Entity/User.php index 2b9ccca..e0a8d1b 100644 --- a/src/Module/Core/Domain/Entity/User.php +++ b/src/Module/Core/Domain/Entity/User.php @@ -4,6 +4,8 @@ declare(strict_types=1); namespace App\Module\Core\Domain\Entity; +use ApiPlatform\Doctrine\Orm\Filter\BooleanFilter; +use ApiPlatform\Metadata\ApiFilter; use ApiPlatform\Metadata\ApiProperty; use ApiPlatform\Metadata\ApiResource; use ApiPlatform\Metadata\Delete; @@ -64,6 +66,9 @@ use Symfony\Component\Serializer\Attribute\Groups; ], denormalizationContext: ['groups' => ['user:write']], )] +// Archived users are hidden from the default /users collection by +// ExcludeArchivedUserExtension; an admin can still list them with ?archived=true. +#[ApiFilter(BooleanFilter::class, properties: ['archived'])] #[Auditable] #[ORM\Entity(repositoryClass: DoctrineUserRepository::class)] #[ORM\Table(name: '`user`')] @@ -118,7 +123,8 @@ class User implements UserInterface, PasswordAuthenticatedUserInterface, SharedU * from selectable user lists. */ #[ORM\Column(options: ['default' => false])] - #[Groups(['me:read', 'user:list'])] + #[ApiProperty(security: "is_granted('ROLE_ADMIN')")] + #[Groups(['me:read', 'user:list', 'user:write'])] private bool $archived = false; // --- HR / absence management fields (readable only by an admin or the user themselves) --- diff --git a/src/Module/Core/Infrastructure/ApiPlatform/Extension/ExcludeArchivedUserExtension.php b/src/Module/Core/Infrastructure/ApiPlatform/Extension/ExcludeArchivedUserExtension.php index 85d063a..a3b27a6 100644 --- a/src/Module/Core/Infrastructure/ApiPlatform/Extension/ExcludeArchivedUserExtension.php +++ b/src/Module/Core/Infrastructure/ApiPlatform/Extension/ExcludeArchivedUserExtension.php @@ -9,14 +9,23 @@ use ApiPlatform\Doctrine\Orm\Util\QueryNameGeneratorInterface; use ApiPlatform\Metadata\Operation; use App\Module\Core\Domain\Entity\User; use Doctrine\ORM\QueryBuilder; +use Symfony\Bundle\SecurityBundle\Security; + +use function array_key_exists; /** * Hides archived (soft-deleted) users from the `/users` collection so they are * no longer offered as assignees/collaborators, while existing references to * them (already stored on tasks, time entries…) keep resolving normally. + * + * An admin can opt back in to see archived users — e.g. to restore one — by + * passing the `archived` query filter explicitly (`?archived=true`), in which + * case the BooleanFilter declared on User handles the predicate instead. */ -final class ExcludeArchivedUserExtension implements QueryCollectionExtensionInterface +final readonly class ExcludeArchivedUserExtension implements QueryCollectionExtensionInterface { + public function __construct(private Security $security) {} + public function applyToCollection( QueryBuilder $queryBuilder, QueryNameGeneratorInterface $queryNameGenerator, @@ -28,6 +37,12 @@ final class ExcludeArchivedUserExtension implements QueryCollectionExtensionInte return; } + // Let an admin explicitly query archived users via ?archived=... + $filters = $context['filters'] ?? []; + if (array_key_exists('archived', $filters) && $this->security->isGranted('ROLE_ADMIN')) { + return; + } + $alias = $queryBuilder->getRootAliases()[0]; $queryBuilder->andWhere(sprintf('%s.archived = false', $alias)); } diff --git a/tests/Functional/Module/Core/UserArchiveApiTest.php b/tests/Functional/Module/Core/UserArchiveApiTest.php new file mode 100644 index 0000000..5d0de72 --- /dev/null +++ b/tests/Functional/Module/Core/UserArchiveApiTest.php @@ -0,0 +1,145 @@ +get(EntityManagerInterface::class); + $target = $this->createUser($em, 'archive-target-'.uniqid()); + $em->flush(); + $targetId = $target->getId(); + + $this->loginAdmin($client); + $client->request('DELETE', '/api/users/'.$targetId); + + self::assertResponseStatusCodeSame(204); + + $em->clear(); + $reloaded = $em->getRepository(User::class)->find($targetId); + self::assertInstanceOf(User::class, $reloaded, 'Row must still exist (soft delete)'); + self::assertTrue($reloaded->isArchived(), 'User must be flagged archived'); + self::assertNull($reloaded->getApiToken(), 'API token must be cleared on archive'); + } + + public function testAdminCannotArchiveOwnAccount(): void + { + $client = self::createClient(); + $em = self::getContainer()->get(EntityManagerInterface::class); + $this->loginAdmin($client); + $adminId = $this->userId('admin'); + + $client->request('DELETE', '/api/users/'.$adminId); + + self::assertResponseStatusCodeSame(403); + + $em->clear(); + $admin = $em->getRepository(User::class)->find($adminId); + self::assertFalse($admin->isArchived(), 'Admin must not have archived itself'); + } + + public function testArchivedUserIsHiddenFromDefaultCollection(): void + { + $client = self::createClient(); + $username = $this->createArchivedUser(); + + $this->loginAdmin($client); + $client->request('GET', '/api/users', server: ['HTTP_ACCEPT' => 'application/json']); + + self::assertResponseIsSuccessful(); + $usernames = array_column(json_decode($client->getResponse()->getContent(), true), 'username'); + self::assertNotContains($username, $usernames, 'Archived user must not appear in the default list'); + } + + public function testAdminCanListArchivedUsersViaFilter(): void + { + $client = self::createClient(); + $username = $this->createArchivedUser(); + + $this->loginAdmin($client); + $client->request('GET', '/api/users?archived=true', server: ['HTTP_ACCEPT' => 'application/json']); + + self::assertResponseIsSuccessful(); + $usernames = array_column(json_decode($client->getResponse()->getContent(), true), 'username'); + self::assertContains($username, $usernames, 'Admin must be able to list archived users via ?archived=true'); + } + + public function testAdminCanRestoreUserViaPatch(): void + { + $client = self::createClient(); + $em = self::getContainer()->get(EntityManagerInterface::class); + + $user = $this->createUser($em, 'restore-target-'.uniqid()); + $user->setArchived(true); + $em->flush(); + $userId = $user->getId(); + $em->clear(); + + $this->loginAdmin($client); + $client->request('PATCH', '/api/users/'.$userId, server: [ + 'CONTENT_TYPE' => 'application/merge-patch+json', + ], content: json_encode(['archived' => false])); + + self::assertResponseIsSuccessful(); + + $em->clear(); + $reloaded = $em->getRepository(User::class)->find($userId); + self::assertFalse($reloaded->isArchived(), 'Admin PATCH must be able to un-archive a user'); + } + + private function createArchivedUser(): string + { + $em = self::getContainer()->get(EntityManagerInterface::class); + $username = 'archived-'.uniqid(); + $user = $this->createUser($em, $username); + $user->setArchived(true); + $em->flush(); + $em->clear(); + + return $username; + } + + private function createUser(EntityManagerInterface $em, string $username): User + { + $user = new User(); + $user->setUsername($username); + $user->setPassword('x'); + $user->setRoles(['ROLE_USER']); + $em->persist($user); + + return $user; + } + + private function loginAdmin(KernelBrowser $client): void + { + $em = self::getContainer()->get(EntityManagerInterface::class); + $user = $em->getRepository(User::class)->findOneBy(['username' => 'admin']); + self::assertInstanceOf(User::class, $user); + $client->loginUser($user); + } + + private function userId(string $username): int + { + $em = self::getContainer()->get(EntityManagerInterface::class); + $user = $em->getRepository(User::class)->findOneBy(['username' => $username]); + self::assertInstanceOf(User::class, $user); + + return $user->getId(); + } +} diff --git a/tests/Unit/Module/Core/Infrastructure/Security/ArchivedUserCheckerTest.php b/tests/Unit/Module/Core/Infrastructure/Security/ArchivedUserCheckerTest.php new file mode 100644 index 0000000..8700e1f --- /dev/null +++ b/tests/Unit/Module/Core/Infrastructure/Security/ArchivedUserCheckerTest.php @@ -0,0 +1,43 @@ +setArchived(true); + + $this->expectException(CustomUserMessageAccountStatusException::class); + + new ArchivedUserChecker()->checkPreAuth($user); + } + + public function testActiveUserPassesPreAuth(): void + { + $user = new User()->setArchived(false); + + new ArchivedUserChecker()->checkPreAuth($user); + + $this->addToAssertionCount(1); + } + + public function testNonAppUserIsIgnored(): void + { + // A user that is not our entity must not be rejected by this checker. + new ArchivedUserChecker()->checkPreAuth(new InMemoryUser('someone', null)); + + $this->addToAssertionCount(1); + } +}