fix(security) : harden ROLE_CLIENT isolation + tighten cross-module contracts

Findings from the post-migration code review. The arrival of ROLE_CLIENT
exposed internal endpoints still guarded only by IS_AUTHENTICATED_FULLY (or no
security), reachable by a client. Verified by re-running a multi-role smoke
test (client -> 403, internal roles -> 200).

Security (closed real client-isolation holes):
- TaskDocumentDownloadController: add ownership check (admin all / client only
  own clientTicket docs / user only task-linked docs) — the custom download
  bypassed the cloistered provider.
- Share browse/download/search/status controllers: IS_AUTHENTICATED_FULLY ->
  ROLE_USER (SMB share is internal).
- User Get/GetCollection: add security ROLE_USER (was exposing the internal
  directory to clients).
- BookStackLink GetCollection/Post/Delete: IS_AUTHENTICATED_FULLY -> ROLE_USER.

Contracts / robustness:
- TaskInterface gains getProject(): ?ProjectInterface; TimeTracking export
  controller/service drop concrete cross-module entities for repo interfaces.
- Shared MCP Serializer signatures widened to the contracts (user/projectRef/
  taskRef/tags/users); project()/userFull()/etc. kept concrete (use getters
  outside the contracts).
- RecurrenceHandler: null-guard before findMaxNumberByProjectForUpdate().

180 tests green, cs-fixer clean, routes unchanged.
This commit is contained in:
Matthieu
2026-06-21 19:31:09 +02:00
parent da3d190216
commit 96ef1bf436
14 changed files with 84 additions and 27 deletions
+2
View File
@@ -39,10 +39,12 @@ use Symfony\Component\Serializer\Attribute\Groups;
normalizationContext: ['groups' => ['me:read']], normalizationContext: ['groups' => ['me:read']],
), ),
new Get( new Get(
security: "is_granted('ROLE_USER')",
normalizationContext: ['groups' => ['user:list']], normalizationContext: ['groups' => ['user:list']],
), ),
new GetCollection( new GetCollection(
paginationEnabled: false, paginationEnabled: false,
security: "is_granted('ROLE_USER')",
normalizationContext: ['groups' => ['user:list']], normalizationContext: ['groups' => ['user:list']],
), ),
new Post(security: "is_granted('ROLE_ADMIN')", processor: UserPasswordHasherProcessor::class), new Post(security: "is_granted('ROLE_ADMIN')", processor: UserPasswordHasherProcessor::class),
@@ -11,6 +11,13 @@ interface UserRepositoryInterface
{ {
public function findById(int $id): ?UserInterface; public function findById(int $id): ?UserInterface;
/**
* @param int[] $ids
*
* @return list<UserInterface>
*/
public function findByIds(array $ids): array;
/** /**
* @return list<UserInterface> * @return list<UserInterface>
*/ */
@@ -26,6 +26,25 @@ class DoctrineUserRepository extends ServiceEntityRepository implements UserRepo
return $this->find($id); return $this->find($id);
} }
/**
* @param int[] $ids
*
* @return list<UserInterface>
*/
public function findByIds(array $ids): array
{
if ([] === $ids) {
return [];
}
return $this->createQueryBuilder('u')
->where('u.id IN (:ids)')
->setParameter('ids', $ids)
->getQuery()
->getResult()
;
}
/** /**
* @return list<UserInterface> * @return list<UserInterface>
*/ */
@@ -24,7 +24,7 @@ use Symfony\Component\Serializer\Attribute\Groups;
], ],
normalizationContext: ['groups' => ['bookstack_link:read']], normalizationContext: ['groups' => ['bookstack_link:read']],
provider: BookStackLinkProvider::class, provider: BookStackLinkProvider::class,
security: "is_granted('IS_AUTHENTICATED_FULLY')", security: "is_granted('ROLE_USER')",
), ),
new Post( new Post(
uriTemplate: '/tasks/{taskId}/bookstack/links', uriTemplate: '/tasks/{taskId}/bookstack/links',
@@ -35,7 +35,7 @@ use Symfony\Component\Serializer\Attribute\Groups;
normalizationContext: ['groups' => ['bookstack_link:read']], normalizationContext: ['groups' => ['bookstack_link:read']],
provider: BookStackLinkProvider::class, provider: BookStackLinkProvider::class,
processor: BookStackLinkProcessor::class, processor: BookStackLinkProcessor::class,
security: "is_granted('IS_AUTHENTICATED_FULLY')", security: "is_granted('ROLE_USER')",
), ),
new Delete( new Delete(
uriTemplate: '/tasks/{taskId}/bookstack/links/{id}', uriTemplate: '/tasks/{taskId}/bookstack/links/{id}',
@@ -45,7 +45,7 @@ use Symfony\Component\Serializer\Attribute\Groups;
], ],
provider: BookStackLinkProvider::class, provider: BookStackLinkProvider::class,
processor: BookStackLinkProcessor::class, processor: BookStackLinkProcessor::class,
security: "is_granted('IS_AUTHENTICATED_FULLY')", security: "is_granted('ROLE_USER')",
), ),
], ],
)] )]
@@ -24,7 +24,7 @@ class ShareBrowseController extends AbstractController
) {} ) {}
#[Route('/api/share/browse', name: 'share_browse', methods: ['GET'], priority: 1)] #[Route('/api/share/browse', name: 'share_browse', methods: ['GET'], priority: 1)]
#[IsGranted('IS_AUTHENTICATED_FULLY')] #[IsGranted('ROLE_USER')]
public function __invoke(Request $request): JsonResponse public function __invoke(Request $request): JsonResponse
{ {
$rawPath = (string) $request->query->get('path', ''); $rawPath = (string) $request->query->get('path', '');
@@ -29,7 +29,7 @@ class ShareDownloadController extends AbstractController
) {} ) {}
#[Route('/api/share/download', name: 'share_download', methods: ['GET'], priority: 1)] #[Route('/api/share/download', name: 'share_download', methods: ['GET'], priority: 1)]
#[IsGranted('IS_AUTHENTICATED_FULLY')] #[IsGranted('ROLE_USER')]
public function __invoke(Request $request): Response public function __invoke(Request $request): Response
{ {
$rawPath = (string) $request->query->get('path', ''); $rawPath = (string) $request->query->get('path', '');
@@ -24,7 +24,7 @@ class ShareSearchController extends AbstractController
) {} ) {}
#[Route('/api/share/search', name: 'share_search', methods: ['GET'], priority: 1)] #[Route('/api/share/search', name: 'share_search', methods: ['GET'], priority: 1)]
#[IsGranted('IS_AUTHENTICATED_FULLY')] #[IsGranted('ROLE_USER')]
public function __invoke(Request $request): JsonResponse public function __invoke(Request $request): JsonResponse
{ {
$query = trim((string) $request->query->get('q', '')); $query = trim((string) $request->query->get('q', ''));
@@ -17,7 +17,7 @@ class ShareStatusController extends AbstractController
) {} ) {}
#[Route('/api/share/status', name: 'share_status', methods: ['GET'], priority: 1)] #[Route('/api/share/status', name: 'share_status', methods: ['GET'], priority: 1)]
#[IsGranted('IS_AUTHENTICATED_FULLY')] #[IsGranted('ROLE_USER')]
public function __invoke(): JsonResponse public function __invoke(): JsonResponse
{ {
$config = $this->configRepository->findSingleton(); $config = $this->configRepository->findSingleton();
@@ -81,8 +81,12 @@ final readonly class RecurrenceHandler
$newTask->setCalendarEventUid($savedEventUid); $newTask->setCalendarEventUid($savedEventUid);
// Generate task number in transaction // Generate task number in transaction
$this->entityManager->wrapInTransaction(function () use ($newTask): void { $project = $newTask->getProject();
$maxNumber = $this->taskRepository->findMaxNumberByProjectForUpdate($newTask->getProject()); if (null === $project) {
return;
}
$this->entityManager->wrapInTransaction(function () use ($newTask, $project): void {
$maxNumber = $this->taskRepository->findMaxNumberByProjectForUpdate($project);
$newTask->setNumber($maxNumber + 1); $newTask->setNumber($maxNumber + 1);
$this->entityManager->persist($newTask); $this->entityManager->persist($newTask);
$this->entityManager->flush(); $this->entityManager->flush();
@@ -10,11 +10,13 @@ use App\Module\Integration\Domain\Service\FileSource;
use App\Module\ProjectManagement\Domain\Entity\TaskDocument; use App\Module\ProjectManagement\Domain\Entity\TaskDocument;
use Doctrine\ORM\EntityManagerInterface; use Doctrine\ORM\EntityManagerInterface;
use Symfony\Bundle\FrameworkBundle\Controller\AbstractController; use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
use Symfony\Bundle\SecurityBundle\Security;
use Symfony\Component\HttpFoundation\BinaryFileResponse; use Symfony\Component\HttpFoundation\BinaryFileResponse;
use Symfony\Component\HttpFoundation\HeaderUtils; use Symfony\Component\HttpFoundation\HeaderUtils;
use Symfony\Component\HttpFoundation\Response; use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\HttpFoundation\ResponseHeaderBag; use Symfony\Component\HttpFoundation\ResponseHeaderBag;
use Symfony\Component\HttpFoundation\StreamedResponse; use Symfony\Component\HttpFoundation\StreamedResponse;
use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
use Symfony\Component\Routing\Attribute\Route; use Symfony\Component\Routing\Attribute\Route;
use Symfony\Component\Security\Http\Attribute\IsGranted; use Symfony\Component\Security\Http\Attribute\IsGranted;
@@ -26,6 +28,7 @@ class TaskDocumentDownloadController extends AbstractController
public function __construct( public function __construct(
private readonly EntityManagerInterface $entityManager, private readonly EntityManagerInterface $entityManager,
private readonly FileSource $fileSource, private readonly FileSource $fileSource,
private readonly Security $security,
private readonly string $uploadDir, private readonly string $uploadDir,
) {} ) {}
@@ -39,6 +42,19 @@ class TaskDocumentDownloadController extends AbstractController
throw new NotFoundHttpException('Document not found.'); throw new NotFoundHttpException('Document not found.');
} }
$isAdmin = $this->security->isGranted('ROLE_ADMIN');
$isClient = $this->security->isGranted('ROLE_CLIENT') && !$isAdmin;
if (!$isAdmin) {
if ($isClient) {
$ticket = $document->getClientTicket();
if (null === $ticket || $ticket->getSubmittedBy() !== $this->security->getUser()) {
throw new AccessDeniedHttpException();
}
} elseif (null === $document->getTask()) {
throw new AccessDeniedHttpException();
}
}
$mimeType = $document->getMimeType() ?? 'application/octet-stream'; $mimeType = $document->getMimeType() ?? 'application/octet-stream';
// Inline for images (except SVG) and PDFs, attachment for everything else. // Inline for images (except SVG) and PDFs, attachment for everything else.
@@ -4,12 +4,13 @@ declare(strict_types=1);
namespace App\Module\TimeTracking\Infrastructure\Controller; namespace App\Module\TimeTracking\Infrastructure\Controller;
use App\Module\Core\Domain\Entity\User; use App\Module\Core\Domain\Repository\UserRepositoryInterface;
use App\Module\ProjectManagement\Domain\Entity\Project; use App\Module\ProjectManagement\Domain\Repository\ProjectRepositoryInterface;
use App\Module\TimeTracking\Domain\Repository\TimeEntryRepositoryInterface; use App\Module\TimeTracking\Domain\Repository\TimeEntryRepositoryInterface;
use App\Module\TimeTracking\Infrastructure\Export\TimeEntryExportService; use App\Module\TimeTracking\Infrastructure\Export\TimeEntryExportService;
use App\Shared\Domain\Contract\ProjectInterface;
use App\Shared\Domain\Contract\UserInterface;
use DateTimeImmutable; use DateTimeImmutable;
use Doctrine\ORM\EntityManagerInterface;
use Exception; use Exception;
use Symfony\Bundle\FrameworkBundle\Controller\AbstractController; use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
use Symfony\Bundle\SecurityBundle\Security; use Symfony\Bundle\SecurityBundle\Security;
@@ -25,7 +26,8 @@ class TimeEntryExportController extends AbstractController
public function __construct( public function __construct(
private readonly TimeEntryRepositoryInterface $timeEntryRepository, private readonly TimeEntryRepositoryInterface $timeEntryRepository,
private readonly TimeEntryExportService $exportService, private readonly TimeEntryExportService $exportService,
private readonly EntityManagerInterface $entityManager, private readonly ProjectRepositoryInterface $projectRepository,
private readonly UserRepositoryInterface $userRepository,
private readonly Security $security, private readonly Security $security,
) {} ) {}
@@ -54,7 +56,7 @@ class TimeEntryExportController extends AbstractController
// --- Users --- // --- Users ---
$users = null; $users = null;
if (!$this->security->isGranted('ROLE_ADMIN')) { if (!$this->security->isGranted('ROLE_ADMIN')) {
/** @var User $currentUser */ /** @var UserInterface $currentUser */
$currentUser = $this->security->getUser(); $currentUser = $this->security->getUser();
$users = [$currentUser]; $users = [$currentUser];
} else { } else {
@@ -64,7 +66,7 @@ class TimeEntryExportController extends AbstractController
fn (int $id) => $id > 0, fn (int $id) => $id > 0,
); );
if ([] !== $userIds) { if ([] !== $userIds) {
$users = $this->entityManager->getRepository(User::class)->findBy(['id' => $userIds]); $users = $this->userRepository->findByIds($userIds);
} }
} }
@@ -72,7 +74,7 @@ class TimeEntryExportController extends AbstractController
$clientId = $request->query->getInt('client'); $clientId = $request->query->getInt('client');
$clientProjects = null; $clientProjects = null;
if ($clientId > 0) { if ($clientId > 0) {
$clientProjects = $this->entityManager->getRepository(Project::class)->findBy(['client' => $clientId]); $clientProjects = $this->projectRepository->findBy(['client' => $clientId]);
} }
// --- Projects --- // --- Projects ---
@@ -84,13 +86,13 @@ class TimeEntryExportController extends AbstractController
fn (int $id) => $id > 0, fn (int $id) => $id > 0,
); );
if ([] !== $projectIds) { if ([] !== $projectIds) {
$projects = $this->entityManager->getRepository(Project::class)->findBy(['id' => $projectIds]); $projects = $this->projectRepository->findBy(['id' => $projectIds]);
} }
// Merge: if both client and projects are set, intersect; if only client, use client projects // Merge: if both client and projects are set, intersect; if only client, use client projects
if (null !== $clientProjects && null !== $projects) { if (null !== $clientProjects && null !== $projects) {
$clientProjectIds = array_map(fn (Project $p) => $p->getId(), $clientProjects); $clientProjectIds = array_map(fn (ProjectInterface $p) => $p->getId(), $clientProjects);
$projects = array_values(array_filter($projects, fn (Project $p) => in_array($p->getId(), $clientProjectIds, true))); $projects = array_values(array_filter($projects, fn (ProjectInterface $p) => in_array($p->getId(), $clientProjectIds, true)));
if ([] === $projects) { if ([] === $projects) {
$projects = null; $projects = null;
// No matching projects — force empty result by using a dummy condition // No matching projects — force empty result by using a dummy condition
@@ -5,6 +5,7 @@ declare(strict_types=1);
namespace App\Module\TimeTracking\Infrastructure\Export; namespace App\Module\TimeTracking\Infrastructure\Export;
use App\Module\TimeTracking\Domain\Entity\TimeEntry; use App\Module\TimeTracking\Domain\Entity\TimeEntry;
use App\Shared\Domain\Contract\ProjectInterface;
use DateTimeImmutable; use DateTimeImmutable;
use PhpOffice\PhpSpreadsheet\Cell\Coordinate; use PhpOffice\PhpSpreadsheet\Cell\Coordinate;
use PhpOffice\PhpSpreadsheet\Spreadsheet; use PhpOffice\PhpSpreadsheet\Spreadsheet;
@@ -70,6 +71,7 @@ class TimeEntryExportService
$task = $entry->getTask(); $task = $entry->getTask();
$taskLabel = ''; $taskLabel = '';
if (null !== $task) { if (null !== $task) {
/** @var null|ProjectInterface $project */
$project = $task->getProject(); $project = $task->getProject();
$code = $project?->getCode() ?? ''; $code = $project?->getCode() ?? '';
$taskLabel = $code.'-'.$task->getNumber().' - '.$task->getTitle(); $taskLabel = $code.'-'.$task->getNumber().' - '.$task->getTitle();
@@ -14,4 +14,6 @@ interface TaskInterface
public function getNumber(): ?int; public function getNumber(): ?int;
public function getTitle(): ?string; public function getTitle(): ?string;
public function getProject(): ?ProjectInterface;
} }
+11 -8
View File
@@ -11,7 +11,6 @@ use App\Module\Core\Domain\Entity\User;
use App\Module\Directory\Domain\Entity\Client; use App\Module\Directory\Domain\Entity\Client;
use App\Module\Directory\Domain\Entity\Prospect; use App\Module\Directory\Domain\Entity\Prospect;
use App\Module\ProjectManagement\Domain\Entity\Project; use App\Module\ProjectManagement\Domain\Entity\Project;
use App\Module\ProjectManagement\Domain\Entity\Task;
use App\Module\ProjectManagement\Domain\Entity\TaskDocument; use App\Module\ProjectManagement\Domain\Entity\TaskDocument;
use App\Module\ProjectManagement\Domain\Entity\TaskEffort; use App\Module\ProjectManagement\Domain\Entity\TaskEffort;
use App\Module\ProjectManagement\Domain\Entity\TaskGroup; use App\Module\ProjectManagement\Domain\Entity\TaskGroup;
@@ -19,6 +18,10 @@ use App\Module\ProjectManagement\Domain\Entity\TaskPriority;
use App\Module\ProjectManagement\Domain\Entity\TaskStatus; use App\Module\ProjectManagement\Domain\Entity\TaskStatus;
use App\Module\ProjectManagement\Domain\Entity\TaskTag; use App\Module\ProjectManagement\Domain\Entity\TaskTag;
use App\Module\TimeTracking\Domain\Entity\TimeEntry; use App\Module\TimeTracking\Domain\Entity\TimeEntry;
use App\Shared\Domain\Contract\ProjectInterface;
use App\Shared\Domain\Contract\TaskInterface;
use App\Shared\Domain\Contract\TaskTagInterface;
use App\Shared\Domain\Contract\UserInterface;
use Doctrine\Common\Collections\Collection; use Doctrine\Common\Collections\Collection;
/** /**
@@ -31,7 +34,7 @@ final class Serializer
/** /**
* @return array{id: ?int, code: ?string, name: ?string} * @return array{id: ?int, code: ?string, name: ?string}
*/ */
public static function projectRef(Project $project): array public static function projectRef(ProjectInterface $project): array
{ {
return [ return [
'id' => $project->getId(), 'id' => $project->getId(),
@@ -126,7 +129,7 @@ final class Serializer
/** /**
* @return null|array{id: ?int, username: ?string} * @return null|array{id: ?int, username: ?string}
*/ */
public static function user(?User $user): ?array public static function user(?UserInterface $user): ?array
{ {
if (null === $user) { if (null === $user) {
return null; return null;
@@ -139,13 +142,13 @@ final class Serializer
} }
/** /**
* @param Collection<int, User> $users * @param Collection<int, UserInterface> $users
* *
* @return list<array{id: ?int, username: ?string}> * @return list<array{id: ?int, username: ?string}>
*/ */
public static function users(Collection $users): array public static function users(Collection $users): array
{ {
return $users->map(fn (User $u) => [ return $users->map(fn (UserInterface $u) => [
'id' => $u->getId(), 'id' => $u->getId(),
'username' => $u->getUsername(), 'username' => $u->getUsername(),
])->toArray(); ])->toArray();
@@ -200,13 +203,13 @@ final class Serializer
} }
/** /**
* @param Collection<int, TaskTag> $tags * @param Collection<int, TaskTagInterface> $tags
* *
* @return list<array{id: ?int, label: ?string}> * @return list<array{id: ?int, label: ?string}>
*/ */
public static function tags(Collection $tags): array public static function tags(Collection $tags): array
{ {
return $tags->map(fn (TaskTag $t) => [ return $tags->map(fn (TaskTagInterface $t) => [
'id' => $t->getId(), 'id' => $t->getId(),
'label' => $t->getLabel(), 'label' => $t->getLabel(),
])->toArray(); ])->toArray();
@@ -244,7 +247,7 @@ final class Serializer
/** /**
* @return null|array{id: ?int, number: ?int, title: ?string} * @return null|array{id: ?int, number: ?int, title: ?string}
*/ */
public static function taskRef(?Task $task): ?array public static function taskRef(?TaskInterface $task): ?array
{ {
if (null === $task) { if (null === $task) {
return null; return null;