Drop django-two-factor-auth and migrate 2FA pages to new frontend #3520

Merged
hansegucker merged 15 commits from drop-django-two-factor-auth into master 2025-06-29 13:48:20 +02:00
Owner

grafik

![grafik](/uploads/ea91855d9c349a2e64979311c388a8c9/grafik.png)
Author
Owner

added 1 commit

  • d3ac37b2 - Add support for allauth recovery codes to frontend

Compare with previous version

added 1 commit <ul><li>d3ac37b2 - Add support for allauth recovery codes to frontend</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/1875/diffs?diff_id=64592&start_sha=7d81ae2f1fb821798c5fdde1be5c81d38d06e5db)
Author
Owner

mentioned in issue #565

mentioned in issue #565
Author
Owner

mentioned in issue #1160

mentioned in issue #1160
Author
Owner

mentioned in issue #1207

mentioned in issue #1207
Author
Owner

mentioned in issue #438

mentioned in issue #438
Author
Owner

changed the description

changed the description
Author
Owner

added 1 commit

  • 73b3b0d5 - Add support for allauth webauthn to frontend

Compare with previous version

added 1 commit <ul><li>73b3b0d5 - Add support for allauth webauthn to frontend</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/1875/diffs?diff_id=65175&start_sha=d3ac37b27e48f259b0d5276880e95b8d46c7d467)
Author
Owner

added 1 commit

  • c5562eb1 - Drop old 2fa frontend

Compare with previous version

added 1 commit <ul><li>c5562eb1 - Drop old 2fa frontend</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/1875/diffs?diff_id=65177&start_sha=73b3b0d50e27dc4c9ec8af4923bc56b04ab23e8e)
Author
Owner

requested review from @ZugBahnHof

requested review from @ZugBahnHof
Author
Owner

added 2 commits

  • 2ffe310f - Refactor new 2fa page
  • 8a11cd76 - Update docs for 2fa

Compare with previous version

added 2 commits <ul><li>2ffe310f - Refactor new 2fa page</li><li>8a11cd76 - Update docs for 2fa</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/1875/diffs?diff_id=65228&start_sha=c5562eb1267b1d1adfab1d000365a66c3e18a9c1)
Author
Owner

marked this merge request as ready

marked this merge request as **ready**
Author
Owner

Migration path?

Migration path?
Owner
        sociallogin = (kwargs.get("signal_kwargs", None) or {}).get("sociallogin")

No need to instantiate two dicts.

```suggestion:-0+0 sociallogin = (kwargs.get("signal_kwargs", None) or {}).get("sociallogin") ``` No need to instantiate two dicts.
Owner

I don't think this helps.

I don't think this helps.
Author
Owner

I agree ;-)

I agree ;-)
Member

I thought we actually use named routes instead of hardcoding paths so the route can change without needing to update all code locations?

I thought we actually use named routes instead of hardcoding paths so the route can change without needing to update all code locations?
Member

undeclared event

undeclared event
Member

undeclared event

undeclared event
Member

why does a dialog action trigger a redirect?

why does a dialog action trigger a redirect?
Member

why change this?

why change this?
Member

what are all of these django templates for?

what are all of these django templates for?
Member

requested changes

requested changes
Author
Owner

Yeah, that's a named route. I just changed the name for consistency with the other names.

Yeah, that's a named route. I just changed the name for consistency with the other names.
Author
Owner

Ah, should be probably this.dialog = false instead.

Ah, should be probably `this.dialog = false` instead.
Author
Owner

Because that's django-two-factor-auth's URL scheme and not django-allauth's URL scheme.

Because that's django-two-factor-auth's URL scheme and not django-allauth's URL scheme.
Author
Owner

To make django-allauth showing things in Materialize ...

To make django-allauth showing things in Materialize ...
Author
Owner

Reminds me: Drop old login templates

Reminds me: Drop old login templates
Member

Recovery code status is shown as "(not) activated", should probably instead be "(not) generated". Also, the regeneration is a destructive (→ dangerous) action, button should be red.

Recovery code status is shown as "(not) activated", should probably instead be "(not) generated". Also, the regeneration is a destructive (→ dangerous) action, button should be red.
Member

don't like this icon here

don't like this icon here
Member

Due to only one 2fa authenticator app at a time being supported (afaik), the button to add/remove it implicitly tells the status, so the chip is unnecessary.
Maybe this can be combined with the button? Idea: clicking the chip does the same as clicking the button

Due to only one 2fa authenticator app at a time being supported (afaik), the button to add/remove it implicitly tells the status, so the chip is unnecessary. Maybe this can be combined with the button? Idea: clicking the chip does the same as clicking the button
Author
Owner

I like a clear, visible, and consistent indicator for each method, so I don't think the chip is unnecessary.

I like a clear, visible, and consistent indicator for each method, so I don't think the chip is unnecessary.
Author
Owner

Do you have a better idea?

Do you have a better idea?
Author
Owner

Same argument with the consistency → recovery codes are a 2fa method and it can be activated or not

Same argument with the consistency → recovery codes are a 2fa method and it can be activated or not
Member

Security keys cannot be deactivated/disabled but only added/removed, so the chip should either say "{None|One|n} keys added" or maybe even contain a button like this:

image

Security keys cannot be deactivated/disabled but only added/removed, so the chip should either say "{None|One|n} keys added" or maybe even contain a button like this: ![image](/uploads/6f1a0eb3426ed267dc23eae17849edb5/image.png)
Member

hm, ok i guess

hm, ok i guess
Member

maybe a store update is required as well

maybe a store update is required as well
Member

but can't our frontend be backend-2fa-lib-agnostic?

but can't our frontend be backend-2fa-lib-agnostic?
Member

they cannot be not activated tho? If they exist they are automatically active, right?

they cannot be not activated tho? If they exist they are automatically active, right?
Member

May just the trashcan or something like cellphone-remove?

May just the trashcan or something like cellphone-remove?
Member

I like the idea of a clear indicator per method, but I think right now they

  • can be confusing (e.g. if they imply something can be deactivated)
  • duplicate info (which is not a bad thing per se)
  • tell the user an action should be performed, but don't allow this directly (a big red chip tells users that something is not ideal, which is correct, but the action-trigger to fix this is at the other end of these cards)
I like the idea of a clear indicator per method, but I think right now they - can be confusing (e.g. if they imply something can be deactivated) - duplicate info (which is not a bad thing per se) - tell the user an action should be performed, but don't allow this directly (a big red chip tells users that something is not ideal, which is correct, but the action-trigger to fix this is at the other end of these cards)
Author
Owner

No, the URL scheme is not a frontend decision. This URL had to change anyway because the account scheme is not used.

No, the URL scheme is not a frontend decision. This URL had to change anyway because the `account` scheme is not used.
Author
Owner

Yeah, so they are activated.

Yeah, so they are activated.
Member

change this icon

change this icon
Member

image

@hansegucker an idea for this page, what do you think?

![image](/uploads/604284a4a476103b7d19350fc5eb18e0/image.png) @hansegucker an idea for this page, what do you think?
Author
Owner

It's missing all the consistency I liked ...

It's missing all the consistency I liked ...
Member

I see your point, maybe we can discuss/find a compromise? (maybe in the devmeeting or a call or sth?)

I see your point, maybe we can discuss/find a compromise? (maybe in the devmeeting or a call or sth?)
Member

yes, but they will never be "not activated". either they exist or they don't. Simplify change the wording…

yes, but they will never be "not activated". either they exist or they don't. Simplify change the wording…
Author
Owner

added 1 commit

  • cb01ffb5 - Drop old templates

Compare with previous version

added 1 commit <ul><li>cb01ffb5 - Drop old templates</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/1875/diffs?diff_id=65285&start_sha=8a11cd76bcd33112447fd427cb5906eaeaa89b50)
Author
Owner

changed this line in version 7 of the diff

changed this line in [version 7 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/1875/diffs?diff_id=65335&start_sha=cb01ffb5f67aa786fed77e9820573f6bfb3420d7#a6a964843a748ae0ba2f0c303eba963855e834f4_56_56)
Author
Owner

changed this line in version 7 of the diff

changed this line in [version 7 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/1875/diffs?diff_id=65335&start_sha=cb01ffb5f67aa786fed77e9820573f6bfb3420d7#017dc1edadeaf4db34f409c9aba47cf569c70c36_23_16)
Author
Owner

changed this line in version 7 of the diff

changed this line in [version 7 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/1875/diffs?diff_id=65335&start_sha=cb01ffb5f67aa786fed77e9820573f6bfb3420d7#c9f85db2c06d6ba0ecacc062a167a3517a01faf4_29_29)
Author
Owner

changed this line in version 7 of the diff

changed this line in [version 7 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/1875/diffs?diff_id=65335&start_sha=cb01ffb5f67aa786fed77e9820573f6bfb3420d7#f7c8452b629343b9626741a05b8038589a42d24c_4_4)
Author
Owner

added 1 commit

  • 950e06ae - Fix things from review

Compare with previous version

added 1 commit <ul><li>950e06ae - Fix things from review</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/1875/diffs?diff_id=65335&start_sha=cb01ffb5f67aa786fed77e9820573f6bfb3420d7)
Author
Owner

@ZugBahnHof grafik

@ZugBahnHof ![grafik](/uploads/169c28d04595e205d74e67c759a8fc83/grafik.png)
Author
Owner

added 1 commit

  • 9b6141a5 - Optimize frontend for 2FA

Compare with previous version

added 1 commit <ul><li>9b6141a5 - Optimize frontend for 2FA</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/1875/diffs?diff_id=65472&start_sha=950e06ae875440c9e9a6ab4634b0072d5003fc7e)
Member

For the codes please add smth like "codes are created in batches of 10", and maybe change the chip text to "... codes remaining"?

For the codes please add smth like "codes are created in batches of 10", and maybe change the chip text to "... codes remaining"?
Author
Owner

changed this line in version 9 of the diff

changed this line in [version 9 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/1875/diffs?diff_id=65613&start_sha=9b6141a55f5247130032558e4601326f905c1f6d#46c0677a276dcd2bb7f0a286a3722d9600e6a7cf_11_0)
Author
Owner

added 4 commits

  • 9acf90f9 - fixup! First steps in getting rid of django-two-factor-auth and using django-allauth[mfa]
  • dea1a191 - Fix messages for 2FA
  • 60f6159e - Update changelog
  • d8986ad7 - Finally drop 2FA

Compare with previous version

added 4 commits <ul><li>9acf90f9 - fixup! First steps in getting rid of django-two-factor-auth and using django-allauth[mfa]</li><li>dea1a191 - Fix messages for 2FA</li><li>60f6159e - Update changelog</li><li>d8986ad7 - Finally drop 2FA</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/1875/diffs?diff_id=65613&start_sha=9b6141a55f5247130032558e4601326f905c1f6d)
Author
Owner

marked this merge request as draft from 9acf90f96a9505e19ce93a5692f1c7670ef5eeda

marked this merge request as **draft** from 9acf90f96a9505e19ce93a5692f1c7670ef5eeda
Author
Owner

added 55 commits

  • d8986ad7...0b048115 - 40 commits from branch master
  • 0b048115...8ccaa9e4 - 5 earlier commits
  • f0eb529d - Add support for allauth webauthn to frontend
  • 529d5354 - Drop old 2fa frontend
  • aaaad844 - Refactor new 2fa page
  • 37a3471f - Update docs for 2fa
  • 0525ca40 - Drop old templates
  • 01fddce4 - Fix things from review
  • 17fe06f0 - Optimize frontend for 2FA
  • fa602e22 - Fix messages for 2FA
  • a9e9b8db - Update changelog
  • 47c174b3 - Finally drop 2FA

Compare with previous version

added 55 commits <ul><li>d8986ad7...0b048115 - 40 commits from branch <code>master</code></li><li>0b048115...8ccaa9e4 - 5 earlier commits</li><li>f0eb529d - Add support for allauth webauthn to frontend</li><li>529d5354 - Drop old 2fa frontend</li><li>aaaad844 - Refactor new 2fa page</li><li>37a3471f - Update docs for 2fa</li><li>0525ca40 - Drop old templates</li><li>01fddce4 - Fix things from review</li><li>17fe06f0 - Optimize frontend for 2FA</li><li>fa602e22 - Fix messages for 2FA</li><li>a9e9b8db - Update changelog</li><li>47c174b3 - Finally drop 2FA</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/1875/diffs?diff_id=65615&start_sha=d8986ad7772ab1204b1e6d64cef99a6829412353)
Author
Owner

resolved all threads

resolved all threads
Author
Owner

marked this merge request as ready

marked this merge request as **ready**
Member

@hansegucker please add a new screenshot

@hansegucker please add a new screenshot
Author
Owner

There is no need for a new screenshot, only some texts were updated.

There is no need for a new screenshot, only some texts were updated.
Author
Owner

added 1 commit

  • fbc111ad - Finally drop 2FA

Compare with previous version

added 1 commit <ul><li>fbc111ad - Finally drop 2FA</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/1875/diffs?diff_id=65674&start_sha=47c174b3540fea4a7c4731e1401893c7e687e93d)
Author
Owner

added 1 commit

  • 08a1a885 - Finally drop 2FA

Compare with previous version

added 1 commit <ul><li>08a1a885 - Finally drop 2FA</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/1875/diffs?diff_id=65676&start_sha=fbc111ad61ff29d2a091669a193709a185332628)
Author
Owner

added 1 commit

Compare with previous version

added 1 commit <ul><li>4d07e0d3 - Finally drop 2FA</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/1875/diffs?diff_id=65678&start_sha=08a1a885901af636812df10969d6e15dc8c4fe1e)
Author
Owner

resolved all threads

resolved all threads
Author
Owner

mentioned in commit 748bf6438d

mentioned in commit 748bf6438dc313238eedbaf8fa4c0816a3b2d55d
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
4 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
aleksis/AlekSIS-Core!3520
No description provided.