Repository navigation
feat(session): add a database session driver and logout from other devices - #400
Conversation
abdulmajidcse
left a comment
There was a problem hiding this comment.
@techmahedy Found these at e63b325 by reading the code (not reproduced locally):
Fix before merge
Authenticate::user(): afterlogoutOtherDevices(), an old device's cookie hits the "possible theft" branch and sets the new remember token toNULL, so the current device's new cookie stops working too.logoutOtherDevices(): afterregenerate()or a re-login, it only excludes the new session ID, so this device's stored row is deleted and the browser keeps a dead cookie.refreshSessionFingerprint(): it only seesresolvedUser, so a password change through another instance (User::where(...)->update(...)) signs out that device.isSessionCurrent(): sessions with no fingerprint are stamped with the new one, so pre-upgrade sessions survivelogoutOtherDevices()on drivers that rely on the fingerprint.create_sessions_table.stub: it ignoressession.connection, so the table is created on the default database while the store uses the configured one.
Smaller
destroyUserSessions()doesn't follow rotation pointers, so a concurrent rotation can leave a session alive or delete the current device's new row.- A model that only overrides
getAuthPassword()still getspasswordfromgetAuthPasswordName(). logoutOtherDevices()issues a new remember token if any remember cookie exists, without validating it.- A dotted table name like
app.sessionsmay be quoted differently by the migration and the store (unconfirmed). - The table-name regex is duplicated in
SessionTableCommandandDatabaseStore.
|
@abdulmajidcse please check now |
abdulmajidcse
left a comment
There was a problem hiding this comment.
@techmahedy Most of my last review is fixed, and the tests and PHPStan pass at 451f6ea, also when merged with 4.x. I reproduced these against this commit, using a scratch test on SQLite:
Fix before merge
logoutOtherDevices(): the new password is saved immediately, but the session's fingerprint is only saved at the end of the request. A parallel request from the same device in that gap looks stale and its session is deleted, so the device that should stay signed in is signed out.logoutOtherDevices()only refreshes the calling actor's fingerprint. Afteractor('admin')->logoutOtherDevices(), thewebcheck fails on the next request and invalidates the whole session, signing out both actors.isSessionCurrent(): a session with no fingerprint is signed out and loses its data unless a remember cookie restores the sign-in. The description and docs say turning the middleware on signs nobody out, so either the code or both claims need to change.
Smaller (from reading the code)
- A wrong remember token no longer clears the stored token. The description still says there are no breaking changes.
touch()only updateslast_activity, sosessions()can show an old IP with a recent time.last_activityis a signedINTEGERon PostgreSQL and overflows in 2038.- An unchanged session reads the full row a second time in
stillCurrent(), which the description's cost per request doesn't mention. Session::ROTATED_TOandROTATED_UNTILare unused, and theexplode('.')intable()can't do anything now that dots are rejected.
… actors when logging out other devices
|
Thanks @abdulmajidcse, all reproduced and fixed:
Smaller: Also fixed two test stubs that replaced |
abdulmajidcse
left a comment
There was a problem hiding this comment.
@techmahedy The three bugs from my last review are fixed at 9941f04, and I confirmed each one. The tests and PHPStan pass too. Two things are left before merge:
- The description wasn't updated. It still says "turning it on signs nobody out" and "Breaking changes: None", and it doesn't mention the remember-token change or the extra lookup of an unchanged session.
- A normal password change still has the parallel-request bug. With
AuthenticateSessionon, the controller saves the new password onauth()->user(), but the fingerprint is only saved at the end of the request. A parallel request in that gap deletes the session, so the device is signed out. I reproduced this on the database and file drivers. The docs promise this device stays signed in, so either fix it or update the docs.
Smaller (also reproduced)
- An actor signed in only by its remember cookie is still signed out when another actor calls
logoutOtherDevices(), because its cookie isn't re-sent and the token is cleared. - On the cookie driver, a parallel request during
logoutOtherDevices()is still treated as a guest.
…d through the model
|
Thanks @abdulmajidcse, both fixed and reproduced first:
Smaller:
Docs updated too: a password changed with a query still needs |
abdulmajidcse
left a comment
There was a problem hiding this comment.
@techmahedy Everything from my last review is addressed at 279aad5, but the new Authable::save()/update() hook adds two regressions. I reproduced both, and both worked at 9941f04:
-
A rolled-back password change signs the device out. The new fingerprint is saved before the password and isn't undone if a surrounding transaction rolls back:
DB::transaction(function () { $user = User::find($id); $user->password = $newHash; $user->save(); throw new RuntimeException('a later step failed'); });
The password in the database doesn't change, but the device is signed out on its next request (database and file drivers).
-
One broken actor config blocks password changes.
actorsSignedInAs()now callsgetModel()for every actor inauth.actorsbefore checking the session. If an unused actor points to a removed model class,save(),update()andlogoutOtherDevices()all throw on a password change, and the password stays unchanged. Checking the session first, as before, fixes it.
|
Thanks @abdulmajidcse, both reproduced and fixed:
|
abdulmajidcse
left a comment
There was a problem hiding this comment.
@techmahedy Both issues from my last review are fixed at 6e96e3d, and the tests and PHPStan pass. One new regression needs fixing before merge. I reproduced it on the database and file drivers, and it worked at 279aad5:
A model hook that hashes the password signs the device out.
$user = auth()->user();
$user->password = 'plain';
$user->save(); // a before_updated hook hashes itThe pending fingerprint comes from the plain value, but the database holds the hash the hook created, so they never match. The middleware refresh that used to cover this is gone. Setting the pending fingerprint again from the model's final password after the write should fix it.
Smaller (also reproduced)
- The same hook case with
User::find()also signs the device out. This isn't new, and the same fix should cover it. - A committed password save followed by a rolled-back one in the same request signs the device out, because the second pending fingerprint overwrites the first.
- On the file driver, an after-hook that throws leaves the new password saved but undoes the fingerprint, so the device is signed out.
- The description still says the fingerprint follows the password after the request, which the middleware no longer does.
|
Thanks @abdulmajidcse, all reproduced and fixed:
Also found while fixing 3: the password write used to run inside the session lock, which on SQLite is a transaction, so a throwing after hook also rolled back the application's own update. The write now runs outside the lock; only recording the fingerprint is locked, before the write, which still covers parallel requests. |
Pull Request Checklist
Summary
Adds a
databasesession driver, and lets a signed-in user sign out of every other device while staying signed in on the current one. Signing out works with every session driver; with the database driver the other sessions are also deleted immediately.Database session driver
DatabaseStore(SESSION_DRIVER=database) for MySQL/MariaDB, PostgreSQL and SQLite, with newconnectionandtableoptions.php pool session:tablegenerates it for the configured table. Whensession.connectionis set, the migration usesSchema::connection('…')and the command prints the matchingphp pool migrate --connection=….last_activityis a big integer, so it does not overflow in 2038 on PostgreSQL.app.sessionsare rejected, because the schema builder and the store would quote them differently; on PostgreSQL, setsearch_pathon the session connection instead.LockableSessionStorewith a lock on one session only:GET_LOCK()on MySQL, an advisory lock on PostgreSQL, a short write transaction on SQLite. Locks are re-entrant and never wait longer than the lock timeout (2 seconds). Concurrent saves are merged exactly as with the file driver.UserSessionStorecontract. Each row recordsuser_id(the default actor's user),ip_addressanduser_agent.SessionExceptionthat says how to create it.last_activityindex.Logout from other devices
auth()->logoutOtherDevices($password)confirms the password, then:UPDATErather thansave().save()silently skips attributes missing from$creatable, and model hooks could hash the new hash a second time. A model that overridesgetAuthPassword()withoutgetAuthPasswordName()gets aLogicExceptionbefore anything is written.Authable::save()andupdate()save a new password of a user this device is signed in as together with this device's fingerprint, so the device and its running requests stay signed in. Changing another user's password leaves this session alone.AuthenticateSessionmiddleware (opt-in): signs out sessions whose fingerprint is stale, which works with every driver. A mismatch is checked again under the session lock against the stored session and the database, so a request that loaded the session before a password change of this device is not signed out. After the request, the fingerprint follows a password changed on the signed-in user model; it is never reloaded from the database, because a change there may come from another device that is signing this one out.auth()->refreshSessionFingerprint()records the password from the database, for a password changed with a query instead of the model.auth()->sessions()lists the signed-in user's devices (IP, user agent, last activity, current or not) and never exposes session IDs.Authablemethods:getAuthPasswordName()andgetRememberTokenName(), for custom column names.New contracts and methods
UserSessionStore:userSessions(int|string $userId): arrayanddestroyUserSessions(int|string $userId, array $keep = []): int.$keepholds this device's session ids; what they were rotated into is kept too.CommittableSession(implemented bySession):storedId(),exclusively(),storedValue()andputThrough(), for code that must change the stored session while it is locked.RotationPointer: the format of the pointer an automatically rotated session id leaves behind, shared bySessionand the database store.Behavior changes
AuthenticateSessionregistered, sessions created before it are signed out once (their other data is kept; a valid remember-me cookie signs them back in).Authable::save()andupdate()save a password change of the signed-in user together with this device's session fingerprint.password_hash_{actor}fingerprint key in the session.Existing drivers otherwise behave as before, and the middleware is opt-in.
Known limits
cookiedriver, a request of this device already carrying the old cookie can't see the new fingerprint, so it is signed out. Keeping running requests signed in needs thefileordatabasedriver.refreshSessionFingerprint()afterwards keeps this device signed in, but a parallel request that checks the session in between can still be signed out.Testing
DatabaseStoretests pass on SQLite, MySQL and PostgreSQL, including the migration generated for a configured connection.config()for every later test once loaded, which hid thatAuthenticateTestfailed when run on its own. Both now apply only while their own tests run.Checklist