diff --git a/phpstan.neon b/phpstan.neon index d840b28e..3962b3e5 100644 --- a/phpstan.neon +++ b/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 diff --git a/src/Entity/UserPreference.php b/src/Entity/UserPreference.php index 9835a52b..42ae54d3 100644 --- a/src/Entity/UserPreference.php +++ b/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 { diff --git a/src/Ldap/LdapCredentialsSubscriber.php b/src/Ldap/LdapCredentialsSubscriber.php index 0fddda7e..6037e5c0 100644 --- a/src/Ldap/LdapCredentialsSubscriber.php +++ b/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()) { - return; - } + // removing this code allows to upgrade from local to LDAP users + // if (!$user->isLdapUser()) { + // return; + // } if (!$this->ldapManager->bind($user->getUserIdentifier(), $presentedPassword)) { throw new BadCredentialsException('The presented password is invalid.'); diff --git a/src/Ldap/LdapManager.php b/src/Ldap/LdapManager.php index 1f3363c0..b9419108 100644 --- a/src/Ldap/LdapManager.php +++ b/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'); - - 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 + // always look up the users current DN first, as the current user might be upgraded from local to 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 */ diff --git a/src/Ldap/LdapUserProvider.php b/src/Ldap/LdapUserProvider.php index 27c0a31c..786f6b9d 100644 --- a/src/Ldap/LdapUserProvider.php +++ b/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())); } diff --git a/tests/Ldap/LdapManagerTest.php b/tests/Ldap/LdapManagerTest.php index a7e0e4c6..d48045a6 100644 --- a/tests/Ldap/LdapManagerTest.php +++ b/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); diff --git a/tests/Ldap/LdapUserProviderTest.php b/tests/Ldap/LdapUserProviderTest.php index 0a92c286..a78f1c3c 100644 --- a/tests/Ldap/LdapUserProviderTest.php +++ b/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'); - self::assertFalse($user->isLdapUser()); + $user->setAuth(User::AUTH_LDAP); + 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); - } } diff --git a/tests/phpstan.neon b/tests/phpstan.neon index 68afa73e..cb8d4bc0 100644 --- a/tests/phpstan.neon +++ b/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