[Django] #35792: Enhance _get_group_permissions method of ModelBackend

[Django]

#35792: Enhance _get_group_permissions method of ModelBackend
-------------------------------------+-------------------------------------
Reporter: Bona Fide IT GmbH | Type:
| Cleanup/optimization
Status: new | Component:
| contrib.auth
Version: 4.2 | Severity: Normal
Keywords: Backend | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
We would like to suggest a change to the `_get_group_permissions` method
in the django `ModelBackend`.

The current code gets the registered `User` model and from it the `groups`
field to get the related query name to compose a filter query for the
`Permission` model at the end.

{{{#!div style="font-size: 80%"
Current version of `django/contrib/auth/backends.py`
(https://github.com/django/django/blob/main/django/contrib/auth/backends.py#L61-L64)
:
{{{#!python
from django.contrib.auth import get_user_model
from django.contrib.auth.models import Permission
from django.db.models import Exists, OuterRef, Q


UserModel = get_user_model()

...

class ModelBackend(BaseBackend):
...

def _get_group_permissions(self, user_obj):
user_groups_field = get_user_model()._meta.get_field("groups")
user_groups_query = "group__%s" %
user_groups_field.related_query_name()
return Permission.objects.filter(**{user_groups_query: user_obj})

...
}}}
}}}



Since in this method both the `group` field of the `Permission` model and
the reverse relation field `groups` of the [custom] `User` model are fixed
anyway, we suggest the following change.

{{{#!div style="font-size: 80%"
{{{#!python
class ModelBackend(BaseBackend):
...

def _get_group_permissions(self, user_obj):
return Permission.objects.filter(group__in=user_obj.groups.all())

...
}}}
}}}


We are not sure if this will fundamentally change the performance of the
query, but it seems to us to be a simpler variant than going via `_meta`
and then constructing a query string that is fixed anyway.

A further argument from our side is that this would probably also make it
possible to use a custom `Group` model and to solve problems such as those
mentioned here: https://github.com/django-guardian/django-
guardian/pull/504#issuecomment-429810401 without introducing something
like an `AUTH_GROUP_MODEL` setting.
--
Ticket URL: <https://code.djangoproject.com/ticket/35792>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#35792: Enhance _get_group_permissions method of ModelBackend
-------------------------------------+-------------------------------------

Reporter: Bona Fide IT GmbH | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: contrib.auth | Version: 4.2
Severity: Normal | Resolution:

Keywords: Backend | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Bona Fide IT GmbH):

* Attachment "django.patch" added.

[Django]

#35792: Enhance _get_group_permissions method of ModelBackend
-------------------------------------+-------------------------------------
Reporter: Bona Fide IT GmbH | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: contrib.auth | Version: 4.2
Severity: Normal | Resolution:
Keywords: Backend | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Claude Paroz):

Thanks for the suggestion. It would be nice to submit your patch as a pull
request on GitHub so that we may see the outcome of the change in the test
suite.
--
Ticket URL: <https://code.djangoproject.com/ticket/35792#comment:1>

[Django]

#35792: Enhance _get_group_permissions method of ModelBackend
-------------------------------------+-------------------------------------
Reporter: Bona Fide IT GmbH | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: contrib.auth | Version: 4.2
Severity: Normal | Resolution:
Keywords: Backend | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Bona Fide IT GmbH):

We added a pull request on GitHub,
https://github.com/django/django/pull/18633.
--
Ticket URL: <https://code.djangoproject.com/ticket/35792#comment:2>

[Django]

#35792: Enhance _get_group_permissions method of ModelBackend
-------------------------------------+-------------------------------------
Reporter: Bona Fide IT GmbH | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: contrib.auth | Version: 4.2
Severity: Normal | Resolution:
Keywords: Backend | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Bona Fide IT GmbH):

As far as we could see, there are no problems with the django test cases
(https://github.com/django/django/pull/18633/checks).

The benchmark test also seems to be successful: `BENCHMARKS NOT
SIGNIFICANTLY CHANGED`.

So it seems to be a good change, doesn't it?
--
Ticket URL: <https://code.djangoproject.com/ticket/35792#comment:3>

[Django]

#35792: Enhance _get_group_permissions method of ModelBackend

--------------------------------------+------------------------------------

Reporter: Bona Fide IT GmbH | Owner: (none)

Type: Cleanup/optimization | Status: new

Component: contrib.auth | Version: 4.2
Severity: Normal | Resolution:

Keywords: Backend | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

--------------------------------------+------------------------------------
Changes (by Sarah Boyce):

* stage: Unreviewed => Accepted

Comment:

Tentatively accepting

Looks like the SQL before:
{{{#!sql
SELECT "django_content_type"."app_label" AS "content_type__app_label",
"auth_permission"."codename" AS "codename"
FROM "auth_permission"
INNER JOIN "auth_group_permissions" ON ("auth_permission"."id" =
"auth_group_permissions"."permission_id")
INNER JOIN "auth_group" ON ("auth_group_permissions"."group_id" =
"auth_group"."id")
INNER JOIN "auth_user_groups" ON ("auth_group"."id" =
"auth_user_groups"."group_id")
INNER JOIN "django_content_type" ON ("auth_permission"."content_type_id" =
"django_content_type"."id")
WHERE "auth_user_groups"."user_id" = 3
}}}

SQL after:
{{{#!sql
SELECT "django_content_type"."app_label" AS "content_type__app_label",
"auth_permission"."codename" AS "codename"
FROM "auth_permission"
INNER JOIN "auth_group_permissions" ON ("auth_permission"."id" =
"auth_group_permissions"."permission_id")
INNER JOIN "django_content_type" ON ("auth_permission"."content_type_id" =
"django_content_type"."id")
WHERE "auth_group_permissions"."group_id" IN
(SELECT U0."id"
FROM "auth_group" U0
INNER JOIN "auth_user_groups" U1 ON (U0."id" = U1."group_id")
WHERE U1."user_id" = 3)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35792#comment:4>

[Django]

#35792: Enhance _get_group_permissions method of ModelBackend

-------------------------------------+-------------------------------------
Reporter: Bona Fide IT GmbH | Owner: Bona Fide
Type: | IT GmbH
Cleanup/optimization | Status: assigned

Component: contrib.auth | Version: 4.2
Severity: Normal | Resolution:
Keywords: Backend | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Sarah Boyce):

* owner: (none) => Bona Fide IT GmbH
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/35792#comment:5>

[Django]

#35792: Enhance _get_group_permissions method of ModelBackend
-------------------------------------+-------------------------------------
Reporter: Bona Fide IT GmbH | Owner: Bona Fide
Type: | IT GmbH
Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: 4.2
Severity: Normal | Resolution:

Keywords: Backend | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Sarah Boyce):

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/35792#comment:6>

[Django]

#35792: Enhance _get_group_permissions method of ModelBackend
-------------------------------------+-------------------------------------
Reporter: Bona Fide IT GmbH | Owner: Bona Fide
Type: | IT GmbH

Cleanup/optimization | Status: closed
Component: contrib.auth | Version: 4.2
Severity: Normal | Resolution: fixed

Keywords: Backend | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce <42296566+sarahboyce@…>):

* resolution: => fixed
* status: assigned => closed

Comment:

In [changeset:"d4e4520efb553d2bfcc68ac8cf007c0c402d4845" d4e4520e]:
{{{#!CommitTicketReference repository=""
revision="d4e4520efb553d2bfcc68ac8cf007c0c402d4845"
Fixed #35792 -- Simplified ModelBackend._get_group_permissions().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35792#comment:7>