From 640595f31fd9fd05b0ee20ca4ac80c7b1a0c1476 Mon Sep 17 00:00:00 2001 From: tuutti Date: Wed, 20 Dec 2023 09:38:47 +0200 Subject: [PATCH 1/5] UHF-9458: Always generate email when missing --- helfi_tunnistamo.module | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/helfi_tunnistamo.module b/helfi_tunnistamo.module index 2fd0c03..26d3a8d 100644 --- a/helfi_tunnistamo.module +++ b/helfi_tunnistamo.module @@ -70,9 +70,9 @@ function helfi_tunnistamo_openid_connect_userinfo_alter( return; } - // If a client has 'ad_groups' scope and user email is missing, replace it - // with a made-up address (issue with @edu.hel.fi users). - if (in_array('ad_groups', $plugin->getClientScopes()) && empty($userinfo['email'])) { + // If the user email is missing, replace it with a made-up address (issue with @edu.hel.fi + // users). + if (empty($userinfo['email'])) { $userinfo['email'] = helfi_tunnistamo_create_email($userinfo); } } From b3cfc52d99d5add3ac5a788a9a046886b3c38829 Mon Sep 17 00:00:00 2001 From: tuutti Date: Wed, 20 Dec 2023 09:46:31 +0200 Subject: [PATCH 2/5] UHF-9458: Test fixes --- helfi_tunnistamo.module | 4 +-- tests/src/Kernel/UserInfoAlterTest.php | 43 +++++++++----------------- 2 files changed, 16 insertions(+), 31 deletions(-) diff --git a/helfi_tunnistamo.module b/helfi_tunnistamo.module index 26d3a8d..cc86b58 100644 --- a/helfi_tunnistamo.module +++ b/helfi_tunnistamo.module @@ -70,8 +70,8 @@ function helfi_tunnistamo_openid_connect_userinfo_alter( return; } - // If the user email is missing, replace it with a made-up address (issue with @edu.hel.fi - // users). + // If the user email is missing, replace it with a made-up address (issue + // with @edu.hel.fi users). if (empty($userinfo['email'])) { $userinfo['email'] = helfi_tunnistamo_create_email($userinfo); } diff --git a/tests/src/Kernel/UserInfoAlterTest.php b/tests/src/Kernel/UserInfoAlterTest.php index 6b1778c..d564fa2 100644 --- a/tests/src/Kernel/UserInfoAlterTest.php +++ b/tests/src/Kernel/UserInfoAlterTest.php @@ -35,25 +35,17 @@ protected function setUp() : void { * * @param array $userInfo * The userinfo. - * @param string $scopes - * The scopes. * * @return \Drupal\openid_connect\OpenIDConnectClientEntityInterface * The client mock. */ - private function getClientMock(array $userInfo, string $scopes = '') : OpenIDConnectClientEntityInterface { + private function getClientMock(array $userInfo) : OpenIDConnectClientEntityInterface { $plugin = $this->prophesize(OpenIDConnectClientInterface::class); $plugin->usesUserInfo()->willReturn(TRUE); $plugin->retrieveUserInfo(Argument::cetera())->willReturn($userInfo); $client = $this->prophesize(OpenIDConnectClientEntityInterface::class); $client->id()->willReturn('tunnistamo'); $client->getPlugin()->willReturn($plugin->reveal()); - // OpenIdConnect::buildContext() passes 'plugin_id' string to - // hook_openid_connect_userinfo_alter() hook instead of the actual plugin - // object, meaning our alter hook has to load the actual client entity, - // and we cannot mock the client scopes here. - // Modify the client scopes on default Tunnistamo client entity. - $this->setPluginConfiguration('client_scopes', $scopes); return $client->reveal(); } @@ -73,20 +65,17 @@ private function openIdConnect() : OpenIDConnect { * * @dataProvider authorizationData */ - public function testAuthorization(array $userInfo, string $scopes, bool $expectedStatus) : void { + public function testAuthorization(array $userInfo, string $expectedEmail) : void { $status = $this->openIdConnect() - ->completeAuthorization($this->getClientMock($userInfo, $scopes), [ + ->completeAuthorization($this->getClientMock($userInfo), [ 'access_token' => '123', ]); - $this->assertSame($expectedStatus, $status); + $this->assertTrue($status); - // Make sure account is actually created. - if ($status) { - /** @var \Drupal\externalauth\Authmap $authmap */ - $authmap = $this->container->get('externalauth.authmap'); - $uid = $authmap->getUid($userInfo['sub'], 'openid_connect.tunnistamo'); - $this->assertEquals(helfi_tunnistamo_create_email($userInfo), User::load($uid)->getEmail()); - } + /** @var \Drupal\externalauth\Authmap $authmap */ + $authmap = $this->container->get('externalauth.authmap'); + $uid = $authmap->getUid($userInfo['sub'], 'openid_connect.tunnistamo'); + $this->assertEquals($expectedEmail, User::load($uid)->getEmail()); } /** @@ -97,26 +86,22 @@ public function testAuthorization(array $userInfo, string $scopes, bool $expecte */ public function authorizationData() : array { return [ - // Make sure authorization fails when a user has no email address or - // client doesn't define 'ad_groups' scope. + // Make sure authorization succeeds, and a random email address is + // generated when a user has no email. [ [ 'email' => '', 'sub' => '123', ], - 'email', - FALSE, + '123+placeholder@hel.fi', ], - // Make sure authorization succeeds, and a random email address is - // generated when a user has no email, but the client has 'ad_groups' - // scope set. + // Make sure the original email is used when set. [ [ - 'email' => '', + 'email' => 'test@example.com', 'sub' => '123', ], - 'ad_groups', - TRUE, + 'test@example.com', ], ]; } From 57f06785d00e7635a07503f89fa1a4514d036687 Mon Sep 17 00:00:00 2001 From: tuutti Date: Wed, 20 Dec 2023 09:51:15 +0200 Subject: [PATCH 3/5] UHF-9458: Removed ad_groups default scope since it does not work with keycloak --- src/Plugin/OpenIDConnectClient/Tunnistamo.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Plugin/OpenIDConnectClient/Tunnistamo.php b/src/Plugin/OpenIDConnectClient/Tunnistamo.php index 302a407..efd5729 100644 --- a/src/Plugin/OpenIDConnectClient/Tunnistamo.php +++ b/src/Plugin/OpenIDConnectClient/Tunnistamo.php @@ -301,7 +301,7 @@ public function getClientScopes(): array { $scopes = $this->configuration['client_scopes'] ?? []; if (!$scopes) { - return ['openid', 'email', 'ad_groups']; + return ['openid', 'email']; } return explode(',', $this->configuration['client_scopes']); } From 23ba1a401316c91b5e0067a5effd06acac019b38 Mon Sep 17 00:00:00 2001 From: tuutti Date: Wed, 20 Dec 2023 09:57:26 +0200 Subject: [PATCH 4/5] UHF-9458: Allow scopes to be empty --- src/Plugin/OpenIDConnectClient/Tunnistamo.php | 2 +- tests/src/Kernel/TunnistamoClientTest.php | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/src/Plugin/OpenIDConnectClient/Tunnistamo.php b/src/Plugin/OpenIDConnectClient/Tunnistamo.php index efd5729..80477b0 100644 --- a/src/Plugin/OpenIDConnectClient/Tunnistamo.php +++ b/src/Plugin/OpenIDConnectClient/Tunnistamo.php @@ -301,7 +301,7 @@ public function getClientScopes(): array { $scopes = $this->configuration['client_scopes'] ?? []; if (!$scopes) { - return ['openid', 'email']; + return []; } return explode(',', $this->configuration['client_scopes']); } diff --git a/tests/src/Kernel/TunnistamoClientTest.php b/tests/src/Kernel/TunnistamoClientTest.php index d0b5509..6398d56 100644 --- a/tests/src/Kernel/TunnistamoClientTest.php +++ b/tests/src/Kernel/TunnistamoClientTest.php @@ -12,6 +12,19 @@ */ class TunnistamoClientTest extends KernelTestBase { + /** + * Make sure the correct scopes are returned. + * + * @covers ::getClientScopes + */ + public function testGetClientScopes() : void { + $plugin = $this->getPlugin(); + $config = $plugin->getConfiguration(); + $this->assertSame($config['client_scopes'], $plugin->defaultConfiguration()['client_scopes']); + $this->setPluginConfiguration('client_scopes', ''); + $this->assertSame([], $this->getPlugin()->getClientScopes()); + } + /** * Make sure Tunnistamo is enabled by default. * From fd76dfc2ec1e0cd637aa48084338e3cb0a5d5dee Mon Sep 17 00:00:00 2001 From: tuutti Date: Wed, 20 Dec 2023 10:25:06 +0200 Subject: [PATCH 5/5] UHF-9458: Test non-existent ad group too --- src/Plugin/OpenIDConnectClient/Tunnistamo.php | 6 +----- tests/src/Kernel/RoleMapTest.php | 5 +++++ 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/Plugin/OpenIDConnectClient/Tunnistamo.php b/src/Plugin/OpenIDConnectClient/Tunnistamo.php index 80477b0..97e2028 100644 --- a/src/Plugin/OpenIDConnectClient/Tunnistamo.php +++ b/src/Plugin/OpenIDConnectClient/Tunnistamo.php @@ -246,6 +246,7 @@ public function mapRoles(UserInterface $account, array $context) : void { return; } + // Remove all existing roles. array_map( fn (string $rid) => $account->removeRole($rid), $account->getRoles(FALSE) @@ -258,11 +259,6 @@ public function mapRoles(UserInterface $account, array $context) : void { if (!in_array($adRole, $context['userinfo']['ad_groups'])) { continue; } - - if (!is_array($drupalRoles)) { - $drupalRoles = [$drupalRoles]; - } - foreach ($drupalRoles as $value) { $roles[] = $value; } diff --git a/tests/src/Kernel/RoleMapTest.php b/tests/src/Kernel/RoleMapTest.php index 875927b..fbdc2b9 100644 --- a/tests/src/Kernel/RoleMapTest.php +++ b/tests/src/Kernel/RoleMapTest.php @@ -49,6 +49,11 @@ public function testRoleMap() : void { 'ad_role' => 'ad_role', 'roles' => [$role2], ], + // Test non-existent ad role. + [ + 'ad_role' => 'non_existent', + 'roles' => [$role2], + ], ]); $this->getPlugin()->mapRoles($account, []); $this->getPlugin()->mapRoles($account, ['userinfo' => ['ad_groups' => ['ad_role']]]);