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.

diff --git a/lib/private/Repair/RepairUnmergedShares.php b/lib/private/Repair/RepairUnmergedShares.php
index 15a4762da79cc8d2461a2da3ffcaa0d869803af1..353877bb8737d49c99e3da218da236b6b0cfb093 100644 (file)
--- 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 20dabac2e6bc2c1807f77c0bcd3973e7ebcf881d..fe9b3e5b96f37ce2ce52e947b6fd951162092919 100644 (file)
--- 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],
+                               ]
+                       ],
                ];
        }