Closed
Bug 949259
Opened 11 years ago
Closed 11 years ago
Add browserid_identity.js module as a Firefox Accounts aware identity module for Firefox Sync
Categories
(Cloud Services :: Server: Firefox Accounts, defect)
Cloud Services
Server: Firefox Accounts
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla29
People
(Reporter: warner, Assigned: warner)
References
Details
(Whiteboard: [qa+])
Attachments
(2 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
ttaubert
:
review+
|
Details | Diff | Splinter Review |
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).
Updated•11 years ago
|
Whiteboard: [qa?]
Updated•11 years ago
|
Summary: add browserid_identity.js module → Add browserid_identity.js module as a Firefox Accounts aware identity module for Firefox Sync
Comment 1•11 years ago
|
||
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.
Updated•11 years ago
|
Whiteboard: [qa?] → [qa+]
Comment 2•11 years ago
|
||
Pushed a version of this to elm as fb85979614ed (so elm is up-to-date for this patch, but m-c still isn't)
Updated•11 years ago
|
Comment 3•11 years ago
|
||
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. :)
Attachment #8350472 -
Flags: review?(ttaubert)
Attachment #8350472 -
Flags: feedback?(rnewman)
Comment 4•11 years ago
|
||
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";
ಠ_ಠ
Attachment #8350472 -
Flags: review?(ttaubert) → review+
Comment 5•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fb85979614ed
https://hg.mozilla.org/mozilla-central/rev/1f24bfdc28ea
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Updated•11 years ago
|
Attachment #8350472 -
Flags: feedback?(rnewman)
Comment 6•11 years ago
|
||
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.
Status: RESOLVED → VERIFIED
Comment 7•11 years ago
|
||
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•11 years ago
|
||
(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!
You need to log in
before you can comment on or make changes to this bug.
Description
•