949259 - Add browserid_identity.js module as a Firefox Accounts aware identity module for Firefox Sync

Status: VERIFIED FIXED

(Cloud Services :: Server: Firefox Accounts, defect)

Description

[Brian Warner [:warner :bwarner]]

Attachment: bidid.diff

We have a browserid_identity.js module, which provides a Sync "identity" object backed by a Firefox Account object (which provides a browserid assertion, which is accepted by the token server). This bug is about landing that on m-c.

We're staging all this on elm. This patch is incomplete (it has the core files, but lacks the manifest bits needed to actually build).

Comment 1

[Chris Karlof [:ckarlof]]

A version of this was initially landed on elm. This is a revamped version to land on m-c. It should not interfere with existing Firefox Sync.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Pushed a version of this to elm as fb85979614ed (so elm is up-to-date for this patch, but m-c still isn't)

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Attachment: bid_identity.patch

This is a patch containing the differences between mozilla-central and elm that relate to this bug.  It is all a little messy as stuff has been landing on elm without review, but now there's a bit of a rush to land it to central.  As a result I might have screwed the patch up in some way (eg, missed a relevant file) - if I've done that, please refer to the elm branch.

This patch should be just related to the browserid_identity module - it does *not* contain code to integrate it with sync - see bug 949695 for that.  Also note that this patch and the one in bug 949695 really aren't stand-alone - eg, the test code depends on changes made to the existing sync test code, which is in bug 949695.

I've looked at this a number of times, but I've also made some of the changes, so I can't review it myself.  It looks relatively fine for an initial landing to me.  Some of the ugly bits (eg, the syncKey getter) aren't ideal, but aren't possible to fix without more intrusive changes to Sync itself, and IMO it is fine for these to be done in a followup.

Requesting review from ttaubert here and feedback from rnewman - in bug 949695 I'll do the reverse :)  If this gets r+ with just some nits, then please feel free to land the fixes for those nits to elm as I'm not going to have the chance to do this myself this year.  That's a big ask though given there is only 1 day left of work this year, so I'll take it up in January if necessary. :)

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8350472 [details] [diff] [review]
bid_identity.patch

Review of attachment 8350472 [details] [diff] [review]:
-----------------------------------------------------------------

This seems okay for an initial landing although it has a lot of rough edges. We can fix them in follow-ups.

::: services/sync/modules/browserid_identity.js
@@ +57,5 @@
> +   * Provide override point for testing token expiration.
> +   */
> +  _now: function() {
> +    return Date.now();
> +  },

This is quite ugly, I think we should rather change _token.expiration than messing around with Date.now().

@@ +117,5 @@
> +      // bundle), but I don't like it. We should probably refactor
> +      // code that is inspecting this to not do validation on this
> +      // field directly and instead call a isSyncKeyValid() function
> +      // that we can override.
> +      return "99999999999999999999999999";

ಠ_ಠ

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/fb85979614ed
https://hg.mozilla.org/mozilla-central/rev/1f24bfdc28ea

Comment 6

[James Bonacci [:jbonacci]]

Cleaning up Resolved/Fixed bugs from December's first release.
Verified that we now have a working first-release of FxA to Desktop/Android Nightly.
Re-open as needed.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So the first patch as landed contains some unhelpful assertions for people who want to use browserid_identity.js. An exception like the following doesn't tell anything what else has to be used:

throw "account setter should be not used in BrowserIDManager";

Also I don't find any documentation of any sync related code (e.g. services and interfaces) on MDN. I need all that for bug 966434. So if anyone can help, I would appreciate that.

Comment 8

[Richard Newman [:rnewman]]

(In reply to Henrik Skupin (:whimboo) from comment #7)

> Also I don't find any documentation of any sync related code (e.g. services
> and interfaces) on MDN. I need all that for bug 966434. So if anyone can
> help, I would appreciate that.

Nobody should be using browserid_identity.js, so it's not documented, and its API is "Sync-shaped". It exists solely as a shim for the use of the ancient, creaking desktop Sync code, which similarly isn't documented, but for less-good reasons.

In the context of TPS, you probably want to read

  services/sync/modules-testing/utils.js

which has some handy methods like 'configureFxAccountIdentity'. That should be a decent crib sheet.

Hope that helps!