Master password: Make sure accounts without password are included

[mickenordin]

Previously only accounts with a password set would be provisioned with the master password, this patch fixes that.

One potential problem with this patch is that it copies a function from here:

https://github.com/nextcloud/mail/blob/c1e332d163302590c8ad6a2292951f224a1497bb/lib/Service/Provisioning/Manager.php#L358-L375

If we make that function public in the provisioning manager, we could reuse the code better, so that might be something to consider.

Edit: This concern is addressed now.

[ChristophWurst]

FYI we are using conventional commits for stableX branches. This is a bugfix I would like to backport. Therefore I'd appreciate if you could change your commit message to something like fix(provisioning): Set master password for passwordless sessions. You can keep pushing to your branch and wait with the amend until this change is approved and CI checks are passing.

[mickenordin]

I think your concernes are addressed now @ChristophWurst ?

[github-actions[bot]]

Hello there, Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

[ChristophWurst]

The code fails when a users logs in with a password but master passwords are enabled. Then the login password took precedence. I think the master password should always go first if it's enabled.

This fixes the code and tests:

diff --git a/lib/Service/Provisioning/Manager.php b/lib/Service/Provisioning/Manager.php
index 2617a4d1e..ddbb55abc 100644
--- a/lib/Service/Provisioning/Manager.php
+++ b/lib/Service/Provisioning/Manager.php
@@ -333,16 +333,12 @@ class Manager {
 			// Maybe research other providers as well.
 			// Ref \OCA\Mail\Controller\PageController::index()
 			//     -> inital state for password-is-unavailable
-			if ($password === null) {
-				$masterPassword = $provisioning->getMasterPassword();
-				$masterPasswordEnabled = $provisioning->getMasterPasswordEnabled();
-				if ($masterPasswordEnabled && $masterPassword !== null) {
-					$password = $masterPassword;
-					$this->logger->debug('Password set to master password for ' . $user->getUID());
-				} else {
-					$this->logger->debug('No password set for ' . $user->getUID());
-					return;
-				}
+			if ($provisioning->getMasterPasswordEnabled() === true && $provisioning->getMasterPassword() !== null) {
+				$password = $provisioning->getMasterPassword();
+				$this->logger->debug('Password set to master password for ' . $user->getUID());
+			} else if ($password === null) {
+				$this->logger->debug('No password set for ' . $user->getUID());
+				return;
 			}
 
 			if (!empty($account->getInboundPassword())
diff --git a/tests/Unit/Http/Middleware/ProvisioningMiddlewareTest.php b/tests/Unit/Http/Middleware/ProvisioningMiddlewareTest.php
index 81f8e40d6..a54e6f5db 100644
--- a/tests/Unit/Http/Middleware/ProvisioningMiddlewareTest.php
+++ b/tests/Unit/Http/Middleware/ProvisioningMiddlewareTest.php
@@ -160,8 +160,6 @@ class ProvisioningMiddlewareTest extends TestCase {
 		$credentials->expects($this->once())
 			->method('getPassword')
 			->willReturn(null);
-		$this->provisioningManager->expects($this->never())
-			->method('updatePassword');
 
 		$this->middleware->beforeController(
 			$this->createMock(PageController::class),
diff --git a/tests/Unit/Service/Provisioning/ManagerTest.php b/tests/Unit/Service/Provisioning/ManagerTest.php
index 486466e67..083ff896e 100644
--- a/tests/Unit/Service/Provisioning/ManagerTest.php
+++ b/tests/Unit/Service/Provisioning/ManagerTest.php
@@ -235,7 +235,7 @@ class ManagerTest extends TestCase {
 		$this->manager->updatePassword($user, '123456', [$config]);
 	}
 
-	public function testUpdateMasterPassword(): void {
+	public function testUpdateMasterPasswordWithExistingLoginPassword(): void {
 		/** @var IUser|MockObject $user */
 		$user = $this->createMock(IUser::class);
 		$account = $this->createMock(MailAccount::class);
@@ -260,6 +260,31 @@ class ManagerTest extends TestCase {
 		$this->manager->updatePassword($user, '123456', [$config]);
 	}
 
+	public function testUpdateMasterPasswordWithoutLoginPassword(): void {
+		/** @var IUser|MockObject $user */
+		$user = $this->createMock(IUser::class);
+		$account = $this->createMock(MailAccount::class);
+		$this->mock->getParameter('mailAccountMapper')
+			->expects($this->once())
+			->method('findProvisionedAccount')
+			->willReturn($account);
+		$config = new Provisioning();
+		$config->setProvisioningDomain(Provisioning::WILDCARD);
+		$config->setMasterPasswordEnabled(true);
+		$config->setMasterPassword('topsecret');
+		$this->mock->getParameter('crypto')
+			->expects(self::atLeast(1))
+			->method('encrypt')
+			->with('topsecret')
+			->willReturn('tercespot');
+		$this->mock->getParameter('mailAccountMapper')
+			->expects($this->once())
+			->method('update')
+			->with($account);
+
+		$this->manager->updatePassword($user, null, [$config]);
+	}
+
 	public function testNewProvisioning(): void {
 		$config = new Provisioning();
 		$this->mock->getParameter('provisioningMapper')

I don't have permissions to push to your branch.

[mickenordin]

The code fails when a users logs in with a password but master passwords are enabled. Then the login password took precedence. I think the master password should always go first if it's enabled.

This fixes the code and tests:

diff --git a/lib/Service/Provisioning/Manager.php b/lib/Service/Provisioning/Manager.php
index 2617a4d1e..ddbb55abc 100644
--- a/lib/Service/Provisioning/Manager.php
+++ b/lib/Service/Provisioning/Manager.php
@@ -333,16 +333,12 @@ class Manager {
 			// Maybe research other providers as well.
 			// Ref \OCA\Mail\Controller\PageController::index()
 			//     -> inital state for password-is-unavailable
-			if ($password === null) {
-				$masterPassword = $provisioning->getMasterPassword();
-				$masterPasswordEnabled = $provisioning->getMasterPasswordEnabled();
-				if ($masterPasswordEnabled && $masterPassword !== null) {
-					$password = $masterPassword;
-					$this->logger->debug('Password set to master password for ' . $user->getUID());
-				} else {
-					$this->logger->debug('No password set for ' . $user->getUID());
-					return;
-				}
+			if ($provisioning->getMasterPasswordEnabled() === true && $provisioning->getMasterPassword() !== null) {
+				$password = $provisioning->getMasterPassword();
+				$this->logger->debug('Password set to master password for ' . $user->getUID());
+			} else if ($password === null) {
+				$this->logger->debug('No password set for ' . $user->getUID());
+				return;
 			}
 
 			if (!empty($account->getInboundPassword())
diff --git a/tests/Unit/Http/Middleware/ProvisioningMiddlewareTest.php b/tests/Unit/Http/Middleware/ProvisioningMiddlewareTest.php
index 81f8e40d6..a54e6f5db 100644
--- a/tests/Unit/Http/Middleware/ProvisioningMiddlewareTest.php
+++ b/tests/Unit/Http/Middleware/ProvisioningMiddlewareTest.php
@@ -160,8 +160,6 @@ class ProvisioningMiddlewareTest extends TestCase {
 		$credentials->expects($this->once())
 			->method('getPassword')
 			->willReturn(null);
-		$this->provisioningManager->expects($this->never())
-			->method('updatePassword');
 
 		$this->middleware->beforeController(
 			$this->createMock(PageController::class),
diff --git a/tests/Unit/Service/Provisioning/ManagerTest.php b/tests/Unit/Service/Provisioning/ManagerTest.php
index 486466e67..083ff896e 100644
--- a/tests/Unit/Service/Provisioning/ManagerTest.php
+++ b/tests/Unit/Service/Provisioning/ManagerTest.php
@@ -235,7 +235,7 @@ class ManagerTest extends TestCase {
 		$this->manager->updatePassword($user, '123456', [$config]);
 	}
 
-	public function testUpdateMasterPassword(): void {
+	public function testUpdateMasterPasswordWithExistingLoginPassword(): void {
 		/** @var IUser|MockObject $user */
 		$user = $this->createMock(IUser::class);
 		$account = $this->createMock(MailAccount::class);
@@ -260,6 +260,31 @@ class ManagerTest extends TestCase {
 		$this->manager->updatePassword($user, '123456', [$config]);
 	}
 
+	public function testUpdateMasterPasswordWithoutLoginPassword(): void {
+		/** @var IUser|MockObject $user */
+		$user = $this->createMock(IUser::class);
+		$account = $this->createMock(MailAccount::class);
+		$this->mock->getParameter('mailAccountMapper')
+			->expects($this->once())
+			->method('findProvisionedAccount')
+			->willReturn($account);
+		$config = new Provisioning();
+		$config->setProvisioningDomain(Provisioning::WILDCARD);
+		$config->setMasterPasswordEnabled(true);
+		$config->setMasterPassword('topsecret');
+		$this->mock->getParameter('crypto')
+			->expects(self::atLeast(1))
+			->method('encrypt')
+			->with('topsecret')
+			->willReturn('tercespot');
+		$this->mock->getParameter('mailAccountMapper')
+			->expects($this->once())
+			->method('update')
+			->with($account);
+
+		$this->manager->updatePassword($user, null, [$config]);
+	}
+
 	public function testNewProvisioning(): void {
 		$config = new Provisioning();
 		$this->mock->getParameter('provisioningMapper')

I don't have permissions to push to your branch.

Nice!

I added you patch, and added you as co-author, and force pushed, so this should be fixed now.

[mickenordin]

Added the fix :)

[ChristophWurst]

/backport to stable3.7

[backportbot[bot]]

The backport to stable3.7 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable3.7
git pull origin stable3.7

# Create the new backport branch
git checkout -b backport/9653/stable3.7

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts, resolve them
git cherry-pick 7ef4d8e8 864b4bb8

# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/9653/stable3.7

Error: Failed to cherry pick commits: error: no cherry-pick or revert in progress fatal: cherry-pick failed

---

Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports.