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

Allow LDAP search users with *term* #45555

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+1 −1
Open

Allow LDAP search users with *term* #45555

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion apps/user_ldap/lib/Access.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1564,7 +1564,7 @@ private function prepareSearchTerm(string $term): string {
if ($term === '') {
$result = '*';
} elseif ($allowEnum !== 'no') {
$result = $term . '*';
$result = '*' . $term . '*';
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the limitation was on purpose. @come-nc and @blizzz?

Copy link
Member

@blizzz blizzz Jun 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It is on purpose, yes. There should be a related issue and PR in this repo, too.

Copy link
Author

Choose a reason for hiding this comment

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

I read some supposed performance limitations on big LDAP organisations. But:

  • all i read on LDAP documentation never mention "don't do term" as it is even described in the main documentation.
  • this might be based on old benchmarks on bad LDAP system
  • why is the "*" allowed that will send "all users" in that case ?
  • the SQL part does the term and nobody asked to remove this for performance reason
  • this is very very confusing for the user experience (i thought this was a bug and i lost hours searching what was wrong with my LDAP until i debug in PHP)

In conclusion, at least, why presuming all ldap are made with millions of users ? Why not let users have the same behaviour between LDAP and NextCloud ? And maybe bring an LDAP option "don't allow term because my ldap is too slow" if someone complain for it ?

}
return $result;
}
Expand Down