fix(auth): Support proxy models in Object Permission checks
Replace ValueError with graceful permission denial when checking permissions against proxy models or invalid model references. Evaluate constraints via the permission model's manager and log warnings for nonexistent models or debug messages for model mismatches. Fixes #22632
This commit is contained in:
parent
46de424447
commit
3561de3d56
|
|
@ -1,13 +1,13 @@
|
|||
import logging
|
||||
from collections import defaultdict
|
||||
|
||||
from django.apps import apps
|
||||
from django.conf import settings
|
||||
from django.contrib.auth.backends import ModelBackend
|
||||
from django.contrib.auth.backends import RemoteUserBackend as _RemoteUserBackend
|
||||
from django.contrib.auth.models import AnonymousUser
|
||||
from django.core.exceptions import ImproperlyConfigured
|
||||
from django.db.models import Q
|
||||
from django.utils.translation import gettext_lazy as _
|
||||
|
||||
from users.constants import CONSTRAINT_TOKEN_USER
|
||||
from users.models import Group, ObjectPermission, User
|
||||
|
|
@ -137,12 +137,18 @@ class ObjectPermissionMixin:
|
|||
if obj is None:
|
||||
return True
|
||||
|
||||
# Sanity check: Ensure that the requested permission applies to the specified object
|
||||
model = obj._meta.concrete_model
|
||||
if model._meta.label_lower != '.'.join((app_label, model_name)):
|
||||
raise ValueError(_("Invalid permission {permission} for model {model}").format(
|
||||
permission=perm, model=model
|
||||
))
|
||||
# Sanity check: the permission must apply to the object's model. Permissions may name proxy
|
||||
# models, so compare concrete models and evaluate constraints via the permission model's manager.
|
||||
try:
|
||||
permission_model = apps.get_model(app_label, model_name)
|
||||
except LookupError:
|
||||
logger = logging.getLogger('netbox.auth.ObjectPermissionBackend')
|
||||
logger.warning(f"Permission {perm} does not reference a valid model")
|
||||
return False
|
||||
if permission_model._meta.concrete_model is not obj._meta.concrete_model:
|
||||
logger = logging.getLogger('netbox.auth.ObjectPermissionBackend')
|
||||
logger.debug(f"Permission {perm} is not valid for {obj._meta.label_lower} objects")
|
||||
return False
|
||||
|
||||
# Compile a QuerySet filter that matches all instances of the specified model
|
||||
tokens = {
|
||||
|
|
@ -153,7 +159,7 @@ class ObjectPermissionMixin:
|
|||
# Permission to perform the requested action on the object depends on whether the specified object matches
|
||||
# the specified constraints. Note that this check is made against the *database* record representing the object,
|
||||
# not the instance itself.
|
||||
return model.objects.filter(qs_filter, pk=obj.pk).exists()
|
||||
return permission_model.objects.filter(qs_filter, pk=obj.pk).exists()
|
||||
|
||||
|
||||
class ObjectPermissionBackend(ObjectPermissionMixin, ModelBackend):
|
||||
|
|
|
|||
|
|
@ -9,8 +9,10 @@ from django.urls import reverse
|
|||
from rest_framework.test import APIClient
|
||||
from social_core.exceptions import AuthFailed
|
||||
|
||||
from core.models import ObjectType
|
||||
from core.choices import ManagedFileRootPathChoices
|
||||
from core.models import ManagedFile, ObjectType
|
||||
from dcim.models import Rack, Site
|
||||
from extras.models import ScriptModule
|
||||
from netbox.authentication.misc import _mirror_groups
|
||||
from netbox.middleware import SocialAuthExceptionMiddleware
|
||||
from users.constants import TOKEN_PREFIX
|
||||
|
|
@ -745,6 +747,54 @@ class ObjectPermissionAPIViewTestCase(TestCase):
|
|||
self.assertEqual(response.status_code, 204)
|
||||
|
||||
|
||||
class ObjectPermissionProxyModelTestCase(TestCase):
|
||||
"""
|
||||
Object-level permission checks against proxy models (e.g. extras.ScriptModule proxying
|
||||
core.ManagedFile) must evaluate the permission rather than raise ValueError.
|
||||
"""
|
||||
@classmethod
|
||||
def setUpTestData(cls):
|
||||
cls.managed_file = ManagedFile.objects.create(
|
||||
file_root=ManagedFileRootPathChoices.SCRIPTS,
|
||||
file_path='proxy_permission_test.py'
|
||||
)
|
||||
cls.script_module = ScriptModule.objects.get(pk=cls.managed_file.pk)
|
||||
|
||||
def _grant_scriptmodule_permission(self, constraints=None):
|
||||
obj_perm = ObjectPermission(name='ScriptModule change', actions=['change'], constraints=constraints)
|
||||
obj_perm.save()
|
||||
obj_perm.users.add(self.user)
|
||||
obj_perm.object_types.add(ObjectType.objects.get_for_model(ScriptModule, for_concrete_model=False))
|
||||
|
||||
def test_has_perm_cross_app_proxy_model(self):
|
||||
"""An unconstrained proxy-model permission grants access to a proxy instance."""
|
||||
self._grant_scriptmodule_permission()
|
||||
self.assertTrue(self.user.has_perm('extras.change_scriptmodule', self.script_module))
|
||||
|
||||
def test_has_perm_cross_app_proxy_model_matching_constraints(self):
|
||||
"""A constrained proxy-model permission grants access when the instance matches."""
|
||||
self._grant_scriptmodule_permission(constraints={'file_path': 'proxy_permission_test.py'})
|
||||
self.assertTrue(self.user.has_perm('extras.change_scriptmodule', self.script_module))
|
||||
|
||||
def test_has_perm_cross_app_proxy_model_nonmatching_constraints(self):
|
||||
"""A constrained proxy-model permission denies access when the instance does not match."""
|
||||
self._grant_scriptmodule_permission(constraints={'file_path': 'other.py'})
|
||||
self.assertFalse(self.user.has_perm('extras.change_scriptmodule', self.script_module))
|
||||
|
||||
def test_has_perm_invalid_permission_object_pair(self):
|
||||
"""A permission checked against an object of an unrelated model denies instead of raising."""
|
||||
site = Site.objects.create(name='Proxy Test Site', slug='proxy-test-site')
|
||||
self._grant_scriptmodule_permission()
|
||||
self.assertFalse(self.user.has_perm('extras.change_scriptmodule', site))
|
||||
|
||||
@override_settings(DEFAULT_PERMISSIONS={'extras.change_nosuchmodel': None})
|
||||
def test_has_perm_unknown_model_permission(self):
|
||||
"""A permission naming a nonexistent model denies and logs a warning instead of raising."""
|
||||
self._grant_scriptmodule_permission()
|
||||
with self.assertLogs('netbox.auth.ObjectPermissionBackend', level='WARNING'):
|
||||
self.assertFalse(self.user.has_perm('extras.change_nosuchmodel', self.script_module))
|
||||
|
||||
|
||||
class SocialAuthExceptionMiddlewareTestCase(SimpleTestCase):
|
||||
"""
|
||||
Verify that SSO/SAML authentication failures are surfaced as a login-page message rather than
|
||||
|
|
|
|||
Loading…
Reference in New Issue