Opened 8 weeks ago

Closed 6 weeks ago

#35792 closed Cleanup/optimization (fixed)

Enhance _get_group_permissions method of ModelBackend

Reported by: Bona Fide IT GmbH Owned by: Bona Fide IT GmbH
Component: contrib.auth Version: 4.2
Severity: Normal Keywords: Backend
Cc: Bona Fide IT GmbH Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Current version of django/contrib/auth/backends.py (​https://github.com/django/django/blob/main/django/contrib/auth/backends.py#L61-L64) : 

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. 

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.

Attachments (1)

django.patch​ (719 bytes ) - added by Bona Fide IT GmbH 8 weeks ago.

Download all attachments as: .zip

Change History (8)

by Bona Fide IT GmbH, 8 weeks ago

Attachment: django.patch​ added

comment:1 by Claude Paroz, 8 weeks ago

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.

comment:2 by Bona Fide IT GmbH, 8 weeks ago

We added a pull request on GitHub, ​https://github.com/django/django/pull/18633.

comment:3 by Bona Fide IT GmbH, 7 weeks ago

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?

comment:4 by Sarah Boyce, 6 weeks ago

Triage Stage: Unreviewed → Accepted

Tentatively accepting

Looks like the SQL before:

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:

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)

comment:5 by Sarah Boyce, 6 weeks ago

Owner: set to Bona Fide IT GmbH
Status: new → assigned

comment:6 by Sarah Boyce, 6 weeks ago

Triage Stage: Accepted → Ready for checkin

comment:7 by Sarah Boyce <42296566+sarahboyce@…>, 6 weeks ago

Resolution: → fixed
Status: assigned → closed

In d4e4520e:

Fixed #35792 -- Simplified ModelBackend._get_group_permissions().

Note: See TracTickets for help on using tickets.
Back to Top