[elcreator]

Follow-up to #2577, which was merged before the review of #2578 was addressed.

Brings in #2578 (against 3.5.x, still open). Role reach now compares the grants that authorize at runtime, on both sides:

• RoleGuard::heldBy(): the actor's own hasPermission() for every defined key and every key any role is granted, custom ones included;
• RoleGuard::reachOf(): the target's stored grants, always-on ones included, less only the obsolete role_actionok / action_ok the earlier seeders left;
• a role save stores only grants the actor holds, and assignment goes through RoleGuard::mayAssignRole() (save_role plus reach);
• the user form's role id is validated once (RoleGuard::postedRoleId(): "0" or the decimal id of an existing role) before anything is saved, and that same int is checked and stored. Before, the posted string reached setRole(), and MySQL/MariaDB in the installer's non-strict sql_mode could coerce it into a role that was never checked.

This replaces the earlier version merged with #2577, which compared only defined permissions. That let a role carrying a plugin key with no permissions row be saved, assigned or deleted by a manager who does not hold the key.

Role save, delete and assignment use the same helpers. 3.6 has no other role reach check.

Verification: merges into 3.6.x without conflicts. Full suite in evo-ci-php:8.4: 2739 passed, 12 skipped; UserRoleAssignmentTest also on MySQL 8.4. PHPStan: 0 errors.

🤖 Generated with Claude Code

[MiddleSokilAI]

I would hold the merge at 39f789d until the remaining role-authorization concern is resolved. This sync includes the same disabled/always-on permission exception discussed in the latest #2578 review. Please keep the reach comparison consistent with effective runtime grants for each operation, and cover varied delegated save/assign/delete permissions and existing roles with controller-level tests.

There is also a separate consistency issue in the assignment controller: the authorization check uses an integer-cast role ID, while the original input is passed on for persistence. That could make the checked and stored role differ depending on input handling and database coercion. Please strictly validate the role ID once and use that same canonical, checked value throughout authorization and persistence. Add a regression test proving that the role written is exactly the role whose permissions were checked, including rejection of invalid or ambiguous input.

These are static review observations. I have not reproduced either case at runtime, and the database-coercion consequences of the role-ID mismatch remain unverified.

SokilAI

[elcreator]

Thanks, both points are fixed on #2578 and merged here.

• Always-on exception: removed in fec047f (merged as 3e7e3b1). Role reach now leaves out only the obsolete role_actionok / action_ok. Each operation needs its own permission plus the reach check, and controller-level tests cover the save_role / delete_role combinations on fresh and upgraded installs. Details: Compare role reach by the grants that authorize at runtime #2578 (comment)
• Role id consistency: confirmed at runtime and fixed in 75dc25b (merged as b0e8949). In the installer's non-strict sql_mode, MySQL/MariaDB coerce the posted string setRole() received, so the stored role could differ from the checked one. The role id is now validated once (RoleGuard::postedRoleId(): "0" or the decimal id of an existing role), before anything is saved, and that same int is checked and stored. UserRoleAssignmentTest runs the real controller and UserSetRole on SQLite and MySQL/MariaDB. Details: Compare role reach by the grants that authorize at runtime #2578 (comment)

Full 3.6 suite in evo-ci-php:8.4: 2739 passed, 12 skipped. The new tests also pass on MySQL 8.4. PHPStan: 0 errors. The branch still merges into 3.6.x without conflicts.