feat: Add Token Credential Support

[elliot-huffman]

Overview

Adds support for the base token credential authentication that is implemented by @Azure/Identity. This PR does not remove support for the other @Azure/Identity methods.

The other auth methods are now able to be removed as the credential chain type supports whatever the end user throws at it. Default, service principal, and any combination of crazy auth methods and all future ones that MSFT adds to the MSAL system. All @Azure/Identity auth classes derive from the credential chain class so all the other existing authentication methods are supported through this specific option as well as 100% of their configurations with no additional code that is required from us.

See #1623 for more info. This would be the core auth system work required to solve issues like #1144 as they would be able to use auth that makes sense for them.

Sample Documentation

• microsoft-credential-chain

const msAuth = new DefaultAzureCredential()

"authentication": {
  "type":'microsoft-credential-chain',
  ...,
  "options": {
    "credential": msAuth
  }
}

• credential (TokenCredential): Instance of a token credential from a package like @Azure/Identity. This could be any of the built-in credential types such as DefaultAzureCredential, EnvironmentCredential, ChainedTokenCredentia, VisualStudioCodeCredential, etc. All the MS supported classes derive from the TokenCredential interface so all the pre-built auth methods are supported by inheritance. See the @Azure/Identity package's documentation (https://www.npmjs.com/package/@azure/identity) for more ways to customize your authentication experience for your environment's specifics.

(optional section) Best Practices Migrate from other azure-active-directory prefixed auth config types to this one as they will be removed in the future to simplify the code base.

[elliot-huffman]

Sweet, this looks great! Can you fix the formatting to use 2 spaces instead of tabs? 🙇

Fixed!

I also bumped the @Azure/Identity package to the latest version as it is not a breaking change here since we are Node 18+ and fixed some missing implementation stuff for the constructor and Logon 7.

[arthurschreiber]

Would you mind updating the lock file as well?

[elliot-huffman]

Would you mind updating the lock file as well?

That would be important. One sec.

[elliot-huffman]

I believe it is fixed now. I found an unrelated bug when I added some types to variables as part of this change. It was a bug where the access token could be null, so I added a type guard that triggered the correct error if this case is ever reached.

[arthurschreiber]

I had some more time to look at this and think about this. 😅

How do you feel about renaming the credential method to azure-identity-token-credential, and change this to only use the TokenCredential interface instead?

TokenCredential is the actual thing we're interested in - we don't actually care whether it's a chained token credential or not (and apparently there's various token credentials that don't inherit from ChainedTokenCredential: https://github.com/search?q=repo%3AAzure%2Fazure-sdk-for-js%20%22implements%20TokenCredential%22&type=code)

[arthurschreiber]

Also, in general I'd be in favor of deprecating (and removing) the other azure identity related authentication methods - it would allow us to completely drop the runtime dependency on @azure/identity, resolving some long standing issues around startup performance.

[elliot-huffman]

Also, in general I'd be in favor of deprecating (and removing) the other azure identity related authentication methods - it would allow us to completely drop the runtime dependency on @azure/identity, resolving some long standing issues around startup performance.

I believe the actual constructor validation that I wrote is for token credential instead of change credential, as I was testing with some non-chained credentials it failed and I had to update the validation to support it properly. The documentation I wrote was definitely for change credentials but I will update that wording even though the back end is looking for the token credential structure. The token credential class is not an actual class, it's just an interface, the constructor type guard I wrote is for validating that interface. I found out the hard way when I tried to do an instanceof keyword against it, LOL.

[elliot-huffman]

I had some more time to look at this and think about this. 😅

How do you feel about renaming the credential method to azure-identity-token-credential, and change this to only use the TokenCredential interface instead?

TokenCredential is the actual thing we're interested in - we don't actually care whether it's a chained token credential or not (and apparently there's various token credentials that don't inherit from ChainedTokenCredential: https://github.com/search?q=repo%3AAzure%2Fazure-sdk-for-js%20%22implements%20TokenCredential%22&type=code)

Renamed, let me know what you think!

[elliot-huffman]

It looks like it ran successfully, it is just failing to connect to codecov? I could be wrong; I don't use that service yet. @arthurschreiber, any todo items for me?

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 0% with 15 lines in your changes are missing coverage. Please review.

Project coverage is 78.29%. Comparing base (934e98e) to head (22ecc99).

Files	Patch %	Lines
src/connection.ts	0.00%	13 Missing and 2 partials :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1624      +/-   ##
==========================================
- Coverage   78.45%   78.29%   -0.17%     
==========================================
  Files          93       93              
  Lines        4860     4870      +10     
  Branches      933      936       +3     
==========================================
  Hits         3813     3813              
- Misses        750      759       +9     
- Partials      297      298       +1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[arthurschreiber]

I'm trying to figure out what's going on with codecov, seems like it needs to be updated to the latest version of the action (see https://github.com/tediousjs/tedious/pull/1627).

[arthurschreiber]

Would you be okay with me taking the changes done so far in here and opening a new PR? I'd like to make sure we have tests running for this new functionality, but azure tests don't run on pull requests originating from forks because of security restrictions.

[elliot-huffman]

Would you be okay with me taking the changes done so far in here and opening a new PR? I'd like to make sure we have tests running for this new functionality, but azure tests don't run on pull requests originating from forks because of security restrictions.

On the condition that I get recognition, my ego demands it :-P

maybe being added to the contributors section of the package.json as "Elliot Huffman [email protected] (https://elliot-labs.com)" or something like that.

[arthurschreiber]

The commit(s) will stay attributed to you. 😬

[elliot-huffman]

The commit(s) will stay attributed to you. 😬

my bad, works for me!