Merge pull request #25262 from owncloud/fed-sharing-error

Only save federated share after remote server is notified

apps/federatedfilesharing/lib/FederatedShareProvider.php

@@ -217,28 +217,32 @@ class FederatedShareProvider implements IShareProvider {
 			$share->getPermissions(),
 			$token
 		);
-		$sharedByFederatedId = $share->getSharedBy();
-		if ($this->userManager->userExists($sharedByFederatedId)) {
-			$sharedByFederatedId = $sharedByFederatedId . '@' . $this->addressHandler->generateRemoteURL();
-		}
-		$send = $this->notifications->sendRemoteShare(
-			$token,
-			$share->getSharedWith(),
-			$share->getNode()->getName(),
-			$shareId,
-			$share->getShareOwner(),
-			$share->getShareOwner() . '@' . $this->addressHandler->generateRemoteURL(),
-			$share->getSharedBy(),
-			$sharedByFederatedId
-		);
 
-		if ($send === false) {
-			$data = $this->getRawShare($shareId);
-			$share = $this->createShareObject($data);
-			$this->removeShareFromTable($share);
-			$message_t = $this->l->t('Sharing %s failed, could not find %s, maybe the server is currently unreachable.',
-				[$share->getNode()->getName(), $share->getSharedWith()]);
-			throw new \Exception($message_t);
+		try {
+			$sharedByFederatedId = $share->getSharedBy();
+			if ($this->userManager->userExists($sharedByFederatedId)) {
+				$sharedByFederatedId = $sharedByFederatedId . '@' . $this->addressHandler->generateRemoteURL();
+			}
+			$send = $this->notifications->sendRemoteShare(
+				$token,
+				$share->getSharedWith(),
+				$share->getNode()->getName(),
+				$shareId,
+				$share->getShareOwner(),
+				$share->getShareOwner() . '@' . $this->addressHandler->generateRemoteURL(),
+				$share->getSharedBy(),
+				$sharedByFederatedId
+			);
+
+			if ($send === false) {
+				$message_t = $this->l->t('Sharing %s failed, could not find %s, maybe the server is currently unreachable.',
+					[$share->getNode()->getName(), $share->getSharedWith()]);
+				throw new \Exception($message_t);
+			}
+		} catch (\Exception $e) {
+			$this->logger->error('Failed to notify remote server of federated share, removing share (' . $e->getMessage() . ')');
+			$this->removeShareFromTableById($shareId);
+			throw $e;
 		}
 
 		return $shareId;
@@ -526,13 +530,22 @@ class FederatedShareProvider implements IShareProvider {
 	 * @param IShare $share
 	 */
 	public function removeShareFromTable(IShare $share) {
+		$this->removeShareFromTableById($share->getId());
+	}
+
+	/**
+	 * remove share from table
+	 *
+	 * @param string $shareId
+	 */
+	private function removeShareFromTableById($shareId) {
 		$qb = $this->dbConnection->getQueryBuilder();
 		$qb->delete('share')
-			->where($qb->expr()->eq('id', $qb->createNamedParameter($share->getId())));
+			->where($qb->expr()->eq('id', $qb->createNamedParameter($shareId)));
 		$qb->execute();
 
 		$qb->delete('federated_reshares')
-			->where($qb->expr()->eq('share_id', $qb->createNamedParameter($share->getId())));
+			->where($qb->expr()->eq('share_id', $qb->createNamedParameter($shareId)));
 		$qb->execute();
 	}
 

apps/federatedfilesharing/tests/FederatedShareProviderTest.php

@@ -217,10 +217,6 @@ class FederatedShareProviderTest extends \Test\TestCase {
 				'sharedBy@http://localhost/'
 			)->willReturn(false);
 
-		$this->rootFolder->expects($this->once())
-			->method('getUserFolder')
-			->with('shareOwner')
-			->will($this->returnSelf());
 		$this->rootFolder->method('getById')
 			->with('42')
 			->willReturn([$node]);
@@ -244,6 +240,62 @@ class FederatedShareProviderTest extends \Test\TestCase {
 		$this->assertFalse($data);
 	}
 
+	public function testCreateException() {
+		$share = $this->shareManager->newShare();
+
+		$node = $this->getMock('\OCP\Files\File');
+		$node->method('getId')->willReturn(42);
+		$node->method('getName')->willReturn('myFile');
+
+		$share->setSharedWith('user@server.com')
+			->setSharedBy('sharedBy')
+			->setShareOwner('shareOwner')
+			->setPermissions(19)
+			->setNode($node);
+
+		$this->tokenHandler->method('generateToken')->willReturn('token');
+
+		$this->addressHandler->expects($this->any())->method('generateRemoteURL')
+			->willReturn('http://localhost/');
+		$this->addressHandler->expects($this->any())->method('splitUserRemote')
+			->willReturn(['user', 'server.com']);
+
+		$this->notifications->expects($this->once())
+			->method('sendRemoteShare')
+			->with(
+				$this->equalTo('token'),
+				$this->equalTo('user@server.com'),
+				$this->equalTo('myFile'),
+				$this->anything(),
+				'shareOwner',
+				'shareOwner@http://localhost/',
+				'sharedBy',
+				'sharedBy@http://localhost/'
+			)->willThrowException(new \Exception('dummy'));
+
+		$this->rootFolder->method('getById')
+			->with('42')
+			->willReturn([$node]);
+
+		try {
+			$share = $this->provider->create($share);
+			$this->fail();
+		} catch (\Exception $e) {
+			$this->assertEquals('dummy', $e->getMessage());
+		}
+
+		$qb = $this->connection->getQueryBuilder();
+		$stmt = $qb->select('*')
+			->from('share')
+			->where($qb->expr()->eq('id', $qb->createNamedParameter($share->getId())))
+			->execute();
+
+		$data = $stmt->fetch();
+		$stmt->closeCursor();
+
+		$this->assertFalse($data);
+	}
+
 	public function testCreateShareWithSelf() {
 		$share = $this->shareManager->newShare();
 

core/js/shareitemmodel.js

@@ -187,7 +187,7 @@
 			}).fail(function(xhr) {
 				var msg = t('core', 'Error');
 				var result = xhr.responseJSON;
-				if (result.ocs && result.ocs.meta) {
+				if (result && result.ocs && result.ocs.meta) {
 					msg = result.ocs.meta.message;
 				}