623795 - Sync setup and Add a Device wizard should trigger master password dialog and bail if that's cancelled

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Richard Newman [:rnewman]]

Its inability to apply common sense leads to Sync being "set up", yet having no credentials after restart. That's no good.

See also Bug 620992.

Comment 1

[Philipp von Weitershausen [:philikon]]

The wizard prompts for the MP at the end, so this is only a problem if you cancel it.

Comment 3

[Richard Newman [:rnewman]]

Looking into this.

Comment 4

[Philipp von Weitershausen [:philikon]]

I think we should prompt for the MP at the very beginning of the wizard and if the MP dialog is cancelled, we just should bail out of the wizard altogether. This also goes for the Add a Device wizard.

There are several good reasons for this, the most important ones of which are:

* Going through a multiple step wizard and THEN being told "owait we can't let you do this if you don't unlock your master password" sucks

* We already create the user account before the last step, so the acocunt would've been created but the credentials wouldn't have been stored locally. FAIL.

This also applies to the Add a Device wizard.

Comment 5

[:Atoll]

Note that I use a master password re-locking plugin that locks MP a certain number of seconds after it's unlocked, so you'll still need to make sure MP is unlocked at various places during setup.

Comment 6

[Richard Newman [:rnewman]]

Attachment: Util for unlocking MP. v1

First half. This adds a function to Weave.Utils to unlock the master password, returning true if it's unlocked (or doesn't need it).

No automated tests for this: we'd only be able to test the uninteresting part.

Comment 7

[Richard Newman [:rnewman]]

Attachment: m-c UI/wizard changes. v1

Second half. This adds appropriate unlock calls to:

* Each button in the setup wizard
* onWizardAdvance, to catch re-locks (though I haven't tested timed re-locks; I don't think that should be an explicitly supported case, but this addition was easy enough)
* The "Add a Device" link in Sync Prefs, which ensures that the J-PAKE sender can actually access its credentials!

I tried to put appropriate User annotations into the patch, Philipp; I don't think my L3 access has been assigned yet, so you'll be on the hook for committing this. Sorry.

Comment 8

[Richard Newman [:rnewman]]

Note that these changes don't affect the add-on UI yet. Coming soon.

Comment 9

[Richard Newman [:rnewman]]

Attachment: Sync UI. v1

Comment 10

[Richard Newman [:rnewman]]

Attachment: Util for unlocking MP. v2

This needs proper review.

Comment 11

[Richard Newman [:rnewman]]

Attachment: m-c UI/wizard changes. v2

Renaming method.

Comment 12

[Philipp von Weitershausen [:philikon]]

Comment on attachment 502862 [details] [diff] [review]
Util for unlocking MP. v2

This is now a blocker, dolske :). Could you review or suggest somebody else who could?

Comment 13

[Justin Dolske [:Dolske]]

Comment on attachment 502862 [details] [diff] [review]
Util for unlocking MP. v2

>--- a/services/sync/modules/util.js
...
>+  ensureMPUnlocked: function ensureMPUnlocked(force) {
>+    let d = Cc["@mozilla.org/security/pk11tokendb;1"].getService(Ci.nsIPK11TokenDB);

I'd ideally like to just use a Login Manager interface here (since the end goal is to store the Sync credential there), but since you want to support this code on Firefox 3.6 nsILoginManagerCrypto isn't available there... So let's not worry too much about that for now. :)

Why is |force| an argument here? It's not used.

>+        if (!token.needsLogin())
>+          return true;
>+        token.login(force);

I'm a little wary of this; nothing else seems to call this particular API, and experience from bug 437904 does not give me the greatest confidence in this code. :/

I'd just just using nsISecretDecoderRing, and try encrypting a test string. Either it will work (no MP, or user entered it), or it will throw (user canceled MP entry or something is terribly broken).

eg

  ensureMPUnlocked() {
    sdr = Cc["@mozilla.org/security/sdr;1"].getService(Ci.nsISecretDecoderRing);
    var ok = false;
    try {
        sdr.encryptString("bacon");
        ok = true;
    } catch(e) { }
    return ok;
  }

Comment 14

[Richard Newman [:rnewman]]

(In reply to comment #13)

> I'd just just using nsISecretDecoderRing, and try encrypting a test string.
> Either it will work (no MP, or user entered it), or it will throw (user
> canceled MP entry or something is terribly broken).

That's what I like to see :D

Thanks dolske! Will resubmit using ISDR.

Comment 15

[Richard Newman [:rnewman]]

Attachment: fx-sync part. v2

This version:

* Incorporates dolske's function in its entirety, so the separate patch is gone. Works on 3.5.16.

* Also fixes Bug 620613 by refusing to open the sync key dialog unless the MP is unlocked.

I'm building and testing on m-c now.

Comment 16

[Richard Newman [:rnewman]]

Attachment: m-c UI/wizard changes. v3

Fixes Bug 620613, tested with dolske's new function.

Comment 17

[Philipp von Weitershausen [:philikon]]

Comment on attachment 503376 [details] [diff] [review]
fx-sync part. v2

>   onWizardAdvance: function () {
>+    // Check pageIndex so we don't prompt before the Sync setup wizard appears.
>+    // This is a fallback in case the Master Password gets locked mid-wizard.
>+    if ((this.wizard.pageIndex >= 0) &&
>+        !Weave.Utils.ensureMPUnlocked()) {
>+      
>+      // Leave canAdvance set according to onPageShow, because it dictates
>+      // whether the Next button stays enabled. Return false to prevent the
>+      // wizard from advancing to the next page.
>+      this.onPageShow();
>+      return false;

Why is onPageShow() needed here? onPageShow() effectively resets the page's state to what it was when you first hit it. I don't think that's the behaviour we want when we refuse to advance. Now fortunately, for some pages it calls checkFields() so it may magically work. That doesn't mean it's correct.

Comment 18

[Philipp von Weitershausen [:philikon]]

Comment on attachment 503388 [details] [diff] [review]
m-c UI/wizard changes. v3

Same question regarding onPageShow()

Comment 19

[Richard Newman [:rnewman]]

(In reply to comment #17)
> Why is onPageShow() needed here?

It's not. Removed!

Pushed in two parts:

Utils:

http://hg.mozilla.org/services/fx-sync/rev/8c27e8eacc65

UI:

http://hg.mozilla.org/services/fx-sync/rev/e7aaaf3d7dae


m-c next.

Comment 20

[Philipp von Weitershausen [:philikon]]

Landed m-c bits: http://hg.mozilla.org/mozilla-central/rev/c5f1cf237550

Comment 21

[Philipp von Weitershausen [:philikon]]

Of course I forgot to apply the review comments: http://hg.mozilla.org/mozilla-central/rev/9f412256da4c

Comment 22

[[e-mail]]

(In reply to comment #13)
>>+        if (!token.needsLogin())
>>+          return true;
>>+        token.login(force);
>I'm a little wary of this; nothing else seems to call this particular API, and
>experience from bug 437904 does not give me the greatest confidence in this
>code. :/
Although to be fair that was logging out, not logging in.

Comment 24

[Tracy Walker [:tracy]]

removing in-litmus flag, it no longer exists