Difference between AdminSite.admin_view and staff_member_required?
265 views
Skip to first unread message
Tobias Bengfort
Mar 9, 2021, 5:57:14 AM3/9/21
to django-d...@googlegroups.com
Hi,
while reading the documentation on django's admin site I noticed there
are two mechanisms to check for authentication: AdminSite.admin_view[0]
and staff_member_required[1]. I am not sure when I should use which of them.
So what is the difference between the two? I expect that
staff_member_required is more low-level and I should use
AdminSite.admin_view in most cases. Is that correct?
I will try to make a pull request to add the answer to the documentation.
thanks
tobias
[0]:
https://docs.djangoproject.com/en/3.1/ref/contrib/admin/#django.contrib.admin.ModelAdmin.get_urls
[1]:
https://docs.djangoproject.com/en/3.1/ref/contrib/admin/#the-staff-member-required-decorator
while reading the documentation on django's admin site I noticed there
are two mechanisms to check for authentication: AdminSite.admin_view[0]
and staff_member_required[1]. I am not sure when I should use which of them.
So what is the difference between the two? I expect that
staff_member_required is more low-level and I should use
AdminSite.admin_view in most cases. Is that correct?
I will try to make a pull request to add the answer to the documentation.
thanks
tobias
[0]:
https://docs.djangoproject.com/en/3.1/ref/contrib/admin/#django.contrib.admin.ModelAdmin.get_urls
[1]:
https://docs.djangoproject.com/en/3.1/ref/contrib/admin/#the-staff-member-required-decorator
Carlton Gibson
Mar 9, 2021, 6:04:56 AM3/9/21
to Django developers (Contributions to Django itself)
Hi Tobias.
I’ve always taken it that @staff_member_required is for decorating views that aren’t part of the admin, and so not accessed via AdminSite.get_urls().
I don’t think I’d use AdminSite.admin_view() outside of the context in the example there.
I can’t think why one would do that… 🤔 — no doubt someone does 🙂
Kind Regards,
Carlton
> --
> You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/cb52dfda-67e4-a336-4eea-39195d6f8bb3%40posteo.de.
I’ve always taken it that @staff_member_required is for decorating views that aren’t part of the admin, and so not accessed via AdminSite.get_urls().
I don’t think I’d use AdminSite.admin_view() outside of the context in the example there.
I can’t think why one would do that… 🤔 — no doubt someone does 🙂
Kind Regards,
Carlton
> You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/cb52dfda-67e4-a336-4eea-39195d6f8bb3%40posteo.de.
Adam Johnson
Mar 9, 2021, 7:22:02 AM3/9/21
to django-d...@googlegroups.com
admin_view does extra stuff like calling the admin site's get_permission method and using the admin login page rather than the default auth one: https://github.com/django/django/blob/98d3fd61026457a435ef5b7afce6b6e64e9f241d/django/contrib/admin/sites.py#L198
It should indeed be used only for admin pages.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/67A3688D-41C0-4E14-B235-51482689D285%40gmail.com.
--
Adam
Tobias Bengfort
Mar 9, 2021, 8:33:27 AM3/9/21
to django-d...@googlegroups.com
Hi,
On Tue, 9 Mar 2021 at 11:04, Carlton Gibson wrote:
> I’ve always taken it that @staff_member_required is for decorating
> views that aren’t part of the admin, and so not accessed via
> AdminSite.get_urls().
default. That contradicts both your answers, doesn't it? Now I am more
confused than before.
tobias
[0]
https://docs.djangoproject.com/en/3.1/ref/contrib/admin/#the-staff-member-required-decorator
On Tue, 9 Mar 2021 at 11:04, Carlton Gibson wrote:
> I’ve always taken it that @staff_member_required is for decorating
> views that aren’t part of the admin, and so not accessed via
> AdminSite.get_urls().
On 09/03/2021 13.21, Adam Johnson wrote:
> admin_view does extra stuff like calling the admin site's get_permission
> method and using the admin login page rather than the default auth one:
According to [0] staff_member_required also uses the admin login page by
> admin_view does extra stuff like calling the admin site's get_permission
> method and using the admin login page rather than the default auth one:
default. That contradicts both your answers, doesn't it? Now I am more
confused than before.
tobias
[0]
https://docs.djangoproject.com/en/3.1/ref/contrib/admin/#the-staff-member-required-decorator
Carlton Gibson
Mar 9, 2021, 8:50:21 AM3/9/21
to Django developers (Contributions to Django itself)
Hi.
On 9 Mar 2021, at 14:33, Tobias Bengfort <tobias....@posteo.de> wrote:
According to [0] ...
[0] https://docs.djangoproject.com/en/3.1/ref/contrib/admin/#the-staff-member-required-decorator
Those docs are unchanged since they were added 6 years ago.
A PR modernising the explanation may be in order. (I’d need to look at the matching history for admin_view judge fully…)
Kind Regards,
Carlton
Tobias Bengfort
Mar 9, 2021, 11:15:14 AM3/9/21
to django-d...@googlegroups.com
I checked the git logs and learned about a neat git feature:
git log -L ':staff_member_required:django/contrib/admin/views/decorators.py
git log -L ':admin_view:django/contrib/admin/sites.py
(requires `*py diff=python` in .gitattributes)
The last commit to admin_view was in 2009. The last commit to
staff_member_required was 2015. It was documented shortly after that.
So it seems like the documentation is up to date with the code.
On 09/03/2021 14.50, Carlton Gibson wrote:
> Hi.
>
>> On 9 Mar 2021, at 14:33, Tobias Bengfort <tobias....@posteo.de
> <https://groups.google.com/d/msgid/django-developers/12592C48-40D3-40FF-832E-D6D14DA8B9B4%40gmail.com?utm_medium=email&utm_source=footer>.
git log -L ':staff_member_required:django/contrib/admin/views/decorators.py
git log -L ':admin_view:django/contrib/admin/sites.py
(requires `*py diff=python` in .gitattributes)
The last commit to admin_view was in 2009. The last commit to
staff_member_required was 2015. It was documented shortly after that.
So it seems like the documentation is up to date with the code.
On 09/03/2021 14.50, Carlton Gibson wrote:
> Hi.
>
>> On 9 Mar 2021, at 14:33, Tobias Bengfort <tobias....@posteo.de
> --
> You received this message because you are subscribed to the Google
> Groups "Django developers (Contributions to Django itself)" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to django-develop...@googlegroups.com
> <mailto:django-develop...@googlegroups.com>.
> You received this message because you are subscribed to the Google
> Groups "Django developers (Contributions to Django itself)" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to django-develop...@googlegroups.com
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/django-developers/12592C48-40D3-40FF-832E-D6D14DA8B9B4%40gmail.com
> <https://groups.google.com/d/msgid/django-developers/12592C48-40D3-40FF-832E-D6D14DA8B9B4%40gmail.com?utm_medium=email&utm_source=footer>.
Carlton Gibson
Mar 9, 2021, 12:03:53 PM3/9/21
to django-d...@googlegroups.com
Hey Tobias.
OK, it looks like I need to review it in more depth. Possibly it does all make sense, but the staff_member_required docs don’t read how I’d expect them to from this conversation.
Do you want to open a PR with suggested changes as a focus for discussion?
Kind regards,
Carlton
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/5df94dc8-34f2-5484-15a9-bddac2eaec52%40posteo.de.
Tobias Bengfort
Mar 9, 2021, 2:08:57 PM3/9/21
to django-d...@googlegroups.com
On 09/03/2021 18.03, Carlton Gibson wrote:
> Do you want to open a PR with suggested changes as a focus for discussion?
I would open a PR that improves the docs, but I still don't understand
> Do you want to open a PR with suggested changes as a focus for discussion?
the difference, so I wouldn't know what to write.
The other alternative would be remove one of them, but at this point
that is probably too drastic.
tobias
Timothy McCurrach
Mar 9, 2021, 4:22:14 PM3/9/21
to django-d...@googlegroups.com
staff_member_required feels like a bit of a relic from the past:
- It looks like before a massive refactor of the admin that happened 12 years ago (https://github.com/django/django/commit/a19ed8aea395e8e07164ff7d85bd7dff2f24edca) staff_member_required played a similar role to what Adminsite.admin_view does now (checks permissions and redirects to login if appropriate). Before the refactor, all of the relevant views were wrapped with staff_member_required.
- The above commit introduced AdminSite which has its own 'has_permissions' method. Adminsite.root (which was the old get_urls) called self.has_permissions before calling any other views, so staff_member_required was now no longer really required.
- staff_member_required did however continue to be used in the admindocs app (which was now split off into its own app).
- Adminsite.root was eventually replaced with get_urls and the call to has_permissions was moved into `Adminsite.admin_view` (which also does a few other things: adds csrf protection, and deals with caching)
- Over time staff_member_required became simpler and simpler as things like user_passes_test were introduced, and there was no need to repeat logic.
- It's now just a very specific version of `user_passes_test` and is only used internally one place - the admindocs app.
Tobias - In terms of permissions and redirecting; the behaviour of admin_view and `staff_member_required` are very similar. The differences are that:
i) admin_view defers to Adminsite.has_permission to do the actual checking of permissions (so if that method is overwritten the behaviour will change)
ii) admin_view does a whole load of other things that all admin views are going to need (mentioned above).
iii) They have slightly different arguments you can pass in to modify the behaviour
If you are decorating a view of ModelAdmin, or AdminSite you should use admin_view - as described in the docs under get_urls.
Since staff_member_required is more or less a very specific version of user_passes_test designed for admin views, it's use-case seems pretty niche. If you were making an app like admindocs which behaves like an extension of the admin app, without actually being part of the admin app you could use it then, but I'm struggling to think of any other time you would want to use it though.
I think there's an argument to be made for deprecating it completely:
- It's only used in one place internally, and the wider use case seems pretty niche.
- user_passes_test can do the same thing and seems to be the standard way of doing this kind of thing anyway.
- user_passes_test with a suitably named test argument would also be more explicit, since the login_url and redirect_field_name would need to be explicitly stated.
Definitely the documentation should be updated though. Thoughts on deprecating it?
Tim
- It looks like before a massive refactor of the admin that happened 12 years ago (https://github.com/django/django/commit/a19ed8aea395e8e07164ff7d85bd7dff2f24edca) staff_member_required played a similar role to what Adminsite.admin_view does now (checks permissions and redirects to login if appropriate). Before the refactor, all of the relevant views were wrapped with staff_member_required.
- The above commit introduced AdminSite which has its own 'has_permissions' method. Adminsite.root (which was the old get_urls) called self.has_permissions before calling any other views, so staff_member_required was now no longer really required.
- staff_member_required did however continue to be used in the admindocs app (which was now split off into its own app).
- Adminsite.root was eventually replaced with get_urls and the call to has_permissions was moved into `Adminsite.admin_view` (which also does a few other things: adds csrf protection, and deals with caching)
- Over time staff_member_required became simpler and simpler as things like user_passes_test were introduced, and there was no need to repeat logic.
- It's now just a very specific version of `user_passes_test` and is only used internally one place - the admindocs app.
Tobias - In terms of permissions and redirecting; the behaviour of admin_view and `staff_member_required` are very similar. The differences are that:
i) admin_view defers to Adminsite.has_permission to do the actual checking of permissions (so if that method is overwritten the behaviour will change)
ii) admin_view does a whole load of other things that all admin views are going to need (mentioned above).
iii) They have slightly different arguments you can pass in to modify the behaviour
If you are decorating a view of ModelAdmin, or AdminSite you should use admin_view - as described in the docs under get_urls.
Since staff_member_required is more or less a very specific version of user_passes_test designed for admin views, it's use-case seems pretty niche. If you were making an app like admindocs which behaves like an extension of the admin app, without actually being part of the admin app you could use it then, but I'm struggling to think of any other time you would want to use it though.
I think there's an argument to be made for deprecating it completely:
- It's only used in one place internally, and the wider use case seems pretty niche.
- user_passes_test can do the same thing and seems to be the standard way of doing this kind of thing anyway.
- user_passes_test with a suitably named test argument would also be more explicit, since the login_url and redirect_field_name would need to be explicitly stated.
Definitely the documentation should be updated though. Thoughts on deprecating it?
Tim
--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/d10799d4-8a42-6caa-8e13-c2ce93256521%40posteo.de.
Matthew Pava
Mar 9, 2021, 4:45:21 PM3/9/21
to django-d...@googlegroups.com
> Thoughts on deprecating it?
It seems like a good idea. However, it leads us to another question. Do we really need the is_staff field on the User Model if it is deprecated?
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAFBPPP0bM2cr9etcMQwmprK1zX8gfKZ2wmEa7BW2CyiCrxTUog%40mail.gmail.com.
Timothy McCurrach
Mar 9, 2021, 4:51:34 PM3/9/21
to django-d...@googlegroups.com
> It seems like a good idea. However, it leads us to another question. Do we really need the is_staff field on the User Model if it is deprecated?
User.is_staff is still used, just from Adminsite.has_permission instead:
https://github.com/django/django/blob/main/django/contrib/admin/sites.py#L196
https://github.com/django/django/blob/main/django/contrib/admin/sites.py#L196
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/0158dd682e26429ea4e5b12ad2a548c4%40iss2.ISS.LOCAL.
Ian Foote
Mar 13, 2021, 10:03:15 AM3/13/21
to django-d...@googlegroups.com
I think deprecating it is a good idea. I think it's actually a small footgun - I've seen it used to restrict access to non-admin functionality, when `user_passes_test` would be appropriate. In the case I found it, it made adding tests trickier than it should have been.
Ian
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAFBPPP0rBhf1vfYa7p9RWY_bkmUCM0w4o2DyTwyUZYhSN_S0vQ%40mail.gmail.com.
Kevin Grinberg
Mar 13, 2021, 10:18:28 AM3/13/21
to Django developers (Contributions to Django itself)
`staff_member_required` is a nice convenience method, quite useful outside of the admin if your authorization needs are sufficiently served by User.is_staff (which isn't always the case of course, but often is on a small project).
Yes, it can be reduced to a `user_passes_test` call, but that's more verbose for a limited gain (given that this convenience method already exists, is tested, etc).
Additionally, for a newer developer, getting equivalent functionality `user_passes_test` requires being able to grok callables - again, certainly something you're going to need past a certain threshold, but it feels unnecessary to require less-experienced folks to grok that when all you want to do is "make it so that only people with the is_staff attribute can see this view" (with the caveat that yes, the login page is going to be the admin login page - which for a smaller project is exactly what you want).
Adam Johnson
Mar 15, 2021, 3:28:24 PM3/15/21
to django-d...@googlegroups.com
+1 for deprecating.
Yes, it can be reduced to a `user_passes_test` call, but that's more verbose for a limited gain (given that this convenience method already exists, is tested, etc).
I don't see this as an argument not to deprecate staff_member_required, since the verbosity of user_passes_test can be limited to one place:
staff_member_required = user_passes_test(
lambda u: u.is_active and u.is_staff,
)
lambda u: u.is_active and u.is_staff,
)
...
@staff_member_required
def my_view(request): ...
Additionally, for a newer developer, getting equivalent functionality `user_passes_test` requires being able to grok callables
I think we can put a little more faith in users. The docs for user_passes_test show a complete example that can be easily copied.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/2422b53d-0201-431e-a1f0-f0ae8bb568e3n%40googlegroups.com.
--
Adam
Reply all
Reply to author
Forward
0 new messages