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..380df4fef7f8 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; @@ -446,26 +455,52 @@ 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) { - 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); - } else { - user.setSource(User.Source.SAML2DISABLED); - } - } else { - return false; - } + if (user == null) { + 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().name(), false); + } + user.setExternalEntity(entityId); + user.setSource(User.Source.SAML2); + } + + private boolean disableSAMLForUser(UserVO user) { + if (!User.Source.SAML2.equals(user.getSource())) { + return false; + } + if (SAML2AuthManager.EnableLoginAfterSAMLDisable.value()) { + user.setSource(getPreSamlSource(user.getId())); + } else { + user.setSource(User.Source.SAML2DISABLED); + } + return true; + } + + /** + * 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); } - _userDao.update(user.getId(), user); - return true; } - return false; + return User.Source.UNKNOWN; } @Override 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..0abe42a231ec 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,12 +19,22 @@ 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; +import org.apache.cloudstack.framework.config.ConfigKey; 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.SAML2AuthManager; import org.apache.cloudstack.saml.SAML2AuthManagerImpl; import org.apache.cloudstack.saml.SAMLTokenDao; import org.apache.cloudstack.saml.SAMLTokenVO; +import org.junit.After; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -50,6 +60,9 @@ public class SAML2AuthManagerImplTest extends TestCase { @Mock private UserDao userDao; + @Mock + private UserDetailsDao userDetailsDao; + @Mock DomainManager domainMgr; @@ -72,6 +85,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); @@ -80,6 +97,39 @@ public void setUp() throws NoSuchFieldException, IllegalAccessException { Mockito.doReturn(true).when(saml2AuthManager).isSAMLPluginEnabled(); } + @After + public void tearDown() { + resetConfigKeyValue(SAML2AuthManager.EnableLoginAfterSAMLDisable); + } + + private static void setConfigKeyValue(ConfigKey configKey, T value) { + try { + Field valueField = ConfigKey.class.getDeclaredField("_value"); + valueField.setAccessible(true); + valueField.set(configKey, value); + + Field dynamicField = ConfigKey.class.getDeclaredField("_isDynamic"); + dynamicField.setAccessible(true); + dynamicField.setBoolean(configKey, false); + } catch (IllegalAccessException | NoSuchFieldException e) { + throw new RuntimeException("Failed to set ConfigKey value", e); + } + } + + private static void resetConfigKeyValue(ConfigKey configKey) { + try { + Field valueField = ConfigKey.class.getDeclaredField("_value"); + valueField.setAccessible(true); + valueField.set(configKey, null); + + Field dynamicField = ConfigKey.class.getDeclaredField("_isDynamic"); + dynamicField.setAccessible(true); + dynamicField.setBoolean(configKey, true); + } catch (IllegalAccessException | NoSuchFieldException e) { + throw new RuntimeException("Failed to reset ConfigKey value", e); + } + } + @Test public void testIsUserAuthorized() { final String entityID = "some IDP ID"; @@ -117,7 +167,142 @@ 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); + when(userDao.getUser(Mockito.anyLong())).thenReturn(user); + + saml2AuthManager.authorizeUser(200L, "someID", true); + + verify(userDetailsDao).addDetail(200L, "PreSamlSource", "LDAP", false); + assertEquals(User.Source.SAML2, user.getSource()); + } + + @Test + public void testAuthorizeUserDoesNotStorePreSamlSourceWhenAlreadySaml() { + UserVO user = new UserVO(200L); + user.setUsername("someuser"); + user.setSource(User.Source.SAML2); + when(userDao.getUser(Mockito.anyLong())).thenReturn(user); + + saml2AuthManager.authorizeUser(200L, "someID", true); + + verify(userDetailsDao, never()).addDetail(Mockito.anyLong(), Mockito.anyString(), Mockito.anyString(), Mockito.anyBoolean()); + } + + @Test + public void testAuthorizeUserEnableWithNullSourceSkipsStore() { + UserVO user = new UserVO(200L); + user.setUsername("someuser"); + user.setSource(null); + when(userDao.getUser(Mockito.anyLong())).thenReturn(user); + + assertTrue(saml2AuthManager.authorizeUser(200L, "someID", true)); + + verify(userDetailsDao, never()).addDetail(Mockito.anyLong(), Mockito.anyString(), Mockito.anyString(), Mockito.anyBoolean()); + assertEquals(User.Source.SAML2, user.getSource()); + } + + @Test + public void testAuthorizeUserEnableSkipsStoreWhenAlreadyDisabled() { + UserVO user = new UserVO(200L); + user.setUsername("someuser"); + user.setSource(User.Source.SAML2DISABLED); + when(userDao.getUser(Mockito.anyLong())).thenReturn(user); + + saml2AuthManager.authorizeUser(200L, "someID", true); + + verify(userDetailsDao, never()).addDetail(Mockito.anyLong(), Mockito.anyString(), Mockito.anyString(), Mockito.anyBoolean()); + assertEquals(User.Source.SAML2, user.getSource()); + } + + @Test + public void testAuthorizeUserDisableReturnsFalseWhenUserNotCurrentlySaml() { + UserVO user = new UserVO(200L); + user.setUsername("someuser"); + user.setSource(User.Source.LDAP); + when(userDao.getUser(Mockito.anyLong())).thenReturn(user); + + assertFalse(saml2AuthManager.authorizeUser(200L, "someID", false)); + assertEquals(User.Source.LDAP, user.getSource()); + Mockito.verify(userDao, never()).update(Mockito.anyLong(), Mockito.any(UserVO.class)); + } + + // With enable.login.with.disabled.saml left at its default (false), disabling SAML must still + // block login via SAML2DISABLED, regardless of any PreSamlSource recorded. + @Test + public void testAuthorizeUserDisableSetsSaml2DisabledWhenFlagOff() { + UserVO user = new UserVO(200L); + user.setUsername("someuser"); + user.setSource(User.Source.SAML2); + when(userDao.getUser(Mockito.anyLong())).thenReturn(user); + + assertTrue(saml2AuthManager.authorizeUser(200L, "someID", false)); + + assertEquals(User.Source.SAML2DISABLED, user.getSource()); + verify(userDetailsDao, never()).findDetail(Mockito.anyLong(), Mockito.anyString()); + } + + @Test + public void testAuthorizeUserDisableRestoresPreSamlSourceWhenFlagOn() { + setConfigKeyValue(SAML2AuthManager.EnableLoginAfterSAMLDisable, true); + + UserVO user = new UserVO(200L); + user.setUsername("someuser"); + user.setSource(User.Source.SAML2); + when(userDao.getUser(Mockito.anyLong())).thenReturn(user); + when(userDetailsDao.findDetail(200L, "PreSamlSource")).thenReturn(new UserDetailVO(200L, "PreSamlSource", "LDAP")); + + assertTrue(saml2AuthManager.authorizeUser(200L, "someID", false)); + + assertEquals(User.Source.LDAP, user.getSource()); + Mockito.verify(userDao, Mockito.atLeastOnce()).update(Mockito.anyLong(), Mockito.any(UserVO.class)); + } + + @Test + public void testAuthorizeUserDisableFallsBackToUnknownWhenFlagOnButNothingStored() { + setConfigKeyValue(SAML2AuthManager.EnableLoginAfterSAMLDisable, true); + + UserVO user = new UserVO(200L); + user.setUsername("someuser"); + user.setSource(User.Source.SAML2); + when(userDao.getUser(Mockito.anyLong())).thenReturn(user); + when(userDetailsDao.findDetail(200L, "PreSamlSource")).thenReturn(null); + + assertTrue(saml2AuthManager.authorizeUser(200L, "someID", false)); + + assertEquals(User.Source.UNKNOWN, user.getSource()); + } + + @Test + public void testGetPreSamlSourceRestoresStoredSource() throws Exception { + when(userDetailsDao.findDetail(200L, "PreSamlSource")).thenReturn(new UserDetailVO(200L, "PreSamlSource", "LDAP")); + + assertEquals(User.Source.LDAP, invokeGetPreSamlSource(200L)); + } + + @Test + public void testGetPreSamlSourceDefaultsToUnknownWhenNothingStored() throws Exception { + when(userDetailsDao.findDetail(200L, "PreSamlSource")).thenReturn(null); + + assertEquals(User.Source.UNKNOWN, invokeGetPreSamlSource(200L)); + } + + @Test + public void testGetPreSamlSourceDefaultsToUnknownOnGarbageValue() throws Exception { + 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() {