From 5445dbefbc4bdf7d520296b57316710c61f930ca Mon Sep 17 00:00:00 2001 From: blaipr Date: Tue, 8 Sep 2026 00:36:13 +0200 Subject: [PATCH] fix: a default profile is one the administrator could have assigned MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three doors set the profile a user is auto-provisioned into, and none asked whether the administrator setting it could have granted that profile by hand. UserProfileService::assertAssignableBy() is that check, and every user create/edit door already calls it — six call sites across web and API. ConfigLdap's import and save had USER_CREATE but not assignability; ConfigAuth's save, which sets ssoDefaultProfile for every SSO auto-provisioned user, had neither. CONFIG_LDAP and CONFIG_GENERAL answer isConfigGeneral() while USER_CREATE answers isMgmUsers() — independent bits — so a delegated administrator holding both could name a profile stronger than their own and have every directory or SSO user provisioned into it. USER_CREATE is 'may create users at all'; it says nothing about how much a particular profile grants. The SSO guard fires only when the settings change, copied from ConfigLdap's, so an administrator of the rest of that page can still save it. --- .../Controllers/ConfigAuth/SaveController.php | 18 +++- .../ConfigLdap/ImportController.php | 11 ++- .../Controllers/ConfigLdap/SaveController.php | 12 ++- .../ConfigAuth/ConfigBackupOnSaveTest.php | 7 ++ .../Controllers/ConfigAuth/RefusalsTest.php | 92 ++++++++++++++++++- .../Controllers/ConfigLdap/RefusalsTest.php | 89 ++++++++++++++++-- 6 files changed, 212 insertions(+), 17 deletions(-) diff --git a/src/Infrastructure/Adapter/In/Web/Controllers/ConfigAuth/SaveController.php b/src/Infrastructure/Adapter/In/Web/Controllers/ConfigAuth/SaveController.php index 8f10c44ba..2cd1419dd 100644 --- a/src/Infrastructure/Adapter/In/Web/Controllers/ConfigAuth/SaveController.php +++ b/src/Infrastructure/Adapter/In/Web/Controllers/ConfigAuth/SaveController.php @@ -3,6 +3,7 @@ namespace SP\Infrastructure\Adapter\In\Web\Controllers\ConfigAuth; +use SP\Application\User\Ports\UserProfileService; use SP\Infrastructure\Adapter\In\Web\Controllers\Helpers\SimpleControllerHelper; use SP\Application\Application; use SP\Application\Config\Ports\ConfigBackupService; @@ -27,7 +28,8 @@ final class SaveController extends SimpleControllerBase public function __construct( Application $application, SimpleControllerHelper $simpleControllerHelper, - private readonly ConfigBackupService $configBackup + private readonly ConfigBackupService $configBackup, + private readonly UserProfileService $userProfileService ) { parent::__construct($application, $simpleControllerHelper); } @@ -67,6 +69,20 @@ private function handleAuthConfig(ConfigDataInterface $configData, EventMessage $configData->setAuthBasicEnabled(true); $configData->setAuthBasicAutoLoginEnabled($authBasicAutologinEnabled); $configData->setAuthBasicDomain($authBasicDomain); + // The same authorisation question ConfigLdap\SaveController asks about its own + // defaults, and this door asked neither half of it. These decide the group and profile + // every user auto-provisioned on their first SSO sign-in receives — + // User::createOnLogin() reads them — so setting them is a user-management decision + // reached here with isConfigGeneral(), an independent bit from the isMgmUsers() that + // USER_CREATE answers. Only when they change, so an administrator of the rest of this + // page can still save it. + if ($authSsoDefaultGroup !== $configData->getSsoDefaultGroup() + || $authSsoDefaultProfile !== $configData->getSsoDefaultProfile() + ) { + $this->checkAccess(AclActionsInterface::USER_CREATE); + $this->userProfileService->assertAssignableBy($authSsoDefaultProfile ?? 0); + } + $configData->setSsoDefaultGroup($authSsoDefaultGroup); $configData->setSsoDefaultProfile($authSsoDefaultProfile); } elseif ($configData->isAuthBasicEnabled()) { diff --git a/src/Infrastructure/Adapter/In/Web/Controllers/ConfigLdap/ImportController.php b/src/Infrastructure/Adapter/In/Web/Controllers/ConfigLdap/ImportController.php index 763ebb262..c38379dc2 100644 --- a/src/Infrastructure/Adapter/In/Web/Controllers/ConfigLdap/ImportController.php +++ b/src/Infrastructure/Adapter/In/Web/Controllers/ConfigLdap/ImportController.php @@ -24,6 +24,7 @@ namespace SP\Infrastructure\Adapter\In\Web\Controllers\ConfigLdap; +use SP\Application\User\Ports\UserProfileService; use SP\Application\Application; use SP\Domain\Core\Events\Event; use SP\Domain\Core\Events\EventMessage; @@ -53,7 +54,8 @@ final class ImportController extends SimpleControllerBase public function __construct( Application $application, SimpleControllerHelper $simpleControllerHelper, - private readonly LdapImportService $ldapImportService + private readonly LdapImportService $ldapImportService, + private readonly UserProfileService $userProfileService ) { parent::__construct($application, $simpleControllerHelper); } @@ -149,6 +151,13 @@ protected function initialize(): void // "may create a user holding any existing profile", including one with mgmUsers itself. $this->checkAccess(AclActionsInterface::USER_CREATE); + // ...and the profile every imported user is given has to be one this administrator could + // have granted by hand. See ConfigLdap\SaveController: USER_CREATE is "may create users", + // not "may grant this much". + $this->userProfileService->assertAssignableBy( + $this->request->analyzeInt('ldap_defaultprofile') ?? 0 + ); + $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 558b6937e..af2055852 100644 --- a/src/Infrastructure/Adapter/In/Web/Controllers/ConfigLdap/SaveController.php +++ b/src/Infrastructure/Adapter/In/Web/Controllers/ConfigLdap/SaveController.php @@ -24,6 +24,7 @@ namespace SP\Infrastructure\Adapter\In\Web\Controllers\ConfigLdap; +use SP\Application\User\Ports\UserProfileService; use SP\Infrastructure\Adapter\In\Web\Controllers\Helpers\SimpleControllerHelper; use SP\Application\Application; use SP\Application\Config\Ports\ConfigBackupService; @@ -56,7 +57,8 @@ final class SaveController extends SimpleControllerBase public function __construct( Application $application, SimpleControllerHelper $simpleControllerHelper, - private readonly ConfigBackupService $configBackup + private readonly ConfigBackupService $configBackup, + private readonly UserProfileService $userProfileService ) { parent::__construct($application, $simpleControllerHelper); } @@ -93,6 +95,14 @@ public function saveAction(): ActionResponse || $ldapDefaultProfile !== $configData->getLdapDefaultProfile() ) { $this->checkAccess(AclActionsInterface::USER_CREATE); + + // ...and the profile has to be one this administrator could have granted by hand. + // USER_CREATE answers "may create users at all"; it says nothing about how much a + // particular profile grants, which is what assertAssignableBy() is for and what + // every user create/edit door already asks. Without it, a delegated administrator + // holding isConfigGeneral() and isMgmUsers() could name a profile stronger than + // their own and have every LDAP user auto-provisioned into it. + $this->userProfileService->assertAssignableBy($ldapDefaultProfile ?? 0); } $ldapParams = LdapParams::fromRequest($this->request); diff --git a/tests/Unit/Infrastructure/Adapter/In/Web/Controllers/ConfigAuth/ConfigBackupOnSaveTest.php b/tests/Unit/Infrastructure/Adapter/In/Web/Controllers/ConfigAuth/ConfigBackupOnSaveTest.php index e9e18ba73..001fc56fe 100644 --- a/tests/Unit/Infrastructure/Adapter/In/Web/Controllers/ConfigAuth/ConfigBackupOnSaveTest.php +++ b/tests/Unit/Infrastructure/Adapter/In/Web/Controllers/ConfigAuth/ConfigBackupOnSaveTest.php @@ -26,6 +26,7 @@ namespace SP\Tests\Unit\Infrastructure\Adapter\In\Web\Controllers\ConfigAuth; +use SP\Application\User\Ports\UserProfileService; use SP\Application\Config\Ports\ConfigBackupService; use PHPUnit\Framework\Attributes\Group; use PHPUnit\Framework\Attributes\Test; @@ -104,6 +105,8 @@ public function savingKeepsTheConfigurationItReplaces(): void $this->signedInUserApplication(), $this->simpleControllerHelper($this->aclThatAllows(), 'configAuth', 'save'), $configBackup + , + self::createStub(UserProfileService::class) ))->saveAction(); } @@ -142,6 +145,8 @@ static function () use (&$order): void { new Application($config, new EventDispatcher(), $this->signedInUserSession()), $this->simpleControllerHelper($this->aclThatAllows(), 'configAuth', 'save'), $configBackup + , + self::createStub(UserProfileService::class) ))->saveAction(); self::assertSame(['backup', 'save'], $order); @@ -169,6 +174,8 @@ public function aRefusedSaveKeepsNothing(): void new Application($config, new EventDispatcher(), $this->signedInUserSession()), $this->simpleControllerHelper($this->aclThatAllows(), 'configAuth', 'save'), $configBackup + , + self::createStub(UserProfileService::class) ))->saveAction(); self::assertSame(ResponseStatus::WARNING, $response->status); diff --git a/tests/Unit/Infrastructure/Adapter/In/Web/Controllers/ConfigAuth/RefusalsTest.php b/tests/Unit/Infrastructure/Adapter/In/Web/Controllers/ConfigAuth/RefusalsTest.php index 1e0f7f1e4..dff4483c4 100644 --- a/tests/Unit/Infrastructure/Adapter/In/Web/Controllers/ConfigAuth/RefusalsTest.php +++ b/tests/Unit/Infrastructure/Adapter/In/Web/Controllers/ConfigAuth/RefusalsTest.php @@ -27,6 +27,9 @@ namespace SP\Tests\Unit\Infrastructure\Adapter\In\Web\Controllers\ConfigAuth; +use SP\Domain\Core\Acl\AclActionsInterface; +use SP\Domain\Common\Services\ServiceException; +use SP\Application\User\Ports\UserProfileService; use SP\Application\Config\Ports\ConfigBackupService; use PHPUnit\Framework\Attributes\Group; use PHPUnit\Framework\Attributes\Test; @@ -95,7 +98,8 @@ public function savingIsRefusedWhenTheAclDenies(): void new SaveController( $application, $this->simpleControllerHelper($acl, 'configAuth', 'save'), - self::createStub(ConfigBackupService::class) + self::createStub(ConfigBackupService::class), + self::createStub(UserProfileService::class) ); } @@ -119,7 +123,8 @@ public function savingReportsAFailureBehindItRatherThanEscaping(): void $response = (new SaveController( $application, $this->simpleControllerHelper($acl, 'configAuth', 'save'), - self::createStub(ConfigBackupService::class) + self::createStub(ConfigBackupService::class), + self::createStub(UserProfileService::class) ))->saveAction(); self::assertSame(ResponseStatus::ERROR, $response->status); @@ -127,6 +132,81 @@ public function savingReportsAFailureBehindItRatherThanEscaping(): void self::assertSame('the configuration file could not be written', $response->extra); } + /** + * The SSO defaults decide who a single-sign-on user becomes, and this door asked nothing. + * + * `ConfigAuth\SaveController` is reached with `isConfigGeneral()`. The two settings below — + * read by `User::createOnLogin()` for every user auto-provisioned on their first SSO sign-in — + * are a user-management decision, and `ConfigLdap\SaveController` already guards its identical + * pair with `USER_CREATE` plus an assignability check. This one had neither half. + * + * @throws Exception + */ + #[Test] + public function changingTheProfileSsoUsersGetIsRefusedWithoutThatPermission(): void + { + $this->expectException(UnauthorizedPageException::class); + + (new SaveController( + $this->signedInUserApplication(), + $this->simpleControllerHelper( + $this->aclThatAllowsAllBut(AclActionsInterface::USER_CREATE), + 'configAuth', + 'save', + enablingAuthBasic: true + ), + self::createStub(ConfigBackupService::class), + self::createStub(UserProfileService::class) + ))->saveAction(); + } + + /** + * ...and the profile named has to be one this administrator could have granted by hand. + * + * @throws Exception + */ + #[Test] + public function namingAnSsoProfileStrongerThanYourOwnIsRefused(): void + { + $userProfileService = $this->createStub(UserProfileService::class); + $userProfileService->method('assertAssignableBy')->willThrowException( + ServiceException::error( + 'You cannot assign a profile with more permissions than your own', + 'Please contact to the administrator' + ) + ); + + $this->expectException(ServiceException::class); + $this->expectExceptionMessage('You cannot assign a profile with more permissions than your own'); + + (new SaveController( + $this->signedInUserApplication(), + $this->simpleControllerHelper($this->aclThatAllows(), 'configAuth', 'save', enablingAuthBasic: true), + self::createStub(ConfigBackupService::class), + $userProfileService + ))->saveAction(); + } + + /** + * An ACL that allows everything except the one action named. + * + * @throws Exception + */ + private function aclThatAllowsAllBut(int $action): AclInterface + { + $acl = $this->createStub(AclInterface::class); + $acl->method('checkUserAccess')->willReturnCallback( + static fn(int $actionId): bool => $actionId !== $action + ); + + return $acl; + } + + /** + * A profile id the stored config does not already hold, so the guarded comparison sees a change. + */ + private const A_DIFFERENT_PROFILE = 99; + /** * `SimpleControllerBase` takes a `SimpleControllerHelper`, not the `WebControllerHelper` the * shared harness builds for `ControllerBase` subclasses — this mirrors @@ -137,14 +217,18 @@ public function savingReportsAFailureBehindItRatherThanEscaping(): void private function simpleControllerHelper( AclInterface $acl, string $controller = 'controller', - string $action = 'action' + string $action = 'action', + bool $enablingAuthBasic = 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); + + // The SSO defaults are only read, and only guarded, on the branch that enables auth basic. + $request->method('analyzeBool')->willReturn($enablingAuthBasic); + $request->method('analyzeInt')->willReturn($enablingAuthBasic ? self::A_DIFFERENT_PROFILE : null); $theme = $this->createStub(ThemeInterface::class); $theme->method('getUri')->willReturn('/theme'); 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 a0f667776..0fee3224a 100644 --- a/tests/Unit/Infrastructure/Adapter/In/Web/Controllers/ConfigLdap/RefusalsTest.php +++ b/tests/Unit/Infrastructure/Adapter/In/Web/Controllers/ConfigLdap/RefusalsTest.php @@ -27,6 +27,8 @@ namespace SP\Tests\Unit\Infrastructure\Adapter\In\Web\Controllers\ConfigLdap; +use SP\Domain\Common\Services\ServiceException; +use SP\Application\User\Ports\UserProfileService; use SP\Application\Config\Ports\ConfigBackupService; use PHPUnit\Framework\Attributes\Group; use PHPUnit\Framework\Attributes\Test; @@ -154,6 +156,8 @@ public function importingIsRefusedWhenTheAclDenies(): void $application, $this->simpleControllerHelper($acl, 'configLdap', 'import'), $ldapImportService + , + self::createStub(UserProfileService::class) ); } @@ -171,7 +175,8 @@ public function savingIsRefusedWhenTheAclDenies(): void new SaveController( $application, $this->simpleControllerHelper($acl, 'configLdap', 'save'), - self::createStub(ConfigBackupService::class) + self::createStub(ConfigBackupService::class), + self::createStub(UserProfileService::class) ); } @@ -198,7 +203,8 @@ public function savingReportsAFailureBehindItRatherThanEscaping(): void $response = (new SaveController( $application, $this->simpleControllerHelper($acl, 'configLdap', 'save'), - self::createStub(ConfigBackupService::class) + self::createStub(ConfigBackupService::class), + self::createStub(UserProfileService::class) ))->saveAction(); self::assertSame(ResponseStatus::ERROR, $response->status); @@ -235,6 +241,8 @@ public function importingIsRefusedWithoutThePermissionToCreateAUser(): void 'import' ), $ldapImportService + , + self::createStub(UserProfileService::class) ); } @@ -260,10 +268,10 @@ public function changingTheProfileLdapUsersGetIsRefusedWithoutThatPermission(): 'configLdap', 'save', enablingLdap: true - ) - , - self::createStub(ConfigBackupService::class) - ))->saveAction(); + ), + self::createStub(ConfigBackupService::class), + self::createStub(UserProfileService::class) + ))->saveAction(); } /** @@ -282,10 +290,10 @@ public function theConnectionStillSavesWithoutThePermissionToCreateAUser(): void $this->aclThatAllowsAllBut(AclActionsInterface::USER_CREATE), 'configLdap', 'save' - ) - , - self::createStub(ConfigBackupService::class) - ))->saveAction(); + ), + self::createStub(ConfigBackupService::class), + self::createStub(UserProfileService::class) + ))->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 @@ -294,6 +302,67 @@ public function theConnectionStillSavesWithoutThePermissionToCreateAUser(): void self::assertSame('Error while saving the configuration', $response->subject); } + /** + * ...and the profile named has to be one this administrator could have granted by hand. + * + * `USER_CREATE` answers "may create users at all". It says nothing about how much a particular + * profile grants, which is what `UserProfileService::assertAssignableBy()` is for and what + * every user create/edit door — both web save controllers and both API ones — already asks. + * Without it here, a delegated administrator holding `isConfigGeneral()` and `isMgmUsers()`, + * two independent profile bits, could name a profile stronger than their own and have every + * LDAP user auto-provisioned into it. + * + * @throws Exception + */ + #[Test] + public function namingAProfileStrongerThanYourOwnIsRefused(): void + { + $this->expectException(ServiceException::class); + $this->expectExceptionMessage('You cannot assign a profile with more permissions than your own'); + + (new SaveController( + $this->signedInUserApplication(), + $this->simpleControllerHelper($this->aclThatAllows(), 'configLdap', 'save', enablingLdap: true), + self::createStub(ConfigBackupService::class), + $this->userProfileServiceThatRefuses() + ))->saveAction(); + } + + /** + * The same for the import, which hands every user it creates that profile directly. + * + * @throws Exception + */ + #[Test] + public function importingUsersIntoAProfileStrongerThanYourOwnIsRefused(): void + { + $this->expectException(ServiceException::class); + $this->expectExceptionMessage('You cannot assign a profile with more permissions than your own'); + + (new ImportController( + $this->signedInUserApplication(), + $this->simpleControllerHelper($this->aclThatAllows(), 'configLdap', 'import', enablingLdap: true), + self::createStub(LdapImportService::class), + $this->userProfileServiceThatRefuses() + ))->importAction(); + } + + /** + * @throws Exception + */ + private function userProfileServiceThatRefuses(): UserProfileService + { + $userProfileService = $this->createStub(UserProfileService::class); + $userProfileService->method('assertAssignableBy')->willThrowException( + ServiceException::error( + 'You cannot assign a profile with more permissions than your own', + 'Please contact to the administrator' + ) + ); + + return $userProfileService; + } + /** * 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