From 3d2d36d11519b5bafdfbcd73ac213a5c009e39f2 Mon Sep 17 00:00:00 2001 From: Shinsuke Sugaya Date: Sat, 22 Aug 2026 15:29:58 +0900 Subject: [PATCH] fix(sso): bound the Entra ID parent group walk when Microsoft Graph stops answering updateMemberOf walks every direct group id and calls processParentGroup for each one, and a lookup that fails is collected rather than short-circuited, so the walk always ran to completion. When Graph answers /me/memberOf but then fails on POST /groups/{id}/getMemberGroups with something that records no backoff -- a 500/502/504, or a transport failure such as DNS, connection refused, or the 10s connect / 30s read timeouts -- that costs one request, one waited-out timeout and one stack trace per direct group, on every login. A 429 or a 503 does record a tenant-wide backoff, so those are already skipped without reaching Graph; nothing else is. The walk runs on corelib's shared TimeoutManager pool: availableProcessors()/2 threads, a LinkedBlockingQueue of the same size and CallerRunsPolicy. Once that overflows, the walk runs on the TimeoutManager timer thread itself and stalls every other Fess timed task. Since the resolution moved wholly into the background the direct membership lookup shares that pool too. The walk now stops after maxConsecutiveGroupLookupFailures (default 3) consecutive lookups Graph did not answer, logs one WARN naming how many direct groups were left unwalked, and still applies everything collected so far. Consecutive rather than total is deliberate: one permanently broken group id must not stop the rest of the walk, while an unreachable Graph trips the bound immediately. A lookup skipped for the tenant-wide backoff does not consume the bound -- it never reached Graph, it costs nothing, and counting it would end the walk and log the WARN on every login for as long as the throttle lasts. maxConsecutiveGroupLookupFailures is listed in the commented block of fess_sso++.xml alongside maxGroupDepth. --- .../sso/entraid/EntraIdAuthenticator.java | 54 ++++++++- src/main/resources/fess_sso++.xml | 1 + .../sso/entraid/EntraIdAuthenticatorTest.java | 108 ++++++++++++++++++ 3 files changed, 160 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/codelibs/fess/sso/entraid/EntraIdAuthenticator.java b/src/main/java/org/codelibs/fess/sso/entraid/EntraIdAuthenticator.java index a2a7b7224..4d1f00d6a 100644 --- a/src/main/java/org/codelibs/fess/sso/entraid/EntraIdAuthenticator.java +++ b/src/main/java/org/codelibs/fess/sso/entraid/EntraIdAuthenticator.java @@ -310,6 +310,24 @@ protected static Map> maskParams(final MapThe walk costs one {@code POST /groups/{id}/getMemberGroups} per direct group. A 429 or a + * 503 records a tenant-wide backoff, so the rest of that walk is skipped without reaching + * Graph at all; a 500/502/504 or a transport failure -- DNS, connection refused, or the + * {@link #graphConnectTimeout} / {@link #graphReadTimeout} expiring -- records nothing, so + * without this bound every direct group costs a full request and a stack trace, on every + * login, each waiting out the timeouts. That runs on corelib's shared {@code TimeoutManager} + * pool, whose {@code CallerRunsPolicy} pushes the overflow onto the timer thread itself. + * + *

Consecutive rather than total is deliberate: one permanently broken group id must not + * stop the rest of the walk, while a Graph that has stopped answering trips the bound + * immediately. + */ + protected int maxConsecutiveGroupLookupFailures = 3; + /** * Connection timeout for Microsoft Graph requests in milliseconds. curl4j leaves this unset, * which means an unbounded wait, and the direct-membership lookup runs on the login thread. @@ -1055,11 +1073,33 @@ public void updateMemberOf(final EntraIdUser user) { } // Every direct group is still walked after one of them fails: a partial parent set is worth - // more than none, so the failures are collected rather than short-circuited. + // more than none, so the failures are collected rather than short-circuited. What does end + // the walk early is maxConsecutiveGroupLookupFailures answers in a row that Graph did not + // give -- past that point the tenant is unreachable rather than one group being broken, + // and continuing only buys one request, one timeout and one stack trace per remaining + // group. Whatever was collected before that is still applied below. boolean parentsResolved = true; + int walkedCount = 0; + int consecutiveFailures = 0; for (final String groupId : groupIdsForParentLookup) { - if (!processParentGroup(user, groupList, roleList, groupId)) { - parentsResolved = false; + ++walkedCount; + if (processParentGroup(user, groupList, roleList, groupId)) { + consecutiveFailures = 0; + continue; + } + parentsResolved = false; + if (isGraphThrottled()) { + // A lookup skipped for the backoff never reached Graph: it costs nothing, and the + // backoff already bounds the tenant. Counting it would end the walk -- and log the + // WARN below -- on every login for as long as the throttle lasts, for no saving. + continue; + } + if (++consecutiveFailures >= maxConsecutiveGroupLookupFailures) { + logger.warn( + "Stopped resolving the nested groups of {} after {} consecutive Microsoft Graph failures." + + " {} of {} direct groups were not walked.", + user.getName(), consecutiveFailures, groupIdsForParentLookup.size() - walkedCount, groupIdsForParentLookup.size()); + break; } } @@ -2001,6 +2041,14 @@ public void setMaxGroupDepth(final int maxGroupDepth) { this.maxGroupDepth = maxGroupDepth; } + /** + * Sets how many consecutive unanswered parent group lookups end the walk. + * @param maxConsecutiveGroupLookupFailures The maximum number of consecutive failures. + */ + public void setMaxConsecutiveGroupLookupFailures(final int maxConsecutiveGroupLookupFailures) { + this.maxConsecutiveGroupLookupFailures = maxConsecutiveGroupLookupFailures; + } + @Override public String logout(final FessUserBean user) { // The client application is shared for the whole server so that its token cache survives diff --git a/src/main/resources/fess_sso++.xml b/src/main/resources/fess_sso++.xml index 0286bff1c..0c79071f3 100644 --- a/src/main/resources/fess_sso++.xml +++ b/src/main/resources/fess_sso++.xml @@ -11,6 +11,7 @@ 600 10000 10 + 3 10000 --> diff --git a/src/test/java/org/codelibs/fess/sso/entraid/EntraIdAuthenticatorTest.java b/src/test/java/org/codelibs/fess/sso/entraid/EntraIdAuthenticatorTest.java index 4f707ed58..20953548b 100644 --- a/src/test/java/org/codelibs/fess/sso/entraid/EntraIdAuthenticatorTest.java +++ b/src/test/java/org/codelibs/fess/sso/entraid/EntraIdAuthenticatorTest.java @@ -2911,4 +2911,112 @@ public void test_getAuthUrl_separatesASlashlessAuthorityFromTheTenant() { fessConfig.setSystemProperty("entraid.reply.url", ""); } } + + /** + * An authenticator whose direct membership lookup hands back a fixed set of group ids without + * reaching Microsoft Graph, and whose parent group walk is scripted by {@code walkResult}. + * Every id the walk is asked for is appended to {@code walked}, so a test can tell how far the + * walk got as well as what it produced. + */ + private EntraIdAuthenticator newAuthenticatorWithScriptedWalk(final List groupIds, final List walked, + final java.util.function.Predicate walkResult, final boolean throttled) { + return new EntraIdAuthenticator() { + @Override + protected boolean processDirectMemberOf(final EntraIdUser user, final List groupList, final List roleList, + final List groupIdsForParentLookup, final String url) { + groupIdsForParentLookup.addAll(groupIds); + groupList.add("direct-group"); + return true; + } + + @Override + protected boolean processParentGroup(final EntraIdUser user, final List groupList, final List roleList, + final String id) { + walked.add(id); + if (walkResult.test(id)) { + groupList.add("parent-of-" + id); + return true; + } + return false; + } + + @Override + protected boolean isGraphThrottled() { + return throttled; + } + }; + } + + @Test + public void test_updateMemberOf_stopsTheWalkAfterConsecutiveGraphFailures() { + // Graph answers /me/memberOf and then fails every getMemberGroups with something that + // records no backoff -- a 500/502/504, or a transport failure such as DNS, connection + // refused or the graphConnectTimeout/graphReadTimeout expiring. Without the bound that is + // one request, one waited-out timeout and one stack trace per direct group, on every + // login, on the shared TimeoutManager pool. + final List walked = new ArrayList<>(); + final EntraIdAuthenticator authenticator = + newAuthenticatorWithScriptedWalk(List.of("g1", "g2", "g3", "g4", "g5", "g6"), walked, id -> false, false); + final EntraIdUser user = newUserWithoutGraph(); + final int before = permissionChangedCount.get(); + + authenticator.updateMemberOf(user); + + assertEquals(3, walked.size(), "the walk has to stop at maxConsecutiveGroupLookupFailures"); + assertEquals(List.of("g1", "g2", "g3"), walked); + // Still applied, and still announced: a partial parent set is worth more than none, and + // FAILED is what tells the user their permissions fell short. + assertTrue(List.of(user.getGroupNames()).contains("direct-group")); + assertEquals(FessUser.PermissionState.FAILED, user.getPermissionState()); + assertEquals(before + 1, permissionChangedCount.get()); + } + + @Test + public void test_updateMemberOf_letsASuccessResetTheFailureCounter() { + // Consecutive, not total: one group id that is permanently broken -- deleted, or one the + // application has no permission for -- must not stop the rest of the walk. + final List walked = new ArrayList<>(); + final EntraIdAuthenticator authenticator = newAuthenticatorWithScriptedWalk(List.of("g1", "g2", "g3", "g4", "g5", "g6"), walked, + id -> "g3".equals(id) || "g6".equals(id), false); + final EntraIdUser user = newUserWithoutGraph(); + + authenticator.updateMemberOf(user); + + assertEquals(6, walked.size(), "a success between failures has to clear the counter"); + assertTrue(List.of(user.getGroupNames()).contains("parent-of-g3")); + assertTrue(List.of(user.getGroupNames()).contains("parent-of-g6")); + // Some parents were still missed, so the user is not fully resolved. + assertEquals(FessUser.PermissionState.FAILED, user.getPermissionState()); + } + + @Test + public void test_updateMemberOf_doesNotCountAThrottledSkipTowardsTheBound() { + // A lookup skipped for the tenant-wide backoff never reaches Graph, so it costs nothing + // and the backoff already bounds it. Counting it would end the walk -- and log the WARN -- + // on every login for as long as the throttle lasts, buying nothing. + final List walked = new ArrayList<>(); + final EntraIdAuthenticator authenticator = + newAuthenticatorWithScriptedWalk(List.of("g1", "g2", "g3", "g4", "g5", "g6"), walked, id -> false, true); + final EntraIdUser user = newUserWithoutGraph(); + + authenticator.updateMemberOf(user); + + assertEquals(6, walked.size(), "a throttled skip must not consume the bound"); + assertEquals(FessUser.PermissionState.FAILED, user.getPermissionState()); + } + + @Test + public void test_updateMemberOf_honoursAConfiguredFailureBound() { + // The bound is a fess_sso++.xml property, so an operator can widen it for a tenant whose + // groups genuinely fail one by one, or narrow it to 1 to give up at the first failure. + final List walked = new ArrayList<>(); + final EntraIdAuthenticator authenticator = + newAuthenticatorWithScriptedWalk(List.of("g1", "g2", "g3", "g4", "g5", "g6"), walked, id -> false, false); + authenticator.setMaxConsecutiveGroupLookupFailures(1); + final EntraIdUser user = newUserWithoutGraph(); + + authenticator.updateMemberOf(user); + + assertEquals(1, walked.size()); + } }