diff --git a/CLAUDE.md b/CLAUDE.md index fbff4ba6c..532a709a0 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -101,7 +101,7 @@ docker compose exec -e DB_SERVER=db -e DB_NAME=syspass -e DB_USER=root -e DB_PAS -w /var/www/html app vendor/bin/phpunit -c tests/phpunit.xml --testsuite integration --no-coverage ``` -Both pass: **3955 unit** + **942 integration**. The integration suite includes the +Both pass: **3973 unit** + **972 integration**. The integration suite includes the end-to-end CLI command tests (`tests/Integration/Infrastructure/Adapter/In/Cli/`, real DI container + real DB via `CliTestCase`, per-test config under `/tmp/syspass-cli-tests`). Test-environment gotchas (the image provides these): @@ -390,6 +390,23 @@ Key constraints: A record of what repeatedly turned out to be broken, because the pattern predicts the next one better than any coverage number does. +**A guard that is not where the change happens.** The commonest shape here, and the one that reads +as correct in review. A public link's view limit and the temporary master password's fifty-attempt +cap were both tested in PHP against a row that had already been read, so two requests arriving +together both passed — and the attempt counter was written back as `$attempts + 1`, an absolute +value worked out from that same stale read, so guesses in parallel advanced it by one between them. +The master password's rotation re-encrypted every secret inside a transaction and then stored the +hash describing them outside it, leaving a vault nobody could open if those last two writes failed. +`40024210101.sql` made two commits out of one logical change, and DDL commits as it goes, so a +refused second statement left an upgrade that could be neither finished nor repeated. + +Where the codebase gets this right it is always the same move — the guard and the change are one +statement: `UserPassRecover::toggleUsedByHash()` consumes a reset token with `used = 0` in its +`WHERE` and throws when it affects nothing, `InstallThrottle` holds an exclusive `flock` across its +whole read-modify-write, `countViews + 1` is arithmetic the server does. **Ask where the decision is +taken and where the change lands; if they are not the same statement, work out what fits between +them.** + **The wiring, not the code.** php-di skips a constructor parameter that has a default *even when the container has a binding for its type*, silently. `Init::$sessionKeyService` was null that way, so `reKey()` — and the `session_regenerate_id()` inside it — never ran, and session identifiers were diff --git a/tests/Unit/Domain/Upgrade/UpgradePathCannotRegressTest.php b/tests/Unit/Domain/Upgrade/UpgradePathCannotRegressTest.php new file mode 100644 index 000000000..f420e07e1 --- /dev/null +++ b/tests/Unit/Domain/Upgrade/UpgradePathCannotRegressTest.php @@ -0,0 +1,236 @@ +. + */ + +namespace SP\Tests\Unit\Domain\Upgrade; + +use PHPUnit\Framework\Attributes\DataProvider; +use PHPUnit\Framework\Attributes\Group; +use PHPUnit\Framework\Attributes\Test; +use PHPUnit\Framework\TestCase; +use ReflectionClass; +use SP\Domain\Common\Providers\Version; +use SP\Domain\Upgrade\Services\UpgradeConfigText; +use SP\Domain\Upgrade\Services\UpgradeDatabase; +use SP\Domain\Common\Attributes\UpgradeVersion; +use SP\Infrastructure\Database\MysqlFileParser; +use SP\Infrastructure\File\FileHandler; + +/** + * An upgrade is the one thing a user runs on data they cannot afford to lose, on an installation + * that is already broken enough to need it. Both of these guard a way it stopped being repeatable. + * + * The migration rule comes from `40024210101.sql`, which gave `CustomFieldData` its natural + * primary key as two statements. DDL commits as it goes, so the drop stood alone when the key was + * refused on a duplicate, and the retry then died on `Can't DROP COLUMN id` before it could reach + * the statement that had failed — `UpgradeDatabase::apply()` writes the new version only after + * every statement succeeds, so the version stayed old and the file was re-run from the top for + * ever. It is one `ALTER` now, and `MigrationIsAtomicTest` holds it to failing as a whole against + * a real server. This holds the *next* migration to being written the same way. + * + * The ordering rule is about how those files are reached. `Upgrade::getTargetUpgradeHandlers()` + * yields a handler's versions in the order `getAttributes()` returns them, which is the order they + * are written in the class — nothing sorts them. Today they ascend, so the schema change runs + * before the data migration that needs it. A version added at the top of the list would run first, + * and out-of-order migrations are not something an installation reports; they are something it + * survives or does not. + */ +#[Group('unitary')] +class UpgradePathCannotRegressTest extends TestCase +{ + private const SCHEMAS = REAL_APP_ROOT . '/schemas'; + + /** + * Every handler that carries version attributes. + * + * @return array + */ + public static function handlerProvider(): array + { + return [ + 'UpgradeDatabase' => [UpgradeDatabase::class], + 'UpgradeConfigText' => [UpgradeConfigText::class], + ]; + } + + /** + * A handler's versions are written in the order they must be applied. + */ + #[Test] + #[DataProvider('handlerProvider')] + public function versionsAreDeclaredInTheOrderTheyApply(string $handler): void + { + $versions = self::versionsOf($handler); + + self::assertNotEmpty($versions, sprintf('%s carries no version, so it can never run', $handler)); + + $sorted = $versions; + usort( + $sorted, + static fn(string $a, string $b) => version_compare( + (string)Version::normalizeVersionForCompare($a), + (string)Version::normalizeVersionForCompare($b) + ) + ); + + self::assertSame( + $sorted, + $versions, + sprintf( + '%s applies its versions in the order they are written, because nothing sorts them. ' + . 'Written out of order they are applied out of order, and a schema change that ' + . 'runs after the data migration needing it does not announce itself.', + $handler + ) + ); + } + + /** + * No two handlers claim the same version twice over in a way that hides which runs first. + * + * They may share a version — `400.24240101` is both a row migration and a config one — and the + * order between handlers is then registration order in `CoreDefinitions`. What must not happen + * is a handler claiming the same version more than once, where the repeat is silently applied + * twice. + */ + #[Test] + #[DataProvider('handlerProvider')] + public function noHandlerClaimsAVersionTwice(string $handler): void + { + $versions = self::versionsOf($handler); + + self::assertSame( + array_values(array_unique($versions)), + $versions, + sprintf('%s would apply a repeated version more than once', $handler) + ); + } + + /** + * Every version file, and the statements it holds. + * + * @return array + */ + public static function migrationProvider(): array + { + $cases = []; + + foreach (glob(self::SCHEMAS . '/*.sql') ?: [] as $path) { + // dbstructure.sql builds a database rather than upgrading one: nothing has been + // applied when it runs, so a failure leaves nothing half-done. + if (basename($path) === 'dbstructure.sql') { + continue; + } + + $cases[basename($path)] = [$path]; + } + + return $cases; + } + + /** + * A migration that is refused part way leaves the operator something to run again. + * + * Three ways to satisfy it, and each is a real answer rather than a formality: + * + * - one statement, which the server applies whole; + * - every statement inside one transaction, which only works while none of them is DDL, + * because DDL commits and would end the transaction under the ones that follow; + * - DDL written so that re-running it is harmless (`IF NOT EXISTS`, `IF EXISTS`). + */ + #[Test] + #[DataProvider('migrationProvider')] + public function aMigrationCanBeRunAgainAfterItIsRefused(string $path): void + { + $statements = iterator_to_array((new MysqlFileParser(new FileHandler($path)))->parse('$$'), false); + + self::assertNotEmpty($statements, sprintf('%s holds no statements', basename($path))); + + if (count($statements) === 1) { + return; + } + + $ddl = array_values( + array_filter( + $statements, + static fn(string $s) => preg_match('/^\s*(alter|create|drop|rename|truncate)\b/i', $s) === 1 + ) + ); + + if ($ddl === []) { + self::assertTrue( + self::isWrappedInATransaction($statements), + sprintf( + '%s applies several statements and none of them is DDL, so it can be one ' + . 'transaction — and has to be, or a failure part way leaves the rows half ' + . 'migrated with the version unchanged.', + basename($path) + ) + ); + + return; + } + + foreach ($ddl as $statement) { + self::assertMatchesRegularExpression( + '/\bIF\s+(NOT\s+)?EXISTS\b/i', + $statement, + sprintf( + "%s applies more than one statement and one of them is DDL, which commits on " + . "its own — so a later failure cannot be undone and the retry meets a change " + . "that is already there. Make it a single statement, or write the DDL so that " + . "running it twice is harmless.\n\n%s", + basename($path), + $statement + ) + ); + } + } + + /** + * @param string[] $statements + */ + private static function isWrappedInATransaction(array $statements): bool + { + $first = strtolower(trim((string)reset($statements))); + $last = strtolower(trim((string)end($statements))); + + return (str_starts_with($first, 'start transaction') || str_starts_with($first, 'begin')) + && str_starts_with($last, 'commit'); + } + + /** + * @param class-string $handler + * + * @return string[] + */ + private static function versionsOf(string $handler): array + { + return array_map( + static fn(\ReflectionAttribute $attribute) => $attribute->newInstance()->version, + (new ReflectionClass($handler))->getAttributes(UpgradeVersion::class) + ); + } +}