Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add hasEnrolledInstrument() again #931

Open
wants to merge 1 commit into
base: gh-pages
Choose a base branch
from

Conversation

marcoscaceres
Copy link
Member

@marcoscaceres marcoscaceres commented Oct 7, 2020

Revert "Remove hasEnrolledInstrument() (#930)"

This reverts commit f697360.

closes #???

The following tasks have been completed:

  • Confirmed there are no ReSpec errors/warnings.
  • Modified Web platform tests (link)
  • Modified MDN Docs (link)
  • Has undergone security/privacy review (link)

Implementation commitment:

  • Safari (link to issue)
  • Chrome (link to issue)
  • Firefox (link to issue)
  • Edge (public signal)

Optional, impact on Payment Handler spec?


Preview | Diff


Preview | Diff

Revert "Remove hasEnrolledInstrument() (#930)"

This reverts commit f697360.
@dcrousso
Copy link
Member

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
Copy link
Collaborator

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
Copy link
Member

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
Copy link
Collaborator

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
Copy link
Member

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
Copy link
Collaborator

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
Copy link
Member

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

@rsolomakhin
Copy link
Collaborator

rsolomakhin commented Nov 12, 2021

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
Copy link
Member

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
Copy link
Member Author

marcoscaceres commented Nov 14, 2021

@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
Copy link
Member Author

Spun off a new issue: #975

@dcrousso
Copy link
Member

@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
Copy link
Member

dcrousso commented Nov 15, 2021

@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
Copy link
Member Author

@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
Copy link
Member

@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
Copy link
Member Author

marcoscaceres commented Nov 18, 2021

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

@marcoscaceres marcoscaceres changed the title Add hasEnrolledInstrument() again (for 1.1) Add hasEnrolledInstrument() again Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants