1296619 - Add test for PK11PasswordPromptRunnable

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

[:Cykesiopka]

PK11PasswordPromptRunnable currently doesn't have any test coverage. It probably should.

Comment 1

[Dana Keeler (she/her) [:keeler]]

Attachment: bug 1296619 - add a test to ensure that prompting for the master password probably works

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

Comment 2

[Dana Keeler (she/her) [:keeler]]

Here's a try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1092ef734f51 (the eslint issue is fixed in the revision I have up for review)

Comment 3

[:Cykesiopka]

Comment on attachment 8783602 [details]
bug 1296619 - add a test to ensure that prompting for the master password probably works

https://reviewboard.mozilla.org/r/73354/#review71388

::: security/manager/ssl/tests/unit/test_password_prompt.js:16
(Diff revision 1)
> +
> +var gMockPrompter = {
> +  passwordToTry: null,
> +  numPrompts: 0,
> +
> +  promptPassword: (dialogTitle, text, password, checkMsg, checkState) => {

The nsIPromptService.idl version of this method uses ```checkState``` as the param name, but the nsIPrompt.idl version uses ```checkValue``` for some reason.

We're mocking out the nsIPrompt version, so maybe s/checkState/checkValue/, but this is very minor, so up to you.

::: security/manager/ssl/tests/unit/test_password_prompt.js:17
(Diff revision 1)
> +    // Note that because of how xpcom wraps things, |this| is a wrapper on a C++
> +    // object that is a wrapper on a JS object, so |this != gMockPrompter|.
> +    // More importantly, properties on |gMockPrompter| aren't necessarily
> +    // exposed on |this| (hence we use |gMockPrompter| instead of |this|).

Hmm, it looks like this is a non-issue if we use shorthand function syntax instead of an arrow function.

::: security/manager/ssl/tests/unit/test_password_prompt.js:58
(Diff revision 1)
> +
> +  // Set an initial password.
> +  let tokenDB = Cc["@mozilla.org/security/pk11tokendb;1"]
> +                  .getService(Ci.nsIPK11TokenDB);
> +  let token = tokenDB.getInternalKeyToken();
> +  token.initPassword("hunter2");

TIL new meme :D

Comment 4

[:Cykesiopka]

Comment on attachment 8783602 [details]
bug 1296619 - add a test to ensure that prompting for the master password probably works

https://reviewboard.mozilla.org/r/73354/#review71390

Oops. Looks good!

Comment 5

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8783602 [details]
bug 1296619 - add a test to ensure that prompting for the master password probably works

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

Comment 6

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8783602 [details]
bug 1296619 - add a test to ensure that prompting for the master password probably works

https://reviewboard.mozilla.org/r/73354/#review71388

Thanks!

> The nsIPromptService.idl version of this method uses ```checkState``` as the param name, but the nsIPrompt.idl version uses ```checkValue``` for some reason.
> 
> We're mocking out the nsIPrompt version, so maybe s/checkState/checkValue/, but this is very minor, so up to you.

Sounds good.

> Hmm, it looks like this is a non-issue if we use shorthand function syntax instead of an arrow function.

Interesting - I wonder if this is a result of some difference between the two or if it's a bug...

> TIL new meme :D

:)

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f96a181ce623
add a test to ensure that prompting for the master password probably works r=Cykesiopka

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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