Closes #22429: Enforce ObjectPermission constraints on grant_token

[bctiemann]

Closes: #22429

Summary

Fixes a privilege escalation where constrained grant_token ObjectPermissions were silently bypassed in the REST API (SR-001 / VM-322, CWE-863).

Root cause

TokenSerializer.validate() called request.user.has_perm('users.grant_token') with no obj argument. In ObjectPermissionMixin.has_perm, obj=None causes an unconditional early return of True once the permission is found in cache — constraints are never evaluated.

An operator holding a constrained grant_token ObjectPermission (e.g. {"user__is_superuser": False}) could supply any user in a token-creation request and the constraint would never fire, allowing tokens to be minted for privileged users.

Fix

Replace has_perm(obj=None) with a new helper _user_may_grant_token(requesting_user, token_user) that:

1. Returns True immediately for superusers (mirrors ObjectPermissionMixin.has_perm's bypass).
2. Confirms the requesting user holds users.grant_token at all.
3. Treats an empty/null constraint as unconstrained (preserves existing semantics).
4. For constrained permissions, remaps Token-model constraint keys to User-model keys (user__<field> → <field>, user → pk) and evaluates them directly against the target User record — the only variable in a new-token creation scenario. Non-user Token fields are treated as fail-closed (deny).

We cannot use has_perm(obj=token_instance) with an existing Token because the token does not exist yet, and passing an unsaved instance returns empty from model.objects.filter(pk=None).

Tests

• test_grant_token_constrained_permission_is_enforced — {"user__is_superuser": False} denies superuser target, allows non-superuser target.
• test_grant_token_superuser_always_allowed — superuser can grant tokens without an explicit ObjectPermission.
• test_grant_token_self_only_constraint — {"user": "$user"} blocks cross-user grants.
• test_grant_token_non_user_field_constraint_fails_closed — non-user Token field constraints fail closed for unsaved tokens.

Ref: SR-001 / VM-322 (internal security review, not an externally-reported disclosure)

🤖 Generated with Claude Code

[bctiemann]

@claude /review

[github-actions]

Claude finished @bctiemann's task in 3m 47s —— View job

---

Code Review: Security: enforce ObjectPermission constraints on grant_token

• Read changed files
• Review serializer implementation
• Review test coverage
• Check authentication module behavior
• Post final review

---

Overview

This PR correctly identifies and addresses a real privilege escalation: has_perm('users.grant_token', obj=None) short-circuits to True in ObjectPermissionMixin.has_perm (line 137–138 of authentication/__init__.py) the moment the permission exists in cache, without ever evaluating constraints. The approach of extracting raw constraints from _object_perm_cache and remapping Token-field paths to User-field paths is sound given the "no saved token yet" constraint.

---

Critical Issue: Superuser Regression

_user_may_grant_token() will deny superusers who lack an explicit grant_token ObjectPermission.

The root cause: requesting_user.get_all_permissions() (Django's User.get_all_permissions()) aggregates results from all auth backends by unioning permission strings. ObjectPermissionMixin.get_all_permissions() returns _object_perm_cache populated from ObjectPermission records only — it has no superuser bypass. That bypass lives exclusively in has_perm() (lines 117–119 of authentication/__init__.py):

# ObjectPermissionMixin.has_perm — has the bypass
if user_obj.is_active and user_obj.is_superuser:
    return True

Since users.grant_token is a custom action (not view/add/change/delete), ModelBackend.get_all_permissions() will never return it either. So for a superuser without an explicit grant_token ObjectPermission:

• 'users.grant_token' not in requesting_user.get_all_permissions() → True
• _user_may_grant_token() → False → 403

The original has_perm('users.grant_token') returned True for these users. This is a regression.

Fix: mirror ObjectPermissionMixin.has_perm's superuser check at the top of the helper:

def _user_may_grant_token(requesting_user, token_user):
    # Mirrors ObjectPermissionMixin.has_perm: superusers implicitly have all permissions.
    if requesting_user.is_active and requesting_user.is_superuser:
        return True
    ...

Fix this →

---

Medium Issues

1. Brittle private API coupling (_object_perm_cache)

tokens.py:43 accesses requesting_user._object_perm_cache[perm] directly — a private implementation detail of ObjectPermissionMixin. While _object_perm_cache is a defaultdict(list) so a missing key returns [] rather than KeyError, the coupling is fragile. If the cache structure changes (e.g., renamed, or populated lazily by a different backend), this silently breaks.

The safer approach would be to use the already-public qs_filter_from_constraints() utility from utilities/permissions.py, or at minimum guard with getattr(requesting_user, '_object_perm_cache', {}).get(perm, []).

2. hasattr(val, 'pk') appears to be dead code

tokens.py:59–60:

if hasattr(val, 'pk'):
    val = val.pk

Constraints are stored as JSON, so values are always primitives (strings, booleans, integers). The $user substitution above maps the string '$user' to requesting_user.pk (an integer). There is no code path where val would have a .pk attribute at this point. This should either be removed or documented.

---

Test Coverage Gaps

The regression test for the constrained permission case is correct and meaningful. However:

1. No test for superuser as requesting user. A superuser calling POST /api/users/tokens/ with another user's pk should succeed (200/201). This test would catch the regression described above.

2. No test for {"user": "$user"} constraint. This maps to "only grant tokens for yourself" — a plausible real-world constraint. It exercises the $user placeholder substitution path.

3. No test for non-user Token field fail-closed behaviour. A constraint like {"write_enabled": True} cannot be evaluated for an unsaved token; the function returns False. Worth an explicit test to guard against regressions.

---

Minor: Constraint Value Path

tokens.py:64–65 uses key[len('user__'):] which is equivalent to key[6:] or key.removeprefix('user__'). The current form is fine but key.removeprefix('user__') would be slightly more readable (available since Python 3.9, which NetBox supports).

---

Summary

The vulnerability identification and the constraint-remapping approach are correct. The implementation has one critical regression (superusers blocked from granting tokens), a fragile dependency on a private cache attribute, and missing test coverage for the superuser and edge-case paths. The superuser issue must be addressed before merging; the others are worth fixing but not blocking.

[arthanson]

This works bug Claude is saying the same has_perm('users.grant_token')-with-no-obj pattern exists at two other token-creation sites, and both have the identical privilege-escalation bug the PR is fixing:

• users/views.py:89 - TokenBulkImportView.save_object() (bulk CSV import)
• users/forms/model_forms.py:198 - TokenForm.clean() (web UI create form)

An operator with a constrained grant_token (e.g. {"user__is_superuser": False}) can still mint a token for a superuser through the UI or bulk import, exactly as described in #22429 - just not through the REST API anymore.