Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refresh and Access tokens are not included in the Redis database #39

Open
8thgencore opened this issue Dec 16, 2022 · 11 comments
Open

Refresh and Access tokens are not included in the Redis database #39

8thgencore opened this issue Dec 16, 2022 · 11 comments

Comments

@8thgencore
Copy link
Contributor

During use, I noticed that Refresh and Access tokens do not get into the Redis database

try to connect and see what data is in the redis database

During authorization, after using the get_valid_tokens function, we get an empty set().
And that's why writing to redis doesn't happen

  valid_access_tokens = await get_valid_tokens(redis_client, user.id, TokenType.ACCESS)
  if valid_access_tokens:
      await add_token_to_redis(
          redis_client,
          user,
          access_token,
          TokenType.ACCESS,
          settings.ACCESS_TOKEN_EXPIRE_MINUTES,
      )

I don't understand why the function is called get_refresh_token if we get a new access token at the end.
It would be logical to update the refresh token together too.

@router.post(
    "/refresh-token",
    response_model=IPostResponseBase[TokenRead],
    status_code=201,
)
async def get_refresh_token(
    body: RefreshToken = Body(...),
    redis_client: Redis = Depends(get_redis_client),
) -> Any:
    """
    Gets a refresh token
    """
@jonra1993
Copy link
Owner

Hello, @8thgencore there is no need to load tokens all time on Redis just when a password is changed please check this diagram. It uses that logic #14 (comment)

@jonra1993
Copy link
Owner

I agree get_refresh_token is not the best name for that handler please check this commit 8846821

I do not think it is a good idea to update the refresh token. They should be created using user credentials.

@8thgencore
Copy link
Contributor Author

It seems to me that the OAuth2PasswordBearer function is better to use on endpoint /login
Because /login and /refresh-token perform the same function, and /refresh-token can be removed

And /new-refresh-token rename as /refresh-token

@8thgencore
Copy link
Contributor Author

When we call this function

 await add_token_to_redis(
      redis_client,
      user,
      access_token,
      TokenType.ACCESS,
      settings.ACCESS_TOKEN_EXPIRE_MINUTES,
  )

the parameter settings.ACCESS_TOKEN_EXPIRE_MINUTES is put as seconds, not minutes.
I check Redis, and i have 3 day instead of 180 days

@jonra1993
Copy link
Owner

ACCESS_TOKEN_EXPIRE_MINUTES

Hello @8thgencore you are right I did a mistake you can have the bug solved here 9f0f62f

@jonra1993
Copy link
Owner

jonra1993 commented Dec 17, 2022

OAuth2PasswordBearer

@8thgencore

  1. login handler is in charge of generating an access and refresh token once the user uses its credentials.
  2. new_access_token handler is in charge of generating a new access token once the user uses its refresh token previously generated. It is common that access token has a short valid time and we use refresh token, which has a longer valid time, to generated new access tokens. refresh token helps re-authenticate a user without the need for login credentials

@jonra1993
Copy link
Owner

in order to have a dedicated system for AuthN and AuthZ I am planning to add https://github.com/yezz123/authx or https://github.com/fastapi-users/fastapi-users or Fief https://docs.fief.dev/self-hosting/quickstart/

@tweaker
Copy link

tweaker commented Feb 15, 2023

Please tell me what the route is used for:

@router.post("/access-token")
async def login_access_token(
    form_data: OAuth2PasswordRequestForm = Depends(),
    redis_client: Redis = Depends(get_redis_client),
) -> TokenRead:

if there is a route:

@router.post("")
async def login(
    email: EmailStr = Body(...),
    password: str = Body(...),
    meta_data: IMetaGeneral = Depends(deps.get_general_meta),
    redis_client: Redis = Depends(get_redis_client),
) -> IPostResponseBase[Token]:

@bazylhorsey
Copy link
Contributor

Keep working with it, eventually you will have the epiphany.
Redis is just there to check if a session needs kicked out to reauthenticate. Think of it as a small security measure. /access-token is part of the oauth2 protocol, it can be confusing but he doesn't really use it as expected. It's basically a refresh token for the active auth in his case, and refresh token is sort of useless because it bears the same expense as setting a refresh token really.

@jonra1993
Copy link
Owner

jonra1993 commented Mar 4, 2023

Hello @tweaker the first one is connected with the openapi documentation so you are able to test APIs when you authorize. it is its main purpose if you remove it you can not use openapi documentation.
image

While the second one is a login API. Its purpose is to allow clients to authenticate using a post method. like a web or mobile

@bazylhorsey
Copy link
Contributor

This should be closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants