From 9a5b51d43cf41163d23be30fb11cdaf5b815777d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Thu, 28 May 2020 20:27:33 +0200 Subject: [PATCH] Fix creating a mail share with a password MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a mail share was created with a password the given password was not hashed, so it was not possible to open the share with that password. Moreover, if passwords were enforced the given password was ignored and a new one was set (although in this case it was hashed so it worked as expected). Now the given password is properly hashed and not overriden. Signed-off-by: Daniel Calviño Sánchez --- apps/sharebymail/lib/ShareByMailProvider.php | 10 +- .../tests/ShareByMailProviderTest.php | 98 +++++++++++++++++++ 2 files changed, 104 insertions(+), 4 deletions(-) diff --git a/apps/sharebymail/lib/ShareByMailProvider.php b/apps/sharebymail/lib/ShareByMailProvider.php index 4292ac9bf1..3a1a069172 100644 --- a/apps/sharebymail/lib/ShareByMailProvider.php +++ b/apps/sharebymail/lib/ShareByMailProvider.php @@ -187,12 +187,16 @@ class ShareByMailProvider implements IShareProvider { // if the admin enforces a password for all mail shares we create a // random password and send it to the recipient - $password = ''; + $password = $share->getPassword() ?: ''; $passwordEnforced = $this->settingsManager->enforcePasswordProtection(); - if ($passwordEnforced) { + if ($passwordEnforced && empty($password)) { $password = $this->autoGeneratePassword($share); } + if (!empty($password)) { + $share->setPassword($this->hasher->hash($password)); + } + $shareId = $this->createMailShare($share); $send = $this->sendPassword($share, $password); if ($passwordEnforced && $send === false) { @@ -233,8 +237,6 @@ class ShareByMailProvider implements IShareProvider { $password = $this->secureRandom->generate($passwordLength, $passwordCharset); - $share->setPassword($this->hasher->hash($password)); - return $password; } diff --git a/apps/sharebymail/tests/ShareByMailProviderTest.php b/apps/sharebymail/tests/ShareByMailProviderTest.php index 431ea5ca8c..2dc8ff9c95 100644 --- a/apps/sharebymail/tests/ShareByMailProviderTest.php +++ b/apps/sharebymail/tests/ShareByMailProviderTest.php @@ -240,6 +240,51 @@ class ShareByMailProviderTest extends TestCase { ); } + public function testCreateSendPasswordByMailWithPasswordAndWithoutEnforcedPasswordProtection() { + $share = $this->getMockBuilder(IShare::class)->getMock(); + $share->expects($this->any())->method('getSharedWith')->willReturn('receiver@example.com'); + $share->expects($this->any())->method('getSendPasswordByTalk')->willReturn(false); + $share->expects($this->any())->method('getSharedBy')->willReturn('owner'); + + $node = $this->getMockBuilder(File::class)->getMock(); + $node->expects($this->any())->method('getName')->willReturn('filename'); + + $instance = $this->getInstance(['getSharedWith', 'createMailShare', 'getRawShare', 'createShareObject', 'createShareActivity', 'autoGeneratePassword', 'createPasswordSendActivity']); + + $instance->expects($this->once())->method('getSharedWith')->willReturn([]); + $instance->expects($this->once())->method('createMailShare')->with($share)->willReturn(42); + $instance->expects($this->once())->method('createShareActivity')->with($share); + $instance->expects($this->once())->method('getRawShare')->with(42)->willReturn('rawShare'); + $instance->expects($this->once())->method('createShareObject')->with('rawShare')->willReturn('shareObject'); + $share->expects($this->any())->method('getNode')->willReturn($node); + + $share->expects($this->once())->method('getPassword')->willReturn('password'); + $this->hasher->expects($this->once())->method('hash')->with('password')->willReturn('passwordHashed'); + $share->expects($this->once())->method('setPassword')->with('passwordHashed'); + + // The given password (but not the autogenerated password) should be + // mailed to the receiver of the share. + $this->settingsManager->expects($this->any())->method('enforcePasswordProtection')->willReturn(false); + $this->settingsManager->expects($this->any())->method('sendPasswordByMail')->willReturn(true); + $instance->expects($this->never())->method('autoGeneratePassword'); + + $message = $this->createMock(IMessage::class); + $message->expects($this->once())->method('setTo')->with(['receiver@example.com']); + $this->mailer->expects($this->once())->method('createMessage')->willReturn($message); + $this->mailer->expects($this->once())->method('createEMailTemplate')->with('sharebymail.RecipientPasswordNotification', [ + 'filename' => 'filename', + 'password' => 'password', + 'initiator' => 'owner', + 'initiatorEmail' => null, + 'shareWith' => 'receiver@example.com', + ]); + $this->mailer->expects($this->once())->method('send'); + + $this->assertSame('shareObject', + $instance->create($share) + ); + } + public function testCreateSendPasswordByMailWithEnforcedPasswordProtection() { $share = $this->getMockBuilder(IShare::class)->getMock(); $share->expects($this->any())->method('getSharedWith')->willReturn('receiver@example.com'); @@ -258,6 +303,10 @@ class ShareByMailProviderTest extends TestCase { $instance->expects($this->once())->method('createShareObject')->with('rawShare')->willReturn('shareObject'); $share->expects($this->any())->method('getNode')->willReturn($node); + $share->expects($this->once())->method('getPassword')->willReturn(null); + $this->hasher->expects($this->once())->method('hash')->with('autogeneratedPassword')->willReturn('autogeneratedPasswordHashed'); + $share->expects($this->once())->method('setPassword')->with('autogeneratedPasswordHashed'); + // The autogenerated password should be mailed to the receiver of the share. $this->settingsManager->expects($this->any())->method('enforcePasswordProtection')->willReturn(true); $this->settingsManager->expects($this->any())->method('sendPasswordByMail')->willReturn(true); @@ -280,6 +329,51 @@ class ShareByMailProviderTest extends TestCase { ); } + public function testCreateSendPasswordByMailWithPasswordAndWithEnforcedPasswordProtection() { + $share = $this->getMockBuilder(IShare::class)->getMock(); + $share->expects($this->any())->method('getSharedWith')->willReturn('receiver@example.com'); + $share->expects($this->any())->method('getSendPasswordByTalk')->willReturn(false); + $share->expects($this->any())->method('getSharedBy')->willReturn('owner'); + + $node = $this->getMockBuilder(File::class)->getMock(); + $node->expects($this->any())->method('getName')->willReturn('filename'); + + $instance = $this->getInstance(['getSharedWith', 'createMailShare', 'getRawShare', 'createShareObject', 'createShareActivity', 'autoGeneratePassword', 'createPasswordSendActivity']); + + $instance->expects($this->once())->method('getSharedWith')->willReturn([]); + $instance->expects($this->once())->method('createMailShare')->with($share)->willReturn(42); + $instance->expects($this->once())->method('createShareActivity')->with($share); + $instance->expects($this->once())->method('getRawShare')->with(42)->willReturn('rawShare'); + $instance->expects($this->once())->method('createShareObject')->with('rawShare')->willReturn('shareObject'); + $share->expects($this->any())->method('getNode')->willReturn($node); + + $share->expects($this->once())->method('getPassword')->willReturn('password'); + $this->hasher->expects($this->once())->method('hash')->with('password')->willReturn('passwordHashed'); + $share->expects($this->once())->method('setPassword')->with('passwordHashed'); + + // The given password (but not the autogenerated password) should be + // mailed to the receiver of the share. + $this->settingsManager->expects($this->any())->method('enforcePasswordProtection')->willReturn(true); + $this->settingsManager->expects($this->any())->method('sendPasswordByMail')->willReturn(true); + $instance->expects($this->never())->method('autoGeneratePassword'); + + $message = $this->createMock(IMessage::class); + $message->expects($this->once())->method('setTo')->with(['receiver@example.com']); + $this->mailer->expects($this->once())->method('createMessage')->willReturn($message); + $this->mailer->expects($this->once())->method('createEMailTemplate')->with('sharebymail.RecipientPasswordNotification', [ + 'filename' => 'filename', + 'password' => 'password', + 'initiator' => 'owner', + 'initiatorEmail' => null, + 'shareWith' => 'receiver@example.com', + ]); + $this->mailer->expects($this->once())->method('send'); + + $this->assertSame('shareObject', + $instance->create($share) + ); + } + public function testCreateSendPasswordByTalkWithEnforcedPasswordProtection() { $share = $this->getMockBuilder(IShare::class)->getMock(); $share->expects($this->any())->method('getSharedWith')->willReturn('receiver@example.com'); @@ -298,6 +392,10 @@ class ShareByMailProviderTest extends TestCase { $instance->expects($this->once())->method('createShareObject')->with('rawShare')->willReturn('shareObject'); $share->expects($this->any())->method('getNode')->willReturn($node); + $share->expects($this->once())->method('getPassword')->willReturn(null); + $this->hasher->expects($this->once())->method('hash')->with('autogeneratedPassword')->willReturn('autogeneratedPasswordHashed'); + $share->expects($this->once())->method('setPassword')->with('autogeneratedPasswordHashed'); + // The autogenerated password should be mailed to the owner of the share. $this->settingsManager->expects($this->any())->method('enforcePasswordProtection')->willReturn(true); $this->settingsManager->expects($this->any())->method('sendPasswordByMail')->willReturn(true);