diff --git a/lib/SP/Providers/Auth/Ldap/LdapBase.php b/lib/SP/Providers/Auth/Ldap/LdapBase.php index 133acb5f..6d4e7495 100644 --- a/lib/SP/Providers/Auth/Ldap/LdapBase.php +++ b/lib/SP/Providers/Auth/Ldap/LdapBase.php @@ -122,7 +122,7 @@ abstract class LdapBase implements LdapInterface return LdapUtil::getGroupName($this->ldapParams->getGroup()) ?: ''; } - return $this->ldapParams->getGroup(); + return $this->ldapParams->getGroup() ?? ''; } /** diff --git a/lib/SP/Providers/Auth/Ldap/LdapMsAds.php b/lib/SP/Providers/Auth/Ldap/LdapMsAds.php index bf0aa0ea..1de4576e 100644 --- a/lib/SP/Providers/Auth/Ldap/LdapMsAds.php +++ b/lib/SP/Providers/Auth/Ldap/LdapMsAds.php @@ -64,7 +64,7 @@ final class LdapMsAds extends LdapBase $attributes = $this->ldapParams->getFilterGroupAttributes(); } - return '(&(|' . LdapUtil::getAttributesForFilter($attributes, $this->getGroupDn()) . ')' . $filter . ')'; + return sprintf("(&(|%s)%s)", LdapUtil::getAttributesForFilter($attributes, $this->getGroupDn()), $filter); } /** @@ -90,11 +90,11 @@ final class LdapMsAds extends LdapBase $attributes = $this->ldapParams->getFilterUserAttributes(); } - return '(&(|' - . LdapUtil::getAttributesForFilter($attributes, $userLogin) - . ')' - . $this->getUserObjectFilter() - . ')'; + return sprintf( + "(&(|%s)%s)", + LdapUtil::getAttributesForFilter($attributes, $userLogin), + $this->getUserObjectFilter() + ); } /** @@ -196,9 +196,7 @@ final class LdapMsAds extends LdapBase $attributes = $this->ldapParams->getFilterGroupAttributes(); } - return '(|' - . LdapUtil::getAttributesForFilter($attributes, $this->getGroupDn()) - . ')'; + return sprintf("(|%s)", LdapUtil::getAttributesForFilter($attributes, $this->getGroupDn())); } protected function pickServer(): string diff --git a/lib/SP/Providers/Auth/Ldap/LdapStd.php b/lib/SP/Providers/Auth/Ldap/LdapStd.php index 680a7baf..ebb8809b 100644 --- a/lib/SP/Providers/Auth/Ldap/LdapStd.php +++ b/lib/SP/Providers/Auth/Ldap/LdapStd.php @@ -63,7 +63,7 @@ final class LdapStd extends LdapBase $attributes = $this->ldapParams->getFilterGroupAttributes(); } - return '(&(|' . LdapUtil::getAttributesForFilter($attributes, $this->getGroupDn()) . ')' . $filter . ')'; + return sprintf("(&(|%s)%s)", LdapUtil::getAttributesForFilter($attributes, $this->getGroupDn()), $filter); } /** @@ -91,7 +91,7 @@ final class LdapStd extends LdapBase $filter = $this->getUserObjectFilter(); - return '(&(|' . LdapUtil::getAttributesForFilter($attributes, $userLogin) . ')' . $filter . ')'; + return sprintf("(&(|%s)%s)", LdapUtil::getAttributesForFilter($attributes, $userLogin), $filter); } /** @@ -104,17 +104,15 @@ final class LdapStd extends LdapBase // los grupos del usuario if (empty($this->ldapParams->getGroup()) || $this->ldapParams->getGroup() === '*' - || in_array($this->getGroupDn(), $groupsDn, true)) { + || in_array($this->getGroupDn(), $groupsDn, true) + ) { $this->eventDispatcher->notifyEvent( 'ldap.check.group', new Event( $this, EventMessage::factory() ->addDescription(__u('User in group verified')) - ->addDetail( - __u('User'), - $userDn - ) + ->addDetail(__u('User'), $userDn) ->addDetail(__u('Group'), $this->ldapParams->getGroup()) ) ); @@ -144,21 +142,26 @@ final class LdapStd extends LdapBase $this, EventMessage::factory() ->addDescription(__u('User does not belong to the group')) - ->addDetail( - __u('User'), - $userDn - ) + ->addDetail(__u('User'), $userDn) ->addDetail(__u('Group'), $this->getGroupFromParams()) - ->addDetail( - 'LDAP FILTER', - $filter - ) + ->addDetail('LDAP FILTER', $filter) ) ); return false; } + $this->eventDispatcher->notifyEvent( + 'ldap.check.group', + new Event( + $this, + EventMessage::factory() + ->addDescription(__u('User in group verified')) + ->addDetail(__u('User'), $userDn) + ->addDetail(__u('Group'), $this->getGroupFromParams()) + ) + ); + return true; } @@ -174,8 +177,14 @@ final class LdapStd extends LdapBase return $this->getUserObjectFilter(); } - return '(&(cn=' . $groupName . ')' . '(|(memberUid=' . $member . ')(member=' . $member . ')(uniqueMember=' . $member . '))' . - $this->getGroupObjectFilter() . ')'; + return sprintf( + '(&(cn=%s)(|(memberUid=%s)(member=%s)(uniqueMember=%s))%s)', + $groupName, + $member, + $member, + $member, + $this->getGroupObjectFilter() + ); } /** diff --git a/tests/SP/Providers/Auth/Ldap/LdapMsAdsTest.php b/tests/SP/Providers/Auth/Ldap/LdapMsAdsTest.php index f3bc4421..d355beea 100644 --- a/tests/SP/Providers/Auth/Ldap/LdapMsAdsTest.php +++ b/tests/SP/Providers/Auth/Ldap/LdapMsAdsTest.php @@ -44,11 +44,11 @@ use SP\Tests\UnitaryTestCase; class LdapMsAdsTest extends UnitaryTestCase { - private LdapConnectionInterface|MockObject $ldapConnection; - private LdapActionsInterface|MockObject|LdapMsAds $ldapActions; - private EventDispatcherInterface|MockObject $eventDispatcher; - private LdapMsAds $ldap; - private LdapParams $ldapParams; + private LdapConnectionInterface|MockObject $ldapConnection; + private LdapActionsInterface|MockObject $ldapActions; + private EventDispatcherInterface|MockObject $eventDispatcher; + private LdapMsAds $ldap; + private LdapParams $ldapParams; public static function groupDataProvider(): array { diff --git a/tests/SP/Providers/Auth/Ldap/LdapStdTest.php b/tests/SP/Providers/Auth/Ldap/LdapStdTest.php new file mode 100644 index 00000000..5373af42 --- /dev/null +++ b/tests/SP/Providers/Auth/Ldap/LdapStdTest.php @@ -0,0 +1,382 @@ +. + */ + +namespace SP\Providers\Auth\Ldap; + +use PHPUnit\Framework\MockObject\MockObject; +use SP\Core\Events\EventDispatcherInterface; +use SP\Core\Exceptions\SPException; +use SP\Domain\Auth\Ports\LdapActionsInterface; +use SP\Domain\Auth\Ports\LdapConnectionInterface; +use SP\Tests\UnitaryTestCase; + +/** + * Class LdapStdTest + * + * @group unitary + */ +class LdapStdTest extends UnitaryTestCase +{ + + private LdapConnectionInterface|MockObject $ldapConnection; + private LdapActionsInterface|MockObject $ldapActions; + private EventDispatcherInterface|MockObject $eventDispatcher; + private LdapStd $ldap; + private LdapParams $ldapParams; + + public static function groupDataProvider(): array + { + return [ + [''], + ['*'], + ['cn=TestGroup,dc=groups,dc=syspass,dc=org'] + ]; + } + + /** + * @throws LdapException + */ + public function testConnect() + { + $user = self::$faker->userName; + $password = self::$faker->password; + + $this->ldapConnection->expects(self::once())->method('connect')->with($user, $password); + + $this->ldap->connect($user, $password); + } + + /** + * @throws LdapException + */ + public function testConnectWithNull() + { + $this->ldapConnection->expects(self::once())->method('connect')->with(null, null); + + $this->ldap->connect(); + } + + /** + * @dataProvider groupDataProvider() + * @throws LdapException + */ + public function testIsUserInGroup(string $group) + { + $this->ldapParams->setGroup($group); + + $userDn = 'cn=TestUser,dc=syspass,dc=org'; + $userLogin = self::$faker->userName; + $groupsDn = [ + 'cn=TestGroup,dc=groups,dc=syspass,dc=org' + ]; + + $this->eventDispatcher + ->expects(self::once()) + ->method('notifyEvent') + ->with('ldap.check.group', self::anything()); + + $out = $this->ldap->isUserInGroup($userDn, $userLogin, $groupsDn); + + self::assertTrue($out); + } + + /** + * @throws LdapException + */ + public function testIsUserInGroupWithSearchGroupDn() + { + $this->ldapParams->setGroup('TestGroup'); + + $userDn = 'cn=TestUser,dc=syspass,dc=org'; + $userLogin = self::$faker->userName; + $groupsDn = [ + 'cn=TestGroup,dc=groups,dc=syspass,dc=org' + ]; + + $this->ldapActions->expects(self::once()) + ->method('searchGroupsDn') + ->with($this->ldap->getGroupObjectFilter()) + ->willReturn($groupsDn); + + $this->eventDispatcher + ->expects(self::once()) + ->method('notifyEvent') + ->with('ldap.check.group', self::anything()); + + $out = $this->ldap->isUserInGroup($userDn, $userLogin, $groupsDn); + + self::assertTrue($out); + } + + /** + * @throws LdapException + */ + public function testIsUserInGroupWithCheckFilter() + { + $this->ldapParams->setGroup('TestGroup'); + + $userDn = 'cn=TestUser,dc=syspass,dc=org'; + $userLogin = self::$faker->userName; + $groupDn = 'cn=TestGroup,dc=groups,dc=syspass,dc=org'; + + $this->ldapActions->expects(self::exactly(1)) + ->method('searchGroupsDn') + ->with($this->ldap->getGroupObjectFilter()) + ->willReturnOnConsecutiveCalls([], [], [$groupDn]); + + $groupsFilter = '(&(cn=TestGroup)(|(memberUid=cn=TestUser,dc=syspass,dc=org)(member=cn=TestUser,dc=syspass,dc=org)(uniqueMember=cn=TestUser,dc=syspass,dc=org))(|(objectClass=groupOfNames)(objectClass=groupOfUniqueNames)(objectClass=group)))'; + + $this->ldapActions + ->expects(self::once()) + ->method('getObjects') + ->with($groupsFilter, ['dn']); + + $this->eventDispatcher + ->expects(self::once()) + ->method('notifyEvent') + ->with('ldap.check.group', self::anything()); + + $out = $this->ldap->isUserInGroup($userDn, $userLogin, []); + + self::assertTrue($out); + } + + /** + * @throws LdapException + */ + public function testIsUserInGroupWithCheckFilterAndZeroResults() + { + $this->ldapParams->setGroup('TestGroup'); + + $userDn = 'cn=TestUser,dc=syspass,dc=org'; + $userLogin = self::$faker->userName; + $groupDn = 'cn=TestGroup,dc=groups,dc=syspass,dc=org'; + + $this->ldapActions->expects(self::exactly(1)) + ->method('searchGroupsDn') + ->with($this->ldap->getGroupObjectFilter()) + ->willReturnOnConsecutiveCalls([], [], [$groupDn]); + + $groupsFilter = '(&(cn=TestGroup)(|(memberUid=cn=TestUser,dc=syspass,dc=org)(member=cn=TestUser,dc=syspass,dc=org)(uniqueMember=cn=TestUser,dc=syspass,dc=org))(|(objectClass=groupOfNames)(objectClass=groupOfUniqueNames)(objectClass=group)))'; + + $this->ldapActions + ->expects(self::once()) + ->method('getObjects') + ->with($groupsFilter, ['dn']) + ->willReturn(['count' => 0]); + + $this->eventDispatcher + ->expects(self::once()) + ->method('notifyEvent') + ->with('ldap.check.group', self::anything()); + + $out = $this->ldap->isUserInGroup($userDn, $userLogin, [$groupDn]); + + self::assertFalse($out); + } + + /** + * @throws SPException + */ + public function testGetGroupMembershipIndirectFilter() + { + $groupDn = 'cn=TestGroup,dc=groups,dc=syspass,dc=org'; + $this->ldapParams->setGroup('TestGroup'); + + $this->ldapActions->expects(self::once()) + ->method('searchGroupsDn') + ->with($this->ldap->getGroupObjectFilter()) + ->willReturn([$groupDn]); + + $out = $this->ldap->getGroupMembershipIndirectFilter(); + + $expected = sprintf( + "(&(|%s)%s)", + LdapUtil::getAttributesForFilter(LdapStd::DEFAULT_FILTER_GROUP_ATTRIBUTES, $groupDn), + '(|(objectClass=inetOrgPerson)(objectClass=person)(objectClass=simpleSecurityObject))' + ); + + self::assertEquals($expected, $out); + } + + /** + * @throws SPException + */ + public function testGetGroupMembershipIndirectFilterWithEmptyGroup() + { + $this->ldapActions->expects(self::never()) + ->method('searchGroupsDn'); + + $out = $this->ldap->getGroupMembershipIndirectFilter(); + $expected = '(|(objectClass=inetOrgPerson)(objectClass=person)(objectClass=simpleSecurityObject))'; + + self::assertEquals($expected, $out); + } + + /** + * @throws SPException + */ + public function testGetGroupMembershipIndirectFilterWithAttributes() + { + $groupDn = 'cn=TestGroup,dc=groups,dc=syspass,dc=org'; + $this->ldapParams->setGroup('TestGroup'); + $this->ldapParams->setFilterGroupAttributes(['testAttribute']); + + $this->ldapActions->expects(self::once()) + ->method('searchGroupsDn') + ->with($this->ldap->getGroupObjectFilter()) + ->willReturn([$groupDn]); + + $out = $this->ldap->getGroupMembershipIndirectFilter(); + + $expected = sprintf( + "(&(|%s)%s)", + LdapUtil::getAttributesForFilter(['testAttribute'], $groupDn), + '(|(objectClass=inetOrgPerson)(objectClass=person)(objectClass=simpleSecurityObject))' + ); + + self::assertEquals($expected, $out); + } + + public function testGetUserDnFilter() + { + $user = self::$faker->userName; + + $out = $this->ldap->getUserDnFilter($user); + + $expected = sprintf( + "(&(|%s)%s)", + LdapUtil::getAttributesForFilter(LdapMsAds::DEFAULT_FILTER_USER_ATTRIBUTES, $user), + '(|(objectClass=inetOrgPerson)(objectClass=person)(objectClass=simpleSecurityObject))' + ); + + self::assertEquals($expected, $out); + } + + public function testGetUserDnFilterWithAttributes() + { + $this->ldapParams->setFilterUserAttributes(['memberOf']); + $user = self::$faker->userName; + + $out = $this->ldap->getUserDnFilter($user); + + $expected = sprintf( + "(&(|%s)%s)", + LdapUtil::getAttributesForFilter(['memberOf'], $user), + '(|(objectClass=inetOrgPerson)(objectClass=person)(objectClass=simpleSecurityObject))' + ); + + self::assertEquals($expected, $out); + } + + public function testGetGroupObjectFilter() + { + $out = $this->ldap->getGroupObjectFilter(); + $expected = '(|(objectClass=groupOfNames)(objectClass=groupOfUniqueNames)(objectClass=group))'; + + self::assertEquals($expected, $out); + } + + public function testGetGroupObjectFilterWithFilter() + { + $this->ldapParams->setFilterGroupObject('test'); + + $out = $this->ldap->getGroupObjectFilter(); + + self::assertEquals('test', $out); + } + + public function testGetServer() + { + self::assertEquals($this->ldapParams->getServer(), $this->ldap->getServer()); + } + + public function testGetGroupMembershipDirectFilter() + { + $this->ldapParams->setGroup('TestGroup'); + $out = $this->ldap->getGroupMembershipDirectFilter(); + + $expected = sprintf( + '(&(cn=%s)(|(memberUid=%s)(member=%s)(uniqueMember=%s))%s)', + 'TestGroup', + '*', + '*', + '*', + '(|(objectClass=groupOfNames)(objectClass=groupOfUniqueNames)(objectClass=group))' + ); + + self::assertEquals($expected, $out); + } + + public function testGetGroupMembershipDirectFilterWithUser() + { + $user = 'TestUser'; + $this->ldapParams->setGroup('TestGroup'); + $out = $this->ldap->getGroupMembershipDirectFilter($user); + + $expected = sprintf( + '(&(cn=%s)(|(memberUid=%s)(member=%s)(uniqueMember=%s))%s)', + 'TestGroup', + $user, + $user, + $user, + '(|(objectClass=groupOfNames)(objectClass=groupOfUniqueNames)(objectClass=group))' + ); + + self::assertEquals($expected, $out); + } + + public function testGetGroupMembershipDirectFilterWithoutGroup() + { + $user = 'TestUser'; + $out = $this->ldap->getGroupMembershipDirectFilter($user); + + $expected = '(|(objectClass=inetOrgPerson)(objectClass=person)(objectClass=simpleSecurityObject))'; + + self::assertEquals($expected, $out); + } + + protected function setUp(): void + { + parent::setUp(); + + $this->ldapConnection = $this->createMock(LdapConnectionInterface::class); + $this->ldapActions = $this->createMock(LdapActionsInterface::class); + $this->eventDispatcher = $this->createMock(EventDispatcherInterface::class); + + $this->ldapParams = new LdapParams( + self::$faker->domainName, + LdapTypeEnum::ADS, + self::$faker->userName, + self::$faker->password + ); + + $this->ldap = new LdapStd( + $this->ldapConnection, + $this->ldapActions, + $this->ldapParams, + $this->eventDispatcher + ); + } +}