fix(sharebymail): Send readable password by mail instead of hash

apps/sharebymail/lib/ShareByMailProvider.php

@@ -101,25 +101,24 @@ public function create(IShare $share): IShare {
 
 		// if the admin enforces a password for all mail shares we create a
 		// random password and send it to the recipient
-		$password = $share->getPassword();
-		if ($password === null && $this->shareManager->shareApiLinkEnforcePassword()) {
-			$password = $this->autoGeneratePassword($share);
-			$share->setPasswordHash($this->hasher->hash($password));
+		$autoGeneratedPassword = null;
+		if ($share->getPassword() === null && $this->shareManager->shareApiLinkEnforcePassword()) {
+			$autoGeneratedPassword = $this->autoGeneratePassword($share);
+			$share->setPasswordHash($this->hasher->hash($autoGeneratedPassword));
 		}
 
 		$shareId = $this->createMailShare($share);
 
 		$this->createShareActivity($share);
 		$data = $this->getRawShare($shareId);
+		$createdShare = $this->createShareObject($data);
 
-		// Temporary set the clear password again to send it by mail
-		// This need to be done after the share was created in the database
-		// as the password is hashed in between.
-		if ($password !== null) {
-			$data['password'] = $password;
+		// The auto-generated password is only known here, so it is put back in clear to send it by mail
+		if ($autoGeneratedPassword !== null) {
+			$createdShare->setPassword($autoGeneratedPassword);
 		}
 
-		return $this->createShareObject($data);
+		return $createdShare;
 	}
 
 	/**
@@ -278,7 +277,8 @@ public function sendMailNotification(IShare $share): bool {
 			$this->sendEmail($share, $validEmails);
 
 			// If we have a password set, we send it to the recipient
-			if ($share->getPassword() !== null) {
+			// A password hash is never sent, as it can't be used to open the share
+			if ($share->getPassword() !== null && !$share->isPasswordHashed()) {
 				// 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.

apps/sharebymail/tests/ShareByMailProviderTest.php

@@ -714,6 +714,59 @@ public function testCreateFailed(): void {
 		);
 	}
 
+	private function newMailShare(): Share {
+		$node = $this->createMock(File::class);
+		$node->method('getId')->willReturn(42);
+
+		$share = new Share($this->rootFolder, $this->userManager);
+		$share->setShareType(IShare::TYPE_EMAIL)
+			->setSharedWith('receiver@example.com')
+			->setSharedBy('owner')
+			->setShareOwner('owner')
+			->setPermissions(Constants::PERMISSION_READ)
+			->setToken('token')
+			->setNode($node);
+
+		return $share;
+	}
+
+	public function testCreateStoresGivenPasswordHash(): void {
+		$this->shareManager->method('shareApiLinkEnforcePassword')->willReturn(true);
+
+		$instance = $this->getInstance(['createShareActivity', 'autoGeneratePassword']);
+		$instance->expects($this->never())->method('autoGeneratePassword');
+
+		$passwordHash = Server::get(IHasher::class)->hash('password');
+		$createdShare = $instance->create($this->newMailShare()->setPasswordHash($passwordHash));
+
+		$this->assertSame($passwordHash, $createdShare->getPassword());
+		$this->assertTrue($createdShare->isPasswordHashed());
+		$this->assertSame($passwordHash, $instance->getShareById($createdShare->getId())->getPassword());
+	}
+
+	public function testCreateStoresAutoGeneratedPasswordHashAndMailsClearPassword(): void {
+		$this->shareManager->method('shareApiLinkEnforcePassword')->willReturn(true);
+		$this->settingsManager->method('sendPasswordByMail')->willReturn(true);
+		$this->secureRandom->method('generate')
+			->with(8, ISecureRandom::CHAR_HUMAN_READABLE)
+			->willReturn('autogeneratedPassword');
+
+		$instance = $this->getInstance(['createShareActivity', 'sendEmail', 'sendPassword']);
+
+		$createdShare = $instance->create($this->newMailShare());
+
+		$this->assertSame('autogeneratedPassword', $createdShare->getPassword());
+		$this->assertFalse($createdShare->isPasswordHashed());
+
+		$storedShare = $instance->getShareById($createdShare->getId());
+		$this->assertTrue($storedShare->isPasswordHashed());
+		$this->assertTrue(Server::get(IHasher::class)->verify('autogeneratedPassword', $storedShare->getPassword()));
+
+		$instance->expects($this->once())->method('sendEmail')->with($createdShare, ['receiver@example.com']);
+		$instance->expects($this->once())->method('sendPassword')->with($createdShare, 'autogeneratedPassword', ['receiver@example.com'])->willReturn(true);
+		$instance->sendMailNotification($createdShare);
+	}
+
 	public function testCreateMailShare(): void {
 		$this->share->expects($this->any())->method('getToken')->willReturn('');
 		$this->share->expects($this->once())->method('setToken')->with('token');
@@ -1461,6 +1514,21 @@ public function testSendEmailListsPasswordInDetails(): void {
 		self::invokePrivate($provider, 'sendEmail', [$share, ['john@doe.com']]);
 	}
 
+	public function testSendMailNotificationDoesNotMailPasswordHash(): void {
+		$this->settingsManager->method('sendPasswordByMail')->willReturn(true);
+
+		$instance = $this->getInstance(['sendEmail', 'sendPassword', 'sendPasswordToOwner']);
+		$share = $this->newMailShare()
+			->setId('42')
+			->setPasswordHash(Server::get(IHasher::class)->hash('password'));
+
+		$instance->expects($this->once())->method('sendEmail')->with($share, ['receiver@example.com']);
+		$instance->expects($this->never())->method('sendPassword');
+		$instance->expects($this->never())->method('sendPasswordToOwner');
+
+		$this->assertTrue($instance->sendMailNotification($share));
+	}
+
 	public function testSendMailNotificationWithSameUserAndUserEmailAndNote(): void {
 		$provider = $this->getInstance();
 		$user = $this->createMock(IUser::class);

lib/private/Share20/Manager.php

@@ -536,6 +536,7 @@ public function createShare(IShare $share): IShare {
 			}
 		}
 
+		$plainTextPassword = null;
 		try {
 			// Verify share type
 			if ($share->getShareType() === IShare::TYPE_USER) {
@@ -569,7 +570,8 @@ public function createShare(IShare $share): IShare {
 
 				// If a password is set. Hash it!
 				if (($share->getShareType() === IShare::TYPE_LINK || $share->getShareType() === IShare::TYPE_EMAIL) && $share->getPassword() !== null && !$share->isPasswordHashed()) {
-					$share->setPasswordHash($this->hasher->hash($share->getPassword()));
+					$plainTextPassword = $share->getPassword();
+					$share->setPasswordHash($this->hasher->hash($plainTextPassword));
 				}
 			}
 
@@ -641,7 +643,13 @@ public function createShare(IShare $share): IShare {
 			if ($share->getMailSend()) {
 				$provider = $this->factory->getProviderForType($share->getShareType());
 				if ($provider instanceof IShareProviderWithNotification) {
-					$provider->sendMailNotification($share);
+					$notifiedShare = $share;
+					if ($plainTextPassword !== null) {
+						// The share only holds the hash, the mail needs the clear password given by the caller
+						$notifiedShare = clone $share;
+						$notifiedShare->setPassword($plainTextPassword);
+					}
+					$provider->sendMailNotification($notifiedShare);
 				} else {
 					$this->logger->debug('Share notification not sent because the provider does not support it.', ['app' => 'share']);
 				}

tests/lib/Share20/ManagerTest.php

@@ -3504,6 +3504,72 @@ public function testCreateShareMail(): void {
 		$this->assertEquals('token', $share->getToken());
 	}
 
+	public function testCreateShareMailSendsClearPassword(): void {
+		$manager = $this->createManagerMock()
+			->onlyMethods([
+				'generalChecks',
+				'pathCreateChecks',
+				'validateExpirationDateLink',
+				'verifyPassword',
+				'setLinkParent',
+			])
+			->getMock();
+
+		$shareOwner = $this->createMock(IUser::class);
+		$shareOwner->method('getUID')->willReturn('shareOwner');
+
+		$path = $this->createMock(File::class);
+		$path->method('getOwner')->willReturn($shareOwner);
+		$path->method('getName')->willReturn('target');
+		$path->method('getId')->willReturn(1);
+		$path->method('getStorage')->willReturn($this->createMock(IStorage::class));
+
+		$share = $this->manager->newShare();
+		$share->setShareType(IShare::TYPE_EMAIL)
+			->setNode($path)
+			->setSharedBy('sharedBy')
+			->setPermissions(Constants::PERMISSION_ALL)
+			->setMailSend(true)
+			->setPassword('password');
+
+		$manager->method('validateExpirationDateLink')->willReturnArgument(0);
+		$manager->expects($this->once())
+			->method('verifyPassword')
+			->with('password');
+
+		$this->secureRandom->method('generate')
+			->willReturn('token');
+		$this->config->method('getSystemValueBool')
+			->with('sharing.enable_share_mail', true)
+			->willReturn(true);
+
+		$storedPassword = null;
+		$this->defaultProvider
+			->expects($this->once())
+			->method('create')
+			->willReturnCallback(function (Share $share) use (&$storedPassword): Share {
+				$storedPassword = $share->getPassword();
+				$storedShare = clone $share;
+				return $storedShare->setId('42');
+			});
+
+		$mailedPassword = null;
+		$this->defaultProvider
+			->expects($this->once())
+			->method('sendMailNotification')
+			->willReturnCallback(function (IShare $share) use (&$mailedPassword): bool {
+				$mailedPassword = $share->getPassword();
+				return true;
+			});
+
+		$createdShare = $manager->createShare($share);
+
+		$this->assertTrue(Server::get(IHasher::class)->verify('password', $storedPassword));
+		$this->assertSame('password', $mailedPassword);
+		$this->assertSame($storedPassword, $createdShare->getPassword());
+		$this->assertTrue($createdShare->isPasswordHashed());
+	}
+
 	public function testCreateShareHookError(): void {
 		$this->expectException(\Exception::class);
 		$this->expectExceptionMessage('I won\'t let you share');