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..85d7bfa267 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,7 @@ protected function data(ServerRequestInterface $request, Document $document) { $actor = $request->getAttribute('actor'); - if ($actor->cannot('viewUserList')) { - throw new PermissionDeniedException; - } + $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/User/AssertPermissionTrait.php b/src/User/AssertPermissionTrait.php index d6310440a6..dee93957cf 100644 --- a/src/User/AssertPermissionTrait.php +++ b/src/User/AssertPermissionTrait.php @@ -11,12 +11,20 @@ namespace Flarum\User; +use Flarum\User\Exception\NotAuthenticatedException; use Flarum\User\Exception\PermissionDeniedException; trait AssertPermissionTrait { /** - * @param $condition + * 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 */ protected function assertPermission($condition) @@ -27,36 +35,43 @@ protected function assertPermission($condition) } /** + * 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 User $actor - * @param string $ability - * @param mixed $arguments - * @throws PermissionDeniedException + * @throws NotAuthenticatedException */ - protected function assertCan(User $actor, $ability, $arguments = []) - { - $this->assertPermission($actor->can($ability, $arguments)); - } - - /** - * @param User $actor - * @throws \Flarum\User\Exception\PermissionDeniedException - */ - protected function assertGuest(User $actor) + protected function assertRegistered(User $actor) { - $this->assertPermission($actor->isGuest()); + if ($actor->isGuest()) { + throw new NotAuthenticatedException; + } } /** * @param User $actor + * @param string $ability + * @param mixed $arguments + * @throws NotAuthenticatedException * @throws PermissionDeniedException */ - protected function assertRegistered(User $actor) + protected function assertCan(User $actor, $ability, $arguments = []) { - $this->assertPermission(! $actor->isGuest()); + // 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) 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'); 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()); } /**