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.
Comment | File | Size | Author |
---|---|---|---|
#6 | remove-2472535-6.patch | 7.1 KB | znerol |
#3 | 2472535-remove-session-manager-delete-3.patch | 7.48 KB | andypost |
#1 | 2472535-remove-session-manager-delete.diff | 7.62 KB | znerol |
Comments
Comment #1
znerol CreditAttribution: znerol commentedFirst try. I'm not quite sure whether this is the correct way to access entity properties from within
User::postSave
.Comment #2
andypostPatch 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:
please use $uid . ':' . $status to prevent collisions
Also result of isActive() is not printable (bool)
please use $session = \Drupal::service('session'); to not call container inline
Comment #3
andypostre-roll + suggested changes and clean-ups, no interdiff - can't reconstruct
Now
session_handler.write_safe
service is used only by account switcherComment #4
znerol CreditAttribution: znerol commentedYes. 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.Oh right! I'm feeling stupid :)
Comment #5
dawehnerI think we should explain the idea behind the $user_state here, shouldn't we?
Comment #6
znerol CreditAttribution: znerol commentedReroll.
Comment #9
cpj CreditAttribution: cpj commentedhmacBase64 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.
Comment #10
znerol CreditAttribution: znerol commentedIt 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 theuser_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.
Comment #11
cpj CreditAttribution: cpj commented@znerol - thanks for the clarification.
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.
Comment #12
almaudoh CreditAttribution: almaudoh commentedWould that be in this patch or a follow-up?
Comment #13
znerol CreditAttribution: znerol commentedFiled #2486415: Rework user_pass_rehash into an AccountTokenGenerator. Let's see whether we can get that in first.
Comment #14
mgiffordComment #21
jibran