Skip to content

[23.1] Renew access tokens from PSA using valid refresh tokens #20040

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

Open
wants to merge 1 commit into
base: release_23.1
Choose a base branch
from

Conversation

kysrpex
Copy link
Contributor

@kysrpex kysrpex commented Apr 17, 2025

Method PSAAuthnz.refresh() from psa_authnz.py holds its promise to refresh tokens only if they reached their half lifetime. However, that does not exclude expired tokens. Add an extra comparison to exclude expired refresh tokens.

This fix gets rid of this kind of error: Apr 17 15:54:53 sn06.galaxyproject.eu gunicorn[2716973]: requests.exceptions.HTTPError: 401 Client Error: 401 for url: https://login.elixir-czech.org/oidc/token.

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

Method `PSAAuthnz.refresh()` from psa_authnz.py holds its promise to refresh tokens only if they reached their half lifetime. However, that does not exclude expired tokens. Add an extra comparison to exclude expired refresh tokens.
@kysrpex kysrpex self-assigned this Apr 17, 2025
@github-actions github-actions bot added the area/auth Authentication and authorization label Apr 17, 2025
@github-actions github-actions bot added this to the 23.2 milestone Apr 17, 2025
@kysrpex
Copy link
Contributor Author

kysrpex commented Apr 17, 2025

@bgruening this fixes some (not all) LS-AAI errors.

@kysrpex kysrpex removed this from the 23.2 milestone Apr 17, 2025
@github-actions github-actions bot added this to the 23.2 milestone Apr 17, 2025
@bgruening
Copy link
Member

@kysrpex thanks a lot.

@martenson ping ...

if (
int(user_authnz_token.extra_data["auth_time"]) + int(expires) / 2
<= int(time.time())
< int(user_authnz_token.extra_data["auth_time"]) + int(expires)
Copy link
Member

Choose a reason for hiding this comment

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

The tokens I am getting have e.g. "exp": 1739534619 so the operation you are doing here will compare to year ~2080. I think you just need the expiration time here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's confusing. In the database (table oidc_user_authnz_tokens), I have tokens with provider in ('elixir', 'egi-checkin') (those are the only providers) and their extra_data looks like this.

{
  "access_token": "************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************",
  "auth_time": 1738768462,
  "expires_in": 3600,
  "id_token": "**************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************",
  "refresh_token": "*************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************",
  "token_type": "Bearer"
}

Don't the tokens you are referring to come from table custos_authnz_token? Those indeed seem to use an expiration timestamp (rather than an increment that has to be added to a timestamp).

Copy link
Member

@martenson martenson Apr 25, 2025

Choose a reason for hiding this comment

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

@kysrpex ugh, that is a disastrous split on how to handle this. :/

Maybe just my tokens (which are indeed from custos_authnz_token) are different, however on L174 you are checking for expires in addition to expires_in though -- could these two have different handling even in the same codepath?

@kysrpex kysrpex removed this from the 23.2 milestone Apr 22, 2025
@@ -179,7 +179,11 @@ def refresh(self, trans, user_authnz_token):
else:
log.debug("No `expires` or `expires_in` key found in token extra data, cannot refresh")
return False
if int(user_authnz_token.extra_data["auth_time"]) + int(expires) / 2 <= int(time.time()):
if (
int(user_authnz_token.extra_data["auth_time"]) + int(expires) / 2
Copy link
Member

Choose a reason for hiding this comment

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

if the expires is "seconds to expire from issue time" then this comparison is broken I think

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/auth Authentication and authorization kind/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants