Make token credentials compatible with @azure/core-auth's TokenCredential by daviwil · Pull Request #66 · Azure/ms-rest-nodeauth

daviwil · 2019-07-19T19:03:09Z

This change makes ms-rest-nodeauth token credential implementations compatible with @azure/core-auth's TokenCredential, enabling them to be used with Track 2 SDK libraries. This is part of the resolution for Azure/azure-sdk-for-js#4108.

I've verified these changes with DeviceTokenCredentials, AzureCliCredentials, and ApplicationTokenCredentials using a version of @azure/arm-resources that does not accept ServiceClientCredentials in its ServiceClient constructor. I also removed the use of signingPolicy in @azure/core-auth in my local build just to make sure these credentials are being used with BearerTokenAuthenticationPolicy.

One implication of this change is that the fix we made to isTokenCredential in Azure/azure-sdk-for-js#4335 now has to be rolled back because all of these credentials now expose both getToken and signRequest due to implementing both ServiceClientCredentials and TokenCredential. I'll make a PR to @azure/core-http if we decide we like this compatibility approach.

Questions for Reviewers

I've made this a major version bump, but do we think the API changes would actually be considered breaking changes since they are only additive?
What do you think about the name prepareToken? Doesn't seem explicit enough to me so I'm open to ideas for a better name.

daviwil · 2019-07-19T19:03:58Z

Worth noting that the CI build will fail because we haven't shipped @azure/core-auth yet. Once we decide whether the fix in Azure/azure-sdk-for-js#4335 should be rolled back, I'll get that change in and then have the engineering systems team ship that library for us.

daviwil · 2019-07-19T22:35:07Z

PR Azure/azure-sdk-for-js#4367 restricts the ServiceClient interface to only accept TokenCredential implementations.

amarzavery · 2019-07-19T22:32:52Z

lib/credentials/coreAuthHelpers.ts

+export function prepareToken<T extends TokenResponseLike>(
+  token: T,
+  scopes: string | string[] | undefined): T | AccessToken {
+  if (scopes) {


What if scopes is an empty string ? This check will fail for that. If the intention is to check specifically for null or undefined then using the "double equal to" or "not with a single equal to" will be a good check as that will take care of null and undefined.
if (scopes != undefined)

Thanks! It shouldn't be an empty string, but you never know what a user might do. I'll make the check stricter.

On second thought, the idea is check for whether a parameter was passed to scopes, not to scrutinize whether the value of scopes is valid. I think in this case I'm only concerned about whether it's explicitly undefined since that's what the parameter would contain if nothing was passed.

BearerTokenAuthenticationPolicy in core-http will always pass a string or an array to the scopes parameter so I think it's pretty safe to only check for undefined since we don't actually use the real value of scopes in these forward-compatible implementations anyway.

amarzavery · 2019-07-19T22:43:55Z

package.json

  "dependencies": {
+    "@azure/core-auth": "1.0.0-preview.1",
    "@azure/ms-rest-azure-env": "^1.1.2",
    "@azure/ms-rest-js": "^1.8.7",


Should this be replaced by @azure/core-http? By not moving to core-http do we risk having type conflicts for the customer in his/her application?

Nah, we pulled the TokenCredential interface and related types into @azure/core-auth so that they can be reused in other places. @azure/core-http depends on @azure/core-auth in the unshipped code in master.

amarzavery · 2019-07-19T22:47:31Z

For safety, I would let this be a major version bump (3.0.0). It keeps a clear boundary between new stuff and the old stuff. Makes things easier for us to maintain.

ramya-rao-a · 2019-07-19T23:29:06Z

lib/credentials/coreAuthHelpers.ts

+    return {
+      token: token.accessToken,
+      expiresOnTimestamp:
+        typeof token.expiresOn === "string"


This assumes that expiresOn exists on all the credential types in ms-rest-nodeauth.
Are we sure about this assumption?

Good point, I can't find expiresOn being assigned anywhere in a few of the credential implementations. @amarzavery, do you remember how users are supposed to deal with token expiration in this library? I only see AzureCliCredentials implementing any kind of token refresh logic.

adal-node has an in memory token cache. It gets a new token if it is about to get expired or gives the token from the token cache.

For AzureCliCredentials we are calling the cli to authenticate and give us the new token that will be valid for the next one hour. Cli knows how to refresh the token if it is using it internally. We can't be calling the Cli for every request, as that will terribly slow down the sdk (spawning a new process and asking the Cli for creds).

Thus we try to do the same thing that adal-python would do anyways (if we are within the token refresh window (last 5 mins before the token expiry) then ask the Cli for token, it will ask adal-python which will then get the new token that will be valid for the next one hour).

Thus we need to check the expiresOn property.

I've loosened the expectation of an expiresOn property on the token and put a default expiration timestamp of an hour if one is not provided. What do you think?

ramya-rao-a · 2019-07-21T00:13:44Z

lib/credentials/coreAuthHelpers.ts

@@ -0,0 +1,28 @@
+import { AccessToken } from "@azure/core-auth";
+
+export interface TokenResponseLike {


Does this have to be exported? I don't see it being referenced anywhere other than this file

Possibly not, I'll give it a shot.

Looks like removing the export worked fine!

ramya-rao-a · 2019-07-21T00:46:24Z

lib/credentials/applicationTokenCertificateCredentials.ts

              return reject(tokenResponse);
            }
-            return resolve(tokenResponse as TokenResponse);
+            return resolve(prepareToken(tokenResponse as TokenResponse, scopes));


Nit: Where-ever applicable, please consider having a const for the prepared token instead of inlining the call to prepareToken in this manner. Having the call in a separate line would make it easier to add breakpoints when debugging

Sounds good, I'll make that change.

ramya-rao-a · 2019-07-21T00:49:29Z

test/credentials/coreAuthHelpersTests.ts

+import { assert } from "chai";
+import { prepareToken } from "../../lib/credentials/coreAuthHelpers";
+
+describe("prepareToken", function() {


There are 2 test files for MSI creds at the moment.
Can we update the test cases in these files to include a call to getToken(scopes) to check that the returned value is of type AccessToken?

I'll give it a shot

Added a test in each MSI spec file to verify that an AccessToken is returned when a scope is passed to getToken. Is that sufficient?

ramya-rao-a · 2019-07-21T01:23:17Z

Once we decide whether the fix in Azure/azure-sdk-for-js#4335 should be rolled back ...

Having creds from ms-rest-nodeauth be compatible with core-auth doesnt solve the problem that was being fixed in Azure/azure-sdk-for-js#4335 does it?

Or are we expecting that a combination of the above and Azure/azure-sdk-for-js#4367, solves the issue in Azure/azure-sdk-for-js#4335

daviwil · 2019-07-22T17:53:42Z

Or are we expecting that a combination of the above and Azure/azure-sdk-for-js#4367, solves the issue in Azure/azure-sdk-for-js#4335

Yep, it's the combination of fixes that mostly solves the problem. Previously the problem was that ms-rest-nodeauth credentials implemented TokenClientCredentials which has its own getToken method that takes no parameters and has a different return type. isTokenCredential would incorrectly identify such a credential as TokenCredential and then use BearerTokenAuthenticationPolicy for it instead of SigningPolicy. This would mean that the credential from ms-rest-nodeauth would get used incorrectly and probably throw an error (or at the very least authentication would fail).

After these new two PRs, we only ever use BearerTokenAuthenticationPolicy inside of ServiceClient so the isTokenCredential check doesn't need to be as strict. In fact, the check for signRequest now becomes a problem for ms-rest-nodeauth credentials because they would not be evaluated as TokenCredential implementations anymore due to having a signRequest method (even though they also implement TokenCredential). Thus the check had to be removed.

This does leave us with one problem, though: we don't have a reliable way to distinguish between a TokenClientCredentials and a TokenCredential because they both have a getToken method, just with different parameters and return type. As @bterlson mentioned in Azure/azure-sdk-for-js#4335, using getToken.length to get the parameter count is unreliable, so at the moment there isn't a better way to distinguish between the two interfaces. This would only be a problem if someone passed us a credential that implemented TokenClientCredentials but not TokenCredential (like one of the credentials from ms-rest-nodeauth before the changes in this PR).

I'd be happy to hear if someone has an idea for how to get around this problem!

daviwil · 2019-07-22T18:35:14Z

Had a better idea on how to make isTokenCredential a bit more reliable, left it as a comment over on the other PR: Azure/azure-sdk-for-js#4367 (comment)

ramya-rao-a · 2019-07-23T05:37:14Z

lib/credentials/azureCliCredentials.ts

   */
-  public async getToken(): Promise<TokenResponse> {
+  public getToken(): Promise<TokenResponse>;
+  public getToken(scopes: string | string[], options?: GetTokenOptions): Promise<AccessToken>;


Why no async keywords for the overloads?

Because the overload definitions are just typings and async is only really needed with a function body that uses await. That said, I'll add the keywords to minimize confusion.

My confusion was because in a couple of places we did add the async keyword for the overloads, and in some places we didnt

ramya-rao-a · 2019-07-23T05:41:39Z

lib/credentials/deviceTokenCredentials.ts

-  public getToken(): Promise<TokenResponse> {
+  public getToken(): Promise<TokenResponse>;
+  public getToken(scopes: string | string[], options?: GetTokenOptions): Promise<AccessToken>;
+  public async getToken(scopes?: string | string[]): Promise<TokenResponse | AccessToken> {


Missing async keywords on the overloads

ramya-rao-a · 2019-07-23T05:43:10Z

Changelog.md

@@ -1,4 +1,9 @@
 # Changelog
+
+## 3.0.0 - 2019/07/19


Date would need an update

I'll update it before merging

Merge from master and update deps

daviwil · 2020-01-30T00:21:54Z

This PR is no longer necessary as we've made @azure/core-http accept both ServiceClientCredentials and TokenCredential

daviwil requested review from a user, amarzavery, bterlson and ramya-rao-a July 19, 2019 19:03

daviwil requested review from kpajdzik and sergey-shandar as code owners July 19, 2019 19:03

daviwil force-pushed the core-auth-compat branch 2 times, most recently from 80f4186 to c9f02fc Compare July 19, 2019 19:53

daviwil mentioned this pull request Jul 19, 2019

[core-http] Remove ServiceClientCredentials from ServiceClient API Azure/azure-sdk-for-js#4367

Merged

daviwil mentioned this pull request Jul 19, 2019

ms-rest-nodeauth back-compat Azure/azure-sdk-for-js#4108

Closed

amarzavery reviewed Jul 19, 2019

View reviewed changes

ramya-rao-a reviewed Jul 19, 2019

View reviewed changes

ramya-rao-a reviewed Jul 21, 2019

View reviewed changes

ramya-rao-a reviewed Jul 23, 2019

View reviewed changes

ramya-rao-a approved these changes Jul 23, 2019

View reviewed changes

daviwil added 4 commits July 23, 2019 12:40

Make token credentials compatible with core-auth's TokenCredential

554e56f

Bump version to 3.0.0

a7f3433

Store intermediate prepareToken result in a const before returning

6ab4b17

Don't export TokenResponseLike

abfbfb0

daviwil added 3 commits July 23, 2019 12:41

Explicitly check for undefined in prepareToken

fab4f86

Verify MSI creds return AccessToken when used like TokenCredential

3f078c6

Make prepareToken more tolerant of omitted expiresOn

10e63d5

daviwil force-pushed the core-auth-compat branch from 565a9fe to 10e63d5 Compare July 23, 2019 19:43

daviwil added 3 commits July 23, 2019 12:44

Add missing 'async' keyword to method overloads

1652892

Add @azure/abort-controller as a devDependency

d100d74

Update 3.0.0 release date in Changelog.md

e9bafdb

ramya-rao-a approved these changes Jul 30, 2019

View reviewed changes

ramya-rao-a and others added 2 commits August 2, 2019 13:55

Merge remote-tracking branch 'origin/master' into pr/daviwil/66

f6b5783

Merge pull request #1 from ramya-rao-a/v3

e8320b8

Merge from master and update deps

daviwil mentioned this pull request Aug 22, 2019

Identity: Implement Interactive Authentication Support for Node.js applications Azure/azure-sdk-for-js#4774

Closed

daviwil closed this Jan 30, 2020

		@@ -0,0 +1,28 @@
		import { AccessToken } from "@azure/core-auth";

		export interface TokenResponseLike {

Conversation

daviwil commented Jul 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daviwil commented Jul 19, 2019

Uh oh!

daviwil commented Jul 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amarzavery Jul 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amarzavery commented Jul 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramya-rao-a Jul 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramya-rao-a commented Jul 21, 2019

Uh oh!

daviwil commented Jul 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daviwil commented Jul 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daviwil commented Jan 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

daviwil commented Jul 19, 2019 •

edited

Loading

amarzavery Jul 19, 2019 •

edited

Loading

ramya-rao-a Jul 21, 2019 •

edited

Loading

daviwil commented Jul 22, 2019 •

edited

Loading