WordPress.org

Make WordPress Core

Opened 7 months ago

Last modified 7 months ago

#46618 new enhancement

Change login behaviour to only set the test cookie when a user attempts to login instead of just on visiting the login page

Reported by: garrett-eclipse Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Login and Registration Keywords: 2nd-opinion
Focuses: javascript, privacy Cc:
PR Number:

Description

Hello,

I'm branching this from #44175 to account for the idea presented by @SergeyBiryukov;

I wonder if we could change the behavior to match the text, so that the test cookie is only set upon trying to log in, as that probably makes more sense. I'd be more comfortable with doing that in a major release though.

The change would be to move the logic for the test cookie to be triggered upon the login action rather than the login page visit.

Cheers

Change History (8)

#1 follow-up: @Clorith
7 months ago

If this were the case, do we even need the test cookie? You're trying to log in already and if it fails passes but you're still not logged in then cookies are a problem.

Personally, I'd prefer knowing that I can't login because cookies are disabled before I type in, and then send, my username and password.

#2 in reply to: ↑ 1 @garrett-eclipse
7 months ago

Replying to Clorith:

If this were the case, do we even need the test cookie? You're trying to log in already and if it fails passes but you're still not logged in then cookies are a problem.

Personally, I'd prefer knowing that I can't login because cookies are disabled before I type in, and then send, my username and password.

Hi @Clorith thanks for the feedback.

Taking a look at the existing login process it appears the cookie error occurs after login attempt and not before. I agree it would be useful to know before login that cookies are needed and that you may not have them enabled.

A potential other flow could be introduce a cookie checkbox similar to the one on comment forms disclaiming the cookie usage, checking the box would attempt to set the cookie via javascript or making a call to navigator.cookieEnabled and if that fails display the cookie error.

Thoughts?

#3 follow-up: @Clorith
7 months ago

If the check isn't done till after (I thought we did it earlier, so my apologies if that's not the case), we could just redo the logic to logging in, and if is_user_logged_in says "no" when a valid login is provided, we know the cookie isn't setting and can act accordingly.

Adding checkboxes to declare cookie consent on login pages adds complexity for the end user, and isn't required from my understanding, because it's implied that there exists an item to maintain your login session in such a scenario.

#4 in reply to: ↑ 3 @garrett-eclipse
7 months ago

  • Keywords needs-patch added

Replying to Clorith:

If the check isn't done till after (I thought we did it earlier, so my apologies if that's not the case), we could just redo the logic to logging in, and if is_user_logged_in says "no" when a valid login is provided, we know the cookie isn't setting and can act accordingly.

Adding checkboxes to declare cookie consent on login pages adds complexity for the end user, and isn't required from my understanding, because it's implied that there exists an item to maintain your login session in such a scenario.

Thanks @Clorith, I appreciate the input and agree that sounds like the patch forward here moving the login into the login process rather than pageload.

As to a cookie checkbox, I agree as well, the suggested policy text covers this quite well so no need to complicate things;

When you log in, we will also set up several cookies to save your login information and your screen display choices. Login cookies last for two days, and screen options cookies last for a year. If you select "Remember Me", your login will persist for two weeks. If you log out of your account, the login cookies will be removed.

If you edit or publish an article, an additional cookie will be saved in your browser. This cookie includes no personal data and simply indicates the post ID of the article you just edited. It expires after 1 day.

#5 @ocean90
7 months ago

  • Keywords close added

Hmm, the cookie needs to be set before otherwise it cannot be used for … testing. That's because $_COOKIE is only populated with the data during the initial page request.

This ticket was mentioned in Slack in #core-privacy by garrett-eclipse. View the logs.


7 months ago

This ticket was mentioned in Slack in #core-privacy by garrett-eclipse. View the logs.


7 months ago

#8 @garrett-eclipse
7 months ago

  • Keywords 2nd-opinion added; needs-patch close removed

Thanks @ocean90 I appreciate the feedback, you're correct that the current implementation requires the page load to save the test cookie. As @Clorith pointed out if the cookie is placed on page load it would be nice to indicate to the user prior to their login attempt that cookies are disabled, this would require a javascript check.

The main point that came out of raising it in the #core-privacy meeting was;

  • Although the cookie only sets a string and doesn't contain any PII (Personally Identifiable Information) it's existence can be used to identify users browsing behaviours and history.

Slack Reference - https://wordpress.slack.com/archives/core-privacy/p1554319956043300

Another issue raised by the cookie notice currently is it's often misleading as it flags in some cases even when the user has cookies enabled.
For instance - #44544

A potential improvement to the login behaviour could be to use Javascripts navigator.cookieEnabled in order to display the cookie error prior to login warning the user they need to enable cookies while avoiding a cookie, and upon login either attempt to determine cause and display an appropriate error or default to a generic error linking to support documentation that can elaborate on potential causes.

I understand the current behaviour requires the test cookie but if the suggestion above has any merit I'd be happy to pursue it further.

All the best
PS: My adding 2nd-opinion was added mostly to get your 2nd opinion on my suggestion and not to seek alternate feedback as I greatly appreciate that already provided.

Last edited 7 months ago by garrett-eclipse (previous) (diff)
Note: See TracTickets for help on using tickets.