Store avatars outside of the home #26124

PVince81 · 2016-09-16T10:15:53Z

Resend of #25790 to master

Store avatars outside of the home

Inside a hidden "metadata-avatars" folder.
Doesn't require home FS setup

@DeepDiver1975 @butonic

mention-bot · 2016-09-16T10:15:54Z

@PVince81, thanks for your PR! By analyzing the annotation information on this pull request, we identified @rullzer, @icewind1991 and @schiessle to be potential reviewers

DeepDiver1975 · 2016-09-22T10:46:58Z

$ ./occ  up
An unhandled exception has been thrown:
TypeError: Argument 3 passed to OC\Repair\MoveAvatarOutsideHome::__construct() must be an instance of OC\Repair\IUserManager, instance of OC\User\Manager given, called in /home/deepdiver/Development/ownCloud/master/lib/private/Repair.php on line 146 and defined in /home/deepdiver/Development/ownCloud/master/lib/private/Repair/MoveAvatarOutsideHome.php:51
Stack trace:
#0 /home/deepdiver/Development/ownCloud/master/lib/private/Repair.php(146): OC\Repair\MoveAvatarOutsideHome->__construct(Object(OC\AllConfig), Object(OC\DB\Connection), Object(OC\User\Manager))
#1 /home/deepdiver/Development/ownCloud/master/core/register_command.php(125): OC\Repair::getRepairSteps()
#2 /home/deepdiver/Development/ownCloud/master/lib/private/Console/Application.php(87): require_once('/home/deepdiver...')
#3 /home/deepdiver/Development/ownCloud/master/console.php(93): OC\Console\Application->loadCommands(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#4 /home/deepdiver/Development/ownCloud/master/occ(11): require_once('/home/deepdiver...')

PVince81 · 2016-09-22T10:52:10Z

Hmm, works for me when upgrading from stable9.1 to this branch.
Make sure you have the latest version of the branch.

DeepDiver1975 · 2016-09-22T12:31:56Z

Loading time on the user page is roughly cut in half

DeepDiver1975 · 2016-09-22T12:32:04Z

tested - works 👍

PVince81 · 2016-09-22T12:33:36Z

We still need to decide whether the name of the folder "metadata-avatars" is acceptable or need something better.

butonic · 2016-09-22T12:47:49Z

since we want #18029 anyway I'd go with /path/to/data/_avatars. _avatars sorts nicely and if we add other global things whe should prefix them with _ as well.

PVince81 · 2016-09-22T13:34:08Z

how about "_metadata/avatars" ? This way there is only a single folder

butonic · 2016-09-22T13:50:43Z

@PVince81 I wrote down my thougts in #18029 (comment). I'd go for /path/to/data/users/a/d/admin/avatar.jpg. We need to move users anyway.

PVince81 · 2016-09-22T13:55:51Z

@butonic but in this case the avatar is back again in the user's home which we don't want.
You probably meant /path/to/data/avatars/a/d/admin/avatar.jpg ?

I don't want to introduce the "a/d" letters at this point, it seems too early. We don't even have a PR ready yet for the user migration which is a big topic.

butonic · 2016-09-22T14:07:23Z

We can do whatever we want for the avatar path, because it is not related to anything our virtual fs does. So '/path/to/data/users/a/d/admin/avatar.jpgshould work just fine, even if a user is calledusersbecause there are currently no other dirs interfering with the1/2/username` scheme.

Currently a file scan would pick up the avatars if a user users existed. But they wouldn't show up.

butonic · 2016-09-22T14:08:27Z

Id place the avatars there because I'd move the users files to /path/to/data/files/a/d/admin/. Then they no longer overlap

PVince81 · 2016-09-22T15:02:06Z

No it won't. /path/to/data/files/a/d/admin/ is still inside the user's home and requires setupFS to mount it, this is exactly what we want to avoid and is the reason why we wanted to move avatars outside the home to avoid setupFS when retrieving avatars. If metadata is outside too, then we can also avoid setupFS.

DeepDiver1975 · 2016-09-22T15:06:11Z

yes - we need to keep avatars out side the users home.

I don't care much how we name the avatars folder - move it out is the most important thing as of now.

butonic · 2016-09-22T15:06:21Z

what is the difference between
/path/to/data/files/a/d/admin/ and
/path/to/data/_avatars/a/d/admin/?

Neither is inside the users home? I'd just already put the users in subdirs to be future proof ...

PVince81 · 2016-09-23T07:13:22Z

As discussed yesterday I thought that /path/to/data/files/a/d/admin/ was the new/future home folder.
If this one is called "files", how would the future home folder be called then ?
I'd rather have it called "metadata" or "avatars" so an admin seeing this exact path can directly understand that it's about metadata, because when reading "files" one will believe it's about the user home's files.

DeepDiver1975 · 2016-09-23T07:26:50Z

Bringing back the user account table and the need to store various additional information with the user - what about storing the avatar in the vcard of the user account table - see #23558

PVince81 · 2016-09-23T09:44:26Z

TODO: change target path to /path/to/data/avatars/a/d/admin/
in case the user id starts with { (LDAP id), use the md5 instead for cutting out the two letters

butonic · 2016-09-26T11:39:58Z

lib/private/AvatarManager.php

+
+	private function buildAvatarPath($userId) {
+		$prefix = $userId;
+		if (strlen($userId) < 2 || !ctype_alnum($userId[0]) || !ctype_alnum($userId[1])) {


KISS: just always md5 the uid ...

note: every time we use md5 we make it very difficult to reverse-lookup to an actual user (for example when an admin wants to find out to which user this folder belongs).
Remember the oc_storages problem with md5 entries.
I guess if it's only a md5 for the user id alone, then one might be able to generate a table of user ids to md5 to do a manual reverse lookup.

PVince81 · 2016-09-27T09:15:30Z

lib/private/Encryption/Util.php

@@ -96,6 +96,9 @@ public function __construct(
 		$this->config = $config;

 		$this->excludedPaths[] = 'files_encryption';
+		$this->excludedPaths[] = 'metadata-avatars';


@DeepDiver1975 @butonic we also need to adjust the path here else encryption is likely to cause trouble like it did on my first test

PVince81 · 2016-09-27T10:10:13Z

BUG: adjust version of repair step
BUG: make repair step use the same new folder (might require to make getAvatarFolder public on a new interface IAvatarManager2 or make it a more generic utility method)
~~BUG?: weird md5 behavior:~~ => was md5'ing the wrong thing

vincent@petry /srv/www/htdocs/owncloud/data/avatars/21/23/2f297a57a5a743894a0e4a801fc3
  % echo -n "2f297a57a5a743894a0e4a801fc3" | md5sum
56597a4557666304b6511a23e29ff93a  -

Not sure where 21 and 23 come from.
The logic with substr_replace also looks strange as if it's overwriting characters instead of inserting the "/"

PVince81 · 2016-09-27T11:16:13Z

Never mind the LDAP issue, I forgot to check out user_ldap app when switching branch.

BUG? many avatar folders created for all LDAP users, not only the ones that have logged in.
@DeepDiver1975 @butonic do we really want that many empty folders ? ^
BUG: moved avatar with encryption is not readable

PVince81 · 2016-10-05T08:55:14Z

rebased for the weird CI failures.

Still need to look at the encryption issues.

PVince81 · 2016-10-05T09:21:52Z

I have retested with encryption twice with local and LDAP users and it all worked fine. Maybe last time I did something wrong.

PVince81 · 2016-10-05T09:24:08Z

Okay, so with a setup LDAP server and going to the users page, it will query the avatar of all users displayed on the page which will create empty avatar folders.

Let's see if we can prevent this by checking the last login flag and avoid creating folders for users who never logged in before.

PVince81 · 2016-10-05T09:27:32Z

Looks like this won't be easily possible. The problem is that even when no avatar exists, we need to return a valid instance of Avatar on which the called might call "get" or "set".
If we'd wanted the folder to be lazily created, we'd need either a LazyAvatar or something like a LazyNonExistingFolder that would create itself once file operations are done on it.
This sounds a bit overcomplicated for not much benefits. Could be solved separately if needed.

@butonic @DeepDiver1975 re-review ? also see text above

PVince81 · 2016-10-05T13:07:12Z

some really weird failures:

Error: Class 'OCA\Files_External\Lib\Storage\SMB' not found

maybe the executor is is a broken state and needs cleanup?

Inside a hidden "metadata-avatars" folder. Doesn't require home FS setup

…ters and refactor tests

PVince81 · 2016-10-07T07:23:55Z

Rebased, hoping for passing CI now

DeepDiver1975 · 2016-10-07T10:24:41Z

SMB and swift again - atleast smb works locally for me ...

let me kick jenkins

DeepDiver1975 · 2016-10-07T11:20:17Z

I see this in the logs - maybe related?

An unhandled exception has been thrown:
TypeError: Argument 5 passed to OC\Repair\MoveAvatarOutsideHome::__construct() must be an instance of OC\Repair\IRootFolder, instance of OC\Files\Node\LazyRoot given, called in /home/deepdiver/Development/ownCloud/master/lib/private/Repair.php on line 150 and defined in /home/deepdiver/Development/ownCloud/master/lib/private/Repair/MoveAvatarOutsideHome.php:68
Stack trace:
#0 /home/deepdiver/Development/ownCloud/master/lib/private/Repair.php(150): OC\Repair\MoveAvatarOutsideHome->__construct(Object(OC\AllConfig), Object(OC\DB\Connection), Object(OC\User\Manager), Object(OC\AvatarManager), Object(OC\Files\Node\LazyRoot), Object(OC\L10N\L10N), Object(OC\Log))
#1 /home/deepdiver/Development/ownCloud/master/core/register_command.php(125): OC\Repair::getRepairSteps()
#2 /home/deepdiver/Development/ownCloud/master/lib/private/Console/Application.php(88): require_once('/home/deepdiver...')
#3 /home/deepdiver/Development/ownCloud/master/console.php(93): OC\Console\Application->loadCommands(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#4 /home/deepdiver/Development/ownCloud/master/occ(11): require_once('/home/deepdiver...')
#5 {main}PHPUnit 5.5.4 by Sebastian Bergmann and contributors.

PVince81 · 2016-10-07T14:17:41Z

Green now. Merge ?

lock · 2019-08-04T15:01:42Z

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 3 - To Review feature:avatars labels Sep 16, 2016

PVince81 added this to the 9.2 milestone Sep 16, 2016

PVince81 mentioned this pull request Sep 16, 2016

[stable9.1] Store avatars outside of the home #25790

Closed

DeepDiver1975 added 2 - Developing and removed 3 - To Review labels Sep 23, 2016

DeepDiver1975 self-assigned this Sep 26, 2016

DeepDiver1975 added 3 - To Review and removed 2 - Developing labels Sep 26, 2016

butonic reviewed Sep 26, 2016

View reviewed changes

PVince81 commented Sep 27, 2016

View reviewed changes

PVince81 mentioned this pull request Sep 27, 2016

Do not try to load avatar for users that never logged in #25802

Closed

PVince81 force-pushed the moveavatarsoutside branch from acfd7ef to c5d0dc7 Compare October 5, 2016 08:55

Vincent Petry and others added 9 commits October 7, 2016 09:23

Store avatars outside of the home

bd5ae40

Inside a hidden "metadata-avatars" folder. Doesn't require home FS setup

Fix it

6ffe67c

Store avatars in e.g. a/d/admin, using md5 on non alphanumeric charac…

72f1d8d

…ters and refactor tests

Always do an md5 of the user name

aabe162

Update Util.php

a0faaf6

Adjust avatar migration version check

067ebe0

Fix LazyRoot folder to properly return get() value

d0ad838

Fix avatar migration with the new folder

9ed0a76

Don't fail move avatar step if user has no home at all

cba3193

PVince81 force-pushed the moveavatarsoutside branch from c5d0dc7 to cba3193 Compare October 7, 2016 07:23

Fix use statement for IRootFolder

d9b7399

PVince81 mentioned this pull request Oct 7, 2016

Can't change user profile picture when there is no more quota available #10565

Closed

DeepDiver1975 merged commit c06f9dc into master Oct 7, 2016

DeepDiver1975 deleted the moveavatarsoutside branch October 7, 2016 14:48

PVince81 mentioned this pull request Oct 18, 2016

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

Closed

butonic mentioned this pull request Nov 8, 2016

Avatar Performance #26585

Open

3 tasks

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

Store avatars outside of the home #26124

Store avatars outside of the home #26124

PVince81 commented Sep 16, 2016

mention-bot commented Sep 16, 2016

DeepDiver1975 commented Sep 22, 2016

PVince81 commented Sep 22, 2016

DeepDiver1975 commented Sep 22, 2016 •

edited

Loading

DeepDiver1975 commented Sep 22, 2016

PVince81 commented Sep 22, 2016

butonic commented Sep 22, 2016

PVince81 commented Sep 22, 2016

butonic commented Sep 22, 2016

PVince81 commented Sep 22, 2016

butonic commented Sep 22, 2016

butonic commented Sep 22, 2016

PVince81 commented Sep 22, 2016

DeepDiver1975 commented Sep 22, 2016

butonic commented Sep 22, 2016

PVince81 commented Sep 23, 2016

DeepDiver1975 commented Sep 23, 2016

PVince81 commented Sep 23, 2016 •

edited by DeepDiver1975

Loading

butonic Sep 26, 2016

PVince81 Sep 27, 2016

PVince81 Sep 27, 2016

PVince81 commented Sep 27, 2016 •

edited

Loading

PVince81 commented Sep 27, 2016 •

edited

Loading

PVince81 commented Oct 5, 2016

PVince81 commented Oct 5, 2016

PVince81 commented Oct 5, 2016

PVince81 commented Oct 5, 2016

PVince81 commented Oct 5, 2016

PVince81 commented Oct 7, 2016

DeepDiver1975 commented Oct 7, 2016

DeepDiver1975 commented Oct 7, 2016

PVince81 commented Oct 7, 2016

lock bot commented Aug 4, 2019

Store avatars outside of the home #26124

Store avatars outside of the home #26124

Conversation

PVince81 commented Sep 16, 2016

mention-bot commented Sep 16, 2016

DeepDiver1975 commented Sep 22, 2016

PVince81 commented Sep 22, 2016

DeepDiver1975 commented Sep 22, 2016 • edited Loading

DeepDiver1975 commented Sep 22, 2016

PVince81 commented Sep 22, 2016

butonic commented Sep 22, 2016

PVince81 commented Sep 22, 2016

butonic commented Sep 22, 2016

PVince81 commented Sep 22, 2016

butonic commented Sep 22, 2016

butonic commented Sep 22, 2016

PVince81 commented Sep 22, 2016

DeepDiver1975 commented Sep 22, 2016

butonic commented Sep 22, 2016

PVince81 commented Sep 23, 2016

DeepDiver1975 commented Sep 23, 2016

PVince81 commented Sep 23, 2016 • edited by DeepDiver1975 Loading

butonic Sep 26, 2016

Choose a reason for hiding this comment

PVince81 Sep 27, 2016

Choose a reason for hiding this comment

PVince81 Sep 27, 2016

Choose a reason for hiding this comment

PVince81 commented Sep 27, 2016 • edited Loading

PVince81 commented Sep 27, 2016 • edited Loading

PVince81 commented Oct 5, 2016

PVince81 commented Oct 5, 2016

PVince81 commented Oct 5, 2016

PVince81 commented Oct 5, 2016

PVince81 commented Oct 5, 2016

PVince81 commented Oct 7, 2016

DeepDiver1975 commented Oct 7, 2016

DeepDiver1975 commented Oct 7, 2016

PVince81 commented Oct 7, 2016

lock bot commented Aug 4, 2019

DeepDiver1975 commented Sep 22, 2016 •

edited

Loading

PVince81 commented Sep 23, 2016 •

edited by DeepDiver1975

Loading

PVince81 commented Sep 27, 2016 •

edited

Loading

PVince81 commented Sep 27, 2016 •

edited

Loading