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..85d7bfa26 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,7 @@ class ListUsersController extends AbstractListController { $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/framework/core/src/Foundation/ErrorServiceProvider.php b/framework/core/src/Foundation/ErrorServiceProvider.php index 1d892976d..8dd5fc31b 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 'invalid_confirmation_token' => 403, diff --git a/framework/core/src/User/AssertPermissionTrait.php b/framework/core/src/User/AssertPermissionTrait.php index d6310440a..dee93957c 100644 --- a/framework/core/src/User/AssertPermissionTrait.php +++ b/framework/core/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) @@ -26,37 +34,44 @@ trait AssertPermissionTrait } } + /** + * 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 + * @throws NotAuthenticatedException + */ + protected function assertRegistered(User $actor) + { + if ($actor->isGuest()) { + throw new NotAuthenticatedException; + } + } + /** * @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 \Flarum\User\Exception\PermissionDeniedException - */ - protected function assertGuest(User $actor) - { - $this->assertPermission($actor->isGuest()); - } - - /** - * @param User $actor - * @throws PermissionDeniedException - */ - protected function assertRegistered(User $actor) - { - $this->assertPermission(! $actor->isGuest()); - } - - /** - * @param User $actor + * @throws NotAuthenticatedException * @throws PermissionDeniedException */ protected function assertAdmin(User $actor) 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'); 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()); } /**