fix: hold a login unique in the database, not just in the application - #825
Merged
Conversation
`uk_User_01` covered `(login, ssoLogin)` rather than `login`, and MySQL treats
NULLs in a unique index as distinct — so two rows with the same login and no SSO
login, which is most of them, were both accepted:
INSERT ('alice', NULL) -> ok
INSERT ('alice', NULL) -> ok
The application's rule is not that. `checkDuplicatedOnAdd()` refuses any login
collision whatever the SSO login is, but it is a SELECT followed by an INSERT:
two requests arriving together both find nothing and both insert, and the index
is what has to stop the second. Two simultaneous first-time logins through
`createOnLogin()` are enough. This is the same shape as the Client key in #806.
`getByLogin()` then answers with `LIMIT 1` and no ordering, so which of the two
a login resolves to is the server's choice — and `DatabaseAuth` is asking.
`ssoLogin` is deliberately left alone. The application exempts an empty one
(`ssoLogin IS NOT NULL AND ssoLogin <> ''`), `Filter::getString('')` returns
`''` rather than null, and the user form always submits the field — so a unique
index over `ssoLogin` refuses the second user who has no SSO login at all. That
is verified rather than assumed, and the integration test rules the regression
out on purpose.
Schema only, following #806: which of two duplicate logins is the real person,
and what becomes of the accounts the other owns, is an administrator's decision
rather than a migration's. A fresh install now gets the constraint.
blaipr
added a commit
that referenced
this pull request
Aug 18, 2026
`UserProfile` and `UserGroup` each refuse a name that is already taken — `UPPER(:name) = UPPER(name)` in `checkDuplicatedOnAdd()` — and neither table had a unique index at all. That check is a SELECT followed by an INSERT, so two requests arriving together both find nothing and both insert, with nothing underneath to stop the second: two groups called Admins, and no way to tell which one a permission refers to. A plain unique index is the application's rule exactly, because `name` collates utf8mb4_unicode_ci — 'Admins' and 'ADMINS' collide, which is what the UPPER() comparison was asking for. Confirmed against a server, with the whole schema reloaded to check nothing else objects. This completes the sweep the User login started. Every `checkDuplicated*` in the repositories now has an index behind it: Category, Client and Tag by hash, PublicLink by hash and by account, AuthToken by (token, actionId) — a composite because that is its rule — User by login, and these two by name. The schema test covers all five through one data provider, so a table added later with a PHP-only uniqueness rule is caught by adding a row to it. Schema only, as in #806 and #825: which of two identically named groups is the real one, and what becomes of the permissions pointing at the other, is an administrator's decision rather than a migration's.
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.
uk_User_01covered(login, ssoLogin)rather thanlogin. MySQL treats NULLs in a unique index as distinct, so two rows with the same login and no SSO login — which is most users — are both accepted:The application's rule is not that one.
checkDuplicatedOnAdd()refuses any login collision whatever the SSO login is:But that is a SELECT followed by an INSERT, so two requests arriving together both find nothing and both insert, and the index is what has to stop the second. Two simultaneous first-time logins through
createOnLogin()are enough — the same shape as theClientkey in #806, and the pattern already recorded for Category/Client/Tag.What makes it more than untidy:
getByLogin()answers withLIMIT 1and no ordering, so which of the two rows a login resolves to is the server's choice.DatabaseAuthis the caller.ssoLoginis deliberately not coveredMy first draft added
UNIQUE(ssoLogin)for symmetry. That would have been a regression, and it is verified rather than reasoned:Filter::getString('')returns'', not null;login_sso, so a blank field stores'';''is not distinct the way NULL is — two of them collide.So a unique index over
ssoLoginwould refuse the second user who has no SSO login at all. The application's own rule exempts the empty case for exactly this reason.usersWithNoSsoLoginDoNotCollideWithEachOtherexists to rule the regression out.Schema only
Following #806: which of two duplicate logins is the real person, and what becomes of the accounts the other owns, is an administrator's decision rather than a migration's. A fresh install now gets the constraint.
Tests
SchemaEnforcesIdentityTest::aLoginIsHeldUniqueOnItsOwnreads the shipped schema — a composite key over(login, ssoLogin)does not satisfy it.LoginIsUniqueTestbuilds theUsertable fromdbstructure.sqlitself on a real server and inserts the duplicate. Restoring the composite key fails it with Failed asserting that exception of type "PDOException" is thrown — the duplicate insert simply succeeds, which is the defect.3974 unit + 976 integration pass; PHPStan level 6 on
srcand PHPCS clean.