fix: a caller cannot assign a profile past their own

src/Application/User/Ports/UserProfileService.php

@@ -47,6 +47,23 @@ interface UserProfileService
      * @throws QueryException
      * @throws NoSuchItemException
      */
+    /**
+     * Refuse a profile that grants more than the signed-in user holds themselves
+     *
+     * Assigning a user a profile hands them everything on it, so a delegate who may create or edit
+     * users must not be able to point one at a profile stronger than their own — otherwise "may
+     * manage users" is "may become an administrator" in two steps. Application administrators are
+     * not constrained; they hold everything by definition.
+     *
+     * @param int $profileId
+     *
+     * @throws ServiceException When the profile grants a permission the caller does not hold
+     * @throws ConstraintException
+     * @throws QueryException
+     * @throws NoSuchItemException
+     */
+    public function assertAssignableBy(int $profileId): void;
+
     public function getById(int $id): UserProfileModel;
 
     /**

src/Application/User/Services/UserProfile.php

@@ -28,6 +28,7 @@
 use SP\Application\Application;
 use SP\Domain\Common\Models\Simple;
 use SP\Domain\Common\Services\Service;
+use SP\Domain\User\Models\ProfileData;
 use SP\Domain\Common\Services\ServiceException;
 use SP\Domain\Core\Dtos\ItemSearchDto;
 use SP\Domain\Core\Exceptions\ConstraintException;
@@ -56,6 +57,32 @@ public function __construct(Application $application, private readonly UserProfi
         parent::__construct($application);
     }
 
+    /**
+     * @inheritDoc
+     */
+    public function assertAssignableBy(int $profileId): void
+    {
+        if ($this->context->getUserData()->isAdminApp) {
+            return;
+        }
+
+        try {
+            $profileData = $this->getById($profileId)->hydrate(ProfileData::class) ?? new ProfileData();
+        } catch (NoSuchItemException) {
+            // Nothing to constrain: a profile that does not exist grants nothing. Refusing here
+            // would change what a bad id reports — the foreign key already rejects it, and this
+            // guard is about how much a profile grants, not whether it is there.
+            return;
+        }
+
+        if ($profileData->grantsBeyond($this->context->getUserProfile())) {
+            throw ServiceException::error(
+                __u('You cannot assign a profile with more permissions than your own'),
+                __u('Please contact to the administrator')
+            );
+        }
+    }
+
     /**
      * @throws ConstraintException
      * @throws QueryException

src/Domain/User/Models/ProfileData.php

@@ -42,6 +42,20 @@ class ProfileData extends Model
      * Every boolean on the model is intersected, so a permission added later is covered without
      * this having to be revisited.
      */
+    /**
+     * Whether this profile grants anything the actor does not itself hold.
+     *
+     * `constrainedTo()` answers the question for a profile being *written*. This answers it for one
+     * being *referenced*: assigning a user a profile hands them everything on it, so a delegate who
+     * may create or edit users must not be able to point one at a profile stronger than their own.
+     * Otherwise "may manage users" is "may become an administrator" in two steps, which is the same
+     * escalation `constrainedTo()` exists to stop through the other door.
+     */
+    public function grantsBeyond(?ProfileData $actorProfile): bool
+    {
+        return $this->constrainedTo($actorProfile)->toArray() !== $this->toArray();
+    }
+
     public function constrainedTo(?ProfileData $actorProfile): ProfileData
     {
         $mutations = [];

src/Infrastructure/Adapter/In/Api/Controllers/User/CreateController.php

@@ -35,6 +35,10 @@ public function createAction(): ApiResponse
 
     private function buildUserData(): User
     {
+        $userProfileId = $this->apiService->getParamInt('userProfileId', true);
+
+        $this->assertProfileIsAssignable($userProfileId);
+
         return new User([
             'name'          => $this->apiService->getParamString('name', true),
             'login'         => $this->apiService->getParamString('login', true),
@@ -45,7 +49,7 @@ private function buildUserData(): User
             'email'         => $this->apiService->getParamString('email'),
             'notes'         => $this->apiService->getParamString('notes'),
             'userGroupId'   => $this->apiService->getParamInt('userGroupId', true),
-            'userProfileId' => $this->apiService->getParamInt('userProfileId', true),
+            'userProfileId' => $userProfileId,
             'isAdminApp'    => $this->context->getUserData()->isAdminApp && (bool) $this->apiService->getParamInt('isAdminApp'),
             'isAdminAcc'    => $this->context->getUserData()->isAdminApp && (bool) $this->apiService->getParamInt('isAdminAcc'),
             'isDisabled'    => (bool) $this->apiService->getParamInt('isDisabled'),

src/Infrastructure/Adapter/In/Api/Controllers/User/EditController.php

@@ -36,14 +36,18 @@ public function editAction(): ApiResponse
 
     private function buildUserData(): User
     {
+        $userProfileId = $this->apiService->getParamInt('userProfileId', true);
+
+        $this->assertProfileIsAssignable($userProfileId);
+
         return new User([
             'id'            => $this->apiService->getParamInt('id', true),
             'name'          => $this->apiService->getParamString('name', true),
             'login'         => $this->apiService->getParamString('login', true),
             'email'         => $this->apiService->getParamString('email'),
             'notes'         => $this->apiService->getParamString('notes'),
             'userGroupId'   => $this->apiService->getParamInt('userGroupId', true),
-            'userProfileId' => $this->apiService->getParamInt('userProfileId', true),
+            'userProfileId' => $userProfileId,
             'isAdminApp'    => $this->context->getUserData()->isAdminApp && (bool) $this->apiService->getParamInt('isAdminApp'),
             'isAdminAcc'    => $this->context->getUserData()->isAdminApp && (bool) $this->apiService->getParamInt('isAdminAcc'),
             'isDisabled'    => (bool) $this->apiService->getParamInt('isDisabled'),

src/Infrastructure/Adapter/In/Api/Controllers/User/UserBase.php

@@ -6,6 +6,7 @@
 use SP\Application\Application;
 use SP\Application\Api\Ports\ApiService;
 use SP\Domain\Core\Acl\AclInterface;
+use SP\Application\User\Ports\UserProfileService;
 use SP\Application\User\Ports\UserService;
 use SP\Domain\User\Models\User as UserModel;
 use SP\Infrastructure\Adapter\In\Api\Controllers\ControllerBase;
@@ -20,7 +21,8 @@ public function __construct(
         Router       $router,
         ApiService   $apiService,
         AclInterface $acl,
-        UserService  $userService
+        UserService  $userService,
+        private readonly UserProfileService $userProfileService
     ) {
         parent::__construct($application, $router, $apiService, $acl);
         $this->userService = $userService;
@@ -39,4 +41,23 @@ protected static function withoutCredentials(UserModel $user): array
     {
         return $user->toArray(null, UserModel::CREDENTIAL_COLS, true);
     }
+
+    /**
+     * A caller may not point a user at a profile stronger than their own.
+     *
+     * `isAdminApp` and `isAdminAcc` are already gated on the caller holding them, here and in the
+     * web form — but the profile itself was not, and a profile is where the other thirty
+     * permissions live. Without this, "may create or edit users" reached every permission in the
+     * installation by assigning an existing profile that has them, which through the API is
+     * reachable with a token minted for USER_EDIT.
+     *
+     * @throws \SP\Domain\Common\Services\ServiceException
+     * @throws \SP\Domain\Core\Exceptions\ConstraintException
+     * @throws \SP\Domain\Core\Exceptions\QueryException
+     * @throws \SP\Domain\Core\Exceptions\NoSuchItemException
+     */
+    final protected function assertProfileIsAssignable(int $profileId): void
+    {
+        $this->userProfileService->assertAssignableBy($profileId);
+    }
 }

src/Infrastructure/Adapter/In/Web/Controllers/User/SaveCreateController.php

@@ -60,6 +60,8 @@ public function saveCreateAction(): ActionResponse
 
             $itemData = $this->form->getItemData();
 
+            $this->assertProfileIsAssignable($itemData);
+
             $id = $this->userService->create($itemData);
 
             $this->eventDispatcher->notify(new Event(

src/Infrastructure/Adapter/In/Web/Controllers/User/SaveEditController.php

@@ -64,6 +64,8 @@ public function saveEditAction(int $id): ActionResponse
 
             $itemData = $this->form->getItemData();
 
+            $this->assertProfileIsAssignable($itemData);
+
             $this->userService->update($itemData);
 
             $this->eventDispatcher->notify(new Event(

src/Infrastructure/Adapter/In/Web/Controllers/User/UserSaveBase.php

@@ -30,11 +30,13 @@
 use SP\Domain\Common\Services\ServiceException;
 use SP\Domain\Core\Exceptions\ConstraintException;
 use SP\Domain\Core\Exceptions\QueryException;
+use SP\Domain\Core\Exceptions\NoSuchItemException;
 use SP\Application\CustomField\Ports\CustomFieldDataService;
 use SP\Application\Notification\Ports\MailService;
 use SP\Domain\CustomField\Models\CustomFieldData as CustomFieldDataModel;
 use SP\Domain\User\Models\User;
 use SP\Application\User\Ports\UserPassRecoverService;
+use SP\Application\User\Ports\UserProfileService;
 use SP\Application\User\Ports\UserService;
 use SP\Application\User\Services\UserPassRecover;
 use SP\Infrastructure\Adapter\In\Web\Controllers\ControllerBase;
@@ -66,7 +68,8 @@ public function __construct(
         UserService $userService,
         CustomFieldDataService $customFieldService,
         MailService            $mailService,
-        UserPassRecoverService $userPassRecoverService
+        UserPassRecoverService $userPassRecoverService,
+        private readonly UserProfileService $userProfileService
     ) {
         parent::__construct($application, $webControllerHelper);
 
@@ -79,6 +82,28 @@ public function __construct(
         $this->form = new UserForm($application, $this->request);
     }
 
+    /**
+     * A caller may not point a user at a profile stronger than their own.
+     *
+     * `UserForm` already gates `isAdminApp` and `isAdminAcc` on the caller holding them, and the
+     * API's user endpoints do the same — but neither constrained `userProfileId`, and a profile is
+     * where the other thirty permissions live. Without this, "may manage users" reached every
+     * permission in the installation by assigning an existing profile that has them.
+     *
+     * Called from here rather than from `UserService::create()`, which is also the path the
+     * installer and LDAP auto-provisioning take, where there is no signed-in caller to constrain
+     * against.
+     *
+     * @throws ServiceException
+     * @throws ConstraintException
+     * @throws QueryException
+     * @throws NoSuchItemException
+     */
+    final protected function assertProfileIsAssignable(User $userData): void
+    {
+        $this->userProfileService->assertAssignableBy($userData->getUserProfileId() ?? 0);
+    }
+
     /**
      * @param  int  $userId
      * @param User $userData

tests/Unit/Application/User/Services/UserProfileTest.php

@@ -36,7 +36,11 @@
 use SP\Application\User\Services\UserProfile;
 use SP\Domain\Core\Exceptions\DuplicatedItemException;
 use SP\Domain\Core\Exceptions\NoSuchItemException;
+use PHPUnit\Framework\MockObject\Rule\InvocationOrder;
 use SP\Domain\Common\Dtos\QueryResult;
+use SP\Tests\Support\Generators\UserDataGenerator;
+use SP\Domain\User\Dtos\UserDto;
+use SP\Domain\User\Models\ProfileData;
 use SP\Tests\Support\Generators\UserProfileDataGenerator;
 use SP\Tests\Support\UnitaryTestCase;
 
@@ -270,6 +274,105 @@ public function testGetUsersForProfile()
         $this->assertEquals([$userProfile], $out);
     }
 
+    /**
+     * A profile granting more than the caller holds cannot be assigned to a user.
+     *
+     * `isAdminApp` and `isAdminAcc` were already gated on the caller holding them, at both doors —
+     * but the profile a user is pointed at was not, and that is where the other thirty permissions
+     * live. Assigning one hands the user everything on it, so without this "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.
+     *
+     * @throws ConstraintException
+     * @throws NoSuchItemException
+     * @throws QueryException
+     * @throws ServiceException
+     */
+    public function testAProfileBeyondTheCallersOwnCannotBeAssigned()
+    {
+        $this->givenTheCallerHolds(new ProfileData(['accView' => true]));
+        $this->givenTheProfileBeingAssignedIs(new ProfileData(['accView' => true, 'mgmUsers' => true]));
+
+        $this->expectException(ServiceException::class);
+        $this->expectExceptionMessage('You cannot assign a profile with more permissions than your own');
+
+        $this->userProfile->assertAssignableBy(100);
+    }
+
+    /**
+     * One within it can, or the guard would have stopped delegates administering users at all.
+     *
+     * @throws ConstraintException
+     * @throws NoSuchItemException
+     * @throws QueryException
+     * @throws ServiceException
+     */
+    public function testAProfileWithinTheCallersOwnIsAssignable()
+    {
+        $this->givenTheCallerHolds(new ProfileData(['accView' => true, 'mgmUsers' => true]));
+        $this->givenTheProfileBeingAssignedIs(new ProfileData(['accView' => true]), self::once());
+
+        $this->userProfile->assertAssignableBy(100);
+
+        // The assertion is the `once()` above: the profile was actually read and compared, rather
+        // than the call short-circuiting somewhere before it got that far.
+    }
+
+    /**
+     * An application administrator is not constrained — they hold everything by definition, and the
+     * profile is not even read for them.
+     *
+     * @throws ConstraintException
+     * @throws NoSuchItemException
+     * @throws QueryException
+     * @throws ServiceException
+     */
+    public function testAnApplicationAdministratorMayAssignAnything()
+    {
+        $this->context->setUserData(
+            UserDto::fromModel(
+                UserDataGenerator::factory()->buildUserData()->mutate(['isAdminApp' => true])
+            )
+        );
+
+        // The `never()` is the assertion: an administrator's profile is not even read.
+        $this->userProfileRepository->expects(self::never())->method('getById');
+
+        $this->userProfile->assertAssignableBy(100);
+    }
+
+    private function givenTheCallerHolds(ProfileData $profileData): void
+    {
+        $this->context->setUserData(
+            UserDto::fromModel(
+                UserDataGenerator::factory()->buildUserData()->mutate(['isAdminApp' => false])
+            )
+        );
+        $this->context->setUserProfile($profileData);
+    }
+
+    /**
+     * @throws Exception
+     */
+    private function givenTheProfileBeingAssignedIs(
+        ProfileData      $profileData,
+        ?InvocationOrder $times = null
+    ): void {
+        $result = new QueryResult([
+                                      UserProfileDataGenerator::factory()
+                                                              ->buildUserProfileData()
+                                                              ->dehydrate($profileData)
+                                  ]);
+
+        // Always through expects(): with() on a bare method() is deprecated in PHPUnit 13, and
+        // any() is too. Both callers read the profile exactly once, so once() is the honest count.
+        $this->userProfileRepository
+            ->expects($times ?? self::once())
+            ->method('getById')
+            ->with(100)
+            ->willReturn($result);
+    }
+
     protected function setUp(): void
     {
         parent::setUp();

tests/Unit/Infrastructure/Adapter/In/Api/Controllers/User/CreateControllerTest.php

@@ -7,6 +7,7 @@
 use PHPUnit\Framework\Attributes\Group;
 use PHPUnit\Framework\MockObject\MockObject;
 use SP\Application\Api\Ports\ApiService;
+use SP\Application\User\Ports\UserProfileService;
 use SP\Application\User\Ports\UserService;
 use SP\Infrastructure\Bootstrap\Router;
 use SP\Domain\Api\Dtos\ApiResponse;
@@ -24,6 +25,7 @@ class CreateControllerTest extends UnitaryTestCase
 {
     private MockObject|ApiService $apiService;
     private MockObject|UserService $userService;
+    private MockObject|UserProfileService $userProfileService;
     private CreateController $controller;
 
     public function testCreateAction(): void
@@ -164,6 +166,7 @@ protected function setUp(): void
 
         $this->apiService = $this->createMock(ApiService::class);
         $this->userService = $this->createMock(UserService::class);
+        $this->userProfileService = $this->createStub(UserProfileService::class);
 
         $router = new Router(
             new SymfonyRequest(),
@@ -177,7 +180,8 @@ protected function setUp(): void
             $router,
             $this->apiService,
             $this->createStub(AclInterface::class),
-            $this->userService
+            $this->userService,
+            $this->userProfileService
         );
     }
 }