From 9c138effbc908114f5326a7fd7e7c06c62ca9af3 Mon Sep 17 00:00:00 2001
From: Nicolas Frati <nicofrati@gmail.com>
Date: Wed, 8 Apr 2026 10:01:35 +0200
Subject: [PATCH] fix: mfa not enforced on session validation (#4726)

Signed-off-by: jnfrati <nicofrati@gmail.com>
---
 server/session.go      | 16 +++++++++
 server/session_test.go | 76 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 92 insertions(+)

diff --git a/server/session.go b/server/session.go
index 437f74a6..e4753b47 100644
--- a/server/session.go
+++ b/server/session.go
@@ -469,6 +469,22 @@ func (s *Server) finishSessionLogin(ctx context.Context, r *http.Request, w http
 		return old, nil
 	})
 
+	// Check if the client requires MFA.
+	mfaChain, err := s.mfaChainForClient(ctx, authReq.ClientID, session.ConnectorID)
+	if err != nil {
+		s.logger.ErrorContext(ctx, "session: failed to get MFA chain", "err", err)
+		return "", false
+	}
+	if len(mfaChain) > 0 {
+		// Re-read auth request to get the updated state (LoggedIn, Claims, ConnectorID).
+		updated, err := s.storage.GetAuthRequest(ctx, authReq.ID)
+		if err != nil {
+			s.logger.ErrorContext(ctx, "session: failed to get auth request", "err", err)
+			return "", false
+		}
+		return s.buildMFARedirectURL(updated, mfaChain[0]), true
+	}
+
 	// Skip approval if globally configured or user already consented to the requested scopes.
 	if !authReq.ForceApprovalPrompt && (s.skipApproval || scopesCoveredByConsent(ui.Consents[authReq.ClientID], authReq.Scopes)) {
 		// Re-read to get the updated AuthRequest (LoggedIn, Claims, ConnectorID set above).
diff --git a/server/session_test.go b/server/session_test.go
index 06e91948..21400b44 100644
--- a/server/session_test.go
+++ b/server/session_test.go
@@ -1355,3 +1355,79 @@ func TestTrySessionLogin_SSO(t *testing.T) {
 		assert.False(t, ok, "SSO login should fail when client does not share")
 	})
 }
+
+func TestFinishSessionLogin_MFA(t *testing.T) {
+	ctx := t.Context()
+
+	setupMFAFixture := func(t *testing.T, mfaProviders map[string]MFAProvider, clientMFAChain []string) (*Server, storage.AuthRequest) {
+		t.Helper()
+		s := newTestSessionServer(t)
+		s.skipApproval = true
+		s.mfaProviders = mfaProviders
+
+		// Create connector in storage and register it in the connectors map.
+		require.NoError(t, s.storage.CreateConnector(ctx, storage.Connector{
+			ID:              "mock",
+			Type:            "ldap",
+			Name:            "Mock LDAP",
+			ResourceVersion: "1",
+		}))
+		s.mu.Lock()
+		s.connectors = map[string]Connector{
+			"mock": {Type: "ldap", ResourceVersion: "1"},
+		}
+		s.mu.Unlock()
+
+		// Create client with MFA chain.
+		require.NoError(t, s.storage.CreateClient(ctx, storage.Client{
+			ID:       "client-1",
+			Secret:   "secret",
+			Name:     "Test Client",
+			MFAChain: clientMFAChain,
+		}))
+
+		authReq := setupSessionLoginFixture(t, s)
+		return s, authReq
+	}
+
+	t.Run("MFA required redirects to MFA page", func(t *testing.T) {
+		s, authReq := setupMFAFixture(t, map[string]MFAProvider{
+			"totp": NewTOTPProvider("test-issuer", nil), // nil connectorTypes = enabled for all
+		}, []string{"totp"})
+
+		r := sessionCookieRequest("user-1", "mock", "test-nonce")
+		w := httptest.NewRecorder()
+
+		redirectURL, ok := s.trySessionLogin(ctx, r, w, &authReq)
+		require.True(t, ok)
+		assert.Contains(t, redirectURL, "/mfa/totp", "should redirect to MFA page")
+		assert.Contains(t, redirectURL, "req="+authReq.ID, "redirect should include auth request ID")
+		assert.Contains(t, redirectURL, "authenticator=totp", "redirect should include authenticator ID")
+
+		// MFAValidated should NOT be set.
+		updated, err := s.storage.GetAuthRequest(ctx, authReq.ID)
+		require.NoError(t, err)
+		assert.False(t, updated.MFAValidated, "MFAValidated should be false when MFA is required")
+		// LoggedIn should still be set even though MFA is pending.
+		assert.True(t, updated.LoggedIn, "LoggedIn should be true even when MFA is pending")
+	})
+
+	t.Run("MFA provider not enabled for connector type skips MFA", func(t *testing.T) {
+		// TOTP provider only enabled for "oidc" connectors, but our connector is "ldap".
+		s, authReq := setupMFAFixture(t, map[string]MFAProvider{
+			"totp": NewTOTPProvider("test-issuer", []string{"oidc"}),
+		}, []string{"totp"})
+		require.NoError(t, s.storage.UpdateAuthRequest(ctx, authReq.ID, func(a storage.AuthRequest) (storage.AuthRequest, error) {
+			a.ForceApprovalPrompt = true
+			return a, nil
+		}))
+		authReq.ForceApprovalPrompt = true
+
+		r := sessionCookieRequest("user-1", "mock", "test-nonce")
+		w := httptest.NewRecorder()
+
+		redirectURL, ok := s.trySessionLogin(ctx, r, w, &authReq)
+		require.True(t, ok)
+		assert.Contains(t, redirectURL, "/approval")
+	})
+}