From dde0c0a4c7d67adb5ee78ce3ffbec5c54601c812 Mon Sep 17 00:00:00 2001 From: Jonathan Johnson Date: Sun, 14 Nov 2021 21:58:06 -0800 Subject: [PATCH 1/8] Require authentication to access by default When we updated our security config a change was needed to default protect resources as it appears symfony moved to default allow with this change. I've added a configuration option to protect everything by default and added some tests to ensure this stays true. --- config/packages/security.yaml | 1 + tests/AbstractEndpointTest.php | 109 ++++++++++++++++++ tests/Endpoints/AcademicYearTest.php | 33 ++++++ tests/Endpoints/AuthenticationTest.php | 45 ++++++++ tests/Endpoints/CurrentSessionTest.php | 18 ++- .../CurriculumInventoryExportTest.php | 12 ++ tests/Endpoints/SchooleventsTest.php | 23 ++++ tests/Endpoints/UsereventTest.php | 23 ++++ tests/Endpoints/UsermaterialsTest.php | 22 ++++ tests/GetEndpointTestable.php | 6 + tests/ReadWriteEndpointTest.php | 26 +++++ 11 files changed, 317 insertions(+), 1 deletion(-) diff --git a/config/packages/security.yaml b/config/packages/security.yaml index 7ce87d72d4..e7e1c96649 100644 --- a/config/packages/security.yaml +++ b/config/packages/security.yaml @@ -3,6 +3,7 @@ security: access_control: - { path: ^/api/doc, roles: PUBLIC_ACCESS } - { path: ^/(auth|application)/(login|config|logout), roles: PUBLIC_ACCESS } + - { path: '^/', roles: IS_AUTHENTICATED_FULLY } access_decision_manager: allow_if_all_abstain: false strategy: unanimous diff --git a/tests/AbstractEndpointTest.php b/tests/AbstractEndpointTest.php index 6c8182b636..6bf9b5b1aa 100644 --- a/tests/AbstractEndpointTest.php +++ b/tests/AbstractEndpointTest.php @@ -960,6 +960,28 @@ protected function badPostTest(array $data, $code = Response::HTTP_BAD_REQUEST) $this->assertJsonResponse($response, $code); } + /** + * Test POSTing without authentication to the API + */ + protected function anonymousDeniedPostTest(array $data) + { + $endpoint = $this->getPluralName(); + $responseKey = $this->getCamelCasedPluralName(); + $this->createJsonRequest( + 'POST', + $this->getUrl( + $this->kernelBrowser, + "app_api_${endpoint}_post", + ['version' => $this->apiVersion] + ), + json_encode([$responseKey => [$data]]), + ); + + $response = $this->kernelBrowser->getResponse(); + + $this->assertJsonResponse($response, Response::HTTP_UNAUTHORIZED); + } + /** * Test POSTing bad data to the API * @param array $data @@ -985,6 +1007,28 @@ protected function badPutTest(array $data, $id, $code = Response::HTTP_BAD_REQUE $this->assertJsonResponse($response, $code); } + /** + * Test PUTing as anonymous to the API + */ + protected function anonymousDeniedPutTest(array $data) + { + $endpoint = $this->getPluralName(); + $responseKey = $this->getCamelCasedPluralName(); + $this->createJsonRequest( + 'PUT', + $this->getUrl( + $this->kernelBrowser, + "app_api_${endpoint}_put", + ['version' => $this->apiVersion, 'id' => $data['id']] + ), + json_encode([$responseKey => [$data]]), + ); + + $response = $this->kernelBrowser->getResponse(); + + $this->assertJsonResponse($response, Response::HTTP_UNAUTHORIZED); + } + /** * When relational data is sent to the API ensure it * is recorded on the non-owning side of the relationship @@ -1126,6 +1170,28 @@ protected function patchJsonApiTest(array $data, object $postData) return $fetchedResponseData; } + /** + * Test PATCHing as anonymous to the API + */ + protected function anonymousDeniedPatchTest(array $data) + { + $endpoint = $this->getPluralName(); + $responseKey = $this->getCamelCasedPluralName(); + $this->createJsonRequest( + 'PATCH', + $this->getUrl( + $this->kernelBrowser, + "app_api_${endpoint}_patch", + ['version' => $this->apiVersion, 'id' => $data['id']] + ), + json_encode([$responseKey => [$data]]), + ); + + $response = $this->kernelBrowser->getResponse(); + + $this->assertJsonResponse($response, Response::HTTP_UNAUTHORIZED); + } + /** * Test deleting an object from the API * @@ -1191,6 +1257,49 @@ protected function notFoundTest($badId) $this->assertJsonResponse($response, Response::HTTP_NOT_FOUND); } + /** + * Ensure that anonymous users cannot access the resource + */ + protected function anonymousAccessDeniedOneTest() + { + $endpoint = $this->getPluralName(); + $loader = $this->getDataLoader(); + $data = $loader->getOne(); + $this->createJsonRequest( + 'GET', + $this->getUrl( + $this->kernelBrowser, + "app_api_${endpoint}_getone", + ['version' => $this->apiVersion, 'id' => $data['id']] + ), + ); + + $response = $this->kernelBrowser->getResponse(); + + $this->assertJsonResponse($response, Response::HTTP_UNAUTHORIZED); + } + + /** + * Ensure that anonymous users cannot access the resource + */ + protected function anonymousAccessDeniedAllTest() + { + $endpoint = $this->getPluralName(); + $loader = $this->getDataLoader(); + $this->createJsonRequest( + 'GET', + $this->getUrl( + $this->kernelBrowser, + "app_api_${endpoint}_getall", + ['version' => $this->apiVersion] + ), + ); + + $response = $this->kernelBrowser->getResponse(); + + $this->assertJsonResponse($response, Response::HTTP_UNAUTHORIZED); + } + /** * Test that a filter returns the expected data * @param array $filters we are using diff --git a/tests/Endpoints/AcademicYearTest.php b/tests/Endpoints/AcademicYearTest.php index 0fcd78b464..6382a5a1b9 100644 --- a/tests/Endpoints/AcademicYearTest.php +++ b/tests/Endpoints/AcademicYearTest.php @@ -137,4 +137,37 @@ protected function getYears() return $academicYears; } + + public function anonymousAccessDeniedOneTest() + { + $academicYears = $this->getYears(); + $id = $academicYears[0]['id']; + $this->createJsonRequest( + 'GET', + $this->getUrl( + $this->kernelBrowser, + "app_api_academicyears_getone", + ['version' => $this->apiVersion, 'id' => $id] + ), + ); + + $response = $this->kernelBrowser->getResponse(); + + $this->assertJsonResponse($response, Response::HTTP_UNAUTHORIZED); + } + public function anonymousAccessDeniedAllTest() + { + $this->createJsonRequest( + 'GET', + $this->getUrl( + $this->kernelBrowser, + "app_api_academicyears_getall", + ['version' => $this->apiVersion] + ), + ); + + $response = $this->kernelBrowser->getResponse(); + + $this->assertJsonResponse($response, Response::HTTP_UNAUTHORIZED); + } } diff --git a/tests/Endpoints/AuthenticationTest.php b/tests/Endpoints/AuthenticationTest.php index 08f6154bee..8567750f85 100644 --- a/tests/Endpoints/AuthenticationTest.php +++ b/tests/Endpoints/AuthenticationTest.php @@ -552,4 +552,49 @@ public function testPutReadOnly($key = null, $id = null, $value = null, $skipped $this->putTest($data, $postData, $id); } } + + public function anonymousAccessDeniedOneTest() + { + $loader = $this->getDataLoader(); + $data = $loader->getOne(); + $this->createJsonRequest( + 'GET', + $this->getUrl( + $this->kernelBrowser, + "app_api_authentications_getone", + ['version' => $this->apiVersion, 'id' => $data['user']] + ), + ); + + $response = $this->kernelBrowser->getResponse(); + + $this->assertJsonResponse($response, Response::HTTP_UNAUTHORIZED); + } + public function anonymousAccessDeniedAllTest() + { + $this->createJsonRequest( + 'GET', + $this->getUrl( + $this->kernelBrowser, + "app_api_authentications_getall", + ['version' => $this->apiVersion] + ), + ); + + $response = $this->kernelBrowser->getResponse(); + + $this->assertJsonResponse($response, Response::HTTP_UNAUTHORIZED); + } + + protected function anonymousDeniedPutTest(array $data) + { + $data['id'] = $data['user']; + parent::anonymousDeniedPutTest($data); + } + + protected function anonymousDeniedPatchTest(array $data) + { + $data['id'] = $data['user']; + parent::anonymousDeniedPatchTest($data); + } } diff --git a/tests/Endpoints/CurrentSessionTest.php b/tests/Endpoints/CurrentSessionTest.php index bed82319cf..875cc9f7e7 100644 --- a/tests/Endpoints/CurrentSessionTest.php +++ b/tests/Endpoints/CurrentSessionTest.php @@ -42,7 +42,7 @@ public function tearDown(): void unset($this->fixtures); } - public function testGetGetCurrentSession() + public function testGetCurrentSession() { $url = $this->getUrl( $this->kernelBrowser, @@ -69,4 +69,20 @@ public function testGetGetCurrentSession() $this->assertEquals(2, $data['userId']); } + public function testAccessDeniedForAnonymousUser() + { + $url = $this->getUrl( + $this->kernelBrowser, + 'ilios_api_currentsession', + ['version' => $this->apiVersion] + ); + $this->makeJsonRequest( + $this->kernelBrowser, + 'GET', + $url, + ); + + $response = $this->kernelBrowser->getResponse(); + $this->assertJsonResponse($response, Response::HTTP_UNAUTHORIZED); + } } diff --git a/tests/Endpoints/CurriculumInventoryExportTest.php b/tests/Endpoints/CurriculumInventoryExportTest.php index fdd3506e7d..26f3ceb12f 100644 --- a/tests/Endpoints/CurriculumInventoryExportTest.php +++ b/tests/Endpoints/CurriculumInventoryExportTest.php @@ -132,4 +132,16 @@ protected function fourOhFourTest($type, array $parameters = []) $this->assertJsonResponse($response, Response::HTTP_NOT_FOUND); } + public function testAnonymousPostDenied() + { + $url = '/api/' . $this->apiVersion . '/curriculuminventoryexports/'; + $this->createJsonRequest( + 'POST', + $url, + ); + + $response = $this->kernelBrowser->getResponse(); + + $this->assertJsonResponse($response, Response::HTTP_UNAUTHORIZED); + } } diff --git a/tests/Endpoints/SchooleventsTest.php b/tests/Endpoints/SchooleventsTest.php index 7330ea04a0..2d43bc15ad 100644 --- a/tests/Endpoints/SchooleventsTest.php +++ b/tests/Endpoints/SchooleventsTest.php @@ -681,6 +681,29 @@ public function testMissingT0() $this->assertJsonResponse($response, Response::HTTP_BAD_REQUEST); } + public function testAccessDenied() + { + $parameters = [ + 'version' => $this->apiVersion, + 'id' => 1, + 'from' => 1000000, + 'to' => 1000000, + ]; + $url = $this->getUrl( + $this->kernelBrowser, + 'ilios_api_schoolevents', + $parameters + ); + $this->createJsonRequest( + 'GET', + $url, + ); + + $response = $this->kernelBrowser->getResponse(); + + $this->assertJsonResponse($response, Response::HTTP_UNAUTHORIZED); + } + protected function getEvents($schoolId, $from, $to, $userId = null) { $parameters = [ diff --git a/tests/Endpoints/UsereventTest.php b/tests/Endpoints/UsereventTest.php index d98bf483eb..e728aff92a 100644 --- a/tests/Endpoints/UsereventTest.php +++ b/tests/Endpoints/UsereventTest.php @@ -1091,6 +1091,29 @@ public function testMissingTo() $this->assertJsonResponse($response, Response::HTTP_BAD_REQUEST); } + public function testAccessDenied() + { + $parameters = [ + 'version' => $this->apiVersion, + 'from' => 1000000, + 'to' => 1000000, + 'id' => 99, + ]; + $url = $this->getUrl( + $this->kernelBrowser, + 'ilios_api_userevents', + $parameters + ); + $this->createJsonRequest( + 'GET', + $url, + ); + + $response = $this->kernelBrowser->getResponse(); + + $this->assertJsonResponse($response, Response::HTTP_UNAUTHORIZED); + } + /** * @param int $userId * @param int $from diff --git a/tests/Endpoints/UsermaterialsTest.php b/tests/Endpoints/UsermaterialsTest.php index a0eb7eec9c..b595882e52 100644 --- a/tests/Endpoints/UsermaterialsTest.php +++ b/tests/Endpoints/UsermaterialsTest.php @@ -156,6 +156,28 @@ public function testGetMaterialsBeforeTheEndOfTime() $this->assertCount(9, $materials, 'All materials returned'); } + public function testAccessDenied() + { + $parameters = [ + 'version' => $this->apiVersion, + 'id' => 99 + ]; + $url = $this->getUrl( + $this->kernelBrowser, + 'ilios_api_usermaterials', + $parameters + ); + + $this->createJsonRequest( + 'GET', + $url + ); + + $response = $this->kernelBrowser->getResponse(); + + $this->assertJsonResponse($response, Response::HTTP_UNAUTHORIZED); + } + protected function getMaterials($userId, $before = null, $after = null, $authUser = null) { $parameters = [ diff --git a/tests/GetEndpointTestable.php b/tests/GetEndpointTestable.php index c0ebfc028e..3878f5e87a 100644 --- a/tests/GetEndpointTestable.php +++ b/tests/GetEndpointTestable.php @@ -66,4 +66,10 @@ public function testFilters(array $dataKeys = [], array $filterParts = [], $skip } $this->filterTest($filters, $expectedData); } + + public function testAccessDenied() + { + $this->anonymousAccessDeniedOneTest(); + $this->anonymousAccessDeniedAllTest(); + } } diff --git a/tests/ReadWriteEndpointTest.php b/tests/ReadWriteEndpointTest.php index 66e81ce072..f8dd9a0ffa 100644 --- a/tests/ReadWriteEndpointTest.php +++ b/tests/ReadWriteEndpointTest.php @@ -66,6 +66,16 @@ public function testPostBad() $this->badPostTest($data); } + /** + * Test a failure when posting an object + */ + public function testPostAnonymousAccessDenied() + { + $dataLoader = $this->getDataLoader(); + $data = $dataLoader->create(); + $this->anonymousDeniedPostTest($data); + } + /** * Test POST several of this type of object */ @@ -226,4 +236,20 @@ public function testDelete() $data = $dataLoader->getOne(); $this->deleteTest($data['id']); } + + public function testPutAnonymousAccessDenied() + { + $dataLoader = $this->getDataLoader(); + $data = $dataLoader->getOne(); + + $this->anonymousDeniedPutTest($data); + } + + public function testPatchAnonymousAccessDenied() + { + $dataLoader = $this->getDataLoader(); + $data = $dataLoader->getOne(); + + $this->anonymousDeniedPatchTest($data); + } } From 7427e6ddc6b8eb9deb426095e985c4b0750e58ca Mon Sep 17 00:00:00 2001 From: Jonathan Johnson Date: Sun, 14 Nov 2021 23:24:30 -0800 Subject: [PATCH 2/8] Update Auth Controller for new firewall Simplified the access checks in the auth controller to just send an error if unauthenticated. This is simpler and more correct since no access should come to this route unauthenticated. The tests were working before the firewall was configured to block all access to these routes. Now we need to modify them slightly to ensure that access is actually blocked. --- src/Controller/AuthController.php | 62 +++++++++++-------------- tests/Controller/AuthControllerTest.php | 8 +--- 2 files changed, 29 insertions(+), 41 deletions(-) diff --git a/src/Controller/AuthController.php b/src/Controller/AuthController.php index bd82124dce..18a22a4709 100644 --- a/src/Controller/AuthController.php +++ b/src/Controller/AuthController.php @@ -17,6 +17,8 @@ use Symfony\Component\HttpFoundation\Response; use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface; +use function sleep; + class AuthController extends AbstractController { /** @@ -35,15 +37,12 @@ public function loginAction(Request $request, AuthenticationInterface $authentic public function whoamiAction(TokenStorageInterface $tokenStorage): JsonResponse { $token = $tokenStorage->getToken(); - if ($token?->isAuthenticated()) { - /** @var SessionUserInterface $sessionUser */ - $sessionUser = $token->getUser(); - if ($sessionUser instanceof SessionUserInterface) { - return new JsonResponse(['userId' => $sessionUser->getId()], Response::HTTP_OK); - } + $sessionUser = $token?->getUser(); + if (!$token?->isAuthenticated() || !$sessionUser instanceof SessionUserInterface) { + throw new Exception('Attempted to access whoami with no valid user'); } - return new JsonResponse(['userId' => null], Response::HTTP_UNAUTHORIZED); + return new JsonResponse(['userId' => $sessionUser->getId()], Response::HTTP_OK); } /** @@ -56,16 +55,14 @@ public function tokenAction( JsonWebTokenManager $jwtManager ): JsonResponse { $token = $tokenStorage->getToken(); - if ($token?->isAuthenticated()) { - $sessionUser = $token->getUser(); - if ($sessionUser instanceof SessionUserInterface) { - $ttl = $request->get('ttl') ? $request->get('ttl') : 'PT8H'; - $jwt = $jwtManager->createJwtFromSessionUser($sessionUser, $ttl); - return new JsonResponse(['jwt' => $jwt], Response::HTTP_OK); - } + $sessionUser = $token?->getUser(); + if (!$token?->isAuthenticated() || !$sessionUser instanceof SessionUserInterface) { + throw new Exception('Attempted to access token with no valid user'); } - return new JsonResponse(['jwt' => null], Response::HTTP_UNAUTHORIZED); + $ttl = $request->get('ttl') ? $request->get('ttl') : 'PT8H'; + $jwt = $jwtManager->createJwtFromSessionUser($sessionUser, $ttl); + return new JsonResponse(['jwt' => $jwt], Response::HTTP_OK); } /** @@ -92,28 +89,25 @@ public function invalidateTokensAction( ): JsonResponse { $now = new \DateTime(); $token = $tokenStorage->getToken(); - if ($token?->isAuthenticated()) { - /** @var SessionUserInterface $sessionUser */ - $sessionUser = $token->getUser(); - if ($sessionUser instanceof SessionUserInterface) { - /** @var UserInterface $user */ - $user = $userRepository->findOneBy(['id' => $sessionUser->getId()]); - $authentication = $authenticationRepository->findOneBy(['user' => $user->getId()]); - if (!$authentication) { - $authentication = $authenticationRepository->create(); - $authentication->setUser($user); - } + $sessionUser = $token?->getUser(); + if (!$token?->isAuthenticated() || !$sessionUser instanceof SessionUserInterface) { + throw new Exception('Attempted to access invalidate tokens with no valid user'); + } - $authentication->setInvalidateTokenIssuedBefore($now); - $authenticationRepository->update($authentication); + /** @var UserInterface $user */ + $user = $userRepository->findOneBy(['id' => $sessionUser->getId()]); + $authentication = $authenticationRepository->findOneBy(['user' => $user->getId()]); + if (!$authentication) { + $authentication = $authenticationRepository->create(); + $authentication->setUser($user); + } - sleep(1); - $jwt = $jwtManager->createJwtFromSessionUser($sessionUser); + $authentication->setInvalidateTokenIssuedBefore($now); + $authenticationRepository->update($authentication); - return new JsonResponse(['jwt' => $jwt], Response::HTTP_OK); - } - } + sleep(1); + $jwt = $jwtManager->createJwtFromSessionUser($sessionUser); - throw new Exception('Attempted to invalidate token with no valid user'); + return new JsonResponse(['jwt' => $jwt], Response::HTTP_OK); } } diff --git a/tests/Controller/AuthControllerTest.php b/tests/Controller/AuthControllerTest.php index 76950e9526..248f8ef72b 100644 --- a/tests/Controller/AuthControllerTest.php +++ b/tests/Controller/AuthControllerTest.php @@ -158,9 +158,6 @@ public function testWhoAmIUnauthenticated() $response = $this->kernelBrowser->getResponse(); $this->assertJsonResponse($response, Response::HTTP_UNAUTHORIZED); - $response = json_decode($response->getContent(), true); - $this->assertArrayHasKey('userId', $response); - $this->assertSame($response['userId'], null); } public function testWhoAmIExpiredToken() @@ -242,9 +239,6 @@ public function testGetTokenForUnauthenticatedUser() ); $response = $this->kernelBrowser->getResponse(); $this->assertJsonResponse($response, Response::HTTP_UNAUTHORIZED); - $response = json_decode($response->getContent(), true); - $this->assertArrayHasKey('jwt', $response); - $this->assertSame($response['jwt'], null); } public function testGetTokenForExpiredToken() @@ -301,7 +295,7 @@ public function testInvalidateTokenForUnauthenticatedUser() ); $response = $this->kernelBrowser->getResponse(); - $this->assertJsonResponse($response, Response::HTTP_INTERNAL_SERVER_ERROR); + $this->assertJsonResponse($response, Response::HTTP_UNAUTHORIZED); } protected function getExpiredToken(int $userId): string From 349eeecae55692a318888109b06c9ccdd276a46b Mon Sep 17 00:00:00 2001 From: Jonathan Johnson Date: Mon, 15 Nov 2021 09:40:36 -0800 Subject: [PATCH 3/8] Allow access to /api for all users This is our API landing page and anyone can see it. --- config/packages/security.yaml | 1 + tests/Controller/ApiControllerTest.php | 27 ++++++++++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/config/packages/security.yaml b/config/packages/security.yaml index e7e1c96649..639571f787 100644 --- a/config/packages/security.yaml +++ b/config/packages/security.yaml @@ -2,6 +2,7 @@ security: enable_authenticator_manager: true access_control: - { path: ^/api/doc, roles: PUBLIC_ACCESS } + - { path: ^/api, roles: PUBLIC_ACCESS } - { path: ^/(auth|application)/(login|config|logout), roles: PUBLIC_ACCESS } - { path: '^/', roles: IS_AUTHENTICATED_FULLY } access_decision_manager: diff --git a/tests/Controller/ApiControllerTest.php b/tests/Controller/ApiControllerTest.php index 1191439de9..b8c171f2f2 100644 --- a/tests/Controller/ApiControllerTest.php +++ b/tests/Controller/ApiControllerTest.php @@ -72,4 +72,31 @@ public function testBadVersion() $response = $this->kernelBrowser->getResponse(); $this->assertJsonResponse($response, Response::HTTP_NOT_FOUND); } + + public function testApiInfoAuthenticated() + { + $this->kernelBrowser->request( + 'GET', + '/api', + [], + [], + ['HTTP_X-JWT-Authorization' => 'Token ' . $this->getTokenForUser($this->kernelBrowser, 1)], + ); + + $response = $this->kernelBrowser->getResponse(); + $this->assertEquals(Response::HTTP_OK, $response->getStatusCode()); + $this->assertStringContainsString('

API Info

', $response->getContent()); + } + + public function testApiInfoNotAuthenticated() + { + $this->kernelBrowser->request( + 'GET', + '/api', + ); + + $response = $this->kernelBrowser->getResponse(); + $this->assertEquals(Response::HTTP_OK, $response->getStatusCode()); + $this->assertStringContainsString('

API Info

', $response->getContent()); + } } From 8947c05f89c808b0a7739adffec838318c957740 Mon Sep 17 00:00:00 2001 From: Jonathan Johnson Date: Mon, 15 Nov 2021 09:51:37 -0800 Subject: [PATCH 4/8] Consolidate all firewalls In the new symfony authentication system these duplicates are no longer necessary. By removing the path entry from our main firewall it will be applied to everything. I also separated the access_control entries to be a bit less confusing and more explicit. Now that LMs are included in the main firewall we also needed to allow public access to maintain our current token system. --- config/packages/security.yaml | 35 ++++++----------------------------- 1 file changed, 6 insertions(+), 29 deletions(-) diff --git a/config/packages/security.yaml b/config/packages/security.yaml index 639571f787..6bd5ae8f9a 100644 --- a/config/packages/security.yaml +++ b/config/packages/security.yaml @@ -1,9 +1,11 @@ security: enable_authenticator_manager: true access_control: - - { path: ^/api/doc, roles: PUBLIC_ACCESS } - - { path: ^/api, roles: PUBLIC_ACCESS } - - { path: ^/(auth|application)/(login|config|logout), roles: PUBLIC_ACCESS } + - { path: '^/api/doc', roles: PUBLIC_ACCESS } + - { path: '^/api$', roles: PUBLIC_ACCESS } + - { path: '^/lm/\s+', roles: PUBLIC_ACCESS } + - { path: '^/application/config', roles: PUBLIC_ACCESS } + - { path: '^/auth/(login|logout)', roles: PUBLIC_ACCESS } - { path: '^/', roles: IS_AUTHENTICATED_FULLY } access_decision_manager: allow_if_all_abstain: false @@ -20,32 +22,7 @@ security: dev: pattern: ^/(_(profiler|wdt)|css|images|js)/ security: false - authenticated_auth: - pattern: ^/auth - stateless: true - custom_authenticators: - - App\Security\JsonWebTokenAuthenticator - provider: session_user - authenticated_application: - pattern: ^/application - stateless: true - custom_authenticators: - - App\Security\JsonWebTokenAuthenticator - provider: session_user - upload: - pattern: ^/upload - stateless: true - custom_authenticators: - - App\Security\JsonWebTokenAuthenticator - provider: session_user - errors: - pattern: ^/errors - stateless: true - custom_authenticators: - - App\Security\JsonWebTokenAuthenticator - provider: session_user - default: - pattern: ^/api + main: stateless: true custom_authenticators: - App\Security\JsonWebTokenAuthenticator From 9df6e684553cba958f0171e7d32594e43bf3f93e Mon Sep 17 00:00:00 2001 From: Jonathan Johnson Date: Mon, 15 Nov 2021 10:31:35 -0800 Subject: [PATCH 5/8] Duplicate LM token regex from route This matches the requirements for a hash string from our router and ensures that only well formed LM tokens are authorized. --- config/packages/security.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/packages/security.yaml b/config/packages/security.yaml index 6bd5ae8f9a..e1a578ac40 100644 --- a/config/packages/security.yaml +++ b/config/packages/security.yaml @@ -3,7 +3,7 @@ security: access_control: - { path: '^/api/doc', roles: PUBLIC_ACCESS } - { path: '^/api$', roles: PUBLIC_ACCESS } - - { path: '^/lm/\s+', roles: PUBLIC_ACCESS } + - { path: '^/lm/[a-zA-Z0-9]{64}$', roles: PUBLIC_ACCESS } - { path: '^/application/config', roles: PUBLIC_ACCESS } - { path: '^/auth/(login|logout)', roles: PUBLIC_ACCESS } - { path: '^/', roles: IS_AUTHENTICATED_FULLY } From b287540717a16f6b8688ad4d9a11dc3394510a41 Mon Sep 17 00:00:00 2001 From: Jonathan Johnson Date: Mon, 15 Nov 2021 10:42:38 -0800 Subject: [PATCH 6/8] Allow anyone with a token to download the CI report --- config/packages/security.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/config/packages/security.yaml b/config/packages/security.yaml index e1a578ac40..e9dbea4290 100644 --- a/config/packages/security.yaml +++ b/config/packages/security.yaml @@ -4,6 +4,7 @@ security: - { path: '^/api/doc', roles: PUBLIC_ACCESS } - { path: '^/api$', roles: PUBLIC_ACCESS } - { path: '^/lm/[a-zA-Z0-9]{64}$', roles: PUBLIC_ACCESS } + - { path: '^/ci-report-dl/[a-zA-Z0-9]{64}$', roles: PUBLIC_ACCESS } - { path: '^/application/config', roles: PUBLIC_ACCESS } - { path: '^/auth/(login|logout)', roles: PUBLIC_ACCESS } - { path: '^/', roles: IS_AUTHENTICATED_FULLY } From 14199102efe82f71d29f07262b4f3bb76c995c5a Mon Sep 17 00:00:00 2001 From: Jonathan Johnson Date: Mon, 15 Nov 2021 10:46:32 -0800 Subject: [PATCH 7/8] Allow public access to health checks These aren't much use if you have to be logged in. --- config/packages/security.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/config/packages/security.yaml b/config/packages/security.yaml index e9dbea4290..7b8baca60e 100644 --- a/config/packages/security.yaml +++ b/config/packages/security.yaml @@ -7,6 +7,7 @@ security: - { path: '^/ci-report-dl/[a-zA-Z0-9]{64}$', roles: PUBLIC_ACCESS } - { path: '^/application/config', roles: PUBLIC_ACCESS } - { path: '^/auth/(login|logout)', roles: PUBLIC_ACCESS } + - { path: '^/ilios/health', roles: PUBLIC_ACCESS } - { path: '^/', roles: IS_AUTHENTICATED_FULLY } access_decision_manager: allow_if_all_abstain: false From fb967f3b934ceb2a395ce49ca8f058c7555e7de5 Mon Sep 17 00:00:00 2001 From: Jonathan Johnson Date: Mon, 15 Nov 2021 11:03:41 -0800 Subject: [PATCH 8/8] Flip back to default allow In order to serve the frontend and all assets and all paths we have to allow access by default. This change does that and then lists routes where access should be denied. --- config/packages/security.yaml | 10 ++++++---- tests/Controller/ErrorControllerTest.php | 19 +++++++++++++++++++ tests/Controller/UploadControllerTest.php | 16 ++++++++++++++++ 3 files changed, 41 insertions(+), 4 deletions(-) diff --git a/config/packages/security.yaml b/config/packages/security.yaml index 7b8baca60e..51307cc6e1 100644 --- a/config/packages/security.yaml +++ b/config/packages/security.yaml @@ -3,12 +3,14 @@ security: access_control: - { path: '^/api/doc', roles: PUBLIC_ACCESS } - { path: '^/api$', roles: PUBLIC_ACCESS } - - { path: '^/lm/[a-zA-Z0-9]{64}$', roles: PUBLIC_ACCESS } - - { path: '^/ci-report-dl/[a-zA-Z0-9]{64}$', roles: PUBLIC_ACCESS } - { path: '^/application/config', roles: PUBLIC_ACCESS } - { path: '^/auth/(login|logout)', roles: PUBLIC_ACCESS } - - { path: '^/ilios/health', roles: PUBLIC_ACCESS } - - { path: '^/', roles: IS_AUTHENTICATED_FULLY } + - { path: '^/auth', roles: IS_AUTHENTICATED_FULLY } + - { path: '^/api', roles: IS_AUTHENTICATED_FULLY } + - { path: '^/application', roles: IS_AUTHENTICATED_FULLY } + - { path: '^/upload', roles: IS_AUTHENTICATED_FULLY } + - { path: '^/error', roles: IS_AUTHENTICATED_FULLY } + - { path: '^/', roles: PUBLIC_ACCESS } access_decision_manager: allow_if_all_abstain: false strategy: unanimous diff --git a/tests/Controller/ErrorControllerTest.php b/tests/Controller/ErrorControllerTest.php index f7ec4b20c2..40fbb51c72 100644 --- a/tests/Controller/ErrorControllerTest.php +++ b/tests/Controller/ErrorControllerTest.php @@ -53,4 +53,23 @@ public function testIndex() $response = $this->kernelBrowser->getResponse(); $this->assertEquals(Response::HTTP_NO_CONTENT, $response->getStatusCode(), $response->getContent()); } + + public function testAnonymousAccessDenied() + { + $faker = FakerFactory::create(); + + $data = [ + 'mainMessage' => $faker->text(100), + 'stack' => $faker->text(1000) + ]; + $this->makeJsonRequest( + $this->kernelBrowser, + 'POST', + '/errors', + json_encode(['data' => json_encode($data)]) + ); + + $response = $this->kernelBrowser->getResponse(); + $this->assertEquals(Response::HTTP_UNAUTHORIZED, $response->getStatusCode()); + } } diff --git a/tests/Controller/UploadControllerTest.php b/tests/Controller/UploadControllerTest.php index 118fa41e50..af35a019de 100644 --- a/tests/Controller/UploadControllerTest.php +++ b/tests/Controller/UploadControllerTest.php @@ -65,6 +65,22 @@ public function testUploadFile() $this->assertSame($data['filename'], 'TESTFILE.txt'); $this->assertSame($data['fileHash'], md5_file(__FILE__)); } + public function testAnonymousUploadFileDenied() + { + $client = static::createClient(); + + $this->makeJsonRequest( + $client, + 'POST', + '/upload', + null, + [], + ['file' => $this->fakeTestFile] + ); + + $response = $client->getResponse(); + $this->assertJsonResponse($response, Response::HTTP_UNAUTHORIZED); + } public function testBadUpload() {