Skip to content

Commit b93589b

Browse files
authored
server: reset 2fa user configuration on incomplete setup (#10247)
Signed-off-by: Abhishek Kumar <[email protected]>
1 parent b989087 commit b93589b

File tree

7 files changed

+95
-1
lines changed

7 files changed

+95
-1
lines changed

engine/schema/src/main/java/com/cloud/user/UserAccountVO.java

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,8 @@
3535

3636
import com.cloud.utils.db.Encrypt;
3737
import com.cloud.utils.db.GenericDao;
38+
39+
import org.apache.cloudstack.utils.reflectiontostringbuilderutils.ReflectionToStringBuilderUtils;
3840
import org.apache.commons.lang3.StringUtils;
3941

4042
@Entity
@@ -368,4 +370,9 @@ public Map<String, String> getDetails() {
368370
public void setDetails(Map<String, String> details) {
369371
this.details = details;
370372
}
373+
374+
@Override
375+
public String toString() {
376+
return String.format("User %s", ReflectionToStringBuilderUtils.reflectOnlySelectedFields(this, "id", "name", "uuid"));
377+
}
371378
}

plugins/network-elements/juniper-contrail/src/test/java/org/apache/cloudstack/network/contrail/management/MockAccountManager.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -516,4 +516,9 @@ public String getConfigComponentName() {
516516
public ConfigKey<?>[] getConfigKeys() {
517517
return null;
518518
}
519+
520+
@Override
521+
public UserAccount clearUserTwoFactorAuthenticationInSetupStateOnLogin(UserAccount user) {
522+
return null;
523+
}
519524
}

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1159,7 +1159,7 @@ public ResponseObject loginUser(final HttpSession session, final String username
11591159
domainId = userDomain.getId();
11601160
}
11611161

1162-
final UserAccount userAcct = accountMgr.authenticateUser(username, password, domainId, loginIpAddress, requestParameters);
1162+
UserAccount userAcct = accountMgr.authenticateUser(username, password, domainId, loginIpAddress, requestParameters);
11631163
if (userAcct != null) {
11641164
final String timezone = userAcct.getTimezone();
11651165
float offsetInHrs = 0f;
@@ -1204,6 +1204,7 @@ public ResponseObject loginUser(final HttpSession session, final String username
12041204
session.setAttribute("timezoneoffset", Float.valueOf(offsetInHrs).toString());
12051205
}
12061206

1207+
userAcct = accountMgr.clearUserTwoFactorAuthenticationInSetupStateOnLogin(userAcct);
12071208
boolean is2faEnabled = false;
12081209
if (userAcct.isUser2faEnabled() || (Boolean.TRUE.equals(AccountManagerImpl.enableUserTwoFactorAuthentication.valueIn(userAcct.getDomainId())) && Boolean.TRUE.equals(AccountManagerImpl.mandateUserTwoFactorAuthentication.valueIn(userAcct.getDomainId())))) {
12091210
is2faEnabled = true;

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -195,6 +195,9 @@ void buildACLViewSearchCriteria(SearchCriteria<? extends ControlledViewEntity> s
195195
UserTwoFactorAuthenticator getUserTwoFactorAuthenticator(final Long domainId, final Long userAccountId);
196196

197197
void verifyUsingTwoFactorAuthenticationCode(String code, Long domainId, Long userAccountId);
198+
198199
UserTwoFactorAuthenticationSetupResponse setupUserTwoFactorAuthentication(SetupUserTwoFactorAuthenticationCmd cmd);
199200

201+
UserAccount clearUserTwoFactorAuthenticationInSetupStateOnLogin(UserAccount user);
202+
200203
}

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

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3475,4 +3475,26 @@ public UserTwoFactorAuthenticator getUserTwoFactorAuthenticator(final String nam
34753475
return userTwoFactorAuthenticationProvidersMap.get(name.toLowerCase());
34763476
}
34773477

3478+
@Override
3479+
public UserAccount clearUserTwoFactorAuthenticationInSetupStateOnLogin(UserAccount user) {
3480+
return Transaction.execute((TransactionCallback<UserAccount>) status -> {
3481+
if (!user.isUser2faEnabled() && StringUtils.isBlank(user.getUser2faProvider())) {
3482+
return user;
3483+
}
3484+
UserDetailVO userDetailVO = _userDetailsDao.findDetail(user.getId(), UserDetailVO.Setup2FADetail);
3485+
if (userDetailVO != null && UserAccountVO.Setup2FAstatus.VERIFIED.name().equals(userDetailVO.getValue())) {
3486+
return user;
3487+
}
3488+
s_logger.info(String.format("Clearing 2FA configurations for %s as it is still in setup on a new login request", user));
3489+
if (userDetailVO != null) {
3490+
_userDetailsDao.remove(userDetailVO.getId());
3491+
}
3492+
UserAccountVO userAccountVO = _userAccountDao.findById(user.getId());
3493+
userAccountVO.setUser2faEnabled(false);
3494+
userAccountVO.setUser2faProvider(null);
3495+
userAccountVO.setKeyFor2fa(null);
3496+
_userAccountDao.update(user.getId(), userAccountVO);
3497+
return userAccountVO;
3498+
});
3499+
}
34783500
}

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

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,10 +39,12 @@
3939
import org.apache.cloudstack.auth.UserTwoFactorAuthenticator;
4040
import org.apache.cloudstack.context.CallContext;
4141
import org.apache.cloudstack.framework.config.ConfigKey;
42+
import org.apache.cloudstack.resourcedetail.UserDetailVO;
4243
import org.junit.Assert;
4344
import org.junit.Before;
4445
import org.junit.Test;
4546
import org.junit.runner.RunWith;
47+
import org.mockito.ArgumentCaptor;
4648
import org.mockito.InOrder;
4749
import org.mockito.Mock;
4850
import org.mockito.Mockito;
@@ -1218,4 +1220,54 @@ public void checkIfAccountManagesProjectsTestThrowExceptionWhenTheAccountIsAProj
12181220
Mockito.when(_projectAccountDao.listAdministratedProjectIds(accountId)).thenReturn(managedProjectIds);
12191221
accountManagerImpl.checkIfAccountManagesProjects(accountId);
12201222
}
1223+
1224+
@Test
1225+
public void testClearUser2FA_When2FADisabled_NoChanges() {
1226+
UserAccount user = Mockito.mock(UserAccount.class);
1227+
Mockito.when(user.isUser2faEnabled()).thenReturn(false);
1228+
Mockito.when(user.getUser2faProvider()).thenReturn(null);
1229+
UserAccount result = accountManagerImpl.clearUserTwoFactorAuthenticationInSetupStateOnLogin(user);
1230+
Assert.assertSame(user, result);
1231+
Mockito.verifyNoInteractions(userDetailsDaoMock, userAccountDaoMock);
1232+
}
1233+
1234+
@Test
1235+
public void testClearUser2FA_When2FAInVerifiedState_NoChanges() {
1236+
UserAccount user = Mockito.mock(UserAccount.class);
1237+
Mockito.when(user.getId()).thenReturn(1L);
1238+
Mockito.when(user.isUser2faEnabled()).thenReturn(true);
1239+
UserDetailVO userDetail = new UserDetailVO();
1240+
userDetail.setValue(UserAccountVO.Setup2FAstatus.VERIFIED.name());
1241+
Mockito.when(userDetailsDaoMock.findDetail(1L, UserDetailVO.Setup2FADetail)).thenReturn(userDetail);
1242+
UserAccount result = accountManagerImpl.clearUserTwoFactorAuthenticationInSetupStateOnLogin(user);
1243+
Assert.assertSame(user, result);
1244+
Mockito.verify(userDetailsDaoMock).findDetail(1L, UserDetailVO.Setup2FADetail);
1245+
Mockito.verifyNoMoreInteractions(userDetailsDaoMock, userAccountDaoMock);
1246+
}
1247+
1248+
@Test
1249+
public void testClearUser2FA_When2FAInSetupState_Disable2FA() {
1250+
UserAccount user = Mockito.mock(UserAccount.class);
1251+
Mockito.when(user.getId()).thenReturn(1L);
1252+
Mockito.when(user.isUser2faEnabled()).thenReturn(true);
1253+
UserDetailVO userDetail = new UserDetailVO();
1254+
userDetail.setValue(UserAccountVO.Setup2FAstatus.ENABLED.name());
1255+
UserAccountVO userAccountVO = new UserAccountVO();
1256+
userAccountVO.setId(1L);
1257+
Mockito.when(userDetailsDaoMock.findDetail(1L, UserDetailVO.Setup2FADetail)).thenReturn(userDetail);
1258+
Mockito.when(userAccountDaoMock.findById(1L)).thenReturn(userAccountVO);
1259+
UserAccount result = accountManagerImpl.clearUserTwoFactorAuthenticationInSetupStateOnLogin(user);
1260+
Assert.assertNotNull(result);
1261+
Assert.assertFalse(result.isUser2faEnabled());
1262+
Assert.assertNull(result.getUser2faProvider());
1263+
Mockito.verify(userDetailsDaoMock).findDetail(1L, UserDetailVO.Setup2FADetail);
1264+
Mockito.verify(userDetailsDaoMock).remove(Mockito.anyLong());
1265+
Mockito.verify(userAccountDaoMock).findById(1L);
1266+
ArgumentCaptor<UserAccountVO> captor = ArgumentCaptor.forClass(UserAccountVO.class);
1267+
Mockito.verify(userAccountDaoMock).update(Mockito.eq(1L), captor.capture());
1268+
UserAccountVO updatedUser = captor.getValue();
1269+
Assert.assertFalse(updatedUser.isUser2faEnabled());
1270+
Assert.assertNull(updatedUser.getUser2faProvider());
1271+
Assert.assertNull(updatedUser.getKeyFor2fa());
1272+
}
12211273
}

server/src/test/java/com/cloud/user/MockAccountManagerImpl.java

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -484,4 +484,8 @@ public ConfigKey<?>[] getConfigKeys() {
484484
return null;
485485
}
486486

487+
@Override
488+
public UserAccount clearUserTwoFactorAuthenticationInSetupStateOnLogin(UserAccount user) {
489+
return null;
490+
}
487491
}

0 commit comments

Comments
 (0)