Fixed auth logic

src/Auth/AuthModule.php

@@ -15,8 +15,8 @@
 use Aphiria\Net\Http\Headers\SameSiteMode;
 use Aphiria\Net\Http\HttpStatusCode;
 use App\Auth\Binders\AuthServiceBinder;
-use App\Auth\Policies\AuthorizedUserDeleterRequirement;
-use App\Auth\Policies\AuthorizedUserDeleterRequirementHandler;
+use App\Auth\Policies\OwnerOrAdminRequirement;
+use App\Auth\Policies\OwnerOrAdminRequirementHandler;
 use App\Auth\Schemes\BasicAuthenticationHandler;
 use App\Auth\Schemes\CookieAuthenticationHandler;
 
@@ -62,8 +62,8 @@ public function configure(IApplicationBuilder $appBuilder): void
             )
             ->withAuthorizationRequirementHandler(
                 $appBuilder,
-                AuthorizedUserDeleterRequirement::class,
-                new AuthorizedUserDeleterRequirementHandler()
+                OwnerOrAdminRequirement::class,
+                new OwnerOrAdminRequirementHandler()
             )
             ->withAuthorizationRequirementHandler(
                 $appBuilder,
@@ -73,15 +73,15 @@ public function configure(IApplicationBuilder $appBuilder): void
             ->withAuthorizationPolicy(
                 $appBuilder,
                 new AuthorizationPolicy(
-                    'authorized-user-role-giver',
+                    'authorized-user-role-granter',
                     new RolesRequirement('admin')
                 )
             )
             ->withAuthorizationPolicy(
                 $appBuilder,
                 new AuthorizationPolicy(
-                    'authorized-user-deleter',
-                    new AuthorizedUserDeleterRequirement('admin')
+                    'owner-or-admin',
+                    new OwnerOrAdminRequirement('admin')
                 )
             )
             ->withProblemDetails(

src/Auth/Policies/AuthorizedUserDeleterRequirement.php → src/Auth/Policies/OwnerOrAdminRequirement.php

@@ -5,15 +5,15 @@
 namespace App\Auth\Policies;
 
 /**
- * Defines the requirement for users to delete other users' accounts
+ * Defines the requirement for users to access other users' accounts
  */
-final readonly class AuthorizedUserDeleterRequirement
+final readonly class OwnerOrAdminRequirement
 {
-    /** @var list<string> The list of roles authorized to delete other users' accounts */
+    /** @var list<string> The list of roles authorized to access other users' accounts */
     public array $authorizedRoles;
 
     /**
-     * @param list<string>|string $authorizedRoles The role or list of roles that are authorized to delete other users' accounts
+     * @param list<string>|string $authorizedRoles The role or list of roles that are authorized to access other users' accounts
      */
     public function __construct(array|string $authorizedRoles)
     {

src/Auth/Policies/AuthorizedUserDeleterRequirementHandler.php → src/Auth/Policies/OwnerOrAdminRequirementHandler.php

@@ -12,37 +12,37 @@
 use InvalidArgumentException;
 
 /**
- * Defines the requirement handler that checks if a user is authorized to delete another user's account
+ * Defines the requirement handler that checks if a user is the owner or an admin
  *
- * @implements IAuthorizationRequirementHandler<AuthorizedUserDeleterRequirement, User>
+ * @implements IAuthorizationRequirementHandler<OwnerOrAdminRequirement, User>
  */
-final class AuthorizedUserDeleterRequirementHandler implements IAuthorizationRequirementHandler
+final class OwnerOrAdminRequirementHandler implements IAuthorizationRequirementHandler
 {
     /**
      * @inheritdoc
      */
     public function handle(IPrincipal $user, object $requirement, AuthorizationContext $authorizationContext): void
     {
-        if (!$requirement instanceof AuthorizedUserDeleterRequirement) {
-            throw new InvalidArgumentException('Requirement must be of type ' . AuthorizedUserDeleterRequirement::class);
+        if (!$requirement instanceof OwnerOrAdminRequirement) {
+            throw new InvalidArgumentException('Requirement must be of type ' . OwnerOrAdminRequirement::class);
         }
 
-        $userToDelete = $authorizationContext->resource;
+        $userBeingAccessed = $authorizationContext->resource;
 
-        if (!$userToDelete instanceof User) {
+        if (!$userBeingAccessed instanceof User) {
             throw new InvalidArgumentException('Resource must be of type ' . User::class);
         }
 
-        if ($userToDelete->id === (int)$user->getPrimaryIdentity()?->getNameIdentifier()) {
-            // The user being deleted is the current user
+        if ($userBeingAccessed->id === (int)$user->getPrimaryIdentity()?->getNameIdentifier()) {
+            // The user being accessed is the current user
             $authorizationContext->requirementPassed($requirement);
 
             return;
         }
 
         foreach ($requirement->authorizedRoles as $authorizedRole) {
             if ($user->hasClaim(ClaimType::Role, $authorizedRole)) {
-                // The user is authorized to delete the user's account
+                // The user is authorized to access the user's account
                 $authorizationContext->requirementPassed($requirement);
 
                 return;

src/Users/Api/Controllers/UserController.php

@@ -6,6 +6,7 @@
 
 use Aphiria\Api\Controllers\Controller;
 use Aphiria\Authentication\Attributes\Authenticate;
+use Aphiria\Authentication\IAuthenticator;
 use Aphiria\Authorization\Attributes\AuthorizeRoles;
 use Aphiria\Authorization\IAuthority;
 use Aphiria\Authorization\PolicyNotFoundException;
@@ -16,12 +17,12 @@
 use Aphiria\Routing\Attributes\Get;
 use Aphiria\Routing\Attributes\Post;
 use Aphiria\Routing\Attributes\RouteGroup;
-use Aphiria\Security\User as Principal;
 use App\Users\InvalidPageException;
 use App\Users\IUserService;
 use App\Users\NewUser;
 use App\Users\User;
 use App\Users\UserNotFoundException;
+use Exception;
 
 /**
  * Defines the user controller
@@ -31,10 +32,12 @@ final class UserController extends Controller
 {
     /**
      * @param IUserService $users The user service
+     * @param IAuthenticator $authenticator The authenticator
      * @param IAuthority $authority The authority
      */
     public function __construct(
         private readonly IUserService $users,
+        private readonly IAuthenticator $authenticator,
         private readonly IAuthority $authority
     ) {
     }
@@ -44,13 +47,21 @@ public function __construct(
      *
      * @param NewUser $user The user to create
      * @return User The created user
+     * @throws Exception Thrown if there was an error authenticating or authorizing this request
      */
     #[Post('')]
     public function createUser(NewUser $user): User
     {
-        $authResult = $this->authority->authorize($this->getUser() ?? new Principal([]), 'authorized-user-role-giver');
+        $canGrantRoles = false;
+        /** @psalm-suppress PossiblyNullArgument The user will be set */
+        $authenticationResult = $this->authenticator->authenticate($this->request, 'cookie');
+
+        if ($authenticationResult->passed) {
+            /** @psalm-suppress PossiblyNullArgument The user will be set */
+            $canGrantRoles = $this->authority->authorize($authenticationResult->user, 'authorized-user-role-granter')->passed;
+        }
 
-        return $this->users->createUser($user, $authResult->passed);
+        return $this->users->createUser($user, $canGrantRoles);
     }
 
     /**
@@ -67,11 +78,12 @@ public function deleteUser(int $id): IResponse
         try {
             $userToDelete = $this->users->getUserById($id);
         } catch (UserNotFoundException $ex) {
-            return $this->notFound();
+            // To hide prevent iterating over our users, we'll just return a 403
+            return $this->forbidden();
         }
 
         /** @psalm-suppress PossiblyNullArgument The user will be set */
-        if (!$this->authority->authorize($this->getUser(), 'authorized-user-deleter', $userToDelete)->passed) {
+        if (!$this->authority->authorize($this->getUser(), 'owner-or-admin', $userToDelete)->passed) {
             return $this->forbidden();
         }
 
@@ -99,12 +111,25 @@ public function getPagedUsers(int $pageNumber = 1, int $pageSize = 100): IRespon
      * Gets a user with the input ID
      *
      * @param int $id The ID of the user to get
-     * @return User The user with the input ID
+     * @return IResponse The response with the user
      * @throws UserNotFoundException Thrown if there was no user with the input ID
+     * @throws Exception Thrown if there was an error authenticating or authorizing the user
      */
     #[Get(':id'), Authenticate()]
-    public function getUserById(int $id): User
+    public function getUserById(int $id): IResponse
     {
-        return $this->users->getUserById($id);
+        try {
+            $userToGet = $this->users->getUserById($id);
+        } catch (UserNotFoundException) {
+            // To hide prevent iterating over our users, we'll just return a 403
+            return $this->forbidden();
+        }
+
+        /** @psalm-suppress PossiblyNullArgument The user will be set */
+        if (!$this->authority->authorize($this->getUser(), 'owner-or-admin', $userToGet)->passed) {
+            return $this->forbidden();
+        }
+
+        return $this->ok($this->users->getUserById($id));
     }
 }

tests/Integration/CreatesUser.php

@@ -8,6 +8,7 @@
 use Aphiria\ContentNegotiation\MediaTypeFormatters\SerializationException;
 use Aphiria\Net\Http\HttpException;
 use Aphiria\Net\Http\HttpStatusCode;
+use Aphiria\Security\PrincipalBuilder;
 use App\Users\NewUser;
 use App\Users\User;
 use Exception;
@@ -21,17 +22,23 @@ trait CreatesUser
     /**
      * Creates a user for use in integration tests
      *
+     * @param bool $createAsAdmin Whether we create this user as the admin
      * @param string $password The user's password
      * @param string|list<string> $roles The user's roles
      * @return User The created user
      * @throws FailedContentNegotiationException|SerializationException|HttpException|RuntimeException|Exception Thrown if there was an error creating the user
      */
-    private function createUser(string $password = 'password', string|array $roles = []): User
+    private function createUser(bool $createAsAdmin = false, string $password = 'password', string|array $roles = []): User
     {
         // Create a unique email address so we do not have collisions
         $roles = \is_string($roles) ? [$roles] : $roles;
         $newUser = new NewUser(\bin2hex(\random_bytes(8)) . '@example.com', $password, $roles);
-        $newUserResponse = $this->post('/users', body: $newUser);
+        $actingAs = $createAsAdmin
+            ? (new PrincipalBuilder('example.com'))->withRoles('admin')
+                ->build()
+            : (new PrincipalBuilder('example.com'))->build();
+
+        $newUserResponse = $this->actingAs($actingAs, fn () => $this->post('/users', body: $newUser));
 
         if ($newUserResponse->getStatusCode() !== HttpStatusCode::Ok) {
             $newUserResponseBody = $newUserResponse->getBody();

tests/Integration/Users/UserTest.php

@@ -6,6 +6,7 @@
 
 use Aphiria\Net\Http\HttpStatusCode;
 use Aphiria\Security\Identity;
+use Aphiria\Security\IPrincipal;
 use Aphiria\Security\PrincipalBuilder;
 use Aphiria\Security\User as Principal;
 use App\Tests\Integration\CreatesUser;
@@ -37,10 +38,11 @@ public static function provideInvalidPageSizes(): array
 
     public function testCreatingUserAsAdminRetainsRoles(): void
     {
-        $createdUser = $this->createUser(roles: ['foo']);
-        $nonAdminUser = (new PrincipalBuilder('example.com'))->withRoles('admin')
-            ->build();
-        $response = $this->actingAs($nonAdminUser, fn () => $this->get("/users/$createdUser->id"));
+        $createdUser = $this->createUser(createAsAdmin: true, roles: ['foo']);
+        $response = $this->actingAs(
+            self::createPrincipalFromUser($createdUser),
+            fn () => $this->get("/users/$createdUser->id")
+        );
         $this->assertStatusCodeEquals(HttpStatusCode::Ok, $response);
         $this->assertParsedBodyEquals($createdUser, $response);
     }
@@ -49,22 +51,14 @@ public function testCreatingUserAsNonAdminRemovesRoles(): void
     {
         $createdUser = $this->createUser(roles: ['foo']);
         $createdUserWithoutRoles = new User($createdUser->id, $createdUser->email, []);
-        $nonAdminUser = (new PrincipalBuilder('example.com'))->build();
-        $response = $this->actingAs($nonAdminUser, fn () => $this->get("/users/$createdUser->id"));
+        $response = $this->actingAs(
+            self::createPrincipalFromUser($createdUser),
+            fn () => $this->get("/users/$createdUser->id")
+        );
         $this->assertStatusCodeEquals(HttpStatusCode::Ok, $response);
         $this->assertParsedBodyEquals($createdUserWithoutRoles, $response);
     }
 
-    public function testCreatingUsersMakesThemRetrievableAsAdminUser(): void
-    {
-        $createdUser = $this->createUser();
-        $adminUser = (new PrincipalBuilder('example.com'))->withRoles('admin')
-            ->build();
-        $response = $this->actingAs($adminUser, fn () => $this->get("/users/$createdUser->id"));
-        $this->assertStatusCodeEquals(HttpStatusCode::Ok, $response);
-        $this->assertParsedBodyEquals($createdUser, $response);
-    }
-
     public function testDeletingAnotherUserAsAdminReturns204(): void
     {
         $createdUserId = $this->createUser()->id;
@@ -77,34 +71,35 @@ public function testDeletingAnotherUserAsAdminReturns204(): void
     public function testDeletingAnotherUserAsNonAdminReturns403(): void
     {
         $createdUserId = $this->createUser()->id;
-        $nonAdminUser = (new PrincipalBuilder('example.com'))->withNameIdentifier(1)
+        $nonAdminUser = (new PrincipalBuilder('example.com'))->withNameIdentifier(10000)
             ->build();
         $response = $this->actingAs($nonAdminUser, fn () => $this->delete("/users/$createdUserId"));
         $this->assertStatusCodeEquals(HttpStatusCode::Forbidden, $response);
     }
 
-    public function testDeletingNonExistentUserReturns404(): void
+    public function testDeletingNonExistentUserReturns403(): void
     {
         $adminUser = (new PrincipalBuilder('example.com'))->withRoles('admin')
             ->build();
         $response = $this->actingAs($adminUser, fn () => $this->get('/users/0'));
-        $this->assertStatusCodeEquals(HttpStatusCode::NotFound, $response);
+        $this->assertStatusCodeEquals(HttpStatusCode::Forbidden, $response);
     }
 
     public function testDeletingYourOwnUserReturns204(): void
     {
-        $createdUserId = $this->createUser()->id;
-        $createdUser = (new PrincipalBuilder('example.com'))->withNameIdentifier($createdUserId)
-            ->build();
-        $response = $this->actingAs($createdUser, fn () => $this->delete("/users/$createdUserId"));
+        $createdUser = $this->createUser();
+        $response = $this->actingAs(
+            self::createPrincipalFromUser($createdUser),
+            fn () => $this->delete("/users/$createdUser->id")
+        );
         $this->assertStatusCodeEquals(HttpStatusCode::NoContent, $response);
     }
 
-    public function testGettingInvalidUserReturns404(): void
+    public function testGettingInvalidUserReturns403(): void
     {
         $user = new Principal(new Identity());
         $response = $this->actingAs($user, fn () => $this->get('/users/0'));
-        $this->assertStatusCodeEquals(HttpStatusCode::NotFound, $response);
+        $this->assertStatusCodeEquals(HttpStatusCode::Forbidden, $response);
     }
 
     public function testGettingPagedUsersRedirectsToForbiddenPageForNonAdmins(): void
@@ -145,4 +140,51 @@ public function testGettingPagedUsersWithInvalidPageSizesReturnsBadRequests(int
         );
         $this->assertStatusCodeEquals(HttpStatusCode::BadRequest, $response);
     }
+
+    public function testGettingUserDoesNotWorkForNonOwnerNonAdmin(): void
+    {
+        $createdUser = $this->createUser();
+        $nonAdminNonOwnerUser = (new PrincipalBuilder('example.com'))->build();
+        $response = $this->actingAs(
+            $nonAdminNonOwnerUser,
+            fn () => $this->get("/users/$createdUser->id")
+        );
+        $this->assertStatusCodeEquals(HttpStatusCode::Forbidden, $response);
+    }
+
+    public function testGettingUserWorksForAdmin(): void
+    {
+        $createdUser = $this->createUser();
+        $admin = (new PrincipalBuilder('example.com'))->withRoles('admin')
+            ->build();
+        $response = $this->actingAs(
+            $admin,
+            fn () => $this->get("/users/$createdUser->id")
+        );
+        $this->assertStatusCodeEquals(HttpStatusCode::Ok, $response);
+    }
+
+    public function testGettingUserWorksForOwner(): void
+    {
+        $createdUser = $this->createUser();
+        $response = $this->actingAs(
+            self::createPrincipalFromUser($createdUser),
+            fn () => $this->get("/users/$createdUser->id")
+        );
+        $this->assertStatusCodeEquals(HttpStatusCode::Ok, $response);
+    }
+
+    /**
+     * Creates a principal from a user
+     *
+     * @param User $user The user to create a principal from
+     * @return IPrincipal The created principal
+     */
+    private static function createPrincipalFromUser(User $user): IPrincipal
+    {
+        return (new PrincipalBuilder('example.com'))->withNameIdentifier($user->id)
+            ->withEmail($user->email)
+            ->withRoles($user->roles)
+            ->build();
+    }
 }