1622293 - Port bug 1194529: Ask the user for their OS account password before showing the password dialogs

Status: RESOLVED FIXED

(Thunderbird :: Preferences, task)

Description

[Richard Marti (:Paenglab)]

This bug is to use the OS to check if the current user is the one that logged in.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: 1622293-ask-OS-password.patch

Tested on all platforms when trying to set a master password or showing/copying the passwords in the Saved Passwords dialog. On Linux, entered once it remembers it during the session.

On Windows it doesn't show the brand name automatically like FX. For this I had to add the messageTitle. And this is a bit weird as I needed to add the -brand-full-name in the other two FTL files as a variable. Directly reference it in the JS files doesn't work because of the leading dash (I tested this with removing it).

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9133210 [details] [diff] [review]
1622293-ask-OS-password.patch

Review of attachment 9133210 [details] [diff] [review]:
-----------------------------------------------------------------

I think we should see if OSKeyStore.jsm can move  to toolkit/

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 9133210 [details] [diff] [review]
1622293-ask-OS-password.patch

Review of attachment 9133210 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/preferences/passwordManager.js
@@ -481,5 @@
>      "PWMGR_MANAGE_VISIBILITY_TOGGLED"
>    );
>  }
>  
> -async function AskUserShowPasswords() {

Since OSKeyStore doesn't work on Linux yet you may want to leave this for them.

@@ +788,5 @@
>    let token = tokendb.getInternalKeyToken();
>  
>    // If there is no master password, still give the user a chance to opt-out of displaying passwords
>    if (token.checkPassword("")) {
> +    // Require OS authentication before the user can set a Master Password

I think you copied this comment here by mistake

::: mail/locales/en-US/messenger/preferences/passwordManager.ftl
@@ +78,5 @@
> +# This message can be seen by trying to show or copy the passwords.
> +password-os-auth-dialog-message = Verify your identity to access your Passwords.
> +
> +# This message can be seen by trying to show or copy the passwords.
> +# The macOS strings are preceded by the operating system with "Thunderbird trying to "

Nit: missing "is"

Comment 4

[Richard Marti (:Paenglab)]

Attachment: Linux-OS-dialog.png

(In reply to Matthew N. [:MattN] (PM me if request are blocking you) from comment #3)

Comment on attachment 9133210 [details] [diff] [review]
1622293-ask-OS-password.patch

Review of attachment 9133210 [details] [diff] [review]:

::: mail/components/preferences/passwordManager.js
@@ -481,5 @@

 "PWMGR_MANAGE_VISIBILITY_TOGGLED"

);
}

-async function AskUserShowPasswords() {

Since OSKeyStore doesn't work on Linux yet you may want to leave this for
them.

Hmm, I see a dialog on Linux, see screenshot. What doesn't work on Linux?

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 9133210 [details] [diff] [review]
1622293-ask-OS-password.patch

Review of attachment 9133210 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/preferences/passwordManager.js
@@ +807,5 @@
> +          id: brandName,
> +        },
> +      ]);
> +      let loggedIn = await OSKeyStore.ensureLoggedIn(messageText.value,
> +        messageTitle.value);

You are missing the last two arguments
https://searchfox.org/mozilla-central/rev/fb3b0075d1a9c4dafbdf339b835d462b5ae55a0e/browser/components/aboutlogins/AboutLoginsParent.jsm#347,350-351 which are important. See https://searchfox.org/mozilla-central/rev/fb3b0075d1a9c4dafbdf339b835d462b5ae55a0e/browser/modules/OSKeyStore.jsm#125-142

(In reply to Richard Marti (:Paenglab) from comment #4)
> Hmm, I see a dialog on Linux, see screenshot. 

That's because you didn't pass the last argument. I don't think this will work properly but I could be wrong. It's been over a year since I looked into it.

> What doesn't work on Linux?

Bug 1527745

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 9133210 [details] [diff] [review]
1622293-ask-OS-password.patch

Review of attachment 9133210 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/modules/OSKeyStore.jsm
@@ +39,5 @@
> +var OSKeyStore = {
> +  /**
> +   * On macOS this becomes part of the name label visible on Keychain Acesss as
> +   * "org.mozilla.nss.keystore.thunderbird" (where "thunderbird" is
> +   * the MOZ_APP_NAME).

We are very likely to break compatibility with this name since it's currently somewhat ugly. You should consider all of the actually key storage parts (like the Linux dialog you saw from not skipping key generation) to be experimental and subject to break at any point.

The OS re-auth dialogs (which are mostly independent of key storage) are more stable but note that we have new crashes already.

Comment 7

[Richard Marti (:Paenglab)]

Attachment: 1622293-ask-OS-password.patch

This should fix all of Matt's comments. I missed to port bug 1506602.

I added now in passwordManager.js a check, if Linux, then show the TB confirm dialog.

Comment 8

[Richard Marti (:Paenglab)]

Attachment: 1622293-ask-OS-password.patch

Updated patch to follow bug 1622304.

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9134756 [details] [diff] [review]
1622293-ask-OS-password.patch

Bug 1622514 was accepted.

Comment 10

[Richard Marti (:Paenglab)]

Attachment: 1622293-ask-OS-password.patch

Updated patch using the toolkit OSKeyStore.jsm.

This time linting works locally and I get:
z:/Mozilla/comm-central/comm/mail/components/preferences/passwordManager.js
843:5 error Async function 'masterPasswordLogin' expected no return value. consistent-return (eslint)
856:3 error Async function 'masterPasswordLogin' expected no return value. consistent-return (eslint)

Please can you guide me what is needed to fix this? Removing the return values makes the passwords no more shown.

Comment 11

[Matthew N. [:MattN]]

(In reply to Richard Marti (:Paenglab) from comment #10)

Created attachment 9135911 [details] [diff] [review]
1622293-ask-OS-password.patch

Updated patch using the toolkit OSKeyStore.jsm.

This time linting works locally and I get:
z:/Mozilla/comm-central/comm/mail/components/preferences/passwordManager.js
843:5 error Async function 'masterPasswordLogin' expected no return value. consistent-return (eslint)
856:3 error Async function 'masterPasswordLogin' expected no return value. consistent-return (eslint)

Please can you guide me what is needed to fix this? Removing the return values makes the passwords no more shown.

Don't remove the return, instead have it return a consistent type. You are adding return; without a value which existing returns are returning a value (a boolean?).

Comment 12

[Magnus Melin [:mkmelin]]

Comment on attachment 9135911 [details] [diff] [review]
1622293-ask-OS-password.patch

Review of attachment 9135911 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/locales/en-US/messenger/preferences/passwordManager.ftl
@@ +72,5 @@
> +
> +## OS Authentication dialog
> +
> +# This message can be seen by trying to show or copy the passwords.
> +password-os-auth-dialog-message = Verify your identity to access your Passwords.

lowercase p
But maybe better to (almost) copy ff: Verify your identity to reveal the saved passwords.

@@ +78,5 @@
> +# This message can be seen by trying to show or copy the passwords.
> +# The macOS strings are preceded by the operating system with "Thunderbird is trying to "
> +# and includes subtitle of "Enter password for the user "xxx" to allow this." These
> +# notes are only valid for English. Please test in your locale.
> +password-os-auth-dialog-message-macosx = access your Passwords

Not sure what's going on with the casing here? I think it should be lower case p in this case.

Comment 13

[Richard Marti (:Paenglab)]

Attachment: 1622293-ask-OS-password.patch

Linting is okay now and it works on Mac/Windows with the system dialog and on Linux with the show password dialog.

Strings fixed.

Comment 14

[Magnus Melin [:mkmelin]]

Comment on attachment 9136112 [details] [diff] [review]
1622293-ask-OS-password.patch

Review of attachment 9136112 [details] [diff] [review]:
-----------------------------------------------------------------

Seems good, thx! r=mkmelin

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/92e53e3108d1
Port bug 1194529: Ask the user for their OS account password before showing the password dialogs. r=mkmelin