From a32f699248c0a4e45a2f5a0272f608fdee04be4f Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Wed, 21 Aug 2024 12:29:03 +0200 Subject: [PATCH] Ensure HTTP exception is thrown when API fails When the pushConnectionsAction fails because the DoctrineMetadataPushRepository failed to save the assembled roles data. We should throw a HTTP exception. Otherwise the action would not output JSON. Resulting in a user facing error page. Which is not user friendly for a consumer Fixes: https://github.com/OpenConext/OpenConext-engineblock/issues/1309 --- .../Controller/Api/ConnectionsController.php | 8 +- .../Api/ConnectionsControllerTest.php | 73 +++++++++++++++++-- 2 files changed, 73 insertions(+), 8 deletions(-) diff --git a/src/OpenConext/EngineBlockBundle/Controller/Api/ConnectionsController.php b/src/OpenConext/EngineBlockBundle/Controller/Api/ConnectionsController.php index 0f79226d08..3db45cc387 100644 --- a/src/OpenConext/EngineBlockBundle/Controller/Api/ConnectionsController.php +++ b/src/OpenConext/EngineBlockBundle/Controller/Api/ConnectionsController.php @@ -18,11 +18,13 @@ namespace OpenConext\EngineBlockBundle\Controller\Api; +use Exception; use OpenConext\EngineBlock\Metadata\Entity\Assembler\MetadataAssemblerInterface; use OpenConext\EngineBlock\Metadata\MetadataRepository\DoctrineMetadataPushRepository; use OpenConext\EngineBlockBundle\Configuration\FeatureConfiguration; use OpenConext\EngineBlockBundle\Configuration\FeatureConfigurationInterface; use OpenConext\EngineBlockBundle\Http\Exception\ApiAccessDeniedHttpException; +use OpenConext\EngineBlockBundle\Http\Exception\ApiInternalServerErrorHttpException; use OpenConext\EngineBlockBundle\Http\Exception\ApiMethodNotAllowedHttpException; use OpenConext\EngineBlockBundle\Http\Exception\ApiNotFoundHttpException; use OpenConext\EngineBlockBundle\Http\Exception\BadApiRequestHttpException; @@ -113,7 +115,11 @@ public function pushConnectionsAction(Request $request) unset($body); - $result = $this->repository->synchronize($roles); + try { + $result = $this->repository->synchronize($roles); + } catch (Exception $exception) { + throw new ApiInternalServerErrorHttpException('Unable to synchronize the assembled roles to the repository', $exception); + } return new JsonResponse($result); } diff --git a/tests/functional/OpenConext/EngineBlockBundle/Controller/Api/ConnectionsControllerTest.php b/tests/functional/OpenConext/EngineBlockBundle/Controller/Api/ConnectionsControllerTest.php index 3c94e203ad..ed922ada3b 100644 --- a/tests/functional/OpenConext/EngineBlockBundle/Controller/Api/ConnectionsControllerTest.php +++ b/tests/functional/OpenConext/EngineBlockBundle/Controller/Api/ConnectionsControllerTest.php @@ -54,7 +54,7 @@ public function authentication_is_required_for_pushing_metadata() */ public function only_post_requests_are_allowed_when_pushing_metadata($invalidHttpMethod) { - $client = $client = static::createClient([], [ + $client = static::createClient([], [ 'PHP_AUTH_USER' => $this->getContainer()->getParameter('api.users.metadataPush.username'), 'PHP_AUTH_PW' => $this->getContainer()->getParameter('api.users.metadataPush.password'), ]); @@ -75,7 +75,7 @@ public function only_post_requests_are_allowed_when_pushing_metadata($invalidHtt */ public function cannot_push_metadata_if_feature_is_disabled() { - $client = $client = static::createClient([], [ + $client = static::createClient([], [ 'PHP_AUTH_USER' => $this->getContainer()->getParameter('api.users.metadataPush.username'), 'PHP_AUTH_PW' => $this->getContainer()->getParameter('api.users.metadataPush.password'), ]); @@ -97,7 +97,7 @@ public function cannot_push_metadata_if_feature_is_disabled() */ public function cannot_push_metadata_if_user_does_not_have_manage_role() { - $client = $client = static::createClient([], [ + $client = static::createClient([], [ 'PHP_AUTH_USER' => 'no_roles', 'PHP_AUTH_PW' => 'no_roles', ]); @@ -120,7 +120,7 @@ public function cannot_push_metadata_if_user_does_not_have_manage_role() */ public function cannot_push_invalid_content_to_the_metadata_push_api($invalidJsonPayload) { - $client = $client = static::createClient([], [ + $client = static::createClient([], [ 'PHP_AUTH_USER' => $this->getContainer()->getParameter('api.users.metadataPush.username'), 'PHP_AUTH_PW' => $this->getContainer()->getParameter('api.users.metadataPush.password'), ]); @@ -150,7 +150,7 @@ public function pushing_data_to_engineblock_should_succeed() { $this->clearMetadataFixtures(); - $client = $client = static::createClient([], [ + $client = static::createClient([], [ 'PHP_AUTH_USER' => $this->getContainer()->getParameter('api.users.metadataPush.username'), 'PHP_AUTH_PW' => $this->getContainer()->getParameter('api.users.metadataPush.password'), ]); @@ -219,7 +219,7 @@ public function pushing_data_with_coins_to_engineblock_should_succeed() { $this->clearMetadataFixtures(); - $client = $client = static::createClient([], [ + $client = static::createClient([], [ 'PHP_AUTH_USER' => $this->getContainer()->getParameter('api.users.metadataPush.username'), 'PHP_AUTH_PW' => $this->getContainer()->getParameter('api.users.metadataPush.password'), ]); @@ -273,7 +273,7 @@ public function pushing_manage_sfo_data_should_succeed() { $this->clearMetadataFixtures(); - $client = $client = static::createClient([], [ + $client = static::createClient([], [ 'PHP_AUTH_USER' => $this->getContainer()->getParameter('api.users.metadataPush.username'), 'PHP_AUTH_PW' => $this->getContainer()->getParameter('api.users.metadataPush.password'), ]); @@ -351,6 +351,42 @@ public function pushing_manage_sfo_data_should_succeed() $this->assertFalse($emptyIdp->getCoins()->stepupConnections()->hasConnections()); } + + /** + * @test + * @group Api + * @group Connections + * @group MetadataPush + */ + public function pushing_data_to_engineblock_can_fail() + { + $this->clearMetadataFixtures(); + + $client = static::createClient([], [ + 'PHP_AUTH_USER' => $this->getContainer()->getParameter('api.users.metadataPush.username'), + 'PHP_AUTH_PW' => $this->getContainer()->getParameter('api.users.metadataPush.password'), + ]); + + // The second 'step' will overwrite the entity id for the one entry. It only changes some case, resulting in + // a sql error in Engine. That exception results in a 500 JSON response + foreach ($this->failingConnectionsData() as $expectedHttpCode => $step) { + + $payload = $this->createJsonData($step); + + $client->request( + 'POST', + 'https://engine-api.vm.openconext.org/api/connections', + [], + [], + [], + $payload + ); + $this->assertStatusCode($expectedHttpCode, $client); + $this->assertJson($client->getResponse()->getContent()); + + } + } + public function invalidHttpMethodProvider() { return [ @@ -602,4 +638,27 @@ private function validConnectionsWithCoinsData() ], ]; } + + private function failingConnectionsData() + { + return + [ + 200 => [ + [ + 'uuid' => '00000000-0000-0000-0000-000000000000', + 'entityId' => 'https://my-idp.test/1', + 'name' => 'SP0', + 'type' => 'saml20-sp', + ], + ], + 500 => [ + [ + 'uuid' => '00000000-0000-0000-0000-000000000000', + 'entityId' => 'https://mY-iDp.test/1', + 'name' => 'SP0', + 'type' => 'saml20-sp', + ], + ] + ]; + } }