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.

Sign up for GitHub

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

Jump to bottom

Update webauthn to version 2.0 #2716

Merged
merged 1 commit into from
May 25, 2024
Merged

Update webauthn to version 2.0 #2716

merged 1 commit into from
May 25, 2024

Conversation

david-venhoff
Copy link
Member

Short description

That version contains some breaking changes, and this pr changes the affected code.
Since I haven't worked much with 2fa yet, it probably makes sense that someone with more experience reviews this.
This pr follows the upgrade guidelines from webauthn: https://github.com/duo-labs/py_webauthn/releases/tag/v2.0.0

Proposed changes

  • Add a new bytes field webauthn_id to the user model that is automatically generated by webauthn and use it instead of the numerical user id
  • Update some renamed function calls

Side effects

/

Resolved issues

Fixes: #2614

Pull Request Review Guidelines

@david-venhoff david-venhoff requested a review from a team as a code owner March 28, 2024 20:27
@david-venhoff david-venhoff force-pushed the deps/upgrade_webauthn_2_0 branch from d14ac64 to 352767a Compare March 28, 2024 20:33
@codeclimate CodeClimate
Copy link

codeclimate bot commented Mar 28, 2024 •
edited
Loading

Code Climate has analyzed commit 71937c1 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 82.2% (0.0% change).

View more on Code Climate.

@david-venhoff david-venhoff changed the title Upgrade webauthn to version 2.0 Update webauthn to version 2.0 Mar 28, 2024
deen13
deen13 reviewed Apr 16, 2024
View reviewed changes
Copy link
Contributor

@deen13 deen13 left a comment

Choose a reason for hiding this comment

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

I'm not particularly familiar with webauthn and 2FA, but hopefully, I can still add some value nonetheless! ☺️

integreat_cms/static/src/js/utils/mfa-utils.ts Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@deen13 deen13 mentioned this pull request Apr 16, 2024
Merged
@david-venhoff david-venhoff requested a review from deen13 April 25, 2024 13:37
deen13
deen13 approved these changes Apr 26, 2024
View reviewed changes
Copy link
Contributor

@deen13 deen13 left a comment

Choose a reason for hiding this comment

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

Noice 😌

@ulliholtgrave
Copy link
Member

@deen13 I did some testing with your newly introduced test suite and it seems nice to have. However, I have no real experience with ts testing and I just wanted to ask if you think that this testing framework will work in general in case we want to extend our testing scope to TS/JS as well? :)

@deen13
Copy link
Contributor

deen13 commented May 21, 2024 •
edited
Loading

I just wanted to ask if you think that this testing framework will work in general in case we want to extend our testing scope to TS/JS as well?

Hey @ulliholtgrave, yes vitest is a general purpose testing framework. It offers out-of-the-box support for typescript, esm and jsx which makes it the preferred testing solution for me. I hope that answers your question. 🙈

@david-venhoff
Copy link
Member Author

@deen13 In the issue grooming we talked about splitting up this pr into only upgrading webauthn and another one for the typescript tests. If you are also okay with that, could you create a new pr with only the test changes?
I have already created a new branch which should have the correct changes at https://github.com/digitalfabrik/integreat-cms/compare/feature/ts_tests

@david-venhoff david-venhoff force-pushed the deps/upgrade_webauthn_2_0 branch from 9571bb8 to 59ce2b2 Compare May 21, 2024 14:12
@ulliholtgrave
Copy link
Member

Perfect :) @david-venhoff Can you put your changes into one commit and this it good to go :)

@deen13 deen13 mentioned this pull request May 22, 2024
Merged
@deen13
Copy link
Contributor

deen13 commented May 22, 2024

@david-venhoff I've created #2810 from your branch :)

@david-venhoff
Update Webauthn to 2.1
71937c1 
This implements Option 2 of the webauthn migration notes
(https://github.com/duo-labs/py_webauthn/releases/tag/v2.0.0)
by adding a new field `webauthn_id` to the user model that is
automatically generated using a helper method by webauthn.
@david-venhoff david-venhoff force-pushed the deps/upgrade_webauthn_2_0 branch from 71dbf35 to 71937c1 Compare May 22, 2024 10:07
ulliholtgrave

This comment was marked as outdated.

Sign in to view

ulliholtgrave
ulliholtgrave approved these changes May 25, 2024
View reviewed changes
Copy link
Member

@ulliholtgrave ulliholtgrave left a comment

Choose a reason for hiding this comment

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

Works like a charm :) 👍

@david-venhoff david-venhoff merged commit a17bfb1 into develop May 25, 2024
5 checks passed
@david-venhoff david-venhoff deleted the deps/upgrade_webauthn_2_0 branch May 25, 2024 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ulliholtgrave ulliholtgrave ulliholtgrave approved these changes

@deen13 deen13 deen13 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update webauthn to version 2.0.0
3 participants
@david-venhoff @ulliholtgrave @deen13