Central user account table arriving ..... #27165

DeepDiver1975 · 2017-02-15T15:34:31Z

Related Issue

fixes #23558

Missing

unit test execution is 3 times slower compared to the implementation we had before 😢
handle permission flags like: can set display et
store password reset token in the account table
store language setting in the account table as well
migration of local users - Syncing user backend to the account table .... #27205
migration of ldap users - Syncing user backend to the account table .... #27205
migration of saml users - Syncing user backend to the account table .... #27205
migration or ..... users - Syncing user backend to the account table .... #27205
ldap integration - Syncing user backend to the account table .... #27205

Types of changes

New feature (non-breaking change which adds functionality)

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.

mention-bot · 2017-02-15T15:34:35Z

@DeepDiver1975, thanks for your PR! By analyzing the history of the files in this pull request, we identified @butonic, @VicDeo and @PVince81 to be potential reviewers.

PVince81

Great start!

PVince81 · 2017-02-15T15:36:20Z

core/Migrations/Version20170214112458.php

+	public function changeSchema(Schema $schema, array $options) {
+		$prefix = $options['tablePrefix'];
+		$table = $schema->createTable("{$prefix}accounts");
+		$table->addColumn('id', 'integer', [


let's go big ? bigint ?

PVince81 · 2017-02-15T15:37:25Z

core/Migrations/Version20170214112458.php

+			'notnull' => false,
+			'length' => 255,
+		]);
+		$table->addColumn('quota', 'string', [


Now seeing this, I remember that we already have a quota value in the oc_preferences table. But not as easy to retrieve as a single select on this table here.

Might need to think of migrating it.

PVince81 · 2017-02-15T15:38:34Z

core/Migrations/Version20170214112458.php

+			'notnull' => false,
+			'length' => 32,
+		]);
+		$table->addColumn('backend', 'string', [


also add a backend_id which is the user id as seen by the backend ? unless we think it is always the same as the "user_id" column. That is, assuming that multiple backends can't provide the same user id.

Maybe not that clear but this is - as of now - the class name of the backend

PVince81 · 2017-02-15T15:40:31Z

lib/private/User/Manager.php

@@ -94,7 +100,7 @@ public function getBackends() {
 	 * @param \OCP\UserInterface $backend
 	 */
 	public function registerBackend($backend) {
-		$this->backends[] = $backend;
+		$this->backends[get_class($backend)] = $backend;


I think in the end this would only be called in the background job (or on-demand) as we won't need the backend any more for every operation since we can rely on the account table.

we need the backend for login, inital setup of new accounts (auto provision mode) and import scenarios

PVince81 · 2017-02-16T16:01:41Z

lib/private/Files/View.php

-		} else {
-			return new User($ownerId, null);
-		}
+		return $this->userManager->get($ownerId);


This will break with federated share owners which are not resolveable through the user manager. That's why we create a "virtual" user...

Hmmmm - ok - then we need a second implementation of IUser for this case.
User requires an instance of Account and AccountMapper to be passed in ...

Indeed, that would align with this idea: #25815 (comment)

Later on I wonder if we should also have a "federated users backend" which would deliver such users, they'd also appear in the account table under the "federation" backend for quick lookup. (but that sounds redundant from the system address book)

something to think about - for sure ....

mrow4a · 2017-02-16T22:26:48Z

core/Migrations/Version20170214112458.php

+			'notnull' => false,
+			'length' => 32,
+		]);
+		$table->addColumn('backend', Type::STRING, [


@DeepDiver1975 Why it is a String? Shouldnt it be INT representing foreign key? Why do we allow NULL here?

Is it connected to 0: initial?

Because it's the php class name

How do you ensure, that if the backend is deleted, all related records are also deleted. Will we do single transaction doing that to ensure atomicity and consistency? Is backend primary key somewhere? This smells like possible inconsistency later, if some developer is not careful in the future.

If a backend is not enabled or has been deleted the user can no longer login. Deleting a user is an explicit operation and nothing we should do 'just because the backend is no longer there'

I wound ensure this not null, and add another state 4: backend-deleted or mark user as deleted, in a single transaction - also not sure about this. Actualy this sound quite dangerous for me, that someone can enable and disable backends and we still have a hanging relation pointing to nowhere. Not sure how to resolve it not to brake consistency

yes - nut null makes sense here - thx

Ok, because I assume we dont have a new table holding backends, we just use classnames to do that? Makes sense.

mrow4a · 2017-02-16T22:28:19Z

core/Migrations/Version20170214112458.php

+			'length' => 64,
+		]);
+		$table->addColumn('home', Type::STRING, [
+			'notnull' => false,


Why do we allow null here?

Is it connected to 0: initial?

mrow4a · 2017-02-16T22:29:26Z

core/Migrations/Version20170214112458.php

+		$table->addColumn('state', Type::SMALLINT, [
+			'notnull' => true,
+			'default' => 0,
+			'comment' => '0: initial, 1: enabled, 2: disabled, 3: deleted'


0: initial, I start loving it. Is it connected with last_login, backend and home?

I'm not yet fully sure if we need 0:initial ...

mrow4a · 2017-02-16T22:34:06Z

lib/private/User/AccountMapper.php

+		$qb->select('*')
+			->from($this->getTableName())
+			->where($qb->expr()->iLike($fieldName, $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($pattern) . '%')))
+			->orderBy($fieldName, $orderBy ? 'ASC' : 'DESC');


orderBy is quite expensive operation, do we need to do that by default? I think it makes sense only for displayname, isnt it?

Why dont we use LIMIT on DB level? What findEntities is exactly doing? Please be aware of that it will go over all never-logged-in accounts in LDAP also, doing sequential read on the disk for pages for all of them!

Limit is on db level. And if we want pagination we need to sort on db level.
Orderby is not expensive is there is an index.
(Need to add it 😆)

Internaly in DB, in order to do limit, you have to finish all others. Adding OrderBy basically tells the plan generator that you need a whole picture of the table, thus whole scan (it is enforced by sort requiring full view on the input - blocking operator). LIMIT just tells the sort to stop sorting. Of course, unless you have index, found good reference https://www.percona.com/blog/2006/09/01/mysql-order-by-limit-performance-optimization/. Just wanted to ensure that we wont do anything crazy here :>

The users are expected to be sorted - this has to be done in the db - no other option available

mrow4a · 2017-02-16T22:55:24Z

lib/private/User/AccountMapper.php

+			->where($qb->expr()->iLike($fieldName, $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($pattern) . '%')))
+			->orderBy($fieldName, $orderBy ? 'ASC' : 'DESC');
+
+		return $this->findEntities($qb->getSQL(), $qb->getParameters(), $limit, $offset);


We are passing here $limit. Why do we first scan whole the table and then limit that on PHP level? What findEntities is exactly doing, is it belonging to the query above or we extract logic from DB to PHP?

It is on db level. See the code inside find entries

mrow4a · 2017-02-17T08:59:39Z

core/Migrations/Version20170214112458.php

+		$prefix = $options['tablePrefix'];
+		$table = $schema->createTable("{$prefix}accounts");
+		$table->addColumn('id', Type::BIGINT, [
+			'autoincrement' => true,


Is it a primary key or foreign key somewhere? I might have a question here about user uniqueness, where backend tries to add user which already exists, and how do we distinguish between them

auto increment always forces to be the primary key

Yes, this is true

mrow4a · 2017-02-17T09:00:27Z

core/Migrations/Version20170214112458.php

+			'notnull' => false,
+			'length' => 255,
+		]);
+		$table->addColumn('user_id', Type::STRING, [


Is it a primary key or foreign key somewhere? I might have a question here about user uniqueness, where backend tries to add user which already exists, and how do we distinguish between them. I dont also understand why we have two IDs. DO we have redundant tables? Do you have any drawing showing relations between all involved tables?

see the index definition - this is unique

Do you have some db relations drawing of Migration feature so we can easier understand?

I think I see the whole picture of relations of this table with others, but it is easier to look at it

Do you have some db relations drawing of Migration feature so we can easier understand?

I don't get this question. The migration feature is unrelated any db relations.

mrow4a

I think I need to fully understand the relationships and redundancy between tables

DeepDiver1975 · 2017-02-17T09:57:29Z

I think I need to fully understand the relationships and redundancy between tables

I guess so 😉

mrow4a · 2017-02-21T12:02:49Z

@DeepDiver1975 I did not go in details in the code yet, but seems the priority in implementation of this table will be to put as much load on table scan of oc_account table, so most of queries will just ask for records of this table, and very very seldomely do the join with e.g. ldap table (this is expensive, if both tables are big, and it will be like that). (Does it replace ldap table, or is just normalisation ?)

PVince81 · 2017-02-21T14:54:40Z

very very seldomely do the join with e.g. ldap table

@mrow4a no, if we had that it would be nice.
No, the old code does a PHP-level "join" by connecting directly to LDAP... or in some cases memcache. But partial searches are not possible with memcache so they always hit LDAP.

With this table partial searches can be done only in a single location: the oc_account table.

butonic · 2017-03-09T07:55:18Z

Obviously, this will require use to kill a lot of code in user_ldap and make migration rock solid.

DeepDiver1975 · 2017-03-09T08:16:12Z

Then there is the task of keeping a user back end and the account table in sync. Which I see as an occ sync command. In addition to a sync all we also need a sync this uid, because for LDAP there may be triggers that can be used to update a user on demand. Gonicus eg allows that.

see #27205

PVince81 · 2017-03-09T08:36:59Z

Something comes to mind: do we need a "lastsynctime" column ?
In LDAP we had "lastFeatureRefresh" in the preferences table which told the LDAP code to not try and refetch the user details based on that value.

butonic

I tried understanding the changes. Impressive list of test changes ... the avatar stuff can be discussed idependently.

butonic · 2017-03-09T08:59:22Z

lib/private/User/AccountMapper.php

+		$parameter = (string)$qb->createNamedParameter($uid);
+		$qb->select('*')
+			->from($this->getTableName())
+			->where($qb->expr()->eq('lower_user_id', $qb->createFunction("LOWER($parameter)")));


strtolower $uid instead of using SQL?

php strtolower and database tolower behave slightly different - we have to choose one way .... (hmmm .... do I?)

grrr - actually I'm mixing it 🤦‍♂️

butonic · 2017-03-09T09:06:54Z

lib/private/User/RemoteUser.php

+	 * @return IImage|null
+	 * @since 9.0.0
+	 */
+	public function getAvatarImage($size) {


I don't think we really need the avatar image as part of the user. I'd prefer to implement it as an app. That app can store the avatar image inside its app folder, no need for our internal filesystem. It could try to fetch the avatar from gravatar based on the email address if an internal avatar is not available.

separate discussion - from an api poc it is there - not much to change right away ...

butonic · 2017-03-09T09:09:26Z

lib/private/User/User.php

@@ -183,18 +157,16 @@ public function setEMailAddress($mailAddress) {
 	 * @return int
 	 */
 	public function getLastLogin() {
-		return $this->lastLogin;
+		return (int)$this->account->getLastLogin();


shouldn't the entity return the right type?

yes - it should ....

butonic · 2017-03-09T09:11:47Z

lib/private/User/User.php


-			// FIXME: Feels like an hack - suggestions?
+		// FIXME: Feels like an hack - suggestions?


foreign keys ftw ;-)

butonic · 2017-03-09T09:25:29Z

lib/private/User/User.php

 	 */
-	public function __construct($uid, $backend, $emitter = null, IConfig $config = null,
+	public function __construct(Account $account, AccountMapper $mapper, $emitter = null, IConfig $config = null,


so we don't really need the user database backend anymore, right?

the backend is accessible via the account

…ction - behave accordingly

…avatar and displayname

PVince81 · 2017-03-17T15:05:12Z

tests/lib/Files/Cache/CacheTest.php

@@ -322,7 +322,7 @@ function testSearchByTag() {
 		$userId = $this->getUniqueId('user');
 		\OC::$server->getUserManager()->createUser($userId, $userId);
 		$this->loginAsUser($userId);
-		$user = new \OC\User\User($userId, null);
+//		$user = new \OC\User\User($userId, null);


commented out ?

lock · 2019-08-03T17:02:51Z

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.

DeepDiver1975 added the 2 - Developing label Feb 15, 2017

DeepDiver1975 added this to the 10.0 milestone Feb 15, 2017

PVince81 reviewed Feb 15, 2017

View reviewed changes

DeepDiver1975 force-pushed the central-account-table branch from 2f66865 to 78e1a6b Compare February 16, 2017 14:09

PVince81 reviewed Feb 16, 2017

View reviewed changes

DeepDiver1975 force-pushed the central-account-table branch from 3e1e383 to 54a77ef Compare February 16, 2017 17:36

mrow4a reviewed Feb 16, 2017

View reviewed changes

mrow4a reviewed Feb 17, 2017

View reviewed changes

mrow4a requested changes Feb 17, 2017

View reviewed changes

DeepDiver1975 force-pushed the central-account-table branch 15 times, most recently from 9f0621a to 331d06c Compare February 21, 2017 11:04

butonic approved these changes Mar 9, 2017

View reviewed changes

DeepDiver1975 force-pushed the central-account-table branch 9 times, most recently from e14c1f2 to 2bc42a7 Compare March 14, 2017 11:23

DeepDiver1975 added 10 commits March 16, 2017 13:16

Central user account table arriving .....

c207432

During installation no UserSession is available and no database conne…

5ac1d02

…ction - behave accordingly

Kill dummy user backend

b285fb8

Fix unit tests ...

5db4232

Fix dynamic loading of user backends

5ada445

Fix login with LDAP

50fc494

Use backends to get an understanding about the possibility to change …

87a5028

…avatar and displayname

Validate session only if we are not in a basic auth request

472aa26

Fix session verification for token based requests

c42fc60

Fix user creation

3b088af

DeepDiver1975 force-pushed the central-account-table branch from 3aaaf2a to 3b088af Compare March 16, 2017 12:16

PVince81 reviewed Mar 17, 2017

View reviewed changes

DeepDiver1975 mentioned this pull request Mar 20, 2017

Central account table replace user manager when testing #27276

Merged

16 tasks

DeepDiver1975 closed this Mar 21, 2017

DeepDiver1975 deleted the central-account-table branch March 21, 2017 09:38

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


		// FIXME: Feels like an hack - suggestions?
		// FIXME: Feels like an hack - suggestions?

Central user account table arriving ..... #27165

Central user account table arriving ..... #27165

Conversation

DeepDiver1975 commented Feb 15, 2017 • edited Loading

Related Issue

Missing

Types of changes

Checklist:

mention-bot commented Feb 15, 2017

PVince81 left a 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

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

mrow4a Feb 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrow4a Feb 17, 2017 • edited Loading

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

mrow4a Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

mrow4a Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrow4a Feb 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrow4a Feb 16, 2017 • edited Loading

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

mrow4a Feb 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrow4a Feb 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrow4a left a comment

Choose a reason for hiding this comment

DeepDiver1975 commented Feb 17, 2017

mrow4a commented Feb 21, 2017 • edited Loading

PVince81 commented Feb 21, 2017

butonic commented Mar 9, 2017

DeepDiver1975 commented Mar 9, 2017

PVince81 commented Mar 9, 2017

butonic left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lock bot commented Aug 3, 2019

DeepDiver1975 commented Feb 15, 2017 •

edited

Loading

mrow4a Feb 17, 2017 •

edited

Loading

mrow4a Feb 17, 2017 •

edited

Loading

mrow4a Feb 16, 2017 •

edited

Loading

mrow4a Feb 16, 2017 •

edited

Loading

mrow4a Feb 17, 2017 •

edited

Loading

mrow4a Feb 16, 2017 •

edited

Loading

mrow4a Feb 17, 2017 •

edited

Loading

mrow4a Feb 17, 2017 •

edited

Loading

mrow4a commented Feb 21, 2017 •

edited

Loading