update unit tests

Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>

10 files changed, 55 insertions, 98 deletions

diff --git a/apps/federatedfilesharing/tests/Controller/RequestHandlerControllerTest.php b/apps/federatedfilesharing/tests/Controller/RequestHandlerControllerTest.php
index 61f87e9ec67..233395dec9f 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 b810a515082..bc525b6ef82 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 540905a7dc9..046ede1f83e 100644
--- a/apps/files_sharing/tests/ApiTest.php
+++ b/apps/files_sharing/tests/ApiTest.php
@@ -938,36 +938,6 @@ class ApiTest extends TestCase {
 
 	/**
 	 * @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
 	 */
 	function testUpdateShareUpload() {
 		$node1 = $this->userFolder->get($this->folder);
diff --git a/apps/files_sharing/tests/MigrationTest.php b/apps/files_sharing/tests/MigrationTest.php
index a357e814f25..708de1c0eca 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 4ec62dc1a03..288ebb4bb45 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 3c234f54665..573d5949118 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 0d02123e001..bf8bcc9c6d9 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 0dade2f7126..292b07d28d5 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;
 				}
 
-				if (count($shares2) === $limit) {
-					break;
-				}
+				$added++;
+				$shares2[] = $share;
 
-				// If there was no limit on the select we are done
-				if ($limit === -1) {
+				if (count($shares2) === $limit) {
 					break;
 				}
+			}
 
-				$offset += $added;
-
-				// Fetch again $limit shares
-				$shares = $provider->getSharesBy($userId, $shareType, $path, $reshares, $limit, $offset);
+			if (count($shares2) === $limit) {
+				break;
+			}
 
-				// No more shares means we are done
-				if (empty($shares)) {
-					break;
-				}
+			// If there was no limit on the select we are done
+			if ($limit === -1) {
+				break;
 			}
 
-			$shares = $shares2;
-		}
+			$offset += $added;
 
+			// Fetch again $limit shares
+			$shares = $provider->getSharesBy($userId, $shareType, $path, $reshares, $limit, $offset);
 
-		// 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
-				}
+			// 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 5b552b51c3c..8deec573c1b 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 a1e78313558..fce5668440d 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');