fixed duplicate filter logic, fix session fixation

src/main/java/io/jenkins/plugins/MfaEnforceFilter.java

@@ -1,80 +0,0 @@
-/*
- * Project: MFA Google Auth Plugin
- *
- * Class: MfaEnforceFilter
- *
- * This servlet filter enforces multi-factor authentication (MFA) for Jenkins users.
- * It intercepts HTTP requests and redirects users with MFA enabled but not yet verified
- * to the MFA verification page before allowing access to other Jenkins pages.
- * Static resources and the verification page itself are excluded from this enforcement.
- *
- * Author: Allan Barcelos
- * Date: 2025-07-17
- */
-
-package io.jenkins.plugins;
-
-import hudson.Extension;
-import hudson.model.User;
-import hudson.util.PluginServletFilter;
-import java.io.IOException;
-import javax.servlet.*;
-import javax.servlet.http.HttpServletRequest;
-import javax.servlet.http.HttpServletResponse;
-import javax.servlet.http.HttpSession;
-
-@Extension
-public class MfaEnforceFilter implements javax.servlet.Filter {
-
-    static {
-        try {
-            PluginServletFilter.addFilter(new MfaEnforceFilter());
-        } catch (ServletException e) {
-            throw new RuntimeException(e);
-        }
-    }
-
-    @Override
-    public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain)
-            throws IOException, ServletException {
-
-        if (request instanceof HttpServletRequest && response instanceof HttpServletResponse) {
-            HttpServletRequest req = (HttpServletRequest) request;
-            HttpServletResponse rsp = (HttpServletResponse) response;
-
-            String path = req.getRequestURI();
-
-            // Skip static resources
-            if (path.startsWith(req.getContextPath() + "/static/")
-                    || path.startsWith(req.getContextPath() + "/adjuncts/")) {
-                chain.doFilter(request, response);
-                return;
-            }
-
-            User current = User.current();
-
-            if (current != null) {
-                HttpSession session = req.getSession(false);
-                boolean verified = session != null && Boolean.TRUE.equals(session.getAttribute("mfa-verified"));
-
-                MfaUserProperty mfa = current.getProperty(MfaUserProperty.class);
-                boolean mfaEnabled = mfa != null && mfa.isMfaEnabled();
-
-                // String path = req.getRequestURI();
-
-                if (mfaEnabled && !verified && !path.contains("/mfa-verify")) {
-                    rsp.sendRedirect(req.getContextPath() + "/mfa-verify/");
-                    return;
-                }
-            }
-        }
-
-        chain.doFilter(request, response);
-    }
-
-    @Override
-    public void init(javax.servlet.FilterConfig filterConfig) {}
-
-    @Override
-    public void destroy() {}
-}

src/main/java/io/jenkins/plugins/MfaFilter.java

@@ -3,12 +3,13 @@
  *
  * Class: MfaFilter
  *
- * This servlet filter blocks access to Jenkins pages for users who have MFA enabled
- * but have not yet completed MFA verification in the current session.
- * It allows free access to MFA verification and login URLs.
+ * Unified servlet filter that enforces multi-factor authentication (MFA) for Jenkins users.
+ * It intercepts HTTP requests and redirects users with MFA enabled but not yet verified
+ * to the MFA verification page. Exclusions include static resources, login pages, and
+ * the verification page itself.
  *
  * Author: Allan Barcelos
- * Date: 2025-07-17
+ * Date: 2025-07-17 (Updated for unified implementation)
  */
 
 package io.jenkins.plugins;
@@ -21,20 +22,17 @@ import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 import jenkins.model.Jenkins;
 
-/**
- * Filtro que bloqueia o acesso se o usuário não tiver passado pela verificação MFA.
- */
 @Extension
 public class MfaFilter implements Filter {
 
     @Override
     public void init(FilterConfig filterConfig) {
-        // nada a inicializar
+        // Initialization not needed
     }
 
     @Override
     public void destroy() {
-        // nada a destruir
+        // Cleanup not needed
     }
 
     @Override
@@ -48,30 +46,30 @@ public class MfaFilter implements Filter {
 
         HttpServletRequest req = (HttpServletRequest) request;
         HttpServletResponse rsp = (HttpServletResponse) response;
+        String path = req.getRequestURI();
+        String contextPath = req.getContextPath();
 
-        // Garante que Jenkins já está inicializado
+        // Skip if Jenkins isn't fully initialized
         if (Jenkins.getInstanceOrNull() == null) {
             chain.doFilter(request, response);
             return;
         }
 
-        User u = User.current();
-        if (u != null) {
-            MfaUserProperty mfa = u.getProperty(MfaUserProperty.class);
-            if (mfa != null && mfa.isMfaEnabled()) {
-                Object verified = req.getSession().getAttribute("mfa-verified");
-                String path = req.getRequestURI();
-                String ctx = req.getContextPath();
+        // Skip static resources and common excluded paths
+        if (isExcludedPath(path, contextPath)) {
+            chain.doFilter(request, response);
+            return;
+        }
 
-                // Permitir acesso às próprias URLs de verificação MFA e login
-                if (path.startsWith(ctx + "/mfa-verify") || path.startsWith(ctx + "/login")) {
-                    chain.doFilter(request, response);
-                    return;
-                }
+        User user = User.current();
+        if (user != null) {
+            MfaUserProperty mfa = user.getProperty(MfaUserProperty.class);
+            if (mfa != null && mfa.isMfaEnabled()) {
+                boolean verified = req.getSession() != null
+                        && Boolean.TRUE.equals(req.getSession().getAttribute("mfa-verified"));
 
-                // Se não verificado, redirecionar
-                if (verified == null) {
-                    rsp.sendRedirect(ctx + "/mfa-verify");
+                if (!verified) {
+                    rsp.sendRedirect(contextPath + "/mfa-verify");
                     return;
                 }
             }
@@ -79,4 +77,18 @@ public class MfaFilter implements Filter {
 
         chain.doFilter(request, response);
     }
+
+    /**
+     * Determines if the requested path should be excluded from MFA enforcement
+     */
+    private boolean isExcludedPath(String path, String contextPath) {
+        // List of paths that don't require MFA verification
+        return path.startsWith(contextPath + "/static/")
+                || path.startsWith(contextPath + "/adjuncts/")
+                || path.startsWith(contextPath + "/mfa-verify")
+                || path.startsWith(contextPath + "/login")
+                || path.startsWith(contextPath + "/signup")
+                || path.startsWith(contextPath + "/error")
+                || path.startsWith(contextPath + "/securityRealm");
+    }
 }

src/main/java/io/jenkins/plugins/MfaVerifyAction.java

@@ -64,28 +64,23 @@ public class MfaVerifyAction implements RootAction {
         if (mfa != null && mfa.isMfaEnabled()) {
             String code = req.getParameter("totpCode");
             if (TOTPUtil.verifyCode(mfa.getSecretKey(), code)) {
-                HttpSession session;
-                try {
-                    // Session Fixation protection
-                    session = req.getSession();
-                    session.invalidate(); // invalidate current session
-                    session = req.getSession(true); // create new session
-                } catch (IllegalStateException e) {
-                    LOGGER.log(Level.WARNING, "Error during session regeneration", e);
-                    session = req.getSession(true); // fallback - just get/create session
-                }
-
-                // Set verification flag on the NEW session
+                // Proteção simplificada - apenas marca como verificado
+                HttpSession session = req.getSession();
                 session.setAttribute("mfa-verified", true);
+                
+                // Alternativa: Rotaciona o ID da sessão sem invalidá-la
+                session = req.getSession(true);
+                
+                LOGGER.log(Level.INFO, "MFA verification successful for user: " + u.getId());
                 rsp.sendRedirect(req.getContextPath() + "/");
                 return;
             } else {
-                rsp.sendRedirect("mfa-verify?error=1");
+                LOGGER.log(Level.WARNING, "Failed MFA attempt for user: " + u.getId());
+                rsp.sendRedirect(req.getContextPath() + "/mfa-verify?error=1");
                 return;
             }
         }
 
-        // Without MFA, redirect
         rsp.sendRedirect(req.getContextPath() + "/");
     }
 }

src/test/java/io/jenkins/plugins/MfaEnforceFilterTest.java

@@ -1,111 +0,0 @@
-package io.jenkins.plugins;
-
-import static org.mockito.Mockito.*;
-
-import hudson.model.User;
-import javax.servlet.FilterChain;
-import javax.servlet.FilterConfig;
-import javax.servlet.http.HttpServletRequest;
-import javax.servlet.http.HttpServletResponse;
-import javax.servlet.http.HttpSession;
-import org.junit.Before;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.mockito.Mock;
-import org.mockito.MockedStatic;
-import org.mockito.junit.MockitoJUnitRunner;
-
-@RunWith(MockitoJUnitRunner.class)
-public class MfaEnforceFilterTest {
-
-    @Mock
-    private HttpServletRequest request;
-
-    @Mock
-    private HttpServletResponse response;
-
-    @Mock
-    private FilterChain chain;
-
-    @Mock
-    private HttpSession session;
-
-    @Mock
-    private User user;
-
-    @Mock
-    private MfaUserProperty mfaProperty;
-
-    @Mock
-    private FilterConfig filterConfig;
-
-    private MfaEnforceFilter filter;
-
-    @Before
-    public void setUp() {
-        filter = new MfaEnforceFilter();
-    }
-
-    @Test
-    public void testStaticResourcesBypass() throws Exception {
-        when(request.getRequestURI()).thenReturn("/jenkins/static/some-resource.css");
-        when(request.getContextPath()).thenReturn("/jenkins");
-
-        filter.doFilter(request, response, chain);
-
-        verify(chain).doFilter(request, response);
-        verifyNoInteractions(response);
-    }
-
-    @Test
-    public void testNoUserLoggedIn() throws Exception {
-        when(request.getRequestURI()).thenReturn("/jenkins/some-page");
-        when(request.getContextPath()).thenReturn("/jenkins");
-
-        try (MockedStatic<User> mockedUser = mockStatic(User.class)) {
-            mockedUser.when(User::current).thenReturn(null);
-
-            filter.doFilter(request, response, chain);
-
-            verify(chain).doFilter(request, response);
-            verifyNoInteractions(response);
-        }
-    }
-
-    @Test
-    public void testUserWithMfaDisabled() throws Exception {
-        when(request.getRequestURI()).thenReturn("/jenkins/some-page");
-        when(request.getContextPath()).thenReturn("/jenkins");
-
-        try (MockedStatic<User> mockedUser = mockStatic(User.class)) {
-            mockedUser.when(User::current).thenReturn(user);
-            when(user.getProperty(MfaUserProperty.class)).thenReturn(null);
-
-            filter.doFilter(request, response, chain);
-
-            verify(chain).doFilter(request, response);
-            verifyNoInteractions(response);
-        }
-    }
-
-    @Test
-    public void testUserWithMfaEnabledButNotVerified() throws Exception {
-        when(request.getRequestURI()).thenReturn("/jenkins/some-page");
-        when(request.getContextPath()).thenReturn("/jenkins");
-        when(request.getSession(false)).thenReturn(session);
-
-        try (MockedStatic<User> mockedUser = mockStatic(User.class)) {
-            mockedUser.when(User::current).thenReturn(user);
-            when(user.getProperty(MfaUserProperty.class)).thenReturn(mfaProperty);
-            when(mfaProperty.isMfaEnabled()).thenReturn(true);
-            when(session.getAttribute("mfa-verified")).thenReturn(null);
-
-            filter.doFilter(request, response, chain);
-
-            verify(response).sendRedirect("/jenkins/mfa-verify/");
-            verify(chain, never()).doFilter(request, response);
-        }
-    }
-
-    // maybe more tests ...
-}

src/test/java/io/jenkins/plugins/MfaFilterTest.java

@@ -51,13 +51,14 @@ public class MfaFilterTest {
     public void setUp() {
         filter = new MfaFilter();
         when(request.getSession()).thenReturn(session);
+        when(request.getContextPath()).thenReturn("/jenkins");
+        when(request.getRequestURI()).thenReturn("/jenkins/some-path");
     }
 
     @Test
     public void testInitAndDestroy() throws Exception {
         filter.init(filterConfig);
         filter.destroy();
-        // Apenas verifica que não lança exceções
     }
 
     @Test
@@ -74,6 +75,7 @@ public class MfaFilterTest {
     public void testJenkinsNotInitializedPassesThrough() throws Exception {
         try (MockedStatic<Jenkins> mockedJenkins = mockStatic(Jenkins.class)) {
             mockedJenkins.when(Jenkins::getInstanceOrNull).thenReturn(null);
+            when(request.getRequestURI()).thenReturn("/jenkins/some-path");
 
             filter.doFilter(request, response, chain);
 
@@ -88,6 +90,7 @@ public class MfaFilterTest {
 
             mockedJenkins.when(Jenkins::getInstanceOrNull).thenReturn(jenkins);
             mockedUser.when(User::current).thenReturn(null);
+            when(request.getRequestURI()).thenReturn("/jenkins/some-path");
 
             filter.doFilter(request, response, chain);
 
@@ -103,6 +106,7 @@ public class MfaFilterTest {
             mockedJenkins.when(Jenkins::getInstanceOrNull).thenReturn(jenkins);
             mockedUser.when(User::current).thenReturn(user);
             when(user.getProperty(MfaUserProperty.class)).thenReturn(null);
+            when(request.getRequestURI()).thenReturn("/jenkins/some-path");
 
             filter.doFilter(request, response, chain);
 
@@ -120,7 +124,6 @@ public class MfaFilterTest {
             when(user.getProperty(MfaUserProperty.class)).thenReturn(mfaProperty);
             when(mfaProperty.isMfaEnabled()).thenReturn(true);
             when(request.getRequestURI()).thenReturn("/jenkins/restricted");
-            when(request.getContextPath()).thenReturn("/jenkins");
 
             filter.doFilter(request, response, chain);
 
@@ -129,6 +132,56 @@ public class MfaFilterTest {
         }
     }
 
-    // maybe more tests ...
+    @Test
+    public void testMfaEnabledAndVerifiedPassesThrough() throws Exception {
+        try (MockedStatic<Jenkins> mockedJenkins = mockStatic(Jenkins.class);
+                MockedStatic<User> mockedUser = mockStatic(User.class)) {
+
+            mockedJenkins.when(Jenkins::getInstanceOrNull).thenReturn(jenkins);
+            mockedUser.when(User::current).thenReturn(user);
+            when(user.getProperty(MfaUserProperty.class)).thenReturn(mfaProperty);
+            when(mfaProperty.isMfaEnabled()).thenReturn(true);
+            when(session.getAttribute("mfa-verified")).thenReturn(true);
+            when(request.getRequestURI()).thenReturn("/jenkins/restricted");
+
+            filter.doFilter(request, response, chain);
+
+            verify(chain).doFilter(request, response);
+            verify(response, never()).sendRedirect(anyString());
+        }
+    }
 
+    @Test
+    public void testExcludedPathsPassThrough() throws Exception {
+        String[] excludedPaths = {
+            "/jenkins/static/resource.css",
+            "/jenkins/adjuncts/script.js",
+            "/jenkins/mfa-verify",
+            "/jenkins/login",
+            "/jenkins/signup",
+            "/jenkins/error",
+            "/jenkins/securityRealm"
+        };
+
+        for (String path : excludedPaths) {
+            when(request.getRequestURI()).thenReturn(path);
+            filter.doFilter(request, response, chain);
+        }
+
+        verify(chain, times(excludedPaths.length)).doFilter(request, response);
+    }
+
+    @Test
+    public void testStaticResourcesWithDifferentContextPath() throws Exception {
+        when(request.getContextPath()).thenReturn("/custom-context");
+        when(request.getRequestURI()).thenReturn("/custom-context/static/resource.css");
+
+        try (MockedStatic<Jenkins> mockedJenkins = mockStatic(Jenkins.class)) {
+            mockedJenkins.when(Jenkins::getInstanceOrNull).thenReturn(jenkins);
+
+            filter.doFilter(request, response, chain);
+
+            verify(chain).doFilter(request, response);
+        }
+    }
 }