[core-http] Remove ServiceClientCredentials from ServiceClient API by daviwil · Pull Request #4367 · Azure/azure-sdk-for-js

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

This change removes ServiceClientCredentials from the credentials parameter on ServiceClient so that only TokenCredential implementations can be used. Those who use ms-rest-nodeauth would be able to leverage the changes in Azure/ms-rest-nodeauth#66 to use those credential types with SDKs using the updated ServiceClient.

It's worth noting that this change does not prevent someone from constructing their own RequestPolicyFactory list with a signingPolicy. I figured it would be better to leave ServiceClientCredentials and SigningPolicy around (for now) if someone really needed to use them. We can delete them entirely at a later date if we decide to, or even now if anyone feels strongly that they should be gone.

Questions for Reviewers

Should RawTokenCredential live in @azure/core-auth so that it can be used outside of @azure/core-http, like in @azure/core-amqp?

sdk/core/core-http/lib/serviceClient.ts

sdk/core/core-http/lib/credentials/rawTokenCredential.ts

daviwil · 2019-07-22T18:34:41Z

Regarding Azure/ms-rest-nodeauth#66 (comment) , I might have a better heuristic for isTokenCredential that may not be 100% perfect but more reliable than just checking for getToken on the credential object. I looked into function.length and it appears to be supported across all browser JS engines. With that in mind, the logic for isTokenCredential could be changed to the following:

  return credential
    && typeof credential.getToken === "function"
    && (credential.signRequest === undefined || credential.getToken.length > 0);

This new logic ensures that if the credential object also has a signRequest method, that the getToken method be defined to take a non-zero number of parameters. My theory is that this scopes the length check to only be applicable when the credential might also be a ServiceClientCredential so that if getToken.length returns 0 on a particular platform when it should be non-zero, at least we're wrong only when we have good reason to suspect the credential might just be a ServiceClientCredential that also implements TokenClientCredentials.

In other words, this new logic reduces the number of false positives we'd get compared to only checking for the existence of getToken on the credential object and it definitely helps to avoid the case where someone tries to use a credential from an incompatible version of ms-rest-nodeauth and succeeds, resulting in errors.

What do you think @bterlson?

bterlson · 2019-07-22T19:17:06Z

@daviwil seems like a good approach!

daviwil · 2019-07-22T20:19:49Z

OK, I updated isTokenCredential with the proposed changes and it seems to work as expected both with the current ms-rest-nodeauth (returns false on those credentials) and the updated version that is compatible with TokenCredential (returns true for those credentials). Can you take another look @ramya-rao-a?

sdk/core/core-auth/src/tokenCredential.ts

sdk/core/core-http/lib/serviceClient.ts

sdk/core/core-http/samples/node-sample.ts

This rolls back the change made in 771614e because it will prevent forward-compatibility in `ms-rest-nodeauth` and `ms-rest-browserauth` credentials.

ramya-rao-a · 2019-07-23T21:25:37Z

sdk/core/core-auth/review/core-auth.api.md

+export class RawTokenCredential implements TokenCredential {
+    constructor(token: string, expiresOn?: Date);
+    expiresOn: Date;
+    getToken(_scopes: string | string[], _options?: GetTokenOptions): Promise<AccessToken | null>;


Why are the params having underscore here?

They are unused in the scope of this function, adding the _ prefix tells the linter to ignore the fact that they aren't used. I can look into using an eslint rule disable comment instead.

nah, not worth it
this is fine

ramya-rao-a

I don't like the name RawTokenCredential, but that doesn't have to block this PR.
Let's log an issue to discuss the name.

Otherwise, things looking good.

Since we have made quite some changes after @amarzavery approved the PR, maybe re-request review?

daviwil · 2019-07-23T21:48:00Z

While waiting on @amarzavery to take another look, how about one of the following names:

BareTokenCredential
StringTokenCredential
BasicTokenCredential
SampleTokenCredential
TestTokenCredential

Trying to think of something that would make it obvious that it's mostly meant for test purposes.

ramya-rao-a · 2019-07-23T21:49:54Z

Trying to think of something that would make it obvious that it's mostly meant for test purposes.

Are we sure that this was meant for tests and samples only?

daviwil · 2019-07-23T21:52:31Z

No, I'm not sure of that, that's why I said "mostly" :) My guess is that it's unlikely anyone would use this in production instead of making their own TokenCredential implementation (or ServiceClientCredentials in the past).

SimpleTokenCredential could be another option.

ramya-rao-a · 2019-07-23T23:26:10Z

SimpleTokenCredential is my winner with BasicTokenCredential as a runner up

daviwil · 2019-07-25T21:49:13Z

Renamed RawTokenCredential to SimpleTokenCredential in the latest change.

daviwil · 2019-07-25T22:35:42Z

Thanks all! Merging this.

…4367) * Remove ServiceClientCredentials from ServiceClient API * Remove additional check added to isTokenCredential This rolls back the change made in 771614e because it will prevent forward-compatibility in `ms-rest-nodeauth` and `ms-rest-browserauth` credentials. * Improve RawTokenCredential comment * Improve credential detection logic in ServiceClient * Add isTokenCredential heuristic to identify TokenClientCredentials * Simplify ServiceClient constructor logic around credentials * Move RawTokenCredential from core-http to core-arm * Delete TokenCredentials, update samples to use RawTokenCredential * Rename RawTokenCredential to SimpleTokenCredential

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

daviwil changed the title ~~Remove ServiceClientCredentials from ServiceClient API~~ [core-http] Remove ServiceClientCredentials from ServiceClient API Jul 19, 2019

daviwil force-pushed the adieu-service-client-credentials branch from f78c4a8 to 433ef10 Compare July 19, 2019 22:34

This was referenced Jul 19, 2019

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

Closed

ms-rest-nodeauth back-compat #4108

Closed

amarzavery approved these changes Jul 19, 2019

View reviewed changes

ramya-rao-a reviewed Jul 21, 2019

View reviewed changes

sdk/core/core-http/lib/serviceClient.ts Outdated Show resolved Hide resolved

ramya-rao-a reviewed Jul 21, 2019

View reviewed changes

sdk/core/core-http/lib/serviceClient.ts Outdated Show resolved Hide resolved

ramya-rao-a reviewed Jul 21, 2019

View reviewed changes

sdk/core/core-http/lib/credentials/rawTokenCredential.ts Outdated Show resolved Hide resolved

daviwil force-pushed the adieu-service-client-credentials branch 2 times, most recently from e55aead to 1d3997e Compare July 22, 2019 20:31

ramya-rao-a reviewed Jul 23, 2019

View reviewed changes

sdk/core/core-auth/src/tokenCredential.ts Outdated Show resolved Hide resolved

ramya-rao-a reviewed Jul 23, 2019

View reviewed changes

sdk/core/core-http/lib/serviceClient.ts Outdated Show resolved Hide resolved

ramya-rao-a reviewed Jul 23, 2019

View reviewed changes

sdk/core/core-http/lib/serviceClient.ts Outdated Show resolved Hide resolved

ramya-rao-a reviewed Jul 23, 2019

View reviewed changes

sdk/core/core-http/samples/node-sample.ts Outdated Show resolved Hide resolved

daviwil added 8 commits July 23, 2019 12:32

Remove ServiceClientCredentials from ServiceClient API

5d7d7e2

Remove additional check added to isTokenCredential

ad27ff9

This rolls back the change made in 771614e because it will prevent forward-compatibility in `ms-rest-nodeauth` and `ms-rest-browserauth` credentials.

Improve RawTokenCredential comment

39eb836

Improve credential detection logic in ServiceClient

7cf5f6e

Add isTokenCredential heuristic to identify TokenClientCredentials

1a6f8bf

Simplify ServiceClient constructor logic around credentials

4439ce6

Move RawTokenCredential from core-http to core-arm

f80f304

Delete TokenCredentials, update samples to use RawTokenCredential

f8f1304

daviwil force-pushed the adieu-service-client-credentials branch from 1d3997e to f8f1304 Compare July 23, 2019 19:33

ramya-rao-a reviewed Jul 23, 2019

View reviewed changes

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

View reviewed changes

daviwil requested a review from amarzavery July 23, 2019 21:43

Rename RawTokenCredential to SimpleTokenCredential

ee97655

daviwil force-pushed the adieu-service-client-credentials branch from a5e2f24 to ee97655 Compare July 25, 2019 21:47

amarzavery approved these changes Jul 25, 2019

View reviewed changes

daviwil merged commit 0a46246 into Azure:master Jul 25, 2019

daviwil deleted the adieu-service-client-credentials branch July 25, 2019 23:03

Conversation

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

bterlson commented Jul 22, 2019

Uh oh!

daviwil commented Jul 22, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ramya-rao-a Jul 23, 2019

Choose a reason for hiding this comment

Uh oh!

daviwil Jul 23, 2019

Choose a reason for hiding this comment

Uh oh!

ramya-rao-a Jul 23, 2019

Choose a reason for hiding this comment

Uh oh!

ramya-rao-a left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

ramya-rao-a commented Jul 23, 2019

Uh oh!

daviwil commented Jul 23, 2019

Uh oh!

ramya-rao-a commented Jul 23, 2019

Uh oh!

daviwil commented Jul 25, 2019

Uh oh!

daviwil commented Jul 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

daviwil commented Jul 19, 2019 •

edited

Loading

daviwil commented Jul 22, 2019 •

edited

Loading

daviwil commented Jul 23, 2019 •

edited

Loading