Opened 8 years ago

Closed 5 years ago

#8060 closed Bug (fixed)

Admin Inlines do not respect user permissions

Reported by: p.patruno@… Owned by: Stephan Jaensch
Component: contrib.admin Version: master
Severity: Normal Keywords: inlines User authentication
Cc: Stephan Jaensch 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 Stephan Jaensch 5 years ago.
admin_inline_permissions_v2.diff (20.8 KB) - added by Stephan Jaensch 5 years ago.
admin_inline_permissions_v3.diff (24.8 KB) - added by Stephan Jaensch 5 years ago.
diff_v2_v3.diff (19.0 KB) - added by Stephan Jaensch 5 years ago.

Download all attachments as: .zip

Change History (38)

comment:1 Changed 8 years ago by adamfast

milestone: 1.0
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedAccepted

comment:2 Changed 8 years ago by Alex Gaynor

Summary: using inlines in Django admin site with User authenticationAdmin Inlines do not respect user permissions

comment:3 Changed 8 years ago by David Gouldin

Owner: changed from nobody to David Gouldin

comment:4 Changed 8 years ago by James Bennett

milestone: 1.0post-1.0
Triage Stage: AcceptedDesign 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 8 years ago by (none)

milestone: post-1.0

Milestone post-1.0 deleted

comment:6 Changed 8 years ago by anonymous

milestone: 1.1 beta

comment:7 Changed 8 years ago by Karen Tracey

milestone: 1.1 beta

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

comment:8 Changed 7 years ago by Russell Keith-Magee

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

comment:9 Changed 6 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 6 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 6 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 5 years ago by Luke Plant

Severity: Normal
Type: Bug

comment:12 Changed 5 years ago by Stephan Jaensch

Cc: Stephan Jaensch 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 5 years ago by Carl Meyer

Triage Stage: Design decision neededAccepted

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 5 years ago by Stephan Jaensch

Owner: changed from David Gouldin to Stephan Jaensch
Status: newassigned

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

Changed 5 years ago by Stephan Jaensch

comment:15 Changed 5 years ago by Stephan Jaensch

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 5 years ago by Carl Meyer

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 Changed 5 years ago by Stephan Jaensch

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 5 years ago by Carl Meyer

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 5 years ago by Stephan Jaensch

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 5 years ago by Stephan Jaensch (previous) (diff)

Changed 5 years ago by Stephan Jaensch

comment:20 Changed 5 years ago by Carl Meyer

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 Changed 5 years ago by Stephan Jaensch

  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 ; Changed 5 years ago by Carl Meyer

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 ; Changed 5 years ago by Stephan Jaensch

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 5 years ago by Carl Meyer

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 5 years ago by Stephan Jaensch

Changed 5 years ago by Stephan Jaensch

Attachment: diff_v2_v3.diff added

comment:25 Changed 5 years ago by Stephan Jaensch

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 5 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 5 years ago by Stephan Jaensch

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

comment:28 Changed 5 years ago by Stephan Jaensch

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 5 years ago by Stephan Jaensch

Patch needs improvement: set

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

comment:30 Changed 5 years ago by Stephan Jaensch

Patch needs improvement: unset

comment:31 Changed 5 years ago by Carl Meyer

Resolution: fixed
Status: assignedclosed

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 5 years ago by pm4public@…

Resolution: fixed
Status: closedreopened

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

comment:33 Changed 5 years ago by Aymeric Augustin

Resolution: fixed
Status: reopenedclosed

It's fixed in 1.4.

Note: See TracTickets for help on using tickets.
Back to Top