Skip to content

Properly handle deleted users in UserMountCache #27028

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
PVince81 merged 1 commit into from
Jan 30, 2017
Merged

Properly handle deleted users in UserMountCache #27028

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 28 additions & 3 deletions lib/private/Files/Config/UserMountCache.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,9 +170,34 @@ private function removeFromCache(ICachedMountInfo $mount) {

private function dbRowToMountInfo(array $row) {
$user = $this->userManager->get($row['user_id']);
if ($user === null) {
// user does not exist any more, delete all mounts of that user directly

@jvillafanez jvillafanez Jan 27, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This feels misplaced. I'm talking strictly about the code, not the behaviour. I'd expect the function to convert the row, not to delete things.

Maybe adding a private function deleteSharesOfMissingUser or similar, and move it to the getMountsForUser function just before starting the conversion might look better.

@PVince81 PVince81 Jan 27, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The bug in question was from yet another code path using dbRowToMountInfo.

Another alternative would be to use a repair step I guess.

@jvillafanez jvillafanez Jan 27, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't like to use a repair step for this because this could happen often and we can't keep on repairing over and over.

I don't have better alternatives, so let's say "this is private stuff" 😄

$builder = $this->connection->getQueryBuilder();
$query = $builder->delete('mounts')
->where($builder->expr()->eq('user_id', $builder->createNamedParameter($row['user_id'])));
$query->execute();
return null;
}
return new CachedMountInfo($user, (int)$row['storage_id'], (int)$row['root_id'], $row['mount_point']);
}

/**
* Convert DB rows to CachedMountInfo
*
* @param array $rows DB rows
* @return CachedMountInfo[]
*/
private function convertRows($rows) {
$mountInfos = [];
foreach ($rows as $row) {
$mountInfo = $this->dbRowToMountInfo($row);
if (!is_null($mountInfo)) {
$mountInfos[] = $mountInfo;
}
}
return $mountInfos;
}

/**
* @param IUser $user
* @return ICachedMountInfo[]
Expand All @@ -186,7 +211,7 @@ public function getMountsForUser(IUser $user) {

$rows = $query->execute()->fetchAll();

$this->mountsForUsers[$user->getUID()] = array_map([$this, 'dbRowToMountInfo'], $rows);
$this->mountsForUsers[$user->getUID()] = $this->convertRows($rows);
}
return $this->mountsForUsers[$user->getUID()];
}
Expand All @@ -203,7 +228,7 @@ public function getMountsForStorageId($numericStorageId) {

$rows = $query->execute()->fetchAll();

return array_map([$this, 'dbRowToMountInfo'], $rows);
return $this->convertRows($rows);
}

/**
Expand All @@ -218,7 +243,7 @@ public function getMountsForRootId($rootFileId) {

$rows = $query->execute()->fetchAll();

return array_map([$this, 'dbRowToMountInfo'], $rows);
return $this->convertRows($rows);
}

/**
Expand Down
26 changes: 26 additions & 0 deletions tests/lib/Files/Config/UserMountCacheTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ public function setUp() {
$userBackend = new Dummy();
$userBackend->createUser('u1', '');
$userBackend->createUser('u2', '');
$userBackend->createUser('u3', '');
$this->userManager->registerBackend($userBackend);
$this->cache = new \OC\Files\Config\UserMountCache($this->connection, $this->userManager, $this->createMock('\OC\Log'));
}
Expand Down Expand Up @@ -183,15 +184,19 @@ public function testChangeMounts() {
public function testGetMountsForUser() {
$user1 = $this->userManager->get('u1');
$user2 = $this->userManager->get('u2');
$user3 = $this->userManager->get('u3');

$mount1 = new MountPoint($this->getStorage(1, 2), '/foo/');
$mount2 = new MountPoint($this->getStorage(3, 4), '/bar/');

$this->cache->registerMounts($user1, [$mount1, $mount2]);
$this->cache->registerMounts($user2, [$mount2]);
$this->cache->registerMounts($user3, [$mount2]);

$this->clearCache();

$user3->delete();

$cachedMounts = $this->cache->getMountsForUser($user1);

$this->assertCount(2, $cachedMounts);
Expand All @@ -204,6 +209,9 @@ public function testGetMountsForUser() {
$this->assertEquals($user1, $cachedMounts[1]->getUser());
$this->assertEquals(4, $cachedMounts[1]->getRootId());
$this->assertEquals(3, $cachedMounts[1]->getStorageId());

$cachedMounts = $this->cache->getMountsForUser($user3);
$this->assertEmpty($cachedMounts);
}

public function testGetMountsByStorageId() {
Expand Down Expand Up @@ -372,4 +380,22 @@ public function testGetMountsForFileIdSubFolderMountOutside() {

$this->assertCount(0, $cachedMounts);
}

public function testGetMountsForFileIdDeletedUser() {
$user1 = $this->userManager->get('u1');

$rootId = $this->createCacheEntry('', 2);

$mount1 = new MountPoint($this->getStorage(2, $rootId), '/foo/');

$this->cache->registerMounts($user1, [$mount1]);

$user1->delete();

$this->clearCache();

$cachedMounts = $this->cache->getMountsForFileId($rootId);

$this->assertEmpty($cachedMounts);
}
}