Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fixed #6327 -- Added has_module_permission method to BaseModelAdmin #2730

Closed
wants to merge 3 commits into from

2 participants

@maxocub
  • Added tests for this new method and for the other ones:
    • has_add_permission()
    • has_change_permission()
    • has_delete_permission()
  • Updated the documentation
@maxocub maxocub Fixed #6327 -- Added has_module_permission method to BaseModelAdmin
* Added tests for this new method and for the other ones:
  * has_add_permission()
  * has_change_permission()
  * has_delete_permission()
* Updated the documentation
4535679
django/contrib/admin/options.py
@@ -477,6 +477,14 @@ def has_delete_permission(self, request, obj=None):
codename = get_permission_codename('delete', opts)
return request.user.has_perm("%s.%s" % (opts.app_label, codename))
+ def has_module_permission(self, request):
+ """
+ Returns True if the given request has any permission in the given
+ app label
+ """
+ opts = self.opts
@timgraham Owner

no need for intermediate variable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham commented on the diff
django/contrib/admin/options.py
@@ -477,6 +477,14 @@ def has_delete_permission(self, request, obj=None):
codename = get_permission_codename('delete', opts)
return request.user.has_perm("%s.%s" % (opts.app_label, codename))
+ def has_module_permission(self, request):
@timgraham Owner

The method needs to be used by the ModelAdmin... (and should add a test that overriding it achieves the desired functionality)

@maxocub
maxocub added a note

I'm not sure I understand where to use this method in ModelAdmin. Should I replace occurrences of user.has_module_perms by model_admin.has_module_permission (like in the index() and app_index() methods of AdminSite) ?

@timgraham Owner

Yes, I believe that's the point of the suggestion -- so you can override the method if you want to customize the behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
docs/ref/contrib/admin/index.txt
@@ -1631,6 +1631,11 @@ templates used by the :class:`ModelAdmin` views:
be interpreted as meaning that the current user is not permitted to delete
any object of this type).
+.. method:: ModelAdmin.has_module_permission(request)
+
+ Should return ``True`` if adding, editing or deleting an object is
@timgraham Owner

comma after editing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham commented on the diff
docs/ref/contrib/admin/index.txt
@@ -1631,6 +1631,11 @@ templates used by the :class:`ModelAdmin` views:
be interpreted as meaning that the current user is not permitted to delete
any object of this type).
+.. method:: ModelAdmin.has_module_permission(request)
+
@timgraham Owner

.. versionadded:: 1.8 and add a mention in the 1.8 release notes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
docs/ref/contrib/admin/index.txt
@@ -1631,6 +1631,11 @@ templates used by the :class:`ModelAdmin` views:
be interpreted as meaning that the current user is not permitted to delete
any object of this type).
+.. method:: ModelAdmin.has_module_permission(request)
+
+ Should return ``True`` if adding, editing or deleting an object is
+ permitted, ``False`` otherwise.
@timgraham Owner

Add: Uses :meth:User.has_module_perms() <django.contrib.auth.models.User.has_module_perms> by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@maxocub maxocub Made changes asked in review by timgraham
* Corrected the documentation
* Updated 1.8 release notes
* Removed intermediate variable in `has_module_permission()`
* Added uses of `has_module_permission()` in options.py
* Replaced `user.has_module_perms()` by
  `model_admin.has_module_permission()` in sites.py
* Added test for overriding `has_module_permission()`
1e79724
django/contrib/admin/options.py
@@ -191,7 +191,8 @@ def formfield_for_dbfield(self, db_field, **kwargs):
if formfield and db_field.name not in self.raw_id_fields:
related_modeladmin = self.admin_site._registry.get(db_field.rel.to)
can_add_related = bool(related_modeladmin and
- related_modeladmin.has_add_permission(request))
+ related_modeladmin.has_add_permission(request) and
+ related_modeladmin.has_module_permission(request))
@timgraham Owner

I wasn't expecting this additional check to be added in all these cases. In the default case, there's no change and it doesn't seem very DRY to require both.

@maxocub
maxocub added a note

I wasn't sure about that, but if you override has_module_permission() to block access to a certain module, someone with add, change, or delete permission would still be able to access those pages by entering the correct url.

@timgraham Owner

Hm, I see that rationale. On the other hand, having to check two methods everywhere seems error prone. I think it's better to document that has_module_permission only controls the visibility of the app_index and that you should override the has_*_permission methods to include has_module_permission if you want to completely shut off access (although I'm not completely happy with that either....). It doesn't seem like a very common use case anyway, but who knows...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/contrib/admin/sites.py
@@ -401,7 +400,7 @@ def index(self, request, extra_context=None):
'name': apps.get_app_config(app_label).verbose_name,
'app_label': app_label,
'app_url': reverse('admin:app_list', kwargs={'app_label': app_label}, current_app=self.name),
- 'has_module_perms': has_module_perms,
@timgraham Owner

Please don't rename the context variable as that's backwards incompatible. Thus, I wouldn't rename the local variable either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/contrib/admin/sites.py
((33 lines not shown))
- if app_dict:
- app_dict['models'].append(model_dict),
- else:
- # First time around, now that we know there's
- # something to display, add in the necessary meta
- # information.
- app_dict = {
- 'name': app_name,
- 'app_label': app_label,
- 'app_url': '',
- 'has_module_perms': has_module_perms,
- 'models': [model_dict],
+ has_module_permission = model_admin.has_module_permission(request)
+ if not has_module_permission:
+ raise PermissionDenied
+ else:
@timgraham Owner

don't need an else here (raising PermissionDenied short-circuits), that'll make the diff less scary too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@maxocub maxocub Made changes asked in review by timgraham
* Changed variable name has_module_permission back to
  has_module_perms in sites.py
* Removed useless else statement in sites.py
* Removed all has_module_permission() checks in options.py
* Updated the documentation
* Updated the tests
9e6c7a9
@timgraham
Owner

merged in 504c89e with some doc edits, thanks.

@timgraham timgraham closed this
@maxocub maxocub deleted the branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 28, 2014
  1. @maxocub

    Fixed #6327 -- Added has_module_permission method to BaseModelAdmin

    maxocub authored
    * Added tests for this new method and for the other ones:
      * has_add_permission()
      * has_change_permission()
      * has_delete_permission()
    * Updated the documentation
Commits on Jun 6, 2014
  1. @maxocub

    Made changes asked in review by timgraham

    maxocub authored
    * Corrected the documentation
    * Updated 1.8 release notes
    * Removed intermediate variable in `has_module_permission()`
    * Added uses of `has_module_permission()` in options.py
    * Replaced `user.has_module_perms()` by
      `model_admin.has_module_permission()` in sites.py
    * Added test for overriding `has_module_permission()`
Commits on Jun 9, 2014
  1. @maxocub

    Made changes asked in review by timgraham

    maxocub authored
    * Changed variable name has_module_permission back to
      has_module_perms in sites.py
    * Removed useless else statement in sites.py
    * Removed all has_module_permission() checks in options.py
    * Updated the documentation
    * Updated the tests
This page is out of date. Refresh to see the latest.
View
7 django/contrib/admin/options.py
@@ -477,6 +477,13 @@ def has_delete_permission(self, request, obj=None):
codename = get_permission_codename('delete', opts)
return request.user.has_perm("%s.%s" % (opts.app_label, codename))
+ def has_module_permission(self, request):
@timgraham Owner

The method needs to be used by the ModelAdmin... (and should add a test that overriding it achieves the desired functionality)

@maxocub
maxocub added a note

I'm not sure I understand where to use this method in ModelAdmin. Should I replace occurrences of user.has_module_perms by model_admin.has_module_permission (like in the index() and app_index() methods of AdminSite) ?

@timgraham Owner

Yes, I believe that's the point of the suggestion -- so you can override the method if you want to customize the behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ """
+ Returns True if the given request has any permission in the given
+ app label
+ """
+ return request.user.has_module_perms(self.opts.app_label)
+
@python_2_unicode_compatible
class ModelAdmin(BaseModelAdmin):
View
11 django/contrib/admin/sites.py
@@ -367,10 +367,9 @@ def index(self, request, extra_context=None):
apps that have been registered in this site.
"""
app_dict = {}
- user = request.user
for model, model_admin in self._registry.items():
app_label = model._meta.app_label
- has_module_perms = user.has_module_perms(app_label)
+ has_module_perms = model_admin.has_module_permission(request)
if has_module_perms:
perms = model_admin.get_model_perms(request)
@@ -424,14 +423,14 @@ def index(self, request, extra_context=None):
current_app=self.name)
def app_index(self, request, app_label, extra_context=None):
- user = request.user
app_name = apps.get_app_config(app_label).verbose_name
- has_module_perms = user.has_module_perms(app_label)
- if not has_module_perms:
- raise PermissionDenied
app_dict = {}
for model, model_admin in self._registry.items():
if app_label == model._meta.app_label:
+ has_module_perms = model_admin.has_module_permission(request)
+ if not has_module_perms:
+ raise PermissionDenied
+
perms = model_admin.get_model_perms(request)
# Check whether user has any perm for this module.
View
14 docs/ref/contrib/admin/index.txt
@@ -1631,6 +1631,19 @@ templates used by the :class:`ModelAdmin` views:
be interpreted as meaning that the current user is not permitted to delete
any object of this type).
+.. method:: ModelAdmin.has_module_permission(request)
+
@timgraham Owner

.. versionadded:: 1.8 and add a mention in the 1.8 release notes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ .. versionadded:: 1.8
+
+ Should return ``True`` if adding, editing, or deleting an object is
+ permitted, ``False`` otherwise. Uses :meth:`User.has_module_perms()
+ <django.contrib.auth.models.User.has_module_perms>` by default. It is
+ used for displaying the module on the admin index page and for accessing
+ the module's index page. Overriding it does not restrict access to the add,
+ change or delete views, :meth:`~ModelAdmin.has_add_permission`,
+ :meth:`~ModelAdmin.has_change_permission`, or
+ :meth:`~ModelAdmin.has_delete_permission` should be used for that.
+
.. method:: ModelAdmin.get_queryset(request)
The ``get_queryset`` method on a ``ModelAdmin`` returns a
@@ -1909,6 +1922,7 @@ adds some of its own (the shared features are actually defined in the
- :meth:`~ModelAdmin.has_add_permission`
- :meth:`~ModelAdmin.has_change_permission`
- :meth:`~ModelAdmin.has_delete_permission`
+- :meth:`~ModelAdmin.has_module_permission`
The ``InlineModelAdmin`` class adds:
View
4 docs/releases/1.8.txt
@@ -31,7 +31,9 @@ Minor features
:mod:`django.contrib.admin`
^^^^^^^^^^^^^^^^^^^^^^^^^^^
-* ...
+* :class:`~django.contrib.admin.ModelAdmin` now has a
+ :meth:`~django.contrib.admin.ModelAdmin.has_module_permission`
+ method to check if the request has any permission for this module.
:mod:`django.contrib.auth`
^^^^^^^^^^^^^^^^^^^^^^^^^^
View
3  tests/admin_ordering/tests.py
@@ -17,6 +17,9 @@ class MockSuperUser(object):
def has_perm(self, perm):
return True
+ def has_module_perms(self, module):
+ return True
+
request = MockRequest()
request.user = MockSuperUser()
View
8 tests/admin_views/admin.py
@@ -124,6 +124,12 @@ def save_model(self, request, obj, form, change=True):
return super(ArticleAdmin, self).save_model(request, obj, form, change)
+class ArticleAdmin2(admin.ModelAdmin):
+
+ def has_module_permission(self, request):
+ return False
+
+
class RowLevelChangePermissionModelAdmin(admin.ModelAdmin):
def has_change_permission(self, request, obj=None):
""" Only allow changing objects with even id number """
@@ -923,3 +929,5 @@ def get_changeform_initial_data(self, request):
site2 = admin.AdminSite(name="namespaced_admin")
site2.register(User, UserAdmin)
site2.register(Group, GroupAdmin)
+site7 = admin.AdminSite(name="admin7")
+site7.register(Article, ArticleAdmin2)
View
64 tests/admin_views/tests.py
@@ -1494,6 +1494,70 @@ def test_shortcut_view_only_available_to_staff(self):
self.assertEqual(response.status_code, 302)
self.assertEqual(response.url, 'http://example.com/dummy/foo/')
+ def test_has_module_permission(self):
+ """
+ Ensure that has_module_permission() returns True for all users who
+ have any permission for that module (add, change, or delete), so that
+ the module is displayed on the admin index page.
+ """
+ login_url = reverse('admin:login') + '?next=/test_admin/admin/'
+
+ self.client.post(login_url, self.super_login)
+ response = self.client.get('/test_admin/admin/')
+ self.assertContains(response, 'admin_views')
+ self.assertContains(response, 'Articles')
+ self.client.get('/test_admin/admin/logout/')
+
+ self.client.post(login_url, self.adduser_login)
+ response = self.client.get('/test_admin/admin/')
+ self.assertContains(response, 'admin_views')
+ self.assertContains(response, 'Articles')
+ self.client.get('/test_admin/admin/logout/')
+
+ self.client.post(login_url, self.changeuser_login)
+ response = self.client.get('/test_admin/admin/')
+ self.assertContains(response, 'admin_views')
+ self.assertContains(response, 'Articles')
+ self.client.get('/test_admin/admin/logout/')
+
+ self.client.post(login_url, self.deleteuser_login)
+ response = self.client.get('/test_admin/admin/')
+ self.assertContains(response, 'admin_views')
+ self.assertContains(response, 'Articles')
+ self.client.get('/test_admin/admin/logout/')
+
+ def test_overriding_has_module_permission(self):
+ """
+ Ensure that overriding has_module_permission() has the desired effect.
+ In this case, it always returns False, so the module should not be
+ displayed on the admin index page for any users.
+ """
+ login_url = reverse('admin:login') + '?next=/test_admin/admin7/'
+
+ self.client.post(login_url, self.super_login)
+ response = self.client.get('/test_admin/admin7/')
+ self.assertNotContains(response, 'admin_views')
+ self.assertNotContains(response, 'Articles')
+ self.client.get('/test_admin/admin7/logout/')
+
+ self.client.post(login_url, self.adduser_login)
+ response = self.client.get('/test_admin/admin7/')
+ self.assertNotContains(response, 'admin_views')
+ self.assertNotContains(response, 'Articles')
+ self.client.get('/test_admin/admin7/logout/')
+
+ self.client.post(login_url, self.changeuser_login)
+ response = self.client.get('/test_admin/admin7/')
+ self.assertNotContains(response, 'admin_views')
+ self.assertNotContains(response, 'Articles')
+ self.client.get('/test_admin/admin7/logout/')
+
+ self.client.post(login_url, self.deleteuser_login)
+ response = self.client.get('/test_admin/admin7/')
+ self.assertNotContains(response, 'admin_views')
+ self.assertNotContains(response, 'Articles')
+ self.client.get('/test_admin/admin7/logout/')
+
@override_settings(PASSWORD_HASHERS=('django.contrib.auth.hashers.SHA1PasswordHasher',),
ROOT_URLCONF="admin_views.urls")
View
1  tests/admin_views/urls.py
@@ -11,4 +11,5 @@
url(r'^test_admin/admin3/', include(admin.site.urls), dict(form_url='pony')),
url(r'^test_admin/admin4/', include(customadmin.simple_site.urls)),
url(r'^test_admin/admin5/', include(admin.site2.urls)),
+ url(r'^test_admin/admin7/', include(admin.site7.urls)),
]
View
90 tests/modeladmin/tests.py
@@ -1518,3 +1518,93 @@ class CustomModelAdmin(ModelAdmin):
validator_class = CustomValidator
self.assertIsInvalid(CustomModelAdmin, ValidationTestModel, 'error!')
+
+
+class ModelAdminPermissionTests(TestCase):
+
+ class MockUser(object):
+ def has_module_perms(self, app_label):
+ if app_label == "modeladmin":
+ return True
+ return False
+
+ class MockAddUser(MockUser):
+ def has_perm(self, perm):
+ if perm == "modeladmin.add_band":
+ return True
+ return False
+
+ class MockChangeUser(MockUser):
+ def has_perm(self, perm):
+ if perm == "modeladmin.change_band":
+ return True
+ return False
+
+ class MockDeleteUser(MockUser):
+ def has_perm(self, perm):
+ if perm == "modeladmin.delete_band":
+ return True
+ return False
+
+ def test_has_add_permission(self):
+ """
+ Ensure that has_add_permission returns True for users who can add
+ objects and False for users who can't.
+ """
+ ma = ModelAdmin(Band, AdminSite())
+ request = MockRequest()
+ request.user = self.MockAddUser()
+ self.assertTrue(ma.has_add_permission(request))
+ request.user = self.MockChangeUser()
+ self.assertFalse(ma.has_add_permission(request))
+ request.user = self.MockDeleteUser()
+ self.assertFalse(ma.has_add_permission(request))
+
+ def test_has_change_permission(self):
+ """
+ Ensure that has_change_permission returns True for users who can edit
+ objects and False for users who can't.
+ """
+ ma = ModelAdmin(Band, AdminSite())
+ request = MockRequest()
+ request.user = self.MockAddUser()
+ self.assertFalse(ma.has_change_permission(request))
+ request.user = self.MockChangeUser()
+ self.assertTrue(ma.has_change_permission(request))
+ request.user = self.MockDeleteUser()
+ self.assertFalse(ma.has_change_permission(request))
+
+ def test_has_delete_permission(self):
+ """
+ Ensure that has_delete_permission returns True for users who can delete
+ objects and False for users who can't.
+ """
+ ma = ModelAdmin(Band, AdminSite())
+ request = MockRequest()
+ request.user = self.MockAddUser()
+ self.assertFalse(ma.has_delete_permission(request))
+ request.user = self.MockChangeUser()
+ self.assertFalse(ma.has_delete_permission(request))
+ request.user = self.MockDeleteUser()
+ self.assertTrue(ma.has_delete_permission(request))
+
+ def test_has_module_permission(self):
+ """
+ Ensure that has_module_permission returns True for users who have any
+ permission for the module and False for users who don't.
+ """
+ ma = ModelAdmin(Band, AdminSite())
+ request = MockRequest()
+ request.user = self.MockAddUser()
+ self.assertTrue(ma.has_module_permission(request))
+ request.user = self.MockChangeUser()
+ self.assertTrue(ma.has_module_permission(request))
+ request.user = self.MockDeleteUser()
+ self.assertTrue(ma.has_module_permission(request))
+ ma.opts.app_label = "anotherapp"
+ request.user = self.MockAddUser()
+ self.assertFalse(ma.has_module_permission(request))
+ request.user = self.MockChangeUser()
+ self.assertFalse(ma.has_module_permission(request))
+ request.user = self.MockDeleteUser()
+ self.assertFalse(ma.has_module_permission(request))
Something went wrong with that request. Please try again.