-
Notifications
You must be signed in to change notification settings - Fork 534
Tokens can be cached beyond the lifetime of the (http) transport. #834
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add tests for supplying a custom ITokenCache that verifies it gets used?
@localden @DavidParks8 Do you have any thoughts on this?
/// </summary> | ||
[JsonIgnore] | ||
[JsonPropertyName("obtained_at")] | ||
public DateTimeOffset ObtainedAt { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the motivation for doing this considering a lot of custom stores are going to use System.Text.Json to serialize and deserialize the token, but I think this is a recipe for trouble. Even though we should trust the OAuth server returning the token, it's non-standard, and even a well-meaning OAuth server could try sending one that fails to parse.
It would be better to remove all the System.Text.Json attributes from the public TokenContainer type and use a mostly identical internal DTO in ClientOAuthProvider.FetchTokenAsync() that uses the attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted TokenContainer
back to internal and introduced a new public type TokenContainerCacheable
without any attributes. That unfortunately now requires copying the values back and forth at cache interactions.
/// <summary> | ||
/// Get the cached token. | ||
/// </summary> | ||
Task<TokenContainer?> GetTokenAsync(CancellationToken cancellationToken); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is called every request. It might be better as a ValueTask<TokenContainer?>
. Then, for consistency, I might make StoreTokenAsync return a ValueTask
. I'm curious if @stephentoub has opinions about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed to ValueTask
.
I added tests that verify a custom token cache is used to look up cached tokens and to store new ones. Since I wanted to test the public API, I had to use What do you think? |
I added the possibility to plugin a token cache, so that the client can be made to reuse its access token beyond the lifetime of the transport. By default tokens are only cached for the lifetime of the transport.
Fixes #749
Fixes #597
Motivation and Context
When the client acquires an access token and a refresh token, it should be able to utilze them to make authenticated requests to the server even after it was restarted. The client should not need to go through the OAuth dance again and again every time it is initialised (unless the access token is no longer valid and no refresh token was acquired or the refresh token was revoked).
How Has This Been Tested?
I tested it via a proof of concept application without a token cache (using the default
InMemoryTokenCache
) and with a custom implementation of a file-based token cache.Breaking Changes
The introduction of the
ITokenCache
is backwards compatible. If no custom token cache is provided, it falls back to anInMemoryTokenCache
with the same lifetime as the (oauth enabled http) transport (like before).Types of changes
Checklist
Additional context
TokenContainer
public, so that it can be passed into custom token caches.ObtainedAt
property serialisable, because it needs to remember when it was obtained after serialization to calculate the expiration.These two changes seemed more reasonable than introducing another token type for cache serialization only. I didn't find usages that could be disrupted by this.