From faad400f1140f096c9450a505f86e38ccc9dcc9e Mon Sep 17 00:00:00 2001 From: blaipr Date: Wed, 26 Aug 2026 22:07:17 +0200 Subject: [PATCH] fix: a fixed preset outranks a default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Which preset applies is decided by `score`, which is `priority + 3 / + 2 / + 1` by how specifically it matches. `priority` is administrator-set, 0 to 128, so a group preset at priority 5 outranks a user-scoped one left at the default 0. That much is the feature: the field exists to arbitrate across scopes and this does not change it. What it quietly did was let a *default* outrank a *rule*. Only a fixed preset is a rule — `AccountPreset::checkPasswordPreset()` and `checkPasswordExpiry()` do nothing at all unless `getFixed()` is 1 — so a non-fixed preset winning the selection 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 any priority above zero, got no policy for them. Nothing anywhere said so: the losing preset is not refused, it is simply not selected, and `getByFilter()` returns one row. `fixed` leads the ordering now. Between two fixed presets, or two non-fixed ones, the score decides exactly as before — this only changes the mixed case, which is the case where the answer was wrong. It is a change to which preset applies for an installation configured that way, which is why it is stated here rather than buried: the alternative is a rule an administrator explicitly marked as one, silently doing nothing. Asserted on the emitted ORDER BY rather than against rows, for the same reason as the tie-break test beside it — which row a database returns for a tie is its own business, and a behavioural test passes on this MariaDB whether or not the query asked for anything. Checked by dropping `fixed DESC`: the new test fails. --- .../ItemPreset/Repositories/ItemPreset.php | 18 ++++++- .../Repositories/ItemPresetTest.php | 51 +++++++++++++++++++ 2 files changed, 68 insertions(+), 1 deletion(-) 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. *