Skip to content

Commit b651573

Browse files
robertlemkerobertlemke
committed
Refactor Authorization database model and state usage
1 parent 9da7e3e commit b651573

File tree

4 files changed

+89
-42
lines changed

4 files changed

+89
-42
lines changed

‎Classes/Authorization.php‎

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@
2525
*/
2626
class Authorization
2727
{
28+
public const GRANT_AUTHORIZATION_CODE = 'authorization_code';
29+
2830
/**
2931
* @ORM\Id
3032
* @var string
@@ -67,18 +69,30 @@ class Authorization
6769
/**
6870
* @param string $serviceName
6971
* @param string $clientId
70-
* @param string $clientSecret
7172
* @param string $grantType
7273
* @param string $scope
7374
*/
74-
public function __construct(string $serviceName, string $clientId, string $clientSecret, string $grantType, string $scope)
75+
public function __construct(string $serviceName, string $clientId, string $grantType, string $scope)
7576
{
77+
$this->authorizationId = self::calculateAuthorizationId($serviceName, $clientId, $grantType, $scope);
7678
$this->serviceName = $serviceName;
7779
$this->clientId = $clientId;
78-
$this->clientSecret = $clientSecret;
7980
$this->grantType = $grantType;
8081
$this->scope = $scope;
81-
$this->authorizationId = sha1($serviceName . $clientId . $grantType . $scope);
82+
}
83+
84+
/**
85+
* Calculate an authorization identifier (for this model) from the given parameters.
86+
*
87+
* @param string $serviceName
88+
* @param string $clientId
89+
* @param string $grantType
90+
* @param string $scope
91+
* @return string
92+
*/
93+
public static function calculateAuthorizationId(string $serviceName, string $clientId, string $grantType, string $scope): string
94+
{
95+
return sha1($serviceName . $clientId . $grantType . $scope);
8296
}
8397

8498
/**
@@ -105,6 +119,14 @@ public function getClientId(): string
105119
return $this->clientId;
106120
}
107121

122+
/**
123+
* @param string $clientSecret
124+
*/
125+
public function setClientSecret(string $clientSecret): void
126+
{
127+
$this->clientSecret = $clientSecret;
128+
}
129+
108130
/**
109131
* @return string
110132
*/

‎Classes/Controller/OAuthController.php‎

Lines changed: 3 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -59,17 +59,12 @@ public function startAuthorizationAction(string $clientId, string $clientSecret,
5959
* @param string $serviceType
6060
* @param string $serviceName
6161
* @param string $scope
62-
* @throws OAuthClientException
63-
* @throws \Doctrine\ORM\ORMException
64-
* @throws \Doctrine\ORM\OptimisticLockException
65-
* @throws \Doctrine\ORM\TransactionRequiredException
66-
* @throws \Neos\Flow\Mvc\Exception\StopActionException
67-
* @throws \Neos\Flow\Mvc\Exception\UnsupportedRequestTypeException
62+
* @throws
6863
*/
6964
public function finishAuthorizationAction(string $code, string $state, string $serviceType, string $serviceName, string $scope = ''): void
7065
{
7166
if (!isset($this->serviceTypes[$serviceType])) {
72-
throw new OAuthClientException(sprintf('Failed finishing OAuth2 authorization because the given service type "%s" is unknown.', $serviceName), 1511193117184);
67+
throw new OAuthClientException(sprintf('OAuth: Failed finishing OAuth2 authorization because the given service type "%s" is unknown.', $serviceName), 1511193117184);
7368
}
7469

7570
$client = new $this->serviceTypes[$serviceType]($serviceName);
@@ -85,12 +80,7 @@ public function finishAuthorizationAction(string $code, string $state, string $s
8580
* @param string $clientId
8681
* @param string $returnToUri
8782
* @param string $serviceName
88-
* @throws OAuthClientException
89-
* @throws \Doctrine\ORM\ORMException
90-
* @throws \Doctrine\ORM\OptimisticLockException
91-
* @throws \Doctrine\ORM\TransactionRequiredException
92-
* @throws \Neos\Flow\Mvc\Exception\StopActionException
93-
* @throws \Neos\Flow\Mvc\Exception\UnsupportedRequestTypeException
83+
* @throws
9484
*/
9585
public function refreshAuthorizationAction(string $clientId, string $returnToUri, string $serviceName): void
9686
{

‎Classes/OAuthClient.php‎

Lines changed: 42 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,12 @@
3434

3535
abstract class OAuthClient
3636
{
37-
public const STATE_QUERY_PARAMETER_NAME = 'flownative_oauth2_state';
37+
/**
38+
* Name of the HTTP query parameter used for passing around the authorization id
39+
*
40+
* @const string
41+
*/
42+
public const AUTHORIZATION_ID_QUERY_PARAMETER_NAME = 'flownative_oauth2_authorization_id';
3843

3944
/**
4045
* @var string
@@ -235,38 +240,52 @@ public function getAccessToken(string $grant, string $clientId, string $clientSe
235240
* @param string $clientId The client id, as provided by the OAuth server
236241
* @param string $clientSecret The client secret, provided by the OAuth server
237242
* @param Uri $returnToUri URI to return to when authorization is finished
238-
* @param array $scopes Scopes to request for authorization
243+
* @param string $scope Scope to request for authorization. Must be scope ids separated by space, e.g. "openid profile email"
239244
* @return Uri The URL the browser should redirect to, asking the user to authorize
240245
* @throws OAuthClientException
241246
*/
242-
public function startAuthorization(string $clientId, string $clientSecret, Uri $returnToUri, array $scopes = []): Uri
247+
public function startAuthorization(string $clientId, string $clientSecret, Uri $returnToUri, string $scope): Uri
243248
{
249+
$authorization = new Authorization($this->getServiceType(), $clientId, Authorization::GRANT_AUTHORIZATION_CODE, $scope);
250+
$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));
251+
252+
try {
253+
$oldAuthorization = $this->entityManager->find(Authorization::class, $authorization->getAuthorizationId());
254+
if ($oldAuthorization !== null) {
255+
$authorization = $oldAuthorization;
256+
}
257+
$authorization->setClientSecret($clientSecret);
258+
$this->entityManager->persist($authorization);
259+
$this->entityManager->flush();
260+
} catch (ORMException $exception) {
261+
throw new OAuthClientException(sprintf('OAuth (%s): Failed storing authorization in database: %s', $this->getServiceType(), $exception->getMessage()), 1568727133);
262+
}
263+
244264
$oAuthProvider = $this->createOAuthProvider($clientId, $clientSecret);
245-
$authorizationUri = new Uri($oAuthProvider->getAuthorizationUrl(['scope' => implode(' ', $scopes)]));
246-
$stateIdentifier = $oAuthProvider->getState();
265+
$authorizationUri = new Uri($oAuthProvider->getAuthorizationUrl(['scope' => $scope]));
247266

248267
try {
249268
$this->stateCache->set(
250-
$stateIdentifier,
269+
$oAuthProvider->getState(),
251270
[
271+
'authorizationId' => $authorization->getAuthorizationId(),
252272
'clientId' => $clientId,
253273
'clientSecret' => $clientSecret,
254274
'returnToUri' => (string)$returnToUri
255275
]
256276
);
257277
} catch (Exception $exception) {
258-
throw new OAuthClientException(sprintf('Failed setting cache entry for OAuth authorization: %s', $exception->getMessage()), 1560178858);
278+
throw new OAuthClientException(sprintf('OAuth (%s): Failed setting cache entry for authorization: %s', $this->getServiceType(), $exception->getMessage()), 1560178858);
259279
}
260280

261-
$this->logger->info(sprintf('OAuth (%s): Starting authorization %s using client id "%s" and a %s bytes long secret, returning to "%s".', $this->getServiceType(), $stateIdentifier, $clientId, strlen($clientSecret), $returnToUri), LogEnvironment::fromMethodName(__METHOD__));
262281
return $authorizationUri;
263282
}
264283

265284
/**
266285
* Finish an OAuth authorization with the Authorization Code flow
267286
*
268287
* @param string $code The authorization code given by the OAuth server
269-
* @param string $stateIdentifier The authorization identifier, passed back by the OAuth server as the "state" parameter
288+
* @param string $stateIdentifier The state identifier, passed back by the OAuth server as the "state" parameter
270289
* @param string $scope The scope for the granted authorization (syntax varies depending on the service)
271290
* @return Uri The URI to return to
272291
* @throws OAuthClientException
@@ -278,36 +297,38 @@ public function finishAuthorization(string $code, string $stateIdentifier, strin
278297
{
279298
$stateFromCache = $this->stateCache->get($stateIdentifier);
280299
if (empty($stateFromCache)) {
281-
throw new OAuthClientException(sprintf('OAuth2: Finishing authorization failed because oAuth state %s could not be retrieved from the state cache.', $stateIdentifier), 1558956494);
300+
throw new OAuthClientException(sprintf('OAuth: Finishing authorization failed because oAuth state %s could not be retrieved from the state cache.', $stateIdentifier), 1558956494);
282301
}
283302

303+
$authorizationId = $stateFromCache['authorizationId'];
284304
$clientId = $stateFromCache['clientId'];
285305
$clientSecret = $stateFromCache['clientSecret'];
286306
$oAuthProvider = $this->createOAuthProvider($clientId, $clientSecret);
287307

308+
$this->logger->info(sprintf('OAuth (%s): Finishing authorization for client "%s", authorization id "%s", using state %s.', $this->getServiceType(), $clientId, $authorizationId, $stateIdentifier));
288309
try {
289-
$this->logger->info(sprintf('OAuth (%s): Finishing authorization for client "%s", state "%s", using a %s bytes long secret.', $this->getServiceType(), $clientId, $stateIdentifier, strlen($clientSecret)), LogEnvironment::fromMethodName(__METHOD__));
290-
291-
$oldOAuthToken = $this->entityManager->find(Authorization::class, ['authorizationId' => $stateIdentifier]);
292-
if ($oldOAuthToken !== null) {
293-
$this->entityManager->remove($oldOAuthToken);
294-
$this->entityManager->flush();
295-
296-
$this->logger->info(sprintf('OAuth (%s): Removed old OAuth token "%s".', $this->getServiceType(), $stateIdentifier), LogEnvironment::fromMethodName(__METHOD__));
310+
$authorization = $this->entityManager->find(Authorization::class, $authorizationId);
311+
if (!$authorization instanceof Authorization) {
312+
throw new OAuthClientException(sprintf('OAuth2 (%s): Finishing authorization failed because authorization %s could not be retrieved from the database.', $this->getServiceType(), $authorizationId), 1568710771);
297313
}
314+
298315
$accessToken = $oAuthProvider->getAccessToken('authorization_code', ['code' => $code]);
299-
$authorization = $this->createNewAuthorization($clientId, $clientSecret, 'authorization_code', $accessToken, $scope);
316+
$this->logger->info(sprintf('OAuth (%s): Persisting OAuth token for authorization "%s" with expiry time %s.', $this->getServiceType(), $authorizationId, $accessToken->getExpires()));
300317

301-
$this->logger->info(sprintf('OAuth (%s): Persisted new OAuth token for authorization "%s" with expiry time %s.', $this->getServiceType(), $stateIdentifier, $accessToken->getExpires()), LogEnvironment::fromMethodName(__METHOD__));
318+
$authorization->setAccessToken($accessToken);
302319

303320
$this->entityManager->persist($authorization);
304321
$this->entityManager->flush();
322+
305323
} catch (IdentityProviderException $exception) {
306324
throw new OAuthClientException($exception->getMessage(), 1511187001671, $exception);
307325
}
308326

309327
$returnToUri = new Uri($stateFromCache['returnToUri']);
310-
return $returnToUri->withQuery(trim($returnToUri->getQuery() . '&' . self::STATE_QUERY_PARAMETER_NAME . '=' . $stateIdentifier, '&'));
328+
$returnToUri = $returnToUri->withQuery(trim($returnToUri->getQuery() . '&' . self::AUTHORIZATION_ID_QUERY_PARAMETER_NAME . '=' . $authorizationId, '&'));
329+
330+
$this->logger->debug(sprintf('OAuth (%s): Finished authorization "%s", $returnToUri is %s.', $this->getServiceType(), $authorizationId, $returnToUri));
331+
return $returnToUri;
311332
}
312333

313334
/**

‎Tests/Unit/AuthorizationTest.php‎

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ public function correctConstructorArguments(): array
4848
*/
4949
public function constructSetsAuthorizationIdentifier(string $expectedAuthorizationId, string $serviceName, string $clientId, string $clientSecret, string $grantType, string $scope): void
5050
{
51-
$authorization = new Authorization($serviceName, $clientId, $clientSecret, $grantType, $scope);
51+
$authorization = new Authorization($serviceName, $clientId, $grantType, $scope);
5252
self::assertSame($expectedAuthorizationId, $authorization->getAuthorizationId());
5353
}
5454

@@ -59,7 +59,7 @@ public function getAccessTokenReturnsClonedObject(): void
5959
{
6060
$accessToken = $this->createValidAccessToken();
6161

62-
$authorization = new Authorization('service', 'clientId', 'clientSecret', 'authorization_code', '');
62+
$authorization = new Authorization('service', 'clientId',Authorization::GRANT_AUTHORIZATION_CODE, 'profile');
6363
$authorization->setAccessToken($accessToken);
6464
$retrievedAccessToken = $authorization->getAccessToken();
6565

@@ -74,7 +74,7 @@ public function getSerializedAccessTokenReturnsCorrectJsonString(): void
7474
{
7575
$accessToken = $this->createValidAccessToken();
7676

77-
$authorization = new Authorization('service', 'clientId', 'clientSecret', 'authorization_code', '');
77+
$authorization = new Authorization('service', 'clientId', Authorization::GRANT_AUTHORIZATION_CODE, '');
7878
$authorization->setAccessToken($accessToken);
7979

8080
$secondAccessToken = new AccessToken($authorization->getSerializedAccessToken());
@@ -88,13 +88,27 @@ public function getAccessTokenReturnsPreviouslySetSerializedToken(): void
8888
{
8989
$accessToken = $this->createValidAccessToken();
9090

91-
$authorization = new Authorization('service', 'clientId', 'clientSecret', 'authorization_code', '');
91+
$authorization = new Authorization('service', 'clientId', Authorization::GRANT_AUTHORIZATION_CODE, '');
9292
$authorization->setSerializedAccessToken($accessToken->jsonSerialize());
9393

9494
$secondAccessToken = new AccessToken($authorization->getSerializedAccessToken());
9595
$this->assertEquals($accessToken, $secondAccessToken);
9696
}
9797

98+
/**
99+
* @test
100+
*/
101+
public function calculateAuthorizationIdReturnsSha1(): void
102+
{
103+
$authorizationId = Authorization::calculateAuthorizationId(
104+
'oidc_test',
105+
'ac36cGG4d2Cef1DeuevA7T1u7V4WOUI14',
106+
'authorization_code',
107+
'oidc profile'
108+
);
109+
self::assertSame('21de19f789834af6edff3346e8d9449fcd0d4dae', $authorizationId);
110+
}
111+
98112
/**
99113
* @return AccessToken
100114
*/

0 commit comments

Comments
 (0)