From bc42cd18470581e503d40626c6eacbc8aef9284b Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Sat, 22 Aug 2026 21:32:47 +0200 Subject: [PATCH 1/2] make sure pre saml login is enabled after saml is disabled --- .../cloudstack/saml/SAML2AuthManagerImpl.java | 30 +++++++++- .../cloudstack/SAML2AuthManagerImplTest.java | 60 +++++++++++++++++++ 2 files changed, 89 insertions(+), 1 deletion(-) diff --git a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java index 9f8101b867d4..0994571ec84e 100644 --- a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java +++ b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java @@ -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; @@ -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 _idpMetadataMap = new HashMap(); @@ -115,6 +121,9 @@ public String getSAMLIdentityProviderMetadataURL(){ @Inject private UserDao _userDao; + @Inject + private UserDetailsDao userDetailsDao; + @Inject DomainManager _domainMgr; @@ -448,13 +457,16 @@ public boolean authorizeUser(Long userId, String entityId, boolean enable) { 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); } @@ -468,6 +480,22 @@ public boolean authorizeUser(Long userId, String entityId, boolean enable) { 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; diff --git a/plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/SAML2AuthManagerImplTest.java b/plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/SAML2AuthManagerImplTest.java index 81594db1009f..5b06072620b6 100644 --- a/plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/SAML2AuthManagerImplTest.java +++ b/plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/SAML2AuthManagerImplTest.java @@ -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; @@ -50,6 +53,9 @@ public class SAML2AuthManagerImplTest extends TestCase { @Mock private UserDao userDao; + @Mock + private UserDetailsDao userDetailsDao; + @Mock DomainManager domainMgr; @@ -72,6 +78,10 @@ public void setUp() throws NoSuchFieldException, IllegalAccessException { 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); @@ -117,7 +127,57 @@ public void testAuthorizeUser() { 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); + + saml2AuthManager.authorizeUser(200L, "someID", true); + + Mockito.verify(userDetailsDao).addDetail(200L, "PreSamlSource", "LDAP", false); + 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); + + saml2AuthManager.authorizeUser(200L, "someID", true); + + Mockito.verify(userDetailsDao, Mockito.never()).addDetail(Mockito.anyLong(), Mockito.anyString(), Mockito.anyString(), Mockito.anyBoolean()); + } + + @Test + public void testGetPreSamlSourceRestoresStoredSource() throws Exception { + Mockito.when(userDetailsDao.findDetail(200L, "PreSamlSource")).thenReturn(new UserDetailVO(200L, "PreSamlSource", "LDAP")); + + assertEquals(User.Source.LDAP, invokeGetPreSamlSource(200L)); + } + + @Test + public void testGetPreSamlSourceDefaultsToUnknownWhenNothingStored() throws Exception { + Mockito.when(userDetailsDao.findDetail(200L, "PreSamlSource")).thenReturn(null); + + 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")); + + 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() { From 08f943d56a28cf701cee6a225793d04fdcc81ebb Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Tue, 25 Aug 2026 11:58:48 +0200 Subject: [PATCH 2/2] sonarqube --- .../cloudstack/saml/SAML2AuthManagerImpl.java | 51 +++++++++++-------- .../cloudstack/SAML2AuthManagerImplTest.java | 18 ++++--- 2 files changed, 40 insertions(+), 29 deletions(-) diff --git a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java index 0994571ec84e..87b40b393622 100644 --- a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java +++ b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java @@ -455,29 +455,36 @@ public boolean isUserAuthorized(Long userId, String entityId) { @Override public boolean authorizeUser(Long userId, String entityId, boolean enable) { 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(getPreSamlSource(user.getId())); - } else { - user.setSource(User.Source.SAML2DISABLED); - } - } else { - return false; - } - } - _userDao.update(user.getId(), user); - return true; + if (user == null) { + return false; } - return false; + if (enable) { + enableSAMLForUser(user, entityId); + } else if (!disableSAMLForUser(user)) { + return false; + } + _userDao.update(user.getId(), user); + return true; + } + + private void enableSAMLForUser(UserVO user, String entityId) { + 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); + } + + private boolean disableSAMLForUser(UserVO user) { + if (!user.getSource().equals(User.Source.SAML2)) { + return false; + } + if (SAML2AuthManager.EnableLoginAfterSAMLDisable.value()) { + user.setSource(getPreSamlSource(user.getId())); + } else { + user.setSource(User.Source.SAML2DISABLED); + } + return true; } /** diff --git a/plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/SAML2AuthManagerImplTest.java b/plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/SAML2AuthManagerImplTest.java index 5b06072620b6..161b20fcd756 100644 --- a/plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/SAML2AuthManagerImplTest.java +++ b/plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/SAML2AuthManagerImplTest.java @@ -19,6 +19,10 @@ package org.apache.cloudstack; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + import java.lang.reflect.Field; import java.lang.reflect.Method; @@ -132,11 +136,11 @@ public void testAuthorizeUserStoresPreSamlSourceOnEnable() { UserVO user = new UserVO(200L); user.setUsername("someuser"); user.setSource(User.Source.LDAP); - Mockito.when(userDao.getUser(Mockito.anyLong())).thenReturn(user); + when(userDao.getUser(Mockito.anyLong())).thenReturn(user); saml2AuthManager.authorizeUser(200L, "someID", true); - Mockito.verify(userDetailsDao).addDetail(200L, "PreSamlSource", "LDAP", false); + verify(userDetailsDao).addDetail(200L, "PreSamlSource", "LDAP", false); assertEquals(User.Source.SAML2, user.getSource()); } @@ -145,30 +149,30 @@ public void testAuthorizeUserDoesNotRestorePreSamlSourceWhenAlreadyAuthorized() UserVO user = new UserVO(200L); user.setUsername("someuser"); user.setSource(User.Source.SAML2); - Mockito.when(userDao.getUser(Mockito.anyLong())).thenReturn(user); + when(userDao.getUser(Mockito.anyLong())).thenReturn(user); saml2AuthManager.authorizeUser(200L, "someID", true); - Mockito.verify(userDetailsDao, Mockito.never()).addDetail(Mockito.anyLong(), Mockito.anyString(), Mockito.anyString(), Mockito.anyBoolean()); + verify(userDetailsDao, never()).addDetail(Mockito.anyLong(), Mockito.anyString(), Mockito.anyString(), Mockito.anyBoolean()); } @Test public void testGetPreSamlSourceRestoresStoredSource() throws Exception { - Mockito.when(userDetailsDao.findDetail(200L, "PreSamlSource")).thenReturn(new UserDetailVO(200L, "PreSamlSource", "LDAP")); + when(userDetailsDao.findDetail(200L, "PreSamlSource")).thenReturn(new UserDetailVO(200L, "PreSamlSource", "LDAP")); assertEquals(User.Source.LDAP, invokeGetPreSamlSource(200L)); } @Test public void testGetPreSamlSourceDefaultsToUnknownWhenNothingStored() throws Exception { - Mockito.when(userDetailsDao.findDetail(200L, "PreSamlSource")).thenReturn(null); + when(userDetailsDao.findDetail(200L, "PreSamlSource")).thenReturn(null); 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")); + when(userDetailsDao.findDetail(200L, "PreSamlSource")).thenReturn(new UserDetailVO(200L, "PreSamlSource", "not-a-real-source")); assertEquals(User.Source.UNKNOWN, invokeGetPreSamlSource(200L)); }