From 04bcf1eef6ddece3f1f2d1f6ac0d14c6a236eba2 Mon Sep 17 00:00:00 2001 From: Franz Liedke Date: Tue, 20 Aug 2019 07:19:55 +0200 Subject: [PATCH 1/5] Fix inconsistent status codes HTTP 401 should be used when logging in (i.e. authenticating) would make a difference; HTTP 403 is reserved for requests that fail because the already authenticated user is not authorized (i.e. lacking permissions) to do something. --- .../ListNotificationsController.php | 8 +++--- src/Api/Controller/ListUsersController.php | 9 ++++--- src/Foundation/ErrorServiceProvider.php | 1 + src/Group/Command/CreateGroupHandler.php | 1 + src/User/AssertPermissionTrait.php | 27 ++++++++++--------- .../Exception/NotAuthenticatedException.php | 23 ++++++++++++++++ .../api/Auth/AuthenticateWithApiKeyTest.php | 2 +- .../Controller/CreateGroupControllerTest.php | 2 +- .../ListNotificationsControllerTest.php | 2 +- .../Controller/ListUsersControllerTest.php | 2 +- 10 files changed, 53 insertions(+), 24 deletions(-) create mode 100644 src/User/Exception/NotAuthenticatedException.php diff --git a/src/Api/Controller/ListNotificationsController.php b/src/Api/Controller/ListNotificationsController.php index b8aaeef539..352e7130a8 100644 --- a/src/Api/Controller/ListNotificationsController.php +++ b/src/Api/Controller/ListNotificationsController.php @@ -15,12 +15,14 @@ use Flarum\Discussion\Discussion; use Flarum\Http\UrlGenerator; use Flarum\Notification\NotificationRepository; -use Flarum\User\Exception\PermissionDeniedException; +use Flarum\User\AssertPermissionTrait; use Psr\Http\Message\ServerRequestInterface; use Tobscure\JsonApi\Document; class ListNotificationsController extends AbstractListController { + use AssertPermissionTrait; + /** * {@inheritdoc} */ @@ -67,9 +69,7 @@ protected function data(ServerRequestInterface $request, Document $document) { $actor = $request->getAttribute('actor'); - if ($actor->isGuest()) { - throw new PermissionDeniedException; - } + $this->assertRegistered($actor); $actor->markNotificationsAsRead()->save(); diff --git a/src/Api/Controller/ListUsersController.php b/src/Api/Controller/ListUsersController.php index 18d36ceab2..b10ed17a47 100644 --- a/src/Api/Controller/ListUsersController.php +++ b/src/Api/Controller/ListUsersController.php @@ -14,7 +14,7 @@ use Flarum\Api\Serializer\UserSerializer; use Flarum\Http\UrlGenerator; use Flarum\Search\SearchCriteria; -use Flarum\User\Exception\PermissionDeniedException; +use Flarum\User\AssertPermissionTrait; use Flarum\User\Search\UserSearcher; use Illuminate\Support\Arr; use Psr\Http\Message\ServerRequestInterface; @@ -22,6 +22,8 @@ class ListUsersController extends AbstractListController { + use AssertPermissionTrait; + /** * {@inheritdoc} */ @@ -70,9 +72,8 @@ protected function data(ServerRequestInterface $request, Document $document) { $actor = $request->getAttribute('actor'); - if ($actor->cannot('viewUserList')) { - throw new PermissionDeniedException; - } + $this->assertRegistered($actor); + $this->assertCan($actor, 'viewUserList'); $query = Arr::get($this->extractFilter($request), 'q'); $sort = $this->extractSort($request); diff --git a/src/Foundation/ErrorServiceProvider.php b/src/Foundation/ErrorServiceProvider.php index aa7060bb50..2e7278039a 100644 --- a/src/Foundation/ErrorServiceProvider.php +++ b/src/Foundation/ErrorServiceProvider.php @@ -31,6 +31,7 @@ public function register() // 401 Unauthorized 'invalid_access_token' => 401, + 'not_authenticated' => 401, // 403 Forbidden 'forbidden' => 403, diff --git a/src/Group/Command/CreateGroupHandler.php b/src/Group/Command/CreateGroupHandler.php index 0364432137..8a000679e2 100644 --- a/src/Group/Command/CreateGroupHandler.php +++ b/src/Group/Command/CreateGroupHandler.php @@ -49,6 +49,7 @@ public function handle(CreateGroup $command) $actor = $command->actor; $data = $command->data; + $this->assertRegistered($actor); $this->assertCan($actor, 'createGroup'); $group = Group::build( diff --git a/src/User/AssertPermissionTrait.php b/src/User/AssertPermissionTrait.php index d6310440a6..8941b1615b 100644 --- a/src/User/AssertPermissionTrait.php +++ b/src/User/AssertPermissionTrait.php @@ -11,12 +11,13 @@ namespace Flarum\User; +use Flarum\User\Exception\NotAuthenticatedException; use Flarum\User\Exception\PermissionDeniedException; trait AssertPermissionTrait { /** - * @param $condition + * @param bool $condition * @throws PermissionDeniedException */ protected function assertPermission($condition) @@ -27,32 +28,34 @@ protected function assertPermission($condition) } /** - * @param User $actor - * @param string $ability - * @param mixed $arguments - * @throws PermissionDeniedException + * @param bool $condition + * @throws NotAuthenticatedException */ - protected function assertCan(User $actor, $ability, $arguments = []) + protected function assertAuthentication($condition) { - $this->assertPermission($actor->can($ability, $arguments)); + if (! $condition) { + throw new NotAuthenticatedException; + } } /** * @param User $actor - * @throws \Flarum\User\Exception\PermissionDeniedException + * @param string $ability + * @param mixed $arguments + * @throws PermissionDeniedException */ - protected function assertGuest(User $actor) + protected function assertCan(User $actor, $ability, $arguments = []) { - $this->assertPermission($actor->isGuest()); + $this->assertPermission($actor->can($ability, $arguments)); } /** * @param User $actor - * @throws PermissionDeniedException + * @throws NotAuthenticatedException */ protected function assertRegistered(User $actor) { - $this->assertPermission(! $actor->isGuest()); + $this->assertAuthentication(! $actor->isGuest()); } /** diff --git a/src/User/Exception/NotAuthenticatedException.php b/src/User/Exception/NotAuthenticatedException.php new file mode 100644 index 0000000000..bb45b87057 --- /dev/null +++ b/src/User/Exception/NotAuthenticatedException.php @@ -0,0 +1,23 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Flarum\User\Exception; + +use Exception; +use Flarum\Foundation\KnownError; + +class NotAuthenticatedException extends Exception implements KnownError +{ + public function getType(): string + { + return 'not_authenticated'; + } +} diff --git a/tests/integration/api/Auth/AuthenticateWithApiKeyTest.php b/tests/integration/api/Auth/AuthenticateWithApiKeyTest.php index 1ba3ce5a10..e895dafdf5 100644 --- a/tests/integration/api/Auth/AuthenticateWithApiKeyTest.php +++ b/tests/integration/api/Auth/AuthenticateWithApiKeyTest.php @@ -65,7 +65,7 @@ public function cannot_authorize_without_key() $response = $api->send(CreateGroupController::class, new Guest); - $this->assertEquals(403, $response->getStatusCode()); + $this->assertEquals(401, $response->getStatusCode()); } /** diff --git a/tests/integration/api/Controller/CreateGroupControllerTest.php b/tests/integration/api/Controller/CreateGroupControllerTest.php index d2edcada16..08275615d9 100644 --- a/tests/integration/api/Controller/CreateGroupControllerTest.php +++ b/tests/integration/api/Controller/CreateGroupControllerTest.php @@ -80,7 +80,7 @@ public function admin_can_create_group() /** * @test */ - public function unauthorized_user_cannot_create_group() + public function normal_user_cannot_create_group() { $this->actor = User::find(2); diff --git a/tests/integration/api/Controller/ListNotificationsControllerTest.php b/tests/integration/api/Controller/ListNotificationsControllerTest.php index 9f301607b4..0e11358258 100644 --- a/tests/integration/api/Controller/ListNotificationsControllerTest.php +++ b/tests/integration/api/Controller/ListNotificationsControllerTest.php @@ -36,7 +36,7 @@ public function disallows_index_for_guest() { $response = $this->callWith(); - $this->assertEquals(403, $response->getStatusCode()); + $this->assertEquals(401, $response->getStatusCode()); } /** diff --git a/tests/integration/api/Controller/ListUsersControllerTest.php b/tests/integration/api/Controller/ListUsersControllerTest.php index 35eee7504a..b57e8c4d90 100644 --- a/tests/integration/api/Controller/ListUsersControllerTest.php +++ b/tests/integration/api/Controller/ListUsersControllerTest.php @@ -42,7 +42,7 @@ public function disallows_index_for_guest() { $response = $this->callWith(); - $this->assertEquals(403, $response->getStatusCode()); + $this->assertEquals(401, $response->getStatusCode()); } /** From 279c7df9b96a387f6f5617997b4c4bdb585b9a96 Mon Sep 17 00:00:00 2001 From: Franz Liedke Date: Tue, 20 Aug 2019 17:18:18 +0200 Subject: [PATCH 2/5] Document permission check methods --- src/User/AssertPermissionTrait.php | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/User/AssertPermissionTrait.php b/src/User/AssertPermissionTrait.php index 8941b1615b..2ba07ee878 100644 --- a/src/User/AssertPermissionTrait.php +++ b/src/User/AssertPermissionTrait.php @@ -17,6 +17,13 @@ trait AssertPermissionTrait { /** + * Ensure the current user is allowed to do something. + * + * If the condition is not met, an exception will be thrown that signals the + * lack of permissions. This is about *authorization*, i.e. retrying such a + * request / operation without a change in permissions (or using another + * user account) is pointless. + * * @param bool $condition * @throws PermissionDeniedException */ @@ -28,6 +35,12 @@ protected function assertPermission($condition) } /** + * Ensure the current user is authenticated. + * + * This will throw an exception for guest users, signaling that + * *authorization* failed. Thus, they could retry the operation after + * logging in (or using other means of authentication). + * * @param bool $condition * @throws NotAuthenticatedException */ From 0836d99e831ab3c08bbfb2fc1312367bf34ad6dd Mon Sep 17 00:00:00 2001 From: Franz Liedke Date: Tue, 20 Aug 2019 17:39:55 +0200 Subject: [PATCH 3/5] Remove unnecessary indirection --- src/User/AssertPermissionTrait.php | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/src/User/AssertPermissionTrait.php b/src/User/AssertPermissionTrait.php index 2ba07ee878..d3e1ad7099 100644 --- a/src/User/AssertPermissionTrait.php +++ b/src/User/AssertPermissionTrait.php @@ -35,18 +35,18 @@ protected function assertPermission($condition) } /** - * Ensure the current user is authenticated. + * Ensure the given actor is authenticated. * * This will throw an exception for guest users, signaling that * *authorization* failed. Thus, they could retry the operation after * logging in (or using other means of authentication). * - * @param bool $condition + * @param User $actor * @throws NotAuthenticatedException */ - protected function assertAuthentication($condition) + protected function assertRegistered(User $actor) { - if (! $condition) { + if ($actor->isGuest()) { throw new NotAuthenticatedException; } } @@ -62,15 +62,6 @@ protected function assertCan(User $actor, $ability, $arguments = []) $this->assertPermission($actor->can($ability, $arguments)); } - /** - * @param User $actor - * @throws NotAuthenticatedException - */ - protected function assertRegistered(User $actor) - { - $this->assertAuthentication(! $actor->isGuest()); - } - /** * @param User $actor * @throws PermissionDeniedException From b60617b849dddfb48d17e5de58a8bdaaac30343b Mon Sep 17 00:00:00 2001 From: Franz Liedke Date: Wed, 21 Aug 2019 23:46:00 +0200 Subject: [PATCH 4/5] Move authentication check into assertCan() method This will cause the right error (HTTP 401) to be thrown whenever we're checking for a specific permission, but the user is not even logged in. Authenticated users will still get HTTP 403. --- src/Api/Controller/ListUsersController.php | 1 - src/Group/Command/CreateGroupHandler.php | 1 - src/User/AssertPermissionTrait.php | 8 ++++++++ 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/Api/Controller/ListUsersController.php b/src/Api/Controller/ListUsersController.php index b10ed17a47..85d7bfa267 100644 --- a/src/Api/Controller/ListUsersController.php +++ b/src/Api/Controller/ListUsersController.php @@ -72,7 +72,6 @@ protected function data(ServerRequestInterface $request, Document $document) { $actor = $request->getAttribute('actor'); - $this->assertRegistered($actor); $this->assertCan($actor, 'viewUserList'); $query = Arr::get($this->extractFilter($request), 'q'); diff --git a/src/Group/Command/CreateGroupHandler.php b/src/Group/Command/CreateGroupHandler.php index 8a000679e2..0364432137 100644 --- a/src/Group/Command/CreateGroupHandler.php +++ b/src/Group/Command/CreateGroupHandler.php @@ -49,7 +49,6 @@ public function handle(CreateGroup $command) $actor = $command->actor; $data = $command->data; - $this->assertRegistered($actor); $this->assertCan($actor, 'createGroup'); $group = Group::build( diff --git a/src/User/AssertPermissionTrait.php b/src/User/AssertPermissionTrait.php index d3e1ad7099..dee93957cf 100644 --- a/src/User/AssertPermissionTrait.php +++ b/src/User/AssertPermissionTrait.php @@ -55,15 +55,23 @@ protected function assertRegistered(User $actor) * @param User $actor * @param string $ability * @param mixed $arguments + * @throws NotAuthenticatedException * @throws PermissionDeniedException */ protected function assertCan(User $actor, $ability, $arguments = []) { + // For non-authenticated users, we throw a different exception to signal + // that logging in may help. + $this->assertRegistered($actor); + + // If we're logged in, then we need to communicate that the current + // account simply does not have enough permissions. $this->assertPermission($actor->can($ability, $arguments)); } /** * @param User $actor + * @throws NotAuthenticatedException * @throws PermissionDeniedException */ protected function assertAdmin(User $actor) From 2b6535525b1e31c5f9c37aa3404036a380d48881 Mon Sep 17 00:00:00 2001 From: Franz Liedke Date: Wed, 21 Aug 2019 23:48:24 +0200 Subject: [PATCH 5/5] When signups are prohibited, respond with HTTP 403 --- src/User/Command/RegisterUserHandler.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/User/Command/RegisterUserHandler.php b/src/User/Command/RegisterUserHandler.php index e839e1c2bd..f2d1818f89 100644 --- a/src/User/Command/RegisterUserHandler.php +++ b/src/User/Command/RegisterUserHandler.php @@ -74,7 +74,7 @@ public function handle(RegisterUser $command) $data = $command->data; if (! $this->settings->get('allow_sign_up')) { - $this->assertAdmin($actor); + $this->assertPermission($actor->can('administrate')); } $password = Arr::get($data, 'attributes.password');