From a79af7c739f94705369eb2e28291acd75e030e8e 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 f0c702e79e427..af3050a668a4e 100644 --- a/apps/sharebymail/lib/ShareByMailProvider.php +++ b/apps/sharebymail/lib/ShareByMailProvider.php @@ -275,10 +275,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 aaa608cc44645..99be30c257f1d 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 0216a381b9e84bfe70971eb8668c8e0f64f4a992 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 af3050a668a4e..74abeea1d0136 100644 --- a/apps/sharebymail/lib/ShareByMailProvider.php +++ b/apps/sharebymail/lib/ShareByMailProvider.php @@ -269,16 +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); - 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); } } } @@ -307,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 99be30c257f1d..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,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');