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

[Identity] Implement new protocol for all credentials #36882

Merged
merged 9 commits into from
Sep 18, 2024
Merged

[Identity] Implement new protocol for all credentials #36882

merged 9 commits into from
Sep 18, 2024
+3,981 −1,627

Conversation

pvaneck
Copy link
Member

@pvaneck pvaneck commented Aug 14, 2024 •
edited
Loading

Based on: #36565

  • Every credential now supports get_token_info.
  • The majority of tests that call get_token were parameterized to call both get_token and get_token_info.
  • Minimum version of azure-core bumped since we need to import the new SupportTokenInfo protocol and AccessTokenInfo class.

@pvaneck pvaneck changed the title [Identity] Implement new protocol for all credentials. [Identity] Implement new protocol for all credentials Aug 14, 2024
@azure-sdk
Copy link
Collaborator

azure-sdk commented Aug 14, 2024 •
edited
Loading

API change check

APIView has identified API level changes in this PR and created following API reviews.

azure-identity

@pvaneck pvaneck force-pushed the identity-new-token-protocol branch 2 times, most recently from c6754dd to d7a9499 Compare September 11, 2024 10:02
@xiangyan99
Copy link
Member

For each credential, let's add tests
isinstance(TokenCredential), isinstance(azure.core.credentials.SupportsTokenInfo), isinstance(corehttp.credentials.SupportsTokenInfo)

Btw, we don't need TokenCredential in corehttp. corehttp only has SupportsTokenInfo.

@pvaneck
Copy link
Member Author

pvaneck commented Sep 12, 2024

For each credential, let's add tests isinstance(TokenCredential), isinstance(azure.core.credentials.SupportsTokenInfo), isinstance(corehttp.credentials.SupportsTokenInfo)

Btw, we don't need TokenCredential in corehttp. corehttp only has SupportsTokenInfo.

Yep, for azure-core TokenCredential and SupportsTokenInfo, I have isinstance tests: https://github.com/Azure/azure-sdk-for-python/pull/36882/files#diff-8aa439c6b471fca403a25dc803f4ef4f2bf2b0a9c2518beac2e0d8aabc48e0fe

Can add additional isinstance checks for corehttp counterparts once that is available.

@pvaneck
[Identity] Implement new protocol methods
9d90e56 
Signed-off-by: Paul Van Eck <[email protected]>
@pvaneck
pytest skips
c4dfbbc 
Signed-off-by: Paul Van Eck <[email protected]>
@pvaneck
Bump core
cf0e44c 
Signed-off-by: Paul Van Eck <[email protected]>
@pvaneck pvaneck marked this pull request as ready for review September 13, 2024 00:37
@pvaneck pvaneck requested a review from a team as a code owner September 13, 2024 00:37
xiangyan99
xiangyan99 reviewed Sep 16, 2024
View reviewed changes
Copy link
Member

@xiangyan99 xiangyan99 left a comment

Choose a reason for hiding this comment

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

Now the chained credential should take TokenProviders rather than TokenCredentials, right?

Also its get_token should call into get_token_info

(We will support the case that user adds SupportsTokenInfo cred into the chain)

@pvaneck
chained updates
3e59581 
Signed-off-by: Paul Van Eck <[email protected]>
@pvaneck
add some ignores
c25e1eb 
Signed-off-by: Paul Van Eck <[email protected]>
@pvaneck
Changes based on feedback
56273c9 
- Updated some typing
- Propagated refresh_on in additional places
- Propagated "token_type" for InteractiveCredential

Signed-off-by: Paul Van Eck <[email protected]>
@pvaneck pvaneck merged commit ddd5c27 into Azure:main Sep 18, 2024
29 checks passed
@pvaneck pvaneck deleted the identity-new-token-protocol branch September 18, 2024 00:24
@miladh miladh mentioned this pull request Sep 20, 2024
Closed
This was referenced Sep 23, 2024
Merged
Closed
Draft
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xiangyan99 xiangyan99 xiangyan99 approved these changes

@maorleger maorleger Awaiting requested review from maorleger maorleger is a code owner automatically assigned from Azure/azure-sdk-write-identity

@christothes christothes Awaiting requested review from christothes christothes is a code owner automatically assigned from Azure/azure-sdk-write-identity

@billwert billwert Awaiting requested review from billwert billwert is a code owner automatically assigned from Azure/azure-sdk-write-identity

@ahsonkhan ahsonkhan Awaiting requested review from ahsonkhan ahsonkhan is a code owner automatically assigned from Azure/azure-sdk-write-identity

@chlowell chlowell Awaiting requested review from chlowell chlowell is a code owner automatically assigned from Azure/azure-sdk-write-identity

@schaabs schaabs Awaiting requested review from schaabs schaabs is a code owner automatically assigned from Azure/azure-sdk-write-identity

@mpodwysocki mpodwysocki Awaiting requested review from mpodwysocki mpodwysocki is a code owner automatically assigned from Azure/azure-sdk-write-identity

Assignees
No one assigned
Labels
Azure.Identity
Projects
Azure Identity SDK Improvements
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pvaneck @azure-sdk @xiangyan99