From ff80aedbb83247ef3637fb4b5eb91187aa7348ce Mon Sep 17 00:00:00 2001 From: nfebe Date: Tue, 28 Jan 2025 23:34:45 +0100 Subject: [PATCH 1/2] fix: Send password to owner when password sending fails, regardless of enforcement Previously, the `sendPasswordToOwner` method would only trigger if password enforcement was enabled and sending the password to the recipient failed. This behavior was inconsistent with the intended functionality, as the owner should receive the password whenever sending to the recipient fails, regardless of whether password enforcement is enabled. This change ensures that the owner is notified of the password whenever sending it to the recipient fails, improving consistency and user experience. Signed-off-by: nfebe WIP test(share_by_mail): Send password to owner if `send_by_mail` is disabled Signed-off-by: nfebe --- apps/sharebymail/lib/ShareByMailProvider.php | 3 +- .../tests/ShareByMailProviderTest.php | 47 +++++++++++++++++++ 2 files changed, 48 insertions(+), 2 deletions(-) diff --git a/apps/sharebymail/lib/ShareByMailProvider.php b/apps/sharebymail/lib/ShareByMailProvider.php index 508515b7438ba..8cd0ac9539374 100644 --- a/apps/sharebymail/lib/ShareByMailProvider.php +++ b/apps/sharebymail/lib/ShareByMailProvider.php @@ -278,10 +278,9 @@ public function sendMailNotification(IShare $share): bool { // 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()) { $send = $this->sendPassword($share, $share->getPassword(), $validEmails); - if ($passwordEnforced && $send === false) { + if ($send === false) { $this->sendPasswordToOwner($share, $share->getPassword()); } } diff --git a/apps/sharebymail/tests/ShareByMailProviderTest.php b/apps/sharebymail/tests/ShareByMailProviderTest.php index aa42a9ccc63af..7b5e57788314b 100644 --- a/apps/sharebymail/tests/ShareByMailProviderTest.php +++ b/apps/sharebymail/tests/ShareByMailProviderTest.php @@ -310,6 +310,8 @@ public function testCreateSendPasswordByMailWithPasswordAndWithoutEnforcedPasswo ->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']); @@ -320,6 +322,51 @@ public function testCreateSendPasswordByMailWithPasswordAndWithoutEnforcedPasswo $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->config->expects($this->once())->method('getSystemValue')->with('sharing.enable_mail_link_password_expiration')->willReturn(true); + $this->settingsManager->expects($this->once())->method('sendPasswordByMail')->willReturn(false); + + $instance->expects($this->once())->method('sendPasswordToOwner')->with($share); + $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); From 32af6d04c227abc134a20aa9c6f89a11d4149d19 Mon Sep 17 00:00:00 2001 From: Ghassen kefi Date: Fri, 11 Sep 2026 18:43:58 +0200 Subject: [PATCH 2/2] Fix: password notification to share owner behavior Signed-off-by: Ghassen kefi --- apps/sharebymail/lib/ShareByMailProvider.php | 34 ++++++++++++++----- .../tests/ShareByMailProviderTest.php | 34 ++++++++----------- 2 files changed, 40 insertions(+), 28 deletions(-) diff --git a/apps/sharebymail/lib/ShareByMailProvider.php b/apps/sharebymail/lib/ShareByMailProvider.php index 8cd0ac9539374..be2a28dfe39a3 100644 --- a/apps/sharebymail/lib/ShareByMailProvider.php +++ b/apps/sharebymail/lib/ShareByMailProvider.php @@ -272,16 +272,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); - 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 ($send === false) { - $this->sendPasswordToOwner($share, $share->getPassword()); + $this->trySendPasswordToOwner($share); } } } @@ -310,6 +313,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 7b5e57788314b..68c394a9a1640 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,66 +306,60 @@ 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('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('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->config->expects($this->once())->method('getSystemValue')->with('sharing.enable_mail_link_password_expiration')->willReturn(true); $this->settingsManager->expects($this->once())->method('sendPasswordByMail')->willReturn(false); - - $instance->expects($this->once())->method('sendPasswordToOwner')->with($share); + + $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); @@ -570,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');