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

StaticTokenCredential #22955

Merged
merged 9 commits into from
Sep 3, 2021
Merged

Conversation

christothes
Copy link
Member

@christothes christothes commented Jul 29, 2021

@ghost ghost added the Azure.Identity label Jul 29, 2021
@christothes christothes self-assigned this Jul 29, 2021
@pakrym
Copy link
Contributor

pakrym commented Jul 29, 2021

What about a static method? TokenCredential.Create(string token)?

We did a similar thing with Page and AsyncPageable.

@Azure Azure deleted a comment from check-enforcer bot Jul 29, 2021
@christothes
Copy link
Member Author

What about a static method? TokenCredential.Create(string token)?

We did a similar thing with Page and AsyncPageable.

Instead of a ctor?

@pakrym
Copy link
Contributor

pakrym commented Jul 29, 2021

Instead of a ctor?

Instead of an additional class. TokenCredential.Create(string token) would return a TokenCredential

@christothes
Copy link
Member Author

Instead of a ctor?

Instead of an additional class. TokenCredential.Create(string token) would return a TokenCredential

Interesting idea - but I'm not sure that I like having a generic TokenCredential that can't be identified specifically as a static token credential.

@pakrym
Copy link
Contributor

pakrym commented Jul 29, 2021

not sure that I like having a generic TokenCredential that can't be identified specifically as a static token credential.

Why not?

@schaabs
Copy link
Member

schaabs commented Jul 29, 2021

What about a static method? TokenCredential.Create(string token)?

I do like this from a design perspective, but honestly I'm not sold on adding the class or the static method. I think either lead users down a bad path because while simply using a static token seems like the path of least resistance, it will only work for a short period of time and the user needs to manage it's lifetime and scope properly which is not trivial. This is why we have be reluctant to add this class, and I think that a static method would exacerbate this problem as users could look at TokenCredential (which is abstract so no ctor) find this static method and assume that's what they are intended to use for authentication.

/// If <see cref="GetToken"/> is called, the provided delegate will be executed sync over async.</param>
/// <returns></returns>
public static TokenCredential Create(
Func<TokenRequestContext, CancellationToken, ValueTask<AccessToken>> getTokenAsync) => new StaticTokenCredential(getTokenAsync);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the func need to return ValueTask? If not I wonder if Task<AccessToken> would be easier for users.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to keep it 1:1 with GetToken's signature, unless you feel strongly otherwise

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel strongly I just wanted to pose the question to others.

@christothes christothes merged commit 71bc267 into Azure:main Sep 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants