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:

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()

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?

+1 to make something like this a core feature.

Another way to look at this might be to stop the user from logging in if the person doesn't have cookies enabled before they actually login. Or just a standard text message on the login page stating that cookies must be enabled to login.

Edit: rewrote this post a bit.

* 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.

The last submitted patch failed testing.

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...

The last submitted patch failed testing.

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?).

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.

The last submitted patch failed testing.

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 first rebuild does not happen
#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.