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

Allow WebAuthn on localhost as well #27537

Merged
merged 1 commit into from
Jun 17, 2021
Merged

Conversation

MorrisJobke
Copy link
Member

@MorrisJobke MorrisJobke added the 3. to review Waiting for reviews label Jun 17, 2021
@MorrisJobke MorrisJobke added this to the Nextcloud 22 milestone Jun 17, 2021
@MorrisJobke
Copy link
Member Author

Additionally I needed to comment the assertions in the 3rdparty module, but then I could use it to locally test login with the Chrome fake device:

diff --git a/web-auth/webauthn-lib/src/AuthenticatorAssertionResponseValidator.php b/web-auth/webauthn-lib/src/AuthenticatorAssertionResponseValidator.php
index 8400ba9c..49279cc7 100644
--- a/web-auth/webauthn-lib/src/AuthenticatorAssertionResponseValidator.php
+++ b/web-auth/webauthn-lib/src/AuthenticatorAssertionResponseValidator.php
@@ -152,7 +152,7 @@ class AuthenticatorAssertionResponseValidator
             Assertion::isArray($parsedRelyingPartyId, 'Invalid origin');
             if (!in_array($facetId, $securedRelyingPartyId, true)) {
                 $scheme = $parsedRelyingPartyId['scheme'] ?? '';
-                Assertion::eq('https', $scheme, 'Invalid scheme. HTTPS required.');
+                #Assertion::eq('https', $scheme, 'Invalid scheme. HTTPS required.');
             }
             $clientDataRpId = $parsedRelyingPartyId['host'] ?? '';
             Assertion::notEmpty($clientDataRpId, 'Invalid origin rpId.');
diff --git a/web-auth/webauthn-lib/src/AuthenticatorAttestationResponseValidator.php b/web-auth/webauthn-lib/src/AuthenticatorAttestationResponseValidator.php
index f3e5a15d..3927bf23 100644
--- a/web-auth/webauthn-lib/src/AuthenticatorAttestationResponseValidator.php
+++ b/web-auth/webauthn-lib/src/AuthenticatorAttestationResponseValidator.php
@@ -150,7 +150,7 @@ class AuthenticatorAttestationResponseValidator
 
             if (!in_array($facetId, $securedRelyingPartyId, true)) {
                 $scheme = $parsedRelyingPartyId['scheme'] ?? '';
-                Assertion::eq('https', $scheme, 'Invalid scheme. HTTPS required.');
+                #Assertion::eq('https', $scheme, 'Invalid scheme. HTTPS required.');
             }
 
             /* @see 7.1.6 */

@blizzz blizzz mentioned this pull request Jun 17, 2021
45 tasks
Copy link
Contributor

@artonge artonge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer two props isHttps and isLocalhost to make the code more explicit

@MorrisJobke
Copy link
Member Author

MorrisJobke commented Jun 17, 2021

@artonge Do you mind to adjust it, because it then needs to pass this down all the layers?

@artonge artonge force-pushed the fix/noid/webauthn-localhost branch 4 times, most recently from a2db46e to e6806ac Compare June 17, 2021 09:36
@artonge artonge self-requested a review June 17, 2021 09:48
Copy link
Contributor

@artonge artonge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made the changes but did not test. But the changes are only for testing purposes (right ?), so 🙈, lets go !

@MorrisJobke
Copy link
Member Author

I made the changes but did not test. But the changes are only for testing purposes (right ?), so 🙈, lets go !

Yes - only for testing. I will give it a try.

@MorrisJobke
Copy link
Member Author

I made the changes but did not test. But the changes are only for testing purposes (right ?), so 🙈, lets go !

There were some properties missing in some intermediate views. And also some handovers of properties to nested views. I added them. And it works here now 👍

@artonge
Copy link
Contributor

artonge commented Jun 17, 2021

There were some properties missing in some intermediate views. And also some handovers of properties to nested views. I added them. And it works here now +1

Arf, sorry, went too fast...

@MorrisJobke MorrisJobke force-pushed the fix/noid/webauthn-localhost branch 2 times, most recently from b9fcff3 to 2e13e1a Compare June 17, 2021 13:19
@artonge artonge force-pushed the fix/noid/webauthn-localhost branch from 2e13e1a to 42d20b8 Compare June 17, 2021 13:56
* browsers typically whiteliste this as well - https://developer.mozilla.org/en-US/docs/Web/API/Web_Authentication_API
* for developing purposes see https://developer.chrome.com/docs/devtools/webauthn/

Signed-off-by: Morris Jobke <[email protected]>
Signed-off-by: Louis Chemineau <[email protected]>
@artonge artonge force-pushed the fix/noid/webauthn-localhost branch from 42d20b8 to 86080e6 Compare June 17, 2021 14:05
@MorrisJobke MorrisJobke requested a review from kesselb June 17, 2021 14:23
@MorrisJobke
Copy link
Member Author

CI is happy 🎉

Copy link
Member

@Pytal Pytal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good :)

@Pytal Pytal merged commit 18ff261 into master Jun 17, 2021
@Pytal Pytal deleted the fix/noid/webauthn-localhost branch June 17, 2021 15:53
@Pytal Pytal added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jun 17, 2021
@blizzz blizzz mentioned this pull request Jun 23, 2021
39 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants