Merge pull request #4961 from owncloud/unshare-over-delete

Use OCP\Share::unshare() instead of directly using OCP\Share::delete()

2 files changed, 85 insertions, 54 deletions

diff --git a/lib/public/share.php b/lib/public/share.php
index e4f68017d26..7f4d918757f 100644
--- a/lib/public/share.php
+++ b/lib/public/share.php
@@ -345,16 +345,9 @@ class Share {
 			\OC_Log::write('OCP\Share', \OC_DB::getErrorMessage($result) . ', token=' . $token, \OC_Log::ERROR);
 		}
 		$row = $result->fetchRow();
-
-		if (!empty($row['expiration'])) {
-			$now = new \DateTime();
-			$expirationDate = new \DateTime($row['expiration'], new \DateTimeZone('UTC'));
-			if ($now > $expirationDate) {
-				self::delete($row['id']);
-				return false;
-			}
+		if (self::expireItem($row)) {
+			return false;
 		}
-
 		return $row;
 	}
 
@@ -634,23 +627,7 @@ class Share {
 	public static function unshare($itemType, $itemSource, $shareType, $shareWith) {
 		if ($item = self::getItems($itemType, $itemSource, $shareType, $shareWith, \OC_User::getUser(),
 			self::FORMAT_NONE, null, 1)) {
-			// Pass all the vars we have for now, they may be useful
-			\OC_Hook::emit('OCP\Share', 'pre_unshare', array(
-				'itemType' => $itemType,
-				'itemSource' => $itemSource,
-				'fileSource' => $item['file_source'],
-				'shareType' => $shareType,
-				'shareWith' => $shareWith,
-				'itemParent' => $item['parent'],
-			));
-			self::delete($item['id']);
-			\OC_Hook::emit('OCP\Share', 'post_unshare', array(
-					'itemType' => $itemType,
-					'itemSource' => $itemSource,
-					'shareType' => $shareType,
-					'shareWith' => $shareWith,
-					'itemParent' => $item['parent'],
-			));
+			self::unshareItem($item);
 			return true;
 		}
 		return false;
@@ -665,19 +642,16 @@ class Share {
 	public static function unshareAll($itemType, $itemSource) {
 		if ($shares = self::getItemShared($itemType, $itemSource)) {
 			// Pass all the vars we have for now, they may be useful
-			\OC_Hook::emit('OCP\Share', 'pre_unshareAll', array(
+			$hookParams = array(
 				'itemType' => $itemType,
 				'itemSource' => $itemSource,
-				'shares' => $shares
-			));
+				'shares' => $shares,
+			);
+			\OC_Hook::emit('OCP\Share', 'pre_unshareAll', $hookParams);
 			foreach ($shares as $share) {
-				self::delete($share['id']);
+				self::unshareItem($share);
 			}
-			\OC_Hook::emit('OCP\Share', 'post_unshareAll', array(
-					'itemType' => $itemType,
-					'itemSource' => $itemSource,
-					'shares' => $shares
-			));
+			\OC_Hook::emit('OCP\Share', 'post_unshareAll', $hookParams);
 			return true;
 		}
 		return false;
@@ -853,6 +827,44 @@ class Share {
 	}
 
 	/**
+	 * Checks whether a share has expired, calls unshareItem() if yes.
+	 * @param array $item Share data (usually database row)
+	 * @return bool True if item was expired, false otherwise.
+	 */
+	protected static function expireItem(array $item) {
+		if (!empty($item['expiration'])) {
+			$now = new \DateTime();
+			$expirationDate = new \DateTime($item['expiration'], new \DateTimeZone('UTC'));
+			if ($now > $expirationDate) {
+				self::unshareItem($item);
+				return true;
+			}
+		}
+		return false;
+	}
+
+	/**
+	 * Unshares a share given a share data array
+	 * @param array $item Share data (usually database row)
+	 * @return null
+	 */
+	protected static function unshareItem(array $item) {
+		// Pass all the vars we have for now, they may be useful
+		$hookParams = array(
+			'itemType'		=> $item['item_type'],
+			'itemSource'	=> $item['item_source'],
+			'shareType'		=> $item['share_type'],
+			'shareWith'		=> $item['share_with'],
+			'itemParent'	=> $item['parent'],
+		);
+		\OC_Hook::emit('OCP\Share', 'pre_unshare', $hookParams + array(
+			'fileSource'	=> $item['file_source'],
+		));
+		self::delete($item['id']);
+		\OC_Hook::emit('OCP\Share', 'post_unshare', $hookParams);
+	}
+
+	/**
 	 * Get the backend class for the specified item type
 	 * @param string $itemType
 	 * @return Share_Backend
@@ -1079,7 +1091,7 @@ class Share {
 		// TODO Optimize selects
 		if ($format == self::FORMAT_STATUSES) {
 			if ($itemType == 'file' || $itemType == 'folder') {
-				$select = '`*PREFIX*share`.`id`, `item_type`, `*PREFIX*share`.`parent`,'
+				$select = '`*PREFIX*share`.`id`, `item_type`, `item_source`, `*PREFIX*share`.`parent`,'
 					.' `share_type`, `file_source`, `path`, `expiration`, `storage`, `mail_send`';
 			} else {
 				$select = '`id`, `item_type`, `item_source`, `parent`, `share_type`, `expiration`, `mail_send`';
@@ -1087,7 +1099,7 @@ class Share {
 		} else {
 			if (isset($uidOwner)) {
 				if ($itemType == 'file' || $itemType == 'folder') {
-					$select = '`*PREFIX*share`.`id`, `item_type`, `*PREFIX*share`.`parent`,'
+					$select = '`*PREFIX*share`.`id`, `item_type`, `item_source`, `*PREFIX*share`.`parent`,'
 						.' `share_type`, `share_with`, `file_source`, `path`, `permissions`, `stime`,'
 						.' `expiration`, `token`, `storage`, `mail_send`';
 				} else {
@@ -1100,7 +1112,7 @@ class Share {
 						&& $format == \OC_Share_Backend_File::FORMAT_GET_FOLDER_CONTENTS
 						|| $format == \OC_Share_Backend_File::FORMAT_FILE_APP_ROOT
 					) {
-						$select = '`*PREFIX*share`.`id`, `item_type`, `*PREFIX*share`.`parent`, `uid_owner`, '
+						$select = '`*PREFIX*share`.`id`, `item_type`, `item_source`, `*PREFIX*share`.`parent`, `uid_owner`, '
 							.'`share_type`, `share_with`, `file_source`, `path`, `file_target`, '
 							.'`permissions`, `expiration`, `storage`, `*PREFIX*filecache`.`parent` as `file_parent`, '
 							.'`name`, `mtime`, `mimetype`, `mimepart`, `size`, `encrypted`, `etag`, `mail_send`';
@@ -1206,12 +1218,8 @@ class Share {
 					}
 				}
 			}
-			if (isset($row['expiration'])) {
-				$time = new \DateTime();
-				if ($row['expiration'] < date('Y-m-d H:i', $time->format('U') - $time->getOffset())) {
-					self::delete($row['id']);
-					continue;
-				}
+			if (self::expireItem($row)) {
+				continue;
 			}
 			// Check if resharing is allowed, if not remove share permission
 			if (isset($row['permissions']) && !self::isResharingAllowed()) {
diff --git a/tests/lib/share/share.php b/tests/lib/share/share.php
index 8e9eef65d32..84e2e5c63eb 100644
--- a/tests/lib/share/share.php
+++ b/tests/lib/share/share.php
@@ -135,15 +135,15 @@ class Test_Share extends PHPUnit_Framework_TestCase {
 			OCP\Share::shareItem('test', 'test.txt', OCP\Share::SHARE_TYPE_USER, $this->user2, OCP\PERMISSION_READ),
 			'Failed asserting that user 1 successfully shared text.txt with user 2.'
 		);
-		$this->assertEquals(
-			array('test.txt'),
+		$this->assertContains(
+			'test.txt',
 			OCP\Share::getItemShared('test', 'test.txt', Test_Share_Backend::FORMAT_SOURCE),
 			'Failed asserting that test.txt is a shared file of user 1.'
 		);
 
 		OC_User::setUserId($this->user2);
-		$this->assertEquals(
-			array('test.txt'),
+		$this->assertContains(
+			'test.txt',
 			OCP\Share::getItemSharedWith('test', 'test.txt', Test_Share_Backend::FORMAT_SOURCE),
 			'Failed asserting that user 2 has access to test.txt after initial sharing.'
 		);
@@ -328,22 +328,22 @@ class Test_Share extends PHPUnit_Framework_TestCase {
 			OCP\Share::shareItem('test', 'test.txt', OCP\Share::SHARE_TYPE_GROUP, $this->group1, OCP\PERMISSION_READ),
 			'Failed asserting that user 1 successfully shared text.txt with group 1.'
 		);
-		$this->assertEquals(
-			array('test.txt'),
+		$this->assertContains(
+			'test.txt',
 			OCP\Share::getItemShared('test', 'test.txt', Test_Share_Backend::FORMAT_SOURCE),
 			'Failed asserting that test.txt is a shared file of user 1.'
 		);
 
 		OC_User::setUserId($this->user2);
-		$this->assertEquals(
-			array('test.txt'),
+		$this->assertContains(
+			'test.txt',
 			OCP\Share::getItemSharedWith('test', 'test.txt', Test_Share_Backend::FORMAT_SOURCE),
 			'Failed asserting that user 2 has access to test.txt after initial sharing.'
 		);
 
 		OC_User::setUserId($this->user3);
-		$this->assertEquals(
-			array('test.txt'),
+		$this->assertContains(
+			'test.txt',
 			OCP\Share::getItemSharedWith('test', 'test.txt', Test_Share_Backend::FORMAT_SOURCE),
 			'Failed asserting that user 3 has access to test.txt after initial sharing.'
 		);
@@ -583,4 +583,27 @@ class Test_Share extends PHPUnit_Framework_TestCase {
 			'Failed asserting that an expired share could not be found.'
 		);
 	}
+
+	public function testUnshareAll() {
+		$this->shareUserOneTestFileWithUserTwo();
+		$this->shareUserOneTestFileWithGroupOne();
+
+		OC_User::setUserId($this->user1);
+		$this->assertEquals(
+			array('test.txt', 'test.txt'),
+			OCP\Share::getItemsShared('test', 'test.txt'),
+			'Failed asserting that the test.txt file is shared exactly two times.'
+		);
+
+		$this->assertTrue(
+			OCP\Share::unshareAll('test', 'test.txt'),
+			'Failed asserting that user 1 successfully unshared all shares of the test.txt share.'
+		);
+
+		$this->assertEquals(
+			array(),
+			OCP\Share::getItemsShared('test'),
+			'Failed asserting that both shares of the test.txt file have been removed.'
+		);
+	}
 }