Skip to content

Commit ac4aabc

Browse files
robertlemkerobertlemke
authored
Merge pull request #12 from flownative/bugfix-authorization-reuse
Authorization ID must not be deterministic for authorization code flow
2 parents 6e60d06 + de5116f commit ac4aabc

File tree

9 files changed

+412
-118
lines changed

9 files changed

+412
-118
lines changed

‎Classes/Authorization.php

Lines changed: 52 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,12 @@
1414
*/
1515

1616
use Doctrine\ORM\Mapping as ORM;
17+
use Exception;
18+
use JsonException;
1719
use League\OAuth2\Client\Token\AccessToken;
1820
use League\OAuth2\Client\Token\AccessTokenInterface;
1921
use Neos\Flow\Annotations as Flow;
22+
use Ramsey\Uuid\Uuid;
2023

2124
/**
2225
* An OAuth2 Authorization
@@ -45,12 +48,6 @@ class Authorization
4548
*/
4649
protected $clientId;
4750

48-
/**
49-
* @var string
50-
* @ORM\Column(nullable = true, length=5000)
51-
*/
52-
protected $clientSecret;
53-
5451
/**
5552
* @var string
5653
*/
@@ -62,38 +59,59 @@ class Authorization
6259
protected $scope;
6360

6461
/**
65-
* @var array
66-
* @ORM\Column(type="json_array", nullable = true)
62+
* @var string
63+
* @ORM\Column(nullable = true, type = "text")
6764
*/
6865
protected $serializedAccessToken;
6966

7067
/**
68+
* @param string $authorizationId
7169
* @param string $serviceName
7270
* @param string $clientId
7371
* @param string $grantType
7472
* @param string $scope
7573
*/
76-
public function __construct(string $serviceName, string $clientId, string $grantType, string $scope)
74+
public function __construct(string $authorizationId, string $serviceName, string $clientId, string $grantType, string $scope)
7775
{
78-
$this->authorizationId = self::calculateAuthorizationId($serviceName, $clientId, $scope, $grantType);
76+
$this->authorizationId = $authorizationId;
7977
$this->serviceName = $serviceName;
8078
$this->clientId = $clientId;
8179
$this->grantType = $grantType;
8280
$this->scope = $scope;
8381
}
8482

83+
/**
84+
* Calculate an authorization identifier (for this model) from the given parameters.
85+
*
86+
* @param string $serviceType
87+
* @param string $serviceName
88+
* @param string $clientId
89+
* @return string
90+
* @throws OAuthClientException
91+
*/
92+
public static function generateAuthorizationIdForAuthorizationCodeGrant(string $serviceType, string $serviceName, string $clientId): string
93+
{
94+
try {
95+
return $serviceType . '-' . $serviceName . '-' . Uuid::uuid4()->toString();
96+
// @codeCoverageIgnoreStart
97+
} catch (Exception $e) {
98+
throw new OAuthClientException(sprintf('Failed generating authorization id for %s %s', $serviceName, $clientId), 1597311416, $e);
99+
}
100+
// @codeCoverageIgnoreEnd
101+
}
102+
85103
/**
86104
* Calculate an authorization identifier (for this model) from the given parameters.
87105
*
88106
* @param string $serviceName
89107
* @param string $clientId
108+
* @param string $clientSecret
90109
* @param string $scope
91-
* @param string $grantType
92110
* @return string
93111
*/
94-
public static function calculateAuthorizationId(string $serviceName, string $clientId, string $scope, string $grantType): string
112+
public static function generateAuthorizationIdForClientCredentialsGrant(string $serviceName, string $clientId, string $clientSecret, string $scope): string
95113
{
96-
return sha1($serviceName . $clientId . $scope. $grantType);
114+
return hash('sha512', $serviceName . $clientId . $clientSecret . $scope . self::GRANT_CLIENT_CREDENTIALS);
97115
}
98116

99117
/**
@@ -120,22 +138,6 @@ public function getClientId(): string
120138
return $this->clientId;
121139
}
122140

123-
/**
124-
* @param string $clientSecret
125-
*/
126-
public function setClientSecret(string $clientSecret): void
127-
{
128-
$this->clientSecret = $clientSecret;
129-
}
130-
131-
/**
132-
* @return string
133-
*/
134-
public function getClientSecret(): string
135-
{
136-
return $this->clientSecret;
137-
}
138-
139141
/**
140142
* @return string
141143
*/
@@ -162,35 +164,49 @@ public function setScope(string $scope): void
162164
}
163165

164166
/**
165-
* @return array
167+
* @return string
166168
*/
167-
public function getSerializedAccessToken(): array
169+
public function getSerializedAccessToken(): string
168170
{
169-
return $this->serializedAccessToken ?? [];
171+
return $this->serializedAccessToken ?? '';
170172
}
171173

172174
/**
173-
* @param array $serializedAccessToken
175+
* @param string $serializedAccessToken
174176
*/
175-
public function setSerializedAccessToken(array $serializedAccessToken): void
177+
public function setSerializedAccessToken(string $serializedAccessToken): void
176178
{
177179
$this->serializedAccessToken = $serializedAccessToken;
178180
}
179181

180182
/**
181183
* @param AccessTokenInterface $accessToken
182184
* @return void
185+
* @throws \InvalidArgumentException
183186
*/
184187
public function setAccessToken(AccessTokenInterface $accessToken): void
185188
{
186-
$this->serializedAccessToken = $accessToken->jsonSerialize();
189+
try {
190+
$this->serializedAccessToken = json_encode($accessToken, JSON_THROW_ON_ERROR, 512);
191+
// @codeCoverageIgnoreStart
192+
} catch (JsonException $e) {
193+
throw new \InvalidArgumentException('Failed serializing the given access token', 1602515717);
194+
// @codeCoverageIgnoreEnd
195+
}
187196
}
188197

189198
/**
190199
* @return AccessToken
191200
*/
192201
public function getAccessToken(): ?AccessToken
193202
{
194-
return !empty($this->serializedAccessToken) ? new AccessToken($this->serializedAccessToken) : null;
203+
try {
204+
if (!empty($this->serializedAccessToken)) {
205+
$unserializedAccessToken = json_decode($this->serializedAccessToken, true, 512, JSON_THROW_ON_ERROR);
206+
return new AccessToken($unserializedAccessToken);
207+
}
208+
} catch (JsonException $e) {
209+
}
210+
return null;
195211
}
196212
}

‎Classes/Controller/OAuthController.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,8 @@
33

44
use Flownative\OAuth2\Client\OAuthClient;
55
use Flownative\OAuth2\Client\OAuthClientException;
6+
use GuzzleHttp\Psr7\Uri;
67
use Neos\Flow\Annotations\CompileStatic;
7-
use Neos\Flow\Http\Uri;
88
use Neos\Flow\Mvc\Controller\ActionController;
99
use Neos\Flow\Mvc\Exception\StopActionException;
1010
use Neos\Flow\Mvc\Exception\UnsupportedRequestTypeException;

‎Classes/OAuthClient.php

Lines changed: 36 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -6,12 +6,10 @@
66
use Doctrine\ORM\ORMException;
77
use GuzzleHttp\Client;
88
use GuzzleHttp\Exception\GuzzleException;
9-
use GuzzleHttp\Psr7\Request;
109
use GuzzleHttp\Psr7\Response;
1110
use GuzzleHttp\Psr7\Uri;
1211
use League\OAuth2\Client\Provider\Exception\IdentityProviderException;
1312
use League\OAuth2\Client\Provider\GenericProvider;
14-
use League\OAuth2\Client\Token\AccessTokenInterface;
1513
use League\OAuth2\Client\Tool\RequestFactory;
1614
use Neos\Cache\Exception;
1715
use Neos\Cache\Frontend\VariableFrontend;
@@ -39,7 +37,7 @@ abstract class OAuthClient
3937
*
4038
* @const string
4139
*/
42-
public const AUTHORIZATION_ID_QUERY_PARAMETER_NAME = 'flownative_oauth2_authorization_id';
40+
public const AUTHORIZATION_ID_QUERY_PARAMETER_NAME_PREFIX = 'flownative_oauth2_authorization_id';
4341

4442
/**
4543
* @var string
@@ -124,14 +122,14 @@ public function injectEntityManager(EntityManagerInterface $entityManager): void
124122
/**
125123
* Returns the service type, i.e. a specific implementation of this client to use
126124
*
127-
* @return string For example, "FlownativeBeach", "oidc", ...
125+
* @return string For example, "Github", "oidc", ...
128126
*/
129127
abstract public function getServiceType(): string;
130128

131129
/**
132130
* Returns the service name, i.e. something like an instance name of the concrete implementation of this client
133131
*
134-
* @return string For example, "Github", "MySpecialService", ...
132+
* @return string For example, "SpecificGithubConnection", "MySpecialService", ...
135133
*/
136134
public function getServiceName(): string
137135
{
@@ -199,25 +197,36 @@ public function getRequestFactory(): RequestFactory
199197
}
200198

201199
/**
202-
* Requests an access token using a specified grant type.
200+
* Generates the URL query parameter name which is used for passing the authorization id of a
201+
* finishing authorization to Flow (via the "Return URL").
203202
*
204-
* This method is usually used using the OAuth Client Credentials Flow for machine-to-machine applications.
205-
* Therefore the grant type is usually Authorization::GRANT_CLIENT_CREDENTIALS. You need to specify the
203+
* @param string $serviceType "Class" of the of the service, for example, "Github", "oidc", ...
204+
* @return string
205+
*/
206+
public static function generateAuthorizationIdQueryParameterName(string $serviceType): string
207+
{
208+
return self::AUTHORIZATION_ID_QUERY_PARAMETER_NAME_PREFIX . '_' . $serviceType;
209+
}
210+
211+
/**
212+
* Requests an access token.
213+
*
214+
* This method is used using the OAuth Client Credentials Flow for machine-to-machine applications.
215+
* Therefore the grant type must be Authorization::GRANT_CLIENT_CREDENTIALS. You need to specify the
206216
* client identifier and client secret and may optionally specify a scope.
207217
*
208218
* @param string $serviceName
209219
* @param string $clientId Client ID
210220
* @param string $clientSecret Client Secret
211221
* @param string $scope Scope which may consist of multiple identifiers, separated by comma
212-
* @param string $grantType One of the Authorization::GRAND_* constants
213222
* @param array $additionalParameters Additional parameters to provide in the request body while requesting the token. For example ['audience' => 'https://www.example.com/api/v1']
214223
* @return void
215224
* @throws IdentityProviderException
216225
*/
217-
public function requestAccessToken(string $serviceName, string $clientId, string $clientSecret, string $scope, string $grantType, array $additionalParameters = []): void
226+
public function requestAccessToken(string $serviceName, string $clientId, string $clientSecret, string $scope, array $additionalParameters = []): void
218227
{
219-
$authorizationId = Authorization::calculateAuthorizationId($serviceName, $clientId, $scope, $grantType);
220-
$this->logger->info(sprintf('OAuth (%s): Retrieving access token using %s grant for client "%s" using a %s bytes long secret. (authorization id: %s)', $this->getServiceType(), $grantType, $clientId, strlen($clientSecret), $authorizationId));
228+
$authorizationId = Authorization::generateAuthorizationIdForClientCredentialsGrant($serviceName, $clientId, $clientSecret, $scope);
229+
$this->logger->info(sprintf('OAuth (%s): Retrieving access token using client credentials grant for client "%s" using a %s bytes long secret. (authorization id: %s)', $this->getServiceType(), $clientId, strlen($clientSecret), $authorizationId));
221230

222231
$existingAuthorization = $this->getAuthorization($authorizationId);
223232
if ($existingAuthorization !== null) {
@@ -227,8 +236,9 @@ public function requestAccessToken(string $serviceName, string $clientId, string
227236
$this->logger->info(sprintf('OAuth (%s): Removed old OAuth token for client "%s". (authorization id: %s)', $this->getServiceType(), $clientId, $authorizationId));
228237
}
229238

230-
$accessToken = $this->createOAuthProvider($clientId, $clientSecret)->getAccessToken($grantType, $additionalParameters);
231-
$authorization = $this->createNewAuthorization($serviceName, $clientId, $scope, $grantType, $accessToken);
239+
$accessToken = $this->createOAuthProvider($clientId, $clientSecret)->getAccessToken(Authorization::GRANT_CLIENT_CREDENTIALS, $additionalParameters);
240+
$authorization = new Authorization($authorizationId, $serviceName, $clientId, Authorization::GRANT_CLIENT_CREDENTIALS, $scope);
241+
$authorization->setAccessToken($accessToken);
232242

233243
$this->logger->info(sprintf('OAuth (%s): Persisted new OAuth authorization %s for client "%s" with expiry time %s. (authorization id: %s)', $this->getServiceType(), $authorizationId, $clientId, $accessToken->getExpires(), $authorizationId));
234244

@@ -248,15 +258,15 @@ public function requestAccessToken(string $serviceName, string $clientId, string
248258
*/
249259
public function startAuthorization(string $clientId, string $clientSecret, UriInterface $returnToUri, string $scope): UriInterface
250260
{
251-
$authorization = new Authorization($this->getServiceType(), $clientId, Authorization::GRANT_AUTHORIZATION_CODE, $scope);
261+
$authorizationId = Authorization::generateAuthorizationIdForAuthorizationCodeGrant($this->getServiceType(), $this->getServiceName(), $clientId);
262+
$authorization = new Authorization($authorizationId, $this->getServiceType(), $clientId, Authorization::GRANT_AUTHORIZATION_CODE, $scope);
252263
$this->logger->info(sprintf('OAuth (%s): Starting authorization %s using client id "%s", a %s bytes long secret and scope "%s".', $this->getServiceType(), $authorization->getAuthorizationId(), $clientId, strlen($clientSecret), $scope));
253264

254265
try {
255266
$oldAuthorization = $this->entityManager->find(Authorization::class, $authorization->getAuthorizationId());
256267
if ($oldAuthorization !== null) {
257268
$authorization = $oldAuthorization;
258269
}
259-
$authorization->setClientSecret($clientSecret);
260270
$this->entityManager->persist($authorization);
261271
$this->entityManager->flush();
262272
} catch (ORMException $exception) {
@@ -315,6 +325,10 @@ public function finishAuthorization(string $stateIdentifier, string $code, strin
315325
throw new OAuthClientException(sprintf('OAuth2 (%s): Finishing authorization failed because authorization %s could not be retrieved from the database.', $this->getServiceType(), $authorizationId), 1568710771);
316326
}
317327

328+
if ($authorization->getGrantType() !== Authorization::GRANT_AUTHORIZATION_CODE) {
329+
throw new OAuthClientException(sprintf('OAuth2 (%s): Finishing authorization failed because authorization %s does not have the authorization code flow type!', $this->getServiceType(), $authorizationId), 1597312780);
330+
}
331+
318332
$this->logger->debug(sprintf('OAuth (%s): Retrieving an OAuth access token for authorization "%s" in exchange for the code %s', $this->getServiceType(), $authorizationId, str_repeat('*', strlen($code) - 3) . substr($code, -3, 3)));
319333
$accessToken = $oAuthProvider->getAccessToken(Authorization::GRANT_AUTHORIZATION_CODE, ['code' => $code]);
320334
$this->logger->info(sprintf('OAuth (%s): Persisting OAuth token for authorization "%s" with expiry time %s.', $this->getServiceType(), $authorizationId, $accessToken->getExpires()));
@@ -333,7 +347,7 @@ public function finishAuthorization(string $stateIdentifier, string $code, strin
333347
}
334348

335349
$returnToUri = new Uri($stateFromCache['returnToUri']);
336-
$returnToUri = $returnToUri->withQuery(trim($returnToUri->getQuery() . '&' . self::AUTHORIZATION_ID_QUERY_PARAMETER_NAME . '=' . $authorizationId, '&'));
350+
$returnToUri = $returnToUri->withQuery(trim($returnToUri->getQuery() . '&' . self::generateAuthorizationIdQueryParameterName($this->getServiceType()) . '=' . $authorizationId, '&'));
337351

338352
$this->logger->debug(sprintf('OAuth (%s): Finished authorization "%s", $returnToUri is %s.', $this->getServiceType(), $authorizationId, $returnToUri));
339353
return $returnToUri;
@@ -350,11 +364,13 @@ public function finishAuthorization(string $stateIdentifier, string $code, strin
350364
*/
351365
public function refreshAuthorization(string $authorizationId, string $clientId, string $returnToUri): string
352366
{
367+
throw new OAuthClientException('refreshAuthorization is currently not implemented', 1602519541);
368+
353369
$authorization = $this->entityManager->find(Authorization::class, ['authorizationId' => $authorizationId]);
354370
if (!$authorization instanceof Authorization) {
355371
throw new OAuthClientException(sprintf('OAuth2: Could not refresh OAuth token because authorization %s was not found in our database.', $authorization), 1505317044316);
356372
}
357-
$oAuthProvider = $this->createOAuthProvider($clientId, $authorization->getClientSecret());
373+
$oAuthProvider = $this->createOAuthProvider($clientId, $authorization->getClientSecret()); // FIXME
358374

359375
$this->logger->info(sprintf('OAuth (%s): Refreshing authorization %s for client "%s" using a %s bytes long secret and refresh token "%s".', $this->getServiceType(), $authorizationId, $clientId, strlen($authorization->getClientSecret()), $authorization->refreshToken));
360376

@@ -375,6 +391,8 @@ public function refreshAuthorization(string $authorizationId, string $clientId,
375391
}
376392

377393
/**
394+
* Returns the specified Authorization record, if it exists
395+
*
378396
* @param string $authorizationId
379397
* @return Authorization|null
380398
*/
@@ -463,23 +481,6 @@ public function renderFinishAuthorizationUri(): string
463481
}
464482
}
465483

466-
/**
467-
* Create a new OAuthToken instance
468-
*
469-
* @param string $serviceName
470-
* @param string $clientId
471-
* @param string $scope
472-
* @param string $grantType
473-
* @param AccessTokenInterface $accessToken
474-
* @return Authorization
475-
*/
476-
protected function createNewAuthorization(string $serviceName, string $clientId, string $scope, string $grantType, AccessTokenInterface $accessToken): Authorization
477-
{
478-
$authorization = new Authorization($serviceName, $clientId, $grantType, $scope);
479-
$authorization->setAccessToken($accessToken);
480-
return $authorization;
481-
}
482-
483484
/**
484485
* @param string $clientId
485486
* @param string $clientSecret

0 commit comments

Comments
 (0)