Closed Bug 804932 Opened 7 years ago Closed 7 years ago

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

Categories

(Core Graveyard :: Identity, enhancement)

enhancement
Not set

Tracking

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

RESOLVED FIXED
mozilla19
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: jedp, Assigned: jedp)

References

Details

(Whiteboard: [LOE:M][qa-])

Attachments

(2 files, 5 obsolete files)

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.
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.
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
Attachment #674564 - Attachment is obsolete: true
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?
Attachment #680964 - Flags: review?(benadida)
I see this is linked to bug 790141 - is this required to get bug 790141 working? If so, this should probably be basecamp+.
(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.
blocking-basecamp: --- → ?
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.
Attachment #680964 - Flags: review?(benadida) → review-
(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.
As per Comment #8, removes '_signature' stuff that belongs in the patch for Bug 790141
Attachment #680964 - Attachment is obsolete: true
Attachment #681311 - Flags: review?(benadida)
Sorry, previous version missed two spots that needed scrubbing.
Attachment #681311 - Attachment is obsolete: true
Attachment #681311 - Flags: review?(benadida)
Attachment #681508 - Flags: review?(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!
Attachment #681508 - Flags: review?(benadida) → review+
The hg changeset here doesn't have the right info - you need your author info there.
Keywords: checkin-needed
See https://bug781534.bugzilla.mozilla.org/attachment.cgi?id=678080 for an example of what needs to included in the changeset.
adding commit details to patch ...
added missing fields from patch header
Attachment #681508 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/f0b70eb5a351
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Whiteboard: [LOE:M] → [LOE:M][qa-]
Patch revised so it can be cleanly applied to beta.
Blocks: basecamp-id
No longer blocks: 795552
Even though this is closed, it should be marked bb+ for patch to be uplifted into beta
(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.
(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)
blocking-basecamp: ? → ---
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
Attachment #689298 - Flags: approval-mozilla-beta?
blocking-basecamp: --- → +
Attachment #689298 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.