restsharp / RestSharp

Simple REST and HTTP API Client for .NET
https://restsharp.dev
Apache License 2.0
9.57k stars 2.34k forks source link

using JwtAuthenticator in v107 #1767

Closed DevOnBike closed 2 years ago

DevOnBike commented 2 years ago

Hello,

according to what is described in https://restsharp.dev/v107/#recommended-usage does it mean that GitHubClient should be registered as singleton in DI?

If so than also authenticator is cached also than how one can provide diffrent values of JWT token to every call in GitHubClient? This is important because lifetime of token sometime is not synchronized with lifetime of RestClient that is using this token.

alexeyzimarev commented 2 years ago

Did you check this? https://restsharp.dev/usage.html#recommended-usage

kendallb commented 2 years ago

I believe the Authenticator should be removed from the client and added to the RestRequest payload instead, so that it can be changed on a per request basis as mentioned here:

https://github.com/restsharp/RestSharp/issues/1786

That would solve this problem.

alexeyzimarev commented 2 years ago

But it will break code for everyone who uses authenticators as it was originally implemented in RestSharp. Btw the reason to have the SetToken is to cover that particular issue.

I am pretty sure that 90% of the APIs just need fixed credentials, and making it work per request would create a lot of overhead.

kendallb commented 2 years ago

Yes, it would break the API. But so much has changed and moved out of RestClient anyway, maybe now is a good time to do that :)

The authenticator is simply setting up header values in the request, but since it's a request thing it really needs to be able to b configured on a per request basis which is just not possible if you configure it on the base RestClient. In my case all my API keys are static and do not change, but they DO come from runtime configuration so it is possible for someone to change those values while the application stack is running. If they are configured against a static, re-used client instance, stuff will break.

So my solution for the moment is to pass in the HttpBasicAuthenticator username and password to my RestClientFactory and use it as part of the cache key. So if someone does change it, then at least a new instance of RestClient is returned. The old one will end up being cached until our applications are recycled/restarted, but that is better than nothing. I thought about checking if it had changed and disposing of the old client instances, but it will rarely happen in practice so not much point in that.

However if the Authenticator used is not a simple HttpBasicAuthenticator, but something more complex such as OAuth or something that requires dynamically getting credentials to pass perhaps based on the logged in user, we have a real problem.

What I would suggest is if you don't want to break compatibility with the Authenticator hanging off the RestClient, is to leave a version of it there and mark it 'Deprecated', but add a new one to the RestRequest and use that one in preference to the one in the client. Then over time at least folks as they migrate will notice the deprecated function and adjust, and eventually you can remove it.

DevOnBike commented 2 years ago

This is exactly my case: my credentials are not fixed, jwt token can change during lifetime of application that uses RestSharp. If there is a chance that this RestSharp will cache credentials then for me it is not acceptable. Currently I'm stuck between keeping old version of RestSharp (<107 which leaks sockets) or switch to M$ implementation.

alexeyzimarev commented 2 years ago

It's not the API issue but more about the fact that the current authenticator implementation is how a lot of people use it, and it hasn't changed.

I assume you looked at my Twitter client sample. It works, and it's not the easiest authentication method. Lots of APIs use simple credential pairs with API keys and secrets (SendGrid, Twilio, FreshDesk, etc, etc). Forcing people to configure it for each request would be a lot of overhead.

I believe there are three different scenarios:

I just want to be sure that we are discussing the third case.

DevOnBike commented 2 years ago

I think choosing the lifestyle of authenticator should be left to users - should it be per request or singleton or whatever else. Id don't think there is so much overhead. Proper bug free usage is more important for me.

alexeyzimarev commented 2 years ago

The question is where it is, right? There's an option, which is the same as the way ThrowOnAnyError is (was?) done. The client has it as an option, which can't be changed. The request, however, can override it. I believe that might work.

kendallb commented 2 years ago

Yes, I think that is a reasonable option. Allow for it to be configured on the client (as read only) but also allow it to be configured on a per request basis. I can work on a PR for that.

stale[bot] commented 2 years ago

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[bot] commented 2 years ago

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

sultanlive commented 2 years ago

It's not the API issue but more about the fact that the current authenticator implementation is how a lot of people use it, and it hasn't changed.

I assume you looked at my Twitter client sample. It works, and it's not the easiest authentication method. Lots of APIs use simple credential pairs with API keys and secrets (SendGrid, Twilio, FreshDesk, etc, etc). Forcing people to configure it for each request would be a lot of overhead.

I believe there are three different scenarios:

  • App with its own credentials, there's no issue
  • Single-user app working with single credentials pair, no issue again
  • Multi-user app, where each user has their own token. Then, the issue exists, and per-request authentication is a must, as we don't want to have one client instance per user.

I just want to be sure that we are discussing the third case.

Third options is my case, for example request to api with difference Bearer token

kendallb commented 2 years ago

Yep the third case is the primary problem.

stale[bot] commented 2 years ago

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.

alexeyzimarev commented 2 years ago

I only have a suggestion to allow setting a request-scoped authenticator.

Alternatively, we need to get back to the original discussion about the scoped client instance and use HttpClientFactory for that.

kendallb commented 2 years ago

Long term I do think we need to figure out how to do scoped client instances, but I looked at using HttpClientFactory and it's not easy. It would be more fruitful to try to do our own implementation of it to be honest, scoping our own instance per request clients that use a pool of HttpHandlers behind the scenes. But there is a lot of complexity in there.

But in the interim allowing both request and client scoped authenticators is an option. I have to rebase my tree again what is done in develop to find what is currently missing and build a new set of patches.

stale[bot] commented 2 years ago

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[bot] commented 2 years ago

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