Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RawDrupalContext::loggedIn() can return false positive #502

Merged
merged 1 commit into from
Nov 26, 2018
Merged

RawDrupalContext::loggedIn() can return false positive #502

merged 1 commit into from
Nov 26, 2018

Conversation

trwill
Copy link

@trwill trwill commented Aug 20, 2018

The default login_form_selector is using form#user-login as a selector, but the actual drupal login form (at least for current d8) uses form#user-login-form as the id. My guess is an update to underlying form changed this at some point (maybe during migration to drupal 8?)?

This can ultimately cause false positives from RawDrupalContext::loggedIn() - in some scenarios the login is validated by manually checking the /user/login page and looking for the form via this selector - if its not found, the user is assumed to be logged in. In this case even though the user is anonymous, the form is not found, so it returns true.

I left the old selector in for any possible BC issues, similar to how the logged_in_selector works.

For example, here's a blank site spun up w/ simplytest.me showing the actual selector:
image

@trwill trwill changed the title login_form_selector no longer matches default RawDrupalContext::loggedIn() can return false positive Aug 24, 2018
@pfrenssen
Copy link
Collaborator

This depends entirely on which theme you are using. The exact selector to use can be configured in behat.yml and it is possible to override the logic by swapping out the AuthenticationManager service.

@trwill
Copy link
Author

trwill commented Nov 6, 2018

I understand it can be overridden but this is from default theme twig file (which I imagine not too many people need to override) - shouldn't we be setting the default that core ships with? Note that other selectors have been updated to use the new defaults, eg logged_in_selector, which supports both the old selector and the new.

See: ad5df08#diff-2cc3f60cf96f66076dd61d14a7354129

@jhedstrom
Copy link
Owner

I can't figure how it's working with the old selector as the form ID doesn't appear to have changed in Drupal 8 since 2013...

@trwill
Copy link
Author

trwill commented Nov 26, 2018

@jhedstrom yeah it certainly appears to have been introduced with the launch of Drupal 8. It only 'falls back' to this logic of checking for the form if the <body> el does not contain the 'logged_in_selector', which as I point out in the diff above actually got a very similar update to the one I'm proposing here. I realize this may be a bit of an edge case, but just needs a simple update for Drupal 8 defaults and it will go back to working as intended.

@jhedstrom jhedstrom merged commit feed003 into jhedstrom:master Nov 26, 2018
@jhedstrom
Copy link
Owner

This fix makes sense to me. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants