Opened 16 years ago
Closed 6 years ago
#8060 closed Bug (fixed)
Admin Inlines do not respect user permissions
Reported by: | Owned by: | Stephan Jaensch | |
---|---|---|---|
Component: | contrib.admin | Version: | dev |
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)
Change History (40)
comment:1 by , 16 years ago
milestone: | → 1.0 |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 16 years ago
Summary: | using inlines in Django admin site with User authentication → Admin Inlines do not respect user permissions |
---|
comment:3 by , 16 years ago
Owner: | changed from | to
---|
comment:4 by , 16 years ago
milestone: | 1.0 → post-1.0 |
---|---|
Triage Stage: | Accepted → Design decision needed |
comment:6 by , 16 years ago
milestone: | → 1.1 beta |
---|
comment:7 by , 16 years ago
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 by , 15 years ago
#12211 points out that this also affects group permissions (not surprising, but worth noting).
comment:9 by , 14 years ago
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 by , 14 years ago
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 by , 14 years ago
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 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → Bug |
comment:12 by , 13 years ago
Cc: | 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 by , 13 years ago
Triage Stage: | Design decision needed → 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 by , 13 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
Great, thanks for the feedback! I'll work on a patch in the coming days.
by , 13 years ago
Attachment: | admin_inline_permissions_v1.diff added |
---|
comment:15 by , 13 years ago
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 by , 13 years ago
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.
follow-up: 18 comment:17 by , 13 years ago
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 by , 13 years ago
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 by , 13 years ago
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.
by , 13 years ago
Attachment: | admin_inline_permissions_v2.diff added |
---|
comment:20 by , 13 years ago
Patch needs improvement: | set |
---|
This is looking good! Thanks for all your work on it. A few comments:
- 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.
- You can get an empty version of any queryset with the
.none()
method, rather than usingpk__isnull=True
.
- 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.
follow-up: 22 comment:21 by , 13 years ago
- 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?
- Thanks! I'll change it in the next version.
- 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?
follow-up: 23 comment:22 by , 13 years ago
Replying to sjaensch:
- 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
.
- 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!
follow-up: 24 comment:23 by , 13 years ago
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
andSite
(if it used inlines, which it doesn't by default). If someone is forbidden from deletingSite
objects, there's no reason that should imply they can't remove a givenFlatPage
from a particular site. Removing aFlatPage
relationship is, if anything, a change to aSite
- it certainly isn't equivalent to deleting aSite
.
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 by , 13 years ago
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!
by , 13 years ago
Attachment: | admin_inline_permissions_v3.diff added |
---|
by , 13 years ago
Attachment: | diff_v2_v3.diff added |
---|
comment:25 by , 13 years ago
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 by , 13 years ago
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 by , 13 years ago
The last comment was by me, not sure why I was suddenly logged out. Maybe because of the outage yesterday?
comment:28 by , 13 years ago
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 by , 13 years ago
Patch needs improvement: | set |
---|
I just added a bit of documentation. All of django's tests pass, by the way.
comment:30 by , 13 years ago
Patch needs improvement: | unset |
---|
comment:32 by , 13 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
I think it is not fixed - django 1.3 and tabular inline.
comment:34 by , 6 years ago
Resolution: | fixed |
---|---|
Status: | closed → new |
This seems to have regressed in (at least) 2.1. I have 2 view
only permissions. I have a ManyToManyField
represented in my main model as a TabularInline
. But, my user with view
only permissions can now add or remove these items at will!
comment:35 by , 6 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
Please open a new ticket rather than reopening one for which a fix is already released. A more complete example would be useful.
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.