Make share target consistent when grouping group share with user share

In some situations, a group share is created before a user share, and
the recipient renamed the received share before the latter is created.
In this situation, the "file_target" was already modified and the second
created share must align to the already renamed share.

To achieve this, the MountProvider now groups only by "item_source"
value and sorts by share time. This makes it so that the least recent
share is selected as super-share and its "file_target" value is then
adjusted in all grouped shares.

This fixes the issue where this situation would have different
"file_target" values resulting in two shared folders appearing instead
of one.

2 files changed, 55 insertions, 12 deletions

diff --git a/apps/files_sharing/lib/MountProvider.php b/apps/files_sharing/lib/MountProvider.php
index 368671d9cfd..284728597cd 100644
--- a/apps/files_sharing/lib/MountProvider.php
+++ b/apps/files_sharing/lib/MountProvider.php
@@ -75,7 +75,7 @@ class MountProvider implements IMountProvider {
 			return $share->getPermissions() > 0 && $share->getShareOwner() !== $user->getUID();
 		});
 
-		$superShares = $this->buildSuperShares($shares);
+		$superShares = $this->buildSuperShares($shares, $user);
 
 		$mounts = [];
 		foreach ($superShares as $share) {
@@ -116,17 +116,22 @@ class MountProvider implements IMountProvider {
 			if (!isset($tmp[$share->getNodeId()])) {
 				$tmp[$share->getNodeId()] = [];
 			}
-			$tmp[$share->getNodeId()][$share->getTarget()][] = $share;
+			$tmp[$share->getNodeId()][] = $share;
 		}
 
 		$result = [];
-		foreach ($tmp as $tmp2) {
-			foreach ($tmp2 as $item) {
-				$result[] = $item;
-			}
+		// sort by stime, the super share will be based on the least recent share
+		foreach ($tmp as &$tmp2) {
+			@usort($tmp2, function($a, $b) {
+				if ($a->getShareTime() < $b->getShareTime()) {
+					return -1;
+				}
+				return 1;
+			});
+			$result[] = $tmp2;
 		}
 
-		return $result;
+		return array_values($result);
 	}
 
 	/**
@@ -136,9 +141,10 @@ class MountProvider implements IMountProvider {
 	 * of all shares within the group.
 	 *
 	 * @param \OCP\Share\IShare[] $allShares
+	 * @param \OCP\IUser $user user
 	 * @return array Tuple of [superShare, groupedShares]
 	 */
-	private function buildSuperShares(array $allShares) {
+	private function buildSuperShares(array $allShares, \OCP\IUser $user) {
 		$result = [];
 
 		$groupedShares = $this->groupShares($allShares);
@@ -161,6 +167,11 @@ class MountProvider implements IMountProvider {
 			$permissions = 0;
 			foreach ($shares as $share) {
 				$permissions |= $share->getPermissions();
+				if ($share->getTarget() !== $superShare->getTarget()) {
+					// adjust target, for database consistency
+					$share->setTarget($superShare->getTarget());
+					$this->shareManager->moveShare($share, $user->getUID());
+				}
 			}
 
 			$superShare->setPermissions($permissions);
diff --git a/apps/files_sharing/tests/MountProviderTest.php b/apps/files_sharing/tests/MountProviderTest.php
index 95e2140897b..576f05d565d 100644
--- a/apps/files_sharing/tests/MountProviderTest.php
+++ b/apps/files_sharing/tests/MountProviderTest.php
@@ -85,6 +85,12 @@ class MountProviderTest extends \Test\TestCase {
 		$share->expects($this->any())
 			->method('getNodeId')
 			->will($this->returnValue($nodeId));
+		$share->expects($this->any())
+			->method('getShareTime')
+			->will($this->returnValue(
+				// compute share time based on id, simulating share order
+				new \DateTime('@' . (1469193980 + 1000 * $id))
+			));
 		return $share;
 	}
 
@@ -101,9 +107,9 @@ class MountProviderTest extends \Test\TestCase {
 			$this->makeMockShare(2, 100, 'user2', '/share2', 31),
 		];
 		$groupShares = [
-			$this->makeMockShare(3, 100, 'user2', '/share2', 0),
-			$this->makeMockShare(4, 100, 'user2', '/share4', 31),
-			$this->makeMockShare(5, 100, 'user1', '/share4', 31),
+			$this->makeMockShare(3, 100, 'user2', '/share2', 0), 
+			$this->makeMockShare(4, 101, 'user2', '/share4', 31), 
+			$this->makeMockShare(5, 100, 'user1', '/share4', 31), 
 		];
 		$this->user->expects($this->any())
 			->method('getUID')
@@ -134,7 +140,7 @@ class MountProviderTest extends \Test\TestCase {
 		$mountedShare2 = $mounts[1]->getShare();
 		$this->assertEquals('4', $mountedShare2->getId());
 		$this->assertEquals('user2', $mountedShare2->getShareOwner());
-		$this->assertEquals(100, $mountedShare2->getNodeId());
+		$this->assertEquals(101, $mountedShare2->getNodeId());
 		$this->assertEquals('/share4', $mountedShare2->getTarget());
 		$this->assertEquals(31, $mountedShare2->getPermissions());
 	}
@@ -229,6 +235,32 @@ class MountProviderTest extends \Test\TestCase {
 					// no received share since "user1" opted out
 				],
 			],
+			// #7: share as outsider with "group1" and "user1" where recipient renamed in between
+			[
+				[
+					[1, 100, 'user2', '/share2-renamed', 31], 
+				],
+				[
+					[2, 100, 'user2', '/share2', 31], 
+				],
+				[
+					// use target of least recent share
+					['1', 100, 'user2', '/share2-renamed', 31],
+				],
+			],
+			// #8: share as outsider with "group1" and "user1" where recipient renamed in between
+			[
+				[
+					[2, 100, 'user2', '/share2', 31], 
+				],
+				[
+					[1, 100, 'user2', '/share2-renamed', 31], 
+				],
+				[
+					// use target of least recent share
+					['1', 100, 'user2', '/share2-renamed', 31],
+				],
+			],
 		];
 	}