Skip to content

fix: a preset naming a deleted user does not break account saving - #882

Merged
blaipr merged 2 commits into
mainfrom
fix/a-preset-naming-a-deleted-user-does-not-break-saving
Aug 26, 2026
Merged

fix: a preset naming a deleted user does not break account saving#882
blaipr merged 2 commits into
mainfrom
fix/a-preset-naming-a-deleted-user-does-not-break-saving

Conversation

@blaipr

@blaipr blaipr commented Aug 26, 2026

Copy link
Copy Markdown
Member

A fixed permission preset keeps the users and groups it shares new accounts with inside a
serialized blob. No foreign key reaches in there — the one on ItemPreset covers the
preset's own scope columns, not its contents — so a user or group named in a preset can be
deleted with nothing to stop it, and nothing to say it mattered.

It matters at the next save. AccountToUser and AccountToUserGroup do have foreign keys
on those ids, so the insert fails with error 1452, inside the transaction
Account::create() and update() run in, and the whole save rolls back. Not for one person
and not once: every account create and edit by anybody the preset resolves to, until an
administrator works out which preset holds the stale id and edits it. The message they get
says "Referenced record not found", which does not point at a preset.

The ids are filtered to the ones that still exist before they are applied. What is left of
the preset is applied; what has been deleted is dropped.

getExistingIds() is new on the user and group repositories, and answers with plain ids
rather than rows: Simple declares no properties — every read goes through the model's
outer-property bag — so returning rows means either static analysis cannot see the column
or the reads have to go through array access at every call site. It is one query per list,
skipped entirely when the list is empty, and it runs only for a fixed permission preset
that names somebody.

Checked by dropping the filter on one of the four lists: the new test fails with the stale
id still being handed to the insert.

The test needed the existence stub to consult a per-test list of deleted ids rather than
being re-stubbed in the test itself — the first stub registered is the one that answers, so
a per-test override of a setUp stub silently does nothing, which is how the first version
of this test passed against the unfixed code.

blaipr added 2 commits August 26, 2026 21:12
A fixed permission preset keeps the users and groups it shares new accounts with inside a
serialized blob. No foreign key reaches in there — the one on ItemPreset covers the
preset's own scope columns, not its contents — so a user or group named in a preset can be
deleted with nothing to stop it, and nothing to say it mattered.

It matters at the next save. AccountToUser and AccountToUserGroup *do* have foreign keys
on those ids, so the insert fails with error 1452, inside the transaction
Account::create() and update() run in, and the whole save rolls back. Not for one person
and not once: every account create and edit by anybody the preset resolves to, until an
administrator works out which preset holds the stale id and edits it. The message they get
says "Referenced record not found", which does not point at a preset.

The ids are filtered to the ones that still exist before they are applied. What is left of
the preset is applied; what has been deleted is dropped.

getExistingIds() is new on the user and group repositories, and answers with plain ids
rather than rows: Simple declares no properties — every read goes through the model's
outer-property bag — so returning rows means either static analysis cannot see the column
or the reads have to go through array access at every call site. It is one query per list,
skipped entirely when the list is empty, and it runs only for a fixed permission preset
that names somebody.

Checked by dropping the filter on one of the four lists: the new test fails with the stale
id still being handed to the insert.

The test needed the existence stub to consult a per-test list of deleted ids rather than
being re-stubbed in the test itself — the first stub registered is the one that answers, so
a per-test override of a setUp stub silently does nothing, which is how the first version
of this test passed against the unfixed code.
PHPCS (PSR2) caught the blank line my helper insertion left before the closing brace.
Worth noting for next time: `composer phpcs` is a separate CI gate from PHPStan, and I
had only been running the latter.
@blaipr
blaipr merged commit 4ee5ddb into main Aug 26, 2026
8 checks passed
@blaipr
blaipr deleted the fix/a-preset-naming-a-deleted-user-does-not-break-saving branch August 26, 2026 19:27
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