feat(provisioning): Master password support by ChristophWurst · Pull Request #9008 · nextcloud/mail

ChristophWurst · 2023-10-27T08:47:00Z

Amended and squashed version of #8917.

How to test

Build feat: Allow a static password for auth ChristophWurst/docker-imap-devel#13
Set up provisioning for the static password from the config file

How to test – Alternative

Set up provisioning with your testing account's email address and the "static" IMAP password 😉
Verify the account works

mickenordin · 2023-11-02T13:46:42Z

Will this be merged in time for 3.5.0? I saw that you guys were tagging beta releases?

ChristophWurst · 2023-11-02T13:53:13Z

Yes, no rush. 3.5 will only be branched off in a week. This PR should make it.

ChristophWurst · 2023-11-07T10:15:49Z

@mickenordin may I ask you to also give the latest state a final test? 🙏

mickenordin · 2023-11-07T10:17:17Z

@mickenordin may I ask you to also give the latest state a final test? 🙏

Definetly! Will get right on it!

mickenordin · 2023-11-07T15:23:21Z

Unfortunately this does no longer work for me. The app now fails to detect that the user should see the mail box, and instead claims that it is disabled due to being a SSO account.

The actual code works though it seems. If I set the master password and give a local user the same email it works, but not for a user signed in with single signon.

I am pretty sure that this used to work, so I am not sure what happened here. How can it have stopped detecting that the user account should be able to see the mail box?

What I did to set this up by the way was this:

git clone https://github.com/nextcloud/mail 
cd mail
git checkout v3.5.0-beta1
wget https://patch-diff.githubusercontent.com/raw/nextcloud/mail/pull/9008.diff
patch -p1 < 9008.diff
git add .
git commit -m "Master password patch"
make build-js-production
make appstore

ChristophWurst · 2023-11-07T20:02:55Z

I'm also unable to test the changes at the moment

mickenordin · 2023-11-08T07:40:50Z

@ChristophWurst I see what is missing now, it is the check in

src/components/Navigation.vue in https://github.com/nextcloud/mail/pull/8917/files#diff-578fcad0646f9b6ab3e4f8bbe2f8f946e54d39670c0da653db620aaead7d9663

And the corresponding getter in store

mickenordin · 2023-11-08T08:10:07Z

From: Micke Nordin ***@***.***>

…

--- src/components/Navigation.vue | 3 ++- src/store/getters.js | 1 + src/store/index.js | 1 + src/store/mutations.js | 3 +++ 4 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/components/Navigation.vue b/src/components/Navigation.vue index 5feaf97b0..adcaa26d2 100644 --- a/src/components/Navigation.vue +++ b/src/components/Navigation.vue @@ -182,7 +182,8 @@ export default { * @return {boolean} True if the account should be disabled */ isDisabled(account) { - return this.passwordIsUnavailable && !!account.provisioningId + + return (this.passwordIsUnavailable && !!account.provisioningId) && !!this.$store.getters.masterPasswordEnabled }, }, } diff --git a/src/store/getters.js b/src/store/getters.js index da6424222..e2d7f1bf5 100644 --- a/src/store/getters.js +++ b/src/store/getters.js @@ -117,6 +117,7 @@ export const getters = { isScheduledSendingDisabled: (state) => state.isScheduledSendingDisabled, isSnoozeDisabled: (state) => state.isSnoozeDisabled, googleOauthUrl: (state) => state.googleOauthUrl, + masterPasswordEnabled: (state) => state.masterPasswordEnabled, microsoftOauthUrl: (state) => state.microsoftOauthUrl, getActiveSieveScript: (state) => (accountId) => state.sieveScript[accountId], getCurrentUserPrincipal: (state) => state.currentUserPrincipal, diff --git a/src/store/index.js b/src/store/index.js index fbd245183..6bdbda6fe 100644 --- a/src/store/index.js +++ b/src/store/index.js @@ -103,6 +103,7 @@ export default new Store({ isSnoozeDisabled: false, currentUserPrincipal: undefined, googleOauthUrl: null, + masterPasswordEnabled: false, sieveScript: {}, calendars: [], smimeCertificates: [], diff --git a/src/store/mutations.js b/src/store/mutations.js index ad7c1beb8..3719dc699 100644 --- a/src/store/mutations.js +++ b/src/store/mutations.js @@ -475,6 +475,9 @@ export default { setGoogleOauthUrl(state, url) { state.googleOauthUrl = url }, + setMasterPasswordEnabled(state, value) { + state.masterPasswordEnabled = value + }, setMicrosoftOauthUrl(state, url) { state.microsoftOauthUrl = url }, -- 2.42.0

mickenordin · 2023-11-08T08:12:04Z

The patch above fixes the issue and here it is as an attachement as well.
8917.diff.txt

ChristophWurst · 2023-11-08T21:04:10Z

The feature works again 🚀

ChristophWurst

👍 on @mickenordin's changes :)

Thanks a lot

SSO users do not have a password set, and the auto provisioning of mail does not work for SSO user. It is also inconvenient to synchronize the password database between Nextcloud and the mail server used. So to allow SSO user to use the mail app we can instead configure a shared password for all users, this will work well with for example Dovecot that has a concept of a "master password"[0] that can be used to authenticate users. To use this feature we must convince the mail app that the user has a password available, which we can set with occ like so: ``` ./occ config:app:set mail master_password --value 'very-secret-master-password' ``` We can then configure dovecot to allow this password from the Nextcloud server, in this example 89.46.21.198: ``` passdb { args = password=very-secret-master-password allow_nets=89.46.21.198/32 driver = static } ``` If we configure postfix to use SASL auth against dovecot, we can then both send and recieve mail from Nextcloud mail app, for SSO users. 0. https://doc.dovecot.org/configuration_manual/authentication/master_users/ Co-Authored-By: Christoph Wurst <[email protected]> Signed-off-by: Micke Nordin <[email protected]> Signed-off-by: Christoph Wurst <[email protected]>

miaulalala · 2023-11-09T15:46:28Z

+ * @method bool|null getMasterPasswordEnabled()
+ * @method void setMasterPasswordEnabled(bool $masterPasswordEnabled)
+ * @method string|null getMasterPassword()
+ * @method void setMasterPassword(string $masterPassword)


Suggested change

* @method void setMasterPassword(string $masterPassword)

* @method void setMasterPassword(?string $masterPassword)

To unset the master password?

miaulalala · 2023-11-09T15:46:43Z

+ * @method bool|null getMasterPasswordEnabled()
+ * @method void setMasterPasswordEnabled(bool $masterPasswordEnabled)
+ * @method string|null getMasterPassword()
+ * @method void setMasterPassword(string $masterPassword)


To unset the master password?

miaulalala · 2023-11-09T15:49:37Z

+			$masterPasswordEnabled = $provisioning->getMasterPasswordEnabled();
+			if ($masterPasswordEnabled && $masterPassword !== null) {
+				$password = $masterPassword;
+				$this->logger->debug('Password set to master password for ' . $user->getUID());


early return here? Having both a master pw and a regular pw is unneccessary, right?

sevmonster · 2023-12-08T21:39:02Z

This PR doesn't work if the user never had a password to begin with. This is the case in my environment as none of my users were created with a password, they were all created via user_saml.

mail/lib/Http/Middleware/ProvisioningMiddleware.php

Lines 60 to 88 in 3fc016f

    
           	public function beforeController($controller, $methodName) { 
        
           		$user = $this->userSession->getUser(); 
        
           		if ($user === null) { 
        
           			// Nothing to update 
        
           			return; 
        
           		} 
        
           		$configs = $this->provisioningManager->getConfigs(); 
        
           		if ($configs === []) { 
        
           			return; 
        
           		} 
        
           		try { 
        
           			$this->provisioningManager->provisionSingleUser($configs, $user); 
        
           			$password = $this->credentialStore->getLoginCredentials()->getPassword(); 
        
           			// FIXME: Need to check for an empty string here too? 
        
           			// The password is empty (and not null) when using WebAuthn passwordless login. 
        
           			// Maybe research other providers as well. 
        
           			// Ref \OCA\Mail\Controller\PageController::index() 
        
           			//     -> inital state for password-is-unavailable 
        
           			if ($password === null) { 
        
           				// Nothing to update, might be passwordless signin 
        
           				$this->logger->debug('No password set for ' . $user->getUID()); 
        
           				return; 
        
           			} 
        
           			$this->provisioningManager->updatePassword( 
        
           				$user, 
        
           				$password, 
        
           				$configs 
        
           			);

I am not sure why the password logic is split between two places like this when it seems the only place updatePassword is used is in the tests. Could all of this logic be moved into the updatePassword function for simplicity and also to help fix this regression? I am not a NC dev so I don't know if this split is a requirement for some reason.

I have monkeypatched this in my environment by always calling updatePassword with the empty string as a password, which is then set overriden in the function by the master password, since none of my provisionable users have a password set anyway.

mickenordin · 2023-12-11T07:38:26Z

My users also don't have a password set and were created by the global site selector. This is what this PR aims to fix by allowing the administrator to set a master password. Can you explain a bit more clearly what is not working, and what the regression is?

ChristophWurst · 2023-12-11T08:58:49Z

Please put this into a new ticket.

sevmonster · 2023-12-15T00:04:09Z

My users also don't have a password set and were created by the global site selector.

Your users probably have something set as their password in the database; they would not be able to use provisioned accounts at all if they didn't. Users without any password set (be it in use or if they signed in via SSO) will have an exception thrown, and updatePassword() is never called. I could reproduce this with all of my users, whom have blank passwords in the database. Please check your actual database for values in the password row.

Can you explain a bit more clearly what is not working, and what the regression is?

@mickenordin @ChristophWurst #9175

ChristophWurst added the 2. developing label Oct 27, 2023

ChristophWurst self-assigned this Oct 27, 2023

ChristophWurst mentioned this pull request Oct 27, 2023

Allow SSO users to send and recieve mail via master password #8917

Closed

ChristophWurst force-pushed the feat/master-password branch from fc1bbef to abf92a6 Compare October 27, 2023 08:50

ChristophWurst added the enhancement label Oct 27, 2023

This comment was marked as outdated.

Sign in to view

ChristophWurst commented Oct 30, 2023

View reviewed changes

Comment thread lib/Db/Provisioning.php Outdated

ChristophWurst commented Nov 2, 2023

View reviewed changes

Comment thread src/components/settings/ProvisioningSettings.vue Outdated

ChristophWurst added 3. to review and removed 2. developing labels Nov 2, 2023

ChristophWurst requested review from kesselb and st3iny November 2, 2023 09:53

ChristophWurst marked this pull request as ready for review November 7, 2023 10:15

ChristophWurst requested a review from hamza221 November 8, 2023 21:04

ChristophWurst commented Nov 8, 2023

View reviewed changes

ChristophWurst force-pushed the feat/master-password branch from c283ab3 to ae74847 Compare November 8, 2023 21:12

ChristophWurst enabled auto-merge November 8, 2023 21:12

ChristophWurst requested a review from miaulalala November 9, 2023 09:34

miaulalala reviewed Nov 9, 2023

View reviewed changes

miaulalala approved these changes Nov 9, 2023

View reviewed changes

ChristophWurst merged commit 76c2dad into main Nov 9, 2023

ChristophWurst deleted the feat/master-password branch November 9, 2023 15:49

sevmonster mentioned this pull request Dec 14, 2023

Master Password feature doesn't work for all accounts #9175

Closed

kesselb mentioned this pull request Jun 25, 2024

Feature Request: allow specifying password for default account #849

Closed

	* @method void setMasterPassword(string $masterPassword)
	* @method void setMasterPassword(?string $masterPassword)

Conversation

ChristophWurst commented Oct 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How to test

How to test – Alternative

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

mickenordin commented Nov 2, 2023

Uh oh!

ChristophWurst commented Nov 2, 2023

Uh oh!

ChristophWurst commented Nov 7, 2023

Uh oh!

mickenordin commented Nov 7, 2023

Uh oh!

mickenordin commented Nov 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChristophWurst commented Nov 7, 2023

Uh oh!

mickenordin commented Nov 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mickenordin commented Nov 8, 2023 via email

Uh oh!

mickenordin commented Nov 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChristophWurst commented Nov 8, 2023

Uh oh!

ChristophWurst left a comment

Choose a reason for hiding this comment

Uh oh!

miaulalala Nov 9, 2023

Choose a reason for hiding this comment

Uh oh!

miaulalala Nov 9, 2023

Choose a reason for hiding this comment

Uh oh!

miaulalala Nov 9, 2023

Choose a reason for hiding this comment

Uh oh!

miaulalala Nov 9, 2023

Choose a reason for hiding this comment

Uh oh!

sevmonster commented Dec 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mickenordin commented Dec 11, 2023

Uh oh!

ChristophWurst commented Dec 11, 2023

Uh oh!

sevmonster commented Dec 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ChristophWurst commented Oct 27, 2023 •

edited

Loading

mickenordin commented Nov 7, 2023 •

edited

Loading

mickenordin commented Nov 8, 2023 •

edited

Loading

mickenordin commented Nov 8, 2023 •

edited

Loading

sevmonster commented Dec 8, 2023 •

edited

Loading

sevmonster commented Dec 15, 2023 •

edited

Loading