Add PaymentRequest.prototype.hasEnrolledInstrument() - #833

Thanks @danyao! I’ll try to review this ASAP!

Thanks! I forgot to update the PaymentRequest interface with the new method in the first commit. This is fixed now.

Now I'm wondering if we should just add a optional dictionary to canMakePayment():

pr.canMakePayment({ withEnrolledInstruments: true });

Firefox would likely never return this information truthfully for built in methods regardless (i.e., we will always return true), so I don’t see what difference it makes from a feature detection POV?

(Note the hasEnrolledInstruments() would introduce another fingerprinting vector - so we might want to document that in the Privacy and Security section, and we should make it ok for UAs to lie for privacy reasons).

Chrome would return the hasEnrolledInstrument() truthfully for the built-in basic-card payment method. The feature detection matters for Chrome, because the old implementation of canMakePayment() was checking for instruments, whereas the new implementation would not be doing that. The only way for a website to determine which is which is to check for hasEnrolledInstrument() in the case of Chrome. Sorry about our legacy shipped code! :-(

Note the hasEnrolledInstruments() would introduce another fingerprinting vector - so we might want to document that in the Privacy and Security section.

Privacy and Security section changes sound reasonable.

We should make it ok for UAs to lie for privacy reasons.

Did you have in mind returning NotAllowedError similar to canMakePayment()? That sounds OK.

Ok, let’s go with the new name 👍 about the exceptions, see my suggestion about using the same abstract algorithm for both this an canMakePayment(). Then they are both protected by the same abuse checking.

Hi @marcoscaceres, thanks for the feedback!

However, I'd like to do this in two parts because there is some duplication in the two algorithms that I'd like to avoid (mostly an editorial/maintenance issue).

Consolidating redundant algorithm makes sense. I went the other way initially because I felt it may be confusing to embed an "if" and the rate-limit note inside the loop. I gave it another try. PTAL.

Also note that the hasEnrolledInstrument() doesn't seem to integrate with the state machine like the other methods.

For example, right now:

pr = new PaymentRequest("")
await pr.abort();
await pr.canMakePayment(); //throws
await pr.hasEnrolledInstrument(); // doesn't throw.

This would be a bug. How did you test it? I added a manual WPT test case for this scenario and it was passing on my debug build, but I could have overlooked something: https://github.com/web-platform-tests/wpt/blob/master/payment-request/payment-request-hasenrolledinstrument-method-manual.https.html#L81

(Note the hasEnrolledInstruments() would introduce another fingerprinting vector - so we might want to document that in the Privacy and Security section, and we should make it ok for UAs to lie for privacy reasons).

Updated the Privacy Considerations section. PTAL.

Do you think it's important to keep the abuse protection for canMakePayment()? I feel it's no longer necessary because with the introduction of JIT and removal of handler-specific can make payment check, the new canMakePayment() is mostly an UA version detector. hasEnrolledInstrument() on the other hand still exposes user information, so should be kept under abuse protection.

Friendly ping @marcoscaceres. I've responded to your feedback. PTAL.

Firefox bug https://bugzilla.mozilla.org/show_bug.cgi?id=1528663

Friendly ping, @aestes.

Hi @marcoscaceres,

Now that this PR is approved, what would be the best next step? Should we create a 1.1 branch to commit this?

@plehegar do you have guidance here for introducing a new branch?

Ian

@danyao @plehegar @ianbjacobs, my preference is for us not to spin up new branches as we are almost ready to republish in the next 2 weeks.

@danyao, just hang tight, we are almost done with 1.0. Once the new CR is out, we don't expect any significant feedback - so it will be easier to deal with back porting, or uplifting, changes as needed (and all this stuff will go into the gh-pages branch).

If we can get commitment from one more implementer we can land this.

@danyao, @rsolomakhin, @zouhir, @aestes see above request from @marcoscaceres. :)

@ianbjacobs : This is shipped in Chrome and there's a bug in WebKit. Is that sufficient?

@rsolomakhin, excellent! thanks for doing the archeology! Yep, that's good enough.

That sounds like 2 implementation commitments. @marcoscaceres, seem ok?

@danyao could you please check the merge conflict? I can modernize the markup a bit after we merge.

Hi @marcoscaceres - I fixed the merge conflicts. PTAL!

Thanks @danyao! looks great.