Add CSV export to RA Management search results - #521
Conversation
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: #498
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().
Covers ExportRaListingCommand, RaListingExport and RaListingService.
| $form = $this->createForm(SearchRaListingType::class, $command, ['method' => 'get']); | ||
| $form->handleRequest($request); | ||
|
|
||
| if ($form->isSubmitted() && $form->getClickedButton()?->getName() === 'export') { |
There was a problem hiding this comment.
🤔 The getClickedButton does seem to be officially supported (https://symfony.com/doc/current/forms.html#handling-multiple-submit-buttons)
But it still needs a phpstan exception. Is the $this->createForm() typehint correct?
nitpick
| $query = $this->buildQuery( | ||
| $command->actorId, | ||
| $pageNumber, | ||
| $command->name, | ||
| $command->email, | ||
| $command->institution, | ||
| $command->roleAtInstitution, | ||
| ); |
There was a problem hiding this comment.
Export pages through an unordered result set, so rows can be duplicated or dropped
export() calls buildQuery() without orderBy/orderDirection. On the middleware side RaListingController sets $query->orderBy = $request->query->getString('orderBy') (so '' when absent), and RaListingRepository::createSearchQuery() returns the query with no ORDER BY in that case. Each page is then a separate LIMIT/OFFSET query over an authorization-filtered join, and MariaDB gives no ordering guarantee between those separate statements, so the aggregated CSV can contain the same RA twice and silently miss others. The paginated UI hides this because it only ever fetches one page.
Suggested approach: force a deterministic sort for the export (commonName is the only column the middleware accepts), or add an unpaginated export endpoint like the token export has.
$query = $this->buildQuery(
$command->actorId,
$pageNumber,
$command->name,
$command->email,
$command->institution,
$command->roleAtInstitution,
'commonName',
'asc',
);| fputcsv($handle, $columnNames); | ||
| foreach ($raListings as $raListing) { | ||
| fputcsv($handle, [ |
There was a problem hiding this comment.
fputcsv() is called without $escape
PHP 8.4 deprecates the implicit default (the new tests emit two deprecations on newer PHP), and the legacy \\ escape produces non-RFC4180 output for values ending in a backslash. The test itself reads back with str_getcsv(..., escape: ''), which shows the asymmetry.
Suggested approach: pass escape: '' explicitly to both fputcsv() calls.
| ->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); |
There was a problem hiding this comment.
Export tests only assert page numbers, not filter propagation
Both tests match on p=1/p=2 only, so a regression that drops name, email, institution or the role filter from the export query would still pass while leaking rows outside the requested filter into the CSV.
Suggested approach: assert on the full toHttpQuery() string for at least one fully-populated ExportRaListingCommand.
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.
2 participants
Summary
RaListingService::export()pages through the existing paginated middleware search (no unpaginated export endpoint exists for RA listings) and streams the aggregated results as CSV via a newRaListingExportservice.ra_export_YYYY-MM-DD,raa_export_YYYY-MM-DD, orra-raa-export_YYYY-MM-DDwhen unfiltered.export()keeps its ownROLE_RAAauthorization check, independent ofmanage().Test plan
php -lon all changed/new PHP filesservices.ymlExportRaListingCommand::fromSearchCommand,RaListingService::export,RaListingExport::export(not yet added)Closes #498