Login fails and no warning is issued if cookies are not enabled

Making this a feature request instead of critical bug report. Tss.

Marking as duplicate of http://drupal.org/node/137678

There's some useful discussion over at http://drupal.org/node/137678 but we should continue with this, the oldest issue, as the active one that needs to be resolved.

Offering a user to log-in in a situation where the login will automatically fail can safely be considered a bug.

This is a bit tricky but I figured it out. Messages do not work here too well because you get a new session on every single page. So we redirect you to a help page if there are no cookies, but not everytime.

Also http://drupal.org/node/137678

trying to log in now brings to a page with a brief description about the missing cookie support. It could be enhanced later, but with this the bug is solved for now.

Yeah but as comment cookies need path set so do these. It's not trivial under testing because it's only a problem if you land on the non-root page and try to log on another page.

Errr... can you explain patch #10. Is it the right issue?

As to patch #7, this is a patch my poor brain can understand. I am satisfied that it is a good approach.
But we are only at the stage of reviewing the concept. #10 has not been reviewed, and #7 is not RTBC in any case: if the patch works and is accepted, at the very least the message on the no_cookie page should be improved (poor grammar, not enough information). It's good enough for testing, so it's good enough for now, but it will have to be improved before we can set it RTBC.

Speakind about the documentation on the no_cookie page, see #6 here:
http://drupal.org/node/137678#comment-271450

I personally think it's a bit too much, but it can give some ideas. What do you think?

yeah #10 looks like the wrong patch.

#10 looks like the completely wrong file got uploaded. Yoohoo, CHX...

I think we know that, by now.

What I would like to know, is the answer to #12, so that we can move ahead...

roll a patch with this text, so we can check it ;)

I think this could be committed though, its only the cosmetics of the text we try to fix now, the issue itself is solved.

Well, #10 and #7 wanted to differ only in the path component and I indeed upped the wrong version. About the text, it's a placeholder, someone else will write the text, it's not my cup of tea.

Also, about the linked comment. One, it misses firefox, opera, safari and ie7. Second, the big problem here is that it's not necessarily the browser that kills your cookies -- it can be your personal firewall (zonealarm is a notorious offender), it can be a bad proxy in the way etc etc

Let's have this committed, and I will open a new issue about the text. This way we keep this thread to the technological issue, and that to discuss the helptext. How about it?

Nice work chx.

Needed this so turned it into a D5 contrib module, http://drupal.org/project/cookie_check. Here's the message I used:

  global $base_url;
  $url = parse_url($base_url);
  $text = t('It seems that your browser does not accept cookies. To log into this site, you will need to accept cookies from the domain @domain.', array('@domain' => $url['host']));
  return $text;

Applied nedjo's text and rerolled for HEAD.

Unfortunately, user/no_cookie gives me "Page not found."

Thanks for the updated patch.

Unfortunately, user/no_cookie gives me "Page not found."

Did you refresh your menu item cache? Since the module is already enabled, you'll need to do so to get the new menu item available. You could try e.g. disabling then reenabling a module.

Ah, yes, clearing the cache has made it work. Thanks!

This looks good to me. Tested, works great.

Actually no, "Cookie problems" isn't a great title - implies that Drupal's having problems rather than the user's browser.

Would suggest "Please Enable Cookies" or something like that.

Ok, I've changed the title to 'Please Enable Cookies!'

Slightly changed wording.

Looks good to me.

On a side note, I tested the patch in IE7 to avoid affecting tabs in firefox. Then left cookies off. Then cleared my D6 install and later wanted to test some js in IE7. How much did I miss this patch the first couple of times I tried to log in! Only took five seconds to work out what was going on in this case, but a big improvement.

- Drupal core paths use dashes instead of underscores...should be user/no-cookie.
- Might as well just return t(...) instead of setting an unneeded $text variable.

subscribe

I suggest that user_no_cookie() should return theme('user_no_cookie') so sites can override this hard-coded text.

New patch. The only change here relative to #30 is the theme_user_no_cookie() instead of hard-coding the text.

This bug has always annoyed me but perhaps it is too late for D6. I think there are valid outstanding questions:

1. The default text (http://drupal.org/node/137678 has something but it isn't done).
2. Should the cookie path really be "/" and not session.cookie_path?
3. Probably others.

Oh, and how long should the login_check cookie survive and/or should it use some kind of unique value to try to identify if the login_check cookie that is coming back is the one that just got set?

I think while testing in IE6 I observed that by turning off cookies I was only turning off accepting new cookies, not disabling the delivering cookies I already had. So a login_check cookie I received while testing yesterday was being delivered even though I had "turned off cookies" so my new session cookie wasn't being accepted. The result was that login failed but I did *not* get redirected to the no_cookie page b/c the login_check cookie was delivered. This could make debugging login problems even harder, so we need to understand this before committing.

The cookie should be deleted after logging in.

Patch looks reasonable. I am not sure, but maybe we can expalain how cookies can be set to be accepted in simple terms (ie. "turn off private browsing" if this is common enough among browsers).

http://drupal.org/node/137678 has good source data for a documentation page to put up. While that should not be included in Drupal itself, and it might not be wise to direct innocent users to drupal.org, it would be a good docs page. :| Thinking out loud.

Gábor, please don't try to save the world with this patch! There are countless reasons why cookies might not go through, beyond what the browser does, and every browser uses its own terminology anyway. I've never heard of "private browsing" -- would you really want to take responsibility for whatever could happen if a user "turns off private browsing" on some unknown system? Helping the user to solve the problem is way beyond what Drupal can do.

The intent of this humble patch is to diagnose the problem, and this alone will put us miles ahead of where we are now, where the user has absolutely no clue about why his password is accepted, but he still doesn't get access.

Users who have cookies turned off, will usually know how to turn them on, because the situation comes up regularly on all sorts of sites -- but they need to know what the problem is!!!

Please excuse me if this comes accross a bit too strong, but I'm desperate about this...

OK, let's fix chx's concern at least first.

A lot better patch. Instead of session.inc , I patched drupal_anonymous_user. Same: if your system can't hold cookies then on the page load your cookie will be set but it's only available on the next page load. I have adjusted the cookie domain to the session cookie domain, too. And now I delete it in user_login_submit.

Actually, drupal_anonymous_user() is used to as generic anonymous user object fetcher function (I have seen it used as an $account parameter generator for access checks for example). So I don't think it is a good idea to build side effects into it.

OK then. it's still better than the older ones because we now patch both places where anonymous user can happen.

Doh.

Adjusted user_no_cookie function to use the relevant domain.

Added domains to the setcookie .

Yes, now it works perfectly, including deleting the cookie upon log-in. Thanks!

I'm not really sure this is worth fixing. Looks a bit crufty to me.

This is a huge useability improvement, and makes Help Desk workers happier :)

++ 1 for the patch.

+1 from me as well. We get regular e-mails from people saying "I can't log in!!11!!" and if this reduces them by even 5% that'd be great.

I run with paranoid firewall and browser settings, and I can't remember encountering a site that didn't tell me I needed to enable cookies when it requires them, except for Drupal sites, that is.

Drupal really, really stands out (negatively!) in that respect...

+1 from me, too. This is not cruft. This is a frequent source of support effort on nearly every Drupal site (certainly all the ones I have personal experience with). Almost all sites on the net are smart enough to diagnose this problem and warn you when you can't log in because your browser is not accepting cookies. Drupal should, too. Quick visual inspection of patch #48 looks good to my eyes.

Maybe I am missing something, but why is it needed to do a "setcookie('drupal_login_check', ..."?
If cookies are disabled, $_COOKIE will always be empty and if cookies are enabled, $_COOKIE will have at least one entry with the session name (except for first visit).
So in user_login_submit() if (count($_COOKIE) == 0) should be equivalent with the if (!isset($_COOKIE['drupal_login_check'])) from patch #48.
If so, there is no need to add "cruft" to session.inc and there is no "cruft" needed for deleting the 'drupal_login_check' cookie.

See attached patch. I tested it and it behaves the same as the patch from #48.
(But I could be wrong about the assumption that $_COOKIE will always be non-empty after first visit when cookies are enabled.)

Interesting thought, but can we be sure that the browser doesn't send any cookies at all, if they're currently turned off? Or could it be sending old cookies?

Maybe we do need the 'drupal_login_check' cookie, but with a very short lifetime? Or add a timestamp so that we can verify that we're getting a fresh cookie?

I re-RTBC #48 -- I would never rely on an empty cookies array. In theory it's not even impossible that this check fails, ie. the browser holds the login checker cookie and not the session one... but it's imo much better than relying on an empty cookie array.

I agree #48 is more reliable and therefore superior.

I would never rely on an empty cookies array. In theory it's not even impossible that this check fails, ie. the browser holds the login checker cookie and not the session one... but it's imo much better than relying on an empty cookie array.

Fair enough. But you can never trust a/every browser. You could even create a browser so that #48 would fail.
This issue is a usability issue, so we should solve it for most use cases/standard browsers, not all imaginable exotic corner cases. The point is to find the right balance between adding cruft and solving the issue for enough people/cases.
It would be nice to have some real life examples that would show that #55 is significantly inferior.
I just wanted to address the cruftiness concern of Dries.

On a slightly different note: why deleting the 'drupal_login_check' cookie in patch of #48?
Some modules might want to check if cookies are enabled. The CAPTCHA module (of which I'm co-maintainer) for example requires sessions (and by consequence cookies) to work properly. It would be nice if there would be a standard drupal way of detecting if cookies are enabled.
So why not renaming 'drupal_login_check' to 'has_cookie' (just like the 'has_js' cookie) and not deleting it.
Just thinking out loud.

@soxofaan: see #36...

(this problem may apply to the has_js cookie, too!)

Looks like last minute campaigning did not get Dries' favor, so moving to the 7.x queue, in the hopes that it will be getting in early in the cycle.

Well, now that soxofaan has successfully kept this patch from getting into D6, maybe we can pick up the pieces and put #48 into D7?

* The patch does no longer apply.

* Is this something we can write a test for?

* The patch does no longer apply.

* Is this something we can write a test for?

Dries -- no, because you can not even get to test page if your install does not keep cookies and also the test path is server->server meanwhile the request path is browser->firewall->ISP proxy->whatever->server.

I made a D5 module based on this patch, http://drupal.org/project/cookie_check

Users reported a bug, possibly limited to IE7:

drupal.org/node/242060

The bug may be due to some change I made in adapting the patch but we shd try to reproduce before applying this.

I shouldn't complain as I didn't do any work to solve this issue, but it's hard to understand why there has been a patch that solves this problem for almost a year and it still hasn't been added. I often have user coming to me and complaining that login doesn't work and it's always because they haven't enabled cookies for this site.

Sometimes it seems a bit strange how difficult it is and how long it takes to get such basic things to be added to drupal. Every other CMS we use at the university does have this cookie warning feature...

@Frank, the patch was posted in May. Count the number of people who've tested it since then, there's your answer. If you tested it, and it worked, why didn't you post back here to say so?

I can only apply it to 6.x and I learned that almost no one cares about this because everything is patched in HEAD and maybe backported afterwards, maybe not.

It works fine in 6.x for my site, but I don't see how this helps :-(

> the patch was posted in May

That's not really true. The patch is from January, but it has been ignored until it didn't apply anymore, see #66. Then chx made the new version which just differs in the context lines, not in the patch code itself. And then this has again been ignored until it didn't apply anymore. And now it looks like chx is not interested in making another version of the patch (which might again be ignored for months?).

According to http://testing.drupal.org/pifr/file/1/user-no-cookies-2946-67.patch it was received for testing on November 8th. So I wonder what happened between May and November. It seems likely that a patch written in May for HEAD doesn't apply in November anymore.
Same for 6.x, but it was again just adjusting the context lines. If I had 7.x I would try to correct the patch, but I've no idea (yet?) how this testing stuff works and what must be done to make a patch get into this testing.

So, I don't know what I can do more to say "Works great in 6.x". But you're right, I could have said that earlier!

Here is an updated and reworked patch.

Changes:

* Move the cookie checking to a validation handler, alongside the three existing user login validation handlers.
* Set a form error rather than forwarding to a separate page.
* Default to $_SERVER['HTTP_HOST'] if ini_get('session.cookie_domain') is empty.

These changes:

* Address changes in HEAD that made the previous approach no longer work.
* Increase flexibility--sites can alter or remove this validation.
* Avoid creating a new menu item.
* Prevent the potential confusion of users being at or returning to a page with a message saying their browser doesn't support cookies after they have enabled cookie acceptance from the domain.
* May increase usability by returning users to the login form they used.

chx noted in #7 that messaging is potentially problematic in that, without cookies, data don't persist in the session.

In this case we remain on the same page and so messages set in the $_SESSION variable remain available for rendering; the form_set_error() call works.

This would break if e.g. a contrib module set a validation handler that called drupal_goto(). I think we can live with that though. Handling this as validation feels the cleanest.

With patch...

I ran tests locally and didn't get the reported error. Setting back to 'code needs review'.

Patch applies and generally looks good. A couple of notes:

We may want to consider the help text "It seems your browser does not accept cookies." seems like slightly odd phrasing perhaps. Also, I wonder if it would be good to link to some generic "what the heck are cookies" page (e.g. on wikipedia?).

 /**
 * A FAPI validate handler. Sets an error if cookies are not supported.
 */

This looks a little nonstandard also - the function below it (which is equivalent) starts as "A validate handler on the login form....".

People who don't know what cookies are, are unlikely to have this problem. And if they do, then this is not the first site where they encounter the problem, and they need to get help anyway. It's quite likely the first site that fails to provide any indication about the nature of the problem though... 8-(

What we desperately need is a terse message that tells those who know, what the problem is, so they can immediately start work on fixing it rather than having to search for why login attempts might be ignored.

There can be many other reasons besides the browser, so the messages must not be more specific than it can rightfully be.

Please propose improved wording for the comments.

I think it's more important to have any hint on what's going wrong than just beeing unable to login. There are pages explaining how to activate cookies, but they usually miss my browser anyway or are for Netscape 6.x or sth.

This affects the installer as well. Some people have cookies turned off by default, but have a whitelist of web sites where it's accepted.
When installing a new site, the new domain may not yet be in the whitelist.
In such conditions, it is impossible to install Drupal at all, but the the warning given make it sounds like it's a sql issue, which is very confusing and does not help finding the solution.

The following critical installation issues are caused by the absence of cookie:
#234539: Critical SQL error in installer with {menu_router}
#261148: menu_masks variable is empty (race condition)
#280015: cannot install drupal - menu_router error

I agree, this is a bug from the users point of view (the only view that counts).
Cookies being disabled/not-supported should be gracefully handled. a fix should also (ideally) be backported to 5 and 6 also.

subscribing.

Fixed domain name display from nedjo's patch in comment #76

As an example of how absence of cookies are handled elsewhere, turn off cookies and visit this web site:
http://www.expedia.com/
Can't miss the (scary) warning!

Another example:
A simple warning is given here (again, visit with cookies turned off).
http://www.autism-society.org/site/PageServer?pagename=asa_home

I think it's time to get the fix for this 2003 request rolling.

I've block cookies, log out of the fresh Drupal 7 head install and applied the patch. As soon as I try to log in, I get the proper warning "It seems your browser does not accept cookies. To log into this site, you need to accept cookies from the domain example.com".

Are there some issues which would prevent this patch from being committed?

Holy Batman, my patch is not in yet????

Batman is not holy. :-)

I guess we should grab webchick today for the code sprint. I've have not seen her yet this morning.

Since this patch is limited to responding to attempted logins using login forms, it doesn't yet address two other needs:

1. We need to issue a warning at install time what cookies are required. (User does not log in using the login form.)

2. There are other cases where cookies may be required although users are not logging in. E.g., functionality that uses cookies to track anonymous users.

Still, it's probably a good first step. It handles the most pressing issue in Drupal core and could later be extended or modified to address these other needs.

I think it would be best for you to create two new issues for:

1. We need to issue a warning at install time what cookies are required. (User does not log in using the login form.)
2. There are other cases where cookies may be required although users are not logging in. E.g., functionality that uses cookies to track anonymous users.

:-)

We need to issue a warning at install time what cookies are required

hook_requirements() of the user module?

There are other cases where cookies may be required although users are not logging in. E.g., functionality that uses cookies to track anonymous users.

new hook?

We need tests for this. I check with cwgordon7 and he said it should be possible to override curlConnect() in your test case.

@webchick

any updates weither this is testable or not?

I relayed what I was told. There's a testing sprint going on this weekend, so plenty of people to ask in #drupal. :)

Can we just commit and add the test later?

Most of the experienced core commiters have already worked on this issue without raising the test issue.

++ 1 Bug Report
++ 1 Usability Issue
++ 1 Expand this issue to cover other forms in addition to user_login

Any functionality that requires the presence of a cookie should alert the user if the cookie is absent.

Many thanks to chx and nedjo for addressing this issue and contributing to a solution.

As nedjo correctly pointed out in #92

2. There are other cases where cookies may be required although users are not logging in. E.g., functionality that uses cookies to track anonymous users.

The issue #502816: Anonymous users do not see status messages is one of those examples.

I was very surprised reading Dries comments on this issue.

No, don't expand, commit!

As chx explained in #68, SimpleTest is close to useless for this. We'd test the behavior with one "browser" (that doesn't even exist in the real world) in an extremely unrealistic environment. The real world is much more complex and we won't know whether it works out in the wild until we let it loose.

Do we really want Drupal 7 (!) to become known as the one CMS/Framework that still, in 2010, doesn't even make an attempt at detecting this obvious stumbling block? What a cool way to fingerprint Drupal sites! :-(

If this had been committed in spring we'd have half a year's worth of almost-real-world testing by now. But we're still dabbling — it's pathetic...

robeano patch in #86 still applies. I'm requesting a re-test of his patch.

As per other comments I'm putting this back to RTBC. :-)

The patch fails. Needs a reroll.

1 out of 3 hunks FAILED -- saving rejects to file modules/user/user.module.rej.
[23:31:46] Encountered error on [apply], details:
array (
  '@filename' => 'cookie-validation-2946.patch',
)

Here is a straight reroll of the patch in #85

I saw the node id of this issue and was sure I was misreading. I'm amazed this hasn't been addressed since Dries reported it in 2003

+++ modules/user/user.module	10 Dec 2009 04:38:39 -0000
@@ -1770,17 +1770,18 @@ function user_login($form, &$form_state)
+  return array('user_login_name_validate', 'user_login_cookie_validate', 'user_login_authenticate_validate', 'user_login_final_validate');

Shouldn't the cookie validator run first, so the potential error message appears first?

+++ modules/user/user.module	10 Dec 2009 04:38:39 -0000
@@ -1794,6 +1795,16 @@ function user_login_name_validate($form,
+ * A FAPI validate handler. Sets an error if cookies are not supported.

"Form validation handler to ..."

+++ modules/user/user.module	10 Dec 2009 04:38:39 -0000
@@ -1794,6 +1795,16 @@ function user_login_name_validate($form,
+  if (!$_COOKIE) {
+    $domain = ini_get('session.cookie_domain') ? ltrim(ini_get('session.cookie_domain'), '.') : $_SERVER['HTTP_HOST'];

These two lines need an inline code comment (each), explaining what is being done here.

+++ modules/user/user.module	10 Dec 2009 04:38:39 -0000
@@ -1920,6 +1931,7 @@ function user_authenticate($name, $passw
+  

@@ -1944,6 +1956,7 @@ function user_login_finalize(&$edit = ar
+  

Trailing white-space.

Lastly, we need a test for this functionality.

This review is powered by Dreditor.

Sub.

#104: cookie_validate_2946_reroll.patch queued for re-testing.

Kind of just hoping that the requested test is now there. If not maybe I can work on writing one.

This is really getting nasty. It's an obvious issue which is seven years old. The patch is few lines of code which everyone can read and understand, but it's been delayed for 2 1/2 years, and now it's blocked again because it needs a simpletest.

I wonder why every other website I know is able to issue a warning when trying to login without cookies, but drupal makes this a huge, unsolvable problem.

Does anyone have any examples of Simpletests that check/manipulate the HTTP headers? I'm not really sure where to start with coding this without an example test to base it off of.

Basically we need to test two "browser"s:
- one that ignores any Set-Cookie response header (ignoring Set-Cookie means that a Cookie header is not added to subsequent requests), and
- one that handles Set-Cookie headers normally (Cookie header is added to requests)

Well, drupal_http_request will handle getting/setting the headers for us, I know there are a few tests out there using it. I have one maybe could put up this weekend, but thinking how about we make this something to work on at the core code sprint next week? I'll be there, so I'll be pushing for this issue.

You cannot assume that just because $_COOKIE is empty, the browser does not accept cookies. Since #201122: Drupal should support disabling anonymous sessions was fixed in January 2009, we no longer create sessions for anonymous users. If JavaScript is not enabled, the has_js cookie is not set. If JavaScript is disabled in your browser, you cannot log in with this patch.

If $_COOKIE is non-empty, then the browser supports cookies, at least partially. I don't know whether any browser has "partial" support for cookies. I would think (but it's just a guess) that if $_COOKIE is non-empty, we can assume that it will accept our cookie. Comments #57-59 discuss this, but without any clear conclusion AFAICT.

In any case, we need to set a cookie in one request a verify its existance in the next request. Setting a cookie from PHP to all anonymous visitors seems like a bad idea, because a Set-Cookie header is usually a sign that the page should not be cached (e.g. by default (and design) Varnish does not cache pages with a Set-Cookie header).

One possibility is to make the login form return to an interim page that just checks for the existance of the session cookie and then redirects to the final destination. This will add an extra request to all logins. The redirect happens automatically, so the additional request will be invisible to the user (except for a small delay).

If we conclude that a non-empty $_COOKIE is a safe sign that the session cookie will be accepted and the form submission contains a non-empty $_COOKIE (usually containing just the has_js cookie), we can skip the extra round-trip and just redirect to the final destination immediately. This would make the login as fast as today for users with JavaScript enabled, while other with JavaScript disabled would need the extra round-trip.

For inspiration on how to write tests for this, you may want to take a look at modules/simpletest/tests/session.test. To emulate a browser without cookie support you should probably tell cURL to not follow redirects using CURLOPT_FOLLOWLOCATION and then instead follow the redirects “manually” and reset the session cookie between each request.

I confirmed that the previous approach does not work with the new lazy session creation.

Here is an alternate approach that avoids adding a redirect simply to detect a cookie. What we do is add a querystring parameter to the destination page the user would end up on. We then check for this parameter and, if the user is not actually logged in, then we know they don't have cookies enabled. We might want to change the user flow a little. For example it may be nicer to do a set_message and redirect to "user", because right now if you log in on /user without cookies you end up on user/123 with the cookies message, but it is an access denied page. This would certainly need a simpletest to check the various combinations of /user vs. block, and with page caching enabled/disabled. First though - what do people think about this approach in general?

Still looking for review on the general approach

I think we should consider doing the redirect here. #547570: Login via user login block renders page twice points out that this happens anyway from the login block, and that the redirect is quite expensive - so it adds a dedicated redirect callback to save page rendering. If we used that same redirect callback for user/login then the additional bootstrap would be very cheap, and this would presumably allow us to redirect somewhere useful on failure too.

subscribing

@catch - my patch does not actually use (or need) any redirect. I think either approach should work fine - but I am not sure which I prefer...what do you think?

Even though the user_init() is just an isset(), I'd be happier with a check just during the process of logging in rather than having that code in the critical path.

Also if we're going to do a redirect from the login block anyway, and have a nice handy callback which controls that, then putting the cookie checking code in there is fairly tidy. The question then is whether the isset() in user_init() and very small bit of code weight there is better or worse than an extra bootstrap (but no page render) on logging in via the form. I'm not sure either way on that but lean slightly towards the log in form.

Actually, now that I think about it, I am not sure #547570: Login via user login block renders page twice really changes anything that helps us here. The basic workflow is the same as it is now (just we hit a lightweight page, rather than a content page). Specifically in the following, it just changes pageload 2 from building an entire node/123 page, to a simple page - however it is still a single redirect, and we actually need 2 redirects.

Load 1: /node/123 - we can't add a cookie here (without breaking caching) because we don't know if the user will actually use the login block, or just click away

Load 2: user submits login form - we process the login then redirect them on to their destination - we can add a cookie here (and we do already, in fact)

Load 3: user back at node/123 - we can check for a cookie here, but this is basically the same as the patch I have already

If we are going to drop the hook_init isset, then we need to add another redirect either before or after load 2, that has the single purpose of setting or detecting cookies. Given this, I think my approach, or some variant of it is preferable to having 2 redirects on login.

I don't like either the double redirect option, or the runtime check on every page here, but neither do I have any better ideas - would be good to get some feedback from others. However some of the information in this issue needs to be added to user_init() in a comment if we go for this option.

This is somewhat along what I proposed earlier, but not completely:

Do you think that we can assume that the browser uses the same logic for determining whether to accept or reject a cookie when set using a HTTP header and from JavaScript, assuming that the cookie is set using the same parameters except for the name? In that case we can set a cookie using JavaScript in the onsubmit handler for the login form. The cookie should have the same path+domain+lifetime as the login cookie. When the login form is submitted, we do the extra redirect as already suggested, but only if the cookie set in JS is not present. This allows most users to login without an extra redirect.

The tests are failing, so the patch still needs some work...

Some of the comments above referred to this being an issue with the installer as well, which is still true. It was reported recently here, and is still an issue in Drupal 7:

#791004: Installer does not warn the user that cookies must be enabled with the correct cookie domain (and fails when they aren't)

Since the installer is something of a separate problem (which the latest patch here doesn't address), it's probably best to keep two separate issues for this, so I am leaving that one open but just cross-linking it here.

Sorry. Issues that have been in Drupal since 2003 don't get to hold "critical" status.

Ignoring the problem for 7 years doesn't make it less critical. Getting rejected to login without any notice why it happens is a critical usability issue.

Shhh, we're aiming for a world record here...

Subscribe.

Subscribing. Is #114 going to work with lazy session creation, or should we try for #124?

#114: cookie_validate_2946_114.patch queued for re-testing.

re: cookies and usability
The most recent 6.17 patch update rendered existing Firefox cookies useless. The result is that no recent visitor to my site can log in without first clearing cookies. Unfortunately, they have no way of knowing that. Truthfully, how many would think to do it given no prompt or message to that effect? Attempts to change password fail when following the emailed link. And what is the pain-in-the-a$$ cost for clearing all one's other, perfectly good cookies?

Nice software, generally, though!! Thanks to its authors :^)

@satchinsb: Please don't hijack this issue — create a new one against 6.17.

subscribing...

Re: 114, I implemented this technique in a module for my own (not generalized) situation. The only minor issue I found is that if the user logs off and uses the browser's history or back button to return to the page containing the "state=loggedin" query, then the cookie warning message is displayed when it shouldn't be. It would be possible to reword the message to allow for this possibility, absent a better solution.

In reading through the 130+ messages, there seems to be a lack of agreement on the urgency which is hard to explain. Perhaps it is relatively uncommon for Drupal sites to be used by authenticated non-technical users -- the type likely to be frustrated by this. My site has exactly this type of user. I would very much like to see this fixed in D7 and backported to D6. Even if the fix is not absolute perfection, could we get something into core and keep open another issue for research for the next 7 years? ;-)

Please. It's time to plug a serious usability bug for those unlucky enough -- or unskilled enough -- to fall into it ... and for the site owners who then suffer. D7 emphasizes usability after all, no?

subscribe

#114: cookie_validate_2946_114.patch queued for re-testing.

Issues like this make we want to stop trying to contribute to Drupal core at all... ;)

- This is at least a major UX bug/WTF. We seem to be totally schizophrenic when it comes to our UI/UX priorities. Half-baked UX solutions are rushed into core under the banner of "UX trumps all" while critical UX bugs like this that actually affect 1000s (millions?) of Drupal users are postponed and punted to low priority. Either creating a "delightful experience" is important or not. If so, we can't downgrade obvious bugs like this to "normal task" just because so many people got burned out trying to get this in and faced various roadblocks.

- I think I'm with catch and I'm leaning towards the double redirect solution. Although it does delay login slightly (in an automated way), a) logging in is a very infrequent operation even on sites that allow authenticated users and b) not all sites even have real authenticated traffic. So, I'd hate to slow down the critical path of user_init() with this check on every page load for every site, just to handle a relatively infrequent thing. Also, it's extremely common on the existing web that when you click the "login" button, you wait a few seconds for at least a few redirects, if not more. It's not like users are going to be shocked or angry if logging in seems complicated and/or slow. And, if the first redirect sends you to a relatively cheap page that just ensures your cookie worked and either prints you the warning message or sends you off to where you wanted to go, it's not going to really be that slow.

Cheers,
-Derek

p.s. Please stop requesting #114 for retesting. The patch will never apply now that #22336: Move all core Drupal files under a /core folder to improve usability and upgrades is done. If we're going that route, we need a re-roll.

So do we need a completely new patch or do we want to wait for #547570 to land?

As for the concerns about a redirect for authenticated users, as someone who has run a few sites that allow *only* authenticated users, I can say that we had to write our own cookie checks anyways since obviously we wanted those users with cookies disabled to know why they were not able to login. I'd imagine this is pretty true across most sites that require authenticated logins (especially government ones trying to meet 508 guidelines).

So, +1 to the double-redirect, the performance hit should be worth the trade-off.

Looking through the session code, it seems that as long as $_SESSION is left empty we should still be able to set other cookies without breaking caching (even reverse proxies with vary-cookie should be OK). If we did this wherever the user form is displayed it seems this might be the most simple option (compared to wiring in an additional redirect). The only downside I can think of is the extra few bytes per-page for all users. We could reduce the latter by using a short cookie name/value. We could even set "has_js" to 0 in php (if it is not yet set), although I think that is more complex since we need to consider the JS interaction.

Note that if we go the double redirect option, it is actually the second redirect (rather than the first) that checks for a cookie. The first redirect would process the login, then redirect to a second page that would check for cookies and direct the user on to their final destination or set a message and send them back (to their original URL, ideally, although we might need to figure in some "origin" querystring parameter for that, I think, since we are no longer in the form processing cycle). Here is the page flow for the double-redirect option.

- Load 1: /node/123 - we can't add a cookie here (without breaking caching) because we don't know if the user will actually use the login block, or just click away
- Load 2: user submits login form - we process the login then redirect them on to a cookie check page - we can add a cookie here (and the login process does already, of course)
- Load 3: user goes to /user/checkcookie?destination=user&origin=node/123, checks for the session cookie and redirects as needed.
- Load 4: user back at node/123 if no cookie, or at user if cookies enabled

Of the 2, I think my preference is for adding a small cookie with login forms, but this is primarily because it feels simpler, rather than performance.

Adding issue summary tag. Also, I'm not convinced #791004: Installer does not warn the user that cookies must be enabled with the correct cookie domain (and fails when they aren't) isn't a duplicate.

#791004: Installer does not warn the user that cookies must be enabled with the correct cookie domain (and fails when they aren't) is not a duplicate. It deals only with the installer.

We need a warning to regular user (on an already installed Drupal site) that they need to enable cookies to be able to log in on the site.

Right now nothing is displayed if cookies and not enabled and the user have not clues as to why.

Adding Needs tests tag.

Added more information in the issue summary.

I am posting here to echo the others -- how unbelievable is it that Drupal, one of the (allegedly) most mature and complete CMS solutions available, doesn't have a fix for such an obvious, disturbing (and ancient!) UX issue?

@Kazanir: It means that no one was able to write the test required for this patch.

@xmacinfo - Since this bug is 10 years old and is -- I think we all agree -- quite a serious UX issue, I would propose that we remove the Needs tests flag and re-roll 114 for testing by the community. If it turns out in the future that a better solution than 114 exists, it can always be proposed in new issue.

#7: no-cookie.patch queued for re-testing.

Given that this is a common need that's been addressed in many other PHP applications but not in Drupal, we'd do well to look around. What have Joomla and Wordpress done? Anyone who's following this issue able to take on that research and report back?

nedjo raises a good point. Another place to look would be Symfony. ;) Isn't solving stuff like this "for free" part of the promise of having imported so much Symfony code into Drupal core?

If the missing tests are the problem, then looking around in other projects won't help at all. Technically this is solved, it's a people problem.

Looking in Symfony is certainly a good idea for D8, but it should go into a new issue IMO, because it won't work for D7. This issue here has the code to fix the problem in D7. We could have committed this years ago -- gasp, without tests -- and have it well-tested in the field by now.

I know what our standard procedures are, but in the tenth (!) year of this embarrassing issue, it's pretty obvious that our standard procedures are a hindrance in this case rather than a help. I've come to the conclusion that we lack the will to fix this. Looking around in other projects is just another way to derail this even further. (Not picking on you, nedjo, I know you've worked to bring this forward in the past.)

Clarified that the patch is too old.

Recategorizing as a bug, since login is broken without cookies. The priority reflects the fact that the bug presents significant usability issues.

D8 maintainer catch has been responsive in this issue and there's every reason to believe we could get this in for D8, but the patch is not yet ready. I've had a stab at updating the summary with remaining tasks.

I took a quick look at Wordpress, and it seems to me they do something similar to Owen Barton's first suggestion in #145 (set a cookie when the form is displayed and check it when the form is submitted).

However:

Looking through the session code, it seems that as long as $_SESSION is left empty we should still be able to set other cookies without breaking caching (even reverse proxies with vary-cookie should be OK).

Don't a lot of Varnish configurations strip cookies they don't recognize, and bypass caching entirely if there are any cookies left over? I'm looking, for example, at http://www.lullabot.com/articles/varnish-multiple-web-servers-drupal :

  # Remove all cookies that Drupal doesn't need to know about. ANY remaining
  # cookie will cause the request to pass-through to Apache.
...
  set req.http.Cookie = regsuball(req.http.Cookie, ";(SESS[a-z0-9]+|NO_CACHE)=", "; \1=");

If I understand that correctly, that means people would need to change their Varnish configurations for this approach to work at all, and if they do, suddenly a whole bunch of pages won't be cached anymore. (On the other hand, one could make the argument that if a site wants this behavior, that's something they'll have to live with.)

In terms of Drupal 7 backport, a new cookie would also break things for people using a "blacklist" approach to cookies in Varnish (rather than a whitelist approach) although I suppose those people should already be getting used to things breaking anyway...

In any case, here is a test. This was kind of a pain to figure out how to write.

It includes a rerolled version of #114; this should at least serve the purpose of verifying that the test behaves as expected.

Those two failures look like they're specific to the solution here (probably code that expects a specific URL after logging in and therefore gets confused by the new ?state=loggedin). So not worth fixing unless this is the actual solution we're going with.

Add issue summary.

@David_Rothstein: thanks, it's great to have a test in place!

I've updated the summary with details from the Wordpress approach.

In general I lean towards the cookie setting and checking approach. It has the advantage of being outside the critical path, it's explicit (testing to see if we have a logged in user feels hackish), it could be used elsewhere, e.g., at install time, and as Owen suggested it feels simpler than a double redirect.

But, yes, interference with Varnish or other cache solutions is an issue. Currently we set various cookies only in response to user actions (form submissions, logins, etc.), see http://api.drupal.org/api/drupal/modules!user!user.module/function/calls.... But a login form is often presented on all pages of a site.

Looking again to Wordpress, some sys admins explicity skip the wordpress_test_cookie in Varnish config, see http://kaanon.com/blog/work/making-wordpress-shine-varnish-caching-syste.... Presumably we'd see the same with Drupal, meaning that this fix would be missing from Varnish-served sites.

Re-roll. I support this approach.

Thanks for rerolling. hook_init() is gone, https://drupal.org/node/2013014, so the patch would need to use a different approach.

Fair enough.

How's this for proof-of-concept? I kind of hijacked the MaintenenceModeSubscriber, so we'd probably have to move the code to its own Subscriber or rename that subscriber, but at least it seems to fix the issue for me. Test might need to be tweaked though.

Cannot be reviewed until the patch is rerolled.

I set it to needs review because I needed input about whether the method was a valid one. It gives the correct result in manual testing (though the test needs to be tweaked, which I couldn't figure out either).

Update summary with test info and example from Wordpress.

After 11 years of surviving with the bug in place, I think we can safely downgrade this to Normal.

I was convinced in chat to put this back to Major.

For context, the reasoning I gave for this being Major is similar to #143. https://drupal.org/node/45111 defines major bugs as ones that have significant repercussions as opposed to normal bugs that are isolated to one piece of functionality. Since the inability to log in affects the use of the entire software, that makes this major by that definition. However, it's not critical, since it affects a tiny percentage of users and has a workaround (enable cookies).

The patch here has tests, removing tag.

Re-rolling the patch, let's waste some test bots time.

And for those wandering.. Yes, this is a 12-year old major bug, first detected in Drupal 4.2 or 4.3 (not sure exactly...).

Test bots are go!

Taking a look on the failures.

There are two methods (\Drupal\system\Tests\Session\SessionHttpsTest::loginHttp() and \Drupal\system\Tests\Session\SessionHttpsTest::loginHttps()) which needed some improvements to support recently added ?state=loggedin query suffix to the login process from comment #177.

I changed them by passing query parameters separately to \Drupal\simpletest\WebTestBase::drupalGet() call:

    // Follow the location header.
    $path = $this->getPathFromLocationHeader(TRUE);
    $parsed_path = parse_url($path);
    $query = array();
    if (isset($parsed_path['query'])) {
      parse_str($parsed_path['query'], $query);
    }
    $this->drupalGet($this->httpsUrl($parsed_path['path']), array('query' => $query));

Since this adds new public functions and interface text, moving to 8.1.x.

Patch also needs updating for $GLOBALS['user'] removal.

Removed global $user and rerolled the patch. Made some minor coder corrections as well.

Sorry BigPipeTest.php, but you can't test for no redirects in a page where we actually redirect on purpose.

Looks like the test needs fixing then.

Sorry BigPipeTest.php, but you can't test for no redirects in a page where we actually redirect on purpose.

This analysis is not correct I'm afraid.

This patch is making changes that causes certain defaults/expectations to change. So, yes, BigPipeTest needs a very minor change: it needs to now expect that ?state=loggedin query argument. Fixed that.

(Also reverted the change in #188, that was the wrong change.)

But, more importantly, this patch uncovers a bug in the redirect.destination service. Because BigPipe calls this:

\Drupal::service('redirect.destination')->getAsArray()])->toString()

… but it returns /user/1, whereas it should return /user/1?state=loggedin.

Fortunately a simple fix! :)

This will also need to update \Drupal\Tests\Core\Routing\RedirectDestinationTest, because that unit test clearly is not yet testing this. Hence Needs tests.

And since I touched this patch now anyway, here's a review:

1. +++ b/core/lib/Drupal/Core/EventSubscriber/MaintenanceModeSubscriber.php
   @@ -140,6 +140,24 @@ protected function getSiteMaintenanceMessage() {
   +    if (isset($_GET['state']) && $_GET['state'] == 'loggedin' && !$this->account->isAuthenticated()) {
   

   This should not use $_GET. It should use $event->getRequest()->query->get('state') === 'loggedin'.

2. +++ b/core/lib/Drupal/Core/EventSubscriber/MaintenanceModeSubscriber.php
   @@ -140,6 +140,24 @@ protected function getSiteMaintenanceMessage() {
   +      $domain = ini_get('session.cookie_domain') ? ltrim(ini_get('session.cookie_domain'), '.') : $_SERVER['HTTP_HOST'];
   

   This should also use the Request object.

3. +++ b/core/modules/user/src/Tests/UserBlocksTest.php
   @@ -73,7 +73,8 @@ function testUserLoginBlock() {
   +    $url = \Drupal::url('user.admin_permissions', [], ['absolute' => TRUE, 'query' => ['state' => 'loggedin']]);
   

   Should use Url::fromRoute().

Failures in RedirectDestinationTest… so that was testing this stuff, but with the wrong expectations!

This patch addresses the problems identified by Wim in his review (#193).

It does not fix the failing tests, but I'm setting it to review to make sure I didn't break other tests.

#196's interdiff looks great! Next: fixing RedirectDestinationTest, and potentially adding missing test coverage there :)