Fix unmerged shares repair with mixed group and direct shares

Whenever a group share is created after a direct share, the stime order
needs to be properly considered in the repair routine, considering that
the direct user share is appended to the $subShares array and breaking
its order.

2 files changed, 57 insertions, 4 deletions

diff --git a/lib/private/Repair/RepairUnmergedShares.php b/lib/private/Repair/RepairUnmergedShares.php
index f14e98c1761..c29de87c4e8 100644
--- a/lib/private/Repair/RepairUnmergedShares.php
+++ b/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')))
@@ -161,13 +161,23 @@ class RepairUnmergedShares implements IRepairStep {
 	 * 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, sorted by stime
+	 * @param array $subShares sub share entries
 	 *
 	 * @return string chosen target name
 	 */
 	private function findBestTargetName($groupShares, $subShares) {
 		$pickedShare = null;
-		// note subShares are sorted by stime from oldest to newest
+		// 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) {
diff --git a/tests/lib/Repair/RepairUnmergedSharesTest.php b/tests/lib/Repair/RepairUnmergedSharesTest.php
index 9fe9a073881..7304bfd6920 100644
--- a/tests/lib/Repair/RepairUnmergedSharesTest.php
+++ b/tests/lib/Repair/RepairUnmergedSharesTest.php
@@ -44,6 +44,9 @@ class RepairUnmergedSharesTest extends TestCase {
 	/** @var \OCP\IDBConnection */
 	private $connection;
 
+	/** @var int */
+	private $lastShareTime;
+
 	protected function setUp() {
 		parent::setUp();
 
@@ -92,6 +95,9 @@ class RepairUnmergedSharesTest extends TestCase {
 				}
 			}));
 
+		// used to generate incremental stimes
+		$this->lastShareTime = time();
+
 		/** @var \OCP\IConfig $config */
 		$this->repair = new RepairUnmergedShares($config, $this->connection, $userManager, $groupManager);
 	}
@@ -108,6 +114,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 +126,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);
@@ -458,6 +465,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],
+				]
+			],
 		];
 	}