Commit c7b97382 authored by Vincent Petry's avatar Vincent Petry Committed by Thomas Müller
Browse files

Creating second share to user who renamed received folder creates a second entry (#25568)

* Add integration tests for double shares with rename in between

* 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.
parent 339ea4db
......@@ -74,7 +74,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) {
......@@ -115,17 +115,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);
}
/**
......@@ -135,9 +140,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);
......@@ -160,6 +166,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);
......
......@@ -83,6 +83,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;
}
......@@ -102,7 +108,7 @@ class MountProviderTest extends \Test\TestCase {
$groupShares = [
$this->makeMockShare(3, 100, 'user2', '/share2', 0),
$this->makeMockShare(4, 100, 'user2', '/share4', 31),
$this->makeMockShare(4, 101, 'user2', '/share4', 31),
$this->makeMockShare(5, 100, 'user1', '/share4', 31),
];
......@@ -141,7 +147,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());
}
......@@ -236,6 +242,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],
],
],
];
}
......
......@@ -46,12 +46,12 @@ trait WebDav {
}
/**
* @Given /^User "([^"]*)" moved file "([^"]*)" to "([^"]*)"$/
* @Given /^User "([^"]*)" moved (file|folder|entry) "([^"]*)" to "([^"]*)"$/
* @param string $user
* @param string $fileSource
* @param string $fileDestination
*/
public function userMovedFile($user, $fileSource, $fileDestination){
public function userMovedFile($user, $entry, $fileSource, $fileDestination){
$fullUrl = substr($this->baseUrl, 0, -4) . $this->davPath;
$headers['Destination'] = $fullUrl . $fileDestination;
$this->response = $this->makeDavRequest($user, "MOVE", $fileSource, $headers);
......@@ -59,12 +59,12 @@ trait WebDav {
}
/**
* @When /^User "([^"]*)" moves file "([^"]*)" to "([^"]*)"$/
* @When /^User "([^"]*)" moves (file|folder|entry) "([^"]*)" to "([^"]*)"$/
* @param string $user
* @param string $fileSource
* @param string $fileDestination
*/
public function userMovesFile($user, $fileSource, $fileDestination){
public function userMovesFile($user, $entry, $fileSource, $fileDestination){
$fullUrl = substr($this->baseUrl, 0, -4) . $this->davPath;
$headers['Destination'] = $fullUrl . $fileDestination;
try {
......
......@@ -838,3 +838,33 @@ Feature: sharing
And as "user0" the folder "merge-test-inside-twogroups-perms (2)" does not exist
And as "user0" the folder "merge-test-inside-twogroups-perms (3)" does not exist
Scenario: Merging shares for recipient when shared from outside with group then user and recipient renames in between
Given As an "admin"
And user "user0" exists
And user "user1" exists
And group "group1" exists
And user "user1" belongs to group "group1"
And user "user0" created a folder "merge-test-outside-groups-renamebeforesecondshare"
When folder "merge-test-outside-groups-renamebeforesecondshare" of user "user0" is shared with group "group1"
And User "user1" moved folder "/merge-test-outside-groups-renamebeforesecondshare" to "/merge-test-outside-groups-renamebeforesecondshare-renamed"
And folder "merge-test-outside-groups-renamebeforesecondshare" of user "user0" is shared with user "user1"
Then as "user1" gets properties of folder "merge-test-outside-groups-renamebeforesecondshare-renamed" with
|{http://owncloud.org/ns}permissions|
And the single response should contain a property "{http://owncloud.org/ns}permissions" with value "SRDNVCK"
And as "user1" the folder "merge-test-outside-groups-renamebeforesecondshare" does not exist
Scenario: Merging shares for recipient when shared from outside with user then group and recipient renames in between
Given As an "admin"
And user "user0" exists
And user "user1" exists
And group "group1" exists
And user "user1" belongs to group "group1"
And user "user0" created a folder "merge-test-outside-groups-renamebeforesecondshare"
When folder "merge-test-outside-groups-renamebeforesecondshare" of user "user0" is shared with user "user1"
And User "user1" moved folder "/merge-test-outside-groups-renamebeforesecondshare" to "/merge-test-outside-groups-renamebeforesecondshare-renamed"
And folder "merge-test-outside-groups-renamebeforesecondshare" of user "user0" is shared with group "group1"
Then as "user1" gets properties of folder "merge-test-outside-groups-renamebeforesecondshare-renamed" with
|{http://owncloud.org/ns}permissions|
And the single response should contain a property "{http://owncloud.org/ns}permissions" with value "SRDNVCK"
And as "user1" the folder "merge-test-outside-groups-renamebeforesecondshare" does not exist
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment