diff --git a/AGENTS.md b/AGENTS.md index 6433394..2d7b72b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -120,6 +120,7 @@ Nextcloud uses two **distinct** code paths for a group share — the picker (aut - Filter for users: `(&(puavoEduPersonAffiliation=teacher))` - Per-record school DN resolution: cached within a single sync run (see `$schoolNameCache`). - Background job runs every 15 minutes via `OCP\BackgroundJob\TimedJob`. Operators trigger immediate sync with `occ groupsharemachine:sync`. +- **A failed search must never prune.** `safeSearch()` returns `null` when the LDAP search throws, as distinct from `[]` for a genuinely empty page — an empty page ends the paging loop and lets `deleteNotIn($kept)` run, so returning `[]` on failure would delete every row and blank the picker for all teachers. On `null` the sync returns early with `complete => false` and skips the prune, leaving the table as the last good run wrote it. Keep that distinction if you touch the paging loop. ## Testing diff --git a/README.md b/README.md index 2a3c303..ee6d41a 100644 --- a/README.md +++ b/README.md @@ -107,6 +107,14 @@ Reading the counters: `kept=0` on either line means nothing matched — see the [Requirements](#requirements) above and the troubleshooting notes in [`DEVELOPMENT.md`](DEVELOPMENT.md). +A line can also end with `INCOMPLETE (prune skipped)`: + +``` +Groups: seen=120 kept=120 pruned=0 INCOMPLETE (prune skipped) +``` + +That means the LDAP walk did not finish — the directory became unreachable part-way, or user_ldap is not available at all. Rows are then left exactly as the last good run wrote them instead of being pruned against a partial read, because pruning against half a directory would delete every group the sync never got to see. The command exits non-zero so a cron wrapper or deploy script notices; the reason is in the Nextcloud log. Rerun once LDAP is reachable and the counters return to normal. + The 15-minute refresh runs as a Nextcloud background job, so it only happens if `cron.php` (or AJAX/webcron) is actually running on the instance — check with `occ background-job:list`. ### 3. Verify diff --git a/lib/BackgroundJob/SyncGroupTypesJob.php b/lib/BackgroundJob/SyncGroupTypesJob.php index fc167c8..bc33428 100644 --- a/lib/BackgroundJob/SyncGroupTypesJob.php +++ b/lib/BackgroundJob/SyncGroupTypesJob.php @@ -26,15 +26,24 @@ public function __construct( protected function run(mixed $argument): void { try { $stats = $this->sync->run(); - $this->logger->info(sprintf( - 'groupsharemachine sync: groups(seen=%d kept=%d pruned=%d) teachers(seen=%d kept=%d pruned=%d)', + $message = sprintf( + 'groupsharemachine sync: groups(seen=%d kept=%d pruned=%d complete=%s) teachers(seen=%d kept=%d pruned=%d complete=%s)', $stats['groups']['seen'], $stats['groups']['kept'], $stats['groups']['pruned'], + $stats['groups']['complete'] ? 'yes' : 'no', $stats['teachers']['seen'], $stats['teachers']['kept'], $stats['teachers']['pruned'], - )); + $stats['teachers']['complete'] ? 'yes' : 'no', + ); + if ($stats['groups']['complete'] && $stats['teachers']['complete']) { + $this->logger->info($message); + } else { + // Worth a warning: the tables are stale but intact, and will + // stay that way until a run completes. + $this->logger->warning($message . ' — prune skipped, tables left as they were'); + } } catch (Throwable $e) { $this->logger->error('groupsharemachine sync failed: ' . $e->getMessage(), ['exception' => $e]); } diff --git a/lib/Command/SyncGroupTypesCommand.php b/lib/Command/SyncGroupTypesCommand.php index 3250457..c3b4979 100644 --- a/lib/Command/SyncGroupTypesCommand.php +++ b/lib/Command/SyncGroupTypesCommand.php @@ -27,17 +27,29 @@ protected function configure(): void { protected function execute(InputInterface $input, OutputInterface $output): int { $stats = $this->sync->run(); $output->writeln(sprintf( - 'Groups: seen=%d kept=%d pruned=%d', + 'Groups: seen=%d kept=%d pruned=%d%s', $stats['groups']['seen'], $stats['groups']['kept'], $stats['groups']['pruned'], + $stats['groups']['complete'] ? '' : ' INCOMPLETE (prune skipped)', )); $output->writeln(sprintf( - 'Teachers: seen=%d kept=%d pruned=%d', + 'Teachers: seen=%d kept=%d pruned=%d%s', $stats['teachers']['seen'], $stats['teachers']['kept'], $stats['teachers']['pruned'], + $stats['teachers']['complete'] ? '' : ' INCOMPLETE (prune skipped)', )); - return self::SUCCESS; + + if ($stats['groups']['complete'] && $stats['teachers']['complete']) { + return self::SUCCESS; + } + + // Non-zero so a cron wrapper or deploy script notices. The tables were + // left alone rather than pruned against a partial read, so the previous + // contents are still in place — rerun once LDAP is reachable again. + $output->writeln(''); + $output->writeln('The LDAP walk did not finish. Nothing was pruned; see the Nextcloud log.'); + return self::FAILURE; } } diff --git a/lib/Service/LdapSync.php b/lib/Service/LdapSync.php index bdbce26..e05eddd 100644 --- a/lib/Service/LdapSync.php +++ b/lib/Service/LdapSync.php @@ -52,9 +52,14 @@ public function __construct( } /** + * `complete` is false when the LDAP walk did not finish — either user_ldap + * was unavailable or a search threw. Pruning is skipped in that case, so + * the table keeps whatever the last good run wrote rather than being + * emptied by a transient failure. + * * @return array{ - * groups: array{seen: int, kept: int, pruned: int}, - * teachers: array{seen: int, kept: int, pruned: int}, + * groups: array{seen: int, kept: int, pruned: int, complete: bool}, + * teachers: array{seen: int, kept: int, pruned: int, complete: bool}, * } */ public function run(): array { @@ -65,7 +70,7 @@ public function run(): array { } /** - * @return array{seen: int, kept: int, pruned: int} + * @return array{seen: int, kept: int, pruned: int, complete: bool} */ private function syncGroups(): array { $access = $this->resolveAccess('OCA\\User_LDAP\\Group_Proxy', isUser: false); @@ -102,6 +107,12 @@ private function syncGroups(): array { offset: $offset, extraAttrs: [$displayAttr, self::GROUP_ABBREVIATION_ATTR], ); + if ($records === null) { + // The walk is incomplete, so $kept is not the full set of + // groups in LDAP. Pruning against it would delete every group + // we never got to read. + return ['seen' => $seen, 'kept' => count($kept), 'pruned' => 0, 'complete' => false]; + } foreach ($records as $record) { $seen++; $dn = $record['dn'][0] ?? null; @@ -134,7 +145,7 @@ private function syncGroups(): array { } while (count($records) === self::PAGE_SIZE); $pruned = $this->groupMapper->deleteNotIn($kept); - return ['seen' => $seen, 'kept' => count($kept), 'pruned' => $pruned]; + return ['seen' => $seen, 'kept' => count($kept), 'pruned' => $pruned, 'complete' => true]; } private function resolveSchoolName(object $access, string $schoolDn): ?string { @@ -152,7 +163,7 @@ private function resolveSchoolName(object $access, string $schoolDn): ?string { } /** - * @return array{seen: int, kept: int, pruned: int} + * @return array{seen: int, kept: int, pruned: int, complete: bool} */ private function syncTeachers(): array { $access = $this->resolveAccess('OCA\\User_LDAP\\User_Proxy', isUser: true); @@ -172,6 +183,9 @@ private function syncTeachers(): array { $offset = 0; do { $records = $this->safeSearch($access, isUser: true, filter: $filter, offset: $offset); + if ($records === null) { + return ['seen' => $seen, 'kept' => count($kept), 'pruned' => 0, 'complete' => false]; + } foreach ($records as $record) { $seen++; $dn = $record['dn'][0] ?? null; @@ -200,14 +214,18 @@ private function syncTeachers(): array { } while (count($records) === self::PAGE_SIZE); $pruned = $this->teacherMapper->deleteNotIn($kept); - return ['seen' => $seen, 'kept' => count($kept), 'pruned' => $pruned]; + return ['seen' => $seen, 'kept' => count($kept), 'pruned' => $pruned, 'complete' => true]; } /** + * Returns null when the search failed, which the callers must not confuse + * with an empty page: an empty array ends the paging loop and lets the + * prune run, so returning one here would delete every row in the table. + * * @param list $extraAttrs additional (lowercased) attributes to request - * @return array + * @return ?array */ - private function safeSearch(object $access, bool $isUser, string $filter, int $offset, array $extraAttrs = []): array { + private function safeSearch(object $access, bool $isUser, string $filter, int $offset, array $extraAttrs = []): ?array { try { $attrs = $isUser ? ['dn', strtolower(self::SCHOOL_ATTR)] @@ -218,10 +236,12 @@ private function safeSearch(object $access, bool $isUser, string $filter, int $o : $access->searchGroups($filter, $attrs, self::PAGE_SIZE, $offset); return is_array($records) ? $records : []; } catch (Throwable $e) { - $this->logger->warning( - ($isUser ? 'searchUsers' : 'searchGroups') . " failed at offset {$offset}: " . $e->getMessage(), + $this->logger->error( + ($isUser ? 'searchUsers' : 'searchGroups') . " failed at offset {$offset}, " + . 'skipping the prune for this run: ' . $e->getMessage(), + ['exception' => $e], ); - return []; + return null; } } @@ -292,9 +312,10 @@ private static function escapeLdapValue(string $value): string { } /** - * @return array{seen: int, kept: int, pruned: int} + * @return array{seen: int, kept: int, pruned: int, complete: bool} */ private function emptyStats(): array { - return ['seen' => 0, 'kept' => 0, 'pruned' => 0]; + // complete=false: nothing was read, so nothing may be pruned either. + return ['seen' => 0, 'kept' => 0, 'pruned' => 0, 'complete' => false]; } } diff --git a/tests/Unit/Service/LdapSyncTest.php b/tests/Unit/Service/LdapSyncTest.php index b3357a9..859a0e5 100644 --- a/tests/Unit/Service/LdapSyncTest.php +++ b/tests/Unit/Service/LdapSyncTest.php @@ -51,8 +51,8 @@ public function testGroupSearchUpsertsAllowedTypes(): void { // Empty users list → no User_Proxy sample → teacher sync skipped entirely. $stats = $sync->run(); - $this->assertSame(['seen' => 2, 'kept' => 2, 'pruned' => 0], $stats['groups']); - $this->assertSame(['seen' => 0, 'kept' => 0, 'pruned' => 0], $stats['teachers']); + $this->assertSame(['seen' => 2, 'kept' => 2, 'pruned' => 0, 'complete' => true], $stats['groups']); + $this->assertSame(['seen' => 0, 'kept' => 0, 'pruned' => 0, 'complete' => false], $stats['teachers']); } public function testGroupSearchSkipsRecordsWithoutDnOrType(): void { @@ -155,7 +155,7 @@ public function testTeacherSearchWritesOneRowPerSchool(): void { ->willReturn(0); $stats = $sync->run(); - $this->assertSame(['seen' => 2, 'kept' => 3, 'pruned' => 0], $stats['teachers']); + $this->assertSame(['seen' => 2, 'kept' => 3, 'pruned' => 0, 'complete' => true], $stats['teachers']); } public function testTeacherWithoutAnySchoolIsSkipped(): void { @@ -227,8 +227,8 @@ protected function resolveProxy(string $class): ?object { $stats = $sync->run(); $this->assertSame( [ - 'groups' => ['seen' => 0, 'kept' => 0, 'pruned' => 0], - 'teachers' => ['seen' => 0, 'kept' => 0, 'pruned' => 0], + 'groups' => ['seen' => 0, 'kept' => 0, 'pruned' => 0, 'complete' => false], + 'teachers' => ['seen' => 0, 'kept' => 0, 'pruned' => 0, 'complete' => false], ], $stats, ); @@ -245,15 +245,40 @@ public function testEmptyLdapBecomesNoOp(): void { $this->assertSame(0, $stats['teachers']['seen']); } + public function testFailedSearchDoesNotPruneTheTables(): void { + // A search that throws used to be indistinguishable from the last page + // of results: the loop ended with an empty $kept and deleteNotIn([]) + // wiped every row, blanking the share picker until the next good sync. + $sync = $this->makeSync( + groups: [ + 'puavoid=10,ou=groups' => ['gid' => 'class_1a', 'puavoedugrouptype' => ['year class']], + ], + users: [ + 'puavoid=20,ou=people' => ['uid' => 'teacher1', 'puavoschool' => ['puavoId=1,ou=Schools']], + ], + searchThrows: true, + ); + + $this->groupMapper->expects($this->never())->method('deleteNotIn'); + $this->teacherMapper->expects($this->never())->method('deleteNotIn'); + + $stats = $sync->run(); + + $this->assertFalse($stats['groups']['complete']); + $this->assertFalse($stats['teachers']['complete']); + $this->assertSame(0, $stats['groups']['pruned']); + $this->assertSame(0, $stats['teachers']['pruned']); + } + /** * Build an LdapSync subclass with fake Group_Proxy / User_Proxy. * * @param array}> $groups DN-keyed * @param array $users DN-keyed */ - private function makeSync(array $groups, array $users, array $schools = []): LdapSyncTestDouble { - $groupAccess = $this->buildAccess($groups, $schools); - $userAccess = $this->buildAccess($users, []); + private function makeSync(array $groups, array $users, array $schools = [], bool $searchThrows = false): LdapSyncTestDouble { + $groupAccess = $this->buildAccess($groups, $schools, $searchThrows); + $userAccess = $this->buildAccess($users, [], $searchThrows); $groupSample = array_values($groups)[0]['gid'] ?? null; $userSample = array_values($users)[0]['uid'] ?? null; @@ -272,8 +297,8 @@ private function makeSync(array $groups, array $users, array $schools = []): Lda /** * @param array> $records DN-keyed test data */ - private function buildAccess(array $records, array $schoolDirectory): object { - return new class($records, $schoolDirectory) { + private function buildAccess(array $records, array $schoolDirectory, bool $searchThrows = false): object { + return new class($records, $schoolDirectory, $searchThrows) { /** @var list> */ public array $capturedFilterCalls = []; @@ -283,6 +308,7 @@ private function buildAccess(array $records, array $schoolDirectory): object { public function __construct( private array $records, private array $schoolDirectory, + private bool $searchThrows, ) { } @@ -300,13 +326,22 @@ public function combineFilterWithAnd(array $filters): string { } public function searchGroups(string $filter, array $attr, int $limit, int $offset): array { + $this->maybeThrow(); return $this->materialize($offset, $limit); } public function searchUsers(string $filter, array $attr, int $limit, int $offset): array { + $this->maybeThrow(); return $this->materialize($offset, $limit); } + private function maybeThrow(): void { + if ($this->searchThrows) { + // What a dropped connection looks like from in here. + throw new \RuntimeException('Can\'t contact LDAP server'); + } + } + private function materialize(int $offset, int $limit): array { $out = []; $dns = array_keys($this->records);