Skip to content

Commit

Permalink
Merge pull request #1867 from gassan/bugfix-lazy-refresh-token-listener
Browse files Browse the repository at this point in the history
Bugfix: Refresh token listener should not be lazy.
  • Loading branch information
stloyd authored Dec 29, 2021
2 parents 14748d6 + 0d5956e commit a25e4a7
Show file tree
Hide file tree
Showing 12 changed files with 220 additions and 11 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
<?php

/*
* This file is part of the HWIOAuthBundle package.
*
* (c) Hardware Info <opensource@hardware.info>
*
* 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');
}
}
}
}
11 changes: 11 additions & 0 deletions src/DependencyInjection/HWIOAuthExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ final class HWIOAuthExtension extends Extension
*/
private \ArrayIterator $firewallNames;

private bool $refreshTokenListenerEnabled = false;

public function __construct()
{
$this->firewallNames = new \ArrayIterator();
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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.
*
Expand Down
2 changes: 2 additions & 0 deletions src/HWIOAuthBundle.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -51,6 +52,7 @@ public function build(ContainerBuilder $container)
}

$container->addCompilerPass(new ResourceOwnerMapCompilerPass());
$container->addCompilerPass(new EnableRefreshOAuthTokenListenerCompilerPass());
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ abstract class AbstractRefreshAccessTokenListener extends AbstractListener

protected ResourceOwnerMap $resourceOwnerMap;

protected bool $enabled = false;

public function setTokenStorage(TokenStorageInterface $tokenStorage)
{
$this->tokenStorage = $tokenStorage;
Expand All @@ -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)
Expand Down
9 changes: 8 additions & 1 deletion tests/App/config/config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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%'
Expand Down
3 changes: 3 additions & 0 deletions tests/App/config/routing.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,6 @@ login_landing:

google_login:
path: /check-login/google

yahoo_login:
path: /check-login/yahoo
7 changes: 4 additions & 3 deletions tests/App/config/security_v4.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 4 additions & 2 deletions tests/App/config/security_v5.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
7 changes: 4 additions & 3 deletions tests/App/config/security_v5_bc.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 3 additions & 1 deletion tests/App/config/security_v6.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
139 changes: 139 additions & 0 deletions tests/Functional/Security/Http/Firewall/RefreshTokenListenerTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
<?php

declare(strict_types=1);

/*
* This file is part of the HWIOAuthBundle package.
*
* (c) Hardware Info <opensource@hardware.info>
*
* 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 = <<<json
{
"access_token": "valid-access-token",
"refresh_token": "valid-refresh-token",
"expires_in": 666
}
json;

private string $userResponse = <<<json
{
"response": {
"user": {
"id": "1",
"firstName": "bar",
"lastName": "foo"
}
}
}
json;

private MockHttpClient $httpClient;

private KernelBrowser $client;

protected function setUp(): void
{
parent::setUp();

$this->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;
}
}

0 comments on commit a25e4a7

Please sign in to comment.