Skip to content

fix: the group and profile new directory users get cannot be deleted - #889

Merged
blaipr merged 1 commit into
mainfrom
fix/the-directory-default-group-cannot-be-deleted
Aug 26, 2026
Merged

fix: the group and profile new directory users get cannot be deleted#889
blaipr merged 1 commit into
mainfrom
fix/the-directory-default-group-cannot-be-deleted

Conversation

@blaipr

@blaipr blaipr commented Aug 26, 2026

Copy link
Copy Markdown
Member

ldapDefaultGroup, ssoDefaultGroup, ldapDefaultProfile and ssoDefaultProfile are
plain ints in config.xml. Nothing points at UserGroup or UserProfile from there, so no
foreign key refuses deleting the row they name — and the RESTRICT on User.userGroupId /
User.userProfileId only catches one somebody currently holds, which is precisely not the
case for a group or profile being tidied up because its members have moved on.

So the delete succeeded cleanly, and every auto-provisioned login afterwards died: with no
row to point at, User::createOnLogin() violates the NOT NULL foreign key, which
LoginAuthHandler catches and reports as "Internal error, check the event log". Directory
sign-in stops working for everyone who does not already have a local account, and nothing
connects that to a group somebody deleted last week.

Both deletes refuse it now, naming which setting holds it.

The batch deletes are guarded too, because they go to the repository directly rather than
through delete() — the foreign keys still stand behind them whichever door is used, but
nothing in the database knows about a setting in config.xml.

Checked by making the comparison always false: all four new tests fail.

Unrelated, and recorded rather than claimed fixed: one full unit run during this change
failed on AccountTest::testCreateCannotChangePermissions, which then passed in isolation
and in three consecutive full runs. I could not reproduce it — it is not the id-zero trap
(forcing the id to 0 passes) and the harness seeds an all-false ProfileData, so it is not
the profile either. Noting it so the next person who sees it does not start from scratch.

`ldapDefaultGroup`, `ssoDefaultGroup`, `ldapDefaultProfile` and `ssoDefaultProfile` are
plain ints in config.xml. Nothing points at UserGroup or UserProfile from there, so no
foreign key refuses deleting the row they name — and the RESTRICT on User.userGroupId /
User.userProfileId only catches one somebody currently holds, which is precisely not the
case for a group or profile being tidied up because its members have moved on.

So the delete succeeded cleanly, and every auto-provisioned login afterwards died: with no
row to point at, `User::createOnLogin()` violates the NOT NULL foreign key, which
`LoginAuthHandler` catches and reports as "Internal error, check the event log". Directory
sign-in stops working for everyone who does not already have a local account, and nothing
connects that to a group somebody deleted last week.

Both deletes refuse it now, naming which setting holds it.

The batch deletes are guarded too, because they go to the repository directly rather than
through delete() — the foreign keys still stand behind them whichever door is used, but
nothing in the database knows about a setting in config.xml.

Checked by making the comparison always false: all four new tests fail.

Unrelated, and recorded rather than claimed fixed: one full unit run during this change
failed on AccountTest::testCreateCannotChangePermissions, which then passed in isolation
and in three consecutive full runs. I could not reproduce it — it is not the id-zero trap
(forcing the id to 0 passes) and the harness seeds an all-false ProfileData, so it is not
the profile either. Noting it so the next person who sees it does not start from scratch.
@blaipr
blaipr merged commit 30755cd into main Aug 26, 2026
8 checks passed
@blaipr
blaipr deleted the fix/the-directory-default-group-cannot-be-deleted branch August 26, 2026 22:18
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