Skip to content

fix(auth): recognize a comma-joined multi-role caller in canAssignRole#4918

Merged
pedrofrxncx merged 1 commit into
mainfrom
fix/canassignrole-multi-role-caller-w4
Jul 21, 2026
Merged

fix(auth): recognize a comma-joined multi-role caller in canAssignRole#4918
pedrofrxncx merged 1 commit into
mainfrom
fix/canassignrole-multi-role-caller-w4

Conversation

@pedrofrxncx

@pedrofrxncx pedrofrxncx commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Source: bug found while hardening apps/mesh/src/auth/roles.ts (Auth & role hardening sweep).

Why a maintainer wants this: Better Auth's organization plugin's parseRoles joins an assigned role array with "," before storing member.role (confirmed in the installed @decocms/better-auth dist: Array.isArray(roles) ? roles.join(",") : roles, used by both addMember and updateMemberRole). canAssignRole's caller-role check only handled callerRole === "owner" / === "admin", so a legitimate multi-role owner/admin — e.g. one whose member.role is stored as "owner,billing-manager" — was silently denied ANY role assignment, even though they hold owner/admin. This is the same multi-role-caller pattern the codebase already handles correctly elsewhere (see the comma-split fallback in builtin-role-permission.ts), just missing here.

Failure scenario: An org owner who also carries an extra custom role (assigned via ORGANIZATION_MEMBER_UPDATE_ROLE/ORGANIZATION_MEMBER_ADD with a multi-entry role array, which Better Auth stores comma-joined) calls ORGANIZATION_MEMBER_UPDATE_ROLE to promote another member. ctx.auth.user?.role for that owner is "owner,billing-manager", which fails the old === "owner" check, so canAssignRole incorrectly returns false and the tool throws "Insufficient privileges" for an actual owner.

Fix: split callerRole on "," and check membership (includes("owner") / includes("admin")) instead of exact equality — mirrors the existing multi-role handling pattern in builtin-role-permission.ts. Added a regression test (recognizes a multi-role caller from a comma-joined callerRole) covering multi-role owner, multi-role admin, an admin-with-extra-role still denied owner, and a caller with no owner/admin component still denied.

Reviewer command: bun test apps/mesh/src/auth/roles.test.ts

Local checks run: bun run fmt, cd apps/mesh && bunx tsc --noEmit (clean), bun test apps/mesh/src/auth/roles.test.ts (9 pass). Full CI validates the rest.


Summary by cubic

Fixes role assignment for multi‑role owners/admins when callerRole is comma-joined by @decocms/better-auth (e.g. owner,billing-manager). Owners/admins are now correctly recognized and no longer blocked.

  • Bug Fixes
    • Update canAssignRole to split callerRole on commas and check for owner/admin.
    • Preserve admin rules: admin can’t assign owner.
    • Add tests for comma-joined multi‑role callers.

Written for commit c3f6965. Summary will update on new commits.

Review in cubic

Better Auth's organization plugin joins an assigned role array with "," before
storing member.role (parseRoles in the org plugin), so a multi-role owner or
admin's OWN role can arrive at canAssignRole as e.g. "owner,billing-manager" —
not a lone "owner"/"admin". The previous strict === check silently denied
such a caller any role assignment, even though they legitimately hold owner
or admin. This mirrors the same comma-split handling already used for
multi-role callers in builtin-role-permission.ts.
@pedrofrxncx
pedrofrxncx merged commit 17f34a2 into main Jul 21, 2026
15 checks passed
@pedrofrxncx
pedrofrxncx deleted the fix/canassignrole-multi-role-caller-w4 branch July 21, 2026 01:09
decocms Bot pushed a commit that referenced this pull request Jul 21, 2026
PR: #4918 fix(auth): recognize a comma-joined multi-role caller in canAssignRole
Bump type: patch

- decocms (apps/mesh/package.json): 4.82.10 -> 4.82.11

Deploy-Scope: server
pedrofrxncx added a commit that referenced this pull request Jul 21, 2026
… bypass (#4923)

createBoundAuthClient's runtime admin/owner bypass (context-factory.ts) checked
`ADMIN_ROLES.includes(role)` with a strict equality match. Better Auth's
organization plugin joins an assigned role array with "," before storing
member.role, so a legitimate multi-role owner/admin (e.g.
"admin,billing-manager") arrives here as that comma-joined string, not a lone
"admin"/"owner". The exact-match check silently missed the bypass and fell
through to the narrower stored `permissions` (a studio JWT / MCP OAuth
payload), incorrectly denying an action the role actually grants.

Same failure mode as #4918 (canAssignRole's callerRole), now fixed at the
actual runtime authorization gate via a shared `hasAdminRole` helper in
auth/roles.ts.
pedrofrxncx added a commit that referenced this pull request Jul 21, 2026
… fast-path bypass (#4926)

AccessControl.checkMemberAccess checked `this.role === "admin" || this.role
=== "owner"` with a strict equality match. Better Auth's organization plugin
joins an assigned role array with "," before storing member.role (and
`setRole()` can set `this.role` to the path-resolved org's role, per its own
docstring), so a legitimate multi-role owner/admin (e.g.
"admin,billing-manager") arrives here as that comma-joined string. The
exact-match check misses it and falls through to `boundAuth.hasPermission`,
which — per builtin-role-permission.ts's own docstring — hits a DB-backed
Better Auth round-trip specifically to avoid ("~40ms event-loop hitches").
Same failure mode already fixed for `canAssignRole`'s caller check (#4918)
and the runtime bypass in context-factory.ts (#4923), now fixed here via the
shared `hasAdminRole` helper in auth/roles.ts.

Not a live authz bypass — the downstream Better Auth path still grants
correctly — but this closes the gap so AccessControl's own bypass is
self-contained and multi-role admins take the fast in-memory path instead of
an avoidable network round-trip on every tool call.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant