markomirosavljev / fastapi-cognito

Basic Cognito-Auth library for Python and FastAPI.
MIT License
43 stars 16 forks source link

[INVESTIGATE] AIOHTTP call to PUBLIC_KEYS_URL getting stuck #23

Open ivansantiagojr opened 3 weeks ago

ivansantiagojr commented 3 weeks ago

When requesting to an endpoint protected by CognitoAuth.auth_required my whole FastAPI app was blocking execution. After some investigation and adding prints around fastapi-cognito I found out the problem was in src/fastapi_cognito/cognito_jwt/decode.py, in the __get_keys_async function, when calling these lines:

@alru_cache(maxsize=1)
async def __get_keys_async(keys_url: str) -> List[dict]:
...
    if keys_url.startswith("http"):
        async with aiohttp.ClientSession() as session:
            async with session.get(keys_url) as resp:
                response = await resp.json()
...

I also tested using this code:

        async with aiohttp.ClientSession() as session:
            async with session.get(keys_url) as resp:
                response = await resp.json()

in a separate script to perform http request to other URLs and also had the execution stucked running forever.

I do not know if this is a problem with my machine, but I managed to solve this problem by using httpx to do that request as the following:

from httpx import AsyncClient
...
@alru_cache(maxsize=1)
async def __get_keys_async(keys_url: str) -> List[dict]:
...
    if keys_url.startswith("http"):
        async with AsyncClient() as client:
            resp = await client.get(keys_url)
            response = resp.json()
...

This way my app worked and I was able to use fastapi-cognito with this modification just fine. Here is my fork (I would be more than happy to send a PR to this amazing project): https://github.com/ivansantiagojr/fastapi-cognito.

There are some details I would like to add:

Thanks for this library, it is really nice to use!

DominikChmiel commented 2 weeks ago

We're seeing the same thing, also on fastapi-cognito 2.5.0 / python 3.11.

Adding onto this: Usually the cache should kick in here, and if it takes a long time once that should be fine, but we have our application configured with multiple user pools (for different levels of auth). In combination with the lru-cache of size 1 this leads to fastapi-cognito constantly re-fetching public keys, since the cache is constantly invalidated, making this issue a lot more prevalent

Either the cache size could be increased to cover these more niche usecases (setting it to e.g. 10 would cover basically all cases, and ressource-usage of this cache will be still rather low) or this could be made configurable

markomirosavljev commented 2 weeks ago

Hello, thank you for pointing this out and investigating the issue. I will work on fix and create release soon.

markomirosavljev commented 1 week ago

@ivansantiagojr I've moved to httpx instead of aiohttp. I have a question, how this manifested in your app, is it blocking all the time or only on specific requests,do you have multiple userpools configured?

@DominikChmiel I've updated maxsize for alru_cache to 10 for now, I will work to make this configurable in future.

Thanks guys, I'll probably create release in next few days(might be tomorrow) since I want to support one additional feature before new version release. :+1:

ivansantiagojr commented 1 week ago

@ivansantiagojr I've moved to httpx instead of aiohttp. I have a question, how this manifested in your app, is it blocking all the time or only on specific requests,do you have multiple userpools configured?

I haven't tested using fastapi-cognito with multiple userpools yet, the case I described on the issue was with only one. For reference, this is the settings file I have, so you can also see if I am misusing the library:

from typing import Any

from pydantic_settings import BaseSettings, SettingsConfigDict

class Settings(BaseSettings):
    model_config = SettingsConfigDict(
        env_file='.env', env_file_encoding='utf-8', extra='ignore'
    )

    DATABASE_URL: str
    USER_POOL_ID: str
    USER_POOL_CLIENT_ID: str
    AWS_REGION: str

settings = Settings()

class CognitoSettings(BaseSettings):
    check_expiration: bool = True
    jwt_header_prefix: str = 'Bearer'
    jwt_header_name: str = 'Authorization'
    userpools: dict[str, dict[str, Any]] = {
        'eu': {
            'region': settings.AWS_REGION,
            'userpool_id': settings.USER_POOL_ID,
            'app_client_id': settings.USER_POOL_CLIENT_ID,
        },
    }

cognito_settings = CognitoSettings()

Then, I had this dependency:

from http import HTTPStatus
from typing import Annotated

from fastapi import Depends, HTTPException
from fastapi_cognito import CognitoAuth, CognitoSettings, CognitoToken
from sqlalchemy.ext.asyncio import AsyncSession

from src.database import get_session
from src.profiles.models import Profile
from src.profiles.services import find_one
from src.settings import cognito_settings

cognito_dep = CognitoAuth(
    settings=CognitoSettings.from_global_settings(cognito_settings),
)

async def get_current_user(
    session: Annotated[AsyncSession, Depends(get_session)],
    cognito: Annotated[CognitoToken, Depends(cognito_dep.auth_required)],
) -> Profile:
    user_sub = cognito.cognito_id
    user = await find_one(session, user_sub)

    if user is None:
        raise HTTPException(
            status_code=HTTPStatus.UNAUTHORIZED,
            detail='Could not validate credentials.',
        )

    return user

CurrentUser = Annotated[Profile, Depends(get_current_user)]

Finally, I used it in my routes like:

@router.get('/me', response_model=ProfileOut)
async def get_profile_me(current_user: CurrentUser):
    return current_user

I tried to give some more details on how I am using fastapi-cognito so you can also see if I did something wrong.