Skip to content

Commit e552242

Browse files
author
Glover, Rene (rg9975)
committed
updates from commit comments
1 parent 26f0bab commit e552242

File tree

3 files changed

+138
-38
lines changed

3 files changed

+138
-38
lines changed

‎plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAMLUtils.java‎

Lines changed: 29 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -180,21 +180,9 @@ public static AuthnRequest buildAuthnRequestObject(final String authnId, final S
180180

181181
// AuthnContextClass. When this is false, the authentication requirements are defered to the SAML IDP and its default or configured workflow
182182
if (requirePasswordAuthentication) {
183-
AuthnContextClassRefBuilder authnContextClassRefBuilder = new AuthnContextClassRefBuilder();
184-
AuthnContextClassRef authnContextClassRef = authnContextClassRefBuilder.buildObject(
185-
SAMLConstants.SAML20_NS,
186-
"AuthnContextClassRef", "saml");
187-
authnContextClassRef.setAuthnContextClassRef(AuthnContext.PPT_AUTHN_CTX);
188-
189-
// AuthnContext
190-
RequestedAuthnContextBuilder requestedAuthnContextBuilder = new RequestedAuthnContextBuilder();
191-
RequestedAuthnContext requestedAuthnContext = requestedAuthnContextBuilder.buildObject();
192-
requestedAuthnContext.setComparison(AuthnContextComparisonTypeEnumeration.EXACT);
193-
requestedAuthnContext.getAuthnContextClassRefs().add(authnContextClassRef);
194-
authnRequest.setRequestedAuthnContext(requestedAuthnContext);
183+
setRequestedAuthnContext(authnRequest, requirePasswordAuthentication);
195184
}
196185

197-
198186
authnRequest.setID(authnId);
199187
authnRequest.setDestination(idpUrl);
200188
authnRequest.setVersion(SAMLVersion.VERSION_20);
@@ -209,6 +197,21 @@ public static AuthnRequest buildAuthnRequestObject(final String authnId, final S
209197
return authnRequest;
210198
}
211199

200+
public static void setRequestedAuthnContext(AuthnRequest authnRequest, boolean requirePasswordAuthentication) {
201+
AuthnContextClassRefBuilder authnContextClassRefBuilder = new AuthnContextClassRefBuilder();
202+
AuthnContextClassRef authnContextClassRef = authnContextClassRefBuilder.buildObject(
203+
SAMLConstants.SAML20_NS,
204+
"AuthnContextClassRef", "saml");
205+
authnContextClassRef.setAuthnContextClassRef(AuthnContext.PPT_AUTHN_CTX);
206+
207+
// AuthnContext
208+
RequestedAuthnContextBuilder requestedAuthnContextBuilder = new RequestedAuthnContextBuilder();
209+
RequestedAuthnContext requestedAuthnContext = requestedAuthnContextBuilder.buildObject();
210+
requestedAuthnContext.setComparison(AuthnContextComparisonTypeEnumeration.EXACT);
211+
requestedAuthnContext.getAuthnContextClassRefs().add(authnContextClassRef);
212+
authnRequest.setRequestedAuthnContext(requestedAuthnContext);
213+
}
214+
212215
public static LogoutRequest buildLogoutRequest(String logoutUrl, String spId, String nameIdString) {
213216
Issuer issuer = new IssuerBuilder().buildObject();
214217
issuer.setValue(spId);
@@ -304,13 +307,8 @@ public static void setupSamlUserCookies(final LoginCmdResponse loginResponse, fi
304307
throw new CloudRuntimeException("Invalid URI: " + redirectUrl);
305308
}
306309

307-
resp.addCookie(newCookie(domain, path, "userid", URLEncoder.encode(loginResponse.getUserId(), HttpUtils.UTF_8)));
308-
resp.addCookie(newCookie(domain, path,"domainid", URLEncoder.encode(loginResponse.getDomainId(), HttpUtils.UTF_8)));
309-
resp.addCookie(newCookie(domain, path,"role", URLEncoder.encode(loginResponse.getType(), HttpUtils.UTF_8)));
310-
resp.addCookie(newCookie(domain, path,"username", URLEncoder.encode(loginResponse.getUsername(), HttpUtils.UTF_8)));
311-
resp.addCookie(newCookie(domain, path,"account", URLEncoder.encode(loginResponse.getAccount(), HttpUtils.UTF_8)));
312-
resp.addCookie(newCookie(domain, path,"isSAML", URLEncoder.encode("true", HttpUtils.UTF_8)));
313-
resp.addCookie(newCookie(domain, path,"twoFaEnabled", URLEncoder.encode(loginResponse.is2FAenabled(), HttpUtils.UTF_8)));
310+
addBaseCookies(loginResponse, resp, domain, path);
311+
314312
String providerFor2FA = loginResponse.getProviderFor2FA();
315313
if (StringUtils.isNotEmpty(providerFor2FA)) {
316314
resp.addCookie(newCookie(domain, path,"twoFaProvider", URLEncoder.encode(loginResponse.getProviderFor2FA(), HttpUtils.UTF_8)));
@@ -319,7 +317,6 @@ public static void setupSamlUserCookies(final LoginCmdResponse loginResponse, fi
319317
if (timezone != null) {
320318
resp.addCookie(newCookie(domain, path,"timezone", URLEncoder.encode(timezone, HttpUtils.UTF_8)));
321319
}
322-
resp.addCookie(newCookie(domain, path,"userfullname", URLEncoder.encode(loginResponse.getFirstName() + " " + loginResponse.getLastName(), HttpUtils.UTF_8).replace("+", "%20")));
323320

324321
String sameSite = ApiServlet.getApiSessionKeySameSite();
325322
String sessionKeyCookie = String.format("%s=%s;Domain=%s;Path=%s;%s", ApiConstants.SESSIONKEY, loginResponse.getSessionKey(), domain, path, sameSite);
@@ -328,6 +325,17 @@ public static void setupSamlUserCookies(final LoginCmdResponse loginResponse, fi
328325
resp.addHeader("SET-COOKIE", String.format("%s=%s;HttpOnly;Path=/client/api;%s", ApiConstants.SESSIONKEY, loginResponse.getSessionKey(), sameSite));
329326
}
330327

328+
private static void addBaseCookies(final LoginCmdResponse loginResponse, final HttpServletResponse resp, String domain, String path) throws IOException {
329+
resp.addCookie(newCookie(domain, path, "userid", URLEncoder.encode(loginResponse.getUserId(), HttpUtils.UTF_8)));
330+
resp.addCookie(newCookie(domain, path,"domainid", URLEncoder.encode(loginResponse.getDomainId(), HttpUtils.UTF_8)));
331+
resp.addCookie(newCookie(domain, path,"role", URLEncoder.encode(loginResponse.getType(), HttpUtils.UTF_8)));
332+
resp.addCookie(newCookie(domain, path,"username", URLEncoder.encode(loginResponse.getUsername(), HttpUtils.UTF_8)));
333+
resp.addCookie(newCookie(domain, path,"account", URLEncoder.encode(loginResponse.getAccount(), HttpUtils.UTF_8)));
334+
resp.addCookie(newCookie(domain, path,"isSAML", URLEncoder.encode("true", HttpUtils.UTF_8)));
335+
resp.addCookie(newCookie(domain, path,"twoFaEnabled", URLEncoder.encode(loginResponse.is2FAenabled(), HttpUtils.UTF_8)));
336+
resp.addCookie(newCookie(domain, path,"userfullname", URLEncoder.encode(loginResponse.getFirstName() + " " + loginResponse.getLastName(), HttpUtils.UTF_8).replace("+", "%20")));
337+
}
338+
331339
private static Cookie newCookie(final String domain, final String path, final String name, final String value) {
332340
Cookie cookie = new Cookie(name, value);
333341
cookie.setDomain(domain);

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

Lines changed: 24 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1451,7 +1451,7 @@ public UserVO createUser(String userName, String password, String firstName, Str
14511451
List<UserVO> duplicatedUsers = _userDao.findUsersByName(userName);
14521452
for (UserVO duplicatedUser : duplicatedUsers) {
14531453
// users can't exist in same account
1454-
assertUserAlreadyInAccount(duplicatedUser, account);
1454+
assertUserNotAlreadyInAccount(duplicatedUser, account);
14551455
}
14561456

14571457
UserVO user = null;
@@ -1579,33 +1579,29 @@ protected void validateCurrentPassword(UserVO user, String currentPassword) {
15791579
* <li> The username must be unique in each domain. Therefore, if there is already another user with the same username, an {@link InvalidParameterValueException} is thrown.
15801580
* </ul>
15811581
*/
1582-
protected void validateAndUpdateUsernameIfNeeded(UpdateUserCmd updateUserCmd, UserVO user, Account account) {
1582+
protected void validateAndUpdateUsernameIfNeeded(UpdateUserCmd updateUserCmd, UserVO newUser, Account newAccount) {
15831583
String userName = updateUserCmd.getUsername();
15841584
if (userName == null) {
15851585
return;
15861586
}
15871587
if (StringUtils.isBlank(userName)) {
15881588
throw new InvalidParameterValueException("Username cannot be empty.");
15891589
}
1590-
List<UserVO> duplicatedUsers = _userDao.findUsersByName(userName);
1591-
for (UserVO duplicatedUser : duplicatedUsers) {
1592-
if (duplicatedUser.getId() == user.getId()) {
1590+
List<UserVO> existingUsers = _userDao.findUsersByName(userName);
1591+
for (UserVO existingUser : existingUsers) {
1592+
if (existingUser.getId() == newUser.getId()) {
15931593
continue;
15941594
}
15951595

15961596
// duplicate usernames cannot exist in same domain unless explicitly configured
1597-
if (!userAllowMultipleAccounts.valueInDomain(account.getDomainId())) {
1598-
Account duplicatedUserAccountWithUserThatHasTheSameUserName = _accountDao.findById(duplicatedUser.getAccountId());
1599-
if (duplicatedUserAccountWithUserThatHasTheSameUserName.getDomainId() == account.getDomainId()) {
1600-
DomainVO domain = _domainDao.findById(duplicatedUserAccountWithUserThatHasTheSameUserName.getDomainId());
1601-
throw new InvalidParameterValueException(String.format("Username [%s] already exists in domain [id=%s,name=%s]", duplicatedUser.getUsername(), domain.getUuid(), domain.getName()));
1602-
}
1597+
if (!userAllowMultipleAccounts.valueInDomain(newAccount.getDomainId())) {
1598+
assertUserNotAlreadyInDomain(existingUser, newAccount);
16031599
}
16041600

16051601
// can't rename a username to an existing one in the same account
1606-
assertUserAlreadyInAccount(duplicatedUser, account);
1602+
assertUserNotAlreadyInAccount(existingUser, newAccount);
16071603
}
1608-
user.setUsername(userName);
1604+
newUser.setUsername(userName);
16091605
}
16101606

16111607
/**
@@ -3526,9 +3522,20 @@ public UserAccount clearUserTwoFactorAuthenticationInSetupStateOnLogin(UserAccou
35263522
});
35273523
}
35283524

3529-
private void assertUserAlreadyInAccount(User user, Account account) {
3530-
if (user.getAccountId() == account.getId()) {
3531-
throw new InvalidParameterValueException(String.format("Username [%s] already exists in account [id=%s,name=%s]", user.getUsername(), account.getUuid(), account.getAccountName()));
3532-
}
3525+
void assertUserNotAlreadyInAccount(User existingUser, Account newAccount) {
3526+
System.out.println(existingUser.getAccountId());
3527+
System.out.println(newAccount.getId());
3528+
if (existingUser.getAccountId() == newAccount.getId()) {
3529+
AccountVO existingAccount = _accountDao.findById(newAccount.getId());
3530+
throw new InvalidParameterValueException(String.format("Username [%s] already exists in account [id=%s,name=%s]", existingUser.getUsername(), existingAccount.getUuid(), existingAccount.getAccountName()));
3531+
}
3532+
}
3533+
3534+
void assertUserNotAlreadyInDomain(User existingUser, Account originalAccount) {
3535+
Account existingAccount = _accountDao.findById(existingUser.getAccountId());
3536+
if (existingAccount.getDomainId() == originalAccount.getDomainId()) {
3537+
DomainVO existingDomain = _domainDao.findById(existingAccount.getDomainId());
3538+
throw new InvalidParameterValueException(String.format("Username [%s] already exists in domain [id=%s,name=%s] user account [id=%s,name=%s]", existingUser.getUsername(), existingDomain.getUuid(), existingDomain.getName(), existingAccount.getUuid(), existingAccount.getAccountName()));
3539+
}
35333540
}
35343541
}

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

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
// under the License.
1717
package com.cloud.user;
1818

19+
import static org.junit.Assert.assertThrows;
1920
import static org.mockito.ArgumentMatchers.nullable;
2021

2122
import java.net.InetAddress;
@@ -1270,4 +1271,88 @@ public void testClearUser2FA_When2FAInSetupState_Disable2FA() {
12701271
Assert.assertNull(updatedUser.getUser2faProvider());
12711272
Assert.assertNull(updatedUser.getKeyFor2fa());
12721273
}
1274+
1275+
@Test(expected = InvalidParameterValueException.class)
1276+
public void testAssertUserNotAlreadyInAccount_UserExistsInAccount() {
1277+
User existingUser = new UserVO();
1278+
existingUser.setUsername("testuser");
1279+
existingUser.setAccountId(1L);
1280+
1281+
Account newAccount = Mockito.mock(Account.class);
1282+
Mockito.when(newAccount.getId()).thenReturn(1L);
1283+
1284+
AccountVO existingAccount = Mockito.mock(AccountVO.class);
1285+
Mockito.when(existingAccount.getUuid()).thenReturn("existing-account-uuid");
1286+
Mockito.when(existingAccount.getAccountName()).thenReturn("existing-account");
1287+
1288+
Mockito.when(_accountDao.findById(1L)).thenReturn(existingAccount);
1289+
1290+
accountManagerImpl.assertUserNotAlreadyInAccount(existingUser, newAccount);
1291+
}
1292+
1293+
@Test
1294+
public void testAssertUserNotAlreadyInAccount_UserExistsInDiffAccount() {
1295+
User existingUser = new UserVO();
1296+
existingUser.setUsername("testuser");
1297+
existingUser.setAccountId(2L);
1298+
1299+
Account newAccount = Mockito.mock(Account.class);
1300+
Mockito.when(newAccount.getId()).thenReturn(1L);
1301+
1302+
AccountVO existingAccount = Mockito.mock(AccountVO.class);
1303+
Mockito.when(existingAccount.getUuid()).thenReturn("existing-account-uuid");
1304+
Mockito.when(existingAccount.getAccountName()).thenReturn("existing-account");
1305+
1306+
Mockito.when(_accountDao.findById(1L)).thenReturn(existingAccount);
1307+
1308+
accountManagerImpl.assertUserNotAlreadyInAccount(existingUser, newAccount);
1309+
}
1310+
1311+
@Test(expected = InvalidParameterValueException.class)
1312+
public void testAssertUserNotAlreadyInDomain_UserExistsInDomain() {
1313+
User existingUser = new UserVO();
1314+
existingUser.setUsername("testuser");
1315+
existingUser.setAccountId(1L);
1316+
1317+
Account originalAccount = Mockito.mock(Account.class);
1318+
Mockito.when(originalAccount.getDomainId()).thenReturn(1L);
1319+
1320+
AccountVO existingAccount = Mockito.mock(AccountVO.class);
1321+
Mockito.when(existingAccount.getDomainId()).thenReturn(1L);
1322+
Mockito.when(existingAccount.getUuid()).thenReturn("existing-account-uuid");
1323+
Mockito.when(existingAccount.getAccountName()).thenReturn("existing-account");
1324+
1325+
DomainVO existingDomain = Mockito.mock(DomainVO.class);
1326+
Mockito.when(existingDomain.getUuid()).thenReturn("existing-domain-uuid");
1327+
Mockito.when(existingDomain.getName()).thenReturn("existing-domain");
1328+
1329+
Mockito.when(_accountDao.findById(1L)).thenReturn(existingAccount);
1330+
Mockito.when(_domainDao.findById(1L)).thenReturn(existingDomain);
1331+
1332+
accountManagerImpl.assertUserNotAlreadyInDomain(existingUser, originalAccount);
1333+
}
1334+
1335+
@Test
1336+
public void testAssertUserNotAlreadyInDomain_UserExistsInDiffDomain() {
1337+
User existingUser = new UserVO();
1338+
existingUser.setUsername("testuser");
1339+
existingUser.setAccountId(1L);
1340+
1341+
Account originalAccount = Mockito.mock(Account.class);
1342+
Mockito.when(originalAccount.getDomainId()).thenReturn(1L);
1343+
1344+
AccountVO existingAccount = Mockito.mock(AccountVO.class);
1345+
Mockito.when(existingAccount.getDomainId()).thenReturn(2L);
1346+
Mockito.when(existingAccount.getUuid()).thenReturn("existing-account-uuid");
1347+
Mockito.when(existingAccount.getAccountName()).thenReturn("existing-account");
1348+
1349+
DomainVO existingDomain = Mockito.mock(DomainVO.class);
1350+
Mockito.when(existingDomain.getUuid()).thenReturn("existing-domain-uuid");
1351+
Mockito.when(existingDomain.getName()).thenReturn("existing-domain");
1352+
1353+
Mockito.when(_accountDao.findById(1L)).thenReturn(existingAccount);
1354+
Mockito.when(_domainDao.findById(1L)).thenReturn(existingDomain);
1355+
1356+
accountManagerImpl.assertUserNotAlreadyInDomain(existingUser, originalAccount);
1357+
}
12731358
}

0 commit comments

Comments
 (0)