From f7222d7e204f541e7b39c9c43665b7794bbb86b8 Mon Sep 17 00:00:00 2001 From: Franz Liedke Date: Tue, 20 Aug 2019 07:19:55 +0200 Subject: [PATCH] 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 18ba383f7..f48e39928 100644 --- a/src/Api/Controller/ListNotificationsController.php +++ b/src/Api/Controller/ListNotificationsController.php @@ -13,12 +13,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} */ @@ -65,9 +67,7 @@ class ListNotificationsController extends AbstractListController { $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 b5afb1a6e..da764920a 100644 --- a/src/Api/Controller/ListUsersController.php +++ b/src/Api/Controller/ListUsersController.php @@ -12,7 +12,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; @@ -20,6 +20,8 @@ use Tobscure\JsonApi\Document; class ListUsersController extends AbstractListController { + use AssertPermissionTrait; + /** * {@inheritdoc} */ @@ -68,9 +70,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/src/Foundation/ErrorServiceProvider.php b/src/Foundation/ErrorServiceProvider.php index 1d892976d..8dd5fc31b 100644 --- a/src/Foundation/ErrorServiceProvider.php +++ b/src/Foundation/ErrorServiceProvider.php @@ -31,6 +31,7 @@ class ErrorServiceProvider extends AbstractServiceProvider // 401 Unauthorized 'invalid_access_token' => 401, + 'not_authenticated' => 401, // 403 Forbidden 'invalid_confirmation_token' => 403, diff --git a/src/Group/Command/CreateGroupHandler.php b/src/Group/Command/CreateGroupHandler.php index d1e22fe35..0efd8fc8a 100644 --- a/src/Group/Command/CreateGroupHandler.php +++ b/src/Group/Command/CreateGroupHandler.php @@ -47,6 +47,7 @@ class CreateGroupHandler $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 18594aa5a..b58139ab4 100644 --- a/src/User/AssertPermissionTrait.php +++ b/src/User/AssertPermissionTrait.php @@ -9,12 +9,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) @@ -24,6 +25,17 @@ trait AssertPermissionTrait } } + /** + * @param bool $condition + * @throws NotAuthenticatedException + */ + protected function assertAuthentication($condition) + { + if (! $condition) { + throw new NotAuthenticatedException; + } + } + /** * @param User $actor * @param string $ability @@ -37,20 +49,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/src/User/Exception/NotAuthenticatedException.php b/src/User/Exception/NotAuthenticatedException.php new file mode 100644 index 000000000..bb45b8705 --- /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 6139adcd2..63f69eab5 100644 --- a/tests/integration/api/Auth/AuthenticateWithApiKeyTest.php +++ b/tests/integration/api/Auth/AuthenticateWithApiKeyTest.php @@ -63,7 +63,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/tests/integration/api/Controller/CreateGroupControllerTest.php b/tests/integration/api/Controller/CreateGroupControllerTest.php index 0a6b13d40..e3d72580a 100644 --- a/tests/integration/api/Controller/CreateGroupControllerTest.php +++ b/tests/integration/api/Controller/CreateGroupControllerTest.php @@ -78,7 +78,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/tests/integration/api/Controller/ListNotificationsControllerTest.php b/tests/integration/api/Controller/ListNotificationsControllerTest.php index cacd8dfb0..8c8bc7ccc 100644 --- a/tests/integration/api/Controller/ListNotificationsControllerTest.php +++ b/tests/integration/api/Controller/ListNotificationsControllerTest.php @@ -34,7 +34,7 @@ class ListNotificationsControllerTest extends ApiControllerTestCase { $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 89242fc8e..7747356bf 100644 --- a/tests/integration/api/Controller/ListUsersControllerTest.php +++ b/tests/integration/api/Controller/ListUsersControllerTest.php @@ -40,7 +40,7 @@ class ListUsersControllerTest extends ApiControllerTestCase { $response = $this->callWith(); - $this->assertEquals(403, $response->getStatusCode()); + $this->assertEquals(401, $response->getStatusCode()); } /**