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

Status: RESOLVED FIXED

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

Description

[:Cykesiopka]

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 1

[:Cykesiopka]

Attachment: Bug 1325648 - Remove token choosing functionality from changepassword.(js|xul).

The functionality is unnecessary duplication of what the Device Manager already
does, and nsITokenDialogs.ChooseToken() exists as a dedicated UI for choosing
tokens anyways.

Since we're already breaking addon compat, the changes here also update
nsITokenPasswordDialogs.setPassword() to not unnecessarily return the `canceled`
value through an outparam.

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

Comment 2

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

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".

Comment 3

[:Cykesiopka]

(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?

Comment 4

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

Yeah, option 2 is basically what I meant, but it's probably not really worth changing at this point.

Comment 5

[:Cykesiopka]

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

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

Comment 6

[:Cykesiopka]

(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.

Comment 7

[:Cykesiopka]

Thanks for the review!

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b8337e9734b0
Remove token choosing functionality from changepassword.(js|xul). r=keeler

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[:Cykesiopka]

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.