OIDC: allow configuring the request timeout #1177
Conversation
fflorent
force-pushed
the
oidc-idp-timeout
branch
from
624034d to
a35baf8
Compare
fflorent
force-pushed
the
oidc-idp-timeout
branch
from
a35baf8 to
4c30043
Compare
fflorent
force-pushed
the
oidc-idp-timeout
branch
from
4c30043 to
a3593c7
Compare
| @@ -173,6 +178,9 @@ export class OIDCConfig { | |||
| this._protectionManager = new ProtectionsManager(enabledProtections); | |||
|
|
|||
| this._redirectUrl = new URL(CALLBACK_URL, spHost).href; | |||
| custom.setHttpOptionsDefaults({ | |||
| ...(httpTimeout !== undefined ? {timeout: httpTimeout} : {}), | |||
There was a problem hiding this comment.
Why not more simply:
if (httpTimeout !== undefined) {
custom.setHttpOptionsDefaults({timeout: httpTimeout});
}
There was a problem hiding this comment.
Hi @dsagal!
For slightly convenient reasons:
- I don't have to care whether the function should have been called or not in the test, it should if no exception has been raised: https://github.com/gristlabs/grist-core/pull/1177/files#diff-0467f3ff93b79307ee0beb1d72eea81866d60efa1efcfb8937d7154117e895f4R238
- I think of passing later more options to fix OIDC issuer behind a proxy cannot be accessed #942
If these are bad reasons to you, I may apply the change you request, I don't have much strong opinions.
There was a problem hiding this comment.
Simplifying testing is a fine reason for me.
BTW, it looks like setHttpOptionsDefaults only pays attention to whether a property isn't undefined (rather than rely on hasOwnProperty or similar), so even this should work correctly: custom.setHttpOptionsDefaults({timeout: httpTimeout}) (without any checks). But I guess it's hard to test if this works, since the test only checks how setHttpOptionsDefaults is called, not its effect. Don't know if there is a better way: your call.
There was a problem hiding this comment.
Yes, I agree with what you tell (we miss integration tests for this module 😬).
I prefer not taking any assumption (for now and the future) and keep this part as it is.
|
Thanks @dsagal for your review! I don't have the rights to merge. If there is no other change you expect from me, could you do that for me if that's OK for you? |
Add IdP timeout, controlled by env var GRIST_OIDC_SP_HTTP_TIMEOUT --------- Co-authored-by: atropos <sv7n@pm.me>
Context
I take up this PR: #1072 (open by @atropos112)
In some case, the identity provider may take time before responding to requests sent by the OIDC client, leading to difficulties or even impossibility to start the server or to login.
For example, if the issuer discovery takes time, you may fail to start the server with this error:
Proposed solution
Introduce the
GRIST_OIDC_SP_HTTP_TIMEOUTenv variable so the user may set a greater value than the default 3500ms, or even set it to 0 to remove any timeout.How to test it
I use this utility to add latency to the requests, so the openid-client requests may timeout: https://github.com/sitespeedio/throttle
Prerequisites:
npm i -g @sitespeed.io/throttletc. On Debian-family Linux distros, you must install iproute2 for that:sudo apt install iproute2.STR:
throttle --localhost --up 9000 --down 9000 --rtt 5000(the RTT is what matters here)yarn start, the server should fail to start with a timeout in the logs (see theContextsection above)GRIST_OIDC_SP_HTTP_TIMEOUT=30000 yarn startto check that the problem is solvedthrottle --localhost stopRelated issues
I also pave the way for #942
Has this been tested?