First off, I like this module because it is a very lean solution to a not so easy problem. However I think I've found a little flaw in the way this module tries to tell whether a user has ticked the box upon login.

Consider this example:

  1. User logs into WS from home-computer, and ticks the remember-me box.
    → hook_user_login → $user->data['remember_me'] = TRUE
  2. Over at his friends place, he likes to check for new messages on WS and logs in with the remember-me checkbox off
    → hook_user_login → $user->data['remember_me'] = FALSE
  3. Back at his computer at home he visits WS again
    → hook_init → _remember_me_set_lifetime(0);
  4. The next time he opens the browser and visits WS from his computer he has to login again because the cookie lifetime has been cleared during the previous visit.

This happens because the remember me module stores the setting per user instead of per session. Patch will follow.

CommentFileSizeAuthor
#13 wo_patch.png69.28 KBdwillems
#13 with_patch.png56.44 KBdwillems
#5 0001-Use-_SESSION-instead-of-user-data-for-remember_me-fl.patch2.76 KBznerol
#5 0002-Fix-cookie-lifetime-during-login-and-not-on-subseque.patch3.65 KBznerol
#2 2082285-2-store-flag-in-session-instead-of-in-user-data.patch5.75 KBznerol
#1 2082285-store-flag-in-session-instead-of-in-user-data.patch1.86 KBznerol

Comments

znerol’s picture

znerol commented
Status: Active » Needs review
StatusFileSize
new 2082285-store-flag-in-session-instead-of-in-user-data.patch1.86 KB
znerol’s picture

znerol commented
StatusFileSize
new 2082285-2-store-flag-in-session-instead-of-in-user-data.patch5.75 KB

Okay, here is an improved attempt. In this version we even get rid of the _remember_me_set_lifetime function which relied on session-reinitialization in order to influence session cookie lifetime.

The new approach is based on the observation, that we only have to make sure that ini_set('session.cookie_lifetime', X); is called before drupal_session_regenerate() when logging in. We ensure this by injecting a #submit-hook into the login form which is called before user_login_submit().

Because I was touching hook_form_alter anyway I cleaned that up a little bit.

nickl’s picture

nickl commented

Nice can of worms you've opened up here. In all the time we've had remember_me you are the first to bring this issue up.
I will have to ponder this...

It is difficult to asses what replaces what exactly as the patch seems to be a complete rewrite.
This is where separate commits have us spoilt on GitHub.
I am all for less code make no mistake but have you still covered all your bases?

Some questions from an initial patch sweep:
Why did you choose 2 hook_form_FORM_ID_alter functions + helpor function over the current implementation of 1 hook_form_alter?
The changing of the submit handler seems a bit agressive is this really necessary?
How does this effect other types of logins (i.e. open_id/oAuth/etc)?
Why did you choose this approach over the hook_user_login?
I notice the tabindex has been removed is this functionality regained by the weights? In all forms?
Your patch now ignores the ini setting completely as configured in settings.php and assumes default is 0, why is that?
You are setting the cookie lifetime on every request now regardless of whether it already has the correct lifetime or not, are there no implications to this?
Is it also required to fix session cookie lifetime on submit even though init is handling this?
Does the submit handler happen before or after init?

Now what to do with all these worms? =)

znerol’s picture

znerol commented
Status: Needs review » Needs work

I assume that you are capable of reproducing the steps given in the issue description. Just use two different browsers (one for home and one for internet cafe), or open a private browsing session. Therefore I won't discuss the $_SESSION vs. $user->data case, it should be pretty obvious.

I also will leave out the cosmetic changes (in hook_form_alter). You are definitely right, atomic changes are safer and easier to understand.

Lets focus on the core functionality of this module. I'll start out with an analyze how the login-process in Drupal works.

User login with remember me disabled

  1. An anonymous user submits its credentials to the user_login or user_login_block form.
  2. Credentials are checked by all validators returned by user_login_default_validators
  3. When validation succeeds user_login_finalize is called by the submit-function
  4. From there drupal_session_regenerate is called (which is responsible for updating the $user variable and setting the session-cookie). The cookie lifetime is set here to whatever ini_get('session_cookie.lifetime'); returns.
  5. And finally hook_user_login is invoked
  6. After that the user gets redirected to its profile page on user/[uid]. For the further analysis it is important to keep in mind, that the initial POST request is closely followed by this GET request issued through a redirect.

Looking at the requests using Firefox and Life HTTP Headers (or Wireshark, or whatever network analyzer at hand), you'll notice that the session cookie is set in the response to the POST-request. On subsequent requests, the session cookie is not set anymore by the server. This is expected behavior.

User login with remember me enabled

With the Remember Me module enabled you have the exact same sequence of events until step 5. That means when hook_user_login is called it is actually too late to influence the cookie session lifetime - at least for the current request. The Set-Cookie header of the response to the POST request will have the cookie lifetime which is configured in settings.php (by default 2000000 which is a bit more than 23 days). However because the initial POST is followed immediately by a redirected GET request where the cookie-lifetime is fixed from within hook_init the initial (wrong) cookie lifetime does not stand out - at least one would not notice it by just looking at the page resource using Firebug / Page inspector tools.

Note that fixing the cookie-lifetime as implemented by the remember me module (see _remember_me_set_lifetime) means restarting the session and effectively sending a Set-Cookie header to the client. Don't take me wrong, but the implementation of this function made me quite a bit nervous.

When looking at the requests going forth and back between browser and server using a network analyzer, one will notice that the Set-Cookie header is not only set twice during login, but also on every subsequent request. I think that hook_init intents to avoid calling into _remember_me_set_lifetime when its not necessary, but it obviously fails.

The reason for the frequent calls to _remember_me_set_lifetime is that ini_get('session.cookie_lifetime') as called from hook_init always returns the value configured from within settings.php.

Takeaways

  • During a request, the only opportunity to influence session-cookie lifetime is before the session is started.
  • During the drupal login process, the session is started (or regenerated) before hook_user_login is called.
  • On a subsequent request, it is not possible to determine the exact session-cookie parameters which were in use when the session was opened in a prior request. It is impossible to figure out whether the cookie-lifetime has to be corrected or not.

Technically it might even be unnecessary to alter the session.cookie_lifetime parameter from within hook_init. It cannot influence session-start anyway. However there might be third party code setting cookies with the same parameters as session cookies, therefore it is probably better to keep session.cookie_lifetime in sync with the user-session preference.

Answers to your questions

Why did you choose 2 hook_form_FORM_ID_alter functions + helpor function over the current implementation of 1 hook_form_alter?

Cosmetics, has no relevance.

The changing of the submit handler seems a bit agressive is this really necessary?

Yes, see above.

How does this effect other types of logins (i.e. open_id/oAuth/etc)?

I don't know.

Why did you choose this approach over the hook_user_login?

Because hook_user_login is called too late, see above.

I notice the tabindex has been removed is this functionality regained by the weights? In all forms?

Cosmetics, has no relevance.

Your patch now ignores the ini setting completely as configured in settings.php and assumes default is 0, why is that?

Ah, I obviously misunderstood the intention of the remember_me_managed variable. I will address that when rerolling the patch(es).

You are setting the cookie lifetime on every request now regardless of whether it already has the correct lifetime or not, are there no implications to this?

Changing the setting only has an effect when a function relying on it is called afterwards. Sessions are started before, except when sessions are regenerated afterwards (like during user login).

Is it also required to fix session cookie lifetime on submit even though init is handling this?

If we want to influence the Set-Cookie header in the first POST response, we need to inject our own submit handler before the built-in one. Also changing the value during init does not change the cookie parameters of a cookie set during a previous response.

Does the submit handler happen before or after init?

The sequence is at follows: hook_init (no user logged in), submit-handler (change user, set cookie), hook_user_login (too late to influence cookie lifetime).

I will attach atomic patches in a follow up.

znerol’s picture

znerol commented
Status: Needs work » Needs review
StatusFileSize
new 0002-Fix-cookie-lifetime-during-login-and-not-on-subseque.patch3.65 KB
new 0001-Use-_SESSION-instead-of-user-data-for-remember_me-fl.patch2.76 KB

First patch: Use $_SESSION instead of $user->data for remember_me flag

Second patch: Fix cookie lifetime during login and not on subsequent page requests

nickl’s picture

nickl commented

Some thoughts so far:

No need to change the session on login submit, this is followed by a redirect which can set the lifetime. The same functionality that allows you to be remembered in future.

Session cookie lifetime is the lifetime the browser saves the cookie for this is not the same as expired sessions on the server.

Since settings.php will always set the session. cookie_lifetime it makes sense that it will always first be incorrect and needs to be changed. This becomes especially important for not being remembered or cookie_lifetime = 0. At hook_init the session was already created hence the recreate (which is std php b.t.w and a good protection against man in the middle anyway so no need for concern) and unless we can hook in before that I don't see how we can avoid it. Drupal hooks are chained where as modifying the session handler functions is arguably not going to play nice with others. Is it enough to create the session once only? I still need to verify this.

znerol’s picture

znerol commented

Some thoughts so far:

Do you agree on having the flag in $user->data['remember_me'] is wrong? Then we could building up a solution from there.

nickl’s picture

nickl commented
Issue summary: View changes
Status: Needs review » Closed (cannot reproduce)

Don't take me wrong, but the implementation of this function made me quite a bit nervous.

When looking at the requests going forth and back between browser and server using a network analyzer, one will notice that the Set-Cookie header is not only set twice during login, but also on every subsequent request. I think that hook_init intents to avoid calling into _remember_me_set_lifetime when its not necessary, but it obviously fails.

Why does this make you nervous? In fact if nothing else changing the session id on every request is good protection against man in the middle attacks. There is nothing that says the session id should remain the same only that you must provide me with the correct id that matches the current state we are keeping this request on your next request. After that we can safely change the session id again and again and again.

I cannot reproduce the cross location (different browser) remembered session as the two different browsers have different session ids and they are not able to log into each others sessions.

dwillems’s picture

dwillems commented

I see that this has been closed but I'd like to make an appeal. Is there any chance of getting the $_SESSION patch committed alone? I have implemented it on a site currently in development and no issues have come up in testing. We've seen a big performance boost since it removes a call to user_save which otherwise adds a lot of unnecessary overhead to the login operation, especially when fields are added to users.

If moving the flag from $user->data is undesirable, an alternate workaround could be to update the data field by making a separate query, similar to how user_login_finalize updates the login timestamp without calling user_save.

Thanks!

alanmackenzie’s picture

alanmackenzie commented

I would like to echo dwillems comments.

I have a Drupal site with 1.2 M users and a fair number of fields related to users so user_save() was causing us issues. To me using $_SESSION sounds like a sensible idea.

nickl’s picture

nickl commented

The drupal hook system is specifically there for module developers to use. This allows for compatibility between different modules and other customisations. If you wanted the data in $_SESSION for example you could catch this with presave.

Without tangible benchmark figures it is not possible to assess why this is a performance issue, please present your findings. This will only effect the user authentication process and has no bearing on general performance. Do you have a problem with the time it takes to authenticate?

If there is an issue with using user_save then I doubt this is where we should address the problem and perhaps it should be taken up with fields or core. Just a thought....

Bypassing the hooks is no way to play nice with others and we wouldn't want that, do we?

nickl’s picture

nickl commented

@dwillems I don't see any patch that only changes the storage of the remember_me token to consider.

The current implementation works with many different ways one may attempt to log into drupal which I fear removing the other functionality may have broken, unless you can confirm otherwise.

dwillems’s picture

dwillems commented
StatusFileSize
new with_patch.png56.44 KB
new wo_patch.png69.28 KB

@nickl,

100% agree on your point about playing nice and not bypassing hooks (I have hundreds implemented in the project implementing remember_me).

Just to clarify, the point I am trying to make is to avoid the costly user_save, not necessarily to put the flag in $_SESSION. This could be done a number of different ways. Another solution (aside from using $_SESSION or a query to update the data field) could be to create a new table for remember_me to store the per-user flag and creating new hooks if needed for other modules to react to it.

The issue here is that the behavior of the login operation is being altered by remember_me to call user_save() when it wouldn't otherwise be called and that is a potentially very expensive call. On a small site with relatively low traffic and little or few fields on the user object this may have a minimal performance hit, but per your request I've added a screenshot comparing the profiling data on a site with many user fields. Specifically in our site, we see the patched version of remember_me_user_login taking a fraction of a millisecond to run, but just over 2.5 seconds when it calls user_save. This is not necessarily an issue with the user or fields module, it's just the nature of saving entities to the database - with many fields come many INSERT queries.

The patch I am referring to was submitted by @znerol in #5:
https://drupal.org/files/0001-Use-_SESSION-instead-of-user-data-for-reme...

alanmackenzie’s picture

alanmackenzie commented

Do you have a problem with the time it takes to authenticate?

Yes.

Without tangible benchmark figures it is not possible to assess why this is a performance issue, please present your findings. This will only effect the user authentication process and has no bearing on general performance.

Thanks to dwillems for his numbers. I do not think anyone is suggesting that remember_me is causing site wide load issues. A slow authentication process gives the perception of a slow site, unfortunately humans are not quantitative and don't measure things precisely.

The drupal hook system is specifically there for module developers to use. This allows for compatibility between different modules and other customisations. If you wanted the data in $_SESSION for example you could catch this with presave.

If there is an issue with using user_save then I doubt this is where we should address the problem and perhaps it should be taken up with fields or core. Just a thought....

Bypassing the hooks is no way to play nice with others and we wouldn't want that, do we?

A compromise solution would be to call user_save() in a shutdown function which keep the API compatibility while improving the user experience. This pattern is used by modules such as workbench moderation.

I have implemented something very similar in one of my own modules, views usage audit. If you are happy with this solution I am more than willing to write a patch.