[Storage] Use @azure/core-http in @azure/storage-* libraries to support @azure/identity

[daviwil]

This PR updates the @azure/storage-* libraries to use the new @azure/core-http and its TokenCredential interface so that the @azure/identity library's credential implementations can be used with it. The largest part of the changes is switching from @azure/ms-rest-js to @azure/core-http and the changes to the generated code from using an updated autorest.typescript.

The real meat of the changes is in the *Client classes like QueueClient where I add an additional TokenCredential type option for the credential argument and then set up the pipeline differently in Pipeline.ts depending on which credential has been passed.

Questions for Reviewers

• Should I split up the changes to the storage libraries into 3 separate PRs so that it doesn't get too crazy?
• Should I add a new source file under the samples/ folder strictly for showing how to use @azure/identity with this library?

Tasks

• Rename storage libraries' TokenCredential to RawTokenCredential
• Add code changes for storage-blob and storage-file
• Use @azure/identity in samples for all 3 libraries
• Add tests to verify that the Pipeline uses a BearerTokenAuthenticationPolicy when a TokenCredential is passed
• Remove type hints for model types that are mistakenly compared to ServiceCallback<void>
• Update BreakingChange.md if necessary to mention spec change ([Storage] Use @azure/core-http in @azure/storage-* libraries to support @azure/identity #3853 (comment))

[jeremymeng]

* Should I add a new source file under the `samples/` folder strictly for showing how to use `@azure/identity` with this library?

That would be valuable!

[jeremymeng]

/cc @XiaoningLiu I think this is a good start for enabling azure-identity. We can discuss better integration with current credentials in Storage but that may need to happen after this preview if it's taking longer to implement.

[jeremymeng]

This looks breaking too. I wonder whether the original generated code is wrong, or intentionally changed to a singleton, as the only valid type here is 'metadata'. @XiaoningLiu do you remember anything?

[daviwil]

Looks like it's represented as array in the OpenAPI json:

    "ListQueuesInclude": {
      "name": "include", 
      "in": "query", 
      "required": false, 
      "type": "array",
      "collectionFormat": "csv",
      "items": {
      "type": "string", 
      "enum": [ 
        "metadata" 
      ], 
      "x-ms-enum": { 
        "name": "ListQueuesIncludeType", 
        "modelAsString": false 
        }
      },
      "x-ms-parameter-location": "method",
      "description": "Include this parameter to specify that the queues's metadata be returned as part of the response body."
    },

[XiaoningLiu]

I believe this is a swagger breaking change introduced in https://msazure.visualstudio.com/One/_git/Storage-XStore/commit/76530600659f4d591d65266c74b2d96c785c3015?_a=compare&path=%2Fsrc%2FXFE%2FOpenApi%2FMicrosoft.QueueStorage%2Fpreview%2F2018-03-28%2Fqueue.json

We can take this change. But please update BreakingChange.md for this.

[XiaoningLiu]

@jeremymeng @daviwil

I had a great idea for the naming conflict regarding TokenCredential in storage SDK and identity package. Refer to comments above.

BTW, in https://github.com/azure/azure-sdk-for-js#authentication , it recommends to use @azure/ms-rest-nodeauth and @azure/ms-rest-browserauth for authentication. What's the relationship between them with @azure/identity?

---

In reply to: 502846562 [](ancestors = 502846562)

[XiaoningLiu]

"build:autorest": "autorest ./swagger/README.md --typescript [email protected]/[email protected]",

@david Wilson For core-http change, do we need a newer autorest.typescript version? If so, please update the autorest.typescript version here.

---

Refers to: sdk/storage/storage-queue/package.json:21 in bcf748f. [](commit_id = bcf748f, deletion_comment = False)

[XiaoningLiu]

Is there any autorest generator configuration change? If so, please update swagger/readmd.md, and any configurations defined in package.json build:autorest

[daviwil]

@XiaoningLiu Thanks for the great feedback! I've made some of the changes you requested and also added @azure/core-http integration for @azure/storage-file and @azure/storage-blob, please take a look.

Regarding the AutoRest changes, I'm currently working out of the azure-core branch of autorest.typescript and unfortunately I don't have a published update yet (we're waiting to settle on the new @azure/core-http library so that we can deprecate @azure/ms-rest-js before doing this).

No configuration changes are needed for the swagger/README.md files, it all works exactly the same as it did before. I've been generating the new StorageClients using the specs in the repo you linked to before on msazure.visualstudio.com.

[XiaoningLiu]

expiresOnTimestamp [](start = 6, length = 18)

We should assume token provided in RawTokenCredential will never expired, because it's up to customers refreshing token string value.

Can we set it to infinity?

[daviwil]

We've implemented token refreshing based on expiration time in BearerTokenAuthenticationPolicy so since it's possible for the user to change the token, we need a relatively short expiration so we aren't using an out of date token.

One possibility is that I could change that caching code to always ask for the current token when expiresOnTimestamp is set to 0.

[XiaoningLiu]

If am correct, RawTokenCredential is a simple wrapper for bearer token string. It needs manually refresh by customers upodating token property.

I'm not sure what happens after 2 mins timeout. Because RawTokenCredential doesn't define any token refresh logic.

So what will hapen after 2mins timeout? Will underlayer just call getToken again? If so, this call can be optimized by setting up an infinite token..

---

In reply to: 295116310 [](ancestors = 295116310)

[XiaoningLiu]

export class TokenCredentialPolicy extends CredentialPolicy {

Should delete this file, as RawTokenCredential doesn't depend on this policy anymore

---

Refers to: sdk/storage/storage-blob/src/policies/TokenCredentialPolicy.ts:17 in 6313f3c. [](commit_id = 6313f3c, deletion_comment = False)

[XiaoningLiu]

export class TokenCredentialPolicy extends CredentialPolicy {

Should remove this file as it's not necessary anymore.

---

Refers to: sdk/storage/storage-queue/src/policies/TokenCredentialPolicy.ts:18 in 6313f3c. [](commit_id = 6313f3c, deletion_comment = False)

[XiaoningLiu]

@jeremymeng Can you help open an issue to track the authentication merging between storage SDK and @azure/identity?

`Credential | TokenCredential' works to compatible with @azure/identity, but still feels not perfect, we can resolve this later before track2 GA.

[daviwil]

Finally completed work on storage-blob and storage-queue. I had to take the new TokenCredential back out of storage-file because it turns out only blob and queue support AAD OAuth (bearer token) authentication.

I've taken the PR out of Draft state so feel free to take a final look over this whenever you have a chance. I'll also review everything in more detail and make some tweaks if necessary.

[DanielRosenwasser]

Can I get a sense of what code bloat you're getting when converting to ES2015? I know ES5 is pretty rough, but I'm having a hard time understanding where things go really wrong.

The following code

let x = {
  async getToken(): Promise<Token> {
    return token;
  }
};

is emitted as follows in ES2015

"use strict";
var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, generator) {
    return new (P || (P = Promise))(function (resolve, reject) {
        function fulfilled(value) { try { step(generator.next(value)); } catch (e) { reject(e); } }
        function rejected(value) { try { step(generator["throw"](value)); } catch (e) { reject(e); } }
        function step(result) { result.done ? resolve(result.value) : new P(function (resolve) { resolve(result.value); }).then(fulfilled, rejected); }
        step((generator = generator.apply(thisArg, _arguments || [])).next());
    });
};
let x = {
    getToken() {
        return __awaiter(this, void 0, void 0, function* () {
            return token;
        });
    }
};

and when using --importHelpers with modules, that should look something like

import * as tslib from "tslib";

let x = {
    getToken() {
        return tslib.__awaiter(this, void 0, void 0, function* () {
            return token;
        });
    }
};

So can you give me an example of where things explode?

[jeremymeng]

[jeremymeng]

I am fine with async function. I don't thin the discussion should block this PR.

[daviwil]

OK, I've updated everything based on all of the review feedback so far. A summary of the latest changes:

• Updated BreakingChanges.md in storage-queue and storage-blob
• Updated samples to rename TokenCredential to RawTokenCredential
• Fixed minor spacing and test issues Jeremy found
• Changed RawTokenCredential.getToken back to an async method

@XiaoningLiu, let me know if you have any final comments, otherwise I'll get this merged first thing Monday morning Pacific time. Thanks!

[XiaoningLiu]

main [](start = 15, length = 4)

Is "Ad" or "Aad"? is this file name azureAdAuth.ts correct?

[daviwil]

I called it "azureAd" to refer to "Azure Active Directory", though I can see how having "aad" explicitly could make things clearer. I'll ask someone about this and will rename these files after the PR gets merged if necessary.

[XiaoningLiu]

Added several minor comments. Please make sure all test cases and samples provided works. Other looks good!

@HarshaNalluru @jeremymeng PR triggered checks skip all storage tests. Is this expected? Considering we added mocking test already, can we enable tests against PRs?

[daviwil]

Thanks @XiaoningLiu, I've made updates to address your feedback. Once CI goes green I will get this merged.

Thanks everyone!

[HarshaNalluru]

@HarshaNalluru @jeremymeng PR triggered checks skip all storage tests. Is this expected? Considering we added mocking test already, can we enable tests against PRs?

@XiaoningLiu,

• "playing back the tests" is still being skipped in the feature/storage branch but is enabled in the master branch.
• I'll merge the master branch into the feature branch which would technically mean that feature branch will have the tests running in playback mode as part of PR validations.
• But the recordings should be updated in the feature branch since some of the tests are updated/different compared to the master branch.

Created a new issue - #4036