Respect User enumeration

[foobarable]

Respect shareapi_allow_share_dialog_user_enumeration in user_ldap filter
generation function to increase seach performance in sharing dialog.

cc @blizzz @MorrisJobke

[scrutinizer-notifier]

A new inspection was created.

[foobarable]

Should also be respected in getAdvancedFilterPartForSearch()

[ghost]

Thanks a lot for your contribution!
Contributions to the core repo require a signed contributors agreement http://owncloud.org/about/contributor-agreement/

Alternatively you can add a comment here where you state that this contribution is MIT licensed.

Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

[MorrisJobke]

@blizzz I just discussed with @foobarable the performance of the fix that was introduced with #18353

The question here is if this has effects to the user management search. Any idea how to handle this?

[MorrisJobke]

@owncloud-bot this is okay to test

[blizzz]

The question here is if this has effects to the user management search. Any idea how to handle this?

I discussed this with @foobarable: Yes, it has impact, when you search for a user on the User page. However, an admin usually knows the user id, so it should not be a problem. Listings without search term work normally.

[blizzz]

Should also be respected in getAdvancedFilterPartForSearch()

The direct approach is:

diff --git a/apps/user_ldap/lib/access.php b/apps/user_ldap/lib/access.php
index a2eb834..7f928e0 100644
--- a/apps/user_ldap/lib/access.php
+++ b/apps/user_ldap/lib/access.php
@@ -1121,8 +1121,12 @@ class Access extends LDAPUtility implements user\IUserTools {
        }
        $searchWords = explode(' ', trim($search));
        $wordFilters = array();
+       $config = \oc::$server->getConfig();
+       $allowEnum = $config->getAppValue('core', 'shareapi_allow_share_dialog_user_enumeration', false);
        foreach($searchWords as $word) {
-           $word .= '*';
+           if($allowEnum === true) {
+               $word .= '*';
+           }
            //every word needs to appear at least once
            $wordMatchOneAttrFilters = array();
            foreach($searchAttributes as $attr) {

It adds a bit of duplication (getting the config instance, fetching the value, modifying the search termn), this could be summed up in a method of it's own e.g. (prepareSearchTerm($term)) that returns the final search term as done by

$search = empty($search) ? '*' : 
     $allowEnum ? $search . '*' : $search;

The advanced method just calls it for every $word.

[MorrisJobke]

@foobarable Can we get the second change that was proposed by @blizzz in?

[DeepDiver1975]

we are beyond feature freeze -> 9.0

[DeepDiver1975]

@blizzz anything missing? THX

[MorrisJobke]

@foobarable Still open ;) Did you recently found time to fix the remaining stuff?

[jancborchardt]

@owncloud/ldap can you test this? You can also use this shorthand mention to get your pull requests reviewed. :)

[foobarable]

@blizzz @MorrisJobke
So you mean like this? I have not testet this yet but can do if you think this is the correct way.
I'll also comment the addition function then.

diff --git a/apps/user_ldap/lib/access.php b/apps/user_ldap/lib/access.php
index 667f107..b3f71a7 100644
--- a/apps/user_ldap/lib/access.php
+++ b/apps/user_ldap/lib/access.php
@@ -1193,7 +1193,7 @@ class Access extends LDAPUtility implements user\IUserTools {
                $searchWords = explode(' ', trim($search));
                $wordFilters = array();
                foreach($searchWords as $word) {
-                       $word .= '*';
+                       $word = $this->prepareSearchTerm($word);
                        //every word needs to appear at least once
                        $wordMatchOneAttrFilters = array();
                        foreach($searchAttributes as $attr) {
@@ -1226,7 +1226,8 @@ class Access extends LDAPUtility implements user\IUserTools {
                                );
                        }
                }
-               $search = empty($search) ? '*' : $search.'*';
+
+               $search = $this->prepareSearchTerm($search);
                if(!is_array($searchAttributes) || count($searchAttributes) === 0) {
                        if(empty($fallbackAttribute)) {
                                return '';
@@ -1243,6 +1244,16 @@ class Access extends LDAPUtility implements user\IUserTools {
                return $this->combineFilterWithOr($filter);
        }

+       private function prepareSearchTerm($term) {
+               $config = \oc::$server->getConfig();
+
+               $allowEnum = $config->getAppValue('core', 'shareapi_allow_share_dialog_user_enumeration', false);
+
+               $term = empty($term) ? '*' :
+                       $allowEnum ? $term . '*' : $term;
+               return $term;
+       }
+
        /**
         * returns the filter used for counting users
         * @return string

[MorrisJobke]

So you mean like this? I

Looks good. Can you please apply the changes? (you don't need to merge in the master branch via github ;))

[blizzz]

I second @MorrisJobke

[foobarable]

The patch as i wrote it does not work yet in Version 8.2.1.4 Still trying to find out what's wrong.

[foobarable]

Created new pull request here: #21324