Skip to content

using JwtAuthenticator in v107 #1767

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

Closed
DevOnBike opened this issue Feb 28, 2022 · 18 comments
Closed

using JwtAuthenticator in v107 #1767

DevOnBike opened this issue Feb 28, 2022 · 18 comments

Comments

@DevOnBike
Copy link

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
Copy link
Member

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

@kendallb
Copy link
Contributor

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:

#1786

That would solve this problem.

@alexeyzimarev
Copy link
Member

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Member

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.

@DevOnBike
Copy link
Author

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
Copy link
Member

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
Copy link
Contributor

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
Copy link

stale bot commented Apr 16, 2022

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.

@stale stale bot added the wontfix label Apr 16, 2022
@repo-ranger
Copy link

repo-ranger bot commented Apr 16, 2022

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

@sultanlive
Copy link

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
Copy link
Contributor

Yep the third case is the primary problem.

@stale
Copy link

stale bot commented Jun 13, 2022

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
Copy link
Member

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
Copy link
Contributor

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
Copy link

stale bot commented Aug 13, 2022

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.

@stale stale bot added the wontfix label Aug 13, 2022
@repo-ranger
Copy link

repo-ranger bot commented Aug 13, 2022

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

@repo-ranger repo-ranger bot closed this as completed Aug 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants