Enforce oauth2.plugins.exclude live, with domain-level scoping

plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/OAuth2AuthManager.java

@@ -40,7 +40,8 @@ public interface OAuth2AuthManager extends PluggableAPIAuthenticator, PluggableS
     public static final ConfigKey<String> OAuth2Plugins = new ConfigKey<String>("Advanced", String.class, "oauth2.plugins", "google,github",
             "List of OAuth plugins", true);
     public static final ConfigKey<String> OAuth2PluginsExclude = new ConfigKey<String>("Advanced", String.class, "oauth2.plugins.exclude", "",
-            "List of OAuth plugins which are excluded", true);
+            "List of OAuth plugins which are excluded. Can also be set at the domain level to exclude further providers on top of whatever the global list already excludes.",
+            true, ConfigKey.Scope.Domain).withStrictScope();
 
     /**
      * Lists user OAuth2 provider plugins
@@ -49,12 +50,21 @@ public interface OAuth2AuthManager extends PluggableAPIAuthenticator, PluggableS
     List<UserOAuth2Authenticator> listUserOAuth2AuthenticationProviders();
 
     /**
-     * Finds user OAuth2 provider by name
+     * Finds user OAuth2 provider by name. Equivalent to {@link #getUserOAuth2AuthenticationProvider(String, Long)}
+     * with a null domainId, so only the global oauth2.plugins.exclude list applies.
      * @param providerName name of the provider
      * @return OAuth2 provider
      */
     UserOAuth2Authenticator getUserOAuth2AuthenticationProvider(final String providerName);
 
+    /**
+     * Finds user OAuth2 provider by name, rejecting it if it's excluded either globally or for the given domain.
+     * @param providerName name of the provider
+     * @param domainId domain the login attempt is scoped to, or null for the global scope
+     * @return OAuth2 provider
+     */
+    UserOAuth2Authenticator getUserOAuth2AuthenticationProvider(final String providerName, final Long domainId);
+
     String verifySecretCodeAndFetchEmail(String code, String provider, Long domainId);
 
     OauthProviderVO registerOauthProvider(RegisterOAuthProviderCmd cmd);

plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/OAuth2AuthManagerImpl.java

@@ -120,20 +120,61 @@ public List<Class<?>> getCommands() {
 
     @Override
     public List<UserOAuth2Authenticator> listUserOAuth2AuthenticationProviders() {
-        return userOAuth2AuthenticationProviders;
+        if (userOAuth2AuthenticationProviders == null) {
+            return userOAuth2AuthenticationProviders;
+        }
+        List<UserOAuth2Authenticator> notExcluded = new ArrayList<>();
+        for (UserOAuth2Authenticator provider : userOAuth2AuthenticationProviders) {
+            if (!isProviderExcluded(provider.getName(), null)) {
+                notExcluded.add(provider);
+            }
+        }
+        return notExcluded;
     }
 
     @Override
     public UserOAuth2Authenticator getUserOAuth2AuthenticationProvider(String providerName) {
+        return getUserOAuth2AuthenticationProvider(providerName, null);
+    }
+
+    @Override
+    public UserOAuth2Authenticator getUserOAuth2AuthenticationProvider(String providerName, Long domainId) {
         if (StringUtils.isEmpty(providerName)) {
             throw new CloudRuntimeException("OAuth2 authentication provider name is empty");
         }
-        if (!userOAuth2AuthenticationProvidersMap.containsKey(providerName.toLowerCase())) {
+        if (!userOAuth2AuthenticationProvidersMap.containsKey(providerName.toLowerCase()) || isProviderExcluded(providerName, domainId)) {
             throw new CloudRuntimeException(String.format("Failed to find OAuth2 authentication provider by the name: %s.", providerName));
         }
         return userOAuth2AuthenticationProvidersMap.get(providerName.toLowerCase());
     }
 
+    // oauth2.plugins.exclude is checked live here rather than relying on the registry's
+    // registration-time filtering, so a config change takes effect without a restart and so a
+    // domain-level override can be layered on top of the global list. A domain can only add
+    // further exclusions, never un-exclude something the global list already excludes.
+    protected boolean isProviderExcluded(String providerName, Long domainId) {
+        if (isExcludedByList(providerName, OAuth2AuthManager.OAuth2PluginsExclude.value())) {
+            return true;
+        }
+        if (domainId == null) {
+            return false;
+        }
+        String domainExcludeList = OAuth2AuthManager.OAuth2PluginsExclude.valueInScope(ConfigKey.Scope.Domain, domainId, true);
+        return isExcludedByList(providerName, domainExcludeList);
+    }
+
+    protected boolean isExcludedByList(String providerName, String excludeList) {
+        if (StringUtils.isEmpty(excludeList)) {
+            return false;
+        }
+        for (String excluded : excludeList.trim().split("\\s*,\\s*")) {
+            if (excluded.equalsIgnoreCase(providerName)) {
+                return true;
+            }
+        }
+        return false;
+    }
+
     public List<UserOAuth2Authenticator> getUserOAuth2AuthenticationProviders() {
         return userOAuth2AuthenticationProviders;
     }
@@ -152,7 +193,7 @@ protected void initializeUserOAuth2AuthenticationProvidersMap() {
 
     @Override
     public String verifySecretCodeAndFetchEmail(String code, String provider, Long domainId) {
-        UserOAuth2Authenticator authenticator = getUserOAuth2AuthenticationProvider(provider);
+        UserOAuth2Authenticator authenticator = getUserOAuth2AuthenticationProvider(provider, domainId);
         String email = authenticator.verifySecretCodeAndFetchEmail(code, domainId);
 
         return email;

plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/OAuth2UserAuthenticator.java

@@ -74,7 +74,7 @@ public Pair<Boolean, ActionOnFailedAuthentication> authenticate(String username,
             String email = ((emailArray == null) ? null : emailArray[0]);
             String secretCode = ((secretCodeArray == null) ? null : secretCodeArray[0]);
 
-            UserOAuth2Authenticator authenticator = userOAuth2mgr.getUserOAuth2AuthenticationProvider(oauthProvider);
+            UserOAuth2Authenticator authenticator = userOAuth2mgr.getUserOAuth2AuthenticationProvider(oauthProvider, domainId);
             if (Objects.nonNull(user) && authenticator.verifyUser(email, secretCode, domainId)) {
                 return new Pair<Boolean, ActionOnFailedAuthentication>(true, null);
             }

plugins/user-authenticators/oauth2/src/test/java/org/apache/cloudstack/oauth2/OAuth2AuthManagerImplTest.java

@@ -24,6 +24,7 @@
 import com.cloud.user.DomainService;
 import com.cloud.utils.exception.CloudRuntimeException;
 import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.auth.UserOAuth2Authenticator;
 import org.apache.cloudstack.framework.messagebus.MessageBus;
 import org.apache.cloudstack.framework.messagebus.MessageSubscriber;
 import org.apache.cloudstack.oauth2.api.command.DeleteOAuthProviderCmd;
@@ -49,8 +50,12 @@
 import java.util.Map;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.ArgumentMatchers.isNull;
 import static org.mockito.Mockito.doNothing;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
@@ -525,6 +530,90 @@ public void testGetUserOAuth2AuthenticationProviderNotFound() {
         }
     }
 
+    @Test
+    public void testGetUserOAuth2AuthenticationProviderRejectsExcludedProvider() {
+        UserOAuth2Authenticator googleAuthenticator = Mockito.mock(UserOAuth2Authenticator.class);
+        OAuth2AuthManagerImpl.userOAuth2AuthenticationProvidersMap.put("google", googleAuthenticator);
+        try {
+            Mockito.doReturn(true).when(_authManager).isProviderExcluded("google", 5L);
+
+            try {
+                _authManager.getUserOAuth2AuthenticationProvider("google", 5L);
+                Assert.fail("Expected CloudRuntimeException was not thrown");
+            } catch (CloudRuntimeException e) {
+                assertTrue(e.getMessage().contains("google"));
+            }
+        } finally {
+            OAuth2AuthManagerImpl.userOAuth2AuthenticationProvidersMap.remove("google");
+        }
+    }
+
+    @Test
+    public void testGetUserOAuth2AuthenticationProviderSingleArgOverloadUsesGlobalScope() {
+        UserOAuth2Authenticator githubAuthenticator = Mockito.mock(UserOAuth2Authenticator.class);
+        OAuth2AuthManagerImpl.userOAuth2AuthenticationProvidersMap.put("github", githubAuthenticator);
+        try {
+            Mockito.doReturn(false).when(_authManager).isProviderExcluded("github", null);
+
+            UserOAuth2Authenticator result = _authManager.getUserOAuth2AuthenticationProvider("github");
+
+            assertEquals(githubAuthenticator, result);
+            verify(_authManager).isProviderExcluded("github", null);
+        } finally {
+            OAuth2AuthManagerImpl.userOAuth2AuthenticationProvidersMap.remove("github");
+        }
+    }
+
+    @Test
+    public void testListUserOAuth2AuthenticationProvidersFiltersOutExcludedPlugins() {
+        UserOAuth2Authenticator googleAuthenticator = Mockito.mock(UserOAuth2Authenticator.class);
+        when(googleAuthenticator.getName()).thenReturn("google");
+        UserOAuth2Authenticator keycloakAuthenticator = Mockito.mock(UserOAuth2Authenticator.class);
+        when(keycloakAuthenticator.getName()).thenReturn("keycloak");
+        _authManager.setUserOAuth2AuthenticationProviders(Arrays.asList(googleAuthenticator, keycloakAuthenticator));
+
+        Mockito.doReturn(false).when(_authManager).isProviderExcluded("google", null);
+        Mockito.doReturn(true).when(_authManager).isProviderExcluded("keycloak", null);
+
+        List<UserOAuth2Authenticator> result = _authManager.listUserOAuth2AuthenticationProviders();
+
+        assertEquals(1, result.size());
+        assertEquals("google", result.get(0).getName());
+    }
+
+    @Test
+    public void testIsExcludedByListMatchesCaseInsensitivelyAndTreatsEmptyAsNoExclusions() {
+        assertFalse(_authManager.isExcludedByList("google", null));
+        assertFalse(_authManager.isExcludedByList("google", ""));
+        assertTrue(_authManager.isExcludedByList("google", "google,github"));
+        assertTrue(_authManager.isExcludedByList("google", "Google"));
+        assertFalse(_authManager.isExcludedByList("keycloak", "google,github"));
+    }
+
+    @Test
+    public void testIsProviderExcludedShortCircuitsOnGlobalExclusion() {
+        // isProviderExcluded's first check is always against the global list (OAuth2PluginsExclude.value(),
+        // which is "" by default with no ConfigDepot wired) — stub that call regardless of exact value
+        Mockito.doReturn(true).when(_authManager).isExcludedByList(eq("google"), any());
+        assertTrue(_authManager.isProviderExcluded("google", 5L));
+    }
+
+    @Test
+    public void testIsProviderExcludedFallsBackToDomainScopeWhenNotGloballyExcluded() {
+        // no ConfigDepot is wired, so the global value is "" and the domain-scope lookup returns null;
+        // stub isExcludedByList for exactly those inputs so only the union logic itself is under test
+        Mockito.doReturn(false).when(_authManager).isExcludedByList(eq("google"), eq(""));
+        Mockito.doReturn(true).when(_authManager).isExcludedByList(eq("google"), isNull());
+        assertTrue(_authManager.isProviderExcluded("google", 5L));
+    }
+
+    @Test
+    public void testIsProviderExcludedSkipsDomainCheckWhenDomainIdIsNull() {
+        Mockito.doReturn(false).when(_authManager).isExcludedByList(eq("google"), eq(""));
+        assertFalse(_authManager.isProviderExcluded("google", null));
+        Mockito.verify(_authManager, Mockito.never()).isExcludedByList(eq("google"), isNull());
+    }
+
     //  Multiple-domain OAuth tests
 
     @Test

plugins/user-authenticators/oauth2/src/test/java/org/apache/cloudstack/oauth2/OAuth2UserAuthenticatorTest.java

@@ -41,6 +41,7 @@
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyLong;
 import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.Mockito.mock;
@@ -92,7 +93,7 @@ public void testAuthenticateWithValidCredentials() {
 
         when(userAccountDao.getUserAccount(username, domainId)).thenReturn(userAccount);
         when(userDao.getUser(userAccount.getId())).thenReturn(user);
-        when(userOAuth2mgr.getUserOAuth2AuthenticationProvider(provider[0])).thenReturn(userOAuth2Authenticator);
+        when(userOAuth2mgr.getUserOAuth2AuthenticationProvider(provider[0], domainId)).thenReturn(userOAuth2Authenticator);
         when(userOAuth2Authenticator.verifyUser(email[0], secretCode[0], domainId)).thenReturn(true);
 
         Map<String, Object[]> requestParameters = new HashMap<>();
@@ -107,7 +108,7 @@ public void testAuthenticateWithValidCredentials() {
 
         verify(userAccountDao).getUserAccount(username, domainId);
         verify(userDao).getUser(userAccount.getId());
-        verify(userOAuth2mgr).getUserOAuth2AuthenticationProvider(provider[0]);
+        verify(userOAuth2mgr).getUserOAuth2AuthenticationProvider(provider[0], domainId);
         verify(userOAuth2Authenticator).verifyUser(email[0], secretCode[0], domainId);
     }
 
@@ -125,7 +126,7 @@ public void testAuthenticateWithInvalidCredentials() {
 
         when(userAccountDao.getUserAccount(username, domainId)).thenReturn(userAccount);
         when(userDao.getUser(userAccount.getId())).thenReturn(user);
-        when(userOAuth2mgr.getUserOAuth2AuthenticationProvider(provider[0])).thenReturn(userOAuth2Authenticator);
+        when(userOAuth2mgr.getUserOAuth2AuthenticationProvider(provider[0], domainId)).thenReturn(userOAuth2Authenticator);
         when(userOAuth2Authenticator.verifyUser(email[0], secretCode[0], domainId)).thenReturn(false);
 
         Map<String, Object[]> requestParameters = new HashMap<>();
@@ -140,7 +141,7 @@ public void testAuthenticateWithInvalidCredentials() {
 
         verify(userAccountDao).getUserAccount(username, domainId);
         verify(userDao).getUser(userAccount.getId());
-        verify(userOAuth2mgr).getUserOAuth2AuthenticationProvider(provider[0]);
+        verify(userOAuth2mgr).getUserOAuth2AuthenticationProvider(provider[0], domainId);
         verify(userOAuth2Authenticator).verifyUser(email[0], secretCode[0], domainId);
     }
 
@@ -166,7 +167,7 @@ public void testAuthenticateWithInvalidUserAccount() {
 
         verify(userAccountDao).getUserAccount(username, domainId);
         verify(userDao, never()).getUser(anyLong());
-        verify(userOAuth2mgr, never()).getUserOAuth2AuthenticationProvider(anyString());
+        verify(userOAuth2mgr, never()).getUserOAuth2AuthenticationProvider(anyString(), any());
     }
 
     @Test
@@ -210,6 +211,6 @@ public void testAuthenticateNullProvider() {
 
         assertFalse(result.first());
         assertNull(result.second());
-        verify(userOAuth2mgr, never()).getUserOAuth2AuthenticationProvider(anyString());
+        verify(userOAuth2mgr, never()).getUserOAuth2AuthenticationProvider(anyString(), any());
     }
 }