632952 - Port |Bug 612699 - Sync UI: Update to simplify crypto|

Status: RESOLVED FIXED

(SeaMonkey :: Sync UI, defect)

Description

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch [Checkin: comment 20]

The idea here is to not have the user bother with coming up with a secure Sync Key (passphrase) in addition to the password. Instead the passphrase is always generated automatically and cannot be entered manually anymore (copy, save and print actions remain, though) except for the legacy case (existing data).

From bug 612699:

"* Surface a notification for existing Sync users that a new Sync Key has been
generated from their old Sync Key/passphrase. They don't need to do anything on
their other machines, but their old Sync Key/passphrase is no longer valid."

[If this has been implemented I must either have missed it or it must have been in the back-end since I couldn't find it in the patch. Related to that, a minimum passphrase length check was introduced for existing data but backed out though bug 618229, which I included.]

"* The account creation wizard won't allow you to set a custom passphrase, it
just shows a generated one."

"* The My Sync Key dialog no longer allows you to set a custom passphrase, it
just allows you to generate a new one."

[Both changes ported.]

"* Everywhere Sync Keys are shown and entered we need to aid the user with
hyphens. The hyphenation/dehyphenation should happen in Weave.Util.* helpers,
the UI code shouldn't have to know about specific Sync Key formats."

[This part has been backed out through bug 616251, and the comments on bug 617264 indicate that it won't come back for FF4 so I applied the removal.]

"Since we're ripping out custom passphrases we can also rip out the whole Sync
Key strength meter thingie."


Note: Both bug 616251 and 618229 are based on bug 602682 which I haven't ported yet (will be next). In both cases the only change that is missing is one to the definition of a local "hasKey" variable. I'll apply those changes when I port bug 602682.

Comment 1

[[e-mail]]

Comment on attachment 511150 [details] [diff] [review]
patch [Checkin: comment 20]

>+  validate: function (event) {
Where is event used?

Comment 2

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #1)
> >+  validate: function (event) {
> Where is event used?

Nowhere, actually. It was removed through bug 616251, so we remove it if you want.

Comment 3

[[e-mail]]

While trying this out for some reason I noticed that the sync key being reported, either with or without the patch, isn't the one that was originally generated on my original profile and manually typed into my other test profile; in fact it isn't even the right length and the hyphenation goes all wrong...

Comment 4

[Philipp von Weitershausen [:philikon]]

The thing you type into the easy setup screens isn't a Sync Key. It's a one-time transfer token that's used by the two devices to agree on a one-time strong key which in turn is used to encrypt all of your credentials (username, password, Sync Key) before they're exchanged.

Comment 5

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #4)
> easy setup screens

I somehow doubt Neil has the patch that ports Bug 602682 applied, since I only have it locally in a rough first version. ;-)

Comment 6

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #3)
> While trying this out for some reason I noticed that the sync key being
> reported, either with or without the patch, isn't the one that was originally
> generated on my original profile and manually typed into my other test profile;
> in fact it isn't even the right length and the hyphenation goes all wrong...

I think this relates to:

"* Surface a notification for existing Sync users that a new Sync Key has been
generated from their old Sync Key/passphrase. They don't need to do anything on
their other machines, but their old Sync Key/passphrase is no longer valid."

To which I wrote in comment 0:

"If this has been implemented I must either have missed it or it must have been
in the back-end since I couldn't find it in the patch."

IOW, I don't know what's going on there. Maybe we're in a bad situation here with porting this change so late compared to all the back-end changes having happened "long" ago. Maybe Philipp knows.

Comment 7

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #3)
> While trying this out for some reason I noticed that the sync key being
> reported, either with or without the patch, isn't the one that was originally
> generated on my original profile and manually typed into my other test profile;
> in fact it isn't even the right length and the hyphenation goes all wrong...

Seems correct. I did a quick check using an "official" SM trunk nightly (which means no extra patches like the one here applied): If you create an account and enter a custom Sync Key, the dialog that appears when you press My Sync Key in Preferences/Sync already shows a different Sync Key. Example:
* original password: 12345678
* original Sync Key: test12345678
* new Sync Key: syj6y6xg8m54izvw9g9i2rmynu (not sure whether this depends on the account name)
* hyphenated Sync Key: s-yj6y6-xg8m5-4izvw-9g9i2-rmynu (with the patch from this bug applied, or using recent FF4 pre-releases. Note that the hyphenation style has changed, too; generated Sync Key with and without this patch differ in style)
So this is already an issue, independent from the changes made in this bug, and I haven't seen any notification, but I think it's not a huge issue since we're still in beta, the changes here are removing the ability to create a custom Sync Key, and this is in line with what FF4 will do (haven't checked the add-on). The new behavior (after this bug is closed) will definitely be more consistent, while still allowing to enter a legacy Sync Key when setting up additional devices.

Comment 8

[Philipp von Weitershausen [:philikon]]

(In reply to comment #7)
> * new Sync Key: syj6y6xg8m54izvw9g9i2rmynu (not sure whether this depends on
> the account name)

It doesn't. It's 128 bits of pure randomness.

> * hyphenated Sync Key: s-yj6y6-xg8m5-4izvw-9g9i2-rmynu (with the patch from
> this bug applied, or using recent FF4 pre-releases. Note that the hyphenation
> style has changed, too; generated Sync Key with and without this patch differ
> in style)
> So this is already an issue, independent from the changes made in this bug, and
> I haven't seen any notification, but I think it's not a huge issue since we're
> still in beta, the changes here are removing the ability to create a custom
> Sync Key, and this is in line with what FF4 will do (haven't checked the
> add-on).

Add-on >=1.6 is the same as Firefox >=4.0b8.

Comment 9

[Jens Hatlak (:InvisibleSmiley)]

This contains string changes, thus should block b3.

Comment 10

[[e-mail]]

(In reply to comment #7)
> * original Sync Key: test12345678
> * new Sync Key: syj6y6xg8m54izvw9g9i2rmynu
Well, apart from my orignal sync key being 20 alphadigits and my new key 26...

But I notice that both of my clients have the (same) new 26 alphadigit key, even though one of them generated a 20 alphadigit key which I entered in to the other. So which key do I use for subsequent clients?

Comment 11

[[e-mail]]

Ah, after reading comment #0 again, I take it that clients running an old version of weave need the 20-alphadigit key, current SeaMonkey nightlies need the 26-alphadigit key. Does this patch change that to transfer tokens?

Comment 12

[Jens Hatlak (:InvisibleSmiley)]

FTR: The upgrade is done by the back-end. For SM, that has been the case from the start since our initial implementation landed after the back-end change in question. The following checkins introduced the change (landed at the same time as the bug we're porting here, Bug 612699 - Sync UI: Update to simplify crypto):

http://hg.mozilla.org/mozilla-central/rev/67138fbf6544 (bug 603489)
-> see function upgradeSyncKey
and
http://hg.mozilla.org/mozilla-central/rev/49607c89698b (bug 614737)


(In reply to comment #11)
> Does this patch change that to transfer tokens?

If you mean the three rows of input fields: No, that's bug 634419 which I'll put up for review once this one is done.

Comment 13

[[e-mail]]

(In reply to comment #12)
> FTR: The upgrade is done by the back-end. For SM, that has been the case from
> the start since our initial implementation landed after the back-end change in
> question.
I'm confused. I saved my 20 letter sync key on Dec 20...

Comment 14

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #13)
> I'm confused. I saved my 20 letter sync key on Dec 20...

I guess you saved it from the initial setup screen then (i.e. before the back-end changed it). Otherwise I have no idea.

Comment 15

[Jens Hatlak (:InvisibleSmiley)]

Neil: ping. No activity here for weeks, and this blocks b3 just like bug 634419 which depends on this one and where I haven't even dared to request reviews yet.

Comment 16

[[e-mail]]

Comment on attachment 511150 [details] [diff] [review]
patch [Checkin: comment 20]

>+          this._passphraseBox.setAttribute("readonly", "true");
[Bah, we have properties for this]

>-  validate: function () {
>+  validate: function (event) {
[event not used?]

>+             onkeyup="gSyncSetup.onPassphraseKeyUp(event);"
>+             onchange="gSyncSetup.checkFields();"/>
[I wonder why they didn't use oninput]

>+#feedback {
>+  height: 2em;
[I wonder whether we still need this or whether auto sizing suffices]

Comment 17

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #16)
> >+          this._passphraseBox.setAttribute("readonly", "true");
> [Bah, we have properties for this]

Changed to .readOnly locally.

> >-  validate: function () {
> >+  validate: function (event) {
> [event not used?]

See comment 2, removed locally.

> >+             onkeyup="gSyncSetup.onPassphraseKeyUp(event);"
> >+             onchange="gSyncSetup.checkFields();"/>
> [I wonder why they didn't use oninput]

Will be fixed in bug 634419.

> >+#feedback {
> >+  height: 2em;
> [I wonder whether we still need this or whether auto sizing suffices]

If by "auto sizing" you mean simply removing the CSS then no, since in that case the status text has not enough top/bottom margin (test case: go offline, press Change Sync Key).

What concerns me more is that the status label doesn't wrap (used DOMI to add more text to the value in the above test case). AFAIU MDC, the <label> needs to be replaced by a <description> (stupid XUL if you ask me; why can't I just add an attribute and be done with it?). Since there are many more occurrences in at least syncSetup.xul (even after bug 634419), I'll rather file a follow-up for that.

I'll wait until tomorrow for your view on my feedback re. #feedback (hehe), and then check in if nothing else pops up. Thanks so far!

Comment 18

[Philipp von Weitershausen [:philikon]]

Btw, I've been lurking on various SeaMonkey Sync UI bugs and seen that Neil is a pretty thorough reviewer. If you guys have suggestions on how to improve our upstream code, please don't hesitate to file bugs on us (Mozilla Services :: Firefox Sync: UI). Even better, if you already know how to fix and have a patch ;)

Comment 19

[[e-mail]]

(In reply to comment #17)
> What concerns me more is that the status label doesn't wrap (used DOMI to add
> more text to the value in the above test case).
Just use .textContent instead of .value (and remove any value=" " of course).

Comment 20

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 511150 [details] [diff] [review]
patch [Checkin: comment 20]

http://hg.mozilla.org/comm-central/rev/2f802948e5f2

Comment 21

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #18)
> Btw, I've been lurking on various SeaMonkey Sync UI bugs and seen that Neil is
> a pretty thorough reviewer.

Yes, a curse and a blessing. [Not meant to sound offending!]

> If you guys have suggestions on how to improve our
> upstream code, please don't hesitate to file bugs on us (Mozilla Services ::
> Firefox Sync: UI). Even better, if you already know how to fix and have a patch
> ;)

Most things here and elsewhere are just nits where it often depends heavily on the reviewer's liking/style/preferences what gets accepted and what gets rejected. Don't be bad with me but I'm generally trying to avoid such discussions wherever I can. I'll try to keep an eye on informing you of real issues, though, like the below:

(In reply to comment #19)
> (In reply to comment #17)
> > What concerns me more is that the status label doesn't wrap (used DOMI to add
> > more text to the value in the above test case).
> Just use .textContent instead of .value (and remove any value=" " of course).

Since this affects multiple files and might require some discussion, I filed bug 643640 for this.

Comment 22

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #20)
> http://hg.mozilla.org/comm-central/rev/2f802948e5f2

Bah, forgot to add this bug's number to the commit message, sorry. Should be findable through the dependencies, though.

Comment 23

[Medka]

Attachment: ahmed1.patch