diff --git a/apps/sharebymail/lib/ShareByMailProvider.php b/apps/sharebymail/lib/ShareByMailProvider.php index f0c702e79e427..74abeea1d0136 100644 --- a/apps/sharebymail/lib/ShareByMailProvider.php +++ b/apps/sharebymail/lib/ShareByMailProvider.php @@ -269,17 +269,19 @@ public function sendMailNotification(IShare $share): bool { // If we have a password set, we send it to the recipient if ($share->getPassword() !== null) { - // If share-by-talk password is enabled, we do not send the notification - // to the recipient. They will have to request it to the owner after opening the link. - // Secondly, if the password expiration is disabled, we send the notification to the recipient - // Lastly, if the mail to recipient failed, we send the password to the owner as a fallback. - // If a password expires, the recipient will still be able to request a new one via talk. - $passwordExpire = $this->config->getSystemValue('sharing.enable_mail_link_password_expiration', false); - $passwordEnforced = $this->shareManager->shareApiLinkEnforcePassword(); - if ($passwordExpire === false || $share->getSendPasswordByTalk()) { + // If sending the password by mail is disabled, we send the password to the owner. + // If share-by-talk password is enabled, we do not send the password to the recipient. + // They can request it from the owner after opening the link. + // Otherwise, we send the password to the recipient. + // If sending the password to the recipient fails, we send the password to the owner as a fallback. + // Password expiration does not affect this flow: the password is either sent to the recipient + // or, if sending fails, sent to the owner as a fallback. + if ($this->settingsManager->sendPasswordByMail() === false || $share->getSendPasswordByTalk()) { + $this->trySendPasswordToOwner($share); + } else { $send = $this->sendPassword($share, $share->getPassword(), $validEmails); - if ($passwordEnforced && $send === false) { - $this->sendPasswordToOwner($share, $share->getPassword()); + if ($send === false) { + $this->trySendPasswordToOwner($share); } } } @@ -308,6 +310,21 @@ public function sendMailNotification(IShare $share): bool { return false; } + /** + * Notifying the owner of the password must not abort an otherwise + * successful share creation, e.g. when the owner has no email address set. + */ + private function trySendPasswordToOwner(IShare $share): void { + try { + $this->sendPasswordToOwner($share, $share->getPassword()); + } catch (\Exception $e) { + $this->logger->error('Failed to send password to the owner of the share.', [ + 'app' => 'sharebymail', + 'exception' => $e, + ]); + } + } + /** * @param IShare $share The share to send the email for * @param array $emails The email addresses to send the email to diff --git a/apps/sharebymail/tests/ShareByMailProviderTest.php b/apps/sharebymail/tests/ShareByMailProviderTest.php index aaa608cc44645..e9ea0ddbbdafa 100644 --- a/apps/sharebymail/tests/ShareByMailProviderTest.php +++ b/apps/sharebymail/tests/ShareByMailProviderTest.php @@ -258,12 +258,12 @@ public function testCreateSendPasswordByMailWithPasswordAndWithoutEnforcedPasswo // The given password (but not the autogenerated password) should not be // mailed to the receiver of the share because permanent passwords are not enforced. $this->shareManager->expects($this->any())->method('shareApiLinkEnforcePassword')->willReturn(false); - $this->config->expects($this->once())->method('getSystemValue')->with('sharing.enable_mail_link_password_expiration')->willReturn(false); + $this->settingsManager->expects($this->once())->method('sendPasswordByMail')->willReturn(true); $instance->expects($this->never())->method('autoGeneratePassword'); // A password is set but no password sent via talk has been requested $instance->expects($this->once())->method('sendEmail')->with($share, ['receiver@example.com']); - $instance->expects($this->once())->method('sendPassword')->with($share, 'password'); + $instance->expects($this->once())->method('sendPassword')->with($share, 'password')->willReturn(true); $instance->expects($this->never())->method('sendPasswordToOwner'); $this->assertSame($expectedShare, $instance->create($share)); @@ -306,20 +306,61 @@ public function testCreateSendPasswordByMailWithPasswordAndWithoutEnforcedPasswo // aside from the main email notification. $this->shareManager->expects($this->any())->method('shareApiLinkEnforcePassword')->willReturn(false); $instance->expects($this->never())->method('autoGeneratePassword'); - $this->config->expects($this->once())->method('getSystemValue') - ->with('sharing.enable_mail_link_password_expiration') - ->willReturn(true); + + $this->settingsManager->expects($this->once())->method('sendPasswordByMail')->willReturn(true); // No password has been set and no password sent via talk has been requested, // but password has been enforced for the whole instance and will be generated. $instance->expects($this->once())->method('sendEmail')->with($share, ['receiver@example.com']); - $instance->expects($this->never())->method('sendPassword'); + $instance->expects($this->once())->method('sendPassword')->with($share, 'password')->willReturn(true); $instance->expects($this->never())->method('sendPasswordToOwner'); $this->assertSame($expectedShare, $instance->create($share)); $instance->sendMailNotification($share); } + public function testCreateSendPasswordToOwnerWhenSendPasswordByMailIsDisabled(): void { + $expectedShare = $this->createMock(IShare::class); + $node = $this->getMockBuilder(File::class)->getMock(); + $node->method('getName')->willReturn('filename'); + + $share = $this->getMockBuilder(IShare::class)->getMock(); + $share->method('getSharedWith')->willReturn('receiver@example.com'); + $share->method('getSendPasswordByTalk')->willReturn(false); + $share->method('getSharedBy')->willReturn('owner'); + $share->method('getNode')->willReturn($node); + $share->method('getId')->willReturn('42'); + $share->method('getNote')->willReturn(''); + $share->method('getToken')->willReturn('token'); + $share->method('getPassword')->willReturn('password'); + + $this->mailer->method('validateMailAddress')->willReturn(true); + $this->hasher->expects($this->once())->method('hash')->with('password')->willReturn('passwordHashed'); + $share->expects($this->once())->method('setPassword')->with('passwordHashed'); + + $instance = $this->getInstance([ + 'getSharedWith', 'createMailShare', 'getRawShare', 'createShareObject', + 'createShareActivity', 'autoGeneratePassword', 'createPasswordSendActivity', + 'sendEmail', 'sendPassword', 'sendPasswordToOwner', + ]); + + $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', 'password' => 'password']); + $instance->expects($this->once())->method('createShareObject')->with(['rawShare', 'password' => 'password'])->willReturn($expectedShare); + + $this->shareManager->method('shareApiLinkEnforcePassword')->willReturn(false); + $this->settingsManager->expects($this->once())->method('sendPasswordByMail')->willReturn(false); + + $instance->expects($this->once())->method('sendPasswordToOwner')->with($share, 'password'); + $instance->expects($this->once())->method('sendEmail')->with($share, ['receiver@example.com']); + $instance->expects($this->never())->method('sendPassword'); + + $this->assertSame($expectedShare, $instance->create($share)); + $instance->sendMailNotification($share); + } + public function testCreateSendPasswordByMailWithEnforcedPasswordProtectionWithPermanentPassword(): void { $expectedShare = $this->createMock(IShare::class); @@ -523,7 +564,7 @@ public function testCreateSendPasswordByTalkWithEnforcedPasswordProtectionWithPe $instance->expects($this->once())->method('getRawShare')->with(42)->willReturn(['rawShare', 'password' => 'autogeneratedPassword']); $instance->expects($this->once())->method('createShareObject')->with(['rawShare', 'password' => 'autogeneratedPassword'])->willReturn($expectedShare); - $share->expects($this->exactly(4))->method('getPassword')->willReturnOnConsecutiveCalls(null, 'autogeneratedPassword', 'autogeneratedPassword', 'autogeneratedPassword'); + $share->expects($this->exactly(3))->method('getPassword')->willReturnOnConsecutiveCalls(null, 'autogeneratedPassword', 'autogeneratedPassword'); $this->hasher->expects($this->once())->method('hash')->with('autogeneratedPassword')->willReturn('autogeneratedPasswordHashed'); $share->expects($this->once())->method('setPassword')->with('autogeneratedPasswordHashed');