From 7d52a49cfbc7c78f15acc19c5258749e97c6cffb 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 +++--- .../Api/Controller/ListUsersController.php | 9 ++++--- .../src/Foundation/ErrorServiceProvider.php | 1 + .../src/Group/Command/CreateGroupHandler.php | 1 + .../core/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 framework/core/src/User/Exception/NotAuthenticatedException.php diff --git a/framework/core/src/Api/Controller/ListNotificationsController.php b/framework/core/src/Api/Controller/ListNotificationsController.php index b8aaeef53..352e7130a 100644 --- a/framework/core/src/Api/Controller/ListNotificationsController.php +++ b/framework/core/src/Api/Controller/ListNotificationsController.php @@ -15,12 +15,14 @@ use Flarum\Api\Serializer\NotificationSerializer; 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 @@ class ListNotificationsController extends AbstractListController { $actor = $request->getAttribute('actor'); - if ($actor->isGuest()) { - throw new PermissionDeniedException; - } + $this->assertRegistered($actor); $actor->markNotificationsAsRead()->save(); diff --git a/framework/core/src/Api/Controller/ListUsersController.php b/framework/core/src/Api/Controller/ListUsersController.php index 18d36ceab..b10ed17a4 100644 --- a/framework/core/src/Api/Controller/ListUsersController.php +++ b/framework/core/src/Api/Controller/ListUsersController.php @@ -14,7 +14,7 @@ namespace Flarum\Api\Controller; 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 @@ use Tobscure\JsonApi\Document; class ListUsersController extends AbstractListController { + use AssertPermissionTrait; + /** * {@inheritdoc} */ @@ -70,9 +72,8 @@ class ListUsersController extends AbstractListController { $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/framework/core/src/Foundation/ErrorServiceProvider.php b/framework/core/src/Foundation/ErrorServiceProvider.php index aa7060bb5..2e7278039 100644 --- a/framework/core/src/Foundation/ErrorServiceProvider.php +++ b/framework/core/src/Foundation/ErrorServiceProvider.php @@ -31,6 +31,7 @@ class ErrorServiceProvider extends AbstractServiceProvider // 401 Unauthorized 'invalid_access_token' => 401, + 'not_authenticated' => 401, // 403 Forbidden 'forbidden' => 403, diff --git a/framework/core/src/Group/Command/CreateGroupHandler.php b/framework/core/src/Group/Command/CreateGroupHandler.php index 036443213..8a000679e 100644 --- a/framework/core/src/Group/Command/CreateGroupHandler.php +++ b/framework/core/src/Group/Command/CreateGroupHandler.php @@ -49,6 +49,7 @@ class CreateGroupHandler $actor = $command->actor; $data = $command->data; + $this->assertRegistered($actor); $this->assertCan($actor, 'createGroup'); $group = Group::build( diff --git a/framework/core/src/User/AssertPermissionTrait.php b/framework/core/src/User/AssertPermissionTrait.php index d6310440a..8941b1615 100644 --- a/framework/core/src/User/AssertPermissionTrait.php +++ b/framework/core/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) @@ -26,6 +27,17 @@ trait AssertPermissionTrait } } + /** + * @param bool $condition + * @throws NotAuthenticatedException + */ + protected function assertAuthentication($condition) + { + if (! $condition) { + throw new NotAuthenticatedException; + } + } + /** * @param User $actor * @param string $ability @@ -39,20 +51,11 @@ trait AssertPermissionTrait /** * @param User $actor - * @throws \Flarum\User\Exception\PermissionDeniedException - */ - protected function assertGuest(User $actor) - { - $this->assertPermission($actor->isGuest()); - } - - /** - * @param User $actor - * @throws PermissionDeniedException + * @throws NotAuthenticatedException */ protected function assertRegistered(User $actor) { - $this->assertPermission(! $actor->isGuest()); + $this->assertAuthentication(! $actor->isGuest()); } /** diff --git a/framework/core/src/User/Exception/NotAuthenticatedException.php b/framework/core/src/User/Exception/NotAuthenticatedException.php new file mode 100644 index 000000000..bb45b8705 --- /dev/null +++ b/framework/core/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/framework/core/tests/integration/api/Auth/AuthenticateWithApiKeyTest.php b/framework/core/tests/integration/api/Auth/AuthenticateWithApiKeyTest.php index 1ba3ce5a1..e895dafdf 100644 --- a/framework/core/tests/integration/api/Auth/AuthenticateWithApiKeyTest.php +++ b/framework/core/tests/integration/api/Auth/AuthenticateWithApiKeyTest.php @@ -65,7 +65,7 @@ class AuthenticateWithApiKeyTest extends TestCase $response = $api->send(CreateGroupController::class, new Guest); - $this->assertEquals(403, $response->getStatusCode()); + $this->assertEquals(401, $response->getStatusCode()); } /** diff --git a/framework/core/tests/integration/api/Controller/CreateGroupControllerTest.php b/framework/core/tests/integration/api/Controller/CreateGroupControllerTest.php index d2edcada1..08275615d 100644 --- a/framework/core/tests/integration/api/Controller/CreateGroupControllerTest.php +++ b/framework/core/tests/integration/api/Controller/CreateGroupControllerTest.php @@ -80,7 +80,7 @@ class CreateGroupControllerTest extends ApiControllerTestCase /** * @test */ - public function unauthorized_user_cannot_create_group() + public function normal_user_cannot_create_group() { $this->actor = User::find(2); diff --git a/framework/core/tests/integration/api/Controller/ListNotificationsControllerTest.php b/framework/core/tests/integration/api/Controller/ListNotificationsControllerTest.php index 9f301607b..0e1135825 100644 --- a/framework/core/tests/integration/api/Controller/ListNotificationsControllerTest.php +++ b/framework/core/tests/integration/api/Controller/ListNotificationsControllerTest.php @@ -36,7 +36,7 @@ class ListNotificationsControllerTest extends ApiControllerTestCase { $response = $this->callWith(); - $this->assertEquals(403, $response->getStatusCode()); + $this->assertEquals(401, $response->getStatusCode()); } /** diff --git a/framework/core/tests/integration/api/Controller/ListUsersControllerTest.php b/framework/core/tests/integration/api/Controller/ListUsersControllerTest.php index 35eee7504..b57e8c4d9 100644 --- a/framework/core/tests/integration/api/Controller/ListUsersControllerTest.php +++ b/framework/core/tests/integration/api/Controller/ListUsersControllerTest.php @@ -42,7 +42,7 @@ class ListUsersControllerTest extends ApiControllerTestCase { $response = $this->callWith(); - $this->assertEquals(403, $response->getStatusCode()); + $this->assertEquals(401, $response->getStatusCode()); } /** From ee4a536de11e50a2d1e82057239a90f5f8632e19 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 --- framework/core/src/User/AssertPermissionTrait.php | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/framework/core/src/User/AssertPermissionTrait.php b/framework/core/src/User/AssertPermissionTrait.php index 8941b1615..2ba07ee87 100644 --- a/framework/core/src/User/AssertPermissionTrait.php +++ b/framework/core/src/User/AssertPermissionTrait.php @@ -17,6 +17,13 @@ use Flarum\User\Exception\PermissionDeniedException; 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 @@ trait AssertPermissionTrait } /** + * 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 152b455acf891c1ca6eb1310f21636fc26d16a8b 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 --- .../core/src/User/AssertPermissionTrait.php | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/framework/core/src/User/AssertPermissionTrait.php b/framework/core/src/User/AssertPermissionTrait.php index 2ba07ee87..d3e1ad709 100644 --- a/framework/core/src/User/AssertPermissionTrait.php +++ b/framework/core/src/User/AssertPermissionTrait.php @@ -35,18 +35,18 @@ trait AssertPermissionTrait } /** - * 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 @@ trait AssertPermissionTrait $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 67aa8d5cefeb88a1571e2432ac7fdcd3d6a15fd8 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. --- framework/core/src/Api/Controller/ListUsersController.php | 1 - framework/core/src/Group/Command/CreateGroupHandler.php | 1 - framework/core/src/User/AssertPermissionTrait.php | 8 ++++++++ 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/framework/core/src/Api/Controller/ListUsersController.php b/framework/core/src/Api/Controller/ListUsersController.php index b10ed17a4..85d7bfa26 100644 --- a/framework/core/src/Api/Controller/ListUsersController.php +++ b/framework/core/src/Api/Controller/ListUsersController.php @@ -72,7 +72,6 @@ class ListUsersController extends AbstractListController { $actor = $request->getAttribute('actor'); - $this->assertRegistered($actor); $this->assertCan($actor, 'viewUserList'); $query = Arr::get($this->extractFilter($request), 'q'); diff --git a/framework/core/src/Group/Command/CreateGroupHandler.php b/framework/core/src/Group/Command/CreateGroupHandler.php index 8a000679e..036443213 100644 --- a/framework/core/src/Group/Command/CreateGroupHandler.php +++ b/framework/core/src/Group/Command/CreateGroupHandler.php @@ -49,7 +49,6 @@ class CreateGroupHandler $actor = $command->actor; $data = $command->data; - $this->assertRegistered($actor); $this->assertCan($actor, 'createGroup'); $group = Group::build( diff --git a/framework/core/src/User/AssertPermissionTrait.php b/framework/core/src/User/AssertPermissionTrait.php index d3e1ad709..dee93957c 100644 --- a/framework/core/src/User/AssertPermissionTrait.php +++ b/framework/core/src/User/AssertPermissionTrait.php @@ -55,15 +55,23 @@ trait AssertPermissionTrait * @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 fbc940412c7b36eb2555be563355bdfe0edbb3ba 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 --- framework/core/src/User/Command/RegisterUserHandler.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/framework/core/src/User/Command/RegisterUserHandler.php b/framework/core/src/User/Command/RegisterUserHandler.php index e839e1c2b..f2d1818f8 100644 --- a/framework/core/src/User/Command/RegisterUserHandler.php +++ b/framework/core/src/User/Command/RegisterUserHandler.php @@ -74,7 +74,7 @@ class RegisterUserHandler $data = $command->data; if (! $this->settings->get('allow_sign_up')) { - $this->assertAdmin($actor); + $this->assertPermission($actor->can('administrate')); } $password = Arr::get($data, 'attributes.password');