Skip to content

Commit

Permalink
Merge pull request #28 from City-of-Helsinki/UHF-9458
Browse files Browse the repository at this point in the history
UHF-9458: Always generate email when missing
  • Loading branch information
tuutti authored Dec 20, 2023
2 parents 3bba767 + fd76dfc commit a866ceb
Show file tree
Hide file tree
Showing 5 changed files with 37 additions and 38 deletions.
6 changes: 3 additions & 3 deletions helfi_tunnistamo.module
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
8 changes: 2 additions & 6 deletions src/Plugin/OpenIDConnectClient/Tunnistamo.php
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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;
}
Expand Down Expand Up @@ -301,7 +297,7 @@ public function getClientScopes(): array {
$scopes = $this->configuration['client_scopes'] ?? [];

if (!$scopes) {
return ['openid', 'email', 'ad_groups'];
return [];
}
return explode(',', $this->configuration['client_scopes']);
}
Expand Down
5 changes: 5 additions & 0 deletions tests/src/Kernel/RoleMapTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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']]]);
Expand Down
13 changes: 13 additions & 0 deletions tests/src/Kernel/TunnistamoClientTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down
43 changes: 14 additions & 29 deletions tests/src/Kernel/UserInfoAlterTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand All @@ -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());
}

/**
Expand All @@ -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,
'[email protected]',
],
// 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' => '[email protected]',
'sub' => '123',
],
'ad_groups',
TRUE,
'[email protected]',
],
];
}
Expand Down

0 comments on commit a866ceb

Please sign in to comment.