Merge pull request #4745 from BacLuc/api-restrict-requests-in-firewall

api: restrict access to protected routes with firewall
ecamp · Mar 7, 2024 · 24b9995 · 24b9995
2 parents c3ce774 + 14354f8
commit 24b9995
Show file tree

Hide file tree

Showing 8 changed files with 198 additions and 7 deletions.
diff --git 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
@@ -0,0 +1,14 @@
+<?php
+
+namespace App\Util;
+
+class ParametrizedTestHelper {
+    public static function asParameterTestSets(array $array): array {
+        return array_reduce($array, function (?array $left, mixed $right) {
+            $newArray = $left ?? [];
+            $newArray[$right.''] = [$right];
+
+            return $newArray;
+        });
+    }
+}
diff --git a/api/tests/Api/FirewallTest.php b/api/tests/Api/FirewallTest.php
@@ -0,0 +1,115 @@
+<?php
+
+namespace Api;
+
+use App\Tests\Api\ECampApiTestCase;
+use App\Util\ParametrizedTestHelper;
+use PHPUnit\Framework\Attributes\DataProvider;
+use Symfony\Contracts\HttpClient\Exception\ClientExceptionInterface;
+use Symfony\Contracts\HttpClient\Exception\DecodingExceptionInterface;
+use Symfony\Contracts\HttpClient\Exception\RedirectionExceptionInterface;
+use Symfony\Contracts\HttpClient\Exception\ServerExceptionInterface;
+use Symfony\Contracts\HttpClient\Exception\TransportExceptionInterface;
+
+use function PHPUnit\Framework\assertThat;
+use function PHPUnit\Framework\equalTo;
+use function PHPUnit\Framework\greaterThanOrEqual;
+
+/**
+ * @internal
+ */
+class FirewallTest extends ECampApiTestCase {
+    /**
+     * @throws ClientExceptionInterface
+     * @throws DecodingExceptionInterface
+     * @throws RedirectionExceptionInterface
+     * @throws ServerExceptionInterface
+     * @throws TransportExceptionInterface
+     */
+    #[DataProvider('getProtectedEnpoints')]
+    public function testProtectedEndpointsDontResultInQuery(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(), 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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() {