Problem/Motivation
Drupal\tfa\Form\TfaLoginFormHelper::setRedirectDestinationBasedOnPermissions() unconditionally redirects to <front> (or to tfa.overview if the user has the setup own tfa permission and the "users without TFA redirect" setting is enabled), without ever preserving a ?destination= query parameter that was present on the login request.
This method is called from both tfaValidateSubmit() (when $login_context->isTfaDisabled()) and loginWithoutTfa() (when $login_context->canLoginWithoutTfa()) — i.e. on every login where the current user does not need to complete a fresh TFA challenge right now. For any of these users, a destination set before login is silently discarded and the user always lands on <front> or tfa.overview instead.
By contrast, loginWithTfa() (the branch used when the user is ready to complete a TFA challenge) correctly preserves the destination
Steps to reproduce
- Enable TFA module; leave users_without_tfa_redirect disabled (default), or set it and grant a test user setup own tfa.
- As an anonymous user, visit /user/login?destination=/node/5 (or trigger the equivalent via an OAuth/OIDC authorization flow that appends destination before the login form is shown).
- Log in as a user who does not currently require a TFA challenge (TFA disabled for the account, or the account can log in without TFA per TfaLoginContext::canLoginWithoutTfa()).
- Observe: the user lands on the front page (or /user/{uid}/security/tfa), not /node/5.
- For comparison, repeat with a user who is required to complete a TFA challenge: the destination is correctly appended to the tfa.entry redirect and honored after the challenge completes.
Proposed resolution
The <front> branch of setRedirectDestinationBasedOnPermissions() should not set a custom redirect target at all. If $form_state's tfa_redirect_target is left unset, tfaSetRedirectAfterSubmit()'s entire block — including the line that strips the destination query parameter — is skipped, and Drupal core's own RedirectResponseSubscriber::checkRedirectUrl() is left free to honor destination exactly as it would for a login where TFA isn't involved at all. See attached patch.
As a more robust alternative, since the destination-stripping happens centrally in tfaSetRedirectAfterSubmit(), that method could instead be changed to only strip/consume destination when the stored redirect target actually needs it (i.e. move the capture-and-forward logic that currently lives only in loginWithTfa() up into tfaSetRedirectAfterSubmit() itself), so any current or future branch that stores a tfa_redirect_target benefits automatically and can't reintroduce this bug by omission.
| Comment | File | Size | Author |
|---|---|---|---|
| tfa-destination-fix.patch | 1.19 KB | mgadev |
Comments
Comment #2
avpadernoComment #3
cmlaraI belive I’ve questioned this behavior before (though never created an issue for it).
I tend to agree the logic of always hard setting
<front>appears out of place to me. I don’t know off hand the history of why it was done to ensure that it was not some sort of fix for otherwise unexpected operations, however I’m hard pressed to imagine there is a legitimate reason to do so.Comment #4
mgadev commentedYeah, that's actually something I was wondering about too.
I quickly checked and in the previous version (8.x-1.12 -
TfaLoginFormextendingUserLoginForm) the 8.x1-x branch simply called$form_state->setRedirect('<front>'), without ever touching the destination query parameter.Might be either an omission post-refactor or a deliberate design choice, but same goes for me, I don't know the history behind it.