Opened 3 months ago

Last modified 2 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: master
Severity: Normal Keywords:
Cc: Carlton Gibson 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 Carlton Gibson)


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 (11)

comment:1 Changed 2 months ago by Carlton Gibson

Cc: Carlton Gibson added
Description: modified (diff)
Resolution: needsinfo
Status: newclosed

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!

comment:2 Changed 2 months ago by Sagar Chalise

comment:3 Changed 2 months ago by Carlton Gibson

Has patch: set
Resolution: needsinfo
Status: closednew

Thanks Sagar. I'll re-open so we can have a look.

comment:4 Changed 2 months ago by Carlton Gibson

Component: Uncategorizedcontrib.auth
Type: UncategorizedNew feature
Version: 2.0master

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.

Last edited 2 months ago by Carlton Gibson (previous) (diff)

comment:5 Changed 2 months ago by Carlton Gibson

Needs documentation: set
Needs tests: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

comment:6 Changed 2 months ago by Sagar Chalise

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: AcceptedUnreviewed

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 Changed 2 months ago by Carlton Gibson

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 Changed 2 months ago by Carlton Gibson

Needs documentation: set
Needs tests: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

comment:9 Changed 2 months ago by Sagar Chalise

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: AcceptedUnreviewed

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.

comment:10 Changed 2 months ago by Carlton Gibson

Needs documentation: set
Needs tests: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

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 in reply to:  10 Changed 2 months ago by Sagar Chalise

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.

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