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

Replace 2fa solution #3844

Merged
merged 11 commits into from
Feb 7, 2024
Merged

Replace 2fa solution #3844

merged 11 commits into from
Feb 7, 2024

Conversation

sergei-maertens
Copy link
Member

@sergei-maertens sergei-maertens commented Feb 3, 2024 •
edited
Loading

Partly closes #3049
Partly closes #3695

TODO:

  • Release new version of maykin-2fa
  • Use django-hijack support of maykin-2fa

@codecov Codecov
Copy link

codecov bot commented Feb 3, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (fa9daad) 96.31% compared to head (34491b3) 96.33%.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #3844      +/-   ##
==========================================
+ Coverage   96.31%   96.33%   +0.01%     
==========================================
  Files         707      707              
  Lines       22172    22162      -10     
  Branches     2542     2540       -2     
==========================================
- Hits        21356    21349       -7     
+ Misses        569      567       -2     
+ Partials      247      246       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sergei-maertens sergei-maertens force-pushed the issue/3049-replace-2fa-library branch from fe0b03b to 9145c8a Compare February 5, 2024 13:22
@sergei-maertens sergei-maertens marked this pull request as ready for review February 5, 2024 13:22
@vaszig vaszig self-assigned this Feb 6, 2024
vaszig
vaszig approved these changes Feb 6, 2024
View reviewed changes
Copy link
Contributor

@vaszig vaszig left a comment

Choose a reason for hiding this comment

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

Tested locally as well and works as expected.

Only one question below and I searched for TWO_FACTOR_PATCH_ADMIN which is/was used in a lot of places and I am wondering if it's everywhere properly documented (for example in config.rst and in the docker-compose.yml)

src/openforms/accounts/signals.py
"""
1. Remove any dummy OTP devices for the hijacked user.
2. Restore the original OTP device for the hijacker.
Add an audit trail entry for the hijack action.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is done on purpose right?I mean we want to have the same text in both functions

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep!

maykin_2fa has the signal handling to manage the TOTP device now, so all OF needs to do is add the audit logging.

@sergei-maertens
➕ [#3049] Replace two-factor fork with maykin-2fa
ffc6e12 
Also addresses part of #3695

* Drop the maykin-django-two-factor-auth dependency - it is replaced
  with the upstream package through maykin-2fa
* Drop phonenumbers, instead phonenumberslite is enforced by maykin-2fa
@sergei-maertens
👽 [#3049] Apply installation instructions for maykin-2fa
cf36a69
@sergei-maertens
👽 [#3049] PhoneDevice was moved to a plugin, which we don't use
77a5e70
@sergei-maertens
👽 [#3049] Update tests to use built-in test utils
f47a686
@sergei-maertens
👽 [#3049] Fix hijack integration
85e044e
@sergei-maertens
💚 [#3049] Disable MFA enforced in admin tests
2e2d30a
@sergei-maertens
♻️ [#3049] Use hijack-integration from maykin-2fa
fba012f 
The signals to manage devices are implemented in maykin-2fa directly,
we only need to take care of the audit logging here.
@sergei-maertens
🔨 [#3049] Disable MFA by default in dev
d5b78f2
@sergei-maertens
🍱 [#3049] Update groups and admin index fixtures
a90bc4d 
Added the new webauthndevice model.
@sergei-maertens
🎨 [#3049] Remove/replace references to the obsoleted environment vari…
dff6ddb 
…ables
@sergei-maertens sergei-maertens force-pushed the issue/3049-replace-2fa-library branch from d664413 to dff6ddb Compare February 7, 2024 08:19
@sergei-maertens
Copy link
Member Author

Tested locally as well and works as expected.

Only one question below and I searched for TWO_FACTOR_PATCH_ADMIN which is/was used in a lot of places and I am wondering if it's everywhere properly documented (for example in config.rst and in the docker-compose.yml)

There were some more references left, so I've updated those files and the docs. Thanks for spotting this!

@sergei-maertens
💚 [#3049] Address missing coverage
34491b3 
* dev-settings coverage is not relevant
* admin-index menu check is obsolete due to the hidden navbar at the
  template level
@sergei-maertens sergei-maertens merged commit 9223c82 into master Feb 7, 2024
26 checks passed
@sergei-maertens sergei-maertens deleted the issue/3049-replace-2fa-library branch February 7, 2024 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vaszig vaszig vaszig approved these changes

Assignees

@vaszig vaszig

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve login screen and process Upgrade to Django 4.2
2 participants
@sergei-maertens @vaszig