[stable9.1] Fix initMountPoints to set usersSetup earlier #26399

PVince81 · 2016-10-18T15:27:56Z

Description

This is needed because in some cases like LDAP, the user manager itself
might trigger avatar updates which would internally also call
initMountPoints with the same user. This could cause the same user to
be setup twice, and in some sharing situations could cause recursive
deduplication of shares by adding "(2)" every time.

Related Issue

Fixes #25718

Motivation and Context

See description

How Has This Been Tested?

Very difficult to reproduce: #25718 (comment)
Tested with the provided DB dump with LDAP + avatars, in an env where the issue could be reproduced.

Screenshots (if appropriate):

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.

Backports:

Potentially this issue could appear in previous versions as well, maybe in different forms, so I'd say let's backport to:

~~stable9~~ not affected
~~ stable8.2~~ not affected
master / 9.2

@jvillafanez @DeepDiver1975

mention-bot · 2016-10-18T15:27:59Z

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

jvillafanez · 2016-10-18T15:33:00Z

lib/private/Files/Filesystem.php

+		}
+
+		self::$usersSetup[$user] = true;
+		if (isset(self::$usersSetup[$user])) {


I don't understand this.... This code will always exit here, doesn't it? 😕

You mean I don't understand... I mixed up the rows during conflict solving, sorry ☹️

jvillafanez · 2016-10-18T15:36:22Z

lib/private/Files/Filesystem.php

 			throw new \OC\User\NoUserException('Backends provided no user object for ' . $user);
 		}

 		// workaround in case of different casings
 		if ($user !== $userObject->getUID()) {
 			$stack = json_encode(debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 50));
 			\OCP\Util::writeLog('files', 'initMountPoints() called with wrong user casing. This could be a bug. Expected: "' . $userObject->getUID() . '" got "' . $user . '". Stack: ' . $stack, \OCP\Util::WARN);
-		}
-		$user = $userObject->getUID();
+			$user = $userObject->getUID();


Calling three times the getUID method in four lines doesn't look good. Better cache it in a local variable.

ok, adjusted. I'll redo a test

jvillafanez · 2016-10-18T15:43:56Z

In general, I feel there are too many calls to self::$usersSetup[$user] for something that should be just checking and setting a flag.

PVince81 · 2016-10-18T15:44:02Z

lib/private/Files/Filesystem.php

+			return;
+		}
+
+		self::$usersSetup[$user] = true;


@jvillafanez fixed, I forgot to push...

jvillafanez · 2016-10-18T16:05:18Z

I don't have better alternatives for my previous comment nor for the "checkAndSet" piece of code, so 👍

PVince81 · 2016-10-18T16:55:10Z

@jvillafanez PR adjusted. I also retested with avatars with different casings to make sure it still works.

PVince81 · 2016-10-19T07:27:51Z

I finally found some steps to reproduce, see #25718 (comment)

PVince81 · 2016-10-19T07:31:26Z

OC 9.0 / stable9 seems unaffected by this bug. It seems that when it gets into initMountPoints the users are already cached so it goes go through the avatar fetching code path. So it's mostly luck.
I still think we should backport the fix to avoid other side effects, in case there are other code paths that lead to non-cached users.

This is needed because in some cases like LDAP, the user manager itself might trigger avatar updates which would internally also call initMountPoints with the same user. This could cause the same user to be setup twice, and in some sharing situations could cause recursive deduplication of shares by adding "(2)" every time.

PVince81 · 2016-10-19T07:42:11Z

Fixed unit test by deleting the usersSetup key in case the user is not found instead of setting to false', because the check above usesisset()`.

PVince81 · 2016-10-19T08:07:19Z

Okay, actually OC < 9.1 is not affected. This is a regression introduced by me in #24187.

So only a forward port is needed.

PVince81 · 2016-10-19T09:49:02Z

build passed, not published due to rate limits => merging

PVince81 · 2016-10-19T09:49:43Z

master port blocked by Add using casing check/fix for initMountPoints #26398 due to future conflicts

PVince81 · 2016-10-19T12:05:15Z

master: #26411

lock · 2019-08-04T13:01:52Z

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.

PVince81 added this to the 9.1.2 milestone Oct 18, 2016

PVince81 mentioned this pull request Oct 18, 2016

[debug] [stable9.1] Fix initMountPoints to set usersSetup earlier #26396

Closed

12 tasks

PVince81 added 3 - To Review backport-request labels Oct 18, 2016

jvillafanez reviewed Oct 18, 2016

View reviewed changes

PVince81 force-pushed the stable9.1-userssetup-check-order branch from 46556dc to cae38de Compare October 18, 2016 15:43

PVince81 commented Oct 18, 2016

View reviewed changes

lib/private/Files/Filesystem.php

return;

}

self::$usersSetup[$user] = true;

Copy link

Contributor Author

PVince81 Oct 18, 2016

Choose a reason for hiding this comment

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

@jvillafanez fixed, I forgot to push...

PVince81 force-pushed the stable9.1-userssetup-check-order branch from cae38de to 2c559f6 Compare October 18, 2016 15:56

PVince81 force-pushed the stable9.1-userssetup-check-order branch from 2c559f6 to ef096ee Compare October 18, 2016 16:53

PVince81 added 4 - To release and removed 3 - To Review labels Oct 18, 2016

PVince81 force-pushed the stable9.1-userssetup-check-order branch from ef096ee to c82d62a Compare October 19, 2016 07:41

PVince81 merged commit 2972d35 into stable9.1 Oct 19, 2016

PVince81 deleted the stable9.1-userssetup-check-order branch October 19, 2016 09:49

This was referenced Oct 19, 2016

Shared files, oC 9.1 adding "(2)" at the end #25718

Closed

[master] Fix initMountPoints to set usersSetup earlier #26411

Merged

PVince81 removed the backport-request label Oct 19, 2016

Nilix007 mentioned this pull request Nov 1, 2016

Share links do not work for logged-in LDAP users with avatars #26517

Closed

Akasch mentioned this pull request Nov 1, 2016

No content is shown on folders with many hidden Folders #26518

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[stable9.1] Fix initMountPoints to set usersSetup earlier #26399

[stable9.1] Fix initMountPoints to set usersSetup earlier #26399

PVince81 commented Oct 18, 2016 •

edited

Loading

mention-bot commented Oct 18, 2016

jvillafanez Oct 18, 2016

PVince81 Oct 18, 2016

jvillafanez Oct 18, 2016

PVince81 Oct 18, 2016

jvillafanez commented Oct 18, 2016

PVince81 Oct 18, 2016

jvillafanez commented Oct 18, 2016

PVince81 commented Oct 18, 2016

PVince81 commented Oct 19, 2016

PVince81 commented Oct 19, 2016

PVince81 commented Oct 19, 2016

PVince81 commented Oct 19, 2016

PVince81 commented Oct 19, 2016

PVince81 commented Oct 19, 2016 •

edited

Loading

PVince81 commented Oct 19, 2016

lock bot commented Aug 4, 2019

[stable9.1] Fix initMountPoints to set usersSetup earlier #26399

[stable9.1] Fix initMountPoints to set usersSetup earlier #26399

Conversation

PVince81 commented Oct 18, 2016 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Backports:

mention-bot commented Oct 18, 2016

jvillafanez Oct 18, 2016

Choose a reason for hiding this comment

PVince81 Oct 18, 2016

Choose a reason for hiding this comment

jvillafanez Oct 18, 2016

Choose a reason for hiding this comment

PVince81 Oct 18, 2016

Choose a reason for hiding this comment

jvillafanez commented Oct 18, 2016

PVince81 Oct 18, 2016

Choose a reason for hiding this comment

jvillafanez commented Oct 18, 2016

PVince81 commented Oct 18, 2016

PVince81 commented Oct 19, 2016

PVince81 commented Oct 19, 2016

PVince81 commented Oct 19, 2016

PVince81 commented Oct 19, 2016

PVince81 commented Oct 19, 2016

PVince81 commented Oct 19, 2016 • edited Loading

PVince81 commented Oct 19, 2016

lock bot commented Aug 4, 2019

PVince81 commented Oct 18, 2016 •

edited

Loading

PVince81 commented Oct 19, 2016 •

edited

Loading