[Service Bus] upgrade ms-rest-nodeauth due to critical bug

[shlomiassaf]

• Package Name: azure-sdk-for-js
• Package Version: 1.1.5
• Operating system: osx
• nodejs
  • version: 12

Describe the bug
A clear and concise description of what the bug is.

The service-bus package depends on @azure/ms-rest-nodeauth ^0.9.2.

The current version of ms-rest-nodeauth is 3.0.3

The problem with version ^0.9.2 (or any other version up to 3.0.3) is a critical bug that cause the application to halt due to an unresolved promise

See the bug fix here:

Azure/ms-rest-nodeauth@43c52f9

Done on Aug 22, 2019 and included in version 3.0.3 (the latest)

This is causing my node application to halt due to that unresolved promise.

[ramya-rao-a]

Thanks for reporting @shlomiassaf

@chradek, I recall removing the dependency on ms-rest-nodeauth from amqp-common preview 7. See https://github.com/Azure/azure-sdk-for-js/blob/amqp-common/sdk/core/amqp-common/changelog.md#2019-9-11-100-preview7

Can you check how that got reverted? I'd prefer if we had no dependency on this package in Service Bus at all

[chradek]

@ramya-rao-a It looks like we removed some runtime code that depended on it, but didn't remove the actual dependency in #5010 It's actually still being imported by amqp-common.

I can work on removing this dependency and rereleasing service-bus.

[ramya-rao-a]

I believe @bterlson's thoughts at the time was that since the current usage is only in method signatures, the final bundle shouldn't have the ms-rest-nodeauth dependency at all

[chradek]

I can investigate if it is showing up in the final bundle.
@shlomiassaf, are you taking a direct dependency on @azure/ms-rest-nodeauth, and if so what version? Can you also run npm ls @azure/ms-rest-nodeauth in your project so where copies of this package are sitting in node_modules?

[chradek]

@ramya-rao-a
Keeping backwards-compatibility in mind, I think our best option is to update amqp-common and service-bus to accept an interface we control (much like TokenCredential) instead of specific AAD classes.

This would allow us to completely remove the @azure/ms-rest-nodeuath dependency from amqp-common and service-bus. The only potentially breaking issue I see with this is we'd have to change the type of AadTokenProvider.credentials to be this more generic interface as well (I'm not sure why that field is public in the first place) but technically that should be fine since amqp-common is still versioned as a preview.

I looked into simply upgrading amqp-common and service-bus to @azure/ms-rest-nodeauth@3.0.3, but this would cause compilation errors for customers using an older version of ms-rest-nodeauth. It does look like there aren't any compilation issues today if customers want to use @azure/ms-rest-nodeauth@3.0.3 with this library.

/cc @bterlson

Edit: I do not see @azure/ms-rest-nodeauth imported or included in the bundles for amqp-common or service-bus. Both packages are only using @azure/ms-rest-nodeauth for typing information.

[ramya-rao-a]

It does look like there aren't any compilation issues today if customers want to use @azure/ms-rest-nodeauth@3.0.3 with this library.

Thanks @chradek

@shlomiassaf, Can you change your direct dependency on ms-rest-nodeauth to be 3.0.3 and see if that works? Use this version to create the credentials and pass them to Service Bus package

[shlomiassaf]

@ramya-rao-a

There is an issue with the types when trying to work with ServiceBusClient.createFromAadTokenCredentials

static createFromAadTokenCredentials(host: string, credentials: ApplicationTokenCredentials | UserTokenCredentials | DeviceTokenCredentials | MSITokenCredentials, options?: ServiceBusClientOptions): ServiceBusClient;

All of the types of credentials are coming from @azure/ms-rest-nodeauth but 0.9.2, however when using 3.0.3 and trying to set credentials with types from 3.0.3 I get this:

Property 'getTokenFromCache' is protected but type 'ApplicationTokenCredentialsBase' is not a class derived from 'ApplicationTokenCredentials'

If I cast the type I provide to the method into a any it works, so I guess it's type only and it's not a runtime issue.

I tested it with a credentials object using ApplicationTokenCredentials, didn't test other credential methods but I guess they will work.

Anyway, I think it's time to upgrade :)

[chradek]

@shlomiassaf
You are correct, I'm seeing TypeScript compilation errors using the newer ms-rest-nodeauth. Apologies, I must have made too many changes when I tested earlier, I thought I had checked that correctly.

@ramya-rao-a
I did verify that updating the version of ms-rest-nodeauth that the SDK depends on then causes compilation errors when trying to use an older version of ms-rest-nodeauth. I think we should get rid of this dependency altogether (since our only direct use of it is for types) and use an inlined interface instead.

[ramya-rao-a]

That makes sense @chradek, lets go ahead with that plan

[ramya-rao-a]

The latest version 1.1.6 of the @azure/service-bus package has been updated to no longer rely on the @azure/ms-rest-nodeauth package.

This allows users to use any version of @azure/ms-rest-nodeauth directly with @azure/service-bus without TypeScript compilation errors.