From b6e3557dbd5ae6671b834ed74bad35eab428f9c8 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 194a402848e11..50d8ac525bd6b 100644 --- a/apps/sharebymail/lib/ShareByMailProvider.php +++ b/apps/sharebymail/lib/ShareByMailProvider.php @@ -269,10 +269,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 05435b835e095..b226b6ea61bf8 100644 --- a/apps/sharebymail/tests/ShareByMailProviderTest.php +++ b/apps/sharebymail/tests/ShareByMailProviderTest.php @@ -313,6 +313,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']); @@ -323,6 +325,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 77805e86720977625ed800fddf3840fd5d6ad3ce 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 50d8ac525bd6b..3b29f80ab0976 100644 --- a/apps/sharebymail/lib/ShareByMailProvider.php +++ b/apps/sharebymail/lib/ShareByMailProvider.php @@ -263,16 +263,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); } } } @@ -301,6 +304,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 b226b6ea61bf8..852676a591e16 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)); @@ -309,66 +309,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); @@ -561,7 +555,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');