Skip to content

Fix RepairUnmergedShares to not skip valid repair cases #25567

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
PVince81 merged 1 commit into from
Jul 22, 2016
Merged

Fix RepairUnmergedShares to not skip valid repair cases #25567

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 16 additions & 11 deletions lib/private/Repair/RepairUnmergedShares.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,10 @@ private function getSharesWithUser($shareType, $shareWiths) {
* @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;
Expand Down Expand Up @@ -253,27 +257,28 @@ private function fixUnmergedShares(IOutput $out, IUser $user) {
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
Expand Down
44 changes: 42 additions & 2 deletions tests/lib/Repair/RepairUnmergedSharesTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,28 @@ public function sharesDataProvider() {
]
],
[
// #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
[
Expand All @@ -347,7 +368,7 @@ public function sharesDataProvider() {
]
],
[
// #7 legitimate shares:
// #9 legitimate shares:
// - group share with same group
// - group share with other group
// - user share where recipient renamed
Expand All @@ -370,6 +391,25 @@ public function sharesDataProvider() {
['/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],
]
],
];
}

Expand Down