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

fix(deps): Bump web-auth/webauthn-lib from 3.3.9 to 4.8.5 #44761

Merged
merged 4 commits into from
Apr 16, 2024

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Apr 9, 2024

Summary

3.x is EOL and as we now bumped to 8.1 we can update to 4.x
Note that a lot of the used functions and classes are deprecated, so if we want to update for 5.x we need to rework our code.

Changes I did:

Checklist

@susnux susnux added this to the Nextcloud 30 milestone Apr 9, 2024
@susnux susnux force-pushed the fix/deps-webauthn-lib branch from 219a973 to 6740b3f Compare April 10, 2024 03:32
@susnux susnux marked this pull request as ready for review April 10, 2024 03:37
@susnux
Copy link
Contributor Author

susnux commented Apr 11, 2024

psalm does not complain here but PsrLoggerAdapter needs to be adjusted. Will do so, but that psalm ignores it is weird...

@susnux susnux force-pushed the fix/deps-webauthn-lib branch from 67e5a1d to f7efd2a Compare April 11, 2024 10:43
@come-nc
Copy link
Contributor

come-nc commented Apr 11, 2024

psalm does not complain here but PsrLoggerAdapter needs to be adjusted. Will do so, but that psalm ignores it is weird...

We’d need psalm level 3 I think

@susnux susnux requested a review from kesselb April 15, 2024 14:51
@kesselb
Copy link
Contributor

kesselb commented Apr 15, 2024

Regarding psr/log

composer installed psr/log 2 because all our dependencies does support it now.

We could also enforce psr/log 1 here and update psr/log in a follow-up pr.
For example to also adjust classes wrapping the Logger like ScopedPsrLogger.

main:

composer why psr/log
doctrine/dbal             3.8.3   requires  psr/log (^1|^2|^3)             
sabre/dav                 4.5.0   requires  psr/log (^1.0 || ^2.0 || ^3.0) 
symfony/console           v5.4.35 conflicts psr/log (>=3)                  
symfony/mailer            v5.4.22 requires  psr/log (^1|^2|^3)             
web-auth/metadata-service v3.3.9  requires  psr/log (^1.1)                 
web-auth/webauthn-lib     v3.3.9  requires  psr/log (^1.1)  

this branch:

composer why psr/log
doctrine/dbal             3.8.3   requires  psr/log (^1|^2|^3)             
sabre/dav                 4.5.0   requires  psr/log (^1.0 || ^2.0 || ^3.0) 
symfony/console           v5.4.35 conflicts psr/log (>=3)                  
symfony/mailer            v5.4.22 requires  psr/log (^1|^2|^3)             
web-auth/metadata-service 4.8.5   requires  psr/log (^1.0|^2.0|^3.0)       
web-auth/webauthn-lib     4.8.5   requires  psr/log (^1.0|^2.0|^3.0)  

@kesselb
Copy link
Contributor

kesselb commented Apr 15, 2024

Message: Webauthn\AuthenticatorSelectionCriteria::__construct(): Argument #2 ($requireResidentKey) must be of type ?bool, string given, called in /var/www/html/lib/private/Authentication/WebAuthn/Manager.php on line 109 in file '/var/www/html/apps-extra/twofactor_webauthn/vendor/web-auth/webauthn-lib/src/AuthenticatorSelectionCriteria.php' line 55

Once merged, we need to log an issue for Christoph and Richard to also update the shipped copy in twofactor_webauthn to avoid conflicts.

Copy link
Contributor

@kesselb kesselb left a comment

Choose a reason for hiding this comment

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

Nice 👍

Worked fine with my YubiKey (add, delete, passwordless login).

Copy link
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

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

It doesn't work on my machine. Do I need to do something else other than check out this branch and disable twofactor_webauthn?

The following error is thrown on the final step when saving the key after typing a name.

Message: "Call to undefined method ParagonIE\ConstantTime\Base64UrlSafe::decodeNoPadding()"
File: "/[...]/master/3rdparty/web-auth/webauthn-lib/src/PublicKeyCredentialLoader.php:84"

Although PHPStorm tells me that the method exists ... 🤔

@nickvergessen
Copy link
Member

It doesn't work on my machine. Do I need to do something else other than check out this branch and disable twofactor_webauthn?

totp app has a different version of that lib installed:
https://github.com/nextcloud/twofactor_totp/blob/0a05055ee2caab5169d0c40c3467592443739887/composer.lock#L471

If I enable totp with installed deps it also doesn't work for me anymore.
So temp fix disable totp and make sure it has an aligned version at the end of the cycle

@nickvergessen
Copy link
Member

the stream of conflicts is troublesome.
Would be better if frontend rewrites (js to ts) could be split of on the next php lib bumps.
But as you have the reviews already, can you rebase and then we get this in quickly, so we can continue bumping the remaining PHP libs?

@susnux
Copy link
Contributor Author

susnux commented Apr 16, 2024

Would be better if frontend rewrites (js to ts) could be split of on the next php lib bumps.

Yes but in this case without fixing the frontend, server would not work because the custom implementation was using invalid encoding that conflicts with the RFC and webauth-lib dropped support for anything not RFC compliant.
As this would have needed a lot of changes anyway using Typescript did not make much of a difference here.

@susnux
Copy link
Contributor Author

susnux commented Apr 16, 2024

But as you have the reviews already, can you rebase and then we get this in quickly, so we can continue bumping the remaining PHP libs?

Sure, I just wanted to wait for someone (Daniel) to test it with real hardware as I only have software keys.

@nickvergessen
Copy link
Member

nickvergessen commented Apr 16, 2024

I tested with a hardware device and passwordless authentication worked (if TOTP app is disabled)

susnux added 3 commits April 16, 2024 11:48
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
This simplifies the code a lot and fixes errors with the exisiting custom code,
where slightly different base64 values were emitted which are not valid according to the standard.

ref: web-auth/webauthn-framework#510

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
…w \Stringable as message)

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Copy link
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

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

Works when twofactor_top and twofactor_webauthn are disabled.

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux susnux force-pushed the fix/deps-webauthn-lib branch from f7efd2a to a1a74cc Compare April 16, 2024 09:52
@susnux
Copy link
Contributor Author

susnux commented Apr 16, 2024

Rebased and using merge commit ✅

@susnux susnux merged commit 7eec3b5 into master Apr 16, 2024
161 of 252 checks passed
@susnux susnux deleted the fix/deps-webauthn-lib branch April 16, 2024 10:57
Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants