Add test for PK11PasswordPromptRunnable

RESOLVED FIXED in Firefox 51

Status

()

Core
Security: PSM
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Cykesiopka, Assigned: keeler)

Tracking

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [psm-assigned])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a year ago
PK11PasswordPromptRunnable currently doesn't have any test coverage. It probably should.
Comment hidden (mozreview-request)
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)
Assignee: nobody → dkeeler
Priority: P3 → P1
Whiteboard: [psm-backlog] → [psm-assigned]
(Reporter)

Comment 3

a year ago
mozreview-review
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
(Reporter)

Comment 4

a year ago
mozreview-review
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!
Attachment #8783602 - Flags: review?(cykesiopka.bmo) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 6

a year ago
mozreview-review-reply
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

a year ago
Pushed by dkeeler@mozilla.com:
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f96a181ce623
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.