Opened 2 days ago

Last modified 8 hours ago

#35792 new Cleanup/optimization

Enhance _get_group_permissions method of ModelBackend

Reported by: Bona Fide IT GmbH Owned by:
Component: contrib.auth Version: 4.2
Severity: Normal Keywords: Backend
Cc: Bona Fide IT GmbH Triage Stage: Unreviewed
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 2 days ago.

Download all attachments as: .zip

Change History (3)

by Bona Fide IT GmbH, 2 days ago

Attachment: django.patch​ added

comment:1 by Claude Paroz, 35 hours 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 hours ago

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

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