Repository navigation
tech-debt(auth): three 'role' vocabularies share the literals 'user' and 'admin' with no type-level separation #14024
Description
Activity
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.
Three unrelated vocabularies use the word "role" and share string values, with nothing at the type level keeping them apart. This already produced a near-miss during #13934.
The three
autobot_shared/auth/permissions.py::Roleautobot-backend/llc/models/enums.py::MembershipRoleautobot_shared/ssot_constants.py::CategoryDefaults.ROLE_*Member-set comparison (names differ, so a same-name search finds nothing):
These are correctly separate concepts — this is not a request to merge them. The problem is that
"user"and"admin"appear in more than one, so a value from the wrong vocabulary type-checks, passes tests, and reads correctly at a glance.The near-miss
tools/tool_registry.py:87had"user_role": "user"— an RBAC input, read byworker_node._validate_user_roleand_check_task_permission.The hardcoded-value guard reported it and suggested
CategoryDefaults.ROLE_USER, i.e. the chat vocabulary. Both hold"user"today, so applying the suggestion would have tied an authorization decision to a presentation constant, gone green, and been invisible in review. It was caught only because the field name was traced to its consumer before the fix was applied (#13934).Any future rename or re-tuning of the chat vocabulary would then have silently changed an authz value.
What would help
Not a merge — a distinction that survives a copy-paste:
user_role—MembershipRoleandauth.Roleare alreadystr, Enum;CategoryDefaultsis a bare string constant class, which is the one that mixes freely.Acceptance criteria
Related