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

Add IProvideEnabledStateBackend interface #34443

Merged
merged 9 commits into from
Jul 3, 2023

Conversation

come-nc
Copy link
Contributor

@come-nc come-nc commented Oct 5, 2022

This allows user backends to present some users as disabled, and to manage themselves how disabling users works.
It is used in the LDAP backend to support a new option to make remnants users appear as disabled.

  • The configuration option is not exposed in the UI

@come-nc come-nc added this to the Nextcloud 26 milestone Oct 5, 2022
@come-nc come-nc requested a review from blizzz October 5, 2022 16:01
@come-nc come-nc self-assigned this Oct 5, 2022
@blizzz blizzz mentioned this pull request Feb 1, 2023
@skjnldsv skjnldsv mentioned this pull request Feb 23, 2023
@come-nc come-nc modified the milestones: Nextcloud 26, Nextcloud 27 Feb 23, 2023
@come-nc come-nc force-pushed the feat/add-enabled-user-backend branch from 6ef2147 to 41caf05 Compare March 30, 2023 13:15
@come-nc
Copy link
Contributor Author

come-nc commented Mar 30, 2023

Rebased and fixed conflicts.

@blizzz Should the configuration be global like implemented or for each ldap configuration? If we want to put in the UI we have to set it as a per-configuration setting anyway, no?

@blizzz
Copy link
Member

blizzz commented Apr 4, 2023

@blizzz Should the configuration be global like implemented or for each ldap configuration? If we want to put in the UI we have to set it as a per-configuration setting anyway, no?

We can also add another section with global LDAP settings. We have a few of them, but none are exposed. Is there a scenario where different settings per config make sense? For me it makes more sense as a global flag, otherwise we are inconsistent across configs, potentially resulting in confused admins.

@come-nc
Copy link
Contributor Author

come-nc commented Apr 5, 2023

We can also add another section with global LDAP settings. We have a few of them, but none are exposed. Is there a scenario where different settings per config make sense? For me it makes more sense as a global flag, otherwise we are inconsistent across configs, potentially resulting in confused admins.

Well I don’t know, a per-configuration setting can always be configured the same way for all configurations while if we make it a global setting people are stuck with it. But I have to say I do not have a particular usecase in mind.
If we make it global, can we leave it as-is and keep adding a config UI for a following PR?

Do we have a list of global settings that would make sense to appear in that UI?

@blizzz
Copy link
Member

blizzz commented Apr 5, 2023

Do we have a list of global settings that would make sense to appear in that UI?

By the power of ack we have in app config:

  • enforce_home_folder_naming_rule (i don't recall this one 😅 )
  • cleanUpJobChunkSize (doc should be sufficient)
  • bgjRefreshInterval (doc should be sufficient)

For completeness:

  • cleanUpJobOffset (controlled by the app, background job state)
  • background_sync_interval (controlled by the app, background job state)
  • background_sync_prefix (controlled by the app, background job state)
  • background_sync_offset (controlled by the app, background job state)

Then we also have system keys (config.php):

  • ldapProviderFactory (do not expose)
  • ldap_log_file (would be a candidate)
  • ldapUserCleanupInterval (doc should be sufficient)

This was referenced May 3, 2023
@skjnldsv skjnldsv modified the milestones: Nextcloud 27, Nextcloud 28 May 9, 2023
@come-nc come-nc force-pushed the feat/add-enabled-user-backend branch from 41caf05 to 50ccfb4 Compare May 23, 2023 14:09
@come-nc
Copy link
Contributor Author

come-nc commented May 23, 2023

Rebased and added the option to the UI.
I added it per-connection, because this is how the UI is organized, it fits nicely next to the other options in advanced.

@come-nc come-nc marked this pull request as ready for review May 23, 2023 14:33
@come-nc come-nc added 3. to review Waiting for reviews and removed 2. developing Work in progress labels May 23, 2023
@come-nc come-nc requested review from a team and removed request for a team May 23, 2023 15:47
Signed-off-by: Côme Chilliet <[email protected]>
Signed-off-by: Côme Chilliet <[email protected]>
* @param callable():bool $queryDatabaseValue A callable to query the enabled state from database
* @param callable(bool):void $setDatabaseValue A callable to set the enabled state in the database.
*/
public function setUserEnabled(string $uid, bool $enabled, callable $queryDatabaseValue, callable $setDatabaseValue): void;
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
public function setUserEnabled(string $uid, bool $enabled, callable $queryDatabaseValue, callable $setDatabaseValue): void;
public function setUserEnabledState(string $uid, bool $enabled, callable $queryDatabaseValue, callable $setDatabaseValue): void;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’d prefer to keep setUserEnabled wording, to match existing OCP\IUser::setEnabled.

lib/public/User/Backend/IProvideEnabledStateBackend.php Outdated Show resolved Hide resolved
lib/private/User/User.php Show resolved Hide resolved
lib/private/User/User.php Outdated Show resolved Hide resolved
lib/public/User/Backend/IProvideEnabledStateBackend.php Outdated Show resolved Hide resolved
@come-nc
Copy link
Contributor Author

come-nc commented Jun 20, 2023

So, since I started working on this I discovered we have no method to list disabled users.
As we need to avoid doing so by calling isEnabled on all users, I intend to add a method to this new interface to list disabled users.

@come-nc
Copy link
Contributor Author

come-nc commented Jun 29, 2023

So, since I started working on this I discovered we have no method to list disabled users. As we need to avoid doing so by calling isEnabled on all users, I intend to add a method to this new interface to list disabled users.

Done.

@come-nc come-nc merged commit b2f01b7 into master Jul 3, 2023
@come-nc come-nc deleted the feat/add-enabled-user-backend branch July 3, 2023 08:19
@ChristophWurst ChristophWurst added the pending documentation This pull request needs an associated documentation update label Oct 18, 2023
@Pytal
Copy link
Member

Pytal commented Mar 7, 2024

/backport to stable26

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants