-
Notifications
You must be signed in to change notification settings - Fork 42
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
WebAuthn integration #22
base: master
Are you sure you want to change the base?
Conversation
HI Adam, Since BobPay is meant to be sample code for a payment app, I would rather have two versions. The current one at /pay, and one that supports authn at /pay-authn for example. It leads to code duplication, but at least people will know where to look. It shouldn't be too much work to duplicate the registration links and functions. Let me know if that's ok with you. |
@madmath How would the payment handler know to open |
Sorry if I was unclear, I meant that there would be two payment apps on bobpay.xyz. One at /pay and the other at /pay-authn, installable separately and usable separately too. |
|
||
paymentRequestClient.postMessage(paymentAppResponse); | ||
window.close(); | ||
var authn = new WebAuthnTransaction(); |
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.
Here we should feature-detect for web authentication functionality to avoid breaking the demo for those who don't have it turned on:
if (window.WebAuthnTransacttion) {
var authn = new WebAuthnTransaction();
// Web Authentication flow.
} else {
var paymentAppResponse = {
// The existing flow without Web Authentication.
}
@@ -225,6 +225,14 @@ <h2 class="section-heading">Install BobPay</h2> | |||
</p> | |||
<input id="alipay" type="checkbox" value="">AliPay</label> | |||
<p id="bobpayerror" class="error-message"></p> | |||
<br> | |||
<br> | |||
<h2 class="section-heading">Setup FIDO / WebAuthn</h2> |
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.
Let's put the WebAuthn code into a <div id="webauthn">
and show it only if Web Authentication is enabled.
<div id="webauthn" style="display: none;">
...
</div>
<script>
if (window.WebAuthnTransaction) {
document.getElementById('webauthn').style.display = 'block';
}
</script>
@madmath Do you think a single payment app would be more manageable with feature-detection for Web Authentication? |
Sorry to disappoint, I don't. The goal for BobPay is to provide the canonical sample for Payment Handler, and in its vanilla form I don't think we should include other code that conflate it with WebAuthn, and make the example less understandable. What's the problem with installing either one of two apps?
Duplicate the "pay" folder to "pay-authn" and provide two links at the bottom of BobPay.xyz so that people can choose which they want to register. |
OK, let's go with two separate apps, @madmath. |
In that case should I just maintain my fork separately? Makes maintenance easier (i.e. - merge upstream changes) and I can add you guys as contributors if you need any control. |
@apowers313 If maintaining a fork is easier for you, go for it. We don't require control. |
Two major updates:
Two major flaws to be solved:
Caveat: WebAuthn isn't currently working on my phone due to SafetyNet issues, so I haven't been able to test this out on a mobile device. Development was done with Firefox on the desktop, and the arguments passed to WebAuthn are probably wrong for Chrome. I'll update my FIDO repo with the correct WebAuthn functionality once I have a working phone.