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

feat: redirect admins to two_factor:setup if two_factors are required a two factor is not enabled for the account #491

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dopry
Copy link
Contributor

@dopry dopry commented May 5, 2022

I'm picking up the work from #370 in this PR since it has seemingly gone stale.

Description

Currently, if OTP is set to required for the admin interface and a user does not have admin privileges. Logging in fails without any feedback. This PR modifies the login process to redirect admins to setup OTP instead.

Motivation and Context

This change is required because there is a dead end in the Login UX. It applies to #219.

How Has This Been Tested?

I am resubmitting the PR as a WIP at this juncture.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@dopry dopry changed the title Redirect Admins to Setup OTP if OTP is required and it is not setup. WIP: Redirect Admins to Setup OTP if OTP is required and it is not setup. May 5, 2022
@dopry dopry marked this pull request as draft May 5, 2022 15:15
@dopry dopry changed the title WIP: Redirect Admins to Setup OTP if OTP is required and it is not setup. Redirect Admins to Setup OTP if OTP is required and it is not setup. May 5, 2022
@codecov
Copy link

codecov bot commented May 5, 2022

Codecov Report

Merging #491 (4bd592c) into master (4bd592c) will not change coverage.
The diff coverage is n/a.

❗ Current head 4bd592c differs from pull request most recent head 4c432a9. Consider uploading reports for the commit 4c432a9 to get more accurate results

@@           Coverage Diff           @@
##           master     #491   +/-   ##
=======================================
  Coverage   98.53%   98.53%           
=======================================
  Files          60       60           
  Lines        2659     2659           
  Branches      278      278           
=======================================
  Hits         2620     2620           
  Misses         24       24           
  Partials       15       15           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@dopry dopry force-pushed the issue-219 branch 4 times, most recently from e947df0 to dfa4241 Compare May 5, 2022 19:50
@dopry dopry changed the title Redirect Admins to Setup OTP if OTP is required and it is not setup. feat: Redirect Admins to Setup OTP if OTP is required and not enabled on their account. May 5, 2022
@dopry dopry force-pushed the issue-219 branch 3 times, most recently from 000633b to 2de44a5 Compare May 5, 2022 19:58
@dopry dopry requested review from claudep and moggers87 May 5, 2022 19:59
@dopry dopry mentioned this pull request May 5, 2022
10 tasks
@dopry dopry force-pushed the issue-219 branch 2 times, most recently from a49aea5 to 1f22961 Compare May 5, 2022 20:26
@dopry dopry changed the title feat: Redirect Admins to Setup OTP if OTP is required and not enabled on their account. feat: redirect admins to two_factor:setup if two_factors are required a two factor is not enabled for the account May 5, 2022
@dopry dopry force-pushed the issue-219 branch 11 times, most recently from 4cfcee7 to edbb4b2 Compare May 7, 2022 05:19
@dopry dopry force-pushed the issue-219 branch 8 times, most recently from 629ed3d to 3bebcd7 Compare May 7, 2022 08:32
@dopry dopry marked this pull request as ready for review May 7, 2022 08:40
@dopry
Copy link
Contributor Author

dopry commented May 9, 2022

@claudep @moggers87, I'd appreciate a review and feedback. I'd really like to get this in along with #493 and #497 then cut a release.

@dopry
Copy link
Contributor Author

dopry commented May 23, 2022

@claudep @moggers87 @Bouke, any chance I can get a review on this?

@claudep
Copy link
Contributor

claudep commented May 23, 2022

Sorry, I don't feel familiar enough with this part of the code to be able to review it right now.

@dopry
Copy link
Contributor Author

dopry commented May 26, 2022

@Bouke @moggers87, claude doesn't feel comfortable reviewing this. Do either of you? This is over two weeks out with no feedback.

@dopry dopry force-pushed the issue-219 branch 3 times, most recently from e27d9f6 to d76311c Compare June 8, 2022 17:46
@dopry
Copy link
Contributor Author

dopry commented Jun 8, 2022

@Bouke @moggers87, I've rebased this PR. @claudep doesn't feel comfortable reviewing this. Do either of you? It has been over a month with no feedback on this issue.

@dopry
Copy link
Contributor Author

dopry commented Jun 16, 2022

@Bouke @moggers87, @claudep doesn't feel comfortable reviewing this. Do either of you?

@dopry
Copy link
Contributor Author

dopry commented Jun 23, 2022

@Bouke @moggers87, @claudep ping? Is there anyone out there?

aseem-hegshetye and others added 2 commits June 23, 2022 09:21
When TOTP is required on an admin view and a user does not have a
TOTP device configured, redirect them to the TOTP setup view.
@dopry dopry marked this pull request as draft June 23, 2022 13:35
@dopry
Copy link
Contributor Author

dopry commented Jun 23, 2022

converted to draft while I review the impact of #500. I pulled monkey patching updates into its own PR that will need to be applied first.

@Jenselme
Copy link

Jenselme commented Sep 9, 2022

Any process on that?

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.

4 participants