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

Autocomplete for LDAP Groups containing a dash ("-") faulty - most likely related to how LDAP backend searches/filters groups #15224

Closed
fwolfst opened this issue Apr 25, 2019 · 14 comments
Labels
0. Needs triage Pending check for reproducibility or if it fits our roadmap bug feature: ldap needs info

Comments

@fwolfst
Copy link

fwolfst commented Apr 25, 2019

Steps to reproduce

  • Using an OpenLDAP with posixGroup and memberUid, create a group "my-test-ldap-group".
  • Also create a "local" (database-driven) group "my-test-local-group".
  • Configure LDAP and nextcloud correctly
  • Open Share-Dialog for some file or folder, type "test" in the "with-whom-to-share"-input field

I guess the issue has nothing to do with sharing or autocomplete but with the ldap group backend and specifically how it is talked to when searching for (sub-)strings and how it constructs the filter. I am very willing to dive into that, if somebody gives me a pointer where the entry point in the code lies (I looked around in server/apps/user_ldap/lib/Group_LDAP.php etc. but am unsure where the string entered into the browsers text field will be consumed first).

Expected behaviour

The autocomplete should propose both (local and ldap) group that contain a dash in its group name.

Actual behaviour

The autocomplete stuff only shows the "local" group.
Autocompletion does work however for the letters in front of the dash ("-"), so typing "my" in the share dialog will correctly show both groups.

Server configuration detail

Operating system: Linux 4.4.0-141-generic #167-Ubuntu SMP Wed Dec 5 10:40:15 UTC 2018 x86_64

Webserver: Apache/2.4.18 (Ubuntu) (fpm-fcgi)

Database: mysql 10.0.38

PHP version:

7.2.17-1+ubuntu16.04.1+deb.sury.org+3
Modules loaded: Core, date, libxml, openssl, pcre, zlib, filter, hash, Reflection, SPL, sodium, session, standard, cgi-fcgi, mysqlnd, PDO, xml, apcu, calendar, ctype, curl, dom, mbstring, fileinfo, ftp, gd, gettext, iconv, igbinary, imagick, intl, json, ldap, exif, mysqli, pdo_mysql, Phar, posix, readline, redis, shmop, SimpleXML, smbclient, sockets, sysvmsg, sysvsem, sysvshm, tokenizer, wddx, xmlreader, xmlwriter, xsl, zip, libsmbclient, Zend OPcache

Nextcloud version: 15.0.7 - 15.0.7.0

Updated from an older Nextcloud/ownCloud or fresh install: update

Where did you install Nextcloud from: unknown

Signing status
List of activated apps
Enabled:
 - accessibility: 1.1.0
 - activity: 2.8.2
 - admin_audit: 1.5.0
 - announcementcenter: 3.4.1
 - audioplayer: 2.7.0
 - calendar: 1.6.5
 - cloud_federation_api: 0.1.0
 - comments: 1.5.0
 - contacts: 3.1.1
 - dav: 1.8.1
 - drawio: 0.9.2
 - external: 3.2.0
 - federatedfilesharing: 1.5.0
 - federation: 1.5.0
 - files: 1.10.0
 - files_accesscontrol: 1.5.0
 - files_automatedtagging: 1.5.0
 - files_downloadactivity: 1.4.0
 - files_external: 1.6.0
 - files_markdown: 2.0.6
 - files_pdfviewer: 1.4.0
 - files_readmemd: 1.0.2
 - files_retention: 1.4.2
 - files_rightclick: 0.13.0
 - files_sharing: 1.7.0
 - files_texteditor: 2.7.0
 - files_trackdownloads: 1.4.0
 - files_trashbin: 1.5.0
 - files_versions: 1.8.0
 - files_videoplayer: 1.4.0
 - gallery: 18.2.0
 - group_everyone: 0.1.2
 - groupfolders: 3.0.0
 - impersonate: 1.2.0
 - issuetemplate: 0.5.0
 - logreader: 2.0.0
 - lookup_server_connector: 1.3.0
 - nextcloud_announcements: 1.4.0
 - notifications: 2.3.0
 - oauth2: 1.3.0
 - password_policy: 1.5.0
 - polls: 0.10.2
 - provisioning_api: 1.5.0
 - richdocuments: 3.2.4
 - serverinfo: 1.5.0
 - sharebymail: 1.5.0
 - spreed: 5.0.3
 - systemtags: 1.5.0
 - tasks: 0.9.8
 - theming: 1.6.0
 - twofactor_backupcodes: 1.4.1
 - updatenotification: 1.5.0
 - user_ldap: 1.5.0
 - workflowengine: 1.5.0
Disabled:
 - bookmarks
 - bruteforcesettings
 - circles
 - encryption
 - firstrunwizard
 - gpxedit
 - mindmaps
 - music
 - rainloop
 - support
 - survey_client
 - user_usage_report

Configuration (config/config.php)
{
    "instanceid": "***REMOVED SENSITIVE VALUE***",
    "passwordsalt": "***REMOVED SENSITIVE VALUE***",
    "secret": "***REMOVED SENSITIVE VALUE***",
    "trusted_domains":"***REMOVED SENSITIVE VALUE***",
    "datadirectory": "***REMOVED SENSITIVE VALUE***",
    "overwrite.cli.url": "***REMOVED SENSITIVE VALUE***",
    "dbtype": "mysql",
    "version": "15.0.7.0",
    "dbname": "***REMOVED SENSITIVE VALUE***",
    "dbhost": "***REMOVED SENSITIVE VALUE***",
    "dbport": "",
    "dbtableprefix": "oc_",
    "dbuser": "***REMOVED SENSITIVE VALUE***",
    "dbpassword": "***REMOVED SENSITIVE VALUE***",
    "installed": true,
    "maintenance": false,
    "ldapIgnoreNamingRules": false,
    "ldapProviderFactory": "\\OCA\\User_LDAP\\LDAPProviderFactory",
    "mail_smtpmode": "smtp",
    "mail_smtpauthtype": "PLAIN",
    "mail_from_address": "***REMOVED SENSITIVE VALUE***",
    "mail_domain": "***REMOVED SENSITIVE VALUE***",
    "mail_smtpauth": 1,
    "mail_smtphost": "***REMOVED SENSITIVE VALUE***",
    "mail_smtpport": "25",
    "mail_smtpname": "***REMOVED SENSITIVE VALUE***",
    "mail_smtppassword": "***REMOVED SENSITIVE VALUE***",
    "theme": "",
    "loglevel": 2,
    "default_language": "de",
    "filelocking.enabled": "true",
    "memcache.local": "\\OC\\Memcache\\APCu",
    "memcache.locking": "\\OC\\Memcache\\Redis",
    "redis": {
        "host": "***REMOVED SENSITIVE VALUE***",
        "port": 0
    },
    "updater.release.channel": "stable"
}

Are you using external storage, if yes which one: local/smb/sftp/...

Are you using encryption:

Are you using an external user-backend, if yes which one: LDAP (OpenLDAP)

LDAP configuration (delete this par if not used)
_lastChange: 1556174703background_sync_interval: 1800background_sync_offset: 0background_sync_prefix: cleanUpJobOffset: 50enabled: yeshas_memberof_filter_support: 1installed_version: 1.5.0ldap_agent_password: QUwtU3N0YmFnaS4=ldap_base: dc=REMOVED,dc=deldap_base_groups: ou=Groups,dc=REMOVED,dc=deldap_base_users: ou=People,dc=REMOVED,dc=deldap_cache_ttl: 600ldap_configuration_active: 1ldap_display_name: cnldap_dn: cn=admin,dc=REMOVED,dc=deldap_email_attr: mailldap_experienced_admin: 1ldap_group_filter: (&(|(objectclass=posixGroup)))ldap_group_member_assoc_attribute: memberUidldap_groupfilter_groups: ldap_groupfilter_objectclass: posixGroupldap_host: REMOVEDldap_login_filter: (&(&(|(objectclass=posixAccount)))(|(uid=%uid)))ldap_login_filter_mode: 1ldap_loginfilter_attributes: cnldap_port: 389ldap_tls: 0ldap_user_filter_mode: 0ldap_userfilter_groups: ldap_userfilter_objectclass: posixAccountldap_userlist_filter: (&(|(objectclass=posixAccount)))types: authentication

Client configuration

Browser: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:66.0) Gecko/20100101 Firefox/66.0

Operating system: Ubuntu

@fwolfst fwolfst added 0. Needs triage Pending check for reproducibility or if it fits our roadmap bug labels Apr 25, 2019
@fwolfst
Copy link
Author

fwolfst commented Apr 25, 2019

But maybe I am on the wrong track with looking for the ldap query filter, as all group names should be cached, isnt it? Like the autocomplete function of a share-dialog should only hit the cached group names and not fire a ldap query for every letter typed? If so, why is there a difference in behaviour between LDAP and "local" groups?

@DagNygren
Copy link

DagNygren commented May 4, 2019

I see the same thing here with the same LDAP config. Something goes has gone wrong in an update during this spring and the wizard doesn't work. The LDAP part doesn't seem to be very much tested outside the AD domain... Anyway: All the logins using LDAP hangs (and creates zombies, which eventually kills nextcloud with "too many connections to the database") and the wizard is not able to list any groups but just stay there spinning the cursor. This worked fine earlier and there has been no changes in the LDAP setup. All the other programs using the LDAP server work fine. No dashes in my group names, though.
My setup:

  • Fedora fc27 server
  • openldap 2.4.45
  • php_ldap 7.1.23
  • Apache 2.4.34,
    At the moment I just had to disable LDAP and create local users.

@fwolfst
Copy link
Author

fwolfst commented May 5, 2019

@DagNygren cool that you want to contribubte and sad to hear that you have some issues. However, I believe your issues are unrelated to this one (you don't have dashes in group names at all). So, you could open a new ticket and maybe even remove your comment here, to reduce message clutter. Chances that someone gets interest in your setup will get much higher when it has the right heading (at the moment, I'd like to talk about autocompletion and dashes in ldap group names here). If you open a new issue, maybe I can help (a tiny bit) looking into whats wrong.

@blizzz
Copy link
Member

blizzz commented Jun 21, 2019

LDAP searches go live against the server (unless cached), so we can take advantage of the LDAP servers search capabilities.

Starting point to look for would be apps/user_ldap/lig/Group_LDAP.php, method search or searchGroups.

@fwolfst
Copy link
Author

fwolfst commented Jun 23, 2019

The general fix to allow "fulltext" search with LDAP (also for username and other entities) would be to change prepareSearchTerm, where the documentation reads:

 * returns the search term depending on whether we are allowed
 * list users found by ldap with the current input appended by
 * a *

(the last "*" is a literal "star" ⭐).

The relevant code then is:

$result = $term . '*';

Imho this has to be changed to $result = '*' . $term . '*'; . I applied that fix on my production instance and did not yet notice any issues. Full text search in LDAP Group- and Usernames now works as expected (by me).

@blizzz I happily craft a PR, any chance it would get merged? Any background on why the LDAP filter is created only with a forward glob?

@blizzz
Copy link
Member

blizzz commented Jul 30, 2019

Imho this has to be changed to $result = '' . $term . '';

Unconditional wildcards may cause really bad performance on slightly bigger setups as indexing is being circumvented. Actually, when you start your query with an asterisk (*), then it would run the query against LDAP with it. It's whitelisted in that case.

@fwolfst
Copy link
Author

fwolfst commented Jul 30, 2019

Actually, when you start your query with an asterisk (*), then it would run the query against LDAP with it. It's whitelisted in that case.

Can you elaborate on that? I am sorry but do not understand. As said, in my case the preprended asterisk seems to lead to the correct results.

I do understand that an asterisk will circumvent the indexing, and I guess "unconditional wildcards" means wildcards without context, or anchor - or do you mean that the query should be guarded, by checking whether term actually contains some characters? Or do you mean that if the first character is an asterisk the query is "whitelisted" as in: will not hit cache?

What would you propose to fix that behaviour? A setting that allows LDAP search to deliver "correct" results with the price of a potential performance hit? Add a info on the search field that some LDAP search is limited?

@blizzz
Copy link
Member

blizzz commented Jul 31, 2019

@fwolfst i meant that there should not be a wildcard prepended by default. If you manually trigger a search with an asterisk up front, it will work, but given this is not really something for end users. A config flag to – if set – to insert the wildcard with every search would be easiest, yet another strange code path to consider. Then, i do not have better idea. Maybe to consider is, with this flag, to do a normal search first, and if this does not yield any result repeat the search with the wildcard. The problem with this is, if it yields an undesired match, nothing is won. And it would make it more complex.

@fwolfst
Copy link
Author

fwolfst commented Aug 11, 2019

Mmmh, I dont think I will find the time to look into how nextcloud handles configuration and provide a clean patch.

@blizzz : Do I understand correctly that you maintain this plugin?
Would you merge a patch that applies the asterisk-search only on "manual" searches (where a user enters something in the text-box), as opposed to every internal search through LDAP? Do you think there would be an easy implementation for such behaviour (I did not yet understand the frontend code paths)?

Another option could be to fork the plugin ("SearcheableLDAP"), but I guess for now I will have to live with manual patches :( . Unfortunately, before finding the bug, I decided on a convention where certain teams have Groups names ending on -team.
The performance penalty is absolutely no issue for me (~120 Users, a big dozen of groups maybe), and correct search behavior is super important, otherwise people do not manage to share their stuff correctly.

@blizzz
Copy link
Member

blizzz commented Aug 12, 2019

@blizzz : Do I understand correctly that you maintain this plugin?
Would you merge a patch that applies the asterisk-search only on "manual" searches (where a user enters something in the text-box), as opposed to every internal search through LDAP? Do you think there would be an easy implementation for such behaviour (I did not yet understand the frontend code paths)?

I am having my doubts that you can identify those spots. As, anything goes via the User backend API methods, getUsers mostly here. So you cannot really distinguish between internal and external uses. Further, I still would not want this to be the default behaviour.

@fwolfst
Copy link
Author

fwolfst commented Nov 14, 2019

Just sharing for people who might run into the same issue with their users (some ldap groups do not show up, although it seems that autocompletion does work in some cases):

The relevant change can be made with

sed -i "s/\$result = \$term . '\*';/\$result = '*' . \$term . '\*';/" apps/user_ldap/lib/Access.php

as of today, NC 16.0.6 .

@szaimen
Copy link
Contributor

szaimen commented May 27, 2021

Is this Issue still valid? If not, please close this issue. Thanks! :)

@fwolfst
Copy link
Author

fwolfst commented May 27, 2021

Is this Issue still valid? If not, please close this issue. Thanks! :)

If you want the user search (e.g. in sharing dialog) to find usernames in ldap that do not begin with whatever you entered, but just contain it (e.g. 'net' finding 'Anette' and 'janet'), you still have to patch after each update.
Imho not closed.

An option to enable the proper search would be an solution, but I have no time to dig into plugin options.

@szaimen szaimen added 1. to develop Accepted and waiting to be taken care of and removed 0. Needs triage Pending check for reproducibility or if it fits our roadmap needs info labels May 27, 2021
@szaimen
Copy link
Contributor

szaimen commented Jan 9, 2023

Hi, please update to 24.0.8 or better 25.0.2 and report back if it fixes the issue. Thank you!

@szaimen szaimen added needs info 0. Needs triage Pending check for reproducibility or if it fits our roadmap and removed 1. to develop Accepted and waiting to be taken care of labels Jan 9, 2023
@szaimen szaimen closed this as completed Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0. Needs triage Pending check for reproducibility or if it fits our roadmap bug feature: ldap needs info
Projects
None yet
Development

No branches or pull requests

4 participants