Unable to reassign the Authenticator property in v109

[Balkoth]

Describe the bug
The changes introduced by making the Authenticator on RestClient immutable makes it impossible to use the same RestClient for authentication and after successfully authentication setting the Authenticator for further method calls which require the client to be authenticated.

To Reproduce

internal sealed class CustomRestClient : IDisposable
{
  private readonly RestClient _restClient;

  public CustomRestClient(string baseUrl) => _restClient = new RestClient(baseUrl);  

  public void Dispose() => _restClient.Dispose();

  public async Task<bool> Authenticate(string clientId, string clientSecret)
  {
    RestRequest request = new RestRequest("api/oauth2/token");
    request.AddParameter("grant_type", "client_credentials");
    request.AddParameter("client_id", clientId);
    request.AddParameter("client_secret", clientSecret);
  
    AppOAuthToken? response = await _restClient.PostAsync<AppOAuthToken>(request);
    _restClient.Authenticator = response != null && response.AccessToken != null ? new JwtAuthenticator(response.AccessToken) : default;
  
    return response != null && response.AccessToken != null;
  }
}

Expected behavior
The docs state that a single instance of RestClient should be used to prevent starvation.

Desktop (please complete the following information):

• OS: Windows 10 21H2
• .NET 7
• Version 109.0.1

[Balkoth]

Working around this by initializing the RestClient with a JwtAuthenticator with an invalid token and using this to authenticate against the rest endpoint. Then after succesfully authenticating use SetBearerToken to set the token on the JwtAuthenticator for further method calls.

internal sealed class CustomRestClient : IDisposable
{
  private const string EMPTY_TOKEN = "EMPTY_TOKEN";

  private readonly RestClient _restClient;

  public CustomRestClient(string baseUrl) => _restClient = new RestClient(new RestClientOptions(baseUrl) { Authenticator = new JwtAuthenticator(EMPTY_TOKEN) });

  public void Dispose() => _restClient.Dispose();

  public async Task<bool> Authenticate(string clientId, string clientSecret)
  {
    RestRequest request = new RestRequest("api/oauth2/token");
    request.AddParameter("grant_type", "client_credentials");
    request.AddParameter("client_id", clientId);
    request.AddParameter("client_secret", clientSecret);
  
    AppOAuthToken? response = await _restClient.PostAsync<AppOAuthToken>(request);
    ((JwtAuthenticator)_restClient.Options.Authenticator!).SetBearerToken(response != null && response.AccessToken != null ? response.AccessToken : EMPTY_TOKEN);
  
    return response != null && response.AccessToken != null;
  }
}

I don't know if any other, than the rest endpoint i currently use, support this behaviour, so use with caution.

[alexeyzimarev]

The docs suggests using a single instance for making frequent calls. If you are doing a single call for authentication purposes you can use a separate, one-time-use client instance.

You can also hold the authenticator instance inside your wrapper:

JwtAuthenticator _authenticator;

public CustomRestClient(string baseUrl) {
    _authenticator = new JwtAuthenticator(EMPTY_TOKEN)
    _restClient = new RestClient(new RestClientOptions(baseUrl) { 
        Authenticator = authenticator 
    });
}

and then call SetBearerToken when you have it.

However, if you look at my old Twitter OAuth2 authenticator sample, it makes a call on-demand using a separate client instance inside the authenticator. It allows you to have full control over the flow.

I think the main deficiency of the authenticator approach with tokens is that it doesn't support token refresh. I believe we need a separate issue to discuss most common scenarios. Getting the token using grant type, client id and secret is very common. But the OIDC token and authentication token, for example, work differently.

It would be great to document specific flows in that new issue to figure out a workable solution, as we all know that the IAuthenticator abstraction is too weak.

[alexeyzimarev]

One comment about this:

The docs state that a single instance of RestClient should be used to prevent starvation.

That is true, but how can a single additional client instance only used for authentication (once) can cause starvation? The infamous issue with too many open sockets is caused by repeated creation of message handlers, which is not the case when we are talking about a single call to get a token.

[rassilon]

@alexeyzimarev , while migrating a C# REST library to 109 I ended up with this authenticator based on your old twitter example.

It is assisted with this additional logic in the core call to RestSharpClient Execute method:

            response = _restSharpClient.Execute(request);
            // NOTE: Completed was returned with earlier versions of RestSharp
            // (Pre 109.x)
            if ((response.ResponseStatus == ResponseStatus.Completed
                 || response.ResponseStatus == ResponseStatus.Error)
                && response.StatusCode == HttpStatusCode.Unauthorized)
            {
                _restSharpClient.Options.Authenticator.Authenticate(_restSharpClient, request);
                response = _restSharpClient.Execute(request);
            }

The authenticator uses an AsyncLock to ensure only one additional RestClient gets created for the new token aquistion.

FYI...

[alexeyzimarev]

I'd really like to find out how authenticators can be made better. Checking the status code for 401 was one of my thoughts too. If you have a proposal for the authentication flow and related changes in code, please write here.

[rassilon]

Given how complex and fractured the authentication space is I think the only easy addition would be to add some assistance to the supplied Authenticator for 401 handling for expired/revoked tokens.
Maybe something like this: (pseudo-code)

       <inside Execute method>:
       If response status is 401:
           if request included an auth header value: 
                add some additional state to pass to .Authenticate call to hint that it might be a re-auth (either due to expiration, or token revocation, the cause really isn't important of course)
                if the returned token is different:
                    re-execute the same url with the new token from the authenticator
                else: 
                     nothing else to do, let the caller deal with the problem

Determining if the request has an auth header could be done a couple of different ways if more than just 'Authorization' needs to be supported.

[rassilon]

Additionally, if Authenticators could somehow re-use the existing RestClient instance instead of needing to create another one to perform the authentication request with, that'd be nice as well but that seems like a tricky road to go down.

[rassilon]

After checking out: https://www.oauth.com/oauth2-servers/making-authenticated-requests/refreshing-an-access-token/

It seems like there should be a second method for authenticators: AuthenticateWithFailedResponse or something so that the Authenticator can choose to examine the content of the failed response for error conditions, decide to utilize a refresh token, or just re-request the token based on whatever OAuth flow that is being used.

So, the above pseudo-code might look like this instead:

       <inside Execute method>:
       If response status is 401:
           if request included an auth header value: 
                call Authenticator.AuthenticateWithFailedRespond(RestResponse)
                if the returned token is different:
                    re-execute the same url with the new token from the authenticator
                    return the new response
                else: 
                     nothing else to do, let the caller deal with the problem and just return the original response

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[repo-ranger]

⚠️ This issue has been marked wontfix and will be closed in 3 days