fix: check for expired token in any header value #79
Conversation
Dovchik
requested review from
650elx,
krogers0607,
asein-sinch and
JPPortier
as code owners
| @@ -128,7 +128,7 @@ internal class Http : IHttp | |||
| // will not retry when no "expired" header for a token. | |||
| const string wwwAuthenticateHeader = "www-authenticate"; | |||
| if (_auth.Scheme == AuthSchemes.Bearer && result.Headers.Contains(wwwAuthenticateHeader) && | |||
| !result.Headers.GetValues(wwwAuthenticateHeader).Contains("expired")) | |||
| !result.Headers.GetValues(wwwAuthenticateHeader).Any(x => x.Contains("expired"))) | |||
There was a problem hiding this comment.
Looking at the fix, it highlights that our expiration detection mechanism is not robust enough: any change on the backend side for the property error_description will break our SDKs.
For instance, if the message changes for Jwt expiry reached at ..., this won't work anymore
There was a problem hiding this comment.
Can we merge that while we settle on the robust way of doing that?
There was a problem hiding this comment.
This commit fixes the reported bug indeed
tests/Sinch.Tests/Core/HttpTests.cs
Outdated
| [Fact] | ||
| public async Task NewTokenOnlyOnceAfterSuccessfulRefresh() | ||
| { | ||
| _tokenManagerMock.GetAuthToken(Arg.Any<bool>()) |
There was a problem hiding this comment.
A three steps approach would be simpler and more realistic towards a real-life scenario. There is no need to use 3 tokens in the test.
- token1 valid
- token1 expired
- token2 valid
There was a problem hiding this comment.
I have the scenario about. And I want to have that one as well
There was a problem hiding this comment.
I can't find the scenario I describe and you say you already have. Can you point it to me please?
Please bear with me while I'm trying to understand the necessity to make 2 requests in this scenario: when sending the request (op2) on line 111, the auth manager is in the same state as at the beginning of the scenario. Can you explain why you want to keep the scenario like this? What does it cover more than with a single request?
There was a problem hiding this comment.
The scenario checks how the state is cached across two different request to make sure the state is preserved and works as expected.
The 3 token approach you describe is also valid, but it's covered by this case, which is:
- Server was running for some time without request to Sinch API so token expired, got Unauthorized
- First request refreshed token and successful
- Idle, token expired again and second request refreshed it successfully
There was a problem hiding this comment.
Thanks for your explanations. I have some comments though:
- In my first comment I don't describe a 3 tokens approach, but only 2, making sure the first one is valid before it expires
- "Server was running for some time without request to Sinch API" => the token doesn't expire because of inactivity. It expires at a fixed time, which can during a surge of requests.
- "Idle, token expired again" => same remark as above
Then the name of the test NewTokenOnlyOnceAfterSuccessfulRefresh is confusing: it says that the framework will fetch a token only once after a successful refresh, which is not describing what you explain about having a shared state between 2 requests which share the same Http object instance. I would have expected to see a concurrency test instead when several objects share the same resource.
On a side note, the terminology "refresh token" is not correct as the framework doesn't refresh the token but fetches a brand new one by sending again the credentials.
There was a problem hiding this comment.
Commenting test out with the intention to revisit
https://tickets.sinch.com/browse/DEVEXP-491
…nTwoRequests comment out test
tests/Sinch.Tests/Core/HttpTests.cs
Outdated
| @@ -76,6 +76,48 @@ public async Task ForceNewTokenOnlyOnce() | |||
| _httpMessageHandlerMock.VerifyNoOutstandingExpectation(); | |||
| } | |||
|
|
|||
| // [Fact] | |||
There was a problem hiding this comment.
To update in next PR:
- description to clarify test intentions
- name
- use
expiredheader here as well
No description provided.