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

avoid using custos refresh tokens which are expired #19411

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

martenson
Copy link
Member

@martenson martenson commented Jan 16, 2025

if merged, I would like to backport this to release_24.2

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. set up OIDC
    2. craft an expired token (or receive & wait)
    3. observe no request is issued and no exception thrown

License

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

- this will prevent galaxy spamming the auth provider endpoint with doomed refresh attempts for each of these users' request
-afaik the consensus is that we do not log out user in this case atm, details in galaxyproject#15300
@martenson martenson added kind/bug area/auth Authentication and authorization area/backend labels Jan 16, 2025
@github-actions github-actions bot added this to the 25.0 milestone Jan 16, 2025
@martenson martenson changed the base branch from dev to release_24.2 January 16, 2025 12:01
@martenson martenson changed the base branch from release_24.2 to dev January 16, 2025 12:02
@github-actions github-actions bot changed the title avoid using custos refresh tokens which are expired [24.2] avoid using custos refresh tokens which are expired Jan 16, 2025
@martenson martenson changed the title [24.2] avoid using custos refresh tokens which are expired avoid using custos refresh tokens which are expired Jan 16, 2025
we are not guaranteed to have it
@kysrpex
Copy link
Contributor

kysrpex commented Jan 23, 2025

For reviewers: I guess if this PR is merged it would also make sense to go to psa_authnz.py and change it to check that "refresh_token" in user_authnz_token.extra_data before attempting to refresh tokens (given that the issue is unlikely to be fixed in social-core itself). I would open a PR myself then.

@martenson martenson requested a review from a team January 24, 2025 10:53
@martenson
Copy link
Member Author

martenson commented Jan 24, 2025

@kysrpex feel free to piggyback onto this PR (open PR against my fork martenson:ignore-expired and I'll bring it here)

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

Successfully merging this pull request may close these issues.

2 participants