Code

Opened 6 years ago

Closed 2 years ago

#8060 closed Bug (fixed)

Admin Inlines do not respect user permissions

Reported by: p.patruno@… Owned by: sjaensch
Component: contrib.admin Version: master
Severity: Normal Keywords: inlines User authentication
Cc: sjaensch Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

use inlines in Django admin site. All is ok but :

1) I set a new user permissions to "can add ticket, can change ticket"
and nothing more
2) login with new user
3) I can change Action inline from Ticket admin inteface ( add, change
and delete !)

If I do not use inline the user is not enable to change/add/delete
Action and is what I expect.

If I try to edit other fields in the admin form Ticket defined
ForeignKey I get the expected "Permission denied"

I think that the possibility of change/add/delete a inline model
without privilege is a bug.

I have in my models:

class Ticket(models.Model):
........

class Action(models.Model):

    ticket = models.ForeignKey(Ticket)
........

in admin.py:

class ActionInline(admin.TabularInline):
    model = Action
    extra=1
    max_num=3

class TicketAdmin(admin.ModelAdmin):

.........
        inlines = [
        ActionInline,
        ]

Thanks

Paolo Patruno

Attachments (4)

admin_inline_permissions_v1.diff (6.7 KB) - added by sjaensch 3 years ago.
admin_inline_permissions_v2.diff (20.8 KB) - added by sjaensch 3 years ago.
admin_inline_permissions_v3.diff (24.8 KB) - added by sjaensch 3 years ago.
diff_v2_v3.diff (19.0 KB) - added by sjaensch 3 years ago.

Download all attachments as: .zip

Change History (38)

comment:1 Changed 6 years ago by adamfast

  • milestone set to 1.0
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 6 years ago by Alex

  • Summary changed from using inlines in Django admin site with User authentication to Admin Inlines do not respect user permissions

comment:3 Changed 6 years ago by dgouldin

  • Owner changed from nobody to dgouldin

comment:4 Changed 6 years ago by ubernostrum

  • milestone changed from 1.0 to post-1.0
  • Triage Stage changed from Accepted to Design decision needed

This needs some design thought, because the fact that you're doing multiple objects inline on one page seems to imply that you mean for them to be a logical unit; saying "you have permission to edit this, but you can't edit it because you'd also have to be able to edit other stuff" feels a bit wonky to me.

comment:5 Changed 5 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:6 Changed 5 years ago by anonymous

  • milestone set to 1.1 beta

comment:7 Changed 5 years ago by kmtracey

  • milestone 1.1 beta deleted

Something that hasn't even been Accepted is an unlikely candidate for 1.1 beta, especially when placed there anonymously.

comment:8 Changed 4 years ago by russellm

#12211 points out that this also affects group permissions (not surprising, but worth noting).

comment:9 Changed 3 years ago by Joshua "jag" Ginsberg <jag@…>

This could be accomplished in a straightforward manner by overloading the get_readonly_fields method of the BaseModelAdmin - return the readonly_fields attribute in the case the user does have the permission and return a tuple with the complete list of fields in the case the user does not have the permission. Similarly, can_delete can be changed to a method with the same request and obj arguments to likewise check for the delete permission.

comment:10 Changed 3 years ago by anonymous

I've just been bitten by this too, and have had to write a series of separate views to reimplement this part of the admin.

In a way I'mg glad though because I realise my assumptions about how model-based permissions work were wrong. I had thought that adding permissions actually altered the model save() and delete() methods in some way – introducing the authentication check there. Is there a reason why that isn't a more appropriate place to do this kind of permission checking? Otherwise it feels like one is always dependent on the admin not to have bugs which mean the permissions aren't respected properly.

comment:10 Changed 3 years ago by anonymous

I've just been bitten by this too, and have had to write a series of separate views to reimplement this part of the admin.

In a way I'mg glad though because I realise my assumptions about how model-based permissions work were wrong. I had thought that adding permissions actually altered the model save() and delete() methods in some way – introducing the authentication check there. Is there a reason why that isn't a more appropriate place to do this kind of permission checking? Otherwise it feels like one is always dependent on the admin not to have bugs which mean the permissions aren't respected properly.

comment:11 Changed 3 years ago by lukeplant

  • Severity set to Normal
  • Type set to Bug

comment:12 Changed 3 years ago by sjaensch

  • Cc sjaensch added
  • Easy pickings unset
  • UI/UX unset

I'd like to fix this bug by introducing those permission checks at the ModelAdmin level. Inlines where the user does not have create/edit privileges would be removed. ubernostrum said that some design thought would be needed. Here's my rationale for this implementation:

While admin.py states that models should be edited together with their inlines, this does not override the permission settings. Permissions are always more important than admin configuration. Inline editing is something that's enabled when writing the software, permissions are set during operation. So either the user cannot access the change view because he does not have the necessary permissions for some inline model or we do remove inline forms for the models where the user lacks sufficient permissions. Obviously, the latter solution would be preferable if it can be implemented reliably.

If there's consensus on this implementation, I'd like to go forward and develop a patch. I already have working prototype code since we needed this feature.

comment:13 Changed 3 years ago by carljm

  • Triage Stage changed from Design decision needed to Accepted

Preventing a user from accessing the change view for an object they do have permissions on, or removing all inlines, just because they lack permissions on one inline, is clearly wrong.

Removing an inline if the user doesn't have full permissions on the inlined model is definitely preferable to that.

Ideally, inlines should respect all three individual permissions properly, just like the rest of the admin does. If you have only add permission, you should be able to add a new inline but not see existing ones (we don't need to do readonly_fields - the precedent set by the rest of the admin is that you only get to see existing records at all if you can change them). If you have change but not add permission, you can change existing inlines but not add new ones. And you only get the delete checkbox if you have delete permission.

comment:14 Changed 3 years ago by sjaensch

  • Owner changed from dgouldin to sjaensch
  • Status changed from new to assigned

Great, thanks for the feedback! I'll work on a patch in the coming days.

Changed 3 years ago by sjaensch

comment:15 Changed 3 years ago by sjaensch

I just added a first version of the patch that does fix the bug (for the cases I tested). I'd appreciate any kind of feedback.

comment:16 Changed 3 years ago by carljm

  • Has patch set
  • Needs tests set
  • Patch needs improvement set

Thanks for your work on this patch! Looked at it briefly, and the general approach looks right.

I'm curious why you concluded that "we can't make sure the user can only edit existing inlines or only add new ones but not edit existing." It seems to me that formsets _ought_ to provide what we need to make edit-only work, via the max-num setting; and that there might-should be a way to make add-only work too (you don't see any of the existing ones but you can add new ones), though it might require some modifications in the formsets code.

I'm willing to consider falling back to the over-conservative approach in the current patch if someone looks into it carefully and concludes that it really is prohibitively complex to try to do it right - but I think we should at least check out what that would entail.

The patch will also definitely need tests.

comment:17 follow-up: Changed 3 years ago by sjaensch

Thanks! I'll take a further look at formsets, it wasn't immediately clear to me how to implement that.

I absolutely agree on the need for tests. But django.contrib.admin does not currently have any tests, does it? Should I start adding them under django/contrib/admin/tests/? Or are they stored somewhere else?

comment:18 in reply to: ↑ 17 Changed 3 years ago by carljm

Replying to sjaensch:

Thanks! I'll take a further look at formsets, it wasn't immediately clear to me how to implement that.

Cool, thanks. Feel free to ping in #django-dev in IRC (carljm) if you want to consult on any of the finer points.

I absolutely agree on the need for tests. But django.contrib.admin does not currently have any tests, does it? Should I start adding them under django/contrib/admin/tests/? Or are they stored somewhere else?

The admin, like some of the other contrib apps, actually has its tests with the core Django test suite rather than in the contrib app. So you'll find them in tests/regressiontests/admin_*

comment:19 Changed 3 years ago by sjaensch

  • Needs tests unset
  • Patch needs improvement unset

Version 2 of the patch is ready. I moved the permission checking code to the InlineModelAdmin class and added some tests. Fine-grained permission checking for the change_view should now work too. Any comments welcome. I'm not very fond of how I return an empty queryset by using pk__isnull=True as filter. Anybody have a better solution for that? I think the patch is more or less ready for review.

Last edited 3 years ago by sjaensch (previous) (diff)

Changed 3 years ago by sjaensch

comment:20 Changed 3 years ago by carljm

  • Patch needs improvement set

This is looking good! Thanks for all your work on it. A few comments:

  1. The tests should be broken up into many test methods, one for each case you're testing (here, I think each block of three related assertContains or assertNotContains constitutes a "case"). Generally it's ok to have a few closely-related asserts in a single test method, but as few as possible. This'll result in some duplication of setup code; that's ok. If the duplication is really bad, you can factor out some of the setup code into a helper method on the testcase (that's better than putting it in setUp() as you still explicitly call the helper method in each test that needs it, so there's less implicit dependency between tests). In tests it's better to duplicate a bit of setup code here and there than it is to have super-long test methods where everything is intertwined and its hard to change the conditions for one test without affecting others later on, and a single failure early on can obscure later failures. (Django does have some super-long test methods in its suite, mostly as a result of conversion from doctests, but that doesn't mean we want more!). I haven't reviewed in depth yet whether the test coverage is adequate, that'll be easier to do once separate tests are broken out.
  1. You can get an empty version of any queryset with the .none() method, rather than using pk__isnull=True.
  1. The other bit I'm not sure is right is the special handling for m2m auto-created through models. In the m2m case, each inlined instance represents a relationship, or an instance of the through model, not an instance of the related model. There are no permissions for an auto-created through model, but I think the most sensible thing is to consider any change, addition, or deletion of a through-model to be a *change* to the related model. In other words, it doesn't make sense to prevent deleting a through-model instance because the user doesn't have delete permissions on the related model; no related-model-instance is being deleted. So in the auto-created case I think all three has_X_permission methods should return has_change_permission on the related model. Also, this reasoning should be encapsulated in a comment.

comment:21 follow-up: Changed 3 years ago by sjaensch

  1. This is a very good point. I was not sure about how I should split them up. Should I create multiple methods on TestInline or should I go and create a separate TestCase class for it?
  2. Thanks! I'll change it in the next version.
  3. Good point! I Hadn't thought about that. I'll post a patch tomorrow with the changed behavior. One point to consider though: We then cannot restrict adding additional relationships (or disallow the editing of existing relationships) for auto-created intermediate m2m models. I think treating the through relationship as if it were the destination model might be better semantics. The through model is hidden from the user (as well as the programmer) anyway, that's why there are no permissions for it. But that's just my personal opinion, I'll gladly defer to your judgment. Or should we ask on django-developers what the others prefer?

comment:22 in reply to: ↑ 21 ; follow-up: Changed 3 years ago by carljm

Replying to sjaensch:

  1. This is a very good point. I was not sure about how I should split them up. Should I create multiple methods on TestInline or should I go and create a separate TestCase class for it?

You could go either way; it'll be enough related test methods that I'd probably make a new TestCase.

  1. Good point! I Hadn't thought about that. I'll post a patch tomorrow with the changed behavior. One point to consider though: We then cannot restrict adding additional relationships (or disallow the editing of existing relationships) for auto-created intermediate m2m models. I think treating the through relationship as if it were the destination model might be better semantics. The through model is hidden from the user (as well as the programmer) anyway, that's why there are no permissions for it. But that's just my personal opinion, I'll gladly defer to your judgment. Or should we ask on django-developers what the others prefer?

I'm pretty sure treating the through model as if it were the destination model is not the right semantic. Consider the M2M relationship between, say, FlatPage and Site (if it used inlines, which it doesn't by default). If someone is forbidden from deleting Site objects, there's no reason that should imply they can't remove a given FlatPage from a particular site. Removing a FlatPage relationship is, if anything, a change to a Site - it certainly isn't equivalent to deleting a Site.

I know this means we wouldn't be able to have granular permissions on an auto-created through model, but really that's just the consequence of auto-created through models not having their own permissions. Tying it directly to the permissions of the destination model doesn't help, it just moves the problem (and IMO makes it more clearly incorrect): I'd have no way to prevent someone from deleting Sites without also preventing them from removing a FlatPage from a site.

If you still aren't convinced, you're welcome to raise it on django-developers and see what other opinions you get!

comment:23 in reply to: ↑ 22 ; follow-up: Changed 3 years ago by sjaensch

Replying to carljm:

I'm pretty sure treating the through model as if it were the destination model is not the right semantic. Consider the M2M relationship between, say, FlatPage and Site (if it used inlines, which it doesn't by default). If someone is forbidden from deleting Site objects, there's no reason that should imply they can't remove a given FlatPage from a particular site. Removing a FlatPage relationship is, if anything, a change to a Site - it certainly isn't equivalent to deleting a Site.

Agreed. I'm not yet sure if removing a FlatPage-Site relationship is a change to the FlatPage or to the Site. I guess this can be argued either way, since the relationship is M2M - there is no direction like in the ForeignKey case. It might also depend on the particular use case. I'll post a patch later that checks the change permission of the related model, let's see how it feels. :)

comment:24 in reply to: ↑ 23 Changed 3 years ago by carljm

Replying to sjaensch:

Agreed. I'm not yet sure if removing a FlatPage-Site relationship is a change to the FlatPage or to the Site.

If the user is already on the edit page for the model on this side, they obviously have change permissions for it. So we're really requiring them to have change permission for both sides of the relationship in order to muck with the inlines, which I think is correct. (If they're on the add page for this side, they might not have change permission - but it's reasonable that if you have add permission for a model you can create some initial m2m relationships when you add an instance of that model).

I'll post a patch later that checks the change permission of the related model, let's see how it feels. :)

Sounds good, thanks!

Changed 3 years ago by sjaensch

Changed 3 years ago by sjaensch

comment:25 Changed 3 years ago by sjaensch

I added a new patch with all the changes requested by carljm. diff_v2_v3 shows the difference between this patch and the last. All the inline permission tests are split up now and are in their own TestCase class.

comment:26 Changed 3 years ago by anonymous

I'm using GitHub to maintain the patch I've posted here: https://github.com/sjaensch/django. I'm periodically rebasing my changes against the tip of Django so the should apply cleanly to Django SVN.

comment:27 Changed 3 years ago by sjaensch

The last comment was by me, not sure why I was suddenly logged out. Maybe because of the outage yesterday?

comment:28 Changed 3 years ago by sjaensch

  • Patch needs improvement unset

I changed the GitHub repository as requested in the IRC channel. There is now a separate branch that contains all changes for this issue: https://github.com/sjaensch/django/tree/admin_inline_perm. I'll periodically rebase the branch.
I also added a release note as suggested by carljm. I'm not really happy about the wording, maybe a native speaker could check it? I also feels this should be documented in the admin documentation. I'll work on that tomorrow.

If anybody can try out the patch, that would be great. Django's testsuite does not produce any failures.

comment:29 Changed 3 years ago by sjaensch

  • Patch needs improvement set

I just added a bit of documentation. All of django's tests pass, by the way.

comment:30 Changed 3 years ago by sjaensch

  • Patch needs improvement unset

comment:31 Changed 3 years ago by carljm

  • Resolution set to fixed
  • Status changed from assigned to closed

In [16934]:

Fixed #8060 - Added permissions-checking for admin inlines. Thanks p.patruno for report and Stephan Jaensch for work on the patch.

comment:32 Changed 2 years ago by pm4public@…

  • Resolution fixed deleted
  • Status changed from closed to reopened

I think it is not fixed - django 1.3 and tabular inline.

comment:33 Changed 2 years ago by aaugustin

  • Resolution set to fixed
  • Status changed from reopened to closed

It's fixed in 1.4.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.