diff --git a/netbox/users/api/serializers_/tokens.py b/netbox/users/api/serializers_/tokens.py index ce467e1a6f5..1a140e90133 100644 --- a/netbox/users/api/serializers_/tokens.py +++ b/netbox/users/api/serializers_/tokens.py @@ -5,6 +5,7 @@ from netbox.api.fields import IPNetworkSerializer from netbox.api.serializers import ValidatedModelSerializer from users.models import Token +from users.utils import user_may_grant_token from .users import * @@ -50,9 +51,11 @@ def get_fields(self): def validate(self, data): # If the Token is being created on behalf of another user, enforce the grant_token permission. + # Use user_may_grant_token() rather than has_perm(obj=None): the latter short-circuits to True + # when the permission is present in cache without evaluating ObjectPermission constraints. request = self.context.get('request') token_user = data.get('user') - if token_user and token_user != request.user and not request.user.has_perm('users.grant_token'): + if token_user and token_user != request.user and not user_may_grant_token(request.user, token_user): raise PermissionDenied("This user does not have permission to create tokens for other users.") return super().validate(data) diff --git a/netbox/users/forms/model_forms.py b/netbox/users/forms/model_forms.py index b86eb7234c8..39539151595 100644 --- a/netbox/users/forms/model_forms.py +++ b/netbox/users/forms/model_forms.py @@ -18,6 +18,7 @@ from users.choices import TokenVersionChoices from users.constants import * from users.models import * +from users.utils import user_may_grant_token from utilities.data import flatten_dict from utilities.forms.fields import ( ContentTypeMultipleChoiceField, @@ -195,7 +196,7 @@ def clean(self): if request is None: # Fail closed: we cannot verify that the acting user is authorized. raise forms.ValidationError(_("Unable to verify permission to create tokens.")) - if token_user != request.user and not request.user.has_perm('users.grant_token'): + if token_user != request.user and not user_may_grant_token(request.user, token_user): raise forms.ValidationError( _("This user does not have permission to create tokens for other users.") ) diff --git a/netbox/users/tests/test_api.py b/netbox/users/tests/test_api.py index a50ce74bc1d..de57d869608 100644 --- a/netbox/users/tests/test_api.py +++ b/netbox/users/tests/test_api.py @@ -310,6 +310,109 @@ def test_provision_token_other_user(self): response = self.client.post(url, data, format='json', **self.header) self.assertEqual(response.status_code, 201) + def test_grant_token_constrained_permission_is_enforced(self): + """ + Regression: SR-001 / VM-322 — constrained grant_token ObjectPermissions must not be + bypassed. has_perm('users.grant_token', obj=None) short-circuits to True without + evaluating constraints; the fix uses user_may_grant_token() which applies them. + """ + # Clear the unconstrained grant_token added by setUp. + ObjectPermission.objects.filter(users=self.user, actions__contains=['grant']).delete() + self.add_permissions('users.add_token') + + superuser = User.objects.create_user(username='sec_superuser', is_superuser=True) + regular = User.objects.create_user(username='sec_regular') + + # Add a *constrained* grant_token permission: only tokens for non-superusers. + token_ct = ObjectType.objects.get_by_natural_key('users', 'token') + perm = ObjectPermission( + name='constrained_grant_token', + constraints={'user__is_superuser': False}, + actions=['grant'], + ) + perm.save() + perm.users.add(self.user) + perm.object_types.add(token_ct) + + url = reverse('users-api:token-list') + + # Attempt to create a token for the superuser — must be denied (constraint violation). + response = self.client.post(url, {'user': superuser.pk}, format='json', **self.header) + self.assertEqual( + response.status_code, 403, + "Constrained grant_token must deny token creation for users that violate the constraint", + ) + + # Attempt to create a token for the regular user — must succeed. + response = self.client.post(url, {'user': regular.pk}, format='json', **self.header) + self.assertEqual( + response.status_code, 201, + "Constrained grant_token must allow token creation for users that satisfy the constraint", + ) + + def test_grant_token_superuser_always_allowed(self): + """ + Superusers must be able to create tokens for any user without an explicit + grant_token ObjectPermission. Regression guard: _user_may_grant_token() must + mirror ObjectPermissionMixin.has_perm's superuser bypass. + """ + ObjectPermission.objects.filter(users=self.user, actions__contains=['grant']).delete() + self.add_permissions('users.add_token') + self.user.is_superuser = True + self.user.save() + try: + other = User.objects.create_user(username='superuser_grant_target') + url = reverse('users-api:token-list') + response = self.client.post(url, {'user': other.pk}, format='json', **self.header) + self.assertEqual(response.status_code, 201, "Superuser must be able to grant tokens for any user") + finally: + self.user.is_superuser = False + self.user.save() + + def test_grant_token_self_only_constraint(self): + """ + A {"user": "$user"} constraint means "only grant tokens for yourself". + Since creating a token for oneself bypasses the grant_token check entirely, + this constraint effectively blocks all cross-user grants for the holder. + Exercises the $user placeholder substitution path. + """ + ObjectPermission.objects.filter(users=self.user, actions__contains=['grant']).delete() + self.add_permissions('users.add_token') + token_ct = ObjectType.objects.get_by_natural_key('users', 'token') + perm = ObjectPermission(name='self_only_grant', constraints={'user': '$user'}, actions=['grant']) + perm.save() + perm.users.add(self.user) + perm.object_types.add(token_ct) + + other = User.objects.create_user(username='self_only_target') + url = reverse('users-api:token-list') + response = self.client.post(url, {'user': other.pk}, format='json', **self.header) + self.assertEqual(response.status_code, 403, "Self-only constraint must deny cross-user token grants") + + def test_grant_token_non_user_field_constraint_fails_closed(self): + """ + A constraint referencing a non-user Token field (e.g. {"write_enabled": True}) + cannot be evaluated for an unsaved token; _user_may_grant_token() must fail + closed and deny rather than bypass the constraint. + """ + ObjectPermission.objects.filter(users=self.user, actions__contains=['grant']).delete() + self.add_permissions('users.add_token') + token_ct = ObjectType.objects.get_by_natural_key('users', 'token') + perm = ObjectPermission( + name='non_user_field_grant', constraints={'write_enabled': True}, actions=['grant'] + ) + perm.save() + perm.users.add(self.user) + perm.object_types.add(token_ct) + + other = User.objects.create_user(username='non_user_field_target') + url = reverse('users-api:token-list') + response = self.client.post(url, {'user': other.pk}, format='json', **self.header) + self.assertEqual( + response.status_code, 403, + "Non-user Token field constraints must fail closed for new (unsaved) tokens", + ) + def test_create_token_returns_plaintext(self): """ Creating a Token via the REST API must return the usable plaintext value in the response. diff --git a/netbox/users/tests/test_views.py b/netbox/users/tests/test_views.py index 97a91ab797c..7eee5ff0e1a 100644 --- a/netbox/users/tests/test_views.py +++ b/netbox/users/tests/test_views.py @@ -463,6 +463,78 @@ def test_bulk_update_cannot_reassign_token_owner(self): self.assertEqual(token.user, owner) self.assertEqual(token.description, 'updated') + def _add_constrained_grant_permission(self, constraints): + """Add a constrained grant_token ObjectPermission to self.user.""" + token_ct = ObjectType.objects.get_by_natural_key('users', 'token') + perm = ObjectPermission( + name='constrained_grant_token', + constraints=constraints, + actions=['grant'], + ) + perm.save() + perm.users.add(self.user) + perm.object_types.add(token_ct) + + def test_create_token_via_form_constrained_grant_is_enforced(self): + """ + Regression: SR-001 / VM-322 — constrained grant_token ObjectPermissions must not + be bypassed via the UI create form. has_perm(obj=None) short-circuits to True + without evaluating constraints; user_may_grant_token() applies them correctly. + """ + superuser = User.objects.create_user(username='form_constrained_super', is_superuser=True) + regular = User.objects.create_user(username='form_constrained_regular') + self._add_constrained_grant_permission({'user__is_superuser': False}) + + # Constrained grant must deny token creation for a superuser. + response = self.client.post(reverse('users:token_add'), data={ + 'version': 2, + 'user': superuser.pk, + 'description': 'constrained denied', + 'enabled': 'on', + }) + self.assertEqual(response.status_code, 200) + self.assertFalse(Token.objects.filter(user=superuser).exists()) + + # Constrained grant must allow token creation for a non-superuser. + response = self.client.post(reverse('users:token_add'), data={ + 'version': 2, + 'user': regular.pk, + 'description': 'constrained allowed', + 'enabled': 'on', + }) + self.assertEqual(response.status_code, 302) + self.assertTrue(Token.objects.filter(description='constrained allowed', user=regular).exists()) + + def test_bulk_import_token_constrained_grant_is_enforced(self): + """ + Regression: SR-001 / VM-322 — constrained grant_token ObjectPermissions must not + be bypassed via bulk CSV import. has_perm(obj=None) short-circuits to True + without evaluating constraints; user_may_grant_token() applies them correctly. + """ + superuser = User.objects.create_user(username='bulk_constrained_super', is_superuser=True) + regular = User.objects.create_user(username='bulk_constrained_regular') + self._add_constrained_grant_permission({'user__is_superuser': False}) + + # Constrained grant must deny bulk import for a superuser. + csv_data = '\n'.join(('user,description', f"{superuser.pk},bulk denied")) + response = self.client.post(reverse('users:token_bulk_import'), data={ + 'data': csv_data, + 'format': ImportFormatChoices.CSV, + 'csv_delimiter': CSVDelimiterChoices.AUTO, + }) + self.assertEqual(response.status_code, 200) + self.assertFalse(Token.objects.filter(user=superuser).exists()) + + # Constrained grant must allow bulk import for a non-superuser. + csv_data = '\n'.join(('user,description', f"{regular.pk},bulk allowed")) + response = self.client.post(reverse('users:token_bulk_import'), data={ + 'data': csv_data, + 'format': ImportFormatChoices.CSV, + 'csv_delimiter': CSVDelimiterChoices.AUTO, + }) + self.assertEqual(response.status_code, 302) + self.assertTrue(Token.objects.filter(description='bulk allowed', user=regular).exists()) + class OwnerGroupTestCase(ViewTestCases.AdminModelViewTestCase): model = OwnerGroup diff --git a/netbox/users/utils.py b/netbox/users/utils.py index c355873a8d0..d585cfa0684 100644 --- a/netbox/users/utils.py +++ b/netbox/users/utils.py @@ -1,12 +1,73 @@ from django.conf import settings +from django.db.models import Q from social_core.storage import NO_ASCII_REGEX, NO_SPECIAL_REGEX __all__ = ( 'clean_username', 'get_current_pepper', + 'user_may_grant_token', ) +def user_may_grant_token(requesting_user, token_user): + """ + Return True if *requesting_user* has permission to create a token for *token_user*, + respecting ObjectPermission constraints on the users.grant_token permission. + + ``has_perm('users.grant_token', obj=None)`` always short-circuits to True when the + permission is present in the cache (obj=None bypasses constraint evaluation in + ObjectPermissionMixin.has_perm). Since the new token does not yet exist in the + database we cannot pass an existing Token as obj. Instead we extract the raw + constraint list, remap Token-field paths to User-field paths, and evaluate them + directly against the target User record — the only variable in a new token creation. + + Field remapping rules: + ``user__`` → ```` (FK traversal into User) + ``user`` → ``pk`` (Token.user FK becomes User.pk) + + Constraints referencing other Token fields cannot be evaluated for an unsaved + token; the function returns False (deny) for those. + """ + from users.models import User + + # Mirrors ObjectPermissionMixin.has_perm: superusers implicitly have all permissions. + if requesting_user.is_active and requesting_user.is_superuser: + return True + + perm = 'users.grant_token' + + # get_all_permissions() populates _object_perm_cache as a side effect. + # Guard against missing cache key in case a non-standard backend is in use. + if perm not in requesting_user.get_all_permissions(): + return False + + constraints = getattr(requesting_user, '_object_perm_cache', {}).get(perm, []) + + # An empty/null constraint means "no restriction" — allow any target. + if any(not c for c in constraints): + return True + + # Substitute the $user token so {"user": "$user"} resolves to the requesting user. + resolved_user_id = requesting_user.pk + + q = Q() + for constraint in constraints: + user_constraint = {} + for key, raw_val in constraint.items(): + val = resolved_user_id if raw_val == '$user' else raw_val + if key == 'user': + user_constraint['pk'] = val + elif key.startswith('user__'): + user_constraint[key.removeprefix('user__')] = val + else: + # Non-user Token field — cannot evaluate for a new (unsaved) token. + # Fail closed. + return False + q |= Q(**user_constraint) + + return User.objects.filter(q, pk=token_user.pk).exists() + + def clean_username(value): """Clean username removing any unsupported character""" value = NO_ASCII_REGEX.sub('', value) diff --git a/netbox/users/views.py b/netbox/users/views.py index 6c0f963a750..0fc140e9e21 100644 --- a/netbox/users/views.py +++ b/netbox/users/views.py @@ -16,6 +16,7 @@ ) from netbox.views import generic from users.ui import panels +from users.utils import user_may_grant_token from utilities.query import count_related from utilities.views import GetRelatedModelsMixin, register_model_view @@ -86,7 +87,7 @@ def save_object(self, object_form, request): # creation; TokenImportForm disables the user field on update, so an existing Token's owner cannot change. token_user = object_form.cleaned_data.get('user') if object_form.instance._state.adding and token_user and token_user != request.user \ - and not request.user.has_perm('users.grant_token'): + and not user_may_grant_token(request.user, token_user): raise ValidationError(_("This user does not have permission to create tokens for other users.")) return super().save_object(object_form, request)