From f59f8004763250b5bf84f03d1dda8ae2dedd99b0 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Thu, 28 May 2026 06:25:48 +0200 Subject: [PATCH] fix(auth): add missing admin gates and remove debug/deprecated endpoints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit H1: exportOrgArchiMate — add admin-or-org-admin gate (queries configured organization-admin groups from SettingsService). H3: getGenericUserGroups, getOrganizationAdminGroups, getSuperUserGroups — add isAdmin() check to match the corresponding setter endpoints. H4: getAllGroups — admin-gate; getCronjobUsers/getCronjobOrganisations — return HTTP 410 Gone (deprecated, enumerate entire NC user/group list). H6: clearArchiMateImportStatus, killArchiMateImport, cancelArchiMateImport, clearArchiMateExportStatus — add isAdmin() gate to prevent DoS of admin import operations by any authenticated user. M3: getProgress/streamProgress — check owner_uid in progress data; add owner_uid field to ProgressTracker::startOperation (optional param). M4: bulkSyncStandards — add explicit auth + admin guard (was only CSRF-free). M5: testBulkUserInfo — remove debug endpoint and its route entry. M6: Content-Disposition filename headers — RFC 5987 encoding with filename*=UTF-8'' prefix on both exportOrgArchiMate and download. --- appinfo/routes.php | 1 - lib/Controller/ContactpersonenController.php | 65 ------- lib/Controller/SettingsController.php | 190 ++++++++++++------- lib/Service/ProgressTracker.php | 8 +- 4 files changed, 129 insertions(+), 135 deletions(-) diff --git a/appinfo/routes.php b/appinfo/routes.php index 961c340e..71b655f1 100644 --- a/appinfo/routes.php +++ b/appinfo/routes.php @@ -146,7 +146,6 @@ ['name' => 'contactpersonen#changePassword', 'url' => '/api/contactpersonen/change-password', 'verb' => 'POST'], ['name' => 'contactpersonen#updateUserGroups', 'url' => '/api/contactpersonen/update-groups', 'verb' => 'POST'], ['name' => 'contactpersonen#getUserInfo', 'url' => '/api/contactpersonen/{contactpersoonId}/user-info', 'verb' => 'GET'], - ['name' => 'contactpersonen#testBulkUserInfo', 'url' => '/api/contactpersonen/test-bulk-user-info', 'verb' => 'GET'], ['name' => 'contactpersonen#getBulkUserInfo', 'url' => '/api/contactpersonen/bulk-user-info', 'verb' => 'POST'], ['name' => 'contactpersonen#getAvailableGroups', 'url' => '/api/contactpersonen/available-groups', 'verb' => 'GET'], ['name' => 'contactpersonen#disableUser', 'url' => '/api/contactpersonen/{contactpersoonId}/disable', 'verb' => 'POST'], diff --git a/lib/Controller/ContactpersonenController.php b/lib/Controller/ContactpersonenController.php index dda3803c..a5f74bcf 100644 --- a/lib/Controller/ContactpersonenController.php +++ b/lib/Controller/ContactpersonenController.php @@ -1176,71 +1176,6 @@ public function enableUser(string $contactpersoonId): JSONResponse }//end try }//end enableUser() - /** - * Test endpoint to debug bulk user info. - * - * @return JSONResponse Debug information about available services. - * - * @NoAdminRequired - * @NoCSRFRequired - * @spec openspec/changes/retrofit-2026-05-26-contactpersonen-api/tasks.md#task-1 - */ - public function testBulkUserInfo(): JSONResponse - { - if ($this->userSession->getUser() === null) { - return new JSONResponse(['message' => 'Not authenticated'], Http::STATUS_UNAUTHORIZED); - } - - try { - $objectServiceAvail = 'null'; - if ($this->contactSvc !== null) { - } - - $userManagerAvail = 'null'; - if ($this->userManager !== null) { - } - - $groupManagerAvail = 'null'; - if ($this->groupManager !== null) { - } - - $this->logger->info( - 'testBulkUserInfo called', - [ - 'objectService' => $objectServiceAvail, - 'userManager' => $userManagerAvail, - 'groupManager' => $groupManagerAvail, - ] - ); - - return new JSONResponse( - [ - 'success' => true, - 'message' => 'Test endpoint working', - 'services' => [ - 'objectService' => $objectServiceAvail, - 'userManager' => $userManagerAvail, - 'groupManager' => $groupManagerAvail, - ], - ] - ); - } catch (\Exception $e) { - $this->logger->error( - 'Test endpoint error', - [ - 'error' => $e->getMessage(), - ] - ); - return new JSONResponse( - [ - 'success' => false, - 'message' => $e->getMessage(), - ], - 500 - ); - }//end try - }//end testBulkUserInfo() - /** * Get user info for multiple contactpersonen in one request. * diff --git a/lib/Controller/SettingsController.php b/lib/Controller/SettingsController.php index 6ca1b52a..caaa386e 100644 --- a/lib/Controller/SettingsController.php +++ b/lib/Controller/SettingsController.php @@ -1201,7 +1201,8 @@ public function consolidatedAutoConfigure(): JSONResponse */ public function getProgress(string $operationId): JSONResponse { - if ($this->userSession->getUser() === null) { + $currentUser = $this->userSession->getUser(); + if ($currentUser === null) { return new JSONResponse(['message' => 'Not authenticated'], Http::STATUS_UNAUTHORIZED); } @@ -1219,6 +1220,18 @@ public function getProgress(string $operationId): JSONResponse ); } + // Verify the caller owns this operation. + if (isset($progress['owner_uid']) === true && $progress['owner_uid'] !== $currentUser->getUID()) { + return new JSONResponse( + [ + 'success' => false, + 'message' => 'Operation not found', + 'error' => 'OPERATION_NOT_FOUND', + ], + 404 + ); + } + return new JSONResponse( [ 'success' => true, @@ -1262,10 +1275,17 @@ public function getProgress(string $operationId): JSONResponse */ public function streamProgress(string $operationId): Response { - if ($this->userSession->getUser() === null) { + $currentUser = $this->userSession->getUser(); + if ($currentUser === null) { return new JSONResponse(['message' => 'Not authenticated'], Http::STATUS_UNAUTHORIZED); } + // Verify the caller owns this operation before streaming. + $progress = $this->progressTracker->getProgress($operationId); + if ($progress !== null && isset($progress['owner_uid']) === true && $progress['owner_uid'] !== $currentUser->getUID()) { + return new JSONResponse(['message' => 'Operation not found', 'error' => 'OPERATION_NOT_FOUND'], 404); + } + // Set headers for Server-Sent Events. $response = new class($operationId, $this->progressTracker, $this->logger) extends Response { /** @@ -1671,10 +1691,28 @@ public function render(): string */ public function exportOrgArchiMate(string $organizationUuid): Response { - if ($this->userSession->getUser() === null) { + $currentUser = $this->userSession->getUser(); + if ($currentUser === null) { return new JSONResponse(['message' => 'Not authenticated'], Http::STATUS_UNAUTHORIZED); } + // Require admin or organisation-admin group membership. + $isAdmin = $this->groupManager->isAdmin($currentUser->getUID()); + if ($isAdmin === false) { + $orgAdminGroups = $this->settingsService->getOrganizationAdminGroups(); + $isOrgAdmin = false; + foreach ($orgAdminGroups as $groupName) { + if ($this->groupManager->isInGroup($currentUser->getUID(), $groupName) === true) { + $isOrgAdmin = true; + break; + } + } + + if ($isOrgAdmin === false) { + return new JSONResponse(['message' => 'Admin or organisation-admin privileges required'], Http::STATUS_FORBIDDEN); + } + } + try { // Read boolean query parameters. $modules = $this->request->getParam('modules', 'true') === 'true'; @@ -1739,7 +1777,10 @@ public function render(): string $response->setStatus(200); $response->addHeader('Content-Type', 'application/xml'); - $response->addHeader('Content-Disposition', 'attachment; filename="'.$fileName.'"'); + $response->addHeader( + 'Content-Disposition', + 'attachment; filename="'.addslashes($fileName).'"; filename*=UTF-8\'\''.rawurlencode($fileName) + ); $response->addHeader('Content-Length', (string) strlen($xmlContent)); $response->addHeader('Cache-Control', 'no-cache'); @@ -1839,7 +1880,10 @@ public function downloadArchiMate(string $fileName): Response // Create download response. $response = new StreamResponse($file->fopen('r')); $response->addHeader('Content-Type', $contentType); - $response->addHeader('Content-Disposition', 'attachment; filename="'.$fileName.'"'); + $response->addHeader( + 'Content-Disposition', + 'attachment; filename="'.addslashes($fileName).'"; filename*=UTF-8\'\''.rawurlencode($fileName) + ); $response->addHeader('Content-Length', (string) $file->getSize()); return $response; @@ -2298,10 +2342,15 @@ public function getEmailTemplateVariables(string $templateName): JSONResponse */ public function getGenericUserGroups(): JSONResponse { - if ($this->userSession->getUser() === null) { + $currentUser = $this->userSession->getUser(); + if ($currentUser === null) { return new JSONResponse(['message' => 'Not authenticated'], Http::STATUS_UNAUTHORIZED); } + if ($this->groupManager->isAdmin($currentUser->getUID()) === false) { + return new JSONResponse(['message' => 'Admin privileges required'], Http::STATUS_FORBIDDEN); + } + try { $groups = $this->settingsService->getGenericUserGroups(); @@ -2390,10 +2439,15 @@ public function setGenericUserGroups(): JSONResponse */ public function getOrganizationAdminGroups(): JSONResponse { - if ($this->userSession->getUser() === null) { + $currentUser = $this->userSession->getUser(); + if ($currentUser === null) { return new JSONResponse(['message' => 'Not authenticated'], Http::STATUS_UNAUTHORIZED); } + if ($this->groupManager->isAdmin($currentUser->getUID()) === false) { + return new JSONResponse(['message' => 'Admin privileges required'], Http::STATUS_FORBIDDEN); + } + try { $groups = $this->settingsService->getOrganizationAdminGroups(); @@ -2482,10 +2536,15 @@ public function setOrganizationAdminGroups(): JSONResponse */ public function getSuperUserGroups(): JSONResponse { - if ($this->userSession->getUser() === null) { + $currentUser = $this->userSession->getUser(); + if ($currentUser === null) { return new JSONResponse(['message' => 'Not authenticated'], Http::STATUS_UNAUTHORIZED); } + if ($this->groupManager->isAdmin($currentUser->getUID()) === false) { + return new JSONResponse(['message' => 'Admin privileges required'], Http::STATUS_FORBIDDEN); + } + try { $groups = $this->settingsService->getSuperUserGroups(); @@ -2574,10 +2633,15 @@ public function setSuperUserGroups(): JSONResponse */ public function getAllGroups(): JSONResponse { - if ($this->userSession->getUser() === null) { + $currentUser = $this->userSession->getUser(); + if ($currentUser === null) { return new JSONResponse(['message' => 'Not authenticated'], Http::STATUS_UNAUTHORIZED); } + if ($this->groupManager->isAdmin($currentUser->getUID()) === false) { + return new JSONResponse(['message' => 'Admin privileges required'], Http::STATUS_FORBIDDEN); + } + try { $allGroups = $this->settingsService->getAllGroups(); @@ -2619,10 +2683,15 @@ public function getAllGroups(): JSONResponse */ public function clearArchiMateImportStatus(): JSONResponse { - if ($this->userSession->getUser() === null) { + $currentUser = $this->userSession->getUser(); + if ($currentUser === null) { return new JSONResponse(['message' => 'Not authenticated'], Http::STATUS_UNAUTHORIZED); } + if ($this->groupManager->isAdmin($currentUser->getUID()) === false) { + return new JSONResponse(['message' => 'Admin privileges required'], Http::STATUS_FORBIDDEN); + } + try { $result = $this->settingsService->clearArchiMateImportStatus(); @@ -2663,10 +2732,15 @@ public function clearArchiMateImportStatus(): JSONResponse */ public function killArchiMateImport(): JSONResponse { - if ($this->userSession->getUser() === null) { + $currentUser = $this->userSession->getUser(); + if ($currentUser === null) { return new JSONResponse(['message' => 'Not authenticated'], Http::STATUS_UNAUTHORIZED); } + if ($this->groupManager->isAdmin($currentUser->getUID()) === false) { + return new JSONResponse(['message' => 'Admin privileges required'], Http::STATUS_FORBIDDEN); + } + try { $result = $this->settingsService->killArchiMateImport(); @@ -2706,16 +2780,18 @@ public function killArchiMateImport(): JSONResponse */ public function cancelArchiMateImport(): JSONResponse { - if ($this->userSession->getUser() === null) { + $currentUser = $this->userSession->getUser(); + if ($currentUser === null) { return new JSONResponse(['message' => 'Not authenticated'], Http::STATUS_UNAUTHORIZED); } - try { - $result = $this->settingsService->cancelArchiMateImport(); + if ($this->groupManager->isAdmin($currentUser->getUID()) === false) { + return new JSONResponse(['message' => 'Admin privileges required'], Http::STATUS_FORBIDDEN); + } - $message = 'ArchiMate import cancellation failed'; - if ($result['cancelled'] === true) { - } + try { + $result = $this->settingsService->cancelArchiMateImport(); + $message = ($result['cancelled'] === true) ? 'ArchiMate import cancellation succeeded' : 'ArchiMate import cancellation failed'; return new JSONResponse( [ @@ -2752,10 +2828,15 @@ public function cancelArchiMateImport(): JSONResponse */ public function clearArchiMateExportStatus(): JSONResponse { - if ($this->userSession->getUser() === null) { + $currentUser = $this->userSession->getUser(); + if ($currentUser === null) { return new JSONResponse(['message' => 'Not authenticated'], Http::STATUS_UNAUTHORIZED); } + if ($this->groupManager->isAdmin($currentUser->getUID()) === false) { + return new JSONResponse(['message' => 'Admin privileges required'], Http::STATUS_FORBIDDEN); + } + try { $this->settingsService->clearArchiMateExportStatus(); @@ -3479,6 +3560,15 @@ public function syncOrganisations(): JSONResponse */ public function bulkSyncStandards(): JSONResponse { + $currentUser = $this->userSession->getUser(); + if ($currentUser === null) { + return new JSONResponse(['message' => 'Not authenticated'], Http::STATUS_UNAUTHORIZED); + } + + if ($this->groupManager->isAdmin($currentUser->getUID()) === false) { + return new JSONResponse(['message' => 'Admin privileges required'], Http::STATUS_FORBIDDEN); + } + try { $this->logger->info('SettingsController: Starting bulk sync of module standards.'); @@ -3617,66 +3707,34 @@ public function updateCronjobConfig(): JSONResponse */ public function getCronjobUsers(): JSONResponse { - if ($this->userSession->getUser() === null) { - return new JSONResponse(['message' => 'Not authenticated'], Http::STATUS_UNAUTHORIZED); - } - - try { - $result = $this->settingsService->getAvailableUsersForCronjobs(); - return new JSONResponse($result); - } catch (\Exception $e) { - $this->logger->error( - 'Failed to get cronjob users', - [ - 'exception' => $e->getMessage(), - ] - ); - return new JSONResponse( - [ - 'success' => false, - 'message' => 'Failed to get cronjob users: '.$e->getMessage(), - 'users' => [], - ], - 500 - ); - } + return new JSONResponse( + [ + 'success' => false, + 'message' => 'This endpoint is deprecated and has been removed. Cronjob user context is no longer required.', + ], + Http::STATUS_GONE + ); }//end getCronjobUsers() /** * Get available organisations for cronjob configuration * - * @deprecated Cronjob context is no longer needed. Will be removed in a future version. + * @deprecated Removed — cronjob context is no longer needed. * * @NoAdminRequired * @NoCSRFRequired * - * @return JSONResponse List of available organisations + * @return JSONResponse 410 Gone * @spec openspec/changes/retrofit-2026-05-26-settings-admin-controller/tasks.md#task-5 */ public function getCronjobOrganisations(): JSONResponse { - if ($this->userSession->getUser() === null) { - return new JSONResponse(['message' => 'Not authenticated'], Http::STATUS_UNAUTHORIZED); - } - - try { - $result = $this->settingsService->getAvailableOrganisationsForCronjobs(); - return new JSONResponse($result); - } catch (\Exception $e) { - $this->logger->error( - 'Failed to get cronjob organisations', - [ - 'exception' => $e->getMessage(), - ] - ); - return new JSONResponse( - [ - 'success' => false, - 'message' => 'Failed to get cronjob organisations: '.$e->getMessage(), - 'organisations' => [], - ], - 500 - ); - } + return new JSONResponse( + [ + 'success' => false, + 'message' => 'This endpoint is deprecated and has been removed. Cronjob organisation context is no longer required.', + ], + Http::STATUS_GONE + ); }//end getCronjobOrganisations() }//end class diff --git a/lib/Service/ProgressTracker.php b/lib/Service/ProgressTracker.php index aa22d5f2..cd40bb8e 100644 --- a/lib/Service/ProgressTracker.php +++ b/lib/Service/ProgressTracker.php @@ -89,20 +89,22 @@ public function __construct( /** * Start tracking a new operation * - * @param string $operationType Type of operation (import, export) - * @param array $options Operation options and metadata + * @param string $operationType Type of operation (import, export) + * @param array $options Operation options and metadata + * @param string|null $ownerUid UID of the user who owns this operation * * @return string Unique operation ID * * @spec openspec/changes/retrofit-2026-05-24-progress-tracking/tasks.md#task-1 */ - public function startOperation(string $operationType, array $options=[]): string + public function startOperation(string $operationType, array $options=[], ?string $ownerUid=null): string { $operationId = uniqid(prefix: $operationType.'_', more_entropy: true); $this->progress = [ 'operation_id' => $operationId, 'operation_type' => $operationType, + 'owner_uid' => $ownerUid, 'phase' => 'initializing', 'phase_description' => self::PHASES['initializing']['description'], 'total_items' => $options['total_items'] ?? 0,