Merge pull request #1854 from flarum/fl/1641-fix-status-codes

Error handling: Fix status codes

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();
 

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);

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,

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)

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');

framework/core/src/User/Exception/NotAuthenticatedException.php

@@ -0,0 +1,23 @@
+<?php
+
+/*
+ * This file is part of Flarum.
+ *
+ * (c) Toby Zerner <toby.zerner@gmail.com>
+ *
+ * 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';
+    }
+}

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());
     }
 
     /**

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);
 

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());
     }
 
     /**

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());
     }
 
     /**