From aeaa2d34e9bd7882f3a9e34b66752185f2e28db6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Antonio=20J=2E=20Garc=C3=ADa=20Lagar?= Date: Fri, 16 Aug 2024 12:09:26 +0200 Subject: [PATCH] Require a logged in user to resolve an authorization request --- src/Converter/UserConverter.php | 26 ++-------- src/Converter/UserConverterInterface.php | 2 +- src/DependencyInjection/Configuration.php | 6 --- .../LeagueOAuth2ServerExtension.php | 4 -- .../AuthorizationRequestResolveEvent.php | 14 ++---- ...uthorizationRequestResolveEventFactory.php | 45 ++++++++--------- ...izationRequestResolveEventFactoryTrait.php | 50 +++++++++++++++++++ ...horizationRequestUserResolvingListener.php | 46 ----------------- ...ationRequestUserResolvingListenerTrait.php | 19 ------- src/Resources/config/services.php | 16 ++---- .../Acceptance/AuthorizationEndpointTest.php | 28 +++++++++++ tests/TestKernel.php | 12 +++++ 12 files changed, 122 insertions(+), 146 deletions(-) create mode 100644 src/Event/AuthorizationRequestResolveEventFactoryTrait.php delete mode 100644 src/EventListener/AuthorizationRequestUserResolvingListener.php delete mode 100644 src/EventListener/AuthorizationRequestUserResolvingListenerTrait.php diff --git a/src/Converter/UserConverter.php b/src/Converter/UserConverter.php index 757980a0..970f3b96 100644 --- a/src/Converter/UserConverter.php +++ b/src/Converter/UserConverter.php @@ -10,34 +10,16 @@ final class UserConverter implements UserConverterInterface { - public const DEFAULT_ANONYMOUS_USER_IDENTIFIER = 'anonymous'; - - /** @var non-empty-string */ - private string $anonymousUserIdentifier; - - /** - * @param non-empty-string $anonymousUserIdentifier - */ - public function __construct(string $anonymousUserIdentifier = self::DEFAULT_ANONYMOUS_USER_IDENTIFIER) - { - $this->anonymousUserIdentifier = $anonymousUserIdentifier; - } - /** + * @psalm-suppress ArgumentTypeCoercion * @psalm-suppress DeprecatedMethod * @psalm-suppress UndefinedInterfaceMethod */ - public function toLeague(?UserInterface $user): UserEntityInterface + public function toLeague(UserInterface $user): UserEntityInterface { $userEntity = new User(); - if ($user instanceof UserInterface) { - $identifier = method_exists($user, 'getUserIdentifier') ? $user->getUserIdentifier() : $user->getUsername(); - if ('' === $identifier) { - $identifier = $this->anonymousUserIdentifier; - } - } else { - $identifier = $this->anonymousUserIdentifier; - } + + $identifier = method_exists($user, 'getUserIdentifier') ? $user->getUserIdentifier() : $user->getUsername(); $userEntity->setIdentifier($identifier); diff --git a/src/Converter/UserConverterInterface.php b/src/Converter/UserConverterInterface.php index a0c402ba..8fcdf3e5 100644 --- a/src/Converter/UserConverterInterface.php +++ b/src/Converter/UserConverterInterface.php @@ -9,5 +9,5 @@ interface UserConverterInterface { - public function toLeague(?UserInterface $user): UserEntityInterface; + public function toLeague(UserInterface $user): UserEntityInterface; } diff --git a/src/DependencyInjection/Configuration.php b/src/DependencyInjection/Configuration.php index 7a8bc7e7..c3566b79 100644 --- a/src/DependencyInjection/Configuration.php +++ b/src/DependencyInjection/Configuration.php @@ -5,7 +5,6 @@ namespace League\Bundle\OAuth2ServerBundle\DependencyInjection; use Defuse\Crypto\Key; -use League\Bundle\OAuth2ServerBundle\Converter\UserConverter; use League\Bundle\OAuth2ServerBundle\Model\AbstractClient; use League\Bundle\OAuth2ServerBundle\Model\Client; use Symfony\Component\Config\Definition\Builder\NodeDefinition; @@ -32,11 +31,6 @@ public function getConfigTreeBuilder(): TreeBuilder ->defaultValue('ROLE_OAUTH2_') ->cannotBeEmpty() ->end() - ->scalarNode('anonymous_user_identifier') - ->info('Set a default user identifier for anonymous users') - ->defaultValue(UserConverter::DEFAULT_ANONYMOUS_USER_IDENTIFIER) - ->cannotBeEmpty() - ->end() ->end(); return $treeBuilder; diff --git a/src/DependencyInjection/LeagueOAuth2ServerExtension.php b/src/DependencyInjection/LeagueOAuth2ServerExtension.php index adbf918a..1abf8809 100644 --- a/src/DependencyInjection/LeagueOAuth2ServerExtension.php +++ b/src/DependencyInjection/LeagueOAuth2ServerExtension.php @@ -8,7 +8,6 @@ use League\Bundle\OAuth2ServerBundle\AuthorizationServer\GrantTypeInterface; use League\Bundle\OAuth2ServerBundle\Command\CreateClientCommand; use League\Bundle\OAuth2ServerBundle\Command\GenerateKeyPairCommand; -use League\Bundle\OAuth2ServerBundle\Converter\UserConverter; use League\Bundle\OAuth2ServerBundle\DBAL\Type\Grant as GrantType; use League\Bundle\OAuth2ServerBundle\DBAL\Type\RedirectUri as RedirectUriType; use League\Bundle\OAuth2ServerBundle\DBAL\Type\Scope as ScopeType; @@ -69,9 +68,6 @@ public function load(array $configs, ContainerBuilder $container) $container->findDefinition(OAuth2Authenticator::class) ->setArgument(3, $config['role_prefix']); - $container->findDefinition(UserConverter::class) - ->setArgument(0, $config['anonymous_user_identifier']); - $container->registerForAutoconfiguration(GrantTypeInterface::class) ->addTag('league.oauth2_server.authorization_server.grant'); diff --git a/src/Event/AuthorizationRequestResolveEvent.php b/src/Event/AuthorizationRequestResolveEvent.php index 33d621a4..aabb1ed4 100644 --- a/src/Event/AuthorizationRequestResolveEvent.php +++ b/src/Event/AuthorizationRequestResolveEvent.php @@ -42,18 +42,19 @@ final class AuthorizationRequestResolveEvent extends Event private $response; /** - * @var UserInterface|null + * @var UserInterface */ private $user; /** * @param Scope[] $scopes */ - public function __construct(AuthorizationRequestInterface $authorizationRequest, array $scopes, ClientInterface $client) + public function __construct(AuthorizationRequestInterface $authorizationRequest, array $scopes, ClientInterface $client, UserInterface $user) { $this->authorizationRequest = $authorizationRequest; $this->scopes = $scopes; $this->client = $client; + $this->user = $user; } public function getAuthorizationResolution(): bool @@ -102,18 +103,11 @@ public function getClient(): ClientInterface /** * @psalm-mutation-free */ - public function getUser(): ?UserInterface + public function getUser(): UserInterface { return $this->user; } - public function setUser(?UserInterface $user): self - { - $this->user = $user; - - return $this; - } - /** * @return Scope[] */ diff --git a/src/Event/AuthorizationRequestResolveEventFactory.php b/src/Event/AuthorizationRequestResolveEventFactory.php index d2b68452..d1650676 100644 --- a/src/Event/AuthorizationRequestResolveEventFactory.php +++ b/src/Event/AuthorizationRequestResolveEventFactory.php @@ -6,36 +6,31 @@ use League\Bundle\OAuth2ServerBundle\Converter\ScopeConverterInterface; use League\Bundle\OAuth2ServerBundle\Manager\ClientManagerInterface; -use League\OAuth2\Server\RequestTypes\AuthorizationRequestInterface; +use Symfony\Bundle\SecurityBundle\Security; +use Symfony\Component\Security\Core\Security as LegacySecurity; -class AuthorizationRequestResolveEventFactory -{ - /** - * @var ScopeConverterInterface - */ - private $scopeConverter; - - /** - * @var ClientManagerInterface - */ - private $clientManager; - - public function __construct(ScopeConverterInterface $scopeConverter, ClientManagerInterface $clientManager) +if (class_exists(Security::class)) { + final class AuthorizationRequestResolveEventFactory { - $this->scopeConverter = $scopeConverter; - $this->clientManager = $clientManager; - } + use AuthorizationRequestResolveEventFactoryTrait; - public function fromAuthorizationRequest(AuthorizationRequestInterface $authorizationRequest): AuthorizationRequestResolveEvent + public function __construct(ScopeConverterInterface $scopeConverter, ClientManagerInterface $clientManager, Security $security) + { + $this->scopeConverter = $scopeConverter; + $this->clientManager = $clientManager; + $this->security = $security; + } + } +} else { + final class AuthorizationRequestResolveEventFactory { - $scopes = $this->scopeConverter->toDomainArray(array_values($authorizationRequest->getScopes())); - - $client = $this->clientManager->find($authorizationRequest->getClient()->getIdentifier()); + use AuthorizationRequestResolveEventFactoryTrait; - if (null === $client) { - throw new \RuntimeException(\sprintf('No client found for the given identifier \'%s\'.', $authorizationRequest->getClient()->getIdentifier())); + public function __construct(ScopeConverterInterface $scopeConverter, ClientManagerInterface $clientManager, LegacySecurity $security) + { + $this->scopeConverter = $scopeConverter; + $this->clientManager = $clientManager; + $this->security = $security; } - - return new AuthorizationRequestResolveEvent($authorizationRequest, $scopes, $client); } } diff --git a/src/Event/AuthorizationRequestResolveEventFactoryTrait.php b/src/Event/AuthorizationRequestResolveEventFactoryTrait.php new file mode 100644 index 00000000..755e05b8 --- /dev/null +++ b/src/Event/AuthorizationRequestResolveEventFactoryTrait.php @@ -0,0 +1,50 @@ +scopeConverter->toDomainArray(array_values($authorizationRequest->getScopes())); + + $client = $this->clientManager->find($authorizationRequest->getClient()->getIdentifier()); + + if (null === $client) { + throw new \RuntimeException(\sprintf('No client found for the given identifier \'%s\'.', $authorizationRequest->getClient()->getIdentifier())); + } + + $user = $this->security->getUser(); + if (null === $user) { + throw new \RuntimeException('A logged in user is required to resolve the authorization request.'); + } + + return new AuthorizationRequestResolveEvent($authorizationRequest, $scopes, $client, $user); + } +} diff --git a/src/EventListener/AuthorizationRequestUserResolvingListener.php b/src/EventListener/AuthorizationRequestUserResolvingListener.php deleted file mode 100644 index e735d8ce..00000000 --- a/src/EventListener/AuthorizationRequestUserResolvingListener.php +++ /dev/null @@ -1,46 +0,0 @@ -security = $security; - } - } -} else { - /** - * Listener sets currently authenticated user to authorization request context - */ - final class AuthorizationRequestUserResolvingListener - { - use AuthorizationRequestUserResolvingListenerTrait; - - /** - * @var LegacySecurity - */ - private $security; - - public function __construct(LegacySecurity $security) - { - $this->security = $security; - } - } -} diff --git a/src/EventListener/AuthorizationRequestUserResolvingListenerTrait.php b/src/EventListener/AuthorizationRequestUserResolvingListenerTrait.php deleted file mode 100644 index f9d4861d..00000000 --- a/src/EventListener/AuthorizationRequestUserResolvingListenerTrait.php +++ /dev/null @@ -1,19 +0,0 @@ -security->getUser(); - if ($user instanceof UserInterface) { - $event->setUser($user); - } - } -} diff --git a/src/Resources/config/services.php b/src/Resources/config/services.php index 2f445379..582d97cd 100644 --- a/src/Resources/config/services.php +++ b/src/Resources/config/services.php @@ -22,7 +22,6 @@ use League\Bundle\OAuth2ServerBundle\Converter\UserConverterInterface; use League\Bundle\OAuth2ServerBundle\Event\AuthorizationRequestResolveEventFactory; use League\Bundle\OAuth2ServerBundle\EventListener\AddClientDefaultScopesListener; -use League\Bundle\OAuth2ServerBundle\EventListener\AuthorizationRequestUserResolvingListener; use League\Bundle\OAuth2ServerBundle\Manager\AccessTokenManagerInterface; use League\Bundle\OAuth2ServerBundle\Manager\AuthorizationCodeManagerInterface; use League\Bundle\OAuth2ServerBundle\Manager\ClientManagerInterface; @@ -55,9 +54,11 @@ use Nyholm\Psr7\Factory\Psr17Factory; use Symfony\Bridge\PsrHttpMessage\Factory\HttpFoundationFactory; use Symfony\Bridge\PsrHttpMessage\Factory\PsrHttpFactory; +use Symfony\Bundle\SecurityBundle\Security; use Symfony\Component\DependencyInjection\Loader\Configurator\ContainerConfigurator; use Symfony\Component\EventDispatcher\EventDispatcherInterface; use Symfony\Component\HttpFoundation\RequestStack; +use Symfony\Component\Security\Core\Security as LegacySecurity; return static function (ContainerConfigurator $container): void { $container->services() @@ -206,18 +207,6 @@ ->tag('controller.service_arguments') ->alias(AuthorizationController::class, 'league.oauth2_server.controller.authorization') - // Authorization listeners - ->set('league.oauth2_server.listener.authorization_request_user_resolving', AuthorizationRequestUserResolvingListener::class) - ->args([ - service('security.helper'), - ]) - ->tag('kernel.event_listener', [ - 'event' => OAuth2Events::AUTHORIZATION_REQUEST_RESOLVE, - 'method' => 'onAuthorizationRequest', - 'priority' => 1024, - ]) - ->alias(AuthorizationRequestUserResolvingListener::class, 'league.oauth2_server.listener.authorization_request_user_resolving') - // Token controller ->set('league.oauth2_server.controller.token', TokenController::class) ->args([ @@ -292,6 +281,7 @@ ->args([ service(ScopeConverterInterface::class), service(ClientManagerInterface::class), + service(class_exists(Security::class) ? Security::class : LegacySecurity::class), ]) ->alias(AuthorizationRequestResolveEventFactory::class, 'league.oauth2_server.factory.authorization_request_resolve_event') diff --git a/tests/Acceptance/AuthorizationEndpointTest.php b/tests/Acceptance/AuthorizationEndpointTest.php index f640e0aa..b28bea98 100644 --- a/tests/Acceptance/AuthorizationEndpointTest.php +++ b/tests/Acceptance/AuthorizationEndpointTest.php @@ -31,6 +31,13 @@ protected function setUp(): void ); } + private function loginUser(string $username = FixtureFactory::FIXTURE_USER, string $firewallContext = 'authorization'): void + { + $userProvider = static::getContainer()->get('security.user_providers'); + $user = method_exists($userProvider, 'loadUserByIdentifier') ? $userProvider->loadUserByIdentifier($username) : $userProvider->loadUserByUsername($username); + $this->client->loginUser($user, $firewallContext); + } + public function testSuccessfulCodeRequest(): void { $this->client @@ -40,6 +47,8 @@ public function testSuccessfulCodeRequest(): void $event->resolveAuthorization(AuthorizationRequestResolveEvent::AUTHORIZATION_APPROVED); }); + $this->loginUser(); + $this->client->request( 'GET', '/authorize', @@ -75,6 +84,8 @@ public function testSuccessfulPKCEAuthCodeRequest(): void '-_' ); + $this->loginUser(); + $this->client ->getContainer() ->get('event_dispatcher') @@ -138,6 +149,8 @@ public function testAuthCodeRequestWithPublicClientWithoutCodeChallengeWhenTheCh $this->fail('This event should not have been dispatched.'); }); + $this->loginUser(); + $this->client->request( 'GET', '/authorize', @@ -176,6 +189,8 @@ public function testAuthCodeRequestWithClientWhoIsNotAllowedToMakeARequestWithPl $this->fail('This event should not have been dispatched.'); }); + $this->loginUser(); + $this->client->request( 'GET', '/authorize', @@ -220,6 +235,8 @@ public function testAuthCodeRequestWithClientWhoIsAllowedToMakeARequestWithPlain $event->resolveAuthorization(AuthorizationRequestResolveEvent::AUTHORIZATION_APPROVED); }); + $this->loginUser(); + $this->client->request( 'GET', '/authorize', @@ -272,6 +289,8 @@ public function testSuccessfulTokenRequest(): void $event->resolveAuthorization(AuthorizationRequestResolveEvent::AUTHORIZATION_APPROVED); }); + $this->loginUser(); + $this->client->request( 'GET', '/authorize', @@ -308,6 +327,8 @@ public function testCodeRequestRedirectToResolutionUri(): void ])); }); + $this->loginUser(); + $this->client->request( 'GET', '/authorize', @@ -341,6 +362,8 @@ public function testAuthorizationRequestEventIsStoppedAfterSettingAResponse(): v ])); }, 200); + $this->loginUser(); + $this->client->request( 'GET', '/authorize', @@ -374,6 +397,8 @@ public function testAuthorizationRequestEventIsStoppedAfterResolution(): void ); }, 100); + $this->loginUser(); + $this->client->request( 'GET', '/authorize', @@ -406,6 +431,8 @@ public function testFailedCodeRequestRedirectWithFakedRedirectUri(): void $event->resolveAuthorization(AuthorizationRequestResolveEvent::AUTHORIZATION_APPROVED); }); + $this->loginUser(); + $this->client->request( 'GET', '/authorize', @@ -430,6 +457,7 @@ public function testFailedCodeRequestRedirectWithFakedRedirectUri(): void public function testFailedAuthorizeRequest(): void { + $this->loginUser(); $this->client->request( 'GET', '/authorize' diff --git a/tests/TestKernel.php b/tests/TestKernel.php index 89d9f40b..1028a085 100644 --- a/tests/TestKernel.php +++ b/tests/TestKernel.php @@ -117,6 +117,12 @@ public function registerContainerConfiguration(LoaderInterface $loader): void 'stateless' => true, 'oauth2' => true, ], + 'authorization' => [ + 'provider' => 'in_memory', + 'pattern' => '^/authorize', + 'http_basic' => true, + 'stateless' => true, + ], ], 'providers' => [ 'in_memory' => [ @@ -138,6 +144,12 @@ public function registerContainerConfiguration(LoaderInterface $loader): void ], ], ], + 'access_control' => [ + [ + 'path' => '^/authorize', + 'roles' => class_exists(Security::class) ? 'IS_AUTHENTICATED' : 'IS_AUTHENTICATED_REMEMBERED', + ], + ], ]; if (!class_exists(Security::class)) {