Problem/Motivation

The SessionManager should not contain any storage related code. The problem with storage related code is that third-party session storage implementations need to override SessionManager in addition to replacing SessionHandler implementation.

We've removed most of the database queries in past issues. One of the two queries which is still there is in SessionManager::delete(). That method is actually responsible for removing all sessions of a user when she/he changes the password or is blocked by an administrator. In the database session storage backend this is easy to implement. The uid column acts as a secondary index there. This is more complex on key-value stores which do not normally provide ways to select on attributes.

Proposed resolution

#2228393: Decouple session from cookie based user authentication makes it possible to drop SessionManager::delete() completely. Instead a hash value over security-critical properties (password hash, blocked status) of the user could be stored in the session and checked by the cookie authentication provider. This works regardless of the session storage backend implementation.

Remaining tasks

User interface changes

None.

API changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

znerol’s picture

Status: Active » Needs review
FileSize
7.62 KB

First try. I'm not quite sure whether this is the correct way to access entity properties from within User::postSave.

andypost’s picture

Patch somehow is not applicable anymore, I'm sure using class methods within class itself is fine ;)
The main issue I see that concatenation of ID and Active could be unpredictable:

  1. +++ b/core/modules/user/src/Authentication/Provider/Cookie.php
    @@ -80,19 +81,22 @@ protected function getUserFromSession(SessionInterface $session) {
    +        $cred_hash = Crypt::hmacBase64($uid . $values['status'], $values['pass']);
    +        if ($cred_hash === $session->get('cred_hash')) {
    
    +++ b/core/modules/user/src/Entity/User.php
    @@ -111,21 +112,15 @@ public function postSave(EntityStorageInterface $storage, $update = TRUE) {
    +          \Drupal::service('session')->set('cred_hash', Crypt::hmacBase64($this->id() . $this->isActive(), $this->getPassword()));
    
    +++ b/core/modules/user/user.module
    @@ -529,6 +529,7 @@ function user_login_finalize(UserInterface $account) {
    +  \Drupal::service('session')->set('cred_hash', Crypt::hmacBase64($account->id() . $account->isActive(), $account->getPassword()));
    

    please use $uid . ':' . $status to prevent collisions
    Also result of isActive() is not printable (bool)

  2. +++ b/core/modules/user/src/Entity/User.php
    @@ -111,21 +112,15 @@ public function postSave(EntityStorageInterface $storage, $update = TRUE) {
               \Drupal::service('session')->migrate();
    
    +++ b/core/modules/user/user.module
    @@ -529,6 +529,7 @@ function user_login_finalize(UserInterface $account) {
       \Drupal::service('session')->migrate();
       \Drupal::service('session')->set('uid', $account->id());
    

    please use $session = \Drupal::service('session'); to not call container inline

andypost’s picture

re-roll + suggested changes and clean-ups, no interdiff - can't reconstruct

Now session_handler.write_safe service is used only by account switcher

znerol’s picture

I'm sure using class methods within class itself is fine ;)

Yes. It's weird though that some lines above they use $this->pass while it probably should be $this->getPassword(). But I just don't know enough about the entity subsystem in order to understand the difference.

please use $uid . ':' . $status to prevent collisions

Oh right! I'm feeling stupid :)

dawehner’s picture

+++ b/core/modules/user/src/Authentication/Provider/Cookie.php
@@ -80,19 +81,19 @@ protected function getUserFromSession(SessionInterface $session) {
+      if (!empty($values)) {
+        $user_state = Crypt::hmacBase64($uid . ':' . $values['status'], $values['pass']);
+        if ($user_state === $session->get('user_state')) {

I think we should explain the idea behind the $user_state here, shouldn't we?

znerol’s picture

FileSize
7.1 KB

Reroll.

Status: Needs review » Needs work

The last submitted patch, 6: remove-2472535-6.patch, failed testing.

Status: Needs work » Needs review

znerol queued 6: remove-2472535-6.patch for re-testing.

cpj’s picture

+++ b/core/modules/user/src/Authentication/Provider/Cookie.php
@@ -80,14 +81,17 @@ protected function getUserFromSession(SessionInterface $session) {
+        $cred_hash = Crypt::hmacBase64($uid . $values['status'], $values['pass']);
+++ b/core/modules/user/src/Entity/User.php
@@ -104,21 +105,15 @@ public function postSave(EntityStorageInterface $storage, $update = TRUE) {
+          \Drupal::service('session')->set('cred_hash', Crypt::hmacBase64($this->id() . $this->isActive(), $this->getPassword()));
+++ b/core/modules/user/user.module
@@ -529,6 +529,7 @@ function user_login_finalize(UserInterface $account) {
+  \Drupal::service('session')->set('cred_hash', Crypt::hmacBase64($account->id() . $account->isActive(), $account->getPassword()));

hmacBase64 is called here in 3 different Classes with 3 different arguments, which apparently all need to match. The chances of getting one of this "cryptic" :-) invocations wrong seems high to me. Also, this feels a lot like a global variable, passed via the Symfony session, with an undefined purpose. This needs to be clearly explained, so that someone writing an alternative authentication provider (like me...) can mimic what's going on.

znerol’s picture

It should not be necessary to replicate that functionality for third-party authentication provider. The session authentication provider is quite unique because the login only happens once and the session cookie then serves as credential equivalent. I have a hard time to think about an authentication scheme which has similar properties.

I agree that this definitely lacks good documentation though. I started with extracting this to a getCredentialsHash() method on the user entity with the aim to document it there. However, I then realized that the user_pass_rehash() function is providing the exact same functionality (just with more specific parameters). I think there are additional use-cases for a HMAC keyed by current user credentials and status. E.g. API keys or persistent login tokens. So my plan is to concentrate this functionality into a service with a very similar interface like the CSRF token generator.

The useful thing about HMAC keyed by current user credentials and status is that it becomes invalid as soon as the user is blocked or she changes the password.

cpj’s picture

@znerol - thanks for the clarification.

I have a hard time to think about an authentication scheme which has similar properties.

Thinking further on this, you're right that other authentication schemes will probably not need / use the same mechanism, but I can image using the HMAC keyed by current user credentials and status itself, so knowing what it is and which classes read/write it will definitely be helpful.

almaudoh’s picture

So my plan is to concentrate this functionality into a service with a very similar interface like the CSRF token generator.

Would that be in this patch or a follow-up?

znerol’s picture

Status: Needs review » Postponed

Filed #2486415: Rework user_pass_rehash into an AccountTokenGenerator. Let's see whether we can get that in first.

mgifford’s picture

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jibran’s picture

Version: 8.6.x-dev » 8.8.x-dev

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.