Supporting proxy models throughout Wagtail

[ababic]

Issue Summary

While there are a number of issues/PRs relating to support of proxy models in various parts of Wagtail (#1736, wagtail-nest/wagtail-modeladmin#16, #1029, #4273, #3594), I feel that we're missing a trick by only looking at the those issues in isolation. This issue aims to discuss the matter more generally, with an aim to establishing a clean, consistent approach that can be applied throughout the wider project.

Where are we now?

There are a good few places in Wagtail where using proxy models fails to work as expected. The exact details vary from place to place, but I feel they can all really be summarised as follows:

1. Django creates Permission objects for each proxy model, but they are associated with the ContentType for the concrete model.
2. When we want to identify the ContentType for a model, we tend to use ContentType.objects.get_for_model() which, when provided with a proxy model, returns the ContentType for the concrete model by default.
3. ContentType.objects.get_for_model() supports a for_concrete_model option (which has a default value of True), which can be used to retrieve a unique ContentType object for each proxy model instead of that of the concrete model. But, Permission objects for the proxy models remain associated with the ContentType of the concrete model.
4. Because of all of the above, identifying the correct ContentType and Permission objects for a proxy model (or worse, identifying the relevant model from a Permission or ContentType object) becomes tricky.

Where do we want to be?

I think we can all agree that supporting proxy models is much simpler/cleaner when proxy models have their own ContentType, and model permissions are related to that ContentType. In fact, this is how Django intends for things to be from version 2.2

There's a clear desire for proxy model support in Wagtail, and the changes in Django 2.2 will make that easier, but changing our code to benefit from those changes would leave a gap in proxy model support in earlier Django versions, which would be nice to fill somehow.

We want using proxy models in Wagtail to be simple for developers, but we don't want to force wholesale data changes on to every project where Wagtail is installed, so there should at least be an 'opt-in' step.

How might we get there?

Below are the most up-to-date recommendations:

1. Update Wagtail to use for_concrete_model=False and for_concrete_models=False options throughout, wherever ContentType.objects.get_for_model() or ContentType.objects.get_for_models() are used (respectively).
2. For Django versions < 2.2, we utilise Django's post_migrate signal to update proxy model permissions to be associated with their respective ContentType objects, making things consistent accross all supported Django versions.
3. The functionality in step 2 would be disabled by default, so as not to force the Permission changes onto projects that utilise proxy models outside of Wagtail, and might not be planning to update to Django 2.2 anytime soon. Developers would opt-in by adding something like WAGTAIL_UPDATE_PROXY_MODEL_PERMISSIONS = True to their project's Django settings.
4. To make the changes reversible in Django < 2.2, a management command could be added, that would essentially do the opposite of the signal handler in step 2.

As always, looking forward to hearing other's thoughts/suggestions on this :)

[chosak]

@ababic thank you for the detailed writeup.

How much will django/django#10381 buy us here? That PR looks like it has been slowly making its way through review (there's an open request for feedback on django-developers). It adds ContentType support for proxy models, and appears to include a migration that updates existing permissions to properly refer to proxy models.

Granted, if and when this is merged into Django (best case scenario might be 2.3), and Wagtail started relying on it, it'd mean inconsistent behavior for developers using older Django versions. Still, that inconsistency might be worth it if we could avoid having to duplicate some of that logic here.

[ababic]

@chosak it looks like the migration in that PR would remove the need for developers to run a separate script/management command to set up the ContentType and Permission objects, which is good.

Part 2 would also be redundant after those changes, since we would no longer have to worry about non-existent ContentType objects being created by ContentType.objects.get_for_model() - though, we would need to update all usages of that method to use for_concrete_model=False, for the new types to be picked up. But, if we're going to support earlier versions of Django in the same release (highly likely), we will need that too.

[ababic]

@chosak Thinking on this some more, I think we could combine the ContentType / Permission creation logic and the "getting a content type without accidentally creating one" into a single method. This would:

• Make the overall solution more centralised (easier to review, maintain, and eventually remove)
• Simplify the process for developers (we would no longer need to provide separate instructions for different Django versions).
• Only create/modify ContentType and Permission objects for proxy models registered with Wagtail (we'll let Django force that globally when they are ready to).

[arthurio]

FYI django/django#10381 has been merged as part of the Django 2.2 alpha release, let me know if I can help with anything.

[ababic]

@arthurio. Congrats on the contribution. That issue had quite the history! Props for getting stuck in and seeing it through.

I should be able to get a PR submitted for this by the end of this week, at which point I'll be grateful for any feedback :)

[ababic]

@arthurio @chosak I've posted some initial workings in #5003, if you'd like to see how things are coming along. It's early days, but any thoughts or feedback would be greatly appreciated.

[arthurio]

@ababic I'll take a closer look but something seems off to me:

1. Django does not create unique ContentType objects for proxy models.

I'm fairly sure it's not true since Django 1.11. If you have django.contrib.contenttypes in your INSTALLED_APPS, create_contenttypes will be called after each migrate

I think the main concern, as you mentioned, is to enforce the use of for_concrete_model=False across the codebase.
The second concern, to make sure the correct permissions are used for proxy models, I would use something like https://gist.github.com/magopian/7543724#gistcomment-2638750 as a back-port fix and have some conditional code to execute it only for Django < 2.2. The snippet would need to be a bit cleaned up, like using update instead of delete + get_or_create but it should do the trick.

What versions of Django are you planning to support with this change?

[ababic]

Thanks for the feedback @arthurio!

I'm fairly sure it's not true since Django 1.11. If you have django.contrib.contenttypes in your INSTALLED_APPS, create_contenttypes will be called after each migrate

Oh, okay! Thanks for pointing that out. I clearly didn't look into the more recent history thoroughly enough. If this is the case, then the approach taken in #5003 is no good, because new content types will barely ever be created there. I'll resort back to my original idea of solving the permission association problem with some kind of separate script, like the gist you recommended.

What versions of Django are you planning to support with this change?

The last release Wagtail (2.4) dropped support for 1.11, so I'm only planning to support Django 2+ here.

Do you know roughly what affect updating the proxy model permissions will have on the Django admin in 2.0 - 2.1? Would it prevent anything from working there?

[arthurio]

Do you know roughly what affect updating the proxy model permissions will have on the Django admin in 2.0 - 2.1? Would it prevent anything from working there?

If done right, it should not affect anything permission wise for Django admin <2.2. I think you want to do the following:

• One time migration to update existing permissions to use the proxy model’s own content type (copy/paste the one I wrote for Django?)
• Add the post_migrate hack from the gist but make it conditional for Django < 2.2
• Update the wagtail code to rely on for_concrete_model=False. Either via a util function or just documentation?

The reason why you need something like the gist is because Django <2.2 will automatically re-create permissions for proxy models on every migrate and it would be really hard to tell the two permissions apart in the admin (one with the concrete model content type and one with the proxy model content type).

[ababic]

Django <2.2 will automatically re-create permissions for proxy models on every migrate and it would be really hard to tell the two permissions apart in the admin

So, it seems we would need to reassociate permissions for new models here, but also detect and delete duplicate ones as they get recreated each time? If this is the case, then I think this same signal handler might just do for parts 1 & 2 - it would just be making updates only on the first run?

A part of me is starting to doubt whether we really should try to correct proxy model permissions in < 2.2. While it would be useful, I worry that continually trying to patch over Django's behaviour here could be considered as 'overstepping the boundaries', as it would affect models not even registered with / used by Wagtail. @chosak, do you have any thoughts on this? Perhaps we could make the behaviour opt-in only, via a Django setting or something?

[arthurio]

So, it seems we would need to reassociate permissions for new models here,

Yes, and more importantly all the existing models for people upgrading wagtail the first time.

but also detect and delete duplicate ones as they get recreated each time?

Correct (assuming "duplicate" is the same permission code for both concrete and proxy model content types).

If this is the case, then I think this same signal handler might just do for parts 1 & 2 - it would just be making updates only on the first run?

That might be tricky to detect the "first run".

A part of me is starting to doubt whether we really should try to correct proxy model permissions in < 2.2. While it would be useful, I worry that continually trying to patch over Django's behaviour here could be considered as 'overstepping the boundaries', as it would affect models not even registered with / used by Wagtail.

I agree that it would be wiser to wait for a wagtail version that supports only Django 2.2+. Especially since it's still an alpha release. What I suggested are the minimum requirements to make it work IMO.

[arthurio]

as it would affect models not even registered with / used by Wagtail

You could update the script to only look at the wagtail models but it could be hard if someone mixes up wagtail models and others in the same app. Unless you check for inheritance maybe?

[ababic]

You could update the script to only look at the wagtail models but it could be hard if someone mixes up wagtail models and others in the same app. Unless you check for inheritance maybe?

What classes as a 'Wagtail model' is a bit blurry, unfortunately. It could be a subclass of Page, or registered as a Snippet or via wagtail.contrib.modeladmin. While a conditional approach might make sense, it could be tricky to make the rules clear.

That might be tricky to detect the "first run".

I could be missing something, but If we made a query to identify the codenames of Permissions associated with the proxy model first, we could use that to determine which concrete model permissions to delete, and which to update. Then, we wouldn't need to 'detect the first-run' as such.

I agree that it would be wiser to wait for a wagtail version that supports only Django 2.2+. Especially since it's still an alpha release. What I suggested are the minimum requirements to make it work IMO

Okay, thanks. I'm pretty sure I could make this work, but the changes would not be reversible easily, and it's going to be very tricky to test everything to make sure we catered for all scenarios. I think I'd rather wait on a feedback from the core team before going ahead and putting much more time into this.

[chosak]

a wagtail version that supports only Django 2.2+

This would make things easier, but would delay this feature until (I think) the next Wagtail LTS, which would support Django 2.2 as its minimum version. @gasman roughly when do you think this would be?

I agree that the complexity of supporting this in Django < 2.2 might not be worth it, given the difficulties you mention in testing comprehensively and the smelly behavior of reversing Django code each time migrations run.

[gasman]

@chosak Assuming we stick to our current release pattern, the next LTS will be around June/July, and by that time Django 2.2 will only be a couple of months old so it's unlikely that we'll be dropping support for <2.2 at that point. I'd say we're most likely to drop Django <2.2 for the next LTS after that, in early 2020, by which time Django 2.1 will be out of support.

[ababic]

FYI, I've updated the issue description with the most up-to-date implementation recommendations, so that it's clearer to see what is being proposed.

[arthurio]

Thanks for updating @ababic , just a few nits:

• I would remove 1. Django does not create unique ContentType objects for proxy models.
• for_concrete_model=True and for_concrete_models=True should be for_concrete_model=False and for_concrete_models=False

[ababic]

Thanks for the keen eyes @arthurio! Sorted now.

[loicteixeira]

I'm a bit late to the party, but if Django 2.2 solves a big chunk of the problem, I think it would be reasonable to say "you need to use Django 2.2+ to benefit from those changes as well", instead of trying to hack/bend Wagtail so it works with previous versions on Django as well (especially if we need to make sure to always use for_concrete_model which will be forgotten at some point and create bugs).

That being said, I don't know whether it would be easier for Wagtail to support Django newest additions while still being compatible with previous Django version or do what you are trying to accomplish here.

[ababic]

Thanks @loicteixeira.

Django 2.2 solves a big chunk of the problem, I think it would be reasonable to say "you need to use Django 2.2+ to benefit from those changes as well", instead of trying to hack/bend Wagtail

I can totally see why messing with permissions after every migration in Django migration < 2.2 is undesirable. Instead of integrating this into the codebase (where we'd have to test and maintain it), how would you feel about just providing this part as a code snippet in the docs or something?

especially if we need to make sure to always use for_concrete_model which will be forgotten at some point and create bugs

We can take two approaches here:

1. Update everywhere to for_concrete_model=False regardless of Django version, and deal with the fallout.
2. Implement our own util/wrapper methods that conditionally add for_concrete_model=False in Django > 2.2.

With 1, I don't think we'd actually suffer much, because wherever we could support proxy models (pages, snippets, modeladmin), we don't . I agree the Django API is a little confusing here, but it IS the Django API - it would at least be clear what the code is doing.

2 is safer. Would make for more readable code throughout Wagtail. It might also put us in a better position to update Wagtail if the Django API changes here in the near future. But, it is obfuscation.

[ababic]

@loicteixeira A correction on my last post:

because wherever we could support proxy models (pages, snippets, modeladmin), we don't.

I haven't actually tried to register proxy models as Snippets before, so unsure whether that's supported or not. However, #1029 seems to indicate that there are some limitations.

[ababic]

I might also add that the first approach is advantageous, because just using for_concrete_model=False would allow Wagtail to support proxy Page type models in < Django 2.2. This is because we override the permission model for pages, so it's not necessary for the Permission objects created by Django to be associated with the proxy model's content type. The Permission association thing is only really an issue in places where we do things with permissions (e.g. in group edit/modeladmin/snippet/ views)

[ababic]

@chosak @loicteixeira. I figured adding support for proxy Page models would be a good place to start here, as permissions aren't an issue there (Wagtail implements it's own tree-based permission model for pages, and so never cares about the ones associated with a model's ContentType).

The PR also adds some documentation to give an indication of proxy model support in Wagtail, and also introduces a couple of util methods we can build upon to expand support to non-page models.

See #5202

[DanielSwain]

@ababic I appreciate all the effort that you have put into this feature. I don't currently have a use for the Page proxy model functionality, but I really do need to have regular proxy models show correctly in the Django admin. This StOfw issue was useful, but as you can see from my comment (and I know you're aware of this), it only partially solved the problem.

[ababic]

Hi @DanAtShenTech,

Thanks. Yes, these issues are definitely known to me, and I'm planning to fix them soon after #5202 has been merged.

There's currently no way to manage proxy model permissions via the Wagtail admin UI, even if all the correct model permissions have been registered via the correct hooks. The form/view needs updating to surface all models instead of just concrete ones. Even with these changes, it's only going to be possible to manage permissions for each individual proxy model separately in Django 2.2+. I still haven't figured out what to do for earlier versions of Django. One option is to only allow permissions to be configurable for the concrete model, and have those permissions filter down to all proxy models.

EDIT: On reflection, I think the only reasistic option is to just 'stay broken' in Django <2.2. But, we should at least fix the group edit screen to ensure it surfaces the correct permissions next to the concrete model label - currently it is a little unpredictable when proxy models are registered, which can cause a lot of confusion.

[DanielSwain]

@ababic I just finished updating a project that uses several proxy models to Wagtail 2.5/Django 2.2. This StOflw answer contains a brief summary.

A thought: The Django migration to update existing proxy model permissions to the new permissions system was only able to be written because the model name (either concrete or proxy) is at the end of the codename in auth_permission. You mentioned above that Wagtail uses its own permissions system for Page models and so does not rely solely on Django's permissions. If you wanted to support Django <2.2, perhaps you could implement a permissions system for non-Page models that also does not rely solely on Django's permissions. It seems like you could use the latter part of the codename to accomplish this. You would, in effect, be doing, on-the-fly, what that Django 2.2 proxy model migration does one time. I don't know, however, whether the <2.2 permissions structure pre-dates the release of Wagtail. I suspect that it does because proxy model support has been around since at least Django 1.5. But maybe the level of interest in proxy model support in Wagtail prior to 2.5 doesn't even warrant the effort required to accommodate it.

In any event, I think that correct proxy model support is such a compelling feature that it shouldn't be held up trying to do something with Django <2.2. I think that appropriate notes in the documentation (at least in the ModelAdmin Customisation Primer) would be fine. As I said in my StOflw answer, at least now I have full capability to have proxy models appear correctly (for the correct users) in the left side of the Wagtail admin. Being able to set the permissions in the Wagtail admin would just be icing on the cake.

[ababic]

Hi @DanAtShenTech. Thanks for the input. In answer to a few of your points:

But maybe the level of interest in proxy model support in Wagtail prior to 2.5 doesn't even warrant the effort required to accommodate it.

Indeed :) When you account for all of additional machinery needed to store, retrieve and check permissions throughout, it adds up to a lot of work and ongoing maintenance. Doing this soley to support proxy models in earlier versions of django (which we probably won't even be supporting in another couple of years) would likely introduce more problems than it solves.

We'll go for full support with Django >2.2 first of all, then see how we go from there. My intention is to centralise the logic that identifies model permissions, which should put us in a better place to do something about earlier Django versions.

I think that appropriate notes in the documentation (at least in the ModelAdmin Customisation Primer) would be fine.

Yes, #5202 adds something to explain the general state of support. But I imagine a few notes in the relevant parts of the docs will also be useful if support will be conditional based on Django version.

[DanielSwain]

OK @ababic, so within the last couple of days I've been assigning user rights in the project referenced above which was updated to Wagtail 2.5/Django 2.2. I was stunned to see all proxy models appear in the Wagtail admin Groups page ready for permissions assignment along with all other regular Django models. I've assigned permissions and tried the different corresponding logins assigned to different groups, and all works as it should. Does this surprise you too, or is this what you expected?

[ababic]

@DanielSwain that is kind of surprising. I thought the form itself only surfaces permissions for concrete models. But I must say, I have been rather a lot more focussed on the page model side of things for a while. I would hazard a guess that things are only working because your models are registered with modeladmin? I'd expect things might be different if you registered those models as snippets instead. It's these kind of inconsistencies that I think need resolving.

[DanielSwain]

Yes @ababic, these proxy models are all registered with ModelAdmin in wagtailhooks.py, and I just confirmed, by removing the ModelAdmin declarations, that this is what is causing them to appear in the Groups permissions screen.

[ababic]

It's possible I forgot to migrate when I tested things before, which would explain things not showing up in the form :( I'll have another go locally and update the info in #5202 accordingly.

[zerolab]

@DanielSwain could you give #9255 a try? that is the latest and most up-to-date work on this.

[AndersFelde]

To get correct permissions when accessing models through SnippetViewSet, you can use this hook

from django.contrib.auth.models import Permission
from django.contrib.contenttypes.models import ContentType
from .models import MyModel

@hooks.register("register_permissions")
def register_ctf_permissions():
    model = MyModel
    content_type = ContentType.objects.get_for_model(model, for_concrete_model=False)
    return Permission.objects.filter(content_type=content_type)