Remove token choosing functionality from changepassword.(js|xul)

RESOLVED FIXED in Firefox 53

Status

()

Core
Security: PSM
P1
minor
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: Cykesiopka, Assigned: Cykesiopka)

Tracking

({addon-compat})

unspecified
mozilla53
addon-compat
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [psm-assigned], URL)

MozReview Requests

()

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

Attachments

(1 attachment)

(Assignee)

Description

5 months ago
If changepassword.(js|xul) is given the empty string as the token name to set a password on, it shows a drop down to allow the user to pick a token from the token DB.

I would like to get rid of this functionality:
 1. AFAICT all code in m-c already provides the name of a specific token.
 2. We already have the Device Manager UI that lists all the know slots, so the drop down functionality seems duplicated.
 3. It makes both files more complicated, and makes it more annoying to write an automated test.

changepassword.(js|xul) is accessible through nsITokenPasswordDialogs.setPassword(), meaning doing this could theoretically break addons. However, it should be fairly straightforward for such addons (if they even exist) to just make use of nsITokenDialogs.ChooseToken() as a replacement.
Comment hidden (mozreview-request)

Comment 2

5 months ago
mozreview-review
Comment on attachment 8823094 [details]
Bug 1325648 - Remove token choosing functionality from changepassword.(js|xul).

https://reviewboard.mozilla.org/r/101700/#review102616

LGTM, although see the first comment.

::: security/manager/ssl/nsITokenPasswordDialogs.idl:21
(Diff revision 1)
>    /**
> -   * setPassword - sets the password/PIN on the named token.
> -   *   The canceled output value should be set to TRUE when
> -   *   the user (or implementation) cancels the operation.
> +   * Brings up a dialog to set the password on a token.
> +   *
> +   * @param ctx A user interface context.
> +   * @param tokenName Name of the token.
> +   * @return true if the user canceled the dialog, false otherwise.

I find the original formulation of this API a little suspect. It seems to be something along the lines of "please ask the user to set a password and by the way did they cancel?" which is oddly specific, given that it's more common to create an API that attempts to do something and returns a general status code depending on the result. Furthermore, the consumers of this API are inconsistent in how they treat the case where the user cancels the dialog. The secret decoder ring completely ignores this case, which seems wrong when compared to the other uses that turn it into an nserror that gets propagated. I think it might be best to just indicate that the user cancelled by returning an error. What do you think?

::: security/manager/ssl/tests/unit/test_sdr.js:18
(Diff revision 1)
>  const gTokenPasswordDialogs = {
> -  setPassword: (ctx, tokenName, canceled) => {
> +  setPassword(ctx, tokenName) {
>      gSetPasswordShownCount++;
>      do_print(`setPassword() called; shown ${gSetPasswordShownCount} times`);
>      do_print(`tokenName: ${tokenName}`);
> -    canceled.value = false;
> +    return false;

Might comment that returning false means "the user didn't cancel".
Attachment #8823094 - Flags: review?(dkeeler) → review+
(Assignee)

Comment 3

5 months ago
(In reply to David Keeler [:keeler] (use needinfo?) from comment #2)
> propagated. I think it might be best to just indicate that the user
> cancelled by returning an error. What do you think?

Hmm, I'm not sure what you mean. Is it:
1. Fix the SDR implementation to propagate when a user cancels upwards?
2. Change nsITokenPasswordDialogs.setPassword() to throw when the user cancels instead of returning a boolean?
3. Something else?
Flags: needinfo?(dkeeler)
Yeah, option 2 is basically what I meant, but it's probably not really worth changing at this point.
Flags: needinfo?(dkeeler)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

5 months ago
(In reply to David Keeler [:keeler] (use needinfo?) from comment #4)
> Yeah, option 2 is basically what I meant, but it's probably not really worth
> changing at this point.

I see.
Yes, I agree that it's probably not worth changing at this point.
(Assignee)

Comment 7

5 months ago
Thanks for the review!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3950dc5d1e06c154a6c06e14048c55a0b20ff47a
Keywords: checkin-needed

Comment 8

5 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b8337e9734b0
Remove token choosing functionality from changepassword.(js|xul). r=keeler
Keywords: checkin-needed

Comment 9

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b8337e9734b0
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Assignee)

Comment 10

5 months ago
I forgot to set the addon-compat keyword before.

To expand on the bug description, it should be possible to implement equivalent functionality using the following pseudo-code:
> let tokens = nsIPK11TokenDB.listTokens()
> let tokenNames = []
> for (let token of tokens) {
>   tokenNames.push(token.tokenName)
> }
> let tokenNameObj = {}
> nsITokenDialogs.ChooseToken(tokenNames, tokenNameObj)
> nsITokenPasswordDialogs.setPassword(tokenNameObj.value)

Alternatively, implement the token chooser in the add-on itself.
Keywords: addon-compat
You need to log in before you can comment on or make changes to this bug.