Fix: password notification to share owner behavior

Signed-off-by: Ghassen kefi <ghassen.kefi.dev@gmail.com>

Author: GhassenKefi

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

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');