From ee674844f2e3dc1eaf8f864fcbfa4d92484075ce Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 24 Jun 2016 15:56:33 +0200 Subject: [PATCH 1/3] better detect errors in fed sharing response --- core/js/shareitemmodel.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/js/shareitemmodel.js b/core/js/shareitemmodel.js index 3ced66a1a7..9f75ac42f2 100644 --- a/core/js/shareitemmodel.js +++ b/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; } From 8d32e1d35b05343ed17c96cbc5f5f360b88c1241 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 24 Jun 2016 15:59:32 +0200 Subject: [PATCH 2/3] Handle exceptions thrown while trying to notify remote server of a fed share --- .../lib/FederatedShareProvider.php | 58 +++++++++++------- .../tests/FederatedShareProviderTest.php | 60 +++++++++++++++++-- 2 files changed, 91 insertions(+), 27 deletions(-) diff --git a/apps/federatedfilesharing/lib/FederatedShareProvider.php b/apps/federatedfilesharing/lib/FederatedShareProvider.php index 4892908c32..c5477ec8fe 100644 --- a/apps/federatedfilesharing/lib/FederatedShareProvider.php +++ b/apps/federatedfilesharing/lib/FederatedShareProvider.php @@ -217,28 +217,31 @@ 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->removeShareFromTableById($shareId); + throw $e; } return $shareId; @@ -526,13 +529,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(); } diff --git a/apps/federatedfilesharing/tests/FederatedShareProviderTest.php b/apps/federatedfilesharing/tests/FederatedShareProviderTest.php index 6792e534cc..8c5efdab7b 100644 --- a/apps/federatedfilesharing/tests/FederatedShareProviderTest.php +++ b/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(); From 646c90cc4a92775e4278f9f8237b5c4173665403 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 27 Jun 2016 16:38:47 +0200 Subject: [PATCH 3/3] Log reason why we removed the fed share --- apps/federatedfilesharing/lib/FederatedShareProvider.php | 1 + 1 file changed, 1 insertion(+) diff --git a/apps/federatedfilesharing/lib/FederatedShareProvider.php b/apps/federatedfilesharing/lib/FederatedShareProvider.php index c5477ec8fe..36ccb7e412 100644 --- a/apps/federatedfilesharing/lib/FederatedShareProvider.php +++ b/apps/federatedfilesharing/lib/FederatedShareProvider.php @@ -240,6 +240,7 @@ class FederatedShareProvider implements IShareProvider { 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; }