inject current user into decorator callbacks

[nestedsoftware]

I have recently started using flask-httpauth on a project. Something that jumped out at me is setting the current user into the flask g thread-local object via something like g.current_user = user inside verify_password and verify_token.

Would it make sense to create a new decorator, something like @auth.current_user, which takes a callback that returns the current user? This callback can then be used to inject the current user as a parameter into the callbacks for the other flask-httpauth decorators, e.g. @auth.verify_password as well as @auth.login_required.

I think it would be nice to be able to just receive the current user as a parameter and remove the need to use flask's g object.

Below is a simplified example just to illustrate the idea:

def login_required(f):    
    user = "jane" # in reality obtain the user from the callback

    @wraps(f) 
    def __decorated_function(*args, **kwargs):    
        new_args = (*args, user) # inject user into parameters to `f`
        f(*new_args, **kwargs)
    
    return __decorated_function

[miguelgrinberg]

Adding arguments to your view function is a fairly unusual way of doing things in Flask, don't you think? The only thing that adds arguments is the dynamic components of the path. If you add a second thing, then you need to know what's the correct order, and that is likely to cause weird errors that might be difficult to debug for some people.

What's wrong with using flask.g? Flask-HTTPAuth is built so that it is not concerned with your users, it does not care at all and you can even use it if you don't have user objects in your application. I like that to stay this way.

[nestedsoftware]

I don't really like the idea of setting a variable in one place and then retrieving it from another place in the code in an unstructured way. Here it's done in a fairly limited way, but it still seems not ideal. I'd prefer for there to be a way to have things that are needed made available where they're needed in some configurable way...

Regarding the idea of flask-httpauth not concerning itself with users, maybe naming the value to be injected a 'user' is not necessary. There could possibly be a more generic idea to inject whatever parameters a user wishes to see into their callbacks.

I thought that appending any injected parameters to the end should be okay and it would be the end-user's responsibility to make sure they configured that correctly. I may be missing something there though...

If manipulating flask.g is normal in flask applications, this idea may go against the grain too much...

[miguelgrinberg]

I'm not sure I follow your logic. This extension does not require you to use flask.g. You are looking at an example, that is not part of the extension, it's just an example that you may or may not like. If you prefer to pass the user as an argument, then nothing prevents you from writing the decorator that does exactly what you want. I want this extension to not be opinionated, if you like the flask.g pattern, then you can do that, if you want something else, your application can add the support for that. All these use cases are supported.

[nestedsoftware]

I'd indeed like to pass the user as an argument if that's possible/reasonable. As things stand now, it seems to me that I'd need to get the credentials out of the request/headers myself, depending on whether I'm authenticating via username/password or token. It seems to me that would mean duplicating the code that gets this information in the extension. Is that right? If so, I'll try that. Please let me know if I'm wrong about that part though.

[miguelgrinberg]

There is actually more than one way to do this without duplicating the effort of decoding the Authorization header. If you want a solution that does not use flask.g at all, you can subclass the BasicAuth/TokenAuth classes and provide a slightly different implementation of the login_required decorator. For example, you can make the verify_password callback return the user or None for valid or invalid logins respectively, which is backwards compatible. This, I think, will give you the cleanest solution.

[nestedsoftware]

If I understand correctly, presently I'd need to copy and paste all of the login_required code from the HTTPAuth class into my own class. It occurs to me that maybe it would be worthwhile to extract the following code from decorated in login_required into a utility function, say, get_auth, to make customizing such logic a bit easier:

           auth = request.authorization
            if auth is None and 'Authorization' in request.headers:
                # Flask/Werkzeug do not recognize any authentication types
                # other than Basic or Digest, so here we parse the header by
                # hand
                try:
                    auth_type, token = request.headers['Authorization'].split(
                        None, 1)
                    auth = Authorization(auth_type, {'token': token})
                except ValueError:
                    # The Authorization header is either empty or has no token
                    pass

            # if the auth type does not match, we act as if there is no auth
            # this is better than failing directly, as it allows the callback
            # to handle special cases, like supporting multiple auth types
            if auth is not None and auth.type.lower() != self.scheme.lower():
                auth = None

The following can also be extracted, say, to get_password:

                if auth and auth.username:
                    password = self.get_password_callback(auth.username)
                else:
                    password = None

I think that the end result could look like this:

@wraps(f)
def decorated(*args, **kwargs):
    auth = self.get_auth()

    if request.method != 'OPTIONS':  # pragma: no cover
        password = self.get_password(auth)
        if (not self.authenticate(auth, password)):
            # Clear TCP receive buffer of any pending data
            request.data
            return self.auth_error_callback()

    return f(args, kwargs)  

It would be a bit easier to copy and paste, and then slightly modify, this code in a subclass... I think it has the added benefit of making the source a bit more readable also.

[miguelgrinberg]

Sure, that all looks okay to me.

[nestedsoftware]

Should I try doing a pull request with just this refactoring in it?

[miguelgrinberg]

Yes, that would be my preference. Thanks!

[nestedsoftware]

I made a pull request! I had to create a second one because the first one failed on a trailing space in the method declaration. This is my first time making a pull request, so I hope I did it right!

[miguelgrinberg]

What you did is okay, you did not create a second pull request, just a second commit on the same PR. That's totally fine, I can combine them into one when I merge them. Thanks!

Also, congrats on your first PR! I hope there are many more to come.

[miguelgrinberg]

Released as version 3.2.4.