Fixup response code on incorrect credentials (apache#8671)

Author: vishesh92

plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/OAuth2UserAuthenticator.java

@@ -31,36 +31,46 @@
 import javax.inject.Inject;
 import java.util.Map;
 
+import static org.apache.cloudstack.oauth2.OAuth2AuthManager.OAuth2IsPluginEnabled;
+
 public class OAuth2UserAuthenticator extends AdapterBase implements UserAuthenticator {
 
     @Inject
-    private UserAccountDao _userAccountDao;
+    private UserAccountDao userAccountDao;
     @Inject
-    private UserDao _userDao;
+    private UserDao userDao;
 
     @Inject
-    private OAuth2AuthManager _userOAuth2mgr;
+    private OAuth2AuthManager userOAuth2mgr;
 
     @Override
     public Pair<Boolean, ActionOnFailedAuthentication> authenticate(String username, String password, Long domainId, Map<String, Object[]> requestParameters) {
         if (logger.isDebugEnabled()) {
             logger.debug("Trying OAuth2 auth for user: " + username);
         }
 
-        final UserAccount userAccount = _userAccountDao.getUserAccount(username, domainId);
-        if (userAccount == null || requestParameters == null || !OAuth2AuthManager.OAuth2IsPluginEnabled.value()) {
-            logger.debug("Unable to find user with " + username + " in domain " + domainId + ", or user source is not OAUTH2");
+        if (!isOAuthPluginEnabled()) {
+            s_logger.debug("OAuth2 plugin is disabled");
+            return new Pair<Boolean, ActionOnFailedAuthentication>(false, null);
+        } else if (requestParameters == null) {
+            s_logger.debug("Request parameters are null");
+            return new Pair<Boolean, ActionOnFailedAuthentication>(false, null);
+        }
+
+        final UserAccount userAccount = userAccountDao.getUserAccount(username, domainId);
+        if (userAccount == null) {
+            s_logger.debug("Unable to find user with " + username + " in domain " + domainId + ", or user source is not OAUTH2");
             return new Pair<Boolean, ActionOnFailedAuthentication>(false, null);
         } else {
-            User user = _userDao.getUser(userAccount.getId());
+            User user = userDao.getUser(userAccount.getId());
             final String[] provider = (String[])requestParameters.get(ApiConstants.PROVIDER);
             final String[] emailArray = (String[])requestParameters.get(ApiConstants.EMAIL);
             final String[] secretCodeArray = (String[])requestParameters.get(ApiConstants.SECRET_CODE);
             String oauthProvider = ((provider == null) ? null : provider[0]);
             String email = ((emailArray == null) ? null : emailArray[0]);
             String secretCode = ((secretCodeArray == null) ? null : secretCodeArray[0]);
 
-            UserOAuth2Authenticator authenticator = _userOAuth2mgr.getUserOAuth2AuthenticationProvider(oauthProvider);
+            UserOAuth2Authenticator authenticator = userOAuth2mgr.getUserOAuth2AuthenticationProvider(oauthProvider);
             if (user != null && authenticator.verifyUser(email, secretCode)) {
                 return new Pair<Boolean, ActionOnFailedAuthentication>(true, null);
             }
@@ -73,4 +83,8 @@ public Pair<Boolean, ActionOnFailedAuthentication> authenticate(String username,
     public String encode(String password) {
         return null;
     }
+
+    protected boolean isOAuthPluginEnabled() {
+        return OAuth2IsPluginEnabled.value();
+    }
 }

plugins/user-authenticators/oauth2/src/test/java/org/apache/cloudstack/oauth2/OAuth2UserAuthenticatorTest.java

@@ -27,21 +27,29 @@
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
+import org.junit.runner.RunWith;
 import org.mockito.InjectMocks;
 import org.mockito.Mock;
 import org.mockito.MockitoAnnotations;
+import org.mockito.Spy;
+import org.mockito.junit.MockitoJUnitRunner;
 
 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;
 import static org.mockito.ArgumentMatchers.anyLong;
 import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
+@RunWith(MockitoJUnitRunner.class)
 public class OAuth2UserAuthenticatorTest {
 
     @Mock
@@ -54,20 +62,24 @@ public class OAuth2UserAuthenticatorTest {
     private OAuth2AuthManager userOAuth2mgr;
 
     @InjectMocks
+    @Spy
     private OAuth2UserAuthenticator authenticator;
+    private AutoCloseable closeable;
 
     private AutoCloseable closeable;
 
     @Before
     public void setUp() {
         closeable = MockitoAnnotations.openMocks(this);
+        doReturn(true).when(authenticator).isOAuthPluginEnabled();
     }
 
     @After
     public void tearDown() throws Exception {
         closeable.close();
     }
 
+
     @Test
     public void testAuthenticateWithValidCredentials() {
         String username = "testuser";
@@ -92,13 +104,13 @@ public void testAuthenticateWithValidCredentials() {
 
         Pair<Boolean, OAuth2UserAuthenticator.ActionOnFailedAuthentication> result = authenticator.authenticate(username, null, domainId, requestParameters);
 
+        assertTrue(result.first());
+        assertNull(result.second());
+
         verify(userAccountDao).getUserAccount(username, domainId);
         verify(userDao).getUser(userAccount.getId());
         verify(userOAuth2mgr).getUserOAuth2AuthenticationProvider(provider[0]);
         verify(userOAuth2Authenticator).verifyUser(email[0], secretCode[0]);
-
-        assertEquals(true, result.first().booleanValue());
-        assertEquals(null, result.second());
     }
 
     @Test
@@ -114,7 +126,7 @@ public void testAuthenticateWithInvalidCredentials() {
         UserOAuth2Authenticator userOAuth2Authenticator = mock(UserOAuth2Authenticator.class);
 
         when(userAccountDao.getUserAccount(username, domainId)).thenReturn(userAccount);
-        when(userDao.getUser(userAccount.getId())).thenReturn( user);
+        when(userDao.getUser(userAccount.getId())).thenReturn(user);
         when(userOAuth2mgr.getUserOAuth2AuthenticationProvider(provider[0])).thenReturn(userOAuth2Authenticator);
         when(userOAuth2Authenticator.verifyUser(email[0], secretCode[0])).thenReturn(false);
 
@@ -125,13 +137,13 @@ public void testAuthenticateWithInvalidCredentials() {
 
         Pair<Boolean, OAuth2UserAuthenticator.ActionOnFailedAuthentication> result = authenticator.authenticate(username, null, domainId, requestParameters);
 
+        assertFalse(result.first());
+        assertEquals(OAuth2UserAuthenticator.ActionOnFailedAuthentication.INCREMENT_INCORRECT_LOGIN_ATTEMPT_COUNT, result.second());
+
         verify(userAccountDao).getUserAccount(username, domainId);
         verify(userDao).getUser(userAccount.getId());
         verify(userOAuth2mgr).getUserOAuth2AuthenticationProvider(provider[0]);
         verify(userOAuth2Authenticator).verifyUser(email[0], secretCode[0]);
-
-        assertEquals(false, result.first().booleanValue());
-        assertEquals(OAuth2UserAuthenticator.ActionOnFailedAuthentication.INCREMENT_INCORRECT_LOGIN_ATTEMPT_COUNT, result.second());
     }
 
     @Test
@@ -151,11 +163,11 @@ public void testAuthenticateWithInvalidUserAccount() {
 
         Pair<Boolean, OAuth2UserAuthenticator.ActionOnFailedAuthentication> result = authenticator.authenticate(username, null, domainId, requestParameters);
 
+        assertFalse(result.first());
+        assertNull(result.second());
+
         verify(userAccountDao).getUserAccount(username, domainId);
         verify(userDao, never()).getUser(anyLong());
         verify(userOAuth2mgr, never()).getUserOAuth2AuthenticationProvider(anyString());
-
-        assertEquals(false, result.first().booleanValue());
-        assertEquals(null, result.second());
     }
 }

server/src/main/java/com/cloud/user/AccountManagerImpl.java

@@ -335,6 +335,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M
 
     private List<SecurityChecker> _securityCheckers;
     private int _cleanupInterval;
+    private static final String OAUTH2_PROVIDER_NAME = "oauth2";
     private List<String> apiNameList;
     private static final String OAUTH2_PROVIDER_NAME = "oauth2";