From b3529e377d953f12b8717bdbfb44cfb9b76e77f4 Mon Sep 17 00:00:00 2001 From: Vincent Chalamon <407859+vincentchalamon@users.noreply.github.com> Date: Mon, 5 Feb 2024 16:34:47 +0100 Subject: [PATCH 1/8] feat: use OIDC server with fine-grained authorization --- api/.env | 2 + api/config/packages/framework.yaml | 11 ++ api/config/packages/security.yaml | 2 - api/src/DataFixtures/Factory/UserFactory.php | 12 +- api/src/DataFixtures/Story/DefaultStory.php | 8 +- api/src/Entity/Book.php | 2 +- api/src/Entity/Bookmark.php | 4 +- api/src/Entity/Review.php | 9 +- api/src/Entity/User.php | 6 +- .../Security/Voter/OIDCPermissionVoter.php | 71 +++++++++ api/src/Security/Voter/OIDCRoleVoter.php | 65 ++++++++ api/src/Security/Voter/OIDCVoterTrait.php | 32 ++++ api/tests/Api/Admin/BookTest.php | 2 +- api/tests/Api/BookmarkTest.php | 6 + ...otocolOpenIdConnectTokenIntrospectMock.php | 73 +++++++++ ...KeycloakProtocolOpenIdConnectTokenMock.php | 70 +++++++++ api/tests/Api/Mock/NotImplementedMock.php | 23 +++ api/tests/Api/ReviewTest.php | 7 + .../keycloak/config/realm-demo.json | 144 +++++++++++++++++- 19 files changed, 526 insertions(+), 23 deletions(-) create mode 100644 api/src/Security/Voter/OIDCPermissionVoter.php create mode 100644 api/src/Security/Voter/OIDCRoleVoter.php create mode 100644 api/src/Security/Voter/OIDCVoterTrait.php create mode 100644 api/tests/Api/Mock/KeycloakProtocolOpenIdConnectTokenIntrospectMock.php create mode 100644 api/tests/Api/Mock/KeycloakProtocolOpenIdConnectTokenMock.php create mode 100644 api/tests/Api/Mock/NotImplementedMock.php diff --git a/api/.env b/api/.env index 4091bba46..67a858f1d 100644 --- a/api/.env +++ b/api/.env @@ -20,6 +20,8 @@ TRUSTED_HOSTS=^(localhost|php)$ OIDC_SERVER_URL=https://localhost/oidc/realms/demo OIDC_SERVER_URL_INTERNAL=http://keycloak:8080/oidc/realms/demo OIDC_SWAGGER_CLIENT_ID=api-platform-swagger +OIDC_API_CLIENT_ID=api-platform-api +OIDC_API_CLIENT_SECRET=sEocbxCy7iFS8NzYzWyQ71QgxTDZ9fnU ###> symfony/framework-bundle ### APP_ENV=dev diff --git a/api/config/packages/framework.yaml b/api/config/packages/framework.yaml index f7ddf88a7..74579b6ac 100644 --- a/api/config/packages/framework.yaml +++ b/api/config/packages/framework.yaml @@ -24,8 +24,19 @@ framework: php_errors: log: true + http_client: + scoped_clients: + security.authorization.client: + base_uri: '%env(OIDC_SERVER_URL_INTERNAL)%/' + when@test: framework: test: true #session: # storage_factory_id: session.storage.factory.mock_file + + services: + App\Tests\Api\Mock\: + resource: '../../tests/Api/Mock/' + autowire: true + autoconfigure: true diff --git a/api/config/packages/security.yaml b/api/config/packages/security.yaml index 55fb5f7b3..b48296ec5 100644 --- a/api/config/packages/security.yaml +++ b/api/config/packages/security.yaml @@ -7,8 +7,6 @@ security: providers: app_user_provider: id: 'App\Security\Core\UserProvider' - role_hierarchy: - ROLE_ADMIN: ROLE_USER firewalls: dev: pattern: ^/(_(profiler|wdt)|css|images|js)/ diff --git a/api/src/DataFixtures/Factory/UserFactory.php b/api/src/DataFixtures/Factory/UserFactory.php index 479bdb995..b87c824e3 100644 --- a/api/src/DataFixtures/Factory/UserFactory.php +++ b/api/src/DataFixtures/Factory/UserFactory.php @@ -58,7 +58,16 @@ public function __construct() public static function createOneAdmin(array $attributes = []): Proxy|User { - return self::createOne(['roles' => ['ROLE_ADMIN']] + $attributes); + return self::new($attributes)->withAdmin()->create(); + } + + public function withAdmin(): self + { + return $this->addState([ + 'email' => 'chuck.norris@example.com', + 'firstName' => 'Chuck', + 'lastName' => 'Norris', + ]); } /** @@ -71,7 +80,6 @@ protected function getDefaults(): array 'email' => self::faker()->unique()->email(), 'firstName' => self::faker()->firstName(), 'lastName' => self::faker()->lastName(), - 'roles' => ['ROLE_USER'], ]; } diff --git a/api/src/DataFixtures/Story/DefaultStory.php b/api/src/DataFixtures/Story/DefaultStory.php index 5c6227ce5..c87bee9f7 100644 --- a/api/src/DataFixtures/Story/DefaultStory.php +++ b/api/src/DataFixtures/Story/DefaultStory.php @@ -58,7 +58,6 @@ public function build(): void 'email' => 'john.doe@example.com', 'firstName' => 'John', 'lastName' => 'Doe', - 'roles' => ['ROLE_USER'], ]); // Default user has a review on the default book @@ -87,11 +86,6 @@ public function build(): void } // Create admin user - UserFactory::createOne([ - 'email' => 'chuck.norris@example.com', - 'firstName' => 'Chuck', - 'lastName' => 'Norris', - 'roles' => ['ROLE_ADMIN'], - ]); + UserFactory::createOneAdmin(); } } diff --git a/api/src/Entity/Book.php b/api/src/Entity/Book.php index 74309f116..7eb9b6d66 100644 --- a/api/src/Entity/Book.php +++ b/api/src/Entity/Book.php @@ -71,7 +71,7 @@ AbstractNormalizer::GROUPS => ['Book:write'], ], collectDenormalizationErrors: true, - security: 'is_granted("ROLE_ADMIN")' + security: 'is_granted("ADMIN")' )] #[ApiResource( types: ['https://schema.org/Book', 'https://schema.org/Offer'], diff --git a/api/src/Entity/Bookmark.php b/api/src/Entity/Bookmark.php index 4002398fe..fde869e92 100644 --- a/api/src/Entity/Bookmark.php +++ b/api/src/Entity/Bookmark.php @@ -36,7 +36,7 @@ operations: [ new GetCollection(), new Delete( - security: 'is_granted("ROLE_USER") and object.user === user' + security: 'object.user == user' ), new Post( processor: BookmarkPersistProcessor::class @@ -54,7 +54,7 @@ ], collectDenormalizationErrors: true, mercure: true, - security: 'is_granted("ROLE_USER")' + security: 'is_granted("USER")' )] #[ORM\Entity(repositoryClass: BookmarkRepository::class)] #[ORM\UniqueConstraint(fields: ['user', 'book'])] diff --git a/api/src/Entity/Review.php b/api/src/Entity/Review.php index 74cdf6546..f1b0b89c6 100644 --- a/api/src/Entity/Review.php +++ b/api/src/Entity/Review.php @@ -14,6 +14,7 @@ use ApiPlatform\Metadata\Patch; use ApiPlatform\Metadata\Post; use ApiPlatform\Metadata\Put; +use ApiPlatform\Metadata\UrlGeneratorInterface; use ApiPlatform\State\CreateProvider; use App\Repository\ReviewRepository; use App\Serializer\IriTransformerNormalizer; @@ -76,7 +77,7 @@ AbstractNormalizer::GROUPS => ['Review:write', 'Review:write:admin'], ], collectDenormalizationErrors: true, - security: 'is_granted("ROLE_ADMIN")' + security: 'is_granted("ADMIN")' )] #[ApiResource( types: ['https://schema.org/Review'], @@ -98,7 +99,7 @@ ] ), new Post( - security: 'is_granted("ROLE_USER")', + security: 'is_granted("USER")', // Mercure publish is done manually in MercureProcessor through ReviewPersistProcessor processor: ReviewPersistProcessor::class, provider: CreateProvider::class, @@ -111,7 +112,7 @@ 'bookId' => new Link(toProperty: 'book', fromClass: Book::class), 'id' => new Link(fromClass: Review::class), ], - security: 'is_granted("ROLE_USER") and user == object.user', + security: 'object.user == user or is_granted("ADMIN")', // Mercure publish is done manually in MercureProcessor through ReviewPersistProcessor processor: ReviewPersistProcessor::class ), @@ -121,7 +122,7 @@ 'bookId' => new Link(toProperty: 'book', fromClass: Book::class), 'id' => new Link(fromClass: Review::class), ], - security: 'is_granted("ROLE_USER") and user == object.user', + security: 'object.user == user or is_granted("ADMIN")', // Mercure publish is done manually in MercureProcessor through ReviewRemoveProcessor processor: ReviewRemoveProcessor::class ), diff --git a/api/src/Entity/User.php b/api/src/Entity/User.php index f9e2e6fb1..cc79ac1a5 100644 --- a/api/src/Entity/User.php +++ b/api/src/Entity/User.php @@ -30,17 +30,17 @@ new GetCollection( uriTemplate: '/admin/users{._format}', itemUriTemplate: '/admin/users/{id}{._format}', - security: 'is_granted("ROLE_ADMIN")', + security: 'is_granted("ADMIN")', filters: ['app.filter.user.admin.name'], paginationClientItemsPerPage: true ), new Get( uriTemplate: '/admin/users/{id}{._format}', - security: 'is_granted("ROLE_ADMIN")' + security: 'is_granted("ADMIN")' ), new Get( uriTemplate: '/users/{id}{._format}', - security: 'is_granted("ROLE_USER") and object.getUserIdentifier() === user.getUserIdentifier()' + security: 'user.sub === object.sub' ), ], normalizationContext: [ diff --git a/api/src/Security/Voter/OIDCPermissionVoter.php b/api/src/Security/Voter/OIDCPermissionVoter.php new file mode 100644 index 000000000..de2ab5589 --- /dev/null +++ b/api/src/Security/Voter/OIDCPermissionVoter.php @@ -0,0 +1,71 @@ +getToken($token); + if (!$accessToken) { + return false; + } + + if (is_object($subject)) { + $subject = $this->iriConverter->getIriFromResource($subject); + } + + if (!is_string($subject)) { + throw new \InvalidArgumentException(sprintf('Invalid subject type, expected "string" or "object", got "%s".', get_debug_type($subject))); + } + + try { + $response = $this->securityAuthorizationClient->request('POST', 'protocol/openid-connect/token', [ + 'auth_bearer' => $accessToken, + 'body' => [ + 'grant_type' => 'urn:ietf:params:oauth:grant-type:uma-ticket', + 'audience' => $this->oidcClientId, + 'response_mode' => 'decision', + 'permission_resource_format' => 'uri', + 'permission_resource_matching_uri' => true, + 'permission' => sprintf('%s', $subject), + ], + ]); + + return $response->toArray()['result'] ?? false; + } catch (ExceptionInterface) { + return false; + } + } +} diff --git a/api/src/Security/Voter/OIDCRoleVoter.php b/api/src/Security/Voter/OIDCRoleVoter.php new file mode 100644 index 000000000..83e277bf2 --- /dev/null +++ b/api/src/Security/Voter/OIDCRoleVoter.php @@ -0,0 +1,65 @@ +getToken($token); + if (!$accessToken) { + return false; + } + + if (!empty($subject)) { + throw new \InvalidArgumentException(sprintf('Invalid subject type, expected empty string or "null", got "%s".', get_debug_type($subject))); + } + + try { + $response = $this->securityAuthorizationClient->request('POST', 'protocol/openid-connect/token/introspect', [ + 'body' => [ + 'client_id' => $this->oidcClientId, + 'client_secret' => $this->oidcClientSecret, + 'token' => $accessToken, + ], + ]); + + $roles = array_map(static fn (string $role): string => strtolower($role), $response->toArray()['realm_access']['roles'] ?? []); + + return in_array(strtolower($attribute), $roles, true); + } catch (ExceptionInterface) { + return false; + } + } +} diff --git a/api/src/Security/Voter/OIDCVoterTrait.php b/api/src/Security/Voter/OIDCVoterTrait.php new file mode 100644 index 000000000..03a390477 --- /dev/null +++ b/api/src/Security/Voter/OIDCVoterTrait.php @@ -0,0 +1,32 @@ +getUser() instanceof UserInterface) { + return false; + } + + $request = $this->requestStack->getCurrentRequest(); + + // user is authenticated, its token should be valid (validated through AccessTokenAuthenticator) + // todo is there a better way to retrieve the access-token? + $accessToken = $this->accessTokenExtractor->extractAccessToken($request); + if (!$accessToken) { + return false; + } + + return $accessToken; + } +} diff --git a/api/tests/Api/Admin/BookTest.php b/api/tests/Api/Admin/BookTest.php index 1c1307113..48be77db6 100644 --- a/api/tests/Api/Admin/BookTest.php +++ b/api/tests/Api/Admin/BookTest.php @@ -172,7 +172,7 @@ public static function getAllUsers(): iterable { yield [null]; yield [UserFactory::new()]; - yield [UserFactory::new(['roles' => ['ROLE_ADMIN']])]; + yield [UserFactory::new()->withAdmin()]; } #[Test] diff --git a/api/tests/Api/BookmarkTest.php b/api/tests/Api/BookmarkTest.php index 796b5842f..c989cbea1 100644 --- a/api/tests/Api/BookmarkTest.php +++ b/api/tests/Api/BookmarkTest.php @@ -64,6 +64,7 @@ public function asAUserICanGetACollectionOfMyBookmarksWithoutFilters(): void $token = $this->generateToken([ 'email' => $user->email, + 'authorize' => true, ]); $response = $this->client->request('GET', '/bookmarks', ['auth_bearer' => $token]); @@ -107,6 +108,7 @@ public function asAUserICannotCreateABookmarkWithInvalidData(): void { $token = $this->generateToken([ 'email' => UserFactory::createOne()->email, + 'authorize' => true, ]); $uuid = Uuid::v7()->__toString(); @@ -150,6 +152,7 @@ public function asAUserICanCreateABookmark(): void $token = $this->generateToken([ 'email' => $user->email, + 'authorize' => true, ]); $response = $this->client->request('POST', '/bookmarks', [ @@ -196,6 +199,7 @@ public function asAUserICannotCreateADuplicateBookmark(): void $token = $this->generateToken([ 'email' => $user->email, + 'authorize' => true, ]); $this->client->request('POST', '/bookmarks', [ @@ -243,6 +247,7 @@ public function asAUserICannotDeleteABookmarkOfAnotherUser(): void $token = $this->generateToken([ 'email' => UserFactory::createOne()->email, + 'authorize' => false, ]); $this->client->request('DELETE', '/bookmarks/' . $bookmark->getId(), [ @@ -287,6 +292,7 @@ public function asAUserICanDeleteMyBookmark(): void $token = $this->generateToken([ 'email' => $bookmark->user->email, + 'authorize' => true, ]); $response = $this->client->request('DELETE', '/bookmarks/' . $bookmark->getId(), [ diff --git a/api/tests/Api/Mock/KeycloakProtocolOpenIdConnectTokenIntrospectMock.php b/api/tests/Api/Mock/KeycloakProtocolOpenIdConnectTokenIntrospectMock.php new file mode 100644 index 000000000..2aa893ddb --- /dev/null +++ b/api/tests/Api/Mock/KeycloakProtocolOpenIdConnectTokenIntrospectMock.php @@ -0,0 +1,73 @@ +handleRequest(...), $this->baseUri); + } + + private function handleRequest(string $method, string $url, array $options): ResponseInterface + { + if (!('POST' === $method && $this->baseUri.'protocol/openid-connect/token/introspect' === $url)) { + return $this->decorated->request($method, $url, $options); + } + + if (!isset($options['body'])) { + return $this->getInvalidMock(); + } + + // retrieve token from body + parse_str($options['body'], $body); + if (!isset($body['token'])) { + return $this->getInvalidMock(); + } + + $serializerManager = new JWSSerializerManager([new CompactSerializer()]); + $jws = $serializerManager->unserialize($body['token']); + $claims = json_decode($jws->getPayload(), true); + + // "authorize" custom claim set in the test + if (array_key_exists('authorize', $claims)) { + return $claims['authorize'] ? $this->getValidMock($claims) : $this->getInvalidMock(); + } + + // no "authorize" custom claim: build roles from user email + return 'chuck.norris@example.com' === ($claims['email'] ?? null) ? $this->getValidMock($claims) : $this->getInvalidMock(); + } + + private function getValidMock(array $claims): MockResponse + { + $roles = ['offline_access', 'uma_authorization', 'user']; + if ('chuck.norris@example.com' === ($claims['email'] ?? null)) { + $roles[] = 'admin'; + } + + return new MockResponse(json_encode($claims + [ + 'realm_access' => [ + 'roles' => $roles, + ], + ]), ['http_code' => Response::HTTP_OK]); + } + + private function getInvalidMock(): MockResponse + { + return new MockResponse('', ['http_code' => Response::HTTP_UNAUTHORIZED]); + } +} diff --git a/api/tests/Api/Mock/KeycloakProtocolOpenIdConnectTokenMock.php b/api/tests/Api/Mock/KeycloakProtocolOpenIdConnectTokenMock.php new file mode 100644 index 000000000..a20b15f72 --- /dev/null +++ b/api/tests/Api/Mock/KeycloakProtocolOpenIdConnectTokenMock.php @@ -0,0 +1,70 @@ +handleRequest(...), $this->baseUri); + } + + private function handleRequest(string $method, string $url, array $options): ResponseInterface + { + if (!('POST' === $method && $this->baseUri.'protocol/openid-connect/token' === $url)) { + return $this->decorated->request($method, $url, $options); + } + + if (!isset($options['normalized_headers']['authorization'][0])) { + return $this->getInvalidMock(); + } + + $accessToken = preg_replace('/^Authorization: Bearer (.*)$/', '$1', $options['normalized_headers']['authorization'][0]); + $serializerManager = new JWSSerializerManager([new CompactSerializer()]); + $jws = $serializerManager->unserialize($accessToken); + $claims = json_decode($jws->getPayload(), true); + + // "authorize" custom claim set in the test + if (array_key_exists('authorize', $claims)) { + return $claims['authorize'] ? $this->getValidMock() : $this->getInvalidMock(); + } + + // no "authorize" custom claim set, try to detect permission from body + parse_str($options['body'], $body); + if (!isset($body['permission'])) { + return $this->getInvalidMock(); + } + + // if permission starts with "/admin", check for user email in token + if (preg_match('/^\/admin\//', $body['permission'])) { + return 'chuck.norris@example.com' === ($claims['email'] ?? null) ? $this->getValidMock() : $this->getInvalidMock(); + } + + // no "authorize" custom claim, permission is not "/admin": consider permission valid + return $this->getValidMock(); + } + + private function getValidMock(): MockResponse + { + return new MockResponse(json_encode(['result' => true]), ['http_code' => Response::HTTP_OK]); + } + + private function getInvalidMock(): MockResponse + { + return new MockResponse(json_encode(['result' => false]), ['http_code' => Response::HTTP_UNAUTHORIZED]); + } +} diff --git a/api/tests/Api/Mock/NotImplementedMock.php b/api/tests/Api/Mock/NotImplementedMock.php new file mode 100644 index 000000000..0518e502a --- /dev/null +++ b/api/tests/Api/Mock/NotImplementedMock.php @@ -0,0 +1,23 @@ +handleRequest(...), $baseUri); + } + + public function handleRequest(string $method, string $url): void + { + throw new \UnexpectedValueException("Mock not implemented: $method/$url"); + } +} diff --git a/api/tests/Api/ReviewTest.php b/api/tests/Api/ReviewTest.php index 686b607b9..e506d529c 100644 --- a/api/tests/Api/ReviewTest.php +++ b/api/tests/Api/ReviewTest.php @@ -164,6 +164,7 @@ public function asAUserICannotAddAReviewOnABookWithInvalidData(array $data, int $token = $this->generateToken([ 'email' => UserFactory::createOne()->email, + 'authorize' => true, ]); $this->client->request('POST', '/books/' . $book->getId() . '/reviews', [ @@ -249,6 +250,7 @@ public function asAUserICanAddAReviewOnABook(): void $token = $this->generateToken([ 'email' => $user->email, + 'authorize' => true, ]); $response = $this->client->request('POST', '/books/' . $book->getId() . '/reviews', [ @@ -303,6 +305,7 @@ public function asAUserICannotAddADuplicateReviewOnABook(): void $token = $this->generateToken([ 'email' => $user->email, + 'authorize' => true, ]); $this->client->request('POST', '/books/' . $book->getId() . '/reviews', [ @@ -393,6 +396,7 @@ public function asAUserICannotUpdateABookReviewOfAnotherUser(): void $token = $this->generateToken([ 'email' => UserFactory::createOne()->email, + 'authorize' => false, ]); $this->client->request('PATCH', '/books/' . $review->book->getId() . '/reviews/' . $review->getId(), [ @@ -450,6 +454,7 @@ public function asAUserICanUpdateMyBookReview(): void $token = $this->generateToken([ 'email' => $review->user->email, + 'authorize' => true, ]); $this->client->request('PATCH', '/books/' . $review->book->getId() . '/reviews/' . $review->getId(), [ @@ -507,6 +512,7 @@ public function asAUserICannotDeleteABookReviewOfAnotherUser(): void $token = $this->generateToken([ 'email' => UserFactory::createOne()->email, + 'authorize' => false, ]); $this->client->request('DELETE', '/books/' . $review->book->getId() . '/reviews/' . $review->getId(), [ @@ -552,6 +558,7 @@ public function asAUserICanDeleteMyBookReview(): void $token = $this->generateToken([ 'email' => $review->user->email, + 'authorize' => true, ]); $response = $this->client->request('DELETE', '/books/' . $bookId . '/reviews/' . $id, [ diff --git a/helm/api-platform/keycloak/config/realm-demo.json b/helm/api-platform/keycloak/config/realm-demo.json index e6e99092a..17a271de5 100755 --- a/helm/api-platform/keycloak/config/realm-demo.json +++ b/helm/api-platform/keycloak/config/realm-demo.json @@ -1,10 +1,52 @@ { "realm": "demo", "displayName": "API Platform - Demo", + "accessCodeLifespan": 1, "enabled": true, "registrationAllowed": false, - "accessCodeLifespan": 1, + "loginWithEmailAllowed": true, "loginTheme": "api-platform-demo", + "roles": { + "realm": [ + { + "name": "admin", + "description": "Admin role (includes 'user' role)", + "composite": true, + "composites": { + "realm": [ + "user" + ] + }, + "clientRole": false + }, + { + "id": "0a34f93f-a81a-4edd-b6a2-7ed3d06a392c", + "name": "user", + "description": "User role (default role on an authenticated user)", + "composite": false, + "clientRole": false + } + ], + "client": { + "api-platform-api": [ + { + "name": "uma_protection", + "composite": false, + "clientRole": true, + "containerId": "6832d039-5543-4e66-afc5-bc5057e8234d" + } + ], + "api-platform-pwa": [], + "api-platform-swagger": [] + } + }, + "defaultRole": { + "id": "0a34f93f-a81a-4edd-b6a2-7ed3d06a392c", + "name": "user", + "description": "User role (default role on an authenticated user)", + "composite": false, + "clientRole": false + }, "users": [ { "username": "chuck.norris", @@ -18,6 +60,9 @@ "type": "password", "value": "Pa55w0rd" } + ], + "realmRoles": [ + "admin" ] }, { @@ -32,12 +77,107 @@ "type": "password", "value": "Pa55w0rd" } + ], + "realmRoles": [ + "user" ] } ], "clients": [ + { + "id": "6832d039-5543-4e66-afc5-bc5057e8234d", + "clientId": "api-platform-api", + "description": "Confidential client to check authorizations (RBAC)", + "name": "API Platform API", + "enabled": true, + "clientAuthenticatorType": "client-secret", + "secret": "sEocbxCy7iFS8NzYzWyQ71QgxTDZ9fnU", + "redirectUris": [ + "/*" + ], + "webOrigins": [ + "/*" + ], + "standardFlowEnabled": true, + "directAccessGrantsEnabled": true, + "serviceAccountsEnabled": true, + "authorizationServicesEnabled": true, + "publicClient": false, + "protocol": "openid-connect", + "fullScopeAllowed": true, + "authorizationSettings": { + "allowRemoteResourceManagement": true, + "policyEnforcementMode": "ENFORCING", + "decisionStrategy": "UNANIMOUS", + "resources": [ + { + "name": "Admin Resource", + "ownerManagedAccess": false, + "displayName": "admin-resource", + "uris": [ + "/admin/*" + ] + }, + { + "name": "Bookmark Resource", + "ownerManagedAccess": false, + "displayName": "bookmark-resource", + "uris": [ + "/bookmarks/*" + ] + } + ], + "policies": [ + { + "name": "Admin Policy", + "description": "Defines that only admin users are allowed", + "type": "role", + "logic": "POSITIVE", + "decisionStrategy": "UNANIMOUS", + "config": { + "roles": "[{\"id\":\"admin\",\"required\":true}]" + } + }, + { + "name": "User Policy", + "description": "Defines that authenticated users are allowed", + "type": "role", + "logic": "POSITIVE", + "decisionStrategy": "UNANIMOUS", + "config": { + "roles": "[{\"id\":\"user\",\"required\":true}]" + } + }, + { + "name": "admin-permission", + "description": "Admin Permission", + "type": "resource", + "logic": "POSITIVE", + "decisionStrategy": "UNANIMOUS", + "config": { + "defaultResourceType": "", + "resources": "[\"Admin Resource\"]", + "applyPolicies": "[\"Admin Policy\"]" + } + }, + { + "name": "user-permission", + "description": "User Permission", + "type": "resource", + "logic": "POSITIVE", + "decisionStrategy": "UNANIMOUS", + "config": { + "resources": "[\"Bookmark Resource\"]", + "applyPolicies": "[\"User Policy\"]" + } + } + ] + } + }, { "clientId": "api-platform-swagger", + "name": "API Platform Swagger UI", + "description": "Public client for Swagger UI", "enabled": true, "redirectUris": ["*"], "webOrigins": ["*"], @@ -45,6 +185,8 @@ }, { "clientId": "api-platform-pwa", + "name": "API Platform PWA", + "description": "Public client for the PWA", "enabled": true, "redirectUris": ["*"], "webOrigins": ["*"], From 7df520395bf90f306ac56caf9df572a37917277d Mon Sep 17 00:00:00 2001 From: Vincent Chalamon <407859+vincentchalamon@users.noreply.github.com> Date: Wed, 13 Mar 2024 20:47:08 +0100 Subject: [PATCH 2/8] chore: use OIDC Discovery with local cache --- api/.env | 1 + api/.env.test | 1 + api/Dockerfile | 1 + api/composer.json | 7 +- api/config/bundles.php | 1 + api/config/packages/framework.yaml | 6 -- api/config/packages/jose.yaml | 44 +++++++++ api/config/packages/security.yaml | 29 ++---- api/config/services.yaml | 8 ++ api/src/Entity/Review.php | 1 - .../Oidc/OidcDiscoveryTokenHandler.php | 98 +++++++++++++++++++ .../Security/Voter/OIDCPermissionVoter.php | 9 +- api/src/Security/Voter/OIDCRoleVoter.php | 21 +++- api/src/Security/Voter/OIDCVoterTrait.php | 2 + api/symfony.lock | 9 ++ api/tests/Api/Admin/BookTest.php | 35 ++++--- api/tests/Api/Admin/ReviewTest.php | 25 +++-- api/tests/Api/Admin/UserTest.php | 13 ++- api/tests/Api/BookmarkTest.php | 17 ++-- api/tests/Api/ReviewTest.php | 23 +++-- api/tests/Api/Security/TokenGenerator.php | 68 +++++++++++++ ...otocolOpenIdConnectTokenIntrospectMock.php | 8 +- ...KeycloakProtocolOpenIdConnectTokenMock.php | 8 +- .../Voter}/Mock/NotImplementedMock.php | 6 +- api/tests/Api/Trait/SecurityTrait.php | 56 ----------- .../IriTransformerNormalizerTest.php | 1 - .../Processor/BookPersistProcessorTest.php | 1 - .../Processor/BookRemoveProcessorTest.php | 2 - .../Processor/ReviewRemoveProcessorTest.php | 2 - compose.yaml | 1 - .../keycloak/config/realm-demo.json | 8 ++ 31 files changed, 348 insertions(+), 164 deletions(-) create mode 100644 api/config/packages/jose.yaml create mode 100644 api/src/Security/Http/AccessToken/Oidc/OidcDiscoveryTokenHandler.php create mode 100644 api/tests/Api/Security/TokenGenerator.php rename api/tests/Api/{ => Security/Voter}/Mock/KeycloakProtocolOpenIdConnectTokenIntrospectMock.php (91%) rename api/tests/Api/{ => Security/Voter}/Mock/KeycloakProtocolOpenIdConnectTokenMock.php (92%) rename api/tests/Api/{ => Security/Voter}/Mock/NotImplementedMock.php (87%) delete mode 100644 api/tests/Api/Trait/SecurityTrait.php diff --git a/api/.env b/api/.env index 67a858f1d..5512e6033 100644 --- a/api/.env +++ b/api/.env @@ -22,6 +22,7 @@ OIDC_SERVER_URL_INTERNAL=http://keycloak:8080/oidc/realms/demo OIDC_SWAGGER_CLIENT_ID=api-platform-swagger OIDC_API_CLIENT_ID=api-platform-api OIDC_API_CLIENT_SECRET=sEocbxCy7iFS8NzYzWyQ71QgxTDZ9fnU +OIDC_AUD=api-platform ###> symfony/framework-bundle ### APP_ENV=dev diff --git a/api/.env.test b/api/.env.test index e3d5d8bdb..d568771b8 100644 --- a/api/.env.test +++ b/api/.env.test @@ -4,6 +4,7 @@ APP_SECRET='$ecretf0rt3st' SYMFONY_DEPRECATIONS_HELPER=999999 PANTHER_APP_ENV=panther PANTHER_ERROR_SCREENSHOT_DIR=./var/error-screenshots +OIDC_JWK='{"kty": "EC","d": "cT3_vKHaGOAhhmzR0Jbi1ko40dNtpjtaiWzm_7VNwLA","use": "sig","crv": "P-256","x": "n6PnJPqNK5nP-ymwwsOIqZvjiCKFNzRyqWA8KNyBsDo","y": "bQSmMlDXOmtgyS1rhsKUmqlxq-8Kw0Iw9t50cSloTMM","alg": "ES256"}' # API Platform distribution TRUSTED_HOSTS=^example\.com|localhost$ diff --git a/api/Dockerfile b/api/Dockerfile index 5df79082f..a0498e6a3 100644 --- a/api/Dockerfile +++ b/api/Dockerfile @@ -34,6 +34,7 @@ RUN apt-get update; \ RUN set -eux; \ install-php-extensions \ apcu \ + bcmath \ intl \ opcache \ zip \ diff --git a/api/composer.json b/api/composer.json index 4862a32f8..fd8d6e3e8 100644 --- a/api/composer.json +++ b/api/composer.json @@ -34,6 +34,7 @@ "symfony/uid": "7.0.*", "symfony/validator": "7.0.*", "symfony/yaml": "7.0.*", + "web-token/jwt-bundle": "^3.3", "web-token/jwt-library": "^3.3", "webonyx/graphql-php": "^15.8", "zenstruck/foundry": "^1.36" @@ -109,7 +110,11 @@ "symfony": { "allow-contrib": false, "require": "7.0.*", - "docker": false + "docker": false, + "endpoint": [ + "https://api.github.com/repos/Spomky-Labs/recipes/contents/index.json?ref=main", + "flex://defaults" + ] } } } diff --git a/api/config/bundles.php b/api/config/bundles.php index 24080a7f5..aafe128b0 100644 --- a/api/config/bundles.php +++ b/api/config/bundles.php @@ -18,4 +18,5 @@ DAMA\DoctrineTestBundle\DAMADoctrineTestBundle::class => ['test' => true], Doctrine\Bundle\FixturesBundle\DoctrineFixturesBundle::class => ['all' => true], Zenstruck\Foundry\ZenstruckFoundryBundle::class => ['all' => true], + Jose\Bundle\JoseFramework\JoseFrameworkBundle::class => ['all' => true], ]; diff --git a/api/config/packages/framework.yaml b/api/config/packages/framework.yaml index 74579b6ac..2610ba9e5 100644 --- a/api/config/packages/framework.yaml +++ b/api/config/packages/framework.yaml @@ -34,9 +34,3 @@ when@test: test: true #session: # storage_factory_id: session.storage.factory.mock_file - - services: - App\Tests\Api\Mock\: - resource: '../../tests/Api/Mock/' - autowire: true - autoconfigure: true diff --git a/api/config/packages/jose.yaml b/api/config/packages/jose.yaml new file mode 100644 index 000000000..1f1b7ab35 --- /dev/null +++ b/api/config/packages/jose.yaml @@ -0,0 +1,44 @@ +jose: + jws: + serializers: + oidc: + serializers: ['jws_compact'] + is_public: true + loaders: + oidc: + serializers: ['jws_compact'] + signature_algorithms: ['HS256', 'RS256', 'ES256'] + header_checkers: ['alg', 'iat', 'nbf', 'exp', 'aud', 'iss'] + is_public: true + +services: + _defaults: + autowire: true + autoconfigure: true + + Jose\Component\Checker\AlgorithmChecker: + arguments: + $supportedAlgorithms: ['HS256', 'RS256', 'ES256'] + tags: + - name: 'jose.checker.header' + alias: 'alg' + Jose\Component\Checker\AudienceChecker: + arguments: + $audience: '%env(OIDC_AUD)%' + tags: + - name: 'jose.checker.header' + alias: 'aud' + Jose\Component\Checker\IssuerChecker: + arguments: + $issuers: ['%env(OIDC_SERVER_URL)%'] + tags: + - name: 'jose.checker.header' + alias: 'iss' + +when@test: + jose: + jws: + builders: + oidc: + signature_algorithms: ['HS256', 'RS256', 'ES256'] + is_public: true diff --git a/api/config/packages/security.yaml b/api/config/packages/security.yaml index b48296ec5..653cf9442 100644 --- a/api/config/packages/security.yaml +++ b/api/config/packages/security.yaml @@ -1,7 +1,3 @@ -parameters: - app.oidc.jwk: '{"kty": "EC","d": "cT3_vKHaGOAhhmzR0Jbi1ko40dNtpjtaiWzm_7VNwLA","use": "sig","crv": "P-256","x": "n6PnJPqNK5nP-ymwwsOIqZvjiCKFNzRyqWA8KNyBsDo","y": "bQSmMlDXOmtgyS1rhsKUmqlxq-8Kw0Iw9t50cSloTMM","alg": "ES256"}' - app.oidc.aud: 'api-platform' - security: # https://symfony.com/doc/current/security.html#loading-the-user-the-user-provider providers: @@ -32,10 +28,14 @@ when@prod: &prod firewalls: main: access_token: - token_handler: - oidc_user_info: - claim: email - base_uri: '%env(OIDC_SERVER_URL_INTERNAL)%/protocol/openid-connect/userinfo' + token_handler: App\Security\Http\AccessToken\Oidc\OidcDiscoveryTokenHandler + # todo support Discovery in Symfony +# oidc: +# claim: 'email' +# base_uri: '%env(OIDC_SERVER_URL)%' +# audience: '%env(OIDC_AUD)%' +# cache: '@cache.app' # default +# cache_ttl: 3600 # default when@dev: *prod @@ -47,16 +47,7 @@ when@test: token_handler: oidc: claim: email - audience: '%app.oidc.aud%' + audience: '%env(OIDC_AUD)%' issuers: [ '%env(OIDC_SERVER_URL)%' ] algorithm: 'ES256' - key: '%app.oidc.jwk%' - # required by App\Tests\Api\Trait\SecurityTrait - parameters: - app.oidc.issuer: '%env(OIDC_SERVER_URL)%' - services: - app.security.jwk: - parent: 'security.access_token_handler.oidc.jwk' - public: true - arguments: - $json: '%app.oidc.jwk%' + key: '%env(OIDC_JWK)%' diff --git a/api/config/services.yaml b/api/config/services.yaml index 2d6a76f94..f5e1a973d 100644 --- a/api/config/services.yaml +++ b/api/config/services.yaml @@ -22,3 +22,11 @@ services: # add more service definitions when explicit configuration is needed # please note that last definitions always *replace* previous ones + +when@test: + services: + App\Tests\Api\Security\: + resource: '../tests/Api/Security/' + autowire: true + autoconfigure: true + public: true diff --git a/api/src/Entity/Review.php b/api/src/Entity/Review.php index f1b0b89c6..442e8a504 100644 --- a/api/src/Entity/Review.php +++ b/api/src/Entity/Review.php @@ -14,7 +14,6 @@ use ApiPlatform\Metadata\Patch; use ApiPlatform\Metadata\Post; use ApiPlatform\Metadata\Put; -use ApiPlatform\Metadata\UrlGeneratorInterface; use ApiPlatform\State\CreateProvider; use App\Repository\ReviewRepository; use App\Serializer\IriTransformerNormalizer; diff --git a/api/src/Security/Http/AccessToken/Oidc/OidcDiscoveryTokenHandler.php b/api/src/Security/Http/AccessToken/Oidc/OidcDiscoveryTokenHandler.php new file mode 100644 index 000000000..89262ad9a --- /dev/null +++ b/api/src/Security/Http/AccessToken/Oidc/OidcDiscoveryTokenHandler.php @@ -0,0 +1,98 @@ +cache->get('oidc.configuration', function (ItemInterface $item): string { + $item->expiresAfter($this->ttl); + $response = HttpClient::create()->request('GET', rtrim($this->issuer, '/') . '/.well-known/openid-configuration'); + + return $response->getContent(); + }), true, 512, \JSON_THROW_ON_ERROR); + + $keyset = JWKSet::createFromJson( + $this->cache->get('oidc.jwkSet', function (ItemInterface $item) use ($oidcConfiguration): string { + $item->expiresAfter($this->ttl); + $client = HttpClient::create(); + $response = $client->request('GET', $oidcConfiguration['jwks_uri']); + // we only need signature key + $keys = array_filter($response->toArray()['keys'], static fn (array $key) => 'sig' === $key['use']); + + return json_encode(['keys' => $keys]); + }) + ); + + try { + // Decode the token + $signature = 0; + $jws = $this->jwsLoader->loadAndVerifyWithKeySet( + token: $accessToken, + keyset: $keyset, + signature: $signature, + ); + + $claims = json_decode($jws->getPayload(), true); + if (empty($claims[$this->claim])) { + throw new MissingClaimException(sprintf('"%s" claim not found.', $this->claim)); + } + + // UserLoader argument can be overridden by a UserProvider on AccessTokenAuthenticator::authenticate + return new UserBadge($claims[$this->claim], new FallbackUserLoader(fn () => $this->createUser($claims)), $claims); + } catch (\Exception $e) { + $this->logger?->error('An error occurred while decoding and validating the token.', [ + 'error' => $e->getMessage(), + 'trace' => $e->getTraceAsString(), + ]); + + throw new BadCredentialsException('Invalid credentials.', $e->getCode(), $e); + } + } +} diff --git a/api/src/Security/Voter/OIDCPermissionVoter.php b/api/src/Security/Voter/OIDCPermissionVoter.php index de2ab5589..8c5458925 100644 --- a/api/src/Security/Voter/OIDCPermissionVoter.php +++ b/api/src/Security/Voter/OIDCPermissionVoter.php @@ -1,5 +1,7 @@ iriConverter->getIriFromResource($subject); } - if (!is_string($subject)) { + if (!\is_string($subject)) { throw new \InvalidArgumentException(sprintf('Invalid subject type, expected "string" or "object", got "%s".', get_debug_type($subject))); } diff --git a/api/src/Security/Voter/OIDCRoleVoter.php b/api/src/Security/Voter/OIDCRoleVoter.php index 83e277bf2..55f5fb53e 100644 --- a/api/src/Security/Voter/OIDCRoleVoter.php +++ b/api/src/Security/Voter/OIDCRoleVoter.php @@ -1,13 +1,17 @@ strtolower($role), $response->toArray()['realm_access']['roles'] ?? []); - - return in_array(strtolower($attribute), $roles, true); - } catch (ExceptionInterface) { + } catch (HttpExceptionInterface) { + // OIDC server said no! return false; + } catch (ExceptionInterface) { + // OIDC server doesn't seem to answer: check roles in token (if present) + $jws = $this->jwsSerializerManager->unserialize($accessToken); + $claims = json_decode($jws->getPayload(), true); + $roles = array_map(static fn (string $role): string => strtolower($role), $claims['realm_access']['roles'] ?? []); } + + return \in_array(strtolower($attribute), $roles, true); } } diff --git a/api/src/Security/Voter/OIDCVoterTrait.php b/api/src/Security/Voter/OIDCVoterTrait.php index 03a390477..ce03ddae2 100644 --- a/api/src/Security/Voter/OIDCVoterTrait.php +++ b/api/src/Security/Voter/OIDCVoterTrait.php @@ -1,5 +1,7 @@ generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => $userFactory->create()->email, ]); $options['auth_bearer'] = $token; @@ -68,7 +67,7 @@ public function asAdminUserICanGetACollectionOfBooks(FactoryCollection $factory, // Cannot use Factory as data provider because BookFactory has a service dependency $factory->create(); - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => UserFactory::createOneAdmin()->email, ]); @@ -135,7 +134,7 @@ public function asAdminUserICanGetACollectionOfBooksOrderedByTitle(): void BookFactory::createOne(['title' => 'The Wandering Earth']); BookFactory::createOne(['title' => 'Ball Lightning']); - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => UserFactory::createOneAdmin()->email, ]); @@ -157,7 +156,7 @@ public function asAnyUserICannotGetAnInvalidBook(?UserFactory $userFactory): voi $options = []; if ($userFactory) { - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => $userFactory->create()->email, ]); $options['auth_bearer'] = $token; @@ -183,7 +182,7 @@ public function asNonAdminUserICannotGetABook(int $expectedCode, string $hydraDe $options = []; if ($userFactory) { - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => $userFactory->create()->email, ]); $options['auth_bearer'] = $token; @@ -207,7 +206,7 @@ public function asAdminUserICanGetABook(): void { $book = BookFactory::createOne(); - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => UserFactory::createOneAdmin()->email, ]); @@ -231,7 +230,7 @@ public function asNonAdminUserICannotCreateABook(int $expectedCode, string $hydr { $options = []; if ($userFactory) { - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => $userFactory->create()->email, ]); $options['auth_bearer'] = $token; @@ -262,7 +261,7 @@ public function asNonAdminUserICannotCreateABook(int $expectedCode, string $hydr #[DataProvider(methodName: 'getInvalidDataOnCreate')] public function asAdminUserICannotCreateABookWithInvalidData(array $data, int $statusCode, array $expected): void { - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => UserFactory::createOneAdmin()->email, ]); @@ -368,7 +367,7 @@ public static function getInvalidData(): iterable #[Test] public function asAdminUserICanCreateABook(): void { - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => UserFactory::createOneAdmin()->email, ]); @@ -429,7 +428,7 @@ public function asNonAdminUserICannotUpdateBook(int $expectedCode, string $hydra $options = []; if ($userFactory) { - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => $userFactory->create()->email, ]); $options['auth_bearer'] = $token; @@ -461,7 +460,7 @@ public function asAdminUserICannotUpdateAnInvalidBook(): void { BookFactory::createOne(); - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => UserFactory::createOneAdmin()->email, ]); @@ -485,7 +484,7 @@ public function asAdminUserICannotUpdateABookWithInvalidData(array $data, int $s { $book = BookFactory::createOne(); - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => UserFactory::createOneAdmin()->email, ]); @@ -516,7 +515,7 @@ public function asAdminUserICanUpdateABook(): void ]); self::getMercureHub()->reset(); - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => UserFactory::createOneAdmin()->email, ]); @@ -576,7 +575,7 @@ public function asNonAdminUserICannotDeleteABook(int $expectedCode, string $hydr $options = []; if ($userFactory) { - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => $userFactory->create()->email, ]); $options['auth_bearer'] = $token; @@ -599,7 +598,7 @@ public function asAdminUserICannotDeleteAnInvalidBook(): void { BookFactory::createOne(); - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => UserFactory::createOneAdmin()->email, ]); @@ -618,7 +617,7 @@ public function asAdminUserICanDeleteABook(): void self::getMercureHub()->reset(); $id = $book->getId(); - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => UserFactory::createOneAdmin()->email, ]); diff --git a/api/tests/Api/Admin/ReviewTest.php b/api/tests/Api/Admin/ReviewTest.php index d9e24c92b..b761be05f 100644 --- a/api/tests/Api/Admin/ReviewTest.php +++ b/api/tests/Api/Admin/ReviewTest.php @@ -13,7 +13,7 @@ use App\Entity\Review; use App\Entity\User; use App\Tests\Api\Admin\Trait\UsersDataProviderTrait; -use App\Tests\Api\Trait\SecurityTrait; +use App\Tests\Api\Security\TokenGenerator; use App\Tests\Api\Trait\SerializerTrait; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Test; @@ -27,7 +27,6 @@ final class ReviewTest extends ApiTestCase { use Factories; use ResetDatabase; - use SecurityTrait; use SerializerTrait; use UsersDataProviderTrait; @@ -44,7 +43,7 @@ public function asNonAdminUserICannotGetACollectionOfReviews(int $expectedCode, { $options = []; if ($userFactory) { - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => $userFactory->create()->email, ]); $options['auth_bearer'] = $token; @@ -68,7 +67,7 @@ public function asAdminUserICanGetACollectionOfReviews(FactoryCollection $factor { $factory->create(); - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => UserFactory::createOneAdmin()->email, ]); @@ -151,7 +150,7 @@ public function asNonAdminUserICannotGetAReview(int $expectedCode, string $hydra $options = []; if ($userFactory) { - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => $userFactory->create()->email, ]); $options['auth_bearer'] = $token; @@ -172,7 +171,7 @@ public function asNonAdminUserICannotGetAReview(int $expectedCode, string $hydra #[Test] public function asAdminUserICannotGetAnInvalidReview(): void { - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => UserFactory::createOneAdmin()->email, ]); @@ -186,7 +185,7 @@ public function asAdminUserICanGetAReview(): void { $review = ReviewFactory::createOne(); - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => UserFactory::createOneAdmin()->email, ]); @@ -205,7 +204,7 @@ public function asNonAdminUserICannotUpdateAReview(int $expectedCode, string $hy $options = []; if ($userFactory) { - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => $userFactory->create()->email, ]); $options['auth_bearer'] = $token; @@ -235,7 +234,7 @@ public function asNonAdminUserICannotUpdateAReview(int $expectedCode, string $hy #[Test] public function asAdminUserICannotUpdateAnInvalidReview(): void { - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => UserFactory::createOneAdmin()->email, ]); @@ -264,7 +263,7 @@ public function asAdminUserICanUpdateAReview(): void $review = ReviewFactory::createOne(['book' => $book]); $user = UserFactory::createOneAdmin(); - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => $user->email, ]); @@ -325,7 +324,7 @@ public function asNonAdminUserICannotDeleteAReview(int $expectedCode, string $hy $options = []; if ($userFactory) { - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => $userFactory->create()->email, ]); $options['auth_bearer'] = $token; @@ -346,7 +345,7 @@ public function asNonAdminUserICannotDeleteAReview(int $expectedCode, string $hy #[Test] public function asAdminUserICannotDeleteAnInvalidReview(): void { - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => UserFactory::createOneAdmin()->email, ]); @@ -365,7 +364,7 @@ public function asAdminUserICanDeleteAReview(): void $id = $review->getId(); $bookId = $review->book->getId(); - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => UserFactory::createOneAdmin()->email, ]); diff --git a/api/tests/Api/Admin/UserTest.php b/api/tests/Api/Admin/UserTest.php index d493df09e..067e6abd1 100644 --- a/api/tests/Api/Admin/UserTest.php +++ b/api/tests/Api/Admin/UserTest.php @@ -9,7 +9,7 @@ use App\DataFixtures\Factory\UserFactory; use App\Repository\UserRepository; use App\Tests\Api\Admin\Trait\UsersDataProviderTrait; -use App\Tests\Api\Trait\SecurityTrait; +use App\Tests\Api\Security\TokenGenerator; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Test; use Symfony\Component\Uid\Uuid; @@ -21,7 +21,6 @@ final class UserTest extends ApiTestCase { use Factories; use ResetDatabase; - use SecurityTrait; use UsersDataProviderTrait; private Client $client; @@ -37,7 +36,7 @@ public function asNonAdminUserICannotGetACollectionOfUsers(int $expectedCode, st { $options = []; if ($userFactory) { - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => $userFactory->create()->email, ]); $options['auth_bearer'] = $token; @@ -61,7 +60,7 @@ public function asAdminUserICanGetACollectionOfUsers(FactoryCollection $factory, { $factory->create(); - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => UserFactory::createOneAdmin()->email, ]); @@ -113,7 +112,7 @@ public function asNonAdminUserICannotGetAUser(int $expectedCode, string $hydraDe $options = []; if ($userFactory) { - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => $userFactory->create()->email, ]); $options['auth_bearer'] = $token; @@ -136,7 +135,7 @@ public function asAdminUserICanGetAUser(): void { $user = UserFactory::createOne(); - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => UserFactory::createOneAdmin()->email, ]); @@ -161,7 +160,7 @@ public function asAUserIAmUpdatedOnLogin(): void ])->disableAutoRefresh(); $sub = Uuid::v7()->__toString(); - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'sub' => $sub, 'email' => $user->email, 'given_name' => 'Chuck', diff --git a/api/tests/Api/BookmarkTest.php b/api/tests/Api/BookmarkTest.php index c989cbea1..679a12937 100644 --- a/api/tests/Api/BookmarkTest.php +++ b/api/tests/Api/BookmarkTest.php @@ -12,7 +12,7 @@ use App\Entity\Book; use App\Entity\Bookmark; use App\Repository\BookmarkRepository; -use App\Tests\Api\Trait\SecurityTrait; +use App\Tests\Api\Security\TokenGenerator; use App\Tests\Api\Trait\SerializerTrait; use PHPUnit\Framework\Attributes\Test; use Symfony\Component\HttpFoundation\Response; @@ -25,7 +25,6 @@ final class BookmarkTest extends ApiTestCase { use Factories; use ResetDatabase; - use SecurityTrait; use SerializerTrait; private Client $client; @@ -62,7 +61,7 @@ public function asAUserICanGetACollectionOfMyBookmarksWithoutFilters(): void $user = UserFactory::createOne(); BookmarkFactory::createMany(35, ['user' => $user]); - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => $user->email, 'authorize' => true, ]); @@ -106,7 +105,7 @@ public function asAnonymousICannotCreateABookmark(): void #[Test] public function asAUserICannotCreateABookmarkWithInvalidData(): void { - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => UserFactory::createOne()->email, 'authorize' => true, ]); @@ -150,7 +149,7 @@ public function asAUserICanCreateABookmark(): void $user = UserFactory::createOne(); self::getMercureHub()->reset(); - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => $user->email, 'authorize' => true, ]); @@ -197,7 +196,7 @@ public function asAUserICannotCreateADuplicateBookmark(): void $user = UserFactory::createOne(); BookmarkFactory::createOne(['book' => $book, 'user' => $user]); - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => $user->email, 'authorize' => true, ]); @@ -245,7 +244,7 @@ public function asAUserICannotDeleteABookmarkOfAnotherUser(): void { $bookmark = BookmarkFactory::createOne(['user' => UserFactory::createOne()]); - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => UserFactory::createOne()->email, 'authorize' => false, ]); @@ -267,7 +266,7 @@ public function asAUserICannotDeleteABookmarkOfAnotherUser(): void #[Test] public function asAUserICannotDeleteAnInvalidBookmark(): void { - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => UserFactory::createOne()->email, ]); @@ -290,7 +289,7 @@ public function asAUserICanDeleteMyBookmark(): void $id = $bookmark->getId(); - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => $bookmark->user->email, 'authorize' => true, ]); diff --git a/api/tests/Api/ReviewTest.php b/api/tests/Api/ReviewTest.php index e506d529c..1a6ee8468 100644 --- a/api/tests/Api/ReviewTest.php +++ b/api/tests/Api/ReviewTest.php @@ -13,7 +13,7 @@ use App\Entity\Review; use App\Entity\User; use App\Repository\ReviewRepository; -use App\Tests\Api\Trait\SecurityTrait; +use App\Tests\Api\Security\TokenGenerator; use App\Tests\Api\Trait\SerializerTrait; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Test; @@ -27,7 +27,6 @@ final class ReviewTest extends ApiTestCase { use Factories; use ResetDatabase; - use SecurityTrait; use SerializerTrait; private Client $client; @@ -162,7 +161,7 @@ public function asAUserICannotAddAReviewOnABookWithInvalidData(array $data, int { $book = BookFactory::createOne(); - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => UserFactory::createOne()->email, 'authorize' => true, ]); @@ -211,7 +210,7 @@ public function asAUserICannotAddAReviewWithValidDataOnAnInvalidBook(): void ReviewFactory::createMany(5, ['book' => $book]); $user = UserFactory::createOne(); - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => $user->email, ]); @@ -248,7 +247,7 @@ public function asAUserICanAddAReviewOnABook(): void $user = UserFactory::createOne(); self::getMercureHub()->reset(); - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => $user->email, 'authorize' => true, ]); @@ -303,7 +302,7 @@ public function asAUserICannotAddADuplicateReviewOnABook(): void $user = UserFactory::createOne(); ReviewFactory::createOne(['book' => $book, 'user' => $user]); - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => $user->email, 'authorize' => true, ]); @@ -394,7 +393,7 @@ public function asAUserICannotUpdateABookReviewOfAnotherUser(): void { $review = ReviewFactory::createOne(['user' => UserFactory::createOne()]); - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => UserFactory::createOne()->email, 'authorize' => false, ]); @@ -425,7 +424,7 @@ public function asAUserICannotUpdateAnInvalidBookReview(): void { $book = BookFactory::createOne(); - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => UserFactory::createOne()->email, ]); @@ -452,7 +451,7 @@ public function asAUserICanUpdateMyBookReview(): void $review = ReviewFactory::createOne(); self::getMercureHub()->reset(); - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => $review->user->email, 'authorize' => true, ]); @@ -510,7 +509,7 @@ public function asAUserICannotDeleteABookReviewOfAnotherUser(): void { $review = ReviewFactory::createOne(['user' => UserFactory::createOne()]); - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => UserFactory::createOne()->email, 'authorize' => false, ]); @@ -534,7 +533,7 @@ public function asAUserICannotDeleteAnInvalidBookReview(): void { $book = BookFactory::createOne(); - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => UserFactory::createOne()->email, ]); @@ -556,7 +555,7 @@ public function asAUserICanDeleteMyBookReview(): void $id = $review->getId(); $bookId = $review->book->getId(); - $token = $this->generateToken([ + $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ 'email' => $review->user->email, 'authorize' => true, ]); diff --git a/api/tests/Api/Security/TokenGenerator.php b/api/tests/Api/Security/TokenGenerator.php new file mode 100644 index 000000000..773454f9e --- /dev/null +++ b/api/tests/Api/Security/TokenGenerator.php @@ -0,0 +1,68 @@ +jwk = JWK::createFromJson(json: $jwk); + } + + public function generateToken(array $claims): string + { + // Defaults + $time = time(); + $sub = Uuid::v7()->__toString(); + $claims += [ + 'sub' => $sub, + 'iat' => $time, + 'nbf' => $time, + 'exp' => $time + 3600, + 'iss' => $this->issuer, + 'aud' => $this->audience, + 'given_name' => 'John', + 'family_name' => 'DOE', + ]; + if (empty($claims['sub'])) { + $claims['sub'] = $sub; + } + if (empty($claims['iat'])) { + $claims['iat'] = $time; + } + if (empty($claims['nbf'])) { + $claims['nbf'] = $time; + } + if (empty($claims['exp'])) { + $claims['exp'] = $time + 3600; + } + + return $this->jwsSerializerManager->serialize( + name: 'jws_compact', + jws: $this->jwsBuilder + ->withPayload(json_encode($claims)) + ->addSignature($this->jwk, ['alg' => $this->jwk->get('alg')]) + ->build(), + ); + } +} diff --git a/api/tests/Api/Mock/KeycloakProtocolOpenIdConnectTokenIntrospectMock.php b/api/tests/Api/Security/Voter/Mock/KeycloakProtocolOpenIdConnectTokenIntrospectMock.php similarity index 91% rename from api/tests/Api/Mock/KeycloakProtocolOpenIdConnectTokenIntrospectMock.php rename to api/tests/Api/Security/Voter/Mock/KeycloakProtocolOpenIdConnectTokenIntrospectMock.php index 2aa893ddb..2accd186f 100644 --- a/api/tests/Api/Mock/KeycloakProtocolOpenIdConnectTokenIntrospectMock.php +++ b/api/tests/Api/Security/Voter/Mock/KeycloakProtocolOpenIdConnectTokenIntrospectMock.php @@ -1,6 +1,8 @@ baseUri.'protocol/openid-connect/token/introspect' === $url)) { + if (!('POST' === $method && $this->baseUri . 'protocol/openid-connect/token/introspect' === $url)) { return $this->decorated->request($method, $url, $options); } @@ -44,7 +46,7 @@ private function handleRequest(string $method, string $url, array $options): Res $claims = json_decode($jws->getPayload(), true); // "authorize" custom claim set in the test - if (array_key_exists('authorize', $claims)) { + if (\array_key_exists('authorize', $claims)) { return $claims['authorize'] ? $this->getValidMock($claims) : $this->getInvalidMock(); } diff --git a/api/tests/Api/Mock/KeycloakProtocolOpenIdConnectTokenMock.php b/api/tests/Api/Security/Voter/Mock/KeycloakProtocolOpenIdConnectTokenMock.php similarity index 92% rename from api/tests/Api/Mock/KeycloakProtocolOpenIdConnectTokenMock.php rename to api/tests/Api/Security/Voter/Mock/KeycloakProtocolOpenIdConnectTokenMock.php index a20b15f72..bc4e151b5 100644 --- a/api/tests/Api/Mock/KeycloakProtocolOpenIdConnectTokenMock.php +++ b/api/tests/Api/Security/Voter/Mock/KeycloakProtocolOpenIdConnectTokenMock.php @@ -1,6 +1,8 @@ baseUri.'protocol/openid-connect/token' === $url)) { + if (!('POST' === $method && $this->baseUri . 'protocol/openid-connect/token' === $url)) { return $this->decorated->request($method, $url, $options); } @@ -39,7 +41,7 @@ private function handleRequest(string $method, string $url, array $options): Res $claims = json_decode($jws->getPayload(), true); // "authorize" custom claim set in the test - if (array_key_exists('authorize', $claims)) { + if (\array_key_exists('authorize', $claims)) { return $claims['authorize'] ? $this->getValidMock() : $this->getInvalidMock(); } diff --git a/api/tests/Api/Mock/NotImplementedMock.php b/api/tests/Api/Security/Voter/Mock/NotImplementedMock.php similarity index 87% rename from api/tests/Api/Mock/NotImplementedMock.php rename to api/tests/Api/Security/Voter/Mock/NotImplementedMock.php index 0518e502a..b9f44e913 100644 --- a/api/tests/Api/Mock/NotImplementedMock.php +++ b/api/tests/Api/Security/Voter/Mock/NotImplementedMock.php @@ -1,6 +1,8 @@ get('security.access_token_handler.oidc.signature.ES256'); - $jwk = $container->get('app.security.jwk'); - $audience = $container->getParameter('app.oidc.aud'); - $issuer = $container->getParameter('app.oidc.issuer'); - - // Defaults - $time = time(); - $sub = Uuid::v7()->__toString(); - $claims += [ - 'sub' => $sub, - 'iat' => $time, - 'nbf' => $time, - 'exp' => $time + 3600, - 'iss' => $issuer, - 'aud' => $audience, - 'given_name' => 'John', - 'family_name' => 'DOE', - ]; - if (empty($claims['sub'])) { - $claims['sub'] = $sub; - } - if (empty($claims['iat'])) { - $claims['iat'] = $time; - } - if (empty($claims['nbf'])) { - $claims['nbf'] = $time; - } - if (empty($claims['exp'])) { - $claims['exp'] = $time + 3600; - } - - return (new CompactSerializer())->serialize((new JWSBuilder(new AlgorithmManager([ - $signatureAlgorithm, - ])))->create() - ->withPayload(json_encode($claims)) - ->addSignature($jwk, ['alg' => $signatureAlgorithm->name()]) - ->build() - ); - } -} diff --git a/api/tests/Serializer/IriTransformerNormalizerTest.php b/api/tests/Serializer/IriTransformerNormalizerTest.php index f2978cce1..aef28b883 100644 --- a/api/tests/Serializer/IriTransformerNormalizerTest.php +++ b/api/tests/Serializer/IriTransformerNormalizerTest.php @@ -5,7 +5,6 @@ namespace App\Tests\Serializer; use ApiPlatform\Api\IriConverterInterface; -use ApiPlatform\Api\UrlGeneratorInterface; use ApiPlatform\Metadata\Operation; use ApiPlatform\Metadata\Operation\Factory\OperationMetadataFactoryInterface; use App\Serializer\IriTransformerNormalizer; diff --git a/api/tests/State/Processor/BookPersistProcessorTest.php b/api/tests/State/Processor/BookPersistProcessorTest.php index c548d99b9..69b31bb9e 100644 --- a/api/tests/State/Processor/BookPersistProcessorTest.php +++ b/api/tests/State/Processor/BookPersistProcessorTest.php @@ -11,7 +11,6 @@ use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; -use Symfony\Component\HttpFoundation\Request; use Symfony\Component\Serializer\Encoder\DecoderInterface; use Symfony\Contracts\HttpClient\HttpClientInterface; use Symfony\Contracts\HttpClient\ResponseInterface; diff --git a/api/tests/State/Processor/BookRemoveProcessorTest.php b/api/tests/State/Processor/BookRemoveProcessorTest.php index 997caecb2..02628ee4f 100644 --- a/api/tests/State/Processor/BookRemoveProcessorTest.php +++ b/api/tests/State/Processor/BookRemoveProcessorTest.php @@ -5,7 +5,6 @@ namespace App\Tests\State\Processor; use ApiPlatform\Api\IriConverterInterface; -use ApiPlatform\Api\UrlGeneratorInterface; use ApiPlatform\Metadata\ApiResource; use ApiPlatform\Metadata\Get; use ApiPlatform\Metadata\Operation; @@ -14,7 +13,6 @@ use ApiPlatform\State\ProcessorInterface; use App\Entity\Book; use App\State\Processor\BookRemoveProcessor; -use App\State\Processor\MercureProcessor; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; diff --git a/api/tests/State/Processor/ReviewRemoveProcessorTest.php b/api/tests/State/Processor/ReviewRemoveProcessorTest.php index 7b3fe9b14..cfd60bc76 100644 --- a/api/tests/State/Processor/ReviewRemoveProcessorTest.php +++ b/api/tests/State/Processor/ReviewRemoveProcessorTest.php @@ -5,7 +5,6 @@ namespace App\Tests\State\Processor; use ApiPlatform\Api\IriConverterInterface; -use ApiPlatform\Api\UrlGeneratorInterface; use ApiPlatform\Metadata\ApiResource; use ApiPlatform\Metadata\Get; use ApiPlatform\Metadata\Operation; @@ -13,7 +12,6 @@ use ApiPlatform\Metadata\Resource\ResourceMetadataCollection; use ApiPlatform\State\ProcessorInterface; use App\Entity\Review; -use App\State\Processor\MercureProcessor; use App\State\Processor\ReviewRemoveProcessor; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\MockObject\MockObject; diff --git a/compose.yaml b/compose.yaml index 05b24cf2b..c27f3bb4b 100644 --- a/compose.yaml +++ b/compose.yaml @@ -38,7 +38,6 @@ services: NEXT_PUBLIC_ENTRYPOINT: http://php AUTH_SECRET: ${AUTH_SECRET:-!ChangeThisNextAuthSecret!} AUTH_URL: ${AUTH_URL:-https://localhost/api/auth} - AUTH_URL_INTERNAL: http://127.0.0.1:3000/api/auth OIDC_CLIENT_ID: ${OIDC_CLIENT_ID:-api-platform-pwa} OIDC_SERVER_URL: ${OIDC_SERVER_URL:-https://localhost/oidc/realms/demo} OIDC_SERVER_URL_INTERNAL: ${OIDC_SERVER_URL_INTERNAL:-http://keycloak:8080/oidc/realms/demo} diff --git a/helm/api-platform/keycloak/config/realm-demo.json b/helm/api-platform/keycloak/config/realm-demo.json index 17a271de5..29d7a3918 100755 --- a/helm/api-platform/keycloak/config/realm-demo.json +++ b/helm/api-platform/keycloak/config/realm-demo.json @@ -83,6 +83,14 @@ ] } ], + "clientScopes": [ + { + "name": "roles", + "attributes": { + "include.in.token.scope": "true" + } + } + ], "clients": [ { "id": "6832d039-5543-4e66-afc5-bc5057e8234d", From b06dd43ade42059dc1f11ca95c35da7a95e83161 Mon Sep 17 00:00:00 2001 From: Vincent Chalamon <407859+vincentchalamon@users.noreply.github.com> Date: Thu, 14 Mar 2024 10:01:32 +0100 Subject: [PATCH 3/8] fix: review --- .../Oidc/OidcDiscoveryTokenHandler.php | 62 ++++++++++--------- .../Security/Voter/OIDCPermissionVoter.php | 12 +++- api/src/Security/Voter/OIDCRoleVoter.php | 11 +++- 3 files changed, 54 insertions(+), 31 deletions(-) diff --git a/api/src/Security/Http/AccessToken/Oidc/OidcDiscoveryTokenHandler.php b/api/src/Security/Http/AccessToken/Oidc/OidcDiscoveryTokenHandler.php index 89262ad9a..bbd53f08e 100644 --- a/api/src/Security/Http/AccessToken/Oidc/OidcDiscoveryTokenHandler.php +++ b/api/src/Security/Http/AccessToken/Oidc/OidcDiscoveryTokenHandler.php @@ -4,13 +4,10 @@ namespace App\Security\Http\AccessToken\Oidc; -use Jose\Component\Checker; use Jose\Component\Core\JWKSet; use Jose\Component\Signature\JWSLoader; -use Jose\Component\Signature\JWSVerifier; use Psr\Log\LoggerInterface; use Symfony\Component\DependencyInjection\Attribute\Autowire; -use Symfony\Component\HttpClient\HttpClient; use Symfony\Component\Security\Core\Exception\BadCredentialsException; use Symfony\Component\Security\Http\AccessToken\AccessTokenHandlerInterface; use Symfony\Component\Security\Http\AccessToken\Oidc\Exception\MissingClaimException; @@ -20,6 +17,7 @@ use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge; use Symfony\Contracts\Cache\CacheInterface; use Symfony\Contracts\Cache\ItemInterface; +use Symfony\Contracts\HttpClient\HttpClientInterface; /** * Completes {@see OidcTokenHandler} with OIDC Discovery and configuration stored in cache. @@ -33,8 +31,7 @@ public function __construct( private CacheInterface $cache, #[Autowire('@jose.jws_loader.oidc')] private JWSLoader $jwsLoader, - #[Autowire('%env(OIDC_SERVER_URL)%')] - private string $issuer, + private readonly HttpClientInterface $securityAuthorizationClient, private string $claim = 'email', private int $ttl = 3600, private ?LoggerInterface $logger = null, @@ -43,36 +40,45 @@ public function __construct( public function getUserBadgeFrom(string $accessToken): UserBadge { - if (!class_exists(JWSVerifier::class) || !class_exists(Checker\HeaderCheckerManager::class)) { - throw new \LogicException('You cannot use the "oidc_discovery" token handler since "web-token/jwt-signature" and "web-token/jwt-checker" are not installed. Try running "composer require web-token/jwt-signature web-token/jwt-checker".'); - } + try { + $oidcConfiguration = json_decode($this->cache->get('oidc.configuration', function (ItemInterface $item): string { + $item->expiresAfter($this->ttl); + $response = $this->securityAuthorizationClient->request('GET', '.well-known/openid-configuration'); - if (!class_exists(HttpClient::class)) { - throw new \LogicException('You cannot use the "oidc_discovery" token handler since "symfony/http-client" is not installed. Try running "composer require symfony/http-client".'); - } + return $response->getContent(); + }), true, 512, \JSON_THROW_ON_ERROR); + } catch (\Throwable $e) { + $this->logger?->error('An error occurred while requesting OIDC configuration.', [ + 'error' => $e->getMessage(), + 'trace' => $e->getTraceAsString(), + ]); - $oidcConfiguration = json_decode($this->cache->get('oidc.configuration', function (ItemInterface $item): string { - $item->expiresAfter($this->ttl); - $response = HttpClient::create()->request('GET', rtrim($this->issuer, '/') . '/.well-known/openid-configuration'); + throw new BadCredentialsException('Invalid credentials.', $e->getCode(), $e); + } - return $response->getContent(); - }), true, 512, \JSON_THROW_ON_ERROR); + try { + $keyset = JWKSet::createFromJson( + $this->cache->get('oidc.jwkSet', function (ItemInterface $item) use ($oidcConfiguration): string { + $item->expiresAfter($this->ttl); + $response = $this->securityAuthorizationClient->request('GET', $oidcConfiguration['jwks_uri']); + // we only need signature key + $keys = array_filter($response->toArray()['keys'], static fn (array $key) => 'sig' === $key['use']); - $keyset = JWKSet::createFromJson( - $this->cache->get('oidc.jwkSet', function (ItemInterface $item) use ($oidcConfiguration): string { - $item->expiresAfter($this->ttl); - $client = HttpClient::create(); - $response = $client->request('GET', $oidcConfiguration['jwks_uri']); - // we only need signature key - $keys = array_filter($response->toArray()['keys'], static fn (array $key) => 'sig' === $key['use']); + return json_encode(['keys' => $keys]); + }) + ); + } catch (\Throwable $e) { + $this->logger?->error('An error occurred while requesting OIDC certs.', [ + 'error' => $e->getMessage(), + 'trace' => $e->getTraceAsString(), + ]); - return json_encode(['keys' => $keys]); - }) - ); + throw new BadCredentialsException('Invalid credentials.', $e->getCode(), $e); + } try { // Decode the token - $signature = 0; + $signature = null; $jws = $this->jwsLoader->loadAndVerifyWithKeySet( token: $accessToken, keyset: $keyset, @@ -86,7 +92,7 @@ public function getUserBadgeFrom(string $accessToken): UserBadge // UserLoader argument can be overridden by a UserProvider on AccessTokenAuthenticator::authenticate return new UserBadge($claims[$this->claim], new FallbackUserLoader(fn () => $this->createUser($claims)), $claims); - } catch (\Exception $e) { + } catch (\Throwable $e) { $this->logger?->error('An error occurred while decoding and validating the token.', [ 'error' => $e->getMessage(), 'trace' => $e->getTraceAsString(), diff --git a/api/src/Security/Voter/OIDCPermissionVoter.php b/api/src/Security/Voter/OIDCPermissionVoter.php index 8c5458925..ef3d95b85 100644 --- a/api/src/Security/Voter/OIDCPermissionVoter.php +++ b/api/src/Security/Voter/OIDCPermissionVoter.php @@ -5,12 +5,14 @@ namespace App\Security\Voter; use ApiPlatform\Metadata\IriConverterInterface; +use Psr\Log\LoggerInterface; use Symfony\Component\DependencyInjection\Attribute\Autowire; use Symfony\Component\HttpFoundation\RequestStack; use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; use Symfony\Component\Security\Core\Authorization\Voter\Voter; use Symfony\Component\Security\Http\AccessToken\AccessTokenExtractorInterface; use Symfony\Contracts\HttpClient\Exception\ExceptionInterface; +use Symfony\Contracts\HttpClient\Exception\HttpExceptionInterface; use Symfony\Contracts\HttpClient\HttpClientInterface; /** @@ -30,6 +32,7 @@ public function __construct( private readonly RequestStack $requestStack, #[Autowire('@security.access_token_extractor.header')] private readonly AccessTokenExtractorInterface $accessTokenExtractor, + private ?LoggerInterface $logger = null, ) { } @@ -67,7 +70,14 @@ protected function voteOnAttribute(string $attribute, mixed $subject, TokenInter ]); return $response->toArray()['result'] ?? false; - } catch (ExceptionInterface) { + } catch (HttpExceptionInterface) { + return false; + } catch (ExceptionInterface $e) { + $this->logger?->error('An error occurred while checking the permissions on OIDC server.', [ + 'error' => $e->getMessage(), + 'trace' => $e->getTraceAsString(), + ]); + return false; } } diff --git a/api/src/Security/Voter/OIDCRoleVoter.php b/api/src/Security/Voter/OIDCRoleVoter.php index 55f5fb53e..de21486a8 100644 --- a/api/src/Security/Voter/OIDCRoleVoter.php +++ b/api/src/Security/Voter/OIDCRoleVoter.php @@ -5,6 +5,7 @@ namespace App\Security\Voter; use Jose\Component\Signature\Serializer\JWSSerializerManager; +use Psr\Log\LoggerInterface; use Symfony\Component\DependencyInjection\Attribute\Autowire; use Symfony\Component\HttpFoundation\RequestStack; use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; @@ -34,6 +35,7 @@ public function __construct( private readonly AccessTokenExtractorInterface $accessTokenExtractor, #[Autowire('@jose.jws_serializer.oidc')] private readonly JWSSerializerManager $jwsSerializerManager, + private ?LoggerInterface $logger = null, ) { } @@ -63,10 +65,15 @@ protected function voteOnAttribute(string $attribute, mixed $subject, TokenInter ]); $roles = array_map(static fn (string $role): string => strtolower($role), $response->toArray()['realm_access']['roles'] ?? []); - } catch (HttpExceptionInterface) { + } catch (HttpExceptionInterface $e) { // OIDC server said no! return false; - } catch (ExceptionInterface) { + } catch (ExceptionInterface $e) { + $this->logger?->error('An error occurred while checking the roles on OIDC server.', [ + 'error' => $e->getMessage(), + 'trace' => $e->getTraceAsString(), + ]); + // OIDC server doesn't seem to answer: check roles in token (if present) $jws = $this->jwsSerializerManager->unserialize($accessToken); $claims = json_decode($jws->getPayload(), true); From da8d37dc2b45c27410f078eddadc7f16da361b8f Mon Sep 17 00:00:00 2001 From: Vincent Chalamon <407859+vincentchalamon@users.noreply.github.com> Date: Fri, 15 Mar 2024 10:34:44 +0100 Subject: [PATCH 4/8] fix: review --- api/composer.json | 1 - api/config/packages/framework.yaml | 1 + api/config/packages/security.yaml | 8 +-- api/frankenphp/conf.d/app.ini | 1 + api/src/Entity/Book.php | 2 +- api/src/Entity/Bookmark.php | 4 +- api/src/Entity/Review.php | 8 +-- api/src/Entity/User.php | 6 +- .../Oidc/OidcDiscoveryTokenHandler.php | 2 +- api/src/Security/Voter/OIDCVoterTrait.php | 34 ----------- api/src/Security/Voter/OidcRoleVoter.php | 59 +++++++++++++++++++ ...r.php => OidcTokenIntrospectRoleVoter.php} | 45 +++++++------- ...Voter.php => OidcTokenPermissionVoter.php} | 37 +++++++----- api/src/Security/Voter/OidcVoter.php | 37 ++++++++++++ api/tests/Api/Admin/BookTest.php | 2 +- api/tests/Api/Security/TokenGenerator.php | 3 + 16 files changed, 159 insertions(+), 91 deletions(-) delete mode 100644 api/src/Security/Voter/OIDCVoterTrait.php create mode 100644 api/src/Security/Voter/OidcRoleVoter.php rename api/src/Security/Voter/{OIDCRoleVoter.php => OidcTokenIntrospectRoleVoter.php} (70%) rename api/src/Security/Voter/{OIDCPermissionVoter.php => OidcTokenPermissionVoter.php} (81%) create mode 100644 api/src/Security/Voter/OidcVoter.php diff --git a/api/composer.json b/api/composer.json index fd8d6e3e8..4f905eaec 100644 --- a/api/composer.json +++ b/api/composer.json @@ -35,7 +35,6 @@ "symfony/validator": "7.0.*", "symfony/yaml": "7.0.*", "web-token/jwt-bundle": "^3.3", - "web-token/jwt-library": "^3.3", "webonyx/graphql-php": "^15.8", "zenstruck/foundry": "^1.36" }, diff --git a/api/config/packages/framework.yaml b/api/config/packages/framework.yaml index 2610ba9e5..ec3dcaa22 100644 --- a/api/config/packages/framework.yaml +++ b/api/config/packages/framework.yaml @@ -26,6 +26,7 @@ framework: http_client: scoped_clients: + # use scoped client to ease mock on functional tests security.authorization.client: base_uri: '%env(OIDC_SERVER_URL_INTERNAL)%/' diff --git a/api/config/packages/security.yaml b/api/config/packages/security.yaml index 653cf9442..992f66fef 100644 --- a/api/config/packages/security.yaml +++ b/api/config/packages/security.yaml @@ -29,13 +29,13 @@ when@prod: &prod main: access_token: token_handler: App\Security\Http\AccessToken\Oidc\OidcDiscoveryTokenHandler - # todo support Discovery in Symfony -# oidc: -# claim: 'email' + # todo support Discovery in Symfony +# oidc: +# claim: 'email' # base_uri: '%env(OIDC_SERVER_URL)%' # audience: '%env(OIDC_AUD)%' # cache: '@cache.app' # default -# cache_ttl: 3600 # default +# cache_ttl: 600 # default when@dev: *prod diff --git a/api/frankenphp/conf.d/app.ini b/api/frankenphp/conf.d/app.ini index 79a17dd81..ebaf594fc 100644 --- a/api/frankenphp/conf.d/app.ini +++ b/api/frankenphp/conf.d/app.ini @@ -3,6 +3,7 @@ date.timezone = UTC apc.enable_cli = 1 session.use_strict_mode = 1 zend.detect_unicode = 0 +memory_limit = 256M ; https://symfony.com/doc/current/performance.html realpath_cache_size = 4096K diff --git a/api/src/Entity/Book.php b/api/src/Entity/Book.php index 7eb9b6d66..d1977e719 100644 --- a/api/src/Entity/Book.php +++ b/api/src/Entity/Book.php @@ -71,7 +71,7 @@ AbstractNormalizer::GROUPS => ['Book:write'], ], collectDenormalizationErrors: true, - security: 'is_granted("ADMIN")' + security: 'is_granted("OIDC_ADMIN")' )] #[ApiResource( types: ['https://schema.org/Book', 'https://schema.org/Offer'], diff --git a/api/src/Entity/Bookmark.php b/api/src/Entity/Bookmark.php index fde869e92..1e315e849 100644 --- a/api/src/Entity/Bookmark.php +++ b/api/src/Entity/Bookmark.php @@ -36,7 +36,7 @@ operations: [ new GetCollection(), new Delete( - security: 'object.user == user' + security: 'object.user === user' ), new Post( processor: BookmarkPersistProcessor::class @@ -54,7 +54,7 @@ ], collectDenormalizationErrors: true, mercure: true, - security: 'is_granted("USER")' + security: 'is_granted("OIDC_USER")' )] #[ORM\Entity(repositoryClass: BookmarkRepository::class)] #[ORM\UniqueConstraint(fields: ['user', 'book'])] diff --git a/api/src/Entity/Review.php b/api/src/Entity/Review.php index 442e8a504..f57e9d9ae 100644 --- a/api/src/Entity/Review.php +++ b/api/src/Entity/Review.php @@ -76,7 +76,7 @@ AbstractNormalizer::GROUPS => ['Review:write', 'Review:write:admin'], ], collectDenormalizationErrors: true, - security: 'is_granted("ADMIN")' + security: 'is_granted("OIDC_ADMIN")' )] #[ApiResource( types: ['https://schema.org/Review'], @@ -98,7 +98,7 @@ ] ), new Post( - security: 'is_granted("USER")', + security: 'is_granted("OIDC_USER")', // Mercure publish is done manually in MercureProcessor through ReviewPersistProcessor processor: ReviewPersistProcessor::class, provider: CreateProvider::class, @@ -111,7 +111,7 @@ 'bookId' => new Link(toProperty: 'book', fromClass: Book::class), 'id' => new Link(fromClass: Review::class), ], - security: 'object.user == user or is_granted("ADMIN")', + security: 'object.user === user', // Mercure publish is done manually in MercureProcessor through ReviewPersistProcessor processor: ReviewPersistProcessor::class ), @@ -121,7 +121,7 @@ 'bookId' => new Link(toProperty: 'book', fromClass: Book::class), 'id' => new Link(fromClass: Review::class), ], - security: 'object.user == user or is_granted("ADMIN")', + security: 'object.user === user', // Mercure publish is done manually in MercureProcessor through ReviewRemoveProcessor processor: ReviewRemoveProcessor::class ), diff --git a/api/src/Entity/User.php b/api/src/Entity/User.php index cc79ac1a5..48b4df4cc 100644 --- a/api/src/Entity/User.php +++ b/api/src/Entity/User.php @@ -30,17 +30,17 @@ new GetCollection( uriTemplate: '/admin/users{._format}', itemUriTemplate: '/admin/users/{id}{._format}', - security: 'is_granted("ADMIN")', + security: 'is_granted("OIDC_ADMIN")', filters: ['app.filter.user.admin.name'], paginationClientItemsPerPage: true ), new Get( uriTemplate: '/admin/users/{id}{._format}', - security: 'is_granted("ADMIN")' + security: 'is_granted("OIDC_ADMIN")' ), new Get( uriTemplate: '/users/{id}{._format}', - security: 'user.sub === object.sub' + security: 'object === user' ), ], normalizationContext: [ diff --git a/api/src/Security/Http/AccessToken/Oidc/OidcDiscoveryTokenHandler.php b/api/src/Security/Http/AccessToken/Oidc/OidcDiscoveryTokenHandler.php index bbd53f08e..6bf7dbe3d 100644 --- a/api/src/Security/Http/AccessToken/Oidc/OidcDiscoveryTokenHandler.php +++ b/api/src/Security/Http/AccessToken/Oidc/OidcDiscoveryTokenHandler.php @@ -33,7 +33,7 @@ public function __construct( private JWSLoader $jwsLoader, private readonly HttpClientInterface $securityAuthorizationClient, private string $claim = 'email', - private int $ttl = 3600, + private int $ttl = 600, private ?LoggerInterface $logger = null, ) { } diff --git a/api/src/Security/Voter/OIDCVoterTrait.php b/api/src/Security/Voter/OIDCVoterTrait.php deleted file mode 100644 index ce03ddae2..000000000 --- a/api/src/Security/Voter/OIDCVoterTrait.php +++ /dev/null @@ -1,34 +0,0 @@ -getUser() instanceof UserInterface) { - return false; - } - - $request = $this->requestStack->getCurrentRequest(); - - // user is authenticated, its token should be valid (validated through AccessTokenAuthenticator) - // todo is there a better way to retrieve the access-token? - $accessToken = $this->accessTokenExtractor->extractAccessToken($request); - if (!$accessToken) { - return false; - } - - return $accessToken; - } -} diff --git a/api/src/Security/Voter/OidcRoleVoter.php b/api/src/Security/Voter/OidcRoleVoter.php new file mode 100644 index 000000000..a09948769 --- /dev/null +++ b/api/src/Security/Voter/OidcRoleVoter.php @@ -0,0 +1,59 @@ +getUser() instanceof UserInterface) { + return false; + } + + $accessToken = $this->getToken(); + if (!$accessToken) { + return false; + } + + // OIDC server doesn't seem to answer: check roles in token (if present) + $jws = $this->jwsSerializerManager->unserialize($accessToken); + $claims = json_decode($jws->getPayload(), true); + $roles = array_map(static fn (string $role): string => strtolower($role), $claims['realm_access']['roles'] ?? []); + + return \in_array(strtolower(substr($attribute, 5)), $roles, true); + } +} diff --git a/api/src/Security/Voter/OIDCRoleVoter.php b/api/src/Security/Voter/OidcTokenIntrospectRoleVoter.php similarity index 70% rename from api/src/Security/Voter/OIDCRoleVoter.php rename to api/src/Security/Voter/OidcTokenIntrospectRoleVoter.php index de21486a8..f81df1619 100644 --- a/api/src/Security/Voter/OIDCRoleVoter.php +++ b/api/src/Security/Voter/OidcTokenIntrospectRoleVoter.php @@ -4,55 +4,56 @@ namespace App\Security\Voter; -use Jose\Component\Signature\Serializer\JWSSerializerManager; use Psr\Log\LoggerInterface; use Symfony\Component\DependencyInjection\Attribute\Autowire; use Symfony\Component\HttpFoundation\RequestStack; use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; -use Symfony\Component\Security\Core\Authorization\Voter\Voter; +use Symfony\Component\Security\Core\User\UserInterface; use Symfony\Component\Security\Http\AccessToken\AccessTokenExtractorInterface; use Symfony\Contracts\HttpClient\Exception\ExceptionInterface; use Symfony\Contracts\HttpClient\Exception\HttpExceptionInterface; use Symfony\Contracts\HttpClient\HttpClientInterface; /** - * Check user roles. + * Check user roles from OIDC server. * * @see https://www.keycloak.org/docs/latest/authorization_services/index.html#obtaining-information-about-an-rpt */ -final class OIDCRoleVoter extends Voter +final class OidcTokenIntrospectRoleVoter extends OidcVoter { - use OIDCVoterTrait; - public function __construct( + RequestStack $requestStack, + #[Autowire('@security.access_token_extractor.header')] + AccessTokenExtractorInterface $accessTokenExtractor, #[Autowire('%env(OIDC_API_CLIENT_ID)%')] private readonly string $oidcClientId, #[Autowire('%env(OIDC_API_CLIENT_SECRET)%')] private readonly string $oidcClientSecret, private readonly HttpClientInterface $securityAuthorizationClient, - private readonly RequestStack $requestStack, - #[Autowire('@security.access_token_extractor.header')] - private readonly AccessTokenExtractorInterface $accessTokenExtractor, - #[Autowire('@jose.jws_serializer.oidc')] - private readonly JWSSerializerManager $jwsSerializerManager, private ?LoggerInterface $logger = null, ) { + parent::__construct($requestStack, $accessTokenExtractor); } protected function supports(string $attribute, mixed $subject): bool { - return empty($subject); + return str_starts_with($attribute, 'OIDC_INTROSPECT_') && empty($subject); } protected function voteOnAttribute(string $attribute, mixed $subject, TokenInterface $token): bool { - $accessToken = $this->getToken($token); - if (!$accessToken) { + if (!empty($subject)) { + throw new \InvalidArgumentException(sprintf('Invalid subject type, expected empty string or "null", got "%s".', get_debug_type($subject))); + } + + // ensure user is authenticated + if (!$token->getUser() instanceof UserInterface) { return false; } - if (!empty($subject)) { - throw new \InvalidArgumentException(sprintf('Invalid subject type, expected empty string or "null", got "%s".', get_debug_type($subject))); + $accessToken = $this->getToken(); + if (!$accessToken) { + return false; } try { @@ -65,21 +66,17 @@ protected function voteOnAttribute(string $attribute, mixed $subject, TokenInter ]); $roles = array_map(static fn (string $role): string => strtolower($role), $response->toArray()['realm_access']['roles'] ?? []); - } catch (HttpExceptionInterface $e) { + + return \in_array(strtolower(substr($attribute, 5)), $roles, true); + } catch (HttpExceptionInterface) { // OIDC server said no! - return false; } catch (ExceptionInterface $e) { $this->logger?->error('An error occurred while checking the roles on OIDC server.', [ 'error' => $e->getMessage(), 'trace' => $e->getTraceAsString(), ]); - - // OIDC server doesn't seem to answer: check roles in token (if present) - $jws = $this->jwsSerializerManager->unserialize($accessToken); - $claims = json_decode($jws->getPayload(), true); - $roles = array_map(static fn (string $role): string => strtolower($role), $claims['realm_access']['roles'] ?? []); } - return \in_array(strtolower($attribute), $roles, true); + return false; } } diff --git a/api/src/Security/Voter/OIDCPermissionVoter.php b/api/src/Security/Voter/OidcTokenPermissionVoter.php similarity index 81% rename from api/src/Security/Voter/OIDCPermissionVoter.php rename to api/src/Security/Voter/OidcTokenPermissionVoter.php index ef3d95b85..49372735f 100644 --- a/api/src/Security/Voter/OIDCPermissionVoter.php +++ b/api/src/Security/Voter/OidcTokenPermissionVoter.php @@ -9,7 +9,7 @@ use Symfony\Component\DependencyInjection\Attribute\Autowire; use Symfony\Component\HttpFoundation\RequestStack; use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; -use Symfony\Component\Security\Core\Authorization\Voter\Voter; +use Symfony\Component\Security\Core\User\UserInterface; use Symfony\Component\Security\Http\AccessToken\AccessTokenExtractorInterface; use Symfony\Contracts\HttpClient\Exception\ExceptionInterface; use Symfony\Contracts\HttpClient\Exception\HttpExceptionInterface; @@ -20,34 +20,29 @@ * * @see https://www.keycloak.org/docs/latest/authorization_services/index.html#_service_obtaining_permissions */ -final class OIDCPermissionVoter extends Voter +final class OidcTokenPermissionVoter extends OidcVoter { - use OIDCVoterTrait; - public function __construct( + RequestStack $requestStack, + #[Autowire('@security.access_token_extractor.header')] + AccessTokenExtractorInterface $accessTokenExtractor, #[Autowire('%env(OIDC_API_CLIENT_ID)%')] private readonly string $oidcClientId, private readonly HttpClientInterface $securityAuthorizationClient, private readonly IriConverterInterface $iriConverter, - private readonly RequestStack $requestStack, - #[Autowire('@security.access_token_extractor.header')] - private readonly AccessTokenExtractorInterface $accessTokenExtractor, private ?LoggerInterface $logger = null, ) { + parent::__construct($requestStack, $accessTokenExtractor); } protected function supports(string $attribute, mixed $subject): bool { - return !empty($subject); + // todo find a feature requiring this voter + return str_starts_with($attribute, 'OIDC_') && !empty($subject); } protected function voteOnAttribute(string $attribute, mixed $subject, TokenInterface $token): bool { - $accessToken = $this->getToken($token); - if (!$accessToken) { - return false; - } - if (\is_object($subject)) { $subject = $this->iriConverter->getIriFromResource($subject); } @@ -56,6 +51,16 @@ protected function voteOnAttribute(string $attribute, mixed $subject, TokenInter throw new \InvalidArgumentException(sprintf('Invalid subject type, expected "string" or "object", got "%s".', get_debug_type($subject))); } + // ensure user is authenticated + if (!$token->getUser() instanceof UserInterface) { + return false; + } + + $accessToken = $this->getToken(); + if (!$accessToken) { + return false; + } + try { $response = $this->securityAuthorizationClient->request('POST', 'protocol/openid-connect/token', [ 'auth_bearer' => $accessToken, @@ -71,14 +76,14 @@ protected function voteOnAttribute(string $attribute, mixed $subject, TokenInter return $response->toArray()['result'] ?? false; } catch (HttpExceptionInterface) { - return false; + // OIDC server said no! } catch (ExceptionInterface $e) { $this->logger?->error('An error occurred while checking the permissions on OIDC server.', [ 'error' => $e->getMessage(), 'trace' => $e->getTraceAsString(), ]); - - return false; } + + return false; } } diff --git a/api/src/Security/Voter/OidcVoter.php b/api/src/Security/Voter/OidcVoter.php new file mode 100644 index 000000000..c435b5de5 --- /dev/null +++ b/api/src/Security/Voter/OidcVoter.php @@ -0,0 +1,37 @@ +requestStack->getCurrentRequest(); + + // user is authenticated, its token should be valid (validated through AccessTokenAuthenticator) + $accessToken = $this->accessTokenExtractor->extractAccessToken($request); + if (!$accessToken) { + throw new TokenNotFoundException(); + } + + return $accessToken; + } +} diff --git a/api/tests/Api/Admin/BookTest.php b/api/tests/Api/Admin/BookTest.php index e2b0f6785..3773c2aaa 100644 --- a/api/tests/Api/Admin/BookTest.php +++ b/api/tests/Api/Admin/BookTest.php @@ -33,7 +33,7 @@ final class BookTest extends ApiTestCase protected function setup(): void { - $this->client = self::createClient(); + $this->client = self::createClient(['debug' => true]); } #[Test] diff --git a/api/tests/Api/Security/TokenGenerator.php b/api/tests/Api/Security/TokenGenerator.php index 773454f9e..4c28a5669 100644 --- a/api/tests/Api/Security/TokenGenerator.php +++ b/api/tests/Api/Security/TokenGenerator.php @@ -56,6 +56,9 @@ public function generateToken(array $claims): string if (empty($claims['exp'])) { $claims['exp'] = $time + 3600; } + if (empty($claims['realm_access']) || empty($claims['realm_access']['roles'])) { + $claims['realm_access']['roles'] = ['chuck.norris@example.com' === ($claims['email'] ?? null) ? 'admin' : 'user']; + } return $this->jwsSerializerManager->serialize( name: 'jws_compact', From bfb38b94db9c8e6abaacdc621d63a0b0705e2ec0 Mon Sep 17 00:00:00 2001 From: Vincent Chalamon <407859+vincentchalamon@users.noreply.github.com> Date: Tue, 19 Mar 2024 20:52:16 +0100 Subject: [PATCH 5/8] chore: configure owner policy + check permission from the pwa --- api/config/packages/security.yaml | 2 +- .../DataFixtures/Factory/ReviewFactory.php | 18 ++- api/src/Entity/Review.php | 7 +- api/src/Entity/User.php | 2 +- .../Protection/ResourceHandlerInterface.php | 31 +++++ .../Protection/ResourceResourceHandler.php | 127 ++++++++++++++++++ .../Voter/OidcTokenPermissionVoter.php | 1 - .../Processor/ReviewPersistProcessor.php | 14 +- .../State/Processor/ReviewRemoveProcessor.php | 11 +- compose.yaml | 3 + helm/api-platform/keycloak/Dockerfile | 1 + .../keycloak/config/realm-demo.json | 67 ++------- .../api-platform/keycloak/providers/README.md | 12 ++ .../keycloak/providers/owner-policy.jar | Bin 0 -> 757 bytes .../owner/META-INF/keycloak-scripts.json | 9 ++ .../keycloak/providers/owner/owner-policy.js | 8 ++ helm/api-platform/templates/configmap.yaml | 1 + .../templates/pwa-deployment.yaml | 5 + helm/api-platform/values.yaml | 1 + pwa/app/auth.tsx | 19 +-- pwa/app/bookmarks/page.tsx | 3 +- pwa/app/books/[id]/[slug]/page.tsx | 10 +- pwa/app/books/page.tsx | 3 +- pwa/components/admin/book/BookInput.tsx | 2 +- pwa/components/review/Item.tsx | 8 +- pwa/config/keycloak.ts | 1 + pwa/tests/admin/pages/AbstractPage.ts | 2 +- pwa/tests/pages/AbstractPage.ts | 2 +- pwa/utils/book.ts | 2 +- pwa/utils/review.ts | 46 +++++++ 30 files changed, 324 insertions(+), 94 deletions(-) create mode 100644 api/src/Security/Http/Protection/ResourceHandlerInterface.php create mode 100644 api/src/Security/Http/Protection/ResourceResourceHandler.php create mode 100644 helm/api-platform/keycloak/providers/README.md create mode 100644 helm/api-platform/keycloak/providers/owner-policy.jar create mode 100644 helm/api-platform/keycloak/providers/owner/META-INF/keycloak-scripts.json create mode 100644 helm/api-platform/keycloak/providers/owner/owner-policy.js create mode 100644 pwa/utils/review.ts diff --git a/api/config/packages/security.yaml b/api/config/packages/security.yaml index 992f66fef..701d910cd 100644 --- a/api/config/packages/security.yaml +++ b/api/config/packages/security.yaml @@ -46,7 +46,7 @@ when@test: access_token: token_handler: oidc: - claim: email + claim: 'email' audience: '%env(OIDC_AUD)%' issuers: [ '%env(OIDC_SERVER_URL)%' ] algorithm: 'ES256' diff --git a/api/src/DataFixtures/Factory/ReviewFactory.php b/api/src/DataFixtures/Factory/ReviewFactory.php index d22a35749..dca75a65e 100644 --- a/api/src/DataFixtures/Factory/ReviewFactory.php +++ b/api/src/DataFixtures/Factory/ReviewFactory.php @@ -5,6 +5,7 @@ namespace App\DataFixtures\Factory; use App\Entity\Review; +use App\Security\Http\Protection\ResourceHandlerInterface; use Doctrine\ORM\EntityRepository; use Zenstruck\Foundry\ModelFactory; use Zenstruck\Foundry\Proxy; @@ -52,8 +53,9 @@ final class ReviewFactory extends ModelFactory /** * @see https://symfony.com/bundles/ZenstruckFoundryBundle/current/index.html#factories-as-services */ - public function __construct() - { + public function __construct( + private readonly ResourceHandlerInterface $resourceHandler, + ) { parent::__construct(); } @@ -76,7 +78,17 @@ protected function getDefaults(): array */ protected function initialize(): self { - return $this; + return $this + // create the resource on the OIDC server + ->afterPersist(function (Review $object): void { + // project specification: only create resource on OIDC server for known users (john.doe and chuck.norris) + if (\in_array($object->user?->email, ['john.doe@example.com', 'chuck.norris@example.com'], true)) { + $this->resourceHandler->create($object, $object->user, [ + 'operation_name' => '/books/{bookId}/reviews/{id}{._format}', + ]); + } + }) + ; // ->afterInstantiate(function(Review $review): void {}) } diff --git a/api/src/Entity/Review.php b/api/src/Entity/Review.php index f57e9d9ae..9c53b0b7b 100644 --- a/api/src/Entity/Review.php +++ b/api/src/Entity/Review.php @@ -16,6 +16,7 @@ use ApiPlatform\Metadata\Put; use ApiPlatform\State\CreateProvider; use App\Repository\ReviewRepository; +use App\Security\Voter\OidcTokenPermissionVoter; use App\Serializer\IriTransformerNormalizer; use App\State\Processor\ReviewPersistProcessor; use App\State\Processor\ReviewRemoveProcessor; @@ -111,7 +112,8 @@ 'bookId' => new Link(toProperty: 'book', fromClass: Book::class), 'id' => new Link(fromClass: Review::class), ], - security: 'object.user === user', + /** @see OidcTokenPermissionVoter */ + security: 'is_granted("OIDC_USER", request.getRequestUri())', // Mercure publish is done manually in MercureProcessor through ReviewPersistProcessor processor: ReviewPersistProcessor::class ), @@ -121,7 +123,8 @@ 'bookId' => new Link(toProperty: 'book', fromClass: Book::class), 'id' => new Link(fromClass: Review::class), ], - security: 'object.user === user', + /** @see OidcTokenPermissionVoter */ + security: 'is_granted("OIDC_USER", request.getRequestUri())', // Mercure publish is done manually in MercureProcessor through ReviewRemoveProcessor processor: ReviewRemoveProcessor::class ), diff --git a/api/src/Entity/User.php b/api/src/Entity/User.php index 48b4df4cc..3445febf6 100644 --- a/api/src/Entity/User.php +++ b/api/src/Entity/User.php @@ -67,7 +67,7 @@ class User implements UserInterface * @see https://schema.org/identifier */ #[ApiProperty(types: ['https://schema.org/identifier'])] - #[Groups(groups: ['User:read', 'Review:read'])] + #[Groups(groups: ['User:read'])] #[ORM\Column(type: UuidType::NAME, unique: true)] public ?Uuid $sub = null; diff --git a/api/src/Security/Http/Protection/ResourceHandlerInterface.php b/api/src/Security/Http/Protection/ResourceHandlerInterface.php new file mode 100644 index 000000000..b9776bb3e --- /dev/null +++ b/api/src/Security/Http/Protection/ResourceHandlerInterface.php @@ -0,0 +1,31 @@ +resourceMetadataCollectionFactory->create(resourceClass: $resource::class)->getOperation( + operationName: $context['operation_name'] ?? null, + httpOperation: true, + ); + $shortName = strtolower(preg_replace('~(?<=\\w)([A-Z])~', '-$1', $operation->getShortName())); + $resourceIri = $this->iriConverter->getIriFromResource( + resource: $resource, + referenceType: UrlGeneratorInterface::ABS_PATH, + operation: $operation, + ); + + // create resource_set on OIDC server + $this->securityAuthorizationClient->request('POST', $this->getResourceRegistrationEndpoint(), [ + 'auth_bearer' => $this->getPAT(), + 'json' => [ + 'name' => sprintf('%s_%s', $shortName, $resource->getId()->__toString()), + 'displayName' => sprintf('%s #%s', $operation->getShortName(), $resource->getId()->__toString()), + 'uris' => [$resourceIri], + 'type' => sprintf('urn:%s:resources:%s', $this->oidcClientId, $shortName), + 'owner' => $owner->getUserIdentifier(), + ], + ]); + } + + public function delete(object $resource, UserInterface $owner, array $context = []): void + { + $operation = $this->resourceMetadataCollectionFactory->create(resourceClass: $resource::class)->getOperation( + operationName: $context['operation_name'] ?? null, + httpOperation: true, + ); + $shortName = strtolower(preg_replace('~(?<=\\w)([A-Z])~', '-$1', $operation->getShortName())); + $resourceIri = $this->iriConverter->getIriFromResource( + resource: $resource, + referenceType: UrlGeneratorInterface::ABS_PATH, + operation: $operation, + ); + + // retrieve corresponding resource_set from OIDC server + $response = $this->securityAuthorizationClient->request( + 'GET', + $this->getResourceRegistrationEndpoint(), + [ + 'auth_bearer' => $this->getPAT(), + 'query' => [ + 'deep' => 'true', + 'first' => 0, + 'max' => 1, + 'uri' => $resourceIri, + 'owner' => $owner->getUserIdentifier(), + 'type' => sprintf('urn:%s:resources:%s', $this->oidcClientId, $shortName), + ], + ] + ); + $content = $response->toArray(); + $resourceSet = $content[0]; + + // delete corresponding resource_set on OIDC server + $this->securityAuthorizationClient->request( + 'DELETE', + sprintf('%s/%s', $this->getResourceRegistrationEndpoint(), $resourceSet['_id']), + [ + 'auth_bearer' => $this->getPAT(), + ] + ); + } + + /** + * @see https://www.keycloak.org/docs/latest/authorization_services/index.html#_service_protection_whatis_obtain_pat + */ + private function getPAT(): string + { + $response = $this->securityAuthorizationClient->request('POST', $this->getTokenEndpoint(), [ + 'body' => [ + 'grant_type' => 'client_credentials', + 'client_id' => $this->oidcClientId, + 'client_secret' => $this->oidcClientSecret, + ], + ]); + $content = $response->toArray(); + + return $content['access_token']; + } + + private function getTokenEndpoint(): string + { + $response = $this->securityAuthorizationClient->request('GET', '.well-known/openid-configuration'); + $content = $response->toArray(); + + return $content['token_endpoint']; + } + + private function getResourceRegistrationEndpoint(): string + { + $response = $this->securityAuthorizationClient->request('GET', '.well-known/uma2-configuration'); + $content = $response->toArray(); + + return $content['resource_registration_endpoint']; + } +} diff --git a/api/src/Security/Voter/OidcTokenPermissionVoter.php b/api/src/Security/Voter/OidcTokenPermissionVoter.php index 49372735f..e97b8748d 100644 --- a/api/src/Security/Voter/OidcTokenPermissionVoter.php +++ b/api/src/Security/Voter/OidcTokenPermissionVoter.php @@ -37,7 +37,6 @@ public function __construct( protected function supports(string $attribute, mixed $subject): bool { - // todo find a feature requiring this voter return str_starts_with($attribute, 'OIDC_') && !empty($subject); } diff --git a/api/src/State/Processor/ReviewPersistProcessor.php b/api/src/State/Processor/ReviewPersistProcessor.php index 867433f70..5ffe57613 100644 --- a/api/src/State/Processor/ReviewPersistProcessor.php +++ b/api/src/State/Processor/ReviewPersistProcessor.php @@ -9,6 +9,7 @@ use ApiPlatform\Metadata\Post; use ApiPlatform\State\ProcessorInterface; use App\Entity\Review; +use App\Security\Http\Protection\ResourceHandlerInterface; use Psr\Clock\ClockInterface; use Symfony\Bundle\SecurityBundle\Security; use Symfony\Component\DependencyInjection\Attribute\Autowire; @@ -28,7 +29,8 @@ public function __construct( #[Autowire(service: MercureProcessor::class)] private ProcessorInterface $mercureProcessor, private Security $security, - private ClockInterface $clock + private ClockInterface $clock, + private ResourceHandlerInterface $resourceHandler, ) { } @@ -53,6 +55,16 @@ public function process(mixed $data, Operation $operation, array $uriVariables = // save entity $data = $this->persistProcessor->process($data, $operation, $uriVariables, $context); + // create resource on OIDC server + if ($operation instanceof Post) { + // project specification: only create resource on OIDC server for known users (john.doe and chuck.norris) + if (\in_array($data->user->email, ['john.doe@example.com', 'chuck.norris@example.com'], true)) { + $this->resourceHandler->create($data, $data->user, [ + 'operation_name' => '/books/{bookId}/reviews/{id}{._format}', + ]); + } + } + // publish on Mercure foreach (['/admin/reviews/{id}{._format}', '/books/{bookId}/reviews/{id}{._format}'] as $uriTemplate) { $this->mercureProcessor->process( diff --git a/api/src/State/Processor/ReviewRemoveProcessor.php b/api/src/State/Processor/ReviewRemoveProcessor.php index bf91259e8..a0dccbd3b 100644 --- a/api/src/State/Processor/ReviewRemoveProcessor.php +++ b/api/src/State/Processor/ReviewRemoveProcessor.php @@ -11,6 +11,7 @@ use ApiPlatform\Metadata\Resource\Factory\ResourceMetadataCollectionFactoryInterface; use ApiPlatform\State\ProcessorInterface; use App\Entity\Review; +use App\Security\Http\Protection\ResourceHandlerInterface; use Symfony\Component\DependencyInjection\Attribute\Autowire; /** @@ -28,7 +29,8 @@ public function __construct( #[Autowire(service: MercureProcessor::class)] private ProcessorInterface $mercureProcessor, private ResourceMetadataCollectionFactoryInterface $resourceMetadataCollectionFactory, - private IriConverterInterface $iriConverter + private IriConverterInterface $iriConverter, + private ResourceHandlerInterface $resourceHandler, ) { } @@ -42,6 +44,13 @@ public function process(mixed $data, Operation $operation, array $uriVariables = // remove entity $this->removeProcessor->process($data, $operation, $uriVariables, $context); + // project specification: only delete resource on OIDC server for known users (john.doe and chuck.norris) + if (\in_array($object->user->email, ['john.doe@example.com', 'chuck.norris@example.com'], true)) { + $this->resourceHandler->delete($object, $object->user, [ + 'operation_name' => '/books/{bookId}/reviews/{id}{._format}', + ]); + } + // publish on Mercure foreach (['/admin/reviews/{id}{._format}', '/books/{bookId}/reviews/{id}{._format}'] as $uriTemplate) { $iri = $this->iriConverter->getIriFromResource( diff --git a/compose.yaml b/compose.yaml index c27f3bb4b..4dad87078 100644 --- a/compose.yaml +++ b/compose.yaml @@ -41,6 +41,7 @@ services: OIDC_CLIENT_ID: ${OIDC_CLIENT_ID:-api-platform-pwa} OIDC_SERVER_URL: ${OIDC_SERVER_URL:-https://localhost/oidc/realms/demo} OIDC_SERVER_URL_INTERNAL: ${OIDC_SERVER_URL_INTERNAL:-http://keycloak:8080/oidc/realms/demo} + OIDC_AUTHORIZATION_CLIENT_ID: ${OIDC_AUTHORIZATION_CLIENT_ID:-api-platform-api} NEXT_SHARP_PATH: /srv/app/node_modules/sharp ###> doctrine/doctrine-bundle ### @@ -85,6 +86,8 @@ services: # https://www.keycloak.org/server/hostname KC_HOSTNAME_URL: https://${SERVER_NAME:-localhost}/oidc/ KC_HOSTNAME_ADMIN_URL: https://${SERVER_NAME:-localhost}/oidc/ + # https://www.keycloak.org/server/features + KC_FEATURES: "scripts" depends_on: - keycloak-database ports: diff --git a/helm/api-platform/keycloak/Dockerfile b/helm/api-platform/keycloak/Dockerfile index 75033b3ec..cd85b3465 100644 --- a/helm/api-platform/keycloak/Dockerfile +++ b/helm/api-platform/keycloak/Dockerfile @@ -16,3 +16,4 @@ FROM bitnami/keycloak:23-debian-11 AS keycloak_upstream FROM keycloak_upstream AS keycloak COPY --link themes/api-platform-demo /opt/bitnami/keycloak/themes/api-platform-demo +COPY --link providers/owner-policy.jar /opt/bitnami/keycloak/providers/owner-policy.jar diff --git a/helm/api-platform/keycloak/config/realm-demo.json b/helm/api-platform/keycloak/config/realm-demo.json index 29d7a3918..58a9c1172 100755 --- a/helm/api-platform/keycloak/config/realm-demo.json +++ b/helm/api-platform/keycloak/config/realm-demo.json @@ -49,12 +49,12 @@ }, "users": [ { - "username": "chuck.norris", + "username": "chuck.norris@example.com", + "email": "chuck.norris@example.com", "enabled": true, "emailVerified": true, "firstName": "Chuck", "lastName": "Norris", - "email": "chuck.norris@example.com", "credentials": [ { "type": "password", @@ -66,12 +66,12 @@ ] }, { - "username": "john.doe", + "username": "john.doe@example.com", + "email": "john.doe@example.com", "enabled": true, "emailVerified": true, "firstName": "John", "lastName": "Doe", - "email": "john.doe@example.com", "credentials": [ { "type": "password", @@ -117,66 +117,23 @@ "allowRemoteResourceManagement": true, "policyEnforcementMode": "ENFORCING", "decisionStrategy": "UNANIMOUS", - "resources": [ - { - "name": "Admin Resource", - "ownerManagedAccess": false, - "displayName": "admin-resource", - "uris": [ - "/admin/*" - ] - }, - { - "name": "Bookmark Resource", - "ownerManagedAccess": false, - "displayName": "bookmark-resource", - "uris": [ - "/bookmarks/*" - ] - } - ], "policies": [ { - "name": "Admin Policy", - "description": "Defines that only admin users are allowed", - "type": "role", - "logic": "POSITIVE", - "decisionStrategy": "UNANIMOUS", - "config": { - "roles": "[{\"id\":\"admin\",\"required\":true}]" - } - }, - { - "name": "User Policy", - "description": "Defines that authenticated users are allowed", - "type": "role", - "logic": "POSITIVE", - "decisionStrategy": "UNANIMOUS", - "config": { - "roles": "[{\"id\":\"user\",\"required\":true}]" - } - }, - { - "name": "admin-permission", - "description": "Admin Permission", - "type": "resource", + "name": "Owner Policy", + "description": "Grants access only for users that own a resource (based on \"owner\" resource attribute)", + "type": "script-owner-policy.js", "logic": "POSITIVE", - "decisionStrategy": "UNANIMOUS", - "config": { - "defaultResourceType": "", - "resources": "[\"Admin Resource\"]", - "applyPolicies": "[\"Admin Policy\"]" - } + "decisionStrategy": "AFFIRMATIVE" }, { - "name": "user-permission", - "description": "User Permission", + "name": "Review Owner Permission", + "description": "Ensure only users that own a review resource have access", "type": "resource", "logic": "POSITIVE", "decisionStrategy": "UNANIMOUS", "config": { - "resources": "[\"Bookmark Resource\"]", - "applyPolicies": "[\"User Policy\"]" + "defaultResourceType": "urn:api-platform-api:resources:review", + "applyPolicies": "[\"Owner Policy\"]" } } ] diff --git a/helm/api-platform/keycloak/providers/README.md b/helm/api-platform/keycloak/providers/README.md new file mode 100644 index 000000000..76a607fec --- /dev/null +++ b/helm/api-platform/keycloak/providers/README.md @@ -0,0 +1,12 @@ +# Building Providers + +Keycloak comes with a bunch of providers. + +To create a custom JavaScript Policy (https://www.keycloak.org/docs/24.0.1/server_development/#_script_providers), +it must be packed in a JAR file. + +Build the provider as following: + +```shell +zip -r owner-policy.jar owner/* +``` diff --git a/helm/api-platform/keycloak/providers/owner-policy.jar b/helm/api-platform/keycloak/providers/owner-policy.jar new file mode 100644 index 0000000000000000000000000000000000000000..6343c2aceb01cab939906171d2e6e42fcf59d56e GIT binary patch literal 757 zcmWIWW@h1H0D%u3#Svfzl;C8LVeoYgan$wnbJGtE;bdTT-tjZl6NpPIxEUB(UNAE- zfQbO05h6fM9AG1=>TjN^1@cw{u^h4y*{PMuIr)j%y2Z&wnFS@qdRfK!d2n-GFwJ%C zb>ur_z{B#s_NZ3+1IC!0hnWT1CrDilm~_f7BsBDYpK#-jHBaVVt$52|zH}n<I{}zQ@+^0G1aPa@d^GrDWX9lm?ec9pR(i6?+KL}h3 zQ4xp;+rH$I{A06)iNMF)n8j2@j5wuVHwav7l4=_=&AhjywoDy zg8ZD!1g^S{y<({A467XBK>0B=SnIc8iDD*^Nd2naCz zbp+8!(aZ{oW{fBX8HX#9A;vKR4cgM^jBFf^C?{+OW|$+}Apx`sBL;wmf?@!};jC;R PA2I_W2T( export const { handlers: { GET, POST }, auth } = NextAuth({ callbacks: { // @ts-ignore - async jwt({ token, account, profile }: { token: JWT, account: Account, profile: Profile }): Promise { + async jwt({ token, account }: { token: JWT, account: Account }): Promise { if (account) { // Save the access token and refresh token in the JWT on the initial login return { @@ -62,7 +53,6 @@ export const { handlers: { GET, POST }, auth } = NextAuth({ idToken: account.id_token, expiresAt: Math.floor(Date.now() / 1000 + account.expires_in), refreshToken: account.refresh_token, - sub: profile.sub, }; } else if (Date.now() < token.expiresAt * 1000) { // If the access token has not expired yet, return it @@ -70,7 +60,7 @@ export const { handlers: { GET, POST }, auth } = NextAuth({ } else { // If the access token has expired, try to refresh it try { - const response = await fetch(`${OIDC_SERVER_URL}/protocol/openid-connect/token`, { + const response = await fetch(`${OIDC_SERVER_URL_INTERNAL}/protocol/openid-connect/token`, { headers: { "Content-Type": "application/x-www-form-urlencoded" }, body: new URLSearchParams({ client_id: OIDC_CLIENT_ID, @@ -113,9 +103,6 @@ export const { handlers: { GET, POST }, auth } = NextAuth({ session.accessToken = token.accessToken; session.idToken = token.idToken; session.error = token.error; - if (session?.user && token?.sub) { - session.user.sub = token.sub; - } } return session; diff --git a/pwa/app/bookmarks/page.tsx b/pwa/app/bookmarks/page.tsx index bc5fc9f44..925790b5d 100644 --- a/pwa/app/bookmarks/page.tsx +++ b/pwa/app/bookmarks/page.tsx @@ -17,7 +17,8 @@ export const metadata: Metadata = { async function getServerSideProps({ page = 1 }: Query, session: Session): Promise { try { const response: FetchResponse> | undefined = await fetchApi(`/bookmarks?page=${Number(page)}`, { - next: { revalidate: 3600 }, + // next: { revalidate: 3600 }, + cache: "no-cache", }, session); if (!response?.data) { throw new Error('Unable to retrieve data from /bookmarks.'); diff --git a/pwa/app/books/[id]/[slug]/page.tsx b/pwa/app/books/[id]/[slug]/page.tsx index 455bcb442..b854363eb 100644 --- a/pwa/app/books/[id]/[slug]/page.tsx +++ b/pwa/app/books/[id]/[slug]/page.tsx @@ -12,12 +12,11 @@ interface Props { export async function generateMetadata({ params }: Props): Promise { const id = params.id; - // @ts-ignore - const session: Session|null = await auth(); try { const response: FetchResponse | undefined = await fetchApi(`/books/${id}`, { - next: { revalidate: 3600 }, - }, session); + // next: { revalidate: 3600 }, + cache: "no-cache", + }); if (!response?.data) { throw new Error(`Unable to retrieve data from /books/${id}.`); } @@ -39,7 +38,8 @@ async function getServerSideProps(id: string, session: Session|null): Promise> | undefined = await fetchApi(buildUriFromFilters("/books", filters), { - cache: "force-cache", + // next: { revalidate: 3600 }, + cache: "no-cache", }, session); if (!response?.data) { throw new Error('Unable to retrieve data from /books.'); diff --git a/pwa/components/admin/book/BookInput.tsx b/pwa/components/admin/book/BookInput.tsx index b0905edb5..117415485 100644 --- a/pwa/components/admin/book/BookInput.tsx +++ b/pwa/components/admin/book/BookInput.tsx @@ -24,7 +24,7 @@ const fetchOpenLibrarySearch = async (query: string, signal?: AbortSignal | unde const response = await fetch(`https://openlibrary.org/search.json?q=${query.replace(/ - /, ' ')}&limit=10`, { signal, method: "GET", - cache: "force-cache", + next: { revalidate: 3600 }, }); const results: Search = await response.json(); diff --git a/pwa/components/review/Item.tsx b/pwa/components/review/Item.tsx index 19ce78534..b4086a093 100644 --- a/pwa/components/review/Item.tsx +++ b/pwa/components/review/Item.tsx @@ -7,6 +7,8 @@ import { Error } from "../common/Error"; import { type Review } from "../../types/Review"; import { fetchApi } from "../../utils/dataAccess"; import { Form } from "./Form"; +import {usePermission} from "../../utils/review"; +import {useOpenLibraryBook} from "../../utils/book"; interface Props { review: Review; @@ -22,10 +24,12 @@ interface DeleteParams { const deleteReview = async (id: string, session) => await fetchApi(id, { method: "DELETE" }, session); export const Item: FunctionComponent = ({ review, onDelete, onEdit }) => { - const { data: session } = useSession(); + const { data: session, status } = useSession(); const [data, setData] = useState(review); const [error, setError] = useState(); const [edit, setEdit] = useState(false); + // @ts-ignore + const isGranted = usePermission(data, session); const deleteMutation = useMutation({ mutationFn: async ({ id }: DeleteParams) => deleteReview(id, session), @@ -79,7 +83,7 @@ export const Item: FunctionComponent = ({ review, onDelete, onEdit }) =>

{data["body"]}

{/* @ts-ignore */} - {!!session && !!session?.user?.sub && !!data["user"] && data["user"]["sub"] === session.user.sub && ( + {isGranted && (
{ e.preventDefault(); diff --git a/pwa/config/keycloak.ts b/pwa/config/keycloak.ts index ac26026bd..185a144a9 100644 --- a/pwa/config/keycloak.ts +++ b/pwa/config/keycloak.ts @@ -1,3 +1,4 @@ export const OIDC_CLIENT_ID: string = process.env.OIDC_CLIENT_ID || 'api-platform-pwa'; export const OIDC_SERVER_URL: string = process.env.OIDC_SERVER_URL || 'https://localhost/oidc/realms/demo'; export const OIDC_SERVER_URL_INTERNAL: string = process.env.OIDC_SERVER_URL_INTERNAL || 'http://keycloak:8080/oidc/realms/demo'; +export const OIDC_AUTHORIZATION_CLIENT_ID: string = process.env.OIDC_AUTHORIZATION_CLIENT_ID || 'api-platform-api'; diff --git a/pwa/tests/admin/pages/AbstractPage.ts b/pwa/tests/admin/pages/AbstractPage.ts index a3122b882..8554fcf18 100644 --- a/pwa/tests/admin/pages/AbstractPage.ts +++ b/pwa/tests/admin/pages/AbstractPage.ts @@ -5,7 +5,7 @@ export abstract class AbstractPage { } public async login() { - await this.page.getByLabel("Username or email").fill("chuck.norris@example.com"); + await this.page.getByLabel("Email").fill("chuck.norris@example.com"); await this.page.getByLabel("Password").fill("Pa55w0rd"); await this.page.getByRole("button", { name: "Sign In" }).click(); if (await this.page.getByRole("button", { name: "Sign in with Keycloak" }).count()) { diff --git a/pwa/tests/pages/AbstractPage.ts b/pwa/tests/pages/AbstractPage.ts index 635e027c3..04350e205 100644 --- a/pwa/tests/pages/AbstractPage.ts +++ b/pwa/tests/pages/AbstractPage.ts @@ -5,7 +5,7 @@ export abstract class AbstractPage { } public async login() { - await this.page.getByLabel("Username or email").fill("john.doe@example.com"); + await this.page.getByLabel("Email").fill("john.doe@example.com"); await this.page.getByLabel("Password").fill("Pa55w0rd"); await this.page.getByRole("button", { name: "Sign In" }).click(); if (await this.page.getByRole("button", { name: "Sign in with Keycloak" }).count()) { diff --git a/pwa/utils/book.ts b/pwa/utils/book.ts index 17ac6a147..0ee535258 100644 --- a/pwa/utils/book.ts +++ b/pwa/utils/book.ts @@ -51,7 +51,7 @@ export const useOpenLibraryBook = (data: TData) => { // retrieve data from work if necessary if ((!data["description"] || !data["images"]) && typeof book["works"] !== "undefined" && book["works"].length > 0) { const response = await fetch(`https://openlibrary.org${book["works"][0]["key"]}.json`, { - cache: "force-cache", + next: { revalidate: 3600 }, }); const work: Work = await response.json(); diff --git a/pwa/utils/review.ts b/pwa/utils/review.ts new file mode 100644 index 000000000..b6cdc5808 --- /dev/null +++ b/pwa/utils/review.ts @@ -0,0 +1,46 @@ +import { useEffect, useState } from "react"; + +import { type Session } from "../app/auth"; +import { type Review } from "../types/Review"; +import { OIDC_AUTHORIZATION_CLIENT_ID, OIDC_SERVER_URL } from "../config/keycloak"; + +interface Permission { + result: boolean; +} + +export const usePermission = (review: Review, session: Session|null): boolean => { + const [isGranted, grant] = useState(false); + + useEffect(() => { + if (!session) { + return; + } + + (async () => { + const response = await fetch(`${OIDC_SERVER_URL}/protocol/openid-connect/token`, { + headers: { + "Content-Type": "application/x-www-form-urlencoded", + Authorization: `Bearer ${session?.accessToken}`, + }, + body: new URLSearchParams({ + grant_type: "urn:ietf:params:oauth:grant-type:uma-ticket", + audience: OIDC_AUTHORIZATION_CLIENT_ID, + response_mode: "decision", + permission_resource_format: "uri", + permission_resource_matching_uri: "true", + // @ts-ignore + permission: review["@id"].toString(), + }), + method: "POST", + }); + const permission: Permission = await response.json(); + console.log(permission); + + if (permission.result) { + grant(true); + } + })(); + }, [review]); + + return isGranted; +}; From 5af717dc3b18c06313aea9a1424c8ad86f85bcec Mon Sep 17 00:00:00 2001 From: Vincent Chalamon <407859+vincentchalamon@users.noreply.github.com> Date: Tue, 19 Mar 2024 21:07:21 +0100 Subject: [PATCH 6/8] fix: remove useless User.sub --- api/migrations/Version20230906094949.php | 4 +--- api/src/DataFixtures/Factory/ReviewFactory.php | 6 +++++- api/src/DataFixtures/Factory/UserFactory.php | 2 -- api/src/Entity/User.php | 8 -------- api/src/Security/Core/UserProvider.php | 10 ---------- api/tests/Api/Admin/BookTest.php | 2 +- api/tests/Api/Admin/UserTest.php | 7 +------ .../Api/Admin/schemas/Review/collection.json | 7 ------- api/tests/Api/Admin/schemas/Review/item.json | 7 ------- .../Api/Admin/schemas/User/collection.json | 7 ------- api/tests/Api/Admin/schemas/User/item.json | 7 ------- api/tests/Api/schemas/Review/collection.json | 7 ------- api/tests/Api/schemas/Review/item.json | 7 ------- api/tests/Security/Core/UserProviderTest.php | 10 +--------- .../Processor/ReviewPersistProcessorTest.php | 18 +++++++++++++++++- .../Processor/ReviewRemoveProcessorTest.php | 16 +++++++++++++++- 16 files changed, 41 insertions(+), 84 deletions(-) diff --git a/api/migrations/Version20230906094949.php b/api/migrations/Version20230906094949.php index 4c37dbb6d..de2268c6d 100644 --- a/api/migrations/Version20230906094949.php +++ b/api/migrations/Version20230906094949.php @@ -41,11 +41,9 @@ public function up(Schema $schema): void $this->addSql('COMMENT ON COLUMN review.user_id IS \'(DC2Type:uuid)\''); $this->addSql('COMMENT ON COLUMN review.book_id IS \'(DC2Type:uuid)\''); $this->addSql('COMMENT ON COLUMN review.published_at IS \'(DC2Type:datetime_immutable)\''); - $this->addSql('CREATE TABLE "user" (id UUID NOT NULL, sub UUID NOT NULL, email VARCHAR(255) NOT NULL, first_name VARCHAR(255) NOT NULL, last_name VARCHAR(255) NOT NULL, roles JSON NOT NULL, PRIMARY KEY(id))'); - $this->addSql('CREATE UNIQUE INDEX UNIQ_8D93D649580282DC ON "user" (sub)'); + $this->addSql('CREATE TABLE "user" (id UUID NOT NULL, email VARCHAR(255) NOT NULL, first_name VARCHAR(255) NOT NULL, last_name VARCHAR(255) NOT NULL, roles JSON NOT NULL, PRIMARY KEY(id))'); $this->addSql('CREATE UNIQUE INDEX UNIQ_8D93D649E7927C74 ON "user" (email)'); $this->addSql('COMMENT ON COLUMN "user".id IS \'(DC2Type:uuid)\''); - $this->addSql('COMMENT ON COLUMN "user".sub IS \'(DC2Type:uuid)\''); $this->addSql('ALTER TABLE bookmark ADD CONSTRAINT FK_DA62921DA76ED395 FOREIGN KEY (user_id) REFERENCES "user" (id) NOT DEFERRABLE INITIALLY IMMEDIATE'); $this->addSql('ALTER TABLE bookmark ADD CONSTRAINT FK_DA62921D16A2B381 FOREIGN KEY (book_id) REFERENCES book (id) NOT DEFERRABLE INITIALLY IMMEDIATE'); $this->addSql('ALTER TABLE review ADD CONSTRAINT FK_794381C6A76ED395 FOREIGN KEY (user_id) REFERENCES "user" (id) NOT DEFERRABLE INITIALLY IMMEDIATE'); diff --git a/api/src/DataFixtures/Factory/ReviewFactory.php b/api/src/DataFixtures/Factory/ReviewFactory.php index dca75a65e..4ee375c5b 100644 --- a/api/src/DataFixtures/Factory/ReviewFactory.php +++ b/api/src/DataFixtures/Factory/ReviewFactory.php @@ -54,7 +54,7 @@ final class ReviewFactory extends ModelFactory * @see https://symfony.com/bundles/ZenstruckFoundryBundle/current/index.html#factories-as-services */ public function __construct( - private readonly ResourceHandlerInterface $resourceHandler, + private readonly ?ResourceHandlerInterface $resourceHandler = null, ) { parent::__construct(); } @@ -81,6 +81,10 @@ protected function initialize(): self return $this // create the resource on the OIDC server ->afterPersist(function (Review $object): void { + if (!$this->resourceHandler) { + return; + } + // project specification: only create resource on OIDC server for known users (john.doe and chuck.norris) if (\in_array($object->user?->email, ['john.doe@example.com', 'chuck.norris@example.com'], true)) { $this->resourceHandler->create($object, $object->user, [ diff --git a/api/src/DataFixtures/Factory/UserFactory.php b/api/src/DataFixtures/Factory/UserFactory.php index b87c824e3..641114a55 100644 --- a/api/src/DataFixtures/Factory/UserFactory.php +++ b/api/src/DataFixtures/Factory/UserFactory.php @@ -6,7 +6,6 @@ use App\Entity\User; use Doctrine\ORM\EntityRepository; -use Symfony\Component\Uid\Uuid; use Zenstruck\Foundry\ModelFactory; use Zenstruck\Foundry\Proxy; use Zenstruck\Foundry\RepositoryProxy; @@ -76,7 +75,6 @@ public function withAdmin(): self protected function getDefaults(): array { return [ - 'sub' => Uuid::v7(), 'email' => self::faker()->unique()->email(), 'firstName' => self::faker()->firstName(), 'lastName' => self::faker()->lastName(), diff --git a/api/src/Entity/User.php b/api/src/Entity/User.php index 3445febf6..74a10633b 100644 --- a/api/src/Entity/User.php +++ b/api/src/Entity/User.php @@ -63,14 +63,6 @@ class User implements UserInterface #[ORM\Id] private ?Uuid $id = null; - /** - * @see https://schema.org/identifier - */ - #[ApiProperty(types: ['https://schema.org/identifier'])] - #[Groups(groups: ['User:read'])] - #[ORM\Column(type: UuidType::NAME, unique: true)] - public ?Uuid $sub = null; - /** * @see https://schema.org/email */ diff --git a/api/src/Security/Core/UserProvider.php b/api/src/Security/Core/UserProvider.php index 55b27e36e..c0c641649 100644 --- a/api/src/Security/Core/UserProvider.php +++ b/api/src/Security/Core/UserProvider.php @@ -10,7 +10,6 @@ use Symfony\Component\Security\Core\Exception\UnsupportedUserException; use Symfony\Component\Security\Core\User\AttributesBasedUserProviderInterface; use Symfony\Component\Security\Core\User\UserInterface; -use Symfony\Component\Uid\Uuid; /** * @implements AttributesBasedUserProviderInterface @@ -46,15 +45,6 @@ public function loadUserByIdentifier(string $identifier, array $attributes = []) $user = $this->repository->findOneBy(['email' => $identifier]) ?: new User(); $user->email = $identifier; - if (!isset($attributes['sub'])) { - throw new UnsupportedUserException('Property "sub" is missing in token attributes.'); - } - try { - $user->sub = Uuid::fromString($attributes['sub']); - } catch (\Throwable $e) { - throw new UnsupportedUserException($e->getMessage(), $e->getCode(), $e); - } - if (!isset($attributes['given_name'])) { throw new UnsupportedUserException('Property "given_name" is missing in token attributes.'); } diff --git a/api/tests/Api/Admin/BookTest.php b/api/tests/Api/Admin/BookTest.php index 3773c2aaa..e2b0f6785 100644 --- a/api/tests/Api/Admin/BookTest.php +++ b/api/tests/Api/Admin/BookTest.php @@ -33,7 +33,7 @@ final class BookTest extends ApiTestCase protected function setup(): void { - $this->client = self::createClient(['debug' => true]); + $this->client = self::createClient(); } #[Test] diff --git a/api/tests/Api/Admin/UserTest.php b/api/tests/Api/Admin/UserTest.php index 067e6abd1..4aaf80eb6 100644 --- a/api/tests/Api/Admin/UserTest.php +++ b/api/tests/Api/Admin/UserTest.php @@ -12,7 +12,6 @@ use App\Tests\Api\Security\TokenGenerator; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Test; -use Symfony\Component\Uid\Uuid; use Zenstruck\Foundry\FactoryCollection; use Zenstruck\Foundry\Test\Factories; use Zenstruck\Foundry\Test\ResetDatabase; @@ -27,7 +26,7 @@ final class UserTest extends ApiTestCase protected function setup(): void { - $this->client = self::createClient(); + $this->client = self::createClient(['debug' => true]); } #[Test] @@ -156,12 +155,9 @@ public function asAUserIAmUpdatedOnLogin(): void $user = UserFactory::createOne([ 'firstName' => 'John', 'lastName' => 'DOE', - 'sub' => Uuid::fromString('b5c5bff1-5b5f-4a73-8fc8-4ea8f18586a9'), ])->disableAutoRefresh(); - $sub = Uuid::v7()->__toString(); $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ - 'sub' => $sub, 'email' => $user->email, 'given_name' => 'Chuck', 'family_name' => 'NORRIS', @@ -176,6 +172,5 @@ public function asAUserIAmUpdatedOnLogin(): void self::assertEquals('Chuck', $user->firstName); self::assertEquals('NORRIS', $user->lastName); self::assertEquals('Chuck NORRIS', $user->getName()); - self::assertEquals($sub, $user->sub); } } diff --git a/api/tests/Api/Admin/schemas/Review/collection.json b/api/tests/Api/Admin/schemas/Review/collection.json index 181231e03..6a1f327d4 100644 --- a/api/tests/Api/Admin/schemas/Review/collection.json +++ b/api/tests/Api/Admin/schemas/Review/collection.json @@ -131,12 +131,6 @@ "type": "string", "pattern": "^https://schema.org/Person$" }, - "sub": { - "externalDocs": { - "url": "https:\/\/schema.org\/identifier" - }, - "type": "string" - }, "firstName": { "description": "The givenName of the person", "externalDocs": { @@ -162,7 +156,6 @@ "required": [ "@id", "@type", - "sub", "firstName", "lastName", "name" diff --git a/api/tests/Api/Admin/schemas/Review/item.json b/api/tests/Api/Admin/schemas/Review/item.json index f2e6902e6..3f81816f5 100644 --- a/api/tests/Api/Admin/schemas/Review/item.json +++ b/api/tests/Api/Admin/schemas/Review/item.json @@ -38,12 +38,6 @@ "type": "string", "pattern": "^https://schema.org/Person$" }, - "sub": { - "externalDocs": { - "url": "https:\/\/schema.org\/identifier" - }, - "type": "string" - }, "firstName": { "description": "The givenName of the person", "externalDocs": { @@ -69,7 +63,6 @@ "required": [ "@id", "@type", - "sub", "firstName", "lastName", "name" diff --git a/api/tests/Api/Admin/schemas/User/collection.json b/api/tests/Api/Admin/schemas/User/collection.json index 915885e01..f94e49ad0 100644 --- a/api/tests/Api/Admin/schemas/User/collection.json +++ b/api/tests/Api/Admin/schemas/User/collection.json @@ -17,12 +17,6 @@ "type": "string", "pattern": "^https://schema.org/Person$" }, - "sub": { - "externalDocs": { - "url": "https:\/\/schema.org\/identifier" - }, - "type": "string" - }, "firstName": { "description": "The givenName of the person", "externalDocs": { @@ -48,7 +42,6 @@ "required": [ "@id", "@type", - "sub", "firstName", "lastName", "name" diff --git a/api/tests/Api/Admin/schemas/User/item.json b/api/tests/Api/Admin/schemas/User/item.json index 0c24d77f0..945d5b051 100644 --- a/api/tests/Api/Admin/schemas/User/item.json +++ b/api/tests/Api/Admin/schemas/User/item.json @@ -18,12 +18,6 @@ "type": "string", "pattern": "^https://schema.org/Person$" }, - "sub": { - "externalDocs": { - "url": "https:\/\/schema.org\/identifier" - }, - "type": "string" - }, "firstName": { "description": "The givenName of the person", "externalDocs": { @@ -50,7 +44,6 @@ "@context", "@id", "@type", - "sub", "firstName", "lastName", "name" diff --git a/api/tests/Api/schemas/Review/collection.json b/api/tests/Api/schemas/Review/collection.json index 1779749f2..cfe83e86c 100644 --- a/api/tests/Api/schemas/Review/collection.json +++ b/api/tests/Api/schemas/Review/collection.json @@ -88,12 +88,6 @@ "type": "string", "pattern": "^https://schema.org/Person$" }, - "sub": { - "externalDocs": { - "url": "https:\/\/schema.org\/identifier" - }, - "type": "string" - }, "firstName": { "description": "The givenName of the person", "externalDocs": { @@ -119,7 +113,6 @@ "required": [ "@id", "@type", - "sub", "firstName", "lastName", "name" diff --git a/api/tests/Api/schemas/Review/item.json b/api/tests/Api/schemas/Review/item.json index 4578faf03..fb4c5716f 100644 --- a/api/tests/Api/schemas/Review/item.json +++ b/api/tests/Api/schemas/Review/item.json @@ -38,12 +38,6 @@ "type": "string", "pattern": "^https://schema.org/Person$" }, - "sub": { - "externalDocs": { - "url": "https:\/\/schema.org\/identifier" - }, - "type": "string" - }, "firstName": { "description": "The givenName of the person", "externalDocs": { @@ -69,7 +63,6 @@ "required": [ "@id", "@type", - "sub", "firstName", "lastName", "name" diff --git a/api/tests/Security/Core/UserProviderTest.php b/api/tests/Security/Core/UserProviderTest.php index e4eb1a69b..6e26887ab 100644 --- a/api/tests/Security/Core/UserProviderTest.php +++ b/api/tests/Security/Core/UserProviderTest.php @@ -15,7 +15,6 @@ use PHPUnit\Framework\TestCase; use Symfony\Component\Security\Core\Exception\UnsupportedUserException; use Symfony\Component\Security\Core\User\UserInterface; -use Symfony\Component\Uid\Uuid; final class UserProviderTest extends TestCase { @@ -102,12 +101,8 @@ public function itCannotLoadUserIfAttributeIsMissing(array $attributes): void public static function getInvalidAttributes(): iterable { - yield 'missing sub' => [[]]; - yield 'missing given_name' => [[ - 'sub' => 'ba86c94b-efeb-4452-a0b4-93ed3c889156', - ]]; + yield 'missing given_name' => [[]]; yield 'missing family_name' => [[ - 'sub' => 'ba86c94b-efeb-4452-a0b4-93ed3c889156', 'given_name' => 'John', ]]; } @@ -128,7 +123,6 @@ public function itLoadsUserFromAttributes(): void ; $this->assertSame($this->userMock, $this->provider->loadUserByIdentifier('john.doe@example.com', [ - 'sub' => 'ba86c94b-efeb-4452-a0b4-93ed3c889156', 'given_name' => 'John', 'family_name' => 'DOE', ])); @@ -140,7 +134,6 @@ public function itCreatesAUserFromAttributes(): void $expectedUser = new User(); $expectedUser->firstName = 'John'; $expectedUser->lastName = 'DOE'; - $expectedUser->sub = Uuid::fromString('ba86c94b-efeb-4452-a0b4-93ed3c889156'); $expectedUser->email = 'john.doe@example.com'; $this->repositoryMock @@ -156,7 +149,6 @@ public function itCreatesAUserFromAttributes(): void ; $this->assertEquals($expectedUser, $this->provider->loadUserByIdentifier('john.doe@example.com', [ - 'sub' => 'ba86c94b-efeb-4452-a0b4-93ed3c889156', 'given_name' => 'John', 'family_name' => 'DOE', ])); diff --git a/api/tests/State/Processor/ReviewPersistProcessorTest.php b/api/tests/State/Processor/ReviewPersistProcessorTest.php index 56d71c892..05dbfb46c 100644 --- a/api/tests/State/Processor/ReviewPersistProcessorTest.php +++ b/api/tests/State/Processor/ReviewPersistProcessorTest.php @@ -9,6 +9,7 @@ use ApiPlatform\State\ProcessorInterface; use App\Entity\Review; use App\Entity\User; +use App\Security\Http\Protection\ResourceHandlerInterface; use App\State\Processor\ReviewPersistProcessor; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\MockObject\MockObject; @@ -25,6 +26,7 @@ final class ReviewPersistProcessorTest extends TestCase private MockObject|User $userMock; private MockObject|Review $objectMock; private ClockInterface|MockObject $clockMock; + private ResourceHandlerInterface|MockObject $resourceHandlerMock; private ReviewPersistProcessor $processor; protected function setUp(): void @@ -35,12 +37,14 @@ protected function setUp(): void $this->userMock = $this->createMock(User::class); $this->objectMock = $this->createMock(Review::class); $this->clockMock = new MockClock(); + $this->resourceHandlerMock = $this->createMock(ResourceHandlerInterface::class); $this->processor = new ReviewPersistProcessor( $this->persistProcessorMock, $this->mercureProcessorMock, $this->securityMock, - $this->clockMock + $this->clockMock, + $this->resourceHandlerMock ); } @@ -53,6 +57,7 @@ public function itUpdatesReviewDataFromOperationBeforeSaveAndSendMercureUpdates( $expectedData->user = $this->userMock; $expectedData->publishedAt = $this->clockMock->now(); + $this->userMock->email = 'john.doe@example.com'; $this->securityMock ->expects($this->once()) ->method('getUser') @@ -64,6 +69,13 @@ public function itUpdatesReviewDataFromOperationBeforeSaveAndSendMercureUpdates( ->with($expectedData, $operation, [], []) ->willReturn($expectedData) ; + $this->resourceHandlerMock + ->expects($this->once()) + ->method('create') + ->with($expectedData, $this->userMock, [ + 'operation_name' => '/books/{bookId}/reviews/{id}{._format}', + ]) + ; $this->mercureProcessorMock ->expects($this->exactly(2)) ->method('process') @@ -105,6 +117,10 @@ public function itUpdatesReviewDataFromContextBeforeSaveAndSendMercureUpdates(): ->with($expectedData, $operation, [], $context) ->willReturn($expectedData) ; + $this->resourceHandlerMock + ->expects($this->never()) + ->method('create') + ; $this->mercureProcessorMock ->expects($this->exactly(2)) ->method('process') diff --git a/api/tests/State/Processor/ReviewRemoveProcessorTest.php b/api/tests/State/Processor/ReviewRemoveProcessorTest.php index cfd60bc76..acedcd2dd 100644 --- a/api/tests/State/Processor/ReviewRemoveProcessorTest.php +++ b/api/tests/State/Processor/ReviewRemoveProcessorTest.php @@ -12,6 +12,8 @@ use ApiPlatform\Metadata\Resource\ResourceMetadataCollection; use ApiPlatform\State\ProcessorInterface; use App\Entity\Review; +use App\Entity\User; +use App\Security\Http\Protection\ResourceHandlerInterface; use App\State\Processor\ReviewRemoveProcessor; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\MockObject\MockObject; @@ -26,6 +28,7 @@ final class ReviewRemoveProcessorTest extends TestCase private IriConverterInterface|MockObject $iriConverterMock; private MockObject|Review $objectMock; private MockObject|Operation $operationMock; + private ResourceHandlerInterface|MockObject $resourceHandlerMock; private ReviewRemoveProcessor $processor; protected function setUp(): void @@ -33,6 +36,7 @@ protected function setUp(): void $this->removeProcessorMock = $this->createMock(ProcessorInterface::class); $this->mercureProcessorMock = $this->createMock(ProcessorInterface::class); $this->resourceMetadataCollectionFactoryMock = $this->createMock(ResourceMetadataCollectionFactoryInterface::class); + $this->resourceHandlerMock = $this->createMock(ResourceHandlerInterface::class); $this->resourceMetadataCollection = new ResourceMetadataCollection(Review::class, [ new ApiResource(operations: [new Get('/admin/reviews/{id}{._format}')]), new ApiResource(operations: [new Get('/books/{bookId}/reviews/{id}{._format}')]), @@ -45,7 +49,8 @@ protected function setUp(): void $this->removeProcessorMock, $this->mercureProcessorMock, $this->resourceMetadataCollectionFactoryMock, - $this->iriConverterMock + $this->iriConverterMock, + $this->resourceHandlerMock ); } @@ -81,6 +86,15 @@ public function itRemovesBookAndSendMercureUpdates(): void '/books/8ad70d36-abaf-4c9b-aeaa-7ec63e6ca6f3/reviews/9aff4b91-31cf-4e91-94b0-1d52bbe23fe6', ) ; + $this->objectMock->user = $this->createMock(User::class); + $this->objectMock->user->email = 'john.doe@example.com'; + $this->resourceHandlerMock + ->expects($this->once()) + ->method('delete') + ->with($this->objectMock, $this->objectMock->user, [ + 'operation_name' => '/books/{bookId}/reviews/{id}{._format}', + ]) + ; $this->mercureProcessorMock ->expects($this->exactly(2)) ->method('process') From 7ef754a68dd8bacf1c96ffa2a79476c5d7df38fa Mon Sep 17 00:00:00 2001 From: Vincent Chalamon <407859+vincentchalamon@users.noreply.github.com> Date: Thu, 28 Mar 2024 09:36:02 +0100 Subject: [PATCH 7/8] chore: update dependencies (post rebase) --- api/composer.lock | 183 +++++++++++++++++++++++++++++++++------------- 1 file changed, 132 insertions(+), 51 deletions(-) diff --git a/api/composer.lock b/api/composer.lock index fb46232ed..9350a0ac5 100644 --- a/api/composer.lock +++ b/api/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", "This file is @generated automatically" ], - "content-hash": "30bf2abc05f735781179f265db9f9426", + "content-hash": "b6e35817aa0d4602d22e2457ed1f9ab6", "packages": [ { "name": "api-platform/core", @@ -7086,18 +7086,99 @@ ], "time": "2023-11-21T18:54:41+00:00" }, + { + "name": "web-token/jwt-bundle", + "version": "3.3.4", + "source": { + "type": "git", + "url": "https://github.com/web-token/jwt-bundle.git", + "reference": "fce2876e1f2b611d8cce0a637d0eb2585ad94d8e" + }, + "dist": { + "type": "zip", + "url": "https://api.github.com/repos/web-token/jwt-bundle/zipball/fce2876e1f2b611d8cce0a637d0eb2585ad94d8e", + "reference": "fce2876e1f2b611d8cce0a637d0eb2585ad94d8e", + "shasum": "" + }, + "require": { + "php": ">=8.1", + "psr/event-dispatcher": "^1.0", + "symfony/config": "^5.4|^6.0|^7.0", + "symfony/dependency-injection": "^5.4|^6.0|^7.0", + "symfony/event-dispatcher": "^5.4|^6.0|^7.0", + "symfony/http-kernel": "^5.4|^6.0|^7.0", + "web-token/jwt-library": "^3.3" + }, + "suggest": { + "symfony/serializer": "Use the Symfony serializer to serialize/unserialize JWS and JWE tokens." + }, + "type": "symfony-bundle", + "autoload": { + "psr-4": { + "Jose\\Bundle\\JoseFramework\\": "" + } + }, + "notification-url": "https://packagist.org/downloads/", + "license": [ + "MIT" + ], + "authors": [ + { + "name": "Florent Morselli", + "homepage": "https://github.com/Spomky" + }, + { + "name": "All contributors", + "homepage": "https://github.com/web-token/jwt-bundle/contributors" + } + ], + "description": "JWT Bundle of the JWT Framework.", + "homepage": "https://github.com/web-token", + "keywords": [ + "JOSE", + "JWE", + "JWK", + "JWKSet", + "JWS", + "Jot", + "RFC7515", + "RFC7516", + "RFC7517", + "RFC7518", + "RFC7519", + "RFC7520", + "bundle", + "jwa", + "jwt", + "symfony" + ], + "support": { + "source": "https://github.com/web-token/jwt-bundle/tree/3.3.4" + }, + "funding": [ + { + "url": "https://github.com/Spomky", + "type": "github" + }, + { + "url": "https://www.patreon.com/FlorentMorselli", + "type": "patreon" + } + ], + "time": "2024-03-24T09:57:06+00:00" + }, { "name": "web-token/jwt-library", - "version": "3.3.1", + "version": "3.3.4", "source": { "type": "git", "url": "https://github.com/web-token/jwt-library.git", - "reference": "37a0665d89865d09579e484e5d7d70950d079198" + "reference": "a9fde8057aa978a4b97436d8875f5bb45a30fb2e" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/web-token/jwt-library/zipball/37a0665d89865d09579e484e5d7d70950d079198", - "reference": "37a0665d89865d09579e484e5d7d70950d079198", + "url": "https://api.github.com/repos/web-token/jwt-library/zipball/a9fde8057aa978a4b97436d8875f5bb45a30fb2e", + "reference": "a9fde8057aa978a4b97436d8875f5bb45a30fb2e", "shasum": "" }, "require": { @@ -7169,7 +7250,7 @@ ], "support": { "issues": "https://github.com/web-token/jwt-library/issues", - "source": "https://github.com/web-token/jwt-library/tree/3.3.1" + "source": "https://github.com/web-token/jwt-library/tree/3.3.4" }, "funding": [ { @@ -7181,7 +7262,7 @@ "type": "patreon" } ], - "time": "2024-02-28T09:04:35+00:00" + "time": "2024-03-24T09:57:06+00:00" }, { "name": "webonyx/graphql-php", @@ -8008,16 +8089,16 @@ }, { "name": "phpstan/phpdoc-parser", - "version": "1.26.0", + "version": "1.27.0", "source": { "type": "git", "url": "https://github.com/phpstan/phpdoc-parser.git", - "reference": "231e3186624c03d7e7c890ec662b81e6b0405227" + "reference": "86e4d5a4b036f8f0be1464522f4c6b584c452757" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/phpstan/phpdoc-parser/zipball/231e3186624c03d7e7c890ec662b81e6b0405227", - "reference": "231e3186624c03d7e7c890ec662b81e6b0405227", + "url": "https://api.github.com/repos/phpstan/phpdoc-parser/zipball/86e4d5a4b036f8f0be1464522f4c6b584c452757", + "reference": "86e4d5a4b036f8f0be1464522f4c6b584c452757", "shasum": "" }, "require": { @@ -8049,22 +8130,22 @@ "description": "PHPDoc parser with support for nullable, intersection and generic types", "support": { "issues": "https://github.com/phpstan/phpdoc-parser/issues", - "source": "https://github.com/phpstan/phpdoc-parser/tree/1.26.0" + "source": "https://github.com/phpstan/phpdoc-parser/tree/1.27.0" }, - "time": "2024-02-23T16:05:55+00:00" + "time": "2024-03-21T13:14:53+00:00" }, { "name": "phpstan/phpstan", - "version": "1.10.62", + "version": "1.10.65", "source": { "type": "git", "url": "https://github.com/phpstan/phpstan.git", - "reference": "cd5c8a1660ed3540b211407c77abf4af193a6af9" + "reference": "3c657d057a0b7ecae19cb12db446bbc99d8839c6" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/phpstan/phpstan/zipball/cd5c8a1660ed3540b211407c77abf4af193a6af9", - "reference": "cd5c8a1660ed3540b211407c77abf4af193a6af9", + "url": "https://api.github.com/repos/phpstan/phpstan/zipball/3c657d057a0b7ecae19cb12db446bbc99d8839c6", + "reference": "3c657d057a0b7ecae19cb12db446bbc99d8839c6", "shasum": "" }, "require": { @@ -8113,25 +8194,25 @@ "type": "tidelift" } ], - "time": "2024-03-13T12:27:20+00:00" + "time": "2024-03-23T10:30:26+00:00" }, { "name": "phpstan/phpstan-doctrine", - "version": "1.3.62", + "version": "1.3.64", "source": { "type": "git", "url": "https://github.com/phpstan/phpstan-doctrine.git", - "reference": "f3abbd8e93e12fed8091be3aeec216b06bed0950" + "reference": "f42828ad684e026054c3d94161d89870150bc3da" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/phpstan/phpstan-doctrine/zipball/f3abbd8e93e12fed8091be3aeec216b06bed0950", - "reference": "f3abbd8e93e12fed8091be3aeec216b06bed0950", + "url": "https://api.github.com/repos/phpstan/phpstan-doctrine/zipball/f42828ad684e026054c3d94161d89870150bc3da", + "reference": "f42828ad684e026054c3d94161d89870150bc3da", "shasum": "" }, "require": { "php": "^7.2 || ^8.0", - "phpstan/phpstan": "^1.10.48" + "phpstan/phpstan": "^1.10.64" }, "conflict": { "doctrine/collections": "<1.0", @@ -8183,9 +8264,9 @@ "description": "Doctrine extensions for PHPStan", "support": { "issues": "https://github.com/phpstan/phpstan-doctrine/issues", - "source": "https://github.com/phpstan/phpstan-doctrine/tree/1.3.62" + "source": "https://github.com/phpstan/phpstan-doctrine/tree/1.3.64" }, - "time": "2024-02-12T11:52:17+00:00" + "time": "2024-03-21T10:19:51+00:00" }, { "name": "phpstan/phpstan-phpunit", @@ -8241,22 +8322,22 @@ }, { "name": "phpstan/phpstan-symfony", - "version": "1.3.8", + "version": "1.3.9", "source": { "type": "git", "url": "https://github.com/phpstan/phpstan-symfony.git", - "reference": "d8a0bc03a68d95288b6471c37d435647fbdaff1a" + "reference": "a32bc86da24495025d7aafd1ba62444d4a364a98" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/phpstan/phpstan-symfony/zipball/d8a0bc03a68d95288b6471c37d435647fbdaff1a", - "reference": "d8a0bc03a68d95288b6471c37d435647fbdaff1a", + "url": "https://api.github.com/repos/phpstan/phpstan-symfony/zipball/a32bc86da24495025d7aafd1ba62444d4a364a98", + "reference": "a32bc86da24495025d7aafd1ba62444d4a364a98", "shasum": "" }, "require": { "ext-simplexml": "*", "php": "^7.2 || ^8.0", - "phpstan/phpstan": "^1.10.36" + "phpstan/phpstan": "^1.10.62" }, "conflict": { "symfony/framework-bundle": "<3.0" @@ -8307,9 +8388,9 @@ "description": "Symfony Framework extensions and rules for PHPStan", "support": { "issues": "https://github.com/phpstan/phpstan-symfony/issues", - "source": "https://github.com/phpstan/phpstan-symfony/tree/1.3.8" + "source": "https://github.com/phpstan/phpstan-symfony/tree/1.3.9" }, - "time": "2024-03-05T16:33:08+00:00" + "time": "2024-03-16T16:50:20+00:00" }, { "name": "phpunit/php-code-coverage", @@ -8636,16 +8717,16 @@ }, { "name": "phpunit/phpunit", - "version": "11.0.6", + "version": "11.0.8", "source": { "type": "git", "url": "https://github.com/sebastianbergmann/phpunit.git", - "reference": "6af32d7938fc366f86e49a5f5ebb314018d1b1fb" + "reference": "48ea58408879a9aad630022186398364051482fc" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/sebastianbergmann/phpunit/zipball/6af32d7938fc366f86e49a5f5ebb314018d1b1fb", - "reference": "6af32d7938fc366f86e49a5f5ebb314018d1b1fb", + "url": "https://api.github.com/repos/sebastianbergmann/phpunit/zipball/48ea58408879a9aad630022186398364051482fc", + "reference": "48ea58408879a9aad630022186398364051482fc", "shasum": "" }, "require": { @@ -8716,7 +8797,7 @@ "support": { "issues": "https://github.com/sebastianbergmann/phpunit/issues", "security": "https://github.com/sebastianbergmann/phpunit/security/policy", - "source": "https://github.com/sebastianbergmann/phpunit/tree/11.0.6" + "source": "https://github.com/sebastianbergmann/phpunit/tree/11.0.8" }, "funding": [ { @@ -8732,7 +8813,7 @@ "type": "tidelift" } ], - "time": "2024-03-12T15:40:01+00:00" + "time": "2024-03-22T04:21:01+00:00" }, { "name": "sebastian/cli-parser", @@ -9108,16 +9189,16 @@ }, { "name": "sebastian/environment", - "version": "7.0.0", + "version": "7.1.0", "source": { "type": "git", "url": "https://github.com/sebastianbergmann/environment.git", - "reference": "100d8b855d7180f79f9a9a5c483f2d960581c3ea" + "reference": "4eb3a442574d0e9d141aab209cd4aaf25701b09a" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/sebastianbergmann/environment/zipball/100d8b855d7180f79f9a9a5c483f2d960581c3ea", - "reference": "100d8b855d7180f79f9a9a5c483f2d960581c3ea", + "url": "https://api.github.com/repos/sebastianbergmann/environment/zipball/4eb3a442574d0e9d141aab209cd4aaf25701b09a", + "reference": "4eb3a442574d0e9d141aab209cd4aaf25701b09a", "shasum": "" }, "require": { @@ -9132,7 +9213,7 @@ "type": "library", "extra": { "branch-alias": { - "dev-main": "7.0-dev" + "dev-main": "7.1-dev" } }, "autoload": { @@ -9160,7 +9241,7 @@ "support": { "issues": "https://github.com/sebastianbergmann/environment/issues", "security": "https://github.com/sebastianbergmann/environment/security/policy", - "source": "https://github.com/sebastianbergmann/environment/tree/7.0.0" + "source": "https://github.com/sebastianbergmann/environment/tree/7.1.0" }, "funding": [ { @@ -9168,7 +9249,7 @@ "type": "github" } ], - "time": "2024-02-02T05:57:54+00:00" + "time": "2024-03-23T08:56:34+00:00" }, { "name": "sebastian/exporter", @@ -9933,16 +10014,16 @@ }, { "name": "symfony/maker-bundle", - "version": "v1.56.0", + "version": "v1.57.0", "source": { "type": "git", "url": "https://github.com/symfony/maker-bundle.git", - "reference": "bbb7949ae048363df7c8439abeddef8befd155ce" + "reference": "2c90181911241648356b828b86b04fe3571aca0b" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/symfony/maker-bundle/zipball/bbb7949ae048363df7c8439abeddef8befd155ce", - "reference": "bbb7949ae048363df7c8439abeddef8befd155ce", + "url": "https://api.github.com/repos/symfony/maker-bundle/zipball/2c90181911241648356b828b86b04fe3571aca0b", + "reference": "2c90181911241648356b828b86b04fe3571aca0b", "shasum": "" }, "require": { @@ -10005,7 +10086,7 @@ ], "support": { "issues": "https://github.com/symfony/maker-bundle/issues", - "source": "https://github.com/symfony/maker-bundle/tree/v1.56.0" + "source": "https://github.com/symfony/maker-bundle/tree/v1.57.0" }, "funding": [ { @@ -10021,7 +10102,7 @@ "type": "tidelift" } ], - "time": "2024-03-04T13:36:45+00:00" + "time": "2024-03-22T12:00:21+00:00" }, { "name": "symfony/process", From 9dc1360df118369dac8140f49294e1f9e9c34429 Mon Sep 17 00:00:00 2001 From: Vincent Chalamon <407859+vincentchalamon@users.noreply.github.com> Date: Thu, 28 Mar 2024 16:55:51 +0100 Subject: [PATCH 8/8] fix: remove User::roles from database --- api/migrations/Version20230906094949.php | 2 +- api/src/Entity/User.php | 5 +---- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/api/migrations/Version20230906094949.php b/api/migrations/Version20230906094949.php index de2268c6d..376622441 100644 --- a/api/migrations/Version20230906094949.php +++ b/api/migrations/Version20230906094949.php @@ -41,7 +41,7 @@ public function up(Schema $schema): void $this->addSql('COMMENT ON COLUMN review.user_id IS \'(DC2Type:uuid)\''); $this->addSql('COMMENT ON COLUMN review.book_id IS \'(DC2Type:uuid)\''); $this->addSql('COMMENT ON COLUMN review.published_at IS \'(DC2Type:datetime_immutable)\''); - $this->addSql('CREATE TABLE "user" (id UUID NOT NULL, email VARCHAR(255) NOT NULL, first_name VARCHAR(255) NOT NULL, last_name VARCHAR(255) NOT NULL, roles JSON NOT NULL, PRIMARY KEY(id))'); + $this->addSql('CREATE TABLE "user" (id UUID NOT NULL, email VARCHAR(255) NOT NULL, first_name VARCHAR(255) NOT NULL, last_name VARCHAR(255) NOT NULL, PRIMARY KEY(id))'); $this->addSql('CREATE UNIQUE INDEX UNIQ_8D93D649E7927C74 ON "user" (email)'); $this->addSql('COMMENT ON COLUMN "user".id IS \'(DC2Type:uuid)\''); $this->addSql('ALTER TABLE bookmark ADD CONSTRAINT FK_DA62921DA76ED395 FOREIGN KEY (user_id) REFERENCES "user" (id) NOT DEFERRABLE INITIALLY IMMEDIATE'); diff --git a/api/src/Entity/User.php b/api/src/Entity/User.php index 74a10633b..2832c1623 100644 --- a/api/src/Entity/User.php +++ b/api/src/Entity/User.php @@ -85,9 +85,6 @@ class User implements UserInterface #[ORM\Column] public ?string $lastName = null; - #[ORM\Column(type: 'json')] - public array $roles = []; - public function getId(): ?Uuid { return $this->id; @@ -102,7 +99,7 @@ public function eraseCredentials(): void */ public function getRoles(): array { - return $this->roles; + return ['ROLE_USER']; } public function getUserIdentifier(): string