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');