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.

Sign up for GitHub

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

Jump to bottom

Delete revoked refresh tokens on successful authentication #320

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Delete revoked refresh tokens on successful authentication #320

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions app/cruds/cruds_auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,3 +108,18 @@ async def revoke_refresh_token_by_client_and_user_id(
)
await db.commit()
return None


async def delete_expired_refresh_tokens(
db: AsyncSession, user_id: str, settings: Settings
) -> None:
"""Delete expired refresh tokens of a specific user"""

await db.execute(
delete(models_auth.RefreshToken).where(
models_auth.RefreshToken.expire_on
< datetime.now(timezone(settings.TIMEZONE))
)
)
await db.commit()
return None
Comment on lines +113 to +125
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function delete all expired tokens, instead of tokens from a specific user, but I kind of like this behaviour.

29 changes: 17 additions & 12 deletions app/endpoints/auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ async def authorize_validation(
)

# The auth_client allows to override the redirect_uri and bypass related verifications
# This behaviour is not part of OAuth or Openid connect specifications
# This behavior is not part of OAuth or Openid connect specifications
if auth_client.override_redirect_uri is not None:
hyperion_access_logger.info(
f"Authorize-validation: Overriding redirect_uri with {auth_client.override_redirect_uri}, as configured in the auth client ({request_id})"
Expand Down Expand Up @@ -632,7 +632,7 @@ async def authorization_code_grant( # noqa: C901 # The function is too complex
)

# The auth_client allows to override the redirect_uri and bypass related verifications
# This behaviour is not part of OAuth or Openid connect specifications
# This behavior is not part of OAuth or Openid connect specifications
if auth_client.override_redirect_uri is not None:
tokenreq.redirect_uri = auth_client.override_redirect_uri
# A redirect_uri may be hardcoded in the client
Expand Down Expand Up @@ -680,6 +680,12 @@ async def authorization_code_grant( # noqa: C901 # The function is too complex
scope=db_authorization_code.scope,
nonce=db_authorization_code.nonce,
)

# We delete the expired refresh tokens from the database so that they doesn't pile up
await cruds_auth.delete_expired_refresh_tokens(
db=db, user_id=db_authorization_code.user_id, settings=settings
)

await cruds_auth.create_refresh_token(db=db, db_refresh_token=new_db_refresh_token)

response_body = create_response_body(
Expand Down Expand Up @@ -723,9 +729,6 @@ async def refresh_token_grant(
)

if db_refresh_token is None:
hyperion_access_logger.warning(
f"Token refresh_token_grant: invalid refresh token ({request_id})"
)
return JSONResponse(
status_code=400,
content={
Expand Down Expand Up @@ -759,9 +762,6 @@ async def refresh_token_grant(
if db_refresh_token.expire_on.astimezone(
timezone(settings.TIMEZONE)
) < datetime.now(timezone(settings.TIMEZONE)):
await cruds_auth.revoke_refresh_token_by_token(
db=db, token=db_refresh_token.token, settings=settings
)
return JSONResponse(
status_code=400,
content={
Expand All @@ -788,7 +788,7 @@ async def refresh_token_grant(

if auth_client is None:
hyperion_access_logger.warning(
f"Token authorization_code_grant: Invalid client_id {tokenreq.client_id} ({request_id})"
f"Token refresh_token_grant: Invalid client_id {tokenreq.client_id} ({request_id})"
)
return JSONResponse(
status_code=400,
Expand All @@ -798,12 +798,12 @@ async def refresh_token_grant(
},
)

# If the auth provider expects to use a client secret, we don't use PKCE
# If the auth provider expects to use a client secret, we verify it
elif auth_client.secret is not None:
# We need to check the correct client_secret was provided
if auth_client.secret != tokenreq.client_secret:
hyperion_access_logger.warning(
f"Token authorization_code_grant: Invalid secret for client {tokenreq.client_id} ({request_id})"
f"Token refresh_token_grant: Invalid secret for client {tokenreq.client_id} ({request_id})"
)
return JSONResponse(
status_code=400,
Expand All @@ -826,10 +826,15 @@ async def refresh_token_grant(
},
)

# If everything is good we can finally create the new access/refresh tokens
# If everything is good we can finally create the new access/refresh tokens.
# We delete the expired refresh tokens from the database so that they doesn't pile up
# We use new refresh tokens every time as we use some client that can't store secrets (see Refresh token rotation in https://www.pingidentity.com/en/resources/blog/post/refresh-token-rotation-spa.html)
# We use automatic reuse detection to prevent from replay attacks (https://auth0.com/docs/secure/tokens/refresh-tokens/refresh-token-rotation)

await cruds_auth.delete_expired_refresh_tokens(
db=db, user_id=db_refresh_token.user_id, settings=settings
)

refresh_token = generate_token()
new_db_refresh_token = models_auth.RefreshToken(
token=refresh_token,
Expand Down
Loading