Remove ability to specify custom group model

[azaghal]

This is a bit of a free-form/meta issue in terms of what should be done regarding ability to specify a custom Django group model, as described in documentation.

The problem as I see it is that nowhere in Django documentation can one find any sort of information about providing a custom group model. The closest thing to it is mentioned in instructions for customizing authentication in Django:

# ... and, since we're not using Django's built-in permissions,
# unregister the Group model from admin.
admin.site.unregister(Group)

Additionally, there are also no Django settings (as far as I know) similar to AUTH_USER_MODEL for a custom group model.

Furthermore, documentation for the Django Wiki setting provides no example on what the group model should look like (this would be the minimum, I think, to be able to use the feature at all).

It should be noted that there are multiple parts of the code that will perform permission checking that will most likely break authentication checks. I ran into the issue while trying to improve test coverage. If needed, I can provide a quick example of how it breaks.

So, to recap reasons for removal:

• Custom group model is not supported by Django Admin.
• Django Wiki documentation lacks instructions on what the group model should satisfy in terms of API.
• Django Wiki code will indirectly depend on Django Admin group model internals (via the User.has_perm call).

Reasons against removal:

• Somebody might be using this thing out in production. Presumably the person who contributed the patches. It would be nice to see if people are actually using this and how.

[benjaoming]

@azaghal if you specify a custom User object, it's quite likely that you will also end up specifying a custom Group, simply because you might disable django.contrib.auth entirely.

Some work was done to add the support here: https://github.com/django-wiki/django-wiki/commit/5595dbc246e157fef86cd338515b571a8a160b22

The original PR is here: https://github.com/django-wiki/django-wiki/pull/465

CC: @fritz-k

[benjaoming]

@azaghal unless you have a strong objection, I think it's reasonable to close this with the following arguments:

1. The current implementation actually works
2. Someone did seem to implement support for it with a valid use case
3. It's not documented by Django but seems to be an intended use in the way django.contrib.auth was implemented

This leaves a bit to desire from our implementation of course:

1. Missing documentation
2. Proper test coverage that catches the issues with the incomplete CustomGroup model in tests/testdata

?

[fritz-k]

Per default User.has_perm method is delegated to the django.contrib.auth.backends.ModelBackend which queries user and group permissions. If you customize the User and/or Group models they have to comply with django.contrib.auth.backends.ModelBackend or you have to modify the authentication backend. Otherwise the built-in authentication/authorization of Django as well as all packages relying on those will fail.

Django Wiki did't impose any additional requirements on the Group model at the time of #465 and I doubt this has changed since.

Edit: I haven't worked with Django for a while so I'm not that firm on its internals anymore. @azaghal could you give an example on how this issue arose? That could help determine how to improve the documentation or implementation of this feature.

[azaghal]

@fritz-k (the stuff I'm about to write will sound harsh, but I'm stating it as matter-of- factly, please do not get offended or take it the wrong way, I am merely stating my opinion on this :)

What I would like to point-out is that Django does not provide any documentation on how to provide customized Group model. If you have a link pointing to something like that, let me know. The only thing I can see in their docs is how to provide custom User model, and this does include documentation on required interface. And this can be considered a supported use-case. Anything else is depending on Django's own internals.

Telling people to "go look into the code of Django framework to figure stuff out" is a very, very bad idea/practice. I could understand it in case of poorly documented frameworks, but Django is pretty precise and thorough in that regard.

As for the failure, this happened when a particular piece of code got hit while I implemented some tests for improving test coverage. This happened because the test implementation had a dummy Group class. Which lead me to try to figure out what the correct implementation should look like, and there was no docs on this, as mentioned, neither in Django nor Django Wiki itself. This has actually happened when I tried using a non-superuser Django user for a test. There is a couple of code paths that behave differently for superuser, so that's why this never surfaced in tests (since existing tests almost exclusively use superuser).

At the end of the day, the real question is - does anybody use this functionality? If not, it would be better to remove it due to concerns I've listed above.

[azaghal]

@benjaoming The way I see it, I think the best way forward while keeping the feature would be to:

1. Update documentation to state how the setting should be used. Even if mentioning a "go see Django auth code" (preferable to this would be to actually show a minimal meaningful custom group implementation).

2. State a super-clear warning that this feature is risky if not done right, since it depends on Django internals, and that it might have weird interactions with 3rd-party apps and even Django built-in provided apps (most likely the auth one).

3. Implement valid custom Group model in tests.

4. Since this can break tests for normal use-case, limit the custom group model to only a specific set of tests (e.g. the ones where custom group is actually used). The reason is that it had me scratching my head for a while until I realised what's going on when I was adding new tests :) The backtrace shown is not quite a normal one, and you're unlikely to run into it by just searching on the Internet.

[azaghal]

I should maybe point out that personally I'd be fine even with just option (4) - since I don't think anyone (and by this primarily me :D )should be using this feature.

[azaghal]

@fritz-k Oh, and thanks for getting back to us, much appreciated :)

[benjaoming]

@azaghal I like the option 3), I would think that it's just a has_perm method that's missing from the test group?

Django didn't write documentation, probably because they couldn't find enough support to make this a truly public interface. It's kind of the same with swappable models.. they were introduced to make the User model swappable, hence a public interface... but not really something anyone would like to see becoming popular.

I think what django-wiki can do is just to support what we perceive is something that Django allows, and then also not document it :)

[azaghal]

Well, given I'm pretty opinionated, I can provide (4), to be honest. This would also let me contribute a couple of tests centred around regular user authorisation. The (3) could be done by whomever wants to delve into that can of worms. Let me know if this option would be acceptable to you :)

[benjaoming]

At least it's not a can of bugs :)

[fritz-k]

I think using something like

class CustomGroup(django.contrib.auth.models.Group):
    pass

and checking that this model is used and everything still works should suffice on django-wiki's side.

While Django does not explicitly state how to customize the group model it says explicitly (e.g. here and there) that User.has_perm is one of the default interfaces for authorization. CustomGroup will definitely comply with Django's permission system. Your tests would then make sure that django-wiki can work with a custom group model while the specifics of its implementation are left to the user of that feature. If you wish to guide him/her a little bit, you could point towards that second link and mention that any custom group model has to work with the authorization system.

Edit: Too late xD @azaghal Glad to help! As you might have guessed I am (possibly the only one) using this feature ^^

[benjaoming]

@fritz-k -- I was about to implement something like that, but I think actually we shouldn't test custom group models at all, if we use a test model that implements the concrete (as in non-abstract) Group model? It just seems a bit half-motivated then. While the previous test didn't require any effort at all, then an actual full testing with a custom group model seems to be a bit too demanding?

Anyways, I'm expanding it a bit in #835 -- and maybe if it works for @azaghal 's case then we could keep it -- otherwise I would also say let's go with option 4) and just skip tests... and hope for the best :)

[benjaoming]

Update: It didn't work. I cannot see that a CustomGroup model can be implemented without changing AnonymousUser, too.

So I would think that someone had to do a lot of documentation and boiler plate to showcase how the Group model can be swapped.

We could leave the setting in place still, I think it's not impossible to customize groups... just pretty impossible to implement proper testing for it without writing a whole bunch of half-hearted code.

It's possible to test it by setting the custom group to django.contrib.auth.models.Group.

[fritz-k]

The last point guarantees that in django-wiki you can use a custom group and that there is at least one implementation of such a group that works (which is "incidentally" a group implementing the exact same methods as django's builtin).

With these guarantees, I can easily add the required methods to my custom group model. I'd expect someone tinkering with django's internals to be able to look into the code and find out which methods those are, which - IMO - makes proper documentation of such advanced features nothing critical.