From bb9814d052e899f6e9cf8313c7e2b1affeadca88 Mon Sep 17 00:00:00 2001 From: Daniel Calviño Sánchez Date: Fri, 25 Oct 2019 01:13:23 +0200 Subject: Replace fully qualified name with alias MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Daniel Calviño Sánchez --- build/integration/features/bootstrap/Sharing.php | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/build/integration/features/bootstrap/Sharing.php b/build/integration/features/bootstrap/Sharing.php index 4e62ea73c5d..d4a31f919d9 100644 --- a/build/integration/features/bootstrap/Sharing.php +++ b/build/integration/features/bootstrap/Sharing.php @@ -25,6 +25,7 @@ * along with this program. If not, see . * */ +use Behat\Gherkin\Node\TableNode; use GuzzleHttp\Client; use PHPUnit\Framework\Assert; use Psr\Http\Message\ResponseInterface; @@ -54,7 +55,7 @@ trait Sharing { /** * @Given /^as "([^"]*)" creating a share with$/ * @param string $user - * @param \Behat\Gherkin\Node\TableNode|null $body + * @param TableNode|null $body */ public function asCreatingAShareWith($user, $body) { $fullUrl = $this->baseUrl . "v{$this->apiVersion}.php/apps/files_sharing/api/v{$this->sharingApiVersion}/shares"; @@ -70,7 +71,7 @@ trait Sharing { $options['auth'] = [$user, $this->regularUser]; } - if ($body instanceof \Behat\Gherkin\Node\TableNode) { + if ($body instanceof TableNode) { $fd = $body->getRowsHash(); if (array_key_exists('expireDate', $fd)){ $dateModification = $fd['expireDate']; @@ -104,7 +105,7 @@ trait Sharing { /** * @When /^creating a share with$/ - * @param \Behat\Gherkin\Node\TableNode|null $body + * @param TableNode|null $body */ public function creatingShare($body) { $this->asCreatingAShareWith($this->currentUser, $body); @@ -187,7 +188,7 @@ trait Sharing { /** * @When /^Updating last share with$/ - * @param \Behat\Gherkin\Node\TableNode|null $body + * @param TableNode|null $body */ public function updatingLastShare($body) { $share_id = (string) $this->lastShareData->data[0]->id; @@ -204,7 +205,7 @@ trait Sharing { $options['auth'] = [$this->currentUser, $this->regularUser]; } - if ($body instanceof \Behat\Gherkin\Node\TableNode) { + if ($body instanceof TableNode) { $fd = $body->getRowsHash(); if (array_key_exists('expireDate', $fd)){ $dateModification = $fd['expireDate']; @@ -457,10 +458,10 @@ trait Sharing { /** * @Then /^Share fields of last share match with$/ - * @param \Behat\Gherkin\Node\TableNode|null $body + * @param TableNode|null $body */ public function checkShareFields($body){ - if ($body instanceof \Behat\Gherkin\Node\TableNode) { + if ($body instanceof TableNode) { $fd = $body->getRowsHash(); foreach($fd as $field => $value) { @@ -545,11 +546,11 @@ trait Sharing { /** * @When /^getting sharees for$/ - * @param \Behat\Gherkin\Node\TableNode $body + * @param TableNode $body */ public function whenGettingShareesFor($body) { $url = '/apps/files_sharing/api/v1/sharees'; - if ($body instanceof \Behat\Gherkin\Node\TableNode) { + if ($body instanceof TableNode) { $parameters = []; foreach ($body->getRowsHash() as $key => $value) { $parameters[] = $key . '=' . $value; @@ -566,7 +567,7 @@ trait Sharing { * @Then /^"([^"]*)" sharees returned (are|is empty)$/ * @param string $shareeType * @param string $isEmpty - * @param \Behat\Gherkin\Node\TableNode|null $shareesList + * @param TableNode|null $shareesList */ public function thenListOfSharees($shareeType, $isEmpty, $shareesList = null) { if ($isEmpty !== 'is empty') { -- cgit v1.2.3 From eaa98ed343e4663cf9281484b26531fecdd175ab Mon Sep 17 00:00:00 2001 From: Daniel Calviño Sánchez Date: Fri, 25 Oct 2019 01:54:17 +0200 Subject: Add integration tests for getting shares including subfiles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Daniel Calviño Sánchez --- build/integration/features/bootstrap/Sharing.php | 108 ++++++++++++ .../sharing_features/sharing-v1-part2.feature | 186 +++++++++++++++++++++ 2 files changed, 294 insertions(+) diff --git a/build/integration/features/bootstrap/Sharing.php b/build/integration/features/bootstrap/Sharing.php index d4a31f919d9..42d2f03221f 100644 --- a/build/integration/features/bootstrap/Sharing.php +++ b/build/integration/features/bootstrap/Sharing.php @@ -480,6 +480,114 @@ trait Sharing { } } + /** + * @Then the list of returned shares has :count shares + */ + public function theListOfReturnedSharesHasShares(int $count) { + $this->theHTTPStatusCodeShouldBe('200'); + $this->theOCSStatusCodeShouldBe('100'); + + $returnedShares = $this->getXmlResponse()->data[0]; + + Assert::assertEquals($count, count($returnedShares->element)); + } + + /** + * @Then share :count is returned with + * + * @param int $number + * @param TableNode $body + */ + public function shareXIsReturnedWith(int $number, TableNode $body) { + $this->theHTTPStatusCodeShouldBe('200'); + $this->theOCSStatusCodeShouldBe('100'); + + if (!($body instanceof TableNode)) { + return; + } + + $returnedShare = $this->getXmlResponse()->data[0]; + if ($returnedShare->element) { + $returnedShare = $returnedShare->element[$number]; + } + + $defaultExpectedFields = [ + 'id' => 'A_NUMBER', + 'permissions' => '19', + 'stime' => 'A_NUMBER', + 'parent' => '', + 'expiration' => '', + 'token' => '', + 'storage' => 'A_NUMBER', + 'item_source' => 'A_NUMBER', + 'file_source' => 'A_NUMBER', + 'file_parent' => 'A_NUMBER', + 'mail_send' => '0' + ]; + $expectedFields = array_merge($defaultExpectedFields, $body->getRowsHash()); + + if (!array_key_exists('uid_file_owner', $expectedFields) && + array_key_exists('uid_owner', $expectedFields)) { + $expectedFields['uid_file_owner'] = $expectedFields['uid_owner']; + } + if (!array_key_exists('displayname_file_owner', $expectedFields) && + array_key_exists('displayname_owner', $expectedFields)) { + $expectedFields['displayname_file_owner'] = $expectedFields['displayname_owner']; + } + + if (array_key_exists('share_type', $expectedFields) && + $expectedFields['share_type'] == 10 /* IShare::TYPE_ROOM */ && + array_key_exists('share_with', $expectedFields)) { + if ($expectedFields['share_with'] === 'private_conversation') { + $expectedFields['share_with'] = 'REGEXP /^private_conversation_[0-9a-f]{6}$/'; + } else { + $expectedFields['share_with'] = FeatureContext::getTokenForIdentifier($expectedFields['share_with']); + } + } + + foreach ($expectedFields as $field => $value) { + $this->assertFieldIsInReturnedShare($field, $value, $returnedShare); + } + } + + /** + * @return SimpleXMLElement + */ + private function getXmlResponse(): \SimpleXMLElement { + return simplexml_load_string($this->response->getBody()); + } + + /** + * @param string $field + * @param string $contentExpected + * @param \SimpleXMLElement $returnedShare + */ + private function assertFieldIsInReturnedShare(string $field, string $contentExpected, \SimpleXMLElement $returnedShare){ + if ($contentExpected === 'IGNORE') { + return; + } + + if (!array_key_exists($field, $returnedShare)) { + Assert::fail("$field was not found in response"); + } + + if ($field === 'expiration' && !empty($contentExpected)){ + $contentExpected = date('Y-m-d', strtotime($contentExpected)) . " 00:00:00"; + } + + if ($contentExpected === 'A_NUMBER') { + Assert::assertTrue(is_numeric((string)$returnedShare->$field), "Field '$field' is not a number: " . $returnedShare->$field); + } else if ($contentExpected === 'A_TOKEN') { + // A token is composed by 15 characters from + // ISecureRandom::CHAR_HUMAN_READABLE. + Assert::assertRegExp('/^[abcdefgijkmnopqrstwxyzABCDEFGHJKLMNPQRSTWXYZ23456789]{15}$/', (string)$returnedShare->$field, "Field '$field' is not a token"); + } else if (strpos($contentExpected, 'REGEXP ') === 0) { + Assert::assertRegExp(substr($contentExpected, strlen('REGEXP ')), (string)$returnedShare->$field, "Field '$field' does not match"); + } else { + Assert::assertEquals($contentExpected, (string)$returnedShare->$field, "Field '$field' does not match"); + } + } + /** * @Then As :user remove all shares from the file named :fileName */ diff --git a/build/integration/sharing_features/sharing-v1-part2.feature b/build/integration/sharing_features/sharing-v1-part2.feature index 3316f3d94ba..49a1afe106d 100644 --- a/build/integration/sharing_features/sharing-v1-part2.feature +++ b/build/integration/sharing_features/sharing-v1-part2.feature @@ -115,6 +115,192 @@ Feature: sharing | displayname_owner | user0 | | mimetype | text/plain | + Scenario: getting all shares including subfiles in a directory + Given user "user0" exists + And user "user1" exists + And user "user2" exists + And file "PARENT/CHILD" of user "user0" is shared with user "user1" + And file "PARENT/parent.txt" of user "user0" is shared with user "user2" + When As an "user0" + And sending "GET" to "/apps/files_sharing/api/v1/shares?subfiles=true&path=PARENT" + Then the list of returned shares has 2 shares + And share 0 is returned with + | share_type | 0 | + | uid_owner | user0 | + | displayname_owner | user0 | + | path | /PARENT/CHILD | + | item_type | folder | + | mimetype | httpd/unix-directory | + | storage_id | home::user0 | + | file_target | /CHILD | + | share_with | user1 | + | share_with_displayname | user1 | + | permissions | 31 | + And share 1 is returned with + | share_type | 0 | + | uid_owner | user0 | + | displayname_owner | user0 | + | path | /PARENT/parent.txt | + | item_type | file | + | mimetype | text/plain | + | storage_id | home::user0 | + | file_target | /parent.txt | + | share_with | user2 | + | share_with_displayname | user2 | + + Scenario: getting all shares including subfiles in a directory with received shares + Given user "user0" exists + And user "user1" exists + And file "textfile0.txt" of user "user0" is shared with user "user1" + And file "textfile0.txt" of user "user1" is shared with user "user0" + When As an "user0" + And sending "GET" to "/apps/files_sharing/api/v1/shares?subfiles=true&path=/" + Then the list of returned shares has 1 shares + And share 0 is returned with + | share_type | 0 | + | uid_owner | user0 | + | displayname_owner | user0 | + | path | /textfile0.txt | + | item_type | file | + | mimetype | text/plain | + | storage_id | home::user0 | + | file_target | /textfile0 (2).txt | + | share_with | user1 | + | share_with_displayname | user1 | + + Scenario: getting all shares including subfiles in a directory with shares in subdirectories + Given user "user0" exists + And user "user1" exists + And user "user2" exists + And file "PARENT/CHILD" of user "user0" is shared with user "user1" + And file "PARENT/CHILD/child.txt" of user "user0" is shared with user "user2" + When As an "user0" + And sending "GET" to "/apps/files_sharing/api/v1/shares?subfiles=true&path=PARENT" + Then the list of returned shares has 1 shares + And share 0 is returned with + | share_type | 0 | + | uid_owner | user0 | + | displayname_owner | user0 | + | path | /PARENT/CHILD | + | item_type | folder | + | mimetype | httpd/unix-directory | + | storage_id | home::user0 | + | file_target | /CHILD | + | share_with | user1 | + | share_with_displayname | user1 | + | permissions | 31 | + + Scenario: getting all shares including subfiles in a shared directory with reshares + Given user "user0" exists + And user "user1" exists + And user "user2" exists + And user "user3" exists + And file "PARENT" of user "user0" is shared with user "user1" + And file "PARENT (2)/CHILD" of user "user1" is shared with user "user2" + And file "CHILD" of user "user2" is shared with user "user3" + When As an "user0" + And sending "GET" to "/apps/files_sharing/api/v1/shares?subfiles=true&path=PARENT" + Then the list of returned shares has 2 shares + And share 0 is returned with + | share_type | 0 | + | uid_owner | user1 | + | displayname_owner | user1 | + | uid_file_owner | user0 | + | displayname_file_owner | user0 | + | path | /PARENT/CHILD | + | item_type | folder | + | mimetype | httpd/unix-directory | + | storage_id | home::user0 | + | file_target | /CHILD | + | share_with | user2 | + | share_with_displayname | user2 | + | permissions | 31 | + And share 1 is returned with + | share_type | 0 | + | uid_owner | user2 | + | displayname_owner | user2 | + | uid_file_owner | user0 | + | displayname_file_owner | user0 | + | path | /PARENT/CHILD | + | item_type | folder | + | mimetype | httpd/unix-directory | + | storage_id | home::user0 | + | file_target | /CHILD | + | share_with | user3 | + | share_with_displayname | user3 | + | permissions | 31 | + + Scenario: getting all shares including subfiles in a directory by a resharer + Given user "user0" exists + And user "user1" exists + And user "user2" exists + And user "user3" exists + And file "PARENT" of user "user0" is shared with user "user1" + And file "PARENT (2)/CHILD" of user "user1" is shared with user "user2" + And file "CHILD" of user "user2" is shared with user "user3" + When As an "user1" + And sending "GET" to "/apps/files_sharing/api/v1/shares?subfiles=true&path=PARENT (2)" + Then the list of returned shares has 2 shares + And share 0 is returned with + | share_type | 0 | + | uid_owner | user1 | + | displayname_owner | user1 | + | uid_file_owner | user0 | + | displayname_file_owner | user0 | + | path | /PARENT (2)/CHILD | + | item_type | folder | + | mimetype | httpd/unix-directory | + | storage_id | shared::/PARENT (2) | + | file_target | /CHILD | + | share_with | user2 | + | share_with_displayname | user2 | + | permissions | 31 | + And share 1 is returned with + | share_type | 0 | + | uid_owner | user2 | + | displayname_owner | user2 | + | uid_file_owner | user0 | + | displayname_file_owner | user0 | + | path | /PARENT (2)/CHILD | + | item_type | folder | + | mimetype | httpd/unix-directory | + | storage_id | shared::/PARENT (2) | + | file_target | /CHILD | + | share_with | user3 | + | share_with_displayname | user3 | + | permissions | 31 | + + Scenario: getting all shares including subfiles in a directory by a resharer after revoking the resharing rights + Given user "user0" exists + And user "user1" exists + And user "user2" exists + And user "user3" exists + And file "PARENT" of user "user0" is shared with user "user1" + And save the last share data as "parent folder" + And file "PARENT (2)/CHILD" of user "user1" is shared with user "user2" + And file "CHILD" of user "user2" is shared with user "user3" + And As an "user0" + And restore the last share data from "parent folder" + And Updating last share with + | permissions | 1 | + When As an "user1" + And sending "GET" to "/apps/files_sharing/api/v1/shares?subfiles=true&path=PARENT (2)" + Then the list of returned shares has 1 shares + And share 0 is returned with + | share_type | 0 | + | uid_owner | user1 | + | displayname_owner | user1 | + | uid_file_owner | user0 | + | displayname_file_owner | user0 | + | path | /PARENT (2)/CHILD | + | item_type | folder | + | mimetype | httpd/unix-directory | + | storage_id | shared::/PARENT (2) | + | file_target | /CHILD | + | share_with | user2 | + | share_with_displayname | user2 | + | permissions | 31 | + Scenario: keep group permissions in sync Given As an "admin" Given user "user0" exists -- cgit v1.2.3 From 9039dd89efccd5c9be75b97692c5da60dc7c680f Mon Sep 17 00:00:00 2001 From: Daniel Calviño Sánchez Date: Fri, 25 Oct 2019 06:31:40 +0200 Subject: Add unit tests for getting shares including subfiles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Daniel Calviño Sánchez --- .../tests/Controller/ShareAPIControllerTest.php | 421 +++++++++++++++++++++ 1 file changed, 421 insertions(+) diff --git a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php index 5a84897fe91..5fd5e7c4a46 100644 --- a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php @@ -784,6 +784,427 @@ class ShareAPIControllerTest extends TestCase { $this->ocs->getShare(42); } + public function dataGetShares() { + $folder = $this->getMockBuilder(Folder::class)->getMock(); + $file1 = $this->getMockBuilder(File::class)->getMock(); + $file1->method('getName') + ->willReturn('file1'); + $file2 = $this->getMockBuilder(File::class)->getMock(); + $file2->method('getName') + ->willReturn('file2'); + + $folder->method('getDirectoryListing') + ->willReturn([$file1, $file2]); + + $file1UserShareOwner = \OC::$server->getShareManager()->newShare(); + $file1UserShareOwner->setShareType(IShare::TYPE_USER) + ->setSharedWith('recipient') + ->setSharedBy('initiator') + ->setShareOwner('currentUser') + ->setPermissions(\OCP\Constants::PERMISSION_READ) + ->setNode($file1) + ->setId(4); + + $file1UserShareOwnerExpected = [ + 'id' => 4, + 'share_type' => IShare::TYPE_USER, + ]; + + $file1UserShareInitiator = \OC::$server->getShareManager()->newShare(); + $file1UserShareInitiator->setShareType(IShare::TYPE_USER) + ->setSharedWith('recipient') + ->setSharedBy('currentUser') + ->setShareOwner('owner') + ->setPermissions(\OCP\Constants::PERMISSION_READ) + ->setNode($file1) + ->setId(8); + + $file1UserShareInitiatorExpected = [ + 'id' => 8, + 'share_type' => IShare::TYPE_USER, + ]; + + $file1UserShareRecipient = \OC::$server->getShareManager()->newShare(); + $file1UserShareRecipient->setShareType(IShare::TYPE_USER) + ->setSharedWith('currentUser') + ->setSharedBy('initiator') + ->setShareOwner('owner') + ->setPermissions(\OCP\Constants::PERMISSION_READ) + ->setNode($file1) + ->setId(15); + + $file1UserShareOther = \OC::$server->getShareManager()->newShare(); + $file1UserShareOther->setShareType(IShare::TYPE_USER) + ->setSharedWith('recipient') + ->setSharedBy('initiator') + ->setShareOwner('owner') + ->setPermissions(\OCP\Constants::PERMISSION_READ) + ->setNode($file1) + ->setId(16); + + $file1UserShareOtherExpected = [ + 'id' => 16, + 'share_type' => IShare::TYPE_USER, + ]; + + $file1GroupShareOwner = \OC::$server->getShareManager()->newShare(); + $file1GroupShareOwner->setShareType(IShare::TYPE_GROUP) + ->setSharedWith('recipient') + ->setSharedBy('initiator') + ->setShareOwner('currentUser') + ->setPermissions(\OCP\Constants::PERMISSION_READ) + ->setNode($file1) + ->setId(23); + + $file1GroupShareOwnerExpected = [ + 'id' => 23, + 'share_type' => IShare::TYPE_GROUP, + ]; + + $file1GroupShareRecipient = \OC::$server->getShareManager()->newShare(); + $file1GroupShareRecipient->setShareType(IShare::TYPE_GROUP) + ->setSharedWith('currentUserGroup') + ->setSharedBy('initiator') + ->setShareOwner('owner') + ->setPermissions(\OCP\Constants::PERMISSION_READ) + ->setNode($file1) + ->setId(42); + + $file1GroupShareRecipientExpected = [ + 'id' => 42, + 'share_type' => IShare::TYPE_GROUP, + ]; + + $file1GroupShareOther = \OC::$server->getShareManager()->newShare(); + $file1GroupShareOther->setShareType(IShare::TYPE_GROUP) + ->setSharedWith('recipient') + ->setSharedBy('initiator') + ->setShareOwner('owner') + ->setPermissions(\OCP\Constants::PERMISSION_READ) + ->setNode($file1) + ->setId(108); + + $file1LinkShareOwner = \OC::$server->getShareManager()->newShare(); + $file1LinkShareOwner->setShareType(IShare::TYPE_LINK) + ->setSharedWith('recipient') + ->setSharedBy('initiator') + ->setShareOwner('currentUser') + ->setPermissions(\OCP\Constants::PERMISSION_READ) + ->setNode($file1) + ->setId(415); + + $file1LinkShareOwnerExpected = [ + 'id' => 415, + 'share_type' => IShare::TYPE_LINK, + ]; + + $file1EmailShareOwner = \OC::$server->getShareManager()->newShare(); + $file1EmailShareOwner->setShareType(IShare::TYPE_EMAIL) + ->setSharedWith('recipient') + ->setSharedBy('initiator') + ->setShareOwner('currentUser') + ->setPermissions(\OCP\Constants::PERMISSION_READ) + ->setNode($file1) + ->setId(416); + + $file1EmailShareOwnerExpected = [ + 'id' => 416, + 'share_type' => IShare::TYPE_EMAIL, + ]; + + $file1RoomShareOwner = \OC::$server->getShareManager()->newShare(); + $file1RoomShareOwner->setShareType(IShare::TYPE_ROOM) + ->setSharedWith('recipient') + ->setSharedBy('initiator') + ->setShareOwner('currentUser') + ->setPermissions(\OCP\Constants::PERMISSION_READ) + ->setNode($file1) + ->setId(423); + + $file1RoomShareOwnerExpected = [ + 'id' => 423, + 'share_type' => IShare::TYPE_ROOM, + ]; + + $file1RemoteShareOwner = \OC::$server->getShareManager()->newShare(); + $file1RemoteShareOwner->setShareType(IShare::TYPE_REMOTE) + ->setSharedWith('recipient') + ->setSharedBy('initiator') + ->setShareOwner('currentUser') + ->setPermissions(\OCP\Constants::PERMISSION_READ) + ->setNode($file1) + ->setId(442); + + $file1RemoteShareOwnerExpected = [ + 'id' => 442, + 'share_type' => IShare::TYPE_REMOTE, + ]; + + $file2UserShareOwner = \OC::$server->getShareManager()->newShare(); + $file2UserShareOwner->setShareType(IShare::TYPE_USER) + ->setSharedWith('recipient') + ->setSharedBy('initiator') + ->setShareOwner('currentUser') + ->setPermissions(\OCP\Constants::PERMISSION_READ) + ->setNode($file2) + ->setId(815); + + $file2UserShareOwnerExpected = [ + 'id' => 815, + 'share_type' => IShare::TYPE_USER, + ]; + + $data = [ + [ + [ + 'path' => $folder, + 'subfiles' => 'true', + ], + [ + 'file1' => [ + IShare::TYPE_USER => [$file1UserShareOwner], + ], + 'file2' => [ + IShare::TYPE_USER => [$file2UserShareOwner], + ], + ], + [ + ], + [ + $file1UserShareOwnerExpected, + $file2UserShareOwnerExpected, + ] + ], + [ + [ + 'path' => $folder, + 'subfiles' => 'true', + ], + [ + 'file1' => [ + IShare::TYPE_USER => [$file1UserShareOwner, $file1UserShareOwner, $file1UserShareOwner], + ], + ], + [ + ], + [ + $file1UserShareOwnerExpected, + ] + ], + [ + [ + 'path' => $folder, + 'subfiles' => 'true', + ], + [ + 'file1' => [ + IShare::TYPE_USER => [$file1UserShareOwner, $file1UserShareRecipient], + ], + ], + [ + ], + [ + $file1UserShareOwnerExpected + ] + ], + [ + [ + 'path' => $folder, + 'subfiles' => 'true', + ], + [ + 'file1' => [ + IShare::TYPE_USER => [$file1UserShareRecipient, $file1UserShareInitiator, $file1UserShareOther], + ], + 'file2' => [ + IShare::TYPE_USER => [$file2UserShareOwner], + ], + ], + [ + ], + [ + $file1UserShareInitiatorExpected, + $file1UserShareOtherExpected, + $file2UserShareOwnerExpected, + ] + ], + // This might not happen in a real environment, as the combination + // of shares does not seem to be possible on a folder without + // resharing rights; if the folder has resharing rights then the + // share with others would be included too in the results. + [ + [ + 'path' => $folder, + 'subfiles' => 'true', + ], + [ + 'file1' => [ + IShare::TYPE_USER => [$file1UserShareRecipient, $file1UserShareInitiator, $file1UserShareOther], + ], + ], + [ + ], + [ + $file1UserShareInitiatorExpected, + ] + ], + [ + [ + 'path' => $folder, + 'subfiles' => 'true', + ], + [ + 'file1' => [ + IShare::TYPE_USER => [$file1UserShareOwner], + IShare::TYPE_GROUP => [$file1GroupShareRecipient], + ], + ], + [ + ], + [ + $file1UserShareOwnerExpected, + $file1GroupShareRecipientExpected, + ] + ], + [ + [ + 'path' => $folder, + 'subfiles' => 'true', + ], + [ + 'file1' => [ + IShare::TYPE_USER => [$file1UserShareOwner], + IShare::TYPE_GROUP => [$file1GroupShareOwner], + IShare::TYPE_LINK => [$file1LinkShareOwner], + IShare::TYPE_EMAIL => [$file1EmailShareOwner], + IShare::TYPE_ROOM => [$file1RoomShareOwner], + IShare::TYPE_REMOTE => [$file1RemoteShareOwner], + ], + ], + [ + ], + [ + $file1UserShareOwnerExpected, + $file1GroupShareOwnerExpected, + $file1LinkShareOwnerExpected, + $file1RoomShareOwnerExpected, + ] + ], + [ + [ + 'path' => $folder, + 'subfiles' => 'true', + ], + [ + 'file1' => [ + IShare::TYPE_USER => [$file1UserShareOwner], + IShare::TYPE_GROUP => [$file1GroupShareOwner], + IShare::TYPE_LINK => [$file1LinkShareOwner], + IShare::TYPE_EMAIL => [$file1EmailShareOwner], + IShare::TYPE_ROOM => [$file1RoomShareOwner], + IShare::TYPE_REMOTE => [$file1RemoteShareOwner], + ], + ], + [ + IShare::TYPE_EMAIL => true, + IShare::TYPE_REMOTE => true, + ], + [ + $file1UserShareOwnerExpected, + $file1GroupShareOwnerExpected, + $file1LinkShareOwnerExpected, + $file1EmailShareOwnerExpected, + $file1RemoteShareOwnerExpected, + $file1RoomShareOwnerExpected, + ] + ], + ]; + + return $data; + } + + /** + * @dataProvider dataGetShares + */ + public function testGetShares(array $getSharesParameters, array $shares, array $extraShareTypes, array $expected) { + /** @var \OCA\Files_Sharing\Controller\ShareAPIController $ocs */ + $ocs = $this->getMockBuilder(ShareAPIController::class) + ->setConstructorArgs([ + $this->appName, + $this->request, + $this->shareManager, + $this->groupManager, + $this->userManager, + $this->rootFolder, + $this->urlGenerator, + $this->currentUser, + $this->l, + $this->config, + $this->appManager, + $this->serverContainer + ])->setMethods(['formatShare']) + ->getMock(); + + $ocs->method('formatShare') + ->will($this->returnCallback( + function($share) { + return [ + 'id' => $share->getId(), + 'share_type' => $share->getShareType() + ]; + } + )); + + $userFolder = $this->getMockBuilder('OCP\Files\Folder')->getMock(); + $userFolder->method('get') + ->with('path') + ->willReturn($getSharesParameters['path']); + + $this->rootFolder->method('getUserFolder') + ->with($this->currentUser) + ->willReturn($userFolder); + + $this->shareManager + ->method('getSharesBy') + ->will($this->returnCallback( + function($user, $shareType, $node) use ($shares) { + if (!isset($shares[$node->getName()]) || !isset($shares[$node->getName()][$shareType])) { + return []; + } + return $shares[$node->getName()][$shareType]; + } + )); + + $shareProviderExistsMap = [ + [IShare::TYPE_EMAIL, $extraShareTypes[IShare::TYPE_EMAIL] ?? false], + ]; + + $this->shareManager + ->method('shareProviderExists') + ->will($this->returnValueMap($shareProviderExistsMap)); + + $this->shareManager + ->method('outgoingServer2ServerSharesAllowed') + ->willReturn($extraShareTypes[ISHARE::TYPE_REMOTE] ?? false); + + $this->groupManager + ->method('isInGroup') + ->will($this->returnCallback( + function($user, $group) { + return $group === 'currentUserGroup'; + } + )); + + $result = $ocs->getShares( + $getSharesParameters['sharedWithMe'] ?? 'false', + $getSharesParameters['reshares'] ?? 'false', + $getSharesParameters['subfiles'] ?? 'false', + 'path' + ); + + $this->assertEquals($expected, $result->getData()); + } + public function testCanAccessShare() { $share = $this->getMockBuilder(IShare::class)->getMock(); $share->method('getShareOwner')->willReturn($this->currentUser); -- cgit v1.2.3 From 52870fd1d24caba10726c95e266b876394e9dbf2 Mon Sep 17 00:00:00 2001 From: Daniel Calviño Sánchez Date: Fri, 25 Oct 2019 10:02:06 +0200 Subject: Add unit tests for getting shares of a file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Daniel Calviño Sánchez --- .../tests/Controller/ShareAPIControllerTest.php | 191 ++++++++++++++++++++- 1 file changed, 185 insertions(+), 6 deletions(-) diff --git a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php index 5fd5e7c4a46..4fda1aa4156 100644 --- a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php @@ -833,6 +833,11 @@ class ShareAPIControllerTest extends TestCase { ->setNode($file1) ->setId(15); + $file1UserShareRecipientExpected = [ + 'id' => 15, + 'share_type' => IShare::TYPE_USER, + ]; + $file1UserShareOther = \OC::$server->getShareManager()->newShare(); $file1UserShareOther->setShareType(IShare::TYPE_USER) ->setSharedWith('recipient') @@ -912,6 +917,20 @@ class ShareAPIControllerTest extends TestCase { 'share_type' => IShare::TYPE_EMAIL, ]; + $file1CircleShareOwner = \OC::$server->getShareManager()->newShare(); + $file1CircleShareOwner->setShareType(IShare::TYPE_CIRCLE) + ->setSharedWith('recipient') + ->setSharedBy('initiator') + ->setShareOwner('currentUser') + ->setPermissions(\OCP\Constants::PERMISSION_READ) + ->setNode($file1) + ->setId(423); + + $file1CircleShareOwnerExpected = [ + 'id' => 423, + 'share_type' => IShare::TYPE_CIRCLE, + ]; + $file1RoomShareOwner = \OC::$server->getShareManager()->newShare(); $file1RoomShareOwner->setShareType(IShare::TYPE_ROOM) ->setSharedWith('recipient') @@ -919,10 +938,10 @@ class ShareAPIControllerTest extends TestCase { ->setShareOwner('currentUser') ->setPermissions(\OCP\Constants::PERMISSION_READ) ->setNode($file1) - ->setId(423); + ->setId(442); $file1RoomShareOwnerExpected = [ - 'id' => 423, + 'id' => 442, 'share_type' => IShare::TYPE_ROOM, ]; @@ -933,13 +952,27 @@ class ShareAPIControllerTest extends TestCase { ->setShareOwner('currentUser') ->setPermissions(\OCP\Constants::PERMISSION_READ) ->setNode($file1) - ->setId(442); + ->setId(815); $file1RemoteShareOwnerExpected = [ - 'id' => 442, + 'id' => 815, 'share_type' => IShare::TYPE_REMOTE, ]; + $file1RemoteGroupShareOwner = \OC::$server->getShareManager()->newShare(); + $file1RemoteGroupShareOwner->setShareType(IShare::TYPE_REMOTE_GROUP) + ->setSharedWith('recipient') + ->setSharedBy('initiator') + ->setShareOwner('currentUser') + ->setPermissions(\OCP\Constants::PERMISSION_READ) + ->setNode($file1) + ->setId(816); + + $file1RemoteGroupShareOwnerExpected = [ + 'id' => 816, + 'share_type' => IShare::TYPE_REMOTE_GROUP, + ]; + $file2UserShareOwner = \OC::$server->getShareManager()->newShare(); $file2UserShareOwner->setShareType(IShare::TYPE_USER) ->setSharedWith('recipient') @@ -947,14 +980,155 @@ class ShareAPIControllerTest extends TestCase { ->setShareOwner('currentUser') ->setPermissions(\OCP\Constants::PERMISSION_READ) ->setNode($file2) - ->setId(815); + ->setId(823); $file2UserShareOwnerExpected = [ - 'id' => 815, + 'id' => 823, 'share_type' => IShare::TYPE_USER, ]; $data = [ + [ + [ + 'path' => $file1, + ], + [ + 'file1' => [ + IShare::TYPE_USER => [$file1UserShareOwner, $file1UserShareOwner, $file1UserShareOwner], + ], + ], + [ + ], + [ + $file1UserShareOwnerExpected, + $file1UserShareOwnerExpected, + $file1UserShareOwnerExpected, + ] + ], + [ + [ + 'path' => $file1, + ], + [ + 'file1' => [ + IShare::TYPE_USER => [$file1UserShareOwner, $file1UserShareRecipient], + ], + ], + [ + ], + [ + $file1UserShareOwnerExpected, + $file1UserShareRecipientExpected, + ] + ], + [ + [ + 'path' => $file1, + ], + [ + 'file1' => [ + IShare::TYPE_USER => [$file1UserShareOwner, $file1UserShareRecipient, $file1UserShareInitiator, $file1UserShareOther], + ], + ], + [ + ], + [ + $file1UserShareOwnerExpected, + $file1UserShareRecipientExpected, + $file1UserShareInitiatorExpected, + $file1UserShareOtherExpected, + ] + ], + [ + [ + 'path' => $file1, + ], + [ + 'file1' => [ + IShare::TYPE_USER => [$file1UserShareRecipient, $file1UserShareInitiator, $file1UserShareOther], + ], + ], + [ + ], + [ + $file1UserShareInitiatorExpected, + ] + ], + [ + [ + 'path' => $file1, + ], + [ + 'file1' => [ + IShare::TYPE_USER => [$file1UserShareOwner], + IShare::TYPE_GROUP => [$file1GroupShareRecipient], + ], + ], + [ + ], + [ + $file1UserShareOwnerExpected, + $file1GroupShareRecipientExpected, + ] + ], + [ + [ + 'path' => $file1, + ], + [ + 'file1' => [ + IShare::TYPE_USER => [$file1UserShareOwner], + IShare::TYPE_GROUP => [$file1GroupShareOwner], + IShare::TYPE_LINK => [$file1LinkShareOwner], + IShare::TYPE_EMAIL => [$file1EmailShareOwner], + IShare::TYPE_CIRCLE => [$file1CircleShareOwner], + IShare::TYPE_ROOM => [$file1RoomShareOwner], + IShare::TYPE_REMOTE => [$file1RemoteShareOwner], + IShare::TYPE_REMOTE_GROUP => [$file1RemoteGroupShareOwner], + ], + ], + [ + ], + [ + $file1UserShareOwnerExpected, + $file1GroupShareOwnerExpected, + $file1LinkShareOwnerExpected, + $file1RoomShareOwnerExpected, + ] + ], + [ + [ + 'path' => $file1, + ], + [ + 'file1' => [ + IShare::TYPE_USER => [$file1UserShareOwner], + IShare::TYPE_GROUP => [$file1GroupShareOwner], + IShare::TYPE_LINK => [$file1LinkShareOwner], + IShare::TYPE_EMAIL => [$file1EmailShareOwner], + IShare::TYPE_CIRCLE => [$file1CircleShareOwner], + IShare::TYPE_ROOM => [$file1RoomShareOwner], + IShare::TYPE_REMOTE => [$file1RemoteShareOwner], + IShare::TYPE_REMOTE_GROUP => [$file1RemoteGroupShareOwner], + ], + ], + [ + IShare::TYPE_EMAIL => true, + IShare::TYPE_CIRCLE => true, + IShare::TYPE_REMOTE => true, + IShare::TYPE_REMOTE_GROUP => true, + ], + [ + $file1UserShareOwnerExpected, + $file1GroupShareOwnerExpected, + $file1LinkShareOwnerExpected, + $file1EmailShareOwnerExpected, + $file1CircleShareOwnerExpected, + $file1RoomShareOwnerExpected, + $file1RemoteShareOwnerExpected, + $file1RemoteGroupShareOwnerExpected, + ] + ], [ [ 'path' => $folder, @@ -1177,6 +1351,7 @@ class ShareAPIControllerTest extends TestCase { $shareProviderExistsMap = [ [IShare::TYPE_EMAIL, $extraShareTypes[IShare::TYPE_EMAIL] ?? false], + [IShare::TYPE_CIRCLE, $extraShareTypes[IShare::TYPE_CIRCLE] ?? false], ]; $this->shareManager @@ -1187,6 +1362,10 @@ class ShareAPIControllerTest extends TestCase { ->method('outgoingServer2ServerSharesAllowed') ->willReturn($extraShareTypes[ISHARE::TYPE_REMOTE] ?? false); + $this->shareManager + ->method('outgoingServer2ServerGroupSharesAllowed') + ->willReturn($extraShareTypes[ISHARE::TYPE_REMOTE_GROUP] ?? false); + $this->groupManager ->method('isInGroup') ->will($this->returnCallback( -- cgit v1.2.3 From 0599536c823eeea76217036a41a891345ff39c28 Mon Sep 17 00:00:00 2001 From: Daniel Calviño Sánchez Date: Fri, 25 Oct 2019 10:15:50 +0200 Subject: Remove unneeded calls to "ShareManager::shareProviderExists()" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "ShareManager::getSharesBy()" already checks if the share provider exists before returning the shares and, if the provider does not exist, it returns an empty array. Therefore it is not needed to explicitly check if the provider exists or not. Signed-off-by: Daniel Calviño Sánchez --- apps/files_sharing/lib/Controller/ShareAPIController.php | 16 +++------------- .../tests/Controller/ShareAPIControllerTest.php | 15 +++------------ 2 files changed, 6 insertions(+), 25 deletions(-) diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php index cde4f93a0f0..04eb8225fbc 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -628,9 +628,7 @@ class ShareAPIController extends OCSController { $shares = array_merge($shares, $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_USER, $node, true, -1, 0)); $shares = array_merge($shares, $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_GROUP, $node, true, -1, 0)); $shares = array_merge($shares, $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_LINK, $node, true, -1, 0)); - if ($this->shareManager->shareProviderExists(Share::SHARE_TYPE_EMAIL)) { - $shares = array_merge($shares, $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_EMAIL, $node, true, -1, 0)); - } + $shares = array_merge($shares, $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_EMAIL, $node, true, -1, 0)); if ($this->shareManager->outgoingServer2ServerSharesAllowed()) { $shares = array_merge($shares, $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_REMOTE, $node, true, -1, 0)); } @@ -729,16 +727,8 @@ class ShareAPIController extends OCSController { $userShares = $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_USER, $path, $reshares, -1, 0); $groupShares = $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_GROUP, $path, $reshares, -1, 0); $linkShares = $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_LINK, $path, $reshares, -1, 0); - if ($this->shareManager->shareProviderExists(Share::SHARE_TYPE_EMAIL)) { - $mailShares = $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_EMAIL, $path, $reshares, -1, 0); - } else { - $mailShares = []; - } - if ($this->shareManager->shareProviderExists(Share::SHARE_TYPE_CIRCLE)) { - $circleShares = $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_CIRCLE, $path, $reshares, -1, 0); - } else { - $circleShares = []; - } + $mailShares = $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_EMAIL, $path, $reshares, -1, 0); + $circleShares = $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_CIRCLE, $path, $reshares, -1, 0); $roomShares = $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_ROOM, $path, $reshares, -1, 0); $shares = array_merge($userShares, $groupShares, $linkShares, $mailShares, $circleShares, $roomShares); diff --git a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php index 4fda1aa4156..fd6aed09a9f 100644 --- a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php @@ -1093,6 +1093,8 @@ class ShareAPIControllerTest extends TestCase { $file1UserShareOwnerExpected, $file1GroupShareOwnerExpected, $file1LinkShareOwnerExpected, + $file1EmailShareOwnerExpected, + $file1CircleShareOwnerExpected, $file1RoomShareOwnerExpected, ] ], @@ -1113,8 +1115,6 @@ class ShareAPIControllerTest extends TestCase { ], ], [ - IShare::TYPE_EMAIL => true, - IShare::TYPE_CIRCLE => true, IShare::TYPE_REMOTE => true, IShare::TYPE_REMOTE_GROUP => true, ], @@ -1261,6 +1261,7 @@ class ShareAPIControllerTest extends TestCase { $file1UserShareOwnerExpected, $file1GroupShareOwnerExpected, $file1LinkShareOwnerExpected, + $file1EmailShareOwnerExpected, $file1RoomShareOwnerExpected, ] ], @@ -1280,7 +1281,6 @@ class ShareAPIControllerTest extends TestCase { ], ], [ - IShare::TYPE_EMAIL => true, IShare::TYPE_REMOTE => true, ], [ @@ -1349,15 +1349,6 @@ class ShareAPIControllerTest extends TestCase { } )); - $shareProviderExistsMap = [ - [IShare::TYPE_EMAIL, $extraShareTypes[IShare::TYPE_EMAIL] ?? false], - [IShare::TYPE_CIRCLE, $extraShareTypes[IShare::TYPE_CIRCLE] ?? false], - ]; - - $this->shareManager - ->method('shareProviderExists') - ->will($this->returnValueMap($shareProviderExistsMap)); - $this->shareManager ->method('outgoingServer2ServerSharesAllowed') ->willReturn($extraShareTypes[ISHARE::TYPE_REMOTE] ?? false); -- cgit v1.2.3 From 24febe1e4145e8446ff8572fe84d78495c45ed38 Mon Sep 17 00:00:00 2001 From: Daniel Calviño Sánchez Date: Fri, 25 Oct 2019 10:20:32 +0200 Subject: Unify share type sorting when getting shares of a file or with subfiles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Daniel Calviño Sánchez --- apps/files_sharing/lib/Controller/ShareAPIController.php | 2 +- apps/files_sharing/tests/Controller/ShareAPIControllerTest.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php index 04eb8225fbc..8b5ad12ae52 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -629,10 +629,10 @@ class ShareAPIController extends OCSController { $shares = array_merge($shares, $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_GROUP, $node, true, -1, 0)); $shares = array_merge($shares, $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_LINK, $node, true, -1, 0)); $shares = array_merge($shares, $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_EMAIL, $node, true, -1, 0)); + $shares = array_merge($shares, $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_ROOM, $node, true, -1, 0)); if ($this->shareManager->outgoingServer2ServerSharesAllowed()) { $shares = array_merge($shares, $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_REMOTE, $node, true, -1, 0)); } - $shares = array_merge($shares, $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_ROOM, $node, true, -1, 0)); } $formatted = $miniFormatted = []; diff --git a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php index fd6aed09a9f..3e0dc29fe95 100644 --- a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php @@ -1288,8 +1288,8 @@ class ShareAPIControllerTest extends TestCase { $file1GroupShareOwnerExpected, $file1LinkShareOwnerExpected, $file1EmailShareOwnerExpected, - $file1RemoteShareOwnerExpected, $file1RoomShareOwnerExpected, + $file1RemoteShareOwnerExpected, ] ], ]; -- cgit v1.2.3 From 222e5a9a9fdb188589adc9eed9d6a843d9e9eff5 Mon Sep 17 00:00:00 2001 From: Daniel Calviño Sánchez Date: Fri, 25 Oct 2019 10:40:11 +0200 Subject: Add missing share types when getting shares of a file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Daniel Calviño Sánchez --- apps/files_sharing/lib/Controller/ShareAPIController.php | 4 ++++ apps/files_sharing/tests/Controller/ShareAPIControllerTest.php | 8 ++++++++ 2 files changed, 12 insertions(+) diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php index 8b5ad12ae52..90d739c5bc1 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -629,10 +629,14 @@ class ShareAPIController extends OCSController { $shares = array_merge($shares, $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_GROUP, $node, true, -1, 0)); $shares = array_merge($shares, $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_LINK, $node, true, -1, 0)); $shares = array_merge($shares, $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_EMAIL, $node, true, -1, 0)); + $shares = array_merge($shares, $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_CIRCLE, $node, true, -1, 0)); $shares = array_merge($shares, $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_ROOM, $node, true, -1, 0)); if ($this->shareManager->outgoingServer2ServerSharesAllowed()) { $shares = array_merge($shares, $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_REMOTE, $node, true, -1, 0)); } + if ($this->shareManager->outgoingServer2ServerGroupSharesAllowed()) { + $shares = array_merge($shares, $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_REMOTE_GROUP, $node, true, -1, 0)); + } } $formatted = $miniFormatted = []; diff --git a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php index 3e0dc29fe95..7c77406e4b5 100644 --- a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php @@ -1251,8 +1251,10 @@ class ShareAPIControllerTest extends TestCase { IShare::TYPE_GROUP => [$file1GroupShareOwner], IShare::TYPE_LINK => [$file1LinkShareOwner], IShare::TYPE_EMAIL => [$file1EmailShareOwner], + IShare::TYPE_CIRCLE => [$file1CircleShareOwner], IShare::TYPE_ROOM => [$file1RoomShareOwner], IShare::TYPE_REMOTE => [$file1RemoteShareOwner], + IShare::TYPE_REMOTE_GROUP => [$file1RemoteGroupShareOwner], ], ], [ @@ -1262,6 +1264,7 @@ class ShareAPIControllerTest extends TestCase { $file1GroupShareOwnerExpected, $file1LinkShareOwnerExpected, $file1EmailShareOwnerExpected, + $file1CircleShareOwnerExpected, $file1RoomShareOwnerExpected, ] ], @@ -1276,20 +1279,25 @@ class ShareAPIControllerTest extends TestCase { IShare::TYPE_GROUP => [$file1GroupShareOwner], IShare::TYPE_LINK => [$file1LinkShareOwner], IShare::TYPE_EMAIL => [$file1EmailShareOwner], + IShare::TYPE_CIRCLE => [$file1CircleShareOwner], IShare::TYPE_ROOM => [$file1RoomShareOwner], IShare::TYPE_REMOTE => [$file1RemoteShareOwner], + IShare::TYPE_REMOTE_GROUP => [$file1RemoteGroupShareOwner], ], ], [ IShare::TYPE_REMOTE => true, + IShare::TYPE_REMOTE_GROUP => true, ], [ $file1UserShareOwnerExpected, $file1GroupShareOwnerExpected, $file1LinkShareOwnerExpected, $file1EmailShareOwnerExpected, + $file1CircleShareOwnerExpected, $file1RoomShareOwnerExpected, $file1RemoteShareOwnerExpected, + $file1RemoteGroupShareOwnerExpected, ] ], ]; -- cgit v1.2.3 From 1afefc2d9a0dba59fc083d4682ca5787023be791 Mon Sep 17 00:00:00 2001 From: Daniel Calviño Sánchez Date: Fri, 25 Oct 2019 10:45:49 +0200 Subject: Extract method to get all shares MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Daniel Calviño Sánchez --- .../lib/Controller/ShareAPIController.php | 77 ++++++++++++---------- 1 file changed, 43 insertions(+), 34 deletions(-) diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php index 90d739c5bc1..c35faabfa12 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -621,23 +621,12 @@ class ShareAPIController extends OCSController { } $nodes = $folder->getDirectoryListing(); + /** @var \OCP\Share\IShare[] $shares */ - $shares = []; - foreach ($nodes as $node) { - - $shares = array_merge($shares, $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_USER, $node, true, -1, 0)); - $shares = array_merge($shares, $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_GROUP, $node, true, -1, 0)); - $shares = array_merge($shares, $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_LINK, $node, true, -1, 0)); - $shares = array_merge($shares, $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_EMAIL, $node, true, -1, 0)); - $shares = array_merge($shares, $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_CIRCLE, $node, true, -1, 0)); - $shares = array_merge($shares, $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_ROOM, $node, true, -1, 0)); - if ($this->shareManager->outgoingServer2ServerSharesAllowed()) { - $shares = array_merge($shares, $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_REMOTE, $node, true, -1, 0)); - } - if ($this->shareManager->outgoingServer2ServerGroupSharesAllowed()) { - $shares = array_merge($shares, $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_REMOTE_GROUP, $node, true, -1, 0)); - } - } + $shares = array_reduce($nodes, function($carry, $node) { + $carry = array_merge($carry, $this->getAllShares($node, true)); + return $carry; + }, []); $formatted = $miniFormatted = []; $resharingRight = false; @@ -728,24 +717,7 @@ class ShareAPIController extends OCSController { } // Get all shares - $userShares = $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_USER, $path, $reshares, -1, 0); - $groupShares = $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_GROUP, $path, $reshares, -1, 0); - $linkShares = $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_LINK, $path, $reshares, -1, 0); - $mailShares = $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_EMAIL, $path, $reshares, -1, 0); - $circleShares = $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_CIRCLE, $path, $reshares, -1, 0); - $roomShares = $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_ROOM, $path, $reshares, -1, 0); - - $shares = array_merge($userShares, $groupShares, $linkShares, $mailShares, $circleShares, $roomShares); - - if ($this->shareManager->outgoingServer2ServerSharesAllowed()) { - $federatedShares = $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_REMOTE, $path, $reshares, -1, 0); - $shares = array_merge($shares, $federatedShares); - } - - if ($this->shareManager->outgoingServer2ServerGroupSharesAllowed()) { - $federatedShares = $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_REMOTE_GROUP, $path, $reshares, -1, 0); - $shares = array_merge($shares, $federatedShares); - } + $shares = $this->getAllShares($path, $reshares); $formatted = $miniFormatted = []; $resharingRight = false; @@ -1318,4 +1290,41 @@ class ShareAPIController extends OCSController { return false; } + /** + * Get all the shares for the current user + * + * @param Node|null $path + * @param boolean $reshares + * @return void + */ + private function getAllShares(?Node $path = null, bool $reshares = false) { + // Get all shares + $userShares = $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_USER, $path, $reshares, -1, 0); + $groupShares = $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_GROUP, $path, $reshares, -1, 0); + $linkShares = $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_LINK, $path, $reshares, -1, 0); + + // EMAIL SHARES + $mailShares = $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_EMAIL, $path, $reshares, -1, 0); + + // CIRCLE SHARES + $circleShares = $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_CIRCLE, $path, $reshares, -1, 0); + + // TALK SHARES + $roomShares = $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_ROOM, $path, $reshares, -1, 0); + + // FEDERATION + if ($this->shareManager->outgoingServer2ServerSharesAllowed()) { + $federatedShares = $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_REMOTE, $path, $reshares, -1, 0); + } else { + $federatedShares = []; + } + if ($this->shareManager->outgoingServer2ServerGroupSharesAllowed()) { + $federatedGroupShares = $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_REMOTE_GROUP, $path, $reshares, -1, 0); + } else { + $federatedGroupShares = []; + } + + return array_merge($userShares, $groupShares, $linkShares, $mailShares, $circleShares, $roomShares, $federatedShares, $federatedGroupShares); + } + } -- cgit v1.2.3 From 8098abee608e58dad8b73a1151233be525c25483 Mon Sep 17 00:00:00 2001 From: Daniel Calviño Sánchez Date: Fri, 25 Oct 2019 11:15:02 +0200 Subject: Do not return shares with the current user MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Getting the shares of a file no longer returns shares with the current user for consistency with the results when getting the shares including subfiles. Signed-off-by: Daniel Calviño Sánchez --- apps/files_sharing/lib/Controller/ShareAPIController.php | 4 ++++ apps/files_sharing/tests/Controller/ShareAPIControllerTest.php | 2 -- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php index c35faabfa12..218d854f2f1 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -723,6 +723,10 @@ class ShareAPIController extends OCSController { $resharingRight = false; foreach ($shares as $share) { /** @var IShare $share */ + if ($share->getSharedWith() === $this->currentUser) { + continue; + } + try { $format = $this->formatShare($share, $path); $formatted[] = $format; diff --git a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php index 7c77406e4b5..c972c5c794e 100644 --- a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php @@ -1018,7 +1018,6 @@ class ShareAPIControllerTest extends TestCase { ], [ $file1UserShareOwnerExpected, - $file1UserShareRecipientExpected, ] ], [ @@ -1034,7 +1033,6 @@ class ShareAPIControllerTest extends TestCase { ], [ $file1UserShareOwnerExpected, - $file1UserShareRecipientExpected, $file1UserShareInitiatorExpected, $file1UserShareOtherExpected, ] -- cgit v1.2.3 From dbb6b4cbb90c21a1256e62014849055b723a0f66 Mon Sep 17 00:00:00 2001 From: "John Molakvoæ (skjnldsv)" Date: Mon, 19 Aug 2019 10:53:29 +0200 Subject: Filter out duplicated shares before processing them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: John Molakvoæ (skjnldsv) --- apps/files_sharing/lib/Controller/ShareAPIController.php | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php index 218d854f2f1..134ff6378e7 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -628,18 +628,26 @@ class ShareAPIController extends OCSController { return $carry; }, []); + // filter out duplicate shares + $known = []; + $shares = array_filter($shares, function($share) use (&$known) { + if (in_array($share->getId(), $known)) { + return false; + } + $known[] = $share->getId(); + return true; + }); + $formatted = $miniFormatted = []; $resharingRight = false; - $known = []; foreach ($shares as $share) { - if (in_array($share->getId(), $known) || $share->getSharedWith() === $this->currentUser) { + if ($share->getSharedWith() === $this->currentUser) { continue; } try { $format = $this->formatShare($share); - $known[] = $share->getId(); $formatted[] = $format; if ($share->getSharedBy() === $this->currentUser) { $miniFormatted[] = $format; -- cgit v1.2.3 From cc4362df4f09aa7f6ad75b5c30e8881122b543b1 Mon Sep 17 00:00:00 2001 From: "John Molakvoæ (skjnldsv)" Date: Mon, 19 Aug 2019 10:53:29 +0200 Subject: Unify code to get shares of a file or also including subfiles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: John Molakvoæ (skjnldsv) --- .../lib/Controller/ShareAPIController.php | 60 ++++++++-------------- 1 file changed, 21 insertions(+), 39 deletions(-) diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php index 134ff6378e7..e44ca84a09f 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -612,10 +612,10 @@ class ShareAPIController extends OCSController { /** * @param \OCP\Files\Folder $folder - * @return DataResponse + * @return array * @throws OCSBadRequestException */ - private function getSharesInDir(Node $folder): DataResponse { + private function getSharesInDir(Node $folder): array { if (!($folder instanceof \OCP\Files\Folder)) { throw new OCSBadRequestException($this->l->t('Not a directory')); } @@ -630,41 +630,13 @@ class ShareAPIController extends OCSController { // filter out duplicate shares $known = []; - $shares = array_filter($shares, function($share) use (&$known) { + return array_filter($shares, function($share) use (&$known) { if (in_array($share->getId(), $known)) { return false; } $known[] = $share->getId(); return true; }); - - $formatted = $miniFormatted = []; - $resharingRight = false; - foreach ($shares as $share) { - if ($share->getSharedWith() === $this->currentUser) { - continue; - } - - try { - $format = $this->formatShare($share); - - $formatted[] = $format; - if ($share->getSharedBy() === $this->currentUser) { - $miniFormatted[] = $format; - } - if (!$resharingRight && $this->shareProviderResharingRights($this->currentUser, $share, $folder)) { - $resharingRight = true; - } - } catch (\Exception $e) { - //Ignore this share - } - } - - if (!$resharingRight) { - $formatted = $miniFormatted; - } - - return new DataResponse($formatted); } /** @@ -713,35 +685,45 @@ class ShareAPIController extends OCSController { return $result; } - if ($subfiles === 'true') { - $result = $this->getSharesInDir($path); - return $result; - } - if ($reshares === 'true') { $reshares = true; } else { $reshares = false; } - // Get all shares - $shares = $this->getAllShares($path, $reshares); + if ($subfiles === 'true') { + $shares = $this->getSharesInDir($path); + $recipientNode = null; + } else { + // get all shares + $shares = $this->getAllShares($path, $reshares); + $recipientNode = $path; + } + // process all shares $formatted = $miniFormatted = []; $resharingRight = false; foreach ($shares as $share) { /** @var IShare $share */ + + // do not list the shares of the current user if ($share->getSharedWith() === $this->currentUser) { continue; } try { - $format = $this->formatShare($share, $path); + $format = $this->formatShare($share, $recipientNode); $formatted[] = $format; + + // let's also build a list of shares created + // by the current user only, in case + // there is no resharing rights if ($share->getSharedBy() === $this->currentUser) { $miniFormatted[] = $format; } + // check if one of those share is shared with me + // and if I have resharing rights on it if (!$resharingRight && $this->shareProviderResharingRights($this->currentUser, $share, $path)) { $resharingRight = true; } -- cgit v1.2.3 From d0b205d0dd59152455a4942d1882c2198623956c Mon Sep 17 00:00:00 2001 From: Daniel Calviño Sánchez Date: Fri, 25 Oct 2019 17:02:05 +0200 Subject: Add more integration tests for getting shares MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Daniel Calviño Sánchez --- .../sharing_features/sharing-v1-part2.feature | 124 +++++++++++++++++++++ .../sharing_features/sharing-v1.feature | 4 +- 2 files changed, 126 insertions(+), 2 deletions(-) diff --git a/build/integration/sharing_features/sharing-v1-part2.feature b/build/integration/sharing_features/sharing-v1-part2.feature index 49a1afe106d..c3f2c8e915e 100644 --- a/build/integration/sharing_features/sharing-v1-part2.feature +++ b/build/integration/sharing_features/sharing-v1-part2.feature @@ -20,6 +20,69 @@ Feature: sharing And User "user2" should be included in the response And User "user3" should not be included in the response + Scenario: getting all shares of a file with a received share after revoking the resharing rights + Given user "user0" exists + And user "user1" exists + And user "user2" exists + And file "textfile0.txt" of user "user1" is shared with user "user0" + And Updating last share with + | permissions | 1 | + And file "textfile0.txt" of user "user1" is shared with user "user2" + When As an "user0" + And sending "GET" to "/apps/files_sharing/api/v1/shares?reshares=true&path=/textfile0 (2).txt" + Then the list of returned shares has 1 shares + And share 0 is returned with + | share_type | 0 | + | uid_owner | user1 | + | displayname_owner | user1 | + | path | /textfile0 (2).txt | + | item_type | file | + | mimetype | text/plain | + | storage_id | shared::/textfile0 (2).txt | + | file_target | /textfile0.txt | + | share_with | user2 | + | share_with_displayname | user2 | + + Scenario: getting all shares of a file with a received share also reshared after revoking the resharing rights + Given user "user0" exists + And user "user1" exists + And user "user2" exists + And user "user3" exists + And file "textfile0.txt" of user "user1" is shared with user "user0" + And save the last share data as "textfile0.txt from user1" + And file "textfile0 (2).txt" of user "user0" is shared with user "user3" + And restore the last share data from "textfile0.txt from user1" + And Updating last share with + | permissions | 1 | + And file "textfile0.txt" of user "user1" is shared with user "user2" + When As an "user0" + And sending "GET" to "/apps/files_sharing/api/v1/shares?reshares=true&path=/textfile0 (2).txt" + Then the list of returned shares has 2 shares + And share 0 is returned with + | share_type | 0 | + | uid_owner | user0 | + | displayname_owner | user0 | + | uid_file_owner | user1 | + | displayname_file_owner | user1 | + | path | /textfile0 (2).txt | + | item_type | file | + | mimetype | text/plain | + | storage_id | shared::/textfile0 (2).txt | + | file_target | /textfile0 (2).txt | + | share_with | user3 | + | share_with_displayname | user3 | + And share 1 is returned with + | share_type | 0 | + | uid_owner | user1 | + | displayname_owner | user1 | + | path | /textfile0 (2).txt | + | item_type | file | + | mimetype | text/plain | + | storage_id | shared::/textfile0 (2).txt | + | file_target | /textfile0.txt | + | share_with | user2 | + | share_with_displayname | user2 | + Scenario: Reshared files can be still accessed if a user in the middle removes it. Given user "user0" exists And user "user1" exists @@ -301,6 +364,67 @@ Feature: sharing | share_with_displayname | user2 | | permissions | 31 | + Scenario: getting all shares including subfiles in a directory after moving a received share not reshareable also shared with another user + Given user "user0" exists + And user "user1" exists + And user "user2" exists + And file "textfile0.txt" of user "user1" is shared with user "user0" + And Updating last share with + | permissions | 1 | + And file "textfile0.txt" of user "user1" is shared with user "user2" + And User "user0" moved file "/textfile0 (2).txt" to "/FOLDER/textfile0.txt" + When As an "user0" + And sending "GET" to "/apps/files_sharing/api/v1/shares?subfiles=true&path=/FOLDER" + Then the list of returned shares has 1 shares + And share 0 is returned with + | share_type | 0 | + | uid_owner | user1 | + | displayname_owner | user1 | + | path | /FOLDER/textfile0.txt | + | item_type | file | + | mimetype | text/plain | + | storage_id | shared::/FOLDER/textfile0.txt | + | file_target | /textfile0.txt | + | share_with | user2 | + | share_with_displayname | user2 | + + Scenario: getting all shares including subfiles in a directory after moving a share and a received share not reshareable also shared with another user + Given user "user0" exists + And user "user1" exists + And user "user2" exists + And file "textfile0.txt" of user "user0" is shared with user "user1" + And file "textfile0.txt" of user "user1" is shared with user "user0" + And Updating last share with + | permissions | 1 | + And file "textfile0.txt" of user "user1" is shared with user "user2" + And User "user0" moved file "/textfile0.txt" to "/FOLDER/textfile0.txt" + And User "user0" moved file "/textfile0 (2).txt" to "/FOLDER/textfile0 (2).txt" + When As an "user0" + And sending "GET" to "/apps/files_sharing/api/v1/shares?subfiles=true&path=/FOLDER" + Then the list of returned shares has 2 shares + And share 0 is returned with + | share_type | 0 | + | uid_owner | user0 | + | displayname_owner | user0 | + | path | /FOLDER/textfile0.txt | + | item_type | file | + | mimetype | text/plain | + | storage_id | home::user0 | + | file_target | /textfile0 (2).txt | + | share_with | user1 | + | share_with_displayname | user1 | + And share 1 is returned with + | share_type | 0 | + | uid_owner | user1 | + | displayname_owner | user1 | + | path | /FOLDER/textfile0 (2).txt | + | item_type | file | + | mimetype | text/plain | + | storage_id | shared::/FOLDER/textfile0 (2).txt | + | file_target | /textfile0.txt | + | share_with | user2 | + | share_with_displayname | user2 | + Scenario: keep group permissions in sync Given As an "admin" Given user "user0" exists diff --git a/build/integration/sharing_features/sharing-v1.feature b/build/integration/sharing_features/sharing-v1.feature index 81c96b8be5c..b655a835bd7 100644 --- a/build/integration/sharing_features/sharing-v1.feature +++ b/build/integration/sharing_features/sharing-v1.feature @@ -305,7 +305,7 @@ Feature: sharing And User "user2" should be included in the response And User "user3" should not be included in the response - Scenario: getting all shares of a file with a user with resharing rights + Scenario: getting all shares of a file with a user with resharing rights but not yourself Given user "user0" exists And user "user1" exists And user "user2" exists @@ -316,7 +316,7 @@ Feature: sharing When sending "GET" to "/apps/files_sharing/api/v1/shares?path=textfile0 (2).txt&reshares=true" Then the OCS status code should be "100" And the HTTP status code should be "200" - And User "user1" should be included in the response + And User "user1" should not be included in the response And User "user2" should be included in the response And User "user3" should not be included in the response -- cgit v1.2.3