fix(sso): stop bouncing an unmatched SAML response back to the IdP

src/main/java/org/codelibs/fess/sso/saml/SamlAuthenticator.java

@@ -133,6 +133,16 @@
  * saml.security.want_assertions_signed=true
  * </pre>
  *
+ * <h2>Session Cookie Settings (Required)</h2>
+ * <p>The IdP returns the assertion as a cross-site POST to the assertion consumer service.
+ * A {@code SameSite=Lax} cookie is not sent on such a request, so the shipped default in
+ * {@code tomcat_config.properties} has to be changed for SAML:</p>
+ * <pre>
+ * tomcat.sameSiteCookies = none
+ * </pre>
+ * <p>{@code none} is only accepted by browsers on a {@code Secure} cookie, so Fess must be
+ * served over HTTPS.</p>
+ *
  * @see <a href="https://fess.codelibs.org/">Fess Documentation</a>
  */
 public class SamlAuthenticator implements SsoAuthenticator {
@@ -311,9 +321,9 @@ public LoginCredential getLoginCredential() {
 
             final HttpServletResponse response = LaResponseUtil.getResponse();
 
-            final HttpSession session = request.getSession(false);
-            if (session != null) {
-                final String requestId = (String) session.getAttribute(SAML_STATE);
+            if (containsSamlResponse(request)) {
+                final HttpSession session = request.getSession(false);
+                final String requestId = session == null ? null : (String) session.getAttribute(SAML_STATE);
                 if (StringUtil.isNotBlank(requestId)) {
                     session.removeAttribute(SAML_STATE);
                     try {
@@ -336,6 +346,14 @@ public LoginCredential getLoginCredential() {
                         return null;
                     }
                 }
+                // The assertion arrived but the matching AuthnRequest ID is unreachable. Sending
+                // another AuthnRequest would come straight back here in the same state, looping
+                // forever, so fail once instead.
+                logger.warn("Received a SAML response with no matching AuthnRequest ID in the session."
+                        + " The assertion consumer service is a cross-site POST, which does not carry a SameSite=Lax cookie;"
+                        + " see tomcat.sameSiteCookies in tomcat_config.properties."
+                        + " An IdP-initiated (unsolicited) response is rejected for the same reason.");
+                return null;
             }
 
             try {
@@ -351,6 +369,25 @@ public LoginCredential getLoginCredential() {
         }).orElse(null);
     }
 
+    /**
+     * Returns whether the request carries a SAML response, which is what the IdP posts to the
+     * assertion consumer service.
+     *
+     * <p>The session is deliberately not consulted here: it is the session cookie that goes
+     * missing when the browser refuses to send it on the cross-site POST, and a callback that is
+     * mistaken for a fresh visit is redirected back to the IdP forever.</p>
+     *
+     * <p>Only solicited responses are accepted. Fess binds every response to the ID of the
+     * AuthnRequest it sent, so an unsolicited (IdP-initiated) response has nothing to match
+     * against and is rejected rather than answered with a fresh AuthnRequest.</p>
+     *
+     * @param request The HTTP request.
+     * @return true if the request carries a SAML response.
+     */
+    protected boolean containsSamlResponse(final HttpServletRequest request) {
+        return StringUtil.isNotBlank(request.getParameter("SAMLResponse"));
+    }
+
     /**
      * Creates a login credential.
      * @param request The HTTP request.

src/test/java/org/codelibs/fess/sso/saml/SamlAuthenticatorTest.java

@@ -30,12 +30,15 @@
 
 import org.codelibs.core.lang.StringUtil;
 import org.codelibs.core.misc.DynamicProperties;
+import org.codelibs.fess.app.web.base.login.ActionResponseCredential;
 import org.codelibs.fess.exception.SsoMessageException;
 import org.codelibs.fess.sso.SsoResponseType;
 import org.codelibs.fess.unit.UnitFessTestCase;
 import org.codelibs.fess.util.ComponentUtil;
 import org.codelibs.saml2.core.settings.Saml2Settings;
+import org.dbflute.utflute.mocklet.MockletHttpServletRequest;
 import org.junit.jupiter.api.Test;
+import org.lastaflute.web.login.credential.LoginCredential;
 
 public class SamlAuthenticatorTest extends UnitFessTestCase {
 
@@ -195,6 +198,96 @@ public void test_getLogoutResponse_withoutIdpSingleLogoutServiceUrl() throws Exc
         }
     }
 
+    // ===================================================================================
+    //                                                             Assertion Consumer Service
+    //                                                             ==========================
+
+    /** Minimal IdP settings, so that an AuthnRequest can actually be built. */
+    private void setUpIdp(final DynamicProperties systemProperties) {
+        systemProperties.setProperty("saml.idp.entityid", "https://idp.example.com/metadata");
+        systemProperties.setProperty("saml.idp.single_sign_on_service.url", "https://idp.example.com/sso");
+        systemProperties.setProperty("saml.idp.certfingerprint", "afe71c28ef740bc87425be13a2263d37971da1f9");
+    }
+
+    private void tearDownIdp(final DynamicProperties systemProperties) {
+        systemProperties.remove("saml.idp.entityid");
+        systemProperties.remove("saml.idp.single_sign_on_service.url");
+        systemProperties.remove("saml.idp.certfingerprint");
+    }
+
+    @Test
+    public void test_containsSamlResponse() throws Exception {
+        final SamlAuthenticator authenticator = new SamlAuthenticator();
+
+        assertFalse(authenticator.containsSamlResponse(getMockRequest()));
+
+        final MockletHttpServletRequest blank = getMockRequest();
+        blank.setParameter("SAMLResponse", "  ");
+        assertFalse(authenticator.containsSamlResponse(blank));
+
+        final MockletHttpServletRequest posted = getMockRequest();
+        posted.setParameter("SAMLResponse", "PHNhbWxwOlJlc3BvbnNlIC8+");
+        assertTrue(authenticator.containsSamlResponse(posted));
+    }
+
+    @Test
+    public void test_getLoginCredential_unmatchedResponseFailsInsteadOfRedirecting() throws Exception {
+        final SamlAuthenticator authenticator = createAuthenticator();
+        final DynamicProperties systemProperties = ComponentUtil.getSystemProperties();
+        final LogCapturingAppender appender = LogCapturingAppender.attach(SamlAuthenticator.class);
+        try {
+            setUpIdp(systemProperties);
+            // the IdP posts the assertion cross-site, so a SameSite=Lax cookie is not sent back
+            // and the session holding the AuthnRequest ID is unreachable
+            final MockletHttpServletRequest request = getMockRequest();
+            request.setMethod("POST");
+            request.setParameter("SAMLResponse", "PHNhbWxwOlJlc3BvbnNlIC8+");
+
+            // redirecting to the IdP again would come straight back in the same state
+            assertNull(authenticator.getLoginCredential());
+            assertEquals(1, appender.warnings().size());
+            assertTrue(appender.warnings().get(0), appender.warnings().get(0).contains("no matching AuthnRequest ID"));
+        } finally {
+            tearDownIdp(systemProperties);
+            appender.detach();
+        }
+    }
+
+    @Test
+    public void test_getLoginCredential_requestWithoutResponseStartsLogin() throws Exception {
+        final SamlAuthenticator authenticator = createAuthenticator();
+        final DynamicProperties systemProperties = ComponentUtil.getSystemProperties();
+        try {
+            setUpIdp(systemProperties);
+            final MockletHttpServletRequest request = getMockRequest();
+
+            final LoginCredential credential = authenticator.getLoginCredential();
+
+            assertTrue(String.valueOf(credential), credential instanceof ActionResponseCredential);
+            assertNotNull(request.getSession(false).getAttribute("SAML_STATE"));
+        } finally {
+            tearDownIdp(systemProperties);
+        }
+    }
+
+    @Test
+    public void test_getLoginCredential_requestWithoutResponseKeepsPendingRequestId() throws Exception {
+        final SamlAuthenticator authenticator = createAuthenticator();
+        final DynamicProperties systemProperties = ComponentUtil.getSystemProperties();
+        try {
+            setUpIdp(systemProperties);
+            // a plain visit to /sso/ while a login is in flight must not consume the pending ID
+            final MockletHttpServletRequest request = getMockRequest();
+            request.getSession().setAttribute("SAML_STATE", "ONELOGIN_pending");
+
+            authenticator.getLoginCredential();
+
+            assertNotNull(request.getSession(false).getAttribute("SAML_STATE"));
+        } finally {
+            tearDownIdp(systemProperties);
+        }
+    }
+
     @Test
     public void test_buildDefaultUrl_withDefaultBaseUrl() throws Exception {
         assertEquals("http://localhost:8080/sso/metadata", new SamlAuthenticator().buildDefaultUrl("/sso/metadata"));