Closed Bug 790141 Opened 7 years ago Closed 7 years ago

Implement navigator.id.get / navigator.id.getVerifiedEmail for sign-in with Persona

Categories

(Core Graveyard :: Identity, enhancement, P1)

enhancement

Tracking

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed, firefox20 fixed)

RESOLVED FIXED
B2G C1 (to 19nov)
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed
firefox20 --- fixed

People

(Reporter: Mook, Assigned: jedp)

References

Details

(Keywords: feature)

Attachments

(1 file, 4 obsolete files)

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 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-
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.)
(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.
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.
I think we must support navigator.id.get for b2g
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
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.
I thought Marketplace had switched to id.watch() in bug 780223 but I just noticed an issue with it.
Assignee: nobody → jparsons
Status: NEW → ASSIGNED
This isn't related to b2g.
No longer blocks: basecamp-id
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.
(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?
(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
subscribers: this is required for b2g feature work see comment #11
danke 
bene
blocking-basecamp: --- → ?
blocking-basecamp: ? → +
Whiteboard: [feature-complete]
Implements get() and getVerifiedEmail()
Attachment #659969 - Attachment is obsolete: true
Attachment #677827 - Flags: review?(benadida)
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-
(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.
Keywords: feature
Priority: -- → P1
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)
Whiteboard: [feature-complete]
(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.
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)
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)
(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!
Thanks for fixes. I'll update the user stories doc with your updates.
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)
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.
Comment on attachment 681127 [details] [diff] [review]
implement get() and getVerifiedEmail()

Canceling review request for this
Attachment #681127 - Flags: review?(benadida)
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 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+
(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.
Actually, I think it's enough that 804932 is landing.
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
(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.
rebased; applies cleanly over m-c now
Attachment #682295 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/0b37a6293086
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Keywords: verifyme
QA Contact: jsmith
Whiteboard: [need-aurora-uplift]
https://hg.mozilla.org/releases/mozilla-aurora/rev/6edad4e1f906

This doesn't apply cleanly to beta. Please post a branch-specific patch.
Whiteboard: [need-aurora-uplift]
Whiteboard: [needs-beta-patch]
(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?
(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.
Agreed with Matt. The best route is probably to just get approval to land whatever other bugs are blocking this one.
Jed, any luck?
Blocks: 811528
(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.
(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!
Many of those bugs don't have approval to land either via bb+ or explicit a+. I assume you'll handle those requests?
(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.
(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
Keywords: verifyme
QA Contact: jsmith
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.