fix: choosing the profile LDAP users get is user management

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

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

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