diff --git a/api/composer.json b/api/composer.json index d122f6d4..7741b5bd 100644 --- a/api/composer.json +++ b/api/composer.json @@ -34,7 +34,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/composer.lock b/api/composer.lock index 304939bc..794b1910 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": "fcefc5290ee2c6a1aa412d0bfecf2cad", + "content-hash": "d797761ca0800aecf9f19c7bb39d7aa3", "packages": [ { "name": "api-platform/core", diff --git a/api/config/packages/framework.yaml b/api/config/packages/framework.yaml index 2610ba9e..ec3dcaa2 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 653cf944..992f66fe 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/src/Entity/Book.php b/api/src/Entity/Book.php index 52973463..e4d8403a 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 fde869e9..1e315e84 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 442e8a50..f57e9d9a 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 05c2063a..5001fa33 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 bbd53f08..6bf7dbe3 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 ce03ddae..00000000 --- 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 00000000..a0994876 --- /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 de21486a..f81df161 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 ef3d95b8..49372735 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 00000000..c435b5de --- /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 e2b0f678..3773c2aa 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 773454f9..4c28a566 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',