From ab1e8cd458a7cf37ca701db47d5554d596da4a0a Mon Sep 17 00:00:00 2001 From: rinkp Date: Tue, 23 Jul 2024 01:07:47 +0200 Subject: [PATCH 01/10] [api] Introduce response statuses --- .../Database/src/Controller/ApiController.php | 39 +++++++++++++++---- .../src/Model/Enums/ApiResponseStatuses.php | 20 ++++++++++ .../src/Listener/AuthorizationListener.php | 12 ++++-- openapi.yaml | 14 ++++++- 4 files changed, 73 insertions(+), 12 deletions(-) create mode 100644 module/Database/src/Model/Enums/ApiResponseStatuses.php diff --git a/module/Database/src/Controller/ApiController.php b/module/Database/src/Controller/ApiController.php index 8d9eb4f5..b0187751 100644 --- a/module/Database/src/Controller/ApiController.php +++ b/module/Database/src/Controller/ApiController.php @@ -4,9 +4,11 @@ namespace Database\Controller; +use Database\Model\Enums\ApiResponseStatuses; use Database\Service\Api as ApiService; use Laminas\Http\Response; use Laminas\Mvc\Controller\AbstractActionController; +use Laminas\Stdlib\ResponseInterface; use Laminas\View\Model\JsonModel; use User\Model\Enums\ApiPermissions; use User\Service\ApiAuthenticationService; @@ -30,7 +32,12 @@ public function healthyAction(): JsonModel $syncPaused = $this->apiService->isSyncPaused(); - return new JsonModel(['healthy' => true, 'sync_paused' => $syncPaused]); + return new JsonModel([ + 'status' => ApiResponseStatuses::Success, + 'healthy' => true, + 'sync_paused' => $syncPaused, + ]); + } } /** @@ -48,7 +55,10 @@ public function membersAction(): JsonModel $allowDeleted = $this->apiAuthService->currentUserCan(ApiPermissions::MembersDeleted); $members = $this->apiService->getMembers($additionalProperties, $allowDeleted); - $res = ['data' => $members]; + $res = [ + 'status' => ApiResponseStatuses::Success, + 'data' => $members, + ]; return new JsonModel($res); } @@ -56,7 +66,7 @@ public function membersAction(): JsonModel /** * Return member */ - public function memberAction(): JsonModel|Response + public function memberAction(): JsonModel|ResponseInterface { $this->apiAuthService->assertCan(ApiPermissions::MembersR); @@ -70,7 +80,10 @@ public function memberAction(): JsonModel|Response return $this->noContent(); } - $res = ['data' => $member]; + $res = [ + 'status' => ApiResponseStatuses::Success, + 'data' => $member, + ]; return new JsonModel($res); } @@ -90,7 +103,10 @@ public function membersActiveAction(): JsonModel $includeInactiveFraternity, $allowDeleted, ); - $res = ['data' => $members]; + $res = [ + 'status' => ApiResponseStatuses::Success, + 'data' => $members, + ]; return new JsonModel($res); } @@ -98,12 +114,19 @@ public function membersActiveAction(): JsonModel /** * To follow best practices, we generate a 204 for empty datasets */ - private function noContent(): Response + private function noContent(): JsonModel { - $response = new Response(); + $response = $this->getResponse(); + if ($response instanceof Response) { $response->setStatusCode(Response::STATUS_CODE_204); + } - return $response; + $res = [ + 'status' => ApiResponseStatuses::Success, + 'data' => null, + ]; + + return new JsonModel($res); } /** diff --git a/module/Database/src/Model/Enums/ApiResponseStatuses.php b/module/Database/src/Model/Enums/ApiResponseStatuses.php new file mode 100644 index 00000000..19506934 --- /dev/null +++ b/module/Database/src/Model/Enums/ApiResponseStatuses.php @@ -0,0 +1,20 @@ +stopPropagation(true); $e->setViewModel(new JsonModel([ - 'status' => 'error', + 'status' => ApiResponseStatuses::Error, 'error' => [ 'type' => NotAllowedException::class, 'message' => $e->getParam('exception')->getMessage(), ], ])); - $e->getResponse()->setStatusCode(403); + $response = $e->getResponse(); + if ($response instanceof Response) { + $response->setStatusCode(Response::STATUS_CODE_403); + } + + $e->stopPropagation(true); } } diff --git a/openapi.yaml b/openapi.yaml index 782f5b15..188106e6 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -9,7 +9,7 @@ info: license: name: GNU GENERAL PUBLIC LICENSE Version 3 url: https://github.com/GEWIS/gewisdb/blob/main/LICENSE.txt - version: 61a3f816c126d454d7bd665dd6788f9d7a619961 + version: 3.0.1 externalDocs: description: Contribute to this API url: https://github.com/GEWIS/gewisdb @@ -90,6 +90,9 @@ paths: schema: type: object properties: + status: + type: string + example: "success" data: type: array items: @@ -121,6 +124,9 @@ paths: schema: type: object properties: + status: + type: string + example: "success" data: $ref: '#/components/schemas/Member' 204: @@ -153,6 +159,9 @@ paths: schema: type: object properties: + status: + type: string + example: "success" data: type: array items: @@ -168,6 +177,9 @@ components: health: type: object properties: + status: + type: string + example: "success" healthy: type: boolean sync_paused: From 782bcb0170dc78f5c26f229a909796780cc19dd5 Mon Sep 17 00:00:00 2001 From: rinkp Date: Tue, 23 Jul 2024 01:08:59 +0200 Subject: [PATCH 02/10] [api] Handle 404 and 500 errors in JsonModel --- .../DispatchErrorFormatterListener.php | 140 ++++++++++++++++++ module/User/src/Module.php | 17 +-- openapi.yaml | 38 +++++ 3 files changed, 185 insertions(+), 10 deletions(-) create mode 100644 module/User/src/Listener/DispatchErrorFormatterListener.php diff --git a/module/User/src/Listener/DispatchErrorFormatterListener.php b/module/User/src/Listener/DispatchErrorFormatterListener.php new file mode 100644 index 00000000..964074c5 --- /dev/null +++ b/module/User/src/Listener/DispatchErrorFormatterListener.php @@ -0,0 +1,140 @@ +getUri(); + $path = $uri->getPath(); + $match = null; + + while (null === $match && false !== strpos($path, '/')) { + $path = substr($path, 0, strrpos($path, '/')); + if ('' === $path) { + $uri->setPath('/'); + } else { + $uri->setPath($path); + } + + $request->setUri($uri); + $match = $router->match($request); + } + + return $match; + } + + private function isApiMatch(RouteMatch $match): bool + { + return AuthenticationListener::AUTH_API === $match->getParam('auth_type', AuthenticationListener::AUTH_DBUSER); + } + + private function handleNoMatchedRoute(MvcEvent $e): void + { + $router = $e->getRouter(); + $request = $e->getRequest(); + + // If this is not a HTTP request, we cannot assume anything about routes + if (!($request instanceof Request)) { + return; + } + + $match = $this->matchAncestorRoute($request, $router); + + // Regular routes are dealt with by default handling + if (!$this->isApiMatch($match)) { + return; + } + + // If this is probably an API route, response should be JSON + $view = new JsonModel([ + 'status' => ApiResponseStatuses::NotFound, + 'error' => [ + 'type' => $e->getError(), + 'exception' => $e->getParam('exception')?->getMessage(), + ], + ]); + $e->setViewModel($view); + $response = $e->getResponse(); + if ($response instanceof Response) { + $response->setStatusCode(Response::STATUS_CODE_404); + } + + $e->stopPropagation(true); + } + + private function handleMatchedRoute( + MvcEvent $e, + RouteMatch $match, + ): void { + // Regular routes are dealt with by default handling + if (!$this->isApiMatch($match)) { + return; + } + + // If this is probably an API route, response should be JSON + $view = new JsonModel([ + 'status' => ApiResponseStatuses::Error, + 'error' => [ + 'type' => $e->getError(), + 'exception' => $e->getParam('exception')?->getMessage(), + ], + ]); + $e->setViewModel($view); + $response = $e->getResponse(); + if ($response instanceof Response) { + $response->setStatusCode(Response::STATUS_CODE_500); + } + + $e->stopPropagation(true); + } + + public function __invoke(MvcEvent $e): void + { + /** + * The source code is public so we can give away 404 errors if it is because of no route + * This does not include 404 responses that are returned by logic, such as when a member does not exist + **/ + if ('error-router-no-match' === $e->getError()) { + $this->handleNoMatchedRoute($e); + + return; + } + + /** + * If this is not an error-router-no-match error, we must have a matching route + */ + $match = $e->getRouteMatch(); + + // We should always have a match here; if we do not, throw an exception + // possibly including previous exceptions + if (null === $match) { + throw new LogicException( + message: 'Assumed route would be present; no route present', + previous: $e->getParam('exception', null), + ); + } + + // If we do have a match, this implies we have properly authenticated before + $this->handleMatchedRoute($e, $match); + } +} diff --git a/module/User/src/Module.php b/module/User/src/Module.php index 22ad8db8..fb73bebb 100644 --- a/module/User/src/Module.php +++ b/module/User/src/Module.php @@ -11,6 +11,7 @@ use User\Adapter\ApiPrincipalAdapter; use User\Listener\AuthenticationListener; use User\Listener\AuthorizationListener; +use User\Listener\DispatchErrorFormatterListener; use User\Service\ApiAuthenticationService; class Module @@ -44,17 +45,13 @@ public function onBootstrap(MvcEvent $event): void $authorizationListener = new AuthorizationListener( $apiAuthService, ); - $eventManager->attach(MvcEvent::EVENT_DISPATCH_ERROR, $authorizationListener); + $eventManager->attach(MvcEvent::EVENT_DISPATCH_ERROR, $authorizationListener, 10); - $eventManager->attach(MvcEvent::EVENT_DISPATCH_ERROR, static function ($e) use ($authService) { - if (!$authService->hasIdentity()) { - $response = $e->getResponse(); - $response->getHeaders()->addHeaderLine('Location', '/login'); - $response->setStatusCode(302); - - return $response; - } - }); + /** + * Format errors in case of dispatch errors after authentication + */ + $dispatchErrorFormatterListener = new DispatchErrorFormatterListener(); + $eventManager->attach(MvcEvent::EVENT_DISPATCH_ERROR, $dispatchErrorFormatterListener, 5); } /** diff --git a/openapi.yaml b/openapi.yaml index 188106e6..a08464d9 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -328,6 +328,44 @@ components: type: string example: Testorgaan responses: + server_error: + description: There was an error in the server. Most likely, this is a permanent error. + content: + application/json: + schema: + type: object + properties: + status: + type: string + default: "error" + error: + type: object + properties: + type: + type: string + default: "error-exception" + exception: + type: string + example: "An example exception was thrown" + not_found_route: + description: This route does not exist (which is different from not finding a specific resource) + content: + application/json: + schema: + type: object + properties: + status: + type: string + default: "error" + error: + type: object + properties: + type: + type: string + default: "error-router-no-match" + exception: + type: string + example: null no_permission: description: The token that was used does not have the required permissions content: From b4dd17921de8fae40aa19d21045b832713ec4309 Mon Sep 17 00:00:00 2001 From: rinkp Date: Tue, 23 Jul 2024 01:09:20 +0200 Subject: [PATCH 03/10] [api] Introduce 404 and 500 test endpoints --- module/Database/config/module.config.php | 10 ++++++++++ .../Database/src/Controller/ApiController.php | 10 +++++++++- openapi.yaml | 20 ++++++++++++++++++- 3 files changed, 38 insertions(+), 2 deletions(-) diff --git a/module/Database/config/module.config.php b/module/Database/config/module.config.php index 6f1c9485..26490d6c 100644 --- a/module/Database/config/module.config.php +++ b/module/Database/config/module.config.php @@ -660,6 +660,16 @@ ], ], ], + 'example500' => [ + 'type' => Literal::class, + 'options' => [ + 'route' => '/example500', + 'defaults' => [ + 'controller' => ApiController::class, + 'action' => 'example500', + ], + ], + ], 'members' => [ 'type' => Literal::class, 'options' => [ diff --git a/module/Database/src/Controller/ApiController.php b/module/Database/src/Controller/ApiController.php index b0187751..9e3adf48 100644 --- a/module/Database/src/Controller/ApiController.php +++ b/module/Database/src/Controller/ApiController.php @@ -10,6 +10,7 @@ use Laminas\Mvc\Controller\AbstractActionController; use Laminas\Stdlib\ResponseInterface; use Laminas\View\Model\JsonModel; +use RuntimeException; use User\Model\Enums\ApiPermissions; use User\Service\ApiAuthenticationService; @@ -38,6 +39,13 @@ public function healthyAction(): JsonModel 'sync_paused' => $syncPaused, ]); } + + /** + * Error 500 action. + */ + public function example500Action(): JsonModel + { + throw new RuntimeException('An example exception was thrown.'); } /** @@ -118,7 +126,7 @@ private function noContent(): JsonModel { $response = $this->getResponse(); if ($response instanceof Response) { - $response->setStatusCode(Response::STATUS_CODE_204); + $response->setStatusCode(Response::STATUS_CODE_204); } $res = [ diff --git a/openapi.yaml b/openapi.yaml index a08464d9..7267b1d2 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -171,7 +171,25 @@ paths: security: - api_auth: - MembersActiveR - + /example404: + get: + summary: Example 404 + description: This 404 will be returned if there is a mistake in the requested path + tags: + - errors + responses: + 404: + $ref: '#/components/responses/not_found_route' + /example500: + get: + summary: Example 500 + description: This 500 will be returned if there is a mistake in the requested path + tags: + - errors + responses: + 500: + $ref: '#/components/responses/server_error' + components: schemas: health: From fe8e23067a4eb4ad82a72da356f0a7d420781bce Mon Sep 17 00:00:00 2001 From: rinkp Date: Wed, 24 Jul 2024 22:10:01 +0200 Subject: [PATCH 04/10] [style] `stopPropagation` default argument --- module/User/src/Listener/AuthenticationListener.php | 3 ++- module/User/src/Listener/AuthorizationListener.php | 2 +- module/User/src/Listener/DispatchErrorFormatterListener.php | 4 ++-- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/module/User/src/Listener/AuthenticationListener.php b/module/User/src/Listener/AuthenticationListener.php index f86a3360..761725d5 100644 --- a/module/User/src/Listener/AuthenticationListener.php +++ b/module/User/src/Listener/AuthenticationListener.php @@ -69,11 +69,12 @@ private function dbuserAuth(MvcEvent $e): ?Response return null; } - $e->stopPropagation(true); $response = $e->getResponse(); $response->getHeaders()->addHeaderLine('Location', '/login'); $response->setStatusCode(302); + $e->stopPropagation(); + return $response; } diff --git a/module/User/src/Listener/AuthorizationListener.php b/module/User/src/Listener/AuthorizationListener.php index c328af60..2875d430 100644 --- a/module/User/src/Listener/AuthorizationListener.php +++ b/module/User/src/Listener/AuthorizationListener.php @@ -39,6 +39,6 @@ public function __invoke(MvcEvent $e): void $response->setStatusCode(Response::STATUS_CODE_403); } - $e->stopPropagation(true); + $e->stopPropagation(); } } diff --git a/module/User/src/Listener/DispatchErrorFormatterListener.php b/module/User/src/Listener/DispatchErrorFormatterListener.php index 964074c5..0d2346f9 100644 --- a/module/User/src/Listener/DispatchErrorFormatterListener.php +++ b/module/User/src/Listener/DispatchErrorFormatterListener.php @@ -79,7 +79,7 @@ private function handleNoMatchedRoute(MvcEvent $e): void $response->setStatusCode(Response::STATUS_CODE_404); } - $e->stopPropagation(true); + $e->stopPropagation(); } private function handleMatchedRoute( @@ -105,7 +105,7 @@ private function handleMatchedRoute( $response->setStatusCode(Response::STATUS_CODE_500); } - $e->stopPropagation(true); + $e->stopPropagation(); } public function __invoke(MvcEvent $e): void From aec5019f853651c3ca6664eab3ae77d04d902632 Mon Sep 17 00:00:00 2001 From: rinkp Date: Wed, 24 Jul 2024 22:13:03 +0200 Subject: [PATCH 05/10] [style] Unused `$apiAuthService` --- module/User/src/Listener/AuthorizationListener.php | 6 ------ module/User/src/Module.php | 4 +--- 2 files changed, 1 insertion(+), 9 deletions(-) diff --git a/module/User/src/Listener/AuthorizationListener.php b/module/User/src/Listener/AuthorizationListener.php index 2875d430..a2d0bb0f 100644 --- a/module/User/src/Listener/AuthorizationListener.php +++ b/module/User/src/Listener/AuthorizationListener.php @@ -9,15 +9,9 @@ use Laminas\Mvc\MvcEvent; use Laminas\View\Model\JsonModel; use User\Model\Exception\NotAllowed as NotAllowedException; -use User\Service\ApiAuthenticationService; final class AuthorizationListener { - public function __construct( - private readonly ApiAuthenticationService $apiAuthService, - ) { - } - public function __invoke(MvcEvent $e): void { if ( diff --git a/module/User/src/Module.php b/module/User/src/Module.php index fb73bebb..ef2393de 100644 --- a/module/User/src/Module.php +++ b/module/User/src/Module.php @@ -42,9 +42,7 @@ public function onBootstrap(MvcEvent $event): void /** * Catch authorization exceptions */ - $authorizationListener = new AuthorizationListener( - $apiAuthService, - ); + $authorizationListener = new AuthorizationListener(); $eventManager->attach(MvcEvent::EVENT_DISPATCH_ERROR, $authorizationListener, 10); /** From 5f35dbe6939e66c56ec3d71b262be276f53e86cd Mon Sep 17 00:00:00 2001 From: rinkp Date: Wed, 24 Jul 2024 22:45:40 +0200 Subject: [PATCH 06/10] [style] `Response` -> `HttpResponse` clarification --- .../src/Controller/IndexController.php | 4 +- .../Database/src/Controller/ApiController.php | 6 +-- .../src/Controller/MeetingController.php | 6 +-- .../src/Controller/MemberController.php | 53 +++++++++++-------- .../ProspectiveMemberController.php | 6 +-- .../src/Controller/QueryController.php | 6 +-- .../src/Controller/SettingsController.php | 4 +- .../src/Controller/ApiSettingsController.php | 12 ++--- .../src/Controller/SettingsController.php | 8 +-- module/User/src/Controller/UserController.php | 6 +-- .../src/Listener/AuthenticationListener.php | 26 ++++++--- .../src/Listener/AuthorizationListener.php | 6 +-- .../DispatchErrorFormatterListener.php | 10 ++-- 13 files changed, 87 insertions(+), 66 deletions(-) diff --git a/module/Application/src/Controller/IndexController.php b/module/Application/src/Controller/IndexController.php index 63a1a679..3da85f60 100644 --- a/module/Application/src/Controller/IndexController.php +++ b/module/Application/src/Controller/IndexController.php @@ -4,7 +4,7 @@ namespace Application\Controller; -use Laminas\Http\Response; +use Laminas\Http\Response as HttpResponse; use Laminas\Mvc\Controller\AbstractActionController; use Laminas\Mvc\Plugin\Identity\Identity; use Laminas\Session\Container as SessionContainer; @@ -16,7 +16,7 @@ */ class IndexController extends AbstractActionController { - public function langAction(): Response + public function langAction(): HttpResponse { $session = new SessionContainer('lang'); $session->lang = $this->params()->fromRoute('lang'); diff --git a/module/Database/src/Controller/ApiController.php b/module/Database/src/Controller/ApiController.php index 9e3adf48..df0c1d07 100644 --- a/module/Database/src/Controller/ApiController.php +++ b/module/Database/src/Controller/ApiController.php @@ -6,7 +6,7 @@ use Database\Model\Enums\ApiResponseStatuses; use Database\Service\Api as ApiService; -use Laminas\Http\Response; +use Laminas\Http\Response as HttpResponse; use Laminas\Mvc\Controller\AbstractActionController; use Laminas\Stdlib\ResponseInterface; use Laminas\View\Model\JsonModel; @@ -125,8 +125,8 @@ public function membersActiveAction(): JsonModel private function noContent(): JsonModel { $response = $this->getResponse(); - if ($response instanceof Response) { - $response->setStatusCode(Response::STATUS_CODE_204); + if ($response instanceof HttpResponse) { + $response->setStatusCode(HttpResponse::STATUS_CODE_204); } $res = [ diff --git a/module/Database/src/Controller/MeetingController.php b/module/Database/src/Controller/MeetingController.php index d5c32cfa..4c642cda 100644 --- a/module/Database/src/Controller/MeetingController.php +++ b/module/Database/src/Controller/MeetingController.php @@ -11,7 +11,7 @@ use Database\Service\Api as ApiService; use Database\Service\Meeting as MeetingService; use Doctrine\DBAL\Exception\ForeignKeyConstraintViolationException; -use Laminas\Http\Response; +use Laminas\Http\Response as HttpResponse; use Laminas\Mvc\Controller\AbstractActionController; use Laminas\View\Model\JsonModel; use Laminas\View\Model\ViewModel; @@ -42,7 +42,7 @@ public function indexAction(): ViewModel /** * Create a new meeting. */ - public function createAction(): Response|ViewModel + public function createAction(): HttpResponse|ViewModel { $request = $this->getRequest(); @@ -159,7 +159,7 @@ protected function getDecisionForms( /** * Decision form action. */ - public function decisionformAction(): Response|ViewModel + public function decisionformAction(): HttpResponse|ViewModel { if (!$this->getRequest()->isPost()) { return $this->redirect()->toRoute('meeting'); diff --git a/module/Database/src/Controller/MemberController.php b/module/Database/src/Controller/MemberController.php index bcc42545..3fe197d3 100644 --- a/module/Database/src/Controller/MemberController.php +++ b/module/Database/src/Controller/MemberController.php @@ -11,10 +11,11 @@ use Database\Service\Stripe as StripeService; use DateTime; use Laminas\Http\Header\HeaderInterface; -use Laminas\Http\Response; +use Laminas\Http\Response as HttpResponse; use Laminas\Mvc\Controller\AbstractActionController; use Laminas\Mvc\I18n\Translator; use Laminas\Mvc\Plugin\FlashMessenger\FlashMessenger; +use Laminas\Stdlib\ResponseInterface; use Laminas\View\Model\JsonModel; use Laminas\View\Model\ViewModel; @@ -48,7 +49,7 @@ public function indexAction(): ViewModel /** * Subscribe action. */ - public function subscribeAction(): Response|ViewModel + public function subscribeAction(): HttpResponse|ViewModel { $ipMCS = ip2long($this->remoteAddress) >= ip2long('131.155.68.0') && ip2long($this->remoteAddress) <= ip2long('131.155.71.255'); @@ -118,7 +119,7 @@ public function checkoutStatusAction(): ViewModel return new ViewModel($results); } - public function checkoutRestartAction(): Response|ViewModel + public function checkoutRestartAction(): HttpResponse|ViewModel { $token = (string) $this->params()->fromRoute('token'); $paymentLink = $this->stripeService->getPaymentLink($token); @@ -138,10 +139,10 @@ public function checkoutRestartAction(): Response|ViewModel return $this->redirect() ->toUrl($restartedCheckoutLink) - ->setStatusCode(Response::STATUS_CODE_303); + ->setStatusCode(HttpResponse::STATUS_CODE_303); } - public function paymentWebhookAction(): Response + public function paymentWebhookAction(): ResponseInterface { $signature = $this->getRequest()->getHeader('Stripe-Signature'); @@ -153,13 +154,23 @@ public function paymentWebhookAction(): Response // (good) support for using Fibers to do this concurrently. $this->stripeService->handleEvent($event); - return $this->getResponse() - ->setStatusCode(Response::STATUS_CODE_200); + // We know this is always going to be an HttpResponse + $response = $this->getResponse(); + if ($response instanceof HttpResponse) { + $response->setStatusCode(HttpResponse::STATUS_CODE_200); + } + + return $response; } } - return $this->getResponse() - ->setStatusCode(Response::STATUS_CODE_400); + // We know this is always going to be an HttpResponse + $response = $this->getResponse(); + if ($response instanceof HttpResponse) { + $response->setStatusCode(HttpResponse::STATUS_CODE_400); + } + + return $response; } /** @@ -243,7 +254,7 @@ public function searchFilteredAction(): JsonModel * * Shows member information. */ - public function showAction(): Response|ViewModel + public function showAction(): HttpResponse|ViewModel { $lidnr = (int) $this->params()->fromRoute('id'); $member = $this->memberService->getMemberWithDecisions($lidnr); @@ -295,7 +306,7 @@ public function showAction(): Response|ViewModel * * Toggles if a member wants a supremum */ - public function setSupremumAction(): Response|ViewModel + public function setSupremumAction(): HttpResponse|ViewModel { $member = $this->memberService->getMember((int) $this->params()->fromRoute('id')); @@ -322,7 +333,7 @@ public function setSupremumAction(): Response|ViewModel * * Edit member information. */ - public function editAction(): Response|ViewModel + public function editAction(): HttpResponse|ViewModel { $member = $this->memberService->getMember((int) $this->params()->fromRoute('id')); @@ -367,7 +378,7 @@ public function editAction(): Response|ViewModel * * Delete a member. */ - public function deleteAction(): Response|ViewModel + public function deleteAction(): HttpResponse|ViewModel { $member = $this->memberService->getMember((int) $this->params()->fromRoute('id')); @@ -404,7 +415,7 @@ public function deleteAction(): Response|ViewModel * * Update list membership. */ - public function listsAction(): Response|ViewModel + public function listsAction(): HttpResponse|ViewModel { $member = $this->memberService->getMember((int) $this->params()->fromRoute('id')); @@ -449,7 +460,7 @@ public function listsAction(): Response|ViewModel * * Update / renew membership. */ - public function membershipAction(): Response|ViewModel + public function membershipAction(): HttpResponse|ViewModel { $member = $this->memberService->getMember((int) $this->params()->fromRoute('id')); @@ -505,7 +516,7 @@ public function membershipAction(): Response|ViewModel * * Extend the duration of the membership. */ - public function expirationAction(): Response|ViewModel + public function expirationAction(): HttpResponse|ViewModel { $member = $this->memberService->getMember((int) $this->params()->fromRoute('id')); @@ -553,7 +564,7 @@ public function expirationAction(): Response|ViewModel * * Edit a member's address. */ - public function editAddressAction(): Response|ViewModel + public function editAddressAction(): HttpResponse|ViewModel { $type = AddressTypes::tryFrom($this->params()->fromRoute('type')); @@ -610,7 +621,7 @@ public function editAddressAction(): Response|ViewModel * * Add a member's address. */ - public function addAddressAction(): Response|ViewModel + public function addAddressAction(): HttpResponse|ViewModel { $type = AddressTypes::tryFrom($this->params()->fromRoute('type')); @@ -672,7 +683,7 @@ public function addAddressAction(): Response|ViewModel * * Remove a member's address. */ - public function removeAddressAction(): Response|ViewModel + public function removeAddressAction(): HttpResponse|ViewModel { $type = AddressTypes::tryFrom($this->params()->fromRoute('type')); @@ -789,7 +800,7 @@ public function showUpdateAction(): ViewModel /** * Approve a pending member update. */ - public function approveUpdateAction(): Response|ViewModel + public function approveUpdateAction(): HttpResponse|ViewModel { $memberUpdate = $this->memberService->getPendingMemberUpdate((int) $this->params()->fromRoute('id')); @@ -835,7 +846,7 @@ public function approveUpdateAction(): Response|ViewModel /** * Reject a member update. */ - public function rejectUpdateAction(): Response|ViewModel + public function rejectUpdateAction(): HttpResponse|ViewModel { $memberUpdate = $this->memberService->getPendingMemberUpdate((int) $this->params()->fromRoute('id')); diff --git a/module/Database/src/Controller/ProspectiveMemberController.php b/module/Database/src/Controller/ProspectiveMemberController.php index 4b7f1843..018f7eae 100644 --- a/module/Database/src/Controller/ProspectiveMemberController.php +++ b/module/Database/src/Controller/ProspectiveMemberController.php @@ -7,7 +7,7 @@ use Database\Model\ProspectiveMember as ProspectiveMemberModel; use Database\Service\Member as MemberService; use Database\Service\Stripe as StripeService; -use Laminas\Http\Response; +use Laminas\Http\Response as HttpResponse; use Laminas\Mvc\Controller\AbstractActionController; use Laminas\Mvc\I18n\Translator; use Laminas\View\Model\JsonModel; @@ -72,7 +72,7 @@ public function showAction(): ViewModel * * Shows prospective member information. */ - public function finalizeAction(): Response + public function finalizeAction(): HttpResponse { $lidnr = (int) $this->params()->fromRoute('id'); @@ -111,7 +111,7 @@ public function finalizeAction(): Response * * Delete a prospective member. */ - public function deleteAction(): Response|ViewModel + public function deleteAction(): HttpResponse|ViewModel { $lidnr = (int) $this->params()->fromRoute('id'); $prospectiveMember = $this->memberService->getProspectiveMember($lidnr); diff --git a/module/Database/src/Controller/QueryController.php b/module/Database/src/Controller/QueryController.php index 9f8298b9..28f79a48 100644 --- a/module/Database/src/Controller/QueryController.php +++ b/module/Database/src/Controller/QueryController.php @@ -5,7 +5,7 @@ namespace Database\Controller; use Database\Service\Query as QueryService; -use Laminas\Http\Response; +use Laminas\Http\Response as HttpResponse; use Laminas\Mvc\Controller\AbstractActionController; use Laminas\View\Model\ViewModel; @@ -18,7 +18,7 @@ public function __construct(private readonly QueryService $queryService) /** * Index action. */ - public function indexAction(): Response|ViewModel + public function indexAction(): HttpResponse|ViewModel { if ($this->getRequest()->isPost()) { $post = $this->getRequest()->getPost()->toArray(); @@ -74,7 +74,7 @@ public function showAction(): ViewModel /** * Export action. */ - public function exportAction(): Response|ViewModel + public function exportAction(): HttpResponse|ViewModel { if ($this->getRequest()->isPost()) { $result = $this->queryService->execute( diff --git a/module/Database/src/Controller/SettingsController.php b/module/Database/src/Controller/SettingsController.php index 1b0c6a2e..7766b781 100644 --- a/module/Database/src/Controller/SettingsController.php +++ b/module/Database/src/Controller/SettingsController.php @@ -6,7 +6,7 @@ use Database\Service\InstallationFunction as InstallationFunctionService; use Database\Service\MailingList as MailingListService; -use Laminas\Http\Response; +use Laminas\Http\Response as HttpResponse; use Laminas\Mvc\Controller\AbstractActionController; use Laminas\View\Model\ViewModel; @@ -59,7 +59,7 @@ public function listAction(): ViewModel /** * List deletion action */ - public function deleteListAction(): Response|ViewModel + public function deleteListAction(): HttpResponse|ViewModel { $name = $this->params()->fromRoute('name'); diff --git a/module/User/src/Controller/ApiSettingsController.php b/module/User/src/Controller/ApiSettingsController.php index 8c7ff36f..c85554af 100644 --- a/module/User/src/Controller/ApiSettingsController.php +++ b/module/User/src/Controller/ApiSettingsController.php @@ -4,7 +4,7 @@ namespace User\Controller; -use Laminas\Http\Response; +use Laminas\Http\Response as HttpResponse; use Laminas\Mvc\Controller\AbstractActionController; use Laminas\Mvc\I18n\Translator as MvcTranslator; use Laminas\Mvc\Plugin\FlashMessenger\FlashMessenger; @@ -41,7 +41,7 @@ public function listPrincipalsAction(): ViewModel /** * Create an API principal */ - public function createPrincipalAction(): Response|ViewModel + public function createPrincipalAction(): HttpResponse|ViewModel { $form = $this->apiPrincipalService->getCreateForm(); @@ -72,7 +72,7 @@ public function createPrincipalAction(): Response|ViewModel /** * Edit an API principal */ - public function editPrincipalAction(): Response|ViewModel + public function editPrincipalAction(): HttpResponse|ViewModel { $id = (int) $this->params()->fromRoute('id'); $principal = $this->apiPrincipalService->find($id); @@ -104,7 +104,7 @@ public function editPrincipalAction(): Response|ViewModel ]); } - public function removePrincipalAction(): Response + public function removePrincipalAction(): HttpResponse { if ($this->getRequest()->isPost()) { $result = $this->apiPrincipalService->remove((int) $this->params()->fromRoute('id')); @@ -122,12 +122,12 @@ public function removePrincipalAction(): Response return $this->redirectList(); } - private function redirectList(): Response + private function redirectList(): HttpResponse { return $this->redirect()->toRoute('settings/api-principals'); } - public function notFoundAction(): Response + public function notFoundAction(): HttpResponse { $this->flashMessenger()->addWarningMessage( sprintf( diff --git a/module/User/src/Controller/SettingsController.php b/module/User/src/Controller/SettingsController.php index 3aff1573..008ddf23 100644 --- a/module/User/src/Controller/SettingsController.php +++ b/module/User/src/Controller/SettingsController.php @@ -4,7 +4,7 @@ namespace User\Controller; -use Laminas\Http\Response; +use Laminas\Http\Response as HttpResponse; use Laminas\Mvc\Controller\AbstractActionController; use Laminas\View\Model\ViewModel; use User\Service\UserService; @@ -34,7 +34,7 @@ public function listUserAction(): ViewModel /** * Create a user. */ - public function createUserAction(): Response|ViewModel + public function createUserAction(): HttpResponse|ViewModel { $form = $this->userService->getCreateForm(); @@ -52,7 +52,7 @@ public function createUserAction(): Response|ViewModel /** * Edit a user. */ - public function editUserAction(): Response|ViewModel + public function editUserAction(): HttpResponse|ViewModel { $form = $this->userService->getEditForm(); $id = (int) $this->params()->fromRoute('id'); @@ -81,7 +81,7 @@ public function editUserAction(): Response|ViewModel /** * Remove a user. */ - public function removeUserAction(): Response + public function removeUserAction(): HttpResponse { if ($this->getRequest()->isPost()) { $this->userService->remove((int) $this->params()->fromRoute('id')); diff --git a/module/User/src/Controller/UserController.php b/module/User/src/Controller/UserController.php index a014d21e..cb915bed 100644 --- a/module/User/src/Controller/UserController.php +++ b/module/User/src/Controller/UserController.php @@ -4,7 +4,7 @@ namespace User\Controller; -use Laminas\Http\Response; +use Laminas\Http\Response as HttpResponse; use Laminas\Mvc\Controller\AbstractActionController; use Laminas\View\Model\ViewModel; use User\Service\UserService; @@ -23,7 +23,7 @@ public function __construct( /** * User login action */ - public function indexAction(): Response|ViewModel + public function indexAction(): HttpResponse|ViewModel { if ($this->getRequest()->isPost()) { $result = $this->service->login($this->getRequest()->getPost()->toArray()); @@ -42,7 +42,7 @@ public function indexAction(): Response|ViewModel /** * User logout action */ - public function logoutAction(): Response + public function logoutAction(): HttpResponse { $this->service->logout(); diff --git a/module/User/src/Listener/AuthenticationListener.php b/module/User/src/Listener/AuthenticationListener.php index 761725d5..51dd1d89 100644 --- a/module/User/src/Listener/AuthenticationListener.php +++ b/module/User/src/Listener/AuthenticationListener.php @@ -6,8 +6,9 @@ use InvalidArgumentException; use Laminas\Authentication\AuthenticationService; +use Laminas\Http\Response as HttpResponse; use Laminas\Mvc\MvcEvent; -use Laminas\Stdlib\ResponseInterface as Response; +use Laminas\Stdlib\ResponseInterface; use LogicException; use User\Adapter\ApiPrincipalAdapter; use User\Service\ApiAuthenticationService; @@ -29,7 +30,7 @@ public function __construct( ) { } - public function __invoke(MvcEvent $e): ?Response + public function __invoke(MvcEvent $e): ?ResponseInterface { if (MvcEvent::EVENT_ROUTE !== $e->getName()) { throw new InvalidArgumentException( @@ -62,17 +63,21 @@ public function __invoke(MvcEvent $e): ?Response /** * Handle authentication for users */ - private function dbuserAuth(MvcEvent $e): ?Response + private function dbuserAuth(MvcEvent $e): ?ResponseInterface { if ($this->authService->hasIdentity()) { // user is logged in, just continue return null; } + // If this is a HTTP request, we redirect the user to the login page $response = $e->getResponse(); - $response->getHeaders()->addHeaderLine('Location', '/login'); - $response->setStatusCode(302); + if ($response instanceof HttpResponse) { + $response->getHeaders()->addHeaderLine('Location', '/login'); + $response->setStatusCode(302); + } + // Return $response will prevent output, but we also stop other listeners when we need to logon first $e->stopPropagation(); return $response; @@ -81,7 +86,7 @@ private function dbuserAuth(MvcEvent $e): ?Response /** * Handle authentication for api tokens */ - private function apiAuth(MvcEvent $e): ?Response + private function apiAuth(MvcEvent $e): ?ResponseInterface { if ($e->getRequest()->getHeaders()->has('Authorization')) { // This is an API call, we do this on every request @@ -93,9 +98,14 @@ private function apiAuth(MvcEvent $e): ?Response } } + // If this is a HTTP request, we add authentication headers $response = $e->getResponse(); - $response->getHeaders()->addHeaderLine('WWW-Authenticate', 'Bearer realm="/api"'); - $response->setStatusCode(401); + if ($response instanceof HttpResponse) { + $response->getHeaders()->addHeaderLine('WWW-Authenticate', 'Bearer realm="/api"'); + $response->setStatusCode(401); + } + + $e->stopPropagation(); return $response; } diff --git a/module/User/src/Listener/AuthorizationListener.php b/module/User/src/Listener/AuthorizationListener.php index a2d0bb0f..8ac53e8a 100644 --- a/module/User/src/Listener/AuthorizationListener.php +++ b/module/User/src/Listener/AuthorizationListener.php @@ -5,7 +5,7 @@ namespace User\Listener; use Database\Model\Enums\ApiResponseStatuses; -use Laminas\Http\Response; +use Laminas\Http\Response as HttpResponse; use Laminas\Mvc\MvcEvent; use Laminas\View\Model\JsonModel; use User\Model\Exception\NotAllowed as NotAllowedException; @@ -29,8 +29,8 @@ public function __invoke(MvcEvent $e): void ], ])); $response = $e->getResponse(); - if ($response instanceof Response) { - $response->setStatusCode(Response::STATUS_CODE_403); + if ($response instanceof HttpResponse) { + $response->setStatusCode(HttpResponse::STATUS_CODE_403); } $e->stopPropagation(); diff --git a/module/User/src/Listener/DispatchErrorFormatterListener.php b/module/User/src/Listener/DispatchErrorFormatterListener.php index 0d2346f9..d2493326 100644 --- a/module/User/src/Listener/DispatchErrorFormatterListener.php +++ b/module/User/src/Listener/DispatchErrorFormatterListener.php @@ -6,7 +6,7 @@ use Database\Model\Enums\ApiResponseStatuses; use Laminas\Http\Request; -use Laminas\Http\Response; +use Laminas\Http\Response as HttpResponse; use Laminas\Mvc\MvcEvent; use Laminas\Router\RouteMatch; use Laminas\Router\RouteStackInterface as Router; @@ -75,8 +75,8 @@ private function handleNoMatchedRoute(MvcEvent $e): void ]); $e->setViewModel($view); $response = $e->getResponse(); - if ($response instanceof Response) { - $response->setStatusCode(Response::STATUS_CODE_404); + if ($response instanceof HttpResponse) { + $response->setStatusCode(HttpResponse::STATUS_CODE_404); } $e->stopPropagation(); @@ -101,8 +101,8 @@ private function handleMatchedRoute( ]); $e->setViewModel($view); $response = $e->getResponse(); - if ($response instanceof Response) { - $response->setStatusCode(Response::STATUS_CODE_500); + if ($response instanceof HttpResponse) { + $response->setStatusCode(HttpResponse::STATUS_CODE_500); } $e->stopPropagation(); From 8c3d8872b67cad837d2afad70f09b49c513a3aad Mon Sep 17 00:00:00 2001 From: rinkp Date: Wed, 24 Jul 2024 22:50:25 +0200 Subject: [PATCH 07/10] [feat] `Forbidden` `ApiResponseStatus` --- module/Database/src/Model/Enums/ApiResponseStatuses.php | 3 +++ module/User/src/Listener/AuthorizationListener.php | 2 +- openapi.yaml | 2 +- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/module/Database/src/Model/Enums/ApiResponseStatuses.php b/module/Database/src/Model/Enums/ApiResponseStatuses.php index 19506934..6ab17603 100644 --- a/module/Database/src/Model/Enums/ApiResponseStatuses.php +++ b/module/Database/src/Model/Enums/ApiResponseStatuses.php @@ -15,6 +15,9 @@ enum ApiResponseStatuses: string // For 404 HTTP code case NotFound = 'notfound'; + // For 403 HTTP code + case Forbidden = 'forbidden'; + // For 5xx HTTP codes case Error = 'error'; } diff --git a/module/User/src/Listener/AuthorizationListener.php b/module/User/src/Listener/AuthorizationListener.php index 8ac53e8a..b6db19e9 100644 --- a/module/User/src/Listener/AuthorizationListener.php +++ b/module/User/src/Listener/AuthorizationListener.php @@ -22,7 +22,7 @@ public function __invoke(MvcEvent $e): void } $e->setViewModel(new JsonModel([ - 'status' => ApiResponseStatuses::Error, + 'status' => ApiResponseStatuses::Forbidden, 'error' => [ 'type' => NotAllowedException::class, 'message' => $e->getParam('exception')->getMessage(), diff --git a/openapi.yaml b/openapi.yaml index 7267b1d2..51d63d2c 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -393,7 +393,7 @@ components: properties: status: type: string - default: "error" + default: "forbidden" error: type: object properties: From ed3408dc8f708f6461d8684869db61d3a24fd03a Mon Sep 17 00:00:00 2001 From: rinkp Date: Wed, 24 Jul 2024 22:57:38 +0200 Subject: [PATCH 08/10] [style] Unnecessary return type --- module/Database/src/Controller/ApiController.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/module/Database/src/Controller/ApiController.php b/module/Database/src/Controller/ApiController.php index df0c1d07..0aefc4aa 100644 --- a/module/Database/src/Controller/ApiController.php +++ b/module/Database/src/Controller/ApiController.php @@ -8,7 +8,6 @@ use Database\Service\Api as ApiService; use Laminas\Http\Response as HttpResponse; use Laminas\Mvc\Controller\AbstractActionController; -use Laminas\Stdlib\ResponseInterface; use Laminas\View\Model\JsonModel; use RuntimeException; use User\Model\Enums\ApiPermissions; @@ -74,7 +73,7 @@ public function membersAction(): JsonModel /** * Return member */ - public function memberAction(): JsonModel|ResponseInterface + public function memberAction(): JsonModel { $this->apiAuthService->assertCan(ApiPermissions::MembersR); From c08633e09461b013c73d100e94cd05b7b9cf6429 Mon Sep 17 00:00:00 2001 From: rinkp Date: Wed, 24 Jul 2024 22:58:08 +0200 Subject: [PATCH 09/10] [style] Main function goes on top --- .../DispatchErrorFormatterListener.php | 60 +++++++++---------- 1 file changed, 30 insertions(+), 30 deletions(-) diff --git a/module/User/src/Listener/DispatchErrorFormatterListener.php b/module/User/src/Listener/DispatchErrorFormatterListener.php index d2493326..0778af55 100644 --- a/module/User/src/Listener/DispatchErrorFormatterListener.php +++ b/module/User/src/Listener/DispatchErrorFormatterListener.php @@ -19,6 +19,36 @@ final class DispatchErrorFormatterListener { + public function __invoke(MvcEvent $e): void + { + /** + * The source code is public so we can give away 404 errors if it is because of no route + * This does not include 404 responses that are returned by logic, such as when a member does not exist + **/ + if ('error-router-no-match' === $e->getError()) { + $this->handleNoMatchedRoute($e); + + return; + } + + /** + * If this is not an error-router-no-match error, we must have a matching route + */ + $match = $e->getRouteMatch(); + + // We should always have a match here; if we do not, throw an exception + // possibly including previous exceptions + if (null === $match) { + throw new LogicException( + message: 'Assumed route would be present; no route present', + previous: $e->getParam('exception', null), + ); + } + + // If we do have a match, this implies we have properly authenticated before + $this->handleMatchedRoute($e, $match); + } + private function matchAncestorRoute( Request $request, Router $router, @@ -107,34 +137,4 @@ private function handleMatchedRoute( $e->stopPropagation(); } - - public function __invoke(MvcEvent $e): void - { - /** - * The source code is public so we can give away 404 errors if it is because of no route - * This does not include 404 responses that are returned by logic, such as when a member does not exist - **/ - if ('error-router-no-match' === $e->getError()) { - $this->handleNoMatchedRoute($e); - - return; - } - - /** - * If this is not an error-router-no-match error, we must have a matching route - */ - $match = $e->getRouteMatch(); - - // We should always have a match here; if we do not, throw an exception - // possibly including previous exceptions - if (null === $match) { - throw new LogicException( - message: 'Assumed route would be present; no route present', - previous: $e->getParam('exception', null), - ); - } - - // If we do have a match, this implies we have properly authenticated before - $this->handleMatchedRoute($e, $match); - } } From ed4c354cf57cfc7654d9cfe107c7151fe7a421cd Mon Sep 17 00:00:00 2001 From: rinkp Date: Wed, 24 Jul 2024 23:04:35 +0200 Subject: [PATCH 10/10] [ci] Psalm baseline because of `Response` -> `HttpResponse` --- psalm/psalm-baseline.xml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/psalm/psalm-baseline.xml b/psalm/psalm-baseline.xml index 95bc9abc..12f06d4e 100644 --- a/psalm/psalm-baseline.xml +++ b/psalm/psalm-baseline.xml @@ -13,7 +13,7 @@ - Response|ViewModel + HttpResponse|ViewModel @@ -43,7 +43,7 @@ $this->redirect()->toRoute('home') - Response|ViewModel + HttpResponse|ViewModel