diff --git a/src/Infrastructure/Adapter/Out/ItemPreset/Repositories/ItemPreset.php b/src/Infrastructure/Adapter/Out/ItemPreset/Repositories/ItemPreset.php index 78818716c..a2f10e853 100644 --- a/src/Infrastructure/Adapter/Out/ItemPreset/Repositories/ItemPreset.php +++ b/src/Infrastructure/Adapter/Out/ItemPreset/Repositories/ItemPreset.php @@ -197,7 +197,23 @@ public function getByFilter(string $type, int $userId, int $userGroupId, int $us // where they are: asked of a tie, the database was in fact returning the lower id, so // defining the rule this way settles the question without quietly moving anybody's // effective policy to a different preset. - ->orderBy(['score DESC', 'id ASC']) + // `fixed` first, because it is the stronger statement and the two are not comparable + // on score alone. `score` is `priority + 3 / + 2 / + 1` by specificity, and priority is + // an administrator-set 0-128, so a group preset at priority 5 outranks a user-scoped + // one left at the default 0 — cross-scope arbitration by priority, which is what the + // field is for. + // + // What that quietly did was let a *default* outrank a *rule*. Only a fixed preset is + // read as a rule: AccountPreset::checkPasswordPreset() and checkPasswordExpiry() do + // nothing at all unless getFixed() is 1, so a non-fixed preset winning means no policy + // is enforced. An administrator who marked a password policy fixed for one person, and + // separately gave that person's group a non-fixed preset carrying default values at a + // priority above zero, got no policy for them — with nothing anywhere saying so, since + // the losing preset is not reported, it is simply not selected. + // + // This changes which preset applies in that mixed case, and only in that case: between + // two fixed presets, or two non-fixed ones, the score decides exactly as before. + ->orderBy(['fixed DESC', 'score DESC', 'id ASC']) ->limit(1); $queryData = QueryData::buildWithMapper($query, ItemPresetModel::class); diff --git a/tests/Unit/Infrastructure/Adapter/Out/ItemPreset/Repositories/ItemPresetTest.php b/tests/Unit/Infrastructure/Adapter/Out/ItemPreset/Repositories/ItemPresetTest.php index c2b56747f..11a929615 100644 --- a/tests/Unit/Infrastructure/Adapter/Out/ItemPreset/Repositories/ItemPresetTest.php +++ b/tests/Unit/Infrastructure/Adapter/Out/ItemPreset/Repositories/ItemPresetTest.php @@ -59,6 +59,57 @@ class ItemPresetTest extends UnitaryTestCase private ItemPreset $itemPreset; + /** + * A fixed preset is chosen ahead of one that is not. + * + * Only a fixed preset is a rule: `AccountPreset::checkPasswordPreset()` and + * `checkPasswordExpiry()` do nothing unless `getFixed()` is 1, so a non-fixed preset winning + * the selection means no policy is enforced at all. And a non-fixed preset can win, because + * `score` is `priority + 3 / + 2 / + 1` and priority is an administrator-set 0-128 — a group + * preset at priority 5 outranks a user-scoped one left at the default 0. + * + * So an administrator who marked a password policy fixed for one person, and separately gave + * that person's group a non-fixed preset at any priority above zero, got no policy for them. + * Nothing reported it: the losing preset is not refused, it is simply not selected. + * + * Asserted on the emitted ORDER BY rather than against rows, for the same reason as the test + * below — which preset a tie returns is the database's business, and a behavioural test would + * pass on this MariaDB whether or not the query asked for it. + */ + public function testAFixedPresetIsChosenAheadOfOneThatIsNot(): void + { + $statement = null; + + $this->database + ->expects(self::once()) + ->method('runQuery') + ->with( + new Callback(static function (QueryData $arg) use (&$statement) { + $statement = $arg->getQuery()->getStatement(); + + return true; + }), + false + ); + + $this->itemPreset->getByFilter('test', 100, 200, 300); + + self::assertIsString($statement); + self::assertSame( + 1, + preg_match('/ORDER BY(.*?)(?:\\bLIMIT\\b|$)/is', $statement, $matches), + 'the query must carry an ORDER BY' + ); + + $columns = array_map(trim(...), explode(',', trim($matches[1]))); + + self::assertSame( + 'fixed DESC', + $columns[0], + 'a rule must be chosen ahead of a default, whatever the priorities say' + ); + } + /** * The preset that applies is decided, not left to the database. *