Merge pull request #891 from nextcloud/us_25810

[OC] Fix unmerged shares repair targetdecision

lib/private/Repair/RepairUnmergedShares.php

@@ -93,7 +93,7 @@ class RepairUnmergedShares implements IRepairStep {
 		 */
 		$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')))
@@ -148,6 +148,52 @@ class RepairUnmergedShares implements IRepairStep {
 		return $groupedShares;
 	}
 
+	private function isPotentialDuplicateName($name) {
+		return (preg_match('/\(\d+\)(\.[^\.]+)?$/', $name) === 1);
+	}
+
+	/**
+	 * 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) {
+			return ((int)$a['stime'] - (int)$b['stime']);
+		});
+
+		foreach ($subShares as $subShare) {
+			// skip entries that have parenthesis with numbers
+			if ($this->isPotentialDuplicateName($subShare['file_target'])) {
+				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
@@ -171,7 +217,7 @@ class RepairUnmergedShares implements IRepairStep {
 			return false;
 		}
 
-		$targetPath = $groupShares[0]['file_target'];
+		$targetPath = $this->findBestTargetName($groupShares, $subShares);
 
 		// check whether the user opted out completely of all subshares
 		$optedOut = true;

tests/lib/Repair/RepairUnmergedSharesTest.php

@@ -28,6 +28,8 @@ use OCP\Migration\IOutput;
 use OCP\Migration\IRepairStep;
 use Test\TestCase;
 use OC\Share20\DefaultShareProvider;
+use OCP\IUserManager;
+use OCP\IGroupManager;
 
 /**
  * Tests for repairing invalid shares
@@ -44,6 +46,15 @@ class RepairUnmergedSharesTest extends TestCase {
 	/** @var \OCP\IDBConnection */
 	private $connection;
 
+	/** @var int */
+	private $lastShareTime;
+
+	/** @var IUserManager */
+	private $userManager;
+
+	/** @var IGroupManager */
+	private $groupManager;
+
 	protected function setUp() {
 		parent::setUp();
 
@@ -58,42 +69,14 @@ class RepairUnmergedSharesTest extends TestCase {
 		$this->connection = \OC::$server->getDatabaseConnection();
 		$this->deleteAllShares();
 
-		$user1 = $this->getMock('\OCP\IUser');
-		$user1->expects($this->any())
-			->method('getUID')
-			->will($this->returnValue('user1'));
-
-		$user2 = $this->getMock('\OCP\IUser');
-		$user2->expects($this->any())
-			->method('getUID')
-			->will($this->returnValue('user2'));
-
-		$users = [$user1, $user2];
-
-		$groupManager = $this->getMock('\OCP\IGroupManager');
-		$groupManager->expects($this->any())
-			->method('getUserGroupIds')
-			->will($this->returnValueMap([
-				// owner
-				[$user1, ['samegroup1', 'samegroup2']],
-				// recipient
-				[$user2, ['recipientgroup1', 'recipientgroup2']],
-			]));
+		$this->userManager = $this->getMock('\OCP\IUserManager');
+		$this->groupManager = $this->getMock('\OCP\IGroupManager');
 
-		$userManager = $this->getMock('\OCP\IUserManager');
-		$userManager->expects($this->once())
-			->method('countUsers')
-			->will($this->returnValue([2]));
-		$userManager->expects($this->once())
-			->method('callForAllUsers')
-			->will($this->returnCallback(function(\Closure $closure) use ($users) {
-				foreach ($users as $user) {
-					$closure($user);
-				}
-			}));
+		// used to generate incremental stimes
+		$this->lastShareTime = time();
 
 		/** @var \OCP\IConfig $config */
-		$this->repair = new RepairUnmergedShares($config, $this->connection, $userManager, $groupManager);
+		$this->repair = new RepairUnmergedShares($config, $this->connection, $this->userManager, $this->groupManager);
 	}
 
 	protected function tearDown() {
@@ -108,6 +91,7 @@ class RepairUnmergedSharesTest extends TestCase {
 	}
 
 	private function createShare($type, $sourceId, $recipient, $targetName, $permissions, $parentId = null) {
+		$this->lastShareTime += 100;
 		$qb = $this->connection->getQueryBuilder();
 		$values = [
 			'share_type' => $qb->expr()->literal($type),
@@ -119,7 +103,7 @@ class RepairUnmergedSharesTest extends TestCase {
 			'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);
@@ -204,7 +188,7 @@ class RepairUnmergedSharesTest extends TestCase {
 			[
 				// #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],
@@ -218,7 +202,7 @@ class RepairUnmergedSharesTest extends TestCase {
 				[
 					['/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
@@ -228,6 +212,54 @@ class RepairUnmergedSharesTest extends TestCase {
 			[
 				// #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
@@ -251,7 +283,7 @@ class RepairUnmergedSharesTest extends TestCase {
 				]
 			],
 			[
-				// #4 bogus share:
+				// #6 bogus share:
 				// - outsider shares with group1, group2
 				// - one subshare for each group share
 				// - non-matching targets
@@ -276,7 +308,7 @@ class RepairUnmergedSharesTest extends TestCase {
 				]
 			],
 			[
-				// #5 bogus share:
+				// #7 bogus share:
 				// - outsider shares with group1, group2
 				// - one subshare for each group share
 				// - non-matching targets
@@ -301,7 +333,7 @@ class RepairUnmergedSharesTest extends TestCase {
 				]
 			],
 			[
-				// #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
@@ -329,7 +361,7 @@ class RepairUnmergedSharesTest extends TestCase {
 				]
 			],
 			[
-				// #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
@@ -350,7 +382,7 @@ class RepairUnmergedSharesTest extends TestCase {
 				]
 			],
 			[
-				// #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
 				[
@@ -368,7 +400,7 @@ class RepairUnmergedSharesTest extends TestCase {
 				]
 			],
 			[
-				// #9 legitimate shares:
+				// #11 legitimate shares:
 				// - group share with same group
 				// - group share with other group
 				// - user share where recipient renamed
@@ -392,7 +424,7 @@ class RepairUnmergedSharesTest extends TestCase {
 				]
 			],
 			[
-				// #10 legitimate share:
+				// #12 legitimate share:
 				// - outsider shares with group and user directly with different permissions
 				// - no subshares
 				// - same targets
@@ -410,6 +442,42 @@ class RepairUnmergedSharesTest extends TestCase {
 					['/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],
+				]
+			],
 		];
 	}
 
@@ -419,6 +487,38 @@ class RepairUnmergedSharesTest extends TestCase {
 	 * @dataProvider sharesDataProvider
 	 */
 	public function testMergeGroupShares($shares, $expectedShares) {
+		$user1 = $this->getMock('\OCP\IUser');
+		$user1->expects($this->any())
+			->method('getUID')
+			->will($this->returnValue('user1'));
+
+		$user2 = $this->getMock('\OCP\IUser');
+		$user2->expects($this->any())
+			->method('getUID')
+			->will($this->returnValue('user2'));
+
+		$users = [$user1, $user2];
+
+		$this->groupManager->expects($this->any())
+			->method('getUserGroupIds')
+			->will($this->returnValueMap([
+				// owner
+				[$user1, ['samegroup1', 'samegroup2']],
+				// recipient
+				[$user2, ['recipientgroup1', 'recipientgroup2']],
+			]));
+
+		$this->userManager->expects($this->once())
+			->method('countUsers')
+			->will($this->returnValue([2]));
+		$this->userManager->expects($this->once())
+			->method('callForAllUsers')
+			->will($this->returnCallback(function(\Closure $closure) use ($users) {
+				foreach ($users as $user) {
+					$closure($user);
+				}
+			}));
+
 		$shareIds = [];
 
 		foreach ($shares as $share) {
@@ -445,5 +545,30 @@ class RepairUnmergedSharesTest extends TestCase {
 			$this->assertEquals($expectedShare[1], $share['permissions']);
 		}
 	}
+
+	public function duplicateNamesProvider() {
+		return [
+			// matching
+			['filename (1).txt', true],
+			['folder (2)', true],
+			['filename (1)(2).txt', true],
+			// non-matching
+			['filename ().txt', false],
+			['folder ()', false],
+			['folder (1x)', false],
+			['folder (x1)', false],
+			['filename (a)', false],
+			['filename (1).', false],
+			['filename (1).txt.txt', false],
+			['filename (1)..txt', false],
+		];
+	}
+
+	/**
+	 * @dataProvider duplicateNamesProvider
+	 */
+	public function testIsPotentialDuplicateName($name, $expectedResult) {
+		$this->assertEquals($expectedResult, $this->invokePrivate($this->repair, 'isPotentialDuplicateName', [$name]));
+	}
 }