PaymentHandler: Should not allow accessing PaymentManager in Extension.

PaymentHandler: Should not allow accessing PaymentManager in Extension.

The PaymentHandler API is a web standard way to delegate payment processing
to third-party web based app via payment request initiated from merchant site.
The API based on the service worker is an extension implementation of
ServiceWorkerRegistration.

On the other hand, we already allow ServiceWorker in Extension side. It means
that we also allow this feature. In terms of security, we are not sure what
will happen if we enable this feature in Extension side. Therefore, it is
correct that we should disable this feature in Extension side.

This CL is initiated from https://codereview.chromium.org/2873683002/#msg28.

BUG=661608, 721897

[zino, 2017-05-14 09:27:32]

Description was changed from

==========
PaymentHandler: Should not allow accessing PaymentManager in Extension.

This CL is initiated from https://codereview.chromium.org/2873683002/#msg28.

BUG=661608,721897
==========

to

==========
PaymentHandler: Should not allow accessing PaymentManager in Extension.

The PaymentHandler API is a web standard way to delegate payment processing
to third-party web based app via payment request initiated from merchant site.
The API based on the service worker is an extension implementation of
ServiceWorkerRegistration.

On the other hand, we already allow ServiceWorker in Extension side. It means
that we also allow this feature. In terms of security, we are not sure what
will happen if we enable this feature in Extension side. Therefore, it is
correct that we should disable this feature in Extension side.

This CL is initiated from https://codereview.chromium.org/2873683002/#msg28.

BUG=661608,721897
==========

[zino, 2017-05-14 09:28:19]

jinho.bang@samsung.com changed reviewers:
+ lazyboy@chromium.org, rouslan@chromium.org

[zino, 2017-05-14 09:28:57]

jinho.bang@samsung.com changed reviewers:
+ nasko@chromium.org

[zino, 2017-05-14 09:34:37]

PTAL

rouslan@ for payments
lazyboy@ for extensions

nasko@, I added you as reviewer in this CL because this CL is initiated from
your comment. But if you don't want to review this, I'll remove your id in
reviewer list.

Thank you!

[please use gerrit instead, 2017-05-15 14:46:47]

https://codereview.chromium.org/2880133002/diff/20001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/service_worker/payments/page.html
(right):

https://codereview.chromium.org/2880133002/diff/20001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/service_worker/payments/page.html:1:
<script src="page.js"></script>
Need a license in <!-- --> comments.

https://codereview.chromium.org/2880133002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/payments/PaymentAppServiceWorkerRegistration.cpp
(right):

https://codereview.chromium.org/2880133002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentAppServiceWorkerRegistration.cpp:50:
if (!origin.ProtocolIsInHTTPFamily()) {
Also check context->IsSecureContext().

https://codereview.chromium.org/2880133002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentAppServiceWorkerRegistration.cpp:52:
"The PaymentManger attirbute is only allowed in https://* context.");
PaymentManger can be used only in HTTPS scheme.

[zino, 2017-05-15 18:38:13]

Thank you for review!

https://codereview.chromium.org/2880133002/diff/20001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/service_worker/payments/page.html
(right):

https://codereview.chromium.org/2880133002/diff/20001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/service_worker/payments/page.html:1:
<script src="page.js"></script>
On 2017/05/15 14:46:47, ಠ_ಠ wrote:
> Need a license in <!-- --> comments.

Done.

https://codereview.chromium.org/2880133002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/payments/PaymentAppServiceWorkerRegistration.cpp
(right):

https://codereview.chromium.org/2880133002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentAppServiceWorkerRegistration.cpp:50:
if (!origin.ProtocolIsInHTTPFamily()) {
On 2017/05/15 14:46:47, ಠ_ಠ wrote:
> Also check context->IsSecureContext().

I don't think this check is needed.
because the ServiceWorkerRegistration is already enabled in SecureContext only.
On the other hand, "chrome-extension://" scheme is treated as a secure context.
So, we need to check whether this is https or not.
Instead I left a comment here. WDYT?

https://codereview.chromium.org/2880133002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentAppServiceWorkerRegistration.cpp:52:
"The PaymentManger attirbute is only allowed in https://* context.");
On 2017/05/15 14:46:47, ಠ_ಠ wrote:
> PaymentManger can be used only in HTTPS scheme.

Done.

[please use gerrit instead, 2017-05-15 18:40:16]

lgtm

[lazyboy, 2017-05-15 20:38:45]

https://codereview.chromium.org/2880133002/diff/40001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/service_worker/payments/page.js
(right):

https://codereview.chromium.org/2880133002/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/service_worker/payments/page.js:11:
}).then(function(paymentManager) {
The param is not a paymentManager, but it is an active worker, right?
I'm assuming the exception occurs because of the log on line 9, and the test
passes.
Can we instead return registration.paymentManger on line 10 and remove the
logging?

[zino, 2017-05-16 13:52:50]

On 2017/05/15 20:38:45, lazyboy wrote:
>
https://codereview.chromium.org/2880133002/diff/40001/chrome/test/data/extens...
> File chrome/test/data/extensions/api_test/service_worker/payments/page.js
> (right):
> 
>
https://codereview.chromium.org/2880133002/diff/40001/chrome/test/data/extens...
> chrome/test/data/extensions/api_test/service_worker/payments/page.js:11:
> }).then(function(paymentManager) {
> The param is not a paymentManager, but it is an active worker, right?
> I'm assuming the exception occurs because of the log on line 9, and the test
> passes.
> Can we instead return registration.paymentManger on line 10 and remove the
> logging?

My mistake :P

Done.

[lazyboy, 2017-05-16 22:28:42]

OK, getting back to the issue:
We probably shouldn't do this for extension. The original issue as I understand
is that you had concerns around StoragePartition for platform apps, but this is
extension. Extension can use service workers and shouldn't have that problem
with StoragePartition.
I think you'd need to find a way to disable this for platform apps exclusively.
You also have take chrome apps <webview> [1] into account, which can load web
content (including https) in a separate StoragePartition [2].

[1] https://developer.chrome.com/apps/tags/webview
[2] https://developer.chrome.com/apps/tags/webview#partition

[nasko, 2017-05-19 17:04:10]

On 2017/05/16 22:28:42, lazyboy wrote:
> OK, getting back to the issue:
> We probably shouldn't do this for extension. The original issue as I
understand
> is that you had concerns around StoragePartition for platform apps, but this
is
> extension. Extension can use service workers and shouldn't have that problem
> with StoragePartition.
> I think you'd need to find a way to disable this for platform apps
exclusively.
> You also have take chrome apps <webview> [1] into account, which can load web
> content (including https) in a separate StoragePartition [2].
> 
> [1] https://developer.chrome.com/apps/tags/webview
> [2] https://developer.chrome.com/apps/tags/webview#partition

In general, I would prefer if we don't restrict where features area available as
a workaround to supporting correct StoragePartition. It makes the code more
complicated than needed and doesn't really accomplish much.