fix(sso): bound Microsoft Graph calls from the Entra ID authenticator - #3221
Open
marevol wants to merge 1 commit into
Open
fix(sso): bound Microsoft Graph calls from the Entra ID authenticator#3221marevol wants to merge 1 commit into
marevol wants to merge 1 commit into
Conversation
None of the four Graph calls set a timeout. curl4j's CurlRequest defaults both connectTimeout and readTimeout to -1 and only calls setConnectTimeout / setReadTimeout when the value is >= 0, so every call waited forever, and Fess sets no JVM-wide default either. processDirectMemberOf() runs synchronously on the login request thread, so an unresponsive Graph endpoint blocked that thread indefinitely. The other three run on the TimeoutManager pool, whose queue is only as deep as its thread count and whose saturation policy is CallerRunsPolicy, so a backlog of unbounded calls ends up executing on the single scheduler thread. Add graphConnectTimeout (10s) and graphReadTimeout (30s) with setters, and route all four call sites through a createGraphRequest() helper that applies them along with the Authorization and Accept headers those sites each repeated.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
None of the four Microsoft Graph calls in
EntraIdAuthenticatorset a timeout.curl4j's
CurlRequestdefaults both to-1:so
setConnectTimeout/setReadTimeoutare never called, which forHttpURLConnectionmeans wait forever. Fess sets no JVM-wide default(
sun.net.client.defaultReadTimeoutetc.) either.Where that hurts:
processDirectMemberOf()runs synchronously on the login request thread (called from theEntraIdUserconstructor). An unresponsive Graph endpoint blocks that thread indefinitely.TimeoutManagerpool. That pool isThreadPoolExecutor(nThreads, nThreads, ..., new LinkedBlockingQueue<>(nThreads), new CallerRunsPolicy())with
nThreads = availableProcessors() / 2(min 1), so once the queue fills, work runs on thecaller — the single
CoreLib-TimeoutManagerscheduler thread.Fix
graphConnectTimeout(10s) andgraphReadTimeout(30s), with setters so they can be tunedthrough LastaDi like the other knobs on this component.
createGraphRequest(CurlRequest, String accessToken)helper that applies the timeouts plusthe
AuthorizationandAcceptheaders, which all four sites repeated verbatim.Only timeouts and the header de-duplication — no change to what is requested or how responses are
parsed.
Tests
EntraIdAuthenticator's unit test class, 2 new tests:test_graphTimeouts_haveBoundedDefaults— guards against a non-positive default silentlyrestoring the unbounded behaviour
test_createGraphRequest_stopsWaitingOnAnUnresponsiveEndpoint— binds a loopbackServerSocketthat accepts and never answers, sets the read timeout to 300ms, and asserts the call gives up
well under the 30s default
Falsification: with
setGraphReadTimeoutstubbed to ignore its argument, the second test failswith
took 30014msinstead of passing — so it distinguishes "timeout applied" from "default used"as well as from "no timeout at all".
(the whole
org.codelibs.fess.ssopackage)