Allow TFA requirement to be configured per user

I'm concerned that this change, as currently proposed, makes a critical area of the code potentially 'fail open', particularly around the fact that the login code now becomes dependent upon an external service (the event system) to take action to enable TFA. There is no integrity in "fire an event and check a response" that ensures our code (or any code) has actually executed to enforce TFA. We could claim that is the fault of local admins if a scenario like that does occur, however as this is a security module there is an onus on us as a project to have code that is harder to defeat.

I can understand the use case where we might want to force TFA enabled for a user who otherwise would not, however the event system allowing TFA to be disabled even if the user is in a role that requires TFA, that concerns me both on security levels and UX design.

@dpi
I'm curious if you could elaborate a bit more on how this interferes with testing? Is this 'live environment' testing where a HOTP token or using a role that doesn't have TFA enabled is not viable?

@cmlara

I'm curious if you could elaborate a bit more on how this interferes with testing?

As in PHPUnit w/ DTT (live db snapshot).

When dealing with a working site snapshot, we dont want to modify TFA and its config. So we'd rather disable TFA by opting into it when needed. Doing this in PHP is more efficient than currently where config needs to be modified and dealing with config resyncs or container rebuilds.

I'm concerned that this change, as currently proposed, makes a critical area of the code potentially 'fail open'

Maybe we can update the event itself to fail on neutral.

As in, if in the final isEnforcingTfa, we can throw an Exception if nothing invoked either unEnforceTfa or enforceTfa on the event. The event class would be responsible for this.

I think its pretty solid that things would work as is with a first class event subscriber, but above would add a safety net.

Whats your opinion on this proposed solution?

Maybe we can update the event itself to fail on neutral.

That could be a good method, it would certainly remove the "it was fired and forgot" concern.

Would it work for you if we kept the required roles portion as part of the LoginContext before we fire the event? Doing so would allow us to address my concerns about the UI and retaining the security of roles where TFA is 100% required while at the same time providing a method for site owners to make other roles/users required conditionally.

Conceptually its wise to have a TFA system usually be as decoupled as much as possible from the system prompting for authentication, however we are already dependent upon external services, and we are never going to be able to fully isolate because of how we have to be structured with Drupal. I just want to make sure we keep a balance between security and functionality.

Bhanu951 → changed the visibility of the branch combined-3318456-3318452-3318453 to hidden.

Dropping into this issue because I have a challenge with automatically created test users for each role; some of these roles have TFA, but it is a pain to actually set it up for the test users every time. So, I am looking for ways to circumvent TFA just for these test users. Would this solution still allow that, or are you saying the setting for the roles is the baseline, and other code could add more restrictions on forcing users to use TFA, not less?

Basically yes to the edit

Re-summarizing my suggested compromise in hopes to make sure its more clear:

Some sites might not have any roles as "TFA Required" inside the TFA provided configuration UI. These sites would enforce TFA required using the Event system. This would allow granular control, down to the user level and could depend upon any external factors a site owner desires on if TFA will be required for each login.

Some sites might use only TFA required from the TFA configuration UI. These sites would know that these users would require TFA and that the event handler can not downgrade that requirement as it would never be called for these roles.

Some sites might use both, protecting high level roles using the TFA UI which would not be allowed to be overridden by the event handler, while for users in all other roles it can be chosen by external factors.

Coming here from ✨ Allow modules to skip TFA through a hook Closed: duplicate .

The MR does not merge to the latest 2.x-dev.

@dpi - an example of an event subscriber which allows a modules to skip TFA would be very helpful (for documentation).