Skip to content

Conversation

@jiasli
Copy link
Member

@jiasli jiasli commented Nov 5, 2020

Refine #15184: {Core} Honor scopes specified by Track 2 SDK

Expected flow

The current Track 2 SDK client takes credential_scopes with only one resource as the scopes when calling get_token.

https://github.com/Azure/azure-sdk-for-python/blob/27e4203818e227ba2604ff52dcf55ce2293c4c37/sdk/resources/azure-mgmt-resource/azure/mgmt/resource/subscriptions/_configuration.py#L60

self.authentication_policy = policies.BearerTokenCredentialPolicy(self.credential, *self.credential_scopes, **kwargs)

Issue 1: Empty scopes

Some old Track 2 SDKs generated before Azure/autorest.python#239 don't maintain credential_scopes and calls get_token with an empty scopes.

https://github.com/Azure/azure-cli-extensions/blob/db8ea65eb4afa9358de39e06a8c37c85e862a61c/src/datashare/azext_datashare/vendored_sdks/datashare/_configuration.py#L61

https://github.com/Azure/azure-cli-extensions/blob/db8ea65eb4afa9358de39e06a8c37c85e862a61c/src/logic/azext_logic/vendored_sdks/logic/_configuration.py#L61

self.authentication_policy = policies.BearerTokenCredentialPolicy(self.credential, **kwargs)

This causes error:

File "/opt/hostedtoolcache/Python/3.6.12/x64/lib/python3.6/site-packages/azure/cli/core/util.py", line 1196, in scopes_to_resource 
scope = scopes[0] 
IndexError: tuple index out of range 

Issue 2: Multiple scopes

Some old Track 2 SDKs generated before Azure/autorest.python#745 extend default credential_scopes with custom credential_scopes. Instead, credential_scopes should be replaced by custom credential_scopes. Azure/azure-sdk-for-python#12947

https://github.com/Azure/azure-cli-extensions/blob/120196e953f7b776fa209f4c49838b5cc7af56b0/src/account/azext_account/vendored_sdks/subscription/_configuration.py#L44-L45

self.credential_scopes = ['https://management.azure.com/.default']
self.credential_scopes.extend(kwargs.pop('credential_scopes', []))

Changes

This PR unifies the fixes for the above 2 issues in one function _try_scopes_to_resource:

Testing Guide

# Test empty scopes
> az extension add --name datashare
> az datashare account list --debug
DEBUG: AdalAuthentication.get_token invoked by Track 2 SDK with scopes=()
DEBUG: No scope is provided by the SDK, use the CLI-managed resource.

# Test multiple scopes
> az extension add --name account
> az account subscription show --id 0b1f6471-1bf0-4dda-aec3-cb9272f09590 --debug
DEBUG: AdalAuthentication.get_token invoked by Track 2 SDK with scopes=('https://management.azure.com/.default', 'https://management.core.windows.net//.default')
DEBUG: Multiple scopes are provided by the SDK, discarding the first one: https://management.azure.com/.default

⚠ The workarounds only work for now as get_token is implemented by Azure CLI. After migrating to Azure Identity, these workarounds will fail. The old problematic SDKs MUST be regenerated.

@yonzhan
Copy link
Collaborator

yonzhan commented Nov 5, 2020

Core

@yonzhan yonzhan added this to the S178 milestone Nov 5, 2020
@jiasli jiasli requested a review from evelyn-ys November 5, 2020 09:05
Copy link
Member

@fengzhou-msft fengzhou-msft left a comment

Choose a reason for hiding this comment

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

verified with datashare extension.

def get_token(self, *scopes, **kwargs): # pylint:disable=unused-argument
logger.debug("MSIAuthenticationWrapper.get_token invoked by Track 2 SDK with scopes=%s", scopes)
self.resource = scopes_to_resource(scopes)
resource = _try_scopes_to_resource(scopes)
Copy link
Contributor

Choose a reason for hiding this comment

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

If the resource is None, which resource is used to get token then?

Copy link
Member Author

Choose a reason for hiding this comment

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

The resource used to create MSIAuthenticationWrapper will be used.

return MSIAuthenticationWrapper(resource=resource)

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.

6 participants