Merge pull request #884 from bergerar/main

 feat: restrict login to users matching a certain group

Author: julien-nc

lib/Controller/LoginController.php

@@ -454,6 +454,18 @@ public function code(string $state = '', string $code = '', string $scope = '',
 			return $this->build403TemplateResponse($message, Http::STATUS_BAD_REQUEST, ['reason' => 'failed to provision user']);
 		}
 
+		// prevent login of users that are not in a whitelisted group (if activated)
+		$restrictLoginToGroups = $this->providerService->getSetting($providerId, ProviderService::SETTING_RESTRICT_LOGIN_TO_GROUPS, '0');
+		if ($restrictLoginToGroups === '1') {
+			$syncGroups = $this->provisioningService->getSyncGroupsOfToken($providerId, $idTokenPayload);
+
+			if ($syncGroups === null || count($syncGroups) === 0) {
+				$this->logger->debug('Prevented user from login as user is not part of a whitelisted group');
+				$message = $this->l10n->t('You do not have permission to log in to this instance. If you think this is an error, please contact an administrator.');
+				return $this->build403TemplateResponse($message, Http::STATUS_FORBIDDEN, ['reason' => 'user not in any whitelisted group']);
+			}
+		}
+
 		$autoProvisionAllowed = (!isset($oidcSystemConfig['auto_provision']) || $oidcSystemConfig['auto_provision']);
 
 		// in case user is provisioned by user_ldap, userManager->search() triggers an ldap search which syncs the results

lib/Service/ProviderService.php

@@ -47,6 +47,8 @@ class ProviderService {
 	public const SETTING_JWKS_CACHE_TIMESTAMP = 'jwksCacheTimestamp';
 	public const SETTING_PROVIDER_BASED_ID = 'providerBasedId';
 	public const SETTING_GROUP_PROVISIONING = 'groupProvisioning';
+	public const SETTING_GROUP_WHITELIST_REGEX = 'groupWhitelistRegex';
+	public const SETTING_RESTRICT_LOGIN_TO_GROUPS = 'restrictLoginToGroups';
 
 	public const BOOLEAN_SETTINGS_DEFAULT_VALUES = [
 		self::SETTING_GROUP_PROVISIONING => false,
@@ -55,6 +57,7 @@ class ProviderService {
 		self::SETTING_UNIQUE_UID => true,
 		self::SETTING_CHECK_BEARER => false,
 		self::SETTING_SEND_ID_TOKEN_HINT => false,
+		self::SETTING_RESTRICT_LOGIN_TO_GROUPS => false,
 	];
 
 	public function __construct(
@@ -159,7 +162,9 @@ private function getSupportedSettings(): array {
 			self::SETTING_BEARER_PROVISIONING,
 			self::SETTING_EXTRA_CLAIMS,
 			self::SETTING_PROVIDER_BASED_ID,
-			self::SETTING_GROUP_PROVISIONING
+			self::SETTING_GROUP_PROVISIONING,
+			self::SETTING_GROUP_WHITELIST_REGEX,
+			self::SETTING_RESTRICT_LOGIN_TO_GROUPS,
 		];
 	}
 

lib/Service/ProvisioningService.php

@@ -385,18 +385,19 @@ private function setUserAvatar(string $userId, string $avatarAttribute): void {
 		}
 	}
 
-	public function provisionUserGroups(IUser $user, int $providerId, object $idTokenPayload): void {
+	public function getSyncGroupsOfToken(int $providerId, object $idTokenPayload) {
 		$groupsAttribute = $this->providerService->getSetting($providerId, ProviderService::SETTING_MAPPING_GROUPS, 'groups');
 		$groupsData = $idTokenPayload->{$groupsAttribute} ?? null;
 
+		$groupsWhitelistRegex = $this->getGroupWhitelistRegex($providerId);
+
 		$event = new AttributeMappedEvent(ProviderService::SETTING_MAPPING_GROUPS, $idTokenPayload, json_encode($groupsData));
 		$this->eventDispatcher->dispatchTyped($event);
 		$this->logger->debug('Group mapping event dispatched');
 
 		if ($event->hasValue() && $event->getValue() !== null) {
 			// casted to null if empty value
 			$groups = json_decode($event->getValue() ?? '');
-			$userGroups = $this->groupManager->getUserGroups($user);
 			$syncGroups = [];
 
 			foreach ($groups as $k => $v) {
@@ -413,13 +414,35 @@ public function provisionUserGroups(IUser $user, int $providerId, object $idToke
 					continue;
 				}
 
+				if ($groupsWhitelistRegex && !preg_match($groupsWhitelistRegex, $group->gid)) {
+					$this->logger->debug('Skipped group `' . $group->gid . '` for importing as not part of whitelist');
+					continue;
+				}
+
 				$group->gid = $this->idService->getId($providerId, $group->gid);
 
 				$syncGroups[] = $group;
 			}
 
+			return $syncGroups;
+		}
+
+		return null;
+	}
+
+	public function provisionUserGroups(IUser $user, int $providerId, object $idTokenPayload): void {
+		$groupsWhitelistRegex = $this->getGroupWhitelistRegex($providerId);
+
+		$syncGroups = $this->getSyncGroupsOfToken($providerId, $idTokenPayload);
+
+		if ($syncGroups !== null) {
+
+			$userGroups = $this->groupManager->getUserGroups($user);
 			foreach ($userGroups as $group) {
 				if (!in_array($group->getGID(), array_column($syncGroups, 'gid'))) {
+					if ($groupsWhitelistRegex && !preg_match($groupsWhitelistRegex, $group->getGID())) {
+						continue;
+					}
 					$group->removeUser($user);
 				}
 			}
@@ -437,4 +460,16 @@ public function provisionUserGroups(IUser $user, int $providerId, object $idToke
 			}
 		}
 	}
+
+	public function getGroupWhitelistRegex(int $providerId): string {
+		$regex = $this->providerService->getSetting($providerId, ProviderService::SETTING_GROUP_WHITELIST_REGEX, '');
+
+		// If regex does not start with '/', add '/' to the beginning and end
+		// Only check first character to allow for flags at the end of the regex
+		if ($regex && substr($regex, 0, 1) !== '/') {
+			$regex = '/' . $regex . '/';
+		}
+
+		return $regex;
+	}
 }

src/components/SettingsForm.vue

@@ -240,6 +240,21 @@
 		<p class="settings-hint">
 			{{ t('user_oidc', 'This will create and update the users groups depending on the groups claim in the id token. The Format of the groups claim value should be [{gid: "1", displayName: "group1"}, ...] or ["group1", "group2", ...]') }}
 		</p>
+		<p>
+			<label for="group-whitelist-regex">{{ t('user_oidc', 'Group whitelist regex') }}</label>
+			<input id="group-whitelist-regex"
+				v-model="localProvider.settings.groupWhitelistRegex"
+				type="text">
+		</p>
+		<p class="settings-hint">
+			{{ t('user_oidc', 'Only groups matching the whitelist regex will be created, updated and deleted by the group claim. For example: {regex} allows all groups which ID starts with {substr}', { regex: '/^blue/', substr: 'blue' }) }}
+		</p>
+		<NcCheckboxRadioSwitch :checked.sync="localProvider.settings.restrictLoginToGroups" wrapper-element="div">
+			{{ t('user_oidc', 'Restrict login for users that are not in any whitelisted group') }}
+		</NcCheckboxRadioSwitch>
+		<p class="settings-hint">
+			{{ t('user_oidc', 'Users that are not part of any whitelisted group are not created and can not login') }}
+		</p>
 		<NcCheckboxRadioSwitch :checked.sync="localProvider.settings.checkBearer" wrapper-element="div">
 			{{ t('user_oidc', 'Check Bearer token on API and WebDav requests') }}
 		</NcCheckboxRadioSwitch>

tests/psalm-baseline.xml

@@ -4,8 +4,5 @@
     <MissingDependency>
       <code><![CDATA[Image]]></code>
     </MissingDependency>
-    <RedundantCondition>
-      <code><![CDATA[isset($group->displayName)]]></code>
-    </RedundantCondition>
   </file>
 </files>

tests/unit/Db/UserMapperTest.php

@@ -20,6 +20,9 @@ class UserMapperTest extends TestCase {
 	/** @var LocalIdService|MockObject */
 	private $idService;
 
+	/** @var IDBConnection|MockObject */
+	private $db;
+
 	/** @var UserMapper|MockObject */
 	private $userMapper;
 

tests/unit/Service/ProviderServiceTest.php

@@ -88,6 +88,8 @@ public function testGetProvidersWithSettings() {
 					'extraClaims' => '1',
 					'providerBasedId' => true,
 					'groupProvisioning' => true,
+					'groupWhitelistRegex' => '1',
+					'restrictLoginToGroups' => true,
 				],
 			],
 			[
@@ -126,6 +128,8 @@ public function testGetProvidersWithSettings() {
 					'extraClaims' => '1',
 					'providerBasedId' => true,
 					'groupProvisioning' => true,
+					'groupWhitelistRegex' => '1',
+					'restrictLoginToGroups' => true,
 				],
 			],
 		], $this->providerService->getProvidersWithSettings());
@@ -161,6 +165,8 @@ public function testSetSettings() {
 			'mappingBiography' => 'biography',
 			'mappingPhonenumber' => 'phone',
 			'mappingGender' => 'gender',
+			'groupWhitelistRegex' => '',
+			'restrictLoginToGroups' => false,
 		];
 		$this->config->expects(self::any())
 			->method('getAppValue')
@@ -193,6 +199,8 @@ public function testSetSettings() {
 				[Application::APP_ID, 'provider-1-' . ProviderService::SETTING_EXTRA_CLAIMS, '', 'claim1 claim2'],
 				[Application::APP_ID, 'provider-1-' . ProviderService::SETTING_PROVIDER_BASED_ID, '', '0'],
 				[Application::APP_ID, 'provider-1-' . ProviderService::SETTING_GROUP_PROVISIONING, '', '1'],
+				[Application::APP_ID, 'provider-1-' . ProviderService::SETTING_GROUP_WHITELIST_REGEX, '', ''],
+				[Application::APP_ID, 'provider-1-' . ProviderService::SETTING_RESTRICT_LOGIN_TO_GROUPS, '', '0'],
 			]);
 
 		Assert::assertEquals(

tests/unit/Service/ProvisioningServiceTest.php

@@ -34,6 +34,9 @@ class ProvisioningServiceTest extends TestCase {
 	/** @var ProvisioningService | MockObject */
 	private $providerService;
 
+	/** @var LdapService | MockObject */
+	private $ldapService;
+
 	/** @var UserMapper | MockObject */
 	private $userMapper;
 
@@ -170,7 +173,9 @@ public function dataProvisionUserGroups() {
 							'displayName' => 'groupName1'
 						]
 					],
-				]
+				],
+				'',
+				true,
 			],
 			[
 				'1',
@@ -179,26 +184,71 @@ public function dataProvisionUserGroups() {
 					'groups' => [
 						'group2'
 					],
-				]
+				],
+				'',
+				true,
+			],
+			[
+				'1_Group_Import',
+				'Imported from OIDC',
+				(object)[
+					'groups' => [
+						(object)[
+							'gid' => '1_Group_Import',
+							'displayName' => 'Imported from OIDC',
+						],
+						(object)[
+							'gid' => '10_Group_NoImport',
+							'displayName' => 'Not Imported',
+						]
+					],
+				],
+				'/^1_/',
+				false
+			],
+			[
+				'1',
+				'users_nextcloud',
+				(object)[
+					'groups' => [
+						'users_nextcloud',
+						'users',
+					],
+				],
+				'nextcloud',
+				false,
 			],
 		];
 	}
 
 	/** @dataProvider dataProvisionUserGroups */
-	public function testProvisionUserGroups(string $gid, string $displayName, object $payload): void {
+	public function testProvisionUserGroups(string $gid, string $displayName, object $payload, string $group_whitelist, bool $expect_delete_local_group): void {
 		$user = $this->createMock(IUser::class);
 		$group = $this->createMock(IGroup::class);
+		$local_group = $this->createMock(IGroup::class);
 		$providerId = 421;
 
 		$this->providerService
 			->method('getSetting')
-			->with($providerId, ProviderService::SETTING_MAPPING_GROUPS, 'groups')
-			->willReturn('groups');
+			->will($this->returnValueMap(
+				[
+					[$providerId, ProviderService::SETTING_GROUP_WHITELIST_REGEX, '', $group_whitelist],
+					[$providerId, ProviderService::SETTING_MAPPING_GROUPS, 'groups', 'groups'],
+				]
+			));
 
 		$this->groupManager
 			->method('getUserGroups')
 			->with($user)
-			->willReturn([]);
+			->willReturn([$local_group]);
+
+		$local_group
+			->method('getGID')
+			->willReturn('local_group');
+
+		$local_group->expects($expect_delete_local_group ? self::once() : self::never())
+			->method('removeUser')
+			->with($user);
 
 		$this->idService->method('getId')
 			->willReturn($gid);