From 7d858cdba67afefa0f2e01637f78da6376529ff5 Mon Sep 17 00:00:00 2001 From: davidbyoung Date: Thu, 10 Aug 2023 11:54:06 -0500 Subject: [PATCH] Fixed auth logic --- src/Auth/AuthModule.php | 14 +-- ...rement.php => OwnerOrAdminRequirement.php} | 8 +- ...php => OwnerOrAdminRequirementHandler.php} | 20 ++-- src/Users/Api/Controllers/UserController.php | 41 +++++++-- tests/Integration/CreatesUser.php | 11 ++- tests/Integration/Users/UserTest.php | 92 ++++++++++++++----- 6 files changed, 130 insertions(+), 56 deletions(-) rename src/Auth/Policies/{AuthorizedUserDeleterRequirement.php => OwnerOrAdminRequirement.php} (64%) rename src/Auth/Policies/{AuthorizedUserDeleterRequirementHandler.php => OwnerOrAdminRequirementHandler.php} (58%) diff --git a/src/Auth/AuthModule.php b/src/Auth/AuthModule.php index 4668010..612303c 100644 --- a/src/Auth/AuthModule.php +++ b/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( diff --git a/src/Auth/Policies/AuthorizedUserDeleterRequirement.php b/src/Auth/Policies/OwnerOrAdminRequirement.php similarity index 64% rename from src/Auth/Policies/AuthorizedUserDeleterRequirement.php rename to src/Auth/Policies/OwnerOrAdminRequirement.php index 734d93e..26261f4 100644 --- a/src/Auth/Policies/AuthorizedUserDeleterRequirement.php +++ b/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 The list of roles authorized to delete other users' accounts */ + /** @var list The list of roles authorized to access other users' accounts */ public array $authorizedRoles; /** - * @param list|string $authorizedRoles The role or list of roles that are authorized to delete other users' accounts + * @param list|string $authorizedRoles The role or list of roles that are authorized to access other users' accounts */ public function __construct(array|string $authorizedRoles) { diff --git a/src/Auth/Policies/AuthorizedUserDeleterRequirementHandler.php b/src/Auth/Policies/OwnerOrAdminRequirementHandler.php similarity index 58% rename from src/Auth/Policies/AuthorizedUserDeleterRequirementHandler.php rename to src/Auth/Policies/OwnerOrAdminRequirementHandler.php index e19ae5f..3ffb53b 100644 --- a/src/Auth/Policies/AuthorizedUserDeleterRequirementHandler.php +++ b/src/Auth/Policies/OwnerOrAdminRequirementHandler.php @@ -12,29 +12,29 @@ 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 + * @implements IAuthorizationRequirementHandler */ -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; @@ -42,7 +42,7 @@ public function handle(IPrincipal $user, object $requirement, AuthorizationConte 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; diff --git a/src/Users/Api/Controllers/UserController.php b/src/Users/Api/Controllers/UserController.php index 7c9a485..056e90a 100644 --- a/src/Users/Api/Controllers/UserController.php +++ b/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)); } } diff --git a/tests/Integration/CreatesUser.php b/tests/Integration/CreatesUser.php index d029137..5366df4 100644 --- a/tests/Integration/CreatesUser.php +++ b/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 $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(); diff --git a/tests/Integration/Users/UserTest.php b/tests/Integration/Users/UserTest.php index 7cfad2d..3c01f63 100644 --- a/tests/Integration/Users/UserTest.php +++ b/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(); + } }