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

Status: RESOLVED FIXED

(Core Graveyard :: Identity, enhancement, P1)

Description

[:Mook]

Attachment: implement navigator.id.get/getVerifiedEmail

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... :(

Comment 1

[Matthew N. [:MattN]]

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.

Comment 2

[Matthew N. [:MattN]]

Ben, is this something you'd consider? Otherwise this is probably WONTFIX.

Comment 3

[:Mook]

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

[Matthew N. [:MattN]]

(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

[Ben Adida [:benadida]]

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.

Comment 6

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

I think we must support navigator.id.get for b2g

Comment 7

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

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.

Comment 8

[Ben Adida [:benadida]]

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

[Kumar McMillan [:kumar]]

I thought Marketplace had switched to id.watch() in bug 780223 but I just noticed an issue with it.

Comment 10

[Jason Smith [:jsmith]]

This isn't related to b2g.

Comment 11

[Ben Adida [:benadida]]

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

[Jason Smith [:jsmith]]

(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

[Ben Adida [:benadida]]

(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.

Comment 14

[Caitlin Galimidi [:c8o]]

subscribers: this is required for b2g feature work see comment #11
danke 
bene

Comment 15

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: implement get() and getVerifiedEmail()

Implements get() and getVerifiedEmail()

Comment 16

[Ben Adida [: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.

Comment 17

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(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.

Comment 18

[Jason Smith [:jsmith]]

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?

Comment 19

[Ben Adida [:benadida]]

(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

[Jason Smith [:jsmith]]

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.

Comment 21

[Alex Keybl [:akeybl]]

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.

Comment 22

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(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

[Jason Smith [:jsmith]]

Thanks for fixes. I'll update the user stories doc with your updates.

Comment 24

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: implement get() and getVerifiedEmail()

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.

Comment 25

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

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 26

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Comment on attachment 681127 [details] [diff] [review]
implement get() and getVerifiedEmail()

Canceling review request for this

Comment 27

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: implement get() and getVerifiedEmail()

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

Comment 28

[Ben Adida [:benadida]]

Comment on attachment 682295 [details] [diff] [review]
implement get() and getVerifiedEmail()

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

very nice, great revision.

Comment 29

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(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.

Comment 30

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Actually, I think it's enough that 804932 is landing.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

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).

Comment 32

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(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.

Comment 33

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: implement get() and getVerifiedEmail()

rebased; applies cleanly over m-c now

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b37a6293086

Comment 35

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0b37a6293086

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6edad4e1f906

This doesn't apply cleanly to beta. Please post a branch-specific patch.

Comment 37

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(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

[Matthew N. [:MattN]]

(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

[Ryan VanderMeulen [:RyanVM]]

Agreed with Matt. The best route is probably to just get approval to land whatever other bugs are blocking this one.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

Jed, any luck?

Comment 41

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(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.

Comment 42

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(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

[Ryan VanderMeulen [:RyanVM]]

Many of those bugs don't have approval to land either via bb+ or explicit a+. I assume you'll handle those requests?

Comment 44

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(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.

Comment 45

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/df2b0a7acc9b