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 Pass AuthenticatorSelectionCriteria as argument to PublicKeyCredentialCreationOptions constructor #157

Merged

Conversation

sabina-talipova
Copy link
Contributor

@sabina-talipova sabina-talipova commented Sep 21, 2023

Description

Parent issue

@sabina-talipova sabina-talipova force-pushed the pulls/5.1/broken-builds-51 branch 2 times, most recently from f198153 to 4dfb2b0 Compare September 21, 2023 21:07
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

For future reference, it's a good idea to add more context in your PR description for changes like this. You only mentioned the change for PublicKeyCredentialCreationOptions, not for PublicKeyCredentialRequestOptions - and you linked to code for 4.7.x but didn't mention anything about 4.4.x which is the minimum dependency for this module.

I've investigated a little and found that for PublicKeyCredentialCreationOptions the 4000 argument wasn't being used at all (there was no parameter there in the constructor), and for PublicKeyCredentialRequestOptions we were passing 4000 in where a "challenge" parameter was and still is.

Did you do any investigation into what that 4000 number refers to? I'm hesitant to drop it entirely without understanding what it was intended to do in the first place.

@sabina-talipova sabina-talipova force-pushed the pulls/5.1/broken-builds-51 branch from 4dfb2b0 to 877e593 Compare September 21, 2023 22:55
@sabina-talipova
Copy link
Contributor Author

sabina-talipova commented Sep 21, 2023

In "web-auth/webauthn-lib" version 3.3, which we used in CMS 4.13, the second parameter is timeout. See https://github.com/web-auth/webauthn-lib/blob/v3.3/src/PublicKeyCredentialRequestOptions.php
In version 4.4, which we use in CMS 5, it accepts only one parameter at all. We invoke parent constructor. See https://github.com/web-auth/webauthn-lib/blob/4.4.x/src/PublicKeyCredentialRequestOptions.php
In version 4.7, the timeout is taken as parameter 5 in constructor. See https://github.com/web-auth/webauthn-lib/blob/4.7.x/src/PublicKeyCredentialRequestOptions.php
I plan to open a task to update our module, since many methods will be deprecated and not be supported from version 4.7. And also this module were not updated since we used "web-auth/webauthn-lib": 3.3

@sabina-talipova
Copy link
Contributor Author

Updated.

src/VerifyHandler.php Outdated Show resolved Hide resolved
src/RegisterHandler.php Show resolved Hide resolved
@sabina-talipova sabina-talipova force-pushed the pulls/5.1/broken-builds-51 branch 4 times, most recently from 1ca64f7 to af430c4 Compare September 22, 2023 02:57
@sabina-talipova
Copy link
Contributor Author

We can update them as part of this ticket.

@sabina-talipova sabina-talipova force-pushed the pulls/5.1/broken-builds-51 branch from af430c4 to 0075e3f Compare September 22, 2023 03:02
Copy link
Member

@GuySartorelli GuySartorelli 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. Sorry about the confusion.

@GuySartorelli GuySartorelli merged commit 7cb3de8 into silverstripe:5.1 Sep 22, 2023
@GuySartorelli GuySartorelli deleted the pulls/5.1/broken-builds-51 branch September 22, 2023 03:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants