fix(login): drop the older session whatever the session save method - #6413
Open
e107help[bot] wants to merge 1 commit into
Open
fix(login): drop the older session whatever the session save method#6413e107help[bot] wants to merge 1 commit into
e107help[bot] wants to merge 1 commit into
Conversation
Disallow multiple logins evicted an account's earlier sessions only where the session save method preference read 'db'. On any other setting the second login was refused outright while Track Online was on, and nothing happened at all while it was off, so what the preference did turned on a second, unrelated preference that its help text never mentioned. Eviction needs an index of an account's session ids, and the session table is the only one e107 keeps: PHP can destroy a session only by an id already held. So the preference now decides the storage, sessions go to the database while it is on, and the one eviction path serves every site. The refusal goes with it, and with it the failed-login autoban counter it fed, which is why #4657 took the refusal off database storage in the first place. LOGIN_MULTIPLE stays defined and listed in the error-code map because it is published API, but its handler goes: no core path can raise it any more, and the default case already gives a caller that does the generic message, without feeding the ban counter. The eviction filters the session table on session_user, a column that carried no index while only sites which had chosen database sessions ever ran the query. Every site with the preference on runs it now, so the column gets one. PRFLAN_130 describes the behaviour an admin actually gets: the storage the preference takes over, and the one-off sign-out that moving storage costs. The Session Save Method control says the same from the other side, because while the preference is on that control shows a value the site is not using.
e107help
Bot
force-pushed
the
e107help/6302
branch
from
September 13, 2026 20:35
e744a2f to
ac88299
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Fixes #6302.
Disallow multiple logins had three behaviours, and which one a site got was decided by two other preferences on the same page:
db(shipped default)The help text described only the middle row, which is the behaviour a stock install does not get. An admin who ticked the box, signed in twice and watched both logins succeed reported the preference as broken, which is where discussion #6293 came from.
Deltik decided on 2026-09-12 that the preference means the newer login drops the older session under every Session Save Method and Track Online setting, that the refusal goes everywhere (a user who changed device and cannot sign in is a support problem, and the refusals fed the autoban counter), and that
PRFLAN_130is reworded to say so.What changed
Eviction needs an index of an account's session ids. e107 keeps exactly one, the
sessiontable, because PHP can destroy a session only by an id you already hold. Under file, redis or memcached sessions there is no such index. So the preference decides the storage:e107_handlers/login.php#L279-L301collapses to one branch. Thesession_save_method === 'db'gate is gone and theelseifrefusal is deleted.e107_handlers/session_handler.phpresolves the save method through a new privateresolveSaveMethod(): the database while the preference is on, otherwise the storedsession_save_methodpreference. The stored preference is never written, so unticking the box restores the admin's own choice.LOGIN_MULTIPLEstays defined and listed in the error-code map, because it is published API, but its handler ininvalidLogin()goes: no core path can raise it now, and a third party that still does falls to thedefaultcase, which gives the generic message without feeding the ban counter.sessiononsession_user, which carried no index while only sites that had chosen database sessions ever ran the query. Every site with the preference on runs it now, so the column gets one. Database Verify carries it onto existing installs.PRFLAN_130is reworded, and the Session Save Method control gets its own help text, because while the preference is on that control shows a value the site is not using.Two trade-offs this design carries, and the alternative
Both are Deltik's call, not mine, which is why they are here rather than buried:
Turning the preference on or off signs everyone out, once. The credential lives in
$_SESSION, and nothing migrates it between handlers. So an admin who ticks the box is signed out by their own next click, and a site that already has it ticked with file sessions drops every session when it deploys this. The reworded help text says so; there is no migration, because there is nothing to migrate a file session into.Sites that chose file sessions now run their session I/O through MySQL while the preference is on, guests included, since e107 starts a session on every page. An operator who picked file sessions because the database is their bottleneck gets the opposite, and the only notice is the help text on those two rows.
The alternative that avoids both is a second registry: register the session id against the account at login even under a foreign handler, then check on every authenticated request that the current session is still the registered one. That keeps file sessions as file sessions, at the cost of a lookup per authenticated request, a second writer for the same table and orphan-row pruning. I did not build it, because it adds a per-request query and a second copy of a path that already exists, but it is the only reading under which a site keeps both file sessions and single-session enforcement. Say the word and I will rebuild it that way.
Two known gaps either way.
e_session_db::write()stampssession_userfromUSERID, which is still 0 during the request that performs a login, so a session that has made no request since it was created survives eviction until its next request stamps it. The post-login redirect closes that in one hop for a browser login; it stays open for the callers that pass$noredirect. Pre-existing on database sessions.setDefaultSystemConfig()returns before it resolves anything when a session is already running, which is whatsession.auto_start=1or anauto_prepend_filegives you. On such a site the forced storage never applies, the eviction deletes from a table that is not the store in use, and the preference enforces nothing. Those sites used to get the refusal, so for them this is a loss. e107's session configuration as a whole, cookie parameters and lifetime and the validation flags, has always been skipped there for the same reason, so this is one symptom of a wider pre-existing condition rather than something the change introduces. I would still rather name it than have it found.How it was tested
Four new tests, each proved to fail on master and pass with the change:
userloginTest::testDisallowMultiLoginDropsTheOlderSessionWhateverTheSaveMethodseeds asessionrow, sets the save method to files with Track Online off, and asserts the login succeeds and the row is gone.userloginTest::testDisallowMultiLoginAdmitsAnAlreadyOnlineUserseeds anonlinerow with Track Online on and asserts the login is no longer refused.e_sessionTest::testAFreshSessionTakesTheStorageThePreferenceForcesboots e107 in a subprocess and asserts a fresh session comes back fromsetDefaultSystemConfig()on database storage. It has to be a subprocess: in the suite's own process a session is already running, and that method returns early when there is one. This is what covers the coupling between the two halves.e_sessionTest::testSaveMethodIsTheDatabaseWhileMultipleLoginsAreDisallowedand its opposite number cover the resolver directly.The full unit suite on PHP 8.5 with MariaDB 10.11 passes, and PHP lint and the downgrade fixed-point check pass on all eight changed files.
I traced the behaviour table above rather than standing a site up for this branch. The table itself comes from the clean-install run recorded in #6302.
Backwards compatibility
LOGIN_MULTIPLEremains defined with the same value and stays in the error-code map, so anything testing for it still compiles and still gets a code. A third party that still raises it now falls to thedefaultarm, which changes three things: the message is the generic login failure rather thanLAN_LOGIN_24, the user note isLAN_LOGIN_26rather thanLAN_LOGIN_16, and the rolling log entry isLAN_ROLL_LOG_10, whichuserlogin::test()reports for that code too ("something's gone wrong") rather thanLAN_ROLL_LOG_07. No autoban, which is the improvement. All four language strings stay defined.session_save_methodis never written, so unticking the preference restores whatever the admin chose.AI Model
Claude Opus 5 (claude-opus-5), as e107help.
Checklist