Repository navigation
tech-debt(auth): "superadmin" is used at 17 require_role call sites but is not a member of the canonical Role enum #12786
Description
Activity
Triage note (no code change): confirmed this is owner-decision-gated, including the lint-gate bullet, so a future session does not re-derive it.
The third bullet ("a lint/test gate rejecting new
role == "admin"comparisons") looks decision-free but is not, because a gate that fails on existing code is unusable and the project prefers fixing over baselining. The current real sites are:autobot-backend/llc/api/companies.py:145 is_platform_admin flag OR role == "admin" autobot-backend/api/auth.py:286 current_user.get("admin") OR role == "admin" autobot-backend/api/ws_security.py:99 user.get("role") == "admin" autobot-backend/api/user_management/users.py:220 autobot-backend/api/user_management/dependencies.py:190 autobot-backend/services/mcp_dispatch.py:287 autobot-slm-backend/services/jwks_verifier.py:154 normalized["admin"] = role == "admin"Converting these to
is_admin_role()is the decision — each one changes who is authorized.jwks_verifier.py:154is the clearest: it derives theadminbool that flows into the SLM's own checks, so widening it there widens authorization across that backend. That is precisely the "no permission set is silently widened or narrowed" cross-backend check this issue asks for, and it needs an owner call rather than an inference.The decision needed: are
superadminandplatform_adminfirst-class members of the sharedRoleenum with explicitROLE_PERMISSIONSentries, or doessuperadminremain admin-equivalent-by-convention withauth_rbac.is_admin_role()(#12785) as the single containment point?Once that is settled, the site conversions and the lint gate are mechanical and can be done in one PR.
PR #12949 delivers the containment half of this (split to #12948). The enum decision stays here.
Delivered: 17 hand-rolled admin-role comparisons converted to
is_admin_role(), plus an AST guard that fails on any new one.Two findings worth recording:
A regex sweep undercounts this class by >3×. It showed 5 offenders; the AST guard found 17. The shapes that actually occur are
user_data["role"] == "admin",current_user.get("role") == "admin"and reversed operands — invisible to a regex keyed on variable names. Anyone re-auditing this should use the AST check, not grep.Two sites were not "grant superadmin too":
security_policy_manager._check_access_policycomparesrole == "admin"to require manager approval — a superadmin was escaping the approval rule. Same class, opposite polarity.services/mcp_dispatchblocked superadmin from admin-only tools at one site and served a reduced tool list at another — the silent-downgrade shape of security: voice_bundle_user._is_admin still admin-only — superadmin can't view others' bundles via GET /voice/users/{id}/bundle #12717, still present outside the voice paths that security: voice_bundle_user._is_admin still admin-only — superadmin can't view others' bundles via GET /voice/users/{id}/bundle #12717 fixed.
services/workflow_rbac.pyalso held its own_ADMIN_ROLES = {"admin"}— a fifth forked notion after the six in #12704 — now on the shared set.One trap for whoever takes the enum work.
ADMIN_ROLES/is_admin_rolehad to move out ofauth_rbacintoautobot_shared.auth.permissions:auth_rbacimportsauth_middleware, soauth_middlewareimporting back closes a cycle. My first cut did exactly that and module-import smoke went from 14/14 to 6/14 — the backend would not have started, and no unit test caught it. Expect the same hazard when wiring a newRolemember into both backends;auth_rbacre-exports the names so existing callers are unaffected.Still open here: adding
superadmin(andplatform_admin) to the sharedRoleenum with explicitROLE_PERMISSIONSentries, plus the cross-backend check that no permission set is silently widened or narrowed. That is an owner decision — the enum drives permissions for both backends.Also noted and deliberately out of scope:
autobot-slm-backend/services/jwks_verifier.pyderives itsadminclaim fromrole == "admin". It cannot import this helper (separate backend) and changing token claims has its own blast radius, so the guard scopes toautobot-backend.- added a commit that references this issue
on Jul 29, 2026 Delivered and merged as
774527f5b(PR #14936).- CI: all 10 required contexts present and green — checked by name, not by tally. 58 success / 3 skipped / 0 failures / 0 pending, deduped to the latest run per name.
- Review: independent security-focused
code-reviewerpass. Every claim verified against source; it could not falsify any.
superadmindid not resolve two ways — it resolved four.role_has_permissionshort-circuited onis_admin_role, which madeADMIN_ROLES— documented as a predicate — silently the most permissive permission source in the system, whileSecurityLayer.check_permissiondenied it those same permissions.Role("superadmin")raised outright, andrequire_roleadmitted it by raw string. Four answers for one input.The decision:
Role.SUPERADMINwith an explicitly EMPTY permission entry. Verified in base:Role.SUPERADMIN: [],
The alternative was +54 permissions including
allow_shell_execute, and it is recorded in the source as a one-line change flagged as needing its own security review and a data migration — which an empty entry does not. That is the security carve-out to the union doctrine: union the checks, never the allowances.Review confirmed nothing grants it implicitly —
_build_system_roles()yields a safepermissions: []seed row,auth_rbac._get_user_permissionsresolves the member but still gets[],SecurityLayer._get_default_role_permissionshas nosuperadminkey, andRoleUpdateRequest's hand-written regex keeps it non-assignable via that API. The 17require_rolecall sites are unchanged and still admit it.The one behaviour change is a tightening, stated rather than buried. Removing the short-circuits refuses superadmin at two granular-permission gates.
adminis unaffected and that is measured:Permissionhas exactly 54 members andROLE_PERMISSIONS[Role.ADMIN]lists exactly those 54, now asserted continuously. A second tightening ships with zero lines changed in its file — SLM's_resolve_rolepreviously downgraded an unknownsuperadmintoRole.USER, i.e. granted it USER's 15 permissions.Two deliberate forks were found and deliberately NOT collapsed. #14024's own body says its three vocabularies are correctly separate concepts, and their member sets confirm three different questions. And
write_guard's{platform_admin, superadmin}excludesadminon purpose — the guard exists to stop an org admin reaching a parent org's KB, so folding it intoADMIN_ROLESwould have addedadminand destroyed the property. A drift guard now compares member sets and pins the exact known overlaps.A miss worth recording. Three tests asserting the old contract survived the first sweep, because that sweep grepped
Role/ROLE_PERMISSIONSsymbols while those files namesuperadminas a bare string through_would_deny/_role_satisfies— searching the symbol rather than the behaviour, which is core rule 7 and the exact trap #14024 exists over. Re-swept on the concept: 21 files, 2 genuinely broken, 16 exercising unchanged admission paths, 3 incidental. The shadow test's intent was preserved rather than flipped — its purpose was that an administrative role must never be reported as an unrecognised string, so that is now asserted directly instead of inferred fromis None.All 12
python-suiteshards pass, including shard 4 — which resolves the open caveat that the other eleven had been cancelled rather than passing on the prior head, so more failures of that class could have been hiding. There were none.Residuals filed and cross-linked: #14937, #14938, #14939, #14940, #14944.
Discovered while fixing #12717 (PR #12785). That PR fixes the four affected voice call sites; this tracks the structural cause, which is a bigger blast radius and deliberately out of its scope.
The gap
autobot_shared/auth/permissions.py:109defines the canonical role enum:There is no
SUPERADMIN. Yetrequire_role("admin", "superadmin")appears at 17 call sites acrossautobot-backend(mcp_token_admin, admin_schedulers, admin_event_logs, voice_bundle_user, admin_pricing, chat_shared_links, admin_retention_policies, voice_bundle_admin, …). It works only becauserequire_role(*roles: Role | str)accepts raw strings and lowercases both sides.Why this keeps producing bugs
The role is real at runtime but invisible to the type system and to
ROLE_PERMISSIONS. That splits authorization into two populations that silently disagree:require_role(...)guard admits superadmin;role == "admin"check rejects it.Wherever a router has both, a superadmin can perform the write but not the read. That has now produced two issues:
_require_admindependencies diverged on the admin role.voice_bundle_user._is_admindid the same, plus two sites where a superadmin was silently downgraded rather than denied (reduced tool count; non-admin realtime bridge).PR #12785 adds
auth_rbac.ADMIN_ROLES/is_admin_role()so imperative checks have one correct answer, but that is a containment measure. As long assuperadminis a magic string, a newrole == "admin"check reintroduces the bug and nothing catches it.Also worth reconciling
autobot-backend/llc/kb/write_guard.py:23defines a third notion:so
platform_adminis superadmin-equivalent there but nowhere else.api/auth.py:286andllc/api/companies.py:145use yet another shape (is_platform_adminflag ORrole == "admin").Requested
superadmin(andplatform_admin) are first-class roles. If so, add them to the sharedRoleenum with explicitROLE_PERMISSIONSentries.auth_rbac.pyand the slm-backend equivalent), the change needs a cross-backend check that no permission set is silently widened or narrowed.role == "admin"string comparisons in favour ofis_admin_role()/require_role(), so the class cannot come back.Refs #12704, #12717, #12645