Reimplement AadClient without msal.oauth2cli #11466
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
chlowell
added
Client
xiangyan99
reviewed
May 27, 2020
sdk/identity/azure-identity/azure/identity/_credentials/authorization_code.py
Show resolved
Hide resolved
xiangyan99
reviewed
May 27, 2020
sdk/identity/azure-identity/azure/identity/_credentials/authorization_code.py
Show resolved
Hide resolved
xiangyan99
reviewed
May 27, 2020
xiangyan99
reviewed
May 27, 2020
| @@ -1,6 +1,10 @@ | |||
| # Release History | |||
|
|
|||
| ## 1.4.0b4 (Unreleased) | |||
| - `azure.identity.aio.AuthorizationCodeCredential.get_token()` no longer accepts | |||
| optional keyword arguments `executor` or `loop`. Prior versions of the method | |||
There was a problem hiding this comment.
executor & loop were already in 1.3.1?
There was a problem hiding this comment.
Yes, they've been around since 1.0.0b4.
There was a problem hiding this comment.
So do we really want 1.4.0 to break 1.3.1?
There was a problem hiding this comment.
As I wrote at the top of this PR, these arguments never worked. Trying to use them just raises exceptions.
There was a problem hiding this comment.
So the behavior is
Raise exception -> silently ignored?
We should add it into Breaking Change section
xiangyan99
reviewed
May 29, 2020
xiangyan99
reviewed
May 29, 2020
xiangyan99
reviewed
May 29, 2020
xiangyan99
reviewed
May 29, 2020
sdk/identity/azure-identity/azure/identity/_internal/shared_token_cache.py
Show resolved
Hide resolved
xiangyan99
reviewed
May 29, 2020
xiangyan99
reviewed
May 29, 2020
sdk/identity/azure-identity/azure/identity/_internal/aad_client_base.py
Outdated
Show resolved
Hide resolved
xiangyan99
approved these changes
May 29, 2020
iscai-msft
added a commit
to iscai-msft/azure-sdk-for-python
that referenced
this pull request
Jun 1, 2020
…into fix_annotation_initial_response * 'master' of https://github.com/Azure/azure-sdk-for-python: Adding digital twins CI configuration. (Azure#11730) Sync eng/common directory with azure-sdk-tools repository (Azure#11692) Reimplement AadClient without msal.oauth2cli (Azure#11466)
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.
Upcoming features need an Azure AD client which separates acquiring tokens from caching them. We have two Azure AD clients,
AuthnClientandAadClient, both of which require reshaping to meet this new requirement. I chose the latter because it has a simpler API. Its implementation, however, is quite complex and usesmsal.oauth2cli, which the MSAL team doesn't consider public. So this PR takes the first step toward supporting new features by simplifying the implementation and removing usage ofmsal.oauth2cli.While making these changes, I observed the async
AuthorizationCodeCredential.get_tokenaccepts two optional keyword arguments but doesn't use them correctly, provoking exceptions when either is passed. This has been the case since I added the credential in 1.0.0b4. Whoops 😇. This PR removes them because they have never worked and theAadClientchanges make them obsolete.