LoggedIn logic is not robust

[jonathanjfshaw]

Currently we do this:

// Look for a css selector to determine if a user is logged in.
    // Default is the logged-in class on the body tag.
    // Which should work with almost any theme.
    try {
      if ($page->has('css', $this->getDrupalSelector('logged_in_selector'))) {
        return TRUE;
      }
    } catch (DriverException $e) {
      // This test may fail if the driver did not load any site yet.
    }
    // Some themes do not add that class to the body, so lets check if the
    // login form is displayed on /user/login.
    $session->visit($this->locatePath('/user/login'));
    if (!$page->has('css', $this->getDrupalSelector('login_form_selector'))) {
      return TRUE;
    }
    $session->visit($this->locatePath('/'));
    // As a last resort, if a logout link is found, we are logged in. While not
    // perfect, this is how Drupal SimpleTests currently work as well.
    if ($page->findLink($this->getDrupalText('log_out'))) {
      return TRUE;
    }
    // The user appears to be anonymous. Clear the current user from the user
    // manager so this reflects the actual situation.
    $this->getUserManager()->setCurrentUser(FALSE);
    return FALSE;

Unknown to me my theme doesn't use the default ID for the login form, so if (!$page->has('css', $this->getDrupalSelector('login_form_selector'))) returns true and therefore this LoggedIn() function has always found me to be logged in on every page. I only discovered this after I dug into some really weird fails - now I find that the logic of all my existing tests may have been flawed.

Let's harden this. How about:

1. Instead of returning TRUE if the form ID is not found, return FALSE if it is found. This way if the form Id is not properly configured, we don't return a false positive, we just pass on to the next approach.

2. Before returning FALSE anywhere in this function, call $this->logout(). This increases the chance that the function is telling the truth, even if not for the right reason.

[pfrenssen]

I think this whole logic should be greatly simplified and should only cover the login functionality of the base themes that are shipped with Drupal core. Themes can override this completely so there is no way for us to cover all possibilities. With the AuthenticationManager service this will be trivial to override.

The way this works now is causing every test to slow down by doing unneeded visits to the homepage.

[jonathanjfshaw]

makes sense.

…

On 2 August 2017 at 10:23, Pieter Frenssen ***@***.***> wrote: I think this whole logic should be greatly simplified and should only cover the login functionality of the base themes that are shipped with Drupal core. Themes can override this completely so there is no way for us to cover all possibilities. With the AuthenticationManager service this will be trivial to override. The way this works now is causing every test to slow down by doing unneeded visits to the homepage. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#398 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADWYQXSyBSInrPOdaUe48kwGi6Mr2Gtoks5sUEAkgaJpZM4OocGE> .

[jonathanjfshaw]

When do you want to do that? For 5.0?

[Berdir]

See also #62, which I created a long time ago and never got around to actually create specific merge requests.