Refactor the AuthClient inheritance tree #849
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sirosen
requested review from
aaschaer,
ada-globus,
derek-globus and
kurtmckee
as code owners
This was referenced Aug 31, 2023
- introduce AuthLoginClient and AuthServiceClient as the dividing line between each part of the tree - AuthClient becomes an alias for AuthLoginClient - the "app client" classes inherit from login client - a new base class, AuthBaseClient, provides OIDC and JWK methods, base data like service name, scopes, and error class, but no other methods The calls provided on AuthBaseClient are those which are simple GET methods (and a decorated version thereof) which do not require authentication. As such, they make sense to expose equally to all client types.
- RefreshTokenAuthorizer must be provided an AuthLoginClient, not an AuthClient - OAuth2FlowManagers should be constrained to the appropriate client classes Some additional type-checking retrofitting and an improvement to the format of a docstring was included with this change.
sirosen
force-pushed
the
loginclient
branch
from
589dedd
to
f84844c
Compare
To protect against a subtle backwards incompatible change, check at runtime to see if an AuthClient (now AuthServiceClient) has been passed to RefreshTokenAuthorizer. Although it is possible to make this work under current SDK versions under very constrained conditions -- using client credentials -- most likely such usage indicates user error. As such, the newly constrained usage (as expressed by type hints and supported methods) is now captured in a usage error.
After discussion, our team agreed that this rename was not providing sufficient value to justify, and that this naming problem may have better solutions in SDK v4 if we reorganize the login clients to potentially act as client wrappers.
1. Define helpers for getting and decoding JWK data 2. Use those helpers to explicitly implement common JWK handling methods in both AuthClient and AuthLoginClient 3. Remove the mixin 4. Assert the base client classes satisfy the internal protocol in a dedicated mypy test case
The methods which are copied (with no sophisticated code sharing) between the two flavors of auth client class now have warning/preceding comments which designate them as copies and indicate a proposed path for SDK v4 to eliminate the duplication.
kurtmckee
requested changes
Oct 3, 2023
Co-authored-by: Kurt McKee <contactme@kurtmckee.org>
kurtmckee
requested changes
Oct 4, 2023
|
This looks great! I have but one last bit of feedback, but if you push back then I'm going to approve this PR. 馃憤 |
kurtmckee
approved these changes
Oct 4, 2023
kurtmckee
approved these changes
Oct 5, 2023
sirosen
added a commit
to sirosen/globus-sdk-python
that referenced
this pull request
Oct 6, 2023
The `oauth2_userinfo` method was accidentally placed on the `AuthLoginClient` class in globus#849 rather than the `AuthClient` class. The mistake was made because it has a name which suggests that it is an OAuth2 login-related method, when that is not the case. In order to more clearly delineate that this is an API-client method, the `oauth2_` prefix is here stripped and deprecated. A compatibility shim method is included for now, but it emits a deprecation warning. The new name is `AuthClient.userinfo`. Data in `_testing` has also been shimmed temporarily. This is an arguably unnecessary step, but it is nearly trivial and saves us potential migration and rollout pain at the time of our next release. Because `_testing` has a weaker compatibility contract, the shim there can be removed after a single release.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The calls provided on AuthBaseClient are those which are simple GET methods (and a decorated version thereof) which do not require authentication. As such, they make sense to expose equally to all client types.
馃摎 Documentation preview 馃摎: https://globus-sdk-python--849.org.readthedocs.build/en/849/