diff --git a/CHANGELOG.md b/CHANGELOG.md index 0371b0b..f2ac389 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,14 @@ instead carried by the app description, the git tag, the release, and this file. ## [Unreleased] +### Added +- Edit a backup location (web interface and `occ backup:target:edit`): change the password, host, + folder, connection type or name without losing its history. Before this the only option was a + new location, which starts an empty repository and, with a retention lock at the provider, + keeps paying for the old one. The new settings are only saved when they lead to the same + repository; stored passwords are never sent to the browser, and an empty password field keeps + the stored one. Administrators are notified of the change. + ### Changed - App store listing: the homepage link points to the GitHub repository instead of back to the app store page itself. diff --git a/README.md b/README.md index d7691f3..3492cd7 100644 --- a/README.md +++ b/README.md @@ -192,7 +192,7 @@ On the backlog; none of these block the beta or 1.0.0, but know them before you |---|---| | `backup:key:init`, `backup:key:kit`, `backup:key:confirm` | create the key, recovery kit, confirmation | | `backup:key:slots`, `backup:key:slot:add/replace/remove` | passphrase slots | -| `backup:target:backends/add/list/remove/append-only` | backup locations | +| `backup:target:backends/add/edit/list/remove/append-only` | backup locations | | `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 | diff --git a/appinfo/info.xml b/appinfo/info.xml index c4ed8a6..3d549c1 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -37,6 +37,7 @@ See the README for known limitations and the recovery kit before relying on it.] OCA\NgBackup\Command\KeyConfirm OCA\NgBackup\Command\TargetBackends OCA\NgBackup\Command\TargetAdd + OCA\NgBackup\Command\TargetEdit OCA\NgBackup\Command\TargetList OCA\NgBackup\Command\TargetRemove OCA\NgBackup\Command\RunBackup diff --git a/appinfo/routes.php b/appinfo/routes.php index 018dc23..10e7a5d 100644 --- a/appinfo/routes.php +++ b/appinfo/routes.php @@ -18,6 +18,8 @@ return [ ['name' => 'AdminApi#untrash', 'url' => '/api/targets/{targetId}/trash/{snapshotId}', 'verb' => 'POST'], ['name' => 'AdminApi#backends', 'url' => '/api/backends', 'verb' => 'GET'], ['name' => 'AdminApi#addTarget', 'url' => '/api/targets', 'verb' => 'POST'], + ['name' => 'AdminApi#targetSettings', 'url' => '/api/targets/{id}/settings', 'verb' => 'GET'], + ['name' => 'AdminApi#updateTarget', 'url' => '/api/targets/{id}', 'verb' => 'PUT'], ['name' => 'AdminApi#removeTarget', 'url' => '/api/targets/{id}', 'verb' => 'DELETE'], ['name' => 'AdminApi#testTarget', 'url' => '/api/targets/{id}/test', 'verb' => 'POST'], ['name' => 'AdminApi#setTargetLimit', 'url' => '/api/targets/{id}/limit', 'verb' => 'PUT'], diff --git a/docs/HOWTO.md b/docs/HOWTO.md index 6256481..197ffdc 100644 --- a/docs/HOWTO.md +++ b/docs/HOWTO.md @@ -50,6 +50,14 @@ occ backup:target:list occ backup:status ``` +Changed password, new host name, or moved the repository? Edit the location instead of adding a +new one, so its history stays (in the web interface: **Edit** next to the location). The new +settings must lead to the same repository; options you do not pass keep their value: + +``` +occ backup:target:edit nas -o password= +``` + ## 3. Run the first backup ``` diff --git a/lib/Backend/StorageBackend.php b/lib/Backend/StorageBackend.php index aff055f..ae279e2 100644 --- a/lib/Backend/StorageBackend.php +++ b/lib/Backend/StorageBackend.php @@ -21,11 +21,12 @@ final class StorageBackend implements IBackend { public function __construct( private IStorage $storage, private string $base = '', + bool $createBase = true, ) { $this->base = trim($base, '/'); self::validate($this->base, true); $this->atomicPut = $storage->instanceOfStorage(\OCA\Files_External\Lib\Storage\AmazonS3::class); - if ($this->base !== '') { + if ($createBase && $this->base !== '') { $this->mkdirs($this->base); } } @@ -109,6 +110,15 @@ final class StorageBackend implements IBackend { } } + /** Whether the storage accepts the connection settings (files_external's own check). */ + public function canConnect(): bool { + try { + return !method_exists($this->storage, 'test') || (bool)$this->storage->test(); + } catch (\Throwable) { + return false; + } + } + public function exists(string $path): bool { return $this->storage->file_exists($this->abs($path)); } diff --git a/lib/Command/TargetEdit.php b/lib/Command/TargetEdit.php new file mode 100644 index 0000000..58a0f39 --- /dev/null +++ b/lib/Command/TargetEdit.php @@ -0,0 +1,71 @@ +setName('backup:target:edit') + ->setDescription('Change the connection settings or the name of a backup location, keeping its backups') + ->addArgument('target', InputArgument::REQUIRED, 'Location name or id') + ->addOption('name', null, InputOption::VALUE_REQUIRED, 'New name') + ->addOption('backend', 'b', InputOption::VALUE_REQUIRED, 'New files_external backend id (see backup:target:backends)') + ->addOption('auth', 'a', InputOption::VALUE_REQUIRED, 'New auth mechanism id') + ->addOption('option', 'o', InputOption::VALUE_REQUIRED | InputOption::VALUE_IS_ARRAY, 'Option key=value to change (repeatable); other options keep their value') + ->addOption('path', 'p', InputOption::VALUE_REQUIRED, 'New folder on the location') + ->setHelp(<<<'HELP' +The new settings must lead to the same repository as before, so the history of the location is +kept. Use it after a password change, a new host name, or after moving the repository. + +Example: + occ backup:target:edit nas -o password=new-secret +HELP); + } + + protected function execute(InputInterface $input, OutputInterface $output): int { + try { + $t = $this->targets->get($input->getArgument('target')); + $options = $this->targets->options($t); + if ($input->getOption('backend') !== null || $input->getOption('auth') !== null) { + $options = []; // a different connection type has its own options + } + foreach ($input->getOption('option') as $pair) { + if (!str_contains($pair, '=')) { + $output->writeln("Option '$pair' must be key=value"); + return 1; + } + $k = (string)strstr($pair, '=', true); + $v = substr($pair, strlen($k) + 1); + $options[$k] = match (strtolower($v)) { 'true' => true, 'false' => false, default => $v }; + } + $before = $t->getName(); + $t = $this->targets->update($t, $input->getOption('name') ?? $t->getName(), $input->getOption('backend') ?? $t->getBackend(), + $input->getOption('auth') ?? $t->getAuth(), $options, $input->getOption('path') ?? $t->getBasePath()); + } catch (\Throwable $e) { + $output->writeln('' . $e->getMessage() . ''); + return 1; + } + $this->alerts->securityEvent('target_changed', ['target' => $before]); + $output->writeln(sprintf('Location %s updated; repository %s found with the new settings.', $t->getName(), (string)$t->getRepositoryId())); + return 0; + } +} diff --git a/lib/Controller/AdminApiController.php b/lib/Controller/AdminApiController.php index 3d07195..63b375f 100644 --- a/lib/Controller/AdminApiController.php +++ b/lib/Controller/AdminApiController.php @@ -181,6 +181,28 @@ final class AdminApiController extends Controller { return new JSONResponse(['id' => $r['target']->getId(), 'created' => $r['created']]); } + /** The stored settings for the edit form; password values are left out (null). */ + #[PasswordConfirmationRequired] + public function targetSettings(int $id): JSONResponse { + $t = $this->targets->get((string)$id); + return new JSONResponse(['name' => $t->getName(), 'backend' => $t->getBackend(), 'auth' => $t->getAuth(), + 'options' => $this->targets->editableOptions($t), 'path' => $t->getBasePath()]); + } + + /** Options with a null value keep their stored value. */ + #[PasswordConfirmationRequired] + public function updateTarget(int $id, string $name, string $backend, string $auth, array $options = [], string $path = 'ng_backup'): JSONResponse { + try { + $t = $this->targets->get((string)$id); + $before = $t->getName(); + $t = $this->targets->update($t, $name, $backend, $auth, $options, $path); + } catch (\Throwable $e) { + return new JSONResponse(['error' => $e->getMessage()], Http::STATUS_BAD_REQUEST); + } + $this->alerts->securityEvent('target_changed', ['target' => $before]); + return new JSONResponse(['id' => $t->getId()]); + } + /** Size limit in GiB; null or 0 = unlimited. */ #[PasswordConfirmationRequired] public function setTargetLimit(int $id, ?float $maxGb = null): JSONResponse { diff --git a/lib/Notification/Notifier.php b/lib/Notification/Notifier.php index df81fa3..3cb9dd9 100644 --- a/lib/Notification/Notifier.php +++ b/lib/Notification/Notifier.php @@ -59,6 +59,7 @@ final class Notifier implements INotifier { $notification->setParsedSubject(match ($p['event'] ?? '') { 'kit_downloaded' => $l->t('%s downloaded the backup recovery kit', [$who]), 'target_removed' => $l->t('%1$s removed backup location %2$s', [$who, $p['target'] ?? '']), + 'target_changed' => $l->t('%1$s changed the connection settings of backup location %2$s', [$who, $p['target'] ?? '']), 'append_only_off' => $l->t('%1$s turned off append-only for backup location %2$s', [$who, $p['target'] ?? '']), 'retention_changed' => $l->t('%1$s changed the backup retention to %2$s', [$who, $p['policy'] ?? '']), 'snapshots_forgotten' => $l->t('%1$s moved %2$s snapshots of %3$s to the trash; they are deleted permanently after %4$s unless restored', [$who, $p['count'] ?? '', $p['target'] ?? '', $p['until'] ?? '']), diff --git a/lib/Service/TargetService.php b/lib/Service/TargetService.php index f705414..b3ecffa 100644 --- a/lib/Service/TargetService.php +++ b/lib/Service/TargetService.php @@ -37,7 +37,7 @@ final class TargetService { ) { } - /** @return list, parameters:array}> */ + /** @return list, parameters:array}> */ public function availableBackends(): array { return $this->factory()->describeBackends(); } @@ -50,20 +50,8 @@ final class TargetService { * @return array{target: Target, created: bool} */ public function add(string $name, string $backend, string $auth, array $options, string $basePath): array { - 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"); - } catch (\OCP\AppFramework\Db\DoesNotExistException) { - } + $this->validateName($name); + $this->requireUsableBackend($backend); $keys = $this->keys->keyRing(); $storageBackend = $this->buildBackend($backend, $auth, $options, $basePath); if ($storageBackend->exists('config')) { @@ -97,6 +85,103 @@ final class TargetService { return ['target' => $target, 'created' => $created]; } + /** + * Change how an existing location is reached (credentials, host, folder, even the connection + * type) or its name. The new settings must lead to the same repository: the history stays + * and a typo cannot silently start a new, empty repository. A null option value keeps the + * stored one, so the web interface never has to send stored passwords back. + * + * @param array $options + */ + public function update(Target $target, string $name, string $backend, string $auth, array $options, string $basePath): Target { + $this->validateName($name, $target->getId()); + $this->requireUsableBackend($backend); + $stored = $this->options($target); + foreach ($options as $key => $value) { + if ($value === null) { + if (array_key_exists($key, $stored)) { + $options[$key] = $stored[$key]; + } else { + unset($options[$key]); + } + } + } + // Do not create the folder: a typo in the path must not leave an empty folder behind. + $storageBackend = $this->buildBackend($backend, $auth, $options, $basePath, false); + if (!$storageBackend->exists('config')) { + if (!$storageBackend->canConnect()) { + throw new \InvalidArgumentException('Cannot connect with these settings. Check the host, user name and password'); + } + throw new \InvalidArgumentException("No NG Backup repository found in folder '" . trim($basePath, '/') . "'. To start an empty repository, add a new location instead"); + } + $repo = Repository::openWithKey($storageBackend, $this->keys->keyRing(), $this->indexCache, $this->anchor); + if ($target->getRepositoryId() !== null && $repo->id() !== $target->getRepositoryId()) { + throw new \InvalidArgumentException('These settings lead to a different repository than the one of this location'); + } + $target->setName($name); + $target->setBackend($backend); + $target->setAuth($auth); + $target->setOptions($this->crypto->encrypt(json_encode($options, JSON_THROW_ON_ERROR))); + $target->setBasePath(trim($basePath, '/')); + return $this->mapper->update($target); + } + + /** + * The stored options for an edit form: password-type values are replaced by null, which + * update() reads as "keep the stored value". + * + * @return array + */ + public function editableOptions(Target $target): array { + $secret = []; + foreach ($this->availableBackends() as $b) { + if ($b['id'] !== $target->getBackend()) { + continue; + } + $params = $b['parameters']; + foreach ($b['auth'] as $a) { + if ($a['id'] === $target->getAuth()) { + $params += $a['parameters']; + } + } + foreach ($params as $key => $p) { + if (($p['type'] ?? 0) === 2) { + $secret[$key] = true; + } + } + } + $options = $this->options($target); + foreach ($options as $key => $value) { + if (isset($secret[$key])) { + $options[$key] = null; + } + } + return $options; + } + + private function validateName(string $name, ?int $ownId = null): void { + if (!preg_match('/^[A-Za-z0-9][A-Za-z0-9 _.-]{0,63}$/', $name)) { + throw new \InvalidArgumentException('Name: letters, digits, space, _ . - (max 64)'); + } + try { + $other = $this->mapper->findByName($name); + } catch (DoesNotExistException) { + return; + } + if ($other->getId() !== $ownId) { + throw new \InvalidArgumentException("A target named '$name' already exists"); + } + } + + private function requireUsableBackend(string $backend): void { + // 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'); + } + } + /** * 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) @@ -193,8 +278,8 @@ final class TargetService { return json_decode($this->crypto->decrypt($target->getOptions()), true, 512, JSON_THROW_ON_ERROR); } - private function buildBackend(string $backend, string $auth, array $options, string $basePath): StorageBackend { - return new StorageBackend($this->factory()->create($backend, $auth, $options), $basePath); + private function buildBackend(string $backend, string $auth, array $options, string $basePath, bool $createBase = true): StorageBackend { + return new StorageBackend($this->factory()->create($backend, $auth, $options), $basePath, $createBase); } private ?ExternalStorageFactory $factory = null; diff --git a/psalm-baseline.xml b/psalm-baseline.xml index 2718b1c..cd981ee 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -474,12 +474,6 @@ - - factory()->describeBackends()]]> - - - , parameters:array}>]]> - diff --git a/src/api.js b/src/api.js index 0c53981..f30c03b 100644 --- a/src/api.js +++ b/src/api.js @@ -27,6 +27,8 @@ export default { confirmKit: (accepted, phrase, code) => call('post', '/key/confirm', { accepted, phrase, code }), backends: () => call('get', '/backends'), addTarget: (target) => call('post', '/targets', target), + targetSettings: (id) => call('get', `/targets/${id}/settings`), + updateTarget: (id, target) => call('put', `/targets/${id}`, target), removeTarget: (id) => call('delete', `/targets/${id}`), testTarget: (id) => call('post', `/targets/${id}/test`), setTargetLimit: (id, maxGb) => call('put', `/targets/${id}/limit`, { maxGb }), diff --git a/src/components/Locations.vue b/src/components/Locations.vue index 2131c7c..c29dd1c 100644 --- a/src/components/Locations.vue +++ b/src/components/Locations.vue @@ -14,12 +14,16 @@ {{ tests[target.id].ok ? t('ng_backup', 'OK, {n} snapshots', { n: tests[target.id].snapshots }) : tests[target.id].error }} {{ t('ng_backup', 'Test') }} + {{ t('ng_backup', 'Edit') }} {{ t('ng_backup', 'Remove') }} - {{ t('ng_backup', 'Add location') }} -
+ {{ t('ng_backup', 'Add location') }} + + + {{ t('ng_backup', 'Editing "{name}". The new settings must lead to the same repository, so its backups and history are kept. Leave a password field empty to keep the stored password.', { name: editing.name }) }} + @@ -32,8 +36,8 @@
- {{ t('ng_backup', 'Connect and add') }} - {{ t('ng_backup', 'Cancel') }} + {{ editing ? t('ng_backup', 'Connect and save') : t('ng_backup', 'Connect and add') }} + {{ t('ng_backup', 'Cancel') }}
{{ error }} @@ -41,7 +45,7 @@