make sure pre saml login is enabled after saml is disabled

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<String, SAMLProviderMetadata> _idpMetadataMap = new HashMap<String, SAMLProviderMetadata>();
 
@@ -115,7 +121,10 @@
     @Inject
     private UserDao _userDao;
 
+    @Inject
+    private UserDetailsDao userDetailsDao;
+
     @Inject
     DomainManager _domainMgr;
 
     @Override
@@ -446,26 +455,52 @@
     @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().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;
+    }
+
+    /**
+     * 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

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 @@
     @Mock
     private UserDao userDao;
 
+    @Mock
+    private UserDetailsDao userDetailsDao;
+
     @Mock
     DomainManager domainMgr;
 
@@ -58,7 +71,7 @@
     @Override
     @Before
     public void setUp() throws NoSuchFieldException, IllegalAccessException {
         saml2AuthManager = Mockito.spy(new SAML2AuthManagerImpl());
 
         Field ksDaoField = SAML2AuthManagerImpl.class.getDeclaredField("_ksDao");
         ksDaoField.setAccessible(true);
@@ -72,14 +85,51 @@
         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);
 
         // enable the plugin
         Mockito.doReturn(true).when(saml2AuthManager).isSAMLPluginEnabled();
     }
 
+    @After
+    public void tearDown() {
+        resetConfigKeyValue(SAML2AuthManager.EnableLoginAfterSAMLDisable);
+    }
+
+    private static <T> void setConfigKeyValue(ConfigKey<T> 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";
@@ -89,60 +139,195 @@
         user.setUsername("someuser");
         user.setSource(User.Source.UNKNOWN);
         user.setExternalEntity(entityID);
         Mockito.when(userDao.getUser(Mockito.anyLong())).thenReturn(user);
         assertFalse(saml2AuthManager.isUserAuthorized(user.getId(), "someID"));
 
         // Test authorized user with wrong IDP
         user.setSource(User.Source.SAML2);
         Mockito.when(userDao.getUser(Mockito.anyLong())).thenReturn(user);
         assertFalse(saml2AuthManager.isUserAuthorized(user.getId(), "someID"));
 
         // Test authorized user with wrong IDP
         user.setSource(User.Source.SAML2);
         Mockito.when(userDao.getUser(Mockito.anyLong())).thenReturn(user);
         assertTrue(saml2AuthManager.isUserAuthorized(user.getId(), entityID));
     }
 
     @Test
     public void testAuthorizeUser() {
         // Test invalid user
         Mockito.when(userDao.getUser(Mockito.anyLong())).thenReturn(null);
         assertFalse(saml2AuthManager.authorizeUser(1L, "someID", true));
 
         // Test valid user
         UserVO user = new UserVO(200L);
         user.setUsername("someuser");
         Mockito.when(userDao.getUser(Mockito.anyLong())).thenReturn(user);
         assertTrue(saml2AuthManager.authorizeUser(1L, "someID", true));
         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 testAuthorizeUserDoesNotRestorePreSamlSourceWhenAlreadyAuthorized() {
+        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() {
         // duplicate token test
         Mockito.when(samlTokenDao.findByUuid(Mockito.anyString())).thenReturn(new SAMLTokenVO());
         saml2AuthManager.saveToken("someAuthnID", null, "https://idp.bhaisaab.org/profile/shibboleth");
         Mockito.verify(samlTokenDao, Mockito.times(0)).persist(Mockito.any(SAMLTokenVO.class));
 
         // valid test
         Mockito.when(samlTokenDao.findByUuid(Mockito.anyString())).thenReturn(null);
         saml2AuthManager.saveToken("someAuthnID", null, "https://idp.bhaisaab.org/profile/shibboleth");
         Mockito.verify(samlTokenDao, Mockito.times(1)).persist(Mockito.any(SAMLTokenVO.class));
     }
 
     @Test
     public void testGetToken() {
         SAMLTokenVO randomToken = new SAMLTokenVO("uuid", 1L, "someIDPDI");
         Mockito.when(samlTokenDao.findByUuid(Mockito.anyString())).thenReturn(randomToken);
         assertEquals(saml2AuthManager.getToken("someAuthnID"), randomToken);
     }
 
     @Test
     public void testExpireToken() {
         saml2AuthManager.expireTokens();
         Mockito.verify(samlTokenDao, Mockito.atLeast(1)).expireTokens();
     }
 
     @Test
@@ -162,7 +347,7 @@
         assertTrue(saml2AuthManager.getAuthCommands().size() > 0);
 
         // Plugin disabled
         Mockito.doReturn(false).when(saml2AuthManager).isSAMLPluginEnabled();
         assertTrue(saml2AuthManager.getCommands().size() == 0);
         assertTrue(saml2AuthManager.getAuthCommands().size() == 0);
         // Re-enable the plugin