Fix enabling send password by Talk with same password in mail shares

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 <danxuliu@gmail.com>

diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php
index f88d884c0db569e6b23cd9e467c39104aa8d90bb..08dca45a2996fd638377bad0ef6c1c4ccf39fa24 100644 (file)
--- 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 962974b231d5e9ad21d7fa31829ed19a8d2a6a3f..e0a0d9bfad2b2287030ebe5faf9be029e012cb06 100644 (file)
--- 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');