Last Comment Bug 811014 - Support for disabled primaries (forced issuer)
: Support for disabled primaries (forced issuer)
Status: RESOLVED FIXED
:
Product: Cloud Services
Classification: Client Software
Component: Server: Identity (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla19
Assigned To: Jed Parsons (use needinfo, please) [:jedp, :jparsons]
:
:
Mentors:
Depends on: 804932 811012
Blocks: basecamp-id 794634
  Show dependency treegraph
 
Reported: 2012-11-12 11:02 PST by Austin King [:ozten]
Modified: 2012-12-07 13:48 PST (History)
3 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed


Attachments
pass an 'issuer' parameter from the RP to the shim (4.17 KB, patch)
2012-11-12 23:50 PST, Jed Parsons (use needinfo, please) [:jedp, :jparsons]
benadida: feedback+
Details | Diff | Splinter Review
pass an 'issuer' parameter from the RP to the shim (4.47 KB, patch)
2012-11-13 17:02 PST, Jed Parsons (use needinfo, please) [:jedp, :jparsons]
no flags Details | Diff | Splinter Review
ensure forceIssuer parameter is passed through (4.58 KB, patch)
2012-11-17 10:09 PST, Jed Parsons (use needinfo, please) [:jedp, :jparsons]
benadida: review+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Austin King [:ozten] 2012-11-12 11:02:33 PST
We're adding an issuer  parameter to navigator.id.request in Bug#794634 Comment#9.

It is a string parameter which is the hostname. When present, the UA should not attempt discovery on the primary or persona secondary. Instead it should try to issue assertions from the specified issuer.

Task break down is a little fuzzy, but for now definitely:
1) Ensure the value flows through via postMessage API
2) Limits callers to Marketplace via whitelist
Comment 1 Austin King [:ozten] 2012-11-12 12:12:47 PST
Ben wants to remove #2.
Comment 2 Jed Parsons (use needinfo, please) [:jedp, :jparsons] 2012-11-12 23:48:43 PST
Marking this as dependent on Bug 811812 because the separate patches must be applied in order
Comment 3 Jed Parsons (use needinfo, please) [:jedp, :jparsons] 2012-11-12 23:50:00 PST
Created attachment 680966 [details] [diff] [review]
pass an 'issuer' parameter from the RP to the shim

To force the issuer, this may be sufficient as the gecko patch.
Comment 4 Jed Parsons (use needinfo, please) [:jedp, :jparsons] 2012-11-13 08:18:26 PST
To clarify my patch a little: It is just tests to confirm that the parameter is passed along.  I think Bug 804932, "pass arbitrary options." actually implements the feature; this is just to test that it works for this parameter.
Comment 5 Ben Adida [:benadida] 2012-11-13 16:27:17 PST
Comment on attachment 680966 [details] [diff] [review]
pass an 'issuer' parameter from the RP to the shim

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

love the test-focused stuff.

::: toolkit/identity/tests/unit/test_minimalidentity.js
@@ +95,5 @@
> +    run_next_test();
> +   });
> +
> +  MinimalIDService.RP.watch(mockedDoc);
> +  MinimalIDService.RP.request(mockedDoc.id, {issuer: "https://jed.gov"});

wait, you're a government entity now? I better watch my review.

::: toolkit/identity/tests/unit/test_relying_party.js
@@ +186,5 @@
> +
> +  RelyingParty.watch(mockedDoc);
> +
> +  makeObserver("identity-request", function(aSubject, aTopic, aData) {
> +    dump("teh obj is " + JSON.stringify(aSubject.wrappedJSObject) + "\n");

I suppose you should fix that typo: "teh" --> "the"
Comment 6 Jed Parsons (use needinfo, please) [:jedp, :jparsons] 2012-11-13 16:36:29 PST
(In reply to Ben Adida [:benadida] from comment #5)
> Comment on attachment 680966 [details] [diff] [review]
> pass an 'issuer' parameter from the RP to the shim
> 
> Review of attachment 680966 [details] [diff] [review]:

> I suppose you should fix that typo: "teh" --> "the"

Now we see which spelling I use more often ...
Comment 7 Jed Parsons (use needinfo, please) [:jedp, :jparsons] 2012-11-13 17:01:00 PST
(In reply to Ben Adida [:benadida] from comment #5)
> Comment on attachment 680966 [details] [diff] [review]
> pass an 'issuer' parameter from the RP to the shim
> 
> Review of attachment 680966 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> love the test-focused stuff.

Yes, the forward-compatible arbitrary params takes care of all this.  Just making sure it doesn't break in the future.

> ::: toolkit/identity/tests/unit/test_minimalidentity.js
> @@ +95,5 @@
> > +    run_next_test();
> > +   });
> > +
> > +  MinimalIDService.RP.watch(mockedDoc);
> > +  MinimalIDService.RP.request(mockedDoc.id, {issuer: "https://jed.gov"});
> 
> wait, you're a government entity now? I better watch my review.

malloc(4 * sizeof(year_t))

> ::: toolkit/identity/tests/unit/test_relying_party.js
> @@ +186,5 @@
> > +
> > +  RelyingParty.watch(mockedDoc);
> > +
> > +  makeObserver("identity-request", function(aSubject, aTopic, aData) {
> > +    dump("teh obj is " + JSON.stringify(aSubject.wrappedJSObject) + "\n");
> 
> I suppose you should fix that typo: "teh" --> "the"

The whole debug statement can go.
Comment 8 Jed Parsons (use needinfo, please) [:jedp, :jparsons] 2012-11-13 17:02:51 PST
Created attachment 681288 [details] [diff] [review]
pass an 'issuer' parameter from the RP to the shim

Addresses Ben's comments.

Checkin must wait until Bug 804932 passes review.
Comment 9 Ryan VanderMeulen [:RyanVM] 2012-11-17 05:15:22 PST
This has f+ but no r+?
Comment 10 Jed Parsons (use needinfo, please) [:jedp, :jparsons] 2012-11-17 10:07:15 PST
I'll resubmit the patch anyway; it's missing username and comment
Comment 11 Jed Parsons (use needinfo, please) [:jedp, :jparsons] 2012-11-17 10:09:55 PST
Created attachment 682779 [details] [diff] [review]
ensure forceIssuer parameter is passed through
Comment 12 Ryan VanderMeulen [:RyanVM] 2012-11-17 21:02:52 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a33aa064e10
Comment 13 Ryan VanderMeulen [:RyanVM] 2012-11-18 05:39:55 PST
https://hg.mozilla.org/mozilla-central/rev/9a33aa064e10
Comment 14 Jed Parsons (use needinfo, please) [:jedp, :jparsons] 2012-12-06 14:06:55 PST
This needs a bb+ for the patch to be uplifted to beta
Comment 15 Jed Parsons (use needinfo, please) [:jedp, :jparsons] 2012-12-06 14:27:06 PST
Comment on attachment 682779 [details] [diff] [review]
ensure forceIssuer parameter is passed through

[Approval Request Comment]
Bug caused by (feature/regressing bug #): feature; these are unit tests
User impact if declined: native identity will not be fully tested
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): none
String or UUID changes made by this patch: none
Comment 16 Jed Parsons (use needinfo, please) [:jedp, :jparsons] 2012-12-06 14:32:32 PST
(In reply to Jed Parsons [:jparsons] from comment #14)
> This needs a bb+ for the patch to be uplifted to beta

Sorry, no it doesn't.  Requested beta approval per jsmith's suggestion.
Comment 17 Ryan VanderMeulen [:RyanVM] 2012-12-07 13:48:11 PST
https://hg.mozilla.org/releases/mozilla-beta/rev/f962f111e31e

Note You need to log in before you can comment on or make changes to this bug.