Posted by anders on September 20, 2003 at 6:30am
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | base system |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | cookie |
Issue Summary
since drupal relies on cookies for logging in, not warning the user if the cookie is not allowed is plain old stupidity
Comments
#1
Making this a feature request instead of critical bug report. Tss.
#2
#3
#4
Marking as duplicate of http://drupal.org/node/137678
#5
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.
#6
Offering a user to log-in in a situation where the login will automatically fail can safely be considered a bug.
#7
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.
#8
Also http://drupal.org/node/137678
#9
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.
#10
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.
#11
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.
#12
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?
#13
yeah #10 looks like the wrong patch.
#14
#10 looks like the completely wrong file got uploaded. Yoohoo, CHX...
#15
I think we know that, by now.
What I would like to know, is the answer to #12, so that we can move ahead...
#16
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.
#17
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.
#18
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
#19
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?
#20
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:
<?phpglobal $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;
?>
#21
Applied nedjo's text and rerolled for HEAD.
Unfortunately, user/no_cookie gives me "Page not found."
#22
Thanks for the updated patch.
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.
#23
Ah, yes, clearing the cache has made it work. Thanks!
#24
This looks good to me. Tested, works great.
#25
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.
#26
Ok, I've changed the title to 'Please Enable Cookies!'
#27
Slightly changed wording.
#28
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.
#29
- 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.
#30
#31
subscribe
#32
#33
I suggest that user_no_cookie() should return theme('user_no_cookie') so sites can override this hard-coded text.
#34
#35
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.
#36
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.
#37
The cookie should be deleted after logging in.
#40
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.
#41
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...
#42
OK, let's fix chx's concern at least first.
#43
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 inuser_login_submit.#44
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.
#45
OK then. it's still better than the older ones because we now patch both places where anonymous user can happen.
#46
Doh.
#47
Adjusted user_no_cookie function to use the relevant domain.
#48
Added domains to the setcookie .
#49
Yes, now it works perfectly, including deleting the cookie upon log-in. Thanks!
#50
I'm not really sure this is worth fixing. Looks a bit crufty to me.
#51
This is a huge useability improvement, and makes Help Desk workers happier :)
++ 1 for the patch.
#52
+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.
#53
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...
#54
+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.
#55
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()
<?phpif (count($_COOKIE) == 0)
?>
<?phpif (!isset($_COOKIE['drupal_login_check']))
?>
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.)
#56
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?
#57
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.
#58
I agree #48 is more reliable and therefore superior.
#59
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.
#60
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.
#61
@soxofaan: see #36...
(this problem may apply to the has_js cookie, too!)
#62
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.
#63
#64
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?
#65
+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.
#66
* The patch does no longer apply.
* Is this something we can write a test for?
#67
* The patch does no longer apply.
* Is this something we can write a test for?
#68
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.
#69
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.
#70
The last submitted patch failed testing.
#71
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...
#72
@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?
#73
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!
#74
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.
#75
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.
#76
With patch...
#77
The last submitted patch failed testing.
#78
I ran tests locally and didn't get the reported error. Setting back to 'code needs review'.
#79
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....".
#80
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.
#81
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.
#82
The last submitted patch failed testing.
#83
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
#84
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.
#85
#86
Fixed domain name display from nedjo's patch in comment #76
#87
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!
#88
Another example:
A simple warning is given here (again, visit with cookies turned off).
http://www.autism-society.org/site/PageServer?pagename=asa_home
#89
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?
#90
Holy Batman, my patch is not in yet????
#91
Batman is not holy. :-)
I guess we should grab webchick today for the code sprint. I've have not seen her yet this morning.
#92
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.
#93
I think it would be best for you to create two new issues for:
:-)
#94
hook_requirements() of the user module?
new hook?
#95
We need tests for this. I check with cwgordon7 and he said it should be possible to override curlConnect() in your test case.
#96
@webchick
any updates weither this is testable or not?
#97
I relayed what I was told. There's a testing sprint going on this weekend, so plenty of people to ask in #drupal. :)
#98
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.
#99
++ 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
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.
#100
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...
#101
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. :-)
#102
xmacinfo requested that failed test be re-tested.
#103
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',
)
#104
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
#105
+++ 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.
#106
The last submitted patch failed testing.
#107
Sub.
#108
#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.
#109
The last submitted patch, cookie_validate_2946_reroll.patch, failed testing.
#110
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.
#111
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)
#112
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.
#113
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.
#114
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?
#115
#116
The last submitted patch, cookie_validate_2946_114.patch, failed testing.
#117
Still looking for review on the general approach
#118
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.
#119
subscribing
#120
@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?
#121
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.
#122
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.
#123
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.
#124
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.
#125
The tests are failing, so the patch still needs some work...
#126
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.
#127
Sorry. Issues that have been in Drupal since 2003 don't get to hold "critical" status.
#128
#129
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.
#130
Shhh, we're aiming for a world record here...
#131
Subscribe.
#132
Subscribing. Is #114 going to work with lazy session creation, or should we try for #124?
#133
#114: cookie_validate_2946_114.patch queued for re-testing.
#134
The last submitted patch, cookie_validate_2946_114.patch, failed testing.
#135
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 :^)
#136
@satchinsb: Please don't hijack this issue — create a new one against 6.17.
#137
subscribing...
#138
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?
#139
subscribe
#140