Make the client lockout policy disabled by default

[#138677887] https://www.pivotaltracker.com/story/show/138677887

Signed-off-by: Bharath Sekar <bharath.sekar@ge.com>

model/src/main/java/org/cloudfoundry/identity/uaa/provider/LockoutPolicy.java

@@ -1,5 +1,7 @@
 package org.cloudfoundry.identity.uaa.provider;
 
+import com.fasterxml.jackson.annotation.JsonIgnore;
+
 public class LockoutPolicy {
     private int lockoutPeriodSeconds;
     private int lockoutAfterFailures;
@@ -15,6 +17,11 @@ public LockoutPolicy(int countFailuresWithin, int lockoutAfterFailures, int lock
         this.lockoutPeriodSeconds = lockoutPeriodSeconds;
     }
 
+    @JsonIgnore
+    public boolean isLockoutEnabled() {
+        return countFailuresWithin > 0;
+    }
+
     public LockoutPolicy setLockoutPeriodSeconds(int lockoutPeriod) {
         this.lockoutPeriodSeconds = lockoutPeriod;
         return this;

server/src/main/java/org/cloudfoundry/identity/uaa/authentication/manager/CommonLoginPolicy.java

@@ -12,12 +12,13 @@
  *******************************************************************************/
 package org.cloudfoundry.identity.uaa.authentication.manager;
 
-import java.util.List;
-
 import org.cloudfoundry.identity.uaa.audit.AuditEvent;
 import org.cloudfoundry.identity.uaa.audit.AuditEventType;
 import org.cloudfoundry.identity.uaa.audit.UaaAuditService;
 import org.cloudfoundry.identity.uaa.provider.LockoutPolicy;
+import org.cloudfoundry.identity.uaa.util.TimeService;
+
+import java.util.List;
 
 
 /**
@@ -29,29 +30,37 @@ public class CommonLoginPolicy implements LoginPolicy {
     private final LockoutPolicyRetriever lockoutPolicyRetriever;
     private final AuditEventType successEventType;
     private final AuditEventType failureEventType;
+    private final TimeService timeService;
 
-    public CommonLoginPolicy(UaaAuditService auditService, LockoutPolicyRetriever lockoutPolicyRetriever, AuditEventType successEventType,
-            AuditEventType failureEventType) {
+    public CommonLoginPolicy(UaaAuditService auditService,
+                             LockoutPolicyRetriever lockoutPolicyRetriever,
+                             AuditEventType successEventType,
+                             AuditEventType failureEventType,
+                             TimeService timeService) {
         this.auditService = auditService;
         this.lockoutPolicyRetriever = lockoutPolicyRetriever;
         this.successEventType = successEventType;
         this.failureEventType = failureEventType;
+        this.timeService = timeService;
     }
 
     @Override
     public Result isAllowed(String principalId) {
         LockoutPolicy lockoutPolicy = lockoutPolicyRetriever.getLockoutPolicy();
-
-        long eventsAfter = System.currentTimeMillis() - lockoutPolicy.getCountFailuresWithin() * 1000;
+
+        if (!lockoutPolicy.isLockoutEnabled()) {
+            return new Result(true, 0);
+        }
+
+        long eventsAfter = timeService.getCurrentTimeMillis() - lockoutPolicy.getCountFailuresWithin() * 1000;
         List<AuditEvent> events = auditService.find(principalId, eventsAfter);
 
         final int failureCount = sequentialFailureCount(events);
 
         if (failureCount >= lockoutPolicy.getLockoutAfterFailures()) {
-            // Check whether time of most recent failure is within the lockout
-            // period
+            // Check whether time of most recent failure is within the lockout period
             AuditEvent lastFailure = mostRecentFailure(events);
-            if (lastFailure != null && lastFailure.getTime() > System.currentTimeMillis() - lockoutPolicy.getLockoutPeriodSeconds() * 1000) {
+            if (lastFailure != null && lastFailure.getTime() > timeService.getCurrentTimeMillis() - lockoutPolicy.getLockoutPeriodSeconds() * 1000) {
                 return new Result(false, failureCount);
             }
         }

server/src/test/java/org/cloudfoundry/identity/uaa/authentication/manager/CommonLoginPolicyTest.java

@@ -0,0 +1,76 @@
+package org.cloudfoundry.identity.uaa.authentication.manager;
+
+import org.cloudfoundry.identity.uaa.audit.AuditEvent;
+import org.cloudfoundry.identity.uaa.audit.AuditEventType;
+import org.cloudfoundry.identity.uaa.audit.UaaAuditService;
+import org.cloudfoundry.identity.uaa.provider.LockoutPolicy;
+import org.cloudfoundry.identity.uaa.util.TimeService;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.util.Arrays;
+import java.util.List;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.Matchers.anyLong;
+import static org.mockito.Matchers.eq;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+public class CommonLoginPolicyTest {
+    private CommonLoginPolicy commonLoginPolicy;
+    private LockoutPolicyRetriever lockoutPolicyRetriever;
+    private TimeService timeService;
+    private UaaAuditService auditService;
+    private AuditEventType failureEventType;
+    private AuditEventType successEventType;
+
+    @Before
+    public void setup() {
+        auditService = mock(UaaAuditService.class);
+        timeService = mock(TimeService.class);
+        lockoutPolicyRetriever = mock(LockoutPolicyRetriever.class);
+        successEventType = AuditEventType.UserAuthenticationSuccess;
+        failureEventType = AuditEventType.UserAuthenticationFailure;
+
+        commonLoginPolicy = new CommonLoginPolicy(auditService, lockoutPolicyRetriever, successEventType, failureEventType, timeService);
+    }
+
+    @Test
+    public void isAllowed_whenLockoutAfterFailuresIsNegative_returnsTrue() {
+        when(lockoutPolicyRetriever.getLockoutPolicy()).thenReturn(new LockoutPolicy(-1, -1, 300));
+
+        LoginPolicy.Result result = commonLoginPolicy.isAllowed("principal");
+
+        assertTrue(result.isAllowed());
+        assertEquals(0, result.getFailureCount());
+    }
+
+    @Test
+    public void isAllowed_whenLockoutAfterFailuresIsPositive_returnsFalseIfTooManyFailedRecentAttempts() {
+        when(lockoutPolicyRetriever.getLockoutPolicy()).thenReturn(new LockoutPolicy(2, 1, 300));
+        AuditEvent auditEvent = new AuditEvent(failureEventType, null, null, null, 1L, null);
+        List<AuditEvent> list = Arrays.asList(auditEvent);
+        when(auditService.find(eq("principal"), anyLong())).thenReturn(list);
+
+        LoginPolicy.Result result = commonLoginPolicy.isAllowed("principal");
+
+        assertFalse(result.isAllowed());
+        assertEquals(1, result.getFailureCount());
+    }
+
+    @Test
+    public void isAllowed_whenLockoutAfterFailuresIsPositive_returnsTrueIfNotTooManyFailedRecentAttempts() {
+        when(lockoutPolicyRetriever.getLockoutPolicy()).thenReturn(new LockoutPolicy(2, 2, 300));
+        AuditEvent auditEvent = new AuditEvent(failureEventType, null, null, null, 1L, null);
+        List<AuditEvent> list = Arrays.asList(auditEvent);
+        when(auditService.find(eq("principal"), anyLong())).thenReturn(list);
+
+        LoginPolicy.Result result = commonLoginPolicy.isAllowed("principal");
+
+        assertTrue(result.isAllowed());
+        assertEquals(1, result.getFailureCount());
+    }
+}

server/src/test/java/org/cloudfoundry/identity/uaa/authentication/manager/PeriodLockoutPolicyTests.java

@@ -15,14 +15,16 @@
 import org.cloudfoundry.identity.uaa.audit.AuditEvent;
 import org.cloudfoundry.identity.uaa.audit.AuditEventType;
 import org.cloudfoundry.identity.uaa.audit.UaaAuditService;
-import org.cloudfoundry.identity.uaa.provider.LockoutPolicy;
 import org.cloudfoundry.identity.uaa.constants.OriginKeys;
-import org.cloudfoundry.identity.uaa.user.UaaUser;
 import org.cloudfoundry.identity.uaa.provider.IdentityProvider;
 import org.cloudfoundry.identity.uaa.provider.IdentityProviderProvisioning;
+import org.cloudfoundry.identity.uaa.provider.LockoutPolicy;
+import org.cloudfoundry.identity.uaa.provider.UaaIdentityProviderDefinition;
+import org.cloudfoundry.identity.uaa.user.UaaUser;
+import org.cloudfoundry.identity.uaa.util.TimeService;
+import org.cloudfoundry.identity.uaa.util.TimeServiceImpl;
 import org.cloudfoundry.identity.uaa.zone.IdentityZone;
 import org.cloudfoundry.identity.uaa.zone.IdentityZoneHolder;
-import org.cloudfoundry.identity.uaa.provider.UaaIdentityProviderDefinition;
 import org.junit.Before;
 import org.junit.Test;
 import org.springframework.security.core.Authentication;
@@ -33,9 +35,7 @@
 import static org.cloudfoundry.identity.uaa.audit.AuditEventType.UserAuthenticationSuccess;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
-import static org.mockito.Matchers.anyLong;
-import static org.mockito.Matchers.anyString;
-import static org.mockito.Matchers.eq;
+import static org.mockito.Matchers.*;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
@@ -58,14 +58,15 @@ public void setUp() throws Exception {
         now = System.currentTimeMillis();
         as = mock(UaaAuditService.class);
         joe = mock(UaaUser.class);
+        TimeService timeService = new TimeServiceImpl();
         when(joe.getId()).thenReturn("1");
         providerProvisioning = mock(IdentityProviderProvisioning.class);
         LockoutPolicy lockoutPolicy = new LockoutPolicy();
         lockoutPolicy.setCountFailuresWithin(ONE_HOUR);
         lockoutPolicy.setLockoutPeriodSeconds(ONE_HOUR);
         when(providerProvisioning.retrieveByOrigin(anyString(), anyString())).thenReturn(new IdentityProvider());
         policyRetriever = new UserLockoutPolicyRetriever(providerProvisioning);
-        innerPolicy = new CommonLoginPolicy(as, policyRetriever, AuditEventType.UserAuthenticationSuccess, AuditEventType.UserAuthenticationFailure);
+        innerPolicy = new CommonLoginPolicy(as, policyRetriever, AuditEventType.UserAuthenticationSuccess, AuditEventType.UserAuthenticationFailure, timeService);
         policyRetriever.setDefaultLockoutPolicy(lockoutPolicy);
         policy = new PeriodLockoutPolicy(innerPolicy);
     }

uaa/src/main/webapp/WEB-INF/spring-servlet.xml

@@ -422,7 +422,7 @@
         <constructor-arg ref="identityProviderProvisioning"/>
         <constructor-arg ref="environment"/>
         <property name="defaultPasswordPolicy" ref="defaultUaaPasswordPolicy"/>
-        <property name="defaultLockoutPolicy" ref="lockoutPolicy"/>
+        <property name="defaultLockoutPolicy" ref="userLockoutPolicy"/>
         <property name="disableInternalUserManagement" value="#{@config['disableInternalUserManagement'] == null ? false : @config['disableInternalUserManagement']}"/>
         <property name="ldapConfig" value="#{@config['ldap']}"/>
         <property name="keystoneConfig" value="#{@config['keystone']}"/>

uaa/src/main/webapp/WEB-INF/spring/oauth-endpoints.xml

@@ -259,10 +259,11 @@
         <constructor-arg ref="clientLockoutPolicyRetriever"/>
         <constructor-arg type="AuditEventType" value="ClientAuthenticationSuccess"/>
         <constructor-arg type="AuditEventType" value="ClientAuthenticationFailure"/>
+        <constructor-arg type="TimeService" ref="timeService" />
     </bean>
 
     <bean id="clientLockoutPolicyRetriever" class="org.cloudfoundry.identity.uaa.authentication.manager.ClientLockoutPolicyRetriever">
-        <property name="defaultLockoutPolicy" ref="globalLockoutPolicy" />
+        <property name="defaultLockoutPolicy" ref="defaultClientLockoutPolicy" />
     </bean>
 
     <bean id="compositeAuthenticationManager" class="org.cloudfoundry.identity.uaa.authentication.manager.CompositeAuthenticationManager" />
@@ -562,16 +563,16 @@
         <property name="caseInsensitive" ref="useCaseInsensitiveQueries"/>
     </bean>
 
-    <bean id="lockoutPolicy" class="org.cloudfoundry.identity.uaa.provider.LockoutPolicy">
+    <bean id="userLockoutPolicy" class="org.cloudfoundry.identity.uaa.provider.LockoutPolicy">
         <property name="lockoutAfterFailures"
-                  value="${authentication.policy.lockoutAfterFailures:#{globalLockoutPolicy.getLockoutAfterFailures()}}"/>
+                  value="${authentication.policy.lockoutAfterFailures:#{defaultUserLockoutPolicy.getLockoutAfterFailures()}}"/>
         <property name="countFailuresWithin"
-                  value="${authentication.policy.countFailuresWithinSeconds:#{globalLockoutPolicy.getCountFailuresWithin()}}"/>
+                  value="${authentication.policy.countFailuresWithinSeconds:#{defaultUserLockoutPolicy.getCountFailuresWithin()}}"/>
         <property name="lockoutPeriodSeconds"
-                  value="${authentication.policy.lockoutPeriodSeconds:#{globalLockoutPolicy.getLockoutPeriodSeconds()}}"/>
+                  value="${authentication.policy.lockoutPeriodSeconds:#{defaultUserLockoutPolicy.getLockoutPeriodSeconds()}}"/>
     </bean>
 
-    <bean id="globalLockoutPolicy" class="org.cloudfoundry.identity.uaa.provider.LockoutPolicy">
+    <bean id="defaultUserLockoutPolicy" class="org.cloudfoundry.identity.uaa.provider.LockoutPolicy">
         <property name="lockoutAfterFailures"
                   value="${authentication.policy.global.lockoutAfterFailures:5}"/>
         <property name="countFailuresWithin"
@@ -580,9 +581,18 @@
                   value="${authentication.policy.global.lockoutPeriodSeconds:300}"/>
     </bean>
 
+    <bean id="defaultClientLockoutPolicy" class="org.cloudfoundry.identity.uaa.provider.LockoutPolicy">
+        <property name="lockoutAfterFailures"
+                  value="${authentication.policy.global.lockoutAfterFailures:5}"/>
+        <property name="countFailuresWithin"
+                  value="${authentication.policy.global.countFailuresWithinSeconds:-1}"/>
+        <property name="lockoutPeriodSeconds"
+                  value="${authentication.policy.global.lockoutPeriodSeconds:300}"/>
+    </bean>
+
     <bean id="globalUserLockoutPolicyRetriever" class="org.cloudfoundry.identity.uaa.authentication.manager.UserLockoutPolicyRetriever">
         <constructor-arg ref="identityProviderProvisioning"/>
-        <property name="defaultLockoutPolicy" ref="globalLockoutPolicy" />
+        <property name="defaultLockoutPolicy" ref="defaultUserLockoutPolicy" />
     </bean>
 
     <bean id="globalPeriodLockoutPolicy" class="org.cloudfoundry.identity.uaa.authentication.manager.PeriodLockoutPolicy">
@@ -594,6 +604,7 @@
         <constructor-arg ref="globalUserLockoutPolicyRetriever"/>
         <constructor-arg type="AuditEventType" value="UserAuthenticationSuccess"/>
         <constructor-arg type="AuditEventType" value="UserAuthenticationFailure"/>
+        <constructor-arg type="TimeService" ref="timeService" />
     </bean>
 
     <bean id="uaaUserDatabaseAuthenticationManager"

uaa/src/test/java/org/cloudfoundry/identity/uaa/mock/token/TokenMvcMockTests.java

@@ -2868,73 +2868,61 @@ public void testGetClientCredentialsTokenForDefaultIdentityZone() throws Excepti
     }
 
     @Test
-    public void testClientCredentialsLockoutWithFormData() throws Exception {
+    public void clientCredentials_byDefault_willNotLockoutClientsUsingFormData() throws Exception {
         String clientId = "testclient" + generator.generate();
         String scopes = "space.*.developer,space.*.admin,org.*.reader,org.123*.admin,*.*,*";
         setUpClients(clientId, scopes, scopes, GRANT_TYPES, true);
 
-        String body = null;
         for(int i = 0; i < 6; i++){
-            body = getMockMvc().perform(post("/oauth/token")
-                .accept(MediaType.APPLICATION_JSON_VALUE)
-                .contentType(MediaType.APPLICATION_FORM_URLENCODED_VALUE)
-                .param("grant_type", "client_credentials")
-                .param("client_id", clientId)
-                .param("client_secret", BADSECRET)
-                )
+            getMockMvc()
+                .perform(post("/oauth/token")
+                    .accept(MediaType.APPLICATION_JSON_VALUE)
+                    .contentType(MediaType.APPLICATION_FORM_URLENCODED_VALUE)
+                    .param("grant_type", "client_credentials")
+                    .param("client_id", clientId)
+                    .param("client_secret", BADSECRET))
                 .andExpect(status().isUnauthorized())
                 .andReturn().getResponse().getContentAsString();
-
         }
 
-        body = getMockMvc().perform(post("/oauth/token")
+        getMockMvc()
+            .perform(post("/oauth/token")
                 .accept(MediaType.APPLICATION_JSON_VALUE)
                 .contentType(MediaType.APPLICATION_FORM_URLENCODED_VALUE)
                 .param("grant_type", "client_credentials")
                 .param("client_id", clientId)
-                .param("client_secret", SECRET)
-                )
-                .andExpect(status().isUnauthorized())
-                .andReturn().getResponse().getContentAsString();
-
-
-        Map<String,Object> bodyMap = JsonUtils.readValue(body, new TypeReference<Map<String,Object>>() {});
-        assertEquals(bodyMap.get("error_description"),"Client " + clientId + " has 5 failed authentications within the last checking period.");
+                .param("client_secret", SECRET))
+            .andExpect(status().isOk())
+            .andReturn().getResponse().getContentAsString();
     }
 
     @Test
-    public void testClientCredentialsLockoutWithBasicAuth() throws Exception {
+    public void clientCredentials_byDefault_WillNotLockoutDuringFailedBasicAuth() throws Exception {
         String clientId = "testclient" + generator.generate();
         String scopes = "space.*.developer,space.*.admin,org.*.reader,org.123*.admin,*.*,*";
         setUpClients(clientId, scopes, scopes, GRANT_TYPES, true);
 
-        String body = null;
         for(int i = 0; i < 6; i++){
-            body = getMockMvc().perform(post("/oauth/token")
-                .accept(MediaType.APPLICATION_JSON_VALUE)
-                .header("Authorization", "Basic " + new String(Base64.encode((clientId + ":" + BADSECRET).getBytes())))
-                .param("grant_type", "client_credentials")
-                )
+            getMockMvc()
+                .perform(post("/oauth/token")
+                    .accept(MediaType.APPLICATION_JSON_VALUE)
+                    .header("Authorization", "Basic " + new String(Base64.encode((clientId + ":" + BADSECRET).getBytes())))
+                    .param("grant_type", "client_credentials"))
                 .andExpect(status().isUnauthorized())
                 .andReturn().getResponse().getContentAsString();
-
         }
 
-        body = getMockMvc().perform(post("/oauth/token")
+        getMockMvc()
+            .perform(post("/oauth/token")
                 .accept(MediaType.APPLICATION_JSON_VALUE)
                 .header("Authorization", "Basic " + new String(Base64.encode((clientId + ":" + SECRET).getBytes())))
-                .param("grant_type", "client_credentials")
-                )
-                .andExpect(status().isUnauthorized())
-                .andReturn().getResponse().getContentAsString();
-
-
-        Map<String,Object> bodyMap = JsonUtils.readValue(body, new TypeReference<Map<String,Object>>() {});
-        assertEquals(bodyMap.get("error_description"),"Client " + clientId + " has 5 failed authentications within the last checking period.");
+                .param("grant_type", "client_credentials"))
+            .andExpect(status().isOk())
+            .andReturn().getResponse().getContentAsString();
     }
 
     @Test
-    public void testClientCredentialsLockoutWithBasicAuthAndFormData() throws Exception {
+    public void clientCredentials_byDefault_WillNotLockoutDuringFailedBasicAuthAndFormData() throws Exception {
         String clientId = "testclient" + generator.generate();
         String scopes = "space.*.developer,space.*.admin,org.*.reader,org.123*.admin,*.*,*";
         setUpClients(clientId, scopes, scopes, GRANT_TYPES, true);
@@ -2966,12 +2954,8 @@ public void testClientCredentialsLockoutWithBasicAuthAndFormData() throws Except
                 .header("Authorization", "Basic " + new String(Base64.encode((clientId + ":" + SECRET).getBytes())))
                 .param("grant_type", "client_credentials")
                 )
-                .andExpect(status().isUnauthorized())
+                .andExpect(status().isOk())
                 .andReturn().getResponse().getContentAsString();
-
-
-        Map<String,Object> bodyMap = JsonUtils.readValue(body, new TypeReference<Map<String,Object>>() {});
-        assertEquals(bodyMap.get("error_description"),"Client " + clientId + " has 5 failed authentications within the last checking period.");
     }
 
     @Test