Skip to content

fix: a caller cannot assign a profile past their own - #874

Merged
blaipr merged 2 commits into
mainfrom
fix/an-api-caller-cannot-assign-a-profile-past-their-own
Aug 24, 2026
Merged

fix: a caller cannot assign a profile past their own#874
blaipr merged 2 commits into
mainfrom
fix/an-api-caller-cannot-assign-a-profile-past-their-own

Conversation

@blaipr

@blaipr blaipr commented Aug 24, 2026

Copy link
Copy Markdown
Member

isAdminApp and isAdminAcc are gated on the caller already holding them, in the web
form and in the API's user endpoints alike. The profile a user is pointed at was not
gated at all, at either door — and a profile is where the other thirty permissions live.
Assigning one hands the user everything on it, so "may manage users" was "may hold any
permission in the installation" in two steps: create or edit a user, point them at the
administrator profile, sign in as them.

The codebase already answers this question for the other door. ProfileData::constrainedTo()
strips a written profile down to what the writer holds, and the API's ProfileBase
applies it — because otherwise "may manage profiles" was "may become an administrator".
The same escalation was still open next to it, by referencing a profile instead of
writing one.

grantsBeyond() is that question asked of a referenced profile, in terms of the existing
one, so a permission added later is covered without either being revisited.

Placed in UserProfileService and called from the web's UserSaveBase and the API's
UserBase, rather than in UserService::create() — that is also the path the installer
and LDAP auto-provisioning take, where there is no signed-in caller to constrain against.

Two things the tests settled:

A profile that does not exist is let through rather than refused. The integration suite
caught the first version changing what a bad id reports — "Profile not found" instead of
the error the foreign key produces. A profile that is not there grants nothing, so there
is nothing for this guard to say about it.

An application administrator's profile is not even read, which is what the never() in
that test asserts. Checked by making the comparison always false: the refusal test fails
and the two that pin what must still work do not.

blaipr added 2 commits August 24, 2026 02:11
`isAdminApp` and `isAdminAcc` are gated on the caller already holding them, in the web
form and in the API's user endpoints alike. The profile a user is pointed at was not
gated at all, at either door — and a profile is where the other thirty permissions live.
Assigning one hands the user everything on it, so "may manage users" was "may hold any
permission in the installation" in two steps: create or edit a user, point them at the
administrator profile, sign in as them.

The codebase already answers this question for the other door. `ProfileData::constrainedTo()`
strips a *written* profile down to what the writer holds, and the API's `ProfileBase`
applies it — because otherwise "may manage profiles" was "may become an administrator".
The same escalation was still open next to it, by referencing a profile instead of
writing one.

`grantsBeyond()` is that question asked of a referenced profile, in terms of the existing
one, so a permission added later is covered without either being revisited.

Placed in `UserProfileService` and called from the web's `UserSaveBase` and the API's
`UserBase`, rather than in `UserService::create()` — that is also the path the installer
and LDAP auto-provisioning take, where there is no signed-in caller to constrain against.

Two things the tests settled:

A profile that does not exist is let through rather than refused. The integration suite
caught the first version changing what a bad id reports — "Profile not found" instead of
the error the foreign key produces. A profile that is not there grants nothing, so there
is nothing for this guard to say about it.

An application administrator's profile is not even read, which is what the `never()` in
that test asserts. Checked by making the comparison always false: the refusal test fails
and the two that pin what must still work do not.
PHPStan resolved the bare NoSuchItemException in UserSaveBase's new docblock against the
controller's own namespace, where there is no such class, so the @throws was not a
Throwable. The other three exceptions on the same docblock were already imported.
@blaipr
blaipr merged commit 5068cb8 into main Aug 24, 2026
8 checks passed
@blaipr
blaipr deleted the fix/an-api-caller-cannot-assign-a-profile-past-their-own branch August 24, 2026 00:36
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