Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

turn LDAP's treat remnants as disabled feature config-independent #46992

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions apps/user_ldap/appinfo/info.xml
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
A user logs into Nextcloud with their LDAP or AD credentials, and is granted access based on an authentication request handled by the LDAP or AD server. Nextcloud does not store LDAP or AD passwords, rather these credentials are used to authenticate a user and then Nextcloud uses a session for the user ID. More information is available in the LDAP User and Group Backend documentation.

</description>
<version>1.21.0</version>
<version>1.21.1</version>
<licence>agpl</licence>
<author>Dominik Schmidt</author>
<author>Arthur Schiwon</author>
Expand All @@ -39,16 +39,18 @@ A user logs into Nextcloud with their LDAP or AD credentials, and is granted acc
</background-jobs>

<repair-steps>
<post-migration>
<step>OCA\User_LDAP\Migration\UUIDFixInsert</step>
<step>OCA\User_LDAP\Migration\RemoveRefreshTime</step>
<step>OCA\User_LDAP\Migration\RearrangeMarkRemnantsAsDisabled</step>
</post-migration>
<install>
<step>OCA\User_LDAP\Migration\SetDefaultProvider</step>
</install>
<uninstall>
<step>OCA\User_LDAP\Migration\UnsetDefaultProvider</step>
</uninstall>
<post-migration>
<step>OCA\User_LDAP\Migration\UUIDFixInsert</step>
<step>OCA\User_LDAP\Migration\RemoveRefreshTime</step>
</post-migration>

</repair-steps>

<commands>
Expand All @@ -69,6 +71,7 @@ A user logs into Nextcloud with their LDAP or AD credentials, and is granted acc

<settings>
<admin>OCA\User_LDAP\Settings\Admin</admin>
<admin>OCA\User_LDAP\Settings\AppSettings</admin>
<admin-section>OCA\User_LDAP\Settings\Section</admin-section>
</settings>
</info>
2 changes: 2 additions & 0 deletions apps/user_ldap/composer/composer/autoload_classmap.php
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@
'OCA\\User_LDAP\\Mapping\\GroupMapping' => $baseDir . '/../lib/Mapping/GroupMapping.php',
'OCA\\User_LDAP\\Mapping\\UserMapping' => $baseDir . '/../lib/Mapping/UserMapping.php',
'OCA\\User_LDAP\\Migration\\GroupMappingMigration' => $baseDir . '/../lib/Migration/GroupMappingMigration.php',
'OCA\\User_LDAP\\Migration\\RearrangeMarkRemnantsAsDisabled' => $baseDir . '/../lib/Migration/RearrangeMarkRemnantsAsDisabled.php',
'OCA\\User_LDAP\\Migration\\RemoveRefreshTime' => $baseDir . '/../lib/Migration/RemoveRefreshTime.php',
'OCA\\User_LDAP\\Migration\\SetDefaultProvider' => $baseDir . '/../lib/Migration/SetDefaultProvider.php',
'OCA\\User_LDAP\\Migration\\UUIDFix' => $baseDir . '/../lib/Migration/UUIDFix.php',
Expand All @@ -83,6 +84,7 @@
'OCA\\User_LDAP\\Service\\BirthdateParserService' => $baseDir . '/../lib/Service/BirthdateParserService.php',
'OCA\\User_LDAP\\Service\\UpdateGroupsService' => $baseDir . '/../lib/Service/UpdateGroupsService.php',
'OCA\\User_LDAP\\Settings\\Admin' => $baseDir . '/../lib/Settings/Admin.php',
'OCA\\User_LDAP\\Settings\\AppSettings' => $baseDir . '/../lib/Settings/AppSettings.php',
'OCA\\User_LDAP\\Settings\\Section' => $baseDir . '/../lib/Settings/Section.php',
'OCA\\User_LDAP\\SetupChecks\\LdapConnection' => $baseDir . '/../lib/SetupChecks/LdapConnection.php',
'OCA\\User_LDAP\\SetupChecks\\LdapInvalidUuids' => $baseDir . '/../lib/SetupChecks/LdapInvalidUuids.php',
Expand Down
2 changes: 2 additions & 0 deletions apps/user_ldap/composer/composer/autoload_static.php
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ class ComposerStaticInitUser_LDAP
'OCA\\User_LDAP\\Mapping\\GroupMapping' => __DIR__ . '/..' . '/../lib/Mapping/GroupMapping.php',
'OCA\\User_LDAP\\Mapping\\UserMapping' => __DIR__ . '/..' . '/../lib/Mapping/UserMapping.php',
'OCA\\User_LDAP\\Migration\\GroupMappingMigration' => __DIR__ . '/..' . '/../lib/Migration/GroupMappingMigration.php',
'OCA\\User_LDAP\\Migration\\RearrangeMarkRemnantsAsDisabled' => __DIR__ . '/..' . '/../lib/Migration/RearrangeMarkRemnantsAsDisabled.php',
'OCA\\User_LDAP\\Migration\\RemoveRefreshTime' => __DIR__ . '/..' . '/../lib/Migration/RemoveRefreshTime.php',
'OCA\\User_LDAP\\Migration\\SetDefaultProvider' => __DIR__ . '/..' . '/../lib/Migration/SetDefaultProvider.php',
'OCA\\User_LDAP\\Migration\\UUIDFix' => __DIR__ . '/..' . '/../lib/Migration/UUIDFix.php',
Expand All @@ -98,6 +99,7 @@ class ComposerStaticInitUser_LDAP
'OCA\\User_LDAP\\Service\\BirthdateParserService' => __DIR__ . '/..' . '/../lib/Service/BirthdateParserService.php',
'OCA\\User_LDAP\\Service\\UpdateGroupsService' => __DIR__ . '/..' . '/../lib/Service/UpdateGroupsService.php',
'OCA\\User_LDAP\\Settings\\Admin' => __DIR__ . '/..' . '/../lib/Settings/Admin.php',
'OCA\\User_LDAP\\Settings\\AppSettings' => __DIR__ . '/..' . '/../lib/Settings/AppSettings.php',
'OCA\\User_LDAP\\Settings\\Section' => __DIR__ . '/..' . '/../lib/Settings/Section.php',
'OCA\\User_LDAP\\SetupChecks\\LdapConnection' => __DIR__ . '/..' . '/../lib/SetupChecks/LdapConnection.php',
'OCA\\User_LDAP\\SetupChecks\\LdapInvalidUuids' => __DIR__ . '/..' . '/../lib/SetupChecks/LdapInvalidUuids.php',
Expand Down
13 changes: 0 additions & 13 deletions apps/user_ldap/js/wizard/wizardTabAdvanced.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,6 @@ OCA = OCA || {};
$element: $('#ldap_attributes_for_user_search'),
setMethod: 'setSearchAttributesUsers'
},
ldap_mark_remnants_as_disabled: {
$element: $('#ldap_mark_remnants_as_disabled'),
setMethod: 'setMarkRemnantsAsDisabled'
},
ldap_group_display_name: {
$element: $('#ldap_group_display_name'),
setMethod: 'setGroupDisplayName'
Expand Down Expand Up @@ -282,15 +278,6 @@ OCA = OCA || {};
this.setElementValue(this.managedItems.ldap_attributes_for_user_search.$element, attributes);
},

/**
* enables or disables marking remnants as disabled
*
* @param {string} markRemnantsAsDisabled contains an int
*/
setMarkRemnantsAsDisabled: function(markRemnantsAsDisabled) {
this.setElementValue(this.managedItems.ldap_mark_remnants_as_disabled.$element, markRemnantsAsDisabled);
},

/**
* sets the display name attribute for groups
*
Expand Down
17 changes: 15 additions & 2 deletions apps/user_ldap/lib/Configuration.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
*/
namespace OCA\User_LDAP;

use OCP\IAppConfig;
use Psr\Log\LoggerInterface;

/**
Expand Down Expand Up @@ -296,6 +297,9 @@ public function readConfiguration(): void {
case 'ldapGroupDisplayName':
$readMethod = 'getLcValue';
break;
case 'markRemnantsAsDisabled':
$readMethod = 'getGlobalAppValueAsBool';
break;
case 'ldapUserDisplayName':
default:
// user display name does not lower case because
Expand Down Expand Up @@ -340,6 +344,7 @@ public function saveConfiguration(): void {
case 'ldapIgnoreNamingRules':
case 'ldapUuidUserAttribute':
case 'ldapUuidGroupAttribute':
case 'markRemnantsAsDisabled':
continue 2;
}
if (is_null($value)) {
Expand Down Expand Up @@ -414,6 +419,14 @@ protected function getLcValue(string $varName): string {
return mb_strtolower($this->getValue($varName), 'UTF-8');
}

protected function getGlobalAppValueAsBool(string $varName): bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it returns bool the magic property in phpdoc before the class should be typed to bool I think.
Also the default value in getDefaults is now unused for this property I think.

static $appConfig;
if (!$appConfig) {
$appConfig = \OCP\Server::get(IAppConfig::class);
}
return $appConfig->getValueBool('user_ldap', $varName, false);
}

protected function getSystemValue(string $varName): string {
//FIXME: if another system value is added, softcode the default value
return \OC::$server->getConfig()->getSystemValue($varName, false);
Expand Down Expand Up @@ -514,7 +527,7 @@ public function getDefaults(): array {
'ldap_expert_uuid_group_attr' => '',
'has_memberof_filter_support' => 0,
'use_memberof_to_detect_membership' => 1,
'ldap_mark_remnants_as_disabled' => 0,
'backend_mark_remnants_as_disabled' => 0,
'last_jpegPhoto_lookup' => 0,
'ldap_nested_groups' => 0,
'ldap_paging_size' => 500,
Expand Down Expand Up @@ -593,7 +606,7 @@ public function getConfigTranslationArray(): array {
'ldap_expert_uuid_group_attr' => 'ldapExpertUUIDGroupAttr',
'has_memberof_filter_support' => 'hasMemberOfFilterSupport',
'use_memberof_to_detect_membership' => 'useMemberOfToDetectMembership',
'ldap_mark_remnants_as_disabled' => 'markRemnantsAsDisabled',
'backend_mark_remnants_as_disabled' => 'markRemnantsAsDisabled',
'last_jpegPhoto_lookup' => 'lastJpegPhotoLookup',
'ldap_nested_groups' => 'ldapNestedGroups',
'ldap_paging_size' => 'ldapPagingSize',
Expand Down
54 changes: 54 additions & 0 deletions apps/user_ldap/lib/Migration/RearrangeMarkRemnantsAsDisabled.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
<?php

declare(strict_types=1);
/**
* SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-or-later
*/

namespace OCA\User_LDAP\Migration;

use OCP\IAppConfig;
use OCP\IDBConnection;
use OCP\Migration\IOutput;
use OCP\Migration\IRepairStep;

class RearrangeMarkRemnantsAsDisabled implements IRepairStep {
public function __construct(
protected IAppConfig $appConfig,
protected IDBConnection $dbc,
) {

}
public function getName(): string {
return 'Rearrange the configuration of ldap_mark_remnants_as_disabled';
}

public function run(IOutput $output): void {
$allKeys = $this->appConfig->getKeys('user_ldap');

if (in_array('backend_mark_remnants_as_disabled', $allKeys, true)) {
return;
}

// if it was enabled for at least one configuration, use it as global configuration
$filteredKeys = array_filter($allKeys, static function (string $key): bool {
return str_contains($key, 'ldap_mark_remnants_as_disabled');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return str_contains($key, 'ldap_mark_remnants_as_disabled');
return str_ends_with($key, 'ldap_mark_remnants_as_disabled');

});
$newValue = false;
foreach ($filteredKeys as $filteredKey) {
$newValue = $newValue || $this->appConfig->getValueBool('user_ldap', $filteredKey);
}

// set the new value
$this->appConfig->setValueBool('user_ldap', 'backend_mark_remnants_as_disabled', $newValue);
if ($newValue) {
$output->info('The option "Disable missing users from LDAP" is activated.');
}

// clean up now that the new value is saved
foreach ($filteredKeys as $filteredKey) {
$this->appConfig->deleteKey('user_ldap', $filteredKey);
}
}
}
43 changes: 43 additions & 0 deletions apps/user_ldap/lib/Settings/AppSettings.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
<?php

namespace OCA\User_LDAP\Settings;

use OCP\AppFramework\Http\TemplateResponse;
use OCP\AppFramework\Services\IInitialState;
use OCP\IAppConfig;
use OCP\IL10N;
use OCP\Settings\IDelegatedSettings;

class AppSettings implements IDelegatedSettings {

public function __construct(
protected IL10N $l,
protected IInitialState $initialState,
protected IAppConfig $appConfig,
) {
}

public function getName(): ?string {
return $this->l->t('Configuration-independent settings');
}

public function getAuthorizedAppConfig(): array {
return [];
}

public function getForm(): TemplateResponse {
$this->initialState->provideInitialState('config', [
'backend_mark_remnants_as_disabled' => $this->appConfig->getValueBool('user_ldap', 'backend_mark_remnants_as_disabled'),
]);

return new TemplateResponse('user_ldap', 'app-settings');
}

public function getSection(): string {
return 'ldap';
}

public function getPriority(): int {
return 9;
}
}
11 changes: 1 addition & 10 deletions apps/user_ldap/lib/User_LDAP.php
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,10 @@
use OCP\Notification\IManager as INotificationManager;
use OCP\User\Backend\ICountMappedUsersBackend;
use OCP\User\Backend\ICountUsersBackend;
use OCP\User\Backend\IProvideEnabledStateBackend;
use OCP\UserInterface;
use Psr\Log\LoggerInterface;

class User_LDAP extends BackendUtility implements IUserBackend, UserInterface, IUserLDAP, ICountUsersBackend, ICountMappedUsersBackend, IProvideEnabledStateBackend {
class User_LDAP extends BackendUtility implements IUserBackend, UserInterface, IUserLDAP, ICountUsersBackend, ICountMappedUsersBackend {
protected INotificationManager $notificationManager;
protected UserPluginManager $userPluginManager;
protected LoggerInterface $logger;
Expand Down Expand Up @@ -632,14 +631,6 @@ public function createUser($username, $password) {
return false;
}

public function isUserEnabled(string $uid, callable $queryDatabaseValue): bool {
if ($this->deletedUsersIndex->isUserMarked($uid) && ((int)$this->access->connection->markRemnantsAsDisabled === 1)) {
return false;
} else {
return $queryDatabaseValue();
}
}

public function setUserEnabled(string $uid, bool $enabled, callable $queryDatabaseValue, callable $setDatabaseValue): bool {
$setDatabaseValue($enabled);
return $enabled;
Expand Down
17 changes: 15 additions & 2 deletions apps/user_ldap/lib/User_Proxy.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
use OCA\User_LDAP\User\DeletedUsersIndex;
use OCA\User_LDAP\User\OfflineUser;
use OCA\User_LDAP\User\User;
use OCP\IAppConfig;
use OCP\IUserBackend;
use OCP\Notification\IManager as INotificationManager;
use OCP\User\Backend\ICountMappedUsersBackend;
Expand Down Expand Up @@ -38,6 +39,7 @@ public function __construct(
UserPluginManager $userPluginManager,
LoggerInterface $logger,
DeletedUsersIndex $deletedUsersIndex,
protected IAppConfig $appConfig,
) {
parent::__construct($ldap, $accessFactory);
$this->helper = $helper;
Expand Down Expand Up @@ -422,15 +424,18 @@ public function createUser($username, $password) {
}

public function isUserEnabled(string $uid, callable $queryDatabaseValue): bool {
return $this->handleRequest($uid, 'isUserEnabled', [$uid, $queryDatabaseValue]);
if (!$this->treatRemnantsAsDisabled()) {
return $queryDatabaseValue();
}
return !$this->deletedUsersIndex->isUserMarked($uid);
Comment on lines +427 to +430
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic is wrong here.
If treatRemnantsAsDisabled is true and the user is not marked we should still query the DB.

Suggested change
if (!$this->treatRemnantsAsDisabled()) {
return $queryDatabaseValue();
}
return !$this->deletedUsersIndex->isUserMarked($uid);
if ($this->treatRemnantsAsDisabled() && $this->deletedUsersIndex->isUserMarked($uid)) {
return false;
} else {
return $queryDatabaseValue();
}

}

public function setUserEnabled(string $uid, bool $enabled, callable $queryDatabaseValue, callable $setDatabaseValue): bool {
return $this->handleRequest($uid, 'setUserEnabled', [$uid, $enabled, $queryDatabaseValue, $setDatabaseValue]);
}

public function getDisabledUserList(?int $limit = null, int $offset = 0, string $search = ''): array {
if ((int)$this->getAccess(array_key_first($this->backends) ?? '')->connection->markRemnantsAsDisabled !== 1) {
if (!$this->treatRemnantsAsDisabled()) {
return [];
}
$disabledUsers = $this->deletedUsersIndex->getUsers();
Expand All @@ -453,4 +458,12 @@ public function getDisabledUserList(?int $limit = null, int $offset = 0, string
)
);
}

protected function treatRemnantsAsDisabled(): bool {
static $setting;
if (!$setting) {
$setting = $this->appConfig->getValueBool('user_ldap', 'markRemnantsAsDisabled', false);
}
return $setting;
}
}
53 changes: 53 additions & 0 deletions apps/user_ldap/src/AppSettings.vue
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
<!--
- SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
- SPDX-License-Identifier: AGPL-3.0-or-later
-->

<template>
<NcSettingsSection :name="t('user_ldap', 'Configuration-independent settings')">
<ul class="user_ldap__app-settings-list">
<li>
<NcCheckboxRadioSwitch :checked="config.backend_mark_remnants_as_disabled"
type="switch"
@update:checked="updateBoolSetting('backend_mark_remnants_as_disabled')">
{{ t('user_ldap', 'Disable users missing from LDAP') }}
</NcCheckboxRadioSwitch>
</li>
</ul>
</NcSettingsSection>
</template>

<script>
import { showError, showSuccess } from '@nextcloud/dialogs'
import { loadState } from '@nextcloud/initial-state'

import NcCheckboxRadioSwitch from '@nextcloud/vue/dist/Components/NcCheckboxRadioSwitch.js'
import NcSettingsSection from '@nextcloud/vue/dist/Components/NcSettingsSection.js'

export default {
name: 'AppSettings',
components: {
NcCheckboxRadioSwitch,
NcSettingsSection,
},

data() {
return {
config: loadState('user_ldap', 'config'),
}
},

methods: {
async updateBoolSetting(setting) {
OCP.AppConfig.setValue('user_ldap', setting, this.config[setting] ? '0' : '1', {
success: () => {
showSuccess(t('user_ldap', 'Settings saved'))
this.config[setting] = !this.config[setting]
},
error: () => showError(t('user_ldap', 'Error while saving settings')),
})
},
},
}
</script>

Check failure on line 53 in apps/user_ldap/src/AppSettings.vue

View workflow job for this annotation

GitHub Actions / NPM lint

Too many blank lines at the end of file. Max of 0 allowed
Loading
Loading