User backend extendable account searching #27906

tomneedham · 2017-05-16T14:47:50Z

Description

Continued from: #27832

Problem

In OC9 we allow LDAP users to specify custom search attributes which can be used for finding users to share
In OC10 we have the accounts table which is used for searching to reduce the hits to LDAP servers
Organisations use these search attributes to help users find other users based on terms they may know (like usernames, emails, staff codes, etc)

Proposed / Implemented Solution

Added a new oc_account_terms table with account_id (foreign key to accounts.id) and term columns
Altered user manager code to find users using these terms
Allow user backends to manipulate these search terms as needed (e.g: LDAP can add these from custom attributes the user configures)

User_LDAP App:

PR in progress to implement these features and supply the search attributes to the core Import search attributes into core account table for faster searching user_ldap#98

Related Issue

owncloud/user_ldap#96

How Has This Been Tested?

Manual testing - needs more 💪
Updated unit tests -> still WIP 🚧

Screenshots (if appropriate):

because they make your PR look more cool:

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

PVince81 · 2017-05-16T15:32:11Z

I wonder if this extra table is really necessary. Hitting the LDAP server when someone types in a share search string might be ok especially as it benefits from LDAP specific searching capabilities (and patterns?). If it doesn't work at all with the current core then maybe the sharing code somehow needs a way for apps to provide their user search functions there. I think previously it was one of the user manager's functions, maybe not suitable any more.

The part we want to prevent is doing a search on LDAP for already existing users we already know, by user id or email, which usually happens when setting up FS for received shares.

But since you guys have worked on this already you might have more insights about the reasons this is needed.

PVince81 · 2017-05-16T15:39:21Z

lib/private/User/AccountTerm.php

+use OCP\AppFramework\Db\Entity;
+
+/**
+ * Class Account


AccountTerm

PVince81

At first look the code looks fine

butonic · 2017-05-16T18:25:11Z

The idea of the account table was to only rely on eg ldap for the login to be able to iterate over all users etc. The code that used to lead to a lot of ldap queries because it would execute pages etc was the search method used when sharing users. But also fetching any other use metadata was triggering ldap queries. Unfortunately, the ldap user bachend allos specifying additional fields that should be taken into account when doing a user search. that might eg be the city or multi value attributes like mailAccounts. That is actually a nice feature. The initial approach does not fit as @tomneedham and I today by chance stumbled over an LDIF entry with 7 email addresses. We were told there were accounts with many more. We came up with the idea of an additional table to hold arbitrary search terms. Another approach would be to add an elasticsearch based framework for this kind of cacheable metadata ... but that is nothing we can add on short notice.

For now I think this is a good approach. We could add a foreign key but did not want to be the first ones to actually use the relevant code. We would have liked to have an ORMapper, but got around it with adding an interface and making the ldap app actively update the search attributes on first login.

DeepDiver1975 · 2017-05-17T06:17:38Z

Hitting the LDAP server when someone types in a share search string might be ok especially as it benefits from LDAP specific searching capabilities (and patterns?).

this is one major complaint I'm hearing from big installations: searching the ldap takes ages. From an UX pov this is not acceptable.

DeepDiver1975 · 2017-05-17T06:21:18Z

lib/private/User/Account.php

+		}
+	}
+
+	public static function fromRow(array $row) {


DeepDiver1975 · 2017-05-17T06:21:29Z

lib/private/User/Account.php

@@ -65,6 +64,9 @@ class Account extends Entity {
 	protected $state;
 	protected $home;

+	protected $terms = [];
+	private $_termsChanged = false;


_ ?? needed ???

_ to indicate it is used like _updatedFields: tracking if the attribute was changed so the mapper can act accordingly

DeepDiver1975 · 2017-05-17T06:25:52Z

lib/private/User/AccountTermMapper.php

+	 * @param $account_id
+	 * @param string[] $terms
+	 */
+	public function setTermsForAccount($account_id, array $terms) {


use Mapper:insert ???

adressed with dd377c9#diff-1e60c46513705f729f8e728292eb6f3bR43

DeepDiver1975 · 2017-05-17T06:26:06Z

lib/private/User/AccountTermMapper.php

+	 * Removes all search terms for a given account id
+	 * @param $account_id
+	 */
+	public function deleteTermsForAccount($account_id) {


use Mapper:delete ???

we want to delete by account_id, not id

DeepDiver1975 · 2017-05-17T06:29:01Z

lib/private/User/Manager.php

@@ -170,7 +176,7 @@ protected function getUserObject(Account $account, $cacheUser = true) {
 			return $this->cachedUsers[$account->getUserId()];
 		}

-		$user = new User($account, $this->accountMapper, $this, $this->config, null, \OC::$server->getEventDispatcher() );
+		$user = new User($account, $this->accountMapper, $this->accountTermMapper, $this, $this->config, null, \OC::$server->getEventDispatcher() );


is the terms mapper necessary inside class User? I'd hide the terms mapper within the account mapper

DeepDiver1975 · 2017-05-17T06:30:03Z

lib/public/IGroupManager.php

+	 * @param string $search
+	 * @param int $limit
+	 * @param int $offset
+	 * @return array an array of display names (value) and user ids (key)


not the User objects?

DeepDiver1975 · 2017-05-17T06:30:54Z

tests/lib/Group/ManagerTest.php

+		/**
+		 * @var \OC\User\Manager $userManager
+		 */
+		$userManager = $this->createMock('\OC\User\Manager');


Manager:class

DeepDiver1975 · 2017-05-17T06:31:05Z

tests/lib/Group/ManagerTest.php

+		 * @var \OC\User\Manager $userManager
+		 */
+		$userManager = $this->createMock('\OC\User\Manager');
+		$userBackend = $this->createMock('\OC_User_Backend');


OC_User_Backend::class

PVince81 · 2017-05-18T08:17:10Z

This doesn't work with guest emails (haven't tried generic emails), steps:

Login as admin
Enable guest app
Create folder "test1"
Type "Vince" in share field for "test1", select "Share to guest"
Type in local email address, "vincent@host.tld"
Log out
Open mail, open link and set password
Login as admin again
Create another folder "test2"
Share "test2" with "vincent@host.tld"

Expected: autocomplete finds the guest user "Vince"
Actual: autocomplete doesn't find the user and only offers a "remote" user (due to the format).

This worked on the previous PR.

@tomneedham did you properly include searching by email ? I can see that the oc_accounts column "email" for that guest user contains the correct email address. (and it's unique)

Because searching by email do not require terms to exist.

PVince81 · 2017-05-18T08:24:48Z

Fixed by replacing "inner join" with "left join": 06d7c36

tomneedham · 2017-05-18T08:28:45Z

🤞 for jenkins ;)

butonic · 2017-05-18T12:33:04Z

investigating the index question in #27832 (comment) made me look up how ldap handles it. turns out we have another config option hidden in the code: https://github.com/owncloud/user_ldap/blob/5e2a75bb78f57b8a406467ad48ea68faa856a85d/lib/Access.php#L1368-L1368

It is disabled by default, so we get the same search behavior as with ldap.

we use individual indexes because every db can combine them and they help with getByEmail and getByUid as well.

@tomneedham @DeepDiver1975 @PVince81 please review.

PVince81 · 2017-05-18T12:35:16Z

that option was supposed to be an unsupported experimental feature, do we need it enabled now ?

PVince81 · 2017-05-18T12:35:23Z

@jvillafanez

PVince81 · 2017-05-18T12:36:52Z

lib/private/User/AccountMapper.php

@@ -205,7 +220,7 @@ public function callForAllUsers($callback, $search, $onlySeen) {
 		$qb->select(['*'])
 			->from($this->getTableName());

-		if ($search) {
+		if ($search) { // TODO use medial search config option here as well


something for much later ?

well if the medial search was only experimental we can get rid of all the TODOs as well as the config option stuff.

hmm, yeah... if the search is running entirely within OC tables then the option is not useful any more.

It was added for people with special requirements but not default due to perf impact.

butonic · 2017-05-18T12:37:07Z

no we dont need it enabled, but since it was there i assumed someone was using it.

PVince81 · 2017-05-18T19:20:20Z

please test extensively in the RC2, thanks

mrow4a · 2017-10-27T18:10:13Z

Hey guys, what was the motivation to add setter and getter of search terms to account https://github.com/owncloud/core/blob/master/lib/private/User/Account.php#L104-L104 ? Is that really needed? I cannot find nowhere in the code sth using it @tomneedham

If you need to search, there is a heavy query joining with terms table, so I wonder why we need in explicitely in the Account class.

tomneedham · 2017-11-01T07:22:30Z

Since our entity ORM stuff doesnt deal with parent/child relationships we had to half bake this in ourselves. When an account is first created it is used to store search terms: https://github.com/owncloud/core/blob/master/lib/private/User/Manager.php#L452 and also when a user is synchronised: https://github.com/owncloud/core/blob/master/lib/private/User/SyncService.php#L174

(but this logic is under a TODO to combine it since it is duplicated)

tomneedham · 2017-11-01T07:22:45Z

group search terms is another idea.....

lock · 2019-08-02T03:02:32Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

tomneedham added the 2 - Developing label May 16, 2017

tomneedham added this to the 10.0.1 milestone May 16, 2017

tomneedham requested review from PVince81 and DeepDiver1975 May 16, 2017 14:47

tomneedham changed the title ~~[:construction] User backend extendable account searching~~ :construction User backend extendable account searching May 16, 2017

tomneedham changed the title ~~:construction User backend extendable account searching~~ [WIP] User backend extendable account searching May 16, 2017

PVince81 requested a review from jvillafanez May 16, 2017 15:32

PVince81 reviewed May 16, 2017

View reviewed changes

lib/private/User/AccountTerm.php Outdated

use OCP\AppFramework\Db\Entity;

/**

* Class Account

Copy link

Contributor

PVince81 May 16, 2017

Choose a reason for hiding this comment

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

AccountTerm

PVince81 reviewed May 16, 2017

View reviewed changes

DeepDiver1975 requested changes May 17, 2017

View reviewed changes

This was referenced May 17, 2017

Extend accounts table for custom search attributes #27832

Closed

share by email, resolve existing users by email #27847

Closed

tomneedham and others added 15 commits May 17, 2017 10:10

Add find methods to user manager and group manager

e3c3261

Add indexes for improved search performance and use lower uid in query

53bef4a

Add search attribute to user interface

9775519

Add search attribute to user interface

ff08373

Update sharee unit tests for custom search attribute support

d5933a3

Renamed search attribute backend interface

53fc71e

Add tests for group|user manager find methods

a73ef47

Update account after changing search attributes string

7a88187

WIP account_terms table

387fd9e

minor fixes

360753a

use dedicated AccountTerm Mapper and Entity, use terms during search

8ca31fe

fix di

3332789

fix fix for di

a73f2f1

return entity en insert, update and delete

c916482

fix insert

8dbeb54

codestyle

14cc9ad

butonic added 3 - To Review app:user_ldap p1-urgent Critical issue, need to consider hotfix with just that issue and removed 2 - Developing labels May 17, 2017

tomneedham changed the title ~~[WIP] User backend extendable account searching~~ User backend extendable account searching May 17, 2017

butonic requested a review from cdamken May 17, 2017 13:39

tomneedham mentioned this pull request May 17, 2017

Implement search column in accounts table #27850

Closed

3 tasks

Catch exact sharee matches for email address searches

8865e33

Use left join for account search

06d7c36

Because searching by email do not require terms to exist.

PVince81 mentioned this pull request May 18, 2017

Use user id instead of user object in mail status owncloud/guests#85

Merged

2 tasks

butonic added 2 commits May 18, 2017 13:11

add indexes, query correct column

f4b1dc1

add index, introduce medial search option, add sample config

148d443

PVince81 reviewed May 18, 2017

View reviewed changes

remove optional medial search, update tech debt todos

d9fc93c

PVince81 merged commit 5330019 into master May 18, 2017

PVince81 deleted the account-terms-table branch May 18, 2017 19:20

lock bot locked as resolved and limited conversation to collaborators Aug 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

User backend extendable account searching #27906

User backend extendable account searching #27906

tomneedham commented May 16, 2017

PVince81 commented May 16, 2017

PVince81 May 16, 2017

PVince81 left a comment

butonic commented May 16, 2017

DeepDiver1975 commented May 17, 2017

DeepDiver1975 May 17, 2017

DeepDiver1975 May 17, 2017

butonic May 17, 2017

DeepDiver1975 May 17, 2017

butonic May 17, 2017

DeepDiver1975 May 17, 2017

butonic May 17, 2017

DeepDiver1975 May 17, 2017

DeepDiver1975 May 17, 2017

DeepDiver1975 May 17, 2017

DeepDiver1975 May 17, 2017

PVince81 commented May 18, 2017

PVince81 commented May 18, 2017

tomneedham commented May 18, 2017

butonic commented May 18, 2017 •

edited

Loading

PVince81 commented May 18, 2017

PVince81 commented May 18, 2017

PVince81 May 18, 2017

butonic May 18, 2017

PVince81 May 18, 2017

butonic commented May 18, 2017

PVince81 commented May 18, 2017

mrow4a commented Oct 27, 2017 •

edited

Loading

tomneedham commented Nov 1, 2017

tomneedham commented Nov 1, 2017

lock bot commented Aug 2, 2019

User backend extendable account searching #27906

User backend extendable account searching #27906

Conversation

tomneedham commented May 16, 2017

Description

Problem

Proposed / Implemented Solution

Related Issue

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

PVince81 commented May 16, 2017

Choose a reason for hiding this comment

PVince81 left a comment

Choose a reason for hiding this comment

butonic commented May 16, 2017

DeepDiver1975 commented May 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented May 18, 2017

PVince81 commented May 18, 2017

tomneedham commented May 18, 2017

butonic commented May 18, 2017 • edited Loading

PVince81 commented May 18, 2017

PVince81 commented May 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

butonic commented May 18, 2017

PVince81 commented May 18, 2017

mrow4a commented Oct 27, 2017 • edited Loading

tomneedham commented Nov 1, 2017

tomneedham commented Nov 1, 2017

lock bot commented Aug 2, 2019

butonic commented May 18, 2017 •

edited

Loading

mrow4a commented Oct 27, 2017 •

edited

Loading