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.

Sign up for GitHub

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

Jump to bottom

[stable24] LDAP to not register new users when outside of fair use or over limits #34730

Merged
merged 7 commits into from
Oct 25, 2022
Merged

[stable24] LDAP to not register new users when outside of fair use or over limits #34730

merged 7 commits into from
Oct 25, 2022

Conversation

backportbot-nextcloud[bot]
Copy link

@backportbot-nextcloud backportbot-nextcloud bot commented Oct 21, 2022 •
edited by come-nc
Loading

backport of #33945

@backportbot-nextcloud backportbot-nextcloud bot added this to the Nextcloud 24.0.7 milestone Oct 21, 2022
github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 21, 2022
View reviewed changes
apps/user_ldap/ajax/clearMappings.php Fixed Show fixed Hide fixed
apps/user_ldap/lib/Jobs/CleanUp.php Fixed Show fixed Hide fixed
apps/user_ldap/lib/Jobs/Sync.php Fixed Show fixed Hide fixed
apps/user_ldap/lib/Mapping/UserMapping.php Fixed Show fixed Hide fixed
apps/user_ldap/lib/Proxy.php Fixed Show fixed Hide fixed
blizzz and others added 5 commits October 24, 2022 10:05
@blizzz @come-nc
LDAP to no register new users when outside of fair use or over limits
6b220ff 
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz @come-nc
[LDAP] throw exception only against prov api
74322a7 
- unbreaks functionality for end users when on demand mapping takes
  place

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz @come-nc
improve admin notification experience
6bedcfe 
- do not stack notifications, replace them
- and replace them once a day only
- with LDAP it might end in total spam terror (also push) otherwise

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz @szaimen @come-nc
extended hint message
2228a7b 
Co-authored-by: Simon L. <szaimen@e.mail.de>
Signed-off-by: blizzz <blizzz@arthur-schiwon.de>
@come-nc
Get rid of calls to OCP\Server which is not available is 24
0f07e43 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Fix createUser return type in docblock to match code
51b295f 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Copy link
Contributor

come-nc commented Oct 24, 2022

This does not work as expected I think:

  1. The user count that is used for testing if the limit is reached is the one returned by the backend, which for LDAP lists users from the LDAP. Which means it will always be the total number of users in the LDAP even if none were mapped yet.
  2. That means with a big LDAP, none of the users will be mapped, as the limit is instantly reached.
  3. In the UI, when listing users, this results in an empty user list, as the hint exception is hit right away. I think the admin should be able to list existing users, always.
  4. If I change the behavior to forget about the hintexception and silently skip mapping instead (counting on the notification that the limit is reached to be enough), then only database users are listed and no user from LDAP ever appear because of behavior explained above.

It’s not clear to me how to fix that, because we do not want to change the LDAP backend to return only mapped user count when asked for user count, it’s quite useful that it returns the actual number.
Adding a new method for counting mapped user is a bit weird and overkill, it means adding a specific interface for this.

@come-nc
Fix return types of createUser method
fa5b0f3 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Copy link
Contributor

come-nc commented Oct 25, 2022

Should be merged and behavior will be fixed by #34795

szaimen
szaimen approved these changes Oct 25, 2022
View reviewed changes
Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

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

🐘

@come-nc come-nc mentioned this pull request Oct 25, 2022
Merged
PVince81
PVince81 approved these changes Oct 25, 2022
View reviewed changes
Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

@PVince81 PVince81 merged commit 02334ce into stable24 Oct 25, 2022
@PVince81 PVince81 deleted the backport/33945/stable24 branch October 25, 2022 12:49
@skjnldsv skjnldsv mentioned this pull request Oct 27, 2022
Merged
2 tasks
@solracsf solracsf mentioned this pull request Nov 8, 2022
Closed
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PVince81 PVince81 PVince81 approved these changes

@szaimen szaimen szaimen approved these changes

@blizzz blizzz Awaiting requested review from blizzz

@CarlSchwan CarlSchwan Awaiting requested review from CarlSchwan

@come-nc come-nc Awaiting requested review from come-nc

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

Assignees
No one assigned
Labels
feature: ldap
Projects
None yet
Milestone
Nextcloud 24.0.7
Development

Successfully merging this pull request may close these issues.
4 participants
@come-nc @PVince81 @szaimen @blizzz