Merge pull request #807 from karuturi/createuser-unittests

unittests to verify empty password is not allowed during account/user create

* pr/807:
  unittests to verify empty password is not allowed during account create

Signed-off-by: Wido den Hollander <wido@widodh.nl>

api/src/org/apache/cloudstack/api/command/admin/account/CreateAccountCmd.java

@@ -19,6 +19,7 @@ package org.apache.cloudstack.api.command.admin.account;
 import java.util.Collection;
 import java.util.Map;
 
+import org.apache.commons.lang.StringUtils;
 import org.apache.log4j.Logger;
 
 import org.apache.cloudstack.api.APICommand;
@@ -31,7 +32,6 @@ import org.apache.cloudstack.api.ServerApiException;
 import org.apache.cloudstack.api.response.AccountResponse;
 import org.apache.cloudstack.api.response.DomainResponse;
 import org.apache.cloudstack.context.CallContext;
-import org.apache.commons.lang.StringUtils;
 
 import com.cloud.user.Account;
 import com.cloud.user.UserAccount;
@@ -175,9 +175,7 @@ public class CreateAccountCmd extends BaseCmd {
 
     @Override
     public void execute() {
-        if (StringUtils.isEmpty(getPassword())) {
-            throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Empty passwords are not allowed");
-        }
+        validateParams();
         CallContext.current().setEventDetails("Account Name: " + getAccountName() + ", Domain Id:" + getDomainId());
         UserAccount userAccount =
             _accountService.createUserAccount(getUsername(), getPassword(), getFirstName(), getLastName(), getEmail(), getTimeZone(), getAccountName(), getAccountType(),
@@ -190,4 +188,13 @@ public class CreateAccountCmd extends BaseCmd {
             throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Failed to create a user account");
         }
     }
+
+    /**
+     * TODO: this should be done through a validator. for now replicating the validation logic in create account and user
+     */
+    private void validateParams() {
+        if(StringUtils.isEmpty(getPassword())) {
+            throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Empty passwords are not allowed");
+        }
+    }
 }

api/src/org/apache/cloudstack/api/command/admin/user/CreateUserCmd.java

@@ -150,9 +150,7 @@ public class CreateUserCmd extends BaseCmd {
 
     @Override
     public void execute() {
-        if (StringUtils.isEmpty(getPassword())) {
-            throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Empty passwords are not allowed");
-        }
+        validateParams();
         CallContext.current().setEventDetails("UserName: " + getUserName() + ", FirstName :" + getFirstName() + ", LastName: " + getLastName());
         User user =
             _accountService.createUser(getUserName(), getPassword(), getFirstName(), getLastName(), getEmail(), getTimezone(), getAccountName(), getDomainId(),
@@ -165,4 +163,13 @@ public class CreateUserCmd extends BaseCmd {
             throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Failed to create a user");
         }
     }
+
+    /**
+     * TODO: this should be done through a validator. for now replicating the validation logic in create account and user
+     */
+    private void validateParams() {
+        if(StringUtils.isEmpty(getPassword())) {
+            throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Empty passwords are not allowed");
+        }
+    }
 }

api/test/org/apache/cloudstack/api/command/admin/account/CreateAccountCmdTest.java

@@ -0,0 +1,100 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.cloudstack.api.command.admin.account;
+
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.context.CallContext;
+import org.apache.log4j.Logger;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.MockitoAnnotations;
+import org.springframework.test.util.ReflectionTestUtils;
+
+import com.cloud.user.Account;
+import com.cloud.user.AccountService;
+import com.cloud.user.User;
+
+public class CreateAccountCmdTest {
+    public static final Logger s_logger = Logger.getLogger(CreateAccountCmdTest.class.getName());
+
+    @Mock
+    private AccountService accountService;
+
+    @InjectMocks
+    private CreateAccountCmd createAccountCmd = new CreateAccountCmd();
+
+    private short accountType = 1;
+    private Long domainId = 1L;
+
+    @Before
+    public void setUp() throws Exception {
+        MockitoAnnotations.initMocks(this);
+        ReflectionTestUtils.setField(createAccountCmd, "domainId", domainId);
+        ReflectionTestUtils.setField(createAccountCmd, "accountType", accountType);
+        CallContext.register(Mockito.mock(User.class), Mockito.mock(Account.class));
+    }
+
+    @After
+    public void tearDown() throws Exception {
+        CallContext.unregister();
+    }
+
+    @Test
+    public void testExecuteWithNotBlankPassword() {
+        ReflectionTestUtils.setField(createAccountCmd, "password", "Test");
+        try {
+            createAccountCmd.execute();
+        } catch (ServerApiException e) {
+            Assert.assertTrue("Received exception as the mock accountService createUserAccount returns null user", true);
+        }
+        Mockito.verify(accountService, Mockito.times(1)).createUserAccount(null, "Test", null, null, null, null, null, accountType, domainId, null, null, null, null);
+    }
+
+    @Test
+    public void testExecuteWithNullPassword() {
+        ReflectionTestUtils.setField(createAccountCmd, "password", null);
+        try {
+            createAccountCmd.execute();
+            Assert.fail("should throw exception for a null password");
+        } catch (ServerApiException e) {
+            Assert.assertEquals(ApiErrorCode.PARAM_ERROR, e.getErrorCode());
+            Assert.assertEquals("Empty passwords are not allowed", e.getMessage());
+        }
+        Mockito.verify(accountService, Mockito.never()).createUserAccount(null, null, null, null, null, null, null, accountType, domainId, null, null, null, null);
+    }
+
+    @Test
+    public void testExecuteWithEmptyPassword() {
+        ReflectionTestUtils.setField(createAccountCmd, "password", "");
+        try {
+            createAccountCmd.execute();
+            Assert.fail("should throw exception for a empty password");
+        } catch (ServerApiException e) {
+            Assert.assertEquals(ApiErrorCode.PARAM_ERROR, e.getErrorCode());
+            Assert.assertEquals("Empty passwords are not allowed", e.getMessage());
+        }
+        Mockito.verify(accountService, Mockito.never()).createUserAccount(null, null, null, null, null, null, null, accountType, domainId, null, null, null, null);
+    }
+}

api/test/org/apache/cloudstack/api/command/admin/user/CreateUserCmdTest.java

@@ -0,0 +1,96 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.cloudstack.api.command.admin.user;
+
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.context.CallContext;
+import org.apache.log4j.Logger;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.MockitoAnnotations;
+import org.springframework.test.util.ReflectionTestUtils;
+
+import com.cloud.user.Account;
+import com.cloud.user.AccountService;
+import com.cloud.user.User;
+
+public class CreateUserCmdTest {
+    public static final Logger s_logger = Logger.getLogger(CreateUserCmdTest.class.getName());
+
+    @Mock
+    private AccountService accountService;
+
+    @InjectMocks
+    private CreateUserCmd createUserCmd = new CreateUserCmd();
+
+    @Before
+    public void setUp() throws Exception {
+        MockitoAnnotations.initMocks(this);
+        CallContext.register(Mockito.mock(User.class), Mockito.mock(Account.class));
+    }
+
+    @After
+    public void tearDown() throws Exception {
+        CallContext.unregister();
+    }
+
+    @Test
+    public void testExecuteWithNotBlankPassword() {
+        ReflectionTestUtils.setField(createUserCmd, "password", "Test");
+        try {
+            createUserCmd.execute();
+        } catch (ServerApiException e) {
+            Assert.assertTrue("Received exception as the mock accountService createUser returns null user", true);
+        }
+        Mockito.verify(accountService, Mockito.times(1)).createUser(null, "Test", null, null, null, null, null, null, null);
+    }
+
+    @Test
+    public void testExecuteWithNullPassword() {
+        ReflectionTestUtils.setField(createUserCmd, "password", null);
+        try {
+            createUserCmd.execute();
+            Assert.fail("should throw exception for a null password");
+        } catch (ServerApiException e) {
+            Assert.assertEquals(ApiErrorCode.PARAM_ERROR,e.getErrorCode());
+            Assert.assertEquals("Empty passwords are not allowed", e.getMessage());
+        }
+        Mockito.verify(accountService, Mockito.never()).createUser(null, null, null, null, null, null, null, null, null);
+    }
+
+    @Test
+    public void testExecuteWithEmptyPassword() {
+        ReflectionTestUtils.setField(createUserCmd, "password", "");
+        try {
+            createUserCmd.execute();
+            Assert.fail("should throw exception for a empty password");
+        } catch (ServerApiException e) {
+            Assert.assertEquals(ApiErrorCode.PARAM_ERROR,e.getErrorCode());
+            Assert.assertEquals("Empty passwords are not allowed", e.getMessage());
+        }
+        Mockito.verify(accountService, Mockito.never()).createUser(null, null, null, null, null, null, null, null, null);
+    }
+}

server/test/com/cloud/user/AccountManagerImplTest.java

@@ -17,11 +17,15 @@
 package com.cloud.user;
 
 import java.lang.reflect.Field;
+import java.net.InetAddress;
+import java.net.UnknownHostException;
 import java.util.Arrays;
 import java.util.ArrayList;
 
 import javax.inject.Inject;
 
+import com.cloud.server.auth.UserAuthenticator;
+import com.cloud.utils.Pair;
 import org.junit.After;
 import org.junit.Assert;
 import org.junit.Before;
@@ -88,6 +92,7 @@ import com.cloud.vm.dao.DomainRouterDao;
 import com.cloud.vm.dao.InstanceGroupDao;
 import com.cloud.vm.dao.UserVmDao;
 import com.cloud.vm.dao.VMInstanceDao;
+import org.springframework.test.util.ReflectionTestUtils;
 
 @RunWith(MockitoJUnitRunner.class)
 public class AccountManagerImplTest {
@@ -197,6 +202,9 @@ public class AccountManagerImplTest {
     @Mock
     SecurityChecker securityChecker;
 
+    @Mock
+    private UserAuthenticator userAuthenticator;
+
     @Before
     public void setup() throws NoSuchFieldException, SecurityException,
             IllegalArgumentException, IllegalAccessException {
@@ -213,6 +221,7 @@ public class AccountManagerImplTest {
                 }
             }
         }
+        ReflectionTestUtils.setField(accountManager, "_userAuthenticators", Arrays.asList(userAuthenticator));
         accountManager.setSecurityCheckers(Arrays.asList(securityChecker));
         CallContext.register(callingUser, callingAccount);
     }
@@ -312,4 +321,38 @@ public class AccountManagerImplTest {
         Mockito.verify(_accountDao, Mockito.atLeastOnce()).markForCleanup(
                 Mockito.eq(42l));
     }
+
+
+    @Test
+    public void testAuthenticateUser() throws UnknownHostException {
+        Pair<Boolean, UserAuthenticator.ActionOnFailedAuthentication> successAuthenticationPair = new Pair<>(true, null);
+        Pair<Boolean, UserAuthenticator.ActionOnFailedAuthentication> failureAuthenticationPair = new Pair<>(false,
+                                                                                                             UserAuthenticator.ActionOnFailedAuthentication.INCREMENT_INCORRECT_LOGIN_ATTEMPT_COUNT);
+
+        UserAccountVO userAccountVO = new UserAccountVO();
+        userAccountVO.setSource(User.Source.UNKNOWN);
+        userAccountVO.setState(Account.State.disabled.toString());
+        Mockito.when(_userAccountDao.getUserAccount("test", 1L)).thenReturn(userAccountVO);
+        Mockito.when(userAuthenticator.authenticate("test", "fail", 1L, null)).thenReturn(failureAuthenticationPair);
+        Mockito.when(userAuthenticator.authenticate("test", null, 1L, null)).thenReturn(successAuthenticationPair);
+        Mockito.when(userAuthenticator.authenticate("test", "", 1L, null)).thenReturn(successAuthenticationPair);
+
+        //Test for incorrect password. authentication should fail
+        UserAccount userAccount = accountManager.authenticateUser("test", "fail", 1L, InetAddress.getByName("127.0.0.1"), null);
+        Assert.assertNull(userAccount);
+
+        //Test for null password. authentication should fail
+        userAccount = accountManager.authenticateUser("test", null, 1L, InetAddress.getByName("127.0.0.1"), null);
+        Assert.assertNull(userAccount);
+
+        //Test for empty password. authentication should fail
+        userAccount = accountManager.authenticateUser("test", "", 1L, InetAddress.getByName("127.0.0.1"), null);
+        Assert.assertNull(userAccount);
+
+        //Verifying that the authentication method is only called when password is specified
+        Mockito.verify(userAuthenticator, Mockito.times(1)).authenticate("test", "fail", 1L, null);
+        Mockito.verify(userAuthenticator, Mockito.never()).authenticate("test", null, 1L, null);
+        Mockito.verify(userAuthenticator, Mockito.never()).authenticate("test", "", 1L, null);
+
+    }
 }