Redirect to validation setup after login without tfa

Doing some testing on this and hit an edge case that might be problematic. If the user hasn't set up TFA and doesn't know their password (e.g. they've got to the site via reset link) then they can't continue to setup as it requires their password. Realise this is probably a base TFA issue, but might be more problematic if we're forcing users to the setup page.

Re-Rolled the patch from #16 ✨ Redirect to validation setup after login without tfa Needs work against latest 2.x-dev head.

Do we need update hook for schema update ?

Quick glancing, yes this should have an update hook, code appears safe without it, but its good practice to do.

Addtionaly this needs tests.

This and ✨ Force user to setup TFA when required and there are no remaining skips Needs work appear to be very similar, though it looks like slightly different scenarios.

I do wonder why in #5 it goes through the complexity of allowing the site admins to choose a redirect location instead of a simple checkbox? For that matter (getting out of scope for this issue, but its related to how we present this) I question if there might actually be a bug if we set redirect to front without regards to a previous destination request.

I've re-factored the patch to work on 1.1 and switched to using a checkbox as it does make sense to either redirect to the front page or to the setup form.

In patch #22 the line $this->getRequest()->query->remove('destination'); is missing which was present in previous patches.
For us this causes an issue in combination with login_redirect_per_role that the redirect set by that module takes precedence and the users_without_tfa_redirect_to_setup setting doesn't work.

The attached patch adds the line back in, which fixes the issue for us.

Thank you, @andy_w and @jeroen_vreuls!

Thanks @jeroen_vreuls I missed that, and it turned out that caused issues for us as well, and low and behold you had already fixed it!

Patch on #23 works like a charm with drupal/tfa 1.3.0. Thanks!

As a feature request we should work on this against 2.x first (that would be !MR28) before we consider the 8.x-1.x branch.

Back to needs work per #21 call for a post_update hook and change of the config setting.

I did call for tests in #21 however since that time I've realized our code base doesn't have great testing of the login process. Until we add Unit/Kernel tests for the login form it would be fair to call a request for tests 'excessively burdensome' and I'll omit that request if this issue is ready before we add Unit/Kernel tests to the code base for the Login form.

Also as a note we have recently moved to GitlabCi, this means we can no longer test patch files in the issue queue, All future work should be done in MR's.

@cmlara : I made code changes to add this feature in 2.x Branch and I believe it is almost ready, if you are satisfied with my approach.

I tested it manually and it works as designed for all three cases.

But it seems there are Unit test failures. I would need some guidance to fix tests. Please review and let me know if any additional changes are needed.

I have left few comments on MR please respond to them as well.

Placed comments in the MR. Did not note every phpstan note as some may disappear based on code changes however I would recommend reviewing them to be sure if any will need to be adjusted.

My largest concern is regarding moving the redirect logic out of the Login form and into the login context.

Noted in the MR however placing here for ease. The main reason the current tests fails was that we are not mocking the Global Container, and that calls to Url objects need to have the urlGenerator set into the object to avoid using the global container.

I have reworked on the issue and modified the code to just have a config to redirect user to TFA setup page. Please review MR #58. I have added post update hook as well.

I have not updated the message type from error to warning in this MR.

@cmlara

Do you want us to revert the code with different messages?

Or do you have any other suggestions?

It seems for now revering the message changes is the best option given how the alternatives have hit limitations.

Yes please revert the message changes (leaving us with the message as it existed before we started working on this issue).

@cmlara, I have reverted the changes related to message. Please re-review and merge it.

@cmlara I have rerolled the patch for 1.x Version as well.

During testing I observed few things :

1. By default the user doesn't have permission to to setup TFA, which makes it to throw access denied error refer below. I think this is not correct. Either we need to assign permission to setup TFA on module install or dont redirect user to TFA setup page.

2. If the Redirect after login module is enabled, Users are not redirected to TFA setup page.

RE: 38.1:
Good catch.

Relevant issues:
In 📌 Inform admin that role does not have permission to setup own tfa Fixed (in 2.x only) we altered the UI to warn admins inform admins when a role doesn't have permissions to setup own TFA.
In 📌 Users are directed to TFA overview regardless of 'setup own tfa' permission Fixed we changed the message to not include the link if the user didn't have permissions.

Given above I'm guessing we might want to skip the redirect when a user doesn't have the permissions to configure TFA, or at least set it to FALSE by default and add some help text for admins to ensure users have the necessary permission before they enable the feature. I imagine the first option is slightly better UX.

#38.2:
While I wasn't thinking of Redirect after login when working this issue, that scenario doesn't surprise me. As we are setting this from the form level, there are numerous locations in API after our rendering where this can be be changed and is why I had concerns about us always removing the link to the setup page in the messages.

Looking at at RAL it looks like it checks destination parameter, which I'm not sure we can override within API.

This may just need to be If you want the redirect you don't use RAL, or you set it to redirect to TFA (which I suppose actually could negate the need for this feature in TFA at all). Alternately there might be a path with Rules or ECA to say "If TFA is enabled redirect to" though I've not looked into that.

In #39 ECA is mentioned and i've added ✨ Add ECA condition plugin "is TFA setup" Active as a related ticket.

Given above I'm guessing we might want to skip the redirect when a user doesn't have the permissions to configure TFA, or at least set it to FALSE by default and add some help text for admins to ensure users have the necessary permission before they enable the feature.

Added Permission Check. Updated default value to FALSE. Added help text to admins.

I have updated the MR to remove the messenger changes along with all the related injection changes. Please re-review.

Merged to 2.x-dev. The 1.x MR will need to be updated for changes since #38

I have back-ported the patch to 8.1.x with changes after #38. Please re-review.

I have made the changes and updated the MR #63 redirect works as expected. But during testing I observed one more thing. There is no possibility to reset Number of times validation skipped for admin on the user profile. But it is available to other user.

Once the admin with uid 1 has exhausted all their logins without TFA, then they wont be able to login through UI. I think it is not correct.

UID 1

UID 2

If you compare above two images there is no option to Reset skip validation attempts for UID 1.

resetting TFA count for currently logged in user is not possible by the logged in user. But it is possible to reset other users. It is intended behaviour

Without looking at the source code and commit history I’m not sure how intentional it was, however I would probably say it’s safe to call that works as designed.

If an admin could reset their own lockout they could avoid setting up TFA indefinitely, at least as described it requires “a second user” to get involved and for them “both to conspire” to breach security.

Automatically closed - issue fixed for 2 weeks with no activity.