Skip to content

Conversation

@vcg-development
Copy link

We use the OpenIDConnect extension where 2FA is enforced, but we also use the default Username-Password-Provider as a fallback and for all external users, with which we collaberate and which are not in our AD.

For the latter group we would like to enforce this plugin. Hence, we extended the options to enforce 2FA for not only all users but also for specific users/authentication providers

@Benjamin-K
Copy link
Contributor

I love the new options of this PR. But I would prefer setting a role that is required to set a second factor instead of being required to update the code for every new user. This could even be an abstract role already provided by this package/pr IMO.

@JamesAlias
Copy link
Contributor

Thank you for this PR 🙂

I second the opinion of @Benjamin-K.

  • Replace the enforceTwoFactorAuthenticationForSpecificUserAccounts option with something like enforceTwoFactorAuthenticationForRoles
  • Adding a Role to the package that can be used out of the box is not necessary but I'm not against it

Copy link
Contributor

@JamesAlias JamesAlias left a comment

Choose a reason for hiding this comment

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

Some small change requests and ideas.

@JamesAlias
Copy link
Contributor

@vcg-development @Benjamin-K

I have fixed a regression where I could delete the last second factor of an account even though it was enforced by role/authProvider for this account.

I also added a SecondFactorService to centralize the logic and make it more readable on the consuming side.

@JamesAlias JamesAlias merged commit 59b3149 into sandstorm:main Mar 27, 2024
@Benjamin-K
Copy link
Contributor

@JamesAlias Thanks for having a look at this again and cleaning the code up a bit.

A (new) question raised for me, when reading your last comment: As an administrator I'm not able to remove the last second factor of an account at the moment, if that user is forced to have a second factor, right?
I think, it should be possible for admins to remove the last second factor of other users no matter what roles they have to help with issues with lost second factors (broken phone, ...). If you agree, I'd create a new issue for that.

@JamesAlias
Copy link
Contributor

@JamesAlias Thanks for having a look at this again and cleaning the code up a bit.

A (new) question raised for me, when reading your last comment: As an administrator I'm not able to remove the last second factor of an account at the moment, if that user is forced to have a second factor, right? I think, it should be possible for admins to remove the last second factor of other users no matter what roles they have to help with issues with lost second factors (broken phone, ...). If you agree, I'd create a new issue for that.

Good idea. Please do 🙂

@Benjamin-K
Copy link
Contributor

Added here: #30

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.

Feature Request: Option to force 2FA for Administrators (or other Editor-Groups) only

4 participants