Skip to content

fix: hold a login unique in the database, not just in the application - #825

Merged
blaipr merged 1 commit into
mainfrom
fix/a-login-is-unique-in-the-database
Aug 18, 2026
Merged

fix: hold a login unique in the database, not just in the application#825
blaipr merged 1 commit into
mainfrom
fix/a-login-is-unique-in-the-database

Conversation

@blaipr

@blaipr blaipr commented Aug 18, 2026

Copy link
Copy Markdown
Member

uk_User_01 covered (login, ssoLogin) rather than login. 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:

INSERT ('alice', NULL)  -> ok
INSERT ('alice', NULL)  -> ok        <- two users called alice

The application's rule is not that one. checkDuplicatedOnAdd() refuses any login collision whatever the SSO login is:

UPPER(:login) = UPPER(login)
OR (UPPER(:ssoLogin) = UPPER(ssoLogin) AND ssoLogin IS NOT NULL AND ssoLogin <> '')
OR ...

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 the Client key in #806, and the pattern already recorded for Category/Client/Tag.

What makes it more than untidy: getByLogin() answers with LIMIT 1 and no ordering, so which of the two rows a login resolves to is the server's choice. DatabaseAuth is the caller.

ssoLogin is deliberately not covered

My 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;
  • the user form always submits login_sso, so a blank field stores '';
  • '' is not distinct the way NULL is — two of them collide.
INSERT ('alice',''), ('bob','')  ->  ERROR 1062 Duplicate entry '' for key 'uk2'

So a unique index over ssoLogin would refuse the second user who has no SSO login at all. The application's own rule exempts the empty case for exactly this reason. usersWithNoSsoLoginDoNotCollideWithEachOther exists 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::aLoginIsHeldUniqueOnItsOwn reads the shipped schema — a composite key over (login, ssoLogin) does not satisfy it.
  • LoginIsUniqueTest builds the User table from dbstructure.sql itself 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 src and PHPCS clean.

`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
blaipr merged commit c7d7ad1 into main Aug 18, 2026
8 checks passed
@blaipr
blaipr deleted the fix/a-login-is-unique-in-the-database branch August 18, 2026 22:28
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant