Fixes #7175 - Allow to search for email address in user management #7419

Abijeet · 2017-12-07T17:12:21Z

Greetings,

@MorrisJobke - Please find the merge request that fixes #7175

I've done the following,

Updated the query to fetch the users in users > everyone tab.
Updated the query to fetch the users in users > admin tab.
Tested to ensure that the disabled users are also being fetched.

Tested with SQLite.

Signed-off-by: Abijeet abijeetpatro@gmail.com

codecov · 2017-12-07T18:05:42Z

Codecov Report

Merging #7419 into master will decrease coverage by 18.1%.
The diff coverage is 91.17%.

@@              Coverage Diff              @@
##             master    #7419       +/-   ##
=============================================
- Coverage     52.87%   34.77%   -18.11%     
- Complexity    23658    24885     +1227     
=============================================
  Files          1449     1602      +153     
  Lines         80454    94750    +14296     
  Branches          0     1368     +1368     
=============================================
- Hits          42541    32949     -9592     
- Misses        37913    61801    +23888

Impacted Files	Coverage Δ	Complexity Δ
lib/private/DB/Connection.php	`65.41% <ø> (-3.01%)`	`45 <0> (ø)`
...lder/ExpressionBuilder/SqliteExpressionBuilder.php	`100% <100%> (ø)`	`2 <1> (+1)`	⬆️
...B/QueryBuilder/FunctionBuilder/FunctionBuilder.php	`71.42% <100%> (+9.89%)`	`7 <1> (+1)`	⬆️
...eryBuilder/ExpressionBuilder/ExpressionBuilder.php	`90.24% <50%> (-2.53%)`	`23 <0> (ø)`
lib/private/User/Database.php	`75.59% <92.85%> (+6.59%)`	`42 <1> (-3)`	⬇️
lib/private/App/AppStore/Bundles/Bundle.php	`0% <0%> (-100%)`	`2% <0%> (ø)`
apps/dav/lib/CalDAV/Activity/Setting/Todo.php	`0% <0%> (-100%)`	`8% <0%> (ø)`
apps/user_ldap/lib/Settings/Section.php	`0% <0%> (-100%)`	`5% <0%> (ø)`
apps/federation/lib/Hooks.php	`0% <0%> (-100%)`	`4% <0%> (ø)`
lib/public/AppFramework/Http/StreamResponse.php	`0% <0%> (-100%)`	`6% <0%> (ø)`
... and 811 more

Abijeet · 2017-12-08T18:48:18Z

Not sure why the test case failed, please let me know if there is anything I've to do or change.

MorrisJobke · 2017-12-08T22:52:47Z

Not sure why the test case failed, please let me know if there is anything I've to do or change.

Fixed in master: #7433

blizzz · 2017-12-12T12:04:58Z

It works, however I'D appreciate "unit" tests in tests/lib/User/DatabaseTest.php to ensure this keeps working in the future. Basically testSearch() from the inherited "Backend" class can be extended to look for the email.

Abijeet · 2017-12-16T05:29:00Z

@blizzz - Thank you. I'll add the test cases some time today.

Fixes #7175. - Updated the query to fetch the users in users > everyone tab. - Updated the query to fetch the users in users > admin tab. - Tested to ensure that the disabled users are also being fetched. - Added test cases. Signed-off-by: Abijeet <abijeetpatro@gmail.com>

Abijeet · 2017-12-16T11:51:29Z

@blizzz , @MorrisJobke -

Couple of things,

I'm not sure if I've implemented the test cases properly. Please let me know if I've to change anything here.
There was an extra space in the .htaccess that Netbeans trimmed off, let me know if I should undo that.

Abijeet · 2017-12-16T11:55:31Z

Also I noticed one more thing, searching with _ in LIKE isn't working.

So for example, if there is a user with uid as admin_user.

If you search for this user in the view with the following search term - admin_, you'll not get any value in the response. This is because escapeLikeParameter function adds a \ before the _ (Ref: https://github.com/nextcloud/server/blob/master/lib/private/DB/Connection.php#L410). Tested on SQLite.

Not sure if this is a known issue.

MorrisJobke · 2017-12-18T12:49:25Z

.htaccess

    Header set Cache-Control "max-age=15778463"
  </FilesMatch>
-  
+


This is fine.

MorrisJobke · 2017-12-18T12:49:43Z

.htaccess

 <IfModule pagespeed_module>
  ModPagespeed Off
-</IfModule>
+</IfModule>


That change should be undone. We usually have a new line at the end of files.

Fixed, added the newline back. Will keep in mind for future.

Signed-off-by: Abijeet <abijeetpatro@gmail.com>

blizzz · 2017-12-19T16:12:29Z

This is because escapeLikeParameter function adds a \ before the _ (Ref: https://github.com/nextcloud/server/blob/master/lib/private/DB/Connection.php#L410). Tested on SQLite.

This was added for security reasons, since % and _ act as wildcards in SQL. The \ is necessary to escape them so user input does not act like a wildcard. For sqlite to work it requires an additional ESCAPE '\' however. This is not the case?

add additional user searching tests Signed-off-by: Robin Appelman <robin@icewind.nl>

icewind1991 · 2017-12-20T14:54:57Z

Took the opportunity to refactor the user searching code and fixed the _ issue

@blizzz please re-review

blizzz · 2018-01-12T11:33:23Z

lib/private/User/Database.php

+			->from('users', 'u')
+			->leftJoin('u', 'preferences', 'p', $query->expr()->andX(
+				$query->expr()->eq('userid', 'uid')),
+				$query->expr()->eq('appid', new Literal('settings')),


Literal cannot be hidden from consumers?

All plain strings are used as column names

I mean whether everywhere where we use strings in queries they need to be instances of Literal or whether it can be abstracted away?

We need to use Literal to explicitly tell the query builder that the string is safe to use

blizzz · 2018-01-12T11:37:58Z

@icewind1991 just the other question, looks good otherwise and is working

Signed-off-by: Robin Appelman <robin@icewind.nl>

blizzz · 2018-01-17T13:36:23Z

needs another reviewer

MorrisJobke

Tested and works 👍

MorrisJobke · 2018-03-06T20:56:25Z

lib/public/DB/QueryBuilder/IFunctionBuilder.php

+	 *
+	 * @param mixed $field
+	 * @return IQueryFunction
+	 * @since 13.0.0


Abijeet · 2018-03-10T06:55:15Z

Finally :)

MorrisJobke requested review from blizzz, rullzer and schiessle December 8, 2017 09:57

MorrisJobke added the 3. to review Waiting for reviews label Dec 8, 2017

MorrisJobke added this to the Nextcloud 14 milestone Dec 8, 2017

MorrisJobke added enhancement feature: users and groups labels Dec 8, 2017

Abijeet closed this Dec 8, 2017

Abijeet reopened this Dec 8, 2017

MorrisJobke reviewed Dec 18, 2017

View reviewed changes

.htaccess

Header set Cache-Control "max-age=15778463"

</FilesMatch>

Copy link

Member

MorrisJobke Dec 18, 2017

Choose a reason for hiding this comment

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

This is fine.

MorrisJobke reviewed Dec 18, 2017

View reviewed changes

Added newline to end of htaccess file

ac14d02

Signed-off-by: Abijeet <abijeetpatro@gmail.com>

blizzz requested a review from icewind1991 December 19, 2017 16:14

blizzz approved these changes Dec 19, 2017

View reviewed changes

refactor user searching

aad0189

add additional user searching tests Signed-off-by: Robin Appelman <robin@icewind.nl>

blizzz reviewed Jan 12, 2018

View reviewed changes

reuse query builder

42b3aa3

Signed-off-by: Robin Appelman <robin@icewind.nl>

MorrisJobke approved these changes Mar 6, 2018

View reviewed changes

MorrisJobke merged commit de56915 into nextcloud:master Mar 6, 2018

MorrisJobke mentioned this pull request Mar 6, 2018

Fix PHPDoc for public API #8696

Merged

MorrisJobke reviewed Mar 6, 2018

View reviewed changes

lib/public/DB/QueryBuilder/IFunctionBuilder.php

*

* @param mixed $field

* @return IQueryFunction

* @since 13.0.0

Copy link

Member

MorrisJobke Mar 6, 2018

Choose a reason for hiding this comment

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

See #8696

Uh oh!

Fixes #7175 - Allow to search for email address in user management #7419

Fixes #7175 - Allow to search for email address in user management #7419

Uh oh!

Conversation

Abijeet commented Dec 7, 2017

Uh oh!

codecov bot commented Dec 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Abijeet commented Dec 8, 2017

Uh oh!

MorrisJobke commented Dec 8, 2017

Uh oh!

blizzz commented Dec 12, 2017

Uh oh!

Abijeet commented Dec 16, 2017

Uh oh!

Abijeet commented Dec 16, 2017

Uh oh!

Abijeet commented Dec 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blizzz commented Dec 19, 2017

Uh oh!

icewind1991 commented Dec 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blizzz commented Jan 12, 2018

Uh oh!

blizzz commented Jan 17, 2018

Uh oh!

MorrisJobke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Abijeet commented Mar 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Dec 7, 2017 •

edited

Loading