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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

User backend extendable account searching #27906

Merged
merged 30 commits into from May 18, 2017

Conversation

@tomneedham
Member

tomneedham commented May 16, 2017

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 owncloud/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:
screen shot 2017-05-16 at 15 44 52

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.

@tomneedham tomneedham added this to the 10.0.1 milestone May 16, 2017

@tomneedham tomneedham requested review from PVince81 and DeepDiver1975 May 16, 2017

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

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

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 16, 2017

Member

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.

Member

PVince81 commented May 16, 2017

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 PVince81 requested a review from jvillafanez May 16, 2017

Show outdated Hide outdated lib/private/User/AccountTerm.php
use OCP\AppFramework\Db\Entity;
/**
* Class Account

This comment has been minimized.

@PVince81

PVince81 May 16, 2017

Member

AccountTerm

@PVince81

PVince81 May 16, 2017

Member

AccountTerm

@PVince81

At first look the code looks fine

@butonic

This comment has been minimized.

Show comment
Hide comment
@butonic

butonic May 16, 2017

Member

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.

Member

butonic commented May 16, 2017

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

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 May 17, 2017

Member

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.

Member

DeepDiver1975 commented May 17, 2017

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.

Show outdated Hide outdated lib/private/User/Account.php
}
}
public static function fromRow(array $row) {

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 May 17, 2017

Member

needed?

@DeepDiver1975
Show outdated Hide outdated lib/private/User/Account.php
@@ -65,6 +64,9 @@ class Account extends Entity {
protected $state;
protected $home;
protected $terms = [];
private $_termsChanged = false;

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 May 17, 2017

Member

_ ?? needed ???

@DeepDiver1975

DeepDiver1975 May 17, 2017

Member

_ ?? needed ???

This comment has been minimized.

@butonic

butonic May 17, 2017

Member

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

@butonic

butonic May 17, 2017

Member

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

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

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 May 17, 2017

Member

use Mapper:insert ???

@DeepDiver1975

DeepDiver1975 May 17, 2017

Member

use Mapper:insert ???

This comment has been minimized.

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

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 May 17, 2017

Member

use Mapper:delete ???

@DeepDiver1975

DeepDiver1975 May 17, 2017

Member

use Mapper:delete ???

This comment has been minimized.

@butonic

butonic May 17, 2017

Member

we want to delete by account_id, not id

@butonic

butonic May 17, 2017

Member

we want to delete by account_id, not id

Show outdated Hide outdated 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() );

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 May 17, 2017

Member

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

@DeepDiver1975

DeepDiver1975 May 17, 2017

Member

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

Show outdated Hide outdated 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)

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 May 17, 2017

Member

not the User objects?

@DeepDiver1975

DeepDiver1975 May 17, 2017

Member

not the User objects?

Show outdated Hide outdated tests/lib/Group/ManagerTest.php
/**
* @var \OC\User\Manager $userManager
*/
$userManager = $this->createMock('\OC\User\Manager');

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 May 17, 2017

Member

Manager:class

@DeepDiver1975

DeepDiver1975 May 17, 2017

Member

Manager:class

Show outdated Hide outdated tests/lib/Group/ManagerTest.php
* @var \OC\User\Manager $userManager
*/
$userManager = $this->createMock('\OC\User\Manager');
$userBackend = $this->createMock('\OC_User_Backend');

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 May 17, 2017

Member

OC_User_Backend::class

@DeepDiver1975

DeepDiver1975 May 17, 2017

Member

OC_User_Backend::class

@tomneedham

This comment has been minimized.

Show comment
Hide comment
@tomneedham

tomneedham May 17, 2017

Member

Rebased

Member

tomneedham commented May 17, 2017

Rebased

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

@butonic butonic requested a review from cdamken May 17, 2017

@tomneedham tomneedham referenced this pull request May 17, 2017

Closed

Implement search column in accounts table #27850

3 of 3 tasks complete
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 18, 2017

Member

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

  1. Login as admin
  2. Enable guest app
  3. Create folder "test1"
  4. Type "Vince" in share field for "test1", select "Share to guest"
  5. Type in local email address, "vincent@host.tld"
  6. Log out
  7. Open mail, open link and set password
  8. Login as admin again
  9. Create another folder "test2"
  10. 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)

Member

PVince81 commented May 18, 2017

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

  1. Login as admin
  2. Enable guest app
  3. Create folder "test1"
  4. Type "Vince" in share field for "test1", select "Share to guest"
  5. Type in local email address, "vincent@host.tld"
  6. Log out
  7. Open mail, open link and set password
  8. Login as admin again
  9. Create another folder "test2"
  10. 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)

Use left join for account search
Because searching by email do not require terms to exist.
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 18, 2017

Member

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

Member

PVince81 commented May 18, 2017

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

@tomneedham

This comment has been minimized.

Show comment
Hide comment
@tomneedham

tomneedham May 18, 2017

Member

馃 for jenkins ;)

Member

tomneedham commented May 18, 2017

馃 for jenkins ;)

@PVince81 PVince81 referenced this pull request May 18, 2017

Merged

Use user id instead of user object in mail status #85

2 of 2 tasks complete
@butonic

This comment has been minimized.

Show comment
Hide comment
@butonic

butonic May 18, 2017

Member

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.

Member

butonic commented May 18, 2017

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

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 18, 2017

Member

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

Member

PVince81 commented May 18, 2017

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

@PVince81

This comment has been minimized.

Show comment
Hide comment
Member

PVince81 commented May 18, 2017

Show outdated Hide outdated 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

This comment has been minimized.

@PVince81

PVince81 May 18, 2017

Member

something for much later ?

@PVince81

PVince81 May 18, 2017

Member

something for much later ?

This comment has been minimized.

@butonic

butonic May 18, 2017

Member

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

@butonic

butonic May 18, 2017

Member

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

This comment has been minimized.

@PVince81

PVince81 May 18, 2017

Member

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.

@PVince81

PVince81 May 18, 2017

Member

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

This comment has been minimized.

Show comment
Hide comment
@butonic

butonic May 18, 2017

Member

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

Member

butonic commented May 18, 2017

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

@PVince81 PVince81 merged commit 5330019 into master May 18, 2017

4 checks passed

Scrutinizer 3 new issues, 33 updated code elements
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@PVince81 PVince81 deleted the account-terms-table branch May 18, 2017

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 18, 2017

Member

please test extensively in the RC2, thanks

Member

PVince81 commented May 18, 2017

please test extensively in the RC2, thanks

@mrow4a

This comment has been minimized.

Show comment
Hide comment
@mrow4a

mrow4a Oct 27, 2017

Contributor

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.

Contributor

mrow4a commented Oct 27, 2017

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

This comment has been minimized.

Show comment
Hide comment
@tomneedham

tomneedham Nov 1, 2017

Member

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)

Member

tomneedham commented Nov 1, 2017

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

This comment has been minimized.

Show comment
Hide comment
@tomneedham

tomneedham Nov 1, 2017

Member

group search terms is another idea.....

Member

tomneedham commented Nov 1, 2017

group search terms is another idea.....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment