REPO-1687: ACE-5662: The AuthenticationServiceImpl.ProtectedUser is not suitable for clustered cache

- Changed the implementation to use Serializable objects in cache.


git-svn-id: https://svn.alfresco.com/repos/alfresco-enterprise/alfresco/BRANCHES/DEV/5.2.N/root@133415 c4b6b30b-aa2e-2d43-bbcb-ca4b014f7261

source/java/org/alfresco/repo/security/authentication/AuthenticationServiceImpl.java

@@ -49,6 +49,7 @@ public class AuthenticationServiceImpl extends AbstractAuthenticationService imp
     private boolean allowsUserPasswordChange = true;
 
     private static final String AUTHENTICATION_UNSUCCESSFUL = "Authentication was not successful.";
+    private static final String BRUTE_FORCE_ATTACK_DETECTED = "Brute force attack was detected for user: %s";
     private int protectionPeriodSeconds;
     private boolean protectionEnabled;
     private int protectionLimit;
@@ -110,13 +111,9 @@ public class AuthenticationServiceImpl extends AbstractAuthenticationService imp
             // clear context - to avoid MT concurrency issue (causing domain mismatch) - see also 'validate' below
             clearCurrentSecurityContext();
             preAuthenticationCheck(userName);
-            if (protectionEnabled)
+            if (isUserProtected(userName))
             {
-                ProtectedUser protectedUser = protectedUsersCache.get(userName);
-                if (protectedUser != null && protectedUser.isProtected())
-                {
-                    throw new AuthenticationException(AUTHENTICATION_UNSUCCESSFUL);
-                }
+                throw new AuthenticationException(AUTHENTICATION_UNSUCCESSFUL);
             }
             authenticationComponent.authenticate(userName, password);
             if (tenant == null)
@@ -136,25 +133,60 @@ public class AuthenticationServiceImpl extends AbstractAuthenticationService imp
         catch(AuthenticationException ae)
         {
             clearCurrentSecurityContext();
-            if (protectionEnabled)
-            {
-                ProtectedUser protectedUser = protectedUsersCache.get(userName);
-                if (protectedUser == null)
-                {
-                    protectedUser = new ProtectedUser(userName);
-                }
-                else
-                {
-                    protectedUser.recordUnsuccessfulLogin();
-                }
-                protectedUsersCache.put(userName, protectedUser);
-            }
+            recordFailedAuthentication(userName);
             throw ae;
         }
         ticketComponent.clearCurrentTicket();
         getCurrentTicket();
     }
 
+    /**
+     * @return <code>true</code> if user is 'protected' from brute force attack
+     */
+    public boolean isUserProtected(String userName)
+    {
+        boolean isProtected = false;
+        if (protectionEnabled)
+        {
+            ProtectedUser protectedUser = protectedUsersCache.get(userName);
+            if (protectedUser != null)
+            {
+                long currentTimeStamp = System.currentTimeMillis();
+                isProtected = protectedUser.getNumLogins() >= protectionLimit &&
+                        currentTimeStamp - protectedUser.getTimeStamp() < protectionPeriodSeconds * 1000;
+            }
+        }
+        return isProtected;
+    }
+
+    /**
+     * Method records a failed login attempt.
+     * If the number of recorded failures exceeds {@link AuthenticationServiceImpl#protectionLimit}
+     * the user will be considered 'protected'.
+     */
+    public void recordFailedAuthentication(String userName)
+    {
+        if (protectionEnabled)
+        {
+            ProtectedUser protectedUser = protectedUsersCache.get(userName);
+            if (protectedUser == null)
+            {
+                protectedUser = new ProtectedUser(userName);
+            }
+            else
+            {
+                protectedUser = new ProtectedUser(userName, protectedUser.getNumLogins() + 1);
+                if (protectedUser.getNumLogins() == protectionLimit && logger.isWarnEnabled())
+                {
+                    // Shows only first 2 symbols of the username and masks all other character with '*'
+                    logger.warn(String.format(BRUTE_FORCE_ATTACK_DETECTED,
+                            userName.substring(0,2) + new String(new char[(userName.length() - 2)]).replace("\0", "*")));
+                }
+            }
+            protectedUsersCache.put(userName, protectedUser);
+        }
+    }
+
     public String getCurrentUserName() throws AuthenticationException
     {
         return authenticationComponent.getCurrentUserName();
@@ -409,74 +441,4 @@ public class AuthenticationServiceImpl extends AbstractAuthenticationService imp
 
         return true;
     }
-
-    /**
-     * An object to hold information about unsuccessful logins
-     */
-    /*package*/ class ProtectedUser
-    {
-        private String userId;
-        /** number of consecutive unsuccessful login attempts */
-        private long numLogins;
-        /** time stamp of last unsuccessful login attempt */
-        private long timeStamp;
-
-        public ProtectedUser(String userId)
-        {
-            this.userId = userId;
-            this.numLogins = 1;
-            this.timeStamp = System.currentTimeMillis();
-        }
-
-        public void recordUnsuccessfulLogin()
-        {
-            this.numLogins+=1;
-            this.timeStamp = System.currentTimeMillis();
-            if (numLogins == protectionLimit + 1 && logger.isWarnEnabled())
-            {
-                // Shows only first 2 symbols of the username and masks all other character with '*'
-                logger.warn("Brute force attack was detected for user " +
-                        userId.substring(0,2) + new String(new char[(userId.length() - 2)]).replace("\0", "*"));
-            }
-        }
-
-        public boolean isProtected()
-        {
-            long currentTimeStamp = System.currentTimeMillis();
-            return numLogins >= protectionLimit &&
-                    currentTimeStamp - timeStamp < protectionPeriodSeconds * 1000;
-        }
-
-        @Override
-        public String toString()
-        {
-            return "ProtectedUser{" +
-                    "userId='" + userId + '\'' +
-                    ", numLogins=" + numLogins +
-                    ", timeStamp=" + timeStamp +
-                    '}';
-        }
-
-        @Override
-        public boolean equals(Object o)
-        {
-            if (this == o) return true;
-            if (o == null || getClass() != o.getClass()) return false;
-
-            ProtectedUser that = (ProtectedUser) o;
-
-            if (numLogins != that.numLogins) return false;
-            if (timeStamp != that.timeStamp) return false;
-            return userId.equals(that.userId);
-        }
-
-        @Override
-        public int hashCode()
-        {
-            int result = userId.hashCode();
-            result = 31 * result + (int) (numLogins ^ (numLogins >>> 32));
-            result = 31 * result + (int) (timeStamp ^ (timeStamp >>> 32));
-            return result;
-        }
-    }
 }

source/java/org/alfresco/repo/security/authentication/ProtectedUser.java

@@ -0,0 +1,102 @@
+/*
+ * #%L
+ * Alfresco Repository
+ * %%
+ * Copyright (C) 2005 - 2016 Alfresco Software Limited
+ * %%
+ * This file is part of the Alfresco software. 
+ * If the software was purchased under a paid Alfresco license, the terms of 
+ * the paid license agreement will prevail.  Otherwise, the software is 
+ * provided under the following open source license terms:
+ * 
+ * Alfresco is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ * 
+ * Alfresco is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ * 
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with Alfresco. If not, see <http://www.gnu.org/licenses/>.
+ * #L%
+ */
+package org.alfresco.repo.security.authentication;
+
+import java.io.Serializable;
+
+/**
+ * An object to hold information about unsuccessful logins.
+ * It is used for brute force attack mitigation in {@link AuthenticationServiceImpl}
+ *
+ * @since 5.2.0
+ * @author amukha
+ */
+/*package*/ class ProtectedUser implements Serializable
+{
+    private static final long serialVersionUID = 1L;
+
+    private String userId;
+    /** number of consecutive unsuccessful login attempts */
+    private long numLogins;
+    /** time stamp of last unsuccessful login attempt */
+    private long timeStamp;
+
+    /*package*/ ProtectedUser(String userId)
+    {
+        this.userId = userId;
+        this.numLogins = 1;
+        this.timeStamp = System.currentTimeMillis();
+    }
+
+    /*package*/ ProtectedUser(String userId, long numLogins)
+    {
+        this.userId = userId;
+        this.numLogins = numLogins;
+        this.timeStamp = System.currentTimeMillis();
+    }
+
+    /*package*/ long getNumLogins()
+    {
+        return numLogins;
+    }
+
+    /*package*/ long getTimeStamp()
+    {
+        return timeStamp;
+    }
+
+    @Override
+    public String toString()
+    {
+        return "ProtectedUser{" +
+                "userId='" + userId + '\'' +
+                ", numLogins=" + numLogins +
+                ", timeStamp=" + timeStamp +
+                '}';
+    }
+
+    @Override
+    public boolean equals(Object o)
+    {
+        if (this == o) return true;
+        if (o == null || getClass() != o.getClass()) return false;
+
+        ProtectedUser that = (ProtectedUser) o;
+
+        if (numLogins != that.numLogins) return false;
+        if (timeStamp != that.timeStamp) return false;
+        return userId.equals(that.userId);
+    }
+
+    @Override
+    public int hashCode()
+    {
+        int result = userId.hashCode();
+        result = 31 * result + (int) (numLogins ^ (numLogins >>> 32));
+        result = 31 * result + (int) (timeStamp ^ (timeStamp >>> 32));
+        return result;
+    }
+}

source/test-java/org/alfresco/repo/security/authentication/AuthenticationServiceImplTest.java

@@ -34,6 +34,7 @@ import java.util.Collection;
 import java.util.HashMap;
 import java.util.Map;
 
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
@@ -53,7 +54,7 @@ import static org.mockito.Mockito.verify;
 public class AuthenticationServiceImplTest
 {
     private AuthenticationComponent authenticationComponent = mock(AuthenticationComponent.class);
-    private SimpleCache<String, AuthenticationServiceImpl.ProtectedUser> cache;
+    private SimpleCache<String, ProtectedUser> cache;
     private TicketComponent ticketComponent = mock(TicketComponent.class);
     private AuthenticationServiceImpl authService;
 
@@ -73,14 +74,15 @@ public class AuthenticationServiceImplTest
     @Test
     public void testProtectedUserBadPassword()
     {
-        int attempts = 3;
+        int limit = 3;
+        int attempts = limit + 3;
         authService.setProtectionPeriodSeconds(99999);
-        authService.setProtectionLimit(attempts);
+        authService.setProtectionLimit(limit);
         authService.setProtectionEnabled(true);
 
         Exception spoofedAE = new AuthenticationException("Bad password");
         doThrow(spoofedAE).when(authenticationComponent).authenticate(USERNAME, PASSWORD);
-        for (int i = 0; i < attempts + 3; i++)
+        for (int i = 0; i < attempts; i++)
         {
             try
             {
@@ -90,7 +92,7 @@ public class AuthenticationServiceImplTest
             catch (AuthenticationException ae)
             {
                 // normal
-                if (i < attempts)
+                if (i < limit)
                 {
                     assertTrue("Expected failure from AuthenticationComponent", ae == spoofedAE);
                 }
@@ -100,8 +102,9 @@ public class AuthenticationServiceImplTest
                 }
             }
         }
-        verify(authenticationComponent, times(attempts)).authenticate(USERNAME, PASSWORD);
-        assertTrue("The user should be protected.", cache.get(USERNAME).isProtected());
+        verify(authenticationComponent, times(limit)).authenticate(USERNAME, PASSWORD);
+        assertTrue("The user should be protected.", authService.isUserProtected(USERNAME));
+        assertEquals("The number of recorded logins did not match.", attempts, cache.get(USERNAME).getNumLogins());
 
         // test that the protection is still in place even if the password is correct
         doNothing().when(authenticationComponent).authenticate(USERNAME, PASSWORD);
@@ -114,20 +117,22 @@ public class AuthenticationServiceImplTest
         {
             // normal
         }
-        verify(authenticationComponent, times(attempts)).authenticate(USERNAME, PASSWORD);
+        verify(authenticationComponent, times(limit)).authenticate(USERNAME, PASSWORD);
+        assertEquals("The number of recorded logins did not match.", attempts + 1, cache.get(USERNAME).getNumLogins());
     }
 
     @Test
     public void testProtectedUserCanLoginAfterProtection() throws Exception
     {
         int timeLimit = 1;
+        int attempts = 2;
         authService.setProtectionPeriodSeconds(timeLimit);
-        authService.setProtectionLimit(2);
+        authService.setProtectionLimit(attempts);
         authService.setProtectionEnabled(true);
 
         doThrow(new AuthenticationException("Bad password"))
                 .when(authenticationComponent).authenticate(USERNAME, PASSWORD);
-        for (int i = 0; i < 2; i++)
+        for (int i = 0; i < attempts; i++)
         {
             try
             {
@@ -139,9 +144,12 @@ public class AuthenticationServiceImplTest
                 // normal
             }
         }
-        assertTrue("The user should be protected.", cache.get(USERNAME).isProtected());
+        assertTrue("The user should be protected.", authService.isUserProtected(USERNAME));
+        assertEquals("The number of recorded logins did not match.", attempts, cache.get(USERNAME).getNumLogins());
         Thread.sleep(timeLimit*1000 + 1);
-        assertFalse("The user should not be protected any more.", cache.get(USERNAME).isProtected());
+        assertFalse("The user should not be protected any more.", authService.isUserProtected(USERNAME));
+        assertEquals("The number of recorded logins should stay the same after protection period ends.",
+                attempts, cache.get(USERNAME).getNumLogins());
 
         doNothing().when(authenticationComponent).authenticate(USERNAME, PASSWORD);
         try
@@ -159,9 +167,10 @@ public class AuthenticationServiceImplTest
     @Test
     public void testProtectionDisabledBadPassword()
     {
-        int attempts = 5;
+        int limit = 3;
+        int attempts = limit + 2;
         authService.setProtectionPeriodSeconds(99999);
-        authService.setProtectionLimit(attempts - 2);
+        authService.setProtectionLimit(limit);
         authService.setProtectionEnabled(false);
 
         Exception spoofedAE = new AuthenticationException("Bad password");