fix LDAP issues and allow to migrate from local to LDAP account (#4445)

phpstan.neon

@@ -4076,31 +4076,16 @@ parameters:
             count: 1
             path: src/Ldap/LdapManager.php
 
-        -
-            message: "#^Method App\\\\Ldap\\\\LdapManager\\:\\:hydrateRoles\\(\\) has no return type specified\\.$#"
-            count: 1
-            path: src/Ldap/LdapManager.php
-
         -
             message: "#^Method App\\\\Ldap\\\\LdapManager\\:\\:hydrateRoles\\(\\) has parameter \\$entries with no value type specified in iterable type array\\.$#"
             count: 1
             path: src/Ldap/LdapManager.php
 
-        -
-            message: "#^Method App\\\\Ldap\\\\LdapManager\\:\\:hydrateUser\\(\\) has no return type specified\\.$#"
-            count: 1
-            path: src/Ldap/LdapManager.php
-
         -
             message: "#^Method App\\\\Ldap\\\\LdapManager\\:\\:hydrateUser\\(\\) has parameter \\$ldapEntry with no value type specified in iterable type array\\.$#"
             count: 1
             path: src/Ldap/LdapManager.php
 
-        -
-            message: "#^Method App\\\\Ldap\\\\LdapManager\\:\\:hydrateUserWithAttributesMap\\(\\) has no return type specified\\.$#"
-            count: 1
-            path: src/Ldap/LdapManager.php
-
         -
             message: "#^Method App\\\\Ldap\\\\LdapManager\\:\\:hydrateUserWithAttributesMap\\(\\) has parameter \\$attributeMap with no value type specified in iterable type array\\.$#"
             count: 1
@@ -4111,11 +4096,6 @@ parameters:
             count: 1
             path: src/Ldap/LdapManager.php
 
-        -
-            message: "#^Method App\\\\Ldap\\\\LdapManager\\:\\:updateUser\\(\\) has no return type specified\\.$#"
-            count: 1
-            path: src/Ldap/LdapManager.php
-
         -
             message: "#^PHPDoc tag @var for variable \\$attr has no value type specified in iterable type array\\.$#"
             count: 1

src/Entity/UserPreference.php

@@ -86,10 +86,6 @@ class UserPreference
         return $this->id;
     }
 
-    /**
-     * @param int $id
-     * @return UserPreference
-     */
     public function setId(int $id): UserPreference
     {
         $this->id = $id;
@@ -111,7 +107,13 @@ class UserPreference
 
     public function getName(): ?string
     {
-        return $this->sanitizeName($this->name);
+        $sanitized = $this->sanitizeName($this->name);
+
+        if ($sanitized !== $this->name) {
+            $this->name = $sanitized;
+        }
+
+        return $this->name;
     }
 
     public function matches(string $name): bool
@@ -119,7 +121,7 @@ class UserPreference
         return $this->sanitizeName($name) === $this->getName();
     }
 
-    public function sanitizeName(?string $name): string
+    private function sanitizeName(?string $name): string
     {
         return str_replace(['.', '-'], '_', $name);
     }
@@ -150,9 +152,6 @@ class UserPreference
 
     /**
      * Sets the form type to edit that setting.
-     *
-     * @param string $type
-     * @return UserPreference
      */
     public function setType(string $type): UserPreference
     {

src/Ldap/LdapCredentialsSubscriber.php

@@ -62,9 +62,10 @@ final class LdapCredentialsSubscriber implements EventSubscriberInterface
             throw new BadCredentialsException('The presented user needs to be a Kimai user.');
         }
 
-        if (!$user->isLdapUser()) {
+        // removing this code allows to upgrade from local to LDAP users
-            return;
+        // if (!$user->isLdapUser()) {
-        }
+        //    return;
+        // }
 
         if (!$this->ldapManager->bind($user->getUserIdentifier(), $presentedPassword)) {
             throw new BadCredentialsException('The presented password is invalid.');

src/Ldap/LdapManager.php

@@ -77,23 +77,16 @@ class LdapManager
      * - syncing user attributes
      * - syncing roles
      *
-     * @param User $user
      * @throws LdapDriverException
      */
-    public function updateUser(User $user)
+    public function updateUser(User $user): void
     {
-        $baseDn = $user->getPreferenceValue('ldap.dn');
+        // always look up the users current DN first, as the current user might be upgraded from local to LDAP
-
-        if (null === $baseDn) {
-            throw new LdapDriverException('This account is not a registered LDAP user');
-        }
-
-        // always look up the users current DN first, as the cached DN might have been renamed in LDAP
         $userFresh = $this->findUserByUsername($user->getUserIdentifier());
-        if (null === $userFresh || null === ($baseDn = $userFresh->getPreferenceValue('ldap.dn'))) {
+        if (null === $userFresh || null === ($baseDn = $userFresh->getPreferenceValue('ldap_dn'))) {
             throw new LdapDriverException(sprintf('Failed fetching user DN for %s', $user->getUserIdentifier()));
         }
-        $user->setPreferenceValue('ldap.dn', $baseDn);
+        $user->setPreferenceValue('ldap_dn', $baseDn);
 
         $params = $this->config->getUserParameters();
         $entries = $this->driver->search($baseDn, $params['attributesFilter']);
@@ -142,23 +135,16 @@ class LdapManager
 
     // ===================================================================
 
-    private function createUser(): User
+    public function hydrate(array $ldapEntry): User
     {
         $user = new User();
         $user->setEnabled(true);
-
-        return $user;
-    }
-
-    public function hydrate(array $ldapEntry): User
-    {
-        $user = $this->createUser();
         $this->hydrateUser($user, $ldapEntry);
 
         return $user;
     }
 
-    public function hydrateUser(User $user, array $ldapEntry)
+    public function hydrateUser(User $user, array $ldapEntry): void
     {
         $userParams = $this->config->getUserParameters();
         $attributeMap = [];
@@ -185,14 +171,14 @@ class LdapManager
             $user->setPassword('');
         }
         $user->setAuth(User::AUTH_LDAP);
-        $user->setPreferenceValue('ldap.dn', $ldapEntry['dn']);
+        $user->setPreferenceValue('ldap_dn', $ldapEntry['dn']);
     }
 
     /**
      * @param User $user
      * @param array $entries
      */
-    public function hydrateRoles(User $user, array $entries)
+    public function hydrateRoles(User $user, array $entries): void
     {
         $roleParams = $this->config->getRoleParameters();
         $allowedRoles = $this->roles->getAvailableNames();
@@ -236,7 +222,7 @@ class LdapManager
         return $role;
     }
 
-    private function hydrateUserWithAttributesMap(UserInterface $user, array $ldapUserAttributes, array $attributeMap)
+    private function hydrateUserWithAttributesMap(UserInterface $user, array $ldapUserAttributes, array $attributeMap): void
     {
         $sawUsername = false;
         /** @var array $attr */

src/Ldap/LdapUserProvider.php

@@ -53,17 +53,12 @@ final class LdapUserProvider implements UserProviderInterface
             throw new UnsupportedUserException(sprintf('Instances of "%s" are not supported.', \get_class($user)));
         }
 
-        if (!$user->isLdapUser() && null === $user->getPreferenceValue('ldap.dn')) {
+        if (!$user->isLdapUser()) {
             throw new UnsupportedUserException(sprintf('Account "%s" is not a registered LDAP user.', $user->getUserIdentifier()));
         }
 
         try {
             $this->ldapManager->updateUser($user);
-
-            // updating old LDAP accounts
-            if (!$user->isLdapUser() && null !== $user->getPreferenceValue('ldap.dn')) {
-                $user->setAuth(User::AUTH_LDAP);
-            }
         } catch (LdapDriverException $ex) {
             throw new UnsupportedUserException(sprintf('Failed to refresh user "%s", probably DN is expired.', $user->getUserIdentifier()));
         }

tests/Ldap/LdapManagerTest.php

@@ -206,7 +206,7 @@ class LdapManagerTest extends TestCase
         $user = new User();
         $user->setUserIdentifier('foobar');
 
-        $user->setPreferenceValue('ldap.dn', 'fooooooooooo');
+        $user->setPreferenceValue('ldap_dn', 'fooooooooooo');
         $expected = [
             [
                 0 => ['dn' => 'blub', 'uid' => ['blub']],
@@ -245,7 +245,7 @@ class LdapManagerTest extends TestCase
 
         $user = new User();
         $user->setUserIdentifier('foobar');
-        $user->setPreferenceValue('ldap.dn', 'xxxxxxx');
+        $user->setPreferenceValue('ldap_dn', 'xxxxxxx');
 
         $expected = [
             [
@@ -280,7 +280,7 @@ class LdapManagerTest extends TestCase
     {
         $user = new User();
         $user->setUserIdentifier('foobar');
-        $user->setPreferenceValue('ldap.dn', 'sssssss');
+        $user->setPreferenceValue('ldap_dn', 'sssssss');
 
         $expected = [
             [
@@ -323,7 +323,7 @@ class LdapManagerTest extends TestCase
         $userOrig = clone $user;
         $sut->updateUser($user);
         self::assertEquals($userOrig->setEmail('foobar')->setAuth(User::AUTH_LDAP), $user);
-        self::assertEquals($user->getPreferenceValue('ldap.dn'), 'blub-updated');
+        self::assertEquals($user->getPreferenceValue('ldap_dn'), 'blub-updated');
     }
 
     public function getValidConfigsTestData()
@@ -452,7 +452,7 @@ class LdapManagerTest extends TestCase
 
         $user = new User();
         $user->setUserIdentifier('Karl-Heinz');
-        $user->setPreferenceValue('ldap.dn', 'blub');
+        $user->setPreferenceValue('ldap_dn', 'blub');
         $userOrig = clone $user;
         $userOrig->setEmail('Karl-Heinz')->setRoles(['ROLE_TEAMLEAD', 'ROLE_ADMIN'])->setAuth(User::AUTH_LDAP);
 

tests/Ldap/LdapUserProviderTest.php

@@ -10,7 +10,6 @@
 namespace App\Tests\Ldap;
 
 use App\Entity\User;
-use App\Ldap\LdapDriverException;
 use App\Ldap\LdapManager;
 use App\Ldap\LdapUserProvider;
 use PHPUnit\Framework\TestCase;
@@ -22,7 +21,7 @@ use Symfony\Component\Security\Core\Exception\UserNotFoundException;
  */
 class LdapUserProviderTest extends TestCase
 {
-    public function testLoadUserByIdentifierReturnsNull()
+    public function testLoadUserByIdentifierReturnsNull(): void
     {
         $this->expectException(UserNotFoundException::class);
         $this->expectExceptionMessage('User "test" not found');
@@ -34,7 +33,7 @@ class LdapUserProviderTest extends TestCase
         $sut->loadUserByIdentifier('test');
     }
 
-    public function testLoadUserByIdentifierReturnsUser()
+    public function testLoadUserByIdentifierReturnsUser(): void
     {
         $user = new User();
         $user->setUserIdentifier('foobar');
@@ -48,12 +47,12 @@ class LdapUserProviderTest extends TestCase
         self::assertSame($user, $actual);
     }
 
-    public function testRefreshUserReturnsUser()
+    public function testRefreshUserReturnsUser(): void
     {
         $user = new User();
         $user->setUserIdentifier('foobar');
-        $user->setPreferenceValue('ldap.dn', 'sdfdsf');
+        $user->setAuth(User::AUTH_LDAP);
-        self::assertFalse($user->isLdapUser());
+        self::assertTrue($user->isLdapUser());
 
         $manager = $this->getMockBuilder(LdapManager::class)->disableOriginalConstructor()->onlyMethods(['updateUser'])->getMock();
 
@@ -65,7 +64,7 @@ class LdapUserProviderTest extends TestCase
         self::assertTrue($user->isLdapUser());
     }
 
-    public function testRefreshUserThrowsExceptionOnNonLdapUser()
+    public function testRefreshUserThrowsExceptionOnNonLdapUser(): void
     {
         $this->expectException(UnsupportedUserException::class);
         $this->expectExceptionMessage('Account "foobar" is not a registered LDAP user.');
@@ -78,20 +77,4 @@ class LdapUserProviderTest extends TestCase
         $sut = new LdapUserProvider($manager);
         $actual = $sut->refreshUser($user);
     }
-
-    public function testRefreshUserThrowsExceptionOnBrokenUpdateUser()
-    {
-        $this->expectException(UnsupportedUserException::class);
-        $this->expectExceptionMessage('Failed to refresh user "foobar", probably DN is expired.');
-
-        $user = new User();
-        $user->setUserIdentifier('foobar');
-        $user->setPreferenceValue('ldap.dn', 'sdfdsf');
-
-        $manager = $this->getMockBuilder(LdapManager::class)->disableOriginalConstructor()->onlyMethods(['updateUser'])->getMock();
-        $manager->expects($this->once())->method('updateUser')->willThrowException(new LdapDriverException('blub'));
-
-        $sut = new LdapUserProvider($manager);
-        $actual = $sut->refreshUser($user);
-    }
 }

tests/phpstan.neon

@@ -6487,31 +6487,6 @@ parameters:
             count: 1
             path: Ldap/LdapManagerTest.php
 
-        -
-            message: "#^Method App\\\\Tests\\\\Ldap\\\\LdapUserProviderTest\\:\\:testLoadUserByIdentifierReturnsNull\\(\\) has no return type specified\\.$#"
-            count: 1
-            path: Ldap/LdapUserProviderTest.php
-
-        -
-            message: "#^Method App\\\\Tests\\\\Ldap\\\\LdapUserProviderTest\\:\\:testLoadUserByIdentifierReturnsUser\\(\\) has no return type specified\\.$#"
-            count: 1
-            path: Ldap/LdapUserProviderTest.php
-
-        -
-            message: "#^Method App\\\\Tests\\\\Ldap\\\\LdapUserProviderTest\\:\\:testRefreshUserReturnsUser\\(\\) has no return type specified\\.$#"
-            count: 1
-            path: Ldap/LdapUserProviderTest.php
-
-        -
-            message: "#^Method App\\\\Tests\\\\Ldap\\\\LdapUserProviderTest\\:\\:testRefreshUserThrowsExceptionOnBrokenUpdateUser\\(\\) has no return type specified\\.$#"
-            count: 1
-            path: Ldap/LdapUserProviderTest.php
-
-        -
-            message: "#^Method App\\\\Tests\\\\Ldap\\\\LdapUserProviderTest\\:\\:testRefreshUserThrowsExceptionOnNonLdapUser\\(\\) has no return type specified\\.$#"
-            count: 1
-            path: Ldap/LdapUserProviderTest.php
-
         -
             message: "#^Method App\\\\Tests\\\\Ldap\\\\SanitizingExceptionTest\\:\\:testMessagesAreSanitized\\(\\) has no return type specified\\.$#"
             count: 1