Looking for feedback on implementation of UserManager.with_perm()

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

Looking for feedback on implementation of UserManager.with_perm()

Berker Peksağ
https://github.com/django/django/pull/7153/ implements
UserManager.with_perm() [1] as:

    def with_perm(self, perm):
        for backend in auth.get_backends():
            if hasattr(backend, 'with_perm'):
                return backend.with_perm(perm)
        return self.get_queryset().none()

[1] "Shortcut to get users by permission":
https://code.djangoproject.com/ticket/18763

With this implementation, users of UserManager.with_perm() won't get
users with permissions for all backends. Also, result of
UserManager.with_perm() will depend on the order of
settings.AUTHENTICATION_BACKENDS. See also
https://code.djangoproject.com/ticket/18763#comment:9 for more
information about the current strategy.

I suggested an alternative approach at
https://github.com/django/django/pull/7153/files#r78226234 with the
following implementation:

    def with_perm(self, perm, backend=None):
        if backend is None:
            backends = _get_backends(return_tuples=True)
            if len(backends) != 1:
                raise ValueError(
                    'You have multiple authentication backends configured and '
                    'therefore must provide the `backend` argument.'
                )
            _, backend = backends[0]
            if hasattr(backend, 'with_perm'):
                return backend.with_perm(perm)
        else:
            backend = load_backend(backend)
            if hasattr(backend, 'with_perm'):
                return backend.with_perm(perm)
        return self.get_queryset().none()

This also simulates what django.contrib.auth.login() does when
multiple authentication backends are defined:

https://github.com/django/django/blob/18c72d59e0807dae75ac2c34890d08c1e0972d0a/django/contrib/auth/__init__.py#L100

Tim suggested to get some feedback about possible use cases:

"I'm not sure about the use cases. For example, someone might want to
get users with permissions for all backends. It would be nice if we
had some feedback about what users are implementing on their own to
confirm we're targeting the largest use case."

Is there any other possible use cases? Which one of the suggested
approaches cover the largest use case?

Thanks!

--Berker

--
You received this message because you are subscribed to the Google Groups "Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAF4280%2BmaOn6m%2BcoHDDdhQaUGNfOvw_KSf%2BsnMEtc_EF-pRn6Q%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: Looking for feedback on implementation of UserManager.with_perm()

Nick Pope
Hi Berker,

I just wanted to highlight my comment on the PR here for the benefit of those discussing this:

    https://github.com/django/django/pull/7153#issuecomment-242672721

We currently horribly abuse the existing permission system to add additional global permissions in a hacky way by manually adding content types. (https://code.djangoproject.com/ticket/24754 would be awesome) 

My scenario is simply having an easy method to find users and groups with a particular permission or set of permissions. Doing so is rather clunky at the moment.
I also feel that any solution should have the ability to optionally include/exclude by is_superuser and is_active flags - we often want to know who has a permission whether implicit or explicit.

Thanks,

Nick

On Wednesday, 14 September 2016 05:10:41 UTC+1, Berker Peksag wrote:
<a href="https://github.com/django/django/pull/7153/" target="_blank" rel="nofollow" onmousedown="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fgithub.com%2Fdjango%2Fdjango%2Fpull%2F7153%2F\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNFC1FtBjXj40IJWu8b_1UNVGXEhhA&#39;;return true;" onclick="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fgithub.com%2Fdjango%2Fdjango%2Fpull%2F7153%2F\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNFC1FtBjXj40IJWu8b_1UNVGXEhhA&#39;;return true;">https://github.com/django/django/pull/7153/ implements
UserManager.with_perm() [1] as:

    def with_perm(self, perm):
        for backend in auth.get_backends():
            if hasattr(backend, 'with_perm'):
                return backend.with_perm(perm)
        return self.get_queryset().none()

[1] "Shortcut to get users by permission":
<a href="https://code.djangoproject.com/ticket/18763" target="_blank" rel="nofollow" onmousedown="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fcode.djangoproject.com%2Fticket%2F18763\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNFgtgXRWEuunqf8WYEQTunbcNlfDA&#39;;return true;" onclick="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fcode.djangoproject.com%2Fticket%2F18763\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNFgtgXRWEuunqf8WYEQTunbcNlfDA&#39;;return true;">https://code.djangoproject.com/ticket/18763

With this implementation, users of UserManager.with_perm() won't get
users with permissions for all backends. Also, result of
UserManager.with_perm() will depend on the order of
settings.AUTHENTICATION_BACKENDS. See also
<a href="https://code.djangoproject.com/ticket/18763#comment:9" target="_blank" rel="nofollow" onmousedown="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fcode.djangoproject.com%2Fticket%2F18763%23comment%3A9\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNFRlm63V9gYBkU19AshippDfQLO_Q&#39;;return true;" onclick="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fcode.djangoproject.com%2Fticket%2F18763%23comment%3A9\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNFRlm63V9gYBkU19AshippDfQLO_Q&#39;;return true;">https://code.djangoproject.com/ticket/18763#comment:9 for more
information about the current strategy.

I suggested an alternative approach at
<a href="https://github.com/django/django/pull/7153/files#r78226234" target="_blank" rel="nofollow" onmousedown="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fgithub.com%2Fdjango%2Fdjango%2Fpull%2F7153%2Ffiles%23r78226234\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNHIrl75wh-FR2X5tM2X5-SCgZ3zZg&#39;;return true;" onclick="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fgithub.com%2Fdjango%2Fdjango%2Fpull%2F7153%2Ffiles%23r78226234\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNHIrl75wh-FR2X5tM2X5-SCgZ3zZg&#39;;return true;">https://github.com/django/django/pull/7153/files#r78226234 with the
following implementation:

    def with_perm(self, perm, backend=None):
        if backend is None:
            backends = _get_backends(return_tuples=True)
            if len(backends) != 1:
                raise ValueError(
                    'You have multiple authentication backends configured and '
                    'therefore must provide the `backend` argument.'
                )
            _, backend = backends[0]
            if hasattr(backend, 'with_perm'):
                return backend.with_perm(perm)
        else:
            backend = load_backend(backend)
            if hasattr(backend, 'with_perm'):
                return backend.with_perm(perm)
        return self.get_queryset().none()

This also simulates what django.contrib.auth.login() does when
multiple authentication backends are defined:

<a href="https://github.com/django/django/blob/18c72d59e0807dae75ac2c34890d08c1e0972d0a/django/contrib/auth/__init__.py#L100" target="_blank" rel="nofollow" onmousedown="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fgithub.com%2Fdjango%2Fdjango%2Fblob%2F18c72d59e0807dae75ac2c34890d08c1e0972d0a%2Fdjango%2Fcontrib%2Fauth%2F__init__.py%23L100\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNFPIuGZ-Ghm_7iWvTMRS2U7tP48Uw&#39;;return true;" onclick="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fgithub.com%2Fdjango%2Fdjango%2Fblob%2F18c72d59e0807dae75ac2c34890d08c1e0972d0a%2Fdjango%2Fcontrib%2Fauth%2F__init__.py%23L100\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNFPIuGZ-Ghm_7iWvTMRS2U7tP48Uw&#39;;return true;">https://github.com/django/django/blob/18c72d59e0807dae75ac2c34890d08c1e0972d0a/django/contrib/auth/__init__.py#L100

Tim suggested to get some feedback about possible use cases:

"I'm not sure about the use cases. For example, someone might want to
get users with permissions for all backends. It would be nice if we
had some feedback about what users are implementing on their own to
confirm we're targeting the largest use case."

Is there any other possible use cases? Which one of the suggested
approaches cover the largest use case?

Thanks!

--Berker

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/c5249500-d576-44ab-b38a-78ac22866782%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.