From 0c0bacec0576b22e64745b7fc06e180fa313bd0b Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Tue, 13 May 2025 13:05:54 +0200 Subject: [PATCH] fix(env-mode): accept multiple comma-separated groups Signed-off-by: Arthur Schiwon --- lib/Controller/SAMLController.php | 4 +- lib/UserBackend.php | 24 +- .../features/EnvironmentVariable.feature | 3 + .../features/bootstrap/FeatureContext.php | 39 ++- tests/unit/UserBackendTest.php | 30 +- tests/unit/UserBackendTest.php.orig | 288 ------------------ 6 files changed, 65 insertions(+), 323 deletions(-) delete mode 100644 tests/unit/UserBackendTest.php.orig diff --git a/lib/Controller/SAMLController.php b/lib/Controller/SAMLController.php index be05127d6..a0e22546a 100644 --- a/lib/Controller/SAMLController.php +++ b/lib/Controller/SAMLController.php @@ -94,7 +94,7 @@ private function autoprovisionIfPossible(): void { $autoProvisioningAllowed = $this->userBackend->autoprovisionAllowed(); if ($userExists) { if ($autoProvisioningAllowed) { - $this->userBackend->updateAttributes($uid, $auth); + $this->userBackend->updateAttributes($uid); } return; } @@ -104,7 +104,7 @@ private function autoprovisionIfPossible(): void { throw new NoUserFoundException('Auto provisioning not allowed and user ' . $uid . ' does not exist'); } elseif (!$userExists && $autoProvisioningAllowed) { $this->userBackend->createUserIfNotExists($uid, $auth); - $this->userBackend->updateAttributes($uid, $auth); + $this->userBackend->updateAttributes($uid); return; } } diff --git a/lib/UserBackend.php b/lib/UserBackend.php index 9abe2bf1f..0a082743d 100644 --- a/lib/UserBackend.php +++ b/lib/UserBackend.php @@ -355,7 +355,7 @@ private function formatUserData($attributes): array { } try { - $result['formatted']['groups'] = $this->getAttributeArrayValue('saml-attribute-mapping-group_mapping', $attributes); + $result['formatted']['groups'] = $this->userData->getGroups(); } catch (\InvalidArgumentException) { $result['formatted']['groups'] = null; } @@ -474,24 +474,8 @@ private function getAttributeValue($name, array $attributes) { return $value; } - private function getAttributeArrayValue($name, array $attributes) { - $keys = $this->getAttributeKeys($name); - - $value = []; - foreach ($keys as $key) { - if (isset($attributes[$key])) { - if (is_array($attributes[$key])) { - $value = array_merge($value, array_values($attributes[$key])); - } else { - $value[] = $attributes[$key]; - } - } - } - - return $value; - } - - public function updateAttributes(string $uid, array $attributes): void { + public function updateAttributes(string $uid): void { + $attributes = $this->userData->getAttributes(); $user = $this->userManager->get($uid); try { $newEmail = $this->getAttributeValue('saml-attribute-mapping-email_mapping', $attributes); @@ -519,7 +503,7 @@ public function updateAttributes(string $uid, array $attributes): void { } try { - $newGroups = $this->getAttributeArrayValue('saml-attribute-mapping-group_mapping', $attributes); + $newGroups = $this->userData->getGroups(); $this->logger->debug('Group attribute content: {groups}', ['app' => 'user_saml', 'groups' => json_encode($newGroups)]); } catch (\InvalidArgumentException $e) { $this->logger->debug('Failed to fetch group attribute: {exception}', ['app' => 'user_saml', 'exception' => $e->getMessage()]); diff --git a/tests/integration/features/EnvironmentVariable.feature b/tests/integration/features/EnvironmentVariable.feature index 5e5e3edb9..7c9907f2b 100644 --- a/tests/integration/features/EnvironmentVariable.feature +++ b/tests/integration/features/EnvironmentVariable.feature @@ -3,11 +3,14 @@ Feature: EnvironmentVariable Scenario: Authenticating using environment variable with SSO and no check if user exists on backend And The setting "type" is set to "environment-variable" And The setting "general-uid_mapping" is set to "REMOTE_USER" + And The setting "saml-attribute-mapping-group_mapping" is set to "REMOTE_GROUPS" And The environment variable "REMOTE_USER" is set to "not-provisioned-user" + And The environment variable "REMOTE_GROUPS" is set to "Department A,Team B" When I send a GET request to "http://localhost:8080/index.php/login" Then I should be redirected to "http://localhost:8080/index.php/apps/dashboard/" Then The user value "id" should be "not-provisioned-user" And The last login timestamp of "not-provisioned-user" should not be empty + And User "not-provisioned-user" is part of the groups "SAML_Department A, SAML_Team B" Scenario: Authenticating using environment variable with SSO and successful check if user exists on backend Given A local user with uid "provisioned-user" exists diff --git a/tests/integration/features/bootstrap/FeatureContext.php b/tests/integration/features/bootstrap/FeatureContext.php index d5ff157a3..ec58532e0 100644 --- a/tests/integration/features/bootstrap/FeatureContext.php +++ b/tests/integration/features/bootstrap/FeatureContext.php @@ -549,16 +549,45 @@ public function theLastLoginTimestampOfShouldNotBeEmpty($uid) { } } + /** + * @Then User :userId is part of the groups :groups + */ + public function theUserIsPartOfTheseGroups(string $userId, string $groups) { + $response = shell_exec( + sprintf( + '%s %s user:info %s --output=json', + PHP_BINARY, + __DIR__ . '/../../../../../../occ', + $userId + ) + ); + + $groupsActual = json_decode(trim($response), true)['groups']; + $groupsExpected = array_map('trim', explode(',', $groups)); + + foreach ($groupsExpected as $expectedGroup) { + if (!in_array($expectedGroup, $groupsActual)) { + $actualGroupStr = implode(', ', $groupsActual); + throw new UnexpectedValueException("Expected to find $expectedGroup in '$actualGroupStr'"); + } + } + } + /** * @Given The environment variable :key is set to :value */ public function theEnvironmentVariableIsSetTo($key, $value) { - // Attention, this works currently for one value only. It generates an - // extra config file that injects the value to $_SERVER (as used in - // `SAMLController::login()`), so that it stays across requests in PHPs - // built-in server. - $envConfigPhp = <<createMock(IUser::class); + $attributes = [ + 'email' => 'new@example.com', + 'displayname' => 'New Displayname', + 'quota' => '50MB', + 'groups' => ['groupB', 'groupC'], + ]; + // Replace at() matcher with willReturnCallback to avoid deprecation warning $this->config ->method('getAppValue') @@ -204,19 +211,20 @@ public function testUpdateAttributes() { ->expects($this->once()) ->method('handleIncomingGroups') ->with($user, ['groupB', 'groupC']); - $this->userBackend->updateAttributes('ExistingUser', [ - 'email' => 'new@example.com', - 'displayname' => 'New Displayname', - 'quota' => '50MB', - 'groups' => ['groupB', 'groupC'], - ]); + $this->userData->expects($this->any()) + ->method('getAttributes') + ->willReturn($attributes); + $this->userData->expects($this->any()) + ->method('getGroups') + ->willReturn($attributes['groups']); + $this->userBackend->updateAttributes('ExistingUser'); } public function testUpdateAttributesQuotaDefaultFallback() { $this->getMockedBuilder(['getDisplayName', 'setDisplayName']); /** @var IUser|MockObject $user */ $user = $this->createMock(IUser::class); - + $attributes = ['email' => 'new@example.com', 'displayname' => 'New Displayname', 'quota' => '']; $this->config->method('getAppValue') ->willReturnCallback(fn (string $appId, string $key, string $default) => @@ -261,6 +269,12 @@ public function testUpdateAttributesQuotaDefaultFallback() { ->expects($this->once()) ->method('handleIncomingGroups') ->with($user, []); - $this->userBackend->updateAttributes('ExistingUser', ['email' => 'new@example.com', 'displayname' => 'New Displayname', 'quota' => '']); + $this->userData->expects($this->any()) + ->method('getAttributes') + ->willReturn($attributes); + $this->userData->expects($this->any()) + ->method('getGroups') + ->willReturn([]); + $this->userBackend->updateAttributes('ExistingUser'); } } diff --git a/tests/unit/UserBackendTest.php.orig b/tests/unit/UserBackendTest.php.orig deleted file mode 100644 index c3d7040ce..000000000 --- a/tests/unit/UserBackendTest.php.orig +++ /dev/null @@ -1,288 +0,0 @@ -config = $this->createMock(IConfig::class); - $this->urlGenerator = $this->createMock(IURLGenerator::class); - $this->session = $this->createMock(ISession::class); - $this->db = $this->createMock(IDBConnection::class); - $this->userManager = $this->createMock(IUserManager::class); - $this->groupManager = $this->createMock(GroupManager::class); - $this->SAMLSettings = $this->getMockBuilder(SAMLSettings::class)->disableOriginalConstructor()->getMock(); - $this->logger = $this->createMock(LoggerInterface::class); - $this->userData = $this->createMock(UserData::class); - $this->eventDispatcher = $this->createMock(IEventDispatcher::class); - } - - public function getMockedBuilder(array $mockedFunctions = []) { - if ($mockedFunctions !== []) { - $this->userBackend = $this->getMockBuilder(UserBackend::class) - ->setConstructorArgs([ - $this->config, - $this->urlGenerator, - $this->session, - $this->db, - $this->userManager, - $this->groupManager, - $this->SAMLSettings, - $this->logger, - $this->userData, - $this->eventDispatcher - ]) - ->setMethods($mockedFunctions) - ->getMock(); - } else { - $this->userBackend = new UserBackend( - $this->config, - $this->urlGenerator, - $this->session, - $this->db, - $this->userManager, - $this->groupManager, - $this->SAMLSettings, - $this->logger, - $this->userData, - $this->eventDispatcher - ); - } - } - - public function testGetBackendName() { - $this->getMockedBuilder(); - $this->assertSame('user_saml', $this->userBackend->getBackendName()); - } - - public function testUpdateAttributesWithoutAttributes() { - $this->getMockedBuilder(['getDisplayName']); - /** @var IUser|MockObject $user */ - $user = $this->createMock(IUser::class); - - $this->config->method('getAppValue') -<<<<<<< HEAD - ->willReturnCallback(function (string $appId, string $key, string $default) { - // Unused parameters are intentionally kept for clarity - return $default; - }); -======= - ->willReturnCallback(fn ($appId, $key, $default) => $default); ->>>>>>> 59039fc1 (refactor(PHP): adjust to rector fixes) - - $this->userManager - ->expects($this->once()) - ->method('get') - ->with('ExistingUser') - ->willReturn($user); - $user - ->expects($this->once()) - ->method('getSystemEMailAddress') - ->willReturn(null); - $user - ->expects($this->never()) - ->method('setSystemEMailAddress'); - $this->userBackend - ->expects($this->once()) - ->method('getDisplayName') - ->with('ExistingUser') - ->willReturn(''); - $this->groupManager - ->expects($this->once()) - ->method('handleIncomingGroups') - ->with($user, []); - $this->userBackend->updateAttributes('ExistingUser', []); - } - - public function testUpdateAttributesWithoutValidUser() { - $this->getMockedBuilder(); - - $this->config->method('getAppValue') -<<<<<<< HEAD - ->willReturnCallback(function (string $appId, string $key, string $default) { - // Unused parameters are intentionally kept for clarity - return $default; - }); -======= - ->willReturnCallback(fn ($appId, $key, $default) => $default); ->>>>>>> 59039fc1 (refactor(PHP): adjust to rector fixes) - - $this->userManager - ->expects($this->once()) - ->method('get') - ->with('ExistingUser') - ->willReturn(null); - $this->userBackend->updateAttributes('ExistingUser', []); - } - - public function testUpdateAttributes() { - $this->getMockedBuilder(['getDisplayName', 'setDisplayName']); - /** @var IUser|MockObject $user */ - $user = $this->createMock(IUser::class); - - // Replace at() matcher with willReturnCallback to avoid deprecation warning - $this->config - ->method('getAppValue') - ->willReturnCallback(function ($appId, $key, $default) { - if ($appId === 'user_saml') { - switch ($key) { - case 'saml-attribute-mapping-email_mapping': - return 'email'; - case 'saml-attribute-mapping-displayName_mapping': - return 'displayname'; - case 'saml-attribute-mapping-quota_mapping': - return 'quota'; - case 'saml-attribute-mapping-group_mapping': - return 'groups'; - } - } - return $default; - }); - - $this->userManager - ->expects($this->once()) - ->method('get') - ->with('ExistingUser') - ->willReturn($user); - $user - ->expects($this->once()) - ->method('getSystemEMailAddress') - ->willReturn('old@example.com'); - $user - ->expects($this->once()) - ->method('setSystemEMailAddress') - ->with('new@example.com'); - $user - ->expects($this->once()) - ->method('setQuota') - ->with('50MB'); - $this->userBackend - ->expects($this->once()) - ->method('getDisplayName') - ->with('ExistingUser') - ->willReturn(''); - $this->userBackend - ->expects($this->once()) - ->method('setDisplayName') - ->with('ExistingUser', 'New Displayname'); - $this->groupManager - ->expects($this->once()) - ->method('handleIncomingGroups') - ->with($user, ['groupB', 'groupC']); - $this->userBackend->updateAttributes('ExistingUser', [ - 'email' => 'new@example.com', - 'displayname' => 'New Displayname', - 'quota' => '50MB', - 'groups' => ['groupB', 'groupC'], - ]); - } - - public function testUpdateAttributesQuotaDefaultFallback() { - $this->getMockedBuilder(['getDisplayName', 'setDisplayName']); - /** @var IUser|MockObject $user */ - $user = $this->createMock(IUser::class); - - - $this->config->method('getAppValue') -<<<<<<< HEAD - ->willReturnCallback(function (string $appId, string $key, string $default) { - // Unused $appId parameter is intentionally kept for clarity - switch ($key) { - case 'saml-attribute-mapping-email_mapping': - return 'email'; - case 'saml-attribute-mapping-displayName_mapping': - return 'displayname'; - case 'saml-attribute-mapping-quota_mapping': - return 'quota'; - } - return $default; -======= - ->willReturnCallback(fn ($appId, $key, $default) => match ($key) { - 'saml-attribute-mapping-email_mapping' => 'email', - 'saml-attribute-mapping-displayName_mapping' => 'displayname', - 'saml-attribute-mapping-quota_mapping' => 'quota', - default => $default, ->>>>>>> 59039fc1 (refactor(PHP): adjust to rector fixes) - }); - - $this->userManager - ->expects($this->once()) - ->method('get') - ->with('ExistingUser') - ->willReturn($user); - $user - ->expects($this->once()) - ->method('getSystemEMailAddress') - ->willReturn('old@example.com'); - $user - ->expects($this->once()) - ->method('setSystemEMailAddress') - ->with('new@example.com'); - $user - ->expects($this->once()) - ->method('setQuota') - ->with('default'); - $this->userBackend - ->expects($this->once()) - ->method('getDisplayName') - ->with('ExistingUser') - ->willReturn(''); - $this->userBackend - ->expects($this->once()) - ->method('setDisplayName') - ->with('ExistingUser', 'New Displayname'); - $this->eventDispatcher->expects($this->once()) - ->method('dispatchTyped') - ->with(new UserChangedEvent($user, 'displayName', 'New Displayname', '')); - $this->groupManager - ->expects($this->once()) - ->method('handleIncomingGroups') - ->with($user, []); - $this->userBackend->updateAttributes('ExistingUser', ['email' => 'new@example.com', 'displayname' => 'New Displayname', 'quota' => '']); - } -}