From 8e5aa03834bc77fe501a5d087ad0f6928d2ea9b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Thu, 28 May 2020 20:37:18 +0200 Subject: [PATCH] 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(-) diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 536f8e0b15..4d93df5e07 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1081,8 +1081,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()); @@ -1092,6 +1098,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 ec4ff4190b..49fa392dc0 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -2963,6 +2963,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); @@ -3055,7 +3137,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(); @@ -3127,7 +3209,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(); @@ -3199,7 +3281,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(); @@ -3230,6 +3312,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'); @@ -3267,7 +3354,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(); @@ -3298,6 +3385,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');