Feature #228
closedWarn users who have cookies turned off
90%
Description
You can't log in properly to the site if you have cookies turned off. We should do something to let users know if they have cookies off.
The solution proposed for now is to check on the login page that they can accept cookies and give them a warning message if they can't. Perhaps even not show the login form.
This has to first be tested for feasibility.
Related issues
Updated by Boone Gorges over 13 years ago
Have you had any more thoughts about this, Chris?
I'm not entirely sure how the entire cookie loading process works, but it seems like we should be able to do the following things in quick succession before a page loads: attempt to set a cookie, check to see if it's set, and display a warning if not. You want to whip something up (maybe hooked to plugins_loaded) that is a proof of concept?
I'm thinking we'd only want to load it on the login page, since we don't want to display the message to people who are just browsing the site.
Updated by Boone Gorges over 13 years ago
- Category name set to WordPress (misc)
- Target version set to Future release
Updated by Boone Gorges almost 12 years ago
- Assignee changed from Chris Stein to Dominic Giglio
Updated by Dominic Giglio almost 12 years ago
Boone, Chris,
I added some javascript to cac-custom-js.js to test for cookie support and display an error message on the login page if they aren't enabled. It currently only works if a user visits the login page. The code needs to be updated to work with the dropdown login form in the toolbar (buddybar?).
Boone,
Where does that login form come from? I've been looking through the theme and the functions.php / bp-custom.php files and I can't seem to find it. If you could point me in the direction of that form I should be able to get this closed out rather quickly.
The code is in a new topic branch titled cookie-test
Commit: https://github.com/castiron/cac/commit/1c0aff0fd10cdde8217ab3af68e8c2554210d514
Updated by Boone Gorges almost 12 years ago
- Status changed from New to Assigned
- % Done changed from 0 to 50
Hi Dom. This approach looks mostly fine. Two comments:
1) The toolbar login appears on every page of the entire network, so a window.location.pathname check won't work. Instead, just check for the presence of the '#wp-admin-bar-bp-login' element, and add the cookie message there. Use wp-content/plugins/cac-bp-admin-bar-mods/js/admin-bar.js for this logic (it contains the rest of the JS for the toolbar, and loads on every page of the network)
2) On wp-login.php page, instead of prepending a generic paragraph element, append a div with id 'login_error'. Then you'll inherit WP's styling. Attempt a login with the wrong password to see what the markup looks like and where it should go.
Updated by Dominic Giglio almost 12 years ago
- % Done changed from 50 to 90
So after a little more testing I've decided that a javascript solution is not the right way to go on this issue. wp-login.php already does cookie checking. The problem is that the "no cookie" error it produces does not show up until a user tries to login. If cookies are disabled, WordPress then displays an nice message telling the user they need to enable cookies and even links to an explanation page.
This means, with my previous javascipt solution, TWO "no cookie" errors were being displayed after a user tried to login.
Instead, I've added a small function to the bottom of functions.php that checks to see if the WordPress test_cookie was successfully set, and if not (which means cookies are disabled) the function adds it. To make sure I'm not doubling up the error messages, the function also checks to see if WordPress has already set the error.
Here's a flow of what happens when a user tries to login (with cookies disabled) BEFORE adding the new function to functions.php:
1.) The login page displays as "normal", no error about cookies being disabled
2.) if you click login with no user/pass the form is re-rendered but WITH the error about no cookies
3.) if you enter an invalid user/pass WordPress displays a username/password error AND the no cookie error
4.) if you enter a VALID username but invalid password, the output is similar to #3
Numbers 2, 3 and 4 are the reason I have to check for the error in the code I've added to functions.php. wp-login.php sets a test_cookie on line 375 and then checks for that cookie on line 642. That check is inside a switch statement that determines what type of login is happening. This is why the error does not show up when you first visit the login page, that 'case' statement hasn't been executed yet and won't be until the user clicks the login button.
The new function gets attached to the login_head action so it is evaluated when the login page is rendered, thereby displaying the no cookie error before a user has a chance to try and login.
Boone, there's only one thing I need help with. The login drop-down works fine with this new functionality in every login scenario except one. Here's the flow for using the drop-down when cookies are disabled AFTER adding the new function to functions.php:
1.) clicking the login button with NO user/pass redirects to wp-login.php, which displays the no cookies error message
2.) entering an invalid user/pass redirects to wp-login.php, displaying the same errors as #3 above
3.) entering a valid username and invalid password redirects to wp-login.php, displaying the same errors as #4 above
4.) if the user enters a valid user/pass the home page is re-rendered with NO errors. Which is confusing for a user
Number 4 is what I need help with. Can we change that form to redirect to wp-login.php when cookies are disabled? How can I capture that failure and properly redirect? Is there a hidden "login failed" input that I can add to handle this situation?
Let me know what you think of this solution, it's pretty simple and I think it covers 95% of what this issue needs to be resolved.
Commit: https://github.com/castiron/cac/commit/d5aa55135d6a903ce872153599fdfed9deca01fa
Updated by Boone Gorges almost 12 years ago
This looks good, Dom.
How can I capture that failure and properly redirect? Is there a hidden "login failed" input that I can add to handle this situation?
I can't see an obvious place to do this, if you're hooking to 'login_head'. The alternative is to find a filter that appears only when the creds were accepted, and to hook something to that filter to check whether the login was successful. Like https://github.com/castiron/cac/commit/d250f67076c80423e32311478614f60bcdec540e This seems like it works. Give it a test run, and maybe write some better inline docs than I've written (none :) )
Updated by Boone Gorges almost 12 years ago
Also, I forgot to mention that bp-nelo/functions.php is not loaded on subdomain sites. You may consider adding this to an existing mu-plugins file, such as the generic cac-functions.php that exists on the master branch.
Updated by Dominic Giglio almost 12 years ago
Awesome, gonna play around with that new filter and move that code to cac-functions later today. Will let you know when I've pushed it up.
Updated by Chris Stein almost 12 years ago
Thanks for the work on this. The error message about the cookies is fine, especially with the link to help them with enabling cookies. You may consider changing the link to, http://support.google.com/accounts/bin/answer.py?hl=en&answer=61416, as the one given doesn't directly explain how to enable cookies and might appear at first glance to be more about Google accounts.
Updated by Dominic Giglio almost 12 years ago
Boone,
Gonna try and have some new commits on this issue later today. It's so close to done I just wanna get it closed out.
Chris,
I would change the link if I could, but it's in WordPress core, so I think we should leave it the way it is for now.
Updated by Dominic Giglio over 11 years ago
Boone,
I've taken my function and your function and moved them both to mu-plugins/cac-functions.php. I also added some comments as you requested, and shortened up your function a little. Together they both seem to resolve this issue. I've tested with cookies off on both the cac home page and some subdomains. I've also tested by going directly to the login page and from the login dropdown in the buddybar. Everything looks good. I also tested with cookies on to make sure there were no unexpected side effects from the code during "normal" login operations.
Chris,
I wasn't totally accurate in my last response. What this code does is check to see if WordPress has already set its internal no cookies error, and if not, it sets it manually. The issue is that WordPress does not check for that error until someone tries to login. So I can use the URL you provided but it will not be the link in that error message in all cases. Basically the only time the manual error is displayed is when a user goes directly to the login page. In all other circumstances WordPress' core error will be rendered. If we use your link then we'll have functionality that displays the same error message but with a different link depending on how the user got to the login page.
Would you (or Boone) please let me know if you'd like me to update the link to your version? Once that's ironed out I think we can close this issue.
Commit: https://github.com/castiron/cac/commit/5eaad5849ab2b8ea61c772356532feb810b7f214
Updated by Boone Gorges over 11 years ago
- Target version changed from Future release to 1.4
Hi Dom. Thanks for the update.
I made some changes at https://github.com/castiron/cac/commit/05bc4fae6086bcc1ce9ee56b63f5cfbd0c13294e. First, I wrote a little more explanation in the docs, so that we don't look back a year from now and wonder what the hell this is for. Second, I refactored the way that values are returned. You have to return a value back to the login_redirect filter no matter what happens, or you'll get redirected into the ether. You were only returning when the user wasn't logged in.
Have a look over the commit. If it looks good, let's call this done.
Updated by Dominic Giglio over 11 years ago
I like the expanded comments. There's something I don't understand about this new logic though.
cac_no_cookie_login_redirect() now FIRST sets $r to wp_login_url() and THEN checks if the user is not logged in and (again) sets $r to wp_login_url() and finally returns $r. Won't this function now always return $r with the value of wp_login_url()? Is that what we want it to do here? If the function sets $r to wp_login_url() as its first action, why even bother checking if the user is logged in?
I have a feeling that you meant to remove that first assignment, but I don't want to change it if there's some reason for it to be there.
I think this can be closed as soon as we figure out if that first assignment is really supposed to be there or not.
Updated by Boone Gorges over 11 years ago
- Status changed from Assigned to Resolved
Correct. https://github.com/castiron/cac/commit/f9663767d77ef17c736fecadefb6bb04f8f66b12
Thanks for your help with this one.
Updated by Boone Gorges over 11 years ago
- Status changed from Resolved to Assigned
- Assignee changed from Dominic Giglio to Boone Gorges
I'm reopening this ticket and assigning to myself. My login_redirect trick is breaking regular logins in certain cases.
Updated by Dominic Giglio over 11 years ago
I've added myself as a watcher. Is there something I can do here so you can focus on the 1.4 deadline?
Updated by Boone Gorges over 11 years ago
Thanks, Dom. Please feel free to have a look. My hack the first time around was to change login_redirect based on whether the current user is logged in. But this is too generous a condition - it catches normal logins too, so that you always have to log in twice. I'm guessing something can be done by looking at the referer URI or something like that, but I'm not sure. If you have time to look, please do, but it's my mess, so don't feel obligated to clean it up :)
Updated by Dominic Giglio over 11 years ago
It's a small mess and I'm already familiar with this issue. You focus on the 1.4 deadline and I'll look into this.
Updated by Boone Gorges over 11 years ago
- Assignee changed from Boone Gorges to Dominic Giglio
Hey Dom. I just had a moment of downtime, so I had a look at this. I think the (somewhat hackish) answer is to keep my filter in place, but instead of checking is_user_logged_in(), check to see whether $_COOKIE is empty - because if $_COOKIE doesn't have anything in it, it means that (at the very least) the wordpress_test_cookie has failed, implying very strongly that cookies are disabled. I don't have time to test this thoroughly (successful logins with cookies on/off, failed logins with cookies on/off, etc) - could you have a look and see if my hunch is right?
Updated by Dominic Giglio over 11 years ago
Your hunch seems to be correct, by checking TEST_COOKIE the redirect appears to work the way we want it to. I tested with cookies on and off and logged in from both the login page and the buddybar. Let me know if it works on your end.
Commit: https://github.com/castiron/cac/commit/b1cde8d3d8cff3d8fe07f659a89761b74c32816b
Updated by Boone Gorges over 11 years ago
- Status changed from Assigned to Resolved
Change looks great. Thanks, Dom. Added to master in https://github.com/castiron/cac/commit/770aac99d036200ffd5bbb79697384df085f4f74
Updated by Boone Gorges over 11 years ago
- Status changed from Resolved to Assigned
- Priority name changed from Normal to Low
- Target version changed from 1.4 to 1.5
- Severity set to Low impact
I'm reopening this ticket and punting it. The problem with the current method is that it attempts to check for the existence of the test cookie on the same page load as when that test cookie is set. But cookies set in PHP are not available until the next page load; see http://www.php.net/manual/en/function.setcookie.php. This means that people who visit wp-login.php directly, without already having a wordpress_test_cookie set, (1) get the cookie set, but (2) also see the "cookies not supported" message on that page load, because $_COOKIE[TEST_COOKIE] is empty until the next refresh.
If we really need to do more realtime cookie checking, we're going to have to rely on asynchronous methods. Ie, when loading wp-login.php, the test cookie will be set; immediately afterwards (at login_init), send an AJAX request to see whether the cookie was successfull set.
For the time being, it's less of a problem to force no-cookie users to attempt their login twice (the original problem raised in the ticket) than it is to force nearly all users to log in twice, as is happening now. So I'm removing our logic, and we can revisit down the line.
https://github.com/castiron/cac/commit/141cdd8e1cb9c77f8563e62cc55d215b3b317035
Updated by Dominic Giglio over 11 years ago
There's gotta be a simpler way to address this issue. I've marked #2193 as a duplicate incase we need to reference any discussion from that thread.
I'll see what else I can come up with to resolve this issue as I close out more 1.4.9/1.4.10 issues.
Updated by Dominic Giglio over 11 years ago
Just wanted to post a quick update here before I forgot.
I'm in the new Fitterman building right now between classes. I'm on the BMCC-FH wireless.
I just experienced the weird "double login" error we've been hearing about from some of our users. My first attempt to login redirected me to wp-login and warned me that I must enable cookies to login to the site. My second attempt let me in with no errors.
I'll do some more testing before my next class to see if I can uncover any potential avenues that could help to address these annoying login issues.
Updated by Boone Gorges over 10 years ago
- Status changed from Assigned to Rejected
- Target version deleted (
1.5)
I've reread this thread, and I think in retrospect that there's really no bug to be fixed here. Users with cookies turned off will get the WP error message after the first login attempt fails. That seems good enough to me, especially given that there is no easy way to make the checks better (as evidenced by the last three years of breakage). So I'm going to close the ticket.
Updated by Chris Stein over 10 years ago
Boone, that sounds fine however, I turned off cookies in Chrome and tried to log in and it just did nothing. There was no WP error message.
I don't think this is worth spending time on right now (there are probably very few users who turn cookies off and those who do probably know enough to turn them back on), but maybe we can keep it in the back of our minds and if we see someone has done something along these lines in the future we can look at creating a new ticket and revisiting.