search

Azure / ms-rest-js

Runtime for isomorphic javascript libraries generated by Autorest
MIT License
55 stars 55 forks source link

Adds property baseUri on the ServiceClientOptions #457

Closed sadasant closed 3 years ago

sadasant commented 3 years ago

Added a new property baseUri on the ServiceClientOptions, so that when a TokenCredential is sent to ServiceClient, it now automatically sets the scope of future token requests based on the options.baseUri property.

This should address https://github.com/Azure/azure-sdk-for-js/issues/15945

ramya-rao-a commented 3 years ago

Any concerns with subsequent interfaces extending ServiceClientOptions and redeclaring the baseUri property?

sadasant commented 3 years ago

@ramya-rao-a I don’t know of use cases like that. This approach is simpler than others we’ve talked, but it’s less considerate of edge cases. We could have something more automated, like a function on the adapter that gets called on getToken and lets you replace the scope cc: @chradek

sadasant commented 3 years ago

Oh I understood now that you meant if nothing breaks if the baseUri property is re-declared on constructors (I thought you meant late assignments). Testing to be sure!

chradek commented 3 years ago

Any concerns with subsequent interfaces extending ServiceClientOptions and redeclaring the baseUri property?

In our repo there are several instances where ServiceClientOptions is extended and the sub-interface defines baseUri?: string. This is compatible with the change.

However I did notice that more common that baseUri was endpoint. endpoint is used to help construct the baseUri for the services that use it (cognitiveservices-entitysearch is an example).
https://github.com/Azure/azure-sdk-for-js/blob/ce2c1f5310bf0e71dbf8bc2b29d5731a9dfd961f/sdk/cognitiveservices/cognitiveservices-entitysearch/src/entitySearchClientContext.ts#L40-L48

    super(credentials, options);

    this.endpoint = 'https://api.cognitive.microsoft.com';
    this.baseUri = "{Endpoint}/bing/v7.0";
    this.requestContentType = "application/json; charset=utf-8";
    this.credentials = credentials;
    if (options.endpoint !== null && options.endpoint !== undefined) {
      this.endpoint = options.endpoint;
    }

So @sadasant, I don't know if you'll also need to make more changes to support clients that expose endpoint. With regards to the IdentityAdapter, where does the scope come from if you don't override it in the constructor?

sadasant commented 3 years ago

@chradek In the AzureIdentityCredentialAdapter (part of this repo) the scope is either the default one, or is assigned at the constructor.

On Identity, the scope is received on each getToken call, which is why the translation is a bit weird. I think that if we wanted to do something robust that could work for every possible use case, we would allow the adapter to overwrite the constructor scope on getToken. However, since our target audience for this adapter is the generated SDKs, I believe the current approach of this PR to be a reasonable one, since baseUri is always sent through by the children classes.

Let me know if I’m missing something 🤔

sadasant commented 3 years ago

@chradek for the case you presented here: https://github.com/Azure/azure-sdk-for-js/blob/ce2c1f5310bf0e71dbf8bc2b29d5731a9dfd961f/sdk/cognitiveservices/cognitiveservices-entitysearch/src/entitySearchClientContext.ts#L40-L48

I believe that scope is specific to the service version they use. For them, the default scope is likely sufficient, unless they’re working on external clouds.

But to confirm I would need to test it, or speak with somebody owning the cognitive search client. Let me see if I can find someone.

chradek commented 3 years ago

I believe that scope is specific to the service version they use. For them, the default scope is likely sufficient, unless they’re working on external clouds.

For what it's worth, I don't think what cognitive services is doing with endpoint precludes us from exposing baseUri. It's possible that for cognitive services, we'd have to construct the baseUri to pass to the ServiceClient if the user specified a custom endpoint, but we'd need to know what the scopes look like for non-public clouds for those services. But that can be done if needed and I don't think has to be part of this PR.

ramya-rao-a commented 3 years ago

That is a good catch regarding endpoint in the cognitive services packages @chradek

@chradek for the case you presented here: https://github.com/Azure/azure-sdk-for-js/blob/ce2c1f5310bf0e71dbf8bc2b29d5731a9dfd961f/sdk/cognitiveservices/cognitiveservices-entitysearch/src/entitySearchClientContext.ts#L40-L48

I believe that scope is specific to the service version they use. For them, the default scope is likely sufficient, unless they’re working on external clouds.

@sadasant

I am assuming that by "default scope" you mean https://management.azure.com/.default. If so, then that will not work for non management plane packages that use @azure/ms-rest-js. I had missed the fact that we have Track 1 data plane auto generated packages when deprioritizing the need for the adapter to respect the baseUri that has been set after the base class ServiceClient is constructed.

For example, I took a quick look at the newer Text Analytics package. Looks like the baseUri is set to the endpoint and then in the core package the /.default gets added to the baseUri to get the scopes.

Therefore, we need to respect the protected property baseUri in the adapter that gets updated after ServiceClient is constructed even if a customer is using the default Azure cloud.

We have one of two options here

ramya-rao-a commented 3 years ago

@sadasant

On second thoughts, none of the older autogenerated cognitive services packages support AAD auth. So, one would not be trying to use the credentials from @azure/identity with any of them. We need to look for other non management plane packages that are auto generated to find a proper repro case :(

@azure/batch is a good candidate. I have logged https://github.com/Azure/azure-sdk-for-js/issues/16663 to update it to use credentials from @azure/identity. Once that is done, please work with @deyaaeldeen to test whether the changes done in this PR is enough to use the @azure/batch package in Azure public cloud

sadasant commented 3 years ago

@ramya-rao-a sounds good! thank you!

ramya-rao-a commented 3 years ago

On another set of second thoughts...

For other Azure clouds, the authority host needs to be set when creating the TokenCredential from @azure/identity. If the authority host was made a read only property on the credential, then can we not do something like the below in the adapter?

switch (azureTokenCredential.authorityHost) {
      case "https://login.chinacloudapi.cn":
        this.scopes = "https://management.chinacloudapi.cn/.default"
        break;
      // and so on for other clouds
      default:
        break;
    }
ramya-rao-a commented 3 years ago

@deyaaeldeen has been experimenting with the @azure/batch package and we concluded that we cannot reverse engineer the scope from the baseUri. Therefore, we are going to keep this @azure/identity support in Track 1 code gen only for the mgmt plane packages. This would mean that us hardcoding the mgmt related scope in the adapter in ms-rest-js is ok as long as we find a way to deal with different clouds. And so comes my suggestion in the previous comment on utilizing the authority host passed to the credential constructor to determine the right scope for mgmt plane packages

sadasant commented 3 years ago

@ramya-rao-a Ok I’ll work on a PR for v1

sadasant commented 3 years ago

@ramya-rao-a @deyaaeldeen on second thought

From the perspective of Identity, authorityHost does not fully overlap with scopes. Scopes are about what one wants to gain access to, and authorityHost is about who we want to authenticate with.

I think that’s part of the problem on ms-rest-js, baseUri is not a reliable scope for the same reason. It would be better if we could derive the scope during the authentication request, and not at the constructor.

ramya-rao-a commented 3 years ago

Fair enough

In that case, lets guard the changes in this PR to only take affect if the baseUri matches the ARM scope That way we atleast reduce the scope of this change to something we are sure should be updated.

We have logged https://github.com/Azure/autorest.typescript/issues/1153 to ensure that only the ARM packages get the TokenCredential support in the client constructor so that we don't have to worry what scope to use for data plane packages

sadasant commented 3 years ago

I pushed a change that makes it so the baseUri is used as the scope only if it matches the management clouds. Please let me know if there’s anything else I can do!

ramya-rao-a commented 3 years ago

Another nit: Is the term resource management endpoint preferred over resource manager endpoints? https://github.com/Azure/ms-rest-azure-env/blob/master/lib/azureEnvironment.ts#L271 uses the latter

And official docs refer to the latter as well. See https://docs.microsoft.com/en-us/azure/azure-resource-manager/management/overview

sadasant commented 3 years ago

@ramya-rao-a I think I’ve addressed all the feedback through this commit: https://github.com/Azure/ms-rest-js/pull/457/commits/7224651161fbf7928a08f861a864d5dd34064ac1