fix(sso): bound the Entra ID parent group walk when Microsoft Graph stops answering

src/main/java/org/codelibs/fess/sso/entraid/EntraIdAuthenticator.java

@@ -310,6 +310,24 @@ protected static Map<String, List<String>> maskParams(final Map<String, List<Str
     /** Maximum depth for processing nested groups to prevent infinite loops. */
     protected int maxGroupDepth = 10;
 
+    /**
+     * How many consecutive parent group lookups Microsoft Graph may fail to answer before
+     * {@link #updateMemberOf} stops walking the rest of the user's direct groups.
+     *
+     * <p>The 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.
+     *
+     * <p>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

src/main/resources/fess_sso++.xml

@@ -11,6 +11,7 @@
 		<property name="groupCacheExpiry">600</property>
 		<property name="maxGroupCacheSize">10000</property>
 		<property name="maxGroupDepth">10</property>
+		<property name="maxConsecutiveGroupLookupFailures">3</property>
 		<property name="maxCachedAccounts">10000</property>
 		-->
 	</component>

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<String> groupIds, final List<String> walked,
+            final java.util.function.Predicate<String> walkResult, final boolean throttled) {
+        return new EntraIdAuthenticator() {
+            @Override
+            protected boolean processDirectMemberOf(final EntraIdUser user, final List<String> groupList, final List<String> roleList,
+                    final List<String> groupIdsForParentLookup, final String url) {
+                groupIdsForParentLookup.addAll(groupIds);
+                groupList.add("direct-group");
+                return true;
+            }
+
+            @Override
+            protected boolean processParentGroup(final EntraIdUser user, final List<String> groupList, final List<String> 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<String> 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<String> 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<String> 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<String> 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());
+    }
 }