From fd7d17db1f38ac7acd77e656aeabd51e7e373fed 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 3b60be0c32d82..e41c6557e7573 100644 --- a/apps/sharebymail/lib/ShareByMailProvider.php +++ b/apps/sharebymail/lib/ShareByMailProvider.php @@ -270,10 +270,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 80b29a6812f08..ff7d0272ff6dd 100644 --- a/apps/sharebymail/tests/ShareByMailProviderTest.php +++ b/apps/sharebymail/tests/ShareByMailProviderTest.php @@ -315,6 +315,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']); @@ -325,6 +327,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 2952a62dafc483a076013a2c14bb71d2831d8c38 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 e41c6557e7573..b295f7505ab5c 100644 --- a/apps/sharebymail/lib/ShareByMailProvider.php +++ b/apps/sharebymail/lib/ShareByMailProvider.php @@ -264,16 +264,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); } } } @@ -302,6 +305,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 ff7d0272ff6dd..f6a8813a673ef 100644 --- a/apps/sharebymail/tests/ShareByMailProviderTest.php +++ b/apps/sharebymail/tests/ShareByMailProviderTest.php @@ -260,12 +260,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)); @@ -311,66 +311,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); @@ -584,7 +578,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');