Re-factor the AuthCheckFilter / ServletRequestAuthenticator

to enable use of testing.
Clear-out lgeacy jmock files, add in new mockito files and make use
of them to test the changes to the AuthCheckFilter

build/lib/versions.txt

@@ -52,13 +52,14 @@ org.mortbay.jasper.apache-jsp.jar                | 8.0.9.M3         (from Jetty
 jaxen.jar                           | 1.1.6  (from DOM4J 2.0.0)                                    | Apache 1.1
 jdom.jar                            | 1.0 (required by rome)                                       | Apache 1.1
 jmdns.jar                           | PRE 1.0, patched                                             | Apache 2.0
-jmock.jar                           | 2.1.0                                                        |
-jmock-junit4.jar                    | 2.1.0                                                        |
-jmock-legacy.jar                    | 2.1.0                                                        |
 jsmpp                               | 2.2.4                                                        | Apache 2.0
 jtds.jar                            | 1.3.1                                                        | LGPL
 junit.jar                           | 4.11                                                         | EPL 1.0
 hamcrest-core.jar                   | 1.3 (required by junit)                                      | new BSD licence
+mockito-core.2.10.0.jar             | 2.10.0                                                       | MIT (https://github.com/mockito/mockito/wiki/License)
+byte-buddy-1.7.4.jar                | 1.7.4 (required by mockito-core)                             | Apache 2.0 (https://github.com/raphw/byte-buddy/blob/master/LICENSE)
+byte-buddy-agent-1.7.4.jar          | 1.7.4 (required by mockito-core)                             | Apache 2.0 (https://github.com/raphw/byte-buddy/blob/master/LICENSE)
+objenesis-2.6.jar                   | 1.7.4 (required by mockito-core)                             | Apache 2.0 (http://objenesis.org/license.html)
 jzlib.jar                           | 1.0.7                                                        | GPL
 libidn.jar                          | 1.15                                                         | GNU Lesser General Public License version 2.1 or later (http://www.gnu.org/licenses/licenses.html)
 log4j.jar                           | 1.2.17                                                       | Apache 2.0 (http://logging.apache.org/log4j/1.2/license.html)
@@ -71,7 +72,6 @@ mina-integration-ognl.jar           | Apache Mina 2.0.7
 javassist.jar                       | 3.11.0.GA (Apache Mina 2.0.7)                                | Apache 2.0
 ognl.jar                            | 3.0.5 (Apache Mina 2.0.7)                                    | Apache 2.0
 mysql.jar                           | 5.1.42                                                       | GPL
-objenesis                           | 1.0 (JMock 2.1.0)                                            | BSD (http://www.jmock.org/license.html)
 pack200task.jar                     | August 5, 2004                                               | LGPL
 postgres.jar                        | 9.4-1201.jdbc4                                               | BSD (http://jdbc.postgresql.org/license.html)
 proxool.jar                         | 0.9.0RC3+ (see note #1)                                      | Apache 1.1 (http://proxool.sourceforge.net/licence.html)

src/java/org/jivesoftware/admin/AuthCheckFilter.java

@@ -33,6 +33,8 @@ import javax.servlet.ServletResponse;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 
+import org.jivesoftware.openfire.admin.AdminManager;
+import org.jivesoftware.openfire.auth.AuthToken;
 import org.jivesoftware.util.ClassUtils;
 import org.jivesoftware.util.JiveGlobals;
 import org.jivesoftware.util.WebManager;
@@ -46,12 +48,62 @@ import org.slf4j.LoggerFactory;
 public class AuthCheckFilter implements Filter {
 
 	private static final Logger Log = LoggerFactory.getLogger(AuthCheckFilter.class);
+    private static AuthCheckFilter instance;
 
     private static Set<String> excludes = Collections.newSetFromMap(new ConcurrentHashMap<String, Boolean>());
 
+    private final AdminManager adminManager;
+    private final LoginLimitManager loginLimitManager;
+    private final ServletRequestAuthenticator servletRequestAuthenticator;
+
     private ServletContext context;
     private String defaultLoginPage;
-    private ServletRequestAuthenticator servletRequestAuthenticator;
+
+    public AuthCheckFilter() {
+        this(AdminManager.getInstance(), LoginLimitManager.getInstance(), JiveGlobals.getProperty("adminConsole.servlet-request-authenticator", "").trim());
+    }
+
+    /* Exposed for test use only */
+    AuthCheckFilter(final AdminManager adminManager, final LoginLimitManager loginLimitManager, final String servletRequestAuthenticatorClassName) {
+        this.adminManager = adminManager;
+        this.loginLimitManager = loginLimitManager;
+        AuthCheckFilter.instance = this;
+        ServletRequestAuthenticator authenticator = null;
+        if (!servletRequestAuthenticatorClassName.isEmpty()) {
+            try {
+                final Class clazz = ClassUtils.forName(servletRequestAuthenticatorClassName);
+                authenticator = (ServletRequestAuthenticator) clazz.newInstance();
+            } catch (final Exception e) {
+                Log.error("Error loading ServletRequestAuthenticator: " + servletRequestAuthenticatorClassName, e);
+            }
+        }
+        this.servletRequestAuthenticator = authenticator;
+    }
+
+    /**
+     * Returns a singleton instance of the AuthCheckFilter.
+     *
+     * @return an instance.
+     */
+    public static AuthCheckFilter getInstance() {
+        return instance;
+    }
+
+    /**
+     * Indicates if the currently-installed ServletRequestAuthenticator is an instance of a specific class.
+     *
+     * @param clazz the class to check
+     * @return {@code true} if the currently-installed ServletRequestAuthenticator is an instance of clazz, otherwise {@code false}.
+     */
+    public static boolean isServletRequestAuthenticatorInstanceOf(Class<? extends ServletRequestAuthenticator> clazz) {
+        final AuthCheckFilter instance = getInstance();
+        if (instance == null) {
+            // We've not yet been instantiated
+            return false;
+        }
+        final ServletRequestAuthenticator authenticator = instance.servletRequestAuthenticator;
+        return authenticator != null && clazz.isAssignableFrom(authenticator.getClass());
+    }
 
     /**
      * Adds a new string that when present in the requested URL will skip
@@ -124,20 +176,6 @@ public class AuthCheckFilter implements Filter {
                 excludes.add(tok);
             }
         }
-        final String servletRequestAuthenticatorClassName = getServletRequestAuthenticatorClassName();
-        if (!servletRequestAuthenticatorClassName.isEmpty()) {
-            try {
-                final Class clazz = ClassUtils.forName(servletRequestAuthenticatorClassName);
-                servletRequestAuthenticator = (ServletRequestAuthenticator) clazz.newInstance();
-            } catch (final Exception e) {
-                Log.error("Error loading ServletRequestAuthenticator: " + servletRequestAuthenticatorClassName, e);
-                servletRequestAuthenticator = null;
-            }
-        }
-    }
-
-    public static String getServletRequestAuthenticatorClassName() {
-        return JiveGlobals.getProperty("adminConsole.servlet-request-authenticator", "").trim();
     }
 
     @Override
@@ -169,7 +207,7 @@ public class AuthCheckFilter implements Filter {
         if (!doExclude) {
             WebManager manager = new WebManager();
             manager.init(request, response, request.getSession(), context);
-            if (manager.getUser() == null && (servletRequestAuthenticator == null || !servletRequestAuthenticator.authenticateRequest(request))) {
+            if (manager.getUser() == null && !authUserFromRequest(request)) {
                 response.sendRedirect(getRedirectURL(request, loginPage, null));
                 return;
             }
@@ -177,6 +215,30 @@ public class AuthCheckFilter implements Filter {
         chain.doFilter(req, res);
     }
 
+    private boolean authUserFromRequest(final HttpServletRequest request) {
+
+        final String userFromRequest = servletRequestAuthenticator == null ? null : servletRequestAuthenticator.authenticateRequest(request);
+        if (userFromRequest == null) {
+            // The user is not authenticated
+            return false;
+        }
+
+        if (!adminManager.isUserAdmin(userFromRequest, true)) {
+            // The user is not authorised
+            Log.warn("The user '" + userFromRequest + "' is not an Openfire administrator.");
+            return false;
+        }
+
+        // We're authenticated and authorised, so record the login,
+        loginLimitManager.recordSuccessfulAttempt(userFromRequest, request.getRemoteAddr());
+
+        // Set the auth token
+        request.getSession().setAttribute("jive.admin.authToken", new AuthToken(userFromRequest));
+
+        // And proceed
+        return true;
+    }
+
     @Override
     public void destroy() {
     }

src/java/org/jivesoftware/admin/ServletRequestAuthenticator.java

@@ -7,8 +7,8 @@ public interface ServletRequestAuthenticator {
     /**
      * Attempts to authenticate an HTTP request to a page on the admin console.
      * @param request the request to authenticate
-     * @return {@code true} if the request was successfully authenticated, otherwise {@code false}
+     * @return the username if it was possible to determine from the request, otherwise {@code null}
      */
-    boolean authenticateRequest(final HttpServletRequest request);
+    String authenticateRequest(final HttpServletRequest request);
 
 }

src/java/org/jivesoftware/admin/SiteMinderServletRequestAuthenticator.java

 package org.jivesoftware.admin;
 
-import org.jivesoftware.openfire.admin.AdminManager;
-import org.jivesoftware.openfire.auth.AuthToken;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-
 import javax.servlet.http.HttpServletRequest;
 
 /**
@@ -18,41 +13,23 @@ import javax.servlet.http.HttpServletRequest;
  */
 public class SiteMinderServletRequestAuthenticator implements ServletRequestAuthenticator {
 
-    private static final Logger Log = LoggerFactory.getLogger(SiteMinderServletRequestAuthenticator.class);
-
+    /**
+     * Indicates if this ServletRequestAuthenticator is enabled or not
+     *
+     * @return {@code true} if enabled, otherwise {@code false}
+     */
     public static boolean isEnabled() {
-        return SiteMinderServletRequestAuthenticator.class.getName().equals(AuthCheckFilter.getServletRequestAuthenticatorClassName());
+        return AuthCheckFilter.isServletRequestAuthenticatorInstanceOf(SiteMinderServletRequestAuthenticator.class);
     }
 
     @Override
-    public boolean authenticateRequest(final HttpServletRequest request) {
-        final AuthToken authToken = getSiteMinderBasedAuthToken(request);
-        if (authToken != null) {
-            // The user has been authenticated
-            request.getSession().setAttribute("jive.admin.authToken", authToken);
-            return true;
-        } else {
-            // We've not authenticated the user - do nothing
-            return false;
-        }
-    }
-
-    private AuthToken getSiteMinderBasedAuthToken(final HttpServletRequest request) {
+    public String authenticateRequest(final HttpServletRequest request) {
         final String smUser = request.getHeader("SM_USER");
         if (smUser == null || smUser.trim().isEmpty()) {
             // SiteMinder has not authenticated the user
             return null;
+        } else {
+            return smUser;
         }
-
-        if (!AdminManager.getInstance().isUserAdmin(smUser, true)) {
-            // The SiteMinder user is not an admin user
-            Log.warn("SiteMinder user '" + smUser + "' is not an Openfire administrator.");
-            return null;
-        }
-
-        // We've got a valid admin user, so record the login attempt
-        LoginLimitManager.getInstance().recordSuccessfulAttempt(smUser, request.getRemoteAddr());
-        // And return the auth token
-        return new AuthToken(smUser);
     }
 }

src/test/java/org/jivesoftware/admin/AuthCheckFilterTest.java

 package org.jivesoftware.admin;
 
+import org.jivesoftware.openfire.admin.AdminManager;
+import org.jivesoftware.openfire.auth.AuthToken;
+import org.junit.Before;
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
 
+import javax.servlet.FilterChain;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import javax.servlet.http.HttpSession;
+
+import static org.hamcrest.CoreMatchers.is;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
 
+@RunWith(MockitoJUnitRunner.class)
 public class AuthCheckFilterTest {
 
+    private static final String adminUser = "test-admin-user";
+    private static final String normalUser = "test-normal-user";
+    private static final String remoteAddr = "a.b.c.d";
+
+    @Mock private HttpServletRequest request;
+    @Mock private HttpSession httpSession;
+    @Mock private HttpServletResponse response;
+    @Mock private FilterChain filterChain;
+    @Mock private AdminManager adminManager;
+    @Mock private LoginLimitManager loginLimitManager;
+
+    @Before
+    public void setUp() throws Exception {
+
+        doReturn("/uri/to/page").when(request).getRequestURI();
+        doReturn(httpSession).when(request).getSession();
+        doReturn(remoteAddr).when(request).getRemoteAddr();
+
+        doReturn(true).when(adminManager).isUserAdmin(adminUser, true);
+        doReturn(false).when(adminManager).isUserAdmin(normalUser, true);
+
+    }
+
     // login.jsp,index.jsp?logout=true,setup/index.jsp,setup/setup-,.gif,.png,error-serverdown.jsp
 
     @Test
@@ -27,4 +70,104 @@ public class AuthCheckFilterTest {
 
         assertFalse(AuthCheckFilter.testURLPassesExclude("another.jsp?login.jsp", "login.jsp"));
     }
+
+    @Test
+    public void willNotRedirectARequestFromAnAdminUser() throws Exception {
+
+        final AuthCheckFilter filter = new AuthCheckFilter(adminManager, loginLimitManager, AdminUserServletAuthenticatorClass.class.getName());
+
+        filter.doFilter(request, response, filterChain);
+
+        verify(response, never()).sendRedirect(anyString());
+        verify(loginLimitManager).recordSuccessfulAttempt(adminUser, remoteAddr);
+        final ArgumentCaptor<AuthToken> argumentCaptor = ArgumentCaptor.forClass(AuthToken.class);
+        verify(httpSession).setAttribute(eq("jive.admin.authToken"), argumentCaptor.capture());
+        final AuthToken authToken = argumentCaptor.getValue();
+        assertThat(authToken.getUsername(), is(adminUser));
+    }
+
+    @Test
+    public void willRedirectARequestWithoutAServletRequestAuthenticator() throws Exception {
+
+        final AuthCheckFilter filter = new AuthCheckFilter(adminManager, loginLimitManager, "");
+
+        filter.doFilter(request, response, filterChain);
+
+        verify(response).sendRedirect(anyString());
+    }
+
+    @Test
+    public void willRedirectARequestWithABrokenServletRequestAuthenticator() throws Exception {
+
+        final AuthCheckFilter filter = new AuthCheckFilter(adminManager, loginLimitManager, "this-is-not-a-class-name");
+
+        filter.doFilter(request, response, filterChain);
+
+        verify(response).sendRedirect(anyString());
+    }
+
+    @Test
+    public void willRedirectARequestIfTheServletRequestAuthenticatorReturnsNoUser() throws Exception {
+
+        final AuthCheckFilter filter = new AuthCheckFilter(adminManager, loginLimitManager, NoUserServletAuthenticatorClass.class.getName());
+
+        filter.doFilter(request, response, filterChain);
+
+        verify(response).sendRedirect(anyString());
+    }
+
+    @Test
+    public void willRedirectARequestIfTheServletRequestAuthenticatorReturnsAnUnauthorisedUser() throws Exception {
+
+        final AuthCheckFilter filter = new AuthCheckFilter(adminManager, loginLimitManager, NormalUserServletAuthenticatorClass.class.getName());
+
+        filter.doFilter(request, response, filterChain);
+
+        verify(response).sendRedirect(anyString());
+    }
+
+    @Test
+    public void willReturnTrueIfTheCorrectServletRequestAuthenticatorIsConfigured() throws Exception {
+
+        new AuthCheckFilter(adminManager, loginLimitManager, NormalUserServletAuthenticatorClass.class.getName());
+
+        assertThat(AuthCheckFilter.isServletRequestAuthenticatorInstanceOf(NormalUserServletAuthenticatorClass.class), is(true));
+    }
+
+    @Test
+    public void willReturnFalseIfTheWrongServletRequestAuthenticatorIsConfigured() throws Exception {
+
+        new AuthCheckFilter(adminManager, loginLimitManager, NormalUserServletAuthenticatorClass.class.getName());
+
+        assertThat(AuthCheckFilter.isServletRequestAuthenticatorInstanceOf(AdminUserServletAuthenticatorClass.class), is(false));
+    }
+
+    @Test
+    public void willReturnFalseIfNoServletRequestAuthenticatorIsConfigured() throws Exception {
+
+        new AuthCheckFilter(adminManager, loginLimitManager, "");
+
+        assertThat(AuthCheckFilter.isServletRequestAuthenticatorInstanceOf(AdminUserServletAuthenticatorClass.class), is(false));
+    }
+
+    public static class AdminUserServletAuthenticatorClass implements ServletRequestAuthenticator {
+        @Override
+        public String authenticateRequest(final HttpServletRequest request) {
+            return adminUser;
+        }
+    }
+
+    public static class NormalUserServletAuthenticatorClass implements ServletRequestAuthenticator {
+        @Override
+        public String authenticateRequest(final HttpServletRequest request) {
+            return normalUser;
+        }
+    }
+
+    public static class NoUserServletAuthenticatorClass implements ServletRequestAuthenticator {
+        @Override
+        public String authenticateRequest(final HttpServletRequest request) {
+            return null;
+        }
+    }
 }