change order of user.set_unusable_password()

[neelay-shah]

setting a password for user needs to be before auth.login()
because get_session_auth_hash() returns the salted_hmac value of salt and password.
If it remains after the auth.login() it will return a different auth_hash
than what's stored in session "request.session[HASH_SESSION_KEY]".
Also we don't need to update the user's password every time he logs in.

[bcail]

Thanks for this PR! Would it work to move the set_unusable_password & save calls to the backend, when the user is created? Seems like that's the only time that set_unusable_password would need to be called, and that way it's only called once (and it's still called before login(), like you're looking for.

[neelay-shah]

I Believe user = auth.authenticate(remote_user=username, shib_meta=shib_meta) is creating a user if doesn't exist. So in order to do what you have suggested; we have to do something like this:

from django.contrib.auth.hashers import make_password
user = auth.authenticate(remote_user=username, shib_meta=shib_meta, password=make_password(None))

But by doing this way, auth.authenticate() will consider password as an argument to login user with remote_username and shib_meta; I think that's not secure. Because let's say if user no longer part of the university, he might able to login with null password.

In Addition, as we are using RemoteUserBackend.authenticate(); This method ignores everything other than username on initial user create.

 class RemoteUserBackend(ModelBackend):
    """
    This backend is to be used in conjunction with the ``RemoteUserMiddleware``
    found in the middleware module of this package, and is used when the server
    is handling authentication outside of Django.

    By default, the ``authenticate`` method creates ``User`` objects for
    usernames that don't already exist in the database.  Subclasses can disable
    this behavior by setting the ``create_unknown_user`` attribute to
    ``False``.
    """

    # Create a User object if not already in the database?
    create_unknown_user = True

    def authenticate(self, remote_user):
        """
        The username passed as ``remote_user`` is considered trusted.  This
        method simply returns the ``User`` object with the given username,
        creating a new ``User`` object if ``create_unknown_user`` is ``True``.

        Returns None if ``create_unknown_user`` is ``False`` and a ``User``
        object with the given username is not found in the database.
        """
        if not remote_user:
            return
        user = None
        username = self.clean_username(remote_user)
        UserModel = get_user_model()
        if self.create_unknown_user:
            user, created = UserModel._default_manager.get_or_create(**{
                UserModel.USERNAME_FIELD: username
            })
            if created:
                user = self.configure_user(user)
        else:
            try:
                user = UserModel._default_manager.get_by_natural_key(username)
            except UserModel.DoesNotExist:
                pass
        return user

That's why I believe my way doing is better. I am also new to django. Without 100% understanding of django RemoteUserLogin, django.cotrib.auth, django.cotrib.backend.ModelBackend and django.cotrib.backend.RemoteUserBackend i don't feel confirtable calling their remote auth() with password.

[bcail]

Let me clarify:

In this package, we have our own remote backend class in backends.py:
class ShibbolethRemoteUserBackend(RemoteUserBackend)

I'm not suggesting we change the auth.authenticate call in middleware.py, but that we update the code in backends.py to run set_unusable_password there.

So it could look something like this in backends.py (note the two added lines):

        if self.create_unknown_user:
            user, created = User.objects.get_or_create(username=username, defaults=shib_user_params)
            if created:
                user.set_unusable_password()
                user.save()
                user = self.configure_user(user)

Does that make more sense?

[neelay-shah]

Yes. It makes sense.

In middleware.py I saw you are importing from django.contrib import auth
and in process_request(self, request): I saw auth.authenticate(), So I thought you are callingauthenticatefrom django`RemoteUserBackend``.

[bcail]

You have to tell django in the settings which RemoteBackend you want to use. With this package, we tell django that we want to use the ShibbolethRemoteUserBackend.

[neelay-shah]

ok..
BTW, I think this will fix the issues: 41

[bcail]

that'd be great if it fixed #41. Are you planning to update the PR?

[neelay-shah]

I have updated pull request. The reason, why I think it will address it, is: he has the same problem, he is losing a session after the first click, which is because user.save() after updating password is updating hash. so for next click request act like not loged-in user. I am not login any other mixins or middleware which cares about if user is login or not, but in his case he does.

[bcail]

Merged - thanks for your contribution!