Skip to content

make sure pre saml login is enabled after saml is disabled #13953

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

Open
DaanHoogland wants to merge 1 commit into
base: main
Choose a base branch
from
+89 −1
Open

make sure pre saml login is enabled after saml is disabled #13953

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
30 changes: 29 additions & 1 deletion ...r-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@
import org.apache.cloudstack.framework.config.Configurable;
import org.apache.cloudstack.framework.security.keystore.KeystoreDao;
import org.apache.cloudstack.framework.security.keystore.KeystoreVO;
import org.apache.cloudstack.resourcedetail.UserDetailVO;
import org.apache.cloudstack.resourcedetail.dao.UserDetailsDao;
import org.apache.cloudstack.utils.security.CertUtils;
import org.apache.commons.codec.binary.Base64;
import org.apache.commons.httpclient.HttpClient;
Expand Down Expand Up @@ -92,6 +94,10 @@
@Component
public class SAML2AuthManagerImpl extends AdapterBase implements SAML2AuthManager, Configurable {

/** Remembers the user's Source (e.g. LDAP) from before SAML was authorized, so disabling
* SAML can fall back to it instead of always defaulting to {@link User.Source#UNKNOWN}. */
private static final String PRE_SAML_SOURCE_DETAIL_KEY = "PreSamlSource";

private SAMLProviderMetadata _spMetadata = new SAMLProviderMetadata();
private Map<String, SAMLProviderMetadata> _idpMetadataMap = new HashMap<String, SAMLProviderMetadata>();

Expand All @@ -115,7 +121,10 @@
@Inject
private UserDao _userDao;

@Inject
private UserDetailsDao userDetailsDao;

@Inject

Check warning on line 127 in plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Remove this field injection and use constructor injection instead. 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AaArJ31krd49U0dPV8fp&open=AaArJ31krd49U0dPV8fp&pullRequest=13953
DomainManager _domainMgr;

@Override
Expand Down Expand Up @@ -444,17 +453,20 @@
}

@Override
public boolean authorizeUser(Long userId, String entityId, boolean enable) {

Check failure on line 456 in plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor this method to reduce its Cognitive Complexity from 17 to the 15 allowed. 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AaArJ31krd49U0dPV8fq&open=AaArJ31krd49U0dPV8fq&pullRequest=13953
UserVO user = _userDao.getUser(userId);
if (user != null) {
if (enable) {
if (user.getSource() != null && !User.Source.SAML2.equals(user.getSource()) && !User.Source.SAML2DISABLED.equals(user.getSource())) {
userDetailsDao.addDetail(user.getId(), PRE_SAML_SOURCE_DETAIL_KEY, user.getSource().toString(), false);
}
user.setExternalEntity(entityId);
user.setSource(User.Source.SAML2);
} else {
boolean enableLoginAfterSAMLDisable = SAML2AuthManager.EnableLoginAfterSAMLDisable.value();
if (user.getSource().equals(User.Source.SAML2)) {
if(enableLoginAfterSAMLDisable) {
user.setSource(User.Source.UNKNOWN);
user.setSource(getPreSamlSource(user.getId()));
} else {
user.setSource(User.Source.SAML2DISABLED);
}
Expand All @@ -468,6 +480,22 @@
return false;
}

/**
* The Source (e.g. LDAP) the user had before SAML was authorized for them, so disabling
* SAML can restore it instead of always falling back to {@link User.Source#UNKNOWN}.
*/
private User.Source getPreSamlSource(long userId) {
UserDetailVO preSamlSource = userDetailsDao.findDetail(userId, PRE_SAML_SOURCE_DETAIL_KEY);
if (preSamlSource != null) {
try {
return User.Source.valueOf(preSamlSource.getValue());
} catch (IllegalArgumentException e) {
logger.warn("Unrecognized pre-SAML source '{}' stored for user {}; falling back to UNKNOWN", preSamlSource.getValue(), userId);
}
}
return User.Source.UNKNOWN;
}

@Override
public void saveToken(String authnId, String domainPath, String entity) {
Long domainId = null;
Expand Down
60 changes: 60 additions & 0 deletions ...er-authenticators/saml2/src/test/java/org/apache/cloudstack/SAML2AuthManagerImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,11 @@
package org.apache.cloudstack;

import java.lang.reflect.Field;
import java.lang.reflect.Method;

import org.apache.cloudstack.framework.security.keystore.KeystoreDao;
import org.apache.cloudstack.resourcedetail.UserDetailVO;
import org.apache.cloudstack.resourcedetail.dao.UserDetailsDao;
import org.apache.cloudstack.saml.SAML2AuthManagerImpl;
import org.apache.cloudstack.saml.SAMLTokenDao;
import org.apache.cloudstack.saml.SAMLTokenVO;
Expand Down Expand Up @@ -50,6 +53,9 @@
@Mock
private UserDao userDao;

@Mock
private UserDetailsDao userDetailsDao;

@Mock
DomainManager domainMgr;

Expand All @@ -72,6 +78,10 @@
userDaoField.setAccessible(true);
userDaoField.set(saml2AuthManager, userDao);

Field userDetailsDaoField = SAML2AuthManagerImpl.class.getDeclaredField("userDetailsDao");
userDetailsDaoField.setAccessible(true);
userDetailsDaoField.set(saml2AuthManager, userDetailsDao);

Field domainMgrField = SAML2AuthManagerImpl.class.getDeclaredField("_domainMgr");
domainMgrField.setAccessible(true);
domainMgrField.set(saml2AuthManager, domainMgr);
Expand Down Expand Up @@ -117,7 +127,57 @@
Mockito.verify(userDao, Mockito.atLeastOnce()).update(Mockito.anyLong(), Mockito.any(user.getClass()));
}

@Test
public void testAuthorizeUserStoresPreSamlSourceOnEnable() {
UserVO user = new UserVO(200L);
user.setUsername("someuser");
user.setSource(User.Source.LDAP);
Mockito.when(userDao.getUser(Mockito.anyLong())).thenReturn(user);

Check warning on line 135 in plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/SAML2AuthManagerImplTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a static import for "when". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AaArJ3x7rd49U0dPV8fh&open=AaArJ3x7rd49U0dPV8fh&pullRequest=13953

saml2AuthManager.authorizeUser(200L, "someID", true);

Mockito.verify(userDetailsDao).addDetail(200L, "PreSamlSource", "LDAP", false);

Check warning on line 139 in plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/SAML2AuthManagerImplTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a static import for "verify". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AaArJ3x7rd49U0dPV8fi&open=AaArJ3x7rd49U0dPV8fi&pullRequest=13953
assertEquals(User.Source.SAML2, user.getSource());
}

@Test
public void testAuthorizeUserDoesNotRestorePreSamlSourceWhenAlreadyAuthorized() {
UserVO user = new UserVO(200L);
user.setUsername("someuser");
user.setSource(User.Source.SAML2);
Mockito.when(userDao.getUser(Mockito.anyLong())).thenReturn(user);

Check warning on line 148 in plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/SAML2AuthManagerImplTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a static import for "when". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AaArJ3x7rd49U0dPV8fj&open=AaArJ3x7rd49U0dPV8fj&pullRequest=13953

saml2AuthManager.authorizeUser(200L, "someID", true);

Mockito.verify(userDetailsDao, Mockito.never()).addDetail(Mockito.anyLong(), Mockito.anyString(), Mockito.anyString(), Mockito.anyBoolean());

Check warning on line 152 in plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/SAML2AuthManagerImplTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a static import for "verify". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AaArJ3x7rd49U0dPV8fk&open=AaArJ3x7rd49U0dPV8fk&pullRequest=13953

Check warning on line 152 in plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/SAML2AuthManagerImplTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a static import for "never". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AaArJ3x7rd49U0dPV8fl&open=AaArJ3x7rd49U0dPV8fl&pullRequest=13953
}

@Test
public void testGetPreSamlSourceRestoresStoredSource() throws Exception {
Mockito.when(userDetailsDao.findDetail(200L, "PreSamlSource")).thenReturn(new UserDetailVO(200L, "PreSamlSource", "LDAP"));

Check warning on line 157 in plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/SAML2AuthManagerImplTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a static import for "when". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AaArJ3x7rd49U0dPV8fm&open=AaArJ3x7rd49U0dPV8fm&pullRequest=13953

assertEquals(User.Source.LDAP, invokeGetPreSamlSource(200L));
}

@Test
public void testGetPreSamlSourceDefaultsToUnknownWhenNothingStored() throws Exception {
Mockito.when(userDetailsDao.findDetail(200L, "PreSamlSource")).thenReturn(null);

Check warning on line 164 in plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/SAML2AuthManagerImplTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a static import for "when". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AaArJ3x7rd49U0dPV8fn&open=AaArJ3x7rd49U0dPV8fn&pullRequest=13953

assertEquals(User.Source.UNKNOWN, invokeGetPreSamlSource(200L));
}

@Test
public void testGetPreSamlSourceDefaultsToUnknownOnGarbageValue() throws Exception {
Mockito.when(userDetailsDao.findDetail(200L, "PreSamlSource")).thenReturn(new UserDetailVO(200L, "PreSamlSource", "not-a-real-source"));

Check warning on line 171 in plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/SAML2AuthManagerImplTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a static import for "when". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AaArJ3x7rd49U0dPV8fo&open=AaArJ3x7rd49U0dPV8fo&pullRequest=13953

assertEquals(User.Source.UNKNOWN, invokeGetPreSamlSource(200L));
}

private User.Source invokeGetPreSamlSource(long userId) throws Exception {
Method method = SAML2AuthManagerImpl.class.getDeclaredMethod("getPreSamlSource", long.class);
method.setAccessible(true);
return (User.Source) method.invoke(saml2AuthManager, userId);
}

@Test
public void testSaveToken() {
Expand Down
Loading