-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
User backend extendable account searching #27906
Conversation
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. |
lib/private/User/AccountTerm.php
Outdated
use OCP\AppFramework\Db\Entity; | ||
|
||
/** | ||
* Class Account |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AccountTerm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first look the code looks fine
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 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. |
this is one major complaint I'm hearing from big installations: searching the ldap takes ages. From an UX pov this is not acceptable. |
lib/private/User/Account.php
Outdated
} | ||
} | ||
|
||
public static function fromRow(array $row) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needed?
lib/private/User/Account.php
Outdated
@@ -65,6 +64,9 @@ class Account extends Entity { | |||
protected $state; | |||
protected $home; | |||
|
|||
protected $terms = []; | |||
private $_termsChanged = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_ ?? needed ???
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_ 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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use Mapper:insert ???
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adressed with dd377c9#diff-1e60c46513705f729f8e728292eb6f3bR43
* Removes all search terms for a given account id | ||
* @param $account_id | ||
*/ | ||
public function deleteTermsForAccount($account_id) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use Mapper:delete ???
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we want to delete by account_id, not id
lib/private/User/Manager.php
Outdated
@@ -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() ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the terms mapper necessary inside class User? I'd hide the terms mapper within the account mapper
lib/public/IGroupManager.php
Outdated
* @param string $search | ||
* @param int $limit | ||
* @param int $offset | ||
* @return array an array of display names (value) and user ids (key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not the User objects?
tests/lib/Group/ManagerTest.php
Outdated
/** | ||
* @var \OC\User\Manager $userManager | ||
*/ | ||
$userManager = $this->createMock('\OC\User\Manager'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Manager:class
tests/lib/Group/ManagerTest.php
Outdated
* @var \OC\User\Manager $userManager | ||
*/ | ||
$userManager = $this->createMock('\OC\User\Manager'); | ||
$userBackend = $this->createMock('\OC_User_Backend'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OC_User_Backend::class
This doesn't work with guest emails (haven't tried generic emails), steps:
Expected: autocomplete finds the guest user "Vince" 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.
Fixed by replacing "inner join" with "left join": 06d7c36 |
🤞 for jenkins ;) |
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 @tomneedham @DeepDiver1975 @PVince81 please review. |
that option was supposed to be an unsupported experimental feature, do we need it enabled now ? |
lib/private/User/AccountMapper.php
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
something for much later ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well if the medial search was only experimental we can get rid of all the TODOs as well as the config option stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
no we dont need it enabled, but since it was there i assumed someone was using it. |
please test extensively in the RC2, thanks |
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. |
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) |
group search terms is another idea..... |
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. |
Description
Continued from: #27832
Problem
Proposed / Implemented Solution
account_id
(foreign key to accounts.id) andterm
columnsUser_LDAP App:
Related Issue
owncloud/user_ldap#96
How Has This Been Tested?
Screenshots (if appropriate):
because they make your PR look more cool:
Types of changes
Checklist: