Azure-sdk-for-js: ms-rest-nodeauth back-compat

[bterlson]

Users who have a dependency today on ms-rest-nodeauth should be able to continue using that library to authenticate against our libraries. This involves two things:

1. Ensuring ms-rest-nodeauth's authentication classes can be passed to our client constructors without error.
2. Adding a test or two for each library to ensure we don't regress this functionality until we intend to.
3. Updating readmes for users who are still using ms-rest-nodeauth showing how to do it.

These three items will help ease the transition to our new libraries.

I believe 1 should be the case across our library as core-http's TokenCredential should be structurally compatible with ms-rest-nodeauth, but we should verify.

[ramya-rao-a]

This would mean that we continue support for 3 separate libraries for the purposes of auth

• @azure/identity
• @azure/ms-rest-nodeauth
• @azure/ms-rest-browserauth

@azure/ms-rest-nodeauth & @azure/ms-rest-browserauth libraries depend on @azure/ms-rest-js which we would like to deprecate in favor of @azure/core-http, but are now forced to fix up as seen in https://github.com/Azure/ms-rest-js/pull/368

If @azure/identity is the area where we plan to focus our current and future efforts, this might not be the best thing to do.

I'd suggest the below approach:

• Drop support for @azure/ms-rest-nodeauth & @azure/ms-rest-browserauth in the below
  
  
  
  • @azure/core-http
  
  
  • @azure/core-arm
  
  
  • The Track 2 libraries that we are currently working on
  
  
  • The version of the Typescript generator that uses the @azure/core-http and @azure/core-arm
  
  
  • The next version of @azure/arm-* libraries that will be generated using the new version of Typescript generator
  
  
• Have a migration guide for users who are currently using @azure/ms-rest-nodeauth & @azure/ms-rest-browserauth.
  
  
  
  • This is not our entire JS customer base. We believe that there is a sizable customer base still using the older libraries in azure-sdk-for-node repo i.e. those who dont use @azure/ms-rest-nodeauth & @azure/ms-rest-browserauth yet.
  
  
  • Either we support the same helper methods (that exist in nodeauth & browserauth) in @azure/identity
  
  
  • Or provide a big table mapping each of the helper methods to a code snippet of how to achieve the same by newing up a credential using @azure/identity
  
  

This way, when we GA the Track 2 data plane libraries, we can also release an updated version of all the @azure/arm-* libraries that will use only @azure/identity

Then, one fine day, deprecate the below at one go
- @azure/ms-rest-nodeauth
- @azure/ms-rest-browserauth
- @azure/ms-rest-js
- @azure/ms-rest-azure-js

cc @bterlson, @daviwil, @amarzavery, @schaabs

[amarzavery]

@ramya-rao-a In sync with everything that you are saying. However, it would be nice if
we can support the same helper methods (that exist in nodeauth & browserauth) in @azure/identity

Reason being,

• Many people use authentication methods from ms-rest-azure (node sdk runtime that has authentication functionality in it) ~300K downloads in June'19 in their applications.
• Moreover, many people have started using @azure/ms-rest-nodeauth as well. I know that download numbers is not an accurate representation of usage. However just for reference,
  
  
  
  • for @azure/ms-rest-nodeauth From ~444 downloads in Jan'19 to ~50K downloads in June'19 as seen here.
  
  
  • whereas, for @azure/ms-rest-browserauth From 40 downloads in Jan'19 to 126 downloads in June'19 as seen here.
    
    This is a good indication that nodeauth is being used a lot compared to browserauth.
  
  
• Thus providing the same functions that internally wrap the identity creds would be a better option rather than breaking so many people.

[daviwil]

@ramya-rao-a, @bterlson, and I had a short sync up this morning to discuss whether we should drop support for ms-rest-nodeauth and ms-rest-browserauth in newly generated management and data plane libraries, here's the tentative plan we've settled on:

• Remove ServiceClientCredentials support in @azure/core-http and @azure/core-arm and allow only TokenCredential implementations to be passed to ServiceClient
• Remove ServiceClientCredentials from credentials parameter in code generated by autorest.typescript
• Ship major version updates to ms-rest-nodeauth and ms-rest-browserauth which depend on @azure/core-auth and change existing credential implementations to implement both ServiceClientCredentials from ms-rest-js and TokenCredential from @azure/core-auth.  This will enable the ms-rest-*auth libraries to be used with both Track 1 and Track 2 libraries and provide a clear path to eventual deprecation.

The direct user impact of this plan is that to use Track 2 libraries, the user will have to bump the major version of their ms-rest-nodeauth or ms-rest-browserauth dependency and then it should work just the same for them with both Track 1 and Track 2 libraries.

Do you think this is a good compromise @amarzavery?

[amarzavery]

That seems like a good plan :).

[daviwil]

Great! I'll start hacking on this approach now to get a sense for how long it might take to complete.

[daviwil]

PRs out for these changes:

• Remove ServiceClientCredentials from ServiceClient interface: https://github.com/Azure/azure-sdk-for-js/pull/4367
• Make ms-rest-nodeauth compatible with TokenCredential: https://github.com/Azure/ms-rest-nodeauth/pull/66

I'll make similar changes to ms-rest-browserauth after I know we're all good with the changes so far.

[xirzec]

@ramya-rao-a I'm not sure what the work remaining here is -- is it only in the identity space?

[ramya-rao-a]

Chatted with @daviwil offline, it looks like both @azure/ms-rest-nodeauth and @azure/ms-rest-browserauth would work with our newer libraries that depend on @azure/core-http based on https://github.com/Azure/ms-rest-nodeauth/pull/66#issuecomment-580026401

If we could test that, then there is nothing more to do here other than take the call of whether we want to continue this support in core-http v2

[ramya-rao-a]

I tried ms-rest-nodeauth with the latest keyvault-keys package, and we get errors :(

Code used:

async function main() {
  const oldCreds = await 
  msRestNodeAuth.loginWithServicePrincipalSecretWithAuthResponse(
    process.env.AZURE_CLIENT_ID,
    process.env.AZURE_CLIENT_SECRET,
    process.env.AZURE_TENANT_ID
  );
  const newCreds = new DefaultAzureCredential();

  const clientWithOldCreds = new KeyClient(vaultUrl, oldCreds);
  const clientWithNewCreds = new KeyClient(vaultUrl, newCreds);

  try {
    await clientWithOldCreds.getKey("mykey");
  } catch (error) {
    console.log("Error when using msRestNodeAuth")
    console.log(error);
  }

  try {
    await clientWithNewCreds.getKey("mykey");
  } catch (error) {
    console.log("Error when using @azure/identity")
    console.log(error);
  }

  console.log("Done.")
}

main();

Output:

Error when using msRestNodeAuth
TypeError: this.authenticationProvider.signRequest is not a function
    at SigningPolicy.signRequest (/Users/ramyar/Documents/GitRepos/test/node_modules/@azure/core-http/dist/coreHttp.node.js:4042:44)
    at SigningPolicy.sendRequest (/Users/ramyar/Documents/GitRepos/test/node_modules/@azure/core-http/dist/coreHttp.node.js:4046:21)
    at RedirectPolicy.sendRequest (/Users/ramyar/Documents/GitRepos/test/node_modules/@azure/core-http/dist/coreHttp.node.js:3436:14)
    at ExponentialRetryPolicy.sendRequest (/Users/ramyar/Documents/GitRepos/test/node_modules/@azure/core-http/dist/coreHttp.node.js:3229:14)
    at SystemErrorRetryPolicy.sendRequest (/Users/ramyar/Documents/GitRepos/test/node_modules/@azure/core-http/dist/coreHttp.node.js:3783:14)
    at ThrottlingRetryPolicy.<anonymous> (/Users/ramyar/Documents/GitRepos/test/node_modules/@azure/core-http/dist/coreHttp.node.js:3976:56)
    at step (/Users/ramyar/Documents/GitRepos/test/node_modules/tslib/tslib.js:141:27)
    at Object.next (/Users/ramyar/Documents/GitRepos/test/node_modules/tslib/tslib.js:122:57)
    at /Users/ramyar/Documents/GitRepos/test/node_modules/tslib/tslib.js:115:75
    at new Promise (<anonymous>)
Done.

[daviwil]

@ramya-rao-a Try it with loginWithServicePrincipalSecret, it returns an instance of ServiceClientCredentials. The variant you're using returns an AuthResponse which contains a token.

[daviwil]

You'll also need a fourth parameter with the proper tokenAudience:

  msRestNodeAuth.loginWithServicePrincipalSecret(
    process.env.AZURE_CLIENT_ID,
    process.env.AZURE_CLIENT_SECRET,
    process.env.AZURE_TENANT_ID,
    { tokenAudience: "https://vault.azure.net" }
  );

[ramya-rao-a]

Thanks @daviwil

Confirmed that after using the non "withResponse" method and adding the token audience, we are good to go!

[ramya-rao-a]

To conclude:

• At this point in time, the below packages that live in this repo are auto-generated using v4 of @microsoft.azure/autorest.typescript code generator.
  
  
  
  • all packages that use ARM for resource management of various Azure resources. The names of these packages start with @azure/arm-
  
  
  • all packages for the various cognitive services under the folder sdk/cognitiveservices. The names of these packages start with @azure/cogntiveservices-
  
  
  • @azure/graph which is the package for the older Azure AD Graph APIs
  
  
  • @azure/servicefabric, @azure/batch, @azure/eventgrid
  
  
• Theses packages require users to use @azure/ms-rest-nodeauth for their auth needs against AAD
• @azure/identity is a new package, also for the auth needs against AAD for the newer packages. @azure/identity is not compatible with any of the above packages
• @azure/ms-rest-nodeauth is compatible with these newer packages