POC v2: TokenProvider and OAuthPipleinePolicy #48234
Conversation
| } | ||
| } | ||
|
|
||
| public class ScopedAuthenticationPolicy : OAuthPipelinePolicy |
There was a problem hiding this comment.
[Policy Only] example stand-alone policy which encapsulates the TokenProvider implementation and type
| // usage for policy only and no public tokenProvider abstraction. | ||
| var policy = new ScopedAuthenticationPolicy(provider, "scope"); | ||
| client = new FooClient(new Uri("http://localhost"), policy); |
There was a problem hiding this comment.
[Policy Only] Sample usage of the policy approach with no public TokenProvider abstraction.
There was a problem hiding this comment.
I don't fully grok this - it's taking a TokenProvider as a param. Is the idea we'd collapse everything more like:
var entraNuggets = ...;
var policy = new EntraScopedAuthPolicy(entraNuggets, "scope");
client = new FooClient(new Uri("http://localhost"), policy);There was a problem hiding this comment.
yeah, this is a typo. I meant to refactor it that way.
There was a problem hiding this comment.
So the policy would be scoped to Entra? We'd need N auth policies for different Entra flows and anyone trying to use a different auth provider would need to write their own policy from scratch to use it?
There was a problem hiding this comment.
Yes - in theory we could have aggregated implementations that wrap something like DefaultAzureCredential for Entra flows. But different providers would need their own unless some generic lowest common denominator OAuth impl would suffice.
There was a problem hiding this comment.
The policy-only approach feels a lot less baked for a TypeSpec customer. Say I generate an Ollama client and it could be hosted locally with no auth, hosted on Azure ML with Entra, hosted on AWS with IAM, hosted on prem with SimpleIdServer, etc. Then with
- Token/TokenProvider: I'd just need to implement a new
TokenProviderfor my identity service and use it with a genericOAuthPipelinePolicythat handled all the refreshing logic. This should be doable by most people without a lot of identity experience. - Policy-only: I'd need to write everything myself if we didn't provide anything out of the box. Getting the token, applying it to a message via an auth header, possibly handling things like challenges, refreshing the token when it expired, ..., ? You'd probably need to know quite a bit about identity to get this right.
Does that understanding match yours?
There was a problem hiding this comment.
The policy-only approach feels a lot less baked for a TypeSpec customer. Say I generate an Ollama client and it could be hosted locally with no auth, hosted on Azure ML with Entra, hosted on AWS with IAM, hosted on prem with SimpleIdServer, etc. Then with
- Token/TokenProvider: I'd just need to implement a new
TokenProviderfor my identity service and use it with a genericOAuthPipelinePolicythat handled all the refreshing logic. This should be doable by most people without a lot of identity experience.
It's debatable how generally applicable the OAuthPipelinePolicy would be for actual cross-cloud scenarios. We could get most of the main parts right generically, like adding the header to the message and refresh. Handling challenges and more advanced scenarios might require some knobs or sub-types to refine, like we even do with our BearerTokenAuthenticationPolicy today for pure Entra scenarios.
- Policy-only: I'd need to write everything myself if we didn't provide anything out of the box. Getting the token, applying it to a message via an auth header, possibly handling things like challenges, refreshing the token when it expired, ..., ? You'd probably need to know quite a bit about identity to get this right.
Yes, although we could probably provide a base type that does most of the generic heavy lifting. We kind of do this with BearerTokenAuthenticationPolicy. However, it's also now tightly coupling token acquisition with message authorization. If we make that extensible, we now are back to having some kind of token provider abstraction.
Does that understanding match yours?
Mostly, yes. But see comments above.
| /// <summary> | ||
| /// Represents a provider that can provide a token. | ||
| /// </summary> | ||
| public abstract class TokenProvider<TContext> where TContext : ITokenContext |
There was a problem hiding this comment.
Example of TokenProvider generic type that implements various interfaces to indicate the capabilities it supports.
|
API change check APIView has identified API level changes in this PR and created following API reviews. |
| /// Represents a provider that can provide a token. | ||
| /// </summary> | ||
| /// <typeparam name="TContext"></typeparam> | ||
| public abstract class TokenProvider2<TContext> : ITokenProvider where TContext : ITokenContext |
There was a problem hiding this comment.
Another example of TokenProvider type that inherits from a common interface to deal with typing issues for client libraries accepting the least specific typed TokenProvider.
| public abstract ValueTask<Token> GetTokenAsync(CancellationToken cancellationToken); | ||
|
|
||
| /// <summary> | ||
| /// Proposed: Gets the token using the context of the policy instance. | ||
| /// </summary> | ||
| /// <param name="cancellationToken"></param> | ||
| /// <returns></returns> | ||
| public abstract Token GetToken(CancellationToken cancellationToken); |
There was a problem hiding this comment.
[Policy Only] This is a possible approach if we go with a policy-only solution so that clients can still fetch stand-alone tokens if needed.
| client = new FooClient(new Uri("http://localhost"), policy); | ||
| } | ||
|
|
||
| public class FooClient |
There was a problem hiding this comment.
Example generated client for the various scenarios
| public System.DateTimeOffset ExpiresOn { get { throw null; } protected set { } } | ||
| public System.DateTimeOffset? RefreshOn { get { throw null; } protected set { } } | ||
| public string Token { get { throw null; } protected set { } } | ||
| public string TokenType { get { throw null; } protected set { } } |
There was a problem hiding this comment.
Is there a reason we're duplicating all of Token inside here instead of just carrying one?
Would it make sense for the credential to optionally include a reference to its provider for refreshing? It's possible I'm thinking too much about the old TokenCredential design, but I'm looking at Credential as the thing I pass around to clients so I'd expect that to contain all the necessary parts kind of like:
public class Credential
{
public Credential(ITokenProvider provider) { }
public Credential(Token staticToken) { }
public Token? Token { get; }
public ITokenProvider? Provider { get; }
}There was a problem hiding this comment.
Added RefreshableToken and removed the Credential types for clarity.
| { | ||
| string[] Scopes { get; } | ||
| } | ||
| public partial interface ITokenContext |
There was a problem hiding this comment.
I really like this design. The idea of Azure's TokenRequestContext layering on top of this makes a lot of sense and feels like we might have designed it this way originally.
| public partial interface ITokenProvider | ||
| { | ||
| object CreateContext(System.Collections.Generic.IReadOnlyDictionary<string, object> properties); | ||
| System.ClientModel.Token GetAccessToken(object context, System.Threading.CancellationToken cancellationToken); |
There was a problem hiding this comment.
Nit - Should this just be called GetToken at this layer if we're dropping the AccessToken class/naming for SCM?
There was a problem hiding this comment.
This was a temporary naming workaround when I was playing with API shapes where I had conflicts in implementing this interface in TokenCredential.
| } | ||
| public partial class Token | ||
| { | ||
| public Token(string token, string tokenType, System.DateTimeOffset expiresOn, System.DateTimeOffset? refreshOn = default(System.DateTimeOffset?)) { } |
There was a problem hiding this comment.
I wonder if it's worth having this. Like maybe this would be too tempting for users of a GitHub library generated from TypeSpec to try using rather than wiring up to a full provider? We could debate making it more obscure with a factory Create and no public .ctor.
| public abstract partial class OAuthPipelinePolicy : System.ClientModel.Primitives.PipelinePolicy | ||
| { | ||
| protected OAuthPipelinePolicy() { } | ||
| public abstract System.ClientModel.Primitives.OAuthPipelinePolicy CreateAuthenticationPolicy(System.Collections.Generic.IReadOnlyDictionary<string, object> context); |
There was a problem hiding this comment.
Should this be static or how is this meant to be used if not?
There was a problem hiding this comment.
This was unintentional. There is a static implementation of the factory here
I was playing with the idea that the client ctor takes a factory instead of the policy implementation and the TypeSpec OAuth2 attribute would generate its defined properties as context to be passed to the factory. The factory could then construct the appropriate policy implementation. It's not fully baked in the test class where the sample usage is though. We'd need a factory interface which the client ctor would accept and the factory would need to implement the interface.
| /// <summary> | ||
| /// Get the token value. | ||
| /// </summary> | ||
| public string Token { get; protected set; } |
There was a problem hiding this comment.
Nit - consider calling this TokenValue so it matches what's in Token.
Contributing to the Azure SDK
Please see our CONTRIBUTING.md if you are not familiar with contributing to this repository or have questions.
For specific information about pull request etiquette and best practices, see this section.