Skip to content

Commit da42883

Browse files
CarlSchwanCarlSchwan
authored
Merge pull request #997 from nextcloud/fix/search-disabled-users-by-name
search disabled users by display name or email
2 parents 7f33e52 + de55afa commit da42883

5 files changed

Lines changed: 236 additions & 17 deletions

File tree

‎.github/workflows/phpunit-oci.yml‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,12 @@ jobs:
7171
php-versions: ${{ fromJson(needs.matrix.outputs.php-version) }}
7272
server-versions: ${{ fromJson(needs.matrix.outputs.server-max) }}
7373
oci-versions: ['11', '18', '21', '23']
74+
exclude:
75+
# Nextcloud master configures Oracle sessions with `Etc/UTC`, which OCI 11
76+
# cannot resolve. Keep OCI 11 coverage on older branches and newer Oracle
77+
# coverage on master.
78+
- oci-versions: '11'
79+
server-versions: 'master'
7480

7581
name: OCI ${{ matrix.oci-versions }} PHP ${{ matrix.php-versions }} Nextcloud ${{ matrix.server-versions }}
7682

@@ -146,6 +152,8 @@ jobs:
146152
DB_PORT: 1521
147153
run: |
148154
mkdir data
155+
# Oracle is opt-in in newer server branches, so enable it before install.
156+
cp tests/databases-all-config.php config/databases-all.config.php
149157
./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
150158
./occ app:enable --force ${{ env.APP_NAME }}
151159

‎lib/GroupBackend.php‎

Lines changed: 55 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -155,15 +155,35 @@ public function groupExistsWithDifferentGid(string $samlGid): ?string {
155155
#[\Override]
156156
public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0): array {
157157
$query = $this->dbc->getQueryBuilder();
158-
$query->select('uid')
159-
->from(self::TABLE_MEMBERS)
160-
->where($query->expr()->eq('gid', $query->createNamedParameter($gid)))
161-
->orderBy('uid', 'ASC');
158+
$query->selectDistinct('m.uid')
159+
->from(self::TABLE_MEMBERS, 'm')
160+
->where($query->expr()->eq('m.gid', $query->createNamedParameter($gid)))
161+
->orderBy('m.uid', 'ASC');
162162

163163
if ($search !== '') {
164-
$query->andWhere($query->expr()->like('uid', $query->createNamedParameter(
165-
'%' . $this->dbc->escapeLikeParameter($search) . '%'
166-
)));
164+
$query->leftJoin('m', 'accounts_data', 'dn',
165+
$query->expr()->andX(
166+
$query->expr()->eq('dn.uid', 'm.uid'),
167+
$query->expr()->eq('dn.name', $query->createNamedParameter('displayname'))
168+
)
169+
);
170+
$query->leftJoin('m', 'accounts_data', 'em',
171+
$query->expr()->andX(
172+
$query->expr()->eq('em.uid', 'm.uid'),
173+
$query->expr()->eq('em.name', $query->createNamedParameter('email'))
174+
)
175+
);
176+
177+
$searchParam1 = $query->createNamedParameter('%' . $this->dbc->escapeLikeParameter($search) . '%');
178+
$searchParam2 = $query->createNamedParameter('%' . $this->dbc->escapeLikeParameter($search) . '%');
179+
$searchParam3 = $query->createNamedParameter('%' . $this->dbc->escapeLikeParameter($search) . '%');
180+
$query->andWhere(
181+
$query->expr()->orX(
182+
$query->expr()->ilike('m.uid', $searchParam1),
183+
$query->expr()->ilike('dn.value', $searchParam2),
184+
$query->expr()->ilike('em.value', $searchParam3)
185+
)
186+
);
167187
}
168188

169189
if ($limit !== -1) {
@@ -174,7 +194,6 @@ public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0): arra
174194
}
175195

176196
$result = $query->executeQuery();
177-
178197
$users = [];
179198
while ($row = $result->fetch()) {
180199
$users[] = $row['uid'];
@@ -244,14 +263,36 @@ public function removeFromGroup(string $uid, string $gid): bool {
244263
#[\Override]
245264
public function countUsersInGroup(string $gid, string $search = ''): int {
246265
$query = $this->dbc->getQueryBuilder();
247-
$query->select($query->func()->count('*', 'num_users'))
248-
->from(self::TABLE_MEMBERS)
249-
->where($query->expr()->eq('gid', $query->createNamedParameter($gid)));
266+
$query->select(
267+
$query->createFunction('COUNT(DISTINCT ' . $query->getColumnName('uid', 'm') . ')')
268+
)
269+
->from(self::TABLE_MEMBERS, 'm')
270+
->where($query->expr()->eq('m.gid', $query->createNamedParameter($gid)));
250271

251272
if ($search !== '') {
252-
$query->andWhere($query->expr()->like('uid', $query->createNamedParameter(
253-
'%' . $this->dbc->escapeLikeParameter($search) . '%'
254-
)));
273+
$query->leftJoin('m', 'accounts_data', 'dn',
274+
$query->expr()->andX(
275+
$query->expr()->eq('dn.uid', 'm.uid'),
276+
$query->expr()->eq('dn.name', $query->createNamedParameter('displayname'))
277+
)
278+
);
279+
$query->leftJoin('m', 'accounts_data', 'em',
280+
$query->expr()->andX(
281+
$query->expr()->eq('em.uid', 'm.uid'),
282+
$query->expr()->eq('em.name', $query->createNamedParameter('email'))
283+
)
284+
);
285+
286+
$searchParam1 = $query->createNamedParameter('%' . $this->dbc->escapeLikeParameter($search) . '%');
287+
$searchParam2 = $query->createNamedParameter('%' . $this->dbc->escapeLikeParameter($search) . '%');
288+
$searchParam3 = $query->createNamedParameter('%' . $this->dbc->escapeLikeParameter($search) . '%');
289+
$query->andWhere(
290+
$query->expr()->orX(
291+
$query->expr()->ilike('m.uid', $searchParam1),
292+
$query->expr()->ilike('dn.value', $searchParam2),
293+
$query->expr()->ilike('em.value', $searchParam3)
294+
)
295+
);
255296
}
256297

257298
$result = $query->executeQuery();

‎lib/UserBackend.php‎

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,14 +30,15 @@
3030
use OCP\User\Backend\ICustomLogout;
3131
use OCP\User\Backend\IGetDisplayNameBackend;
3232
use OCP\User\Backend\IGetHomeBackend;
33+
use OCP\User\Backend\IProvideEnabledStateBackend;
3334
use OCP\User\Backend\ISetDisplayNameBackend;
3435
use OCP\User\Events\UserChangedEvent;
3536
use OCP\User\Events\UserFirstTimeLoggedInEvent;
3637
use OCP\UserInterface;
3738
use Override;
3839
use Psr\Log\LoggerInterface;
3940

40-
class UserBackend extends ABackend implements IApacheBackend, IUserBackend, IGetDisplayNameBackend, ICountUsersBackend, IGetHomeBackend, ICustomLogout, ISetDisplayNameBackend {
41+
class UserBackend extends ABackend implements IApacheBackend, IUserBackend, IGetDisplayNameBackend, ICountUsersBackend, IGetHomeBackend, ICustomLogout, ISetDisplayNameBackend, IProvideEnabledStateBackend {
4142
/** @var \OCP\UserInterface[] */
4243
private static array $backends = [];
4344

@@ -389,6 +390,33 @@ public function autoprovisionAllowed(): bool {
389390
return $this->appConfig->getAppValueInt('general-require_provisioned_account') === 0;
390391
}
391392

393+
#[\Override]
394+
public function isUserEnabled(string $uid, callable $queryDatabaseValue): bool {
395+
$backend = $this->getActualUserBackend($uid);
396+
if ($backend instanceof IProvideEnabledStateBackend) {
397+
return $backend->isUserEnabled($uid, $queryDatabaseValue);
398+
}
399+
400+
return $queryDatabaseValue();
401+
}
402+
403+
#[\Override]
404+
public function setUserEnabled(string $uid, bool $enabled, callable $queryDatabaseValue, callable $setDatabaseValue): bool {
405+
$backend = $this->getActualUserBackend($uid);
406+
if ($backend instanceof IProvideEnabledStateBackend) {
407+
return $backend->setUserEnabled($uid, $enabled, $queryDatabaseValue, $setDatabaseValue);
408+
}
409+
410+
$setDatabaseValue($enabled);
411+
return $enabled;
412+
}
413+
414+
#[\Override]
415+
public function getDisabledUserList(?int $limit = null, int $offset = 0, string $search = ''): array {
416+
// user_saml does not keep a backend-specific disabled-user list beyond the core preference.
417+
return [];
418+
}
419+
392420
/**
393421
* Gets the actual user backend of the user
394422
*

‎tests/unit/GroupBackendTest.php‎

Lines changed: 57 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,16 +15,21 @@
1515
*/
1616
class GroupBackendTest extends TestCase {
1717
private GroupBackend $groupBackend;
18+
private IDBConnection $connection;
1819
private array $users = [
1920
[
2021
'uid' => 'user_saml_integration_test_uid1',
22+
'displayname' => 'SAML Integration User One',
23+
'email' => 'saml-integration-one@example.test',
2124
'groups' => [
2225
'user_saml_integration_test_gid1',
2326
'SAML_user_saml_integration_test_gid2'
2427
]
2528
],
2629
[
2730
'uid' => 'user_saml_integration_test_uid2',
31+
'displayname' => 'SAML Integration User Two',
32+
'email' => 'saml-integration-two@example.test',
2833
'groups' => [
2934
'user_saml_integration_test_gid1'
3035
]
@@ -59,21 +64,25 @@ class GroupBackendTest extends TestCase {
5964
#[Override]
6065
public function setUp(): void {
6166
parent::setUp();
62-
$this->groupBackend = new GroupBackend(\OCP\Server::get(IDBConnection::class), $this->createMock(LoggerInterface::class));
67+
$this->connection = \OCP\Server::get(IDBConnection::class);
68+
$this->resetAccountData();
69+
$this->groupBackend = new GroupBackend($this->connection, $this->createMock(LoggerInterface::class));
6370
foreach ($this->groups as $group) {
6471
$this->groupBackend->createGroup($group['gid'], $group['saml_gid']);
6572
}
6673
foreach ($this->users as $user) {
6774
foreach ($user['groups'] as $group) {
6875
$this->groupBackend->addToGroup($user['uid'], $group);
6976
}
77+
$this->setAccountData($user['uid'], 'displayname', $user['displayname']);
78+
$this->setAccountData($user['uid'], 'email', $user['email']);
7079
}
7180
}
7281

7382
#[Override]
7483
public function tearDown(): void {
7584
parent::tearDown();
76-
$this->groupBackend = new GroupBackend(\OCP\Server::get(IDBConnection::class), $this->createMock(LoggerInterface::class));
85+
$this->groupBackend = new GroupBackend($this->connection, $this->createMock(LoggerInterface::class));
7786
foreach ($this->users as $user) {
7887
foreach ($user['groups'] as $group) {
7988
$this->groupBackend->removeFromGroup($user['uid'], $group);
@@ -82,6 +91,7 @@ public function tearDown(): void {
8291
foreach ($this->groups as $group) {
8392
$this->groupBackend->deleteGroup($group['gid']);
8493
}
94+
$this->resetAccountData();
8595
}
8696

8797
public function testInGroup(): void {
@@ -130,4 +140,49 @@ public function testUsersInGroups(): void {
130140
}
131141
}
132142
}
143+
144+
public function testUsersInGroupMatchesDisplayNameAndEmail(): void {
145+
$groupId = $this->groups[0]['gid'];
146+
147+
$byDisplayName = $this->groupBackend->usersInGroup($groupId, $this->users[0]['displayname']);
148+
$this->assertContains($this->users[0]['uid'], $byDisplayName, 'Display name search should return the matching user');
149+
150+
$byEmail = $this->groupBackend->usersInGroup($groupId, $this->users[1]['email']);
151+
$this->assertContains($this->users[1]['uid'], $byEmail, 'Email search should return the matching user');
152+
153+
$byUid = $this->groupBackend->usersInGroup($groupId, $this->users[0]['uid']);
154+
$this->assertContains($this->users[0]['uid'], $byUid, 'UID search should still work');
155+
}
156+
157+
public function testCountUsersInGroupMatchesDisplayNameAndEmail(): void {
158+
$groupId = $this->groups[0]['gid'];
159+
160+
$this->assertSame(1, $this->groupBackend->countUsersInGroup($groupId, $this->users[0]['displayname']));
161+
$this->assertSame(1, $this->groupBackend->countUsersInGroup($groupId, $this->users[1]['email']));
162+
$this->assertSame(1, $this->groupBackend->countUsersInGroup($groupId, $this->users[0]['uid']));
163+
}
164+
165+
private function resetAccountData(): void {
166+
foreach ($this->users as $user) {
167+
$qb = $this->connection->getQueryBuilder();
168+
$qb->delete('accounts_data')
169+
->where($qb->expr()->eq('uid', $qb->createNamedParameter($user['uid'])))
170+
->executeStatement();
171+
}
172+
}
173+
174+
private function setAccountData(string $uid, string $name, string $value): void {
175+
$qb = $this->connection->getQueryBuilder();
176+
$qb->delete('accounts_data')
177+
->where($qb->expr()->eq('uid', $qb->createNamedParameter($uid)))
178+
->andWhere($qb->expr()->eq('name', $qb->createNamedParameter($name)))
179+
->executeStatement();
180+
181+
$qb = $this->connection->getQueryBuilder();
182+
$qb->insert('accounts_data')
183+
->setValue('uid', $qb->createNamedParameter($uid))
184+
->setValue('name', $qb->createNamedParameter($name))
185+
->setValue('value', $qb->createNamedParameter($value))
186+
->executeStatement();
187+
}
133188
}

‎tests/unit/UserBackendTest.php‎

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,12 +21,17 @@
2121
use OCP\IURLGenerator;
2222
use OCP\IUser;
2323
use OCP\IUserManager;
24+
use OCP\User\Backend\IProvideEnabledStateBackend;
2425
use OCP\User\Events\UserChangedEvent;
26+
use OCP\UserInterface;
2527
use Override;
2628
use PHPUnit\Framework\MockObject\MockObject;
2729
use Psr\Log\LoggerInterface;
2830
use Test\TestCase;
2931

32+
interface EnabledStateUserInterface extends UserInterface, IProvideEnabledStateBackend {
33+
}
34+
3035
class UserBackendTest extends TestCase {
3136
private UserData&MockObject $userData;
3237
private IConfig&MockObject $config;
@@ -103,6 +108,88 @@ public function testGetBackendName(): void {
103108
$this->assertSame('user_saml', $this->userBackend->getBackendName());
104109
}
105110

111+
public function testIsUserEnabledDelegatesToActualBackend(): void {
112+
$this->userBackend = $this->getMockedBuilder(['getActualUserBackend']);
113+
/** @var EnabledStateUserInterface&MockObject $backend */
114+
$backend = $this->createMock(EnabledStateUserInterface::class);
115+
$queryDatabaseValue = static fn (): bool => true;
116+
117+
$this->userBackend
118+
->expects($this->once())
119+
->method('getActualUserBackend')
120+
->with('ExistingUser')
121+
->willReturn($backend);
122+
$backend
123+
->expects($this->once())
124+
->method('isUserEnabled')
125+
->with('ExistingUser', $queryDatabaseValue)
126+
->willReturn(false);
127+
128+
$this->assertFalse($this->userBackend->isUserEnabled('ExistingUser', $queryDatabaseValue));
129+
}
130+
131+
public function testIsUserEnabledFallsBackToDatabaseValue(): void {
132+
$this->userBackend = $this->getMockedBuilder(['getActualUserBackend']);
133+
$queryDatabaseValue = static fn (): bool => false;
134+
135+
$this->userBackend
136+
->expects($this->once())
137+
->method('getActualUserBackend')
138+
->with('ExistingUser')
139+
->willReturn(null);
140+
141+
$this->assertFalse($this->userBackend->isUserEnabled('ExistingUser', $queryDatabaseValue));
142+
}
143+
144+
public function testSetUserEnabledDelegatesToActualBackend(): void {
145+
$this->userBackend = $this->getMockedBuilder(['getActualUserBackend']);
146+
/** @var EnabledStateUserInterface&MockObject $backend */
147+
$backend = $this->createMock(EnabledStateUserInterface::class);
148+
$queryDatabaseValue = static fn (): bool => true;
149+
$databaseValues = [];
150+
$setDatabaseValue = static function (bool $enabled) use (&$databaseValues): void {
151+
$databaseValues[] = $enabled;
152+
};
153+
154+
$this->userBackend
155+
->expects($this->once())
156+
->method('getActualUserBackend')
157+
->with('ExistingUser')
158+
->willReturn($backend);
159+
$backend
160+
->expects($this->once())
161+
->method('setUserEnabled')
162+
->with('ExistingUser', false, $queryDatabaseValue, $setDatabaseValue)
163+
->willReturn(false);
164+
165+
$this->assertFalse($this->userBackend->setUserEnabled('ExistingUser', false, $queryDatabaseValue, $setDatabaseValue));
166+
$this->assertSame([], $databaseValues);
167+
}
168+
169+
public function testSetUserEnabledFallsBackToDatabaseValue(): void {
170+
$this->userBackend = $this->getMockedBuilder(['getActualUserBackend']);
171+
$queryDatabaseValue = static fn (): bool => true;
172+
$databaseValues = [];
173+
$setDatabaseValue = static function (bool $enabled) use (&$databaseValues): void {
174+
$databaseValues[] = $enabled;
175+
};
176+
177+
$this->userBackend
178+
->expects($this->once())
179+
->method('getActualUserBackend')
180+
->with('ExistingUser')
181+
->willReturn(null);
182+
183+
$this->assertFalse($this->userBackend->setUserEnabled('ExistingUser', false, $queryDatabaseValue, $setDatabaseValue));
184+
$this->assertSame([false], $databaseValues);
185+
}
186+
187+
public function testGetDisabledUserListReturnsEmptyArray(): void {
188+
$this->userBackend = $this->getMockedBuilder();
189+
190+
$this->assertSame([], $this->userBackend->getDisabledUserList());
191+
}
192+
106193
public function testUpdateAttributesWithoutAttributes(): void {
107194
$this->userBackend = $this->getMockedBuilder(['getDisplayName']);
108195
$user = $this->createMock(IUser::class);

0 commit comments

Comments
 (0)