Support for disabled primaries (forced issuer)

RESOLVED FIXED in Firefox 18

Status

Cloud Services
Server: Identity
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ozten, Assigned: jedp)

Tracking

unspecified
mozilla19
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox18 fixed, firefox19 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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
(Reporter)

Updated

5 years ago
Blocks: 794634
(Reporter)

Comment 1

5 years ago
Ben wants to remove #2.
Depends on: 804932
Marking this as dependent on Bug 811812 because the separate patches must be applied in order
Depends on: 811012
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.
Attachment #680966 - Flags: feedback?(benadida)
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 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"
Attachment #680966 - Flags: feedback?(benadida) → feedback+
(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 ...
(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.
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.
This has f+ but no r+?
I'll resubmit the patch anyway; it's missing username and comment
Created attachment 682779 [details] [diff] [review]
ensure forceIssuer parameter is passed through
Attachment #680966 - Attachment is obsolete: true
Attachment #681288 - Attachment is obsolete: true
Attachment #682779 - Flags: review?(benadida)

Updated

5 years ago
Attachment #682779 - Flags: review?(benadida) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a33aa064e10
Assignee: nobody → jparsons
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9a33aa064e10
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
This needs a bb+ for the patch to be uplifted to beta
Blocks: 794552
blocking-basecamp: --- → ?
blocking-basecamp: ? → ---
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
Attachment #682779 - Flags: approval-mozilla-beta?
(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.

Updated

5 years ago
Attachment #682779 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/f962f111e31e
status-firefox18: --- → fixed
status-firefox19: --- → fixed
You need to log in before you can comment on or make changes to this bug.