Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Storage] Better integration of @azure/identity #3944

Closed
jeremymeng opened this issue Jun 19, 2019 · 10 comments
Closed

[Storage] Better integration of @azure/identity #3944

jeremymeng opened this issue Jun 19, 2019 · 10 comments
Labels
Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files)
Milestone

Comments

@jeremymeng
Copy link
Member

Continue discussion at #3853 (comment).

Storage already have a set of Credential policies that derived from its own Credential base type. when we added @azure/identity we have introduced a TokenCredential which is not compatible with Storage's types. There are changes like

  credential: Credential | TokenCredential,

There might be ways to follow Storage authentication pattern.

@jeremymeng jeremymeng added this to the Sprint 155 milestone Jun 19, 2019
@jeremymeng jeremymeng added Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files) labels Jun 19, 2019
@jeremymeng
Copy link
Member Author

@XiaoningLiu @daviwil

@daviwil
Copy link
Contributor

daviwil commented Jun 19, 2019

I think the idea is that the mainline auth implementations should be in @azure/identity since we're trying to centralize shared behavior (like authentication) in Core libraries. @bterlson and @schaabs, do you know whether all libraries should eventually snap to the authentication mechanisms provided by TokenCredential and @azure/identity?

@bterlson
Copy link
Member

@daviwil that's my understanding, but @schaabs should know best!

@praries880 praries880 removed the triage label Jun 19, 2019
@XiaoningLiu
Copy link
Member

SharedKey authentication is one of storage specific. Does @azure/identity export a higher level Credential interface? So we can make all storage credentials implement this Credential interface, and make all XXXClient constructors accept this Credential

@ramya-rao-a
Copy link
Contributor

@XiaoningLiu As far as I know, @azure/identity will not be supporting Shared Key auth.

@bterlson, @daviwil For Track 2 are we planning to support all the older credentials or just TokenCredential and the custom Shared Key auth?

Currently core-http supports both the older credentials and the new TokenCredential

@XiaoningLiu
Copy link
Member

XiaoningLiu commented Jul 11, 2019

@XiaoningLiu As far as I know, @azure/identity will not be supporting Shared Key auth.

@bterlson, @daviwil For Track 2 are we planning to support all the older credentials or just TokenCredential and the custom Shared Key auth?

Currently core-http supports both the older credentials and the new TokenCredential

In first track2 release, we just make the credential parameter accept both Credential from storage package and TokenCredential from identity package. It's not a good design, because the union type is confusing when putting 2 levels concept into one level. TokenCredential should be a lower level of Credential.

credential: Credential | TokenCredential,

@daviwil
Copy link
Contributor

daviwil commented Jul 11, 2019

@ramya-rao-a: The storage libraries will continue to provide any credential types that are specific to storage and won't be covered by @azure/identity. We'll have to take a look at what's been implemented in the storage libraries and decide which ones we want to explicitly reference in the various client constructors.

@XiaoningLiu: I discussed with @schaabs about whether we want to have a base Credential type that every other credential type flows from, his desire is to be explicit about which credential types are accepted in our API classes so that it's more obvious for the user when using the APIs in an editor with IntelliSense. Thus, we'd either continue with the union types or use constructor overloads to express the various credential types that might be passed in to the client classes.

@XiaoningLiu
Copy link
Member

Thanks daviwil @jeremymeng Or we can update union type to credential: SharedKeyCredential | AnonymousCredential | TokenCredential, this seems better.

@daviwil
Copy link
Contributor

daviwil commented Jul 11, 2019

@XiaoningLiu yep, exactly!

@HarshaNalluru
Copy link
Member

#4466

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files)
Projects
None yet
Development

No branches or pull requests

7 participants