Skip to content

Commit 87b55af

Browse files
vishesh92vishesh92
authored
Fixup response code on incorrect credentials (#8671)
1 parent c970141 commit 87b55af

4 files changed

Lines changed: 53 additions & 18 deletions

File tree

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

Lines changed: 20 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -32,37 +32,47 @@
3232
import javax.inject.Inject;
3333
import java.util.Map;
3434

35+
import static org.apache.cloudstack.oauth2.OAuth2AuthManager.OAuth2IsPluginEnabled;
36+
3537
public class OAuth2UserAuthenticator extends AdapterBase implements UserAuthenticator {
3638
public static final Logger s_logger = Logger.getLogger(OAuth2UserAuthenticator.class);
3739

3840
@Inject
39-
private UserAccountDao _userAccountDao;
41+
private UserAccountDao userAccountDao;
4042
@Inject
41-
private UserDao _userDao;
43+
private UserDao userDao;
4244

4345
@Inject
44-
private OAuth2AuthManager _userOAuth2mgr;
46+
private OAuth2AuthManager userOAuth2mgr;
4547

4648
@Override
4749
public Pair<Boolean, ActionOnFailedAuthentication> authenticate(String username, String password, Long domainId, Map<String, Object[]> requestParameters) {
4850
if (s_logger.isDebugEnabled()) {
4951
s_logger.debug("Trying OAuth2 auth for user: " + username);
5052
}
5153

52-
final UserAccount userAccount = _userAccountDao.getUserAccount(username, domainId);
54+
if (!isOAuthPluginEnabled()) {
55+
s_logger.debug("OAuth2 plugin is disabled");
56+
return new Pair<Boolean, ActionOnFailedAuthentication>(false, null);
57+
} else if (requestParameters == null) {
58+
s_logger.debug("Request parameters are null");
59+
return new Pair<Boolean, ActionOnFailedAuthentication>(false, null);
60+
}
61+
62+
final UserAccount userAccount = userAccountDao.getUserAccount(username, domainId);
5363
if (userAccount == null) {
5464
s_logger.debug("Unable to find user with " + username + " in domain " + domainId + ", or user source is not OAUTH2");
5565
return new Pair<Boolean, ActionOnFailedAuthentication>(false, null);
5666
} else {
57-
User user = _userDao.getUser(userAccount.getId());
67+
User user = userDao.getUser(userAccount.getId());
5868
final String[] provider = (String[])requestParameters.get(ApiConstants.PROVIDER);
5969
final String[] emailArray = (String[])requestParameters.get(ApiConstants.EMAIL);
6070
final String[] secretCodeArray = (String[])requestParameters.get(ApiConstants.SECRET_CODE);
6171
String oauthProvider = ((provider == null) ? null : provider[0]);
6272
String email = ((emailArray == null) ? null : emailArray[0]);
6373
String secretCode = ((secretCodeArray == null) ? null : secretCodeArray[0]);
6474

65-
UserOAuth2Authenticator authenticator = _userOAuth2mgr.getUserOAuth2AuthenticationProvider(oauthProvider);
75+
UserOAuth2Authenticator authenticator = userOAuth2mgr.getUserOAuth2AuthenticationProvider(oauthProvider);
6676
if (user != null && authenticator.verifyUser(email, secretCode)) {
6777
return new Pair<Boolean, ActionOnFailedAuthentication>(true, null);
6878
}
@@ -75,4 +85,8 @@ public Pair<Boolean, ActionOnFailedAuthentication> authenticate(String username,
7585
public String encode(String password) {
7686
return null;
7787
}
88+
89+
protected boolean isOAuthPluginEnabled() {
90+
return OAuth2IsPluginEnabled.value();
91+
}
7892
}

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

Lines changed: 29 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -24,23 +24,32 @@
2424
import com.cloud.user.dao.UserDao;
2525
import com.cloud.utils.Pair;
2626
import org.apache.cloudstack.auth.UserOAuth2Authenticator;
27+
import org.junit.After;
2728
import org.junit.Before;
2829
import org.junit.Test;
30+
import org.junit.runner.RunWith;
2931
import org.mockito.InjectMocks;
3032
import org.mockito.Mock;
3133
import org.mockito.MockitoAnnotations;
34+
import org.mockito.Spy;
35+
import org.mockito.junit.MockitoJUnitRunner;
3236

3337
import java.util.HashMap;
3438
import java.util.Map;
3539

3640
import static org.junit.Assert.assertEquals;
41+
import static org.junit.Assert.assertFalse;
42+
import static org.junit.Assert.assertNull;
43+
import static org.junit.Assert.assertTrue;
3744
import static org.mockito.ArgumentMatchers.anyLong;
3845
import static org.mockito.ArgumentMatchers.anyString;
46+
import static org.mockito.Mockito.doReturn;
3947
import static org.mockito.Mockito.mock;
4048
import static org.mockito.Mockito.never;
4149
import static org.mockito.Mockito.verify;
4250
import static org.mockito.Mockito.when;
4351

52+
@RunWith(MockitoJUnitRunner.class)
4453
public class OAuth2UserAuthenticatorTest {
4554

4655
@Mock
@@ -53,13 +62,22 @@ public class OAuth2UserAuthenticatorTest {
5362
private OAuth2AuthManager userOAuth2mgr;
5463

5564
@InjectMocks
65+
@Spy
5666
private OAuth2UserAuthenticator authenticator;
67+
private AutoCloseable closeable;
5768

5869
@Before
5970
public void setUp() {
60-
MockitoAnnotations.initMocks(this);
71+
closeable = MockitoAnnotations.openMocks(this);
72+
doReturn(true).when(authenticator).isOAuthPluginEnabled();
6173
}
6274

75+
@After
76+
public void tearDown() throws Exception {
77+
closeable.close();
78+
}
79+
80+
6381
@Test
6482
public void testAuthenticateWithValidCredentials() {
6583
String username = "testuser";
@@ -84,13 +102,13 @@ public void testAuthenticateWithValidCredentials() {
84102

85103
Pair<Boolean, OAuth2UserAuthenticator.ActionOnFailedAuthentication> result = authenticator.authenticate(username, null, domainId, requestParameters);
86104

105+
assertTrue(result.first());
106+
assertNull(result.second());
107+
87108
verify(userAccountDao).getUserAccount(username, domainId);
88109
verify(userDao).getUser(userAccount.getId());
89110
verify(userOAuth2mgr).getUserOAuth2AuthenticationProvider(provider[0]);
90111
verify(userOAuth2Authenticator).verifyUser(email[0], secretCode[0]);
91-
92-
assertEquals(true, result.first().booleanValue());
93-
assertEquals(null, result.second());
94112
}
95113

96114
@Test
@@ -106,7 +124,7 @@ public void testAuthenticateWithInvalidCredentials() {
106124
UserOAuth2Authenticator userOAuth2Authenticator = mock(UserOAuth2Authenticator.class);
107125

108126
when(userAccountDao.getUserAccount(username, domainId)).thenReturn(userAccount);
109-
when(userDao.getUser(userAccount.getId())).thenReturn( user);
127+
when(userDao.getUser(userAccount.getId())).thenReturn(user);
110128
when(userOAuth2mgr.getUserOAuth2AuthenticationProvider(provider[0])).thenReturn(userOAuth2Authenticator);
111129
when(userOAuth2Authenticator.verifyUser(email[0], secretCode[0])).thenReturn(false);
112130

@@ -117,13 +135,13 @@ public void testAuthenticateWithInvalidCredentials() {
117135

118136
Pair<Boolean, OAuth2UserAuthenticator.ActionOnFailedAuthentication> result = authenticator.authenticate(username, null, domainId, requestParameters);
119137

138+
assertFalse(result.first());
139+
assertEquals(OAuth2UserAuthenticator.ActionOnFailedAuthentication.INCREMENT_INCORRECT_LOGIN_ATTEMPT_COUNT, result.second());
140+
120141
verify(userAccountDao).getUserAccount(username, domainId);
121142
verify(userDao).getUser(userAccount.getId());
122143
verify(userOAuth2mgr).getUserOAuth2AuthenticationProvider(provider[0]);
123144
verify(userOAuth2Authenticator).verifyUser(email[0], secretCode[0]);
124-
125-
assertEquals(false, result.first().booleanValue());
126-
assertEquals(OAuth2UserAuthenticator.ActionOnFailedAuthentication.INCREMENT_INCORRECT_LOGIN_ATTEMPT_COUNT, result.second());
127145
}
128146

129147
@Test
@@ -143,11 +161,11 @@ public void testAuthenticateWithInvalidUserAccount() {
143161

144162
Pair<Boolean, OAuth2UserAuthenticator.ActionOnFailedAuthentication> result = authenticator.authenticate(username, null, domainId, requestParameters);
145163

164+
assertFalse(result.first());
165+
assertNull(result.second());
166+
146167
verify(userAccountDao).getUserAccount(username, domainId);
147168
verify(userDao, never()).getUser(anyLong());
148169
verify(userOAuth2mgr, never()).getUserOAuth2AuthenticationProvider(anyString());
149-
150-
assertEquals(false, result.first().booleanValue());
151-
assertEquals(null, result.second());
152170
}
153171
}

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -332,6 +332,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M
332332

333333
private List<SecurityChecker> _securityCheckers;
334334
private int _cleanupInterval;
335+
private static final String OAUTH2_PROVIDER_NAME = "oauth2";
335336
private List<String> apiNameList;
336337

337338
protected static Map<String, UserTwoFactorAuthenticator> userTwoFactorAuthenticationProvidersMap = new HashMap<>();
@@ -2650,7 +2651,8 @@ private UserAccount getUserAccount(String username, String password, Long domain
26502651
continue;
26512652
}
26522653
}
2653-
if (secretCode != null && !authenticator.getName().equals("oauth2")) {
2654+
if ((secretCode != null && !authenticator.getName().equals(OAUTH2_PROVIDER_NAME))
2655+
|| (secretCode == null && authenticator.getName().equals(OAUTH2_PROVIDER_NAME))) {
26542656
continue;
26552657
}
26562658
Pair<Boolean, ActionOnFailedAuthentication> result = authenticator.authenticate(username, password, domainId, requestParameters);

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -203,6 +203,7 @@ public void testAuthenticateUser() throws UnknownHostException {
203203
Mockito.when(userAuthenticator.authenticate("test", "fail", 1L, new HashMap<>())).thenReturn(failureAuthenticationPair);
204204
Mockito.lenient().when(userAuthenticator.authenticate("test", null, 1L, new HashMap<>())).thenReturn(successAuthenticationPair);
205205
Mockito.lenient().when(userAuthenticator.authenticate("test", "", 1L, new HashMap<>())).thenReturn(successAuthenticationPair);
206+
Mockito.when(userAuthenticator.getName()).thenReturn("test");
206207

207208
//Test for incorrect password. authentication should fail
208209
UserAccount userAccount = accountManagerImpl.authenticateUser("test", "fail", 1L, InetAddress.getByName("127.0.0.1"), new HashMap<>());

0 commit comments

Comments
 (0)