From 2141d25117332c643c918f7a17c540c2036d166e Mon Sep 17 00:00:00 2001 From: Kay Joosten Date: Tue, 21 Jul 2026 12:11:47 +0200 Subject: [PATCH 1/4] feat(ra): add CSV export to RA Management search results If applied, this commit will let RAA users export the current RA Management search results as a CSV file, and rename the blank Role filter option to a descriptive label. Why is this change needed? Prior to this change, the RA Management page had no export option, unlike the existing Tokens (Second Factors) page. Institution admins had no way to get an offline copy of their RA(A) listing. The Role filter dropdown also had an unlabeled blank option between "RA" and "RAA", which was unclear to users. How does it address the issue? This change adds an Export button next to Search, following the same button-group pattern used on the Tokens page. Since RA listings have no unpaginated export endpoint in the middleware client (unlike Second Factors), RaListingService::export() pages through the existing paginated search results and aggregates them before handing off to the new RaListingExport service, which streams the results as CSV via fputcsv. The exported filename is derived from the active role filter: ra_export_YYYY-MM-DD, raa_export_YYYY-MM-DD, or ra-raa-export_YYYY-MM-DD when no role filter is set. The Role filter's blank option now shows a "RA/RAA" placeholder instead of being empty. The export action keeps its own ROLE_RAA authorization check independent of the search action, so it stays protected even if the search page's access level changes later. Institution-scoping and existing authorization boundaries are unchanged. Links / references: https://github.com/OpenConext/Stepup-RA/issues/498 --- .../Command/ExportRaListingCommand.php | 82 ++++++++++++++ .../Controller/RaManagementController.php | 16 +++ .../Form/Type/RoleAtInstitutionType.php | 1 + .../Form/Type/SearchRaListingType.php | 17 ++- .../RaBundle/Resources/config/services.yml | 7 +- .../RaBundle/Service/RaListingExport.php | 79 ++++++++++++++ .../RaBundle/Service/RaListingService.php | 103 ++++++++++++++---- translations/messages+intl-icu.en_GB.xliff | 8 ++ translations/messages+intl-icu.nl_NL.xliff | 8 ++ 9 files changed, 298 insertions(+), 23 deletions(-) create mode 100644 src/Surfnet/StepupRa/RaBundle/Command/ExportRaListingCommand.php create mode 100644 src/Surfnet/StepupRa/RaBundle/Service/RaListingExport.php diff --git a/src/Surfnet/StepupRa/RaBundle/Command/ExportRaListingCommand.php b/src/Surfnet/StepupRa/RaBundle/Command/ExportRaListingCommand.php new file mode 100644 index 00000000..6a73756c --- /dev/null +++ b/src/Surfnet/StepupRa/RaBundle/Command/ExportRaListingCommand.php @@ -0,0 +1,82 @@ +actorId = $command->actorId; + $exportCommand->name = $command->name; + $exportCommand->email = $command->email; + $exportCommand->institution = $command->institution; + $exportCommand->roleAtInstitution = $command->roleAtInstitution; + + return $exportCommand; + } + + public function getFileName(): string + { + $date = new DateTime(); + $date = $date->format('Y-m-d'); + + $role = $this->roleAtInstitution instanceof RoleAtInstitution && $this->roleAtInstitution->hasRole() + ? $this->roleAtInstitution->getRole() + : null; + + return match ($role) { + 'ra' => "ra_export_{$date}", + 'raa' => "raa_export_{$date}", + default => "ra-raa-export_{$date}", + }; + } +} diff --git a/src/Surfnet/StepupRa/RaBundle/Controller/RaManagementController.php b/src/Surfnet/StepupRa/RaBundle/Controller/RaManagementController.php index e9547258..33572fec 100755 --- a/src/Surfnet/StepupRa/RaBundle/Controller/RaManagementController.php +++ b/src/Surfnet/StepupRa/RaBundle/Controller/RaManagementController.php @@ -24,6 +24,7 @@ use Surfnet\StepupMiddlewareClientBundle\Identity\Dto\RaListing; use Surfnet\StepupRa\RaBundle\Command\AccreditCandidateCommand; use Surfnet\StepupRa\RaBundle\Command\AmendRegistrationAuthorityInformationCommand; +use Surfnet\StepupRa\RaBundle\Command\ExportRaListingCommand; use Surfnet\StepupRa\RaBundle\Command\RetractRegistrationAuthorityCommand; use Surfnet\StepupRa\RaBundle\Command\SearchRaCandidatesCommand; use Surfnet\StepupRa\RaBundle\Command\SearchRaListingCommand; @@ -89,6 +90,11 @@ public function manage(Request $request): Response $form = $this->createForm(SearchRaListingType::class, $command, ['method' => 'get']); $form->handleRequest($request); + if ($form->isSubmitted() && $form->getClickedButton()?->getName() === 'export') { + $this->logger->notice('Forwarding to export RA(A) listing action'); + return $this->forward('\Surfnet\StepupRa\RaBundle\Controller\RaManagementController::export', ['command' => $command]); + } + $raList = $this->raListingService->search($command); $pagination = $this->paginator->paginate( @@ -117,6 +123,16 @@ public function manage(Request $request): Response ); } + #[IsGranted('ROLE_RAA')] + public function export(SearchRaListingCommand $command): Response + { + $this->logger->notice('Starting export of searched RA(A) listing'); + + $exportCommand = ExportRaListingCommand::fromSearchCommand($command); + + return $this->raListingService->export($exportCommand); + } + #[Route( path: '/management/search-ra-candidate', name: 'ra_management_ra_candidate_search', diff --git a/src/Surfnet/StepupRa/RaBundle/Form/Type/RoleAtInstitutionType.php b/src/Surfnet/StepupRa/RaBundle/Form/Type/RoleAtInstitutionType.php index db7c1cfa..f6f6c1d9 100755 --- a/src/Surfnet/StepupRa/RaBundle/Form/Type/RoleAtInstitutionType.php +++ b/src/Surfnet/StepupRa/RaBundle/Form/Type/RoleAtInstitutionType.php @@ -52,6 +52,7 @@ public function buildForm(FormBuilderInterface $builder, array $options): void 'choices' => RaRoleChoiceList::create(), 'choice_value' => fn($choice) => $choice, 'required' => $isRequired, + 'placeholder' => $isRequired ? null : 'ra.form.role_at_institution.placeholder.role', ])->add('institution', ChoiceType::class, [ 'label' => 'ra.form.role_at_institution.label.institution', 'choices' => $selectRaaOptions, diff --git a/src/Surfnet/StepupRa/RaBundle/Form/Type/SearchRaListingType.php b/src/Surfnet/StepupRa/RaBundle/Form/Type/SearchRaListingType.php index 3ab96c48..0cee0a8f 100755 --- a/src/Surfnet/StepupRa/RaBundle/Form/Type/SearchRaListingType.php +++ b/src/Surfnet/StepupRa/RaBundle/Form/Type/SearchRaListingType.php @@ -50,10 +50,25 @@ public function buildForm(FormBuilderInterface $builder, array $options): void 'choices' => $data->raInstitutionFilterOptions, 'label' => 'ra.form.ra_search_ra_listing.label.role', 'required' => false, - ])->add('search', SubmitType::class, [ + ]); + + $buttonGroup = $builder->create( + 'button-group', + ButtonGroupType::class, + [ + 'mapped' => false, + ], + ) + ->add('search', SubmitType::class, [ 'label' => 'ra.form.ra_search_ra_listing.button.search', 'attr' => ['class' => 'btn btn-primary search-button'], + ]) + ->add('export', SubmitType::class, [ + 'label' => 'ra.form.ra_search_ra_listing.button.export', + 'attr' => ['class' => 'btn btn-secondary'], ]); + + $builder->add($buttonGroup); } public function configureOptions(OptionsResolver $resolver): void diff --git a/src/Surfnet/StepupRa/RaBundle/Resources/config/services.yml b/src/Surfnet/StepupRa/RaBundle/Resources/config/services.yml index 3ae3ff9c..68045670 100755 --- a/src/Surfnet/StepupRa/RaBundle/Resources/config/services.yml +++ b/src/Surfnet/StepupRa/RaBundle/Resources/config/services.yml @@ -215,11 +215,16 @@ services: Surfnet\StepupRa\RaBundle\Service\RaLocationService: alias: ra.service.ra_location + ra.service.ra_listing_exporter: + class: Surfnet\StepupRa\RaBundle\Service\RaListingExport + arguments: + - "@logger" + ra.service.ra_listing: class: Surfnet\StepupRa\RaBundle\Service\RaListingService arguments: - "@surfnet_stepup_middleware_client.identity.service.ra_listing" - - "@logger" + - "@ra.service.ra_listing_exporter" Surfnet\StepupRa\RaBundle\Service\RaListingService: alias: ra.service.ra_listing diff --git a/src/Surfnet/StepupRa/RaBundle/Service/RaListingExport.php b/src/Surfnet/StepupRa/RaBundle/Service/RaListingExport.php new file mode 100644 index 00000000..af1cd36f --- /dev/null +++ b/src/Surfnet/StepupRa/RaBundle/Service/RaListingExport.php @@ -0,0 +1,79 @@ +logger->notice(sprintf('Exporting %d rows to "%s"', count($raListings), $fileName)); + + $columnNames = $this->getColumnNames(); + + return new StreamedResponse( + function () use ($raListings, $columnNames) { + $handle = fopen('php://output', 'r+'); + fputcsv($handle, $columnNames); + foreach ($raListings as $raListing) { + fputcsv($handle, [ + $raListing->commonName, + $raListing->email, + $raListing->institution, + $raListing->role, + $raListing->raInstitution, + $raListing->location, + $raListing->contactInformation, + ]); + } + fflush($handle); + fclose($handle); + }, + Response::HTTP_OK, + [ + 'Content-Type' => 'application/csv', + 'Content-Disposition' => sprintf('attachment; filename="%s.csv"', $fileName), + ], + ); + } + + private function getColumnNames(): array + { + return [ + 'Common Name', + 'Email', + 'Institution', + 'Role', + 'RA Institution', + 'Location', + 'Contact Information', + ]; + } +} diff --git a/src/Surfnet/StepupRa/RaBundle/Service/RaListingService.php b/src/Surfnet/StepupRa/RaBundle/Service/RaListingService.php index a540e286..5577b8a6 100755 --- a/src/Surfnet/StepupRa/RaBundle/Service/RaListingService.php +++ b/src/Surfnet/StepupRa/RaBundle/Service/RaListingService.php @@ -22,12 +22,16 @@ use Surfnet\StepupMiddlewareClientBundle\Identity\Dto\RaListing; use Surfnet\StepupMiddlewareClientBundle\Identity\Dto\RaListingCollection; use Surfnet\StepupMiddlewareClientBundle\Identity\Service\RaListingService as ApiRaListingService; +use Surfnet\StepupRa\RaBundle\Command\ExportRaListingCommand; use Surfnet\StepupRa\RaBundle\Command\SearchRaListingCommand; +use Surfnet\StepupRa\RaBundle\Value\RoleAtInstitution; +use Symfony\Component\HttpFoundation\StreamedResponse; final readonly class RaListingService { public function __construct( private ApiRaListingService $apiRaListingService, + private RaListingExport $raListingExport, ) { } @@ -37,41 +41,98 @@ public function __construct( */ public function search(SearchRaListingCommand $command): RaListingCollection { - $query = new RaListingSearchQuery($command->actorId, $command->pageNumber); + $query = $this->buildQuery( + $command->actorId, + $command->pageNumber, + $command->name, + $command->email, + $command->institution, + $command->roleAtInstitution, + $command->orderBy, + $command->orderDirection, + ); - if ($command->name) { - $query->setName($command->name); - } + return $this->apiRaListingService->search($query); + } + + public function get(string $identityId, string $institution, string $actorId): ?RaListing + { + return $this->apiRaListingService->get($identityId, $institution, $actorId); + } + + /** + * Pages through all RA listing results matching the given filters (bypassing UI pagination) + * and streams them as a CSV export. + */ + public function export(ExportRaListingCommand $command): StreamedResponse + { + $raListings = []; + $pageNumber = 1; + + do { + $query = $this->buildQuery( + $command->actorId, + $pageNumber, + $command->name, + $command->email, + $command->institution, + $command->roleAtInstitution, + ); + + $collection = $this->apiRaListingService->search($query); + $raListings = array_merge($raListings, $collection->getElements()); - if ($command->email) { - $query->setEmail($command->email); + $lastPage = (int) ceil($collection->getTotalItems() / max($collection->getItemsPerPage(), 1)); + $pageNumber++; + } while ($pageNumber <= $lastPage); + + return $this->raListingExport->export($raListings, $command->getFileName()); + } + + /** + * @SuppressWarnings("PHPMD.CyclomaticComplexity") -- The command to query mapping exceeds the + * @SuppressWarnings("PHPMD.NPathComplexity") CyclomaticComplexity and NPathComplexity threshold. + */ + private function buildQuery( + string $actorId, + int $pageNumber, + ?string $name, + ?string $email, + ?string $institution, + ?RoleAtInstitution $roleAtInstitution, + ?string $orderBy = null, + ?string $orderDirection = null, + ): RaListingSearchQuery { + $query = new RaListingSearchQuery($actorId, $pageNumber); + + if ($name) { + $query->setName($name); } - if ($command->institution) { - $query->setInstitution($command->institution); + if ($email) { + $query->setEmail($email); } - if ($command->roleAtInstitution && $command->roleAtInstitution->hasRole()) { - $query->setRole($command->roleAtInstitution->getRole()); + if ($institution) { + $query->setInstitution($institution); } - if ($command->roleAtInstitution && $command->roleAtInstitution->hasInstitution()) { - $query->setRaInstitution($command->roleAtInstitution->getInstitution()); + if ($roleAtInstitution && $roleAtInstitution->hasRole()) { + $query->setRole($roleAtInstitution->getRole()); } - if ($command->orderBy) { - $query->setOrderBy($command->orderBy); + if ($roleAtInstitution && $roleAtInstitution->hasInstitution()) { + $query->setRaInstitution($roleAtInstitution->getInstitution()); } - if ($command->orderDirection) { - $query->setOrderDirection($command->orderDirection); + if ($orderBy) { + $query->setOrderBy($orderBy); } - return $this->apiRaListingService->search($query); - } + if ($orderDirection) { + $query->setOrderDirection($orderDirection); + } - public function get(string $identityId, string $institution, string $actorId): ?RaListing - { - return $this->apiRaListingService->get($identityId, $institution, $actorId); + return $query; } } diff --git a/translations/messages+intl-icu.en_GB.xliff b/translations/messages+intl-icu.en_GB.xliff index ec1e129b..51911100 100644 --- a/translations/messages+intl-icu.en_GB.xliff +++ b/translations/messages+intl-icu.en_GB.xliff @@ -241,6 +241,10 @@ ra.form.ra_search_ra_candidates.label.name Name + + ra.form.ra_search_ra_listing.button.export + Export + ra.form.ra_search_ra_listing.button.search Search @@ -373,6 +377,10 @@ ra.form.role_at_institution.label.institution @ + + ra.form.role_at_institution.placeholder.role + RA/RAA + ra.form.select_institution.button.switch Switch diff --git a/translations/messages+intl-icu.nl_NL.xliff b/translations/messages+intl-icu.nl_NL.xliff index 2fb99919..4ac60577 100644 --- a/translations/messages+intl-icu.nl_NL.xliff +++ b/translations/messages+intl-icu.nl_NL.xliff @@ -241,6 +241,10 @@ ra.form.ra_search_ra_candidates.label.name Naam + + ra.form.ra_search_ra_listing.button.export + Exporteren + ra.form.ra_search_ra_listing.button.search Zoeken @@ -373,6 +377,10 @@ ra.form.role_at_institution.label.institution ra.form.role_at_institution.label.institution + + ra.form.role_at_institution.placeholder.role + RA/RAA + ra.form.select_institution.button.switch Laden From 3bb06adebe9d05dc6c222106a881813490944a50 Mon Sep 17 00:00:00 2001 From: Kay Joosten Date: Tue, 21 Jul 2026 12:19:48 +0200 Subject: [PATCH 2/4] fix(ra): satisfy phpstan for RA Management export If applied, this commit will make the CI QA pipeline pass for the RA Management CSV export feature. Why is this change needed? Prior to this change, CI's phpstan step failed on the new export code: an untyped/uninitialized $actorId property, a stale baseline entry for a booleanAnd.leftAlwaysTrue error that the buildQuery() refactor already fixed by making its $roleAtInstitution parameter nullable, unchecked resource|false returns from fopen() in RaListingExport, a missing iterable value type on getColumnNames(), and a new getClickedButton() call on FormInterface that needed the same baseline treatment already used for the equivalent Second Factors export form. How does it address the issue? This change adds a `@var string` docblock to ExportRaListingCommand::$actorId (matching the existing ExportRaSecondFactorsCommand convention), removes the now-stale booleanAnd.leftAlwaysTrue baseline entry for RaListingService, adds a baseline entry for RaManagementController's getClickedButton() call mirroring SecondFactorController's, guards the fopen() result in RaListingExport before using it, and adds a `@return string[]` docblock to getColumnNames(). --- ci/qa/phpstan-baseline.php | 12 ++++++------ .../RaBundle/Command/ExportRaListingCommand.php | 3 +++ .../StepupRa/RaBundle/Service/RaListingExport.php | 7 +++++++ 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/ci/qa/phpstan-baseline.php b/ci/qa/phpstan-baseline.php index 684e3f5f..7ed0bfef 100644 --- a/ci/qa/phpstan-baseline.php +++ b/ci/qa/phpstan-baseline.php @@ -343,6 +343,12 @@ 'count' => 1, 'path' => __DIR__ . '/../../src/Surfnet/StepupRa/RaBundle/Controller/SecondFactorController.php', ]; +$ignoreErrors[] = [ + 'message' => '#^Call to an undefined method Symfony\\\\Component\\\\Form\\\\FormInterface\\\\:\\:getClickedButton\\(\\)\\.$#', + 'identifier' => 'method.notFound', + 'count' => 1, + 'path' => __DIR__ . '/../../src/Surfnet/StepupRa/RaBundle/Controller/RaManagementController.php', +]; $ignoreErrors[] = [ 'message' => '#^Call to an undefined method Symfony\\\\Component\\\\Security\\\\Core\\\\User\\\\UserProviderInterface\\:\\:findById\\(\\)\\.$#', 'identifier' => 'method.notFound', @@ -715,12 +721,6 @@ 'count' => 1, 'path' => __DIR__ . '/../../src/Surfnet/StepupRa/RaBundle/Service/RaCandidateService.php', ]; -$ignoreErrors[] = [ - 'message' => '#^Left side of && is always true\\.$#', - 'identifier' => 'booleanAnd.leftAlwaysTrue', - 'count' => 2, - 'path' => __DIR__ . '/../../src/Surfnet/StepupRa/RaBundle/Service/RaListingService.php', -]; $ignoreErrors[] = [ 'message' => '#^Parameter \\#1 \\$raInstitution of method Surfnet\\\\StepupMiddlewareClient\\\\Identity\\\\Dto\\\\RaListingSearchQuery\\:\\:setRaInstitution\\(\\) expects string, string\\|null given\\.$#', 'identifier' => 'argument.type', diff --git a/src/Surfnet/StepupRa/RaBundle/Command/ExportRaListingCommand.php b/src/Surfnet/StepupRa/RaBundle/Command/ExportRaListingCommand.php index 6a73756c..fc473ecc 100644 --- a/src/Surfnet/StepupRa/RaBundle/Command/ExportRaListingCommand.php +++ b/src/Surfnet/StepupRa/RaBundle/Command/ExportRaListingCommand.php @@ -24,6 +24,9 @@ final class ExportRaListingCommand { + /** + * @var string + */ #[Assert\NotBlank(message: 'ra.search_ra_listing.actor_id.blank')] #[Assert\Type('string', message: 'ra.search_ra_listing.actor_id.type')] public $actorId; diff --git a/src/Surfnet/StepupRa/RaBundle/Service/RaListingExport.php b/src/Surfnet/StepupRa/RaBundle/Service/RaListingExport.php index af1cd36f..f9b9f20d 100644 --- a/src/Surfnet/StepupRa/RaBundle/Service/RaListingExport.php +++ b/src/Surfnet/StepupRa/RaBundle/Service/RaListingExport.php @@ -19,6 +19,7 @@ namespace Surfnet\StepupRa\RaBundle\Service; use Psr\Log\LoggerInterface; +use RuntimeException; use Surfnet\StepupMiddlewareClientBundle\Identity\Dto\RaListing; use Symfony\Component\HttpFoundation\Response; use Symfony\Component\HttpFoundation\StreamedResponse; @@ -41,6 +42,9 @@ public function export(array $raListings, string $fileName): StreamedResponse return new StreamedResponse( function () use ($raListings, $columnNames) { $handle = fopen('php://output', 'r+'); + if ($handle === false) { + throw new RuntimeException('Unable to open php://output for writing the RA(A) listing export'); + } fputcsv($handle, $columnNames); foreach ($raListings as $raListing) { fputcsv($handle, [ @@ -64,6 +68,9 @@ function () use ($raListings, $columnNames) { ); } + /** + * @return string[] + */ private function getColumnNames(): array { return [ From 89257715c29ef9ff8ca67e5b1723973c9ee66e81 Mon Sep 17 00:00:00 2001 From: Kay Joosten Date: Mon, 10 Aug 2026 10:46:57 +0200 Subject: [PATCH 3/4] test(ra): add unit tests for RA Management CSV export Covers ExportRaListingCommand, RaListingExport and RaListingService. --- .../Command/ExportRaListingCommandTest.php | 94 +++++++++++++ .../Tests/Service/RaListingExportTest.php | 100 ++++++++++++++ .../Tests/Service/RaListingServiceTest.php | 128 ++++++++++++++++++ 3 files changed, 322 insertions(+) create mode 100644 src/Surfnet/StepupRa/RaBundle/Tests/Command/ExportRaListingCommandTest.php create mode 100644 src/Surfnet/StepupRa/RaBundle/Tests/Service/RaListingExportTest.php create mode 100644 src/Surfnet/StepupRa/RaBundle/Tests/Service/RaListingServiceTest.php diff --git a/src/Surfnet/StepupRa/RaBundle/Tests/Command/ExportRaListingCommandTest.php b/src/Surfnet/StepupRa/RaBundle/Tests/Command/ExportRaListingCommandTest.php new file mode 100644 index 00000000..d30b59a0 --- /dev/null +++ b/src/Surfnet/StepupRa/RaBundle/Tests/Command/ExportRaListingCommandTest.php @@ -0,0 +1,94 @@ +setRole('ra'); + + $searchCommand = new SearchRaListingCommand(); + $searchCommand->actorId = 'actor-id'; + $searchCommand->name = 'Jane Doe'; + $searchCommand->email = 'jane@example.org'; + $searchCommand->institution = 'institution-a'; + $searchCommand->roleAtInstitution = $roleAtInstitution; + $searchCommand->pageNumber = 3; + $searchCommand->orderBy = 'name'; + $searchCommand->orderDirection = 'desc'; + + $exportCommand = ExportRaListingCommand::fromSearchCommand($searchCommand); + + $this->assertSame('actor-id', $exportCommand->actorId); + $this->assertSame('Jane Doe', $exportCommand->name); + $this->assertSame('jane@example.org', $exportCommand->email); + $this->assertSame('institution-a', $exportCommand->institution); + $this->assertSame($roleAtInstitution, $exportCommand->roleAtInstitution); + } + + #[Test] + #[DataProvider('fileNameProvider')] + public function get_file_name_reflects_the_role_filter(?string $role, string $expectedPrefix) + { + $command = new ExportRaListingCommand(); + $command->actorId = 'actor-id'; + + if ($role !== null) { + $roleAtInstitution = new RoleAtInstitution(); + $roleAtInstitution->setRole($role); + $command->roleAtInstitution = $roleAtInstitution; + } + + $date = (new DateTime())->format('Y-m-d'); + + $this->assertSame($expectedPrefix . '_' . $date, $command->getFileName()); + } + + public static function fileNameProvider(): array + { + return [ + 'ra role filter' => ['ra', 'ra_export'], + 'raa role filter' => ['raa', 'raa_export'], + 'no role filter' => [null, 'ra-raa-export'], + ]; + } + + #[Test] + public function get_file_name_treats_role_at_institution_without_a_role_as_unfiltered() + { + $command = new ExportRaListingCommand(); + $command->actorId = 'actor-id'; + $command->roleAtInstitution = new RoleAtInstitution(); + + $date = (new DateTime())->format('Y-m-d'); + + $this->assertSame('ra-raa-export_' . $date, $command->getFileName()); + } +} diff --git a/src/Surfnet/StepupRa/RaBundle/Tests/Service/RaListingExportTest.php b/src/Surfnet/StepupRa/RaBundle/Tests/Service/RaListingExportTest.php new file mode 100644 index 00000000..5716e86c --- /dev/null +++ b/src/Surfnet/StepupRa/RaBundle/Tests/Service/RaListingExportTest.php @@ -0,0 +1,100 @@ +shouldReceive('notice'); + + $export = new RaListingExport($logger); + + $raListing = new RaListing(); + $raListing->identityId = 'identity-id'; + $raListing->commonName = 'Jane Doe'; + $raListing->email = 'jane@example.org'; + $raListing->institution = 'institution-a'; + $raListing->role = 'ra'; + $raListing->raInstitution = 'institution-a'; + $raListing->location = 'Room 101'; + $raListing->contactInformation = '+31 6 12345678'; + + $response = $export->export([$raListing], 'ra_export_2026-07-24'); + + $this->assertSame('application/csv', $response->headers->get('Content-Type')); + $this->assertSame( + 'attachment; filename="ra_export_2026-07-24.csv"', + $response->headers->get('Content-Disposition'), + ); + + ob_start(); + $response->sendContent(); + $csv = ob_get_clean(); + + $rows = array_map( + fn(string $line) => str_getcsv($line, escape: ''), + explode("\n", rtrim(str_replace("\r\n", "\n", $csv), "\n")), + ); + + $this->assertSame( + ['Common Name', 'Email', 'Institution', 'Role', 'RA Institution', 'Location', 'Contact Information'], + $rows[0], + ); + $this->assertSame( + ['Jane Doe', 'jane@example.org', 'institution-a', 'ra', 'institution-a', 'Room 101', '+31 6 12345678'], + $rows[1], + ); + } + + #[Test] + public function it_streams_only_the_header_row_when_there_are_no_listings() + { + $logger = Mockery::mock(LoggerInterface::class); + $logger->shouldReceive('notice'); + + $export = new RaListingExport($logger); + + $response = $export->export([], 'ra-raa-export_2026-07-24'); + + ob_start(); + $response->sendContent(); + $csv = ob_get_clean(); + + $rows = array_map( + fn(string $line) => str_getcsv($line, escape: ''), + explode("\n", rtrim(str_replace("\r\n", "\n", $csv), "\n")), + ); + + $this->assertCount(1, $rows); + $this->assertSame( + ['Common Name', 'Email', 'Institution', 'Role', 'RA Institution', 'Location', 'Contact Information'], + $rows[0], + ); + } +} diff --git a/src/Surfnet/StepupRa/RaBundle/Tests/Service/RaListingServiceTest.php b/src/Surfnet/StepupRa/RaBundle/Tests/Service/RaListingServiceTest.php new file mode 100644 index 00000000..ff871dc5 --- /dev/null +++ b/src/Surfnet/StepupRa/RaBundle/Tests/Service/RaListingServiceTest.php @@ -0,0 +1,128 @@ +collectionOf(['identity-1', 'identity-2'], totalItems: 3, page: 1, itemsPerPage: 2); + $secondPage = $this->collectionOf(['identity-3'], totalItems: 3, page: 2, itemsPerPage: 2); + + $apiService = Mockery::mock(ApiRaListingService::class); + $apiService + ->shouldReceive('search') + ->once() + ->with(Mockery::on(fn (RaListingSearchQuery $query) => str_contains($query->toHttpQuery(), 'p=1'))) + ->andReturn($firstPage); + $apiService + ->shouldReceive('search') + ->once() + ->with(Mockery::on(fn (RaListingSearchQuery $query) => str_contains($query->toHttpQuery(), 'p=2'))) + ->andReturn($secondPage); + + $expectedResponse = Mockery::mock(StreamedResponse::class); + $export = Mockery::mock(RaListingExport::class); + $export + ->shouldReceive('export') + ->once() + ->with( + Mockery::on(function (array $listings) { + return array_map(fn (RaListing $listing) => $listing->identityId, $listings) + === ['identity-1', 'identity-2', 'identity-3']; + }), + 'ra-raa-export_' . (new DateTime())->format('Y-m-d'), + ) + ->andReturn($expectedResponse); + + $service = new RaListingService($apiService, $export); + + $command = new ExportRaListingCommand(); + $command->actorId = 'actor-id'; + + $this->assertSame($expectedResponse, $service->export($command)); + } + + #[Test] + public function export_only_queries_a_single_page_when_there_are_no_results() + { + $emptyPage = RaListingCollection::empty(); + + $apiService = Mockery::mock(ApiRaListingService::class); + $apiService + ->shouldReceive('search') + ->once() + ->andReturn($emptyPage); + + $expectedResponse = Mockery::mock(StreamedResponse::class); + $export = Mockery::mock(RaListingExport::class); + $export + ->shouldReceive('export') + ->once() + ->with([], Mockery::type('string')) + ->andReturn($expectedResponse); + + $service = new RaListingService($apiService, $export); + + $command = new ExportRaListingCommand(); + $command->actorId = 'actor-id'; + + $this->assertSame($expectedResponse, $service->export($command)); + } + + /** + * @param string[] $identityIds + */ + private function collectionOf( + array $identityIds, + int $totalItems, + int $page, + int $itemsPerPage, + ): RaListingCollection { + $elements = array_map(function (string $identityId) { + $listing = new RaListing(); + $listing->identityId = $identityId; + $listing->commonName = $identityId; + $listing->email = $identityId . '@example.org'; + $listing->institution = 'institution-a'; + $listing->role = 'ra'; + $listing->raInstitution = 'institution-a'; + $listing->location = ''; + $listing->contactInformation = ''; + + return $listing; + }, $identityIds); + + return new RaListingCollection($elements, $totalItems, $page, $itemsPerPage); + } +} From 3bda199135af18948b8a76bef0b0e9b298db9996 Mon Sep 17 00:00:00 2001 From: Kay Joosten Date: Wed, 9 Sep 2026 11:24:12 +0200 Subject: [PATCH 4/4] Fix CSV injection, preserve sort order, and stream RA listing exports The RA/RAA listing CSV export wrote identity fields straight into the file, so a value starting with =, -, @, tab or CR could be executed as a formula when the export is opened in Excel, LibreOffice or Google Sheets. Cell values are now sanitized before being written, following the OWASP CSV injection mitigation. A leading + is intentionally left untouched, since Contact Information routinely holds international phone numbers. The export also ignored the orderBy/orderDirection the user selected in the search screen, always falling back to the API default sort. ExportRaListingCommand now copies these fields from the search command, matching the existing pattern used by ExportRaSecondFactorsCommand. Finally, export() eagerly merged every page into memory before handing it to the CSV writer, despite the response being served as a streamed download. Fetching is now done through a generator, so pages are only requested as the streamed response is actually written. Updated the accompanying tests to match: iterable/generator handling in mocks, order-by/order-direction propagation and a CSV injection regression test. --- .../Command/ExportRaListingCommand.php | 14 ++++ .../RaBundle/Service/RaListingExport.php | 51 ++++++++++--- .../RaBundle/Service/RaListingService.php | 19 +++-- .../Command/ExportRaListingCommandTest.php | 2 + .../Tests/Service/RaListingExportTest.php | 45 ++++++++++++ .../Tests/Service/RaListingServiceTest.php | 71 +++++++++++++++++-- 6 files changed, 182 insertions(+), 20 deletions(-) diff --git a/src/Surfnet/StepupRa/RaBundle/Command/ExportRaListingCommand.php b/src/Surfnet/StepupRa/RaBundle/Command/ExportRaListingCommand.php index fc473ecc..f7593bee 100644 --- a/src/Surfnet/StepupRa/RaBundle/Command/ExportRaListingCommand.php +++ b/src/Surfnet/StepupRa/RaBundle/Command/ExportRaListingCommand.php @@ -51,6 +51,18 @@ final class ExportRaListingCommand */ public $roleAtInstitution; + /** + * @var string|null + */ + #[Assert\Choice(choices: ['name', 'email'], message: 'ra.search_ra_candidates.order_by.invalid_choice')] + public $orderBy; + + /** + * @var string|null + */ + #[Assert\Choice(choices: ['asc', 'desc'], message: 'ra.search_ra_candidates.order_direction.invalid_choice')] + public $orderDirection; + /** * Builds the command from a SearchRaListingCommand */ @@ -63,6 +75,8 @@ public static function fromSearchCommand(SearchRaListingCommand $command): Expor $exportCommand->email = $command->email; $exportCommand->institution = $command->institution; $exportCommand->roleAtInstitution = $command->roleAtInstitution; + $exportCommand->orderBy = $command->orderBy; + $exportCommand->orderDirection = $command->orderDirection; return $exportCommand; } diff --git a/src/Surfnet/StepupRa/RaBundle/Service/RaListingExport.php b/src/Surfnet/StepupRa/RaBundle/Service/RaListingExport.php index f9b9f20d..6290345d 100644 --- a/src/Surfnet/StepupRa/RaBundle/Service/RaListingExport.php +++ b/src/Surfnet/StepupRa/RaBundle/Service/RaListingExport.php @@ -31,32 +31,38 @@ public function __construct(private readonly LoggerInterface $logger) } /** - * @param RaListing[] $raListings + * @param iterable $raListings */ - public function export(array $raListings, string $fileName): StreamedResponse + public function export(iterable $raListings, string $fileName): StreamedResponse { - $this->logger->notice(sprintf('Exporting %d rows to "%s"', count($raListings), $fileName)); + $this->logger->notice(sprintf('Starting RA(A) listing export to "%s"', $fileName)); $columnNames = $this->getColumnNames(); return new StreamedResponse( - function () use ($raListings, $columnNames) { + function () use ($raListings, $columnNames, $fileName) { $handle = fopen('php://output', 'r+'); if ($handle === false) { throw new RuntimeException('Unable to open php://output for writing the RA(A) listing export'); } fputcsv($handle, $columnNames); + + $rowCount = 0; foreach ($raListings as $raListing) { fputcsv($handle, [ - $raListing->commonName, - $raListing->email, - $raListing->institution, - $raListing->role, - $raListing->raInstitution, - $raListing->location, - $raListing->contactInformation, + $this->sanitizeCsvCell($raListing->commonName), + $this->sanitizeCsvCell($raListing->email), + $this->sanitizeCsvCell($raListing->institution), + $this->sanitizeCsvCell($raListing->role), + $this->sanitizeCsvCell($raListing->raInstitution), + $this->sanitizeCsvCell($raListing->location), + $this->sanitizeCsvCell($raListing->contactInformation), ]); + $rowCount++; } + + $this->logger->notice(sprintf('Exported %d rows to "%s"', $rowCount, $fileName)); + fflush($handle); fclose($handle); }, @@ -68,6 +74,29 @@ function () use ($raListings, $columnNames) { ); } + /** + * Neutralizes CSV/spreadsheet formula injection: values coming from identity/profile + * data are attacker-influenceable and must not be allowed to start a formula when the + * exported file is opened in Excel/LibreOffice/Google Sheets. The leading "+" is + * intentionally not treated as a formula trigger here, since Contact Information + * routinely holds international phone numbers (e.g. "+31 6 12345678") that must be + * exported unmodified. + * + * @see https://owasp.org/www-community/attacks/CSV_Injection + */ + private function sanitizeCsvCell(?string $value): ?string + { + if ($value === null || $value === '') { + return $value; + } + + if (in_array($value[0], ['=', '-', '@', "\t", "\r"], true)) { + return "'" . $value; + } + + return $value; + } + /** * @return string[] */ diff --git a/src/Surfnet/StepupRa/RaBundle/Service/RaListingService.php b/src/Surfnet/StepupRa/RaBundle/Service/RaListingService.php index 5577b8a6..5658023c 100755 --- a/src/Surfnet/StepupRa/RaBundle/Service/RaListingService.php +++ b/src/Surfnet/StepupRa/RaBundle/Service/RaListingService.php @@ -66,7 +66,18 @@ public function get(string $identityId, string $institution, string $actorId): ? */ public function export(ExportRaListingCommand $command): StreamedResponse { - $raListings = []; + return $this->raListingExport->export($this->fetchAllPages($command), $command->getFileName()); + } + + /** + * Lazily fetches every page of RA listing results. Wrapped in a generator so no page is + * requested until the exporter actually starts consuming rows (which happens when the + * streamed response is sent), avoiding buffering the full result set in memory. + * + * @return iterable + */ + private function fetchAllPages(ExportRaListingCommand $command): iterable + { $pageNumber = 1; do { @@ -77,16 +88,16 @@ public function export(ExportRaListingCommand $command): StreamedResponse $command->email, $command->institution, $command->roleAtInstitution, + $command->orderBy, + $command->orderDirection, ); $collection = $this->apiRaListingService->search($query); - $raListings = array_merge($raListings, $collection->getElements()); + yield from $collection->getElements(); $lastPage = (int) ceil($collection->getTotalItems() / max($collection->getItemsPerPage(), 1)); $pageNumber++; } while ($pageNumber <= $lastPage); - - return $this->raListingExport->export($raListings, $command->getFileName()); } /** diff --git a/src/Surfnet/StepupRa/RaBundle/Tests/Command/ExportRaListingCommandTest.php b/src/Surfnet/StepupRa/RaBundle/Tests/Command/ExportRaListingCommandTest.php index d30b59a0..f1f482f1 100644 --- a/src/Surfnet/StepupRa/RaBundle/Tests/Command/ExportRaListingCommandTest.php +++ b/src/Surfnet/StepupRa/RaBundle/Tests/Command/ExportRaListingCommandTest.php @@ -51,6 +51,8 @@ public function from_search_command_copies_the_relevant_filter_fields() $this->assertSame('jane@example.org', $exportCommand->email); $this->assertSame('institution-a', $exportCommand->institution); $this->assertSame($roleAtInstitution, $exportCommand->roleAtInstitution); + $this->assertSame('name', $exportCommand->orderBy); + $this->assertSame('desc', $exportCommand->orderDirection); } #[Test] diff --git a/src/Surfnet/StepupRa/RaBundle/Tests/Service/RaListingExportTest.php b/src/Surfnet/StepupRa/RaBundle/Tests/Service/RaListingExportTest.php index 5716e86c..f7125ad8 100644 --- a/src/Surfnet/StepupRa/RaBundle/Tests/Service/RaListingExportTest.php +++ b/src/Surfnet/StepupRa/RaBundle/Tests/Service/RaListingExportTest.php @@ -72,6 +72,51 @@ public function it_streams_a_csv_with_a_header_row_and_a_row_per_listing() ); } + #[Test] + public function it_neutralizes_values_that_could_be_interpreted_as_spreadsheet_formulas() + { + $logger = Mockery::mock(LoggerInterface::class); + $logger->shouldReceive('notice'); + + $export = new RaListingExport($logger); + + $raListing = new RaListing(); + $raListing->identityId = 'identity-id'; + $raListing->commonName = '=1+1'; + $raListing->email = 'jane@example.org'; + $raListing->institution = '-2+3'; + $raListing->role = '@SUM(1,1)'; + $raListing->raInstitution = 'institution-a'; + $raListing->location = 'Room 101'; + // A leading "+" is intentionally left untouched, since this column holds + // international phone numbers (e.g. "+31 6 12345678"). + $raListing->contactInformation = '+31 6 12345678'; + + $response = $export->export([$raListing], 'ra_export_2026-07-24'); + + ob_start(); + $response->sendContent(); + $csv = ob_get_clean(); + + $rows = array_map( + fn(string $line) => str_getcsv($line, escape: ''), + explode("\n", rtrim(str_replace("\r\n", "\n", $csv), "\n")), + ); + + $this->assertSame( + [ + "'=1+1", + 'jane@example.org', + "'-2+3", + "'@SUM(1,1)", + 'institution-a', + 'Room 101', + '+31 6 12345678', + ], + $rows[1], + ); + } + #[Test] public function it_streams_only_the_header_row_when_there_are_no_listings() { diff --git a/src/Surfnet/StepupRa/RaBundle/Tests/Service/RaListingServiceTest.php b/src/Surfnet/StepupRa/RaBundle/Tests/Service/RaListingServiceTest.php index ff871dc5..61e3f807 100644 --- a/src/Surfnet/StepupRa/RaBundle/Tests/Service/RaListingServiceTest.php +++ b/src/Surfnet/StepupRa/RaBundle/Tests/Service/RaListingServiceTest.php @@ -57,10 +57,7 @@ public function export_pages_through_all_results_and_hands_the_full_set_to_the_e ->shouldReceive('export') ->once() ->with( - Mockery::on(function (array $listings) { - return array_map(fn (RaListing $listing) => $listing->identityId, $listings) - === ['identity-1', 'identity-2', 'identity-3']; - }), + $this->matchesIdentityIdsOnce(['identity-1', 'identity-2', 'identity-3']), 'ra-raa-export_' . (new DateTime())->format('Y-m-d'), ) ->andReturn($expectedResponse); @@ -73,6 +70,42 @@ public function export_pages_through_all_results_and_hands_the_full_set_to_the_e $this->assertSame($expectedResponse, $service->export($command)); } + #[Test] + public function export_preserves_the_requested_sort_order_across_pages() + { + $emptyPage = RaListingCollection::empty(); + + $apiService = Mockery::mock(ApiRaListingService::class); + $apiService + ->shouldReceive('search') + ->once() + ->with(Mockery::on( + fn (RaListingSearchQuery $query) => str_contains($query->toHttpQuery(), 'orderBy=email') + && str_contains($query->toHttpQuery(), 'orderDirection=desc'), + )) + ->andReturn($emptyPage); + + $expectedResponse = Mockery::mock(StreamedResponse::class); + $export = Mockery::mock(RaListingExport::class); + $export + ->shouldReceive('export') + ->once() + ->with( + $this->matchesIdentityIdsOnce([]), + Mockery::type('string'), + ) + ->andReturn($expectedResponse); + + $service = new RaListingService($apiService, $export); + + $command = new ExportRaListingCommand(); + $command->actorId = 'actor-id'; + $command->orderBy = 'email'; + $command->orderDirection = 'desc'; + + $this->assertSame($expectedResponse, $service->export($command)); + } + #[Test] public function export_only_queries_a_single_page_when_there_are_no_results() { @@ -89,7 +122,10 @@ public function export_only_queries_a_single_page_when_there_are_no_results() $export ->shouldReceive('export') ->once() - ->with([], Mockery::type('string')) + ->with( + $this->matchesIdentityIdsOnce([]), + Mockery::type('string'), + ) ->andReturn($expectedResponse); $service = new RaListingService($apiService, $export); @@ -100,6 +136,31 @@ public function export_only_queries_a_single_page_when_there_are_no_results() $this->assertSame($expectedResponse, $service->export($command)); } + /** + * Builds a Mockery::on() matcher that materializes an iterable/generator argument + * exactly once. Mockery may re-evaluate an argument matcher after the initial call + * (e.g. while verifying expectation counts), and generators cannot be traversed twice, + * so the result of the first evaluation is cached and reused on any later calls. + * + * @param string[] $expectedIdentityIds + */ + private function matchesIdentityIdsOnce(array $expectedIdentityIds): Mockery\Matcher\Closure + { + $matches = null; + + return Mockery::on(function (iterable $listings) use ($expectedIdentityIds, &$matches) { + if ($matches === null) { + $identityIds = array_map( + fn (RaListing $listing) => $listing->identityId, + iterator_to_array($listings, preserve_keys: false), + ); + $matches = $identityIds === $expectedIdentityIds; + } + + return $matches; + }); + } + /** * @param string[] $identityIds */