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

[#2076] Replace 2FA with maykin-2fa #1003

Merged
merged 3 commits into from
Feb 21, 2024
Merged

[#2076] Replace 2FA with maykin-2fa #1003

merged 3 commits into from
Feb 21, 2024

Conversation

alextreme
Copy link
Member

All in all an easy upgrade!

A few things to note:

  • Seems to clear the TOTP-devices, will every admin have to re-register their TOTP-device or is a migration path possible?
  • Wasn't able to get my yubikey to work with Webauthn yet but this may be due to my Firefox upgrade recently
  • Could use some additional texts/explaination (point to Google Authenticator / Microsoft Authenticator, explain what a Webauthn is or point to a resource etc)
  • django-admin-index happy surprise menu appears again:

2024-02-01_18-28

sergei-maertens
sergei-maertens reviewed Feb 5, 2024
View reviewed changes
requirements/base.txt Outdated Show resolved Hide resolved
requirements/base.txt Outdated Show resolved Hide resolved
requirements/base.txt Outdated Show resolved Hide resolved
@pi-sigma pi-sigma force-pushed the feature/replace-2fa branch 3 times, most recently from dae4f31 to 51fec04 Compare February 6, 2024 11:06
@pi-sigma pi-sigma marked this pull request as draft February 6, 2024 11:07
@pi-sigma pi-sigma changed the title [WIP] First attempt to replace 2FA with maykin-2fa [#2076] Replace 2FA with maykin-2fa Feb 6, 2024
@pi-sigma pi-sigma force-pushed the feature/replace-2fa branch 4 times, most recently from d2df7c9 to 562c0e2 Compare February 7, 2024 13:28
@codecov-commenter
Copy link

codecov-commenter commented Feb 8, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (651f7cd) 94.91% compared to head (2330eff) 94.92%.

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop    #1003      +/-   ##
===========================================
+ Coverage    94.91%   94.92%   +0.01%     
===========================================
  Files          882      882              
  Lines        30748    30784      +36     
===========================================
+ Hits         29183    29221      +38     
+ Misses        1565     1563       -2

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

@pi-sigma
Copy link
Contributor

pi-sigma commented Feb 8, 2024

We're running a very old version of django-admin-index (1.5.0 vs current 3.1.0), and an old version of django-ordered-model (3.4.3 vs 3.7). The upgrade is not entirely straightforward, as the styling for django-admin-index has been completely revamped. I've created a separate issue for the upgrade (here).

@pi-sigma pi-sigma force-pushed the feature/replace-2fa branch 3 times, most recently from b38e1d6 to 13de0b8 Compare February 8, 2024 10:37
@pi-sigma pi-sigma mentioned this pull request Feb 8, 2024
Merged
@pi-sigma pi-sigma marked this pull request as ready for review February 9, 2024 12:04
@pi-sigma
Copy link
Contributor

pi-sigma commented Feb 9, 2024 •
edited
Loading

@stevenbal Just a heads-up when you're trying this out. Once you've created a TOTP device for a staff/superuser, you can no longer disable 2fa via development settings for that user (while on the ticket branch). That's not expected, but it's not (yet) determined if we should treat this as a bug or just document it and go along with it (after all, this only affects development, and you can easily create and drop superusers as you like). I've created a ticket at maykin-2fa for this (link).

stevenbal
stevenbal reviewed Feb 12, 2024
View reviewed changes
Copy link
Contributor

@stevenbal stevenbal 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! I ran into some issues while testing OIDC login, but this seems to be unrelated to this PR (https://taiga.maykinmedia.nl/project/open-inwoner/issue/2101)

src/open_inwoner/cms/utils/middleware.py Show resolved Hide resolved
src/open_inwoner/configurations/tests/test_oidc.py Outdated Show resolved Hide resolved
src/open_inwoner/configurations/tests/test_oidc.py Outdated Show resolved Hide resolved
@pi-sigma pi-sigma mentioned this pull request Feb 13, 2024
Merged
5 tasks
@pi-sigma pi-sigma force-pushed the feature/replace-2fa branch 2 times, most recently from 43d9c39 to 3c620cd Compare February 15, 2024 12:59
@pi-sigma pi-sigma force-pushed the feature/replace-2fa branch 3 times, most recently from c6bada4 to 45f284f Compare February 19, 2024 11:47
stevenbal
stevenbal reviewed Feb 19, 2024
View reviewed changes
Copy link
Contributor

@stevenbal stevenbal 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, just have one question regarding CMS toolbar

src/open_inwoner/scss/admin/_admin_theme.scss Outdated
@@ -101,9 +100,13 @@ div.breadcrumbs {
#content {
/* adjusting for django-cms page */
#changelist:not([class~='cms-pagetree-root']) {
display: grid;
grid-template-columns: 1fr 360px;
display: block;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what causes this (or if it is an issue at all, because I know very little about django-cms), but if I log in as an admin, I don't get to see the CMS toolbar if I navigate to the frontend (if I try the same on develop it does show the toolbar):

image

I can only get the toolbar to show if I go to a page in the admin and then click the view button
Screenshot 2024-02-19 at 14-47-10 Open Inwoner Deventer

Copy link
Contributor

Choose a reason for hiding this comment

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

This might be related to the changes to DropToolbarMiddleware. I'll look into it.

Copy link
Contributor

Choose a reason for hiding this comment

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

The toobar seems to be disabled by installing maykin-2fa, not sure why. You can re-enable it by appending /?edit in the url bar. I tried logging out in in again and at least it seems to stick around after being re-enabled.

alextreme and others added 3 commits February 21, 2024 09:51
@alextreme @pi-sigma
[WIP] First attempt to replace 2FA with maykin-2fa
1d5e190
@pi-sigma
[#2076] Upgrade dependencies and fix tests
1e8807b 
    * upgrade mayin-2fa to 1.0.0
    * upgrade django-otp to 1.3.0
    * upgrade django-two-factor-auth to 1.16.0
@pi-sigma
[#2076] Fix admin index with 2fa
2330eff
@pi-sigma
Copy link
Contributor

  • admin is fixed
  • yubikey works
  • old TOTP devices work
  • once enabled, 2fa cannot be circumvented for dev purposes via dev settings (you need to log in and delete the TOTP device for your user)

Bartvaderkin
Bartvaderkin approved these changes Feb 21, 2024
View reviewed changes
Copy link
Contributor

@Bartvaderkin Bartvaderkin left a comment

Choose a reason for hiding this comment

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

I left a small observation not sure if it is something.

@pi-sigma
Copy link
Contributor

@Bartvaderkin Do you mean the clarification about DropToolbarMiddleware (I don't see anything else)?

@pi-sigma pi-sigma merged commit 8f11f62 into develop Feb 21, 2024
17 checks passed
@pi-sigma pi-sigma deleted the feature/replace-2fa branch February 21, 2024 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pi-sigma pi-sigma pi-sigma left review comments

@sergei-maertens sergei-maertens sergei-maertens left review comments

@Bartvaderkin Bartvaderkin Bartvaderkin approved these changes

@stevenbal stevenbal Awaiting requested review from stevenbal

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

Successfully merging this pull request may close these issues.
6 participants
@alextreme @codecov-commenter @pi-sigma @sergei-maertens @stevenbal @Bartvaderkin