diff --git a/apps/federatedfilesharing/tests/Controller/RequestHandlerControllerTest.php b/apps/federatedfilesharing/tests/Controller/RequestHandlerControllerTest.php index 61f87e9ec6..233395dec9 100644 --- a/apps/federatedfilesharing/tests/Controller/RequestHandlerControllerTest.php +++ b/apps/federatedfilesharing/tests/Controller/RequestHandlerControllerTest.php @@ -67,7 +67,7 @@ class RequestHandlerControllerTest extends TestCase { /** @var \OCA\FederatedFileSharing\AddressHandler|\PHPUnit_Framework_MockObject_MockObject */ private $addressHandler; - + /** @var IUserManager|\PHPUnit_Framework_MockObject_MockObject */ private $userManager; @@ -107,7 +107,7 @@ class RequestHandlerControllerTest extends TestCase { $this->userManager = $this->getMockBuilder('OCP\IUserManager')->getMock(); $this->cloudIdManager = new CloudIdManager(); - + $this->registerHttpHelper($httpHelperMock); $this->s2s = new RequestHandlerController( @@ -384,6 +384,7 @@ class RequestHandlerControllerTest extends TestCase { 'parent' => null, 'accepted' => '0', 'expiration' => null, + 'password' => null, 'mail_send' => '0' ]; diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php index b810a51508..bc525b6ef8 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -669,13 +669,14 @@ class ShareAPIController extends OCSController { throw new OCSNotFoundException($this->l->t('Wrong share ID, share doesn\'t exist')); } + if ($permissions === null && $password === null && $publicUpload === null && $expireDate === null) { + throw new OCSBadRequestException($this->l->t('Wrong or no update parameter given')); + } + /* * expirationdate, password and publicUpload only make sense for link shares */ if ($share->getShareType() === \OCP\Share::SHARE_TYPE_LINK) { - if ($permissions === null && $password === null && $publicUpload === null && $expireDate === null) { - throw new OCSBadRequestException($this->l->t('Wrong or no update parameter given')); - } $newPermissions = null; if ($publicUpload === 'true') { diff --git a/apps/files_sharing/tests/ApiTest.php b/apps/files_sharing/tests/ApiTest.php index 540905a7dc..046ede1f83 100644 --- a/apps/files_sharing/tests/ApiTest.php +++ b/apps/files_sharing/tests/ApiTest.php @@ -936,36 +936,6 @@ class ApiTest extends TestCase { $this->shareManager->deleteShare($share2); } - /** - * @medium - * @depends testCreateShareUserFile - */ - public function testUpdateShareInvalidPermissions() { - $node1 = $this->userFolder->get($this->filename); - $share1 = $this->shareManager->newShare(); - $share1->setNode($node1) - ->setSharedBy(self::TEST_FILES_SHARING_API_USER1) - ->setSharedWith(self::TEST_FILES_SHARING_API_USER2) - ->setShareType(\OCP\Share::SHARE_TYPE_USER) - ->setPermissions(19); - $share1 = $this->shareManager->createShare($share1); - - $ocs = $this->createOCS(self::TEST_FILES_SHARING_API_USER1); - try { - $ocs->updateShare($share1->getId()); - $this->fail(); - } catch (OCSBadRequestException $e) { - - } - $ocs->cleanup(); - - //Permissions should not have changed! - $share1 = $this->shareManager->getShareById('ocinternal:' . $share1->getId()); - $this->assertEquals(19, $share1->getPermissions()); - - $this->shareManager->deleteShare($share1); - } - /** * @medium */ diff --git a/apps/files_sharing/tests/MigrationTest.php b/apps/files_sharing/tests/MigrationTest.php index a357e814f2..708de1c0ec 100644 --- a/apps/files_sharing/tests/MigrationTest.php +++ b/apps/files_sharing/tests/MigrationTest.php @@ -429,11 +429,11 @@ class MigrationTest extends TestCase { foreach ($allShares as $share) { if ((int)$share['share_type'] === Share::SHARE_TYPE_LINK) { - $this->assertSame(null, $share['share_with']); + $this->assertNull( $share['share_with']); $this->assertSame('shareWith', $share['password']); } else { $this->assertSame('shareWith', $share['share_with']); - $this->assertSame(null, $share['password']); + $this->assertNull($share['password']); } } } diff --git a/apps/sharebymail/tests/ShareByMailProviderTest.php b/apps/sharebymail/tests/ShareByMailProviderTest.php index 4ec62dc1a0..288ebb4bb4 100644 --- a/apps/sharebymail/tests/ShareByMailProviderTest.php +++ b/apps/sharebymail/tests/ShareByMailProviderTest.php @@ -23,7 +23,6 @@ namespace OCA\ShareByMail\Tests; -use OC\HintException; use OCA\ShareByMail\Settings\SettingsManager; use OCA\ShareByMail\ShareByMailProvider; use OCP\Files\IRootFolder; @@ -33,9 +32,7 @@ use OCP\ILogger; use OCP\IURLGenerator; use OCP\IUserManager; use OCP\Mail\IMailer; -use OCP\Security\IHasher; use OCP\Security\ISecureRandom; -use OCP\Share\Exceptions\ShareNotFound; use OCP\Share\IManager; use OCP\Share\IShare; use Test\TestCase; @@ -127,7 +124,8 @@ class ShareByMailProviderTest extends TestCase { $this->logger, $this->mailer, $this->urlGenerator, - $this->activityManager + $this->activityManager, + $this->settingsManager ] ); @@ -318,7 +316,7 @@ class ShareByMailProviderTest extends TestCase { $this->share->expects($this->once())->method('getPermissions')->willReturn($permissions + 1); $this->share->expects($this->once())->method('getShareOwner')->willReturn($uidOwner); $this->share->expects($this->once())->method('getSharedBy')->willReturn($sharedBy); - $this->share->expects($this->once())->method('getId')->willReturn($id); + $this->share->expects($this->atLeastOnce())->method('getId')->willReturn($id); $this->assertSame($this->share, $instance->update($this->share) diff --git a/core/js/sharedialogshareelistview.js b/core/js/sharedialogshareelistview.js index 3c234f5466..573d594911 100644 --- a/core/js/sharedialogshareelistview.js +++ b/core/js/sharedialogshareelistview.js @@ -510,7 +510,7 @@ passwordField.attr('value', ''); passwordField.attr('placeholder', PASSWORD_PLACEHOLDER_MESSAGE); } else { - var passwordField = '#passwordField-' + this.cid + '-' + shareId; + passwordField = '#passwordField-' + this.cid + '-' + shareId; this.$(passwordField).focus(); } }, @@ -623,10 +623,9 @@ var $li = $element.closest('li[data-share-id]'); var shareId = $li.data('share-id'); + var permissions = OC.PERMISSION_CREATE | OC.PERMISSION_UPDATE | OC.PERMISSION_DELETE | OC.PERMISSION_READ; if ($element.is(':checked')) { - var permissions = OC.PERMISSION_CREATE | OC.PERMISSION_UPDATE | OC.PERMISSION_DELETE; - } else { - var permissions = OC.PERMISSION_CREATE | OC.PERMISSION_UPDATE | OC.PERMISSION_DELETE | OC.PERMISSION_READ; + permissions = OC.PERMISSION_CREATE | OC.PERMISSION_UPDATE | OC.PERMISSION_DELETE; } /** disable checkboxes during save operation to avoid race conditions **/ diff --git a/lib/private/Share20/DefaultShareProvider.php b/lib/private/Share20/DefaultShareProvider.php index 0d02123e00..bf8bcc9c6d 100644 --- a/lib/private/Share20/DefaultShareProvider.php +++ b/lib/private/Share20/DefaultShareProvider.php @@ -116,7 +116,7 @@ class DefaultShareProvider implements IShareProvider { //If a password is set store it if ($share->getPassword() !== null) { - $qb->setValue('share_with', $qb->createNamedParameter($share->getPassword())); + $qb->setValue('password', $qb->createNamedParameter($share->getPassword())); } //If an expiration date is set store it diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 0dade2f712..292b07d28d 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -266,8 +266,8 @@ class Manager implements IManager { // Check that read permissions are always set // Link shares are allowed to have no read permissions to allow upload to hidden folders - $noReadPermissionRequired = $share->getShareType() !== \OCP\Share::SHARE_TYPE_LINK - || $share->getShareType() !== \OCP\Share::SHARE_TYPE_EMAIL; + $noReadPermissionRequired = $share->getShareType() === \OCP\Share::SHARE_TYPE_LINK + || $share->getShareType() === \OCP\Share::SHARE_TYPE_EMAIL; if (!$noReadPermissionRequired && ($share->getPermissions() & \OCP\Constants::PERMISSION_READ) === 0) { throw new \InvalidArgumentException('Shares need at least read permissions'); @@ -936,59 +936,49 @@ class Manager implements IManager { * Work around so we don't return expired shares but still follow * proper pagination. */ - if ($shareType === \OCP\Share::SHARE_TYPE_LINK) { - $shares2 = []; - while(true) { - $added = 0; - foreach ($shares as $share) { + $shares2 = []; - $added++; - $shares2[] = $share; + while(true) { + $added = 0; + foreach ($shares as $share) { - if (count($shares2) === $limit) { - break; - } + try { + $this->checkExpireDate($share); + } catch (ShareNotFound $e) { + //Ignore since this basically means the share is deleted + continue; } + $added++; + $shares2[] = $share; + if (count($shares2) === $limit) { break; } - - // If there was no limit on the select we are done - if ($limit === -1) { - break; - } - - $offset += $added; - - // Fetch again $limit shares - $shares = $provider->getSharesBy($userId, $shareType, $path, $reshares, $limit, $offset); - - // No more shares means we are done - if (empty($shares)) { - break; - } } - $shares = $shares2; - } + if (count($shares2) === $limit) { + break; + } + // If there was no limit on the select we are done + if ($limit === -1) { + break; + } - // remove all shares which are already expired - foreach ($shares as $key => $share) { - try { - $this->checkExpireDate($share); - } catch (ShareNotFound $e) { - unset($shares[$key]); - try { - $this->deleteShare($share); - } catch (NotFoundException $e) { - //Ignore since this basically means the share is deleted - } + $offset += $added; + + // Fetch again $limit shares + $shares = $provider->getSharesBy($userId, $shareType, $path, $reshares, $limit, $offset); + + // No more shares means we are done + if (empty($shares)) { + break; } } + $shares = $shares2; return $shares; } @@ -1011,11 +1001,6 @@ class Manager implements IManager { $this->checkExpireDate($share); } catch (ShareNotFound $e) { unset($shares[$key]); - try { - $this->deleteShare($share); - } catch (NotFoundException $e) { - //Ignore since this basically means the share is deleted - } } } diff --git a/lib/public/Share/IShare.php b/lib/public/Share/IShare.php index 5b552b51c3..8deec573c1 100644 --- a/lib/public/Share/IShare.php +++ b/lib/public/Share/IShare.php @@ -189,7 +189,7 @@ interface IShare { /** * Set the expiration date * - * @param \DateTime $expireDate + * @param null|\DateTime $expireDate * @return \OCP\Share\IShare The modified object * @since 9.0.0 */ diff --git a/tests/lib/Share20/DefaultShareProviderTest.php b/tests/lib/Share20/DefaultShareProviderTest.php index a1e7831355..fce5668440 100644 --- a/tests/lib/Share20/DefaultShareProviderTest.php +++ b/tests/lib/Share20/DefaultShareProviderTest.php @@ -338,7 +338,7 @@ class DefaultShareProviderTest extends \Test\TestCase { $qb->insert('share') ->values([ 'share_type' => $qb->expr()->literal(\OCP\Share::SHARE_TYPE_LINK), - 'share_with' => $qb->expr()->literal('sharedWith'), + 'password' => $qb->expr()->literal('password'), 'uid_owner' => $qb->expr()->literal('shareOwner'), 'uid_initiator' => $qb->expr()->literal('sharedBy'), 'item_type' => $qb->expr()->literal('file'), @@ -366,7 +366,8 @@ class DefaultShareProviderTest extends \Test\TestCase { $this->assertEquals($id, $share->getId()); $this->assertEquals(\OCP\Share::SHARE_TYPE_LINK, $share->getShareType()); - $this->assertEquals('sharedWith', $share->getPassword()); + $this->assertNull($share->getSharedWith()); + $this->assertEquals('password', $share->getPassword()); $this->assertEquals('sharedBy', $share->getSharedBy()); $this->assertEquals('shareOwner', $share->getShareOwner()); $this->assertEquals($ownerPath, $share->getNode()); @@ -752,7 +753,7 @@ class DefaultShareProviderTest extends \Test\TestCase { $qb->insert('share') ->values([ 'share_type' => $qb->expr()->literal(\OCP\Share::SHARE_TYPE_LINK), - 'share_with' => $qb->expr()->literal('password'), + 'password' => $qb->expr()->literal('password'), 'uid_owner' => $qb->expr()->literal('shareOwner'), 'uid_initiator' => $qb->expr()->literal('sharedBy'), 'item_type' => $qb->expr()->literal('file'), @@ -814,7 +815,7 @@ class DefaultShareProviderTest extends \Test\TestCase { ['home::shareOwner', 'files/test.txt', 'files/test2.txt'], // regular file on external storage ['smb::whatever', 'files/test.txt', 'files/test2.txt'], - // regular file on external storage in trashbin-like folder, + // regular file on external storage in trashbin-like folder, ['smb::whatever', 'files_trashbin/files/test.txt', 'files_trashbin/files/test2.txt'], ]; } @@ -2353,9 +2354,11 @@ class DefaultShareProviderTest extends \Test\TestCase { $rootFolder ); - $u1 = $userManager->createUser('testShare1', 'test'); - $u2 = $userManager->createUser('testShare2', 'test'); - $u3 = $userManager->createUser('testShare3', 'test'); + $password = md5(time()); + + $u1 = $userManager->createUser('testShare1', $password); + $u2 = $userManager->createUser('testShare2', $password); + $u3 = $userManager->createUser('testShare3', $password); $g1 = $groupManager->createGroup('group1');