Opened 4 months ago
Last modified 20 hours ago
#35530 assigned Cleanup/optimization
`django.contrib.auth.login` inconsistently guards `request.user`
Reported by: | Jaap Roes | Owned by: | Jaap Roes |
---|---|---|---|
Component: | contrib.auth | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Jacob | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | yes |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
In https://github.com/django/django/blob/a0c44d4e23f8f509757f97f28fbbb1ced3382361/django/contrib/auth/__init__.py#L102-L152 request.user
is accessed twice.
The first time here:
The second time here:
The first time there is no hasattr
guard to verify if the request
object has a user
attribute. The second time there is.
Is the hasattr
check in the second case redundant? Or should the first case be guarded as well?
Change History (12)
comment:2 by , 4 months ago
Resolution: | → invalid |
---|---|
Status: | new → closed |
Hello! Thank you for taking the time to create this ticket. I can see how the code seems confusing, and I don't have an answer for you without getting deep into the code. I would recommend the following if you are interested in answering the questions you are proposing:
- change the code and run the Django tests, see what fails, that could provide answers about the use cases
- ask in any of the user support channels listed in this link.
Following the ticket triaging process, I'd need to close this ticket as invalid
because is not clear that this is an issue in Django. If you can provide a small Django project or a failing test case showing how the code triggers an error or has a bug, I'll be happy to re-open.
Thank you!
comment:3 by , 4 months ago
Thanks. The reason I filed this issue is because I tasked myself with describing how Django's authentication and login flow works. This bit stuck out as particularly confusing and basically unexplainable. I haven't been able to come up with a rational use case where passing in None
for the user
argument will make login
behave in a way that I would expect.
I removed the confusing bit:
if user is None: user = request.user
then ran the tests again.
The only test that breaks is async def test_alogin_without_user(self):, which is a test for the async wrapper of this function.
Based on this test I have created another testcase that shows the issue when request.user
is AnonymousUser
(which is common when AuthenticationMiddleware
is used).
async def test_alogin_without_user_anonymous_request(self): request = HttpRequest() request.user = AnonymousUser() request.session = await self.client.asession() await alogin(request, None) user = await aget_user(request) self.assertIsInstance(user, AnonymousUser)
This will fail with an AttributeError: 'AnonymousUser' object has no attribute '_meta'
.
Another way this function will fail is when request.user
is absent (i.e. AuthenticationMiddleware
is not in use):
async def test_alogin_without_user_or_request_user(self): request = HttpRequest() request.session = await self.client.asession() await alogin(request, None) user = await aget_user(request) self.assertIsInstance(user, None)
This will fail with an AttributeError: 'HttpRequest' object has no attribute 'user'
.
Setting request.user = None
and passing in user=None
will do the same thing as just removing the if user is None
test and fail with AttributeError: 'NoneType' object has no attribute '_meta'
.
There seems no real reason for this behaviour to exists. The only thing touching this code in the Django code base is a recently added test for the async wrapper. The code branch only works in very specific circumstances, and does not fail gracefully if these circumstances are not met.
Not sure if this is enough background to make you open this ticket again?
comment:4 by , 4 months ago
Just to be clear, these tests I provided are not supposed to pass. They're just examples of reasonable scenarios that break as soon as the user
argument to login
is None
.
Calling login
with None
just doesn't make sense in my opinion. The function signature also doesn't imply that the user is optional, or allowed to be None
in the first place.
comment:5 by , 4 months ago
Has patch: | set |
---|---|
Resolution: | invalid |
Status: | closed → new |
I've decided to deprecate the request.user
fallback path (see linked PR). If anyone depends on this behaviour it should be possible to migrate to a pattern where login is called with a user, instead of setting it on the request and calling login without one.
comment:6 by , 3 months ago
Cc: | added |
---|---|
Resolution: | → needsinfo |
Status: | new → closed |
I agree that, looking at the docs for login, this user=None
shouldn't be accepted, and in the example code, there is a guard after authenticate
(which can return None for user).
This is a good sign that we might be able to remove this.
However, this code was added a long time ago aab3a418ac9293bb4abd7670f65d930cb0426d58 (roughly 18 years old)
It is likely someone is using this. This should roughly "work" for example
@login_required def change_account(request): # This view is when some user has access to multiple accounts. username = request.POST["username"] password = request.POST["password"] user = authenticate(request, username=username, password=password) login(request, user) if user is not None: # Redirect to a success page. ... else: # Return an 'invalid login' error message # but I am still logged in as the original user. ...
I would love to hear some opinions of people who have written custom authentication backends (maybe the maintainer of django-allauth) or others who might remember some of the history of this before we precede here as I think the value gained here (removing ~2 lines) is very small.
Can you discuss this on the Django Forum? Check if the community is in agreement to do this?
comment:7 by , 3 months ago
Thanks, I'll look into making a post on the forum.
Note that in the PR I've only deprecated the current "happy path". That should shake out any project that's relying on it. The way to mitigate any fallout is adding a guard before the call to login, so the inconvenience seems minor.
Regarding the value gained. The current login function in Django has a code path that, as far we can tell, doesn't need to be there for any true valid reason. This is a security critical function, and I'd feel a lot better if it didn't have unexplained behaviour.
comment:8 by , 3 months ago
Made a post in the forum, but that has not gained any feedback.
The patch I proposed only deprecates this path, so that should give people relying on this behaviour ample time to fix their code (I doubt there are any).
comment:9 by , 7 days ago
Resolution: | needsinfo |
---|---|
Status: | closed → new |
Triage Stage: | Unreviewed → Accepted |
Hello Jaap, I have re-reviewed your proposal and I think it makes sense. I will accept this ticket and provide a review for your PR.
Can you please paste here the link to the forum conversation?
comment:10 by , 7 days ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:11 by , 7 days ago
Needs documentation: | set |
---|---|
Patch needs improvement: | set |
In addition; the first check seems to operate on the assumption that if
user
isNone
thenrequest.user
must be a valid user. Ifrequest.user
isNone
orAnonymousUser
the code after it will fail. Is there a clear reason for this behaviour?