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

@@ -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('<error>' . $e->getMessage() . '</error>');

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('<error>Cannot read the passphrase file</error>');
+				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);

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('<comment>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.</comment>');
 			$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('<error>' . $e->getMessage() . '</error>');

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('<error>--user-last must be a whole number</error>');
+			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('<error>--user-last must be a whole number</error>');
-				return 1;
-			}
 			try {
-				$this->prune->setUserExportRetention((int)$userLast);
+				$this->prune->setUserExportRetention((int)$input->getOption('user-last'));
 			} catch (\InvalidArgumentException $e) {
 				$output->writeln('<error>' . $e->getMessage() . '</error>');
 				return 1;

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<string, array{forgottenAt:int, by:string, db:?string, dbInTrash:?string, time:?string, label:string}> */
@@ -442,23 +442,26 @@ final class Repository {
 
 	/** Permanently delete trash entries older than $delaySeconds. @return list<string> 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 !== []) {
+		return array_keys($doomed);
-			$this->catalog->write();
-		}
-		return $purged;
 	}
 
 	private function trashInfo(string $snapshotId): array {

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<string, mixed> $backupConfig */
 			$backupConfig = $this->fetchBackupConfig($target, $snapshotId);
 
 			$dataDir = rtrim($this->config->getSystemValueString('datadirectory'), '/');

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
+			// allows another concurrent backup (full or per-user) to be writing the catalog at
-			// catalog generation at the same time, and both writing the next one would let
+			// the same time. The repository is reopened inside the lock, so the write builds on
-			// one silently overwrite the other's addition.
+			// the latest catalog generation instead of the one loaded before the lock was taken.
-			$this->withCatalogLock($target, fn () => $repo->recordUserExport($dest->manifestPath()));
+			$this->withCatalogLock($target, fn () => $this->targets->repository($target)->recordUserExport($dest->manifestPath()));
 			return $dest->manifestPath();
 		});
 	}

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
+				// Same race as UserBackupService::backupUser(): reopen the repository inside the
-				// itself against any other concurrent backup/restore for this target.
+				// catalog lock so the write builds on the latest generation.
-				$this->withCatalogLock($target, fn () => $repo->recordUserExport($safetyManifestPath));
+				$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");
 				}

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();
+		throw new \RuntimeException('user_migration is installed and enabled; run this command again');
-		if (!$this->isEnabled()) {
-			throw new \RuntimeException('user_migration was installed; run this command again (check with "occ app:list" if it stays disabled)');
-		}
 	}
 }