From 21907c4f3ea098162aee8a5dd737d6541ee16ab1 Mon Sep 17 00:00:00 2001
From: Vincent Petry <pvince81@owncloud.com>
Date: Fri, 22 Jul 2016 10:08:05 +0200
Subject: [PATCH] Fix RepairUnmergedShares to not skip valid repair cases

The repair step was a bit overeager to skip repairing so it missed the
case where a group share exists without subshares but with an
additional direct user share.
---
 lib/private/Repair/RepairUnmergedShares.php   | 27 +++++++-----
 tests/lib/Repair/RepairUnmergedSharesTest.php | 44 ++++++++++++++++++-
 2 files changed, 58 insertions(+), 13 deletions(-)

diff --git a/lib/private/Repair/RepairUnmergedShares.php b/lib/private/Repair/RepairUnmergedShares.php
index 15a4762da79..353877bb873 100644
--- a/lib/private/Repair/RepairUnmergedShares.php
+++ b/lib/private/Repair/RepairUnmergedShares.php
@@ -158,6 +158,10 @@ class RepairUnmergedShares implements IRepairStep {
 	 * @return boolean false if the share was not repaired, true if it was
 	 */
 	private function fixThisShare($groupShares, $subShares) {
+		if (empty($subShares)) {
+			return false;
+		}
+
 		$groupSharesById = [];
 		foreach ($groupShares as $groupShare) {
 			$groupSharesById[$groupShare['id']] = $groupShare;
@@ -253,27 +257,28 @@ class RepairUnmergedShares implements IRepairStep {
 			return;
 		}
 
+		// get all subshares grouped by item source
 		$subSharesByItemSource = $this->getSharesWithUser(DefaultShareProvider::SHARE_TYPE_USERGROUP, [$user->getUID()]);
-		if (empty($subSharesByItemSource)) {
-			// nothing to repair for this user
+
+		// because sometimes one wants to give the user more permissions than the group share
+		$userSharesByItemSource = $this->getSharesWithUser(Constants::SHARE_TYPE_USER, [$user->getUID()]);
+
+		if (empty($subSharesByItemSource) && empty($userSharesByItemSource)) {
+			// nothing to repair for this user, no need to do extra queries
 			return;
 		}
 
 		$groupSharesByItemSource = $this->getSharesWithUser(Constants::SHARE_TYPE_GROUP, $groups);
-		if (empty($groupSharesByItemSource)) {
-			// shouldn't happen, those invalid shares must be cleant already by RepairInvalidShares
+		if (empty($groupSharesByItemSource) && empty($userSharesByItemSource)) {
+			// nothing to repair for this user
 			return;
 		}
 
-		// because sometimes one wants to give the user more permissions than the group share
-		$userSharesByItemSource = $this->getSharesWithUser(Constants::SHARE_TYPE_USER, [$user->getUID()]);
-
 		foreach ($groupSharesByItemSource as $itemSource => $groupShares) {
-			if (!isset($subSharesByItemSource[$itemSource])) {
-				// no subshares for this item source, skip it
-				continue;
+			$subShares = [];
+			if (isset($subSharesByItemSource[$itemSource])) {
+				$subShares = $subSharesByItemSource[$itemSource];
 			}
-			$subShares = $subSharesByItemSource[$itemSource];
 
 			if (isset($userSharesByItemSource[$itemSource])) {
 				// add it to the subshares to get a similar treatment
diff --git a/tests/lib/Repair/RepairUnmergedSharesTest.php b/tests/lib/Repair/RepairUnmergedSharesTest.php
index 20dabac2e6b..fe9b3e5b96f 100644
--- a/tests/lib/Repair/RepairUnmergedSharesTest.php
+++ b/tests/lib/Repair/RepairUnmergedSharesTest.php
@@ -329,7 +329,28 @@ class RepairUnmergedSharesTest extends TestCase {
 				]
 			],
 			[
-				// #7 legitimate share with own group:
+				// #7 bogus share:
+				// - outsider shares with group1 and also user2
+				// - no subshare at all
+				// - one extra share entry for direct share to user2
+				// - non-matching targets
+				// - user share has more permissions
+				[
+					[Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup1', '/test', 1],
+					[Constants::SHARE_TYPE_USER, 123, 'user2', '/test (2)', 31],
+					// different unrelated share
+					[Constants::SHARE_TYPE_GROUP, 456, 'recipientgroup1', '/test (5)', 31],
+				],
+				[
+					['/test', 1],
+					// user share repaired
+					['/test', 31],
+					// leave unrelated alone
+					['/test (5)', 31],
+				]
+			],
+			[
+				// #8 legitimate share with own group:
 				// - insider shares with both groups the user is already in
 				// - no subshares in this case
 				[
@@ -347,7 +368,7 @@ class RepairUnmergedSharesTest extends TestCase {
 				]
 			],
 			[
-				// #7 legitimate shares:
+				// #9 legitimate shares:
 				// - group share with same group
 				// - group share with other group
 				// - user share where recipient renamed
@@ -370,6 +391,25 @@ class RepairUnmergedSharesTest extends TestCase {
 					['/test (4)', 31],
 				]
 			],
+			[
+				// #10 legitimate share:
+				// - outsider shares with group and user directly with different permissions
+				// - no subshares
+				// - same targets
+				[
+					[Constants::SHARE_TYPE_GROUP, 123, 'samegroup1', '/test', 1],
+					[Constants::SHARE_TYPE_USER, 123, 'user3', '/test', 31],
+					// different unrelated share
+					[Constants::SHARE_TYPE_GROUP, 456, 'recipientgroup1', '/test (4)', 31],
+				],
+				[
+					// leave all alone
+					['/test', 1],
+					['/test', 31],
+					// leave unrelated alone
+					['/test (4)', 31],
+				]
+			],
 		];
 	}
 
-- 
2.39.5