diff --git a/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/OAuth2AuthManager.java b/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/OAuth2AuthManager.java index 133131d3928a..a35a3b277a5a 100644 --- a/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/OAuth2AuthManager.java +++ b/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 OAuth2Plugins = new ConfigKey("Advanced", String.class, "oauth2.plugins", "google,github", "List of OAuth plugins", true); public static final ConfigKey OAuth2PluginsExclude = new ConfigKey("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 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); diff --git a/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/OAuth2AuthManagerImpl.java b/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/OAuth2AuthManagerImpl.java index c3bad43be40e..ae644edc4425 100644 --- a/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/OAuth2AuthManagerImpl.java +++ b/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/OAuth2AuthManagerImpl.java @@ -120,20 +120,61 @@ public List> getCommands() { @Override public List listUserOAuth2AuthenticationProviders() { - return userOAuth2AuthenticationProviders; + if (userOAuth2AuthenticationProviders == null) { + return userOAuth2AuthenticationProviders; + } + List 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 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; diff --git a/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/OAuth2UserAuthenticator.java b/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/OAuth2UserAuthenticator.java index 49df94709836..e3ef116d883a 100644 --- a/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/OAuth2UserAuthenticator.java +++ b/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/OAuth2UserAuthenticator.java @@ -74,7 +74,7 @@ public Pair 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(true, null); } diff --git a/plugins/user-authenticators/oauth2/src/test/java/org/apache/cloudstack/oauth2/OAuth2AuthManagerImplTest.java b/plugins/user-authenticators/oauth2/src/test/java/org/apache/cloudstack/oauth2/OAuth2AuthManagerImplTest.java index e3e8f7594b37..b02e410837be 100644 --- a/plugins/user-authenticators/oauth2/src/test/java/org/apache/cloudstack/oauth2/OAuth2AuthManagerImplTest.java +++ b/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 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 diff --git a/plugins/user-authenticators/oauth2/src/test/java/org/apache/cloudstack/oauth2/OAuth2UserAuthenticatorTest.java b/plugins/user-authenticators/oauth2/src/test/java/org/apache/cloudstack/oauth2/OAuth2UserAuthenticatorTest.java index 1351c1ea4791..fe0704a31cfa 100644 --- a/plugins/user-authenticators/oauth2/src/test/java/org/apache/cloudstack/oauth2/OAuth2UserAuthenticatorTest.java +++ b/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 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 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()); } }