Skip to content

Commit

Permalink
Merge pull request #25812 from owncloud/stable9.1-fix-unmerged-shares…
Browse files Browse the repository at this point in the history 
…-repair-targetdecision

[stable9.1] Fix unmerged shares repair targetdecision
  • Loading branch information
Vincent Petry authored Aug 17, 2016
2 parents b7356d8 + 8cca364 commit 64d7887
Show file tree
Hide file tree
Showing 2 changed files with 151 additions and 12 deletions.
52 changes: 50 additions & 2 deletions lib/private/Repair/RepairUnmergedShares.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ private function buildPreparedQueries() {
*/
$query = $this->connection->getQueryBuilder();
$query
->select('item_source', 'id', 'file_target', 'permissions', 'parent', 'share_type')
->select('item_source', 'id', 'file_target', 'permissions', 'parent', 'share_type', 'stime')
->from('share')
->where($query->expr()->eq('share_type', $query->createParameter('shareType')))
->andWhere($query->expr()->in('share_with', $query->createParameter('shareWiths')))
Expand Down Expand Up @@ -148,6 +148,54 @@ private function getSharesWithUser($shareType, $shareWiths) {
return $groupedShares;
}

/**
* Decide on the best target name based on all group shares and subshares,
* goal is to increase the likeliness that the chosen name matches what
* the user is expecting.
*
* For this, we discard the entries with parenthesis "(2)".
* In case the user also renamed the duplicates to a legitimate name, this logic
* will still pick the most recent one as it's the one the user is most likely to
* remember renaming.
*
* If no suitable subshare is found, use the least recent group share instead.
*
* @param array $groupShares group share entries
* @param array $subShares sub share entries
*
* @return string chosen target name
*/
private function findBestTargetName($groupShares, $subShares) {
$pickedShare = null;
// sort by stime, this also properly sorts the direct user share if any
@usort($subShares, function($a, $b) {
if ($a['stime'] < $b['stime']) {
return -1;
} else if ($a['stime'] > $b['stime']) {
return 1;
}

return 0;
});

foreach ($subShares as $subShare) {
// skip entries that have parenthesis with numbers
if (preg_match('/\([0-9]*\)/', $subShare['file_target']) === 1) {
continue;
}
// pick any share found that would match, the last being the most recent
$pickedShare = $subShare;
}

// no suitable subshare found
if ($pickedShare === null) {
// use least recent group share target instead
$pickedShare = $groupShares[0];
}

return $pickedShare['file_target'];
}

/**
* Fix the given received share represented by the set of group shares
* and matching sub shares
Expand All @@ -171,7 +219,7 @@ private function fixThisShare($groupShares, $subShares) {
return false;
}

$targetPath = $groupShares[0]['file_target'];
$targetPath = $this->findBestTargetName($groupShares, $subShares);

// check whether the user opted out completely of all subshares
$optedOut = true;
Expand Down
111 changes: 101 additions & 10 deletions tests/lib/Repair/RepairUnmergedSharesTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ class RepairUnmergedSharesTest extends TestCase {
/** @var \OCP\IDBConnection */
private $connection;

/** @var int */
private $lastShareTime;

protected function setUp() {
parent::setUp();

Expand Down Expand Up @@ -92,6 +95,9 @@ protected function setUp() {
}
}));

// used to generate incremental stimes
$this->lastShareTime = time();

/** @var \OCP\IConfig $config */
$this->repair = new RepairUnmergedShares($config, $this->connection, $userManager, $groupManager);
}
Expand All @@ -108,6 +114,7 @@ protected function deleteAllShares() {
}

private function createShare($type, $sourceId, $recipient, $targetName, $permissions, $parentId = null) {
$this->lastShareTime += 100;
$qb = $this->connection->getQueryBuilder();
$values = [
'share_type' => $qb->expr()->literal($type),
Expand All @@ -119,7 +126,7 @@ private function createShare($type, $sourceId, $recipient, $targetName, $permiss
'file_source' => $qb->expr()->literal($sourceId),
'file_target' => $qb->expr()->literal($targetName),
'permissions' => $qb->expr()->literal($permissions),
'stime' => $qb->expr()->literal(time()),
'stime' => $qb->expr()->literal($this->lastShareTime),
];
if ($parentId !== null) {
$values['parent'] = $qb->expr()->literal($parentId);
Expand Down Expand Up @@ -204,7 +211,7 @@ public function sharesDataProvider() {
[
// #2 bogus share
// - outsider shares with group1, group2
// - one subshare for each group share
// - one subshare for each group share, both with parenthesis
// - but the targets do not match when grouped
[
[Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup1', '/test', 31],
Expand All @@ -218,7 +225,7 @@ public function sharesDataProvider() {
[
['/test', 31],
['/test', 31],
// reset to original name
// reset to original name as the sub-names have parenthesis
['/test', 31],
['/test', 31],
// leave unrelated alone
Expand All @@ -228,6 +235,54 @@ public function sharesDataProvider() {
[
// #3 bogus share
// - outsider shares with group1, group2
// - one subshare for each group share, both renamed manually
// - but the targets do not match when grouped
[
[Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup1', '/test', 31],
[Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup2', '/test', 31],
// child of the previous ones
[DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/test_renamed (1 legit paren)', 31, 0],
[DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/test_renamed (2 legit paren)', 31, 1],
// different unrelated share
[Constants::SHARE_TYPE_GROUP, 456, 'recipientgroup1', '/test (4)', 31],
],
[
['/test', 31],
['/test', 31],
// reset to less recent subshare name
['/test_renamed (2 legit paren)', 31],
['/test_renamed (2 legit paren)', 31],
// leave unrelated alone
['/test (4)', 31],
]
],
[
// #4 bogus share
// - outsider shares with group1, group2
// - one subshare for each group share, one with parenthesis
// - but the targets do not match when grouped
[
[Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup1', '/test', 31],
[Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup2', '/test', 31],
// child of the previous ones
[DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/test (2)', 31, 0],
[DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/test_renamed', 31, 1],
// different unrelated share
[Constants::SHARE_TYPE_GROUP, 456, 'recipientgroup1', '/test (4)', 31],
],
[
['/test', 31],
['/test', 31],
// reset to less recent subshare name but without parenthesis
['/test_renamed', 31],
['/test_renamed', 31],
// leave unrelated alone
['/test (4)', 31],
]
],
[
// #5 bogus share
// - outsider shares with group1, group2
// - one subshare for each group share
// - first subshare not renamed (as in real world scenario)
// - but the targets do not match when grouped
Expand All @@ -251,7 +306,7 @@ public function sharesDataProvider() {
]
],
[
// #4 bogus share:
// #6 bogus share:
// - outsider shares with group1, group2
// - one subshare for each group share
// - non-matching targets
Expand All @@ -276,7 +331,7 @@ public function sharesDataProvider() {
]
],
[
// #5 bogus share:
// #7 bogus share:
// - outsider shares with group1, group2
// - one subshare for each group share
// - non-matching targets
Expand All @@ -301,7 +356,7 @@ public function sharesDataProvider() {
]
],
[
// #6 bogus share:
// #8 bogus share:
// - outsider shares with group1, group2 and also user2
// - one subshare for each group share
// - one extra share entry for direct share to user2
Expand Down Expand Up @@ -329,7 +384,7 @@ public function sharesDataProvider() {
]
],
[
// #7 bogus share:
// #9 bogus share:
// - outsider shares with group1 and also user2
// - no subshare at all
// - one extra share entry for direct share to user2
Expand All @@ -350,7 +405,7 @@ public function sharesDataProvider() {
]
],
[
// #8 legitimate share with own group:
// #10 legitimate share with own group:
// - insider shares with both groups the user is already in
// - no subshares in this case
[
Expand All @@ -368,7 +423,7 @@ public function sharesDataProvider() {
]
],
[
// #9 legitimate shares:
// #11 legitimate shares:
// - group share with same group
// - group share with other group
// - user share where recipient renamed
Expand All @@ -392,7 +447,7 @@ public function sharesDataProvider() {
]
],
[
// #10 legitimate share:
// #12 legitimate share:
// - outsider shares with group and user directly with different permissions
// - no subshares
// - same targets
Expand All @@ -410,6 +465,42 @@ public function sharesDataProvider() {
['/test (4)', 31],
]
],
[
// #13 bogus share:
// - outsider shares with group1, user2 and then group2
// - user renamed share as soon as it arrived before the next share (order)
// - one subshare for each group share
// - one extra share entry for direct share to user2
// - non-matching targets
[
// first share with group
[Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup1', '/test', 31],
// recipient renames
[DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/first', 31, 0],
// then direct share, user renames too
[Constants::SHARE_TYPE_USER, 123, 'user2', '/second', 31],
// another share with the second group
[Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup2', '/test', 31],
// use renames it
[DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/third', 31, 1],
// different unrelated share
[Constants::SHARE_TYPE_GROUP, 456, 'recipientgroup1', '/test (5)', 31],
],
[
// group share with group1 left alone
['/test', 31],
// first subshare repaired
['/third', 31],
// direct user share repaired
['/third', 31],
// group share with group2 left alone
['/test', 31],
// second subshare repaired
['/third', 31],
// leave unrelated alone
['/test (5)', 31],
]
],
];
}

Expand Down

0 comments on commit 64d7887

Please sign in to comment.