804932 - Pass arbitrary options from RP callers to nsDOM up to BrowserID internal api methods

Status: RESOLVED FIXED

(Core Graveyard :: Identity, enhancement)

Description

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

When an RP caller invokes a browserid method (watch, request, or logout), we should ensure that arbitrary parameters are passed all the way up to the browserid internal api calls.

Comment 1

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

Attachment: pass rp caller params from nsDOM through and up to browserid

This patch is to be applied over the patch for bug 794680.  Local xpcshell tests pass.  Small adjustments to browserid internal api methods must be made before we can do a full test.

Comment 2

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

Attachment: v2: pass rp caller params from nsDOM through and up to browserid

Fixes some bugs in object duplication and option passing up to internal_api.  Introduces new bug on logout: the dreaded JavaScript error: , line 0: Permission denied to access object

Comment 3

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

Attachment: pass rp caller params from nsDOM through and up to browserid

Comment 4

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

tbpl: https://tbpl.mozilla.org/?tree=Try&rev=c8cdfb03b442

Comment 5

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

Comment on attachment 680964 [details] [diff] [review]
pass rp caller params from nsDOM through and up to browserid

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

Ben, how does this look for passing arbitrary RP parameters up and back through the pipe?

Comment 6

[Jason Smith [:jsmith]]

I see this is linked to bug 790141 - is this required to get bug 790141 working? If so, this should probably be basecamp+.

Comment 7

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

(In reply to Jason Smith [:jsmith] from comment #6)
> I see this is linked to bug 790141 - is this required to get bug 790141
> working? If so, this should probably be basecamp+.

Yes, as I'm splitting some of that work out into this bug, that makes sense.  I don't have the superpowers to flip that switch, though.

Comment 8

[Ben Adida [:benadida]]

Comment on attachment 680964 [details] [diff] [review]
pass rp caller params from nsDOM through and up to browserid

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

I *think* that this patch includes unrelated code, specifically the _signature stuff. r-, but tell me if I got it wrong.

::: b2g/chrome/content/identity.js
@@ +54,5 @@
>   *   method:             one of 'login', 'logout', 'ready'
>   *   assertion:          optional assertion
>   */
>  function identityCall(message) {
> +  // If the calling RP context had a signature, include it in the message.

is this from a different patch? I don't see the purpose of this code for this particular feature.

Comment 9

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

(In reply to Ben Adida [:benadida] from comment #8)
> Comment on attachment 680964 [details] [diff] [review]
> pass rp caller params from nsDOM through and up to browserid
> 
> Review of attachment 680964 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I *think* that this patch includes unrelated code, specifically the
> _signature stuff. r-, but tell me if I got it wrong.
> 
> ::: b2g/chrome/content/identity.js
> @@ +54,5 @@
> >   *   method:             one of 'login', 'logout', 'ready'
> >   *   assertion:          optional assertion
> >   */
> >  function identityCall(message) {
> > +  // If the calling RP context had a signature, include it in the message.
> 
> is this from a different patch? I don't see the purpose of this code for
> this particular feature.

Quite right.  This was sloppy of me.  The signature stuff belongs in Bug 790141.

I shall fix and re-submit.

Comment 10

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

Attachment: pass rp caller params from nsDOM through and up to browserid

As per Comment #8, removes '_signature' stuff that belongs in the patch for Bug 790141

Comment 11

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

Attachment: pass rp caller params from nsDOM through and up to browserid

Sorry, previous version missed two spots that needed scrubbing.

Comment 12

[Ben Adida [:benadida]]

Comment on attachment 681508 [details] [diff] [review]
pass rp caller params from nsDOM through and up to browserid

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

great, thanks for cleaning this up!

Comment 13

[Jason Smith [:jsmith]]

The hg changeset here doesn't have the right info - you need your author info there.

Comment 14

[Jason Smith [:jsmith]]

See https://bug781534.bugzilla.mozilla.org/attachment.cgi?id=678080 for an example of what needs to included in the changeset.

Comment 15

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

adding commit details to patch ...

Comment 16

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

Attachment: pass rp caller params from nsDOM through and up to browserid

added missing fields from patch header

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0b70eb5a351

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f0b70eb5a351

Comment 19

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

Attachment: rereshed patch can be applied to beta

Patch revised so it can be cleanly applied to beta.

Comment 20

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

Even though this is closed, it should be marked bb+ for patch to be uplifted into beta

Comment 21

[Jason Smith [:jsmith]]

(In reply to Jed Parsons [:jparsons] from comment #20)
> Even though this is closed, it should be marked bb+ for patch to be uplifted
> into beta

If it isn't basecamp+ btw, you can also nominate this for approval instead to get the uplift if it's +ed.

Comment 22

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

(In reply to Jason Smith [:jsmith] from comment #21)
> (In reply to Jed Parsons [:jparsons] from comment #20)
> > Even though this is closed, it should be marked bb+ for patch to be uplifted
> > into beta
> 
> If it isn't basecamp+ btw, you can also nominate this for approval instead
> to get the uplift if it's +ed.

I figured that since these are blockers of blockers, they are effectively bb+.  I don't know what would be easier for everyone, but this seemed like the clearest route (even though it makes noise to stick a bb+ on a bunch of patches)

Comment 23

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

Comment on attachment 689298 [details] [diff] [review]
rereshed patch can be applied to beta

[Approval Request Comment]
Bug caused by (feature/regressing bug #): feature
User impact if declined: native identity will fail in some cases
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low; isolated component
String or UUID changes made by this patch: none

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1a43b1b0ac96