From f5765680af05e2dbf734dad539a832c41245f703 Mon Sep 17 00:00:00 2001 From: Matthias Schuhmayer <38959016+mattamon@users.noreply.github.com> Date: Mon, 22 Apr 2024 09:42:43 +0200 Subject: [PATCH] Return exception response (#37) * Introducing exception subscriber to create api error responses * Remove test parameter * Apply php-cs-fixer changes * Fix tests * Apply php-cs-fixer changes * Add abstract ApiException * Apply php-cs-fixer changes * Declare strict --------- Co-authored-by: mattamon --- config/event_subscribers.yaml | 9 +++ .../Response/Error/BadRequestResponse.php | 35 +++++++++++ .../Error/MethodNotAllowedResponse.php | 35 +++++++++++ .../{ => Error}/UnauthorizedResponse.php | 6 +- .../Api/Assets/CollectionController.php | 6 +- src/Controller/Api/Assets/GetController.php | 4 +- .../Authorization/AuthorizationController.php | 5 +- .../Api/DataObjects/CollectionController.php | 6 +- src/Controller/Api/TranslationController.php | 4 +- .../PimcoreStudioApiExtension.php | 1 + .../ApiExceptionSubscriber.php | 60 +++++++++++++++++++ src/Exception/AbstractApiException.php | 23 +++++++ src/Exception/AccessDeniedException.php | 4 +- src/Exception/ApiExceptionInterface.php | 27 +++++++++ src/Exception/InvalidFilterTypeException.php | 4 +- src/Exception/InvalidQueryTypeException.php | 4 +- src/Exception/InvalidSearchException.php | 4 +- src/Exception/NoRequestException.php | 4 +- .../NonPublicTranslationException.php | 4 +- src/Exception/NotAuthorizedException.php | 4 +- src/Factory/QueryFactory.php | 2 +- .../Schema/{Unauthorized.php => Error.php} | 10 ++-- src/Service/Filter/FilterService.php | 2 +- .../V1/DataObjectSearchAdapter.php | 1 + src/Service/SecurityService.php | 8 ++- .../Service/Security/SecurityServiceTest.php | 7 ++- 26 files changed, 239 insertions(+), 40 deletions(-) create mode 100644 config/event_subscribers.yaml create mode 100644 src/Attributes/Response/Error/BadRequestResponse.php create mode 100644 src/Attributes/Response/Error/MethodNotAllowedResponse.php rename src/Attributes/Response/{ => Error}/UnauthorizedResponse.php (78%) create mode 100644 src/EventSubscriber/ApiExceptionSubscriber.php create mode 100644 src/Exception/AbstractApiException.php create mode 100644 src/Exception/ApiExceptionInterface.php rename src/Response/Schema/{Unauthorized.php => Error.php} (73%) diff --git a/config/event_subscribers.yaml b/config/event_subscribers.yaml new file mode 100644 index 000000000..b284d97fe --- /dev/null +++ b/config/event_subscribers.yaml @@ -0,0 +1,9 @@ +services: + _defaults: + autowire: true + autoconfigure: true + public: false + + #Subscriber + Pimcore\Bundle\StudioApiBundle\EventSubscriber\ApiExceptionSubscriber: + tags: [ 'kernel.event_subscriber' ] \ No newline at end of file diff --git a/src/Attributes/Response/Error/BadRequestResponse.php b/src/Attributes/Response/Error/BadRequestResponse.php new file mode 100644 index 000000000..bc11089aa --- /dev/null +++ b/src/Attributes/Response/Error/BadRequestResponse.php @@ -0,0 +1,35 @@ + 'Something bad you did']) + ); + } +} diff --git a/src/Attributes/Response/Error/MethodNotAllowedResponse.php b/src/Attributes/Response/Error/MethodNotAllowedResponse.php new file mode 100644 index 000000000..bc35c1543 --- /dev/null +++ b/src/Attributes/Response/Error/MethodNotAllowedResponse.php @@ -0,0 +1,35 @@ + 'Using the wrong method you are']) + ); + } +} diff --git a/src/Attributes/Response/UnauthorizedResponse.php b/src/Attributes/Response/Error/UnauthorizedResponse.php similarity index 78% rename from src/Attributes/Response/UnauthorizedResponse.php rename to src/Attributes/Response/Error/UnauthorizedResponse.php index 165827f62..277f35ecc 100644 --- a/src/Attributes/Response/UnauthorizedResponse.php +++ b/src/Attributes/Response/Error/UnauthorizedResponse.php @@ -14,12 +14,12 @@ * @license http://www.pimcore.org/license GPLv3 and PCL */ -namespace Pimcore\Bundle\StudioApiBundle\Attributes\Response; +namespace Pimcore\Bundle\StudioApiBundle\Attributes\Response\Error; use Attribute; use OpenApi\Attributes\JsonContent; use OpenApi\Attributes\Response; -use Pimcore\Bundle\StudioApiBundle\Response\Schema\Unauthorized; +use Pimcore\Bundle\StudioApiBundle\Response\Schema\Error; #[Attribute(Attribute::TARGET_CLASS | Attribute::TARGET_METHOD | Attribute::IS_REPEATABLE)] final class UnauthorizedResponse extends Response @@ -29,7 +29,7 @@ public function __construct() parent::__construct( response: 401, description: 'Unauthorized', - content: new JsonContent(ref: Unauthorized::class) + content: new JsonContent(ref: Error::class, example: ['message' => 'Computer says no']) ); } } diff --git a/src/Controller/Api/Assets/CollectionController.php b/src/Controller/Api/Assets/CollectionController.php index 09a04a4f1..0ef5cbbd7 100644 --- a/src/Controller/Api/Assets/CollectionController.php +++ b/src/Controller/Api/Assets/CollectionController.php @@ -26,9 +26,11 @@ use Pimcore\Bundle\StudioApiBundle\Attributes\Parameters\Query\PathIncludeParentParameter; use Pimcore\Bundle\StudioApiBundle\Attributes\Parameters\Query\PathParameter; use Pimcore\Bundle\StudioApiBundle\Attributes\Response\Content\CollectionJson; +use Pimcore\Bundle\StudioApiBundle\Attributes\Response\Error\BadRequestResponse; +use Pimcore\Bundle\StudioApiBundle\Attributes\Response\Error\MethodNotAllowedResponse; +use Pimcore\Bundle\StudioApiBundle\Attributes\Response\Error\UnauthorizedResponse; use Pimcore\Bundle\StudioApiBundle\Attributes\Response\Property\AnyOfAsset; use Pimcore\Bundle\StudioApiBundle\Attributes\Response\SuccessResponse; -use Pimcore\Bundle\StudioApiBundle\Attributes\Response\UnauthorizedResponse; use Pimcore\Bundle\StudioApiBundle\Config\Tags; use Pimcore\Bundle\StudioApiBundle\Controller\Api\AbstractApiController; use Pimcore\Bundle\StudioApiBundle\Controller\Trait\PaginatedResponseTrait; @@ -81,7 +83,9 @@ public function __construct( description: 'Paginated assets with total count as header param', content: new CollectionJson(new AnyOfAsset()) )] + #[BadRequestResponse] #[UnauthorizedResponse] + #[MethodNotAllowedResponse] public function getAssets(#[MapQueryString] Parameters $parameters): JsonResponse { $assetQuery = $this->filterService->applyFilters( diff --git a/src/Controller/Api/Assets/GetController.php b/src/Controller/Api/Assets/GetController.php index 42ff95282..6ecdc679b 100644 --- a/src/Controller/Api/Assets/GetController.php +++ b/src/Controller/Api/Assets/GetController.php @@ -19,8 +19,9 @@ use OpenApi\Attributes\Get; use Pimcore\Bundle\StudioApiBundle\Attributes\Parameters\Path\IdParameter; use Pimcore\Bundle\StudioApiBundle\Attributes\Response\Content\OneOfAssetJson; +use Pimcore\Bundle\StudioApiBundle\Attributes\Response\Error\MethodNotAllowedResponse; +use Pimcore\Bundle\StudioApiBundle\Attributes\Response\Error\UnauthorizedResponse; use Pimcore\Bundle\StudioApiBundle\Attributes\Response\SuccessResponse; -use Pimcore\Bundle\StudioApiBundle\Attributes\Response\UnauthorizedResponse; use Pimcore\Bundle\StudioApiBundle\Config\Tags; use Pimcore\Bundle\StudioApiBundle\Controller\Api\AbstractApiController; use Pimcore\Bundle\StudioApiBundle\Service\AssetSearchServiceInterface; @@ -56,6 +57,7 @@ public function __construct( content: new OneOfAssetJson() )] #[UnauthorizedResponse] + #[MethodNotAllowedResponse] public function getAssetById(int $id): JsonResponse { return $this->jsonResponse($this->assetSearchService->getAssetById($id)); diff --git a/src/Controller/Api/Authorization/AuthorizationController.php b/src/Controller/Api/Authorization/AuthorizationController.php index 9eae44274..753884e4a 100644 --- a/src/Controller/Api/Authorization/AuthorizationController.php +++ b/src/Controller/Api/Authorization/AuthorizationController.php @@ -20,8 +20,9 @@ use OpenApi\Attributes\Post; use Pimcore\Bundle\StudioApiBundle\Attributes\Request\CredentialsRequestBody; use Pimcore\Bundle\StudioApiBundle\Attributes\Request\TokenRequestBody; +use Pimcore\Bundle\StudioApiBundle\Attributes\Response\Error\MethodNotAllowedResponse; +use Pimcore\Bundle\StudioApiBundle\Attributes\Response\Error\UnauthorizedResponse; use Pimcore\Bundle\StudioApiBundle\Attributes\Response\SuccessResponse; -use Pimcore\Bundle\StudioApiBundle\Attributes\Response\UnauthorizedResponse; use Pimcore\Bundle\StudioApiBundle\Config\Tags; use Pimcore\Bundle\StudioApiBundle\Controller\Api\AbstractApiController; use Pimcore\Bundle\StudioApiBundle\Request\Credentials; @@ -61,6 +62,7 @@ public function __construct( content: new JsonContent(ref: Token::class) )] #[UnauthorizedResponse] + #[MethodNotAllowedResponse] public function login(#[MapRequestPayload] Credentials $credentials): JsonResponse { /** @var User $user */ @@ -84,6 +86,7 @@ public function login(#[MapRequestPayload] Credentials $credentials): JsonRespon content: new JsonContent(ref: Token::class) )] #[UnauthorizedResponse] + #[MethodNotAllowedResponse] public function refresh(#[MapRequestPayload] Refresh $refresh): JsonResponse { $tokenInfo = $this->tokenService->refreshToken($refresh->getToken()); diff --git a/src/Controller/Api/DataObjects/CollectionController.php b/src/Controller/Api/DataObjects/CollectionController.php index e73d89372..d3ccb650f 100644 --- a/src/Controller/Api/DataObjects/CollectionController.php +++ b/src/Controller/Api/DataObjects/CollectionController.php @@ -27,9 +27,11 @@ use Pimcore\Bundle\StudioApiBundle\Attributes\Parameters\Query\PathIncludeParentParameter; use Pimcore\Bundle\StudioApiBundle\Attributes\Parameters\Query\PathParameter; use Pimcore\Bundle\StudioApiBundle\Attributes\Response\Content\CollectionJson; +use Pimcore\Bundle\StudioApiBundle\Attributes\Response\Error\BadRequestResponse; +use Pimcore\Bundle\StudioApiBundle\Attributes\Response\Error\MethodNotAllowedResponse; +use Pimcore\Bundle\StudioApiBundle\Attributes\Response\Error\UnauthorizedResponse; use Pimcore\Bundle\StudioApiBundle\Attributes\Response\Property\DataObjectCollection; use Pimcore\Bundle\StudioApiBundle\Attributes\Response\SuccessResponse; -use Pimcore\Bundle\StudioApiBundle\Attributes\Response\UnauthorizedResponse; use Pimcore\Bundle\StudioApiBundle\Config\Tags; use Pimcore\Bundle\StudioApiBundle\Controller\Api\AbstractApiController; use Pimcore\Bundle\StudioApiBundle\Controller\Trait\PaginatedResponseTrait; @@ -80,7 +82,9 @@ public function __construct( description: 'Paginated data objects with total count as header param', content: new CollectionJson(new DataObjectCollection()) )] + #[BadRequestResponse] #[UnauthorizedResponse] + #[MethodNotAllowedResponse] public function getDataObjects(#[MapQueryString] DataObjectParameters $parameters): JsonResponse { diff --git a/src/Controller/Api/TranslationController.php b/src/Controller/Api/TranslationController.php index 155eae0f4..0cde64818 100644 --- a/src/Controller/Api/TranslationController.php +++ b/src/Controller/Api/TranslationController.php @@ -19,8 +19,9 @@ use OpenApi\Attributes\JsonContent; use OpenApi\Attributes\Post; use Pimcore\Bundle\StudioApiBundle\Attributes\Request\TranslationRequestBody; +use Pimcore\Bundle\StudioApiBundle\Attributes\Response\Error\MethodNotAllowedResponse; +use Pimcore\Bundle\StudioApiBundle\Attributes\Response\Error\UnauthorizedResponse; use Pimcore\Bundle\StudioApiBundle\Attributes\Response\SuccessResponse; -use Pimcore\Bundle\StudioApiBundle\Attributes\Response\UnauthorizedResponse; use Pimcore\Bundle\StudioApiBundle\Config\Tags; use Pimcore\Bundle\StudioApiBundle\Request\Query\Translation; use Pimcore\Bundle\StudioApiBundle\Service\TranslatorServiceInterface; @@ -59,6 +60,7 @@ public function __construct( content: new JsonContent(ref: Translation::class) )] #[UnauthorizedResponse] + #[MethodNotAllowedResponse] public function getTranslations( #[MapRequestPayload] Translation $translation, ): JsonResponse { diff --git a/src/DependencyInjection/PimcoreStudioApiExtension.php b/src/DependencyInjection/PimcoreStudioApiExtension.php index 4f5ca80b0..cdcbf8e99 100644 --- a/src/DependencyInjection/PimcoreStudioApiExtension.php +++ b/src/DependencyInjection/PimcoreStudioApiExtension.php @@ -49,6 +49,7 @@ public function load(array $configs, ContainerBuilder $container): void $loader = new YamlFileLoader($container, new FileLocator(__DIR__ . '/../../config')); $loader->load('services.yaml'); $loader->load('filters.yaml'); + $loader->load('event_subscribers.yaml'); $definition = $container->getDefinition(TokenServiceInterface::class); $definition->setArgument('$tokenLifetime', $config['api_token']['lifetime']); diff --git a/src/EventSubscriber/ApiExceptionSubscriber.php b/src/EventSubscriber/ApiExceptionSubscriber.php new file mode 100644 index 000000000..01b4e31a8 --- /dev/null +++ b/src/EventSubscriber/ApiExceptionSubscriber.php @@ -0,0 +1,60 @@ + 'onKernelException', + ]; + } + + public function onKernelException(ExceptionEvent $event): void + { + $exception = $event->getThrowable(); + $request = $event->getRequest(); + + if(!$exception instanceof ApiExceptionInterface && !$this->isStudioApiCall($request->getPathInfo())) { + return; + } + /** @var ApiExceptionInterface $exception */ + $response = new JsonResponse( + [ + 'message' => $exception->getMessage(), + ], + $exception->getStatusCode() + ); + + $event->setResponse($response); + } + + private function isStudioApiCall(string $path): bool + { + return str_starts_with($path, AbstractApiController::API_PATH); + } +} diff --git a/src/Exception/AbstractApiException.php b/src/Exception/AbstractApiException.php new file mode 100644 index 000000000..1f326d5b8 --- /dev/null +++ b/src/Exception/AbstractApiException.php @@ -0,0 +1,23 @@ + $this->assetQueryProvider->createAssetQuery(), 'dataObject' => $this->dataObjectQueryProvider->createDataObjectQuery(), - default => throw new InvalidQueryTypeException("Unknown query type: $type") + default => throw new InvalidQueryTypeException(400, "Unknown query type: $type") }; } } diff --git a/src/Response/Schema/Unauthorized.php b/src/Response/Schema/Error.php similarity index 73% rename from src/Response/Schema/Unauthorized.php rename to src/Response/Schema/Error.php index 5c2ae6f40..9d3847c7c 100644 --- a/src/Response/Schema/Unauthorized.php +++ b/src/Response/Schema/Error.php @@ -23,15 +23,15 @@ * @internal */ #[Schema( - schema: 'Unauthorized', - title: 'Unauthorized', - description: 'Bad credentials or missing token', + schema: 'Error', + title: 'Error', + description: 'Bad credentials or missing token, bad request, method not allowed, etc.', type: 'object' )] -final readonly class Unauthorized +final readonly class Error { public function __construct( - #[Property(description: 'Message', type: 'string')] + #[Property(description: 'Message', type: 'string', example: 'I got a bad feeling about this')] protected string $message ) { diff --git a/src/Service/Filter/FilterService.php b/src/Service/Filter/FilterService.php index 5c903132c..9044d96e4 100644 --- a/src/Service/Filter/FilterService.php +++ b/src/Service/Filter/FilterService.php @@ -65,7 +65,7 @@ private function getTypeFilters(Filters $filters, string $type): array FilterServiceInterface::TYPE_ASSET => $filters->getAssetFilters(), FilterServiceInterface::TYPE_DATA_OBJECT => $filters->getDataObjectFilters(), FilterServiceInterface::TYPE_DOCUMENT => $filters->getDocumentFilters(), - default => throw new InvalidFilterTypeException("Unknown filter type: $type") + default => throw new InvalidFilterTypeException(400, "Unknown filter type: $type") }; } } diff --git a/src/Service/GenericData/V1/DataObjectSearchAdapter.php b/src/Service/GenericData/V1/DataObjectSearchAdapter.php index d1c1bce4e..4d7813085 100644 --- a/src/Service/GenericData/V1/DataObjectSearchAdapter.php +++ b/src/Service/GenericData/V1/DataObjectSearchAdapter.php @@ -43,6 +43,7 @@ public function searchDataObjects(QueryInterface $dataObjectQuery): DataObjectSe $search = $dataObjectQuery->getSearch(); if (!$search instanceof DataObjectSearchInterface) { throw new InvalidSearchException( + 400, sprintf( 'Expected search to be an instance of %s, got %s', DataObjectSearchInterface::class, diff --git a/src/Service/SecurityService.php b/src/Service/SecurityService.php index 10a6e3ce4..2eb4115a1 100644 --- a/src/Service/SecurityService.php +++ b/src/Service/SecurityService.php @@ -29,6 +29,10 @@ */ final readonly class SecurityService implements SecurityServiceInterface { + private const STATUS_CODE = 401; + + private const MESSAGE = 'Bad credentials'; + public function __construct( private UserProvider $userProvider, private UserPasswordHasherInterface $passwordHasher, @@ -41,14 +45,14 @@ public function authenticateUser(Credentials $credentials): PasswordAuthenticate try { $user = $this->userProvider->loadUserByIdentifier($credentials->getUsername()); } catch (UserNotFoundException) { - throw new AccessDeniedException(401, 'Invalid credentials'); + throw new AccessDeniedException(self::STATUS_CODE, self::MESSAGE); } if( !$user instanceof PasswordAuthenticatedUserInterface || !$this->passwordHasher->isPasswordValid($user, $credentials->getPassword()) ) { - throw new AccessDeniedException(401, 'Invalid credentials'); + throw new AccessDeniedException(self::STATUS_CODE, self::MESSAGE); } return $user; diff --git a/tests/Unit/Service/Security/SecurityServiceTest.php b/tests/Unit/Service/Security/SecurityServiceTest.php index e0ed4c647..e83dc2e79 100644 --- a/tests/Unit/Service/Security/SecurityServiceTest.php +++ b/tests/Unit/Service/Security/SecurityServiceTest.php @@ -19,6 +19,7 @@ use Codeception\Test\Unit; use Exception; use Pimcore\Bundle\StaticResolverBundle\Models\Tool\TmpStoreResolverInterface; +use Pimcore\Bundle\StudioApiBundle\Exception\AccessDeniedException; use Pimcore\Bundle\StudioApiBundle\Request\Credentials; use Pimcore\Bundle\StudioApiBundle\Service\SecurityService; use Pimcore\Bundle\StudioApiBundle\Service\SecurityServiceInterface; @@ -49,7 +50,8 @@ public function testInvalidPassword(): void { $securityService = $this->mockSecurityService(false); - $this->expectExceptionMessage('Invalid credentials'); + $this->expectException(AccessDeniedException::class); + $this->expectExceptionMessage('Bad credentials'); $securityService->authenticateUser(new Credentials('test', 'test')); } @@ -60,7 +62,8 @@ public function testUserNotFound(): void { $securityService = $this->mockSecurityService(false, false); - $this->expectExceptionMessage('Invalid credentials'); + $this->expectException(AccessDeniedException::class); + $this->expectExceptionMessage('Bad credentials'); $securityService->authenticateUser(new Credentials('test', 'test')); }