Skip to content

Commit

Permalink
Release refresh token only if offline_access scope is used (#191)
Browse files Browse the repository at this point in the history
* Introduce offline_access scope
* Add offline_access scope to client
* Only release refresh token if offline_access scope is used
* Validate with some tests
* Add config option to always issue refresh token

Co-authored-by: Marko Ivančić <marko.ivancic@srce.hr>
  • Loading branch information
cicnavi and Marko Ivančić authored Oct 10, 2022
1 parent 36f12eb commit 40e2ea3
Show file tree
Hide file tree
Showing 15 changed files with 500 additions and 15 deletions.
21 changes: 21 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,27 @@ All notable changes to this project will be documented in this file.
The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).

## [2.0.4]
### Fixed
- Attempt fix for 'pull access denied for symfonycorp/cli' by @pradtke in #188
- Add Access-Control-Allow-Origin header to responses, if not already present by @cicnavi in #190


## [2.0.3]
### Fixed
- Use InMemory::empty by @pkoenig10 in #186

## [2.0.2] - 2022-07-22
### Fixed
- Correct readme typo for module_oidc.php template path by @dgoosens in #168
- Allow overriding cert+key name/location by @pradtke in #167
- Fix access token timestamps, add issuer by @cicnavi in #174
- Fix PK constraint name for allowed origin table - make it unique by @cicnavi in #173
- Set restart url for authorize commands by @pradtke in #180
- Fix admin-clients link by @Pyrex-FWI in #177
- Logout tokens should have typ header with value 'logout+jwt' by @IlanaRadinsky in #185
- Fail actions on code quality issues by @pradtke in #175

## [2.0.1]
### Fixed
- Make lib/Store/* available for Symfony DI.
Expand Down
9 changes: 9 additions & 0 deletions UPGRADE.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,15 @@ not released. If access token was released, user claims will have to be fetched
Note that this option only applies to authorization code flow since implicit flow was not available in version 1.
If you are to use the spec compliant behavior, make sure to warn existing Relying Parties about the change.

Similarly, in version 1, in authorization code flow, refresh token was always released, instead of only
releasing it if the client specifically requested it using 'offline_access' scope. Since changing this
behavior is a potential breaking change for Relying Parties, in version 2 a config option
'alwaysIssueRefreshToken' is introduced to enable OpenID Providers to keep the behavior from version 1
by setting it to 'true'. If 'alwaysIssueRefreshToken' is set to 'false', refresh token will be released
only if it was requested using 'offline_access' scope. If you are to use the spec compliant behavior, make
sure to warn existing Relying Parties about the change. Note that in that case the client must have the
'offline_access' scope registered.

Token endpoint was renamed from '.../access_token.php' to '.../token.php'. This is a potential breaking change
for clients that do not fetch OP configuration from the /.well-known/openid-configuration URI dynamically, but
instead hardcode endpoints in their configuration. You should probably warn existing Relying Parties about this
Expand Down
9 changes: 8 additions & 1 deletion config-templates/module_oidc.php
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,16 @@

// The claims from the standard scopes should only be put in the ID token when no access token is issued
// For module backwards compatibility you can always include claims in the id token.
// See: https://openid.net/specs/openid-connect-core-1_0.html#rfc.section.5.4
// @see https://openid.net/specs/openid-connect-core-1_0.html#rfc.section.5.4
// @deprecated option will be removed in v3.
'alwaysAddClaimsToIdToken' => true,

// Refresh token should only be released if the client requests it using the 'offline_access' scope.
// For module backwards compatibility you can always issue refresh token.
// @see https://openid.net/specs/openid-connect-core-1_0.html#OfflineAccess
// @deprecated option will be removed in v3.
'alwaysIssueRefreshToken' => true,

// Settings regarding Authentication Processing Filters.
// Note: OIDC authN state array will not contain all of the keys which are available during SAML authN,
// like Service Provider metadata, etc.
Expand Down
4 changes: 2 additions & 2 deletions docker/conformance.sql
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ CREATE TABLE oidc_client (
);
-- Used 'httpd' host for back-channel logout url (https://httpd:8443/test/a/simplesamlphp-module-oidc/backchannel_logout)
-- since this is the hostname of conformance server while running in container environment
INSERT INTO oidc_client VALUES('_55a99a1d298da921cb27d700d4604352e51171ebc4','_8967dd97d07cc59db7055e84ac00e79005157c1132','Conformance Client 1',replace('Client 1 for Conformance Testing https://openid.net/certification/connect_op_testing/\n','\n',char(10)),'example-userpass','["https:\/\/localhost.emobix.co.uk:8443\/test\/a\/simplesamlphp-module-oidc\/callback","https:\/\/www.certification.openid.net\/test\/a\/simplesamlphp-module-oidc\/callback"]','["openid","profile","email","address","phone"]',1,1,NULL,'["https:\/\/localhost.emobix.co.uk:8443\/test\/a\/simplesamlphp-module-oidc\/post_logout_redirect"]','https://httpd:8443/test/a/simplesamlphp-module-oidc/backchannel_logout');
INSERT INTO oidc_client VALUES('_34efb61060172a11d62101bc804db789f8f9100b0e','_91a4607a1c10ba801268929b961b3f6c067ff82d21','Conformance Client 2','','example-userpass','["https:\/\/localhost.emobix.co.uk:8443\/test\/a\/simplesamlphp-module-oidc\/callback","https:\/\/www.certification.openid.net\/test\/a\/simplesamlphp-module-oidc\/callback"]','["openid","profile","email"]',1,1,NULL,NULL,NULL);
INSERT INTO oidc_client VALUES('_55a99a1d298da921cb27d700d4604352e51171ebc4','_8967dd97d07cc59db7055e84ac00e79005157c1132','Conformance Client 1',replace('Client 1 for Conformance Testing https://openid.net/certification/connect_op_testing/\n','\n',char(10)),'example-userpass','["https:\/\/localhost.emobix.co.uk:8443\/test\/a\/simplesamlphp-module-oidc\/callback","https:\/\/www.certification.openid.net\/test\/a\/simplesamlphp-module-oidc\/callback"]','["openid","profile","email","address","phone","offline_access"]',1,1,NULL,'["https:\/\/localhost.emobix.co.uk:8443\/test\/a\/simplesamlphp-module-oidc\/post_logout_redirect"]','https://httpd:8443/test/a/simplesamlphp-module-oidc/backchannel_logout');
INSERT INTO oidc_client VALUES('_34efb61060172a11d62101bc804db789f8f9100b0e','_91a4607a1c10ba801268929b961b3f6c067ff82d21','Conformance Client 2','','example-userpass','["https:\/\/localhost.emobix.co.uk:8443\/test\/a\/simplesamlphp-module-oidc\/callback","https:\/\/www.certification.openid.net\/test\/a\/simplesamlphp-module-oidc\/callback"]','["openid","profile","email","offline_access"]',1,1,NULL,NULL,NULL);
INSERT INTO oidc_client VALUES('_0afb7d18e54b2de8205a93e38ca119e62ee321d031','_944e73bbeec7850d32b68f1b5c780562c955967e4e','Conformance Client 3','Client for client_secret_post','example-userpass','["https:\/\/localhost.emobix.co.uk:8443\/test\/a\/simplesamlphp-module-oidc\/callback","https:\/\/www.certification.openid.net\/test\/a\/simplesamlphp-module-oidc\/callback"]','["openid","profile","email"]',1,1,NULL,NULL,NULL);
INSERT INTO oidc_client VALUES('_8957eda35234902ba8343c0cdacac040310f17dfca','_322d16999f9da8b5abc9e9c0c08e853f60f4dc4804','RP-Initiated Logout Client','Client for testing RP-Initiated Logout','example-userpass','["https:\/\/localhost.emobix.co.uk:8443\/test\/a\/simplesamlphp-module-oidc\/callback","https:\/\/www.certification.openid.net\/test\/a\/simplesamlphp-module-oidc\/callback"]','["openid","profile","email","address","phone"]',1,1,NULL,'["https:\/\/localhost.emobix.co.uk:8443\/test\/a\/simplesamlphp-module-oidc\/post_logout_redirect"]',NULL);
INSERT INTO oidc_client VALUES('_9fe2f7589ece1b71f5ef75a91847d71bc5125ec2a6','_3c0beb20194179c01d7796c6836f62801e9ed4b368','Back-Channel Logout Client','Client for testing Back-Channel Logout','example-userpass','["https:\/\/localhost.emobix.co.uk:8443\/test\/a\/simplesamlphp-module-oidc\/callback","https:\/\/www.certification.openid.net\/test\/a\/simplesamlphp-module-oidc\/callback"]','["openid","profile","email","address","phone"]',1,1,NULL,'["https:\/\/localhost.emobix.co.uk:8443\/test\/a\/simplesamlphp-module-oidc\/post_logout_redirect"]','https://httpd:8443/test/a/simplesamlphp-module-oidc/backchannel_logout');
Expand Down
2 changes: 2 additions & 0 deletions docker/ssp/module_oidc.php
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@
],

'alwaysAddClaimsToIdToken' => false,
'alwaysIssueRefreshToken' => false,

// Settings regarding Authentication Processing Filters.
// Note: OIDC authN state array will not contain all of the keys which are available during SAML authN,
// like Service Provider metadata, etc.
Expand Down
10 changes: 8 additions & 2 deletions lib/Factories/Grant/AuthCodeGrantFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
use SimpleSAML\Module\oidc\Repositories\AuthCodeRepository;
use SimpleSAML\Module\oidc\Repositories\RefreshTokenRepository;
use SimpleSAML\Module\oidc\Server\Grants\AuthCodeGrant;
use SimpleSAML\Module\oidc\Services\ConfigurationService;
use SimpleSAML\Module\oidc\Utils\Checker\RequestRulesManager;

class AuthCodeGrantFactory
Expand Down Expand Up @@ -51,20 +52,24 @@ class AuthCodeGrantFactory
*/
private $requestRulesManager;

private ConfigurationService $configurationService;

public function __construct(
AuthCodeRepository $authCodeRepository,
AccessTokenRepository $accessTokenRepository,
RefreshTokenRepository $refreshTokenRepository,
\DateInterval $refreshTokenDuration,
\DateInterval $authCodeDuration,
RequestRulesManager $requestRulesManager
RequestRulesManager $requestRulesManager,
ConfigurationService $configurationService
) {
$this->authCodeRepository = $authCodeRepository;
$this->accessTokenRepository = $accessTokenRepository;
$this->refreshTokenRepository = $refreshTokenRepository;
$this->refreshTokenDuration = $refreshTokenDuration;
$this->authCodeDuration = $authCodeDuration;
$this->requestRulesManager = $requestRulesManager;
$this->configurationService = $configurationService;
}

public function build(): AuthCodeGrant
Expand All @@ -74,7 +79,8 @@ public function build(): AuthCodeGrant
$this->accessTokenRepository,
$this->refreshTokenRepository,
$this->authCodeDuration,
$this->requestRulesManager
$this->requestRulesManager,
$this->configurationService
);
$authCodeGrant->setRefreshTokenTTL($this->refreshTokenDuration);

Expand Down
32 changes: 26 additions & 6 deletions lib/Server/Grants/AuthCodeGrant.php
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
use SimpleSAML\Module\oidc\Server\ResponseTypes\Interfaces\AuthTimeResponseTypeInterface;
use SimpleSAML\Module\oidc\Server\ResponseTypes\Interfaces\NonceResponseTypeInterface;
use SimpleSAML\Module\oidc\Server\ResponseTypes\Interfaces\SessionIdResponseTypeInterface;
use SimpleSAML\Module\oidc\Services\ConfigurationService;
use SimpleSAML\Module\oidc\Utils\Arr;
use SimpleSAML\Module\oidc\Utils\Checker\Interfaces\ResultBagInterface;
use SimpleSAML\Module\oidc\Utils\Checker\RequestRulesManager;
Expand All @@ -49,8 +50,10 @@
use SimpleSAML\Module\oidc\Utils\Checker\Rules\RedirectUriRule;
use SimpleSAML\Module\oidc\Utils\Checker\Rules\RequestedClaimsRule;
use SimpleSAML\Module\oidc\Utils\Checker\Rules\RequestParameterRule;
use SimpleSAML\Module\oidc\Utils\Checker\Rules\ScopeOfflineAccessRule;
use SimpleSAML\Module\oidc\Utils\Checker\Rules\ScopeRule;
use SimpleSAML\Module\oidc\Utils\Checker\Rules\StateRule;
use SimpleSAML\Module\oidc\Utils\ScopeHelper;
use stdClass;
use Throwable;

Expand Down Expand Up @@ -85,14 +88,17 @@ class AuthCodeGrant extends OAuth2AuthCodeGrant implements

private RequestRulesManager $requestRulesManager;

protected ConfigurationService $configurationService;

protected bool $requireCodeChallengeForPublicClients = true;

public function __construct(
OAuth2AuthCodeRepositoryInterface $authCodeRepository,
AccessTokenRepositoryInterface $accessTokenRepository,
RefreshTokenRepositoryInterface $refreshTokenRepository,
DateInterval $authCodeTTL,
RequestRulesManager $requestRulesManager
RequestRulesManager $requestRulesManager,
ConfigurationService $configurationService
) {
parent::__construct($authCodeRepository, $refreshTokenRepository, $authCodeTTL);

Expand All @@ -102,6 +108,7 @@ public function __construct(

$this->authCodeTTL = $authCodeTTL;
$this->requestRulesManager = $requestRulesManager;
$this->configurationService = $configurationService;

if (in_array('sha256', hash_algos(), true)) {
$s256Verifier = new S256Verifier();
Expand Down Expand Up @@ -434,12 +441,24 @@ public function respondToAccessTokenRequest(
$responseType->setSessionId($authCodePayload->session_id);
}

// Issue and persist new refresh token if given
$refreshToken = $this->issueRefreshToken($accessToken, $authCodePayload->auth_code_id);
// Refresh token should only be released if the client requests it using the 'offline_access' scope.
// However, for module backwards compatibility we have enabled the deployer to explicitly state that
// the refresh token should always be released.
// @see https://openid.net/specs/openid-connect-core-1_0.html#OfflineAccess
// TODO in v3 remove this config option and do as per spec.
$alwaysIssueRefreshToken = $this->configurationService
->getOpenIDConnectConfiguration()
->getBoolean('alwaysIssueRefreshToken', true);

// Release refresh token per deployer config or if it is requested by using offline_access scope.
if ($alwaysIssueRefreshToken || ScopeHelper::scopeExists($scopes, 'offline_access')) {
// Issue and persist new refresh token if given
$refreshToken = $this->issueRefreshToken($accessToken, $authCodePayload->auth_code_id);

if ($refreshToken !== null) {
$this->getEmitter()->emit(new RequestEvent(RequestEvent::REFRESH_TOKEN_ISSUED, $request));
$responseType->setRefreshToken($refreshToken);
if ($refreshToken !== null) {
$this->getEmitter()->emit(new RequestEvent(RequestEvent::REFRESH_TOKEN_ISSUED, $request));
$responseType->setRefreshToken($refreshToken);
}
}

// Revoke used auth code
Expand Down Expand Up @@ -506,6 +525,7 @@ public function validateAuthorizationRequestWithCheckerResultBag(
ScopeRule::class,
RequestedClaimsRule::class,
AcrValuesRule::class,
ScopeOfflineAccessRule::class,
];

// Since we have already validated redirect_uri and we have state, make it available for other checkers.
Expand Down
1 change: 1 addition & 0 deletions lib/Server/ResponseTypes/IdTokenResponse.php
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ protected function getExtraParams(AccessTokenEntityInterface $accessToken): arra
}
// Per https://openid.net/specs/openid-connect-core-1_0.html#rfc.section.5.4 certain claims
// should only be added in certain scenarios. Allow deployer to control this.
// TODO in v3 remove this config option and do as per spec.
$addClaimsFromScopes = $this->configurationService
->getOpenIDConnectConfiguration()
->getBoolean('alwaysAddClaimsToIdToken', true);
Expand Down
5 changes: 4 additions & 1 deletion lib/Services/ConfigurationService.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ class ConfigurationService
'openid' => [
'description' => 'openid',
],
'offline_access' => [
'description' => 'offline_access',
],
'profile' => [
'description' => 'profile',
],
Expand Down Expand Up @@ -116,7 +119,7 @@ private function validateConfiguration()
* @throws ConfigurationError
*/
function (array $scope, string $name): void {
if (in_array($name, ['openid', 'profile', 'email', 'address', 'phone'], true)) {
if (in_array($name, array_keys(self::$standardClaims), true)) {
throw new ConfigurationError('Can not overwrite protected scope: ' . $name, 'oidc_config.php');
}
if (!array_key_exists('description', $scope)) {
Expand Down
5 changes: 4 additions & 1 deletion lib/Services/Container.php
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@
use SimpleSAML\Module\oidc\Utils\Checker\Rules\RequiredNonceRule;
use SimpleSAML\Module\oidc\Utils\Checker\Rules\RequiredOpenIdScopeRule;
use SimpleSAML\Module\oidc\Utils\Checker\Rules\ResponseTypeRule;
use SimpleSAML\Module\oidc\Utils\Checker\Rules\ScopeOfflineAccessRule;
use SimpleSAML\Module\oidc\Utils\Checker\Rules\ScopeRule;
use SimpleSAML\Module\oidc\Utils\Checker\Rules\StateRule;
use SimpleSAML\Module\oidc\Utils\Checker\Rules\IdTokenHintRule;
Expand Down Expand Up @@ -204,6 +205,7 @@ public function __construct()
new PostLogoutRedirectUriRule($clientRepository),
new UiLocalesRule(),
new AcrValuesRule(),
new ScopeOfflineAccessRule($configurationService),
];
$requestRuleManager = new RequestRulesManager($requestRules, $loggerService);
$this->services[RequestRulesManager::class] = $requestRuleManager;
Expand Down Expand Up @@ -251,7 +253,8 @@ public function __construct()
$refreshTokenRepository,
$refreshTokenDuration,
$authCodeDuration,
$requestRuleManager
$requestRuleManager,
$configurationService
);
$this->services[AuthCodeGrant::class] = $authCodeGrantFactory->build();

Expand Down
77 changes: 77 additions & 0 deletions lib/Utils/Checker/Rules/ScopeOfflineAccessRule.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
<?php

namespace SimpleSAML\Module\oidc\Utils\Checker\Rules;

use League\OAuth2\Server\Entities\ScopeEntityInterface;
use Psr\Http\Message\ServerRequestInterface;
use SimpleSAML\Module\oidc\Entity\Interfaces\ClientEntityInterface;
use SimpleSAML\Module\oidc\Server\Exceptions\OidcServerException;
use SimpleSAML\Module\oidc\Services\ConfigurationService;
use SimpleSAML\Module\oidc\Services\LoggerService;
use SimpleSAML\Module\oidc\Utils\Checker\Interfaces\ResultBagInterface;
use SimpleSAML\Module\oidc\Utils\Checker\Interfaces\ResultInterface;
use SimpleSAML\Module\oidc\Utils\Checker\Result;
use SimpleSAML\Module\oidc\Utils\ScopeHelper;

class ScopeOfflineAccessRule extends AbstractRule
{
protected ConfigurationService $configurationService;

public function __construct(ConfigurationService $configurationService)
{
$this->configurationService = $configurationService;
}

/**
* @inheritDoc
*/
public function checkRule(
ServerRequestInterface $request,
ResultBagInterface $currentResultBag,
LoggerService $loggerService,
array $data = [],
bool $useFragmentInHttpErrorResponses = false,
array $allowedServerRequestMethods = ['GET']
): ?ResultInterface {
/** @var string $redirectUri */
$redirectUri = $currentResultBag->getOrFail(RedirectUriRule::class)->getValue();
/** @var string|null $state */
$state = $currentResultBag->getOrFail(StateRule::class)->getValue();
/** @var ClientEntityInterface $client */
$client = $currentResultBag->getOrFail(ClientIdRule::class)->getValue();
/** @var ScopeEntityInterface[] $validScopes */
$validScopes = $currentResultBag->getOrFail(ScopeRule::class)->getValue();

// Refresh token should only be released if the client requests it using the 'offline_access' scope.
// However, for module backwards compatibility we have enabled the deployer to explicitly state that
// the refresh token should always be released.
// @see https://openid.net/specs/openid-connect-core-1_0.html#OfflineAccess
// TODO in v3 remove this config option and do as per spec.
$alwaysIssueRefreshToken = $this->configurationService
->getOpenIDConnectConfiguration()
->getBoolean('alwaysIssueRefreshToken', true);
// If the deployer decided to always issue refresh token, we don't have to check offline_access scope.
if ($alwaysIssueRefreshToken) {
return new Result($this->getKey(), true);
}

// Check if offline_access scope is used. If not, we don't have to check anything else.
if (! ScopeHelper::scopeExists($validScopes, 'offline_access')) {
return new Result($this->getKey(), false);
}

// Scope offline_access is used. Check if the client has it registered.
if (! in_array('offline_access', $client->getScopes())) {
throw OidcServerException::invalidRequest(
'scope',
'offline_access scope is not registered for the client',
null,
$redirectUri,
$state,
$useFragmentInHttpErrorResponses
);
}

return new Result($this->getKey(), true);
}
}
Loading

0 comments on commit 40e2ea3

Please sign in to comment.