fix: choosing the profile LDAP users get is user management - #871
Merged
blaipr merged 1 commit intoAug 23, 2026
Merged
Conversation
`Acl::checkUserAccess()` answers CONFIG_LDAP with `isConfigGeneral()` and USER_CREATE with `isMgmUsers()`. A profile is thirty independent booleans and none implies another — the docs say so explicitly. Two paths reached across that line. The LDAP import creates sysPass users. `ImportController` was gated on CONFIG_LDAP alone, and it takes `ldap_defaultprofile` straight from the request (`getImportParams()` → `analyzeInt`), which `LdapImport::importUsers()` writes onto every user it creates. The Config Manager page that renders the form lists every profile unfiltered, so a holder of the config bit could pick any of them — including one carrying `mgmUsers` itself, or `accViewPass`. `ProfileData::constrainedTo()` exists to stop exactly this escalation when a profile is *edited*; it never runs here, because nothing is being edited, an existing profile is just being handed to a new user through the wrong door. The same escalation is reachable without importing anything. `SaveController` persists `ldapDefaultProfile`, and `User::createOnLogin()` reads it for every user auto-provisioned on a first directory sign-in — which `LoginAuthHandler::authLdap()` does unconditionally whenever a bind succeeds and no local record exists. Point that setting at a powerful profile once and the next LDAP login is created holding it. Both now require USER_CREATE as well: - The import requires it outright. It creates users; it needs the permission for creating users. - The save requires it only when `ldap_defaultgroup` or `ldap_defaultprofile` actually changes, so administering the connection — the thing the config permission is for — is untouched. The check runs before `LdapParams::fromRequest()` rather than after, because it is an authorisation question and belongs ahead of the work. The tests needed an ACL double that allows everything except one named action: refusing the lot cannot tell a missing USER_CREATE from a missing CONFIG_LDAP, and would have passed against the unfixed code. There is a third test that the connection still saves without the permission, so a guard that simply refused the whole page would not satisfy these. Checked by removing both guards: the two new tests fail and the rest do not.
blaipr
deleted the
fix/choosing-the-profile-ldap-users-get-is-user-management
branch
August 23, 2026 23:58
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.
Acl::checkUserAccess()answers CONFIG_LDAP withisConfigGeneral()and USER_CREATE withisMgmUsers(). A profile is thirty independent booleans and none implies another — thedocs say so explicitly. Two paths reached across that line.
The LDAP import creates sysPass users.
ImportControllerwas gated on CONFIG_LDAP alone,and it takes
ldap_defaultprofilestraight from the request(
getImportParams()→analyzeInt), whichLdapImport::importUsers()writes onto everyuser it creates. The Config Manager page that renders the form lists every profile
unfiltered, so a holder of the config bit could pick any of them — including one carrying
mgmUsersitself, oraccViewPass.ProfileData::constrainedTo()exists to stop exactlythis escalation when a profile is edited; it never runs here, because nothing is being
edited, an existing profile is just being handed to a new user through the wrong door.
The same escalation is reachable without importing anything.
SaveControllerpersistsldapDefaultProfile, andUser::createOnLogin()reads it for every user auto-provisionedon a first directory sign-in — which
LoginAuthHandler::authLdap()does unconditionallywhenever a bind succeeds and no local record exists. Point that setting at a powerful
profile once and the next LDAP login is created holding it.
Both now require USER_CREATE as well:
users.
ldap_defaultgrouporldap_defaultprofileactuallychanges, so administering the connection — the thing the config permission is for — is
untouched. The check runs before
LdapParams::fromRequest()rather than after, becauseit is an authorisation question and belongs ahead of the work.
The tests needed an ACL double that allows everything except one named action: refusing
the lot cannot tell a missing USER_CREATE from a missing CONFIG_LDAP, and would have
passed against the unfixed code. There is a third test that the connection still saves
without the permission, so a guard that simply refused the whole page would not satisfy
these.
Checked by removing both guards: the two new tests fail and the rest do not.