mirror of
https://github.com/projectsend/projectsend.git
synced 2026-09-21 02:53:24 +00:00
denkfabrik-li
706ebf6166
StaffAccounts opens with the rule: "Nobody hands out authority they do not hold ... that turns one permission into every permission and makes the rest of the matrix decorative." mayGrant() enforces it for a role's permissions, guardTarget() applies the same test to an existing account, and RolesController::guardGrantablePermissions() names the attack in full -- a non-administrator holding manage_users minting a role that carries more than they do, and then holding it. A role carries one more thing, and it is the larger one. `client_scoped` decides whether the role reaches the clients assigned to its holder or the whole library, which is the boundary StaffLibraryScope, ActivityLogScope and every listing in the application are built around. Nothing weighed it. store() and update() wrote the flag straight from the request, and mayGrant() looked only at permissions -- so `manage_users` on a client-scoped role was enough to take the limit off that role and keep working, or to mint a role without one and move into it. Either way the next request read the whole library, and the `assigned_clients` roster that #1697 protects stopped meaning anything for that account. Both halves of the existing pair get the missing clause: - guardScopeRemoval() in RolesController refuses a client-scoped actor who creates a role without the limit, or takes the limit off one that has it. Phrased as "removes the limit" rather than "is not limited", so only what this request changes is weighed -- the same reasoning guardGrantablePermissions() gives for looking at the diff. Editing an already-unlimited role's permissions is not this actor lifting a limit. Both writers resolve the flag with Request::boolean() and hand that same value to the guard and to the write: the `boolean` validation rule accepts "0" and 0 as well as false and validates without casting, so reading the validated array and comparing it strictly would leave this guard and the model's own `boolean` cast disagreeing about one value -- which is the shape the guard exists to prevent. - mayGrant() refuses a client-scoped actor granting a role that is not client-scoped, which closes assigning an existing one. It reaches both surfaces at once: assignableRoleIds() validates role_id on the web and API staff forms and on the account converter, assignableRoles() fills the pickers, and guardTarget() covers the account itself. Administrators are unaffected -- mayGrant() returns early for them, and an administrator role is never client-scoped. Unscoped staff are unaffected: the clause is conditioned on the actor's own scope, so a non-administrator with manage_users and no limit creates, edits and grants exactly as before. The seeded roles are untouched; update() already refused to move the flag on a system role, which is why the stock Client Manager was never the way in. Two changes a client-scoped holder of manage_users will notice, both following from mayGrant(): - the role picker on the staff form and the account converter now offers only client-scoped roles, rather than offering one the request behind it would refuse; - editing or deleting a staff account whose role is not client-scoped now answers 403, through guardTarget(), on the same "if you could not grant their role you have no business editing that account" rule that already applied to permissions. The roles API is read-only (GET /roles is the whole surface), so this half has no API twin to mirror; the account half is covered above.