fix: corrected 401/403 behaviour

If a user's session has expired, these methods will throw a ForbiddenException
(403 Forbidden) instead of an UnauthorizedHttpException (401 Unauthorized),
which is semantically incorrect.

Author: thorsten

phpmyfaq/src/phpMyFAQ/Controller/AbstractController.php

@@ -183,10 +183,14 @@ protected function userIsSuperAdmin(): void
     }
 
     /**
-     * @throws ForbiddenException
+     * @throws UnauthorizedHttpException|ForbiddenException
      */
     protected function userHasGroupPermission(): void
     {
+        if (!$this->currentUser->isLoggedIn()) {
+            throw new UnauthorizedHttpException(challenge: 'User is not authenticated.');
+        }
+
         $currentUser = $this->currentUser;
         if (
             !$currentUser->perm->hasPermission($currentUser->getUserId(), PermissionType::USER_ADD->value)
@@ -199,10 +203,14 @@ protected function userHasGroupPermission(): void
     }
 
     /**
-     * @throws ForbiddenException
+     * @throws UnauthorizedHttpException|ForbiddenException
      */
     protected function userHasUserPermission(): void
     {
+        if (!$this->currentUser->isLoggedIn()) {
+            throw new UnauthorizedHttpException(challenge: 'User is not authenticated.');
+        }
+
         $currentUser = $this->currentUser;
         if (
             !$currentUser->perm->hasPermission($currentUser->getUserId(), PermissionType::USER_ADD->value)
@@ -214,10 +222,14 @@ protected function userHasUserPermission(): void
     }
 
     /**
-     * @throws ForbiddenException
+     * @throws UnauthorizedHttpException|ForbiddenException
      */
     protected function userHasPermission(PermissionType $permissionType): void
     {
+        if (!$this->currentUser->isLoggedIn()) {
+            throw new UnauthorizedHttpException(challenge: 'User is not authenticated.');
+        }
+
         $currentUser = $this->currentUser;
         if (!$currentUser->perm->hasPermission($currentUser->getUserId(), $permissionType->value)) {
             throw new ForbiddenException(message: sprintf('User has no "%s" permission.', $permissionType->name));

tests/phpMyFAQ/Controller/AbstractControllerTest.php

@@ -246,6 +246,12 @@ public function testUserIsSuperAdminSucceedsWhenSuperAdmin(): void
 
     public function testUserHasGroupPermissionThrowsExceptionWhenMissingPermissions(): void
     {
+        // Mock that user is logged in (so we get to the permission check)
+        $this->currentUserMock
+            ->expects($this->once())
+            ->method('isLoggedIn')
+            ->willReturn(true);
+
         $this->currentUserMock
             ->expects($this->once()) // Only called once since the exception is thrown on the first check
             ->method('getUserId')
@@ -265,6 +271,12 @@ public function testUserHasGroupPermissionThrowsExceptionWhenMissingPermissions(
 
     public function testUserHasGroupPermissionSucceedsWhenAllPermissionsPresent(): void
     {
+        // Mock that user is logged in
+        $this->currentUserMock
+            ->expects($this->once())
+            ->method('isLoggedIn')
+            ->willReturn(true);
+
         $this->currentUserMock
             ->expects($this->exactly(4))
             ->method('getUserId')
@@ -291,6 +303,12 @@ public function testUserHasGroupPermissionSucceedsWhenAllPermissionsPresent(): v
 
     public function testUserHasUserPermissionThrowsExceptionWhenMissingPermissions(): void
     {
+        // Mock that user is logged in (so we get to the permission check)
+        $this->currentUserMock
+            ->expects($this->once())
+            ->method('isLoggedIn')
+            ->willReturn(true);
+
         $this->currentUserMock
             ->expects($this->once()) // Only called once since exception is thrown on first check
             ->method('getUserId')
@@ -310,6 +328,12 @@ public function testUserHasUserPermissionThrowsExceptionWhenMissingPermissions()
 
     public function testUserHasUserPermissionSucceedsWhenAllPermissionsPresent(): void
     {
+        // Mock that user is logged in
+        $this->currentUserMock
+            ->expects($this->once())
+            ->method('isLoggedIn')
+            ->willReturn(true);
+
         $this->currentUserMock
             ->expects($this->exactly(3))
             ->method('getUserId')
@@ -335,6 +359,12 @@ public function testUserHasUserPermissionSucceedsWhenAllPermissionsPresent(): vo
 
     public function testUserHasPermissionThrowsExceptionWhenMissingPermission(): void
     {
+        // Mock that user is logged in (so we get to the permission check)
+        $this->currentUserMock
+            ->expects($this->once())
+            ->method('isLoggedIn')
+            ->willReturn(true);
+
         $permissionType = PermissionType::FAQ_ADD;
         $this->currentUserMock
             ->expects($this->once())
@@ -354,6 +384,12 @@ public function testUserHasPermissionThrowsExceptionWhenMissingPermission(): voi
 
     public function testUserHasPermissionSucceedsWhenPermissionPresent(): void
     {
+        // Mock that user is logged in
+        $this->currentUserMock
+            ->expects($this->once())
+            ->method('isLoggedIn')
+            ->willReturn(true);
+
         $permissionType = PermissionType::FAQ_ADD;
         $this->currentUserMock
             ->expects($this->once())
@@ -512,6 +548,12 @@ public static function permissionDataProvider(): array
     #[DataProvider('permissionDataProvider')]
     public function testUserHasPermissionWithDifferentPermissionTypes(PermissionType $permissionType): void
     {
+        // Mock that user is logged in
+        $this->currentUserMock
+            ->expects($this->once())
+            ->method('isLoggedIn')
+            ->willReturn(true);
+
         $this->currentUserMock
             ->expects($this->once())
             ->method('getUserId')