1174900 - Capture doorhanger password field toggle should stay hidden for master password users

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Bernardo Rittmeyer [:rittme]]

Bug 1153217 and Bug 1169702 allow editing and seeing the password in the login capture doorhanger.
Users with Master Password enabled probably don't want the possibility to see their password in clear text. We should disable this feature in this case.

Comment 1

[Matthew N. [:MattN]]

We can probably look at the places that the pref signon.rememberSignons.visibilityToggle affects[1] and add a master password check like so:
`
let tokendb = Cc["@mozilla.org/security/pk11tokendb;1"].
              createInstance(Ci.nsIPK11TokenDB);
let token = tokendb.getInternalKeyToken();
token.checkPassword("")
`

`token.checkPassword("")` returns true if no master password is set.

[1] https://mxr.mozilla.org/mozilla-central/search?string=signon.rememberSignons.visibilityToggle&find=%2Ftoolkit%2Fcomponents%2Fpasswordmgr

Comment 2

[Prabhjyot Sodhi [:psd]]

Hi!
My last contribution was sometime back, Would love to work on this and brush up! :)
I'll need the mozilla-central repo, right?

Comment 3

[Matthew N. [:MattN]]

(In reply to Prabhjyot Sodhi [:psd] from comment #2)
> Hi!
> My last contribution was sometime back, Would love to work on this and brush
> up! :)

Hello, glad to see you're interested.

> I'll need the mozilla-central repo, right?

That's correct.

Comment 4

[Prabhjyot Sodhi [:psd]]

Hey Matthew!
(In reply to Matthew N. [:MattN] from comment #1)
> `
> let tokendb = Cc["@mozilla.org/security/pk11tokendb;1"].
>               createInstance(Ci.nsIPK11TokenDB);
> let token = tokendb.getInternalKeyToken();

Can you elaborate more on the working and significance of these lines of code.
I have recently started out with javascript, and thought that contributing some code in js might be helpful!

Comment 5

[Prabhjyot Sodhi [:psd]]

Attachment: 1174900.patch

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8649495 [details] [diff] [review]
1174900.patch

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

This is almost correct but doesn't work due to the mistake. Could you write an automated test like toolkit/components/passwordmgr/test/browser/browser_notifications.js which checks that the patch works. See https://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/test/pwmgr_common.js?rev=ac26180f1a16#211 for how to set a master password in a test. Make sure to cleanup and remove the master password in your cleanup function.

Find me in #passwords on IRC if you need assistance.

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js
@@ +857,5 @@
>        passwordField.setAttribute("type", "password");
>        passwordField.setAttribute("value", login.password);
> +      // Check if Master password is set
> +      let tokendb =  Cc["@mozilla.org/security/pk11tokendb;1"].
> +                    createInstance(Ci.nsIPK11TokenDB);

Nit: extra space so indentation is also off

@@ +861,5 @@
> +                    createInstance(Ci.nsIPK11TokenDB);
> +      let token = tokendb.getInternalKeyToken();
> +
> +      if (Services.prefs.getBoolPref("signon.rememberSignons.visibilityToggle" &&
> +           token.checkPassword(""))) {

I can tell you didn't test the patch since the ")" is in the wrong spot… Please do manual testing if you aren't submitting automated tests.

Comment 7

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

Prabhjyot, will you be following up on MattN's review of your patch? MattN, is there another developer/volunteer topick this up if Prabhjyot is unable to?

Comment 8

[Amal Oueslati]

Attachment: disablemasterpassword.patch

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8697076 [details] [diff] [review]
disablemasterpassword.patch

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

Thank you for the patch and apologies for the delay due to holidays and travel.

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js
@@ +856,5 @@
>        // Ensure the type is reset so the field is masked.
>        passwordField.setAttribute("type", "password");
>        passwordField.setAttribute("value", login.password);
> +	   //Users with Master Password enabled  don't see their passwords in clear text
> +	  let tokendb = Cc["@mozilla.org/security/pk11tokendb;1"].createInstance(Ci.nsIPK11TokenDB);

Nit: indentation seems to be a bit off.

@@ +858,5 @@
>        passwordField.setAttribute("value", login.password);
> +	   //Users with Master Password enabled  don't see their passwords in clear text
> +	  let tokendb = Cc["@mozilla.org/security/pk11tokendb;1"].createInstance(Ci.nsIPK11TokenDB);
> +	  let token = tokendb.getInternalKeyToken();
> +      if (Services.prefs.getBoolPref("signon.rememberSignons.visibilityToggle")&&(token.change-password("")) {

`change-password` isn't a method  on `token` so this code isn't working. Please try to do a manual test in a test profile with and without a Master Password to verify your patch works.

You should use `checkPassword` like is mentioned in comment 1.

Can you also add an automated test like I mentioned in comment 6?

Comment 10

[Matthew N. [:MattN]]

Gasolin, do you want to take this after bug 1217134? It's one of the two remaining blockers to enabling the checkbox by default.

See comment 1 for details. I think we just need to add a `token.checkPassword("")` check beside the pref check and leave it hidden if a master password is specified.

Comment 11

[[e-mail]]

Sure, I'll check it after bug 1217134

Comment 12

[[e-mail]]

after bug 1217134, it seems just need to add 

> let tokendb = Cc["@mozilla.org/security/pk11tokendb;1"]
>                              .createInstance(Ci.nsIPK11TokenDB);
> let token = tokendb.getInternalKeyToken();
> // Check if No Master password is set
> if (token.change-password("") &&
>    Services.prefs.getBoolPref("signon.rememberSignons.visibilityToggle")) {(

I only saw mochitest in passwordmgr/test/test_master_password.html , is there any other master password related test can be referenced?

Is there any master password related test can be referenced?

Comment 13

[[e-mail]]

sorry for dup the last line question :p

Comment 14

[Matthew N. [:MattN]]

Bug 1269039 is converting the master password tests for e10s but I didn't get around to investigating the failure causing the backout yet.

Comment 15

[[e-mail]]

Searched dxr and I found all calls with Ci.nsIPK11TokenDB are through `.getService` instead of `createInstance`.

https://dxr.mozilla.org/mozilla-central/search?q=Ci.nsIPK11TokenDB&redirect=false&case=true


The behaviour of those calls are different. After set a master password in preference:

With createInstance, the master password prompt is opened after press accept button in passwordMgr prompt.

With getService, the master password prompt is opened when enter a web site.


I think createInstance is the right behavior, but wonder why it is (and maybe it worth to add some comment in code)

Comment 16

[[e-mail]]

Attachment: Bug 1174900 - Capture doorhanger password field toggle should stay hidden for master password users;

Review commit: https://reviewboard.mozilla.org/r/52692/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52692/

Comment 17

[Matthew N. [:MattN]]

Comment on attachment 8752654 [details]
Bug 1174900 - Capture doorhanger password field toggle should stay hidden for master password users;

https://reviewboard.mozilla.org/r/52692/#review51278

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:973
(Diff revision 1)
>                         .addEventListener("input", onInput);
> -              if (Services.prefs.getBoolPref("signon.rememberSignons.visibilityToggle")) {
> +              let tokendb = Cc["@mozilla.org/security/pk11tokendb;1"]
> +                              .createInstance(Ci.nsIPK11TokenDB);
> +              let token = tokendb.getInternalKeyToken();
> +              // proceed if No Master password is set
> +              if (token.change-password("") &&

I don't think there is such a function on the token. Comment 1 has the correct method: `token.checkPassword("")`

Comment 18

[Matthew N. [:MattN]]

Though now I see the comment:
> boolean checkPassword(in wstring password);  /* Logs out if check fails */
at https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/nsIPK11Token.idl#45

and we don't want to log users out so if that comment is true then we should use whatever method the preferences use to check the master password checkbox.

Comment 19

[[e-mail]]

Sorry I should trace code a bit to make sure all works.

I guess you mean doing masterPasswordLogin
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/content/passwordManager.js#494


Though in test `isLoggedIn` seems sufficient
https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/nsIPK11Token.idl#30

There's even a Service.logins.isLoggedIn wrapper so we don't need to import extra token library
https://dxr.mozilla.org/mozilla-central/search?q=logins.islo&redirect=false

Comment 20

[Matthew N. [:MattN]]

(In reply to Fred Lin [:gasolin] from comment #19)
> Sorry I should trace code a bit to make sure all works.
> 
> I guess you mean doing masterPasswordLogin
> https://dxr.mozilla.org/mozilla-central/source/toolkit/components/
> passwordmgr/content/passwordManager.js#494

No, I'm talking about the about:preferences#security "Use a master password" logic.

> Though in test `isLoggedIn` seems sufficient
> https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/
> nsIPK11Token.idl#30
> 
> There's even a Service.logins.isLoggedIn wrapper so we don't need to import
> extra token library
> https://dxr.mozilla.org/mozilla-central/search?q=logins.islo&redirect=false

I don't think we want this since that would mean that users who have a master password but who are already logged in would be able to see the plaintext password with re-entering their master password (like they normally would have to in the management UI). We don't want to ask for the master password each time they toggle because the MP dialog would cause the doorhanger to close. The shortcut is to just don't give the feature to MP users regardless of their logged in state.

Comment 21

[[e-mail]]

Please correct me if I miss-understand the issue.
After UI change, the new requirement will be:

If master password is set:
1. disable the password field
2. disable the checkbox

therefore we can use _masterPasswordSet function in about:preferences#security to detect the condition

https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/security.js#224

Then disable both password field and the checkbox

Comment 22

[[e-mail]]

To make password still changable, might be only disable or hide the checkbox, and keep password field as it is.

Comment 23

[[e-mail]]

Comment on attachment 8752654 [details]
Bug 1174900 - Capture doorhanger password field toggle should stay hidden for master password users;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52692/diff/1-2/

Comment 24

[[e-mail]]

Comment on attachment 8752654 [details]
Bug 1174900 - Capture doorhanger password field toggle should stay hidden for master password users;

Set feedback to make sure its the right behaviour (disable the password toggle checkbox if master password is set, so there's no way to show password in plain text).

Would like add some test afterwards.

Comment 25

[[e-mail]]

Comment on attachment 8752654 [details]
Bug 1174900 - Capture doorhanger password field toggle should stay hidden for master password users;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52692/diff/2-3/

Comment 26

[[e-mail]]

test added by referencing https://dxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/MasterPassword.js#31

Comment 27

[Matthew N. [:MattN]]

Updating summary to match comment 10

Comment 28

[Matthew N. [:MattN]]

Comment on attachment 8752654 [details]
Bug 1174900 - Capture doorhanger password field toggle should stay hidden for master password users;

https://reviewboard.mozilla.org/r/52692/#review54654

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:43
(Diff revision 3)
> +/**
> + * check if master password is set.
> + */
> +function isMasterPasswordSet() {

Can you move this to LoginHelper.jsm please and have the preferences code use it too? I think there are a few tests which could also use the shared helper but you don't need to do that.

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:988
(Diff revision 3)
>                chromeDoc.getElementById("password-notification-username")
>                         .addEventListener("input", onInput);
>                chromeDoc.getElementById("password-notification-password")
>                         .addEventListener("input", onInput);
> +
>                if (Services.prefs.getBoolPref("signon.rememberSignons.visibilityToggle")) {

I think for now we can just hide it like we do with the pref (as I mentioned in comment 10) as we would need an explanation tooltip if we only disable I think and I think it adds confusion.

::: toolkit/components/passwordmgr/test/browser/head.js:121
(Diff revision 3)
> +const masterPassword = "omgsecret!";
> +
> +function enableMasterPassword() {
> +  _setMasterPassword(true);
> +}
> +
> +function disableMasterPassword() {
> +  _setMasterPassword(false);
> +}
> +
> +function _setMasterPassword(enable) {

Can you please put all of this in LoginTestUtils.jsm so we can share it with the mochitest-plain/chrome version?

Comment 29

[[e-mail]]

https://reviewboard.mozilla.org/r/52692/#review54654

> Can you move this to LoginHelper.jsm please and have the preferences code use it too? I think there are a few tests which could also use the shared helper but you don't need to do that.

thanks for review! I'll move isMasterPasswordSet to `LoginHelper.jsm`. 

Not sure what do you mean for `have the preferences code use it too`?

Comment 30

[[e-mail]]

Comment on attachment 8752654 [details]
Bug 1174900 - Capture doorhanger password field toggle should stay hidden for master password users;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52692/diff/3-4/

Comment 31

[[e-mail]]

Comment on attachment 8752654 [details]
Bug 1174900 - Capture doorhanger password field toggle should stay hidden for master password users;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52692/diff/4-5/

Comment 32

[[e-mail]]

as irc discussion, moved reference of _masterPasswordSet from perference/in-content/security.js to LoginHelper.jsm

Comment 33

[Matthew N. [:MattN]]

Comment on attachment 8752654 [details]
Bug 1174900 - Capture doorhanger password field toggle should stay hidden for master password users;

https://reviewboard.mozilla.org/r/52692/#review54958

To avoid another iteration I fixed the issues myself since I've been slow reviewing. Thanks!

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:976
(Diff revision 5)
> -                         .setAttribute("label", togglePasswordLabel);
> -                chromeDoc.getElementById("password-notification-visibilityToggle")
> +                toggleBtn.setAttribute("hidden",
> +                  LoginHelper.isMasterPasswordSet());

Nit: No need to wrap this as it's < 100 and also narrow than the getBoolPref line above.

::: toolkit/components/passwordmgr/test/LoginTestUtils.jsm:253
(Diff revision 5)
> +this.LoginTestUtils.master = {
> +  masterPassword: "omgsecret!",
> +
> +  enableMasterPassword() {
> +    this._setMasterPassword(true);
> +  },
> +
> +  disableMasterPassword() {
> +    this._setMasterPassword(false);
> +  },

I think it would be nicer to have the object called "masterPassword" but then remove the redundancy in the method names like so:
```
LoginTestUtils.masterPassword.enable();
```

::: toolkit/components/passwordmgr/test/LoginTestUtils.jsm:278
(Diff revision 5)
> +      let secmodDB = Cc["@mozilla.org/security/pkcs11moduledb;1"]
> +                          .getService(Ci.nsIPKCS11ModuleDB);

Nit: indentation

::: toolkit/components/passwordmgr/test/LoginTestUtils.jsm:292
(Diff revision 5)
> +      } else if (status == Ci.nsIPKCS11Slot.SLOT_READY) {
> +        info("MP change from " + oldPW + " to " + newPW);
> +        token.changePassword(oldPW, newPW);

`info` isn't defined in this module as that comes from mochitest.

::: toolkit/components/passwordmgr/test/LoginTestUtils.jsm:297
(Diff revision 5)
> +    } catch(e) {
> +      dump("MasterPassword.setPassword: " + e);
> +    }

This `catch` may hide test errors so please remove it.

::: toolkit/components/passwordmgr/test/LoginTestUtils.jsm:302
(Diff revision 5)
> +    } catch(e) {
> +      dump("MasterPassword.setPassword: " + e);
> +    }
> +    return false;
> +  }
> +}

Nit: missing semicolon

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/ae0ba94e684b
Capture doorhanger password field toggle should stay hidden for master password users;r=MattN

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ae0ba94e684b

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ae0ba94e684b

Comment 37

[[e-mail]]

Thanks for help landing it!