Fixed #18763 -- Added with_perm() to User manager. #7153

berkerpeksag · 2016-08-25T16:26:21Z

https://code.djangoproject.com/ticket/18763

ngnpope · 2016-08-26T09:00:32Z

docs/ref/contrib/auth.txt

...an empty query set...

This seems like it should be saying that it doesn't return inactive users but is saying something else.

ngnpope · 2016-08-26T09:00:40Z

Generally I think this would be useful - I've wanted to know which users have which permissions in the past. However I can think of a many other cases that would be useful:

Which groups have certain permissions, i.e. add GroupManager.with_perm()
Which users have certain permissions directly assigned, i.e. not from membership of a group.
I may want to know what users have permissions regardless of whether they are active.
I may want to to exclude superusers from the output - only show explicit assignations.

Also of note is the obj parameter which is untested. According to the documentation with_perm() should return no users if obj is not None.

timgraham · 2016-09-09T18:07:44Z

docs/releases/1.11.txt

The new UserManger.with_perm() method returns all users...

berkerpeksag · 2016-09-13T08:41:33Z

Hi @pope1ni,

Thank you for review and suggestions.

Which groups have certain permissions, i.e. add GroupManager.with_perm()

This should probably needs to be discussed in a separate ticket.

I may want to know what users have permissions regardless of whether they are active.
I may want to to exclude superusers from the output - only show explicit assignations.

+1 for adding is_superuser and is_active parameters (or exclude_*) Tim, what do you think?

berkerpeksag · 2016-09-13T08:48:29Z

I think I've addressed all review comments except https://github.com/django/django/pull/7153/files#r78226234 (pending discussion) Thanks!

berkerpeksag · 2016-09-22T20:49:49Z

I've now implemented both my idea from the django-developers thread and Nick Pope's suggestions about adding is_superuser and is_active flags.

rivol · 2016-11-06T10:58:10Z

Two comments regarding is_active and is_superuser parameters:

is_active has weird behavior IMHO. It's impossible to get ALL users with a permission, including both active and non-active users. I would skip filtering by UserModel.is_active when the value of is_active parameter is None. The valid param values would then be True/False/None. The default value could probably remain True as you only care about active users most of the time (?).
I would rename is_superuser to indicate that it selects whether all superusers are automatically included in results. Maybe e.g. include_superusers?

timgraham · 2016-12-30T19:26:14Z

Closing due to inactivity.

berkerpeksag · 2016-12-30T20:22:52Z

I'm not sure I understand why you closed it. I didn't get any feedback or review comments from you or someone else on the Django team since September. If you think Rovi's suggestions should be addressed, you should have left a comment (for example "did you see Rovi's suggestions?" or "do you have time to address the suggestions Rovi made?") before prematurely closing the pull request.

timgraham · 2016-12-30T20:50:05Z

Oh, it's a misunderstanding that a team member has to validate feedback from someone else. You two could at least discuss among yourselves and come to a consensus. I guess it would be better to discuss it on the mailing list thread rather than here. Anyway, I had marked the ticket as "patch needs improvement" given the unresolved comment, so it wasn't going anywhere in the current state.

tests/auth_tests/test_models.py

berkerpeksag · 2017-01-05T00:18:46Z

Well, from a contributor's point of view, it doesn't matter whether we (the contributors) have reached a consensus on our end or not if it doesn't get accepted by the core developers. So what I was trying to say is that at some point it would be nice to know whether we are on the same page or not.

Anyway, thanks for reopening it! I will try to address all review comments this week.

timgraham · 2017-01-05T00:39:44Z

Unfortunately, it's impossible for a team member to give feedback on every issue. The lack of feedback on the mailing list is concerning (maybe there's little demand for this) but If two people come to a consensus, that's a good start.

rixx · 2019-04-16T11:56:35Z

@berkerpeksag Are you still interested in pursuing this PR?

berkerpeksag · 2019-04-16T13:19:48Z

@rixx thank you for the ping! Yes, I'm still interested in getting this merged. However, last time I discussed it on django-developers, I wasn't able to gather much positive feedback. Perhaps it should be released on PyPI or proposed to a project such as django-extensions first?

rixx · 2019-04-16T13:48:06Z

@berkerpeksag As far as I could see, there were still some discussions that were not resolved, and the PR is by nature limited, so there are at least documentation warnings missing ("only works with the Django permission backend" would be a start). But I still think this could make for a helpful feature in Django.

berkerpeksag · 2019-04-16T13:59:34Z

I'm glad you found it useful :) I will try to update my branch this weekend.

ngnpope · 2019-04-16T16:19:06Z

@berkerpeksag I'd be happy to pick up reviewing this one again and to see some of the other improvements I mentioned up thread implemented. Will keep an eye out for your rebase...

berkerpeksag · 2019-04-16T16:23:57Z

@pope1ni thank you again! I will make sure I've addressed your comments and ping both you and @rixx.

ngnpope

On second thought, here is something to get you going...

django/contrib/auth/backends.py

docs/releases/1.11.txt

docs/ref/contrib/auth.txt

tests/auth_tests/test_models.py

django/contrib/auth/models.py

tests/auth_tests/test_models.py

django/contrib/auth/models.py

tests/auth_tests/test_models.py

berkerpeksag · 2019-04-18T19:42:41Z

I've addressed most of the review comments. I will add support for Permission instances later tonight.

felixxm · 2019-05-20T10:46:09Z

django/contrib/auth/backends.py

+                    groups__permissions__content_type__app_label=app_label)
+        has_permission_q = Q(is_active=is_active) & (Q(is_superuser=is_superuser) | user_q | group_q)
+
+        return UserModel._default_manager.filter(has_permission_q).distinct()


I would make it more DRY and readable, e.g.

if isinstance(perm, Permission): has_permission = Q(user_permissions=perm) | Q(groups__permissions=perm) else: has_permission = Q( Q(user_permissions__codename=codename) & Q(user_permissions__content_type__app_label=app_label) ) | Q ( Q(groups__permissions__codename=codename) & Q(groups__permissions__content_type__app_label=app_label) ) return UserModel._default_manager.filter( Q(is_active=is_active) & Q(Q(is_superuser=is_superuser) | has_permission) ).distinct()

What do you think?

I wonder if we can use Exists() to avoid distinct() call.

Yes, your suggestion definitely looks better. I'll update the PR. Thanks!

Do you mean the queryset.exists() method or does Exists() mean something else?

I was talking about Exists() subquery.

tests/auth_tests/test_models.py

felixxm · 2019-06-26T10:08:40Z

@berkerpeksag Thanks for this patch and your patience 🧘‍♂️ Are you able to prepare this for a final review? (rebase, squash commits, address two nitpicks from me).

berkerpeksag · 2019-06-26T11:52:59Z

Thank you for the review comments! I will address all of them this week.

On Wed, 26 Jun 2019 at 13.10, Mariusz Felisiak ***@***.***> wrote: @berkerpeksag <https://github.com/berkerpeksag> Thanks for this patch and your patience 🧘‍♂️ Are you able to prepare this for a final review? (rebase, squash commits, address two nitpicks from me). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7153?email_source=notifications&email_token=AAAGNYVLJUGUO6OEPVZGC6TP4M6A5A5CNFSM4CNVA2P2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYTBAZY#issuecomment-505811047>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAGNYQIKHQJ67YKGRBQT5TP4M6A5ANCNFSM4CNVA2PQ> .

-- --Berker

felixxm · 2019-07-09T11:43:57Z

Please uncheck Patch needs improvement flag on the ticket when it's ready.

felixxm · 2019-08-03T09:09:22Z

Superseded by #11625.

ngnpope reviewed Aug 26, 2016
View reviewed changes

timgraham reviewed Sep 9, 2016
View reviewed changes

docs/releases/1.11.txt Outdated

Copy link

Member

timgraham Sep 9, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new UserManger.with_perm() method returns all users...

timgraham closed this Dec 30, 2016

timgraham reopened this Dec 30, 2016

timgraham reviewed Dec 30, 2016

View reviewed changes

tests/auth_tests/test_models.py Outdated Show resolved Hide resolved

ngnpope suggested changes Apr 16, 2019

View reviewed changes

berkerpeksag added 2 commits April 18, 2019 21:52

Fixed #18763 -- Added with_perm() to User manager.

a5e5efa

Address most of the review comments

5d72120

Fix isort

4c49c1e

berkerpeksag added 2 commits April 21, 2019 22:49

Add support for Permission instances

132689e

Drop with_perm from test names

108b447

felixxm reviewed May 20, 2019

View reviewed changes

felixxm reviewed Jun 26, 2019

View reviewed changes

tests/auth_tests/test_models.py Show resolved Hide resolved

ngnpope mentioned this pull request Aug 2, 2019

Fixed #18763 -- Added ModelBackend/UserManager.with_perm() methods. #11625

Merged

felixxm closed this Aug 3, 2019

Uh oh!

Fixed #18763 -- Added with_perm() to User manager. #7153

Fixed #18763 -- Added with_perm() to User manager. #7153

Uh oh!

Conversation

berkerpeksag commented Aug 25, 2016

Uh oh!

ngnpope Aug 26, 2016

Choose a reason for hiding this comment

Uh oh!

timgraham Sep 9, 2016

Choose a reason for hiding this comment

Uh oh!

ngnpope commented Aug 26, 2016

Uh oh!

timgraham Sep 9, 2016

Choose a reason for hiding this comment

Uh oh!

berkerpeksag commented Sep 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

berkerpeksag commented Sep 13, 2016

Uh oh!

berkerpeksag commented Sep 22, 2016

Uh oh!

rivol commented Nov 6, 2016

Uh oh!

timgraham commented Dec 30, 2016

Uh oh!

berkerpeksag commented Dec 30, 2016

Uh oh!

timgraham commented Dec 30, 2016

Uh oh!

Uh oh!

berkerpeksag commented Jan 5, 2017

Uh oh!

timgraham commented Jan 5, 2017

Uh oh!

rixx commented Apr 16, 2019

Uh oh!

berkerpeksag commented Apr 16, 2019

Uh oh!

rixx commented Apr 16, 2019

Uh oh!

berkerpeksag commented Apr 16, 2019

Uh oh!

ngnpope commented Apr 16, 2019

Uh oh!

berkerpeksag commented Apr 16, 2019

Uh oh!

ngnpope left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

berkerpeksag commented Apr 18, 2019

Uh oh!

felixxm May 20, 2019

Choose a reason for hiding this comment

Uh oh!

berkerpeksag May 20, 2019

Choose a reason for hiding this comment

Uh oh!

felixxm May 20, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

felixxm commented Jun 26, 2019

Uh oh!

berkerpeksag commented Jun 26, 2019 via email

Uh oh!

felixxm commented Jul 9, 2019

berkerpeksag commented Sep 13, 2016 •

edited

Loading