811014 - Support for disabled primaries (forced issuer)

Status: RESOLVED FIXED

(Cloud Services :: Server: Identity, defect)

Description

[Austin King [:ozten]]

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]]

Ben wants to remove #2.

Comment 2

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

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]]

Attachment: 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]]

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]]

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]]

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

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

Attachment: 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]]

This has f+ but no r+?

Comment 10

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

I'll resubmit the patch anyway; it's missing username and comment

Comment 11

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

Attachment: ensure forceIssuer parameter is passed through

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a33aa064e10

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9a33aa064e10

Comment 14

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

This needs a bb+ for the patch to be uplifted to beta

Comment 15

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

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]]

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

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