Closed
Bug 790141
Opened 12 years ago
Closed 12 years ago
Implement navigator.id.get / navigator.id.getVerifiedEmail for sign-in with Persona
Categories
(Core Graveyard :: Identity, enhancement, P1)
Core Graveyard
Identity
Tracking
(blocking-basecamp:+, firefox18 fixed, firefox19 fixed, firefox20 fixed)
People
(Reporter: Mook, Assigned: jedp)
References
Details
(Keywords: feature)
Attachments
(1 file, 4 obsolete files)
14.25 KB,
patch
|
Details | Diff | Splinter Review |
The production sites using Persona (I tested developer.mozilla.org, mozillians.org, and bugzilla) all seem to be using navigator.id.getVerifiedEmail. Unfortunately, the desktop sign-in code from bug 764213 doesn't implement that, just .watch/.request, so it's... not very useful :( Asking for f? on the patch-in-progress for now, since I'm still in the progress of writing tests. (I can't figure out the code path that would let me fake the login to get .oncancel() from the request() options to get called...) Also, can't seem to find a way to undo navigator.id.watch(); will post in dev-identity about that in a bit. For now, I'm doing evil things because I can... :(
Attachment #659969 -
Flags: feedback?(mnoorenberghe+bmo)
Comment 1•12 years ago
|
||
Comment on attachment 659969 [details] [diff] [review] implement navigator.id.get/getVerifiedEmail navigator.id.getVerifiedEmail is deprecated and my understanding is that the "Observer" API (which has a partial implementation in the tree) is where things are headed. I don't think we want to support older versions of the API especially since there is still work to do on the native observer API. https://developer.mozilla.org/docs/DOM/navigator.id If you want to test out the observer API, use {dev,beta,www}.123done.org.
Attachment #659969 -
Flags: feedback?(mnoorenberghe+bmo) → feedback-
Comment 2•12 years ago
|
||
Ben, is this something you'd consider? Otherwise this is probably WONTFIX.
Component: General → Identity
Product: Firefox → Core
Yep, my code works fine with dev.123done.org; it's just not... actually useful if I can't use it in production websites. Dogfooding, etc. ;) If I were actually writing an RP, I'd probably want to be using the new APIs - but I'm just trying to log on to other RPs :) Thanks for the really quick f-! (And yes, if the decision is "old APIs are not supported, existing RPs will simply not work", then that's that; I'll just be unable to login to them until they switch. After all, I don't control those RPs.)
Comment 4•12 years ago
|
||
(In reply to :Mook from comment #3) > Thanks for the really quick f-! (And yes, if the decision is "old APIs are > not supported, existing RPs will simply not work", then that's that; I'll > just be unable to login to them until they switch. After all, I don't > control those RPs.) You can flip dom.identity.enabled back to false, the default, (and reload the tabs) so you can use those websites. It's unfortunate the shim can't still implement these two function on the navigator.id object when the native observer API is on. I'm not sure if there is anything we can do about that though.
Comment 5•12 years ago
|
||
I'd rather we not complicate things in the native implementation for now. If we want backwards compatibility, let's do it in the shim, which web sites have to include anyways since they cannot be sure that the browser supports navigator.id natively.
Assignee | ||
Comment 6•12 years ago
|
||
I think we must support navigator.id.get for b2g
Assignee | ||
Comment 7•12 years ago
|
||
The marketplace is currently implemented using navigator.id.get(); it is not yet using the observer api. Also, get() has not been deprecated for general use by sites. So I believe this is a blocker for b2g identity.
Blocks: basecamp-id
Comment 8•12 years ago
|
||
Correcting Jed's point above: in order to ensure that the payment flow works, marketplace *must* implement the Observer API. If time permits, we can also implement navigator.id.get() natively.
Comment 9•12 years ago
|
||
I thought Marketplace had switched to id.watch() in bug 780223 but I just noticed an issue with it.
Updated•12 years ago
|
Assignee: nobody → jparsons
Updated•12 years ago
|
Status: NEW → ASSIGNED
Comment 11•12 years ago
|
||
Jason: this actually matters for b2g, because if we don't do it, then legacy web sites will see a broken Persona implementation on B2G, which is a problem because that will likely break some apps. We should do this.
Comment 12•12 years ago
|
||
(In reply to Ben Adida [:benadida] from comment #11) > Jason: this actually matters for b2g, because if we don't do it, then legacy > web sites will see a broken Persona implementation on B2G, which is a > problem because that will likely break some apps. We should do this. Wait, I'm confused. the bug title references desktop though. Can you clarify?
Comment 13•12 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #12) > > Wait, I'm confused. the bug title references desktop though. Can you clarify? Oh you're right it does... that's a goof, it's just an API support bug, not a UI issue. I will rename the bug accordingly. Sorry about that.
Summary: Implement navigator.id.get / navigator.id.getVerifiedEmail in Desktop UI for sign-in with Persona → Implement navigator.id.get / navigator.id.getVerifiedEmail for sign-in with Persona
Updated•12 years ago
|
Blocks: basecamp-id
Comment 14•12 years ago
|
||
subscribers: this is required for b2g feature work see comment #11 danke bene
blocking-basecamp: --- → ?
Updated•12 years ago
|
blocking-basecamp: ? → +
Updated•12 years ago
|
Whiteboard: [feature-complete]
Assignee | ||
Comment 15•12 years ago
|
||
Implements get() and getVerifiedEmail()
Attachment #659969 -
Attachment is obsolete: true
Attachment #677827 -
Flags: review?(benadida)
Comment 16•12 years ago
|
||
Comment on attachment 677827 [details] [diff] [review] implement get() and getVerifiedEmail() Review of attachment 677827 [details] [diff] [review]: ----------------------------------------------------------------- r-, see needed changes below. ::: dom/identity/nsDOMIdentity.js @@ +115,5 @@ > > + // The only time we permit calling of request() outside of a user > + // input handler is when we are handling the (deprecated) get() or > + // getVerifiedEmail() calls. > + if (!util.isHandlingUserInput && !this.handlingInternalRequest) { is this necessary? Ahah, I see why you're doing this, more comment below. @@ +212,5 @@ > + > + // Get an assertion by using our observer api: watch + request. > + var self = this; > + this.watch({ > + onlogin: function(assertion) { there's a risk that this will be called automatically when watch() returns. I'm guessing we don't want to implement .get({silent:true}), just the .get() call that requires user interaction. So I would recommend checking a flag in this callback that invokes the callback only if both (a) nav.id.request() has been called and (b) onready() has been called.
Attachment #677827 -
Flags: review?(benadida) → review-
Assignee | ||
Comment 17•12 years ago
|
||
(In reply to Ben Adida [:benadida] from comment #16) > Comment on attachment 677827 [details] [diff] [review] > implement get() and getVerifiedEmail() > > Review of attachment 677827 [details] [diff] [review]: > ----------------------------------------------------------------- > > r-, see needed changes below. > > @@ +212,5 @@ > > + > > + // Get an assertion by using our observer api: watch + request. > > + var self = this; > > + this.watch({ > > + onlogin: function(assertion) { > > there's a risk that this will be called automatically when watch() returns. You are so right. Thank you; good catch. > I'm guessing we don't want to implement .get({silent:true}), just the .get() > call that requires user interaction. > > So I would recommend checking a flag in this callback that invokes the > callback only if both (a) nav.id.request() has been called and (b) onready() > has been called. Yes, checking onready will also ensure that I don't have a race-condition between calling watch and request, which was the reason I went down this path in the first place.
Updated•12 years ago
|
Priority: -- → P1
Comment 18•12 years ago
|
||
This is marked as a blocker for ship. However, I'm not sure if there's a user story called out that links to this feature. Josh - Is there an existing user story this bug would map to? If not, do we need a user story for this?
Flags: needinfo?(jcarpenter)
Updated•12 years ago
|
Whiteboard: [feature-complete]
Comment 19•12 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #18) > This is marked as a blocker for ship. However, I'm not sure if there's a > user story called out that links to this feature. Josh - Is there an > existing user story this bug would map to? If not, do we need a user story > for this? We might need a new user story. Briefly it looks like: - Alice adds app that uses Persona with the older API - Alice gets to use the existing identity she set up for Marketplace. That's basically it.
Comment 20•12 years ago
|
||
Gotcha. So based on your comment, maybe something like this? As a user, I should be able to login with persona on any existing site that currently makes use of persona so that I can continue to use persona on sites that may be using an older version of persona. If that looks good, I'll add that into the tracking doc as PAYANDID-028.
Flags: needinfo?(jcarpenter)
Comment 21•12 years ago
|
||
We're marking this bug with the C1 milestone since it follows the criteria of "unfinished feature work" (see https://etherpad.mozilla.org/b2g-convergence-schedule). If this work is not finished by Nov19, this bug will need an exception and will be called out at the upcoming Exec Review.
Target Milestone: --- → B2G C1 (to 19nov)
Assignee | ||
Comment 22•12 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #20) > Gotcha. So based on your comment, maybe something like this? > > As a user, I should be able to login with persona on any existing site that > currently makes use of persona so that I can continue to use persona on > sites that may be using an older version of persona. > > If that looks good, I'll add that into the tracking doc as PAYANDID-028. That sounds pretty good, though I think I'd want to change two things: - Remove the "so that" (it's not purpose or result) - Be more specific about what an older version of persona is "As a user, I should be able to use the identity I have set up for Marketplace on a persona-powered site or app that uses the older persona get() API." Hope that helps. Thanks for putting these stories together!
Comment 23•12 years ago
|
||
Thanks for fixes. I'll update the user stories doc with your updates.
Assignee | ||
Comment 24•12 years ago
|
||
Depends on the patch in Bug 804932 Implements get() and getVerifiedEmail() by using the observer api, tagging the request() message with a signature. Responses from the shim come back tagged with caller's signature. When onlogin() is called, if the signature matches the request signature, it's good. Otherwise ignore, because it could be an assertion that simply got returned by watch(). Also inhibits {silent: true}, as the persona.org shim does.
Attachment #677827 -
Attachment is obsolete: true
Attachment #681127 -
Flags: review?(benadida)
Assignee | ||
Comment 25•12 years ago
|
||
Having talked this over with Ben, I'm going to try having the internal_api in the shim signal whether an assertion was automatic or not. That should let us remove this _signature business but still keep using the observer api.
Assignee | ||
Comment 26•12 years ago
|
||
Comment on attachment 681127 [details] [diff] [review] implement get() and getVerifiedEmail() Canceling review request for this
Attachment #681127 -
Flags: review?(benadida)
Assignee | ||
Comment 27•12 years ago
|
||
Depends on https://github.com/mozilla/browserid/pull/2727 (which is currently deployed on http://notoriousb2g.personatest.org/) You can test this patch in b2g with http://people.mozilla.com/~jparsons/get.html
Attachment #681127 -
Attachment is obsolete: true
Attachment #682295 -
Flags: review?(benadida)
Comment 28•12 years ago
|
||
Comment on attachment 682295 [details] [diff] [review] implement get() and getVerifiedEmail() Review of attachment 682295 [details] [diff] [review]: ----------------------------------------------------------------- very nice, great revision.
Attachment #682295 -
Flags: review?(benadida) → review+
Assignee | ||
Comment 29•12 years ago
|
||
(In reply to Ben Adida [:benadida] from comment #28) > Comment on attachment 682295 [details] [diff] [review] > implement get() and getVerifiedEmail() > > Review of attachment 682295 [details] [diff] [review]: > ----------------------------------------------------------------- > > very nice, great revision. Thanks for your help in sorting this one out, Ben. I'm very glad we've got away from the messy signature approach. Always so helpful to talk things through with you. I'll tag this one for check-in once the patches before it have landed. They are already all r+: Bug 804932, Bug 811012, Bug 811014.
Assignee | ||
Comment 30•12 years ago
|
||
Actually, I think it's enough that 804932 is landing.
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 31•12 years ago
|
||
This doesn't apply cleanly to inbound (or m-c at this point). Please rebase (or tell me what it's supposed to be landing on top of if that's the case).
Keywords: checkin-needed
Assignee | ||
Comment 32•12 years ago
|
||
(In reply to Ryan VanderMeulen from comment #31) > This doesn't apply cleanly to inbound (or m-c at this point). Please rebase > (or tell me what it's supposed to be landing on top of if that's the case). I'll check it out and resubmit. Thanks, Ryan.
Assignee | ||
Comment 33•12 years ago
|
||
rebased; applies cleanly over m-c now
Attachment #682295 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 34•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b37a6293086
Keywords: checkin-needed
Comment 35•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0b37a6293086
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Whiteboard: [need-aurora-uplift]
Comment 36•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/6edad4e1f906 This doesn't apply cleanly to beta. Please post a branch-specific patch.
status-firefox18:
--- → affected
status-firefox19:
--- → fixed
status-firefox20:
--- → fixed
Whiteboard: [need-aurora-uplift]
Updated•12 years ago
|
Whiteboard: [needs-beta-patch]
Assignee | ||
Comment 37•12 years ago
|
||
(In reply to Ryan VanderMeulen from comment #36) > https://hg.mozilla.org/releases/mozilla-aurora/rev/6edad4e1f906 > > This doesn't apply cleanly to beta. Please post a branch-specific patch. Ryan, it looks like there's a number of previous patches that never got applied to beta. Shall I prepare a mongo patch to bring all the identity stuff in beta up-to-date with m-c?
Comment 38•12 years ago
|
||
(In reply to Jed Parsons [:jparsons] from comment #37) > Ryan, it looks like there's a number of previous patches that never got > applied to beta. Shall I prepare a mongo patch to bring all the identity > stuff in beta up-to-date with m-c? If it's just a matter of uplifting other changesets first, flattening the changesets should be avoided as it may make it harder to see what was uplifted and to dig through blame later. Just specify the ordered list of changesets to uplift or it may be easier to do it yourself with a mozilla-beta clone.
Comment 39•12 years ago
|
||
Agreed with Matt. The best route is probably to just get approval to land whatever other bugs are blocking this one.
Comment 40•11 years ago
|
||
Jed, any luck?
Assignee | ||
Comment 41•11 years ago
|
||
(In reply to Ryan VanderMeulen from comment #40) > Jed, any luck? Ryan, I haven't done it yet, no. I'm hoping to get it done tomorrow. Sorry about the delay.
Assignee | ||
Comment 42•11 years ago
|
||
(In reply to Ryan VanderMeulen from comment #39) > Agreed with Matt. The best route is probably to just get approval to land > whatever other bugs are blocking this one. Hi, Ryan, Here's the list of patches that need uplifting to beta, in the order they should be applied: First Bug 806605 (ignore IdentityUtils rej; this is ok) Then Bug 811095 Then Bug 804932 (ignore rejected file) Then Bug 811012 Then Bug 811014 Then Bug 790141 Lastly Bug 817955 can be applied on top of all these I will upload a refreshed patch to each of the bugs whose patches didn't apply cleanly. Whew!
Comment 43•11 years ago
|
||
Many of those bugs don't have approval to land either via bb+ or explicit a+. I assume you'll handle those requests?
Assignee | ||
Comment 44•11 years ago
|
||
(In reply to Ryan VanderMeulen from comment #43) > Many of those bugs don't have approval to land either via bb+ or explicit > a+. I assume you'll handle those requests? I will, yes.
Assignee | ||
Comment 45•11 years ago
|
||
(In reply to Jed Parsons [:jparsons] from comment #44) > (In reply to Ryan VanderMeulen from comment #43) > > Many of those bugs don't have approval to land either via bb+ or explicit > > a+. I assume you'll handle those requests? > > I will, yes. This has been done. I've requested mozilla-beta approval on the non bb+ bugs
Comment 46•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/df2b0a7acc9b
Whiteboard: [needs-beta-patch]
Updated•11 years ago
|
QA Contact: jsmith
Updated•5 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•