From 0d5956eba57e12f328ca211659dcec6587afdbc4 Mon Sep 17 00:00:00 2001 From: Gassan Gousseinov Date: Tue, 28 Dec 2021 11:52:57 +0100 Subject: [PATCH] Refresh token listener cannot be lazy. it will be only enabled if at least one resource owner has option 'refresh_on_expire' set to true. --- CHANGELOG.md | 2 + ...eRefreshOAuthTokenListenerCompilerPass.php | 32 ++++ src/DependencyInjection/HWIOAuthExtension.php | 11 ++ src/HWIOAuthBundle.php | 2 + .../AbstractRefreshAccessTokenListener.php | 9 +- tests/App/config/config.yaml | 9 +- tests/App/config/routing.yaml | 3 + tests/App/config/security_v4.yaml | 7 +- tests/App/config/security_v5.yaml | 6 +- tests/App/config/security_v5_bc.yaml | 7 +- tests/App/config/security_v6.yaml | 4 +- .../Firewall/RefreshTokenListenerTest.php | 139 ++++++++++++++++++ 12 files changed, 220 insertions(+), 11 deletions(-) create mode 100644 src/DependencyInjection/CompilerPass/EnableRefreshOAuthTokenListenerCompilerPass.php create mode 100644 tests/Functional/Security/Http/Firewall/RefreshTokenListenerTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 7f9425718..431162574 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,10 +1,12 @@ Changelog ========= ## 2.0.0-BETA2 (202x-xx-xx) +* Enhancement: Refresh token listener is disabled by default and will only be enabled if at least one resource owner has option `refresh_on_expure` set to `true` * Deprecated: configuration parameter `firewall_names`, firewalls are now computed automatically - all firewalls that have defined `oauth` authenticator/provider will be collected. * Added: Ability to automatically refresh expired access tokens (only for derived from `GenericOAuth2ResourceOwner` resource owners), if option `refresh_on_expire` set to `true`. * Enhancement: (@internal) Removed/replaced redundant argument `$firewallNames` from controllers. If controller class was copied and replaced, adapt list of arguments: In controller use `$resourceOwnerMapLocator->getFirewallNames()`. * Changed config files from `*.xml` to `*.php` (services and routes). Xml routing configs `connect.xml`, `login.xml` and `redirect.xml` are steel present but deprecated. Please use `*.php` variants in your includes instead. +* Bugfix: RefreshTokenListener can not be lazy. If current firewall is lazy (or anonymous: lazy) then current auth token is often initializing on `kernel.response`. In this case new access token will not be stored in session. Therefore the expired token will be refreshed on each request. ## 2.0.0-BETA1 (2021-12-10) * BC Break: Dropped PHP 7.3 support, diff --git a/src/DependencyInjection/CompilerPass/EnableRefreshOAuthTokenListenerCompilerPass.php b/src/DependencyInjection/CompilerPass/EnableRefreshOAuthTokenListenerCompilerPass.php new file mode 100644 index 000000000..270e39bc2 --- /dev/null +++ b/src/DependencyInjection/CompilerPass/EnableRefreshOAuthTokenListenerCompilerPass.php @@ -0,0 +1,32 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace HWI\Bundle\OAuthBundle\DependencyInjection\CompilerPass; + +use HWI\Bundle\OAuthBundle\DependencyInjection\HWIOAuthExtension; +use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface; +use Symfony\Component\DependencyInjection\ContainerBuilder; + +final class EnableRefreshOAuthTokenListenerCompilerPass implements CompilerPassInterface +{ + public function process(ContainerBuilder $container) + { + /** @var HWIOAuthExtension $extension */ + $extension = $container->getExtension('hwi_oauth'); + + if ($extension->isRefreshTokenListenerEnabled()) { + foreach ($extension->getFirewallNames() as $firewallName => $_) { + $container->getDefinition('hwi_oauth.context_listener.token_refresher.'.$firewallName) + ->addMethodCall('enable'); + } + } + } +} diff --git a/src/DependencyInjection/HWIOAuthExtension.php b/src/DependencyInjection/HWIOAuthExtension.php index a9552694c..fa589a4d9 100644 --- a/src/DependencyInjection/HWIOAuthExtension.php +++ b/src/DependencyInjection/HWIOAuthExtension.php @@ -51,6 +51,8 @@ final class HWIOAuthExtension extends Extension */ private \ArrayIterator $firewallNames; + private bool $refreshTokenListenerEnabled = false; + public function __construct() { $this->firewallNames = new \ArrayIterator(); @@ -103,6 +105,10 @@ public function load(array $configs, ContainerBuilder $container): void foreach ($config['resource_owners'] as $name => $options) { $resourceOwners[$name] = $name; $this->createResourceOwnerService($container, $name, $options); + + if (!$this->refreshTokenListenerEnabled) { + $this->refreshTokenListenerEnabled = $options['options']['refresh_on_expire'] ?? false; + } } $container->setParameter('hwi_oauth.resource_owners', $resourceOwners); @@ -168,6 +174,11 @@ public function getFirewallNames(): \ArrayIterator return $this->firewallNames; } + public function isRefreshTokenListenerEnabled(): bool + { + return $this->refreshTokenListenerEnabled; + } + /** * Check of the connect controllers etc should be enabled. * diff --git a/src/HWIOAuthBundle.php b/src/HWIOAuthBundle.php index 796d1ff35..7022d8d5c 100644 --- a/src/HWIOAuthBundle.php +++ b/src/HWIOAuthBundle.php @@ -11,6 +11,7 @@ namespace HWI\Bundle\OAuthBundle; +use HWI\Bundle\OAuthBundle\DependencyInjection\CompilerPass\EnableRefreshOAuthTokenListenerCompilerPass; use HWI\Bundle\OAuthBundle\DependencyInjection\CompilerPass\RefreshOAuthTokenCompilerPass; use HWI\Bundle\OAuthBundle\DependencyInjection\CompilerPass\ResourceOwnerMapCompilerPass; use HWI\Bundle\OAuthBundle\DependencyInjection\Security\Factory\OAuthAuthenticatorFactory; @@ -51,6 +52,7 @@ public function build(ContainerBuilder $container) } $container->addCompilerPass(new ResourceOwnerMapCompilerPass()); + $container->addCompilerPass(new EnableRefreshOAuthTokenListenerCompilerPass()); } /** diff --git a/src/Security/Http/Firewall/AbstractRefreshAccessTokenListener.php b/src/Security/Http/Firewall/AbstractRefreshAccessTokenListener.php index d3498e1ef..63dc7518c 100644 --- a/src/Security/Http/Firewall/AbstractRefreshAccessTokenListener.php +++ b/src/Security/Http/Firewall/AbstractRefreshAccessTokenListener.php @@ -26,6 +26,8 @@ abstract class AbstractRefreshAccessTokenListener extends AbstractListener protected ResourceOwnerMap $resourceOwnerMap; + protected bool $enabled = false; + public function setTokenStorage(TokenStorageInterface $tokenStorage) { $this->tokenStorage = $tokenStorage; @@ -36,9 +38,14 @@ public function setResourceOwnerMap(ResourceOwnerMap $resourceOwnerMap) $this->resourceOwnerMap = $resourceOwnerMap; } + public function enable(bool $enabled = true) + { + $this->enabled = $enabled; + } + public function supports(Request $request): ?bool { - return null; + return $this->enabled; } public function authenticate(RequestEvent $event) diff --git a/tests/App/config/config.yaml b/tests/App/config/config.yaml index d5f8da386..7952d177e 100644 --- a/tests/App/config/config.yaml +++ b/tests/App/config/config.yaml @@ -53,7 +53,14 @@ hwi_oauth: type: google client_id: 'google_client_id' client_secret: 'google_client_secret' - scope: "https://www.googleapis.com/auth/userinfo.email https://www.googleapis.com/auth/userinfo.profile" + scope: 'https://www.googleapis.com/auth/userinfo.email https://www.googleapis.com/auth/userinfo.profile' + yahoo: + type: yahoo + client_id: 'yahoo_client_id' + client_secret: 'yahoo_client_secret' + scope: 'scope=mail-r sdct-r' + options: + refresh_on_expire: true twig: strict_variables: '%kernel.debug%' diff --git a/tests/App/config/routing.yaml b/tests/App/config/routing.yaml index cc0663556..21cf03993 100644 --- a/tests/App/config/routing.yaml +++ b/tests/App/config/routing.yaml @@ -24,3 +24,6 @@ login_landing: google_login: path: /check-login/google + +yahoo_login: + path: /check-login/yahoo diff --git a/tests/App/config/security_v4.yaml b/tests/App/config/security_v4.yaml index d1ce7013a..ed6be36ab 100644 --- a/tests/App/config/security_v4.yaml +++ b/tests/App/config/security_v4.yaml @@ -9,14 +9,15 @@ security: firewalls: login_area: pattern: ^/(login$|connect|login_hwi) - anonymous: true + anonymous: lazy context: hwi_context main: pattern: ^/ - anonymous: true + anonymous: lazy oauth: resource_owners: - google: "/check-login/google" + google: '/check-login/google' + yahoo: '/check-login/yahoo' login_path: /login use_forward: false failure_path: /login diff --git a/tests/App/config/security_v5.yaml b/tests/App/config/security_v5.yaml index 3810475f8..70ddea655 100644 --- a/tests/App/config/security_v5.yaml +++ b/tests/App/config/security_v5.yaml @@ -10,11 +10,13 @@ security: firewalls: main: + lazy: true pattern: ^/ oauth: resource_owners: - google: "/check-login/google" - login_path: /login + google: '/check-login/google' + yahoo: '/check-login/yahoo' + login_path: /login use_forward: false failure_path: /login oauth_user_provider: diff --git a/tests/App/config/security_v5_bc.yaml b/tests/App/config/security_v5_bc.yaml index b3842e2f0..9bcad1fa1 100644 --- a/tests/App/config/security_v5_bc.yaml +++ b/tests/App/config/security_v5_bc.yaml @@ -11,14 +11,15 @@ security: firewalls: login_area: pattern: ^/(login$|connect|login_hwi) - anonymous: true + anonymous: lazy context: hwi_context main: pattern: ^/ - anonymous: true + anonymous: lazy oauth: resource_owners: - google: "/check-login/google" + google: '/check-login/google' + yahoo: '/check-login/yahoo' login_path: /login use_forward: false failure_path: /login diff --git a/tests/App/config/security_v6.yaml b/tests/App/config/security_v6.yaml index 3810475f8..068f0fd96 100644 --- a/tests/App/config/security_v6.yaml +++ b/tests/App/config/security_v6.yaml @@ -10,10 +10,12 @@ security: firewalls: main: + lazy: true pattern: ^/ oauth: resource_owners: - google: "/check-login/google" + google: '/check-login/google' + yahoo: '/check-login/yahoo' login_path: /login use_forward: false failure_path: /login diff --git a/tests/Functional/Security/Http/Firewall/RefreshTokenListenerTest.php b/tests/Functional/Security/Http/Firewall/RefreshTokenListenerTest.php new file mode 100644 index 000000000..4a05320fd --- /dev/null +++ b/tests/Functional/Security/Http/Firewall/RefreshTokenListenerTest.php @@ -0,0 +1,139 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace HWI\Bundle\OAuthBundle\Tests\Functional\Security\Http\Firewall; + +use HWI\Bundle\OAuthBundle\Security\Core\Authentication\Token\OAuthToken; +use HWI\Bundle\OAuthBundle\Tests\App\AppKernel; +use HWI\Bundle\OAuthBundle\Tests\Fixtures\User; +use HWI\Bundle\OAuthBundle\Tests\Functional\AuthenticationHelperTrait; +use Symfony\Bundle\FrameworkBundle\KernelBrowser; +use Symfony\Bundle\FrameworkBundle\Test\WebTestCase; +use Symfony\Component\HttpClient\MockHttpClient; +use Symfony\Component\HttpClient\Response\MockResponse; +use Symfony\Component\HttpFoundation\Session\SessionInterface; + +final class RefreshTokenListenerTest extends WebTestCase +{ + use AuthenticationHelperTrait; + + private string $tokenResponse = <<httpClient = new MockHttpClient([ + new MockResponse($this->tokenResponse, [ + 'response_headers' => ['content-type' => 'application/json'], + ]), + new MockResponse($this->userResponse, [ + 'response_headers' => ['content-type' => 'application/json'], + ]), + ]); + + $this->client = self::createClient(); + $this->client->getContainer()->set('hwi_oauth.http_client', $this->httpClient); + } + + public static function getKernelClass(): string + { + return AppKernel::class; + } + + public function testExpiredTokenWillNotBeRefreshed(): void + { + // refresh_on_expire not set + $session = $this->createExpiredTokenAndStoreToSession('google'); + + $this->client->request('GET', '/'); + + $this->assertEquals(0, $this->httpClient->getRequestsCount()); + + $this->assertResponseIsSuccessful(); + + $securityContext = $session->get('_security_hwi_context'); + + $this->assertNotNull($securityContext); + $newToken = unserialize($securityContext); + $this->assertInstanceOf(OAuthToken::class, $newToken); + $this->assertTrue($newToken->isExpired()); + // same old expired token + $this->assertEquals(1000, $newToken->getCreatedAt()); + } + + public function testExpiredTokenWillBeRefreshed(): void + { + // refresh_on_expire: true + $session = $this->createExpiredTokenAndStoreToSession('yahoo'); + + $this->client->request('GET', '/'); + + $this->assertEquals(2, $this->httpClient->getRequestsCount()); + + $this->assertResponseIsSuccessful(); + + $securityContext = $session->get('_security_hwi_context'); + + $this->assertNotNull($securityContext); + $newToken = unserialize($securityContext); + $this->assertInstanceOf(OAuthToken::class, $newToken); + $this->assertFalse($newToken->isExpired()); + } + + private function createExpiredTokenAndStoreToSession(string $resourceOwnerName): SessionInterface + { + $expectedToken = [ + 'access_token' => 'access_token', + 'refresh_token' => 'refresh_token', + 'expires_in' => 666, + 'oauth_token_secret' => 'secret', + ]; + + $user = new User(); + $oauthToken = new OAuthToken($expectedToken, $user->getRoles()); + $oauthToken->setUser($user); + $oauthToken->setResourceOwnerName($resourceOwnerName); + $oauthToken->setCreatedAt(1000); + + $this->assertTrue($oauthToken->isExpired()); + + $session = $this->getSession($this->client); + $session->set('_security_hwi_context', serialize($oauthToken)); + $this->saveSession($this->client, $session); + + return $session; + } +}