Decouple session from cookie based user authentication

Is someone working on this in Szeged? I'm happy to pick up the work of others or attempt a first patch. Won't have time until Sunday.

Is someone working on this in Szeged?

I will presumably have a first patch ready soon.

Ok, this is just an experiment. The goal here is to discover ways of extracting storage from the session handler. The storage handler should be as simple as possible, but at the same time should account for mixed mode ssl sessions.

I did not bother to inject the storage into the service container yet. This definitely needs some more thought.

@ParisLiakos: I am well aware of the work that has been done already, and I appreciate it very much. Nevertheless sometimes it can be helpful to rethink and explore a bit in different directions in order to deblock things.

Introducing SslAwareSessionHandlerInterface in this iteration. Session storage handlers implementing this interface automatically profit from SSL and SSL mixed-mode logic. Also removed the requirement to supply two session storage handlers for SSL sites.

10: 2228393-session-handler-extract-storage-10.diff queued for re-testing.

Needs work: I think we need to pass around the uid as part of a extendend MetadataBag instead of as a session attribute.

@znerol can you describe what that would buy us?

How does SslAwareSessionHandlerInterface impact #961508: Dual http/https session cookies interfere with drupal_valid_token()?

Re #14: I think that using session_id anywhere outside session handling is a bad idea. I left comment in #961508-272: Dual http/https session cookies interfere with drupal_valid_token().

I guess it makes sense to postpone this on #2238087: Rebase SessionManager onto Symfony NativeSessionStorage (?)

Back to active again.

still green

Related: #2345611: Load user entity in Cookie AuthenticationProvider instead of using manual queries

Uploading a patch that was derived from #8 and used in #2286971-73: Remove dependency of current_user on request and authentication manager but was afterwards separated again from that issue. The patch is rolled on top of #2286971: Remove dependency of current_user on request and authentication manager, while the do-not-test patch is the actual patch for this issue.

This is just a starting point.

Thought I was smart :) #2286971: Remove dependency of current_user on request and authentication manager is in now (yay!!), so the do-not-test.patch becomes the main patch.

So user last access time update should maybe reside at AccountProxy::setAccount() (next to the default timezone set call). Still mulling over this.

This doesn't do exactly what the IS says. This patch takes the authentication parts of SessionHandler into the Cookie authentication provider where it should be actually residing.

If testbot is okay, then we can look at:

• Using the User entity to load the account info (see #2345611: Load user entity in Cookie AuthenticationProvider instead of using manual queries)
• Where should last access time update be done?
• to @todo's in SessionHandler

Thanks @almaudoh. Just for the record the two patches in #26 are identical, I checked :)

#27: Lol :) Fingers too quick. Test fails are suspicious. Retesting....

Looks like the KernelTests that call AccountProxy::setAccount() blow up because of the extra dependency on UserEntityStorage introduced there.

So the question now is: where do we put the call to update last accessed time?

Why not keep it in Cookie.php?

1. +++ b/core/lib/Drupal/Core/Authentication/Provider/Cookie.php
   @@ -44,12 +59,42 @@ public function applies(Request $request) {
   +
   +      $values = $this->connection->query("SELECT u.*, s.* FROM {users_field_data} u INNER JOIN {sessions} s ON u.uid = s.uid WHERE u.default_langcode = 1 AND s.sid = :sid", array(
   +        ':sid' => Crypt::hashBase64($session->getId()),
   +      ))->fetchAssoc();
   

   We've already loaded the session data, the join here shouldn't be necessary anymore?

   I'm not sure where we want to do what, but @znerol opened an issue to store the uid directly in the session data, so we know it already here. Maybe we could already do a part of that here, then we can optimize the code here to only do a second query if we really have a user that we can load?

2. +++ b/core/lib/Drupal/Core/Session/SessionHandler.php
   @@ -166,8 +132,12 @@ public function destroy($sid) {
   +    // @todo: Manipulating the current user is not the business of the session
   +    // handler. This should be moved to the authentication mananger.
   +    $this->currentUser->setAccount(new AnonymousUserSession());
   +
   +    // @todo: cookie management should be moved to Cookie auth manager.
        // Unset the session cookies.
        $this->deleteCookie($this->getName());
   

   Any idea on how to achieve this? Who exactly destroys session right now?

   I guess we need to wrap session_destroy() calls into something that can take care of this? A method on the authentication manager that forwards to the provider? It's not really provider-generic, you can't log out from a basic auth session, the client has to just stop sending the authentication.

Thanks for the review @Berdir. I had some time to think about this some more on how to decouple the SessionHandler completely from Cookie and EntityManager.

I guess we need to wrap session_destroy() calls into something that can take care of this? A method on the authentication manager that forwards to the provider?

Cookies are just a way of storing the session id (other implementations also possible), so deleting cookies should not be in SessionHandler because that means SessionHandler would need to know about all these other implementations as well. How about a SESSION_DESTROYED event which the Cookie auth and AccountProxy can listen to and react accordingly. Is this a good way? The benefit is that other implementations of session ID storage can use this event to clean up. Would we need a similar SESSION_CREATED event?

OTOH, the best place to implement user last access time update would be in a DestructibleInterface::destruct() method (implemented maybe in AccountProxy).

Will try this approach tomorrow if there are no better ideas.

but @znerol opened an issue to store the uid directly in the session data, so we know it already here

I haven't been able to find that issue...

1. +++ b/core/lib/Drupal/Core/Authentication/Provider/Cookie.php
   @@ -44,12 +59,42 @@ public function applies(Request $request) {
   +    if ($session->start()) {
   +      // Handle the case of first time visitors and clients that don't store
   +      // cookies (eg. web crawlers).
   +      if (!$this->sessionConfiguration->hasSession($request)) {
   +        return new AnonymousUserSession();
   +      }
   

   The hasSession check is useless in this place. If $session->start() returns TRUE, it means that there is a session cookie on the request. This check was already redundant before, so we should leave it out completely here.

   However, the original code has an additional check for an empty $sid (see the D7 _drupal_session_read(). This is due to SA-CORE-2014-006). That one must be left in place.

2. +++ b/core/lib/Drupal/Core/Session/SessionHandler.php
   @@ -64,58 +67,30 @@ public function open($save_path, $name) {
   +    // Read the session data from the database.
   +    $record = $this->connection->select('sessions', 's')
   +      ->fields('s')
   +      ->condition('sid', Crypt::hashBase64($sid))
   +      ->execute()
   +      ->fetchAssoc();
    
   -    $values = $this->connection->query("SELECT u.*, s.* FROM {users_field_data} u INNER JOIN {sessions} s ON u.uid = s.uid WHERE u.default_langcode = 1 AND s.sid = :sid", array(
   -      ':sid' => Crypt::hashBase64($sid),
   -    ))->fetchAssoc();
   -
   -    // We found the client's session record and they are an authenticated,
   -    // active user.
   -    if ($values && $values['uid'] > 0 && $values['status'] == 1) {
   -      // Add roles element to $user.
   -      $rids = $this->connection->query("SELECT ur.roles_target_id as rid FROM {user__roles} ur WHERE ur.entity_id = :uid", array(
   -        ':uid' => $values['uid'],
   -      ))->fetchCol();
   -      $values['roles'] = array_merge(array(AccountInterface::AUTHENTICATED_ROLE), $rids);
   -      $_session_user = new UserSession($values);
   -    }
   -    elseif ($values) {
   -      // The user is anonymous or blocked. Only preserve two fields from the
   -      // {sessions} table.
   -      $_session_user = new UserSession(array(
   -        'session' => $values['session'],
   -        'access' => $values['access'],
   -      ));
   +    if (isset($record['session'])) {
   +      return $record['session'];
        }
        else {
   -      // The session has expired.
   -      $_session_user = new UserSession();
   +      return '';
   

   This is fantastic. Except for the missing empty($sid) check mentioned above.

3. +++ b/core/lib/Drupal/Core/Session/SessionManager.php
   @@ -187,7 +183,7 @@ public function save() {
   -    if ($user->isAnonymous() && $this->isSessionObsolete()) {
   +    if (!$user || ($user->isAnonymous() && $this->isSessionObsolete())) {
   

   This change should not be necessary. \Drupal::currentUser() is expected to always return a user. If it is necessary to make tests pass, then that indicates a problem with the patch.

This doesn't do exactly what the IS says. This patch takes the authentication parts of SessionHandler into the Cookie authentication provider where it should be actually residing.

This is the logical thing to do and is inline with the way BasicAuth works today. In a follow-up we should change authenticate() to only return the uid and leave the actual loading code and the checks for blocked users to the code triggering the authentication, i.e. to AuthenticationSubscriber or better a new helper class. This will further simplify the implementation of authentication providers.

+++ b/core/lib/Drupal/Core/Session/SessionHandler.php
@@ -166,8 +132,12 @@ public function destroy($sid) {
+    // @todo: Manipulating the current user is not the business of the session
+    // handler. This should be moved to the authentication mananger.
+    $this->currentUser->setAccount(new AnonymousUserSession());
+
+    // @todo: cookie management should be moved to Cookie auth manager.
     // Unset the session cookies.
     $this->deleteCookie($this->getName());

Let's revisit the use-cases the new code needs to accommodate in order to define responsibilities:
User: authenticated / Authentication provider: Cookie. During the log-out process, session_destroy is called. Expected outcome: Session record removed from the database (responsibility: SessionHandler), current user switched to anonymous (responsibility: the log-out code), session cookies removed from the browser (responsibility: SessionManager).
User: not-authenticated / Authentication provider: not relevant. An anonymous session is destroyed because it became empty. Expected outcome: Session record removed from the database (responsibility: SessionHandler), current user not changed at all, session cookies removed from the browser (responsibility: SessionManager).
User: authentication status not relevant / Authentication provider: not Cookie. A session is destroyed because it became empty and user authentication is not performed by cookie auth. Expected outcome: Session record removed from the database (responsibility: SessionHandler), current user not changed at all, session cookies removed from the browser (responsibility: SessionManager).

To sum it up, I do not agree with neither one of those @todos.

There is a related issue in Symfony [Session] SessionInterface should have a destroy() method. Currently there is no way to simply destroy a session #12375.

We may introduce our own SessionInterface and add the destroy() method to it (and also to SessionManager). But this seems to be a bit overkill. Another option is to extract the deleteCookie logic into a session handler proxy. That way people who genuinely have a reason to not use cookies might simply swap that out.

but @znerol opened an issue to store the uid directly in the session data, so we know it already here

#2432911: Provide test coverage to prove that a third party authentication provider does not automatically start a session

I just realized that I failed to write down my proposal in that issue. I remember mentioning the idea to @berdir in IRC though.

Thanks for the review and feedback @znerol. That clarifies a lot.

To sum it up, I do not agree with neither one of those @todos.

But looks like you agree SessionHandler should not be deleting cookies, rather SessionManager maybe.

We may introduce our own SessionInterface and add the destroy() method to it (and also to SessionManager). But this seems to be a bit overkill.

I also agree a Drupal SessionInterface would be too much, especially as we are moving towards using Symfony session components. So while we wait for Symfony to fix the SessionInterface::invalidate() / SessionInterface::destroy() issues, we'll use a work around in the SessionManager.

Another option is to extract the deleteCookie logic into a session handler proxy.

Thinking this is a great idea too.

Step 1: Moved the user last access time update to AccountProxy::destruct() method implementing DestructableInterface. This removes the Kernel test fails.

Not addressed review comments yet.

Step 2: Created a SessionManagerInterface::destroy() method wrapper for session_destroy() with cookie cleanup in there. This may end up being temporary since there is further refactoring needed in SessionManager

Also addressed review comments from #37:
1. Removed.
2. Great. Added empty($sid) check.
3. Removed.
Also removed the current user setAccount() call since that should be in the calling code.

#34.1 is a bit more complex since it requires $uid to be set in the session. Will look at this in the next patch.

Over to testbot.

+++ b/core/lib/Drupal/Core/Session/AccountProxy.php
@@ -183,6 +186,18 @@ public function setInitialAccountId($account_id) {
+  public function destruct() {
...
+      $storage = \Drupal::entityManager()->getStorage('user');
+      $storage->updateLastAccessTimestamp($this->account, REQUEST_TIME);

Not sure that's right:
1) proxy should not carry this logic.
2) needs EM injected

Suppose once this is not happen in each request so better to move to some UserRequestSubscriber

needs EM injected

This is impossible, see the comment elsewhere in the source file.

Suppose once this is not happen in each request so better to move to some UserRequestSubscriber

Very much agree.

1. +++ b/core/core.services.yml
   @@ -1200,6 +1200,8 @@ services:
      current_user:
        class: Drupal\Core\Session\AccountProxy
   +    tags:
   +      - { name: needs_destruction }
   

   It would be cool to have a one line comment why we need this tag, ... this would stop people from breaking it in the future.

2. +++ b/core/lib/Drupal/Core/Authentication/Provider/Cookie.php
   @@ -44,12 +59,36 @@ public function applies(Request $request) {
   +    $session = $request->getSession();
   +    if ($session->start()) {
   +      $values = $this->connection->query("SELECT u.*, s.* FROM {users_field_data} u INNER JOIN {sessions} s ON u.uid = s.uid WHERE u.default_langcode = 1 AND s.sid = :sid", array(
   +        ':sid' => Crypt::hashBase64($session->getId()),
   +      ))->fetchAssoc();
    
   +      // We found the client's session record and they are an authenticated,
   +      // active user.
   +      if ($values && $values['uid'] > 0 && $values['status'] == 1) {
   +        // Add roles element to $user.
   +        $rids = $this->connection->query("SELECT ur.roles_target_id as rid FROM {user__roles} ur WHERE ur.entity_id = :uid", array(
   +          ':uid' => $values['uid'],
   +        ))->fetchCol();
   +        $values['roles'] = array_merge(array(DRUPAL_AUTHENTICATED_RID), $rids);
   +        $user = new UserSession($values);
   +      }
   +      elseif ($values) {
   +        // The user is anonymous or blocked. Only preserve two fields from the
   +        // {sessions} table.
   +        $user = new UserSession(array(
   +          'session' => $values['session'],
   +          'access' => $values['access'],
   +        ));
   +      }
   +      else {
   +        // The session has expired.
   +        $user = new AnonymousUserSession();
   +      }
   +      return $user;
   

   Honestly, it feels weird to have the database connect in here, directly, what about at least moving the DB queries to methods on this class, so they can be overridden easily?

#45
2. I've attempted to split out the database calls to class methods, as suggested.

Oops... Some silly typos in #46. Try again...

New patch, addresses #44 & #45
- Added UserRequestSubscriber, suppose KernelEvents::FINISH_REQUEST is a right one
- Moved user wrapper creation into protected getUserFromSession()

Interdiff with #46, that was buggy:

+++ b/core/lib/Drupal/Core/Authentication/Provider/Cookie.php
@@ -44,13 +60,62 @@ public function applies(Request $request) {
+      $values = getSessionRecord($session);
...
+        $rids = getUserRoles($values['uid']);

missing $this
@cpj please provide interdiff next time between previous patch

I found no tests for "update last access" for user, so it makes sense to file new issue

+1 to the UserRequestSubscriber. Nicely decouples things.

Discussed at IRC with @Crell @neclimud this should not block request send.
Also added comment that user should be updated before other core subscribers start to write their caches

Trying to remove the query join to the sessions table is proving difficult. Session variables set in user_login_finalize() are not getting carried over into the next request, so I can't set the just logged in $uid in the session. Still looking at this though. Any ideas?

Trying to remove the query join to the sessions table is proving difficult. Session variables set in user_login_finalize() are not getting carried over into the next request, so I can't set the just logged in $uid in the session. Still looking at this though. Any ideas?

Is this about session variables before or after the ->migrate() call?

Is this about session variables before or after the ->migrate() call?

After the ->migrate() call.
Here's the block of new code in user_login_finalize()

  // Regenerate the session ID to prevent against session fixation attacks.
  // This is called before hook_user_login() in case one of those functions
  // fails or incorrectly does a redirect which would leave the old session
  // in place.
  \Drupal::currentUser()->setAccount($account);
  \Drupal::service('session')->migrate();
  \Drupal::service('session')->set('uid', $account->id());
  \Drupal::logger('user')->notice('Session opened for %name.', array('%name' => $account->getUsername()));
  \Drupal::moduleHandler()->invokeAll('user_login', array($account));

In the Session stack middleware, the uid session attribute is still there, but somehow doesn't get serialized with the $request->getSession()->save call.

  public function handle(Request $request, $type = self::MASTER_REQUEST, $catch = TRUE) {
    if ($type === self::MASTER_REQUEST && PHP_SAPI !== 'cli') {
      $session = $this->container->get($this->sessionServiceName);
      $session->start();
      $request->setSession($session);
    }

    $result = $this->httpKernel->handle($request, $type, $catch);

    // $session->get('uid') returns the value set in user_login_finalize().
    // But $_SESSION['_sf2_attributes'] is still empty.
    if ($type === self::MASTER_REQUEST && $request->hasSession()) {
      $request->getSession()->save();
    }

    return $result;
  }

In digging deeper, I've observed that values in the Symfony session object are stored in the attribute bag which is serialized in the $_SESSION['_sf2_attributes'] session value which would get serialized to the database.
The problem is the _sf2_attributes value doesn't get updated in the $_SESSION superglobal after $session->set('uid', ...)is called, so when the session is saved, that value is lost.

Oh, that's likely the same as #2437761: CSRF token seed and possibly other session data lost when set after a session regenerate.

Quote from #2437761: CSRF token seed and possibly other session data lost when set after a session regenerate:

Bags are references on $_SESSION['bag_name'], so maybe those references need to be refreshed after the call to session_id. I guess that SessionManager::startNow() is the culprit, because that tries to transfer session data by copying it. Maybe it is enough to call parent::loadSession() after restoring the session data?

@znerol: I came to that same conclusion an hour ago after digging through Symfony's session implementation and how it uses references to sync $_SESSION superglobal with symfony session bags (I wish I had seen that issue a week ago). NativeSessionStorage::loadSession() seems to fix it.

Attached patch attempts to remove the join with sessions table. But it re-introduces the $_SESSION superglobal (which I'm not happy with) to maintain the expectations that UserSession::session should hold a copy of the serialized $_SESSION contents. I don't know if that property is still necessary.

Expect some test fails around SessionHttpsTest and SessionTest. Let's see what else fails.

In this patch I've removed the 'session' values as I've seen it is not used anywhere else in core (usages for AccountInterface::getSessionData() in core is zero).

Additionally, a bug was exposed by this patch: UserSession::getLastAccessedTime() returns the session save timestamp, while User::getLastAccessedTime() returns the user 'access' timestamp. This would result in inconsistent behavior since we use these two interchangeably to represent the $account variable in various places in core.

The only reason why this doesn't show up is because they only go out of sync if the session is accessed twice (or more) within 180 seconds.

A logical follow up would be to clarify the relationship between the User entity and the UserSession value object. #2272189: Uncouple session handling from user module and #2347799: Remove bugged session-related methods from AccountInterface mentioned these already as well as #2345611: Load user entity in Cookie AuthenticationProvider instead of using manual queries.

Marked #2437761: CSRF token seed and possibly other session data lost when set after a session regenerate as duplicate. Hopefully, we can get this reviewed, RTBC'd and committed soon.

In this patch I've removed the 'session' values as I've seen it is not used anywhere else in core (usages for AccountInterface::getSessionData() in core is zero).

Please also remove AccountInterface::getSessionData() in addition to removing the session attribute. I think that we even should go further and also kill getSessionId() and getSecureSessionId(). Optionally that could be cleaned up later in #2347799: Remove bugged session-related methods from AccountInterface.

Additionally, a bug was exposed by this patch: UserSession::getLastAccessedTime() returns the session save timestamp, while User::getLastAccessedTime() returns the user 'access' timestamp.

As pointed out in #2345611-19: Load user entity in Cookie AuthenticationProvider instead of using manual queries, getLastAccessedTime is really only useful to order the list of users in the administrative interface. This is not something we need on the currently logged in user because that should obviously be equal to REQUEST_TIME. Therefore remove getLastAccessedTime from the interface and add it to the user entity.

1. +++ b/core/lib/Drupal/Core/Session/SessionHandler.php
   @@ -67,58 +70,34 @@ public function open($save_path, $name) {
   -        'uid' => $user->id(),
   +        'uid' => $this->currentUser->id(),
   

   The whole point on having an uid attribute in the session is to be able to eliminate the current user from the SessionHandler. Just inject the session and call $this->session->get('uid', 0); here instead.

2. +++ b/core/modules/user/user.module
   @@ -514,8 +514,6 @@ function template_preprocess_username(&$variables) {
   -  \Drupal::currentUser()->setAccount($account);
   -  \Drupal::logger('user')->notice('Session opened for %name.', array('%name' => $account->getUsername()));
   
   @@ -527,8 +525,10 @@ function user_login_finalize(UserInterface $account) {
   +  \Drupal::currentUser()->setAccount($account);
   ...
   +  \Drupal::logger('user')->notice('Session opened for %name.', array('%name' => $account->getUsername()));
   

   What is the reason for this relocation?

Please also remove AccountInterface::getSessionData()...

Let's do this in #2347799: Remove bugged session-related methods from AccountInterface

Therefore remove getLastAccessedTime from the interface and add it to the user entity.

I think we can also do this in #2347799: Remove bugged session-related methods from AccountInterface, or maybe raise a new issue for it.

#64.1: Seems like this introduces a circular dependency: session_handler.write_safe -> session_handler.write_check -> session_handler.storage -> session -> session_manager -> session_handler.write_safe, much as I had thought. I will take a closer look at this.

#64.2: Logically, I think the log entry should be made after all the necessary operations are completed successfully, not before.

This issue is at least major since it seems to block some other issues
#2432911: Provide test coverage to prove that a third party authentication provider does not automatically start a session
#2468873: Test that the authentication provider doesn't leak authentication credentials from the previous request
#2283637: Provide test coverage to prove that an AuthenticationProvider can initiate a session

Seems like this introduces a circular dependency.

Right, we should use $session = $this->requestStack->getCurrentRequest()->getSession() and then retrieve the uid from there.

Logically, I think the log entry should be made after all the necessary operations are completed successfully, not before.

Ok. I think if this is not strictly required on this patch, it should probably be separated out.

Right, we should use $session = $this->requestStack->getCurrentRequest()->getSession() and then retrieve the uid from there.

My thoughts exactly, also removes one dependency.

I think if this is not strictly required on this patch, it should probably be separated out.

Fine.

I will re-open the issue at #2347799: Remove bugged session-related methods from AccountInterface to follow up on comments #64 not already addressed in this patch.

I guess the test failures are due to the missing session on the installer request and probably also because the lazy session start is broken in the installer. Let's make sure that a) the session is set on the request whenever a session is started and b) the session is saved (and thus started lazily if necessary) before each exit if there was one on the request.

I guess something like in the attached interdiff should do it.

Thanks for the interdiff @znerol. Updated patch attached. Interdiff is #69.

BrowserTestBaseTest is based on the new mink stuff. I guess that runs in $install_state['interactive'] = FALSE but still requires the session. Therefore let's drop the new $install_state['interactive'] condition in install_bootstrap_full(). SimpleTestBrowserTest depends on BrowserTestBaseTest, that's the reason for its failure.

No idea what is causing the test fail in MigrateFileTest. Edit: seems like a random test failure #2417549: Drupal\migrate_drupal\Tests\d6\MigrateFileTest fail in MigrateTestBase]

Suggesting to remove this form the patch :

@@ -174,6 +170,7 @@ protected function startNow() {
     // Restore session data.
     if ($this->startedLazy) {
       $_SESSION = $session_data;
+      $this->loadSession();
     }

and solve it in it's dedicated issue : #2437761: CSRF token seed and possibly other session data lost when set after a session regenerate.
Patch is already quite complicated and we may want to split it in one-by-one issue fix.

No need to remove that line. The important thing is that the other issue brings in a proper test.

New patch with #72 applied, still not fulfilling tests though.

Rerolled #75 after #2437761: CSRF token seed and possibly other session data lost when set after a session regenerate went in.

Since this resolves or unblocks several bugs, including a critical, let's bump this issue to critical.

@Berdir said in #2468873-16: Test that the authentication provider doesn't leak authentication credentials from the previous request:

just storing the uid might not be enough, we possibly want to explicitly store the used provider?

Response in #2468873-21: Test that the authentication provider doesn't leak authentication credentials from the previous request:

I think storing uid is enough. It just means uid>0 = authenticated by cookie provider, otherwise authenticated by another provider or anonymous.

I'd like to point out that storing the uid into session storage has two purposes:

1. It allows the cookie authentication provider to load the proper user.
2. It allows us to clear all sessions of a given user from the storage (e.g. when the password is changed).

Before this patch, both of those use-cases were covered by storing the uid exclusively on the appropriate database column in the session table. As a result it was necessary to load the user as part of SessionHandler::read. Storing that information as a session attribute allows us to move that code into the cookie authentication provider. Note that the uid session attribute is intended to be used by the cookie authentication provider exclusively, it is not meant to be used by any other code.

It follows that we do not have any possibility to distinguish anonymous sessions from sessions which were used on requests where another authentication provider was in use. Note that in HEAD the uid is populated regardless of whether or not the request was authenticated by the cookie provider and that is then reused on subsequent requests (this bug is addressed by the current patch).

@Berdir basically proposes that we should store the uid in session storage regardless of the authentication provider in use and in addition also store the provider id in order to fix the bug pointed out above. The cookie provider would then only authenticate the request if both session attributes contain appropriate values. The benefit of this approach is that it would allow us to distinguish sessions which were authenticated by some third-party provider from anonymous ones and clear them out on a per-user basis (use-case 2).

Let's take a closer look at use-case 2. The place where SessionManager::delete() is called is in User::postSave on two occasions: When the password was changed and when the user was blocked.

Dropping sessions of blocked users is in fact not strictly necessary, because authentication providers are currently responsible to check that bit.

The impact of the patch on the removal of sessions when a password is changed is as follows:

1. Current HEAD and @Berdir approach: Any session of a given user is removed regardless of the authentication provider in use.
2. Patch: Only those sessions of a user are removed which were authenticated by the cookie authentication provider.

Note that the intention of this mechanism is to prevent that compromised sessions can be used to access a site after the password for an account was changed. This is still the case with the patch. Removing sessions of a user which do not allow an adversary to impersonate that user is not necessary.

Another reason why I do not like the authentication provider id to be stored in the session is that I consider that information private to the AuthenticationManager implementation (see #2456303-9: AuthenticationManager needs interface and ::getProviderKeys).

Instead of clearing the session on a per-user basis (which can be in fact difficult to implement in non-db session backends), it would also be possible to store a hash over relevant attributes (eg. password hash, blocked state) in the session. Upon authentication the provider compares the hash to one derived from the loaded user and fails if it does not match. This would make it possible to get rid of SessionManager::delete() and that again would again reduce the amount backend specific code inside that class. But that should be done in a follow-up.

While we wait for a decision on #79, let's fix some minor issues I've found while looking through the patch again:

1. +++ b/core/includes/install.core.inc
   @@ -1553,8 +1557,10 @@ function install_load_profile(&$install_state) {
   -function install_bootstrap_full() {
   ...
   +function install_bootstrap_full(&$install_state) {
   

   Nitpick: $install_state is unused. The function signature does not need to be changed.

2. +++ b/core/lib/Drupal/Core/Session/SessionHandler.php
   @@ -67,58 +59,34 @@ public function open($save_path, $name) {
      public function read($sid) {
   +    if (empty($sid)) {
          return '';
        }
    
   +    // Read the session data from the database.
   +    $record = $this->connection->select('sessions', 's')
   +      ->fields('s')
   +      ->condition('sid', Crypt::hashBase64($sid))
   +      ->execute()
   +      ->fetchAssoc();
   +
   +    if (isset($record['session'])) {
   +      return $record['session'];
        }
        else {
   +      return '';
        }
      }
   

   I really like how simple this method is now. It could be even easier to read if there was only one return statement. E.g.

   public function read($sid) {
     $session_data = '';
     if (!empty($sid)) {
       $record = // query
       if (isset($record['session'])) {
         $session_data = $record['session'];
       }
     }
     return $session_data;
   

   Also we do only need the session column, therefore it would be enough to fetchField() instead of fetchAssoc().

3. +++ b/core/lib/Drupal/Core/Session/SessionHandler.php
   @@ -67,58 +59,34 @@ public function open($save_path, $name) {
          $fields = array(
   -        'uid' => $user->id(),
   +        'uid' => $this->requestStack->getCurrentRequest()->getSession()->get('uid', 0),
            'hostname' => $this->requestStack->getCurrentRequest()->getClientIP(),
   

   This could profit if we'd populate $request = $this->requestStack->getCurrentRequest() beforehand and use $request->getSession() and $request->getClientIP(). That also would reduce the uid row to less than 80 characters.

+++ b/core/lib/Drupal/Core/Authentication/Provider/Cookie.php
@@ -44,13 +59,56 @@ public function applies(Request $request) {
+    elseif ($values) {
+      // The user is anonymous or blocked. Only preserve access field from the
+      // {users} table.
+      return new UserSession([
+        'access' => $values['access'],
+      ]);
+    }

I don't understand this part, why would we return a UserSession in which only the 'access' property is populated when the user is anonymous or blocked? This property is unused as far as I can see. It stores the 'last access time', but if the UserSession::getLastAccessedTime() does not even use this property, it uses $this->timestamp instead.

The access property is not even declared on the UserSession class.

Re #81 I agree, see also #64. This is left over code but that is easy to remove at a later stage.

From #64:

As pointed out in #2345611-19: Load user entity in SessionHandler instead of using manual queries, getLastAccessedTime is really only useful to order the list of users in the administrative interface. This is not something we need on the currently logged in user because that should obviously be equal to REQUEST_TIME. Therefore remove getLastAccessedTime from the interface and add it to the user entity.

This does not seem to be correct. getLastAccessedTime() is also used in SessionHandler to prevent session data from being saved more than once per 3 minutes. In this case it is not certain that the user object is a fully populated entity, it might also be a UserSession object. This means that we still need getLastAccessedTime on AccountInterface.

Looks great, review below, looks like it overlaps a bit with what @znerol said, didn't read that before.

1. +++ b/core/lib/Drupal/Core/Authentication/Provider/Cookie.php
   @@ -24,13 +29,23 @@ class Cookie implements AuthenticationProviderInterface {
   +   *   The database connection.
       */
   -  public function __construct(SessionConfigurationInterface $session_configuration) {
   +  public function __construct(SessionConfigurationInterface $session_configuration, Connection $connection) {
   

   I've been wondering for a while now about moving cookie to the user module, not sure if we can still do tha, but it would kind of make sense and help to further decouple the user module from system/core.

2. +++ b/core/lib/Drupal/Core/Authentication/Provider/Cookie.php
   @@ -44,13 +59,56 @@ public function applies(Request $request) {
   +    elseif ($values) {
   +      // The user is anonymous or blocked. Only preserve access field from the
   +      // {users} table.
   +      return new UserSession([
   +        'access' => $values['access'],
   +      ]);
   +    }
   +    // The session has expired.
   +    return new AnonymousUserSession();
   

   As commented above, I hink we can just drop this code here, since it's dead here. Doesn't increase the complexity of the code here, it actually simplifies the patch.

3. +++ b/core/lib/Drupal/Core/Session/SessionHandler.php
   @@ -67,58 +59,34 @@ public function open($save_path, $name) {
   +    // Read the session data from the database.
   +    $record = $this->connection->select('sessions', 's')
   +      ->fields('s')
   +      ->condition('sid', Crypt::hashBase64($sid))
   +      ->execute()
   +      ->fetchAssoc();
   +
   +    if (isset($record['session'])) {
   +      return $record['session'];
        }
        else {
   -      // The session has expired.
   -      $_session_user = new UserSession();
   +      return '';
        }
   -
   -    return $_session_user->session;
   

   Why are we selecting all fields when we only need the uid?

   Can't just select the session column and then do a fetchField? Then we could even simplify it to:

   return (string) $query->fetchField(); ?

4. +++ b/core/modules/user/src/EventSubscriber/UserRequestSubscriber.php
   @@ -0,0 +1,73 @@
   +   * {@inheritdoc}
   +   */
   +  public static function getSubscribedEvents() {
   +    // Should go before other subscribers start to write their caches.
   +    $events[KernelEvents::TERMINATE][] = ['onKernelTerminate', 300];
   +    return $events;
   +  }
   

   We also have a DestructableInterface, but that's specificaly for services that if instantiated, want to be destructed, so not a replacement for this.

   Anyway, since this is now in user.module, that means we should definitely move cookie (in a different issue) too?

5. +++ b/core/modules/user/user.module
   @@ -1386,7 +1386,8 @@ function user_logout() {
      // Session::invalidate(). Regrettably this method is currently broken and may
      // lead to the creation of spurious session records in the database.
      // @see https://github.com/symfony/symfony/issues/12375
   -  session_destroy();
   

   are those comments still accurate? can we maybe move or remove them?

Re #84.5: Yes the comments are still accurate. I tend to think that we should not access session_manager at all if possible. The canonical way to access and manipulate the session is via $request->getSession().

This patch fixes #80.1, .2, .3, #81, and #84.2, .3.

1. +++ b/core/lib/Drupal/Core/Authentication/Provider/Cookie.php
   @@ -44,13 +59,49 @@ public function applies(Request $request) {
   +  protected function getUserFromSession(SessionInterface $session) {
   

   while reading this method name, I got confused, ... wait we load a user? Maybe name it getAccountFromSession?

2. +++ b/core/modules/user/src/EventSubscriber/UserRequestSubscriber.php
   @@ -0,0 +1,73 @@
   +    // Should go before other subscribers start to write their caches.
   +    $events[KernelEvents::TERMINATE][] = ['onKernelTerminate', 300];
   

   Should we open a follow up to discuss that? At least we should certainly try to run before \Drupal\Core\EventSubscriber\KernelDestructionSubscriber::onKernelTerminate as we should not re-init the services.

3. +++ b/core/modules/user/user.module
   @@ -528,7 +528,7 @@ function user_login_finalize(UserInterface $account) {
   -
   +  \Drupal::service('session')->set('uid', $account->id());
   

   I'm curious how / where we document how the entire system works ...

1. There is no difference between account and user, that's all made up :) The interface is AccountInterface and the class that we load is actually UserSession. And I want to switch to using a user entity there anyway. I'd stick with getUserFromSession().

1. +++ b/core/lib/Drupal/Core/Authentication/Provider/Cookie.php
   @@ -44,13 +59,49 @@ public function applies(Request $request) {
   +    $values = $this->connection->select('users_field_data', 'u')
   +      ->fields('u')
   +      ->condition('default_langcode', 1)
   +      ->condition('uid', $session->get('uid'))
   +      ->execute()
   +      ->fetchAssoc();
   +
   +    if ($values && $values['uid'] > 0 && $values['status'] == 1) {
   

   If $session->get('uid') returns 0 or NULL, we should not bother querying the database at all.

2. +++ b/core/lib/Drupal/Core/Authentication/Provider/Cookie.php
   @@ -44,13 +59,49 @@ public function applies(Request $request) {
   +      // UserSession::getLastAccessedTime() returns session save timestamp,
   +      // while User::getLastAccessedTime() returns the user 'access' timestamp.
   +      // This ensures they are synchronized.
   +      $values['timestamp'] = $values['access'];
   

   Authenticated users are currently represented by either a UserSession or a User entity. The UserSession uses the timestamp property internally while the User entity uses access for the same thing. This is why we have to currently copy over the accessed loaded from the database to the timestamp value in the UserSession. We can take care of this in #2345611: Load user entity in Cookie AuthenticationProvider instead of using manual queries.

Regarding #81 ( user is anonymous or blocked. Only preserve access field from the {users} table.). I think I have an explanation for that weird bit.

In Drupal 7 we used $user->timestamp in order to prevent that unchanged sessions are written at every request. Obviously that timestamp needed to be in place even for anonymous users with an open session. However, we replaced that bit with the WriteCheckSessionHandler proxy. For that reason it is justifiable to drop UserSession for anonymous/blocked users here (additionally it is broken already as pointed out in #81).

So the unnecessary query should be fixed, the rest is fine in my opinion.

Also while testing manually I discovered that we still open new sessions on every request with this patch. This is due to SessionManager::save() still checking the current user. Instead it should just rely on isSessionObsolete() because that will return FALSE for authenticated sessions (i.e. uid in session attributes). I guess something like the attached interdiff will solve that problem.

moving cookie to the user module

+1 to #84.1

Also I'm sure that we should not use database::select() because it needlessly executes hook_query*_alter() hooks so loads all modules on this early stage.

+    $values = $this->connection
+      ->query('SELECT * FROM {users_field_data} WHERE uid = :uid AND default_langcode = 1', [':uid', $session->get('uid')])
       ->fetchAssoc();

Guess you were trying to say [':uid' => $uid]

Fix typo and added #90, maybe code-comment could be improved

+++ b/core/modules/user/src/Authentication/Provider/Cookie.php
@@ -0,0 +1,109 @@
+      // Do not disturb database for anonymous users.
+      return new AnonymousUserSession();
...
+    // The session has expired or user is blocked or anonymous.
+    return new AnonymousUserSession();

First comment should be improved a bit, maybe just say like something like "Do not execute additional database queries for anonymous users." ?

Similar for the second comment, I'm also not sure we still need the anyonmous there and I think the session is expired is also not relevant here, weren't not checking for that?

So just..

// The user was blocked or deleted.

?

Re #95: Those comments would actually be superflous if we'd just stick to self documenting code:

protected function getUserFromSession(SessionInterface $session) {
  $result = new AnonymousUserSession();

  if ($uid = $session->get('uid')) {
    $values = // run query
    if ($values && $values['status'] === 1) {
      [...]
      $result = new UserSession($values);
    }
  }

  return $result
}

Notes: Do not use multiple returns unless it really makes the code more readable. In conditionals, test first for the happy path. It would be great if we could use the identity operator here, not sure if it works though (is the status int or bool?)

Edit:@Berdir notes that database results are always string. Therefore typesafe comparison is not possible.

Oh, and btw. getUserFromSession() could simply return NULL instead of AnonymousUserSession. Additionally, because we do now access the session via Symfony session object, it is not necessary to start it explicitly, because that is taken care of by NativeSessionStorage::getBac() called by Session::get().

So we could basically write:

  public function authenticate(Request $request) {
    return $this->getUserFromSession($request->getSession());
  }

Opened follow-up #2472535: Remove SessionManager::delete in favor of a portable mechanism to invalid sessions of authenticated users.

What is left to be done here?

Basically #95 and following. I'll try to summarize:

• No need to use/return AnonymousUserSession at all in cookie provider because AuthenticationSubscriber interprets NULL as anonymous session. This is in accordance to AuthenticationProviderInterface
• It follows that authenticate boils down to:
  

    public function authenticate(Request $request) {
      return $this->getUserFromSession($request->getSession());
    }
  

• And getUserFromSession can be simplified to:
  

  protected function getUserFromSession(SessionInterface $session) {
    if ($uid = $session->get('uid')) {
      $values = // run query
      if ($values && $values['status'] == 1) {
        // [...] mess with $values
        return new UserSession($values);
      }
    }
  }
  

Picking this up. @andypost and @almoudoh are working in different time zones and I'm at the Drupal Dev Days sprint so I can get help from @znerol and @Berdir to get me up to speed on this issue.

Thanks @pfrenssen. #101 is a summary of what's left to be done.

I did a first pass through the patch to familiarize myself with the issue. Made a few small improvements and clarified documentation. Will start addressing the remarks starting from #95 now.

• Addressed remarks.
• Uncluttered SessionHandler::read() to avoid multiple return statements.
• Updated the documentation for Cookie::getUserFromSession() to clarify that a UserSession object is returned and not a full User entity. Since they both implement AccountInterface this might otherwise cause confusion.
• Removed the check on $values['uid'] since we are already checking that $uid is not empty before executing the query. If the query returns a result then this value will always be there.

Thank you! I've posted the first two patches in this issue, but they are something completely different than what @almaudoh started almost one year later. So I think it is okay for me to RTBC this.

1. +++ b/core/modules/user/src/Authentication/Provider/Cookie.php
   @@ -0,0 +1,100 @@
   +use Drupal\Core\Session\AnonymousUserSession;
   

   Not used.

2. +++ b/core/modules/user/src/Authentication/Provider/Cookie.php
   @@ -0,0 +1,100 @@
   +   * @return \Drupal\Core\Session\AccountInterface
   +   *   The UserSession object for the current user.
   

   We should document that this method returns NULL if the user is not active or the session does not have a UID attribute. Plus given that authenticate specifically needs to return NULL to indicate that the authentication failed I think we should have an explicit return NULL; here.

BTW - this patch is looking great :) - should have mentioned that in #107

New patch coming up!

Thanks.

RTBC+1

Back to alexpott.

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 0fa529e and pushed to 8.0.x. Thanks!