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

Remove identity ID consistency validation from ClientApp #1111

Merged
merged 3 commits into from
Nov 21, 2024

Conversation

derek-globus
Copy link
Contributor

@derek-globus derek-globus commented Nov 21, 2024

What?

  • Remove identity id consistency validation from ClientApps (UserApps are unaffected).

Why?

  1. ClientApps don't have the same token refresh erganomics as UserApps.
    • If an access token is expired in a UserApp, it will raise an error which can be caught by the app and initiate a login flow.
    • If an access token is expired in a ClientApp, it will still return a ClientCredentialAuthorizer expecting that the access token will be refreshed by that authorizer down the line.
  2. ClientCredentialAuthorizers exclusively request one token at a time.

Because of these two facts, a refresh initiated by a ClientCredentialAuthorizer will only ever contain identity info if it's refreshing specifically an globus auth token because that identity info only comes if you specify the special auth-managed "openid" scope.

Because this is a pretty widespread bug (any usage of a ClientApp), I advocate that we simply remove identity id consistency validation from ClientApps (thank you @rjmello for this suggestion) while planning to move off of ClientCredntialAuthorizer in the future.

Testing

  1. Added a unit test that recreates a scenario users reported. Verified it failed before the fix & succeeded after it.
  2. Simulated a client app token expiry scenario locally & observed that the groups token was successfully refreshed.

Copy link
Contributor

@ada-globus ada-globus left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, two minor notes.

src/globus_sdk/globus_app/app.py Outdated Show resolved Hide resolved
tests/unit/globus_app/test_globus_app.py Outdated Show resolved Hide resolved
@derek-globus derek-globus changed the title Client app expired token bug Removed identity ID consistency validation from ClientApp Nov 21, 2024
@derek-globus derek-globus changed the title Removed identity ID consistency validation from ClientApp Remove identity ID consistency validation from ClientApp Nov 21, 2024
Copy link
Contributor

@ada-globus ada-globus left a comment

Choose a reason for hiding this comment

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

🙌

@derek-globus derek-globus merged commit 19268e6 into globus:main Nov 21, 2024
7 checks passed
@derek-globus derek-globus deleted the client-app-expired-token-bug branch November 21, 2024 20:32
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

Successfully merging this pull request may close these issues.

3 participants