From 4c7cbfa67db168c7fdf63fff94f8ae28208fb480 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Mon, 13 Nov 2023 14:53:19 +0100 Subject: [PATCH] migrate: Also backup roles Logging is useful, but a backup even more. --- application/clicommands/MigrateCommand.php | 47 ++++++++++++++----- .../clicommands/MigrateCommandTest.php | 41 ++++++++++++++++ 2 files changed, 75 insertions(+), 13 deletions(-) diff --git a/application/clicommands/MigrateCommand.php b/application/clicommands/MigrateCommand.php index 51470747..9881b4b0 100644 --- a/application/clicommands/MigrateCommand.php +++ b/application/clicommands/MigrateCommand.php @@ -239,11 +239,14 @@ class MigrateCommand extends Command * OPTIONS: * * --override Reset any existing Icinga DB Web rules + * + * --no-backup Don't back up roles */ public function roleAction(): void { /** @var ?bool $override */ $override = $this->params->get('override'); + $noBackup = $this->params->get('no-backup'); /** @var ?string $groupName */ $groupName = $this->params->get('group'); @@ -257,6 +260,8 @@ class MigrateCommand extends Command } $rc = 0; + $changed = false; + $restrictions = Config::$configDir . '/roles.ini'; $rolesConfig = $this->readFromIni($restrictions, $rc); $monitoringRestriction = 'monitoring/filter/objects'; @@ -301,6 +306,7 @@ class MigrateCommand extends Command if ($transformedFilter) { $role[$icingadbRestrictions['objects']] = QueryString::render($transformedFilter); + $changed = true; } } @@ -320,6 +326,8 @@ class MigrateCommand extends Command '*', implode(',', array_unique($icingadbProperties)) ); + + $changed = true; } if (isset($role['permissions'])) { @@ -335,6 +343,7 @@ class MigrateCommand extends Command if ($monitoringProtection !== null) { $role['icingadb/protect/variables'] = $monitoringProtection; + $changed = true; } } @@ -342,9 +351,11 @@ class MigrateCommand extends Command if (Str::startsWith($permission, 'icingadb/') || $permission === 'module/icingadb') { continue; } elseif (Str::startsWith($permission, 'monitoring/command/')) { + $changed = true; $updatedPermissions[] = $permission; $updatedPermissions[] = str_replace('monitoring/', 'icingadb/', $permission); } elseif ($permission === 'no-monitoring/contacts') { + $changed = true; $updatedPermissions[] = $permission; $role['icingadb/denylist/routes'] = 'users,usergroups'; } else { @@ -366,6 +377,7 @@ class MigrateCommand extends Command if (Str::startsWith($refusal, 'icingadb/') || $refusal === 'module/icingadb') { continue; } elseif (Str::startsWith($refusal, 'monitoring/command/')) { + $changed = true; $updatedRefusals[] = $refusal; $updatedRefusals[] = str_replace('monitoring/', 'icingadb/', $refusal); } else { @@ -395,6 +407,7 @@ class MigrateCommand extends Command ); $role[$icingadbRestriction] = $filter; + $changed = true; } } } @@ -404,23 +417,31 @@ class MigrateCommand extends Command $rolesConfig->setSection($name, $role); } - try { - $rolesConfig->saveIni(); - } catch (NotWritableError $error) { - Logger::error($error); - if ($this->skipMigration) { - Logger::error('Failed to transform icingadb restrictions'); - } else { - Logger::error('Failed to migrate monitoring restrictions'); + if ($changed) { + if (! $noBackup) { + $this->createBackupIni(Config::$configDir . '/roles'); } - exit(256); - } + try { + $rolesConfig->saveIni(); + } catch (NotWritableError $error) { + Logger::error($error); + if ($this->skipMigration) { + Logger::error('Failed to transform icingadb restrictions'); + } else { + Logger::error('Failed to migrate monitoring restrictions'); + } - if ($this->skipMigration) { - Logger::info('Successfully transformed all icingadb restrictions'); + exit(256); + } + + if ($this->skipMigration) { + Logger::info('Successfully transformed all icingadb restrictions'); + } else { + Logger::info('Successfully migrated monitoring restrictions and permissions in roles'); + } } else { - Logger::info('Successfully migrated monitoring restrictions and permissions in roles'); + Logger::info('Nothing to do'); } } diff --git a/test/php/application/clicommands/MigrateCommandTest.php b/test/php/application/clicommands/MigrateCommandTest.php index 199486f3..013d92a2 100644 --- a/test/php/application/clicommands/MigrateCommandTest.php +++ b/test/php/application/clicommands/MigrateCommandTest.php @@ -1135,6 +1135,10 @@ class MigrateCommandTest extends TestCase * - Whether refusals are properly migrated * - Whether restrictions are properly migrated * - Whether blacklists are properly migrated + * - Whether backups are created + * - Whether a second run changes nothing, if nothing changed + * - Whether a second run keeps the backup, if nothing changed + * - Whether a new backup isn't created, if nothing changed */ public function testRoleMigrationMigratesAllRoles() { @@ -1147,6 +1151,43 @@ class MigrateCommandTest extends TestCase $config = $this->loadConfig('roles.ini'); $this->assertSame($expected, $config); + + $backup = $this->loadConfig('roles.backup.ini'); + $this->assertSame($initialConfig, $backup); + + $command = $this->createCommandInstance('--role', '*'); + $command->roleAction(); + + $configAfterSecondRun = $this->loadConfig('roles.ini'); + $this->assertSame($config, $configAfterSecondRun); + + $backupAfterSecondRun = $this->loadConfig('roles.backup.ini'); + $this->assertSame($backup, $backupAfterSecondRun); + + $backup2 = $this->loadConfig('roles.backup1.ini'); + $this->assertEmpty($backup2); + } + + /** + * Checks the following: + * - Whether backups are skipped + * + * @depends testRoleMigrationMigratesAllRoles + */ + public function testRoleMigrationSkipsBackupIfRequested() + { + [$initialConfig, $expected] = $this->getConfig('all-roles'); + + $this->createConfig('roles.ini', $initialConfig); + + $command = $this->createCommandInstance('--role', '*', '--no-backup'); + $command->roleAction(); + + $config = $this->loadConfig('roles.ini'); + $this->assertSame($expected, $config); + + $backup = $this->loadConfig('roles.backup.ini'); + $this->assertEmpty($backup); } /**