Skip to content

Commit

Permalink
Merge pull request #35530 from owncloud/stable10-fix-35311
Browse files Browse the repository at this point in the history
[stable10] Fix avatar path
  • Loading branch information
phil-davis authored Jun 14, 2019
2 parents 49bd817 + 1e5d792 commit 343f593
Show file tree
Hide file tree
Showing 5 changed files with 234 additions and 23 deletions.
2 changes: 1 addition & 1 deletion lib/private/Avatar.php
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ public function __construct(IStorage $storage, IL10N $l, User $user, ILogger $lo
}

private function buildAvatarPath() {
return \substr_replace(\substr_replace(\md5($this->user->getUID()), '/', 4, 0), '/', 2, 0);
return 'avatars/' . \substr_replace(\substr_replace(\md5($this->user->getUID()), '/', 4, 0), '/', 2, 0);
}

/**
Expand Down
10 changes: 10 additions & 0 deletions lib/private/Repair.php
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
use OC\Repair\Collation;
use OC\Repair\DisableExtraThemes;
use OC\Repair\DropOldJobs;
use OC\Repair\MoveAvatarIntoSubFolder;
use OC\Repair\OldGroupMembershipShares;
use OC\Repair\RemoveGetETagEntries;
use OC\Repair\RemoveRootShares;
Expand Down Expand Up @@ -151,6 +152,15 @@ public static function getRepairSteps() {
\OC::$server->getL10N('core'),
\OC::$server->getLogger()
),
new MoveAvatarIntoSubFolder(
\OC::$server->getConfig(),
\OC::$server->getDatabaseConnection(),
\OC::$server->getUserManager(),
\OC::$server->getAvatarManager(),
\OC::$server->getLazyRootFolder(),
\OC::$server->getL10N('core'),
\OC::$server->getLogger()
),
new RemoveRootShares(\OC::$server->getDatabaseConnection(), \OC::$server->getUserManager(), \OC::$server->getLazyRootFolder()),
new RepairUnmergedShares(
\OC::$server->getConfig(),
Expand Down
200 changes: 200 additions & 0 deletions lib/private/Repair/MoveAvatarIntoSubFolder.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,200 @@
<?php
/**
* @author Viktar Dubiniuk <dubiniuk@owncloud.com>
*
* @copyright Copyright (c) 2019, ownCloud GmbH
* @license AGPL-3.0
*
* This code is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License, version 3,
* as published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License, version 3,
* along with this program. If not, see <http://www.gnu.org/licenses/>
*
*/
namespace OC\Repair;

use OC\Files\Node\File;
use OC\NotSquareException;
use OC\User\NoUserException;
use OCP\Files\Folder;
use OCP\Files\IRootFolder;
use OCP\Files\Storage\IStorage;
use OCP\Files\StorageNotAvailableException;
use OCP\IDBConnection;
use OCP\IL10N;
use OCP\ILogger;
use OCP\IUserManager;
use OCP\Migration\IOutput;
use OCP\Migration\IRepairStep;
use OCP\IUser;
use OC\Avatar;
use OCP\IConfig;
use OCP\IAvatarManager;
use OCP\Files\NotFoundException;

/**
* OC 10.2.0 spreads avatars into multiple subdirectories of data dir
* This repair step puts them under data/avatars/
*
* @package OC\Repair
*/
class MoveAvatarIntoSubFolder implements IRepairStep {
/** @var \OCP\IConfig */
protected $config;

/** @var IDBConnection */
private $connection;

/** @var IUserManager */
private $userManager;

/** @var IAvatarManager */
private $avatarManager;

/** @var IRootFolder */
private $rootFolder;

/** @var \OCP\ILogger */
private $logger;

/** @var \OCP\IL10N */
private $l;

/**
* @param IConfig $config config
* @param IDBConnection $connection database connection
* @param IUserManager $userManager user manager
* @param IAvatarManager $avatarManager
* @param IRootFolder $rootFolder
* @param IL10N $l10n
* @param ILogger $logger
*/
public function __construct(
IConfig $config,
IDBConnection $connection,
IUserManager $userManager,
IAvatarManager $avatarManager,
IRootFolder $rootFolder,
IL10N $l10n,
ILogger $logger
) {
$this->config = $config;
$this->connection = $connection;
$this->userManager = $userManager;
$this->avatarManager = $avatarManager;
$this->rootFolder = $rootFolder;
$this->l = $l10n;
$this->logger = $logger;
}

/**
* @return string
*/
public function getName() {
return 'Fix user avatars location';
}

/**
* Move avatars outside of their homes
*
* @param IOutput $out
* @param IUser $user
*/
private function moveAvatars(IOutput $out, IUser $user) {
$userId = $user->getUID();

try {
\OC\Files\Filesystem::initMountPoints($userId);

$brokenPath = \substr_replace(
\substr_replace(\md5($userId), '/', 4, 0),
'/',
2,
0
);
if ($this->brokenAvatarExists($brokenPath)) {
$newAvatarStorage = $this->rootFolder->get('/avatars/')->getStorage();
$avatar = new Avatar($newAvatarStorage, $this->l, $user, $this->logger);
// Overwrite pre 10.2 avatar if any as they were reuploaded
try {
$oldAvatarFile = $this->rootFolder->get("{$brokenPath}/avatar.jpg");
} catch (NotFoundException $e) {
$oldAvatarFile = $this->rootFolder->get("{$brokenPath}/avatar.png");
}
$avatar->set($oldAvatarFile->getContent());

// Delete old avatars path only if it does not contain any other items
// as it might collide with a real user directory
// e.g. data/de/ad/3b7a931377ad4ab5ad6a9cd718aa could be used by a real user de
// so we need to keep de if it has any other child except ad
$oldAvatarDir = $this->rootFolder->get($brokenPath);
$oldAvatarDir->delete();
$brokenParent = $oldAvatarDir->getParent();
$brokenRoot = $brokenParent->getParent();
foreach ([$brokenParent, $brokenRoot] as $dir) {
/** @var \OCP\Files\Folder $dir */
if ($dir->isDeletable() === false
|| \count($dir->getDirectoryListing()) > 0
) {
break;
}
$dir->delete();
}
}
} catch (NoUserException $e) {
$this->logger->warning("Skipping avatar move for $userId: User does not exist.", ['app' => __METHOD__]);
} catch (NotFoundException $e) {
// not all users have a home, ignore
$this->logger->debug("Skipping avatar move for $userId: User has no home or avatar folder.", ['app' => __METHOD__]);
} catch (NotSquareException $e) {
$this->logger->warning("Skipping avatar move for $userId: avatar image {$oldAvatarFile->getPath()} for user $userId is not square.", ['app' => __METHOD__]);
} catch (\Exception $e) {
$this->logger->logException($e, ['app' => __METHOD__, 'message' => "Skipping avatar move for $userId"]);
}

\OC_Util::tearDownFS();
}

/**
* @param string $basePath
* @return bool
* @throws NotFoundException
*/
private function brokenAvatarExists($basePath) {
try {
$storage = $this->rootFolder->get('/')->getStorage();
return $storage->file_exists("{$basePath}/avatar.jpg")
|| $storage->file_exists("{$basePath}/avatar.png");
} catch (StorageNotAvailableException $e) {
return false;
}
}

/**
* @param IOutput $output
*/
public function run(IOutput $output) {
$ocVersionFromBeforeUpdate = $this->config->getSystemValue('version', '0.0.0');
if (\version_compare($ocVersionFromBeforeUpdate, '10.2.0.5', '=')) {
$function = function (IUser $user) use ($output) {
$this->moveAvatars($output, $user);
$output->advance();
};

$output->startProgress($this->userManager->countSeenUsers());

$this->userManager->callForSeenUsers($function);

$output->finishProgress();
} else {
$output->info("No action required");
}
}
}
5 changes: 3 additions & 2 deletions tests/acceptance/features/cliMain/maintenance.feature
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ Feature: Maintenance command
Scenario: Repair steps should be listed correctly
When the administrator list the repair steps using the occ command
Then the command should have been successful
And the command output should contain the text "Found 16 repair steps"
And the command output should contain the text "Found 17 repair steps"
And the command output table should contain the following text:
| table_column |
| OC\Repair\RepairMimeTypes |
Expand All @@ -22,6 +22,7 @@ Feature: Maintenance command
| OC\Repair\RepairSubShares |
| OC\Repair\SharePropagation |
| OC\Repair\MoveAvatarOutsideHome |
| OC\Repair\MoveAvatarIntoSubFolder |
| OC\Repair\RemoveRootShares |
| OC\Repair\RepairUnmergedShares |
| OC\Repair\DisableExtraThemes |
Expand All @@ -31,4 +32,4 @@ Feature: Maintenance command
Scenario: Running single repair step without providing value should fail
When the administrator invokes occ command "maintenance:repair --single"
Then the command should have failed with exit code 1
And the command error output should contain the text 'The "--single" option requires a value'
And the command error output should contain the text 'The "--single" option requires a value'
40 changes: 20 additions & 20 deletions tests/lib/AvatarTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -54,10 +54,10 @@ public function testPathBuilding($expectedPath, $userId) {

public function providesUserIds() {
return [
['21/23/2f297a57a5a743894a0e4a801fc3', 'admin'],
['c4/ca/4238a0b923820dcc509a6f75849b', '1'],
['f9/5b/70fdc3088560732a5ac135644506', '{'],
['d4/1d/8cd98f00b204e9800998ecf8427e', ''],
['avatars/21/23/2f297a57a5a743894a0e4a801fc3', 'admin'],
['avatars/c4/ca/4238a0b923820dcc509a6f75849b', '1'],
['avatars/f9/5b/70fdc3088560732a5ac135644506', '{'],
['avatars/d4/1d/8cd98f00b204e9800998ecf8427e', ''],
];
}

Expand All @@ -68,35 +68,35 @@ public function testGetNoAvatar() {
public function testGetAvatarSizeMatch() {
$this->storage->method('file_exists')
->will($this->returnValueMap([
['d4/1d/8cd98f00b204e9800998ecf8427e/avatar.jpg', true],
['d4/1d/8cd98f00b204e9800998ecf8427e/avatar.128.jpg', true],
['avatars/d4/1d/8cd98f00b204e9800998ecf8427e/avatar.jpg', true],
['avatars/d4/1d/8cd98f00b204e9800998ecf8427e/avatar.128.jpg', true],
]));

$expected = new \OC_Image(\OC::$SERVERROOT . '/tests/data/testavatar.png');

$this->storage->method('file_get_contents')->with('d4/1d/8cd98f00b204e9800998ecf8427e/avatar.128.jpg')->willReturn($expected->data());
$this->storage->method('file_get_contents')->with('avatars/d4/1d/8cd98f00b204e9800998ecf8427e/avatar.128.jpg')->willReturn($expected->data());

$this->assertEquals($expected->data(), $this->avatar->get(128)->data());
}

public function testGetAvatarSizeMinusOne() {
$this->storage->method('file_exists')
->will($this->returnValueMap([
['d4/1d/8cd98f00b204e9800998ecf8427e/avatar.jpg', true],
['avatars/d4/1d/8cd98f00b204e9800998ecf8427e/avatar.jpg', true],
]));

$expected = new \OC_Image(\OC::$SERVERROOT . '/tests/data/testavatar.png');

$this->storage->method('file_get_contents')->with('d4/1d/8cd98f00b204e9800998ecf8427e/avatar.jpg')->willReturn($expected->data());
$this->storage->method('file_get_contents')->with('avatars/d4/1d/8cd98f00b204e9800998ecf8427e/avatar.jpg')->willReturn($expected->data());

$this->assertEquals($expected->data(), $this->avatar->get(-1)->data());
}

public function testGetAvatarNoSizeMatch() {
$this->storage->method('file_exists')
->will($this->returnValueMap([
['d4/1d/8cd98f00b204e9800998ecf8427e/avatar.png', true],
['d4/1d/8cd98f00b204e9800998ecf8427e/avatar.32.png', false],
['avatars/d4/1d/8cd98f00b204e9800998ecf8427e/avatar.png', true],
['avatars/d4/1d/8cd98f00b204e9800998ecf8427e/avatar.32.png', false],
]));

$expected = new \OC_Image(\OC::$SERVERROOT . '/tests/data/testavatar.png');
Expand All @@ -106,10 +106,10 @@ public function testGetAvatarNoSizeMatch() {
$this->storage->method('file_get_contents')
->will($this->returnCallback(
function ($path) use ($expected, $expected2) {
if ($path === 'd4/1d/8cd98f00b204e9800998ecf8427e/avatar.png') {
if ($path === 'avatars/d4/1d/8cd98f00b204e9800998ecf8427e/avatar.png') {
return $expected->data();
}
if ($path === 'd4/1d/8cd98f00b204e9800998ecf8427e/avatar.32.png') {
if ($path === 'avatars/d4/1d/8cd98f00b204e9800998ecf8427e/avatar.32.png') {
return $expected2->data();
}
throw new \OCP\Files\NotFoundException();
Expand All @@ -118,7 +118,7 @@ function ($path) use ($expected, $expected2) {

$this->storage->expects($this->once())
->method('file_put_contents')
->with('d4/1d/8cd98f00b204e9800998ecf8427e/avatar.32.png', $expected2->data());
->with('avatars/d4/1d/8cd98f00b204e9800998ecf8427e/avatar.32.png', $expected2->data());

$this->assertEquals($expected2->data(), $this->avatar->get(32)->data());
}
Expand All @@ -130,17 +130,17 @@ public function testExistsNo() {
public function testExiststJPG() {
$this->storage->method('file_exists')
->will($this->returnValueMap([
['d4/1d/8cd98f00b204e9800998ecf8427e/avatar.jpg', true],
['d4/1d/8cd98f00b204e9800998ecf8427e/avatar.png', false],
['avatars/d4/1d/8cd98f00b204e9800998ecf8427e/avatar.jpg', true],
['avatars/d4/1d/8cd98f00b204e9800998ecf8427e/avatar.png', false],
]));
$this->assertTrue($this->avatar->exists());
}

public function testExistsPNG() {
$this->storage->method('file_exists')
->will($this->returnValueMap([
['d4/1d/8cd98f00b204e9800998ecf8427e/avatar.jpg', false],
['d4/1d/8cd98f00b204e9800998ecf8427e/avatar.png', true],
['avatars/d4/1d/8cd98f00b204e9800998ecf8427e/avatar.jpg', false],
['avatars/d4/1d/8cd98f00b204e9800998ecf8427e/avatar.png', true],
]));
$this->assertTrue($this->avatar->exists());
}
Expand All @@ -153,11 +153,11 @@ public function testExistsStorageNotAvailable() {

public function testSetAvatar() {
$this->storage->expects($this->once())->method('rmdir')
->with('d4/1d/8cd98f00b204e9800998ecf8427e');
->with('avatars/d4/1d/8cd98f00b204e9800998ecf8427e');

$image = new \OC_Image(\OC::$SERVERROOT . '/tests/data/testavatar.png');
$this->storage->method('file_put_contents')
->with('d4/1d/8cd98f00b204e9800998ecf8427e/avatar.png', $image->data());
->with('avatars/d4/1d/8cd98f00b204e9800998ecf8427e/avatar.png', $image->data());

// One on remove and once on setting the new avatar
$this->user->expects($this->exactly(2))->method('triggerChange');
Expand Down

0 comments on commit 343f593

Please sign in to comment.