Admin Inlines do not respect user permissions

[p.patruno@…]

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

[James Bennett]

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.

[(none)]

Milestone post-1.0 deleted

[Karen Tracey]

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

[Russell Keith-Magee]

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

[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.

[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.

[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.

[Stephan Jaensch]

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.

[Carl Meyer]

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.

[Stephan Jaensch]

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

[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.

[Carl Meyer]

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.

[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?

[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_*

[Stephan Jaensch]

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.

[Carl Meyer]

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.

2. You can get an empty version of any queryset with the .none() method, rather than using pk__isnull=True.

3. 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.

[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?

[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.

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?

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!

[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. :)

[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!

[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.

[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.

[Stephan Jaensch]

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

[Stephan Jaensch]

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.

[Stephan Jaensch]

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

[Carl Meyer]

In [16934]:

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

[pm4public@…]

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

[Aymeric Augustin]

It's fixed in 1.4.

[Will Gordon]

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!

[Tim Graham]

Please open a new ticket rather than reopening one for which a fix is already released. A more complete example would be useful.