Opened 7 years ago
Last modified 8 months ago
#29381 new New feature
Move some parts of `django.contrib.auth.models` to `django.contrib.auth.base_user` for reusability
Reported by: | Sagar Chalise | Owned by: | nobody |
---|---|---|---|
Component: | contrib.auth | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Carlton Gibson, Ülgen Sarıkavak | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | yes |
Needs tests: | yes | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
This is more of an idea than bug but most of the time when need to use authentication without using contrib.auth
one needs to rewrite some of the parts that can be reused if it were in base_user when the interface is similar to django.contrib.auth.
I am mostly talking about update_last_login function and AnonymousUser when custom user is derived from AbstractBaseUser.
May be create AnonymousBaseUser with parts that are compatible to AbstractBaseUser in base_user.py
and use that as base class for AnonymousUser. Also, update_last_login function can be moved to base_user.py
so that it can be reused.
Change History (13)
comment:1 by , 6 years ago
Cc: | added |
---|---|
Description: | modified (diff) |
Resolution: | → needsinfo |
Status: | new → closed |
comment:3 by , 6 years ago
Has patch: | set |
---|---|
Resolution: | needsinfo |
Status: | closed → new |
Thanks Sagar. I'll re-open so we can have a look.
comment:4 by , 6 years ago
Component: | Uncategorized → contrib.auth |
---|---|
Type: | Uncategorized → New feature |
Version: | 2.0 → master |
Hi Sagar. This is at least related to #20313.
Q: How are you specifying your custom AnonymousUser
class? It seems like we'd need a solution to #20313 before this change would make sense.
I'm going to accept this, rather than call it a duplicate, for now. Since #20313 is Accepted this seems a natural follow-up.
Perhaps your PR could address both issues at once?
The existing tests pass with the patch as is, so nothing broken. That's a good sign.
The PR would need docs and new tests, etc.
comment:5 by , 6 years ago
Needs documentation: | set |
---|---|
Needs tests: | set |
Patch needs improvement: | set |
Triage Stage: | Unreviewed → Accepted |
comment:6 by , 6 years ago
Needs documentation: | unset |
---|---|
Needs tests: | unset |
Patch needs improvement: | unset |
Triage Stage: | Accepted → Unreviewed |
Well, I hadn't thought of scenarios regarding swappable anonymous user, the simple use case I am seeing is similar to AbstractBaseUser
that is recommended as base class of custom user in django documentation. The methods and attributes that are available in AbstractBaseUser
are availabe in AnonymousBaseUser
. This way I see users creating AnonymousUser
class based on AnonymousBaseUser
if they need to implement AnonymousUser on their own especially in authentication middleware and when not using contrib.auth
.
Maybe something like:
class AnonymousUser(AnonymousBaseUser): pass
comment:7 by , 6 years ago
Yes, I see how the patch is meant to work but unless/until we have #20313 in play custom anonymous users simply aren't supported. (In that case there's little motivation for this change.)
Are you using a custom anonymous user yourself? If so, how are you switching it in? With a custom middleware? (If not, why do you want this change?)
comment:8 by , 6 years ago
Needs documentation: | set |
---|---|
Needs tests: | set |
Patch needs improvement: | set |
Triage Stage: | Unreviewed → Accepted |
comment:9 by , 6 years ago
Needs documentation: | unset |
---|---|
Needs tests: | unset |
Patch needs improvement: | unset |
Triage Stage: | Accepted → Unreviewed |
Yes, when I avoid contrib.auth
I need to rewrite the part I moved to be used by my custom authentication middleware. I think it makes sense to provide the interface similar to AbstractBaseUser
for maybe AbstractAnonymousUser
just for code reusabilty. Obviously this can be thought of for other stuffs such as backends as well where there is no need of Permission
model.
follow-up: 11 comment:10 by , 6 years ago
Needs documentation: | set |
---|---|
Needs tests: | set |
Patch needs improvement: | set |
Triage Stage: | Unreviewed → Accepted |
Please stop changing the flags on the ticket.
Accepted means "Yes, this is a reasonable ticket" — Unreviewed just means someone else has to make the same decisions.
I reviewed the patch: it does need improvement tests and docs etc. When those points are addressed it's OK to unflag those boxes.
comment:11 by , 6 years ago
Sorry for that, I was just replying to your queries and didn't mean to make any changes to ticket status.
Replying to Carlton Gibson:
Please stop changing the flags on the ticket.
Accepted means "Yes, this is a reasonable ticket" — Unreviewed just means someone else has to make the same decisions.
I reviewed the patch: it does need improvement tests and docs etc. When those points are addressed it's OK to unflag those boxes.
comment:12 by , 2 years ago
comment:13 by , 8 months ago
Cc: | added |
---|---|
Description: | modified (diff) |
Hi Sagar. Thanks for the input.
I'm going to close this as
needsinfo
as is. The general idea is Yeah, possibly but it's too vague to be actionable in its current form.If you want to push this forwards then I would recommend an exploratory PR: make some (small) changes; what tests break?; what does it allow you to do?; etc. From there we've got something concrete to assess. Does that make sense?
Please feel free to re-open this issue if you come up with something concrete along these lines!