MushroomMaula / fastapi_login

FastAPI-Login tries to provide similar functionality as Flask-Login does.
https://pypi.org/project/fastapi-login
MIT License
639 stars 58 forks source link

Middleware seems less reliable than Depends #91

Closed bnmnetp closed 8 months ago

bnmnetp commented 2 years ago

Hi,

Thanks for the fastapi_login package.

I have noticed what I think is some odd behavior. I set up the middleware as suggested to populate the request.state.user object. But I noticed that I was not getting the user information even though there was a valid cookie with the token...

So I switched to using user=Depends(auth_manager) on my endpoints that require a login and I have not seen a problem since...

Is this a known problem? Is there anything that I could have messed up in my code that would cause this kind of behavior? Is there some logging debugging I can turn on to provide additional data?

MushroomMaula commented 2 years ago

Hey, thanks for bringing awareness to this. Do you have this issue for every route, or just a subset of routes? Can you try using the dependency and the middleware together to check when the problem happens?

bnmnetp commented 2 years ago

In this little app I only have one route.

I have another with many routes where I exclusively use the middleware. The logs and analytics for that app make me think that this issue is happening there as well...

I will do some investigation and and try the Depends on one or more routes there to see if my numbers go down.

bnmnetp commented 2 years ago

I'm still trying to work on this on my other server. The problem there is that I have a number of routes where the route does one thing if the user is logged in and a different thing if the user is not.

Is there a way to make dependency injection optional? I would love it if user = Depends(auth_manager) could return None for user if they are not logged in rather than throwing an exception. I don't see a way to do that in either the FastAPI docs or your docs. Using the middleware seemed like the perfect solution to that problem.

MushroomMaula commented 2 years ago

Currently, there is no way to achieve this using the Dependency directly. However, I am open for suggestions how this could be implemented, as this functionality has been needed by multiple people. What could also work is to create another dependency which basically implements the code of the middleware and returns None. Just for completeness, do you happen to use OAuth2 scopes?

https://github.com/MushroomMaula/fastapi_login/blob/dcdf31d074f54598c943fd53e99ecf8f17d5d728/fastapi_login/fastapi_login.py#L431-L438

Kind of like this:

_manager = LoginManager(...)

def optional_manager(request: Request, security_scopes: SecurityScopes = None):
    try:
        user = _manager(request, security_scopes)
    except Exception:  # Should catch all errors including FastAPI's HTTPException
        # No valid token or user is not authenticated
        user = None
   return user
bnmnetp commented 2 years ago

I am not currently using Oauth2 scopes, although I plan to in the future. I am in the middle of a long term project migrating my servers from web2py to FastAPI...

I don't know enough about dependencies and how they are implemented to be very useful, so this might be total nonsense. But couldn't the LoginManager just take a parameter that tells it how to handle a missing JWT? ie return None instead of throwing an error?

MushroomMaula commented 2 years ago

Theoretically, that's absolutely possible, however that would require an almost complete rewrite. Right now, the LoginManager is a global instance, i.e. it is instantiated once and can then be used as a dependency for every route, however this also means that the configuration is global for all routes. If we wanted to be able to do something like the following:

def route(user = Depends(manager(optional=True)):
    ...

The LoginManager itself would have to act like some kind of factory, that returns new objects which are the actual dependency. But then we would have a new object for each route...

At the beginning of writing the package I saw the need to i.e. redirect the user to the login page when he is not authenticated to access the route, in that case, IMO at least it makes sense to throw an exception and let an exception handler do the redirecting.

In the end I am still unsure why exactly the middleware does not seem to be as reliable as the dependency approach, if nothing else helps, and you don't want to change a lot of your code you could try to create a subclass that executes the code from my last comment and use that as a dependency:

# The file where you setup the manager and where you import from
from fastapi_login import LoginManager as _LoginManager

class LoginManager(_LoginManager):
    def __init__(*args, **kwargs):
        super().__init__(*args, **kwargs)

    def __call__(request: Request, security_scopes: SecurityScopes = None):
        try:
            user = super().__call__(request, security_scopes)
        except Exception:  # Should catch all errors including FastAPI's HTTPException
            # No valid token or user is not authenticated
            user = None
       return user

manager = LoginManager(...)
MushroomMaula commented 1 year ago

Now available in the latest release. See here for an example.