User is not found when is searched by name and one of surnames #7510

[fgsl]

Fixes #7510

[MorrisJobke]

Followup to #8178

[blizzz]

wrt #8178 (comment), $this->connection->ldapMedialSearch would not need to be needed, but it would check the presence of an asterisk at the beginning of the word. So, all that needs to be done is breaking up the term into words.

The queston is whether you need to have an asterisk before each word (less user friendly, more exact) or just once in the beginning of it (easier, less flexbile, potential for frustration, but essentially the same as if the setting would be set → I'd favor this).

[fgsl]

wrt #8178 (comment), $this->connection->ldapMedialSearch would not need to be needed, but it would check the presence of an asterisk at the beginning of the word. So, all that needs to be done is breaking up the term into words.

The queston is whether you need to have an asterisk before each word (less user friendly, more exact) or just once in the beginning of it (easier, less flexbile, potential for frustration, but essentially the same as if the setting would be set → I'd favor this).

@blizzz, let me understand, do you propose something like this?

// remove $allowMedialSearch = $this->connection->ldapMedialSearch;
if ($term === '') {
    $result = '*';
} else if ($allowEnum !== 'no') {
    if ($term[0] == '*'){ // checks for asterisk at beginning
        $term = $substr($term,1);
        $term = explode(' ',$term); // break up the term in words based on space
        array_walk($term, function(&$value, $key) {
            $value = '*' . $value; // prepend each word with asterisk
        });
        $result = implode(' ',$term);
    } else {
        $result = $term . '*';
    }    
}

[blizzz]

@blizzz, let me understand, do you propose something like this?

yes… well sort of. breaking up words is dependend on whether spaces are provided, and only then asterisk are prepended or not.

[fgsl]

yes… well sort of. breaking up words is dependend on whether spaces are provided, and only then asterisk are prepended or not.

Right, @blizzz. Would the change below be more appropriate then?

if ($term[0] == '*' && strpos($term, ' ') !== false ){ // checks for asterisk at beginning and existence of spaces

[blizzz]

The method will get single words, only, see getAdvancedFilterPartForSearch which breaks up the words.

[fgsl]

The method will get single words, only, see getAdvancedFilterPartForSearch which breaks up the words.

Yes, @blizzz, excuse me, I was, as we say here, "traveling on mayonnaise". Last snippets are unuseful because only repeat what previous method does. Variable $term already is a single word because getAdvancedFilterPartForSearch has exploded searched term.

Then, I think that following change maybe is that you should favor for prepareSearchTerm:

// remove $allowMedialSearch = $this->connection->ldapMedialSearch;
if ($term === '') {
    $result = '*';
} else if ($allowEnum !== 'no') {
    if ($term[0] !== '*'){ // checks for absence of asterisk at beginning
        $result = '*' . $term . '*';  // round term with asterisks
    } else {
        $result = $term . '*'; // only appends asterisk
    }    
}

[blizzz]

In that case you will end up with double asterisk in front. When passing any word to prepareSearchTerm you need to ensure that they will have an asterisk in the beginning, when the very first word begins with one.

[fgsl]

In that case you will end up with double asterisk in front. When passing any word to prepareSearchTerm you need to ensure that they will have an asterisk in the beginning, when the very first word begins with one.

Hey, @blizzz, then it is a bug in current code, because if $term is equal to asterisk, $result will be a double asterisk:

$result = $term . '*';

I want to say that this code needs to be changed anyway. It seems to me that is enough to check the length of string, so:

Replace:

} else {

for:

} elseif (strlen($term)>1){

Can it be so?

[blizzz]

No, there can be only an asterisk at the beginning of the whole search term, any other occurrence is escaped.

[fgsl]

Right, if I understand now - otherwise I will generate a test case because it's getting confused - we can change if.. else.. structure to two if statements so:

if ($term === '' xor ($allowEnum !== 'no')) {
    $result = $term . '*'; // only appends asterisk
}
if ($term !== '*' && $term[0] !== '*'){ // checks for absence of asterisk at beginning
        $result = '*' . $term;  // prepend  asterisks
}

if $term is equal to '', $result will be ''.
if $term is different to '' and $allowEnum is different of 'no', $result is $term concatenated to ''. If current code is correct, $term is not equal to '*', so this condition won't produce a double asterisk. Otherwise, current code already produces double asterisk - and I understand that it doesn't happen.
For any $result, an asterisk is prepended before return it.

I hope I didn't drink this time.

[blizzz]

Test cases would be great :D again, prepending should only happen, when the whole phrase was started with an asterisk.

[codecov]

Codecov Report

Merging #8760 into master will increase coverage by 0.18%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master    #8760      +/-   ##
============================================
+ Coverage     51.68%   51.87%   +0.18%     
+ Complexity    25720    25379     -341     
============================================
  Files          1641     1608      -33     
  Lines         96441    95180    -1261     
  Branches       1393     1377      -16     
============================================
- Hits          49850    49370     -480     
+ Misses        46591    45810     -781

Impacted Files	Coverage Δ	Complexity Δ
apps/user_ldap/lib/Configuration.php	43.68% <ø> (ø)	87 <0> (ø)	⬇️
apps/user_ldap/lib/Connection.php	54.74% <ø> (-0.22%)	122 <0> (-2)
apps/user_ldap/lib/Access.php	36.21% <0%> (-0.07%)	325 <0> (-1)
lib/private/Preview/GeneratorHelper.php	0% <0%> (-80%)	5% <0%> (ø)
lib/private/PreviewManager.php	3.79% <0%> (-50%)	49% <0%> (ø)
settings/Controller/LogSettingsController.php	0% <0%> (-30%)	1% <0%> (-2%)
...aring/lib/Controller/MountPublicLinkController.php	21.52% <0%> (-20.37%)	24% <0%> (+10%)
lib/public/Util.php	44.28% <0%> (-15.91%)	59% <0%> (+14%)
core/js/sharedialogview.js	75.43% <0%> (-12.11%)	0% <0%> (ø)
apps/admin_audit/lib/AppInfo/Application.php	8.52% <0%> (-11.48%)	20% <0%> (-2%)
... and 300 more

[fgsl]

If "prepending should only happen, when the whole phrase was started with an asterisk", I think that proposal change can be discarded and replaced by that:

	private function getAdvancedFilterPartForSearch($search, $searchAttributes) {
		if(!is_array($searchAttributes) || count($searchAttributes) < 2) {
			throw new \Exception('searchAttributes must be an array with at least two string');
		}
		$searchWords = explode(' ', trim($search));
		$wordFilters = array();
                $prepend = ($searchWords[0][0] == '*' ? '*' : ''); // checks for asterisk at beginning
		foreach($searchWords as $word) {
			$word = $this->prepareSearchTerm($word);
                        $word = (($word[0] == '*' ? '' : $prepend) . $word); // prepend asterisk if phrase starts with asterisk
			//every word needs to appear at least once
			$wordMatchOneAttrFilters = array();
			foreach($searchAttributes as $attr) {
				$wordMatchOneAttrFilters[] = $attr . '=' . $word;
			}
			$wordFilters[] = $this->combineFilterWithOr($wordMatchOneAttrFilters);
		}
		return $this->combineFilterWithAnd($wordFilters);
	}

[fgsl]

Let me show about need of asterisk prepended to term with some tests.

I want to find Roger. Complete name is "Roger Luis Padilha de Souza". See the results according use of asterisk prepended and/or appended:

~# ldapsearch -x -LLL -h [host] -D [bind dn] -w [password] -b dc=gov,dc=br cn="Roger*" > test1.txt
~# ldapsearch -x -LLL -h [host] -D [bind dn] -w [password] -b dc=gov,dc=br cn="Roger Luis*" > test2.txt
~# ldapsearch -x -LLL -h [host] -D [bind dn] -w [password] -b dc=gov,dc=br cn="Roger Luis Padilha*" > test3.txt
~# ldapsearch -x -LLL -h [host] -D [bind dn] -w [password] -b dc=gov,dc=br cn="Roger Padilha*" > test4.txt
~# ldapsearch -x -LLL -h [host] -D [bind dn] -w [password] -b dc=gov,dc=br cn="Roger* Souza" > test5.txt
~# ldapsearch -x -LLL -h [host] -D [bind dn] -w [password] -b dc=gov,dc=br cn="Roger *Padilha*" > test6.txt
~# ldapsearch -x -LLL -h [host] -D [bind dn] -w [password] -b dc=gov,dc=br cn="Roger *Souza*" > test7.txt
~# ldapsearch -x -LLL -h [host] -D [bind dn] -w [password] -b dc=gov,dc=br cn="Roger *Souza" > test8.txt
~# ldapsearch -x -LLL -h [host] -D [bind dn] -w [password] -b dc=gov,dc=br cn="Roger* Padilha" > test9.txt
~# ldapsearch -x -LLL -h [host] -D [bind dn] -w [password] -b dc=gov,dc=br cn="Roger* Padilha*" > test10.txt

• test1.txt: 229 entries
• test2.txt: 1 entry
• test3.txt: 1 entry
• test4.txt: no entries
• test5.txt: 8 entries
• test6.txt: 1 entry
• test7.txt: 4 entries
• test8.txt: 4 entries
• test9.txt: no entries
• test10.txt: 1 entry

Search only for Roger in this case is not appropriate. If I know him as "Roger Luis" (test2.txt) and better as "Roger Luis Padilha" (test3.txt), the current implementation ($term . '*') is enough. But if I know him as "Roger Padilha"? Current implementation does not show results (test4.txt). But with an asterisk prepended (test6.txt), I got him. In theory, current implementation should find Roger from "Roger Padilha" in search box, according test10.txt, but it does not happen and I don't know why.

Below there are some examples of names that can be found in our LDAP directory. Inside context presented before, imagine to find "Maristela Midlej Silva de Araujo Veloso" knowing her only "Maristela Veloso":

• Francisco Fernando Magalhaes Paes de Barros Filho
• Antonio Carlos Morbeck de Souza Junior
• Diego Manoel de Santana Oliveira Santos
• Evelin Dayana Alves de Santana Oliveira
• Anna Carla Freire Luna Campelo Bastos
• Maristela Midlej Silva de Araujo Veloso
• Joao Paulo Caminha de Souza Ribeiro
• Ivaldina Maria Queiros Pereira Alves de Souza
• Maria Villani Gusmao Ferraz de Andrade
• Mariana Pontes de Miranda Sarmento Cordeiro
• Maria Beatriz Mello Leitao Moreira de Carvalho

[blizzz]

@fgsl yes, the approach in #8760 (comment) looks good. Just, when doing comparions, always do it strictly: ===

For the tests I had unit tests in mind (apps/user_ldap/tests/AccessTest.php). If feasible?

[MorrisJobke]

Nothing for 14 it seems -> moved to 15