Unify the core sync logic #29669

tomneedham · 2017-11-23T16:56:45Z

remove some code duplication and get the core to run a proper sync on the user metadata during a login

@DeepDiver1975 @PVince81 note that the account table introduced a change in behavior:
setDisplayName() was no longer called on the backend while canChangeDisplayName() would still check the backend if it implemented that function. With the account table we can always change display name, email, avatar ... but we may want to prevent that. canChangeDisplayName() already contains a check on the system value allow_user_to_change_display_name. canChangeAvatar() and canChangePassword() already exist, but email is missing.

Conceptually there are now two cases for setDisplayName:

End user triggered in the UI. Changeability may depend on the backend, eg LDAP is readonly.
Sync triggered.

The current User object code is intended only for the first case.

@tomneedham we need to add a codepath for the second case. AFAICT we should directly work on the accounts... the sync case bypasses any end user limitations.

DeepDiver1975 · 2017-11-24T08:01:45Z

Tests hate you 😢

butonic · 2017-11-27T11:58:35Z

lib/private/User/Manager.php

@@ -216,10 +221,13 @@ public function checkPassword($loginName, $password) {
 				if ($uid !== false) {
 					try {
 						$account = $this->accountMapper->getByUid($uid);
+						$this->syncService->setupAccount($account, $backend, $account->getUserId());


move to a different place that is also executed after login via apache / saml

tomneedham · 2017-11-27T13:02:10Z

CacheTest seems to not find the displayname on a test user.. weird

codecov · 2017-11-29T10:28:16Z

Codecov Report

Merging #29669 into master will increase coverage by 45.02%.
The diff coverage is 58.69%.

@@              Coverage Diff              @@
##             master   #29669       +/-   ##
=============================================
+ Coverage     15.82%   60.84%   +45.02%     
- Complexity      920    18549    +17629     
=============================================
  Files            51     1092     +1041     
  Lines          3356    61280    +57924     
=============================================
+ Hits            531    37287    +36756     
- Misses         2825    23993    +21168

Impacted Files	Coverage Δ	Complexity Δ
lib/private/User/Account.php	`73.91% <ø> (ø)`	`9 <0> (?)`
core/Migrations/Version20170221114437.php	`0% <0%> (ø)`	`2 <1> (?)`
core/Command/User/SyncBackend.php	`0% <0%> (ø)`	`24 <7> (?)`
lib/private/legacy/user.php	`22.98% <0%> (ø)`	`72 <0> (?)`
lib/private/User/Database.php	`72.03% <100%> (ø)`	`43 <0> (?)`
lib/private/User/User.php	`83.52% <100%> (ø)`	`64 <0> (?)`
lib/private/Server.php	`82.84% <100%> (ø)`	`129 <0> (?)`
lib/private/User/Session.php	`58.91% <34.88%> (ø)`	`139 <14> (?)`
lib/private/User/SyncService.php	`57.86% <63.3%> (ø)`	`55 <52> (?)`
lib/private/User/Manager.php	`82.01% <83.33%> (ø)`	`50 <2> (?)`
... and 1042 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f8f428d...05a82b5. Read the comment docs.

tomneedham · 2017-11-29T19:57:36Z

I most definitely just broke stuff - but I hope the logic is right.... LDAP login works, DB users work, saml seems to die.

Made PRs in LDAP owncloud/user_ldap#156 and Shibboleth https://github.com/owncloud/user_shibboleth/pull/208 to move sync to core

tomneedham · 2017-11-29T20:55:06Z

Safe to say stuff broke...

tomneedham · 2017-11-29T22:22:25Z

Ok - fixed saml. But, for some reason the home path is not set so when you login with a new user they done have a home and you get some weird error about symlinks

Edit: checked with master of core and user_shibboleth: by default, we somehow get a default home path

tomneedham · 2017-11-29T23:04:02Z

Fixed to use default home path if backend does not provide one.

However - this could break the home path during a sync if the backend suddently cannot provide a path or provides an invalid path.... I will add a check to not reset it

tomneedham · 2017-11-29T23:08:48Z

core should validate emails before setting them to oc_accounts.email

tomneedham · 2017-11-29T23:14:01Z

Might have killed some pre and post create user hooks

tomneedham · 2017-11-29T23:16:12Z

Since the syncservice now does user creation, maybe the hooks wont be connected properly anymore...

tomneedham · 2017-11-29T23:28:56Z

I did slightly re-order the hooks here, preCreateUser is now called after the validatePassword GenericEvent...

tomneedham · 2017-11-30T10:42:00Z

lib/private/User/Session.php


 		$uid = $backend->getCurrentUserId();
-		if ($uid === $this->getUser()->getUID()) {
+
+		if ($this->getUser() != null && $uid === $this->getUser()->getUID()) {


If session is active and current session uid matches the request user id then we are authenticated and have nothing more to do

tomneedham · 2017-11-30T10:49:05Z

lib/private/User/Session.php

+		// Die here if not valid
+		if(!$backend->isSessionActive()) {
+			return false;
+		}


This is redundant. OC_User::handleApacheAuth will only call this method if there is already an active IApacheBackend with an active session

jvillafanez · 2018-01-25T08:27:15Z

lib/private/User/SyncService.php

+	private function syncHome(Account $a, UserInterface $backend) {
+		// Home is handled differently, it should only be set on account creation, when there is no home already set
+		// Otherwise it could change on a sync and result in a new user folder being created
+		if($a->getHome() === null) {


I'm not sure if we should do something if the home is different between the account and the backend. As annoying as it will likely be, a warning message saying that we're refusing to update the home folder might be a good idea, specially if the admin expects the home to be updated. Since we don't have any command to change the home folder of a user, I think this is enough; the admin should revert the change in the backend.

Makes sense to at least output something here - because the admin should be aware.

butonic · 2018-01-26T09:38:34Z

lib/private/User/Session.php

+			// completed before we can safely create the users folder.
+			// For example encryption needs to initialize the users keys first
+			// before we can create the user folder with the skeleton files
+			//OC_Hook::emit("OC_User", "post_login", ["uid" => $uid, 'password' => '']);


@tomneedham wrong hook? remove line?

tomneedham · 2018-01-30T17:13:33Z

Started writing tests. Couldn't mock $backend->getHome because it doesnt exist in UserInterface. Talked to @DeepDiver1975 - so just went the whole way and added interfaces for providing a home and providing a displayname.

mrow4a · 2018-02-04T16:01:53Z

lib/private/User/Session.php

+		}
+
+		// Now we try to create the account or sync
+		$this->userSyncService->createOrSyncAccount($uid, $backend);


This means each new login with result in sync, right?

mrow4a · 2018-02-04T16:04:01Z

lib/private/User/SyncService.php

+	 * @param UserInterface $backend of the user
+	 * @return Account
+	 */
+	public function syncAccount(Account $a, UserInterface $backend) {


Isnt it 10 calls to LDAP just for one login? @DeepDiver1975 @butonic @tomneedham

I guess no way it can be 1 call right?

mrow4a · 2018-02-04T16:06:01Z

Well have to admit, this PR cleaned up a lot of things... code looks now... like a code and not spaghetti ;d

phil-davis · 2018-02-05T13:56:18Z

lib/private/User/SyncService.php

 					$this->mapper->update($a);
 				} catch(DoesNotExistException $ex) {
-					$a = $this->createNewAccount($uid);


change in this line fixed/reverted by #30367

lock · 2019-08-01T07:01:43Z

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 3 - To Review label Nov 23, 2017

tomneedham added this to the development milestone Nov 23, 2017

tomneedham assigned tomneedham and butonic Nov 23, 2017

tomneedham requested review from PVince81 and patrickjahns November 23, 2017 16:56

DeepDiver1975 added 2 - Developing and removed 3 - To Review labels Nov 24, 2017

butonic reviewed Nov 27, 2017

View reviewed changes

butonic mentioned this pull request Nov 28, 2017

fake set displayname support owncloud/user_ldap#151

Closed

butonic force-pushed the sync-after-login branch from d7b45f9 to d4c795d Compare November 28, 2017 13:33

tomneedham force-pushed the sync-after-login branch 2 times, most recently from de3e1ca to a905cdf Compare November 29, 2017 12:46

ownclouders added the review carefully! label Nov 29, 2017

tomneedham force-pushed the sync-after-login branch from b08999b to 8444b7f Compare November 29, 2017 13:44

tomneedham force-pushed the sync-after-login branch from 0fe0976 to b4acfb3 Compare November 29, 2017 22:21

tomneedham force-pushed the sync-after-login branch from 92c4d63 to 75a728c Compare November 29, 2017 23:18

tomneedham commented Nov 30, 2017

View reviewed changes

butonic mentioned this pull request Jan 16, 2018

Don't overwrite quota with null #30158

Closed

ownclouders added the status/STALE label Jan 23, 2018

jvillafanez reviewed Jan 25, 2018

View reviewed changes

butonic reviewed Jan 26, 2018

View reviewed changes

ownclouders removed the status/STALE label Jan 30, 2018

butonic force-pushed the sync-after-login branch from ce3144a to 23a7db2 Compare February 2, 2018 13:09

tomneedham force-pushed the sync-after-login branch from 23a7db2 to ce3144a Compare February 2, 2018 13:12

butonic force-pushed the sync-after-login branch from 1e077d4 to a4ddcbc Compare February 2, 2018 15:59

Core controlled user metadata sync

05a82b5

tomneedham force-pushed the sync-after-login branch from a4ddcbc to 05a82b5 Compare February 2, 2018 21:35

tomneedham added 3 - To Review and removed 2 - Developing review carefully! labels Feb 2, 2018

butonic approved these changes Feb 3, 2018

View reviewed changes

butonic merged commit cfd797e into master Feb 3, 2018

butonic deleted the sync-after-login branch February 3, 2018 08:49

mrow4a reviewed Feb 4, 2018

View reviewed changes

individual-it mentioned this pull request Feb 5, 2018

cannot sync LDAP users the first time #30369

Closed

phil-davis reviewed Feb 5, 2018

View reviewed changes

butonic mentioned this pull request Feb 6, 2018

When using encryption, ldap users login for the first time have invalid private keys. owncloud/user_ldap#177

Closed

This was referenced Feb 21, 2018

User Manager loses attributes when cache is full owncloud/user_ldap#184

Closed

[stable10] Sync code deduplication and core controlled sync #30638

Merged

This was referenced Mar 2, 2018

Ensure backend is verified on sync. #30412

Merged

Tidy error messages in acceptance tests run.sh #30679

Merged

butonic mentioned this pull request Jun 18, 2018

Displayname nor email won't be editable from the profile page owncloud/user_ldap#218

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify the core sync logic #29669

Unify the core sync logic #29669

tomneedham commented Nov 23, 2017 •

edited by butonic

Loading

DeepDiver1975 commented Nov 24, 2017

butonic Nov 27, 2017 •

edited

Loading

tomneedham commented Nov 27, 2017

codecov bot commented Nov 29, 2017 •

edited

Loading

tomneedham commented Nov 29, 2017 •

edited

Loading

tomneedham commented Nov 29, 2017

tomneedham commented Nov 29, 2017 •

edited

Loading

tomneedham commented Nov 29, 2017

tomneedham commented Nov 29, 2017

tomneedham commented Nov 29, 2017

tomneedham commented Nov 29, 2017

tomneedham commented Nov 29, 2017

tomneedham Nov 30, 2017

tomneedham Nov 30, 2017

jvillafanez Jan 25, 2018

tomneedham Jan 30, 2018

butonic Jan 26, 2018

tomneedham Jan 30, 2018

tomneedham commented Jan 30, 2018

mrow4a Feb 4, 2018

mrow4a Feb 4, 2018

mrow4a Feb 4, 2018

mrow4a commented Feb 4, 2018 •

edited

Loading

phil-davis Feb 5, 2018

lock bot commented Aug 1, 2019

Unify the core sync logic #29669

Unify the core sync logic #29669

Conversation

tomneedham commented Nov 23, 2017 • edited by butonic Loading

DeepDiver1975 commented Nov 24, 2017

butonic Nov 27, 2017 • edited Loading

Choose a reason for hiding this comment

tomneedham commented Nov 27, 2017

codecov bot commented Nov 29, 2017 • edited Loading

Codecov Report

tomneedham commented Nov 29, 2017 • edited Loading

tomneedham commented Nov 29, 2017

tomneedham commented Nov 29, 2017 • edited Loading

tomneedham commented Nov 29, 2017

tomneedham commented Nov 29, 2017

tomneedham commented Nov 29, 2017

tomneedham commented Nov 29, 2017

tomneedham commented Nov 29, 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

tomneedham commented Jan 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrow4a commented Feb 4, 2018 • edited Loading

Choose a reason for hiding this comment

lock bot commented Aug 1, 2019

tomneedham commented Nov 23, 2017 •

edited by butonic

Loading

butonic Nov 27, 2017 •

edited

Loading

codecov bot commented Nov 29, 2017 •

edited

Loading

tomneedham commented Nov 29, 2017 •

edited

Loading

tomneedham commented Nov 29, 2017 •

edited

Loading

mrow4a commented Feb 4, 2018 •

edited

Loading