From fbb1aa919c9af1495261fc296c5061d78cae27ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <1005065+DeepDiver1975@users.noreply.github.com> Date: Thu, 24 Sep 2020 16:04:18 +0200 Subject: [PATCH] Azure AD: Use access token payload instead of user info endpoint --- lib/Client.php | 9 ++++ lib/Controller/LoginFlowController.php | 12 +++-- lib/OpenIdConnectAuthModule.php | 18 ++++---- lib/Service/UserLookupService.php | 3 ++ tests/unit/ClientTest.php | 46 +++++++++++++++++++ .../LoginFlowControllerLoginTest.php | 6 +-- tests/unit/OpenIdConnectAuthModuleTest.php | 4 +- 7 files changed, 81 insertions(+), 17 deletions(-) diff --git a/lib/Client.php b/lib/Client.php index d078efb..762aa6e 100644 --- a/lib/Client.php +++ b/lib/Client.php @@ -99,6 +99,15 @@ public function getWellKnownConfig() { return $this->wellKnownConfig; } + public function getUserInfo() { + $openIdConfig = $this->getOpenIdConfig(); + if (isset($openIdConfig['use-access-token-payload-for-user-info']) && $openIdConfig['use-access-token-payload-for-user-info']) { + return $this->getAccessTokenPayload(); + } + + return $this->requestUserInfo(); + } + /** * @codeCoverageIgnore */ diff --git a/lib/Controller/LoginFlowController.php b/lib/Controller/LoginFlowController.php index 2027d73..818d894 100644 --- a/lib/Controller/LoginFlowController.php +++ b/lib/Controller/LoginFlowController.php @@ -126,9 +126,15 @@ public function login(): RedirectResponse { $this->logger->logException($ex); throw new HintException('Error in OpenIdConnect:' . $ex->getMessage()); } - $this->logger->debug('Access token: ' . $openid->getAccessToken()); - $this->logger->debug('Refresh token: ' . $openid->getRefreshToken()); - $userInfo = $openid->requestUserInfo(); + $debugInfo = \json_encode([ + 'access_token' => $openid->getAccessToken(), + 'refresh_token' => $openid->getRefreshToken(), + 'id_token' => $openid->getIdToken(), + 'access_token_payload' => $openid->getAccessTokenPayload(), + ], JSON_PRETTY_PRINT); + $this->logger->debug('LoginFlowController::login : Token info: ' . $debugInfo); + + $userInfo = $openid->getUserInfo(); $this->logger->debug('User info: ' . \json_encode($userInfo)); if (!$userInfo) { throw new LoginException('No user information available.'); diff --git a/lib/OpenIdConnectAuthModule.php b/lib/OpenIdConnectAuthModule.php index 5ea8a01..4bbf4cb 100644 --- a/lib/OpenIdConnectAuthModule.php +++ b/lib/OpenIdConnectAuthModule.php @@ -93,6 +93,7 @@ public function auth(IRequest $request): ?IUser { * @throws LoginException */ public function authToken(string $bearerToken): ?IUser { + $this->logger->debug('OpenIdConnectAuthModule::authToken ' . $bearerToken); try { if ($this->client->getOpenIdConfig() === null) { return null; @@ -104,6 +105,7 @@ public function authToken(string $bearerToken): ?IUser { if ($expiry) { $expiring = $expiry - \time(); if ($expiring < 0) { + $this->logger->debug("OpenID Connect token expired at $expiry"); throw new LoginException('OpenID Connect token expired'); } } @@ -114,6 +116,7 @@ public function authToken(string $bearerToken): ?IUser { $this->updateCache($bearerToken, $user, $expiry); return $user; } + $this->logger->debug('OpenIdConnectAuthModule::authToken : no user retrieved from token ' . $bearerToken); return null; } catch (OpenIDConnectClientException $ex) { $this->logger->logException($ex); @@ -173,20 +176,22 @@ private function verifyJWT($bearerToken) { * @return ICache */ private function getCache(): ICache { - return $this->cacheFactory->create('oca.openid-connect'); + // TODO: needs cleanup and consolidation with SessionVerifier usage of the cache + return $this->cacheFactory->create('oca.openid-connect.2'); } private function getUserResource($bearerToken) { $cache = $this->getCache(); $userInfo = $cache->get($bearerToken); if ($userInfo) { + $this->logger->debug('OpenIdConnectAuthModule::getUserResource from cache: ' . \json_encode($userInfo)); return $this->manager->get($userInfo['uid']); } $this->client->setAccessToken($bearerToken); - $userInfo = $this->client->requestUserInfo(); - $this->logger->debug('User info: ' . \json_encode($userInfo)); + $userInfo = $this->client->getUserInfo(); + $this->logger->debug('OpenIdConnectAuthModule::getUserResource from cache: ' . \json_encode($userInfo)); if ($userInfo === null) { return null; } @@ -194,12 +199,7 @@ private function getUserResource($bearerToken) { return $this->lookupService->lookupUser($userInfo); } - /** - * @param string $bearerToken - * @param IUser $user - * @param int $expiry - */ - private function updateCache($bearerToken, IUser $user, $expiry): void { + private function updateCache(string $bearerToken, IUser $user, int $expiry): void { $cache = $this->getCache(); $cache->set($bearerToken, [ 'uid' => $user->getUID(), diff --git a/lib/Service/UserLookupService.php b/lib/Service/UserLookupService.php index 9913b72..a7882b5 100644 --- a/lib/Service/UserLookupService.php +++ b/lib/Service/UserLookupService.php @@ -60,6 +60,9 @@ public function lookupUser($userInfo): IUser { $searchByEmail = false; } $attribute = $openIdConfig['search-attribute'] ?? 'email'; + if (!\property_exists($userInfo, $attribute)) { + throw new LoginException("Configured attribute $attribute is not known."); + } if ($searchByEmail) { $user = $this->userManager->getByEmail($userInfo->$attribute); diff --git a/tests/unit/ClientTest.php b/tests/unit/ClientTest.php index 62a8f78..fa57de8 100644 --- a/tests/unit/ClientTest.php +++ b/tests/unit/ClientTest.php @@ -48,6 +48,13 @@ class ClientTest extends TestCase { */ private $config; + public function providesGetUserInfoData(): array { + return [ + 'access-token' => [true], + 'user-info-endpoint' => [false], + ]; + } + protected function setUp(): void { parent::setUp(); $this->config = $this->createMock(IConfig::class); @@ -125,4 +132,43 @@ public function testCtorInsecure(): void { self::assertEquals(false, $this->client->getVerifyHost()); self::assertEquals(false, $this->client->getVerifyPeer()); } + + /** + * @dataProvider providesGetUserInfoData + * @param $useAccessTokenPayloadForUserInfo + */ + public function testGetUserInfo($useAccessTokenPayloadForUserInfo): void { + $this->config->method('getSystemValue')->willReturnCallback(static function ($key) use ($useAccessTokenPayloadForUserInfo) { + if ($key === 'openid-connect') { + return [ + 'provider-url' => '$providerUrl', + 'client-id' => 'client-id', + 'client-secret' => 'secret', + 'use-access-token-payload-for-user-info' => $useAccessTokenPayloadForUserInfo + ]; + } + throw new \InvalidArgumentException("Unexpected key: $key"); + }); + + $this->client = $this->getMockBuilder(Client::class) + ->setConstructorArgs([$this->config, $this->urlGenerator, $this->session]) + ->onlyMethods(['requestUserInfo', 'getAccessTokenPayload']) + ->getMock(); + if ($useAccessTokenPayloadForUserInfo) { + $this->client->expects(self::never())->method('requestUserInfo'); + $this->client->expects(self::once())->method('getAccessTokenPayload')->willReturn([ + 'preferred_username' => 'alice@example.net' + ]); + } else { + $this->client->expects(self::never())->method('getAccessTokenPayload'); + $this->client->expects(self::once())->method('requestUserInfo')->willReturn([ + 'preferred_username' => 'alice@example.net' + ]); + } + + $info = $this->client->getUserInfo(); + self::assertEquals([ + 'preferred_username' => 'alice@example.net' + ], $info); + } } diff --git a/tests/unit/Controller/LoginFlowControllerLoginTest.php b/tests/unit/Controller/LoginFlowControllerLoginTest.php index 29a4151..e712373 100644 --- a/tests/unit/Controller/LoginFlowControllerLoginTest.php +++ b/tests/unit/Controller/LoginFlowControllerLoginTest.php @@ -117,7 +117,7 @@ public function testLoginNoUserInfo(): void { public function testLoginUnknownUser(): void { $this->client->method('getOpenIdConfig')->willReturn([]); - $this->client->method('requestUserInfo')->willReturn((object)['email' => 'foo@exmaple.net']); + $this->client->method('getUserInfo')->willReturn((object)['email' => 'foo@exmaple.net']); $this->userLookup->method('lookupUser')->willThrowException(new LoginException('User foo is not known.')); $this->expectException(LoginException::class); $this->expectExceptionMessage('User foo is not known.'); @@ -127,7 +127,7 @@ public function testLoginUnknownUser(): void { public function testLoginCreateSessionFailed(): void { $this->client->method('getOpenIdConfig')->willReturn([]); - $this->client->method('requestUserInfo')->willReturn((object)['email' => 'foo@exmaple.net']); + $this->client->method('getUserInfo')->willReturn((object)['email' => 'foo@exmaple.net']); $user = $this->createMock(IUser::class); $this->userLookup->method('lookupUser')->willReturn($user); $this->userSession->method('createSessionToken')->willReturn(false); @@ -138,7 +138,7 @@ public function testLoginCreateSessionFailed(): void { public function testLoginCreateSuccess(): void { $this->client->method('getOpenIdConfig')->willReturn([]); - $this->client->method('requestUserInfo')->willReturn((object)['email' => 'foo@exmaple.net']); + $this->client->method('getUserInfo')->willReturn((object)['email' => 'foo@exmaple.net']); $this->client->method('getIdToken')->willReturn('id'); $this->client->method('getAccessToken')->willReturn('access'); $this->client->method('getRefreshToken')->willReturn('refresh'); diff --git a/tests/unit/OpenIdConnectAuthModuleTest.php b/tests/unit/OpenIdConnectAuthModuleTest.php index 68020ac..3e1a607 100644 --- a/tests/unit/OpenIdConnectAuthModuleTest.php +++ b/tests/unit/OpenIdConnectAuthModuleTest.php @@ -126,7 +126,7 @@ public function testInvalidTokenWithIntrospectionNotActive(): void { public function testValidTokenWithIntrospection(): void { $this->client->method('getOpenIdConfig')->willReturn(['use-token-introspection-endpoint' => true]); $this->client->method('introspectToken')->willReturn((object)['active' => true, 'exp' => \time() + 3600]); - $this->client->method('requestUserInfo')->willReturn((object)['email' => 'foo@example.com']); + $this->client->method('getUserInfo')->willReturn((object)['email' => 'foo@example.com']); $this->cacheFactory->method('create')->willReturn(new ArrayCache()); $user = $this->createMock(IUser::class); $this->lookupService->expects(self::once())->method('lookupUser')->willReturn($user); @@ -141,7 +141,7 @@ public function testValidTokenWithJWT(): void { $this->client->method('getOpenIdConfig')->willReturn(['use-token-introspection-endpoint' => false]); $this->client->method('verifyJWTsignature')->willReturn(true); $this->client->method('getAccessTokenPayload')->willReturn((object)['exp' => \time() + 3600]); - $this->client->method('requestUserInfo')->willReturn((object)['email' => 'foo@example.com']); + $this->client->method('getUserInfo')->willReturn((object)['email' => 'foo@example.com']); $this->cacheFactory->method('create')->willReturn(new ArrayCache()); $user = $this->createMock(IUser::class); $this->lookupService->expects(self::once())->method('lookupUser')->willReturn($user);