From 26e4c292c7284ce6e1e3ddb52ae02123f19482b1 Mon Sep 17 00:00:00 2001 From: Daniel Calviño Sánchez Date: Thu, 28 May 2020 20:29:28 +0200 Subject: Fix enabling send password by Talk with empty password in link shares MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When "send password by Talk" is enabled in a link share now a non empty password is enforced. Signed-off-by: Daniel Calviño Sánchez --- lib/private/Share20/Manager.php | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'lib') diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 73860fd39f4..f88d884c0db 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -972,8 +972,14 @@ class Manager implements IManager { } else if ($share->getShareType() === \OCP\Share::SHARE_TYPE_LINK) { $this->linkCreateChecks($share); + $plainTextPassword = $share->getPassword(); + $this->updateSharePasswordIfNeeded($share, $originalShare); + if (empty($plainTextPassword) && $share->getSendPasswordByTalk()) { + throw new \InvalidArgumentException('Can’t enable sending the password by Talk with an empty password'); + } + if ($share->getExpirationDate() != $originalShare->getExpirationDate()) { //Verify the expiration date $this->validateExpirationDate($share); -- cgit v1.2.3 From 1c580351da069bbdfdb55559ca6800f11c6d6090 Mon Sep 17 00:00:00 2001 From: Daniel Calviño Sánchez Date: Thu, 28 May 2020 20:37:18 +0200 Subject: Fix enabling send password by Talk with same password in mail shares MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When "send password by Talk" is enabled in a mail share a new password must be also set. However, when the passwords of the original and the new share were compared it was not taken into account that the original password is now hashed, while the new one is not (unless no new password was sent, in which case the password of the original share was set in the new share by the controller, but that was already prevented due to both passwords being literally the same), so it was possible to set the same password again. Signed-off-by: Daniel Calviño Sánchez --- lib/private/Share20/Manager.php | 12 ++++- tests/lib/Share20/ManagerTest.php | 100 ++++++++++++++++++++++++++++++++++++-- 2 files changed, 107 insertions(+), 5 deletions(-) (limited to 'lib') diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index f88d884c0db..08dca45a299 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1085,8 +1085,14 @@ class Manager implements IManager { * @return boolean whether the password was updated or not. */ private function updateSharePasswordIfNeeded(\OCP\Share\IShare $share, \OCP\Share\IShare $originalShare) { + $passwordsAreDifferent = ($share->getPassword() !== $originalShare->getPassword()) && + (($share->getPassword() !== null && $originalShare->getPassword() === null) || + ($share->getPassword() === null && $originalShare->getPassword() !== null) || + ($share->getPassword() !== null && $originalShare->getPassword() !== null && + !$this->hasher->verify($share->getPassword(), $originalShare->getPassword()))); + // Password updated. - if ($share->getPassword() !== $originalShare->getPassword()) { + if ($passwordsAreDifferent) { //Verify the password $this->verifyPassword($share->getPassword()); @@ -1096,6 +1102,10 @@ class Manager implements IManager { return true; } + } else { + // Reset the password to the original one, as it is either the same + // as the "new" password or a hashed version of it. + $share->setPassword($originalShare->getPassword()); } return false; diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index 962974b231d..e0a0d9bfad2 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -2954,6 +2954,88 @@ class ManagerTest extends \Test\TestCase { $manager->updateShare($share); } + public function testUpdateShareMailEnableSendPasswordByTalkWithDifferentPassword() { + $manager = $this->createManagerMock() + ->setMethods([ + 'canShare', + 'getShareById', + 'generalCreateChecks', + 'verifyPassword', + 'pathCreateChecks', + 'linkCreateChecks', + 'validateExpirationDate', + ]) + ->getMock(); + + $originalShare = $this->manager->newShare(); + $originalShare->setShareType(\OCP\Share::SHARE_TYPE_EMAIL) + ->setPermissions(\OCP\Constants::PERMISSION_ALL) + ->setPassword('anotherPasswordHash') + ->setSendPasswordByTalk(false); + + $tomorrow = new \DateTime(); + $tomorrow->setTime(0,0,0); + $tomorrow->add(new \DateInterval('P1D')); + + $file = $this->createMock(File::class); + $file->method('getId')->willReturn(100); + + $share = $this->manager->newShare(); + $share->setProviderId('foo') + ->setId('42') + ->setShareType(\OCP\Share::SHARE_TYPE_EMAIL) + ->setToken('token') + ->setSharedBy('owner') + ->setShareOwner('owner') + ->setPassword('password') + ->setSendPasswordByTalk(true) + ->setExpirationDate($tomorrow) + ->setNode($file) + ->setPermissions(\OCP\Constants::PERMISSION_ALL); + + $manager->expects($this->once())->method('canShare')->willReturn(true); + $manager->expects($this->once())->method('getShareById')->with('foo:42')->willReturn($originalShare); + $manager->expects($this->once())->method('generalCreateChecks')->with($share); + $manager->expects($this->once())->method('verifyPassword')->with('password'); + $manager->expects($this->once())->method('pathCreateChecks')->with($file); + $manager->expects($this->never())->method('linkCreateChecks'); + $manager->expects($this->never())->method('validateExpirationDate'); + + $this->hasher->expects($this->once()) + ->method('verify') + ->with('password', 'anotherPasswordHash') + ->willReturn(false); + + $this->hasher->expects($this->once()) + ->method('hash') + ->with('password') + ->willReturn('hashed'); + + $this->defaultProvider->expects($this->once()) + ->method('update') + ->with($share, 'password') + ->willReturn($share); + + $hookListner = $this->getMockBuilder('Dummy')->setMethods(['post'])->getMock(); + \OCP\Util::connectHook('OCP\Share', 'post_set_expiration_date', $hookListner, 'post'); + $hookListner->expects($this->never())->method('post'); + + $hookListner2 = $this->getMockBuilder('Dummy')->setMethods(['post'])->getMock(); + \OCP\Util::connectHook('OCP\Share', 'post_update_password', $hookListner2, 'post'); + $hookListner2->expects($this->once())->method('post')->with([ + 'itemType' => 'file', + 'itemSource' => 100, + 'uidOwner' => 'owner', + 'token' => 'token', + 'disabled' => false, + ]); + + $hookListner3 = $this->getMockBuilder('Dummy')->setMethods(['post'])->getMock(); + \OCP\Util::connectHook('OCP\Share', 'post_update_permissions', $hookListner3, 'post'); + $hookListner3->expects($this->never())->method('post'); + + $manager->updateShare($share); + } public function testUpdateShareMailEnableSendPasswordByTalkWithNoPassword() { $this->expectException(\InvalidArgumentException::class); @@ -3046,7 +3128,7 @@ class ManagerTest extends \Test\TestCase { $originalShare = $this->manager->newShare(); $originalShare->setShareType(\OCP\Share::SHARE_TYPE_EMAIL) ->setPermissions(\OCP\Constants::PERMISSION_ALL) - ->setPassword('password') + ->setPassword('passwordHash') ->setSendPasswordByTalk(false); $tomorrow = new \DateTime(); @@ -3118,7 +3200,7 @@ class ManagerTest extends \Test\TestCase { $originalShare = $this->manager->newShare(); $originalShare->setShareType(\OCP\Share::SHARE_TYPE_EMAIL) ->setPermissions(\OCP\Constants::PERMISSION_ALL) - ->setPassword('password') + ->setPassword('passwordHash') ->setSendPasswordByTalk(false); $tomorrow = new \DateTime(); @@ -3190,7 +3272,7 @@ class ManagerTest extends \Test\TestCase { $originalShare = $this->manager->newShare(); $originalShare->setShareType(\OCP\Share::SHARE_TYPE_EMAIL) ->setPermissions(\OCP\Constants::PERMISSION_ALL) - ->setPassword('password') + ->setPassword('passwordHash') ->setSendPasswordByTalk(false); $tomorrow = new \DateTime(); @@ -3221,6 +3303,11 @@ class ManagerTest extends \Test\TestCase { $manager->expects($this->never())->method('linkCreateChecks'); $manager->expects($this->never())->method('validateExpirationDate'); + $this->hasher->expects($this->once()) + ->method('verify') + ->with('password', 'passwordHash') + ->willReturn(true); + $this->hasher->expects($this->never()) ->method('hash'); @@ -3258,7 +3345,7 @@ class ManagerTest extends \Test\TestCase { $originalShare = $this->manager->newShare(); $originalShare->setShareType(\OCP\Share::SHARE_TYPE_EMAIL) ->setPermissions(\OCP\Constants::PERMISSION_ALL) - ->setPassword('password') + ->setPassword('passwordHash') ->setSendPasswordByTalk(true); $tomorrow = new \DateTime(); @@ -3289,6 +3376,11 @@ class ManagerTest extends \Test\TestCase { $manager->expects($this->never())->method('linkCreateChecks'); $manager->expects($this->never())->method('validateExpirationDate'); + $this->hasher->expects($this->once()) + ->method('verify') + ->with('password', 'passwordHash') + ->willReturn(true); + $this->hasher->expects($this->never()) ->method('hash'); -- cgit v1.2.3 From d6f193750212c2181bce32afe2220c0434cdacf1 Mon Sep 17 00:00:00 2001 From: Daniel Calviño Sánchez Date: Thu, 28 May 2020 20:40:33 +0200 Subject: Fix disabling send password by Talk without new password in mail shares MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When "send password by Talk" was disabled in a mail share it was possible to keep the same password as before, as it does not pose any security issue (unlike keeping it when "send password by Talk" is enabled, as in that case the password was already disclosed by mail). However, if a mail share is updated but the password is not set again only the hashed password will be available. In that case it would not make sense to send the password by mail, so now the password must be changed when disabling "send password by Talk". Note that, even if explicitly setting the same password again along with the "send password by Talk" property would work, this was also prevented for simplicity. Signed-off-by: Daniel Calviño Sánchez --- lib/private/Share20/Manager.php | 8 ++-- tests/lib/Share20/ManagerTest.php | 84 ++++++++++++++++++++++++++++++++++++--- 2 files changed, 83 insertions(+), 9 deletions(-) (limited to 'lib') diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 08dca45a299..fead605eb03 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -987,11 +987,9 @@ class Manager implements IManager { } } else if ($share->getShareType() === \OCP\Share::SHARE_TYPE_EMAIL) { // The new password is not set again if it is the same as the old - // one, unless when switching from sending by Talk to sending by - // mail. + // one. $plainTextPassword = $share->getPassword(); - if (!empty($plainTextPassword) && !$this->updateSharePasswordIfNeeded($share, $originalShare) && - !($originalShare->getSendPasswordByTalk() && !$share->getSendPasswordByTalk())) { + if (!empty($plainTextPassword) && !$this->updateSharePasswordIfNeeded($share, $originalShare)) { $plainTextPassword = null; } if (empty($plainTextPassword) && !$originalShare->getSendPasswordByTalk() && $share->getSendPasswordByTalk()) { @@ -999,6 +997,8 @@ class Manager implements IManager { // would already have access to the share without having to call // the sharer to verify her identity throw new \InvalidArgumentException('Can’t enable sending the password by Talk without setting a new password'); + } elseif (empty($plainTextPassword) && $originalShare->getSendPasswordByTalk() && !$share->getSendPasswordByTalk()) { + throw new \InvalidArgumentException('Can’t disable sending the password by Talk without setting a new password'); } } diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index e0a0d9bfad2..946c7192f37 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -3330,6 +3330,9 @@ class ManagerTest extends \Test\TestCase { } public function testUpdateShareMailDisableSendPasswordByTalkWithPreviousPassword() { + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage('Can’t disable sending the password by Talk without setting a new password'); + $manager = $this->createManagerMock() ->setMethods([ 'canShare', @@ -3371,8 +3374,8 @@ class ManagerTest extends \Test\TestCase { $manager->expects($this->once())->method('canShare')->willReturn(true); $manager->expects($this->once())->method('getShareById')->with('foo:42')->willReturn($originalShare); $manager->expects($this->once())->method('generalCreateChecks')->with($share); - $manager->expects($this->once())->method('pathCreateChecks')->with($file); $manager->expects($this->never())->method('verifyPassword'); + $manager->expects($this->never())->method('pathCreateChecks'); $manager->expects($this->never())->method('linkCreateChecks'); $manager->expects($this->never())->method('validateExpirationDate'); @@ -3384,10 +3387,8 @@ class ManagerTest extends \Test\TestCase { $this->hasher->expects($this->never()) ->method('hash'); - $this->defaultProvider->expects($this->once()) - ->method('update') - ->with($share, 'password') - ->willReturn($share); + $this->defaultProvider->expects($this->never()) + ->method('update'); $hookListner = $this->getMockBuilder('Dummy')->setMethods(['post'])->getMock(); \OCP\Util::connectHook('OCP\Share', 'post_set_expiration_date', $hookListner, 'post'); @@ -3404,6 +3405,79 @@ class ManagerTest extends \Test\TestCase { $manager->updateShare($share); } + public function testUpdateShareMailDisableSendPasswordByTalkWithoutChangingPassword() { + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage('Can’t disable sending the password by Talk without setting a new password'); + + $manager = $this->createManagerMock() + ->setMethods([ + 'canShare', + 'getShareById', + 'generalCreateChecks', + 'verifyPassword', + 'pathCreateChecks', + 'linkCreateChecks', + 'validateExpirationDate', + ]) + ->getMock(); + + $originalShare = $this->manager->newShare(); + $originalShare->setShareType(\OCP\Share::SHARE_TYPE_EMAIL) + ->setPermissions(\OCP\Constants::PERMISSION_ALL) + ->setPassword('passwordHash') + ->setSendPasswordByTalk(true); + + $tomorrow = new \DateTime(); + $tomorrow->setTime(0,0,0); + $tomorrow->add(new \DateInterval('P1D')); + + $file = $this->createMock(File::class); + $file->method('getId')->willReturn(100); + + $share = $this->manager->newShare(); + $share->setProviderId('foo') + ->setId('42') + ->setShareType(\OCP\Share::SHARE_TYPE_EMAIL) + ->setToken('token') + ->setSharedBy('owner') + ->setShareOwner('owner') + ->setPassword('passwordHash') + ->setSendPasswordByTalk(false) + ->setExpirationDate($tomorrow) + ->setNode($file) + ->setPermissions(\OCP\Constants::PERMISSION_ALL); + + $manager->expects($this->once())->method('canShare')->willReturn(true); + $manager->expects($this->once())->method('getShareById')->with('foo:42')->willReturn($originalShare); + $manager->expects($this->once())->method('generalCreateChecks')->with($share); + $manager->expects($this->never())->method('verifyPassword'); + $manager->expects($this->never())->method('pathCreateChecks'); + $manager->expects($this->never())->method('linkCreateChecks'); + $manager->expects($this->never())->method('validateExpirationDate'); + + $this->hasher->expects($this->never()) + ->method('verify'); + + $this->hasher->expects($this->never()) + ->method('hash'); + + $this->defaultProvider->expects($this->never()) + ->method('update'); + + $hookListner = $this->getMockBuilder('Dummy')->setMethods(['post'])->getMock(); + \OCP\Util::connectHook('OCP\Share', 'post_set_expiration_date', $hookListner, 'post'); + $hookListner->expects($this->never())->method('post'); + + $hookListner2 = $this->getMockBuilder('Dummy')->setMethods(['post'])->getMock(); + \OCP\Util::connectHook('OCP\Share', 'post_update_password', $hookListner2, 'post'); + $hookListner2->expects($this->never())->method('post'); + + $hookListner3 = $this->getMockBuilder('Dummy')->setMethods(['post'])->getMock(); + \OCP\Util::connectHook('OCP\Share', 'post_update_permissions', $hookListner3, 'post'); + $hookListner3->expects($this->never())->method('post'); + + $manager->updateShare($share); + } public function testMoveShareLink() { $this->expectException(\InvalidArgumentException::class); -- cgit v1.2.3