-
Notifications
You must be signed in to change notification settings - Fork 47
[Spec] Add output enum for isSecurePaymentConfirmationAvailable and rename API #285
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
[Spec] Add output enum for isSecurePaymentConfirmationAvailable and rename API #285
Conversation
@rsolomakhin @ianbjacobs - PTAL cc @pejic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this pull request. Some questions:
-
What is the relationship between unavailable-no-user-verifying-platform-authenticator and isUserVerifyingPlatformAuthenticatorAvailable() ? Could the latter be called outside of SPC (and thus moot the need for the SPC value)?
-
I think the text should explain why there is an enum. At a high level this is meant to help developers without leaking additional information. But what's not clear from the text is the developer needs or use cases driving the move to the enum.
Thanks Ian! I'll address your latter bullet shortly, but for the first:
They are identical, but it doesn't feel like it makes sense to me to have spcAvailability == 'available' but then you also have to check Getting into the weeds, I could even see it making more sense to return a set of not-available reasons, since technically you could have SPC not working for multiple reasons. But at some point we start getting into silly territory, when we started with a simple true/false boolean! |
I suggest simply calling out the relationship to avoid questions. |
Ack Ian, sounds good and I'll address that and re-ping. cc @domenic also, just because I'm curious as to whether you have any "best practice" thoughts on this sort of API. A few things I'm only musing now (whoops; we live and we learn 🤦 ): (1) should it be a set of reasons?, (2) if not, should the spec be explicit about the ordering of priority (e.g., if feature is not enabled return that, else if permission policy not set return that, else if ... ), and (3) should this be called EDIT: Edited to add (3) |
Easy answers first...
The spec should indeed be explicit about the ordering. Ideally, this would even be true if you did a set of reasons! Since it's possible to imagine web developers doing something like (An easy way to order sets is by sorting them, though.)
I like omitting the Harder question:
This seems like it depends on your use case. What code do you expect developers to write? Is this for reporting to the server, so that they can aggregate all the failures and get insight into statistical patterns? If so, a set is often useful. An example is bfcache blocking reasons. Or is this for runtime determination of what UI to show? If so, it's a bit harder to imagine how listing all the reasons would help. (But maybe it would; I'm not the domain expert!) |
Thanks Dominic! I chatted this over with our privacy reviewer, and given that there's a slight increase in fingerprinting potential by allowing multiple reasons to be listed, plus a solid increase in implementation complexity and no clear developer need yet, I opted for just making the order specific but leaving it as a single outcome for now. Thank you for your input! |
@ianbjacobs - I have addressed your comments, please take another look :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See two typo fixes, otherwise thanks!!
Co-authored-by: ianbjacobs <[email protected]>
Co-authored-by: ianbjacobs <[email protected]>
The SPC specification changed from a boolean true/false return value, to an enum of reasons. This CL updates the Chromium implementation and feature/flags to match. Note that the flag is still default-off. See w3c/secure-payment-confirmation#285 Bug: 40258712 Change-Id: I82ab9b3e287604c3935bc67a3624a226c61b10b4 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6408735 Reviewed-by: Mike Taylor <[email protected]> Reviewed-by: Kavita Soni <[email protected]> Reviewed-by: Giovanni Ortuno Urquidi <[email protected]> Commit-Queue: Stephen McGruer <[email protected]> Cr-Commit-Position: refs/heads/main@{#1448310}
Fixes #284
Preview | Diff