Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Modeladmin doesn't discover proxy models #4952

Closed

Conversation

ababic
Copy link
Contributor

@ababic ababic commented Dec 11, 2018

Resolves wagtail-nest/wagtail-modeladmin#16.

PermissionHelper.get_all_model_permissions() currently only works for concrete models, because the cross-table query created by the method presumes the model has its own content type. Although there's more work to be done in supporting proxy models more fully throughout Wagtail, there's no reason why PermissionHelper should get in the way here.

The fix utilises the ContentTypeManager's get_for_model() method to fetch a ContentType object for the model - which, when provided with a proxy model, returns the type for the 'concrete' model instead.

Not only does this fix the issue, but it's also more consistent with how content types are used throughout Wagtail. It also simplifies the query made by get_all_model_permissions() slightly, and benefits from the ContentTypeManager's built-in cache.

Andy Babic added 3 commits December 11, 2018 10:26
…ieve the type for the current model, and using that in get_all_model_permissions() instead to query for Permission objects (simplifying the query in the process)
@ababic ababic changed the title Update modeladmin PermissionHelper class to support proxy models Fix: Modeladmin doesn't discover proxy models Dec 12, 2018
@chosak
Copy link
Member

chosak commented Dec 19, 2018

@ababic this works well, thanks.

I did encounter some unexpected behavior, which is perhaps what you are referring to by "there's more work to be done in supporting proxy models". Specifically, given the test example you provide here, with Author and LegendaryAuthor, and ModelAdmins defined for both:

  • A user having only one of "Can view author" permission or "Can view legendary author" permission still sees (read-only) model admin entries for both in the sidebar.
  • A user given Add, Edit, or Delete permission to the Author model unexpectedly gets that permission over only the proxy LegendaryAuthor model, and not the Author model.

Granted, the use case of having ModelAdmins for both models could be rare.

If there's only a single ModelAdmin defined for the proxy model, everything works fine, although there's an unexpected "Can view author" permission option in the Groups screen that doesn't seem to do anything.

A bigger problem comes if there's only a single ModelAdmin defined for the base model. The base model appears in the sidebar, but add/change/delete permissions don't seem to work (and again, there's an unexpected "Can view legendary author" permission option that doesn't do anything).

This all can perhaps be summed up as: if you want to create a proxy model on a model that's being exposed via ModelAdmin, the ModelAdmin has to be defined for the proxy model, and not the base model.

Does this match with your expectations? I'm not that familiar with Django ModelAdmin and if it has any similar constraints with proxy models. If you think it makes sense it might be good to add a note in the ModelAdmin documentation about use of proxy models if it seems likely that users might encounter these issues.

@ababic
Copy link
Contributor Author

ababic commented Dec 19, 2018

Hi @chosak. Thanks so much for taking the time to review this, and for concise feedback.

A lot of these peculiarities are expected, but there are a couple of things that are not. Specifically these two...

A user given Add, Edit, or Delete permission to the Author model unexpectedly gets that permission over only the proxy LegendaryAuthor model, and not the Author model.

A bigger problem comes if there's only a single ModelAdmin defined for the base model. The base model appears in the sidebar, but add/change/delete permissions don't seem to work.

I'll investigate these and see if anything can be done without too much scope creep, but my gut feeling is that separate issues/PRs will be required.

I'm a touch short on time right now, so sorry for not addressing all of your points here. I will find the time to reply more thoroughly soon. Thanks again :)

@ababic
Copy link
Contributor Author

ababic commented Dec 21, 2018

Hi @chosak,

I wanted to start by outlining some of the current behaviour pertaining to proxy models, which I think will aid in the review of these changes:

You may or may not know this, but since proxy models became a thing in Django, Django hasn't ever created a new ContentType object for a proxy model when you migrate. However, it will create a new set of Permission objects for each proxy model - which are actually tied to the base model's ContentType. There's an open ticket in Django to address this, but it's an ongoing effort.

With the above in mind, If you register a ModelAdmin class for a base model (one that is subclassed to create one or more proxy models), for an instance of that class, get_permissions_for_registration() returns a queryset of Permission objects for the base model, e.g:

  • add_author
  • change_author
  • delete_author
  • view_author

AND any proxy models that subclass it, e.g:

  • add_legendaryauthor
  • change_legendaryauthor
  • delete_legendaryauthor
  • view_legendaryauthor

This might not be apparent if you're using the Group permission UI to gauge which permissions are registered, because that view itself has some behaviour that prevents most of those permissions from appearing (with the exception of the 'Can view' permissions for each model - which Django started adding from version 2.1) - which should probably be the subject of a separate issue / PR.

If you register a ModelAdmin class for a proxy model currently, for an instance of that class,
get_permissions_for_registration() will return an empty queryset. Because of this, you'll see no permissions at all in the Group permission UI. This is all because PermissionHelper. get_all_model_permissions() cannot identify any permissions for the model. Another side-effect of this is that the menu item for this ModelAdmin class will never show up, because PermissionHelper doesn't know which permissions it should be checking for.

With all this in mind, I guess the ideal scenario is this:

  • If you register a ModelAdmin class for a proxy model, for an instance of that class permission_helper.get_all_model_permissions() would only return permissions pertaining to that proxy model.
  • If you register a ModelAdmin class for a concrete model, for an instance of that class permission_helper.get_all_model_permissions() would not return any permissions pertaining to proxy models.

This first one seems achievable, and is perhaps something this PR should cover to help reduce potential confusion? Though, the only way I can think to do this currently would be to filter by codename, which seems a little messy to me.

The second one is difficult for a couple of reasons:

  1. We cannot exclude custom permissions that users might have added (which might not follow Django's action_model codename convention), because doing so would break backwards-compatibility.
  2. I don't think there's an easy way to figure out what all the proxy models are for a concrete model, so even if we were cool with excluding permissions based on codename, knowing which codenames to exclude is a bit of a challenge in itself.

What are your thoughts?

@ababic
Copy link
Contributor Author

ababic commented Dec 23, 2018

@chosak I think a lot of the very weird behaviour you reported is actually more to do with what's happening in the Group permissions UI/view. I think it could be unexpectedly showing you a set of checkboxes for LegendaryAuthor permissions, but labelling them as 'Author' permissions.

@ababic
Copy link
Contributor Author

ababic commented Jan 2, 2019

Hi @chosak. I've come to realise there are two ways to resolve this problem...

One that aims to support Django's default proxy model/ContentType/Permission setup (the approach taken by this PR). And another that focuses on ensuring proxy models have their own unique ContentType object, with correctly associated Permissions (the approach outlined in #4973).

Personally, I think the latter is the better approach, which would make this PR redundant. However, I'll await feedback on that issue before closing this.

@chosak
Copy link
Member

chosak commented Jan 2, 2019

@ababic thank you very much for the detailed explanation! You've taught me a thing or two about proxy models. My apologies for the delay in responding (holidays). That 10-year old Django ticket has quite an interesting history.

I agree with your definition of the ideal scenario, and also with the pain that would be required to implement it, given how Django proxy models don't have their own ContentType.

I think a lot of the very weird behaviour you reported is actually more to do with what's happening in the Group permissions UI/view.

I think you're right, based on this code that creates a set of permissions based on content type. I suppose this could be improved to somehow distinguish proxy models, but you'd have to use some kind of codename-based logic (which also wouldn't properly handle custom permissions). It feels like this would just be better fixed if Django gave proxy models their own ContentType (or we do something as you propose in #4973).

I agree that a ContentType-related fix would be better than what's proposed in this PR, if we think developers are likely to run into some of the unexpected issues mentioned above (although I say that reluctantly, given that you've already done the work here). I'll comment over on #4973.

@ababic
Copy link
Contributor Author

ababic commented Jan 2, 2019

I agree that a ContentType-related fix would be better than what's proposed in this PR, if we think developers are likely to run into some of the unexpected issues mentioned above (although I say that reluctantly, given that you've already done the work here).

No worries :) It took doing this work to really get my head around everything, so it was all useful.

@ababic
Copy link
Contributor Author

ababic commented Jan 28, 2019

Am closing this, as the solution relies on Django behaviour that will be changing in 2.2.

@ababic ababic closed this Jan 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modeladmin doesn't discover proxy models
2 participants