diff --git a/.github/workflows/phpunit-oci.yml b/.github/workflows/phpunit-oci.yml index 8c8f860c9..91d7b74f7 100644 --- a/.github/workflows/phpunit-oci.yml +++ b/.github/workflows/phpunit-oci.yml @@ -71,6 +71,12 @@ jobs: php-versions: ${{ fromJson(needs.matrix.outputs.php-version) }} server-versions: ${{ fromJson(needs.matrix.outputs.server-max) }} oci-versions: ['11', '18', '21', '23'] + exclude: + # Nextcloud master configures Oracle sessions with `Etc/UTC`, which OCI 11 + # cannot resolve. Keep OCI 11 coverage on older branches and newer Oracle + # coverage on master. + - oci-versions: '11' + server-versions: 'master' name: OCI ${{ matrix.oci-versions }} PHP ${{ matrix.php-versions }} Nextcloud ${{ matrix.server-versions }} @@ -146,6 +152,8 @@ jobs: DB_PORT: 1521 run: | mkdir data + # Oracle is opt-in in newer server branches, so enable it before install. + cp tests/databases-all-config.php config/databases-all.config.php ./occ maintenance:install --verbose --database=oci --database-name=${{ matrix.oci-versions < 23 && 'XE' || 'FREE' }} --database-host=127.0.0.1 --database-port=$DB_PORT --database-user=system --database-pass=oracle --admin-user admin --admin-pass admin ./occ app:enable --force ${{ env.APP_NAME }} diff --git a/lib/GroupBackend.php b/lib/GroupBackend.php index c3e6a79f7..600510019 100644 --- a/lib/GroupBackend.php +++ b/lib/GroupBackend.php @@ -155,15 +155,35 @@ public function groupExistsWithDifferentGid(string $samlGid): ?string { #[\Override] public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0): array { $query = $this->dbc->getQueryBuilder(); - $query->select('uid') - ->from(self::TABLE_MEMBERS) - ->where($query->expr()->eq('gid', $query->createNamedParameter($gid))) - ->orderBy('uid', 'ASC'); + $query->selectDistinct('m.uid') + ->from(self::TABLE_MEMBERS, 'm') + ->where($query->expr()->eq('m.gid', $query->createNamedParameter($gid))) + ->orderBy('m.uid', 'ASC'); if ($search !== '') { - $query->andWhere($query->expr()->like('uid', $query->createNamedParameter( - '%' . $this->dbc->escapeLikeParameter($search) . '%' - ))); + $query->leftJoin('m', 'accounts_data', 'dn', + $query->expr()->andX( + $query->expr()->eq('dn.uid', 'm.uid'), + $query->expr()->eq('dn.name', $query->createNamedParameter('displayname')) + ) + ); + $query->leftJoin('m', 'accounts_data', 'em', + $query->expr()->andX( + $query->expr()->eq('em.uid', 'm.uid'), + $query->expr()->eq('em.name', $query->createNamedParameter('email')) + ) + ); + + $searchParam1 = $query->createNamedParameter('%' . $this->dbc->escapeLikeParameter($search) . '%'); + $searchParam2 = $query->createNamedParameter('%' . $this->dbc->escapeLikeParameter($search) . '%'); + $searchParam3 = $query->createNamedParameter('%' . $this->dbc->escapeLikeParameter($search) . '%'); + $query->andWhere( + $query->expr()->orX( + $query->expr()->ilike('m.uid', $searchParam1), + $query->expr()->ilike('dn.value', $searchParam2), + $query->expr()->ilike('em.value', $searchParam3) + ) + ); } if ($limit !== -1) { @@ -174,7 +194,6 @@ public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0): arra } $result = $query->executeQuery(); - $users = []; while ($row = $result->fetch()) { $users[] = $row['uid']; @@ -244,14 +263,36 @@ public function removeFromGroup(string $uid, string $gid): bool { #[\Override] public function countUsersInGroup(string $gid, string $search = ''): int { $query = $this->dbc->getQueryBuilder(); - $query->select($query->func()->count('*', 'num_users')) - ->from(self::TABLE_MEMBERS) - ->where($query->expr()->eq('gid', $query->createNamedParameter($gid))); + $query->select( + $query->createFunction('COUNT(DISTINCT ' . $query->getColumnName('uid', 'm') . ')') + ) + ->from(self::TABLE_MEMBERS, 'm') + ->where($query->expr()->eq('m.gid', $query->createNamedParameter($gid))); if ($search !== '') { - $query->andWhere($query->expr()->like('uid', $query->createNamedParameter( - '%' . $this->dbc->escapeLikeParameter($search) . '%' - ))); + $query->leftJoin('m', 'accounts_data', 'dn', + $query->expr()->andX( + $query->expr()->eq('dn.uid', 'm.uid'), + $query->expr()->eq('dn.name', $query->createNamedParameter('displayname')) + ) + ); + $query->leftJoin('m', 'accounts_data', 'em', + $query->expr()->andX( + $query->expr()->eq('em.uid', 'm.uid'), + $query->expr()->eq('em.name', $query->createNamedParameter('email')) + ) + ); + + $searchParam1 = $query->createNamedParameter('%' . $this->dbc->escapeLikeParameter($search) . '%'); + $searchParam2 = $query->createNamedParameter('%' . $this->dbc->escapeLikeParameter($search) . '%'); + $searchParam3 = $query->createNamedParameter('%' . $this->dbc->escapeLikeParameter($search) . '%'); + $query->andWhere( + $query->expr()->orX( + $query->expr()->ilike('m.uid', $searchParam1), + $query->expr()->ilike('dn.value', $searchParam2), + $query->expr()->ilike('em.value', $searchParam3) + ) + ); } $result = $query->executeQuery(); diff --git a/lib/UserBackend.php b/lib/UserBackend.php index 3ee34fc41..baa1ef5e7 100644 --- a/lib/UserBackend.php +++ b/lib/UserBackend.php @@ -30,6 +30,7 @@ use OCP\User\Backend\ICustomLogout; use OCP\User\Backend\IGetDisplayNameBackend; use OCP\User\Backend\IGetHomeBackend; +use OCP\User\Backend\IProvideEnabledStateBackend; use OCP\User\Backend\ISetDisplayNameBackend; use OCP\User\Events\UserChangedEvent; use OCP\User\Events\UserFirstTimeLoggedInEvent; @@ -37,7 +38,7 @@ use Override; use Psr\Log\LoggerInterface; -class UserBackend extends ABackend implements IApacheBackend, IUserBackend, IGetDisplayNameBackend, ICountUsersBackend, IGetHomeBackend, ICustomLogout, ISetDisplayNameBackend { +class UserBackend extends ABackend implements IApacheBackend, IUserBackend, IGetDisplayNameBackend, ICountUsersBackend, IGetHomeBackend, ICustomLogout, ISetDisplayNameBackend, IProvideEnabledStateBackend { /** @var \OCP\UserInterface[] */ private static array $backends = []; @@ -389,6 +390,33 @@ public function autoprovisionAllowed(): bool { return $this->appConfig->getAppValueInt('general-require_provisioned_account') === 0; } + #[\Override] + public function isUserEnabled(string $uid, callable $queryDatabaseValue): bool { + $backend = $this->getActualUserBackend($uid); + if ($backend instanceof IProvideEnabledStateBackend) { + return $backend->isUserEnabled($uid, $queryDatabaseValue); + } + + return $queryDatabaseValue(); + } + + #[\Override] + public function setUserEnabled(string $uid, bool $enabled, callable $queryDatabaseValue, callable $setDatabaseValue): bool { + $backend = $this->getActualUserBackend($uid); + if ($backend instanceof IProvideEnabledStateBackend) { + return $backend->setUserEnabled($uid, $enabled, $queryDatabaseValue, $setDatabaseValue); + } + + $setDatabaseValue($enabled); + return $enabled; + } + + #[\Override] + public function getDisabledUserList(?int $limit = null, int $offset = 0, string $search = ''): array { + // user_saml does not keep a backend-specific disabled-user list beyond the core preference. + return []; + } + /** * Gets the actual user backend of the user * diff --git a/tests/unit/GroupBackendTest.php b/tests/unit/GroupBackendTest.php index 70bda39aa..d14691edf 100644 --- a/tests/unit/GroupBackendTest.php +++ b/tests/unit/GroupBackendTest.php @@ -15,9 +15,12 @@ */ class GroupBackendTest extends TestCase { private GroupBackend $groupBackend; + private IDBConnection $connection; private array $users = [ [ 'uid' => 'user_saml_integration_test_uid1', + 'displayname' => 'SAML Integration User One', + 'email' => 'saml-integration-one@example.test', 'groups' => [ 'user_saml_integration_test_gid1', 'SAML_user_saml_integration_test_gid2' @@ -25,6 +28,8 @@ class GroupBackendTest extends TestCase { ], [ 'uid' => 'user_saml_integration_test_uid2', + 'displayname' => 'SAML Integration User Two', + 'email' => 'saml-integration-two@example.test', 'groups' => [ 'user_saml_integration_test_gid1' ] @@ -59,7 +64,9 @@ class GroupBackendTest extends TestCase { #[Override] public function setUp(): void { parent::setUp(); - $this->groupBackend = new GroupBackend(\OCP\Server::get(IDBConnection::class), $this->createMock(LoggerInterface::class)); + $this->connection = \OCP\Server::get(IDBConnection::class); + $this->resetAccountData(); + $this->groupBackend = new GroupBackend($this->connection, $this->createMock(LoggerInterface::class)); foreach ($this->groups as $group) { $this->groupBackend->createGroup($group['gid'], $group['saml_gid']); } @@ -67,13 +74,15 @@ public function setUp(): void { foreach ($user['groups'] as $group) { $this->groupBackend->addToGroup($user['uid'], $group); } + $this->setAccountData($user['uid'], 'displayname', $user['displayname']); + $this->setAccountData($user['uid'], 'email', $user['email']); } } #[Override] public function tearDown(): void { parent::tearDown(); - $this->groupBackend = new GroupBackend(\OCP\Server::get(IDBConnection::class), $this->createMock(LoggerInterface::class)); + $this->groupBackend = new GroupBackend($this->connection, $this->createMock(LoggerInterface::class)); foreach ($this->users as $user) { foreach ($user['groups'] as $group) { $this->groupBackend->removeFromGroup($user['uid'], $group); @@ -82,6 +91,7 @@ public function tearDown(): void { foreach ($this->groups as $group) { $this->groupBackend->deleteGroup($group['gid']); } + $this->resetAccountData(); } public function testInGroup(): void { @@ -130,4 +140,49 @@ public function testUsersInGroups(): void { } } } + + public function testUsersInGroupMatchesDisplayNameAndEmail(): void { + $groupId = $this->groups[0]['gid']; + + $byDisplayName = $this->groupBackend->usersInGroup($groupId, $this->users[0]['displayname']); + $this->assertContains($this->users[0]['uid'], $byDisplayName, 'Display name search should return the matching user'); + + $byEmail = $this->groupBackend->usersInGroup($groupId, $this->users[1]['email']); + $this->assertContains($this->users[1]['uid'], $byEmail, 'Email search should return the matching user'); + + $byUid = $this->groupBackend->usersInGroup($groupId, $this->users[0]['uid']); + $this->assertContains($this->users[0]['uid'], $byUid, 'UID search should still work'); + } + + public function testCountUsersInGroupMatchesDisplayNameAndEmail(): void { + $groupId = $this->groups[0]['gid']; + + $this->assertSame(1, $this->groupBackend->countUsersInGroup($groupId, $this->users[0]['displayname'])); + $this->assertSame(1, $this->groupBackend->countUsersInGroup($groupId, $this->users[1]['email'])); + $this->assertSame(1, $this->groupBackend->countUsersInGroup($groupId, $this->users[0]['uid'])); + } + + private function resetAccountData(): void { + foreach ($this->users as $user) { + $qb = $this->connection->getQueryBuilder(); + $qb->delete('accounts_data') + ->where($qb->expr()->eq('uid', $qb->createNamedParameter($user['uid']))) + ->executeStatement(); + } + } + + private function setAccountData(string $uid, string $name, string $value): void { + $qb = $this->connection->getQueryBuilder(); + $qb->delete('accounts_data') + ->where($qb->expr()->eq('uid', $qb->createNamedParameter($uid))) + ->andWhere($qb->expr()->eq('name', $qb->createNamedParameter($name))) + ->executeStatement(); + + $qb = $this->connection->getQueryBuilder(); + $qb->insert('accounts_data') + ->setValue('uid', $qb->createNamedParameter($uid)) + ->setValue('name', $qb->createNamedParameter($name)) + ->setValue('value', $qb->createNamedParameter($value)) + ->executeStatement(); + } } diff --git a/tests/unit/UserBackendTest.php b/tests/unit/UserBackendTest.php index ace908a73..3ab06a72e 100644 --- a/tests/unit/UserBackendTest.php +++ b/tests/unit/UserBackendTest.php @@ -21,12 +21,17 @@ use OCP\IURLGenerator; use OCP\IUser; use OCP\IUserManager; +use OCP\User\Backend\IProvideEnabledStateBackend; use OCP\User\Events\UserChangedEvent; +use OCP\UserInterface; use Override; use PHPUnit\Framework\MockObject\MockObject; use Psr\Log\LoggerInterface; use Test\TestCase; +interface EnabledStateUserInterface extends UserInterface, IProvideEnabledStateBackend { +} + class UserBackendTest extends TestCase { private UserData&MockObject $userData; private IConfig&MockObject $config; @@ -103,6 +108,88 @@ public function testGetBackendName(): void { $this->assertSame('user_saml', $this->userBackend->getBackendName()); } + public function testIsUserEnabledDelegatesToActualBackend(): void { + $this->userBackend = $this->getMockedBuilder(['getActualUserBackend']); + /** @var EnabledStateUserInterface&MockObject $backend */ + $backend = $this->createMock(EnabledStateUserInterface::class); + $queryDatabaseValue = static fn (): bool => true; + + $this->userBackend + ->expects($this->once()) + ->method('getActualUserBackend') + ->with('ExistingUser') + ->willReturn($backend); + $backend + ->expects($this->once()) + ->method('isUserEnabled') + ->with('ExistingUser', $queryDatabaseValue) + ->willReturn(false); + + $this->assertFalse($this->userBackend->isUserEnabled('ExistingUser', $queryDatabaseValue)); + } + + public function testIsUserEnabledFallsBackToDatabaseValue(): void { + $this->userBackend = $this->getMockedBuilder(['getActualUserBackend']); + $queryDatabaseValue = static fn (): bool => false; + + $this->userBackend + ->expects($this->once()) + ->method('getActualUserBackend') + ->with('ExistingUser') + ->willReturn(null); + + $this->assertFalse($this->userBackend->isUserEnabled('ExistingUser', $queryDatabaseValue)); + } + + public function testSetUserEnabledDelegatesToActualBackend(): void { + $this->userBackend = $this->getMockedBuilder(['getActualUserBackend']); + /** @var EnabledStateUserInterface&MockObject $backend */ + $backend = $this->createMock(EnabledStateUserInterface::class); + $queryDatabaseValue = static fn (): bool => true; + $databaseValues = []; + $setDatabaseValue = static function (bool $enabled) use (&$databaseValues): void { + $databaseValues[] = $enabled; + }; + + $this->userBackend + ->expects($this->once()) + ->method('getActualUserBackend') + ->with('ExistingUser') + ->willReturn($backend); + $backend + ->expects($this->once()) + ->method('setUserEnabled') + ->with('ExistingUser', false, $queryDatabaseValue, $setDatabaseValue) + ->willReturn(false); + + $this->assertFalse($this->userBackend->setUserEnabled('ExistingUser', false, $queryDatabaseValue, $setDatabaseValue)); + $this->assertSame([], $databaseValues); + } + + public function testSetUserEnabledFallsBackToDatabaseValue(): void { + $this->userBackend = $this->getMockedBuilder(['getActualUserBackend']); + $queryDatabaseValue = static fn (): bool => true; + $databaseValues = []; + $setDatabaseValue = static function (bool $enabled) use (&$databaseValues): void { + $databaseValues[] = $enabled; + }; + + $this->userBackend + ->expects($this->once()) + ->method('getActualUserBackend') + ->with('ExistingUser') + ->willReturn(null); + + $this->assertFalse($this->userBackend->setUserEnabled('ExistingUser', false, $queryDatabaseValue, $setDatabaseValue)); + $this->assertSame([false], $databaseValues); + } + + public function testGetDisabledUserListReturnsEmptyArray(): void { + $this->userBackend = $this->getMockedBuilder(); + + $this->assertSame([], $this->userBackend->getDisabledUserList()); + } + public function testUpdateAttributesWithoutAttributes(): void { $this->userBackend = $this->getMockedBuilder(['getDisplayName']); $user = $this->createMock(IUser::class);