Skip to content

Commit 2674757

Browse files
alexeyzimarevclaude
alexeyzimarev
and
claude
committed
Address Qodo code review: CancellationToken, nullable ExpiresIn, scope on refresh
- Add optional CancellationToken to IAuthenticator.Authenticate and propagate it through all authenticators to SemaphoreSlim.WaitAsync, HttpClient.PostAsync, and the user delegate in OAuth2TokenAuthenticator - Make OAuth2TokenResponse.ExpiresIn nullable (int?) so missing expires_in from the server is treated as non-expiring instead of causing a refresh storm - Send scope parameter in OAuth2RefreshTokenAuthenticator when configured Co-Authored-By: Claude Opus 4.6 <[email protected]>
1 parent ae15387 commit 2674757

11 files changed

Lines changed: 81 additions & 19 deletions

‎src/RestSharp/Authenticators/AuthenticatorBase.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,6 @@ public abstract class AuthenticatorBase(string token) : IAuthenticator {
1919

2020
protected abstract ValueTask<Parameter> GetAuthenticationParameter(string accessToken);
2121

22-
public async ValueTask Authenticate(IRestClient client, RestRequest request)
22+
public async ValueTask Authenticate(IRestClient client, RestRequest request, CancellationToken cancellationToken = default)
2323
=> request.AddOrUpdateParameter(await GetAuthenticationParameter(Token).ConfigureAwait(false));
2424
}

‎src/RestSharp/Authenticators/IAuthenticator.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,5 +15,5 @@
1515
namespace RestSharp.Authenticators;
1616

1717
public interface IAuthenticator {
18-
ValueTask Authenticate(IRestClient client, RestRequest request);
18+
ValueTask Authenticate(IRestClient client, RestRequest request, CancellationToken cancellationToken = default);
1919
}

‎src/RestSharp/Authenticators/OAuth/OAuth1Authenticator.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ public class OAuth1Authenticator : IAuthenticator {
4141
public virtual string? ClientUsername { get; set; }
4242
public virtual string? ClientPassword { get; set; }
4343

44-
public ValueTask Authenticate(IRestClient client, RestRequest request) {
44+
public ValueTask Authenticate(IRestClient client, RestRequest request, CancellationToken cancellationToken = default) {
4545
var workflow = new OAuthWorkflow {
4646
ConsumerKey = ConsumerKey,
4747
ConsumerSecret = ConsumerSecret,

‎src/RestSharp/Authenticators/OAuth2/OAuth2EndpointAuthenticatorBase.cs‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -49,8 +49,8 @@ protected void SetInitialToken(string accessToken, DateTimeOffset expiresAt) {
4949
_tokenExpiry = expiresAt;
5050
}
5151

52-
public async ValueTask Authenticate(IRestClient client, RestRequest request) {
53-
var token = await GetOrRefreshTokenAsync().ConfigureAwait(false);
52+
public async ValueTask Authenticate(IRestClient client, RestRequest request, CancellationToken cancellationToken = default) {
53+
var token = await GetOrRefreshTokenAsync(cancellationToken).ConfigureAwait(false);
5454
request.AddOrUpdateParameter(new HeaderParameter(KnownHeaders.Authorization, $"Bearer {token}"));
5555
}
5656

@@ -65,11 +65,11 @@ public async ValueTask Authenticate(IRestClient client, RestRequest request) {
6565
/// </summary>
6666
protected virtual void OnTokenResponse(OAuth2TokenResponse response) { }
6767

68-
async Task<string> GetOrRefreshTokenAsync() {
68+
async Task<string> GetOrRefreshTokenAsync(CancellationToken cancellationToken) {
6969
if (_accessToken != null && DateTimeOffset.UtcNow < _tokenExpiry)
7070
return _accessToken;
7171

72-
await _lock.WaitAsync().ConfigureAwait(false);
72+
await _lock.WaitAsync(cancellationToken).ConfigureAwait(false);
7373

7474
try {
7575
if (_accessToken != null && DateTimeOffset.UtcNow < _tokenExpiry)
@@ -83,7 +83,7 @@ async Task<string> GetOrRefreshTokenAsync() {
8383
}
8484

8585
using var content = new FormUrlEncodedContent(parameters);
86-
using var response = await _tokenClient.PostAsync(TokenRequest.TokenEndpointUrl, content).ConfigureAwait(false);
86+
using var response = await _tokenClient.PostAsync(TokenRequest.TokenEndpointUrl, content, cancellationToken).ConfigureAwait(false);
8787

8888
var body = await response.Content.ReadAsStringAsync().ConfigureAwait(false);
8989

@@ -96,7 +96,9 @@ async Task<string> GetOrRefreshTokenAsync() {
9696
throw new InvalidOperationException($"Token endpoint returned an invalid response: {body}");
9797

9898
_accessToken = tokenResponse.AccessToken;
99-
_tokenExpiry = DateTimeOffset.UtcNow.AddSeconds(tokenResponse.ExpiresIn) - TokenRequest.ExpiryBuffer;
99+
_tokenExpiry = tokenResponse.ExpiresIn.HasValue
100+
? DateTimeOffset.UtcNow.AddSeconds(tokenResponse.ExpiresIn.Value) - TokenRequest.ExpiryBuffer
101+
: DateTimeOffset.MaxValue;
100102

101103
OnTokenResponse(tokenResponse);
102104
TokenRequest.OnTokenRefreshed?.Invoke(tokenResponse);

‎src/RestSharp/Authenticators/OAuth2/OAuth2RefreshTokenAuthenticator.cs‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,14 +38,20 @@ DateTimeOffset expiresAt
3838
SetInitialToken(accessToken, expiresAt);
3939
}
4040

41-
protected override Dictionary<string, string> BuildRequestParameters()
42-
=> new() {
41+
protected override Dictionary<string, string> BuildRequestParameters() {
42+
var parameters = new Dictionary<string, string> {
4343
["grant_type"] = "refresh_token",
4444
["client_id"] = TokenRequest.ClientId,
4545
["client_secret"] = TokenRequest.ClientSecret,
4646
["refresh_token"] = _refreshToken
4747
};
4848

49+
if (TokenRequest.Scope != null)
50+
parameters["scope"] = TokenRequest.Scope;
51+
52+
return parameters;
53+
}
54+
4955
protected override void OnTokenResponse(OAuth2TokenResponse response) {
5056
if (!string.IsNullOrEmpty(response.RefreshToken))
5157
_refreshToken = response.RefreshToken;

‎src/RestSharp/Authenticators/OAuth2/OAuth2TokenAuthenticator.cs‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -36,22 +36,22 @@ public OAuth2TokenAuthenticator(Func<CancellationToken, Task<OAuth2Token>> getTo
3636
_tokenType = tokenType;
3737
}
3838

39-
public async ValueTask Authenticate(IRestClient client, RestRequest request) {
40-
var token = await GetOrRefreshTokenAsync().ConfigureAwait(false);
39+
public async ValueTask Authenticate(IRestClient client, RestRequest request, CancellationToken cancellationToken = default) {
40+
var token = await GetOrRefreshTokenAsync(cancellationToken).ConfigureAwait(false);
4141
request.AddOrUpdateParameter(new HeaderParameter(KnownHeaders.Authorization, $"{_tokenType} {token}"));
4242
}
4343

44-
async Task<string> GetOrRefreshTokenAsync() {
44+
async Task<string> GetOrRefreshTokenAsync(CancellationToken cancellationToken) {
4545
if (_accessToken != null && DateTimeOffset.UtcNow < _tokenExpiry)
4646
return _accessToken;
4747

48-
await _lock.WaitAsync().ConfigureAwait(false);
48+
await _lock.WaitAsync(cancellationToken).ConfigureAwait(false);
4949

5050
try {
5151
if (_accessToken != null && DateTimeOffset.UtcNow < _tokenExpiry)
5252
return _accessToken;
5353

54-
var result = await _getToken(CancellationToken.None).ConfigureAwait(false);
54+
var result = await _getToken(cancellationToken).ConfigureAwait(false);
5555
_accessToken = result.AccessToken;
5656
_tokenExpiry = result.ExpiresAt;
5757

‎src/RestSharp/Authenticators/OAuth2/OAuth2TokenResponse.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ public record OAuth2TokenResponse {
2828
public string TokenType { get; init; } = "";
2929

3030
[JsonPropertyName("expires_in")]
31-
public int ExpiresIn { get; init; }
31+
public int? ExpiresIn { get; init; }
3232

3333
[JsonPropertyName("refresh_token")]
3434
public string? RefreshToken { get; init; }

‎src/RestSharp/RestClient.Async.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ async Task<HttpResponse> ExecuteRequestAsync(RestRequest request, CancellationTo
108108
var authenticator = request.Authenticator ?? Options.Authenticator;
109109

110110
if (authenticator != null) {
111-
await authenticator.Authenticate(this, request).ConfigureAwait(false);
111+
await authenticator.Authenticate(this, request, cancellationToken).ConfigureAwait(false);
112112
}
113113

114114
var contentToDispose = new List<RequestContent>();

‎test/RestSharp.Tests/Auth/AuthenticatorTests.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ public async Task Should_add_authorization_form_parameter() {
3434
}
3535

3636
class TestAuthenticator(ParameterType type, string name, string value) : IAuthenticator {
37-
public ValueTask Authenticate(IRestClient client, RestRequest request) {
37+
public ValueTask Authenticate(IRestClient client, RestRequest request, CancellationToken cancellationToken = default) {
3838
request.AddParameter(name, value, type);
3939
return default;
4040
}

‎test/RestSharp.Tests/Auth/OAuth2ClientCredentialsAuthenticatorTests.cs‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,33 @@ await act.Should().ThrowAsync<InvalidOperationException>()
153153
.WithMessage("*invalid response*");
154154
}
155155

156+
[Fact]
157+
public async Task Should_treat_missing_expires_in_as_non_expiring() {
158+
var callCount = 0;
159+
160+
_mockHttp.When(HttpMethod.Post, TokenEndpoint)
161+
.Respond(_ => {
162+
Interlocked.Increment(ref callCount);
163+
return new HttpResponseMessage(HttpStatusCode.OK) {
164+
Content = new StringContent(
165+
"""{"access_token":"no-expiry-token","token_type":"Bearer"}""",
166+
System.Text.Encoding.UTF8,
167+
"application/json"
168+
)
169+
};
170+
});
171+
172+
using var authenticator = new OAuth2ClientCredentialsAuthenticator(CreateRequest());
173+
174+
var request1 = new RestRequest();
175+
await authenticator.Authenticate(null!, request1);
176+
177+
var request2 = new RestRequest();
178+
await authenticator.Authenticate(null!, request2);
179+
180+
callCount.Should().Be(1, "token without expires_in should be cached indefinitely");
181+
}
182+
156183
[Fact]
157184
public async Task Should_send_scope_when_configured() {
158185
_mockHttp.When(HttpMethod.Post, TokenEndpoint)

0 commit comments

Comments
 (0)