Closed Bug 1383293 Opened 3 years ago Closed 3 years ago

Add new readonly attribute principal in nsIPaymentRequest

Categories

(Core :: DOM: Web Payments, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: jonathanGB, Assigned: edenchuang)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 5 obsolete files)

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!
Summary: Add origin "principle" in payment-request-service → Add origin "principal" in payment-request-service
Blocks: 1383597
Hi Jonathan

I want to make sure what information you really need. Is "origin" enough, or you need "principal"?
Flags: needinfo?(jguillotteblouin)
Attached image origin UI
Flags: needinfo?(jguillotteblouin)
(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.
Flags: needinfo?(MattN+bmo)
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.
Flags: needinfo?(MattN+bmo)
Assignee: nobody → echuang
Status: NEW → ASSIGNED
Summary: Add origin "principal" in payment-request-service → Add new readonly attribute principal in nsIPaymentRequest
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.
The principal is the top level principal of new PaymentRequest() caller.
Attachment #8897393 - Attachment is obsolete: true
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.
Attachment #8897758 - Attachment is obsolete: true
Attachment #8899719 - Flags: review?(amarchesini)
Adding checks in ConstructorChromeScript.js to test the new readonly attribute.
Removing useless test case.
Attachment #8897394 - Attachment is obsolete: true
Attachment #8899724 - Flags: review?(amarchesini)
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.
Attachment #8899719 - Flags: feedback-
Modify the patch according to comment 12.

Change the attribute name to topLevelPrincipal.
Attachment #8899719 - Attachment is obsolete: true
Attachment #8899719 - Flags: review?(amarchesini)
Attachment #8900146 - Flags: review?(amarchesini)
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.
Attachment #8899724 - Attachment is obsolete: true
Attachment #8899724 - Flags: review?(amarchesini)
Attachment #8900148 - Flags: review?(amarchesini)
Attachment #8900146 - Flags: review?(amarchesini) → review+
Attachment #8900148 - Flags: review?(amarchesini) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8f9e0ce46ae4
https://hg.mozilla.org/mozilla-central/rev/33d4c40de9a2
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.