Skip to content

4.19 fix saml account selector #10311

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
DaanHoogland merged 9 commits into from
Apr 14, 2025
Merged

4.19 fix saml account selector #10311

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ef8d1ea
Fix issues with SAML account switcher
Jan 31, 2025
fdb654b
Fix issues with SAML account switcher
Jan 31, 2025
1f50e61
updates from PR feedback
Feb 3, 2025
730155c
resolve format build issue in SamlDomainSwitcher
Feb 11, 2025
0b54c72
add config check when validating username rename
Feb 12, 2025
d61991f
remove redirect check in saml test as its moved to ui side
Feb 14, 2025
26f0bab
Merge branch '4.19' into 4.19-fix-saml-account-selector
Feb 26, 2025
e552242
updates from commit comments
Feb 27, 2025
9ed57b5
fix unused test imports and stubbings
Mar 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
updates from commit comments
  • Loading branch information
Glover, Rene (rg9975) committed Feb 27, 2025
commit e552242f90593ba1c14073831688e7e163f04eb6
50 changes: 29 additions & 21 deletions plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAMLUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,21 +180,9 @@ public static AuthnRequest buildAuthnRequestObject(final String authnId, final S

// AuthnContextClass. When this is false, the authentication requirements are defered to the SAML IDP and its default or configured workflow
if (requirePasswordAuthentication) {
AuthnContextClassRefBuilder authnContextClassRefBuilder = new AuthnContextClassRefBuilder();
AuthnContextClassRef authnContextClassRef = authnContextClassRefBuilder.buildObject(
SAMLConstants.SAML20_NS,
"AuthnContextClassRef", "saml");
authnContextClassRef.setAuthnContextClassRef(AuthnContext.PPT_AUTHN_CTX);

// AuthnContext
RequestedAuthnContextBuilder requestedAuthnContextBuilder = new RequestedAuthnContextBuilder();
RequestedAuthnContext requestedAuthnContext = requestedAuthnContextBuilder.buildObject();
requestedAuthnContext.setComparison(AuthnContextComparisonTypeEnumeration.EXACT);
requestedAuthnContext.getAuthnContextClassRefs().add(authnContextClassRef);
authnRequest.setRequestedAuthnContext(requestedAuthnContext);
setRequestedAuthnContext(authnRequest, requirePasswordAuthentication);
}

Comment thread
rg9975 marked this conversation as resolved.

authnRequest.setID(authnId);
authnRequest.setDestination(idpUrl);
authnRequest.setVersion(SAMLVersion.VERSION_20);
Expand All @@ -209,6 +197,21 @@ public static AuthnRequest buildAuthnRequestObject(final String authnId, final S
return authnRequest;
}

public static void setRequestedAuthnContext(AuthnRequest authnRequest, boolean requirePasswordAuthentication) {
AuthnContextClassRefBuilder authnContextClassRefBuilder = new AuthnContextClassRefBuilder();
AuthnContextClassRef authnContextClassRef = authnContextClassRefBuilder.buildObject(
SAMLConstants.SAML20_NS,
"AuthnContextClassRef", "saml");
authnContextClassRef.setAuthnContextClassRef(AuthnContext.PPT_AUTHN_CTX);

// AuthnContext
RequestedAuthnContextBuilder requestedAuthnContextBuilder = new RequestedAuthnContextBuilder();
RequestedAuthnContext requestedAuthnContext = requestedAuthnContextBuilder.buildObject();
requestedAuthnContext.setComparison(AuthnContextComparisonTypeEnumeration.EXACT);
requestedAuthnContext.getAuthnContextClassRefs().add(authnContextClassRef);
authnRequest.setRequestedAuthnContext(requestedAuthnContext);
}

public static LogoutRequest buildLogoutRequest(String logoutUrl, String spId, String nameIdString) {
Issuer issuer = new IssuerBuilder().buildObject();
issuer.setValue(spId);
Expand Down Expand Up @@ -304,13 +307,8 @@ public static void setupSamlUserCookies(final LoginCmdResponse loginResponse, fi
throw new CloudRuntimeException("Invalid URI: " + redirectUrl);
}

resp.addCookie(newCookie(domain, path, "userid", URLEncoder.encode(loginResponse.getUserId(), HttpUtils.UTF_8)));
resp.addCookie(newCookie(domain, path,"domainid", URLEncoder.encode(loginResponse.getDomainId(), HttpUtils.UTF_8)));
resp.addCookie(newCookie(domain, path,"role", URLEncoder.encode(loginResponse.getType(), HttpUtils.UTF_8)));
resp.addCookie(newCookie(domain, path,"username", URLEncoder.encode(loginResponse.getUsername(), HttpUtils.UTF_8)));
resp.addCookie(newCookie(domain, path,"account", URLEncoder.encode(loginResponse.getAccount(), HttpUtils.UTF_8)));
resp.addCookie(newCookie(domain, path,"isSAML", URLEncoder.encode("true", HttpUtils.UTF_8)));
resp.addCookie(newCookie(domain, path,"twoFaEnabled", URLEncoder.encode(loginResponse.is2FAenabled(), HttpUtils.UTF_8)));
addBaseCookies(loginResponse, resp, domain, path);

String providerFor2FA = loginResponse.getProviderFor2FA();
if (StringUtils.isNotEmpty(providerFor2FA)) {
resp.addCookie(newCookie(domain, path,"twoFaProvider", URLEncoder.encode(loginResponse.getProviderFor2FA(), HttpUtils.UTF_8)));
Expand All @@ -319,7 +317,6 @@ public static void setupSamlUserCookies(final LoginCmdResponse loginResponse, fi
if (timezone != null) {
resp.addCookie(newCookie(domain, path,"timezone", URLEncoder.encode(timezone, HttpUtils.UTF_8)));
}
resp.addCookie(newCookie(domain, path,"userfullname", URLEncoder.encode(loginResponse.getFirstName() + " " + loginResponse.getLastName(), HttpUtils.UTF_8).replace("+", "%20")));

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

private static void addBaseCookies(final LoginCmdResponse loginResponse, final HttpServletResponse resp, String domain, String path) throws IOException {
resp.addCookie(newCookie(domain, path, "userid", URLEncoder.encode(loginResponse.getUserId(), HttpUtils.UTF_8)));
resp.addCookie(newCookie(domain, path,"domainid", URLEncoder.encode(loginResponse.getDomainId(), HttpUtils.UTF_8)));
resp.addCookie(newCookie(domain, path,"role", URLEncoder.encode(loginResponse.getType(), HttpUtils.UTF_8)));
resp.addCookie(newCookie(domain, path,"username", URLEncoder.encode(loginResponse.getUsername(), HttpUtils.UTF_8)));
resp.addCookie(newCookie(domain, path,"account", URLEncoder.encode(loginResponse.getAccount(), HttpUtils.UTF_8)));
resp.addCookie(newCookie(domain, path,"isSAML", URLEncoder.encode("true", HttpUtils.UTF_8)));
resp.addCookie(newCookie(domain, path,"twoFaEnabled", URLEncoder.encode(loginResponse.is2FAenabled(), HttpUtils.UTF_8)));
resp.addCookie(newCookie(domain, path,"userfullname", URLEncoder.encode(loginResponse.getFirstName() + " " + loginResponse.getLastName(), HttpUtils.UTF_8).replace("+", "%20")));
}

private static Cookie newCookie(final String domain, final String path, final String name, final String value) {
Cookie cookie = new Cookie(name, value);
cookie.setDomain(domain);
Expand Down
41 changes: 24 additions & 17 deletions server/src/main/java/com/cloud/user/AccountManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1451,7 +1451,7 @@ public UserVO createUser(String userName, String password, String firstName, Str
List<UserVO> duplicatedUsers = _userDao.findUsersByName(userName);
for (UserVO duplicatedUser : duplicatedUsers) {
// users can't exist in same account
assertUserAlreadyInAccount(duplicatedUser, account);
assertUserNotAlreadyInAccount(duplicatedUser, account);
}

UserVO user = null;
Expand Down Expand Up @@ -1579,33 +1579,29 @@ protected void validateCurrentPassword(UserVO user, String currentPassword) {
* <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.
* </ul>
*/
protected void validateAndUpdateUsernameIfNeeded(UpdateUserCmd updateUserCmd, UserVO user, Account account) {
protected void validateAndUpdateUsernameIfNeeded(UpdateUserCmd updateUserCmd, UserVO newUser, Account newAccount) {
String userName = updateUserCmd.getUsername();
if (userName == null) {
return;
}
if (StringUtils.isBlank(userName)) {
throw new InvalidParameterValueException("Username cannot be empty.");
}
List<UserVO> duplicatedUsers = _userDao.findUsersByName(userName);
for (UserVO duplicatedUser : duplicatedUsers) {
if (duplicatedUser.getId() == user.getId()) {
List<UserVO> existingUsers = _userDao.findUsersByName(userName);
for (UserVO existingUser : existingUsers) {
if (existingUser.getId() == newUser.getId()) {
continue;
}

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

// can't rename a username to an existing one in the same account
assertUserAlreadyInAccount(duplicatedUser, account);
assertUserNotAlreadyInAccount(existingUser, newAccount);
}
user.setUsername(userName);
newUser.setUsername(userName);
}

/**
Expand Down Expand Up @@ -3526,9 +3522,20 @@ public UserAccount clearUserTwoFactorAuthenticationInSetupStateOnLogin(UserAccou
});
}

private void assertUserAlreadyInAccount(User user, Account account) {
if (user.getAccountId() == account.getId()) {
throw new InvalidParameterValueException(String.format("Username [%s] already exists in account [id=%s,name=%s]", user.getUsername(), account.getUuid(), account.getAccountName()));
}
void assertUserNotAlreadyInAccount(User existingUser, Account newAccount) {
System.out.println(existingUser.getAccountId());
System.out.println(newAccount.getId());
if (existingUser.getAccountId() == newAccount.getId()) {
AccountVO existingAccount = _accountDao.findById(newAccount.getId());
throw new InvalidParameterValueException(String.format("Username [%s] already exists in account [id=%s,name=%s]", existingUser.getUsername(), existingAccount.getUuid(), existingAccount.getAccountName()));
}
}

void assertUserNotAlreadyInDomain(User existingUser, Account originalAccount) {
Account existingAccount = _accountDao.findById(existingUser.getAccountId());
if (existingAccount.getDomainId() == originalAccount.getDomainId()) {
DomainVO existingDomain = _domainDao.findById(existingAccount.getDomainId());
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()));
}
}
}
85 changes: 85 additions & 0 deletions server/src/test/java/com/cloud/user/AccountManagerImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
// under the License.
package com.cloud.user;

import static org.junit.Assert.assertThrows;
import static org.mockito.ArgumentMatchers.nullable;

import java.net.InetAddress;
Expand Down Expand Up @@ -1270,4 +1271,88 @@ public void testClearUser2FA_When2FAInSetupState_Disable2FA() {
Assert.assertNull(updatedUser.getUser2faProvider());
Assert.assertNull(updatedUser.getKeyFor2fa());
}

@Test(expected = InvalidParameterValueException.class)
public void testAssertUserNotAlreadyInAccount_UserExistsInAccount() {
User existingUser = new UserVO();
existingUser.setUsername("testuser");
existingUser.setAccountId(1L);

Account newAccount = Mockito.mock(Account.class);
Mockito.when(newAccount.getId()).thenReturn(1L);

AccountVO existingAccount = Mockito.mock(AccountVO.class);
Mockito.when(existingAccount.getUuid()).thenReturn("existing-account-uuid");
Mockito.when(existingAccount.getAccountName()).thenReturn("existing-account");

Mockito.when(_accountDao.findById(1L)).thenReturn(existingAccount);

accountManagerImpl.assertUserNotAlreadyInAccount(existingUser, newAccount);
}

@Test
public void testAssertUserNotAlreadyInAccount_UserExistsInDiffAccount() {
User existingUser = new UserVO();
existingUser.setUsername("testuser");
existingUser.setAccountId(2L);

Account newAccount = Mockito.mock(Account.class);
Mockito.when(newAccount.getId()).thenReturn(1L);

AccountVO existingAccount = Mockito.mock(AccountVO.class);
Mockito.when(existingAccount.getUuid()).thenReturn("existing-account-uuid");
Mockito.when(existingAccount.getAccountName()).thenReturn("existing-account");

Mockito.when(_accountDao.findById(1L)).thenReturn(existingAccount);

accountManagerImpl.assertUserNotAlreadyInAccount(existingUser, newAccount);
}

@Test(expected = InvalidParameterValueException.class)
public void testAssertUserNotAlreadyInDomain_UserExistsInDomain() {
User existingUser = new UserVO();
existingUser.setUsername("testuser");
existingUser.setAccountId(1L);

Account originalAccount = Mockito.mock(Account.class);
Mockito.when(originalAccount.getDomainId()).thenReturn(1L);

AccountVO existingAccount = Mockito.mock(AccountVO.class);
Mockito.when(existingAccount.getDomainId()).thenReturn(1L);
Mockito.when(existingAccount.getUuid()).thenReturn("existing-account-uuid");
Mockito.when(existingAccount.getAccountName()).thenReturn("existing-account");

DomainVO existingDomain = Mockito.mock(DomainVO.class);
Mockito.when(existingDomain.getUuid()).thenReturn("existing-domain-uuid");
Mockito.when(existingDomain.getName()).thenReturn("existing-domain");

Mockito.when(_accountDao.findById(1L)).thenReturn(existingAccount);
Mockito.when(_domainDao.findById(1L)).thenReturn(existingDomain);

accountManagerImpl.assertUserNotAlreadyInDomain(existingUser, originalAccount);
}

@Test
public void testAssertUserNotAlreadyInDomain_UserExistsInDiffDomain() {
User existingUser = new UserVO();
existingUser.setUsername("testuser");
existingUser.setAccountId(1L);

Account originalAccount = Mockito.mock(Account.class);
Mockito.when(originalAccount.getDomainId()).thenReturn(1L);

AccountVO existingAccount = Mockito.mock(AccountVO.class);
Mockito.when(existingAccount.getDomainId()).thenReturn(2L);
Mockito.when(existingAccount.getUuid()).thenReturn("existing-account-uuid");
Mockito.when(existingAccount.getAccountName()).thenReturn("existing-account");

DomainVO existingDomain = Mockito.mock(DomainVO.class);
Mockito.when(existingDomain.getUuid()).thenReturn("existing-domain-uuid");
Mockito.when(existingDomain.getName()).thenReturn("existing-domain");

Mockito.when(_accountDao.findById(1L)).thenReturn(existingAccount);
Mockito.when(_domainDao.findById(1L)).thenReturn(existingDomain);

accountManagerImpl.assertUserNotAlreadyInDomain(existingUser, originalAccount);
}
}