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

  1. Enable TFA module; leave users_without_tfa_redirect disabled (default), or set it and grant a test user setup own tfa.
  2. 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).
  3. 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()).
  4. Observe: the user lands on the front page (or /user/{uid}/security/tfa), not /node/5.
  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.

CommentFileSizeAuthor
tfa-destination-fix.patch1.19 KBmelakb

Comments

melakb created an issue. See original summary.

avpaderno’s picture

Version: 2.0.0-alpha5 » 2.x-dev
avpaderno’s picture

Status: Needs review » Closed (duplicate)
Related issues: +#3618099: Destination is lost on login when TFA is not required

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.