From 6cbc5d79d42c412124a4d0a2ce476818b2ee8c51 Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Mon, 5 Oct 2026 17:13:49 +0200 Subject: [PATCH 01/13] Phase 2: per-user backup and restore via user_migration Wires the existing RepositoryExportDestination/RepositoryImportSource (see lib/UserMigration/) into user_migration's own UserMigrationService, resolved on demand through a new UserMigrationServiceLocator since the app is an optional dependency (PLAN.md 3.8) -- without it, only this feature is unavailable, the full backup still works. New: UserBackupService::backupUser() (occ backup:backup:user) streams a user_migration export straight into the repository, under the same shared location lock as a regular backup. UserRestoreService::restoreUser() (occ backup:restore:user) never imports over a live account: --mode=replace safety-exports the current state first, then deletes and recreates the account from the backup under the same uid; --mode=as-backup restores into a new "-bak" account instead, since Nextcloud cannot rename a uid. The catalog now also tracks user export manifests (Repository::recordUserExport/ listUserExports/forgetUserExport/isRecordedUserExport), rollback-protected like a snapshot: listUserExports() verifies each one is still present on the location, and restore only ever reads a manifest the verified catalog actually recorded, never an arbitrary caller-supplied path. prune()'s existing raw users/ scan for blob retention is unchanged. A new short exclusive "catalog" lock (PruneService::catalogLockKey()) serializes the catalog write itself, so two concurrent per-user backups (or one alongside a full backup) can no longer race to write the same next generation and silently drop one of them -- the broader per-target shared lock only ever protected against a concurrent cleanup, not against other shared holders. Two real bugs found by testing this against a live Nextcloud instance, not by unit tests (RepositoryExportDestination/RepositoryImportSource implement OCP interfaces that plain PHPUnit can't load, see tests/Unit/Repository/UserExportTest.php's docblock; the new tests/integration/user-roundtrip.sh covers the real round trip instead): - RepositoryImportSource::copyToFolder() double-closed the stream it handed to putContent()/newFile() (both already close it themselves), which throws a TypeError on PHP 8 and broke every restored file with content. - A replace-mode restore failed on NotPermittedException: IRootFolder::getUserFolder() caches a Folder per uid for the process lifetime, so after delete+recreate of the same uid the import still got the deleted account's (now storage-less) folder. Fixed by dispatching FilesystemTornDownEvent, which Root already listens for to clear exactly that cache -- no internal OC\ classes needed. Local review (CodeRabbit + Codex) against this diff found three more real issues, all fixed: the catalog write race above, a failed as-backup import leaving a broken stub account behind (now deleted on failure), and an export stream leak on a read/backend failure in RepositoryExportDestination::copyFolder() (now closed in a finally). Deliberately left out: wiring per-user export into the automatic backup schedule (on-demand via occ only, for now) and retention/pruning of old user exports -- both open design questions, not yet decided. The lease covering a single export/import still has a fixed TTL (3600s) with no mid-call refresh, since UserMigrationService exposes no progress; a resumable, step-based per-user backup (like the main BackupService run) would remove that cap but is a bigger change than this pass. --- .github/workflows/ci.yml | 17 +++ appinfo/info.xml | 2 + lib/Command/BackupUser.php | 59 ++++++++ lib/Command/RestoreUser.php | 57 ++++++++ lib/Repository/Catalog.php | 11 +- lib/Repository/Repository.php | 47 +++++- lib/Service/PruneService.php | 13 ++ lib/Service/UserBackupService.php | 69 +++++++++ lib/Service/UserRestoreService.php | 137 ++++++++++++++++++ .../RepositoryExportDestination.php | 7 +- lib/UserMigration/RepositoryImportSource.php | 9 +- .../UserMigrationServiceLocator.php | 35 +++++ psalm.xml | 9 ++ tests/Unit/Repository/UserExportTest.php | 109 ++++++++++++++ tests/integration/user-roundtrip.sh | 59 ++++++++ 15 files changed, 634 insertions(+), 6 deletions(-) create mode 100644 lib/Command/BackupUser.php create mode 100644 lib/Command/RestoreUser.php create mode 100644 lib/Service/UserBackupService.php create mode 100644 lib/Service/UserRestoreService.php create mode 100644 lib/UserMigration/UserMigrationServiceLocator.php create mode 100644 tests/Unit/Repository/UserExportTest.php create mode 100755 tests/integration/user-roundtrip.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a8591bd..3a37530 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -60,3 +60,20 @@ jobs: - uses: actions/checkout@v4 - name: Round trip against S3 and SFTP run: tests/integration/targets-matrix.sh + + integration-user: + # Per-user backup/restore via user_migration (occ backup:backup:user / backup:restore:user), + # an optional dependency installed from the app store. Does not depend on the Nextcloud + # version or database beyond what the main integration job already covers, so this runs once. + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - name: Set up a fresh Nextcloud instance + run: | + docker run -d --name ngbu -e SQLITE_DATABASE=nextcloud -e NEXTCLOUD_ADMIN_USER=admin \ + -e NEXTCLOUD_ADMIN_PASSWORD=admin-user-roundtrip -e NEXTCLOUD_TRUSTED_DOMAINS=localhost \ + -v "$PWD:/var/www/html/custom_apps/ng_backup:ro" nextcloud:35-apache + for i in $(seq 1 90); do docker exec -u www-data ngbu php occ status 2>/dev/null | grep -q "installed: true" && break; sleep 4; done + docker exec ngbu chown www-data:www-data /var/www/html/custom_apps + - name: Per-user backup/restore round trip + run: tests/integration/user-roundtrip.sh "docker exec -u www-data ngbu" /var/www/html/data diff --git a/appinfo/info.xml b/appinfo/info.xml index 2314101..348c967 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -47,6 +47,8 @@ relying on this for anything.]]> OCA\NgBackup\Command\KeySlotAdd OCA\NgBackup\Command\KeySlotReplace OCA\NgBackup\Command\KeySlotRemove + OCA\NgBackup\Command\BackupUser + OCA\NgBackup\Command\RestoreUser OCA\NgBackup\Settings\Admin diff --git a/lib/Command/BackupUser.php b/lib/Command/BackupUser.php new file mode 100644 index 0000000..75f0d35 --- /dev/null +++ b/lib/Command/BackupUser.php @@ -0,0 +1,59 @@ +setName('backup:backup:user') + ->setDescription('Back up one user via user_migration (account, settings, files and everything else a migrator covers)') + ->addArgument('target', InputArgument::REQUIRED, 'Location name or id') + ->addArgument('user', InputArgument::REQUIRED, 'User id') + ->setHelp(<<<'HELP' +Streams a user_migration export straight into the encrypted repository: no zip, no copy in the +user's own storage. Shares data with the regular backup; this needs the user_migration app +enabled. The manifest path this prints is what occ backup:restore:user expects. + +Example: + occ backup:backup:user offsite alice +HELP); + } + + protected function execute(InputInterface $input, OutputInterface $output): int { + $uid = (string)$input->getArgument('user'); + $user = $this->userManager->get($uid); + if ($user === null) { + $output->writeln("No such user: $uid"); + return 1; + } + try { + $target = $this->targets->get($input->getArgument('target')); + $manifest = $this->userBackup->backupUser($target, $user); + } catch (\Throwable $e) { + $output->writeln('' . $e->getMessage() . ''); + return 1; + } + $output->writeln("Backed up $uid to $manifest."); + return 0; + } +} diff --git a/lib/Command/RestoreUser.php b/lib/Command/RestoreUser.php new file mode 100644 index 0000000..90e2a95 --- /dev/null +++ b/lib/Command/RestoreUser.php @@ -0,0 +1,57 @@ +setName('backup:restore:user') + ->setDescription('Restore a user from a user_migration export (see backup:backup:user)') + ->addArgument('target', InputArgument::REQUIRED, 'Location name or id') + ->addArgument('manifest', InputArgument::REQUIRED, 'Manifest path printed by backup:backup:user (users//)') + ->addOption('mode', 'm', InputOption::VALUE_REQUIRED, 'replace or as-backup', UserRestoreService::MODE_REPLACE) + ->setHelp(<<<'HELP' +Never imports over a live account. Modes: + replace (default) safety-exports the current account first, then deletes and recreates it + fresh from the backup, under the same user id + as-backup restores into a new account "-bak" (or "-bak-2", ...) instead, so an admin can + compare and transfer by hand; Nextcloud cannot rename a user id, so the backup gets + the new name, never the existing account + +Example: + occ backup:restore:user offsite users/alice/20261005T120000Z-ab12cd34 + occ backup:restore:user offsite users/alice/20261005T120000Z-ab12cd34 --mode=as-backup +HELP); + } + + protected function execute(InputInterface $input, OutputInterface $output): int { + try { + $target = $this->targets->get($input->getArgument('target')); + $user = $this->userRestore->restoreUser($target, (string)$input->getArgument('manifest'), (string)$input->getOption('mode')); + } catch (\Throwable $e) { + $output->writeln('' . $e->getMessage() . ''); + return 1; + } + $output->writeln('Restored ' . $user->getUID() . '.'); + return 0; + } +} diff --git a/lib/Repository/Catalog.php b/lib/Repository/Catalog.php index 56ec8fd..44fb6c4 100644 --- a/lib/Repository/Catalog.php +++ b/lib/Repository/Catalog.php @@ -28,6 +28,8 @@ final class Catalog { public array $snapshots = []; /** @var array */ public array $trash = []; + /** @var array manifest path => true, for a per-user export (users//) */ + public array $userExports = []; public string $hash = ''; public function __construct( @@ -39,7 +41,7 @@ final class Catalog { } /** Load and verify the newest catalog; create the first one for repositories that have none. */ - public function load(callable $listSnapshots, callable $listTrash): void { + public function load(callable $listSnapshots, callable $listTrash, callable $listUserExports): void { $gens = $this->generations(); if ($gens === []) { // First use (new or pre-catalog repository): adopt the current state as generation 1. @@ -49,6 +51,7 @@ final class Catalog { } $this->snapshots = array_fill_keys($listSnapshots(), true); $this->trash = array_fill_keys($listTrash(), true); + $this->userExports = array_fill_keys($listUserExports(), true); $this->write(); return; } @@ -76,6 +79,9 @@ final class Catalog { $this->hash = $hash; $this->snapshots = array_fill_keys($data['snapshots'], true); $this->trash = array_fill_keys($data['trash'], true); + // Catalogs written before user exports existed have no such key: default to empty rather + // than adopting the location's current users/ listing (that would skip verification). + $this->userExports = array_fill_keys($data['userExports'] ?? [], true); $this->anchor?->set($this->repositoryId, $this->gen, $this->hash); } @@ -83,7 +89,8 @@ final class Catalog { public function write(): void { $next = $this->gen + 1; $plain = json_encode(['repository' => $this->repositoryId, 'gen' => $next, 'time' => gmdate('c'), 'prev' => $this->hash, - 'snapshots' => array_keys($this->snapshots), 'trash' => array_keys($this->trash)], JSON_THROW_ON_ERROR); + 'snapshots' => array_keys($this->snapshots), 'trash' => array_keys($this->trash), + 'userExports' => array_keys($this->userExports)], JSON_THROW_ON_ERROR); $encrypted = $this->cipher->encryptString($plain, $this->ad($next)); $stream = fopen('php://memory', 'w+b'); fwrite($stream, $encrypted); diff --git a/lib/Repository/Repository.php b/lib/Repository/Repository.php index f575fa5..59d83bf 100644 --- a/lib/Repository/Repository.php +++ b/lib/Repository/Repository.php @@ -91,7 +91,8 @@ final class Repository { $this->catalog = new Catalog($this->backend, $this->cipher, $this->repositoryId, $anchor); $this->catalog->load( fn () => array_map('basename', $this->backend->list('snapshots')), - fn () => array_map('basename', $this->backend->list('trash/info'))); + fn () => array_map('basename', $this->backend->list('trash/info')), + fn () => $this->backend->list('users')); } /** Refuse uploads that would make the stored data larger than this (null = no limit). */ @@ -374,6 +375,50 @@ final class Repository { $this->backend->delete('trash/info/' . $snapshotId); } + /** Record a user_migration export's manifest in the catalog (rollback-protected like a snapshot). */ + public function recordUserExport(string $manifestPath): void { + $this->catalog->userExports[$manifestPath] = true; + $this->catalog->write(); + } + + /** + * User export manifests according to the verified catalog, newest first. A manifest the + * catalog lists but the location no longer has means it was deleted or hidden behind NG + * Backup's back, same as a missing snapshot (see listSnapshots()). + * + * @return list + */ + public function listUserExports(?string $uid = null): array { + $prefix = $uid !== null ? 'users/' . $uid . '/' : 'users/'; + $out = []; + foreach (array_keys($this->catalog->userExports) as $path) { + if (!str_starts_with($path, $prefix)) { + continue; + } + if (!$this->backend->exists($path)) { + throw new RepositoryException("User export $path is missing on the location (deleted or hidden outside NG Backup)"); + } + $out[] = $path; + } + sort($out, SORT_STRING); + return array_reverse($out); + } + + /** Whether $manifestPath is a user export the verified catalog actually recorded. */ + public function isRecordedUserExport(string $manifestPath): bool { + return isset($this->catalog->userExports[$manifestPath]); + } + + /** Permanently delete a user export. Its blobs are freed by the next prune() like any other. */ + public function forgetUserExport(string $manifestPath): void { + if (!isset($this->catalog->userExports[$manifestPath])) { + throw new RepositoryException("$manifestPath is not a recorded user export"); + } + $this->backend->delete($manifestPath); + unset($this->catalog->userExports[$manifestPath]); + $this->catalog->write(); + } + /** @return array */ public function trash(): array { $out = []; diff --git a/lib/Service/PruneService.php b/lib/Service/PruneService.php index 97b2d6e..2c78399 100644 --- a/lib/Service/PruneService.php +++ b/lib/Service/PruneService.php @@ -98,6 +98,19 @@ final class PruneService { return 'ng_backup/target/' . $target->getId(); } + /** + * A separate, short-lived exclusive lock serializing writes to the catalog itself. The + * per-target lock above only keeps readers/writers out while a cleanup runs; it does not + * stop two SHARED holders (e.g. two concurrent per-user backups, or one alongside a full + * backup) from both reading the same catalog generation and racing to write the next one, + * silently dropping whichever's change loses. The full-backup path avoids this today only + * because a Run row makes concurrent runs for the same target mutually exclusive; per-user + * backup/restore has no such row, so it takes this lock around the catalog write itself. + */ + public static function catalogLockKey(Target $target): string { + return self::lockKey($target) . '/catalog'; + } + public function policy(): RetentionPolicy { return RetentionPolicy::fromArray(json_decode($this->appConfig->getValueString(Application::APP_ID, 'retention', '{}'), true) ?: []); } diff --git a/lib/Service/UserBackupService.php b/lib/Service/UserBackupService.php new file mode 100644 index 0000000..e4eb877 --- /dev/null +++ b/lib/Service/UserBackupService.php @@ -0,0 +1,69 @@ +/. + * + * user_migration is an optional dependency (unlike files_external): without it, only this + * per-user backup/restore is unavailable, the full backup still works. + */ +final class UserBackupService { + public function __construct( + private TargetService $targets, + private LeaseService $leases, + private UserMigrationServiceLocator $locator, + ) { + } + + /** @throws UserMigrationException when user_migration is not enabled, or the export itself fails */ + public function backupUser(Target $target, IUser $user): string { + $service = $this->locator->get(); + return $this->withSharedLock($target, function () use ($target, $user, $service) { + $repo = $this->targets->repository($target); + $dest = new RepositoryExportDestination($repo, $user->getUID()); + $service->export($dest, $user); + // A short exclusive lock around the catalog write only: the shared lock above + // allows another concurrent backup (full or per-user) to be reading the same + // catalog generation at the same time, and both writing the next one would let + // one silently overwrite the other's addition. + $this->withCatalogLock($target, fn () => $repo->recordUserExport($dest->manifestPath())); + return $dest->manifestPath(); + }); + } + + private function withSharedLock(Target $target, callable $fn): mixed { + try { + // UserMigrationService::export() exposes no progress, so unlike + // RestoreService::restoreUserFiles() this lease cannot be refreshed mid-call; a very + // large account could in theory outrun this TTL. Resumable, step-based per-user + // backup (like the main BackupService run) is a possible future improvement. + return $this->leases->with(PruneService::lockKey($target), LeaseService::SHARED, 3600, $fn); + } catch (LockedException) { + throw new \RuntimeException('A cleanup of ' . $target->getName() . ' is in progress; try again later'); + } + } + + private function withCatalogLock(Target $target, callable $fn): mixed { + try { + return $this->leases->with(PruneService::catalogLockKey($target), LeaseService::EXCLUSIVE, 30, $fn); + } catch (LockedException) { + throw new \RuntimeException('Another catalog write for ' . $target->getName() . ' is in progress; try again'); + } + } +} diff --git a/lib/Service/UserRestoreService.php b/lib/Service/UserRestoreService.php new file mode 100644 index 0000000..68cf24a --- /dev/null +++ b/lib/Service/UserRestoreService.php @@ -0,0 +1,137 @@ +-bak" (or "-bak-2", ...) instead, + * so an admin can compare and transfer by hand. Nextcloud cannot rename a uid, so the *backup* + * gets the new name, never the existing account. + */ +final class UserRestoreService { + public const MODE_REPLACE = 'replace'; + public const MODE_AS_BACKUP = 'as-backup'; + + public function __construct( + private TargetService $targets, + private LeaseService $leases, + private UserMigrationServiceLocator $locator, + private IUserManager $userManager, + private ISecureRandom $random, + private IEventDispatcher $events, + ) { + } + + /** @throws UserMigrationException when user_migration is not enabled, or the restore itself fails */ + public function restoreUser(Target $target, string $manifestPath, string $mode = self::MODE_REPLACE): IUser { + $service = $this->locator->get(); + return $this->withSharedLock($target, function () use ($target, $manifestPath, $mode, $service): IUser { + $repo = $this->targets->repository($target); + // Only ever read a manifest the verified catalog actually recorded: a path alone + // (even one this app generated before) proves nothing about it still being current + // or genuine once the catalog's rollback protection is supposed to vouch for it. + if (!$repo->isRecordedUserExport($manifestPath)) { + throw new \InvalidArgumentException("$manifestPath is not a recorded user export for this location"); + } + $source = new RepositoryImportSource($repo, $manifestPath); + $originalUid = $source->getOriginalUid(); + + if ($mode === self::MODE_AS_BACKUP) { + $backupUid = $this->freeBackupUid($originalUid); + $user = $this->userManager->createUser($backupUid, $this->random->generate(32)); + if ($user === false) { + throw new \RuntimeException("Could not create the account $backupUid"); + } + try { + return $service->import($source, $user); + } catch (\Throwable $e) { + // A partially-imported, never-used account left behind is strictly worse + // than no account at all; the original source account was never touched. + $user->delete(); + throw $e; + } + } + + if ($mode !== self::MODE_REPLACE) { + throw new \InvalidArgumentException("Unknown restore mode: $mode"); + } + $existing = $this->userManager->get($originalUid); + $safetyManifestPath = null; + if ($existing !== null) { + // Never import over a live account: export its current state first, so nothing + // is lost if the restore turns out to be the wrong choice. + $safety = new RepositoryExportDestination($repo, $originalUid); + $service->export($safety, $existing); + $safetyManifestPath = $safety->manifestPath(); + // Same race as UserBackupService::backupUser(): serialize the catalog write + // itself against any other concurrent backup/restore for this target. + $this->withCatalogLock($target, fn () => $repo->recordUserExport($safetyManifestPath)); + if (!$existing->delete()) { + throw new \RuntimeException("Could not delete the existing account $originalUid before restoring; its safety export is at $safetyManifestPath"); + } + // IRootFolder caches a Folder per uid for the lifetime of this process; without + // this it would still hand the just-deleted account's (now storage-less) folder + // to the import below, and every write would fail. + $this->events->dispatchTyped(new FilesystemTornDownEvent()); + } + try { + return $service->import($source, null); + } catch (\Throwable $e) { + // The original account is already gone at this point (or never existed); a + // failure here can only be reported, not undone in place. Its safety export + // (if any) is the recovery path, already recorded above. + throw new UserMigrationException( + "Could not restore $originalUid" . ($safetyManifestPath !== null ? "; its safety export is at $safetyManifestPath" : '') . ': ' . $e->getMessage(), + 0, + $e, + ); + } + }); + } + + private function freeBackupUid(string $uid): string { + $candidate = $uid . '-bak'; + for ($n = 2; $this->userManager->userExists($candidate); $n++) { + $candidate = $uid . '-bak-' . $n; + } + return $candidate; + } + + private function withSharedLock(Target $target, callable $fn): mixed { + try { + // See UserBackupService::withSharedLock(): UserMigrationService exposes no progress, + // so this lease cannot be refreshed mid-call. + return $this->leases->with(PruneService::lockKey($target), LeaseService::SHARED, 3600, $fn); + } catch (LockedException) { + throw new \RuntimeException('A cleanup of ' . $target->getName() . ' is in progress; try again later'); + } + } + + private function withCatalogLock(Target $target, callable $fn): mixed { + try { + return $this->leases->with(PruneService::catalogLockKey($target), LeaseService::EXCLUSIVE, 30, $fn); + } catch (LockedException) { + throw new \RuntimeException('Another catalog write for ' . $target->getName() . ' is in progress; try again'); + } + } +} diff --git a/lib/UserMigration/RepositoryExportDestination.php b/lib/UserMigration/RepositoryExportDestination.php index 3eb1ce1..8eec0d1 100644 --- a/lib/UserMigration/RepositoryExportDestination.php +++ b/lib/UserMigration/RepositoryExportDestination.php @@ -65,8 +65,11 @@ final class RepositoryExportDestination implements IExportDestination { if ($stream === false) { throw new \RuntimeException('Cannot read ' . $node->getPath()); } - $this->addFileAsStream($target, $stream); - fclose($stream); + try { + $this->addFileAsStream($target, $stream); + } finally { + fclose($stream); + } } } } diff --git a/lib/UserMigration/RepositoryImportSource.php b/lib/UserMigration/RepositoryImportSource.php index a8b4898..84a6acc 100644 --- a/lib/UserMigration/RepositoryImportSource.php +++ b/lib/UserMigration/RepositoryImportSource.php @@ -109,7 +109,14 @@ final class RepositoryImportSource implements IImportSource { $destination->newFile($name, $stream); } } finally { - fclose($stream); + // putContent()/newFile() already close the stream once they've consumed it: + // despite the @return resource docblock above, $stream is no longer a + // resource by the time we get here (verified against a real storage; a + // double fclose() throws a TypeError on PHP 8). + /** @psalm-suppress RedundantConditionGivenDocblockType */ + if (is_resource($stream)) { + fclose($stream); + } } } } diff --git a/lib/UserMigration/UserMigrationServiceLocator.php b/lib/UserMigration/UserMigrationServiceLocator.php new file mode 100644 index 0000000..aa95807 --- /dev/null +++ b/lib/UserMigration/UserMigrationServiceLocator.php @@ -0,0 +1,35 @@ +appManager->isEnabledForAnyone('user_migration')) { + throw new UserMigrationException('The user_migration app is not enabled; per-user backup/restore is unavailable'); + } + $this->appManager->loadApp('user_migration'); + return Server::get(\OCA\UserMigration\Service\UserMigrationService::class); + } +} diff --git a/psalm.xml b/psalm.xml index d694c57..7561477 100644 --- a/psalm.xml +++ b/psalm.xml @@ -31,8 +31,17 @@ + + + + + + + + diff --git a/tests/Unit/Repository/UserExportTest.php b/tests/Unit/Repository/UserExportTest.php new file mode 100644 index 0000000..8750dc6 --- /dev/null +++ b/tests/Unit/Repository/UserExportTest.php @@ -0,0 +1,109 @@ +root = $this->tempDir(); + $this->backend = new LocalBackend($this->root); + $this->keys = KeyRing::generate(); + $this->anchor = new class implements ICatalogAnchor { + public array $data = []; + public function get(string $r): ?array { return $this->data[$r] ?? null; } + public function set(string $r, int $g, string $h): void { $this->data[$r] = ['gen' => $g, 'hash' => $h]; } + }; + } + + private function open(): Repository { + return Repository::openWithKey($this->backend, $this->keys, null, $this->anchor); + } + + /** Stands in for an export destination's close(): a manifest object plus a catalog record. */ + private function putManifest(Repository $repo, string $path): void { + $repo->putObject($path, json_encode(['uid' => 'alice'])); + $repo->recordUserExport($path); + } + + public function testRecordAndListSurviveAReopen(): void { + $repo = Repository::initWithKey($this->backend, $this->keys, $this->keys->wrap('pw'), $this->anchor); + $this->putManifest($repo, 'users/alice/20261005T120000Z-aaaa0001'); + + $again = $this->open(); + $this->assertSame(['users/alice/20261005T120000Z-aaaa0001'], $again->listUserExports()); + $this->assertSame(['users/alice/20261005T120000Z-aaaa0001'], $again->listUserExports('alice')); + $this->assertSame([], $again->listUserExports('bob')); + } + + public function testListIsNewestFirst(): void { + $repo = Repository::initWithKey($this->backend, $this->keys, $this->keys->wrap('pw'), $this->anchor); + $this->putManifest($repo, 'users/alice/20261005T120000Z-aaaa0001'); + $this->putManifest($repo, 'users/alice/20261006T120000Z-aaaa0002'); + + $this->assertSame( + ['users/alice/20261006T120000Z-aaaa0002', 'users/alice/20261005T120000Z-aaaa0001'], + $repo->listUserExports('alice'), + ); + } + + public function testForgetRemovesItFromTheCatalogAndLocation(): void { + $repo = Repository::initWithKey($this->backend, $this->keys, $this->keys->wrap('pw'), $this->anchor); + $path = 'users/alice/20261005T120000Z-aaaa0001'; + $this->putManifest($repo, $path); + + $repo->forgetUserExport($path); + + $this->assertSame([], $repo->listUserExports()); + $this->assertFalse($this->backend->exists($path)); + } + + public function testForgetRejectsAPathNotInTheCatalog(): void { + $repo = Repository::initWithKey($this->backend, $this->keys, $this->keys->wrap('pw'), $this->anchor); + $this->expectException(RepositoryException::class); + $repo->forgetUserExport('users/nobody/not-a-real-export'); + } + + public function testIsRecordedUserExport(): void { + $repo = Repository::initWithKey($this->backend, $this->keys, $this->keys->wrap('pw'), $this->anchor); + $path = 'users/alice/20261005T120000Z-aaaa0001'; + $this->putManifest($repo, $path); + + $this->assertTrue($repo->isRecordedUserExport($path)); + $this->assertFalse($repo->isRecordedUserExport('users/alice/not-the-real-one')); + } + + public function testListingDetectsAManifestRemovedBehindItsBack(): void { + // A catalog-listed export whose object disappeared from the location (deleted or + // hidden outside NG Backup) must be detected, the same as a missing snapshot. + $repo = Repository::initWithKey($this->backend, $this->keys, $this->keys->wrap('pw'), $this->anchor); + $path = 'users/alice/20261005T120000Z-aaaa0001'; + $this->putManifest($repo, $path); + $this->backend->delete($path); + + $this->expectException(RepositoryException::class); + $this->expectExceptionMessage('missing on the location'); + $repo->listUserExports(); + } +} diff --git a/tests/integration/user-roundtrip.sh b/tests/integration/user-roundtrip.sh new file mode 100755 index 0000000..69d3197 --- /dev/null +++ b/tests/integration/user-roundtrip.sh @@ -0,0 +1,59 @@ +#!/bin/bash +# Per-user backup/restore round trip via user_migration (optional dependency, see PLAN.md 3.8): +# occ backup:backup:user / occ backup:restore:user --mode=as-backup|replace. +# Usage: user-roundtrip.sh "" [target-args] +# e.g. user-roundtrip.sh "docker exec -u www-data ngb-nc35-pgsql" /var/www/html/data +set -euo pipefail +X="$1"; DATA="$2" +TARGET_ARGS="${3:-local -a null::null -o datadir=/tmp/ngb-urt-target}" +occ() { $X php occ "$@"; } +fail() { echo "FAIL: $*"; exit 1; } +ok() { echo "ok $*"; } +U=ngbutest-$RANDOM + +occ app:enable ng_backup >/dev/null +if ! occ app:install user_migration >/dev/null 2>&1 && ! occ app:enable user_migration >/dev/null 2>&1; then + echo "SKIP: user_migration is not available (no network, or the app store is unreachable) -- per-user backup/restore not tested" + exit 0 +fi +sleep 8 # APCu app config cache + +$X sh -c "printf 'user roundtrip passphrase 2026\n' > /tmp/ngb-urt-pass && mkdir -p /tmp/ngb-urt-target" +occ backup:key:init --passphrase-file=/tmp/ngb-urt-pass --label=CI --delete-delay-days=7 >/dev/null || fail "key init" +CODE=$(occ backup:key:kit | php -r 'echo json_decode(stream_get_contents(STDIN), true)["confirmation_code"];') +occ backup:key:confirm -u admin --code="$CODE" --confirm="Yes, I confirm" >/dev/null || fail "confirm" +ok "key, kit code and confirmation" + +occ backup:target:add ci $TARGET_ARGS --path=repo >/dev/null || fail "target add" +ok "location added" + +$X sh -c "OC_PASS='User-roundtrip-pw-2026' php occ user:add --password-from-env $U >/dev/null" +$X sh -c "mkdir -p $DATA/$U/files/Docs && for i in 1 2 3; do head -c 100000 /dev/urandom > $DATA/$U/files/Docs/f\$i.bin; done" +occ files:scan "$U" >/dev/null +SUM_BEFORE=$($X sh -c "cd $DATA/$U/files && find . -type f -print0 | sort -z | xargs -0 md5sum | md5sum") + +MANIFEST=$(occ backup:backup:user ci "$U" | sed -n 's/.* to \(.*\)\.$/\1/p') +[ -n "$MANIFEST" ] || fail "backup:backup:user printed no manifest path" +ok "backed up $U to $MANIFEST" + +# as-backup: restore into -bak; the original account must stay untouched. +occ backup:restore:user ci "$MANIFEST" --mode=as-backup >/dev/null || fail "restore as-backup" +occ files:scan "$U-bak" >/dev/null +SUM_BAK=$($X sh -c "cd $DATA/$U-bak/files && find . -type f -print0 | sort -z | xargs -0 md5sum | md5sum") +[ "$SUM_BEFORE" = "$SUM_BAK" ] || fail "the -bak account's files differ from the original" +occ user:delete "$U-bak" >/dev/null +$X sh -c "[ -d '$DATA/$U/files/Docs' ]" || fail "the original account should be untouched by an as-backup restore" +ok "as-backup restores into -bak, byte-identical, original account untouched" + +# replace: damage the account, then restore it in place under the same uid. +$X sh -c "rm '$DATA/$U/files/Docs/f1.bin' && echo damaged > '$DATA/$U/files/Docs/f1.bin'" +occ files:scan "$U" >/dev/null +occ backup:restore:user ci "$MANIFEST" --mode=replace >/dev/null || fail "restore replace" +occ files:scan "$U" >/dev/null +SUM_AFTER=$($X sh -c "cd $DATA/$U/files && find . -type f -print0 | sort -z | xargs -0 md5sum | md5sum") +[ "$SUM_BEFORE" = "$SUM_AFTER" ] || fail "files differ after a replace restore" +ok "replace restores the original content under the same uid, byte-identical" + +occ user:delete "$U" >/dev/null +$X sh -c "rm -rf /tmp/ngb-urt-target /tmp/ngb-urt-pass" +echo "USER ROUNDTRIP PASS" From f18b4f0862270a7c5350570c504153ba3779c910 Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Mon, 5 Oct 2026 23:11:31 +0200 Subject: [PATCH 02/13] Phase 2: disaster recovery (occ backup:restore:full) New commands, per PLAN.md 3.7: - occ backup:key:import: adopt the master key from a recovery kit on a fresh installation (instead of occ backup:key:init generating a new one), so occ backup:target:add opens the existing repository on the location rather than creating an empty one. - occ backup:restore:full: replace the database (consistent, one transaction), restore the data directory in place, and merge config.php -- refuses up front and lists what's missing when the Nextcloud major version or any app enabled at backup time doesn't match here (their tables must exist before the database restore can use them), rather than guessing or partially applying. Repository::restore() gained an optional $stripPrefix (write "data//..." straight into the real data directory, not a "data/" subdirectory of it) -- restoring "data/" and "config/" this way needs no temporary copy of what can be the largest part of an instance. occ backup:target:add now backfills the local snapshot cache (ngb_snapshots, what occ backup:list actually reads) when it adopts an existing repository: without this, a freshly connected location showed no restore points at all, even though the repository itself had them. One unreadable snapshot is skipped rather than aborting the backfill or the target add itself. Three self-referential problems found and fixed by testing this against two real, separate Nextcloud instances sharing only the backup location (tests/integration/full-restore-roundtrip.sh covers this; the per-process nature of these bugs meant they were invisible from inside a single occ invocation, including the one used to write the fixes, which is exactly why a second, fresh process re-running occ backup:status etc. was necessary to catch them): - The full database dump includes ng_backup's own tables and appconfig rows; restoring them verbatim would overwrite the key/target that is actively performing the restore, on THIS installation, with the backed-up instance's. Both are now preserved: ng_backup's own tables (and, on PostgreSQL, their sequences -- excluding a table does not exclude its sequence) are excluded from what gets restored, and its appconfig rows are captured before the database restore and written back after, in their own transaction. - config.php's 'secret' is merged in from the backup (needed so already-encrypted content in the restored database, e.g. share tokens, stays decryptable), but Target::options, the master key, the wrapped key and the recovery kit code are all encrypted with that same instance secret (OCP's ICrypto, or Nextcloud AppConfig's own "sensitive" flag underneath), not with ng_backup's own key. Changing it without re-encrypting them first left all four permanently undecryptable the moment any later occ command ran. Fixed with two helpers (TargetService::reencryptOptionsAround(), KeyService::reencryptForSecretRotation()) that decrypt before and re-encrypt after whatever callback actually changes the secret; config.php is applied dead last in restoreFull(), after every other repository access. Not crash-safe (a process kill mid-rotation would leave a key missing, not just stale) -- disaster recovery is run once by hand, right after occ maintenance:install, a window too narrow yet to justify a two-phase write. - A subtler variant of the same bug: Nextcloud's own AppConfig additionally encrypts a "sensitive" value itself, tied to the same secret, underneath our own encryption. Overwriting an existing sensitive key makes it read the old value first to compare, which then also fails against the new secret. Deleting the key before setting the new value avoids that read entirely. Roadmap (PLAN.md 9): phase 3 (in-place full restore, user self-service) moved out to v1.3.0; phase 2 continues toward v1.2.0 with FTP/SMB, a verification/checksum-audit command, user-export retention, and deciding whether per-user export joins the automatic schedule. --- .github/workflows/ci.yml | 10 + PLAN.md | 12 ++ appinfo/info.xml | 2 + lib/Command/KeyImport.php | 77 +++++++ lib/Command/RestoreFull.php | 71 ++++++ lib/Repository/Repository.php | 14 +- lib/Service/KeyService.php | 78 +++++++ lib/Service/RestoreFullService.php | 225 ++++++++++++++++++++ lib/Service/RestoreService.php | 12 +- lib/Service/TargetService.php | 61 +++++- psalm-baseline.xml | 16 ++ tests/integration/full-restore-roundtrip.sh | 86 ++++++++ 12 files changed, 657 insertions(+), 7 deletions(-) create mode 100644 lib/Command/KeyImport.php create mode 100644 lib/Command/RestoreFull.php create mode 100644 lib/Service/RestoreFullService.php create mode 100755 tests/integration/full-restore-roundtrip.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3a37530..a216970 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -77,3 +77,13 @@ jobs: docker exec ngbu chown www-data:www-data /var/www/html/custom_apps - name: Per-user backup/restore round trip run: tests/integration/user-roundtrip.sh "docker exec -u www-data ngbu" /var/www/html/data + + integration-full-restore: + # Disaster recovery (occ backup:restore:full) on a SECOND, fresh Nextcloud instance that + # never saw the first one, sharing only the backup location. Does not depend on the database + # beyond what the main integration job already covers, so this runs once, on one NC version. + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - name: Full restore round trip + run: tests/integration/full-restore-roundtrip.sh 35-apache diff --git a/PLAN.md b/PLAN.md index de67187..34876e3 100644 --- a/PLAN.md +++ b/PLAN.md @@ -338,6 +338,18 @@ een backup-app die niet betrouwbaar is, is erger dan geen backup-app. Volgorde: fase 1 grondig testen, reviewen en debuggen; dan nadenken over 0.1.0-alpha.1. Een minimale volledige restore (`occ backup:restore:full` op een verse installatie) komt vóór de beta. +**Roadmap-indeling fase 2 (besluit 2026-10-05):** naar v1.2.0 toewerken met in fase 2: +1. `backup:restore:full` (disaster recovery, verse installatie) — **in uitvoering** +2. FTP/SMB-ondersteuning +3. Verificatie/checksum-audit-commando +4. Retentie/prune voor user-exports +5. Automatische user-export inpluggen in het reguliere schema (of expliciet besluit dat het on-demand blijft) +9. SMB/NFS intern testen op de eigen OMV-instantie + +Doorgeschoven naar **v1.3.0** (fase 3, niet meer fase 2): in-place volledige restore +(eigen restore-modus i.p.v. onderhoudsmodus) en zelfbediening voor gebruikers (eigen bestanden +terugzetten vanuit de Files-app). + **Let op (ontdekt 2026-10-05):** Nextcloud's eigen `info.xsd` staat voor `` alleen `[0-9]+(\.[0-9]+){0,2}` toe — geen `-alpha`/`-beta`/`-rc`-achtervoegsel. De stabiliteitsaanduiding uit deze tabel gaat dus niet in `info.xml` (dat blijft gewoon oplopend, bijv. `0.1.0`), maar in de diff --git a/appinfo/info.xml b/appinfo/info.xml index 348c967..99cc4e4 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -49,6 +49,8 @@ relying on this for anything.]]> OCA\NgBackup\Command\KeySlotRemove OCA\NgBackup\Command\BackupUser OCA\NgBackup\Command\RestoreUser + OCA\NgBackup\Command\KeyImport + OCA\NgBackup\Command\RestoreFull OCA\NgBackup\Settings\Admin diff --git a/lib/Command/KeyImport.php b/lib/Command/KeyImport.php new file mode 100644 index 0000000..6f9dd02 --- /dev/null +++ b/lib/Command/KeyImport.php @@ -0,0 +1,77 @@ +setName('backup:key:import') + ->setDescription('Disaster recovery: adopt the backup key from a recovery kit on a fresh installation') + ->addArgument('kit-file', InputArgument::REQUIRED, 'The recovery kit file (occ backup:key:kit on the original installation)') + ->addOption('passphrase-file', null, InputOption::VALUE_REQUIRED, 'Read the passphrase from this file instead of asking') + ->setHelp(<<<'HELP' +For a fresh installation only (occ backup:key:init must not have been run here yet). Reads one +of the kit's passphrase slots to recover the original master key, so occ backup:target:add opens +the existing repository on the location instead of creating a new, empty one. + +Example: + occ backup:key:import /root/ng_backup-recovery-kit.json + occ backup:target:add offsite sftp -a password::password -o host=... -o user=... -o password=... + occ backup:restore:full offsite +HELP); + } + + protected function execute(InputInterface $input, OutputInterface $output): int { + if ($this->keys->isInitialized()) { + $output->writeln('A backup key already exists here (fingerprint ' . $this->keys->fingerprint() . '). occ backup:key:import is only for a fresh installation.'); + return 1; + } + $kitFile = (string)$input->getArgument('kit-file'); + $raw = @file_get_contents($kitFile); + if ($raw === false) { + $output->writeln("Cannot read $kitFile"); + return 1; + } + try { + $kit = json_decode($raw, true, 512, JSON_THROW_ON_ERROR); + } catch (\JsonException $e) { + $output->writeln('Not a valid recovery kit file: ' . $e->getMessage() . ''); + return 1; + } + $file = $input->getOption('passphrase-file'); + if ($file !== null) { + $pass = rtrim((string)@file_get_contents($file), "\r\n"); + } else { + $helper = $this->getHelper('question'); + $q = (new Question('Passphrase of one of the kit\'s slots: '))->setHidden(true)->setHiddenFallback(false); + $pass = (string)$helper->ask($input, $output, $q); + } + try { + $this->keys->importRecoveryKit($kit, $pass); + } catch (\Throwable $e) { + $output->writeln('' . $e->getMessage() . ''); + return 1; + } + $output->writeln('Backup key imported, fingerprint ' . $this->keys->fingerprint() . '.'); + $output->writeln('Next: connect the backup location with the same details as before (occ backup:target:add), then occ backup:list and occ backup:restore:full.'); + return 0; + } +} diff --git a/lib/Command/RestoreFull.php b/lib/Command/RestoreFull.php new file mode 100644 index 0000000..4487b54 --- /dev/null +++ b/lib/Command/RestoreFull.php @@ -0,0 +1,71 @@ +setName('backup:restore:full') + ->setDescription('Disaster recovery: restore the database, data directory and config on a fresh installation') + ->addArgument('target', InputArgument::REQUIRED, 'Location name or id') + ->addArgument('snapshot', InputArgument::REQUIRED, 'Snapshot id (see backup:list)') + ->setHelp(<<<'HELP' +For a FRESH Nextcloud installation only, same major version as the backup, right after +occ maintenance:install, occ backup:key:import and occ backup:target:add -- before anyone else +uses this instance. + +Every app that was enabled when the snapshot was taken must already be installed here (its +database tables must exist before the restore can write into them); the command lists what is +missing and refuses to continue rather than guess. + +Steps: the database is replaced (consistent, in one transaction); the data directory is restored +directly in place; config.php is merged in -- everything from the backup except the database +connection and data directory settings, which stay as occ maintenance:install configured them +here. Other files under config/ (if any) are not restored. + +Afterwards: check trusted_domains and overwritehost/overwriteprotocol match this server, and +occ maintenance:repair. + +Example: + occ backup:restore:full offsite 537bf7e9c1a4... +HELP); + } + + protected function execute(InputInterface $input, OutputInterface $output): int { + try { + $target = $this->targets->get($input->getArgument('target')); + $snapshot = (string)$input->getArgument('snapshot'); + $result = $this->restoreFull->restoreFull($target, $snapshot); + } catch (\Throwable $e) { + $output->writeln('' . $e->getMessage() . ''); + return 1; + } + $output->writeln(sprintf( + 'Database restored (%d tables, %d unchanged), config merged (%d keys), %d data files restored.', + count($result['dbReport']['restored']), + count($result['dbReport']['skipped']), + $result['configKeys'], + $result['dataFiles'], + )); + $output->writeln('Next: check trusted_domains and overwritehost/overwriteprotocol match this server, then occ maintenance:repair.'); + return 0; + } +} diff --git a/lib/Repository/Repository.php b/lib/Repository/Repository.php index 59d83bf..ad65baf 100644 --- a/lib/Repository/Repository.php +++ b/lib/Repository/Repository.php @@ -190,8 +190,15 @@ final class Repository { return ['snapshot' => $snapshotId] + $stats; } - /** Restore a snapshot (or only paths starting with $prefix) into a local directory. */ - public function restore(string $snapshotId, string $target, string $prefix = ''): int { + /** + * Restore a snapshot (or only paths starting with $prefix) into a local directory. + * + * @param bool $stripPrefix write "$target/" instead of + * "$target/" (disaster recovery: restoring e.g. "data/" or + * "config/" directly into the real data/config directory, not a subdirectory + * named after the prefix). Ignored when $prefix is '' (nothing to strip). + */ + public function restore(string $snapshotId, string $target, string $prefix = '', bool $stripPrefix = false): int { if ($prefix !== '' && !self::isSafePath(rtrim($prefix, '/'))) { throw new RepositoryException('Invalid path prefix'); } @@ -200,7 +207,8 @@ final class Repository { if ($prefix !== '' && !str_starts_with($entry['p'], $prefix)) { continue; } - $dest = rtrim($target, '/') . '/' . $entry['p']; + $relPath = ($stripPrefix && $prefix !== '') ? substr($entry['p'], strlen(rtrim($prefix, '/')) + 1) : $entry['p']; + $dest = rtrim($target, '/') . '/' . $relPath; if (!is_dir(dirname($dest))) { mkdir(dirname($dest), 0700, true); } diff --git a/lib/Service/KeyService.php b/lib/Service/KeyService.php index 06ac581..9538c79 100644 --- a/lib/Service/KeyService.php +++ b/lib/Service/KeyService.php @@ -71,6 +71,84 @@ final class KeyService { $this->cached = $keys; } + /** + * Disaster recovery on a fresh installation: adopt the master key from a recovery kit + * (occ backup:key:kit on the original installation) instead of generating a new one. + * Afterwards occ backup:target:add opens the existing repository on the location with this + * key, instead of creating a new, empty one. + * + * @param array $kit as produced by recoveryKit() + * @throws \OCA\NgBackup\Crypto\CryptoException wrong passphrase, or a damaged/foreign kit + */ + public function importRecoveryKit(array $kit, #[\SensitiveParameter] string $passphrase): void { + if ($this->isInitialized()) { + throw new \RuntimeException('A backup key already exists'); + } + if (!isset($kit['wrapped_key'])) { + throw new \InvalidArgumentException('Not a NG Backup recovery kit file'); + } + $keys = KeyRing::unwrapAny($kit['wrapped_key'], $passphrase); + $this->appConfig->setValueString(Application::APP_ID, self::KEY_MASTER, + $this->crypto->encrypt(base64_encode($keys->exportMasterKey())), true, true); + $this->cached = $keys; + $this->appConfig->setValueString(Application::APP_ID, self::KEY_WRAPPED, json_encode($kit['wrapped_key'], JSON_THROW_ON_ERROR), true, true); + $this->appConfig->setValueString(Application::APP_ID, self::KEY_FINGERPRINT, $this->fingerprintOf($keys), true); + $this->appConfig->setValueInt(Application::APP_ID, self::KEY_DELETE_DELAY, self::DEFAULT_DELETE_DELAY, true); + $this->appConfig->setValueInt(Application::APP_ID, self::KEY_KIT_VERSION, (int)($kit['kit_version'] ?? 1), true); + // Having and successfully using a real kit file is stronger proof than the normal + // download-and-confirm flow it replaces. + $this->confirmRecoveryKit('cli-import'); + } + + /** + * The master key and the recovery kit code are stored encrypted with Nextcloud's own + * instance secret (ICrypto), not with themselves; the wrapped key is not (it is sealed with + * the admin's passphrase instead), but Nextcloud's own AppConfig additionally encrypts any + * "sensitive" value with the instance secret regardless, underneath whatever we store. So + * changing that secret (occ backup:restore:full merging config.php) would otherwise leave + * all three permanently undecryptable. Call $applySecretChange (which must be the only thing + * that changes the secret) through here instead: all three are decrypted first, then + * re-encrypted under whatever secret is active once $applySecretChange returns. + * + * Not crash-safe: a process kill between the delete and the re-save of one of these keys + * would leave it missing rather than merely stale. Disaster recovery is run once, by hand, + * right after occ maintenance:install; a dedicated two-phase write is not justified yet for + * that narrow a window. + */ + public function reencryptForSecretRotation(callable $applySecretChange): mixed { + $master = $this->appConfig->getValueString(Application::APP_ID, self::KEY_MASTER, '', true); + $wrapped = $this->appConfig->getValueString(Application::APP_ID, self::KEY_WRAPPED, '', true); + $kitCodeRaw = $this->appConfig->getValueString(Application::APP_ID, self::KEY_KIT_CODE, '', true); + $kitCode = $kitCodeRaw !== '' ? json_decode($kitCodeRaw, true, 512, JSON_THROW_ON_ERROR) : null; + $masterPlain = $master !== '' ? $this->crypto->decrypt($master) : null; + $kitCodePlain = $kitCode !== null && isset($kitCode['code']) ? $this->crypto->decrypt($kitCode['code']) : null; + + $result = $applySecretChange(); + $this->cached = null; // the master key is about to be re-saved; do not keep the pre-rotation copy + + // Nextcloud's own AppConfig additionally encrypts a "sensitive" value itself (tied to + // the same secret, underneath our own encrypt() above); setValueString() on an existing + // key reads the old value first to compare, which would try to decrypt it with the new + // secret and fail the same way. Delete first so there is nothing to compare against. + if ($masterPlain !== null) { + $this->appConfig->deleteKey(Application::APP_ID, self::KEY_MASTER); + $this->appConfig->setValueString(Application::APP_ID, self::KEY_MASTER, $this->crypto->encrypt($masterPlain), true, true); + } + if ($wrapped !== '') { + // Already plaintext JSON here: getValueString() above transparently undid + // AppConfig's own sensitive-value encryption; there is no encrypt() of our own to + // redo (wrapping is sealed with the passphrase, not the instance secret). + $this->appConfig->deleteKey(Application::APP_ID, self::KEY_WRAPPED); + $this->appConfig->setValueString(Application::APP_ID, self::KEY_WRAPPED, $wrapped, true, true); + } + if ($kitCodePlain !== null) { + $kitCode['code'] = $this->crypto->encrypt($kitCodePlain); + $this->appConfig->deleteKey(Application::APP_ID, self::KEY_KIT_CODE); + $this->appConfig->setValueString(Application::APP_ID, self::KEY_KIT_CODE, json_encode($kitCode, JSON_THROW_ON_ERROR), true, true); + } + return $result; + } + public function keyRing(): KeyRing { if ($this->cached === null) { $enc = $this->appConfig->getValueString(Application::APP_ID, self::KEY_MASTER, '', true); diff --git a/lib/Service/RestoreFullService.php b/lib/Service/RestoreFullService.php new file mode 100644 index 0000000..8fc72b9 --- /dev/null +++ b/lib/Service/RestoreFullService.php @@ -0,0 +1,225 @@ +} + */ + public function checkCompatibility(Target $target, string $snapshotId): array { + $meta = $this->targets->repository($target)->snapshotMeta($snapshotId); + $currentMajor = explode('.', $this->config->getSystemValueString('version'))[0]; + $snapshotMajor = explode('.', (string)($meta['nextcloud'] ?? ''))[0]; + $enabled = array_flip($this->appManager->getEnabledApps()); + $missingApps = array_values(array_diff(array_keys($meta['apps'] ?? []), array_keys($enabled))); + return [ + 'ok' => $currentMajor === $snapshotMajor && $missingApps === [], + 'currentMajor' => $currentMajor, + 'snapshotMajor' => $snapshotMajor, + 'missingApps' => $missingApps, + ]; + } + + /** @return array{dbReport:array, dataFiles:int, configKeys:int} */ + public function restoreFull(Target $target, string $snapshotId): array { + $check = $this->checkCompatibility($target, $snapshotId); + if (!$check['ok']) { + throw new \RuntimeException($this->describeBlockers($check)); + } + return $this->withSharedLock($target, function () use ($target, $snapshotId) { + $repo = $this->targets->repository($target); + $meta = $repo->snapshotMeta($snapshotId); + + $dbReport = null; + if (isset($meta['db'])) { + $manifest = json_decode($repo->getObject($meta['db']), true, 512, JSON_THROW_ON_ERROR); + $prefix = $this->config->getSystemValueString('dbtableprefix', 'oc_'); + // ng_backup's own tables (and its own rows in the shared appconfig table) must + // survive this restore untouched: they describe the backup/key/target that is + // actively performing it right now, on THIS installation, not the one that was + // backed up. Overwriting them mid-restore would pull the rug from under it. + foreach (array_keys($manifest['tables']) as $table) { + /** @var string $table */ + if (str_starts_with($table, $prefix . 'ngb_')) { + unset($manifest['tables'][$table]); + } + } + // Sequences are keyed separately from tables; excluding a table does not exclude + // its sequence, which would otherwise rewind a retained ngb_* table's sequence + // below its current ids (PostgreSQL only -- see DbDumper::restoreSequences()). + foreach (array_keys($manifest['sequences'] ?? []) as $seq) { + /** @var string $seq */ + if (str_starts_with((string)($manifest['sequences'][$seq]['table'] ?? ''), $prefix . 'ngb_')) { + unset($manifest['sequences'][$seq]); + } + } + $ownAppConfig = $this->preserveOwnAppConfig(); + $dumper = new DbDumper($this->db->getInner(), $prefix); + try { + $dumper->restore($repo, $manifest, $dbReport); + } finally { + $this->restoreOwnAppConfig($ownAppConfig); + } + } + + // Read (but do not yet apply) the backed-up config.php before touching the data + // directory: once applied, 'secret' may differ from what ng_backup's own Target + // options were encrypted with (see restoreFull()'s docblock), and every further + // repository access through $this->targets would fail to decrypt them. + $backupConfig = $this->fetchBackupConfig($target, $snapshotId); + + $dataDir = rtrim($this->config->getSystemValueString('datadirectory'), '/'); + $dataFiles = $this->restore->restoreToDirectory($target, $snapshotId, 'data', $dataDir, stripPrefix: true); + + // Last step: nothing after this may need $this->targets->repository($target) again. + if ($backupConfig !== []) { + $this->targets->reencryptOptionsAround( + fn (): mixed => $this->keys->reencryptForSecretRotation(fn (): mixed => $this->config->setSystemValues($backupConfig)), + ); + } + + return ['dbReport' => $dbReport ?? ['skipped' => [], 'restored' => []], 'dataFiles' => $dataFiles, 'configKeys' => count($backupConfig)]; + }); + } + + /** @return array config.php's $CONFIG from the backup, minus the connection keys */ + private function fetchBackupConfig(Target $target, string $snapshotId): array { + $tmp = sys_get_temp_dir() . '/ngb-restore-config-' . bin2hex(random_bytes(6)); + try { + $this->restore->restoreToDirectory($target, $snapshotId, 'config', $tmp, stripPrefix: true); + $path = $tmp . '/config.php'; + if (!is_file($path)) { + return []; // nothing to merge; the live config.php stays as occ maintenance:install made it + } + $backupConfig = (static function () use ($path): array { + $CONFIG = []; + include $path; + return $CONFIG; + })(); + foreach (self::CONNECTION_KEYS as $key) { + unset($backupConfig[$key]); + } + return $backupConfig; + } finally { + $this->rmrf($tmp); + } + } + + /** @return list> raw appconfig rows (appid, configkey, configvalue, type, lazy) */ + private function preserveOwnAppConfig(): array { + $qb = $this->db->getQueryBuilder(); + $qb->select('*')->from('appconfig') + ->where($qb->expr()->eq('appid', $qb->createNamedParameter(\OCA\NgBackup\AppInfo\Application::APP_ID))); + /** @var list> */ + return $qb->executeQuery()->fetchAll(); + } + + /** + * @param list> $rows raw appconfig rows, as returned by preserveOwnAppConfig() + * + * Runs in its own transaction (DbDumper::restore() already committed its own by the time + * this is called): not a full guarantee against a crash spanning both, but means this step + * itself cannot leave ng_backup's appconfig rows half deleted, half reinserted. + */ + private function restoreOwnAppConfig(array $rows): void { + $this->db->beginTransaction(); + try { + $qb = $this->db->getQueryBuilder(); + $qb->delete('appconfig') + ->where($qb->expr()->eq('appid', $qb->createNamedParameter(\OCA\NgBackup\AppInfo\Application::APP_ID))); + $qb->executeStatement(); + foreach ($rows as $row) { + $ins = $this->db->getQueryBuilder(); + $ins->insert('appconfig')->values([ + 'appid' => $ins->createNamedParameter($row['appid']), + 'configkey' => $ins->createNamedParameter($row['configkey']), + 'configvalue' => $ins->createNamedParameter($row['configvalue']), + 'type' => $ins->createNamedParameter($row['type'], \OCP\DB\QueryBuilder\IQueryBuilder::PARAM_INT), + 'lazy' => $ins->createNamedParameter($row['lazy'], \OCP\DB\QueryBuilder\IQueryBuilder::PARAM_INT), + ]); + $ins->executeStatement(); + } + $this->db->commit(); + } catch (\Throwable $e) { + $this->db->rollBack(); + throw $e; + } + } + + private function rmrf(string $dir): void { + if (!is_dir($dir)) { + return; + } + foreach (new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator($dir, \FilesystemIterator::SKIP_DOTS), \RecursiveIteratorIterator::CHILD_FIRST) as $f) { + $f->isDir() ? rmdir($f->getPathname()) : unlink($f->getPathname()); + } + rmdir($dir); + } + + private function describeBlockers(array $check): string { + $parts = []; + if ($check['currentMajor'] !== $check['snapshotMajor']) { + $parts[] = "this installation is Nextcloud {$check['currentMajor']}, the backup is from {$check['snapshotMajor']} (same major version required)"; + } + if ($check['missingApps'] !== []) { + $parts[] = 'install these apps first (their tables must exist before the database restore can use them): ' + . implode(', ', array_map(fn (string $a) => "occ app:install $a", $check['missingApps'])); + } + return implode('; ', $parts); + } + + private function withSharedLock(Target $target, callable $fn): mixed { + try { + // No progress from DbDumper::restore()/Repository::restore() to refresh this lease + // against; a very large instance could in theory outrun this TTL (same limitation as + // RestoreService::restoreToDirectory(), see also UserBackupService). + return $this->leases->with(PruneService::lockKey($target), LeaseService::SHARED, 7200, $fn); + } catch (LockedException) { + throw new \RuntimeException('A cleanup of ' . $target->getName() . ' is in progress; try again later'); + } + } +} diff --git a/lib/Service/RestoreService.php b/lib/Service/RestoreService.php index c68c677..d45d7f4 100644 --- a/lib/Service/RestoreService.php +++ b/lib/Service/RestoreService.php @@ -149,15 +149,21 @@ final class RestoreService { return ['restored' => $restored, 'trashed' => $trashed, 'target' => $uid . ':' . $userFolder->getRelativePath($dest->getPath())]; } - /** Raw restore of any snapshot path to a local directory (administrators, disaster recovery). */ - public function restoreToDirectory(Target $target, string $snapshotId, string $prefix, string $directory): int { + /** + * Raw restore of any snapshot path to a local directory (administrators, disaster recovery). + * + * @param bool $stripPrefix write into $directory itself instead of $directory/$prefix (see + * Repository::restore()); used to restore "data/" or "config/" straight into the + * real data/config directory for occ backup:restore:full. + */ + public function restoreToDirectory(Target $target, string $snapshotId, string $prefix, string $directory, bool $stripPrefix = false): int { if ($prefix !== '' && !Repository::isSafePath($prefix)) { throw new \InvalidArgumentException('Invalid path: ' . $prefix); } if (!is_dir($directory) && !mkdir($directory, 0750, true) && !is_dir($directory)) { throw new \RuntimeException("Cannot create $directory"); } - return $this->withSharedLock($target, fn () => $this->targets->repository($target)->restore($snapshotId, $directory, trim($prefix, '/') === '' ? '' : trim($prefix, '/') . '/'), 3600); + return $this->withSharedLock($target, fn () => $this->targets->repository($target)->restore($snapshotId, $directory, trim($prefix, '/') === '' ? '' : trim($prefix, '/') . '/', $stripPrefix), 3600); } private function writeFile(Repository $repo, Folder $dest, string $relPath, array $entry): void { diff --git a/lib/Service/TargetService.php b/lib/Service/TargetService.php index 67a81a9..3878feb 100644 --- a/lib/Service/TargetService.php +++ b/lib/Service/TargetService.php @@ -10,10 +10,13 @@ namespace OCA\NgBackup\Service; use OCA\NgBackup\Backend\ExternalStorageFactory; use OCA\NgBackup\Backend\IBackend; use OCA\NgBackup\Backend\StorageBackend; +use OCA\NgBackup\Db\Snapshot; +use OCA\NgBackup\Db\SnapshotMapper; use OCA\NgBackup\Db\Target; use OCA\NgBackup\Db\TargetMapper; use OCA\NgBackup\Repository\Repository; use OCP\App\IAppManager; +use OCP\AppFramework\Db\DoesNotExistException; use OCP\Security\ICrypto; use OCP\Server; @@ -30,6 +33,7 @@ final class TargetService { private IAppManager $appManager, private \OCA\NgBackup\Db\DbIndexCache $indexCache, private \OCA\NgBackup\Db\AppConfigCatalogAnchor $anchor, + private SnapshotMapper $snapshots, ) { } @@ -78,7 +82,62 @@ final class TargetService { $target->setBasePath(trim($basePath, '/')); $target->setRepositoryId($repo->id()); $target->setCreatedAt(time()); - return ['target' => $this->mapper->insert($target), 'created' => $created]; + $target = $this->mapper->insert($target); + if (!$created) { + // Disaster recovery: occ backup:list reads this local cache, not the repository + // directly, so a snapshot made by another installation must be backfilled here too. + $this->adoptSnapshots($target, $repo); + } + return ['target' => $target, 'created' => $created]; + } + + /** + * A target's options are encrypted with Nextcloud's own instance secret (ICrypto), not + * ng_backup's key, so changing that secret (occ backup:restore:full merging config.php) + * would otherwise leave every target permanently undecryptable. Call $applySecretChange + * (which must be the only thing that changes the secret) through here instead: every + * target's options are decrypted first, then re-encrypted under whatever secret is active + * once $applySecretChange returns. + */ + public function reencryptOptionsAround(callable $applySecretChange): mixed { + $plain = []; + foreach ($this->mapper->findAll() as $target) { + $plain[$target->getId()] = $this->options($target); + } + $result = $applySecretChange(); + foreach ($this->mapper->findAll() as $target) { + if (!isset($plain[$target->getId()])) { + continue; // added by $applySecretChange itself; nothing to re-encrypt + } + $target->setOptions($this->crypto->encrypt(json_encode($plain[$target->getId()], JSON_THROW_ON_ERROR))); + $this->mapper->update($target); + } + return $result; + } + + private function adoptSnapshots(Target $target, Repository $repo): void { + foreach ($repo->listSnapshots() as $snapshotId) { + try { + $this->snapshots->findOne($target->getId(), $snapshotId); + continue; // already known locally + } catch (DoesNotExistException) { + } + // One unreadable/corrupt snapshot must not stop the rest of the location's + // snapshots from being backfilled, nor abort adding the target itself. + try { + $meta = $repo->snapshotMeta($snapshotId); + $s = new Snapshot(); + $s->setTargetId($target->getId()); + $s->setSnapshotId($snapshotId); + $s->setKind((string)($meta['kind'] ?? 'full')); + $s->setLabel(($meta['label'] ?? '') !== '' ? $meta['label'] : null); + $s->setCreatedAt((int)strtotime((string)($meta['time'] ?? 'now'))); + $s->setFiles((int)($meta['stats']['files'] ?? 0)); + $s->setBytes((int)($meta['stats']['bytes'] ?? 0)); + $this->snapshots->insert($s); + } catch (\Throwable) { + } + } } /** @return list */ diff --git a/psalm-baseline.xml b/psalm-baseline.xml index 98da393..d874a89 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -49,6 +49,11 @@ + + + + + @@ -455,6 +460,17 @@ >, forgotten: list, purged: list, prune: array}]]> + + + + + + + + + + + diff --git a/tests/integration/full-restore-roundtrip.sh b/tests/integration/full-restore-roundtrip.sh new file mode 100755 index 0000000..c0d270c --- /dev/null +++ b/tests/integration/full-restore-roundtrip.sh @@ -0,0 +1,86 @@ +#!/bin/bash +# Disaster recovery: occ backup:key:import + backup:target:add + backup:restore:full on a +# SECOND, fresh Nextcloud instance that never saw the first one, sharing only the backup +# location (a local directory, bind-mounted into both containers) -- see PLAN.md 3.7. +# Usage: full-restore-roundtrip.sh +# e.g. full-restore-roundtrip.sh 35-apache +set -euo pipefail +IMAGE="nextcloud:${1:-35-apache}" +APP=$(cd "$(dirname "$0")/../.." && pwd) +NET=ngb-frt-net +SHARED=$(mktemp -d) +chmod 777 "$SHARED" +fail() { echo "FAIL: $*"; exit 1; } +ok() { echo "ok $*"; } + +cleanup() { + docker rm -fv ngb-frt-origin ngb-frt-restore >/dev/null 2>&1 || true + docker network rm "$NET" >/dev/null 2>&1 || true + docker run --rm -v "$SHARED:/target" alpine sh -c 'rm -rf /target/*' >/dev/null 2>&1 || true + rmdir "$SHARED" 2>/dev/null || true + rm -f /tmp/ngb-frt-kit.json /tmp/ngb-frt-pass +} +trap cleanup EXIT + +run_on() { docker exec -u www-data "$1" php occ "${@:2}"; } + +docker network create "$NET" >/dev/null +docker run -d --name ngb-frt-origin --network "$NET" -e SQLITE_DATABASE=nextcloud \ + -e NEXTCLOUD_ADMIN_USER=admin -e NEXTCLOUD_ADMIN_PASSWORD=admin \ + -v "$APP:/var/www/html/custom_apps/ng_backup:ro" -v "$SHARED:/mnt/shared" "$IMAGE" >/dev/null +for i in $(seq 1 90); do run_on ngb-frt-origin status 2>/dev/null | grep -q "installed: true" && break; sleep 3; done +docker exec ngb-frt-origin chown www-data:www-data /var/www/html/custom_apps +run_on ngb-frt-origin app:enable ng_backup >/dev/null +ok "origin instance up" + +docker exec ngb-frt-origin sh -c "printf 'full restore roundtrip passphrase 2026\n' > /tmp/pass" +run_on ngb-frt-origin backup:key:init --passphrase-file=/tmp/pass --label=CI >/dev/null || fail "key init" +run_on ngb-frt-origin backup:key:kit --output=/tmp/kit.json >/dev/null +CODE=$(run_on ngb-frt-origin backup:key:kit | php -r 'echo json_decode(stream_get_contents(STDIN), true)["confirmation_code"];') +run_on ngb-frt-origin backup:key:confirm -u admin --code="$CODE" --confirm="Yes, I confirm" >/dev/null || fail "confirm" +run_on ngb-frt-origin backup:target:add offsite local -a null::null -o datadir=/mnt/shared --path=repo >/dev/null || fail "target add" +ok "key, kit and location on the origin" + +NC_PASS=$(openssl rand -hex 16) +docker exec -u www-data -e OC_PASS="$NC_PASS" ngb-frt-origin php occ user:add --password-from-env frtest >/dev/null +docker exec ngb-frt-origin sh -c "mkdir -p /var/www/html/data/frtest/files/Docs && head -c 200000 /dev/urandom > /var/www/html/data/frtest/files/Docs/report.bin && echo 'hello disaster recovery' > /var/www/html/data/frtest/files/Docs/note.txt && chown -R www-data:www-data /var/www/html/data/frtest" +run_on ngb-frt-origin files:scan frtest >/dev/null +RUN_OUT=$(run_on ngb-frt-origin backup:run offsite -l disaster-test) +SNAP=$(printf '%s\n' "$RUN_OUT" | head -1 | awk -F'snapshot ' '{print $2}' | tr -d ',') +[ -n "$SNAP" ] || fail "no snapshot id from backup:run" +ok "test data backed up, snapshot $SNAP" + +docker cp ngb-frt-origin:/tmp/kit.json /tmp/ngb-frt-kit.json +docker exec ngb-frt-origin sh -c "cat /tmp/pass" > /tmp/ngb-frt-pass +docker run -d --name ngb-frt-restore --network "$NET" -e SQLITE_DATABASE=nextcloud \ + -e NEXTCLOUD_ADMIN_USER=admin -e NEXTCLOUD_ADMIN_PASSWORD=admin \ + -v "$APP:/var/www/html/custom_apps/ng_backup:ro" -v "$SHARED:/mnt/shared" "$IMAGE" >/dev/null +for i in $(seq 1 90); do run_on ngb-frt-restore status 2>/dev/null | grep -q "installed: true" && break; sleep 3; done +docker exec ngb-frt-restore chown www-data:www-data /var/www/html/custom_apps +run_on ngb-frt-restore app:enable ng_backup >/dev/null +docker cp /tmp/ngb-frt-kit.json ngb-frt-restore:/tmp/kit.json +docker exec ngb-frt-restore chmod 644 /tmp/kit.json +docker cp /tmp/ngb-frt-pass ngb-frt-restore:/tmp/pass +ok "fresh restore instance up" + +run_on ngb-frt-restore backup:key:import /tmp/kit.json --passphrase-file=/tmp/pass >/dev/null || fail "key import" +run_on ngb-frt-restore backup:target:add offsite local -a null::null -o datadir=/mnt/shared --path=repo >/dev/null || fail "target add on the restore instance" +run_on ngb-frt-restore backup:list offsite | grep -q "$SNAP" || fail "the origin's snapshot is not listed after connecting the location" +ok "recovery kit imported, existing repository adopted, snapshot visible" + +run_on ngb-frt-restore backup:restore:full offsite "$SNAP" >/dev/null || fail "restore:full" +ok "backup:restore:full completed" + +# Everything below must work in a FRESH occ process (a new PHP process per docker exec), since +# earlier bugs here only showed up after config.php's secret was rewritten mid-restore. +run_on ngb-frt-restore backup:status >/dev/null || fail "backup:status fails after restore (own key/target undecryptable?)" +run_on ngb-frt-restore user:list | grep -q frtest || fail "the backed-up user is missing from the restored database" +docker exec ngb-frt-restore test -f /var/www/html/data/frtest/files/Docs/note.txt || fail "restored file is missing" +[ "$(docker exec ngb-frt-restore cat /var/www/html/data/frtest/files/Docs/note.txt)" = "hello disaster recovery" ] || fail "restored file content differs" +run_on ngb-frt-restore files:scan frtest >/dev/null || fail "files:scan fails after restore" +ok "database, files and ng_backup's own state are all intact and usable after the restore" + +run_on ngb-frt-restore backup:run offsite -l post-restore-check >/dev/null || fail "a new backup after the restore fails" +ok "the restored instance can run a new backup of its own" + +echo "FULL RESTORE ROUNDTRIP PASS" From 935c7b6a0873ec63501dc3a5cfbd711d24148a01 Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Mon, 5 Oct 2026 23:46:50 +0200 Subject: [PATCH 03/13] Phase 2: checksum audit (occ backup:verify) Repository::verify(): every blob a snapshot's files and database dump reference must be in a freshly re-downloaded, re-authenticated index (never trusted from the local cache, same reasoning as prune()) and have its pack still present on the location -- shallow, cheap, no blob content read, catches a deleted pack, a tampered/rolled-back index, or a database manifest that went missing (reported as a failure, not silently skipped). With $deep, each blob is also downloaded and decrypted, same as loadBlob() already does on every real read (AEAD authentication tag, content hash against its blob id), done here deliberately for everything in the snapshot rather than waiting for it to come up during an actual restore. A blob referenced by more than one file is only checked once; pack presence is cached per pack, not re-checked per blob. occ backup:verify [--deep], meant to run periodically (cron, a sample of recent snapshots), per PLAN.md's "Verificatie: periodiek checksums controleren, steekproef-restore". The lease covering the audit is refreshed roughly every half of its TTL while blobs are checked, rather than relying on a single long-lived lease like the other new phase-2 commands do. Verified against a real S3-backed 1.4 GiB / 603-blob snapshot (shallow: instant; --deep: ~28s, fully decrypted) in addition to the new unit tests and the roundtrip.sh addition (shallow and deep on a snapshot with real, varied file sizes, now covered on every CI matrix combination). Local review (Codex) on the first pass found three real issues, all fixed: a missing database manifest was silently skipped instead of reported; shallow verification trusted the (possibly stale or unauthenticated) local index cache instead of re-checking the authenticated index on the location; and the lease had no refresh, so a --deep audit of a very large snapshot could outrun its TTL and run unprotected against a concurrent prune. --- appinfo/info.xml | 1 + lib/Command/Verify.php | 71 +++++++++++++++++++++ lib/Repository/Repository.php | 77 ++++++++++++++++++++++ lib/Service/VerifyService.php | 39 ++++++++++++ tests/Unit/Repository/VerifyTest.php | 95 ++++++++++++++++++++++++++++ tests/integration/roundtrip.sh | 5 ++ 6 files changed, 288 insertions(+) create mode 100644 lib/Command/Verify.php create mode 100644 lib/Service/VerifyService.php create mode 100644 tests/Unit/Repository/VerifyTest.php diff --git a/appinfo/info.xml b/appinfo/info.xml index 99cc4e4..90b064f 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -51,6 +51,7 @@ relying on this for anything.]]> OCA\NgBackup\Command\RestoreUser OCA\NgBackup\Command\KeyImport OCA\NgBackup\Command\RestoreFull + OCA\NgBackup\Command\Verify OCA\NgBackup\Settings\Admin diff --git a/lib/Command/Verify.php b/lib/Command/Verify.php new file mode 100644 index 0000000..bae03fb --- /dev/null +++ b/lib/Command/Verify.php @@ -0,0 +1,71 @@ +setName('backup:verify') + ->setDescription('Checksum audit of a snapshot: every blob it needs must be in the (freshly re-checked) index and its pack present') + ->addArgument('target', InputArgument::REQUIRED, 'Location name or id') + ->addArgument('snapshot', InputArgument::REQUIRED, 'Snapshot id (see backup:list)') + ->addOption('deep', null, InputOption::VALUE_NONE, 'Also download and decrypt every blob (verifies its authentication tag and content hash, not just that it is listed); much slower') + ->setHelp(<<<'HELP' +Without --deep this re-downloads and re-authenticates the index (never trusting the local cache) +and confirms every blob the snapshot needs is listed and its pack is present -- fast, no blob +content is read, so it cannot by itself catch a truncated or bit-rotted pack. With --deep every +blob is also downloaded and decrypted, which fails loudly on exactly that even if the index still +lists it -- the same verification an actual restore would do, run here deliberately instead of +waiting to find out during one. + +Run this periodically (cron, picking a sample of recent snapshots) rather than only right before +you'd need to rely on a restore. + +Examples: + occ backup:verify offsite 537bf7e9c1a4... + occ backup:verify offsite 537bf7e9c1a4... --deep +HELP); + } + + protected function execute(InputInterface $input, OutputInterface $output): int { + try { + $target = $this->targets->get($input->getArgument('target')); + $result = $this->verify->verify($target, (string)$input->getArgument('snapshot'), (bool)$input->getOption('deep')); + } catch (\Throwable $e) { + $output->writeln('' . $e->getMessage() . ''); + return 1; + } + $output->writeln(sprintf('%d files, %d blobs checked%s.', $result['filesChecked'], $result['blobsChecked'], + $input->getOption('deep') ? sprintf(' (%.1f MiB decrypted)', $result['bytesChecked'] / 1048576) : '')); + if ($result['missing'] !== []) { + $output->writeln('' . count($result['missing']) . ' blob(s) missing from the index: ' . implode(', ', $result['missing'])); + } + if ($result['failed'] !== []) { + $output->writeln('' . count($result['failed']) . ' blob(s) failed verification (corrupted or tampered): ' . implode(', ', $result['failed'])); + } + if ($result['missing'] === [] && $result['failed'] === []) { + $output->writeln('OK'); + return 0; + } + return 1; + } +} diff --git a/lib/Repository/Repository.php b/lib/Repository/Repository.php index ad65baf..2d83758 100644 --- a/lib/Repository/Repository.php +++ b/lib/Repository/Repository.php @@ -662,6 +662,83 @@ final class Repository { return $this->streamEntries($snapshotId); } + /** + * Checksum audit of a snapshot: every blob it references (files and the database dump, when + * the snapshot has one) must be in the index, freshly re-downloaded and re-authenticated from + * the location rather than trusted from the local cache (same reasoning as prune()), and its + * pack must still be present. With $deep, each blob is also downloaded and decrypted, which + * verifies its AEAD authentication tag and its content hash against the blob id (loadBlob() + * already does both on every real read -- this just does it for everything in the snapshot, + * deliberately, rather than relying on it to come up during a later restore). A blob + * referenced by more than one file is only checked once; a missing database manifest is a + * failure, not something to silently skip over. + * + * @param ?callable $heartbeat called after every blob (shallow) or every blob read (deep); + * callers use it to keep a time-boxed lease alive during a large audit + * @return array{filesChecked:int, blobsChecked:int, bytesChecked:int, missing:list, failed:list} + */ + public function verify(string $snapshotId, bool $deep = false, ?callable $heartbeat = null): array { + $heartbeat ??= static function (): void { + }; + $this->index->load(true); + $stats = ['filesChecked' => 0, 'blobsChecked' => 0, 'bytesChecked' => 0, 'missing' => [], 'failed' => []]; + $seen = []; + $packSeen = []; // pack id => exists on the backend; avoids one exists() call per blob + foreach ($this->streamEntries($snapshotId) as $entry) { + $stats['filesChecked']++; + foreach ($entry['b'] as $id) { + if (isset($seen[$id])) { + continue; + } + $seen[$id] = true; + $this->verifyBlob($id, $deep, $stats, $packSeen); + $heartbeat(); + } + } + $meta = $this->snapshotMeta($snapshotId); + if (isset($meta['db']) && is_string($meta['db'])) { + if (!$this->backend->exists($meta['db'])) { + $stats['missing'][] = $meta['db']; + } else { + $dbManifest = json_decode($this->getObject($meta['db']), true, 512, JSON_THROW_ON_ERROR); + foreach ($dbManifest['tables'] as $info) { + foreach ($info['blobs'] as $id) { + if (isset($seen[$id])) { + continue; + } + $seen[$id] = true; + $this->verifyBlob($id, $deep, $stats, $packSeen); + $heartbeat(); + } + } + } + } + return $stats; + } + + /** @param array $packSeen */ + private function verifyBlob(string $id, bool $deep, array &$stats, array &$packSeen): void { + if (!$this->index->has($id)) { + $stats['missing'][] = $id; + return; + } + [$pack] = $this->index->get($id); + $packExists = $packSeen[$pack] ??= $this->backend->exists('packs/' . substr($pack, 0, 2) . '/' . $pack); + if (!$packExists) { + $stats['missing'][] = $id; + return; + } + $stats['blobsChecked']++; + if (!$deep) { + return; + } + try { + $stats['bytesChecked'] += strlen($this->loadBlob($id)); + } catch (\Throwable) { + $stats['failed'][] = $id; + } + } + /** The meta record (first line) of a snapshot. */ public function snapshotMeta(string $snapshotId): array { $plain = fopen('php://temp/maxmemory:' . (4 * 1048576), 'w+b'); diff --git a/lib/Service/VerifyService.php b/lib/Service/VerifyService.php new file mode 100644 index 0000000..ef3d893 --- /dev/null +++ b/lib/Service/VerifyService.php @@ -0,0 +1,39 @@ +, failed:list} */ + public function verify(Target $target, string $snapshotId, bool $deep = false): array { + $ttl = $deep ? 600 : 300; + try { + return $this->leases->with(PruneService::lockKey($target), LeaseService::SHARED, $ttl, function (callable $refresh) use ($target, $snapshotId, $deep, $ttl) { + $last = time(); + $heartbeat = function () use ($refresh, $ttl, &$last): void { + if (time() - $last >= intdiv($ttl, 2)) { + $refresh(); + $last = time(); + } + }; + return $this->targets->repository($target)->verify($snapshotId, $deep, $heartbeat); + }); + } catch (LockedException) { + throw new \RuntimeException('A cleanup of ' . $target->getName() . ' is in progress; try again later'); + } + } +} diff --git a/tests/Unit/Repository/VerifyTest.php b/tests/Unit/Repository/VerifyTest.php new file mode 100644 index 0000000..7b9ed77 --- /dev/null +++ b/tests/Unit/Repository/VerifyTest.php @@ -0,0 +1,95 @@ +root = $this->tempDir(); + $this->backend = new LocalBackend($this->root); + $this->keys = KeyRing::generate(); + $this->src = $this->tempDir(); + file_put_contents("$this->src/a", random_bytes(100000)); + file_put_contents("$this->src/b", random_bytes(100000)); + } + + private function open(): Repository { + return Repository::openWithKey($this->backend, $this->keys, null, null); + } + + public function testShallowVerifyPassesOnAHealthyRepository(): void { + $repo = Repository::initWithKey($this->backend, $this->keys, $this->keys->wrap('pw')); + $s = $repo->backupDirectory($this->src); + + $result = $this->open()->verify($s['snapshot']); + + $this->assertSame(2, $result['filesChecked']); + $this->assertGreaterThanOrEqual(1, $result['blobsChecked']); + $this->assertSame(0, $result['bytesChecked']); // not downloaded without --deep + $this->assertSame([], $result['missing']); + $this->assertSame([], $result['failed']); + } + + public function testDeepVerifyDownloadsAndCountsBytes(): void { + $repo = Repository::initWithKey($this->backend, $this->keys, $this->keys->wrap('pw')); + $s = $repo->backupDirectory($this->src); + + $result = $this->open()->verify($s['snapshot'], deep: true); + + $this->assertSame(200000, $result['bytesChecked']); + $this->assertSame([], $result['missing']); + $this->assertSame([], $result['failed']); + } + + public function testShallowVerifyDetectsAMissingPack(): void { + $repo = Repository::initWithKey($this->backend, $this->keys, $this->keys->wrap('pw')); + $s = $repo->backupDirectory($this->src); + foreach ($this->backend->list('packs') as $pack) { + $this->backend->delete($pack); + } + + $result = $this->open()->verify($s['snapshot']); + + $this->assertNotSame([], $result['missing']); + } + + public function testVerifyFailsOnAMissingDatabaseManifest(): void { + $repo = Repository::initWithKey($this->backend, $this->keys, $this->keys->wrap('pw')); + $repo->writeSnapshot('snap-no-db', ['time' => gmdate('c'), 'db' => 'db/does-not-exist'], []); + + $result = $this->open()->verify('snap-no-db'); + + $this->assertSame(['db/does-not-exist'], $result['missing']); + } + + public function testDeepVerifyDetectsATamperedPack(): void { + $repo = Repository::initWithKey($this->backend, $this->keys, $this->keys->wrap('pw')); + $s = $repo->backupDirectory($this->src); + $packs = $this->backend->list('packs'); + $this->assertNotEmpty($packs); + $path = $this->root . '/' . $packs[0]; + $bytes = file_get_contents($path); + // Flip a byte well inside the file, away from any header, so decryption's own + // authentication tag catches it (this must fail loudly, not just "do nothing"). + $bytes[(int)(strlen($bytes) / 2)] = chr(~ord($bytes[(int)(strlen($bytes) / 2)]) & 0xFF); + file_put_contents($path, $bytes); + + $result = $this->open()->verify($s['snapshot'], deep: true); + + $this->assertNotSame([], $result['failed']); + } +} diff --git a/tests/integration/roundtrip.sh b/tests/integration/roundtrip.sh index b94ce2a..3b76b80 100755 --- a/tests/integration/roundtrip.sh +++ b/tests/integration/roundtrip.sh @@ -49,6 +49,11 @@ SUM_AFTER=$($X sh -c "cd $DATA/$U/files && find . -type f -print0 | sort -z | xa $X sh -c "ls $DATA/$U/files_trashbin/files/ | grep -q '^extra.txt'" || fail "extra file not in trash" ok "restore new-folder and replace: files identical, extra file in the trash" +# Checksum audit: shallow (index presence) and deep (download, decrypt, verify every blob). +occ backup:verify ci "$SNAP" | grep -q "^OK$" || fail "shallow verify" +occ backup:verify ci "$SNAP" --deep | grep -q "^OK$" || fail "deep verify" +ok "shallow and deep checksum audit" + # Database dump/restore round trip (all tables, checksums, sequences on PostgreSQL). $X php -d memory_limit=512M "$APPS/ng_backup/spikes/03-db-dump.php" /tmp/ngb-rt-db | grep "ALL PASS" >/dev/null || fail "database round trip" ok "database dump and restore" From 932f60bc5e93439f5a2600ed728566ba2cdb3de7 Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Tue, 6 Oct 2026 00:18:51 +0200 Subject: [PATCH 04/13] Phase 2: FTP, SMB and NFS storage backends No ng_backup code changes needed: files_external's generic backend/auth adapter (the same ExternalStorageFactory/StorageBackend already used for S3/SFTP) already covers FTP and SMB, and NFS was never a files_external backend to begin with (PLAN.md 3.5) -- the "local" backend plus an admin-managed OS-level mount already does the job. This commit is test coverage and documentation. tests/integration/targets-matrix.sh now also runs the full round trip against: - FTP (stilliard/pure-ftpd). A first attempt with delfer/alpine-ftp-server (vsftpd) failed with a generic "create directory operation failed": files_external's FTP backend always builds absolute paths, which that server's chroot rejects even via plain php ftp_mkdir() with an absolute path (a relative path on the same server works fine) -- not a files_external or ng_backup bug, just a server-specific quirk, confirmed by switching test servers. - SMB (dperson/samba), via a small Nextcloud image built on the fly with the smbclient PECL extension (libsmbclient-dev + pecl install), in its own temp directory as the build context (never /tmp directly). The backend *lists* as available with just the smbclient CLI binary too (files_external's shell-wrapped fallback), but that fallback's stream cannot seek, and ng_backup reads blobs back by byte range while backing up -- confirmed live, it fails the very first (small) backup with "Seek failed in packs/...". The native extension is genuinely required here, not just preferred. README: new "Storage backend experiences" entries for both findings (and a pointer from the "no extra PHP extensions" intro line, since SMB is the one real exception), plus NFS guidance (mount it yourself, point the local backend at the mount point). PLAN.md: FTP/SMB marked done; SMB/NFS testing on the user's own OMV instance stays open (internal, not something CI can cover). Local review (Codex + CodeRabbit) found three issues, all fixed: the README's first-pass SMB claim ("CLI tool is not enough") was imprecise about *why* -- it lists fine without the extension, it just doesn't work for ng_backup's read pattern; the cleanup trap could abort before removing the network if an earlier docker rm failed, and didn't remove the built image/Dockerfile; and the Docker build used all of /tmp as its context instead of a dedicated directory. --- PLAN.md | 12 ++++-- README.md | 29 +++++++++++++- tests/integration/targets-matrix.sh | 59 +++++++++++++++++++++++++---- 3 files changed, 86 insertions(+), 14 deletions(-) diff --git a/PLAN.md b/PLAN.md index 34876e3..53ff3a6 100644 --- a/PLAN.md +++ b/PLAN.md @@ -339,12 +339,16 @@ Volgorde: fase 1 grondig testen, reviewen en debuggen; dan nadenken over 0.1.0-a minimale volledige restore (`occ backup:restore:full` op een verse installatie) komt vóór de beta. **Roadmap-indeling fase 2 (besluit 2026-10-05):** naar v1.2.0 toewerken met in fase 2: -1. `backup:restore:full` (disaster recovery, verse installatie) — **in uitvoering** -2. FTP/SMB-ondersteuning -3. Verificatie/checksum-audit-commando +1. `backup:restore:full` (disaster recovery, verse installatie) — **klaar** (2026-10-05) +2. FTP/SMB-ondersteuning — **klaar** (2026-10-06): beide liepen al via files_external's generieke + adapter zonder enige ng_backup-code; alleen testdekking toegevoegd + (`tests/integration/targets-matrix.sh`) + bevindingen in README ("Storage backend experiences"). + SMB vereist de `smbclient` PECL-extensie (niet standaard in de Nextcloud-image, in CI nu + on-the-fly gebouwd). NFS is geen eigen backend: lokaal mounten + `local`-backend erop richten. +3. Verificatie/checksum-audit-commando — **klaar** (2026-10-05) 4. Retentie/prune voor user-exports 5. Automatische user-export inpluggen in het reguliere schema (of expliciet besluit dat het on-demand blijft) -9. SMB/NFS intern testen op de eigen OMV-instantie +9. SMB/NFS intern testen op de eigen OMV-instantie — nog open (intern, niet in CI) Doorgeschoven naar **v1.3.0** (fase 3, niet meer fase 2): in-place volledige restore (eigen restore-modus i.p.v. onderhoudsmodus) en zelfbediening voor gebruikers (eigen bestanden diff --git a/README.md b/README.md index 965d49a..4455b8a 100644 --- a/README.md +++ b/README.md @@ -2,8 +2,10 @@ Encrypted, incremental backups of Nextcloud (database, configuration and data) with restore from the web interface. Works without shell access: no external programs, no extra PHP -extensions beyond sodium. Runs on Nextcloud All-in-One, regular installations and hosted -Nextcloud. +extensions beyond sodium for ng_backup itself. Runs on Nextcloud All-in-One, regular +installations and hosted Nextcloud. (Choosing SMB as a storage backend is the one exception: +see "Storage backend experiences" below — that requirement comes from files_external, not from +ng_backup, and applies to any Nextcloud app that writes to an SMB share.) **Status: in development (phase 1). Not ready for production use.** See `PLAN.md` for the design. @@ -119,6 +121,29 @@ Contributions welcome via PR — add what you ran into with your own provider. verified whether this also applies to a Hetzner Storage Box **main** account (only tested with a sub-account so far) — if you've tried a main account, please say so in a PR. +- **FTP, server-dependent absolute-path handling:** files_external's FTP backend always builds + absolute paths (e.g. `MKD /repo`) for the configured root, never relative ones. `atmoz/sftp`-style + `vsftpd` test servers (chrooted) can reject that with a generic "create directory operation + failed", even though a plain `ftp_mkdir()` with a *relative* path on the same server succeeds; + `pure-ftpd` accepts the absolute form fine. If a working FTP server rejects ng_backup's writes, + try a different FTP server implementation before assuming the backend itself is broken. + +- **SMB/CIFS *lists* as available with just the `smbclient` CLI binary, but does not actually + work for ng_backup without the `smbclient` PECL extension too.** files_external falls back to a + shell-wrapped `smbclient` process when the native extension is missing, which registers the + backend in `occ backup:target:backends` fine, but that fallback's stream cannot seek. ng_backup + reads blobs back by byte range while backing up, so even the very first (small) backup fails + with `Seek failed in packs/...`. Install the extension (`libsmbclient-dev` + `pecl install + smbclient` + `docker-php-ext-enable smbclient` on Debian-based images), not just the CLI tool — + the backend being listed is not proof it will work here. Once the extension is present it works + the same as any other backend; no ng_backup-specific config beyond host/share/user/password. + +- **NFS is not a files_external backend at all.** Mount the NFS export on the server yourself + (e.g. via `/etc/fstab` or a systemd `.mount` unit) and point ng_backup's `local` backend at the + mount point; ng_backup never mounts or unmounts anything on its own, so an NFS mount dropping + mid-run behaves like any other local-path failure (the run resumes). See PLAN.md 3.5 for why NFS + can't be driven from PHP directly. + ## Commands | Command | Purpose | diff --git a/tests/integration/targets-matrix.sh b/tests/integration/targets-matrix.sh index e6c6173..602db55 100755 --- a/tests/integration/targets-matrix.sh +++ b/tests/integration/targets-matrix.sh @@ -1,17 +1,25 @@ #!/bin/bash -# Round trip of NG Backup against real S3 (s3mock) and SFTP backends, through files_external's own -# backend/auth classes, same as a production location would use. Backend correctness does not -# depend on the Nextcloud version or database, so this runs once (not matrixed across NC/DB), -# on a recent Nextcloud with sqlite. Each backend gets its own fresh Nextcloud container, since -# NG Backup's key can only be initialised once per instance. +# Round trip of NG Backup against real S3 (s3mock), SFTP, FTP and SMB backends, through +# files_external's own backend/auth classes, same as a production location would use. Backend +# correctness does not depend on the Nextcloud version or database, so this runs once (not +# matrixed across NC/DB), on a recent Nextcloud with sqlite. Each backend gets its own fresh +# Nextcloud container, since NG Backup's key can only be initialised once per instance. # Usage: tests/integration/targets-matrix.sh [nextcloud-version] set -euo pipefail V=${1:-35} APP=$(cd "$(dirname "$0")/../.." && pwd) NET=ngb-targets-net +SMB_IMAGE=ngb-targets-smb-nc +SMB_BUILD_DIR=$(mktemp -d) RESULTS=() -cleanup() { docker rm -fv ngb-targets-nc-s3 ngb-targets-nc-sftp ngb-targets-s3 ngb-targets-sftp >/dev/null 2>&1; docker network rm "$NET" >/dev/null 2>&1; } +cleanup() { + docker rm -fv ngb-targets-nc-s3 ngb-targets-nc-sftp ngb-targets-nc-ftp ngb-targets-nc-smb \ + ngb-targets-s3 ngb-targets-sftp ngb-targets-ftp ngb-targets-smb >/dev/null 2>&1 || true + docker rmi "$SMB_IMAGE" >/dev/null 2>&1 || true + docker network rm "$NET" >/dev/null 2>&1 || true + rm -rf "$SMB_BUILD_DIR" +} trap cleanup EXIT wait_tcp() { # host port @@ -19,11 +27,11 @@ wait_tcp() { # host port || { echo "FAIL: $1:$2 never came up"; exit 1; } } -start_nc() { # container-name +start_nc() { # container-name [image] docker run -d --name "$1" --network "$NET" \ -e NEXTCLOUD_ADMIN_USER=admin -e NEXTCLOUD_ADMIN_PASSWORD=admin-targets-pw -e NEXTCLOUD_TRUSTED_DOMAINS=localhost \ -e SQLITE_DATABASE=nextcloud \ - -v "$APP:/var/www/html/custom_apps/ng_backup:ro" nextcloud:$V-apache >/dev/null + -v "$APP:/var/www/html/custom_apps/ng_backup:ro" "${2:-nextcloud:$V-apache}" >/dev/null for i in $(seq 1 90); do docker exec -u www-data "$1" php occ status 2>/dev/null | grep -q "installed: true" && break; sleep 4; done docker exec "$1" chown www-data:www-data /var/www/html/custom_apps echo "=== NC $V / sqlite ($1): $(docker exec -u www-data "$1" php occ -V 2>/dev/null)" @@ -51,8 +59,37 @@ docker run -d --name ngb-targets-s3 --network "$NET" \ docker run -d --name ngb-targets-sftp --network "$NET" \ atmoz/sftp ngbackup:ngbackuppw:::upload >/dev/null +# stilliard/pure-ftpd: delfer/alpine-ftp-server (vsftpd) was tried first and rejects the absolute +# paths files_external's FTP backend always builds, inside this server's chroot specifically -- +# not a files_external bug (confirmed with plain php ftp_mkdir()), but pure-ftpd sidesteps it. +docker run -d --name ngb-targets-ftp --network "$NET" \ + -e "FTP_USER_NAME=ngbackup" -e "FTP_USER_PASS=ngbackuppw" -e "FTP_USER_HOME=/home/ngbackup" \ + -e "PUBLICHOST=ngb-targets-ftp" \ + stilliard/pure-ftpd >/dev/null + +# dperson/samba. The *native* smbclient PECL extension is required: files_external's shell-based +# fallback (just the smbclient CLI binary) still registers this backend, but its stream cannot +# seek, and ng_backup reads blobs back by byte range while backing up -- confirmed live, that +# fallback fails the very first (small) backup with "Seek failed in packs/...". The extension is +# not in the stock nextcloud image (needs libsmbclient-dev + a compile), so this backend gets its +# own small image, built once here, in its own directory (never /tmp directly: that would send +# every unrelated file under /tmp to the docker daemon as build context). +cat > "$SMB_BUILD_DIR/Dockerfile" </dev/null \\ + && pecl install smbclient >/dev/null 2>&1 \\ + && docker-php-ext-enable smbclient \\ + && apt-get clean +EOF +docker build -q -t "$SMB_IMAGE" "$SMB_BUILD_DIR" >/dev/null +docker run -d --name ngb-targets-smb --network "$NET" \ + -e USERID=1000 -e GROUPID=1000 \ + dperson/samba -p -u "ngbackup;ngbackuppw" -s "backup;/share;yes;no;no;ngbackup" >/dev/null + wait_tcp ngb-targets-s3 9090 wait_tcp ngb-targets-sftp 22 +wait_tcp ngb-targets-ftp 21 +wait_tcp ngb-targets-smb 445 start_nc ngb-targets-nc-s3 run "S3 (s3mock)" ngb-targets-nc-s3 "amazons3 -a amazons3::accesskey -o bucket=ngbackups -o hostname=ngb-targets-s3 -o port=9090 -o region=us-east-1 -o use_ssl=false -o use_path_style=true -o key=test -o secret=test" @@ -60,6 +97,12 @@ run "S3 (s3mock)" ngb-targets-nc-s3 "amazons3 -a amazons3::accesskey -o bucket=n start_nc ngb-targets-nc-sftp run "SFTP" ngb-targets-nc-sftp "sftp -a password::password -o host=ngb-targets-sftp -o root=/upload -o user=ngbackup -o password=ngbackuppw" +start_nc ngb-targets-nc-ftp +run "FTP" ngb-targets-nc-ftp "ftp -a password::password -o host=ngb-targets-ftp -o user=ngbackup -o password=ngbackuppw" + +start_nc ngb-targets-nc-smb "$SMB_IMAGE" +run "SMB" ngb-targets-nc-smb "smb -a password::password -o host=ngb-targets-smb -o share=backup -o user=ngbackup -o password=ngbackuppw" + printf '%s\n' "${RESULTS[@]}" printf '%s\n' "${RESULTS[@]}" | grep -q FAIL && exit 1 exit 0 From c9f82a79e48447aa9ab0c644c2d459ca59fdfba7 Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Tue, 6 Oct 2026 00:44:23 +0200 Subject: [PATCH 05/13] Phase 2: retention for per-user exports (occ backup:retention --user-last) PruneService::userExportRetention()/setUserExportRetention(): a single, app-wide "keep the newest N" setting (default 3, same default as the snapshot policy's "last"), deliberately simpler than the full daily/weekly/monthly RetentionPolicy -- per-user exports are on-demand only (occ backup:backup:user, see the fold-out task about whether they ever join the automatic schedule), so there is no regular cadence yet to bucket by day/week/month. PruneService::applyUserExports(): groups a location's user exports by uid (read from each manifest's own 'time' field, the same field snapshot retention already reads), keeps the newest N per user, forgets the rest. No trash, unlike a snapshot: these are not yet the primary safety net the deletion delay protects. Runs under the same exclusive lock as snapshot retention. occ backup:retention gained --user-last=N; occ backup:prune now also applies it for every target it processes, reporting counts the same way it already does for snapshots and blob packs. Verified live against the dev rig (four real exports across four users, confirmed exactly the expected ones were kept/forgotten at two different --user-last values) and added coverage to tests/integration/user-roundtrip.sh (an account's manual export plus the automatic safety-export a replace-mode restore makes of it, pruned down to --user-last=2). --- PLAN.md | 5 ++- lib/Command/Prune.php | 14 +++++++ lib/Command/Retention.php | 12 +++++- lib/Service/PruneService.php | 64 +++++++++++++++++++++++++++++ tests/integration/user-roundtrip.sh | 12 ++++++ 5 files changed, 105 insertions(+), 2 deletions(-) diff --git a/PLAN.md b/PLAN.md index 53ff3a6..3709d3d 100644 --- a/PLAN.md +++ b/PLAN.md @@ -346,7 +346,10 @@ minimale volledige restore (`occ backup:restore:full` op een verse installatie) SMB vereist de `smbclient` PECL-extensie (niet standaard in de Nextcloud-image, in CI nu on-the-fly gebouwd). NFS is geen eigen backend: lokaal mounten + `local`-backend erop richten. 3. Verificatie/checksum-audit-commando — **klaar** (2026-10-05) -4. Retentie/prune voor user-exports +4. Retentie/prune voor user-exports — **klaar** (2026-10-06): `occ backup:retention --user-last=N` + (standaard 3, per installatie, net als de bestaande snapshot-policy), toegepast door + `occ backup:prune` (geen prullenbak voor user-exports, in tegenstelling tot snapshots — on-demand, + niet het primaire vangnet). 5. Automatische user-export inpluggen in het reguliere schema (of expliciet besluit dat het on-demand blijft) 9. SMB/NFS intern testen op de eigen OMV-instantie — nog open (intern, niet in CI) diff --git a/lib/Command/Prune.php b/lib/Command/Prune.php index aaafc3e..6b5bf3b 100644 --- a/lib/Command/Prune.php +++ b/lib/Command/Prune.php @@ -37,6 +37,20 @@ final class Prune extends Command { $output->writeln('Policy: ' . json_encode($this->prune->policy()->toArray()) . ($dry ? ' (dry run)' : '')); $rc = 0; foreach ($targets as $t) { + // User exports first: Repository::prune()'s blob garbage collection (run as part of + // apply() below) scans the users/ manifests still on the location at that point, so + // forgetting one here lets its now-unused blobs be freed in this same pass. + try { + $ru = $this->prune->applyUserExports($t, $dry); + } catch (\Throwable $e) { + $output->writeln('' . $t->getName() . ' (user exports): ' . $e->getMessage() . ''); + $rc = 1; + continue; + } + if ($ru['forgotten'] !== []) { + $output->writeln(sprintf('%s: %s %d user export(s) for %d user(s)', + $t->getName(), $dry ? 'would forget' : 'forgot', count($ru['forgotten']), count($ru['kept']))); + } try { $r = $this->prune->apply($t, $dry); } catch (\Throwable $e) { diff --git a/lib/Command/Retention.php b/lib/Command/Retention.php index 9e205e5..68c60c6 100644 --- a/lib/Command/Retention.php +++ b/lib/Command/Retention.php @@ -28,7 +28,8 @@ final class Retention extends Command { ->addOption('daily', null, InputOption::VALUE_REQUIRED, 'Keep one per day for the last N days with a backup') ->addOption('weekly', null, InputOption::VALUE_REQUIRED, 'Keep one per week for N weeks') ->addOption('monthly', null, InputOption::VALUE_REQUIRED, 'Keep one per month for N months') - ->setHelp("Example: occ backup:retention --last=3 --daily=7 --weekly=4 --monthly=12"); + ->addOption('user-last', null, InputOption::VALUE_REQUIRED, 'Keep the newest N of each user\'s own exports (occ backup:backup:user)') + ->setHelp("Example: occ backup:retention --last=3 --daily=7 --weekly=4 --monthly=12 --user-last=3"); } protected function execute(InputInterface $input, OutputInterface $output): int { @@ -48,7 +49,16 @@ final class Retention extends Command { return 1; } } + if ($input->getOption('user-last') !== null) { + try { + $this->prune->setUserExportRetention((int)$input->getOption('user-last')); + } catch (\InvalidArgumentException $e) { + $output->writeln('' . $e->getMessage() . ''); + return 1; + } + } $output->writeln(sprintf('Keep the last %d, plus one per day for %d days, per week for %d weeks, per month for %d months.', $p['last'], $p['daily'], $p['weekly'], $p['monthly'])); + $output->writeln(sprintf('Keep the newest %d of each user\'s own exports.', $this->prune->userExportRetention())); return 0; } } diff --git a/lib/Service/PruneService.php b/lib/Service/PruneService.php index 2c78399..284de2c 100644 --- a/lib/Service/PruneService.php +++ b/lib/Service/PruneService.php @@ -111,6 +111,70 @@ final class PruneService { return self::lockKey($target) . '/catalog'; } + /** How many of each user's own exports (occ backup:backup:user) to keep, newest first. */ + public function userExportRetention(): int { + return $this->appConfig->getValueInt(Application::APP_ID, 'user_export_retention', 3); + } + + public function setUserExportRetention(int $keep): void { + if ($keep < 1) { + throw new \InvalidArgumentException('Keep at least the last export'); + } + $old = $this->userExportRetention(); + $this->appConfig->setValueInt(Application::APP_ID, 'user_export_retention', $keep); + if ($old !== $keep) { + $this->alerts->securityEvent('user_export_retention_changed', ['keep' => (string)$keep]); + } + } + + /** + * Forgets a user's older exports once more than userExportRetention() exist for them, + * newest first; no trash, unlike a full snapshot (occ backup:backup:user is on-demand, not + * yet the primary safety net the deletion delay protects). + * + * @return array{kept: array>, forgotten: list} + */ + public function applyUserExports(Target $target, bool $dryRun = false): array { + if ($target->getAppendOnly()) { + throw new \RuntimeException('Location ' . $target->getName() . ' is append-only: NG Backup does not delete anything there'); + } + $lease = $this->lockExclusive($target, 1800); + try { + $repo = $this->targets->repository($target); + $keep = $this->userExportRetention(); + $byUser = []; + foreach ($repo->listUserExports() as $path) { + $uid = explode('/', $path)[1] ?? ''; + $manifest = json_decode($repo->getObject($path), true, 512, JSON_THROW_ON_ERROR); + $byUser[$uid][$path] = (int)strtotime((string)($manifest['time'] ?? '@0')); + } + $kept = []; + $forget = []; + foreach ($byUser as $uid => $times) { + arsort($times); + $i = 0; + foreach (array_keys($times) as $path) { + if ($i++ < $keep) { + $kept[$uid][] = $path; + } else { + $forget[] = $path; + } + } + } + if (!$dryRun) { + foreach ($forget as $path) { + $repo->forgetUserExport($path); + } + if ($forget !== []) { + $this->alerts->securityEvent('user_exports_forgotten', ['target' => $target->getName(), 'count' => (string)count($forget)], false); + } + } + return ['kept' => $kept, 'forgotten' => $forget]; + } finally { + $this->leases->release($lease); + } + } + public function policy(): RetentionPolicy { return RetentionPolicy::fromArray(json_decode($this->appConfig->getValueString(Application::APP_ID, 'retention', '{}'), true) ?: []); } diff --git a/tests/integration/user-roundtrip.sh b/tests/integration/user-roundtrip.sh index 69d3197..e6efe5a 100755 --- a/tests/integration/user-roundtrip.sh +++ b/tests/integration/user-roundtrip.sh @@ -54,6 +54,18 @@ SUM_AFTER=$($X sh -c "cd $DATA/$U/files && find . -type f -print0 | sort -z | xa [ "$SUM_BEFORE" = "$SUM_AFTER" ] || fail "files differ after a replace restore" ok "replace restores the original content under the same uid, byte-identical" +# Retention: $U now has the manual export above plus the automatic safety-export the replace +# restore made of it just before deleting it (4 total after these two more), then --user-last=2 +# must forget exactly the 2 oldest. +sleep 1.1 # exportId is time-based to the second; force each one to be distinct +occ backup:backup:user ci "$U" >/dev/null || fail "third backup:backup:user" +sleep 1.1 +occ backup:backup:user ci "$U" >/dev/null || fail "fourth backup:backup:user" +occ backup:retention --user-last=2 >/dev/null || fail "retention --user-last" +PRUNE_OUT=$(occ backup:prune ci) +echo "$PRUNE_OUT" | grep -q "forgot 2 user export(s) for 1 user(s)" || fail "prune did not forget the expected two user exports" +ok "user-export retention forgets exactly the exports over the keep count" + occ user:delete "$U" >/dev/null $X sh -c "rm -rf /tmp/ngb-urt-target /tmp/ngb-urt-pass" echo "USER ROUNDTRIP PASS" From d70a869070f81565529174c7cfee9c141ef22468 Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Tue, 6 Oct 2026 03:47:02 +0200 Subject: [PATCH 06/13] Phase 2: verify per-user exports, refuse SMB without the native extension, matrix user CI backup:verify accepts a user export path (users//): every blob its manifest references is checked the same way as for a snapshot, shallow or --deep. The SMB backend is refused at backup:target:add when the native smbclient PECL extension is not loaded, instead of registering and then failing the first backup with "Seek failed": with only the smbclient binary files_external falls back to a shell stream that cannot seek. The per-user integration job now runs on Nextcloud 32-35, not just 35: it calls user_migration's internal UserMigrationService, whose API may change between versions. --- .github/workflows/ci.yml | 13 +++++++++---- lib/Command/Verify.php | 8 ++++++-- lib/Repository/Repository.php | 34 ++++++++++++++++++++++++++++++++++ lib/Service/TargetService.php | 6 ++++++ lib/Service/VerifyService.php | 13 +++++++++++-- 5 files changed, 66 insertions(+), 8 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a216970..44d9a35 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -63,19 +63,24 @@ jobs: integration-user: # Per-user backup/restore via user_migration (occ backup:backup:user / backup:restore:user), - # an optional dependency installed from the app store. Does not depend on the Nextcloud - # version or database beyond what the main integration job already covers, so this runs once. + # an optional dependency installed from the app store. It calls user_migration's internal + # UserMigrationService, whose API can change between Nextcloud versions, so this runs on every + # supported version (not the database, which the main integration job already covers). runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + nextcloud: ['32', '33', '34', '35'] steps: - uses: actions/checkout@v4 - name: Set up a fresh Nextcloud instance run: | docker run -d --name ngbu -e SQLITE_DATABASE=nextcloud -e NEXTCLOUD_ADMIN_USER=admin \ -e NEXTCLOUD_ADMIN_PASSWORD=admin-user-roundtrip -e NEXTCLOUD_TRUSTED_DOMAINS=localhost \ - -v "$PWD:/var/www/html/custom_apps/ng_backup:ro" nextcloud:35-apache + -v "$PWD:/var/www/html/custom_apps/ng_backup:ro" nextcloud:${{ matrix.nextcloud }}-apache for i in $(seq 1 90); do docker exec -u www-data ngbu php occ status 2>/dev/null | grep -q "installed: true" && break; sleep 4; done docker exec ngbu chown www-data:www-data /var/www/html/custom_apps - - name: Per-user backup/restore round trip + - name: Per-user backup/restore round trip on Nextcloud ${{ matrix.nextcloud }} run: tests/integration/user-roundtrip.sh "docker exec -u www-data ngbu" /var/www/html/data integration-full-restore: diff --git a/lib/Command/Verify.php b/lib/Command/Verify.php index bae03fb..a9d1a7c 100644 --- a/lib/Command/Verify.php +++ b/lib/Command/Verify.php @@ -49,13 +49,17 @@ HELP); protected function execute(InputInterface $input, OutputInterface $output): int { try { $target = $this->targets->get($input->getArgument('target')); - $result = $this->verify->verify($target, (string)$input->getArgument('snapshot'), (bool)$input->getOption('deep')); + $ref = (string)$input->getArgument('snapshot'); + $deep = (bool)$input->getOption('deep'); + $result = str_starts_with($ref, 'users/') + ? $this->verify->verifyUserExport($target, $ref, $deep) + : $this->verify->verify($target, $ref, $deep); } catch (\Throwable $e) { $output->writeln('' . $e->getMessage() . ''); return 1; } $output->writeln(sprintf('%d files, %d blobs checked%s.', $result['filesChecked'], $result['blobsChecked'], - $input->getOption('deep') ? sprintf(' (%.1f MiB decrypted)', $result['bytesChecked'] / 1048576) : '')); + $deep ? sprintf(' (%.1f MiB decrypted)', $result['bytesChecked'] / 1048576) : '')); if ($result['missing'] !== []) { $output->writeln('' . count($result['missing']) . ' blob(s) missing from the index: ' . implode(', ', $result['missing'])); } diff --git a/lib/Repository/Repository.php b/lib/Repository/Repository.php index 2d83758..1721308 100644 --- a/lib/Repository/Repository.php +++ b/lib/Repository/Repository.php @@ -716,6 +716,40 @@ final class Repository { return $stats; } + /** + * Same checks as verify(), for a user export: every blob its manifest references must be in + * the freshly re-authenticated index with its pack present; $deep also decrypts each. + * + * @return array{filesChecked:int, blobsChecked:int, bytesChecked:int, missing:list, failed:list} + */ + public function verifyUserExport(string $manifestPath, bool $deep = false, ?callable $heartbeat = null): array { + if (!$this->isRecordedUserExport($manifestPath)) { + throw new RepositoryException("$manifestPath is not a recorded user export for this location"); + } + $heartbeat ??= static function (): void { + }; + $this->index->load(true); + $stats = ['filesChecked' => 0, 'blobsChecked' => 0, 'bytesChecked' => 0, 'missing' => [], 'failed' => []]; + $seen = []; + $packSeen = []; + $manifest = json_decode($this->getObject($manifestPath), true, 512, JSON_THROW_ON_ERROR); + foreach ($manifest['entries'] as $entry) { + if (($entry['t'] ?? '') !== 'f') { + continue; + } + $stats['filesChecked']++; + foreach ($entry['b'] ?? [] as $id) { + if (isset($seen[$id])) { + continue; + } + $seen[$id] = true; + $this->verifyBlob($id, $deep, $stats, $packSeen); + $heartbeat(); + } + } + return $stats; + } + /** @param array $packSeen */ private function verifyBlob(string $id, bool $deep, array &$stats, array &$packSeen): void { if (!$this->index->has($id)) { diff --git a/lib/Service/TargetService.php b/lib/Service/TargetService.php index 3878feb..dfa8735 100644 --- a/lib/Service/TargetService.php +++ b/lib/Service/TargetService.php @@ -53,6 +53,12 @@ final class TargetService { if (!preg_match('/^[A-Za-z0-9][A-Za-z0-9 _.-]{0,63}$/', $name)) { throw new \InvalidArgumentException('Name: letters, digits, space, _ . - (max 64)'); } + // Without the native extension files_external falls back to the smbclient binary, which + // registers fine but cannot seek: the first backup then fails with "Seek failed". Refuse + // up front instead of letting the admin find out from a failed run. + if ($backend === 'smb' && !extension_loaded('smbclient')) { + throw new \InvalidArgumentException('The SMB backend needs the PHP smbclient extension (pecl install smbclient); the smbclient binary alone cannot seek and every backup would fail'); + } try { $this->mapper->findByName($name); throw new \InvalidArgumentException("A target named '$name' already exists"); diff --git a/lib/Service/VerifyService.php b/lib/Service/VerifyService.php index ef3d893..bef188e 100644 --- a/lib/Service/VerifyService.php +++ b/lib/Service/VerifyService.php @@ -20,9 +20,18 @@ final class VerifyService { /** @return array{filesChecked:int, blobsChecked:int, bytesChecked:int, missing:list, failed:list} */ public function verify(Target $target, string $snapshotId, bool $deep = false): array { + return $this->run($target, $deep, fn (\OCA\NgBackup\Repository\Repository $repo, callable $heartbeat): array => $repo->verify($snapshotId, $deep, $heartbeat)); + } + + /** @return array{filesChecked:int, blobsChecked:int, bytesChecked:int, missing:list, failed:list} */ + public function verifyUserExport(Target $target, string $manifestPath, bool $deep = false): array { + return $this->run($target, $deep, fn (\OCA\NgBackup\Repository\Repository $repo, callable $heartbeat): array => $repo->verifyUserExport($manifestPath, $deep, $heartbeat)); + } + + private function run(Target $target, bool $deep, callable $fn): array { $ttl = $deep ? 600 : 300; try { - return $this->leases->with(PruneService::lockKey($target), LeaseService::SHARED, $ttl, function (callable $refresh) use ($target, $snapshotId, $deep, $ttl) { + return $this->leases->with(PruneService::lockKey($target), LeaseService::SHARED, $ttl, function (callable $refresh) use ($target, $deep, $ttl, $fn): array { $last = time(); $heartbeat = function () use ($refresh, $ttl, &$last): void { if (time() - $last >= intdiv($ttl, 2)) { @@ -30,7 +39,7 @@ final class VerifyService { $last = time(); } }; - return $this->targets->repository($target)->verify($snapshotId, $deep, $heartbeat); + return $fn($this->targets->repository($target), $heartbeat); }); } catch (LockedException) { throw new \RuntimeException('A cleanup of ' . $target->getName() . ' is in progress; try again later'); From ba7f70e0f8c408b6305c8333c1815aad51d84f55 Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Tue, 6 Oct 2026 03:51:35 +0200 Subject: [PATCH 07/13] Rename per-user backup/restore commands to backup:user:backup and backup:user:restore --- .github/workflows/ci.yml | 2 +- PLAN.md | 2 +- lib/Command/BackupUser.php | 6 +++--- lib/Command/RestoreUser.php | 10 +++++----- lib/Command/Retention.php | 2 +- lib/Service/PruneService.php | 4 ++-- tests/integration/user-roundtrip.sh | 14 +++++++------- 7 files changed, 20 insertions(+), 20 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 44d9a35..8c37031 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -62,7 +62,7 @@ jobs: run: tests/integration/targets-matrix.sh integration-user: - # Per-user backup/restore via user_migration (occ backup:backup:user / backup:restore:user), + # Per-user backup/restore via user_migration (occ backup:user:backup / backup:user:restore), # an optional dependency installed from the app store. It calls user_migration's internal # UserMigrationService, whose API can change between Nextcloud versions, so this runs on every # supported version (not the database, which the main integration job already covers). diff --git a/PLAN.md b/PLAN.md index 3709d3d..d146e9a 100644 --- a/PLAN.md +++ b/PLAN.md @@ -276,7 +276,7 @@ onderdelen wel en niet terugkomen, op basis van de geregistreerde migrators. | `backup:list [--target=]` | herstelpunten | | `backup:verify [--deep]` | checksums controleren, optioneel alles ontsleutelen | | `backup:restore:file [--to=]` | bestand/map terugzetten | -| `backup:restore:user [--to-user=]` | gebruiker terugzetten via user_migration (fase 2) | +| `backup:user:restore [--to-user=]` | gebruiker terugzetten via user_migration (fase 2) | | `backup:restore:full ` | volledige restore (fase 2, verse installatie) | | `backup:target:add/list/remove/test` | opslagdoelen beheren en testen | | `backup:key:export / import` | herstelkit | diff --git a/lib/Command/BackupUser.php b/lib/Command/BackupUser.php index 75f0d35..a73c8ca 100644 --- a/lib/Command/BackupUser.php +++ b/lib/Command/BackupUser.php @@ -25,17 +25,17 @@ final class BackupUser extends Command { } protected function configure(): void { - $this->setName('backup:backup:user') + $this->setName('backup:user:backup') ->setDescription('Back up one user via user_migration (account, settings, files and everything else a migrator covers)') ->addArgument('target', InputArgument::REQUIRED, 'Location name or id') ->addArgument('user', InputArgument::REQUIRED, 'User id') ->setHelp(<<<'HELP' Streams a user_migration export straight into the encrypted repository: no zip, no copy in the user's own storage. Shares data with the regular backup; this needs the user_migration app -enabled. The manifest path this prints is what occ backup:restore:user expects. +enabled. The manifest path this prints is what occ backup:user:restore expects. Example: - occ backup:backup:user offsite alice + occ backup:user:backup offsite alice HELP); } diff --git a/lib/Command/RestoreUser.php b/lib/Command/RestoreUser.php index 90e2a95..198097a 100644 --- a/lib/Command/RestoreUser.php +++ b/lib/Command/RestoreUser.php @@ -24,10 +24,10 @@ final class RestoreUser extends Command { } protected function configure(): void { - $this->setName('backup:restore:user') - ->setDescription('Restore a user from a user_migration export (see backup:backup:user)') + $this->setName('backup:user:restore') + ->setDescription('Restore a user from a user_migration export (see backup:user:backup)') ->addArgument('target', InputArgument::REQUIRED, 'Location name or id') - ->addArgument('manifest', InputArgument::REQUIRED, 'Manifest path printed by backup:backup:user (users//)') + ->addArgument('manifest', InputArgument::REQUIRED, 'Manifest path printed by backup:user:backup (users//)') ->addOption('mode', 'm', InputOption::VALUE_REQUIRED, 'replace or as-backup', UserRestoreService::MODE_REPLACE) ->setHelp(<<<'HELP' Never imports over a live account. Modes: @@ -38,8 +38,8 @@ Never imports over a live account. Modes: the new name, never the existing account Example: - occ backup:restore:user offsite users/alice/20261005T120000Z-ab12cd34 - occ backup:restore:user offsite users/alice/20261005T120000Z-ab12cd34 --mode=as-backup + occ backup:user:restore offsite users/alice/20261005T120000Z-ab12cd34 + occ backup:user:restore offsite users/alice/20261005T120000Z-ab12cd34 --mode=as-backup HELP); } diff --git a/lib/Command/Retention.php b/lib/Command/Retention.php index 68c60c6..43eff92 100644 --- a/lib/Command/Retention.php +++ b/lib/Command/Retention.php @@ -28,7 +28,7 @@ final class Retention extends Command { ->addOption('daily', null, InputOption::VALUE_REQUIRED, 'Keep one per day for the last N days with a backup') ->addOption('weekly', null, InputOption::VALUE_REQUIRED, 'Keep one per week for N weeks') ->addOption('monthly', null, InputOption::VALUE_REQUIRED, 'Keep one per month for N months') - ->addOption('user-last', null, InputOption::VALUE_REQUIRED, 'Keep the newest N of each user\'s own exports (occ backup:backup:user)') + ->addOption('user-last', null, InputOption::VALUE_REQUIRED, 'Keep the newest N of each user\'s own exports (occ backup:user:backup)') ->setHelp("Example: occ backup:retention --last=3 --daily=7 --weekly=4 --monthly=12 --user-last=3"); } diff --git a/lib/Service/PruneService.php b/lib/Service/PruneService.php index 284de2c..68852b9 100644 --- a/lib/Service/PruneService.php +++ b/lib/Service/PruneService.php @@ -111,7 +111,7 @@ final class PruneService { return self::lockKey($target) . '/catalog'; } - /** How many of each user's own exports (occ backup:backup:user) to keep, newest first. */ + /** How many of each user's own exports (occ backup:user:backup) to keep, newest first. */ public function userExportRetention(): int { return $this->appConfig->getValueInt(Application::APP_ID, 'user_export_retention', 3); } @@ -129,7 +129,7 @@ final class PruneService { /** * Forgets a user's older exports once more than userExportRetention() exist for them, - * newest first; no trash, unlike a full snapshot (occ backup:backup:user is on-demand, not + * newest first; no trash, unlike a full snapshot (occ backup:user:backup is on-demand, not * yet the primary safety net the deletion delay protects). * * @return array{kept: array>, forgotten: list} diff --git a/tests/integration/user-roundtrip.sh b/tests/integration/user-roundtrip.sh index e6efe5a..73c2283 100755 --- a/tests/integration/user-roundtrip.sh +++ b/tests/integration/user-roundtrip.sh @@ -1,6 +1,6 @@ #!/bin/bash # Per-user backup/restore round trip via user_migration (optional dependency, see PLAN.md 3.8): -# occ backup:backup:user / occ backup:restore:user --mode=as-backup|replace. +# occ backup:user:backup / occ backup:user:restore --mode=as-backup|replace. # Usage: user-roundtrip.sh "" [target-args] # e.g. user-roundtrip.sh "docker exec -u www-data ngb-nc35-pgsql" /var/www/html/data set -euo pipefail @@ -32,12 +32,12 @@ $X sh -c "mkdir -p $DATA/$U/files/Docs && for i in 1 2 3; do head -c 100000 /dev occ files:scan "$U" >/dev/null SUM_BEFORE=$($X sh -c "cd $DATA/$U/files && find . -type f -print0 | sort -z | xargs -0 md5sum | md5sum") -MANIFEST=$(occ backup:backup:user ci "$U" | sed -n 's/.* to \(.*\)\.$/\1/p') -[ -n "$MANIFEST" ] || fail "backup:backup:user printed no manifest path" +MANIFEST=$(occ backup:user:backup ci "$U" | sed -n 's/.* to \(.*\)\.$/\1/p') +[ -n "$MANIFEST" ] || fail "backup:user:backup printed no manifest path" ok "backed up $U to $MANIFEST" # as-backup: restore into -bak; the original account must stay untouched. -occ backup:restore:user ci "$MANIFEST" --mode=as-backup >/dev/null || fail "restore as-backup" +occ backup:user:restore ci "$MANIFEST" --mode=as-backup >/dev/null || fail "restore as-backup" occ files:scan "$U-bak" >/dev/null SUM_BAK=$($X sh -c "cd $DATA/$U-bak/files && find . -type f -print0 | sort -z | xargs -0 md5sum | md5sum") [ "$SUM_BEFORE" = "$SUM_BAK" ] || fail "the -bak account's files differ from the original" @@ -48,7 +48,7 @@ ok "as-backup restores into -bak, byte-identical, original account untouche # replace: damage the account, then restore it in place under the same uid. $X sh -c "rm '$DATA/$U/files/Docs/f1.bin' && echo damaged > '$DATA/$U/files/Docs/f1.bin'" occ files:scan "$U" >/dev/null -occ backup:restore:user ci "$MANIFEST" --mode=replace >/dev/null || fail "restore replace" +occ backup:user:restore ci "$MANIFEST" --mode=replace >/dev/null || fail "restore replace" occ files:scan "$U" >/dev/null SUM_AFTER=$($X sh -c "cd $DATA/$U/files && find . -type f -print0 | sort -z | xargs -0 md5sum | md5sum") [ "$SUM_BEFORE" = "$SUM_AFTER" ] || fail "files differ after a replace restore" @@ -58,9 +58,9 @@ ok "replace restores the original content under the same uid, byte-identical" # restore made of it just before deleting it (4 total after these two more), then --user-last=2 # must forget exactly the 2 oldest. sleep 1.1 # exportId is time-based to the second; force each one to be distinct -occ backup:backup:user ci "$U" >/dev/null || fail "third backup:backup:user" +occ backup:user:backup ci "$U" >/dev/null || fail "third backup:user:backup" sleep 1.1 -occ backup:backup:user ci "$U" >/dev/null || fail "fourth backup:backup:user" +occ backup:user:backup ci "$U" >/dev/null || fail "fourth backup:user:backup" occ backup:retention --user-last=2 >/dev/null || fail "retention --user-last" PRUNE_OUT=$(occ backup:prune ci) echo "$PRUNE_OUT" | grep -q "forgot 2 user export(s) for 1 user(s)" || fail "prune did not forget the expected two user exports" From 943c95bf070ea9baf8395cae2be0f78c7f904680 Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Tue, 6 Oct 2026 16:57:56 +0200 Subject: [PATCH 08/13] Ask before installing user_migration; add the beta how-to backup:user:backup and backup:user:restore now ask the administrator before installing or enabling user_migration (or take --install-user-migration), so nothing changes on the server without consent. The restore warns that user_migration imports only its allowlisted app settings. Also: a clear error for unknown locations instead of a raw database message; prune reports the number of affected users, not the number of kept ones; and --user-last rejects values that are not whole numbers. Adds docs/HOWTO.md and updates the README and PLAN.md for the beta. --- PLAN.md | 8 +- README.md | 13 +- docs/HOWTO.md | 149 +++++++++++++++++++ lib/Command/BackupUser.php | 5 + lib/Command/Prune.php | 3 +- lib/Command/RestoreUser.php | 5 + lib/Command/Retention.php | 7 +- lib/Service/TargetService.php | 6 +- lib/UserMigration/UserMigrationInstaller.php | 80 ++++++++++ 9 files changed, 268 insertions(+), 8 deletions(-) create mode 100644 docs/HOWTO.md create mode 100644 lib/UserMigration/UserMigrationInstaller.php diff --git a/PLAN.md b/PLAN.md index d146e9a..6627a34 100644 --- a/PLAN.md +++ b/PLAN.md @@ -350,8 +350,12 @@ minimale volledige restore (`occ backup:restore:full` op een verse installatie) (standaard 3, per installatie, net als de bestaande snapshot-policy), toegepast door `occ backup:prune` (geen prullenbak voor user-exports, in tegenstelling tot snapshots — on-demand, niet het primaire vangnet). -5. Automatische user-export inpluggen in het reguliere schema (of expliciet besluit dat het on-demand blijft) -9. SMB/NFS intern testen op de eigen OMV-instantie — nog open (intern, niet in CI) +5. Automatische user-export in het reguliere schema — **naar backlog** (niet voor deze release; on-demand blijft) +9. SMB-roundtrip (protocol, niet een specifieke server) — **klaar** (2026-10-06): CI-matrix, 2 GB-roundtrip en + user-export via SMB; NFS via de OMV-share getest (`local`-backend op een gemounte share, 2026-10-06) +10. `backup:user:restore` zonder user_migration: vraagt om toestemming om de app te installeren en te activeren (of + `--install-user-migration`) — **klaar** (2026-10-06); waarschuwing over de settings-allowlist — **klaar**; + agenda/contacten bij migratie naar een server zonder die gebruiker — **getest** (2026-10-06, naar `migrated-personal`/`migrated-contacts`) Doorgeschoven naar **v1.3.0** (fase 3, niet meer fase 2): in-place volledige restore (eigen restore-modus i.p.v. onderhoudsmodus) en zelfbediening voor gebruikers (eigen bestanden diff --git a/README.md b/README.md index 4455b8a..d64caf9 100644 --- a/README.md +++ b/README.md @@ -7,7 +7,8 @@ installations and hosted Nextcloud. (Choosing SMB as a storage backend is the on see "Storage backend experiences" below — that requirement comes from files_external, not from ng_backup, and applies to any Nextcloud app that writes to an SMB share.) -**Status: in development (phase 1). Not ready for production use.** See `PLAN.md` for the design. +**Status: pre-release. Not yet tested on production.** See `PLAN.md` for the design and `docs/HOWTO.md` for a +step-by-step guide from setup to a tested restore. License: AGPL-3.0-or-later. @@ -30,7 +31,7 @@ License: AGPL-3.0-or-later. occ backup:key:init --label="Safe" # first passphrase, deletion delay (default 7 days) occ backup:key:slot:add --label="CTO" # optional second and third passphrase occ backup:key:slot:add --label="Head of IT" -occ backup:key:kit --output=/root/ng_backup-kit.json +occ backup:key:kit --output=/var/tmp/ng_backup-kit.json occ backup:key:confirm occ backup:target:add offsite amazons3 -a amazons3::accesskey -o bucket=... -o key=... -o secret=... occ backup:run @@ -106,7 +107,9 @@ What you should do on the location (strongly recommended): - **Files and folders** of a user: in the web interface or `occ backup:restore`, into a new folder ("Restored "), merged into the original place, or replacing it. - **Raw restore** of any path (for example `config/`) to a local directory: `occ backup:restore ... --to-directory=DIR`. -- **Whole server** (disaster recovery onto a fresh installation): planned for phase 2. +- **Whole server** (disaster recovery onto a fresh installation): `occ backup:restore:full`, see the how-to. +- **One user** (account, settings, files, calendars, contacts via user_migration): `occ backup:user:backup` and + `occ backup:user:restore`, replacing the account or restoring it as `-bak`. See the how-to. ## Storage backend experiences @@ -154,3 +157,7 @@ Contributions welcome via PR — add what you ran into with your own provider. | `backup:run`, `backup:status`, `backup:list` | back up, status, restore points | | `backup:browse`, `backup:restore` | browse and restore | | `backup:retention`, `backup:prune`, `backup:trash`, `backup:trash:restore` | retention, cleanup, trash | +| `backup:verify` | checksum audit of a restore point (`--deep` downloads and decrypts everything) | +| `backup:restore:full` | whole-server restore onto a fresh installation (after `backup:key:import`) | +| `backup:key:import` | import a recovery kit on a fresh installation | +| `backup:user:backup`, `backup:user:restore` | one user via user_migration (asks before installing it) | diff --git a/docs/HOWTO.md b/docs/HOWTO.md new file mode 100644 index 0000000..f90a0e9 --- /dev/null +++ b/docs/HOWTO.md @@ -0,0 +1,149 @@ +# NG Backup how-to + +Step by step: from first install to a tested restore. All commands run as the web user +(`sudo -u www-data php occ ...`, or `docker exec -u www-data php occ ...` in +All-in-One and Docker setups). Replace names in `` with your own. + +## 1. Create the key and the recovery kit + +``` +occ backup:key:init --label="Main" --delete-delay-days=7 +occ backup:key:kit --output=/var/tmp/ng_backup-kit.json +occ backup:key:confirm -u --code= --confirm="Yes, I confirm" +``` + +- The passphrase protects the backup. Keep it somewhere other than this server. +- Optional: extra passphrases for other people with `occ backup:key:slot:add --label="..."`. + Any one passphrase is enough to restore. +- The recovery kit (`/var/tmp/ng_backup-kit.json` above) is also required for disaster recovery. The web + user writes it there; then move it off the server as root, next to the passphrase, and delete the + copy in `/var/tmp`. Without the kit and a passphrase, the backup cannot be read. + +## 2. Add a backup location + +NG Backup uses Nextcloud's External storage backends. Check what your server supports first: + +``` +occ backup:target:backends +``` + +Examples: + +``` +occ backup:target:add offsite amazons3 -a amazons3::accesskey -o bucket= -o key= -o secret= +occ backup:target:add nas sftp -a sftp::password -o host= -o user= -o password= -o root=home +occ backup:target:add local local -a null::null -o datadir=/mnt/backup +``` + +- **SFTP on a Hetzner Storage Box sub-account:** use `-o root=home`, not `/` (see README). +- **FTP:** use a server that accepts absolute paths (for example `pure-ftpd`). +- **SMB:** requires the native `smbclient` PHP extension. The CLI binary alone is not enough (see README). + `backup:target:add` refuses SMB when the extension is missing. +- **NFS:** not a backend of its own. Mount the export on the server and use the `local` backend + with the mount point as `datadir`. + +Verify the location before relying on it: + +``` +occ backup:target:list +occ backup:status +``` + +## 3. Run the first backup + +``` +occ backup:run offsite -l first +occ backup:list +``` + +Later runs upload only changed data. An unchanged run uploads (almost) nothing; the output shows +the number of uploaded bytes, so check that value when you test incremental behaviour. + +Schedule `occ backup:run` from system cron (or use the Administration settings, if you prefer). + +## 4. Check the backup regularly + +``` +occ backup:verify offsite # shallow: all blocks are present +occ backup:verify offsite --deep # also downloads and decrypts every block +``` + +Run the deep check at least once after setting up a location, and after changing it. + +## 5. Restore files and folders + +In the web interface, or with occ: + +``` +occ backup:browse offsite +occ backup:restore offsite data//files/Docs --mode=new-folder +occ backup:restore offsite data//files/Docs --mode=merge +occ backup:restore offsite data//files/Docs --mode=replace +occ backup:restore offsite config --to-directory=/var/tmp/restore-config +``` + +- `new-folder` (default): puts the files in "Restored " next to the original. Safest choice. +- `merge`: adds missing files, keeps current ones. +- `replace`: overwrites the folder. Replaced files keep their old content as a version, and removed + files go to the trash. +- `--to-directory`: writes raw files to a local directory (for example `config/`). + +## 6. Back up and restore one user + +Needs the `user_migration` app. NG Backup asks before installing it: + +``` +occ backup:user:backup offsite +occ backup:user:restore offsite users// # replace (default) +occ backup:user:restore offsite users// --mode=as-backup +``` + +- If `user_migration` is missing, the command asks to install and enable it. For non-interactive + use, add `--install-user-migration` to approve that explicitly. The command stops after the + installation; run it once more. +- `replace` first makes a safety export of the current account, then recreates the user under the + same id. `as-backup` restores into `-bak` and leaves the original untouched. +- A restore onto a server where the user does not exist works. Calendars and address books are + placed in `migrated-personal` and `migrated-contacts`, not in the original names. +- **Settings:** on restore, `user_migration` imports only the app settings on its own allowlist + (for example calendar view and reminder settings). Other app settings of the user are skipped. + NG Backup reuses `user_migration`'s importer, so this list is not extended. +- Keep the last few exports per user: `occ backup:retention --user-last=3`. + +## 7. Disaster recovery: restore the whole server + +Use a fresh Nextcloud installation of the same major version, with NG Backup installed and +the same location available: + +``` +occ backup:key:import /path/ng_backup-kit.json --passphrase-file=/path/passphrase +occ backup:target:add offsite ... # same details as on the original server +occ backup:list offsite # the snapshots must be listed +occ backup:restore:full offsite +``` + +Afterwards, check that users, files and the database are complete, and run a backup of the restored +server. The restored installation keeps its own app configuration and secret. + +## 8. Retention + +``` +occ backup:retention --last=3 --daily=7 --weekly=4 --monthly=12 +occ backup:prune offsite --dry-run # show first +occ backup:prune offsite # apply the policy; forgotten snapshots go to the trash first +occ backup:trash # list the trash +occ backup:trash:restore offsite +``` + +Snapshots are kept according to the policy, and their data is deleted only after the deletion delay +(set with `backup:key:init --delete-delay-days`). `occ backup:prune offsite --dry-run` shows what would be +removed without changing anything. + +## 9. Checklist before you rely on it + +- [ ] Key created, passphrase stored off-server, recovery kit stored off-server +- [ ] Location added; `backup:status` shows it reachable +- [ ] First backup done and listed in `backup:list` +- [ ] `backup:verify --deep` succeeds +- [ ] One file restore tested (`new-folder`) +- [ ] Disaster recovery tested on a second, fresh installation (or in a test environment) diff --git a/lib/Command/BackupUser.php b/lib/Command/BackupUser.php index a73c8ca..6f3072f 100644 --- a/lib/Command/BackupUser.php +++ b/lib/Command/BackupUser.php @@ -9,10 +9,12 @@ namespace OCA\NgBackup\Command; use OCA\NgBackup\Service\TargetService; use OCA\NgBackup\Service\UserBackupService; +use OCA\NgBackup\UserMigration\UserMigrationInstaller; use OCP\IUserManager; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Input\InputArgument; use Symfony\Component\Console\Input\InputInterface; +use Symfony\Component\Console\Input\InputOption; use Symfony\Component\Console\Output\OutputInterface; final class BackupUser extends Command { @@ -20,6 +22,7 @@ final class BackupUser extends Command { private TargetService $targets, private UserBackupService $userBackup, private IUserManager $userManager, + private UserMigrationInstaller $installer, ) { parent::__construct(); } @@ -29,6 +32,7 @@ final class BackupUser extends Command { ->setDescription('Back up one user via user_migration (account, settings, files and everything else a migrator covers)') ->addArgument('target', InputArgument::REQUIRED, 'Location name or id') ->addArgument('user', InputArgument::REQUIRED, 'User id') + ->addOption('install-user-migration', null, InputOption::VALUE_NONE, 'Install and enable user_migration if it is missing (non-interactive consent)') ->setHelp(<<<'HELP' Streams a user_migration export straight into the encrypted repository: no zip, no copy in the user's own storage. Shares data with the regular backup; this needs the user_migration app @@ -47,6 +51,7 @@ HELP); return 1; } try { + $this->installer->ensureEnabled($input, $output); $target = $this->targets->get($input->getArgument('target')); $manifest = $this->userBackup->backupUser($target, $user); } catch (\Throwable $e) { diff --git a/lib/Command/Prune.php b/lib/Command/Prune.php index 6b5bf3b..eee2a1e 100644 --- a/lib/Command/Prune.php +++ b/lib/Command/Prune.php @@ -48,8 +48,9 @@ final class Prune extends Command { continue; } if ($ru['forgotten'] !== []) { + $affected = count(array_unique(array_map(static fn (string $path): string => explode('/', $path)[1] ?? '', $ru['forgotten']))); $output->writeln(sprintf('%s: %s %d user export(s) for %d user(s)', - $t->getName(), $dry ? 'would forget' : 'forgot', count($ru['forgotten']), count($ru['kept']))); + $t->getName(), $dry ? 'would forget' : 'forgot', count($ru['forgotten']), $affected)); } try { $r = $this->prune->apply($t, $dry); diff --git a/lib/Command/RestoreUser.php b/lib/Command/RestoreUser.php index 198097a..c2f86ad 100644 --- a/lib/Command/RestoreUser.php +++ b/lib/Command/RestoreUser.php @@ -9,6 +9,7 @@ namespace OCA\NgBackup\Command; use OCA\NgBackup\Service\TargetService; use OCA\NgBackup\Service\UserRestoreService; +use OCA\NgBackup\UserMigration\UserMigrationInstaller; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Input\InputArgument; use Symfony\Component\Console\Input\InputInterface; @@ -19,6 +20,7 @@ final class RestoreUser extends Command { public function __construct( private TargetService $targets, private UserRestoreService $userRestore, + private UserMigrationInstaller $installer, ) { parent::__construct(); } @@ -29,6 +31,7 @@ final class RestoreUser extends Command { ->addArgument('target', InputArgument::REQUIRED, 'Location name or id') ->addArgument('manifest', InputArgument::REQUIRED, 'Manifest path printed by backup:user:backup (users//)') ->addOption('mode', 'm', InputOption::VALUE_REQUIRED, 'replace or as-backup', UserRestoreService::MODE_REPLACE) + ->addOption('install-user-migration', null, InputOption::VALUE_NONE, 'Install and enable user_migration if it is missing (non-interactive consent)') ->setHelp(<<<'HELP' Never imports over a live account. Modes: replace (default) safety-exports the current account first, then deletes and recreates it @@ -45,6 +48,8 @@ HELP); protected function execute(InputInterface $input, OutputInterface $output): int { try { + $this->installer->ensureEnabled($input, $output); + $output->writeln('Note: on restore, user_migration only imports the app settings on its own allowlist (e.g. calendar view and reminder settings); other app settings of this user are skipped. Reason: NG Backup uses user_migration\'s importer; widening the list would mean building a settings importer of our own.'); $target = $this->targets->get($input->getArgument('target')); $user = $this->userRestore->restoreUser($target, (string)$input->getArgument('manifest'), (string)$input->getOption('mode')); } catch (\Throwable $e) { diff --git a/lib/Command/Retention.php b/lib/Command/Retention.php index 43eff92..bd44742 100644 --- a/lib/Command/Retention.php +++ b/lib/Command/Retention.php @@ -50,8 +50,13 @@ final class Retention extends Command { } } if ($input->getOption('user-last') !== null) { + $userLast = (string)$input->getOption('user-last'); + if (!ctype_digit($userLast)) { + $output->writeln('--user-last must be a whole number'); + return 1; + } try { - $this->prune->setUserExportRetention((int)$input->getOption('user-last')); + $this->prune->setUserExportRetention((int)$userLast); } catch (\InvalidArgumentException $e) { $output->writeln('' . $e->getMessage() . ''); return 1; diff --git a/lib/Service/TargetService.php b/lib/Service/TargetService.php index dfa8735..f705414 100644 --- a/lib/Service/TargetService.php +++ b/lib/Service/TargetService.php @@ -152,7 +152,11 @@ final class TargetService { } public function get(string $nameOrId): Target { - return ctype_digit($nameOrId) ? $this->mapper->find((int)$nameOrId) : $this->mapper->findByName($nameOrId); + try { + return ctype_digit($nameOrId) ? $this->mapper->find((int)$nameOrId) : $this->mapper->findByName($nameOrId); + } catch (DoesNotExistException) { + throw new \InvalidArgumentException("No such location: $nameOrId"); + } } /** Size limit for the data stored on this location (null = none). */ diff --git a/lib/UserMigration/UserMigrationInstaller.php b/lib/UserMigration/UserMigrationInstaller.php new file mode 100644 index 0000000..2c902ab --- /dev/null +++ b/lib/UserMigration/UserMigrationInstaller.php @@ -0,0 +1,80 @@ +appManager->isEnabledForAnyone(self::APP); + } + + private function codeIsPresent(): bool { + try { + $this->appManager->getAppPath(self::APP); + return true; + } catch (AppPathNotFoundException) { + return false; + } + } + + /** Installs (from the app store, if missing) and enables user_migration; throws on failure. */ + public function install(): void { + $args = $this->codeIsPresent() ? ['app:enable', self::APP] : ['app:install', self::APP]; + $occ = dirname($this->appManager->getAppPath(Application::APP_ID), 2) . '/occ'; + $command = implode(' ', array_map('escapeshellarg', [PHP_BINARY, $occ, ...$args])) . ' 2>&1'; + exec($command, $lines, $code); + if ($code !== 0) { + throw new \RuntimeException("occ " . implode(' ', $args) . " failed:\n" . implode("\n", $lines)); + } + } + + /** + * Returns when user_migration is enabled. Otherwise asks the administrator (interactive shell) or + * requires --install-user-migration, and throws when neither gives consent. + * + * @throws \RuntimeException + */ + public function ensureEnabled(InputInterface $input, OutputInterface $output): void { + if ($this->isEnabled()) { + return; + } + $consent = $input->getOption('install-user-migration') === true; + if (!$consent && $input->isInteractive()) { + $consent = (new QuestionHelper())->ask($input, $output, new ConfirmationQuestion( + 'The user_migration app is not enabled. Install and enable it now? [y/N] ', false)) === true; + } + if (!$consent) { + throw new \RuntimeException('The user_migration app is not enabled; per-user backup/restore needs it. Run "occ ' + . 'app:install ' . self::APP . '" and "occ app:enable ' . self::APP . '", or repeat with --install-user-migration'); + } + $output->writeln('Installing and enabling user_migration (approved by the administrator)...'); + $this->install(); + $this->appManager->clearAppsCache(); + if (!$this->isEnabled()) { + throw new \RuntimeException('user_migration was installed; run this command again (check with "occ app:list" if it stays disabled)'); + } + } +} From 614caf9f63bd736a7d5f67430c527fa6d78ac927 Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Tue, 6 Oct 2026 16:59:45 +0200 Subject: [PATCH 09/13] Document migrated- calendar names in the how-to; add backlog items --- PLAN.md | 2 ++ docs/HOWTO.md | 6 ++++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/PLAN.md b/PLAN.md index 6627a34..fd7d829 100644 --- a/PLAN.md +++ b/PLAN.md @@ -356,6 +356,8 @@ minimale volledige restore (`occ backup:restore:full` op een verse installatie) 10. `backup:user:restore` zonder user_migration: vraagt om toestemming om de app te installeren en te activeren (of `--install-user-migration`) — **klaar** (2026-10-06); waarschuwing over de settings-allowlist — **klaar**; agenda/contacten bij migratie naar een server zonder die gebruiker — **getest** (2026-10-06, naar `migrated-personal`/`migrated-contacts`) + Backlog: (a) de originele weergavenaam herstellen (het label uit de export is niet getest als + weergavenaam); (b) de interne `migrated-`-naam hernoemen (vereist directe databasewijziging, niet voor de beta). Doorgeschoven naar **v1.3.0** (fase 3, niet meer fase 2): in-place volledige restore (eigen restore-modus i.p.v. onderhoudsmodus) en zelfbediening voor gebruikers (eigen bestanden diff --git a/docs/HOWTO.md b/docs/HOWTO.md index f90a0e9..9f15f5e 100644 --- a/docs/HOWTO.md +++ b/docs/HOWTO.md @@ -103,8 +103,10 @@ occ backup:user:restore offsite users// --mode=as-backup installation; run it once more. - `replace` first makes a safety export of the current account, then recreates the user under the same id. `as-backup` restores into `-bak` and leaves the original untouched. -- A restore onto a server where the user does not exist works. Calendars and address books are - placed in `migrated-personal` and `migrated-contacts`, not in the original names. +- A restore onto a server where the user does not exist works. Calendars and address books get an + internal name with the prefix `migrated-` (for example `migrated-personal`). That prefix comes from + user_migration and cannot be turned off. The user can change the display name of these calendars and + address books in the app as usual; the internal name stays as it is. - **Settings:** on restore, `user_migration` imports only the app settings on its own allowlist (for example calendar view and reminder settings). Other app settings of the user are skipped. NG Backup reuses `user_migration`'s importer, so this list is not extended. From eb9100a74dfdaa8e658b19fce214ba83152de540 Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Tue, 6 Oct 2026 17:40:51 +0200 Subject: [PATCH 10/13] Fix review findings: catalog writes, user restore consent, retention validation Catalog: forgetting a user export or purging trash now writes the catalog before deleting the objects, so a crash leaves at most an unreferenced file instead of a catalog entry pointing at nothing. Per-user backup and the safety export reopen the repository inside the catalog lock, so they build on the latest catalog generation. Commands: backup:user:backup and backup:user:restore resolve the location before asking to install user_migration, and a successful install always stops with a rerun instruction. backup:retention rejects a malformed --user-last before changing any policy. backup:key:import fails clearly when the passphrase file cannot be read. --- lib/Command/BackupUser.php | 2 +- lib/Command/KeyImport.php | 7 ++++++- lib/Command/RestoreUser.php | 2 +- lib/Command/Retention.php | 11 +++++------ lib/Repository/Repository.php | 19 +++++++++++-------- lib/Service/RestoreFullService.php | 1 + lib/Service/UserBackupService.php | 8 ++++---- lib/Service/UserRestoreService.php | 6 +++--- lib/UserMigration/UserMigrationInstaller.php | 9 +++------ 9 files changed, 35 insertions(+), 30 deletions(-) diff --git a/lib/Command/BackupUser.php b/lib/Command/BackupUser.php index 6f3072f..42fdc8c 100644 --- a/lib/Command/BackupUser.php +++ b/lib/Command/BackupUser.php @@ -51,8 +51,8 @@ HELP); return 1; } try { - $this->installer->ensureEnabled($input, $output); $target = $this->targets->get($input->getArgument('target')); + $this->installer->ensureEnabled($input, $output); $manifest = $this->userBackup->backupUser($target, $user); } catch (\Throwable $e) { $output->writeln('' . $e->getMessage() . ''); diff --git a/lib/Command/KeyImport.php b/lib/Command/KeyImport.php index 6f9dd02..4e03669 100644 --- a/lib/Command/KeyImport.php +++ b/lib/Command/KeyImport.php @@ -58,7 +58,12 @@ HELP); } $file = $input->getOption('passphrase-file'); if ($file !== null) { - $pass = rtrim((string)@file_get_contents($file), "\r\n"); + $raw = @file_get_contents($file); + if ($raw === false) { + $output->writeln('Cannot read the passphrase file'); + return 1; + } + $pass = rtrim($raw, "\r\n"); } else { $helper = $this->getHelper('question'); $q = (new Question('Passphrase of one of the kit\'s slots: '))->setHidden(true)->setHiddenFallback(false); diff --git a/lib/Command/RestoreUser.php b/lib/Command/RestoreUser.php index c2f86ad..6ead952 100644 --- a/lib/Command/RestoreUser.php +++ b/lib/Command/RestoreUser.php @@ -48,9 +48,9 @@ HELP); protected function execute(InputInterface $input, OutputInterface $output): int { try { - $this->installer->ensureEnabled($input, $output); $output->writeln('Note: on restore, user_migration only imports the app settings on its own allowlist (e.g. calendar view and reminder settings); other app settings of this user are skipped. Reason: NG Backup uses user_migration\'s importer; widening the list would mean building a settings importer of our own.'); $target = $this->targets->get($input->getArgument('target')); + $this->installer->ensureEnabled($input, $output); $user = $this->userRestore->restoreUser($target, (string)$input->getArgument('manifest'), (string)$input->getOption('mode')); } catch (\Throwable $e) { $output->writeln('' . $e->getMessage() . ''); diff --git a/lib/Command/Retention.php b/lib/Command/Retention.php index bd44742..7851020 100644 --- a/lib/Command/Retention.php +++ b/lib/Command/Retention.php @@ -33,6 +33,10 @@ final class Retention extends Command { } protected function execute(InputInterface $input, OutputInterface $output): int { + if ($input->getOption('user-last') !== null && !ctype_digit((string)$input->getOption('user-last'))) { + $output->writeln('--user-last must be a whole number'); + return 1; + } $p = $this->prune->policy()->toArray(); $changed = false; foreach (['last', 'daily', 'weekly', 'monthly'] as $k) { @@ -50,13 +54,8 @@ final class Retention extends Command { } } if ($input->getOption('user-last') !== null) { - $userLast = (string)$input->getOption('user-last'); - if (!ctype_digit($userLast)) { - $output->writeln('--user-last must be a whole number'); - return 1; - } try { - $this->prune->setUserExportRetention((int)$userLast); + $this->prune->setUserExportRetention((int)$input->getOption('user-last')); } catch (\InvalidArgumentException $e) { $output->writeln('' . $e->getMessage() . ''); return 1; diff --git a/lib/Repository/Repository.php b/lib/Repository/Repository.php index 1721308..51e0999 100644 --- a/lib/Repository/Repository.php +++ b/lib/Repository/Repository.php @@ -422,9 +422,9 @@ final class Repository { if (!isset($this->catalog->userExports[$manifestPath])) { throw new RepositoryException("$manifestPath is not a recorded user export"); } - $this->backend->delete($manifestPath); unset($this->catalog->userExports[$manifestPath]); $this->catalog->write(); + $this->backend->delete($manifestPath); } /** @return array */ @@ -442,23 +442,26 @@ final class Repository { /** Permanently delete trash entries older than $delaySeconds. @return list purged snapshot ids */ public function purgeTrash(int $delaySeconds): array { - $purged = []; + $doomed = []; foreach ($this->trash() as $id => $info) { if (time() - $info['forgottenAt'] < $delaySeconds) { continue; } + $doomed[$id] = $info; + unset($this->catalog->trash[$id]); + } + if ($doomed === []) { + return []; + } + $this->catalog->write(); + foreach ($doomed as $id => $info) { if ($info['dbInTrash'] !== null) { $this->backend->delete($info['dbInTrash']); } $this->backend->delete('trash/snapshots/' . $id); $this->backend->delete('trash/info/' . $id); - unset($this->catalog->trash[$id]); - $purged[] = $id; } - if ($purged !== []) { - $this->catalog->write(); - } - return $purged; + return array_keys($doomed); } private function trashInfo(string $snapshotId): array { diff --git a/lib/Service/RestoreFullService.php b/lib/Service/RestoreFullService.php index 8fc72b9..fdcf754 100644 --- a/lib/Service/RestoreFullService.php +++ b/lib/Service/RestoreFullService.php @@ -110,6 +110,7 @@ final class RestoreFullService { // directory: once applied, 'secret' may differ from what ng_backup's own Target // options were encrypted with (see restoreFull()'s docblock), and every further // repository access through $this->targets would fail to decrypt them. + /** @var array $backupConfig */ $backupConfig = $this->fetchBackupConfig($target, $snapshotId); $dataDir = rtrim($this->config->getSystemValueString('datadirectory'), '/'); diff --git a/lib/Service/UserBackupService.php b/lib/Service/UserBackupService.php index e4eb877..23aac20 100644 --- a/lib/Service/UserBackupService.php +++ b/lib/Service/UserBackupService.php @@ -39,10 +39,10 @@ final class UserBackupService { $dest = new RepositoryExportDestination($repo, $user->getUID()); $service->export($dest, $user); // A short exclusive lock around the catalog write only: the shared lock above - // allows another concurrent backup (full or per-user) to be reading the same - // catalog generation at the same time, and both writing the next one would let - // one silently overwrite the other's addition. - $this->withCatalogLock($target, fn () => $repo->recordUserExport($dest->manifestPath())); + // allows another concurrent backup (full or per-user) to be writing the catalog at + // the same time. The repository is reopened inside the lock, so the write builds on + // the latest catalog generation instead of the one loaded before the lock was taken. + $this->withCatalogLock($target, fn () => $this->targets->repository($target)->recordUserExport($dest->manifestPath())); return $dest->manifestPath(); }); } diff --git a/lib/Service/UserRestoreService.php b/lib/Service/UserRestoreService.php index 68cf24a..56e61e4 100644 --- a/lib/Service/UserRestoreService.php +++ b/lib/Service/UserRestoreService.php @@ -83,9 +83,9 @@ final class UserRestoreService { $safety = new RepositoryExportDestination($repo, $originalUid); $service->export($safety, $existing); $safetyManifestPath = $safety->manifestPath(); - // Same race as UserBackupService::backupUser(): serialize the catalog write - // itself against any other concurrent backup/restore for this target. - $this->withCatalogLock($target, fn () => $repo->recordUserExport($safetyManifestPath)); + // Same race as UserBackupService::backupUser(): reopen the repository inside the + // catalog lock so the write builds on the latest generation. + $this->withCatalogLock($target, fn () => $this->targets->repository($target)->recordUserExport($safetyManifestPath)); if (!$existing->delete()) { throw new \RuntimeException("Could not delete the existing account $originalUid before restoring; its safety export is at $safetyManifestPath"); } diff --git a/lib/UserMigration/UserMigrationInstaller.php b/lib/UserMigration/UserMigrationInstaller.php index 2c902ab..d352638 100644 --- a/lib/UserMigration/UserMigrationInstaller.php +++ b/lib/UserMigration/UserMigrationInstaller.php @@ -7,7 +7,6 @@ declare(strict_types=1); namespace OCA\NgBackup\UserMigration; -use OCA\NgBackup\AppInfo\Application; use OCP\App\AppPathNotFoundException; use OCP\App\IAppManager; use Symfony\Component\Console\Input\InputInterface; @@ -43,7 +42,8 @@ final class UserMigrationInstaller { /** Installs (from the app store, if missing) and enables user_migration; throws on failure. */ public function install(): void { $args = $this->codeIsPresent() ? ['app:enable', self::APP] : ['app:install', self::APP]; - $occ = dirname($this->appManager->getAppPath(Application::APP_ID), 2) . '/occ'; + /** @psalm-suppress UndefinedClass */ + $occ = \OC::$SERVERROOT . '/occ'; $command = implode(' ', array_map('escapeshellarg', [PHP_BINARY, $occ, ...$args])) . ' 2>&1'; exec($command, $lines, $code); if ($code !== 0) { @@ -72,9 +72,6 @@ final class UserMigrationInstaller { } $output->writeln('Installing and enabling user_migration (approved by the administrator)...'); $this->install(); - $this->appManager->clearAppsCache(); - if (!$this->isEnabled()) { - throw new \RuntimeException('user_migration was installed; run this command again (check with "occ app:list" if it stays disabled)'); - } + throw new \RuntimeException('user_migration is installed and enabled; run this command again'); } } From cc09d7494211d5d1ed65290785fc24a64e8cce2f Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Tue, 6 Oct 2026 17:58:21 +0200 Subject: [PATCH 11/13] Commit full-backup snapshots to the catalog under the catalog lock A full backup opened its repository at the start of a step and wrote the catalog at the end of it, without the catalog lock. A per-user export or restore that wrote the catalog in between could lose its entry, or the snapshot could lose its own. The final step now only stores the snapshot object and moves the run to a new 'commit' phase. BackupService then records the snapshot under the catalog lock, from a repository opened inside that lock; if the lock is busy, the next step retries the commit. Repository::writeSnapshot() keeps its old behaviour as putSnapshot() plus recordSnapshot(). --- lib/Job/BackupRun.php | 4 ++-- lib/Repository/Repository.php | 14 ++++++++++++++ lib/Service/BackupService.php | 11 +++++++++++ tests/Unit/Job/BackupRunTest.php | 16 +++++++++++----- 4 files changed, 38 insertions(+), 7 deletions(-) diff --git a/lib/Job/BackupRun.php b/lib/Job/BackupRun.php index ca928b5..86ef1e5 100644 --- a/lib/Job/BackupRun.php +++ b/lib/Job/BackupRun.php @@ -133,7 +133,7 @@ final class BackupRun { if ($state['phase'] === 'finish') { $segments = $state['segments']; - $repo->writeSnapshot($state['snapshot'], + $repo->putSnapshot($state['snapshot'], ['time' => $state['started'], 'parent' => $state['parent'], 'label' => $state['label'], 'roots' => array_keys($state['roots']), 'stats' => $state['stats']] + $state['meta'], (function () use ($repo, $segments) { @@ -141,7 +141,7 @@ final class BackupRun { yield from $repo->readLines($blobs); } })()); - $state['phase'] = 'done'; + $state['phase'] = 'commit'; } return $state; } diff --git a/lib/Repository/Repository.php b/lib/Repository/Repository.php index 51e0999..5e51c51 100644 --- a/lib/Repository/Repository.php +++ b/lib/Repository/Repository.php @@ -288,6 +288,17 @@ final class Repository { * @param iterable $lines */ public function writeSnapshot(string $snapshotId, array $meta, iterable $lines): void { + $this->putSnapshot($snapshotId, $meta, $lines); + $this->recordSnapshot($snapshotId); + } + + /** + * Store a snapshot object without touching the catalog. Use recordSnapshot() afterwards, from a + * repository opened inside the catalog lock, so the catalog write builds on the latest generation. + * + * @param iterable $lines + */ + public function putSnapshot(string $snapshotId, array $meta, iterable $lines): void { $tree = fopen('php://temp/maxmemory:' . (4 * 1048576), 'w+b'); fwrite($tree, json_encode(['snapshot' => $snapshotId] + $meta, JSON_THROW_ON_ERROR) . "\n"); foreach ($lines as $line) { @@ -301,6 +312,9 @@ final class Repository { fclose($tree); rewind($encrypted); $this->backend->put('snapshots/' . $snapshotId, $encrypted); + } + + public function recordSnapshot(string $snapshotId): void { $this->catalog->snapshots[$snapshotId] = true; $this->catalog->write(); } diff --git a/lib/Service/BackupService.php b/lib/Service/BackupService.php index 3d2ae34..3649383 100644 --- a/lib/Service/BackupService.php +++ b/lib/Service/BackupService.php @@ -147,6 +147,17 @@ final class BackupService { } } else { $state['files'] = BackupRun::step($repo, $state['files'], $deadline); + if ($state['files']['phase'] === 'commit') { + // Catalog write under the catalog lock, from a repository opened inside it: a per-user + // export or restore may have written the catalog since this step opened $repo. + try { + $this->leases->with(PruneService::catalogLockKey($target), LeaseService::EXCLUSIVE, 30, + fn () => $this->targets->repository($target)->recordSnapshot((string)$state['files']['snapshot'])); + $state['files']['phase'] = 'done'; + } catch (LockedException) { + // Another catalog write is in progress: the next step retries this commit. + } + } if ($state['files']['phase'] === 'done') { $this->recordSnapshot($run, $state['files']); $this->alerts->storedBytes($target, $repo->storedBytes()); diff --git a/tests/Unit/Job/BackupRunTest.php b/tests/Unit/Job/BackupRunTest.php index 9df3667..6ad73c4 100644 --- a/tests/Unit/Job/BackupRunTest.php +++ b/tests/Unit/Job/BackupRunTest.php @@ -27,13 +27,14 @@ class BackupRunTest extends TestCase { $state = BackupRun::start($src); $steps = 0; - while ($state['phase'] !== 'done' && $steps < 500) { + while ($state['phase'] !== 'commit' && $steps < 500) { // Fresh repository object per step, state through JSON: as separate processes would. $repo = Repository::openWithKey($backend, $keys); $state = json_decode(json_encode(BackupRun::step($repo, $state, microtime(true) + 0.05)), true); $steps++; } - $this->assertSame('done', $state['phase']); + $this->assertSame('commit', $state['phase']); + Repository::openWithKey($backend, $keys)->recordSnapshot($state['snapshot']); $this->assertGreaterThan(2, $steps, 'the run was actually split into steps'); $target = $this->tempDir(); @@ -50,7 +51,7 @@ class BackupRunTest extends TestCase { $state = BackupRun::start($src); $mutated = false; - for ($i = 0; $i < 500 && $state['phase'] !== 'done'; $i++) { + for ($i = 0; $i < 500 && $state['phase'] !== 'commit'; $i++) { $state = BackupRun::step(Repository::openWithKey($backend, $keys), $state, microtime(true) + 0.01); if (!$mutated && $state['cur'] !== null && $state['cur']['offset'] > 0) { $fh = fopen("$src/big.bin", 'r+b'); @@ -60,6 +61,8 @@ class BackupRunTest extends TestCase { $mutated = true; } } + $this->assertSame('commit', $state['phase']); + Repository::openWithKey($backend, $keys)->recordSnapshot($state['snapshot']); $this->assertTrue($mutated); $this->assertGreaterThanOrEqual(1, $state['stats']['retried'] ?? 0); $target = $this->tempDir(); @@ -78,14 +81,17 @@ class BackupRunTest extends TestCase { Repository::initWithKey($backend, $keys, $keys->wrap('pw')); $run = function (?string $parent) use ($backend, $src, $keys): array { $state = BackupRun::start(['data' => $src], $parent); - for ($i = 0; $i < 2000 && $state['phase'] !== 'done'; $i++) { + for ($i = 0; $i < 2000 && $state['phase'] !== 'commit'; $i++) { $state = json_decode(json_encode(BackupRun::step(Repository::openWithKey($backend, $keys), $state, microtime(true))), true); } + if ($state['phase'] === 'commit') { + Repository::openWithKey($backend, $keys)->recordSnapshot($state['snapshot']); + } return $state; }; $first = $run(null); $second = $run($first['snapshot']); - $this->assertSame('done', $second['phase']); + $this->assertSame('commit', $second['phase']); $this->assertGreaterThan(150, $second["steps"], "one file per step"); $this->assertSame(0, $second['stats']['read'], 'merge-join with the parent across steps reuses every file'); $this->assertSame(200, $second['stats']['reused']); From 70965dd4bd9954558a66c993e088899a5d17b7be Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Tue, 6 Oct 2026 22:38:47 +0200 Subject: [PATCH 12/13] Keep leases alive during long restores and exports; PR review fixes Full restore, raw directory restore, per-user export and the safety export held a fixed-TTL lease on the location that nothing refreshed. A restore or export running longer than that TTL let a cleanup take over and delete data still in use. They now refresh the lease while they work (per blob, per row batch, per written chunk), throttled to once a minute. Also: - backup:prune still applies snapshot retention when user-export retention fails for a location. - The recovery kit carries the deletion delay, so a disaster recovery keeps the original protection instead of falling back to 7 days. Older kits still import with the default. - CI checkouts no longer persist the token in the git config. - recoveryKit() has its real return type; the stale baseline entry is gone. --- .github/workflows/ci.yml | 12 +++++++ lib/Command/Prune.php | 3 +- lib/Db/DbDumper.php | 5 ++- lib/Repository/Repository.php | 5 ++- lib/Service/KeyService.php | 7 ++-- lib/Service/LeaseService.php | 11 ++++++ lib/Service/RestoreFullService.php | 12 +++---- lib/Service/RestoreService.php | 13 +++++-- lib/Service/UserBackupService.php | 9 ++--- lib/Service/UserRestoreService.php | 7 ++-- .../RepositoryExportDestination.php | 10 ++++++ psalm-baseline.xml | 18 ---------- tests/Unit/Service/LeaseThrottleTest.php | 35 +++++++++++++++++++ 13 files changed, 106 insertions(+), 41 deletions(-) create mode 100644 tests/Unit/Service/LeaseThrottleTest.php diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8c37031..ebfa4a2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -17,6 +17,8 @@ jobs: php: ['8.2', '8.3', '8.4'] steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - uses: shivammathur/setup-php@v2 with: php-version: ${{ matrix.php }} @@ -29,6 +31,8 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - uses: shivammathur/setup-php@v2 with: php-version: '8.2' @@ -48,6 +52,8 @@ jobs: database: ['sqlite', 'mariadb', 'pgsql'] steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - name: Round trip on Nextcloud ${{ matrix.nextcloud }} / ${{ matrix.database }} run: tests/integration/matrix.sh "${{ matrix.nextcloud }}" "${{ matrix.database }}" @@ -58,6 +64,8 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - name: Round trip against S3 and SFTP run: tests/integration/targets-matrix.sh @@ -73,6 +81,8 @@ jobs: nextcloud: ['32', '33', '34', '35'] steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - name: Set up a fresh Nextcloud instance run: | docker run -d --name ngbu -e SQLITE_DATABASE=nextcloud -e NEXTCLOUD_ADMIN_USER=admin \ @@ -90,5 +100,7 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - name: Full restore round trip run: tests/integration/full-restore-roundtrip.sh 35-apache diff --git a/lib/Command/Prune.php b/lib/Command/Prune.php index eee2a1e..c60ad20 100644 --- a/lib/Command/Prune.php +++ b/lib/Command/Prune.php @@ -43,9 +43,10 @@ final class Prune extends Command { try { $ru = $this->prune->applyUserExports($t, $dry); } catch (\Throwable $e) { + // Snapshot retention is independent of this: report and carry on with it. $output->writeln('' . $t->getName() . ' (user exports): ' . $e->getMessage() . ''); $rc = 1; - continue; + $ru = ['forgotten' => []]; } if ($ru['forgotten'] !== []) { $affected = count(array_unique(array_map(static fn (string $path): string => explode('/', $path)[1] ?? '', $ru['forgotten']))); diff --git a/lib/Db/DbDumper.php b/lib/Db/DbDumper.php index d1fb64c..a43ce98 100644 --- a/lib/Db/DbDumper.php +++ b/lib/Db/DbDumper.php @@ -132,7 +132,7 @@ final class DbDumper { * * @param array{tables: array, rows: int}>} $manifest */ - public function restore(Repository $repo, array $manifest, ?array &$report = null): int { + public function restore(Repository $repo, array $manifest, ?array &$report = null, ?callable $heartbeat = null): int { $sm = $this->db->createSchemaManager(); $existing = array_flip($sm->listTableNames()); $total = 0; @@ -175,6 +175,9 @@ final class DbDumper { if (count($batch) >= self::INSERT_ROWS) { $total += $this->insertBatch($table, $columns, $batch, $binary); $batch = []; + if ($heartbeat !== null) { + $heartbeat(); + } } } if ($batch !== []) { diff --git a/lib/Repository/Repository.php b/lib/Repository/Repository.php index 5e51c51..5c6ccc3 100644 --- a/lib/Repository/Repository.php +++ b/lib/Repository/Repository.php @@ -198,7 +198,7 @@ final class Repository { * "config/" directly into the real data/config directory, not a subdirectory * named after the prefix). Ignored when $prefix is '' (nothing to strip). */ - public function restore(string $snapshotId, string $target, string $prefix = '', bool $stripPrefix = false): int { + public function restore(string $snapshotId, string $target, string $prefix = '', bool $stripPrefix = false, ?callable $heartbeat = null): int { if ($prefix !== '' && !self::isSafePath(rtrim($prefix, '/'))) { throw new RepositoryException('Invalid path prefix'); } @@ -227,6 +227,9 @@ final class Repository { } foreach ($entry['b'] as $id) { fwrite($out, $this->loadBlob($id)); + if ($heartbeat !== null) { + $heartbeat(); + } } fclose($out); if (filesize($tmp) !== $entry['s']) { diff --git a/lib/Service/KeyService.php b/lib/Service/KeyService.php index 9538c79..fca1d40 100644 --- a/lib/Service/KeyService.php +++ b/lib/Service/KeyService.php @@ -93,7 +93,9 @@ final class KeyService { $this->cached = $keys; $this->appConfig->setValueString(Application::APP_ID, self::KEY_WRAPPED, json_encode($kit['wrapped_key'], JSON_THROW_ON_ERROR), true, true); $this->appConfig->setValueString(Application::APP_ID, self::KEY_FINGERPRINT, $this->fingerprintOf($keys), true); - $this->appConfig->setValueInt(Application::APP_ID, self::KEY_DELETE_DELAY, self::DEFAULT_DELETE_DELAY, true); + // Kits from before this field existed fall back to the default. + $delay = (int)($kit['delete_delay_days'] ?? self::DEFAULT_DELETE_DELAY); + $this->appConfig->setValueInt(Application::APP_ID, self::KEY_DELETE_DELAY, ($delay >= 1 && $delay <= 365) ? $delay : self::DEFAULT_DELETE_DELAY, true); $this->appConfig->setValueInt(Application::APP_ID, self::KEY_KIT_VERSION, (int)($kit['kit_version'] ?? 1), true); // Having and successfully using a real kit file is stronger proof than the normal // download-and-confirm flow it replaces. @@ -260,13 +262,14 @@ final class KeyService { return $this->appConfig->getValueString(Application::APP_ID, self::KEY_FINGERPRINT, '', true); } - /** @return array{version:int, app:string, fingerprint:string, created:string, wrapped_key:array, instructions:string} */ + /** @return array{version:int, app:string, fingerprint:string, kit_version:int, delete_delay_days:int, confirmation_code:string, created:string, wrapped_key:array, slots:list, instructions:string} */ public function recoveryKit(): array { return [ 'version' => 1, 'app' => 'NG Backup', 'fingerprint' => $this->fingerprint(), 'kit_version' => $this->kitVersion(), + 'delete_delay_days' => $this->deleteDelayDays(), 'confirmation_code' => $this->kitCode(), 'created' => gmdate('c'), 'wrapped_key' => $this->wrappedKey(), diff --git a/lib/Service/LeaseService.php b/lib/Service/LeaseService.php index 874dbcb..43437af 100644 --- a/lib/Service/LeaseService.php +++ b/lib/Service/LeaseService.php @@ -76,6 +76,17 @@ final class LeaseService { $qb->executeStatement(); } + /** Wraps $refresh so calling it often (per file, per row batch) only touches the database every $seconds. */ + public static function throttled(callable $refresh, int $seconds = 60): \Closure { + $last = time(); + return static function () use ($refresh, $seconds, &$last): void { + if (time() - $last >= $seconds) { + $refresh(); + $last = time(); + } + }; + } + /** Run $fn under a lease; $refresh() can be called during long work. */ public function with(string $name, string $mode, int $ttl, callable $fn): mixed { $id = $this->acquire($name, $mode, $ttl); diff --git a/lib/Service/RestoreFullService.php b/lib/Service/RestoreFullService.php index fdcf754..7269caf 100644 --- a/lib/Service/RestoreFullService.php +++ b/lib/Service/RestoreFullService.php @@ -70,7 +70,8 @@ final class RestoreFullService { if (!$check['ok']) { throw new \RuntimeException($this->describeBlockers($check)); } - return $this->withSharedLock($target, function () use ($target, $snapshotId) { + return $this->withSharedLock($target, function (callable $refresh) use ($target, $snapshotId) { + $beat = LeaseService::throttled($refresh); $repo = $this->targets->repository($target); $meta = $repo->snapshotMeta($snapshotId); @@ -100,7 +101,7 @@ final class RestoreFullService { $ownAppConfig = $this->preserveOwnAppConfig(); $dumper = new DbDumper($this->db->getInner(), $prefix); try { - $dumper->restore($repo, $manifest, $dbReport); + $dumper->restore($repo, $manifest, $dbReport, $beat); } finally { $this->restoreOwnAppConfig($ownAppConfig); } @@ -114,7 +115,7 @@ final class RestoreFullService { $backupConfig = $this->fetchBackupConfig($target, $snapshotId); $dataDir = rtrim($this->config->getSystemValueString('datadirectory'), '/'); - $dataFiles = $this->restore->restoreToDirectory($target, $snapshotId, 'data', $dataDir, stripPrefix: true); + $dataFiles = $this->restore->restoreToDirectory($target, $snapshotId, 'data', $dataDir, stripPrefix: true, heartbeat: $beat); // Last step: nothing after this may need $this->targets->repository($target) again. if ($backupConfig !== []) { @@ -215,9 +216,8 @@ final class RestoreFullService { private function withSharedLock(Target $target, callable $fn): mixed { try { - // No progress from DbDumper::restore()/Repository::restore() to refresh this lease - // against; a very large instance could in theory outrun this TTL (same limitation as - // RestoreService::restoreToDirectory(), see also UserBackupService). + // Refreshed during the database and data restore (per row batch, per blob), so a + // cleanup cannot take over the location while a long restore is still reading it. return $this->leases->with(PruneService::lockKey($target), LeaseService::SHARED, 7200, $fn); } catch (LockedException) { throw new \RuntimeException('A cleanup of ' . $target->getName() . ' is in progress; try again later'); diff --git a/lib/Service/RestoreService.php b/lib/Service/RestoreService.php index d45d7f4..20bee68 100644 --- a/lib/Service/RestoreService.php +++ b/lib/Service/RestoreService.php @@ -156,14 +156,23 @@ final class RestoreService { * Repository::restore()); used to restore "data/" or "config/" straight into the * real data/config directory for occ backup:restore:full. */ - public function restoreToDirectory(Target $target, string $snapshotId, string $prefix, string $directory, bool $stripPrefix = false): int { + public function restoreToDirectory(Target $target, string $snapshotId, string $prefix, string $directory, bool $stripPrefix = false, ?callable $heartbeat = null): int { if ($prefix !== '' && !Repository::isSafePath($prefix)) { throw new \InvalidArgumentException('Invalid path: ' . $prefix); } if (!is_dir($directory) && !mkdir($directory, 0750, true) && !is_dir($directory)) { throw new \RuntimeException("Cannot create $directory"); } - return $this->withSharedLock($target, fn () => $this->targets->repository($target)->restore($snapshotId, $directory, trim($prefix, '/') === '' ? '' : trim($prefix, '/') . '/', $stripPrefix), 3600); + return $this->withSharedLock($target, function (callable $refresh) use ($target, $snapshotId, $prefix, $directory, $stripPrefix, $heartbeat): int { + $own = LeaseService::throttled($refresh); + $beat = static function () use ($own, $heartbeat): void { + $own(); + if ($heartbeat !== null) { + $heartbeat(); + } + }; + return $this->targets->repository($target)->restore($snapshotId, $directory, trim($prefix, '/') === '' ? '' : trim($prefix, '/') . '/', $stripPrefix, $beat); + }, 3600); } private function writeFile(Repository $repo, Folder $dest, string $relPath, array $entry): void { diff --git a/lib/Service/UserBackupService.php b/lib/Service/UserBackupService.php index 23aac20..4c5bf5c 100644 --- a/lib/Service/UserBackupService.php +++ b/lib/Service/UserBackupService.php @@ -34,9 +34,9 @@ final class UserBackupService { /** @throws UserMigrationException when user_migration is not enabled, or the export itself fails */ public function backupUser(Target $target, IUser $user): string { $service = $this->locator->get(); - return $this->withSharedLock($target, function () use ($target, $user, $service) { + return $this->withSharedLock($target, function (callable $refresh) use ($target, $user, $service) { $repo = $this->targets->repository($target); - $dest = new RepositoryExportDestination($repo, $user->getUID()); + $dest = new RepositoryExportDestination($repo, $user->getUID(), LeaseService::throttled($refresh)); $service->export($dest, $user); // A short exclusive lock around the catalog write only: the shared lock above // allows another concurrent backup (full or per-user) to be writing the catalog at @@ -49,10 +49,7 @@ final class UserBackupService { private function withSharedLock(Target $target, callable $fn): mixed { try { - // UserMigrationService::export() exposes no progress, so unlike - // RestoreService::restoreUserFiles() this lease cannot be refreshed mid-call; a very - // large account could in theory outrun this TTL. Resumable, step-based per-user - // backup (like the main BackupService run) is a possible future improvement. + // Refreshed by RepositoryExportDestination as it writes data, so a long export keeps it. return $this->leases->with(PruneService::lockKey($target), LeaseService::SHARED, 3600, $fn); } catch (LockedException) { throw new \RuntimeException('A cleanup of ' . $target->getName() . ' is in progress; try again later'); diff --git a/lib/Service/UserRestoreService.php b/lib/Service/UserRestoreService.php index 56e61e4..d4877e1 100644 --- a/lib/Service/UserRestoreService.php +++ b/lib/Service/UserRestoreService.php @@ -45,7 +45,7 @@ final class UserRestoreService { /** @throws UserMigrationException when user_migration is not enabled, or the restore itself fails */ public function restoreUser(Target $target, string $manifestPath, string $mode = self::MODE_REPLACE): IUser { $service = $this->locator->get(); - return $this->withSharedLock($target, function () use ($target, $manifestPath, $mode, $service): IUser { + return $this->withSharedLock($target, function (callable $refresh) use ($target, $manifestPath, $mode, $service): IUser { $repo = $this->targets->repository($target); // Only ever read a manifest the verified catalog actually recorded: a path alone // (even one this app generated before) proves nothing about it still being current @@ -80,7 +80,7 @@ final class UserRestoreService { if ($existing !== null) { // Never import over a live account: export its current state first, so nothing // is lost if the restore turns out to be the wrong choice. - $safety = new RepositoryExportDestination($repo, $originalUid); + $safety = new RepositoryExportDestination($repo, $originalUid, LeaseService::throttled($refresh)); $service->export($safety, $existing); $safetyManifestPath = $safety->manifestPath(); // Same race as UserBackupService::backupUser(): reopen the repository inside the @@ -119,8 +119,7 @@ final class UserRestoreService { private function withSharedLock(Target $target, callable $fn): mixed { try { - // See UserBackupService::withSharedLock(): UserMigrationService exposes no progress, - // so this lease cannot be refreshed mid-call. + // Refreshed while the safety export writes data (see UserBackupService::withSharedLock()). return $this->leases->with(PruneService::lockKey($target), LeaseService::SHARED, 3600, $fn); } catch (LockedException) { throw new \RuntimeException('A cleanup of ' . $target->getName() . ' is in progress; try again later'); diff --git a/lib/UserMigration/RepositoryExportDestination.php b/lib/UserMigration/RepositoryExportDestination.php index 8eec0d1..ce78995 100644 --- a/lib/UserMigration/RepositoryExportDestination.php +++ b/lib/UserMigration/RepositoryExportDestination.php @@ -28,6 +28,8 @@ final class RepositoryExportDestination implements IExportDestination { public function __construct( private Repository $repo, private string $uid, + /** Called while data is written (per chunk), to keep the location lease alive. */ + private ?\Closure $heartbeat = null, ) { $this->exportId = gmdate('Ymd\THis\Z') . '-' . bin2hex(random_bytes(4)); } @@ -36,6 +38,7 @@ final class RepositoryExportDestination implements IExportDestination { $writer = $this->repo->blobWriter($this->stats); $writer->write($content); $this->entries[self::norm($path)] = ['t' => 'f', 'b' => $writer->finish(), 's' => strlen($content)]; + $this->beat(); } public function addFileAsStream(string $path, $stream): void { @@ -46,6 +49,7 @@ final class RepositoryExportDestination implements IExportDestination { throw new UserMigrationException('Read error while exporting ' . $path); } $writer->write($chunk); + $this->beat(); } $this->entries[self::norm($path)] = ['t' => 'f', 'b' => $writer->finish(), 's' => $writer->bytes()]; } @@ -92,6 +96,12 @@ final class RepositoryExportDestination implements IExportDestination { return $this->stats; } + private function beat(): void { + if ($this->heartbeat !== null) { + ($this->heartbeat)(); + } + } + private static function norm(string $path): string { return trim($path, '/'); } diff --git a/psalm-baseline.xml b/psalm-baseline.xml index d874a89..ab61ac7 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -414,24 +414,6 @@ - - 1, - 'app' => 'NG Backup', - 'fingerprint' => $this->fingerprint(), - 'kit_version' => $this->kitVersion(), - 'confirmation_code' => $this->kitCode(), - 'created' => gmdate('c'), - 'wrapped_key' => $this->wrappedKey(), - 'slots' => $this->slots(), - 'instructions' => 'Keep this file safe, outside this server. Any one of the passphrases of the slots listed here opens it. ' - . 'To restore on a new server: install NG Backup, add the backup location and enter this kit with one passphrase. ' - . 'Without the kit (or the location) and a passphrase, the backups cannot be decrypted.', - ]]]> - - - - ['slot' => $s['slot'], 'label' => $s['label'], 'created' => $s['created']], $this->wrappedKey()['slots'])]]> diff --git a/tests/Unit/Service/LeaseThrottleTest.php b/tests/Unit/Service/LeaseThrottleTest.php new file mode 100644 index 0000000..a4ef4c0 --- /dev/null +++ b/tests/Unit/Service/LeaseThrottleTest.php @@ -0,0 +1,35 @@ +assertSame(0, $calls, 'no refresh before the interval has passed'); + } + + public function testRefreshesEveryCallWithZeroInterval(): void { + $calls = 0; + $beat = LeaseService::throttled(function () use (&$calls): void { + $calls++; + }, 0); + for ($i = 0; $i < 5; $i++) { + $beat(); + } + $this->assertSame(5, $calls); + } +} From e8ecb77df5ad9d790cb858f0a2d2a4d5912894b8 Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Tue, 6 Oct 2026 22:59:49 +0200 Subject: [PATCH 13/13] Refresh the location lease while a per-user restore reads its export A long import could outlive the lease, letting a cleanup forget the export being restored and free its blobs mid-read. --- lib/Service/UserRestoreService.php | 7 ++++--- lib/UserMigration/RepositoryImportSource.php | 5 +++++ 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/lib/Service/UserRestoreService.php b/lib/Service/UserRestoreService.php index d4877e1..eff3621 100644 --- a/lib/Service/UserRestoreService.php +++ b/lib/Service/UserRestoreService.php @@ -53,7 +53,8 @@ final class UserRestoreService { if (!$repo->isRecordedUserExport($manifestPath)) { throw new \InvalidArgumentException("$manifestPath is not a recorded user export for this location"); } - $source = new RepositoryImportSource($repo, $manifestPath); + $beat = LeaseService::throttled($refresh); + $source = new RepositoryImportSource($repo, $manifestPath, $beat); $originalUid = $source->getOriginalUid(); if ($mode === self::MODE_AS_BACKUP) { @@ -80,7 +81,7 @@ final class UserRestoreService { if ($existing !== null) { // Never import over a live account: export its current state first, so nothing // is lost if the restore turns out to be the wrong choice. - $safety = new RepositoryExportDestination($repo, $originalUid, LeaseService::throttled($refresh)); + $safety = new RepositoryExportDestination($repo, $originalUid, $beat); $service->export($safety, $existing); $safetyManifestPath = $safety->manifestPath(); // Same race as UserBackupService::backupUser(): reopen the repository inside the @@ -119,7 +120,7 @@ final class UserRestoreService { private function withSharedLock(Target $target, callable $fn): mixed { try { - // Refreshed while the safety export writes data (see UserBackupService::withSharedLock()). + // Refreshed while the safety export writes and the import reads (see UserBackupService::withSharedLock()). return $this->leases->with(PruneService::lockKey($target), LeaseService::SHARED, 3600, $fn); } catch (LockedException) { throw new \RuntimeException('A cleanup of ' . $target->getName() . ' is in progress; try again later'); diff --git a/lib/UserMigration/RepositoryImportSource.php b/lib/UserMigration/RepositoryImportSource.php index 84a6acc..8ba5a07 100644 --- a/lib/UserMigration/RepositoryImportSource.php +++ b/lib/UserMigration/RepositoryImportSource.php @@ -20,6 +20,8 @@ final class RepositoryImportSource implements IImportSource { public function __construct( private Repository $repo, string $manifestPath, + /** Called while blobs are read, to keep the location lease alive. */ + private ?\Closure $heartbeat = null, ) { $this->manifest = json_decode($repo->getObject($manifestPath), true, 512, JSON_THROW_ON_ERROR); } @@ -40,6 +42,9 @@ final class RepositoryImportSource implements IImportSource { throw new UserMigrationException('Cannot buffer ' . $path); } foreach ($this->repo->readBlobs($entry['b']) as $data) { + if ($this->heartbeat !== null) { + ($this->heartbeat)(); + } for ($done = 0, $len = strlen($data); $done < $len; $done += $n) { $n = fwrite($stream, $done === 0 ? $data : substr($data, $done)); if ($n === false || $n === 0) {