Skip to content

Commit d161cdd

Browse files
julien-ncjulien-nc
authored
Merge pull request #884 from bergerar/main
 feat: restrict login to users matching a certain group
2 parents 42fde60 + c308b58 commit d161cdd

File tree

8 files changed

+137
-12
lines changed

8 files changed

+137
-12
lines changed

‎lib/Controller/LoginController.php‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -454,6 +454,18 @@ public function code(string $state = '', string $code = '', string $scope = '',
454454
return $this->build403TemplateResponse($message, Http::STATUS_BAD_REQUEST, ['reason' => 'failed to provision user']);
455455
}
456456

457+
// prevent login of users that are not in a whitelisted group (if activated)
458+
$restrictLoginToGroups = $this->providerService->getSetting($providerId, ProviderService::SETTING_RESTRICT_LOGIN_TO_GROUPS, '0');
459+
if ($restrictLoginToGroups === '1') {
460+
$syncGroups = $this->provisioningService->getSyncGroupsOfToken($providerId, $idTokenPayload);
461+
462+
if ($syncGroups === null || count($syncGroups) === 0) {
463+
$this->logger->debug('Prevented user from login as user is not part of a whitelisted group');
464+
$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.');
465+
return $this->build403TemplateResponse($message, Http::STATUS_FORBIDDEN, ['reason' => 'user not in any whitelisted group']);
466+
}
467+
}
468+
457469
$autoProvisionAllowed = (!isset($oidcSystemConfig['auto_provision']) || $oidcSystemConfig['auto_provision']);
458470

459471
// in case user is provisioned by user_ldap, userManager->search() triggers an ldap search which syncs the results

‎lib/Service/ProviderService.php‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,8 @@ class ProviderService {
4747
public const SETTING_JWKS_CACHE_TIMESTAMP = 'jwksCacheTimestamp';
4848
public const SETTING_PROVIDER_BASED_ID = 'providerBasedId';
4949
public const SETTING_GROUP_PROVISIONING = 'groupProvisioning';
50+
public const SETTING_GROUP_WHITELIST_REGEX = 'groupWhitelistRegex';
51+
public const SETTING_RESTRICT_LOGIN_TO_GROUPS = 'restrictLoginToGroups';
5052

5153
public const BOOLEAN_SETTINGS_DEFAULT_VALUES = [
5254
self::SETTING_GROUP_PROVISIONING => false,
@@ -55,6 +57,7 @@ class ProviderService {
5557
self::SETTING_UNIQUE_UID => true,
5658
self::SETTING_CHECK_BEARER => false,
5759
self::SETTING_SEND_ID_TOKEN_HINT => false,
60+
self::SETTING_RESTRICT_LOGIN_TO_GROUPS => false,
5861
];
5962

6063
public function __construct(
@@ -159,7 +162,9 @@ private function getSupportedSettings(): array {
159162
self::SETTING_BEARER_PROVISIONING,
160163
self::SETTING_EXTRA_CLAIMS,
161164
self::SETTING_PROVIDER_BASED_ID,
162-
self::SETTING_GROUP_PROVISIONING
165+
self::SETTING_GROUP_PROVISIONING,
166+
self::SETTING_GROUP_WHITELIST_REGEX,
167+
self::SETTING_RESTRICT_LOGIN_TO_GROUPS,
163168
];
164169
}
165170

‎lib/Service/ProvisioningService.php‎

Lines changed: 37 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -385,18 +385,19 @@ private function setUserAvatar(string $userId, string $avatarAttribute): void {
385385
}
386386
}
387387

388-
public function provisionUserGroups(IUser $user, int $providerId, object $idTokenPayload): void {
388+
public function getSyncGroupsOfToken(int $providerId, object $idTokenPayload) {
389389
$groupsAttribute = $this->providerService->getSetting($providerId, ProviderService::SETTING_MAPPING_GROUPS, 'groups');
390390
$groupsData = $idTokenPayload->{$groupsAttribute} ?? null;
391391

392+
$groupsWhitelistRegex = $this->getGroupWhitelistRegex($providerId);
393+
392394
$event = new AttributeMappedEvent(ProviderService::SETTING_MAPPING_GROUPS, $idTokenPayload, json_encode($groupsData));
393395
$this->eventDispatcher->dispatchTyped($event);
394396
$this->logger->debug('Group mapping event dispatched');
395397

396398
if ($event->hasValue() && $event->getValue() !== null) {
397399
// casted to null if empty value
398400
$groups = json_decode($event->getValue() ?? '');
399-
$userGroups = $this->groupManager->getUserGroups($user);
400401
$syncGroups = [];
401402

402403
foreach ($groups as $k => $v) {
@@ -413,13 +414,35 @@ public function provisionUserGroups(IUser $user, int $providerId, object $idToke
413414
continue;
414415
}
415416

417+
if ($groupsWhitelistRegex && !preg_match($groupsWhitelistRegex, $group->gid)) {
418+
$this->logger->debug('Skipped group `' . $group->gid . '` for importing as not part of whitelist');
419+
continue;
420+
}
421+
416422
$group->gid = $this->idService->getId($providerId, $group->gid);
417423

418424
$syncGroups[] = $group;
419425
}
420426

427+
return $syncGroups;
428+
}
429+
430+
return null;
431+
}
432+
433+
public function provisionUserGroups(IUser $user, int $providerId, object $idTokenPayload): void {
434+
$groupsWhitelistRegex = $this->getGroupWhitelistRegex($providerId);
435+
436+
$syncGroups = $this->getSyncGroupsOfToken($providerId, $idTokenPayload);
437+
438+
if ($syncGroups !== null) {
439+
440+
$userGroups = $this->groupManager->getUserGroups($user);
421441
foreach ($userGroups as $group) {
422442
if (!in_array($group->getGID(), array_column($syncGroups, 'gid'))) {
443+
if ($groupsWhitelistRegex && !preg_match($groupsWhitelistRegex, $group->getGID())) {
444+
continue;
445+
}
423446
$group->removeUser($user);
424447
}
425448
}
@@ -437,4 +460,16 @@ public function provisionUserGroups(IUser $user, int $providerId, object $idToke
437460
}
438461
}
439462
}
463+
464+
public function getGroupWhitelistRegex(int $providerId): string {
465+
$regex = $this->providerService->getSetting($providerId, ProviderService::SETTING_GROUP_WHITELIST_REGEX, '');
466+
467+
// If regex does not start with '/', add '/' to the beginning and end
468+
// Only check first character to allow for flags at the end of the regex
469+
if ($regex && substr($regex, 0, 1) !== '/') {
470+
$regex = '/' . $regex . '/';
471+
}
472+
473+
return $regex;
474+
}
440475
}

‎src/components/SettingsForm.vue‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -240,6 +240,21 @@
240240
<p class="settings-hint">
241241
{{ 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", ...]') }}
242242
</p>
243+
<p>
244+
<label for="group-whitelist-regex">{{ t('user_oidc', 'Group whitelist regex') }}</label>
245+
<input id="group-whitelist-regex"
246+
v-model="localProvider.settings.groupWhitelistRegex"
247+
type="text">
248+
</p>
249+
<p class="settings-hint">
250+
{{ 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' }) }}
251+
</p>
252+
<NcCheckboxRadioSwitch :checked.sync="localProvider.settings.restrictLoginToGroups" wrapper-element="div">
253+
{{ t('user_oidc', 'Restrict login for users that are not in any whitelisted group') }}
254+
</NcCheckboxRadioSwitch>
255+
<p class="settings-hint">
256+
{{ t('user_oidc', 'Users that are not part of any whitelisted group are not created and can not login') }}
257+
</p>
243258
<NcCheckboxRadioSwitch :checked.sync="localProvider.settings.checkBearer" wrapper-element="div">
244259
{{ t('user_oidc', 'Check Bearer token on API and WebDav requests') }}
245260
</NcCheckboxRadioSwitch>

‎tests/psalm-baseline.xml‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,5 @@
44
<MissingDependency>
55
<code><![CDATA[Image]]></code>
66
</MissingDependency>
7-
<RedundantCondition>
8-
<code><![CDATA[isset($group->displayName)]]></code>
9-
</RedundantCondition>
107
</file>
118
</files>

‎tests/unit/Db/UserMapperTest.php‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,9 @@ class UserMapperTest extends TestCase {
2020
/** @var LocalIdService|MockObject */
2121
private $idService;
2222

23+
/** @var IDBConnection|MockObject */
24+
private $db;
25+
2326
/** @var UserMapper|MockObject */
2427
private $userMapper;
2528

‎tests/unit/Service/ProviderServiceTest.php‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,8 @@ public function testGetProvidersWithSettings() {
8888
'extraClaims' => '1',
8989
'providerBasedId' => true,
9090
'groupProvisioning' => true,
91+
'groupWhitelistRegex' => '1',
92+
'restrictLoginToGroups' => true,
9193
],
9294
],
9395
[
@@ -126,6 +128,8 @@ public function testGetProvidersWithSettings() {
126128
'extraClaims' => '1',
127129
'providerBasedId' => true,
128130
'groupProvisioning' => true,
131+
'groupWhitelistRegex' => '1',
132+
'restrictLoginToGroups' => true,
129133
],
130134
],
131135
], $this->providerService->getProvidersWithSettings());
@@ -161,6 +165,8 @@ public function testSetSettings() {
161165
'mappingBiography' => 'biography',
162166
'mappingPhonenumber' => 'phone',
163167
'mappingGender' => 'gender',
168+
'groupWhitelistRegex' => '',
169+
'restrictLoginToGroups' => false,
164170
];
165171
$this->config->expects(self::any())
166172
->method('getAppValue')
@@ -193,6 +199,8 @@ public function testSetSettings() {
193199
[Application::APP_ID, 'provider-1-' . ProviderService::SETTING_EXTRA_CLAIMS, '', 'claim1 claim2'],
194200
[Application::APP_ID, 'provider-1-' . ProviderService::SETTING_PROVIDER_BASED_ID, '', '0'],
195201
[Application::APP_ID, 'provider-1-' . ProviderService::SETTING_GROUP_PROVISIONING, '', '1'],
202+
[Application::APP_ID, 'provider-1-' . ProviderService::SETTING_GROUP_WHITELIST_REGEX, '', ''],
203+
[Application::APP_ID, 'provider-1-' . ProviderService::SETTING_RESTRICT_LOGIN_TO_GROUPS, '', '0'],
196204
]);
197205

198206
Assert::assertEquals(

‎tests/unit/Service/ProvisioningServiceTest.php‎

Lines changed: 56 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,9 @@ class ProvisioningServiceTest extends TestCase {
3434
/** @var ProvisioningService | MockObject */
3535
private $providerService;
3636

37+
/** @var LdapService | MockObject */
38+
private $ldapService;
39+
3740
/** @var UserMapper | MockObject */
3841
private $userMapper;
3942

@@ -170,7 +173,9 @@ public function dataProvisionUserGroups() {
170173
'displayName' => 'groupName1'
171174
]
172175
],
173-
]
176+
],
177+
'',
178+
true,
174179
],
175180
[
176181
'1',
@@ -179,26 +184,71 @@ public function dataProvisionUserGroups() {
179184
'groups' => [
180185
'group2'
181186
],
182-
]
187+
],
188+
'',
189+
true,
190+
],
191+
[
192+
'1_Group_Import',
193+
'Imported from OIDC',
194+
(object)[
195+
'groups' => [
196+
(object)[
197+
'gid' => '1_Group_Import',
198+
'displayName' => 'Imported from OIDC',
199+
],
200+
(object)[
201+
'gid' => '10_Group_NoImport',
202+
'displayName' => 'Not Imported',
203+
]
204+
],
205+
],
206+
'/^1_/',
207+
false
208+
],
209+
[
210+
'1',
211+
'users_nextcloud',
212+
(object)[
213+
'groups' => [
214+
'users_nextcloud',
215+
'users',
216+
],
217+
],
218+
'nextcloud',
219+
false,
183220
],
184221
];
185222
}
186223

187224
/** @dataProvider dataProvisionUserGroups */
188-
public function testProvisionUserGroups(string $gid, string $displayName, object $payload): void {
225+
public function testProvisionUserGroups(string $gid, string $displayName, object $payload, string $group_whitelist, bool $expect_delete_local_group): void {
189226
$user = $this->createMock(IUser::class);
190227
$group = $this->createMock(IGroup::class);
228+
$local_group = $this->createMock(IGroup::class);
191229
$providerId = 421;
192230

193231
$this->providerService
194232
->method('getSetting')
195-
->with($providerId, ProviderService::SETTING_MAPPING_GROUPS, 'groups')
196-
->willReturn('groups');
233+
->will($this->returnValueMap(
234+
[
235+
[$providerId, ProviderService::SETTING_GROUP_WHITELIST_REGEX, '', $group_whitelist],
236+
[$providerId, ProviderService::SETTING_MAPPING_GROUPS, 'groups', 'groups'],
237+
]
238+
));
197239

198240
$this->groupManager
199241
->method('getUserGroups')
200242
->with($user)
201-
->willReturn([]);
243+
->willReturn([$local_group]);
244+
245+
$local_group
246+
->method('getGID')
247+
->willReturn('local_group');
248+
249+
$local_group->expects($expect_delete_local_group ? self::once() : self::never())
250+
->method('removeUser')
251+
->with($user);
202252

203253
$this->idService->method('getId')
204254
->willReturn($gid);

0 commit comments

Comments
 (0)