From 84203c039dd7a14e52dd4cd1f7194dee96fc007b Mon Sep 17 00:00:00 2001 From: blaipr Date: Mon, 24 Aug 2026 01:41:30 +0200 Subject: [PATCH] fix: choosing the profile LDAP users get is user management MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- .../ConfigLdap/ImportController.php | 7 + .../Controllers/ConfigLdap/SaveController.php | 18 ++- .../Controllers/ConfigLdap/RefusalsTest.php | 121 +++++++++++++++++- 3 files changed, 142 insertions(+), 4 deletions(-) diff --git a/src/Infrastructure/Adapter/In/Web/Controllers/ConfigLdap/ImportController.php b/src/Infrastructure/Adapter/In/Web/Controllers/ConfigLdap/ImportController.php index 41c8b40b9..763ebb262 100644 --- a/src/Infrastructure/Adapter/In/Web/Controllers/ConfigLdap/ImportController.php +++ b/src/Infrastructure/Adapter/In/Web/Controllers/ConfigLdap/ImportController.php @@ -142,6 +142,13 @@ protected function initialize(): void $this->checks(); $this->checkAccess(AclActionsInterface::CONFIG_LDAP); + // This action creates sysPass users, with a profile the caller names in the request, so it + // needs the permission that creating a user needs. CONFIG_LDAP answers isConfigGeneral() + // and USER_CREATE answers isMgmUsers(), and a profile is thirty independent booleans — + // none of them implies another. Without this, "may configure the LDAP connection" reached + // "may create a user holding any existing profile", including one with mgmUsers itself. + $this->checkAccess(AclActionsInterface::USER_CREATE); + $this->extensionChecker->checkLdap(true); } } diff --git a/src/Infrastructure/Adapter/In/Web/Controllers/ConfigLdap/SaveController.php b/src/Infrastructure/Adapter/In/Web/Controllers/ConfigLdap/SaveController.php index 15b654908..7b0cfae8b 100644 --- a/src/Infrastructure/Adapter/In/Web/Controllers/ConfigLdap/SaveController.php +++ b/src/Infrastructure/Adapter/In/Web/Controllers/ConfigLdap/SaveController.php @@ -66,11 +66,25 @@ public function saveAction(): ActionResponse $eventMessage->addDescription(__u('LDAP enabled')); } - $ldapParams = LdapParams::fromRequest($this->request); - $ldapDefaultGroup = $this->request->analyzeInt('ldap_defaultgroup'); $ldapDefaultProfile = $this->request->analyzeInt('ldap_defaultprofile'); + // Before any of the work, because it is an authorisation question. These two decide the + // group and profile every user auto-provisioned on their first LDAP sign-in receives — + // User::createOnLogin() reads them, and LoginAuthHandler creates that user whenever a + // directory bind succeeds and no local record exists. Setting them is a user-management + // decision rather than a connection setting, and it needs the permission that creating + // a user needs: this action is reached with isConfigGeneral(), an independent bit from + // the isMgmUsers() that USER_CREATE answers. Only when they change, so an administrator + // of the connection can still save the rest of this page. + if ($ldapDefaultGroup !== $configData->getLdapDefaultGroup() + || $ldapDefaultProfile !== $configData->getLdapDefaultProfile() + ) { + $this->checkAccess(AclActionsInterface::USER_CREATE); + } + + $ldapParams = LdapParams::fromRequest($this->request); + $configData->setLdapEnabled(true); $configData->setLdapType($ldapParams->getType()->value); $configData->setLdapTlsEnabled($ldapParams->isTlsEnabled()); diff --git a/tests/Unit/Infrastructure/Adapter/In/Web/Controllers/ConfigLdap/RefusalsTest.php b/tests/Unit/Infrastructure/Adapter/In/Web/Controllers/ConfigLdap/RefusalsTest.php index 31acb5bd1..a4f0292b3 100644 --- a/tests/Unit/Infrastructure/Adapter/In/Web/Controllers/ConfigLdap/RefusalsTest.php +++ b/tests/Unit/Infrastructure/Adapter/In/Web/Controllers/ConfigLdap/RefusalsTest.php @@ -31,6 +31,7 @@ use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\MockObject\Exception; use RuntimeException; +use SP\Domain\Core\Acl\AclActionsInterface; use SP\Application\Application; use SP\Application\Auth\Ports\LdapCheckService; use SP\Application\Config\Ports\ConfigFileService; @@ -202,6 +203,113 @@ public function savingReportsAFailureBehindItRatherThanEscaping(): void self::assertSame('the configuration file could not be written', $response->extra); } + /** + * Importing users from the directory needs the permission that creating a user needs. + * + * The import creates sysPass users, with the group and profile named in the request. It was + * reached with CONFIG_LDAP alone, which `Acl` answers with `isConfigGeneral()` — an entirely + * separate bit from the `isMgmUsers()` that USER_CREATE answers, and the profile list the form + * offers is unfiltered, so a holder of the first could mint users carrying any existing + * profile, `mgmUsers` included. + * + * @throws Exception + */ + #[Test] + public function importingIsRefusedWithoutThePermissionToCreateAUser(): void + { + $ldapImportService = $this->createMock(LdapImportService::class); + $ldapImportService->expects(self::never())->method('importUsers'); + + $application = $this->signedInUserApplication(); + + $this->expectException(UnauthorizedPageException::class); + + new ImportController( + $application, + $this->simpleControllerHelper( + $this->aclThatAllowsAllBut(AclActionsInterface::USER_CREATE), + 'configLdap', + 'import' + ), + $ldapImportService + ); + } + + /** + * And so does changing the profile that LDAP users are created with. + * + * `User::createOnLogin()` reads `ldapDefaultProfile`, and `LoginAuthHandler` creates that user + * on any first successful directory bind — so writing this setting decides the profile of every + * user auto-provisioned afterwards. It is the same escalation as the import, reached without + * importing anything. + * + * @throws Exception + */ + #[Test] + public function changingTheProfileLdapUsersGetIsRefusedWithoutThatPermission(): void + { + $this->expectException(UnauthorizedPageException::class); + + (new SaveController( + $this->signedInUserApplication(), + $this->simpleControllerHelper( + $this->aclThatAllowsAllBut(AclActionsInterface::USER_CREATE), + 'configLdap', + 'save', + enablingLdap: true + ) + ))->saveAction(); + } + + /** + * The rest of the page still saves without it. The guard is on the two settings that decide who + * a directory user becomes, not on administering the connection — otherwise this would take the + * feature away from the permission that is meant to have it. + * + * @throws Exception + */ + #[Test] + public function theConnectionStillSavesWithoutThePermissionToCreateAUser(): void + { + $response = (new SaveController( + $this->applicationWhoseConfigSaveThrows(), + $this->simpleControllerHelper( + $this->aclThatAllowsAllBut(AclActionsInterface::USER_CREATE), + 'configLdap', + 'save' + ) + ))->saveAction(); + + // Reaching saveConfig() at all is the point: the request carries no ldap_enabled flag, so + // this is the "disable it" path, which touches neither of the two guarded settings. That it + // then reports the stubbed write failure is how we know it got that far. + self::assertSame(ResponseStatus::ERROR, $response->status); + self::assertSame('Error while saving the configuration', $response->subject); + } + + /** + * An ACL that allows everything except the one action named, so a test can be specific about + * which permission it is withholding. `aclThatRefuses()` refuses the lot, which cannot tell a + * missing USER_CREATE from a missing CONFIG_LDAP. + * + * @throws Exception + */ + /** + * A profile id the stored config does not already hold, so the guarded comparison sees a change. + */ + private const A_DIFFERENT_PROFILE = 99; + + private function aclThatAllowsAllBut(int $action): AclInterface + { + $acl = $this->createStub(AclInterface::class); + $acl->method('checkUserAccess')->willReturnCallback( + static fn(int $actionId): bool => $actionId !== $action + ); + $acl->method('getRouteFor')->willReturn('a/route'); + + return $acl; + } + /** * `SimpleControllerBase` takes a `SimpleControllerHelper`, not the `WebControllerHelper` the * shared harness builds for `ControllerBase` subclasses — this mirrors @@ -212,14 +320,23 @@ public function savingReportsAFailureBehindItRatherThanEscaping(): void private function simpleControllerHelper( AclInterface $acl, string $controller = 'controller', - string $action = 'action' + string $action = 'action', + bool $enablingLdap = false ): SimpleControllerHelper { $request = $this->createStub(RequestService::class); $request->method('isAjax')->willReturn(false); $request->method('getServer')->willReturn('0'); $request->method('analyzeString')->willReturn(null); $request->method('analyzeArray')->willReturn(null); - $request->method('analyzeInt')->willReturn(null); + + if ($enablingLdap) { + // saveAction() only reaches the settings this is about when the request is turning LDAP + // on; with the flag absent it takes the "disable it" path and never reads them. + $request->method('analyzeBool')->willReturn(true); + $request->method('analyzeInt')->willReturn(self::A_DIFFERENT_PROFILE); + } else { + $request->method('analyzeInt')->willReturn(null); + } $theme = $this->createStub(ThemeInterface::class); $theme->method('getUri')->willReturn('/theme');