[elcreator]

Raised in the review of #2577; the reach checks come from 3.5.x (46ece99), so 3.5 needs the fix too.

Problem

Saving, deleting or assigning a role compared all of the target role's stored grants with the permission keys the actor holds. The actor side lists only defined permissions, so a target grant that no permission defines could never match:

• Obsolete grant: sites installed with the earlier default seeders still give every default role role_actionok. The seeders no longer grant it (del(seeders): drop the dead action_ok permission and its dangling role grants #2570), but no update removes the existing rows. A role manager holding every real permission of such a role was therefore refused editing, deleting or assigning it.
• Always-on permission: a role save stores every always-on (disabled) permission on the role. On those sites that includes action_ok, so a target saved recently carried it while the actor's role, saved before, did not. That also refused the actor.

Fix

Both sides of the comparison follow the grants that authorize at runtime. The session holds every stored grant of a role, defined by a permissions row or not, always-on ones included.

• RoleGuard::heldBy($hasPermission): the actor's own hasPermission(), asked for every defined key and every key any role is granted.
• RoleGuard::reachOf($roleId): the target's stored grants, less only the obsolete keys the earlier seeders left (RoleGuard::OBSOLETE: role_actionok, action_ok, which nothing checks).
• Role save stores, for a non-administrator, only grants the actor holds, always-on ones included.
• Each operation needs its own permission plus the reach check: save_role to save or assign (RoleGuard::mayAssignRole(), used by EditOrNewUser), delete_role to delete. The administrator role stays out of reach for non-administrators.
• Role id: EditOrNewUser checked (int) $_POST['role'] but stored the posted string, which MySQL/MariaDB in the installer's non-strict sql_mode coerce into another role id. RoleGuard::postedRoleId() now accepts only "0" or the decimal id of an existing role, validated once before anything is saved; the same int is checked and stored. New lexicon key user_role_invalid.

Tests

RoleReachLegacyGrantsTest runs the real role save and delete and mayAssignRole(), every case checking all three operations:

• fresh and upgraded installs × save_role/delete_role combinations: each operation goes through exactly when the actor holds its permission;
• a role whose effective grants exceed the actor (an always-on grant the actor's role lacks, settings, a defined custom permission, a custom grant with no row): refused under every combination;
• custom grants the actor holds: allowed; a save stores only held grants.

UserRoleAssignmentTest runs the real EditOrNewUser::process() and UserSetRole on SQLite and, with EVO_TEST_MYSQL_HOST, MySQL/MariaDB non-strict: the stored role is the role checked; invalid or ambiguous ids are refused before anything is written.

Full suite in evo-ci-php:8.4: 2535 passed, 12 skipped; the new tests also on MySQL 8.4 and MariaDB 11.4.

🤖 Generated with Claude Code

[MiddleSokilAI]

Thanks for addressing the legacy-role regression. I would still hold the merge at adb4e31: the permission filtering used for role reach is not fully aligned with the effective permissions established at login and used at runtime. A grant omitted from the reach comparison can still matter to runtime authorization, so the delegated role-management boundary is not yet consistently enforced.

Please align the actor and target reach checks with the effective runtime grants, while preserving support for custom permissions and safely handling genuinely obsolete grants. The same rules should apply to role editing, deletion and assignment.

Please add regression tests covering fresh and upgraded installations, legacy roles and custom permissions. They should verify both legitimate delegated operations and rejection when the target's effective permissions exceed the actor's permissions.

This is a static review finding; I have not reproduced it on a running system.

SokilAI

[elcreator]

Thanks, confirmed at runtime and fixed in 044ecd3.

Confirmed: at login the session receives every stored grant of the role (StoreContextService::buildManagerPermissions()), whether or not a permissions row defines it. A plugin key that only a grant carries therefore authorizes like a core permission. adb4e31 compared only grants of defined permissions: on the actor side it asked about defined keys only, and on the target side it dropped undefined grants. So a role holding such a key (shop_refunds in the test) could be saved, assigned and deleted by a role manager who does not hold it. The new test shows all three allowed on adb4e31.

Fix: both sides now follow the runtime grants, through two helpers that role save, delete and assignment all use:

• RoleGuard::heldBy($hasPermission): the actor's own hasPermission(), the check runtime authorization makes, asked for every defined key and every key any role is granted. Custom keys the actor holds count, defined or not.
• RoleGuard::reachOf($roleId): the target's stored grants (grantsOf()), less two kinds that grant nothing an actor could lack:
  • genuinely obsolete keys, listed explicitly in RoleGuard::OBSOLETE: role_actionok (the lang key the earlier seeders granted in place of action_ok) and action_ok, which nothing checks;
  • always-on (disabled) permissions: grantablePermissions() stores them on every role any role manager saves, so the actor can already confer them, and counting them would only block managers whose own role was saved before one was added.

Anything else a role is granted, including any custom key, counts.

Regression tests (RoleReachLegacyGrantsTest, the real controller paths for save and delete, and the assignment check EditOrNewUser makes). Every case checks all three operations:

• fresh install and upgraded install (role_actionok and always-on action_ok on the role): a role manager holding every real permission of the default Editor role may manage it;
• both installs: refused when the actor lacks one of the role's real permissions;
• a custom permission with a permissions row and one that only a grant carries: allowed when the actor holds it, refused when it does not;
• a role holding a stronger core permission (settings): refused. The administrator role stays out of reach;
• reachOf() leaves out exactly the obsolete and always-on keys.

Full suite in evo-ci-php:8.4: 2482 passed, 12 skipped.

[MiddleSokilAI]

Thanks, 044ecd3 addresses the custom-permission case. One remaining concern is the blanket exclusion of disabled/always-on permissions from the target's reach.

A stored grant can still be effective at runtime even when the acting manager does not hold it. The fact that saving a role adds these grants does not establish that every actor allowed to assign an existing role is also allowed to save roles. With different delegated permission combinations, that exception may therefore cross the intended authorization boundary.

Before merging, please make this exception consistent with the effective grants and permissions required by each operation. Add controller-level regression tests for varied save/assign/delete permission combinations and existing legacy roles, checking both legitimate operations and rejection when the target's effective grants exceed the actor's authority.

This is a static review finding; I have not reproduced this remaining case at runtime.

SokilAI

[elcreator]

Thanks, confirmed and fixed in fec047f.

Confirmed: "a role save stores always-on keys anyway" only covers a manager holding save_role. A manager allowed to delete roles (delete_role only) could delete a role whose always-on grants it lacks, and those grants authorize at runtime like any other. The new tests reproduce it: with 044ecd3's reachOf(), a role holding home is assignable, savable and deletable by a manager whose role lacks it, under every permission combination.

Fix: no blanket exception remains.

• reachOf() leaves out only the obsolete keys in RoleGuard::OBSOLETE (role_actionok, action_ok). Every other stored grant counts, always-on and custom ones included.
• Role save: a non-administrator stores on a role only grants it holds, always-on ones included. A re-saved legacy role also loses role_actionok.
• Each operation keeps its own permission plus the same reach check: save_role to save, delete_role to delete. Assignment needs save_role plus reach, and that decision moved from EditOrNewUser into RoleGuard::mayAssignRole(), so the tests run the exact rule the controller uses.

Every default role is seeded with all five always-on permissions, so this changes nothing on a standard site. A manager whose role predates a new always-on permission picks it up by saving its own role, which is always within its reach.

Controller-level regression tests (RoleReachLegacyGrantsTest): the real role save (updateOrCreate() through process()) and delete, and mayAssignRole(). Every case checks all three operations:

• fresh and upgraded installs (the role carries role_actionok and an always-on action_ok) × save_role+delete_role / save_role only / delete_role only / neither. Each operation goes through exactly when the actor holds its permission;
• save_role+delete_role / save_role only / delete_role only, against a role whose effective grants exceed the actor: an always-on grant the actor's role lacks, settings, a defined custom permission, and a custom grant with no permissions row. All refused, and the role is unchanged;
• custom grants the actor holds: allowed;
• a save stores only held grants (no settings, no unheld custom key, no role_actionok); the administrator role stays out of reach.

Full suite in evo-ci-php:8.4: 2496 passed, 12 skipped.

[MiddleSokilAI]

Thanks, fec047f addresses the custom-permission and always-on grant issues. I would still hold the merge for the separate role-ID consistency issue reported on #2579, which remains in this assignment path: authorization checks an integer-cast role ID, while persistence receives the original value.

Please validate the role ID once and use the same canonical, checked value for both authorization and persistence. Add a controller-level regression test that rejects invalid or ambiguous input and asserts that the persisted role ID is exactly the one whose permissions were checked.

This is a static finding, not a runtime reproduction. Any consequence involving database coercion remains unverified.

SokilAI

[elcreator]

Thanks, confirmed at runtime and fixed in 75dc25b.

Confirmed: EditOrNewUser checked the reach of (int) $_POST['role'] but passed the posted string on to setRole(). The installer configures the connection with 'strict' => false. In that mode, MySQL 8.4 and MariaDB 11.4 coerce a non-canonical string written to the INT column ('2abc' → 2), and a fractional one is rounded rather than truncated. So the stored role can be one whose grants were never checked, the administrator role included. With fec047f's controller, the new test reproduces that on MySQL; SQLite does not coerce, which is why the suite never showed it. 3.5.x and 3.6.x have the same code.

Fix:

• RoleGuard::postedRoleId() accepts only what the form writes: "0" (no role) or the decimal id of an existing role. Anything else is refused, not cast: a fraction, a suffix, surrounding whitespace, a leading zero or sign, an exponent, hex, an empty value, an array, or a role that does not exist.
• EditOrNewUser validates the role once, before anything is saved. An invalid value stops the request with an alert (user_role_invalid, new lexicon key in every locale), and no user is created or changed.
• The validated int replaces $userData['role'], so the TV lookup, mayAssignRole() and setRole() all receive that same value. The "role changed" test compares ints.

Controller-level regression tests (UserRoleAssignmentTest): they run the real EditOrNewUser::process() and the real UserSetRole in their own process. The backends are SQLite, plus MySQL/MariaDB in the installer's non-strict mode when EVO_TEST_MYSQL_HOST is set.

• Valid assignments: no role for another user or for itself, a role within reach, a role out of reach (left unchanged), the administrator role (refused), and a new user's role. Each asserts that the role stored is the role id whose grants were read for the check and whose TVs were loaded.
• 12 invalid or ambiguous inputs, on editing another user, editing itself and creating a user: each is refused before anything is written, and no grants or TVs are read.
• postedRoleId() unit cases for accepted and refused values, strings and non-strings.

Full suite in evo-ci-php:8.4: 2535 passed, 12 skipped. The new tests also pass on MySQL 8.4 and MariaDB 11.4. PHPStan: 0 errors.