From 48291ab6416435db6be79ba03155022ae28ef85a Mon Sep 17 00:00:00 2001 From: openhands Date: Fri, 9 Oct 2026 18:21:09 +0200 Subject: [PATCH] fix: correct the ACL revoke migration and update the login redirect e2e MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The deploy gate found two defects in the previous commits, both mine. 0034_acl_midrank_revoke.sql never applied: it joined `acl_roles` on `ar.model_type`, a column that table does not have (only `acl_model_permissions` does). It now joins on the id and keeps the `model_type` check where it belongs. That hid a second, worse bug. The rank was extracted with `SUBSTRING(slug, 7)`, but MySQL's SUBSTRING is 1-based and the digits start at position 6, right after `rank_`. rank_10 therefore parsed as 0 and rank_7 as an empty string, so every rank >= 7 would have lost exactly the grants the migration exists to preserve — the ACL repair would have made things worse, not better. Now reads from position 6. Verified against a real MariaDB with a fixture covering rank_1, rank_6, rank_7, rank_9, rank_10 and a non-rank slug: only the sub-7 roles lose their non-view admin.* grants, the multi-digit and higher ranks keep everything, and the non-rank slug is untouched. The mail index was checked the same way — it applies idempotently and EXPLAIN confirms `users_mail_index` with rows: 1. news.spec.ts expected to land on /me after signing in. That expectation predates the `?from=` honouring added in 39332149, which lands a bounced admin back where they were heading. The same step navigates to /admin/articles/new explicitly a few lines later, so nothing depended on it; the assertion now covers the redirect target instead. --- drizzle/migrations/0034_acl_midrank_revoke.sql | 10 ++++++++-- e2e/news-real/news.spec.ts | 5 ++++- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/drizzle/migrations/0034_acl_midrank_revoke.sql b/drizzle/migrations/0034_acl_midrank_revoke.sql index 324f65f0..7b3951d9 100644 --- a/drizzle/migrations/0034_acl_midrank_revoke.sql +++ b/drizzle/migrations/0034_acl_midrank_revoke.sql @@ -10,16 +10,22 @@ -- rank keeps dashboard + *.view and loses every other admin.* grant. Ranks -- that legitimately hold tools keep them, because rule 3 only targets -- rank >= 7 and those roles are not touched here. +-- +-- Note on the rank extraction: `acl_roles.slug` looks like `rank_7`, and +-- MySQL's SUBSTRING is 1-based, so the digits start at position 6 — right +-- after the 5-character `rank_`. Reading from position 7 truncates the first +-- digit, which turns rank_10 into 0 and rank_7 into an empty string, i.e. both +-- would compare as < 7 and lose grants this migration is supposed to preserve. +-- The REGEXP guard below guarantees the remainder really is all digits. DELETE `amp` FROM `acl_model_permissions` `amp` JOIN `acl_roles` `ar` ON `ar`.`id` = `amp`.`model_id` - AND `ar`.`model_type` = 'Role' AND `amp`.`model_type` = 'Role' JOIN `acl_permissions` `ap` ON `ap`.`id` = `amp`.`permission_id` WHERE `ap`.`slug` LIKE 'admin.%' AND `ap`.`slug` NOT LIKE '%.view' AND `ar`.`slug` REGEXP '^rank_[0-9]+$' - AND CAST(SUBSTRING(`ar`.`slug`, 7) AS UNSIGNED) < 7; + AND CAST(SUBSTRING(`ar`.`slug`, 6) AS UNSIGNED) < 7; \ No newline at end of file diff --git a/e2e/news-real/news.spec.ts b/e2e/news-real/news.spec.ts index d337de11..32e61950 100644 --- a/e2e/news-real/news.spec.ts +++ b/e2e/news-real/news.spec.ts @@ -88,7 +88,10 @@ test("staff signs in, saves a draft, previews it and publishes to anonymous read await page .locator('input[autocomplete="current-password"]') .press("Enter"); - await expect(page).toHaveURL(/\/me(?:\?|$)/); + // The login page honours `?from=`, so an admin bounced off /admin lands + // back where they were heading instead of on /me. The step below + // navigates there explicitly anyway; this asserts the redirect target. + await expect(page).toHaveURL(/\/admin\/articles\/new(?:\?|$)/); const session = await context.request .get("/api/auth/session") .then((response) => response.json());