`django.contrib.auth.login` inconsistently guards `request.user`

[Jaap Roes]

In ​https://github.com/django/django/blob/a0c44d4e23f8f509757f97f28fbbb1ced3382361/django/contrib/auth/__init__.py#L102-L152 request.user is accessed twice.

The first time here:

​https://github.com/django/django/blob/a0c44d4e23f8f509757f97f28fbbb1ced3382361/django/contrib/auth/__init__.py#L109-L110

The second time here:

​https://github.com/django/django/blob/a0c44d4e23f8f509757f97f28fbbb1ced3382361/django/contrib/auth/__init__.py#L149-L150

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?

[Jaap Roes]

In addition; the first check seems to operate on the assumption that if user is None then request.user must be a valid user. If request.user is None or AnonymousUser the code after it will fail. Is there a clear reason for this behaviour?

[Natalia Bidart]

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:

1. change the code and run the Django tests, see what fails, that could provide answers about the use cases
2. 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!

[Jaap Roes]

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, User)
        self.assertEqual(user.username, self.test_user.username)

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, User)
        self.assertEqual(user.username, self.test_user.username)

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?

[Jaap Roes]

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.

[Jaap Roes]

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.

[Sarah Boyce]

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?

[Jaap Roes]

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.

[Jaap Roes]

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).

[Natalia Bidart]

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?

[Jacob Walls]

​Forum post