Skip to content

Commit

Permalink
chore: refactor to avoid duplication of code in SessionVerifier and A…
Browse files Browse the repository at this point in the history
…uthModule
  • Loading branch information
DeepDiver1975 committed Aug 2, 2022
1 parent 7e2e586 commit af61681
Show file tree
Hide file tree
Showing 9 changed files with 209 additions and 138 deletions.
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ vendor-bin/**/composer.lock
/build/
/tests/output/
.phpunit.result.cache
tests/unit/PopTest.php

# SonarCloud scanner
.scannerwork
Expand Down
66 changes: 61 additions & 5 deletions lib/Client.php
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ class Client extends OpenIDConnectClient {
* @param IURLGenerator $generator
* @param ISession $session
* @param ILogger $logger
*
* @throws \JsonException
*/
public function __construct(
IConfig $config,
Expand Down Expand Up @@ -147,11 +149,12 @@ public function getAutoUpdateConfig(): array {

/**
* @throws OpenIDConnectClientException
* @throws \JsonException
*/
public function getWellKnownConfig() {
if (!$this->wellKnownConfig) {
$well_known_config_url = \rtrim($this->getProviderURL(), '/') . '/.well-known/openid-configuration';
$this->wellKnownConfig = \json_decode($this->fetchURL($well_known_config_url), false);
$this->wellKnownConfig = \json_decode($this->fetchURL($well_known_config_url), false, 512, JSON_THROW_ON_ERROR);
}
return $this->wellKnownConfig;
}
Expand All @@ -160,12 +163,64 @@ public function mode() {
return $this->getOpenIdConfig()['mode'] ?? 'userid';
}

/**
* @return object|null
*/
public function getAccessTokenPayload(): ?object {
return parent::getAccessTokenPayload();
}

/**
* @throws OpenIDConnectClientException
* @throws \JsonException
*/
public function verifyToken(string $token) {
$config = $this->getOpenIdConfig();
$this->setAccessToken($token);
$payload = $this->getAccessTokenPayload();
if ($payload) {
if (!$this->verifyJWTsignature($token)) {
$this->logger->error('Token cannot be verified: ' . $token);
throw new OpenIDConnectClientException('Token cannot be verified.');
}
$this->logger->debug('Access token payload: ' . \json_encode($payload, JSON_THROW_ON_ERROR));
/* @phan-suppress-next-line PhanTypeExpectedObjectPropAccess */
return $payload->exp;
}

# use token introspection to verify the token
$introspectionClientId = $config['token-introspection-endpoint-client-id'] ?? null;
$introspectionClientSecret = $config['token-introspection-endpoint-client-secret'] ?? null;
$tokenExchangeMode = $config['exchange-token-mode-before-introspection'] ?? null;

if ($tokenExchangeMode) {
$token = $tokenExchangeMode === 'refresh-token' ? $this->session->get('oca.openid-connect.refresh-token') : $this->session->get('oca.openid-connect.access-token');
$this->logger->debug("Starting token-exchange to verify session with subject_token mode: $tokenExchangeMode");

$token = $this->exchangeToken($token, $tokenExchangeMode);
}

$introData = $this->introspectToken($token, '', $introspectionClientId, $introspectionClientSecret);
$this->logger->debug('Introspection info: ' . \json_encode($introData, JSON_THROW_ON_ERROR));
if (\property_exists($introData, 'error')) {
$this->logger->error('Token introspection failed: ' . \json_encode($introData, JSON_THROW_ON_ERROR));
throw new OpenIDConnectClientException("Verifying token failed: {$introData->error}");
}
if (!$introData->active) {
$this->logger->error('Token (as per introspection) is inactive: ' . \json_encode($introData, JSON_THROW_ON_ERROR));
throw new OpenIDConnectClientException('Token (as per introspection) is inactive');
}
return $introData->exp;
}

/**
* @throws OpenIDConnectClientException
* @throws \JsonException
*/
public function getUserInfo() {
$openIdConfig = $this->getOpenIdConfig();
if (isset($openIdConfig['use-access-token-payload-for-user-info']) && $openIdConfig['use-access-token-payload-for-user-info']) {
if ($payload = $this->getAccessTokenPayload()) {
return $payload;
}
if ($payload = $this->getAccessTokenPayload()) {
return $payload;
}

if (isset($openIdConfig['use-access-token-introspection-for-user-info']) && $openIdConfig['use-access-token-introspection-for-user-info']) {
Expand Down Expand Up @@ -312,6 +367,7 @@ public function getCodeChallengeMethod() {
*
* @return bool
* @throws OpenIDConnectClientException
* @throws \JsonException
*/
public function authenticate() : bool {
$redirectUrl = $this->generator->linkToRouteAbsolute('openidconnect.loginFlow.login');
Expand Down
36 changes: 5 additions & 31 deletions lib/OpenIdConnectAuthModule.php
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,8 @@ public function authToken(string $type, string $token): ?IUser {
if ($this->client->getOpenIdConfig() === null) {
return null;
}
// 1. verify JWT signature
$expiry = $this->verifyJWT($type, $token);
// 1. verify token
$expiry = $this->verifyToken($token);

// 2. verify expiry
if ($expiry) {
Expand Down Expand Up @@ -150,40 +150,14 @@ public function getUserPassword(IRequest $request): string {
/**
* @throws OpenIDConnectClientException
*/
private function verifyJWT(string $type, string $token) {
private function verifyToken(string $token) {
$cache = $this->getCache();
$userInfo = $cache->get($token);
if ($userInfo) {
return $userInfo['exp'];
}
$config = $this->client->getOpenIdConfig();
$payload = $this->client->getAccessTokenPayload();

if (!$payload) {
$introspectionClientId = $config['token-introspection-endpoint-client-id'] ?? null;
$introspectionClientSecret = $config['token-introspection-endpoint-client-secret'] ?? null;

$introData = $this->client->introspectToken($token, '', $introspectionClientId, $introspectionClientSecret);
$this->logger->debug('Introspection info: ' . \json_encode($introData));
if (\property_exists($introData, 'error')) {
$this->logger->error('Token introspection failed: ' . \json_encode($introData));
throw new OpenIDConnectClientException("Verifying token failed: {$introData->error}");
}
if (!$introData->active) {
$this->logger->error('Token (as per introspection) is inactive: ' . \json_encode($introData));
throw new OpenIDConnectClientException('Token (as per introspection) is inactive');
}
return $introData->exp;
}
if (!$this->client->verifyJWTsignature($token)) {
$this->logger->error('Token cannot be verified: ' . $token);
throw new OpenIDConnectClientException('Token cannot be verified.');
}
$this->client->setAccessToken($token);
$payload = $this->client->getAccessTokenPayload();
$this->logger->debug('Access token payload: ' . \json_encode($payload));
/* @phan-suppress-next-line PhanTypeExpectedObjectPropAccess */
return $payload->exp;

return $this->client->verifyToken($token);
}

/**
Expand Down
69 changes: 16 additions & 53 deletions lib/SessionVerifier.php
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ public function __construct(
/**
* @throws HintException
* @throws OpenIDConnectClientException
* @throws \JsonException
*/
public function verifySession(): void {
// verify open id token/session
Expand Down Expand Up @@ -97,65 +98,26 @@ public function verifySession(): void {
return;
}

$client->setAccessToken($accessToken);
$openIdConfig = $client->getOpenIdConfig();

# if the access token is a JWT we use it for verification
$payload = $this->client->getAccessTokenPayload();
if (!$payload) {
$introspectionClientId = $openIdConfig['token-introspection-endpoint-client-id'] ?? null;
$introspectionClientSecret = $openIdConfig['token-introspection-endpoint-client-secret'] ?? null;

if (isset($openIdConfig['exchange-token-mode-before-introspection'])) {
$mode = $openIdConfig['exchange-token-mode-before-introspection'];
$token = $mode === 'refresh-token' ? $this->session->get('oca.openid-connect.refresh-token') : $this->session->get('oca.openid-connect.access-token');
$this->logger->debug("Starting token-exchange to verify session with subject_token mode: $mode");

try {
$accessToken = $this->client->exchangeToken($token, $mode);
} catch (\Exception $e) {
$this->logger->debug("Token Exchange failed, logout: " . $e->getMessage());
$this->logout();
return;
}
}

$introData = $client->introspectToken($accessToken, '', $introspectionClientId, $introspectionClientSecret);
$this->logger->debug('Introspection info: ' . \json_encode($introData) . ' for access token:' . $accessToken);
if (\property_exists($introData, 'error')) {
$this->logger->error('Token introspection failed: ' . \json_encode($introData));
$this->logout();
throw new HintException("Verifying token failed: {$introData->error}");
}
if (!$introData->active) {
$this->logger->error('Token (as per introspection) is inactive: ' . \json_encode($introData));
$this->logout();
return;
}

$this->getCache()->set($accessToken, $introData->exp);
$this->refreshToken($introData->exp);
} else {
if (!$client->verifyJWTsignature($accessToken)) {
$this->logger->error('Token cannot be verified: ' . $accessToken);
$this->logout();
throw new OpenIDConnectClientException('Token cannot be verified.');
}
$payload = $client->getAccessTokenPayload();
$this->logger->debug('Access token payload: ' . \json_encode($payload) . ' for access token:' . $accessToken);

$this->getCache()->set($accessToken, $payload->exp);
try {
$expiry = $client->verifyToken($accessToken);
$this->getCache()->set($accessToken, $expiry);
/* @phan-suppress-next-line PhanTypeExpectedObjectPropAccess */
$this->refreshToken($payload->exp);
$this->refreshToken($expiry);
} catch (\Exception $ex) {
$this->logout();
throw $ex;
}
}

/**
* @throws \JsonException
*/
public function afterLogout($accessToken, $idToken): void {
// only call if access token is still valid
try {
$this->logger->debug('OIDC Logout: revoking token' . $accessToken);
$revokeData = $this->client->revokeToken($accessToken);
$this->logger->debug('Revocation info: ' . \json_encode($revokeData));
$this->logger->debug('Revocation info: ' . \json_encode($revokeData, JSON_THROW_ON_ERROR));
} catch (OpenIDConnectClientException $ex) {
$this->logger->logException($ex);
}
Expand All @@ -180,18 +142,19 @@ private function getCache(): ICache {
* @param int $exp
* @throws OpenIDConnectClientException
* @throws HintException
* @throws \JsonException
*/
private function refreshToken($exp): void {
$expiring = $exp - \time();
if ($expiring < 5 * 60) {
$refreshToken = $this->session->get('oca.openid-connect.refresh-token');
if ($refreshToken) {
$response = $this->client->refreshToken($refreshToken);
$this->logger->debug('RefreshToken response: ' . \json_encode($response));
$this->logger->debug('RefreshToken response: ' . \json_encode($response, JSON_THROW_ON_ERROR));
if (\property_exists($response, 'error')) {
$this->logger->error('Refresh token failed: ' . \json_encode($response));
$this->logger->error('Refresh token failed: ' . \json_encode($response, JSON_THROW_ON_ERROR));
$this->logout();
throw new HintException("Verifying token failed: {$response->error}");
throw new HintException("Verifying token failed: $response->error");
}

$this->session->set('oca.openid-connect.id-token', $this->client->getIdToken());
Expand Down
33 changes: 23 additions & 10 deletions tests/unit/ClientTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@

namespace OCA\OpenIdConnect\Tests\Unit;

use JsonException;
use Jumbojett\OpenIDConnectClientException;
use OCA\OpenIdConnect\Client;
use OCP\IConfig;
use OCP\ISession;
Expand Down Expand Up @@ -76,7 +78,7 @@ protected function setUp(): void {

$this->client = $this->getMockBuilder(Client::class)
->setConstructorArgs([$this->config, $this->urlGenerator, $this->session, $this->logger])
->setMethods(['fetchURL'])
->onlyMethods(['fetchURL'])
->getMock();
}

Expand All @@ -91,7 +93,7 @@ public function testGetConfig(): void {
*/
public function testGetAppConfig($expectedData, $dataInConfig, $expectedErrorMessage = null): void {
$this->config->method('getSystemValue')->willReturn('from system config');
$this->config->expects(self::once())->method('getAppValue')->willReturnCallback(function ($app, $key, $default) use ($dataInConfig) {
$this->config->expects(self::once())->method('getAppValue')->willReturnCallback(function () use ($dataInConfig) {
return $dataInConfig;
});
if ($expectedErrorMessage) {
Expand All @@ -101,13 +103,20 @@ public function testGetAppConfig($expectedData, $dataInConfig, $expectedErrorMes
self::assertEquals($expectedData, $return);
}

/**
* @throws OpenIDConnectClientException
* @throws JsonException
*/
public function testGetWellKnown(): void {
$this->client->setProviderURL('https://example.net');
$this->client->expects(self::once())->method('fetchURL')->with('https://example.net/.well-known/openid-configuration')->willReturn('{"foo": "bar"}');
$return = $this->client->getWellKnownConfig();
self::assertEquals((object)['foo' => 'bar'], $return);
}

/**
* @throws OpenIDConnectClientException
*/
public function testCtor(): void {
$providerUrl = 'https://example.net';

Expand All @@ -132,14 +141,17 @@ public function testCtor(): void {
});
$this->client = $this->getMockBuilder(Client::class)
->setConstructorArgs([$this->config, $this->urlGenerator, $this->session, $this->logger])
->setMethods(['fetchURL'])
->onlyMethods(['fetchURL'])
->getMock();

self::assertEquals($providerUrl, $this->client->getProviderURL());
self::assertEquals(true, $this->client->getVerifyHost());
self::assertEquals(true, $this->client->getVerifyPeer());
}

/**
* @throws OpenIDConnectClientException
*/
public function testCtorInsecure(): void {
$providerUrl = 'https://example.net';

Expand All @@ -165,7 +177,7 @@ public function testCtorInsecure(): void {
});
$this->client = $this->getMockBuilder(Client::class)
->setConstructorArgs([$this->config, $this->urlGenerator, $this->session, $this->logger])
->setMethods(['fetchURL'])
->onlyMethods(['fetchURL'])
->getMock();

self::assertEquals($providerUrl, $this->client->getProviderURL());
Expand All @@ -176,15 +188,16 @@ public function testCtorInsecure(): void {
/**
* @dataProvider providesGetUserInfoData
* @param $useAccessTokenPayloadForUserInfo
* @throws JsonException
* @throws OpenIDConnectClientException
*/
public function testGetUserInfo($useAccessTokenPayloadForUserInfo): void {
$this->config->method('getSystemValue')->willReturnCallback(static function ($key) use ($useAccessTokenPayloadForUserInfo) {
$this->config->method('getSystemValue')->willReturnCallback(static function ($key) {
if ($key === 'openid-connect') {
return [
'provider-url' => '$providerUrl',
'client-id' => 'client-id',
'client-secret' => 'secret',
'use-access-token-payload-for-user-info' => $useAccessTokenPayloadForUserInfo
];
}
if ($key === 'proxy') {
Expand All @@ -202,18 +215,18 @@ public function testGetUserInfo($useAccessTokenPayloadForUserInfo): void {
->getMock();
if ($useAccessTokenPayloadForUserInfo) {
$this->client->expects(self::never())->method('requestUserInfo');
$this->client->expects(self::once())->method('getAccessTokenPayload')->willReturn([
$this->client->expects(self::once())->method('getAccessTokenPayload')->willReturn((object)[
'preferred_username' => '[email protected]'
]);
} else {
$this->client->expects(self::never())->method('getAccessTokenPayload');
$this->client->expects(self::once())->method('requestUserInfo')->willReturn([
$this->client->expects(self::once())->method('getAccessTokenPayload')->willReturn(null);
$this->client->expects(self::once())->method('requestUserInfo')->willReturn((object)[
'preferred_username' => '[email protected]'
]);
}

$info = $this->client->getUserInfo();
self::assertEquals([
self::assertEquals((object)[
'preferred_username' => '[email protected]'
], $info);
}
Expand Down
Loading

0 comments on commit af61681

Please sign in to comment.