django-loginas

django-loginas copied to clipboard

Hello, The default implementation of django.contrib.auth.backends.ModelBackend.get_user prevents inactive users to logging in since Django 1.10 (see user_can_authenticate). I do believe this error should't be critical since you could fix it without updating project configuration and better showing warning / error message instead. How about allow throwing custom exception in can_login_as with the reason why you can't log in as a target user and showing it to the authenticated user in this case?

Hmm, what happens right now in that case? Does the code just crash?

We could either show an error message or just let the user log in, since it's an admin overriding the login and not the actual user themselves.

Hmm, what happens right now in that case? Does the code just crash?

500 error if none of the auth backends allowed to log in as a target user, e.g. on django 2.2.x this test will be ok

    def test_login_as_inactive_user(self):
        superuser = create_user("me", "pass", is_staff=True, is_superuser=True)
        self.assertTrue(self.client.login(username="me", password="pass"))
        self.target_user.is_active = False
        self.target_user.save()
        message = "Could not found an appropriate authentication backend"
        with self.assertRaisesMessage(ImproperlyConfigured, message):
            self.get_target_url()
        self.assertCurrentUserIs(superuser)
        self.target_user.is_active = True
        self.target_user.save()

We could either show an error message or just let the user log in, since it's an admin overriding the login and not the actual user themselves.

But how to achieve the second option? In general, we don't know get_user implementation details and couldn't predict why auth backend doesn't return the target user, does it happened because of is_active flag or something else.

Hm yes, that's true, unfortunately. This makes the library less useful, but I think an error message is an acceptable workaround for now.

Please look at https://github.com/skorokithakis/django-loginas/pull/84 With this PR I could check target user activity in can_login_as function and raise proper error message like "You can't log in as inactive user".

How about another PR for showing error message instead of raising ImproperlyConfigured? To me it looks more helpful than 500 but it would change current behavior, not just extend it like #84

@pacahon That looks good, I merged it, thank you!

How would it change the current behavior? Would we not just catch the exception and display an error message? Isn't that what #84 fixed?

@skorokithakis ImproperlyConfigured could be raised later in user_login view if use default can_login_as function and try to log in as inactive user. https://github.com/skorokithakis/django-loginas/blob/0525348f3b9229215ba455085389b36af585b8ee/loginas/views.py#L72-L76 There is no try-except block right now. I suggest adding it.

Ahh, I see. Would adding a try-except and showing an error in the admin not be okay? How does it change behaviour?

Well, technically it would stop propagating exception which could be processed by middleware, but I couldn't imagine anyone who actually relies on this behavior. :) Showing an error should be totally fine.

Yeah, I think we should consider that a bug at this point, if anyone is relying on it, they'll have to unrely on it :P