Skip to content

fix(sso): stop swallowing Entra ID login failures at DEBUG level - #3218

Merged
marevol merged 1 commit into
masterfrom
entraid-surface-login-errors
Aug 10, 2026
Merged

fix(sso): stop swallowing Entra ID login failures at DEBUG level#3218
marevol merged 1 commit into
masterfrom
entraid-surface-login-errors

Conversation

@marevol

@marevol marevol commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Stacked on #3215. Base will retarget to master once that merges — review only the last commit.

Problem

EntraIdAuthenticator.getLoginCredential() caught every exception, logged it only at DEBUG,
and returned null:

} catch (final Exception e) {
    if (logger.isDebugEnabled()) {
        logger.debug("Failed to process a login request on Entra ID.", e);
    }
}
return null;

SsoAction.index() already has the right handler one frame up:

} catch (final SsoLoginException e) {
    if (ssoManager.available()) {
        logger.warn("Failed to process SSO login.", e);
        saveError(messages -> messages.addErrorsSsoLoginError(GLOBAL));
    }
    ...
}

but the inner catch meant it never ran for Entra ID. OpenIdConnectAuthenticator does not
swallow, so it gets the WARN — Entra ID was the outlier.

The practical effect: a state mismatch, a nonce mismatch, a failed token acquisition and a
Microsoft Graph outage all produce the same silent redirect to the login page, with nothing above
DEBUG to tell them apart.

Fix

Rethrow SsoLoginException unchanged, and wrap anything else in one, so both reach SsoAction.

The user-visible outcome does not change — the SSO error message, then the login page. Wrapping
rather than letting the raw exception propagate matters because the Graph client throws
CurlException, a RuntimeException, which SsoAction does not catch and which would otherwise
surface as a 500 error page.

Tests

EntraIdAuthenticator's unit test class, 2 new tests, both failing before the change:

  • test_getLoginCredential_surfacesUnexpectedFailures — an IllegalStateException from
    processAuthenticationData() comes back as an SsoLoginException wrapping it
  • test_getLoginCredential_propagatesSsoLoginExceptionUnwrapped — an SsoLoginException is
    rethrown as the same instance, not double-wrapped
Tests run: 140, Failures: 0, Errors: 0, Skipped: 0

(the whole org.codelibs.fess.sso package plus SsoActionTest)

Base automatically changed from entraid-response-mode-query to master August 10, 2026 01:15
getLoginCredential() caught every exception from processAuthenticationData(),
logged it only when DEBUG was enabled, and returned null. SsoAction already has
a handler that logs SsoLoginException at WARN and shows the SSO error message --
which is what the OpenID Connect authenticator relies on -- but the inner catch
meant it never ran for Entra ID.

The result was that a state mismatch, a nonce mismatch, a failed token
acquisition and a Microsoft Graph outage all produced the same silent redirect
to the login page, with nothing above DEBUG to tell them apart.

Rethrow SsoLoginException as-is and wrap anything else in one, so both reach
SsoAction. The user-visible outcome is unchanged: the SSO error message and a
redirect to the login page. Wrapping rather than propagating the raw exception
keeps a RuntimeException from the Graph client, such as CurlException, from
escaping as a 500 error page.
@marevol
marevol force-pushed the entraid-surface-login-errors branch from c53bb39 to f3a3e84 Compare August 10, 2026 01:19
@marevol marevol self-assigned this Aug 10, 2026
@marevol marevol added this to the 15.8.0 milestone Aug 10, 2026
@marevol
marevol merged commit b981c7d into master Aug 10, 2026
2 checks passed
@marevol
marevol deleted the entraid-surface-login-errors branch August 11, 2026 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant