1383293 - Add new readonly attribute principal in nsIPaymentRequest

Status: RESOLVED FIXED

(Core :: DOM: Web Payments, enhancement)

Description

[Jonathan Guillotte-Blouin [:jonathanGB]]

As discussed on #payments, it would be simpler/more robust for the RequestPayment UI to simply fetch the origin "principle" from the DOM API rather than going through layers of tabs and windows to find the information.

It would be great if that information was part of the "payment-request-service".

Thanks!

Comment 1

[Eden Chuang[:edenchuang]]

Hi Jonathan

I want to make sure what information you really need. Is "origin" enough, or you need "principal"?

Comment 2

[Jonathan Guillotte-Blouin [:jonathanGB]]

Attachment: origin UI

Comment 3

[Jonathan Guillotte-Blouin [:jonathanGB]]

(In reply to Eden Chuang[:edenchuang] from comment #1)
> Hi Jonathan
> 
> I want to make sure what information you really need. Is "origin" enough, or
> you need "principal"?

As you can see in the screenshot attached, we really only need the origin. I doubt we'll need the principal eventually, but if the cost is minimal maybe it'd be useful to just use the principal in order for it to be future-proof.

Comment 4

[Matthew N. [:MattN]]

The principal should always be preferred for security information so that's what should be passed. To be clear though, it should be the top-level origin and should be labelled as such.

Comment 5

[Eden Chuang[:edenchuang]]

Attachment: Bug 1383293 - Adding a new readonly attribute in nsIPaymentRequest. r?baku

Comment 6

[Eden Chuang[:edenchuang]]

Attachment: Bug 1383293 - Mochitest for adding a new readonly attribute in nsIPaymentRequest. r?baku

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8897393 [details] [diff] [review]
Bug 1383293 - Adding a new readonly attribute in nsIPaymentRequest. r?baku

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

From skimming the patch I don't think this is implementing what we need in the UI. See the 2nd sentence in comment 4 where I explain we need the top-level document's principal, not the principal of the document that creates the request. The test case should handle this cross-origin sub-frame approach.

Comment 8

[Eden Chuang[:edenchuang]]

Attachment: Bug 1383293 - Adding a new readonly attribute principal in nsIPaymentRequest. r?baku

The principal is the top level principal of new PaymentRequest() caller.

Comment 9

[Eden Chuang[:edenchuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d5a23f3e317e9dd4acf553c469f45e2e51d5a89

Comment 10

[Eden Chuang[:edenchuang]]

Attachment: Bug 1383293 - Adding a new readonly attribute principal in nsIPaymentRequest for UI to query top level principal information. r?baku

Saving top level principal information in nsIPaymentRequest when constructing PaymentRequest. And providing a new readonly attribute nsIPrincipal principal in nsIPaymentRequest for UI to query the top level principal information.

Comment 11

[Eden Chuang[:edenchuang]]

Attachment: Bug 1383293 - mochitest for nsIPaymentRequest new attribute. r?baku

Adding checks in ConstructorChromeScript.js to test the new readonly attribute.
Removing useless test case.

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 8899719 [details] [diff] [review]
Bug 1383293 - Adding a new readonly attribute principal in nsIPaymentRequest for UI to query top level principal information. r?baku

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

::: dom/interfaces/payments/nsIPaymentActionRequest.idl
@@ +63,5 @@
>  
>    /*
> +   *  The triggering principal
> +   */
> +  readonly attribute nsIPrincipal principal;

If this is the top-level document's principal then the name should reflect that as I said before. The comment would also be stale since IIUC the triggering principal would normally refer to the principal that triggered the dialog from the subframe.

Comment 13

[Eden Chuang[:edenchuang]]

Attachment: Bug 1383293 - Adding a new readonly attribute topLevelPrincipal in nsIPaymentRequest for UI to query top level document's principal information. r?baku

Modify the patch according to comment 12.

Change the attribute name to topLevelPrincipal.

Comment 14

[Eden Chuang[:edenchuang]]

Attachment: Bug 1383293 - Mochitest for the new readonly attribute in nsIPaymentRequest. r?baku

modify the patch according to comment 12.

1. change the attribute name to topLevelPrincipal
2. adding a new test testCrossOriginTopLevelPrincipal in test_constructor.html to testing topLevelPrincipal under cross origin situation.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f9e0ce46ae4
Add a new readonly attribute topLevelPrincipal in nsIPaymentRequest for UI support. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/33d4c40de9a2
Mochitest for testing the new readonly attribute topLevelPrincipal in nsIPaymentRequest. r=baku

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8f9e0ce46ae4
https://hg.mozilla.org/mozilla-central/rev/33d4c40de9a2