When a user has not setup TFA yet, but their role requires it (tfa_basic functionality), they are logged out without any UI feedback as to what happened. The hook_tfa_ready_require() implementation in tfa_basic that implements this functionality does call drupal_set_message():

/**
 * Implements hook_tfa_ready_require().
 */
function tfa_basic_tfa_ready_require($account) {
  if (tfa_basic_tfa_required($account)) {
    drupal_set_message(t('Login disallowed. You are required to set up two-factor authentication. Please contact a site administrator.'), 'error');
    return TRUE;
  }
  return FALSE;
}

However, this is called from tfa.module (inside tfa_user_login()):

  $tfa = tfa_get_process($account);
  // Check if TFA has been set up by the account.
  if (!$tfa->ready()) {
    // Allow other modules to act on login when account is not set up for TFA.
    $require_tfa = array_filter(module_invoke_all('tfa_ready_require', $account));
    if (!empty($require_tfa)) {
      tfa_logout();
      drupal_goto('user');
    }
  }

tfa_logout() does a call to session_destroy(), which also destroys the message that was intended to tell the user what's going on.

CommentFileSizeAuthor
#14 2564813-tfa-login_disallowed_feedback-14.patch612 byteskporras07
#13 2564813-13-rescue-messages.patch1.48 KBbanviktor
#4 tfa-2564813-rescue-messages-4.patch705 byteseelkeblok

Comments

eelkeblok created an issue. See original summary.

eelkeblok’s picture

eelkeblok
he/him
Primary language Dutch
Location Netherlands 🇳🇱
commented

I'm not sure what the best way forward is. I have things working by pulling the messages array out of $_SESSION before calling session_destroy() and subsequently writing it back into the $_SESSION, but I'm not sure if that's the best approach. I'll attach a patch shortly.

eelkeblok’s picture

eelkeblok
he/him
Primary language Dutch
Location Netherlands 🇳🇱
commented
Version: 7.x-1.0 » 7.x-2.0-beta2
eelkeblok’s picture

eelkeblok
he/him
Primary language Dutch
Location Netherlands 🇳🇱
commented
Status: Active » Needs review
StatusFileSize
new tfa-2564813-rescue-messages-4.patch705 bytes

Here's the patch that implements the discussed change. One worry I have is whether there might be messages set by other modules that are no longer valid after having been logged out by TFA.

Leeteq’s picture

Leeteq commented
Related issues: +#2507409: Immediately logged out when using password reset form
greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

I feel like this is a core bug that session_destroy should transfer the messages like this. It's possible the answer is to work around it in a contrib, but does anyone else feel like this should be fixed in core as well?

eelkeblok’s picture

eelkeblok
he/him
Primary language Dutch
Location Netherlands 🇳🇱
commented

Hmm, yes, I had not considered that. I thought it was intended behaviour for the messages to disappear, but now that you mention it, it's probably an edge-case and if there are messages in the session that is destroyed, they probably should be displayed to the user regardless of what happens to the session itself. I am not sure whether there might be any reason for core not to transfer the messages, but a core issue is probably a better place than anything to explore that.

eelkeblok’s picture

eelkeblok
he/him
Primary language Dutch
Location Netherlands 🇳🇱
commented
Related issues: +#2576741: Messages disappear when session is destroyed before showing them

I created #2576741: Messages disappear when session is destroyed before showing them as somewhat of a placeholder, didn't have much time to prepare a first stab at a patch.

banviktor’s picture

banviktor commented

I don't think this is a core issue as I have commented in #2576741: Messages disappear when session is destroyed before showing them. The provided patch works well.

banviktor’s picture

banviktor commented
Status: Needs review » Reviewed & tested by the community

I feel like this patch is perfect. In my interpretation user_logout() logs the user out, and the next user could be anyone, we don't want to show them messages that were targeted for the previous user. On the other hand tfa_logout() is not a real logout and we assume that the next user is the same as the current one, so it is acceptable to forward the messages.

I think this could be committed.

banviktor’s picture

banviktor commented

* It is a real logout, but when tfa_logout() is called we do expect the next user to be the same as the current one.

banviktor’s picture

banviktor commented
Status: Reviewed & tested by the community » Needs work

Actually we should only rescue TFA's messages as @coltrane suggested on one of our meetings. Other messages might contain information that are intended for the fully authenticated user.

banviktor’s picture

banviktor commented
Title: UI feedback ("Login disallowed") disappears » UI feedback ("Login disallowed") can disappear
Version: 7.x-2.0-beta2 » 7.x-2.x-dev
Status: Needs work » Needs review
StatusFileSize
new 2564813-13-rescue-messages.patch1.48 KB

For the record if only TFA and TFA Basic are enabled this issue is not reproducible, as hook_user_login() won't even fire:
tfa_login_submit():

$tfa = tfa_get_process($account);
  // Check if TFA has been set up by the account.
  if (!$tfa->ready() && !$tfa->isFallback()) {
    // Allow other modules to act on login when account is not set up for TFA.
    $require_tfa = array_filter(module_invoke_all('tfa_ready_require', $account));
    if (!empty($require_tfa)) {
      $form_state['redirect'] = !empty($form_state['redirect']) ? $form_state['redirect'] : 'user';
      return;
    }
    else {
      // Not required so continue with log in.
      user_login_submit($form, $form_state);
      return;
    }
  }
  else {

But with other modules enabled it's possible that the user gets logged in and TFA has to log them out in tfa_user_login(), causing the messages to disappear.

To test this I simply readded user_login_submit as a submit handler for the login forms.

Here's a patch that on a logout only shows TFA's messages.

kporras07’s picture

kporras07 commented
StatusFileSize
new 2564813-tfa-login_disallowed_feedback-14.patch612 bytes

I have the same issue on D8 version of this module.

Patch attached.

Status: Needs review » Needs work

The last submitted patch, 14: 2564813-tfa-login_disallowed_feedback-14.patch, failed testing. View results

cmlara’s picture

cmlara
he/him
commented
Status: Needs work » Closed (outdated)

Drupal 7 end-of-life triage:
Drupal 7 reached end of life on January 5th.

The 7.x branches of TFA do not have any additional planned releases.

The requests in this issue do not appear to exist in the 8.x-1.x and newer branches.