From 14354f86fe511397385ada17625f3f3405218320 Mon Sep 17 00:00:00 2001 From: BacLuc Date: Wed, 6 Mar 2024 23:11:41 +0100 Subject: [PATCH] api: restrict access to protected routes with firewall Else the queries are executed without a user filter, which keeps our database busy. --- api/config/packages/security.yaml | 10 +- api/src/Util/ParametrizedTestHelper.php | 14 +++ api/tests/Api/FirewallTest.php | 115 ++++++++++++++++++ .../Api/Invitations/AcceptInvitationTest.php | 30 +++++ .../Api/Invitations/InvitationGraphQLTest.php | 30 ++++- api/tests/Api/Profiles/ReadProfileTest.php | 2 +- api/tests/Api/Profiles/UpdateProfileTest.php | 2 +- api/tests/Api/Users/ReadUserTest.php | 2 +- 8 files changed, 198 insertions(+), 7 deletions(-) create mode 100644 api/src/Util/ParametrizedTestHelper.php create mode 100644 api/tests/Api/FirewallTest.php diff --git a/api/config/packages/security.yaml b/api/config/packages/security.yaml index da12cb22db..5e9a060ede 100644 --- a/api/config/packages/security.yaml +++ b/api/config/packages/security.yaml @@ -41,8 +41,14 @@ security: access_control: - { path: ^/docs, roles: PUBLIC_ACCESS } # Allows accessing the Swagger UI - - { path: ^/authentication_token, roles: PUBLIC_ACCESS } - - { path: ^/, roles: PUBLIC_ACCESS } # Some endpoints are available to unauthenticated clients + - { path: ^/?$, roles: PUBLIC_ACCESS } # Without /, the frontend does not know how to login + - { path: ^/authentication_token, roles: PUBLIC_ACCESS } # Endpoint to login + - { path: ^/auth, roles: PUBLIC_ACCESS } # OAuth and resend password endpoints + - { path: ^/content_types, roles: PUBLIC_ACCESS } # Content types is more or less static and the same for all camps + - { path: ^/invitations/.*/(find|reject), roles: PUBLIC_ACCESS } + - { path: ^/users$, methods: [POST], roles: PUBLIC_ACCESS } # register + - { path: ^/users/.*/activate$, methods: [PATCH], roles: PUBLIC_ACCESS } + - { path: .*, roles: [ROLE_USER] } # Protect all other routes must be at the end when@test: security: diff --git a/api/src/Util/ParametrizedTestHelper.php b/api/src/Util/ParametrizedTestHelper.php new file mode 100644 index 0000000000..c70461ef41 --- /dev/null +++ b/api/src/Util/ParametrizedTestHelper.php @@ -0,0 +1,14 @@ +enableProfiler(); + $client->request('GET', $endpoint); + + $collector = $client->getProfile()->getCollector('db'); + /* + * 3 is: + * BEGIN TRANSACTION + * SAVEPOINT + * RELEASE SAVEPOINT + */ + assertThat($collector->getQueryCount(), equalTo(3)); + } + + public static function getProtectedEnpoints(): array { + $protectedEnpoints = array_filter(self::getEndPoints(), function (string $endpoint) { + return self::isProtectedByFirewall($endpoint); + }); + + return ParametrizedTestHelper::asParameterTestSets($protectedEnpoints); + } + + /** + * @throws ClientExceptionInterface + * @throws DecodingExceptionInterface + * @throws RedirectionExceptionInterface + * @throws ServerExceptionInterface + * @throws TransportExceptionInterface + */ + #[DataProvider('getUnprotectedEnpoints')] + public function testUnprotectedEndpointsMayResultInQuery(string $endpoint) { + $client = self::createBasicClient(); + $client->enableProfiler(); + $client->request('GET', $endpoint); + + $collector = $client->getProfile()->getCollector('db'); + /* + * 3 is: + * BEGIN TRANSACTION + * SAVEPOINT + * RELEASE SAVEPOINT + */ + assertThat($collector->getQueryCount(), greaterThanOrEqual(3)); + } + + public static function getUnprotectedEnpoints() { + $protectedEnpoints = array_filter(self::getEndPoints(), function (string $endpoint) { + return !self::isProtectedByFirewall($endpoint); + }); + + return ParametrizedTestHelper::asParameterTestSets($protectedEnpoints); + } + + /** + * @throws RedirectionExceptionInterface + * @throws DecodingExceptionInterface + * @throws ClientExceptionInterface + * @throws TransportExceptionInterface + * @throws ServerExceptionInterface + */ + private static function getEndPoints() { + static::bootKernel(); + $client = static::createBasicClient(); + $response = $client->request('GET', '/'); + + $responseArray = $response->toArray(); + $onlyUrls = array_map(fn (array $item) => $item['href'], $responseArray['_links']); + + return array_map(fn (string $uriTemplate) => preg_replace('/\\{[^}]*}/', '', $uriTemplate), $onlyUrls); + } + + private static function isProtectedByFirewall(mixed $endpoint): bool { + return match ($endpoint) { + '/authentication_token' => false, + '/auth/google' => false, + '/auth/pbsmidata' => false, + '/auth/cevidb' => false, + '/auth/jubladb' => false, + '/auth/reset_password' => false, + '/content_types' => false, + '/invitations' => false, + default => true + }; + } +} diff --git a/api/tests/Api/Invitations/AcceptInvitationTest.php b/api/tests/Api/Invitations/AcceptInvitationTest.php index f97858c56e..307112e5bc 100644 --- a/api/tests/Api/Invitations/AcceptInvitationTest.php +++ b/api/tests/Api/Invitations/AcceptInvitationTest.php @@ -12,6 +12,9 @@ use Symfony\Contracts\HttpClient\Exception\ServerExceptionInterface; use Symfony\Contracts\HttpClient\Exception\TransportExceptionInterface; +use function PHPUnit\Framework\assertThat; +use function PHPUnit\Framework\greaterThanOrEqual; + /** * @internal */ @@ -33,6 +36,33 @@ public function testAcceptInvitationFailsWhenNotLoggedIn() { $this->assertResponseStatusCodeSame(401); } + /** + * @throws TransportExceptionInterface + */ + public function testAcceptInvitationDoesNotHitDBWhenNotLoggedIn() { + /** @var CampCollaboration $campCollaboration */ + $campCollaboration = static::getFixture('campCollaboration2invitedCampUnrelated'); + $client = static::createBasicClient(); + $client->enableProfiler(); + $client->request( + 'PATCH', + "/invitations/{$campCollaboration->inviteKey}/".Invitation::ACCEPT, + [ + 'json' => [], + 'headers' => ['Content-Type' => 'application/merge-patch+json'], + ] + ); + + $collector = $client->getProfile()->getCollector('db'); + /* + * 3 is: + * BEGIN TRANSACTION + * SAVEPOINT + * RELEASE SAVEPOINT + */ + assertThat($collector->getQueryCount(), greaterThanOrEqual(3)); + } + /** * @throws RedirectionExceptionInterface * @throws DecodingExceptionInterface diff --git a/api/tests/Api/Invitations/InvitationGraphQLTest.php b/api/tests/Api/Invitations/InvitationGraphQLTest.php index 6f1dc40087..12ae576756 100644 --- a/api/tests/Api/Invitations/InvitationGraphQLTest.php +++ b/api/tests/Api/Invitations/InvitationGraphQLTest.php @@ -37,14 +37,40 @@ public function testFindInvitationWhenNotLoggedIn() { static::createClient()->request('GET', '/graphql?'.http_build_query(['query' => $query])); + $this->assertResponseStatusCodeSame(401); + } + + /** + * @throws ClientExceptionInterface + * @throws DecodingExceptionInterface + * @throws RedirectionExceptionInterface + * @throws ServerExceptionInterface + * @throws TransportExceptionInterface + */ + public function testFindInvitationWhenLoggedIn() { + /** @var CampCollaboration $campCollaboration */ + $campCollaboration = static::getFixture('campCollaboration4invited'); + $query = " + { + invitation(id: \"invitations/{$campCollaboration->inviteKey}/find\") { + campTitle + campId + userDisplayName + userAlreadyInCamp + } + } + "; + + static::createClientWithCredentials()->request('GET', '/graphql?'.http_build_query(['query' => $query])); + $this->assertResponseStatusCodeSame(200); $this->assertJsonContains([ 'data' => [ 'invitation' => [ 'campId' => $campCollaboration->camp->getId(), 'campTitle' => $campCollaboration->camp->title, - 'userDisplayName' => null, - 'userAlreadyInCamp' => null, + 'userDisplayName' => 'Bi-Pi', + 'userAlreadyInCamp' => true, ], ], ]); diff --git a/api/tests/Api/Profiles/ReadProfileTest.php b/api/tests/Api/Profiles/ReadProfileTest.php index c7d94dd337..3c9528d388 100644 --- a/api/tests/Api/Profiles/ReadProfileTest.php +++ b/api/tests/Api/Profiles/ReadProfileTest.php @@ -12,7 +12,7 @@ class ReadProfileTest extends ECampApiTestCase { public function testGetSingleProfileIsDeniedForAnonymousUser() { $user = static::getFixture('user1manager'); static::createBasicClient()->request('GET', '/profiles/'.$user->getId()); - $this->assertResponseStatusCodeSame(404); + $this->assertResponseStatusCodeSame(401); } public function testGetSingleProfileIsDeniedForUnrelatedUser() { diff --git a/api/tests/Api/Profiles/UpdateProfileTest.php b/api/tests/Api/Profiles/UpdateProfileTest.php index 1826388a2a..2270e3da14 100644 --- a/api/tests/Api/Profiles/UpdateProfileTest.php +++ b/api/tests/Api/Profiles/UpdateProfileTest.php @@ -15,7 +15,7 @@ public function testPatchProfileIsDeniedForAnonymousProfile() { static::createBasicClient()->request('PATCH', '/profiles/'.$user->getId(), ['json' => [ 'nickname' => 'Linux', ], 'headers' => ['Content-Type' => 'application/merge-patch+json']]); - $this->assertResponseStatusCodeSame(404); + $this->assertResponseStatusCodeSame(401); } public function testPatchProfileIsDeniedForRelatedProfile() { diff --git a/api/tests/Api/Users/ReadUserTest.php b/api/tests/Api/Users/ReadUserTest.php index e84aad8ad5..23a1f308dd 100644 --- a/api/tests/Api/Users/ReadUserTest.php +++ b/api/tests/Api/Users/ReadUserTest.php @@ -22,7 +22,7 @@ public function testGetSingleUserIsDeniedForAnonymousUser() { public function testGetUnknownUserReturns404ForAnonymousUser() { static::createBasicClient()->request('GET', '/users/1'); - $this->assertResponseStatusCodeSame(404); + $this->assertResponseStatusCodeSame(401); } public function testGetUnknownUserReturns404() {