w3c / payment-request

Payment Request API
https://www.w3.org/TR/payment-request-1.1/
Other
482 stars 183 forks source link

Add hasEnrolledInstrument() again #931

Open marcoscaceres opened 3 years ago

marcoscaceres commented 3 years ago

Revert "Remove hasEnrolledInstrument() (#930)"

This reverts commit f697360ed9a34fc6974117a66a6a653bf3f2ecd5.

closes #???

The following tasks have been completed:

Implementation commitment:

Optional, impact on Payment Handler spec?


Preview | Diff


Preview | Diff

dcrousso commented 2 years ago

WebKit is interested in adding something along these lines.

For Apple Pay specifically, we require that a unsigned long version is provided as part of the object data in PaymentMethodData that corresponds to the desired supported Apple Pay version. Currently, we suggest that developers use ApplePaySession.supportsVersion to check if the desired feature (more specifically the related first version to include that feature) is supported before attempting to do anything with payments (including showing UI). We'd love for there to be a standardized approach for this :)

Along these lines, would hasEnrolledInstrument just result in false or reject with an explanatory error if the value provided for object data in PaymentMethodData (or a PaymentDetailsModifier) has issues? As an example, if a developer specified a total with a negative value in its amount and then called hasEnrolledInstrument, what would happen? What about if, using the Apple Pay example above, a developer specified version: 999? I ask because it would be nice to have some way of distinguishing between the data provided being invalid (e.g. the Promise is rejected with something like "total cannot be negative") and the data provided being unsupported (e.g. the Promise resolves with false).

rsolomakhin commented 2 years ago

Hi @dcrousso! A negative total will throw an exception in PaymentRequest constructor, I believe. By the way, can the version be checked in canMakePayment() instead of hasEnrolledInstrument()? What is the advantage of hasEnrolledInstrument() over canMakePayment() for WebKit's use case, if any?

dcrousso commented 2 years ago

A negative total will throw an exception in PaymentRequest constructor, I believe.

Ah then that was a bad example. Hopefully you get the idea I was going for :)

By the way, can the version be checked in canMakePayment() instead of hasEnrolledInstrument()? What is the advantage of hasEnrolledInstrument() over canMakePayment() for WebKit's use case, if any?

That would also work! AFAICT though this is not allowed by the can make payment algorithm, as it only looks at the identifier of the paymentMethod.

rsolomakhin commented 2 years ago

Thank you for the explanation, @dcrousso . That use case makes sense. If the user agent knows the version of the payment app and the minVersion parameter to PaymentRequest is somehow standardized, then the user agent should be able to check the version of the payment app without the payment app's knowledge, thus not leaking any information to it. Is minversion the only piece of information that's necessary for your use case, or were thinking about more data?

dcrousso commented 2 years ago

Ideally there'd be a way of checking "is all of the data I've provided to the PaymentRequest (specifically the object data inside PaymentMethodData and PaymentDetailsModifier) valid?". So no, it'd be more than just version/minVersion. It'd really be the whole thing.

rsolomakhin commented 2 years ago

Are there any privacy concerns about passing an object data between origins without explicit user consent? (Probably applies more to platforms where a user can install a 3p payment app.)

dcrousso commented 2 years ago

Can you elaborate when/how that'd happen with PaymentRequest?

rsolomakhin commented 2 years ago

Can you elaborate when/how that'd happen with PaymentRequest?

  1. Install a payment handler or an Android payment app from https://payment-app.example.
  2. Visit https://shop.example.
  3. https://shop.example calls new PaymentRequest([{supportedMethods: 'https://payment-app.example', data}]).hasEnrolledInstrument().
  4. User agent silently passes data from https://shop.example to the https://payment-app.example payment app.

Passing data between origins silently is not something desirable from privacy perspective. Would you agree?

That's why my preference had been to remove the hasEnrolledInstrument() method, the "canmakepayment" event in Payment Handler API, and the IS_READY_TO_PAY intent filter from Android payment apps.

However, I can see how the use case of validating the object data is desirable. Do you have any ideas how that can be done in a privacy preserving way?

For example, the spec could define a "read-only mode" for the payment app, so it cannot write user identifying information to disk or have any network access when the user agent passes object data into it. That may be feasible for "canmakepayment" event, but I am not aware of how this can be guaranteed for native Android or IOS apps, aside from operating system support. Does iOS have this ability?

dcrousso commented 2 years ago

Ah I see. We definitely don't want data to be sent without user knowledge/approval/etc..

A "readonly" (or maybe "validation") mode would be ideal. All I'm really after is a way to perform steps 16.3-16.4 of the show() method without having to actually call show.

marcoscaceres commented 2 years ago

@dcrousso, I wonder if this is already covered by the constructor's step 4.3.6.2. As the notes states, "This step assures that any IDL type conversion errors are caught as early as possible."

When doing the conversion there, it would throw if the required unsigned long version is missing.

However, we could clarify in the constructor steps that custom validation could also be performed there.

marcoscaceres commented 2 years ago

Spun off a new issue: https://github.com/w3c/payment-request/issues/975

dcrousso commented 2 years ago

@dcrousso, I wonder if this is already covered by the constructor's step 4.3.6.2. As the notes states, "This step assures that any IDL type conversion errors are caught as early as possible."

When doing the conversion there, it would throw if the required unsigned long version is missing.

Oh wow yeah that would work! I'll this in WebKit :)

dcrousso commented 2 years ago

@marcoscaceres err, actually, reading very literally, I don't think it would as currently written. It only says that we can

Convert object to an IDL value of the type specified by the specification that defines the paymentMethod

That doesn't mention anything about actually looking at the values inside the IDL type (in this case ApplePayRequest) to see if they're valid. Based on that, I think #976 is desirable/necessary.

marcoscaceres commented 2 years ago

@dcrousso agree. Let’s go with #976. If by chance you could share a WebKit bug number, that would be really helpful (tho not required… just helps us track implementation commitment/status).

dcrousso commented 2 years ago

@marcoscaceres

If by chance you could share a WebKit bug number, that would be really helpful (tho not required… just helps us track implementation commitment/status).

I created \https://webkit.org/b/233292 ([Payment Request] Validate payment method data on construction) 🙂

marcoscaceres commented 2 years ago

Thanks @dcrousso. I've linked it from #976.