address review comment, add unit test

Author: sudo87

api/src/main/java/org/apache/cloudstack/api/response/LoginCmdResponse.java

@@ -232,7 +232,7 @@ public Boolean getPasswordChangeRequired() {
         return passwordChangeRequired;
     }
 
-    public void setPasswordChangeRequired(String passwordChangeRequired) {
-        this.passwordChangeRequired = Boolean.parseBoolean(passwordChangeRequired);
+    public void setPasswordChangeRequired(Boolean passwordChangeRequired) {
+        this.passwordChangeRequired = passwordChangeRequired;
     }
 }

api/src/test/java/org/apache/cloudstack/api/command/admin/user/UpdateUserCmdTest.java

@@ -0,0 +1,64 @@
+/*
+ * 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.ApiCommandResourceType;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.InjectMocks;
+import org.mockito.junit.MockitoJUnitRunner;
+import org.springframework.test.util.ReflectionTestUtils;
+
+@RunWith(MockitoJUnitRunner.class)
+public class UpdateUserCmdTest {
+    @InjectMocks
+    private UpdateUserCmd cmd;
+
+    @Test
+    public void testGetApiResourceId() {
+        Long userId = 99L;
+        cmd.setId(userId);
+        Assert.assertEquals(userId, cmd.getApiResourceId());
+    }
+
+    @Test
+    public void testGetApiResourceType() {
+        Assert.assertEquals(ApiCommandResourceType.User, cmd.getApiResourceType());
+    }
+
+    @Test
+    public void testIsPasswordChangeRequired_True() {
+        ReflectionTestUtils.setField(cmd, "passwordChangeRequired", Boolean.TRUE);
+        Assert.assertTrue(cmd.isPasswordChangeRequired());
+    }
+
+    @Test
+    public void testIsPasswordChangeRequired_False() {
+        ReflectionTestUtils.setField(cmd, "passwordChangeRequired", Boolean.FALSE);
+        Assert.assertFalse(cmd.isPasswordChangeRequired());
+    }
+
+    @Test
+    public void testIsPasswordChangeRequired_Null() {
+        ReflectionTestUtils.setField(cmd, "passwordChangeRequired", null);
+        Assert.assertFalse(cmd.isPasswordChangeRequired());
+    }
+}

api/src/test/java/org/apache/cloudstack/api/response/LoginCmdResponseTest.java

@@ -0,0 +1,87 @@
+// 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.response;
+
+
+import org.junit.Assert;
+import org.junit.Test;
+
+public class LoginCmdResponseTest {
+
+    @Test
+    public void testAllGettersAndSetters() {
+        LoginCmdResponse response = new LoginCmdResponse();
+
+        response.setUsername("user1");
+        response.setUserId("100");
+        response.setDomainId("200");
+        response.setTimeout(3600);
+        response.setAccount("account1");
+        response.setFirstName("John");
+        response.setLastName("Doe");
+        response.setType("admin");
+        response.setTimeZone("UTC");
+        response.setTimeZoneOffset("+00:00");
+        response.setRegistered("true");
+        response.setSessionKey("session-key");
+        response.set2FAenabled("true");
+        response.set2FAverfied("false");
+        response.setProviderFor2FA("totp");
+        response.setIssuerFor2FA("cloudstack");
+        response.setManagementServerId("ms-1");
+
+        Assert.assertEquals("user1", response.getUsername());
+        Assert.assertEquals("100", response.getUserId());
+        Assert.assertEquals("200", response.getDomainId());
+        Assert.assertEquals(Integer.valueOf(3600), response.getTimeout());
+        Assert.assertEquals("account1", response.getAccount());
+        Assert.assertEquals("John", response.getFirstName());
+        Assert.assertEquals("Doe", response.getLastName());
+        Assert.assertEquals("admin", response.getType());
+        Assert.assertEquals("UTC", response.getTimeZone());
+        Assert.assertEquals("+00:00", response.getTimeZoneOffset());
+        Assert.assertEquals("true", response.getRegistered());
+        Assert.assertEquals("session-key", response.getSessionKey());
+        Assert.assertEquals("true", response.is2FAenabled());
+        Assert.assertEquals("false", response.is2FAverfied());
+        Assert.assertEquals("totp", response.getProviderFor2FA());
+        Assert.assertEquals("cloudstack", response.getIssuerFor2FA());
+        Assert.assertEquals("ms-1", response.getManagementServerId());
+    }
+
+    @Test
+    public void testPasswordChangeRequired_True() {
+        LoginCmdResponse response = new LoginCmdResponse();
+        response.setPasswordChangeRequired(true);
+        Assert.assertTrue(response.getPasswordChangeRequired());
+    }
+
+    @Test
+    public void testPasswordChangeRequired_False() {
+        LoginCmdResponse response = new LoginCmdResponse();
+        response.setPasswordChangeRequired(false);
+        Assert.assertFalse(response.getPasswordChangeRequired());
+    }
+
+    @Test
+    public void testPasswordChangeRequired_Null() {
+        LoginCmdResponse response = new LoginCmdResponse();
+        response.setPasswordChangeRequired(null);
+        Assert.assertNull("Boolean.parseBoolean(null) should return null", response.getPasswordChangeRequired());
+    }
+}

server/src/main/java/com/cloud/api/ApiServer.java

@@ -1230,8 +1230,8 @@ private ResponseObject createLoginResponse(HttpSession session) {
                 if (ApiConstants.MANAGEMENT_SERVER_ID.equalsIgnoreCase(attrName)) {
                     response.setManagementServerId(attrObj.toString());
                 }
-                if (PASSWORD_CHANGE_REQUIRED.equalsIgnoreCase(attrName)) {
-                    response.setPasswordChangeRequired(attrObj.toString());
+                if (PASSWORD_CHANGE_REQUIRED.equalsIgnoreCase(attrName) && attrObj instanceof Boolean) {
+                    response.setPasswordChangeRequired((Boolean) attrObj);
                 }
             }
         }

server/src/test/java/com/cloud/api/ApiServerTest.java

@@ -17,11 +17,21 @@
 package com.cloud.api;
 
 import com.cloud.domain.Domain;
+import com.cloud.domain.DomainVO;
+import com.cloud.exception.CloudAuthenticationException;
 import com.cloud.user.Account;
+import com.cloud.user.AccountManager;
+import com.cloud.user.DomainManager;
 import com.cloud.user.User;
 import com.cloud.user.UserAccount;
+import com.cloud.user.UserVO;
 import com.cloud.utils.exception.CloudRuntimeException;
+
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ResponseObject;
+import org.apache.cloudstack.api.response.LoginCmdResponse;
 import org.apache.cloudstack.framework.config.ConfigKey;
+import org.apache.cloudstack.resourcedetail.UserDetailVO;
 import org.apache.cloudstack.user.UserPasswordResetManager;
 import org.junit.AfterClass;
 import org.junit.Assert;
@@ -35,10 +45,22 @@
 import org.mockito.junit.MockitoJUnitRunner;
 
 import java.lang.reflect.Field;
+import java.net.InetAddress;
 import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 
+import static org.apache.cloudstack.api.ApiConstants.PASSWORD_CHANGE_REQUIRED;
 import static org.apache.cloudstack.user.UserPasswordResetManager.UserPasswordResetEnabled;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.mock;
+
+import javax.servlet.http.HttpSession;
 
 @RunWith(MockitoJUnitRunner.class)
 public class ApiServerTest {
@@ -49,6 +71,15 @@ public class ApiServerTest {
     @Mock
     UserPasswordResetManager userPasswordResetManager;
 
+    @Mock
+    DomainManager domainManager;
+
+    @Mock
+    AccountManager accountManager;
+
+    @Mock
+    HttpSession session;
+
     @BeforeClass
     public static void beforeClass() throws Exception {
         overrideDefaultConfigValue(UserPasswordResetEnabled, "_value", true);
@@ -96,8 +127,8 @@ public void testSetupIntegrationPortListenerValidPort() {
 
     @Test
     public void testForgotPasswordSuccess() {
-        UserAccount userAccount = Mockito.mock(UserAccount.class);
-        Domain domain = Mockito.mock(Domain.class);
+        UserAccount userAccount = mock(UserAccount.class);
+        Domain domain = mock(Domain.class);
 
         Mockito.when(userAccount.getEmail()).thenReturn("test@test.com");
         Mockito.when(userAccount.getState()).thenReturn("ENABLED");
@@ -110,17 +141,17 @@ public void testForgotPasswordSuccess() {
 
     @Test(expected = CloudRuntimeException.class)
     public void testForgotPasswordFailureNoEmail() {
-        UserAccount userAccount = Mockito.mock(UserAccount.class);
-        Domain domain = Mockito.mock(Domain.class);
+        UserAccount userAccount = mock(UserAccount.class);
+        Domain domain = mock(Domain.class);
 
         Mockito.when(userAccount.getEmail()).thenReturn("");
         apiServer.forgotPassword(userAccount, domain);
     }
 
     @Test(expected = CloudRuntimeException.class)
     public void testForgotPasswordFailureDisabledUser() {
-        UserAccount userAccount = Mockito.mock(UserAccount.class);
-        Domain domain = Mockito.mock(Domain.class);
+        UserAccount userAccount = mock(UserAccount.class);
+        Domain domain = mock(Domain.class);
 
         Mockito.when(userAccount.getEmail()).thenReturn("test@test.com");
         Mockito.when(userAccount.getState()).thenReturn("DISABLED");
@@ -129,8 +160,8 @@ public void testForgotPasswordFailureDisabledUser() {
 
     @Test(expected = CloudRuntimeException.class)
     public void testForgotPasswordFailureDisabledAccount() {
-        UserAccount userAccount = Mockito.mock(UserAccount.class);
-        Domain domain = Mockito.mock(Domain.class);
+        UserAccount userAccount = mock(UserAccount.class);
+        Domain domain = mock(Domain.class);
 
         Mockito.when(userAccount.getEmail()).thenReturn("test@test.com");
         Mockito.when(userAccount.getState()).thenReturn("ENABLED");
@@ -140,8 +171,8 @@ public void testForgotPasswordFailureDisabledAccount() {
 
     @Test(expected = CloudRuntimeException.class)
     public void testForgotPasswordFailureInactiveDomain() {
-        UserAccount userAccount = Mockito.mock(UserAccount.class);
-        Domain domain = Mockito.mock(Domain.class);
+        UserAccount userAccount = mock(UserAccount.class);
+        Domain domain = mock(Domain.class);
 
         Mockito.when(userAccount.getEmail()).thenReturn("test@test.com");
         Mockito.when(userAccount.getState()).thenReturn("ENABLED");
@@ -153,8 +184,8 @@ public void testForgotPasswordFailureInactiveDomain() {
     @Test
     public void testVerifyApiKeyAccessAllowed() {
         Long domainId = 1L;
-        User user = Mockito.mock(User.class);
-        Account account = Mockito.mock(Account.class);
+        User user = mock(User.class);
+        Account account = mock(Account.class);
 
         Mockito.when(user.getApiKeyAccess()).thenReturn(true);
         Assert.assertEquals(true, apiServer.verifyApiKeyAccessAllowed(user, account));
@@ -176,4 +207,73 @@ public void testVerifyApiKeyAccessAllowed() {
         Mockito.when(account.getApiKeyAccess()).thenReturn(null);
         Assert.assertEquals(true, apiServer.verifyApiKeyAccessAllowed(user, account));
     }
+
+    @Test
+    public void testLoginUserSuccess() throws Exception {
+        String username = "user";
+        String password = "password";
+        Long domainId = 1L;
+        String domainPath = "/";
+        InetAddress loginIp = InetAddress.getByName("127.0.0.1");
+        Map<String, Object[]> requestParams = new HashMap<>();
+
+        DomainVO domain = mock(DomainVO.class);
+        Mockito.when(domain.getId()).thenReturn(domainId);
+        Mockito.when(domain.getUuid()).thenReturn("domain-uuid");
+
+        Mockito.when(domainManager.findDomainByIdOrPath(domainId, domainPath)).thenReturn(domain);
+        Mockito.when(domainManager.getDomain(domainId)).thenReturn(domain);
+
+        UserAccount userAccount = mock(UserAccount.class);
+        Mockito.when(userAccount.getId()).thenReturn(100L);
+        Mockito.when(userAccount.getAccountId()).thenReturn(200L);
+        Mockito.when(userAccount.getUsername()).thenReturn(username);
+        Mockito.when(userAccount.getFirstname()).thenReturn("First");
+        Mockito.when(userAccount.getLastname()).thenReturn("Last");
+        Mockito.when(userAccount.getTimezone()).thenReturn("UTC");
+        Mockito.when(userAccount.getRegistrationToken()).thenReturn("token");
+        Mockito.when(userAccount.isRegistered()).thenReturn(true);
+        Mockito.when(userAccount.getDomainId()).thenReturn(domainId);
+        Map<String, String> userAccDetails = new HashMap<>();
+        userAccDetails.put(UserDetailVO.PasswordChangeRequired, "true");
+        Mockito.when(userAccount.getDetails()).thenReturn(userAccDetails);
+
+        Mockito.when(accountManager.authenticateUser(username, password, domainId, loginIp, requestParams)).thenReturn(userAccount);
+        Mockito.when(accountManager.clearUserTwoFactorAuthenticationInSetupStateOnLogin(userAccount)).thenReturn(userAccount);
+
+        Account account = mock(Account.class);
+        Mockito.when(account.getAccountName()).thenReturn("account");
+        Mockito.when(account.getDomainId()).thenReturn(domainId);
+        Mockito.when(account.getType()).thenReturn(Account.Type.NORMAL);
+        Mockito.when(account.getType()).thenReturn(Account.Type.NORMAL);
+        Mockito.when(accountManager.getAccount(200L)).thenReturn(account);
+
+        UserVO userVO = mock(UserVO.class);
+        Mockito.when(userVO.getUuid()).thenReturn("user-uuid");
+        Mockito.when(accountManager.getActiveUser(100L)).thenReturn(userVO);
+
+        Mockito.when(session.getAttributeNames()).thenReturn(Collections.enumeration(List.of(PASSWORD_CHANGE_REQUIRED)));
+        Mockito.when(session.getAttribute(PASSWORD_CHANGE_REQUIRED)).thenReturn(Boolean.TRUE);
+
+        ResponseObject response = apiServer.loginUser(session, username, password, domainId, domainPath, loginIp, requestParams);
+        Assert.assertNotNull(response);
+        Assert.assertTrue(response instanceof LoginCmdResponse);
+        Mockito.verify(session).setAttribute(eq("userid"), eq(100L));
+        Mockito.verify(session).setAttribute(eq(ApiConstants.SESSIONKEY), anyString());
+    }
+
+    @Test(expected = CloudAuthenticationException.class)
+    public void testLoginUserDomainNotFound() throws Exception {
+        Mockito.when(domainManager.findDomainByIdOrPath(anyLong(), anyString())).thenReturn(null);
+        apiServer.loginUser(session, "user", "pass", 1L, "/", null, null);
+    }
+
+    @Test(expected = CloudAuthenticationException.class)
+    public void testLoginUserAuthFailed() throws Exception {
+        DomainVO domain = mock(DomainVO.class);
+        Mockito.when(domain.getId()).thenReturn(1L);
+        Mockito.when(domainManager.findDomainByIdOrPath(anyLong(), anyString())).thenReturn(domain);
+        Mockito.when(accountManager.authenticateUser(anyString(), anyString(), anyLong(), any(), any())).thenReturn(null);
+        apiServer.loginUser(session, "user", "pass", 1L, "/", null, null);
+    }
 }