Add global requestTimeoutInSeconds setting to OidcClientSettings

[dbfr3qs]

Closes #1414

This adds a requestTimeoutInSeconds setting. When this is defined, it applies a retry timeout to all fetch requests made to the authorization server (e.g. /token, /userinfo, /.well-known/openid-configuration) except for requests made with silentRequestTimeoutInSeconds (if it is defined).

So the logic goes

1. requestTimeoutInSeconds is set -> all requests have the same timeout
2. silentRequestTimeoutInSeconds is set -> only silent request have a specific timeout
3. both are set -> all requests have the same timeout except silent requests are set to silentRequestTimeoutInSeconds

Checklist

• [ ] This PR makes changes to the public API
• [x] I have included links for closing relevant issue numbers

[pamapa]

@Badisi Can you have a look at this?

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 87.50000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 80.49%. Comparing base (f0ad76e) to head (e4fe7c6). Report is 11 commits behind head on main.

:exclamation: Current head e4fe7c6 differs from pull request most recent head 2e19329. Consider uploading reports for the commit 2e19329 to get more accurate results

Files	Patch %	Lines
src/UserManager.ts	0.00%	0 Missing and 1 partial :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1430      +/-   ##
==========================================
- Coverage   80.53%   80.49%   -0.04%     
==========================================
  Files          45       45              
  Lines        1731     1733       +2     
  Branches      344      345       +1     
==========================================
+ Hits         1394     1395       +1     
  Misses        299      299              
- Partials       38       39       +1     

Flag	Coverage Δ
unittests	80.49% <87.50%> (-0.04%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[Badisi]

@dbfr3qs, thanks for the PR !

Note that when this is configured, it overrides the silentRequestTimeoutInSeconds used when retrieving refresh tokens

I would go with the opposite: silentRequestTimeoutInSeconds should override requestTimeoutInSeconds if it is also set. Otherwise I wouldn't be able to define a global timeout + a different one for silent request.

Either:

1. requestTimeoutInSeconds is set -> all requests have the same timeout
2. silentRequestTimeoutInSeconds is set -> only silent request have a specific timeout
3. both are set -> all requests have the same timeout except silent request that have a different one

[Badisi]

@Badisi Can you have a look at this?

I'm abroad right now, will be able to look into this in 2 weeks

[dbfr3qs]

@pamapa @Badisi Thankyou both for your excellent feedback. I've made the suggested changes to try and satisfy the rules described above, such that the silentRequestTimeoutInSeconds setting is not overridden by requestTimeoutInSeconds unless it is not defined:

Either:

1. requestTimeoutInSeconds is set -> all requests have the same timeout
2. silentRequestTimeoutInSeconds is set -> only silent request have a specific timeout
3. both are set -> all requests have the same timeout except silent request that have a different one

However, there's a default setting for the silentRequestTimeoutInSeconds setting so the logic will likely not work unless silentRequestTimeoutInSeconds is intentionally set to undefined. Thoughts?

[pamapa]

I would go with the opposite: silentRequestTimeoutInSeconds should override requestTimeoutInSeconds if it is also set. Otherwise I wouldn't be able to define a global timeout + a different one for silent request.

Makes sense! We can handle this also within UserManagerSettingsStore...

[dbfr3qs]

@pamapa @Badisi Thankyou both for your excellent feedback. I've made the suggested changes to try and satisfy the rules described above, such that the silentRequestTimeoutInSeconds setting is not overridden by requestTimeoutInSeconds unless it is not defined:

Either:

1. requestTimeoutInSeconds is set -> all requests have the same timeout
2. silentRequestTimeoutInSeconds is set -> only silent request have a specific timeout
3. both are set -> all requests have the same timeout except silent request that have a different one

However, there's a default setting for the silentRequestTimeoutInSeconds setting so the logic will likely not work unless silentRequestTimeoutInSeconds is intentionally set to undefined. Thoughts?

Any thoughts on this @Badisi @pamapa ?

[pamapa]

However, there's a default setting for the silentRequestTimeoutInSeconds setting so the logic will likely not work unless silentRequestTimeoutInSeconds is intentionally set to undefined. Thoughts?

True, silentRequestTimeoutInSeconds is set by default to 10s. In order to not break the existing behavior we can not leave it undefined.

Probably something like this within UserManagerSettingsStore should work:

 this.requestTimeoutInSeconds = requestTimeoutInSeconds;
 this.silentRequestTimeoutInSeconds = silentRequestTimeoutInSeconds ? silentRequestTimeoutInSeconds
   : requestTimeoutInSeconds ? requestTimeoutInSeconds
   : DefaultSilentRequestTimeoutInSeconds;

[dbfr3qs]

Let me know if this does the trick @pamapa @Badisi 🙏

[pamapa]

Looks good to me. @Badisi What do you think?

[pamapa]

Thanks for contributing and being patient!