Option to enforce 2FA on specific users and/or Authentication Providers

sandstorm / NeosTwoFactorAuthentication

Extend the Neos Backend Login with 2FA

MIT License

12 stars 8 forks source link

Option to enforce 2FA on specific users and/or Authentication Providers #28

Closed vcg-development closed 7 months ago

vcg-development commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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

JamesAlias commented 7 months ago

@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.

Benjamin-K commented 7 months ago

@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 commented 7 months ago

@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 commented 7 months ago

Added here: #30