Merge pull request #954 from nextcloud/fix/293/groups-from-env

fix(env-mode): accept multiple comma-separated groups

Author: blizzz

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;
 		}
 	}

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()]);

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

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 = <<<EOF
+		// 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.
+		if (file_exists(self::ENV_CONFIG_FILE)) {
+			$envConfigPhp = file_get_contents(self::ENV_CONFIG_FILE) . PHP_EOL;
+		} else {
+			$envConfigPhp = <<<EOF
 <?php
+EOF . PHP_EOL;
+		}
+		$envConfigPhp .= <<<EOF
 \$_SERVER["$key"] = "$value";
 EOF;
 		file_put_contents(self::ENV_CONFIG_FILE, $envConfigPhp);

tests/unit/UserBackendTest.php

@@ -155,6 +155,13 @@ public function testUpdateAttributes() {
 		/** @var IUser|MockObject $user */
 		$user = $this->createMock(IUser::class);
 
+		$attributes = [
+			'email' => '[email protected]',
+			'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' => '[email protected]',
-			'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' => '[email protected]', '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' => '[email protected]', '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');
 	}
 }