diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index c6ee5e588..447bb815e 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -72,6 +72,8 @@ jobs: OSGI-INF/org.forgerock.openidm.identityProviders.xml | grep -q 'bind="bindIdentityProviderConfig"' unzip -p openidm-authnfilter/target/openidm-authnfilter-*[0-9T].jar \ OSGI-INF/org.forgerock.openidm.authentication.xml | grep -q 'bind="bindIdentityProviderService"' + unzip -p openidm-selfservice/target/openidm-selfservice-*[0-9T].jar \ + OSGI-INF/org.forgerock.openidm.selfservice.xml | grep -q 'bind="bindIdentityProviderService"' - name: Test on Unix if: runner.os != 'Windows' run: | diff --git a/openidm-authnfilter/src/main/java/org/forgerock/openidm/auth/AuthenticationService.java b/openidm-authnfilter/src/main/java/org/forgerock/openidm/auth/AuthenticationService.java index 12bcd3835..b643b41c3 100644 --- a/openidm-authnfilter/src/main/java/org/forgerock/openidm/auth/AuthenticationService.java +++ b/openidm-authnfilter/src/main/java/org/forgerock/openidm/auth/AuthenticationService.java @@ -250,7 +250,7 @@ public class AuthenticationService implements SingletonResourceProvider, Identit policy = ReferencePolicy.DYNAMIC, cardinality = ReferenceCardinality.OPTIONAL, unbind = "unbindIdentityProviderService") - void bindIdentityProviderService(IdentityProviderService identityProviderService) + synchronized void bindIdentityProviderService(IdentityProviderService identityProviderService) throws IdentityProviderServiceException { this.identityProviderService = identityProviderService; identityProviderService.registerIdentityProviderListener(this); @@ -258,7 +258,7 @@ void bindIdentityProviderService(IdentityProviderService identityProviderService identityProviderConfigChanged(); } - void unbindIdentityProviderService(IdentityProviderService identityProviderService) + synchronized void unbindIdentityProviderService(IdentityProviderService identityProviderService) throws IdentityProviderServiceException { identityProviderService.unregisterIdentityProviderListener(this); if (this.identityProviderService == identityProviderService) { @@ -312,8 +312,10 @@ public JsonValue apply(JsonValue value) { new Predicate() { @Override public boolean apply(JsonValue jsonValue) { - return jsonValue.get(AUTH_MODULE_NAME_KEY).asString().equals(IDMAuthModule.OPENID_CONNECT.name()) - || jsonValue.get(AUTH_MODULE_NAME_KEY).asString().equals(IDMAuthModule.OAUTH.name()); + // a module configured by className alone has no name; the rebuild runs this on every module + final String name = jsonValue.get(AUTH_MODULE_NAME_KEY).asString(); + return IDMAuthModule.OPENID_CONNECT.name().equals(name) + || IDMAuthModule.OAUTH.name().equals(name); } }; @@ -346,7 +348,7 @@ public Map apply(JsonValue jsonValue) { private static final Function resolvers = new Function() { @Override public JsonValue apply(JsonValue jsonValue) { - setType.apply(jsonValue); + // the resolver type is set during the rebuild: request threads must not write the published config return jsonValue.get(AUTH_MODULE_PROPERTIES_KEY).get(AUTH_MODULE_RESOLVERS_KEY); } }; @@ -375,8 +377,9 @@ public boolean apply(JsonValue jsonValue) { @Override public JsonValue apply(JsonValue jsonValue) { final JsonValue resolvers = jsonValue.get(AUTH_MODULE_PROPERTIES_KEY).get(AUTH_MODULE_RESOLVERS_KEY); - if (resolvers.isNotNull()) { - // currently we only support one resolver per auth module + // currently we only support one resolver per auth module; this runs during the rebuild, after the + // filter is set, on disabled modules too, so a malformed resolver must not fail it + if (resolvers.isList() && resolvers.size() > 0 && resolvers.get(0).isMap()) { return resolvers.get(0).put("type", jsonValue.get(AUTH_MODULE_NAME_KEY).asString()); } // return with no modification @@ -530,17 +533,24 @@ public synchronized void identityProviderConfigChanged() throws IdentityProvider throw new IdentityProviderServiceException(e.getMessage(), e); } - // this now runs on DS bind threads while request threads read both fields without a lock, - // so publish complete values only - amendedConfig = newAmendedConfig; // filter enabled module configs and get their properties; // then filter those with valid auth properties, and build an authenticator - authenticators = FluentIterable.from(authModuleConfig) + final List newAuthenticators = FluentIterable.from(authModuleConfig) .filter(enabledAuthModules) .transform(toModuleProperties) .filter(authModulesThatHaveValidAuthenticatorProperties) .transform(toAuthenticatorFromProperties) .toList(); + // readInstance and getIdentityProviderConfig report the resolver type; set it here, once, + // so that request threads only read the published config + for (final JsonValue authModule : FluentIterable.from(authModuleConfig).filter(oidcAndOauth2Modules)) { + setType.apply(authModule); + } + + // this now runs on DS bind threads while request threads read both fields without a lock, + // so publish complete values only + amendedConfig = newAmendedConfig; + authenticators = newAuthenticators; } /** @@ -553,7 +563,14 @@ public synchronized void activate(final ComponentContext context) throws AuthenticationException, IdentityProviderServiceException { logger.info("Activating Authentication Service with configuration {}", context.getProperties()); config = enhancedConfig.getConfigurationAsJson(context); - identityProviderConfigChanged(); + try { + identityProviderConfigChanged(); + } catch (IdentityProviderServiceException | RuntimeException e) { + // DS calls no deactivate after a failed activate, but it still unbinds the references, and + // unbindIdentityProviderService rebuilds: leave this instance no configuration to rebuild from + config = null; + throw e; + } logger.debug("OpenIDM Config for Authentication {} is activated.", config.get(Constants.SERVICE_PID)); } @@ -586,7 +603,7 @@ public synchronized void deactivate(ComponentContext context) { * @throws AuthenticationException on missing or incorrect configuration, or failure to construct an auth module * from the config */ - private Filter configureAuthenticationFilter(JsonValue jsonConfig) throws AuthenticationException { + Filter configureAuthenticationFilter(JsonValue jsonConfig) throws AuthenticationException { if (jsonConfig == null || jsonConfig.size() == 0) { throw new AuthenticationException("No auth modules configured"); } @@ -816,7 +833,6 @@ private ProviderConfig getIdentityProviderConfig(final String providerName) thro .transformAndConcat(resolvers) .filter(enabledResolvers) .filter(forProvider(providerName)) - .transform(setType) .transform(ProviderConfigMapper.toProviderConfig) .first(); diff --git a/openidm-authnfilter/src/test/java/org/forgerock/openidm/auth/AuthenticationServiceTest.java b/openidm-authnfilter/src/test/java/org/forgerock/openidm/auth/AuthenticationServiceTest.java index 5ba518975..f495c1330 100644 --- a/openidm-authnfilter/src/test/java/org/forgerock/openidm/auth/AuthenticationServiceTest.java +++ b/openidm-authnfilter/src/test/java/org/forgerock/openidm/auth/AuthenticationServiceTest.java @@ -21,14 +21,20 @@ import static org.forgerock.json.resource.Requests.newActionRequest; import static org.forgerock.json.resource.Requests.newReadRequest; import static org.forgerock.openidm.auth.AuthenticationService.Action; +import static org.assertj.core.api.Assertions.fail; +import static org.mockito.Matchers.any; import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import javax.security.auth.message.MessageInfo; +import java.lang.reflect.Field; import java.util.ArrayList; import java.util.List; import java.util.Map; @@ -37,6 +43,8 @@ import com.fasterxml.jackson.core.JsonParser; import com.fasterxml.jackson.databind.DeserializationFeature; import com.fasterxml.jackson.databind.ObjectMapper; +import org.forgerock.caf.authentication.api.AuthenticationException; +import org.forgerock.http.Filter; import org.forgerock.jaspi.modules.session.jwt.JwtSessionModule; import org.forgerock.json.JsonPointer; import org.forgerock.json.JsonValue; @@ -45,13 +53,16 @@ import org.forgerock.json.resource.ResourceException; import org.forgerock.json.resource.ResourcePath; import org.forgerock.json.resource.ResourceResponse; +import org.forgerock.openidm.config.enhanced.EnhancedConfig; import org.forgerock.openidm.idp.config.ProviderConfig; import org.forgerock.openidm.idp.impl.IdentityProviderService; +import org.forgerock.openidm.idp.impl.IdentityProviderServiceException; import org.forgerock.openidm.idp.impl.ProviderConfigMapper; import org.forgerock.services.context.AttributesContext; import org.forgerock.services.context.RootContext; import org.forgerock.util.promise.Promise; import org.forgerock.util.test.assertj.AssertJPromiseAssert; +import org.osgi.service.component.ComponentContext; import org.testng.annotations.AfterMethod; import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; @@ -310,6 +321,197 @@ public void amendAuthConfigShouldSkipProvidersOfUnsupportedType() throws Excepti assertThat(authenticationJson.get(AUTH_MODULES).get(1).get("name").asString()).isEqualTo(OPENID_CONNECT); } + @Test + public void identityProviderConfigChangedShouldPublishOnlyAfterTheFilterIsSet() throws Exception { + final AuthenticationService service = spy(new AuthenticationService()); + doReturn(mock(Filter.class)).when(service).configureAuthenticationFilter(any(JsonValue.class)); + final AuthFilterWrapper authFilterWrapper = mock(AuthFilterWrapper.class); + setField(service, "authFilterWrapper", authFilterWrapper); + final IdentityProviderService identityProviderService = mock(IdentityProviderService.class); + final List providerConfigs = new ArrayList<>(); + providerConfigs.add(ProviderConfigMapper.toProviderConfig(googleIdentityProvider)); + when(identityProviderService.getIdentityProviders()).thenReturn(providerConfigs); + // an enabled stand-alone module whose resolver does not carry its type + final JsonValue explicitModule = authenticationJson.get(AUTH_MODULES).get(1); + explicitModule.put("enabled", true); + explicitModule.get("properties").get("resolvers").get(0).remove("type"); + + service.bindIdentityProviderService(identityProviderService); + service.setConfig(authenticationJson); + service.identityProviderConfigChanged(); + // the type is in the published config before any request reads it + final JsonValue first = (JsonValue) getField(service, "amendedConfig"); + for (final JsonValue module : first.get(AUTH_MODULES)) { + if (OPENID_CONNECT.equals(module.get("name").asString())) { + assertThat(module.get("properties").get("resolvers").get(0).get("type").asString()) + .isEqualTo(OPENID_CONNECT); + } + } + final Object snapshot = first.copy().getObject(); + // a second rebuild must not modify the values the first one published + service.identityProviderConfigChanged(); + assertThat(getField(service, "amendedConfig")).isNotSameAs(first); + assertThat(first.getObject()).isEqualTo(snapshot); + + assertProviders(service, "explicit-oidc", "oidc"); + + // a rebuild whose filter is not set leaves the published config describing the filter in force + providerConfigs.add(ProviderConfigMapper.toProviderConfig(json(OBJECT_MAPPER.readValue( + getClass().getResource("/config/identityProvider-oauth.json"), Map.class)))); + doThrow(new IllegalStateException("filter not set")).when(authFilterWrapper).setFilter(any(Filter.class)); + try { + service.identityProviderConfigChanged(); + fail("Expected IllegalStateException"); + } catch (IllegalStateException e) { + assertThat(e).hasMessage("filter not set"); + } + + assertProviders(service, "explicit-oidc", "oidc"); + } + + @Test + public void failedActivationShouldLeaveNothingForUnbindToRebuild() throws Exception { + final AuthenticationService service = spy(new AuthenticationService()); + // the first filter, at activation, cannot be built; a later one could + doThrow(new AuthenticationException("invalid module")) + .doReturn(mock(Filter.class)) + .when(service).configureAuthenticationFilter(any(JsonValue.class)); + final AuthFilterWrapper authFilterWrapper = mock(AuthFilterWrapper.class); + setField(service, "authFilterWrapper", authFilterWrapper); + final EnhancedConfig enhancedConfig = mock(EnhancedConfig.class); + when(enhancedConfig.getConfigurationAsJson(any(ComponentContext.class))).thenReturn(authenticationJson); + setField(service, "enhancedConfig", enhancedConfig); + final IdentityProviderService identityProviderService = mock(IdentityProviderService.class); + + service.bindIdentityProviderService(identityProviderService); + try { + service.activate(mock(ComponentContext.class)); + fail("Expected IdentityProviderServiceException"); + } catch (IdentityProviderServiceException e) { + assertThat(e.getCause()).isInstanceOf(AuthenticationException.class); + } + // DS unbinds the references of a component whose activation failed + service.unbindIdentityProviderService(identityProviderService); + + verify(authFilterWrapper, never()).setFilter(any(Filter.class)); + } + + @Test + public void failedActivationOnARuntimeExceptionShouldLeaveNothingForUnbindToRebuild() throws Exception { + final AuthenticationService service = spy(new AuthenticationService()); + doThrow(new IllegalStateException("invalid module")) + .doReturn(mock(Filter.class)) + .when(service).configureAuthenticationFilter(any(JsonValue.class)); + final AuthFilterWrapper authFilterWrapper = mock(AuthFilterWrapper.class); + setField(service, "authFilterWrapper", authFilterWrapper); + final EnhancedConfig enhancedConfig = mock(EnhancedConfig.class); + when(enhancedConfig.getConfigurationAsJson(any(ComponentContext.class))).thenReturn(authenticationJson); + setField(service, "enhancedConfig", enhancedConfig); + final IdentityProviderService identityProviderService = mock(IdentityProviderService.class); + + service.bindIdentityProviderService(identityProviderService); + try { + service.activate(mock(ComponentContext.class)); + fail("Expected IllegalStateException"); + } catch (IllegalStateException e) { + assertThat(e).hasMessage("invalid module"); + } + // DS unbinds the references of a component whose activation failed + service.unbindIdentityProviderService(identityProviderService); + + verify(authFilterWrapper, never()).setFilter(any(Filter.class)); + } + + @Test + public void activationShouldTolerateMalformedResolversOfDisabledModules() throws Exception { + for (final Object malformed : new Object[] { array(), object(), array("not a resolver") }) { + final AuthenticationService service = spy(new AuthenticationService()); + doReturn(mock(Filter.class)).when(service).configureAuthenticationFilter(any(JsonValue.class)); + final AuthFilterWrapper authFilterWrapper = mock(AuthFilterWrapper.class); + setField(service, "authFilterWrapper", authFilterWrapper); + final JsonValue config = authenticationJson.copy(); + config.get(AUTH_MODULES).add(object( + field("name", OPENID_CONNECT), + field("enabled", false), + field("properties", object(field("resolvers", malformed))))); + final EnhancedConfig enhancedConfig = mock(EnhancedConfig.class); + when(enhancedConfig.getConfigurationAsJson(any(ComponentContext.class))).thenReturn(config); + setField(service, "enhancedConfig", enhancedConfig); + + // the rebuild sets the resolver type after the filter is set: it must not fail the activation + service.activate(mock(ComponentContext.class)); + + verify(authFilterWrapper).setFilter(any(Filter.class)); + // both OPENID_CONNECT modules are disabled + assertProviders(service); + } + } + + @Test + public void activationShouldTolerateAModuleConfiguredByClassNameOnly() throws Exception { + final AuthenticationService service = spy(new AuthenticationService()); + doReturn(mock(Filter.class)).when(service).configureAuthenticationFilter(any(JsonValue.class)); + final AuthFilterWrapper authFilterWrapper = mock(AuthFilterWrapper.class); + setField(service, "authFilterWrapper", authFilterWrapper); + // a module without a name, after the enabled SOCIAL_PROVIDERS template + final JsonValue config = authenticationJson.copy(); + config.get(AUTH_MODULES).add(object(field("className", "com.example.CustomAuthModule"))); + final EnhancedConfig enhancedConfig = mock(EnhancedConfig.class); + when(enhancedConfig.getConfigurationAsJson(any(ComponentContext.class))).thenReturn(config); + setField(service, "enhancedConfig", enhancedConfig); + + // the rebuild sets the resolver type of every OPENID_CONNECT/OAUTH module after the filter is set + service.activate(mock(ComponentContext.class)); + + verify(authFilterWrapper).setFilter(any(Filter.class)); + // the read skips the nameless module as well + assertProviders(service); + } + + @Test + public void readInstanceShouldNotWriteTheResolverTypeIntoThePublishedConfig() throws Exception { + final AuthenticationService service = new AuthenticationService(); + final JsonValue published = amendedAuthentication.copy(); + for (final JsonValue module : published.get(AUTH_MODULES)) { + module.get("properties").get("resolvers").get(0).remove("type"); + } + service.setConfig(published); + service.setAmendedConfig(published); + + service.readInstance(new RootContext(), newReadRequest(AUTHENTICATION_PATH)).get(); + + // the read covers the enabled OAUTH and OPENID_CONNECT modules; it must leave their resolvers as published + final JsonValue afterRead = (JsonValue) getField(service, "amendedConfig"); + for (final JsonValue module : afterRead.get(AUTH_MODULES)) { + assertThat(module.get("properties").get("resolvers").get(0).isDefined("type")).isFalse(); + } + } + + private static void assertProviders(final AuthenticationService service, final String... names) + throws Exception { + final JsonValue providers = service.readInstance(new RootContext(), newReadRequest(AUTHENTICATION_PATH)) + .get().getContent().get(IdentityProviderService.PROVIDERS); + assertThat(providers.size()).isEqualTo(names.length); + for (int i = 0; i < names.length; i++) { + assertThat(providers.get(i).get("name").asString()).isEqualTo(names[i]); + // the rebuild sets the type; readInstanceShouldNotWriteTheResolverTypeIntoThePublishedConfig pins the read + assertThat(providers.get(i).get("type").asString()).isEqualTo(OPENID_CONNECT); + } + } + + private static Object getField(final AuthenticationService service, final String name) throws Exception { + final Field field = AuthenticationService.class.getDeclaredField(name); + field.setAccessible(true); + return field.get(service); + } + + private static void setField(final AuthenticationService service, final String name, final Object value) + throws Exception { + final Field field = AuthenticationService.class.getDeclaredField(name); + field.setAccessible(true); + field.set(service, value); + } + /** * Tests that the attribute that {@link JwtSessionModule#isLogoutRequest(MessageInfo)} expects is present in the * attributesContext. diff --git a/openidm-identity-provider/src/main/java/org/forgerock/openidm/idp/impl/IdentityProviderService.java b/openidm-identity-provider/src/main/java/org/forgerock/openidm/idp/impl/IdentityProviderService.java index 710751536..b36be4834 100644 --- a/openidm-identity-provider/src/main/java/org/forgerock/openidm/idp/impl/IdentityProviderService.java +++ b/openidm-identity-provider/src/main/java/org/forgerock/openidm/idp/impl/IdentityProviderService.java @@ -155,16 +155,23 @@ private enum Action { availableProviders, getProfile } unbind = "unbindIdentityProviderConfig") protected void bindIdentityProviderConfig(final IdentityProviderConfig config) throws IdentityProviderServiceException { - identityProviders.computeIfAbsent(config.getIdentityProviderConfig().getType(), - type -> new CopyOnWriteArrayList<>()).add(config); + final String type = config.getIdentityProviderConfig().getType(); + if (type == null) { + logger.warn("Identity provider {} has no type and is ignored", + config.getIdentityProviderConfig().getName()); + return; + } + identityProviders.computeIfAbsent(type, t -> new CopyOnWriteArrayList<>()).add(config); notifyListeners(); } protected void unbindIdentityProviderConfig(final IdentityProviderConfig config) throws IdentityProviderServiceException { - if (identityProviders.get(config.getIdentityProviderConfig().getType()) != null) { + final String type = config.getIdentityProviderConfig().getType(); + // a provider without a type was never bound + if (type != null && identityProviders.get(type) != null) { logger.debug("Removed the {} identity provider.", config.getIdentityProviderConfig().getName()); - identityProviders.get(config.getIdentityProviderConfig().getType()).remove(config); + identityProviders.get(type).remove(config); notifyListeners(); } } diff --git a/openidm-identity-provider/src/test/java/org/forgerock/openidm/idp/impl/IdentityProviderServiceTest.java b/openidm-identity-provider/src/test/java/org/forgerock/openidm/idp/impl/IdentityProviderServiceTest.java index 43fa6d6c0..cce621a63 100644 --- a/openidm-identity-provider/src/test/java/org/forgerock/openidm/idp/impl/IdentityProviderServiceTest.java +++ b/openidm-identity-provider/src/test/java/org/forgerock/openidm/idp/impl/IdentityProviderServiceTest.java @@ -23,6 +23,7 @@ import static org.forgerock.json.test.assertj.AssertJJsonValueAssert.assertThat; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -135,24 +136,46 @@ public void testUnbindIdentityProviderConfig() throws Exception { @Test public void notifyListenersShouldNotifyEveryListenerWhenOneFails() throws Exception { - IdentityProviderListener failing = mock(IdentityProviderListener.class); - when(failing.getListenerName()).thenReturn("failing"); - doThrow(new IllegalArgumentException("unsupported type")).when(failing).identityProviderConfigChanged(); - IdentityProviderListener healthy = mock(IdentityProviderListener.class); - when(healthy.getListenerName()).thenReturn("healthy"); + // the listener map iterates in key-hash order: give each name the failing role in turn, + // so that in one of the two runs the failing listener is notified first + for (String failingName : new String[] { "first", "second" }) { + String healthyName = "first".equals(failingName) ? "second" : "first"; + IdentityProviderListener failing = mock(IdentityProviderListener.class); + when(failing.getListenerName()).thenReturn(failingName); + doThrow(new IllegalArgumentException("unsupported type")).when(failing).identityProviderConfigChanged(); + IdentityProviderListener healthy = mock(IdentityProviderListener.class); + when(healthy.getListenerName()).thenReturn(healthyName); + + IdentityProviderService service = new IdentityProviderService(); + service.registerIdentityProviderListener(failing); + service.registerIdentityProviderListener(healthy); + + try { + service.notifyListeners(); + fail("Expected IdentityProviderServiceException"); + } catch (IdentityProviderServiceException e) { + assertThat(e.getCause()).isInstanceOf(IllegalArgumentException.class); + } + verify(failing).identityProviderConfigChanged(); + verify(healthy).identityProviderConfigChanged(); + } + } + + @Test + public void providerWithoutTypeShouldBeIgnored() throws Exception { + final JsonValue untyped = ProviderConfigMapper.toJsonValue(googleIdentityProvider).copy(); + untyped.remove("type"); + IdentityProviderConfig idpConfig = mock(IdentityProviderConfig.class); + when(idpConfig.getIdentityProviderConfig()).thenReturn(ProviderConfigMapper.toProviderConfig(untyped)); + IdentityProviderListener listener = mock(IdentityProviderListener.class); + when(listener.getListenerName()).thenReturn("listener"); IdentityProviderService service = new IdentityProviderService(); - service.registerIdentityProviderListener(failing); - service.registerIdentityProviderListener(healthy); - - try { - service.notifyListeners(); - fail("Expected IdentityProviderServiceException"); - } catch (IdentityProviderServiceException e) { - assertThat(e.getCause()).isInstanceOf(IllegalArgumentException.class); - } - // whatever the iteration order, the failing listener does not stop the other one - verify(failing).identityProviderConfigChanged(); - verify(healthy).identityProviderConfigChanged(); + service.registerIdentityProviderListener(listener); + service.bindIdentityProviderConfig(idpConfig); + service.unbindIdentityProviderConfig(idpConfig); + + assertThat(service.getIdentityProviders()).isEmpty(); + verify(listener, never()).identityProviderConfigChanged(); } -} \ No newline at end of file +} diff --git a/openidm-selfservice/src/main/java/org/forgerock/openidm/selfservice/impl/SelfService.java b/openidm-selfservice/src/main/java/org/forgerock/openidm/selfservice/impl/SelfService.java index 0f073f3eb..fbefe887c 100644 --- a/openidm-selfservice/src/main/java/org/forgerock/openidm/selfservice/impl/SelfService.java +++ b/openidm-selfservice/src/main/java/org/forgerock/openidm/selfservice/impl/SelfService.java @@ -27,6 +27,7 @@ import java.util.Collections; import java.util.Dictionary; import java.util.HashMap; +import java.util.List; import java.util.Map; import org.apache.commons.lang3.StringUtils; @@ -46,6 +47,7 @@ import org.forgerock.openidm.core.IdentityServer; import org.forgerock.openidm.core.ServerConstants; import org.forgerock.openidm.keystore.SharedKeyService; +import org.forgerock.openidm.idp.config.ProviderConfig; import org.forgerock.openidm.idp.impl.IdentityProviderListener; import org.forgerock.openidm.idp.impl.IdentityProviderService; import org.forgerock.openidm.idp.impl.IdentityProviderServiceException; @@ -134,10 +136,31 @@ public class SelfService implements IdentityProviderListener { @Reference private SharedKeyService sharedKeyService; - @Reference(policy = ReferencePolicy.DYNAMIC, cardinality = ReferenceCardinality.OPTIONAL) private volatile IdentityProviderService identityProviderService; - void bindIdentityProviderService(IdentityProviderService identityProviderService) { + + @Reference( + name = "identityProviderService", + policy = ReferencePolicy.DYNAMIC, + cardinality = ReferenceCardinality.OPTIONAL, + unbind = "unbindIdentityProviderService") + synchronized void bindIdentityProviderService(IdentityProviderService identityProviderService) + throws IdentityProviderServiceException { this.identityProviderService = identityProviderService; + // no-op until activated; otherwise the rebuild registers this listener on the new service + identityProviderConfigChanged(); + } + + synchronized void unbindIdentityProviderService(IdentityProviderService identityProviderService) + throws IdentityProviderServiceException { + // the listener name needs the component context: without one, nothing was registered + if (context != null) { + identityProviderService.unregisterIdentityProviderListener(this); + } + // DS replaces a dynamic 0..1 reference by binding the new service before unbinding the old one + if (this.identityProviderService == identityProviderService) { + this.identityProviderService = null; + identityProviderConfigChanged(); + } } @Reference(policy = ReferencePolicy.STATIC) @@ -173,26 +196,37 @@ synchronized void activate(ComponentContext context) throws Exception { } catch (Exception ex) { LOGGER.warn("Configuration invalid, can not start self-service.", ex); + // DS calls no deactivate after a failed activate: drop the listener amendConfig may have + // registered, and leave no configuration for a later provider change to rebuild from + config = null; + if (identityProviderService != null) { + identityProviderService.unregisterIdentityProviderListener(this); + } throw ex; } LOGGER.info("Self-service started."); } void amendConfig(final JsonValue config) throws ResourceException { + final IdentityProviderService identityProviderService = this.identityProviderService; for (JsonValue stageConfig : config.get(STAGE_CONFIGS)) { if (stageConfig.isDefined(KBA_CONFIG)) { // overwrite kbaConfig with config from KBA config service stageConfig.put(KBA_CONFIG, kbaConfiguration.getConfig().getObject()); - } else if (identityProviderService != null - && SocialUserDetailsConfig.NAME.equals(stageConfig.get("name").asString())) { - // add oauth provider config - identityProviderService.registerIdentityProviderListener(this); + } else if (SocialUserDetailsConfig.NAME.equals(stageConfig.get("name").asString())) { + // add oauth provider config; without the service, replace the providers of a previously + // bound one, since this config is amended in place + final List providers; + if (identityProviderService != null) { + identityProviderService.registerIdentityProviderListener(this); + providers = FluentIterable.from(identityProviderService.getIdentityProviders()) + .filter(providerEnabled) + .toList(); + } else { + providers = Collections.emptyList(); + } stageConfig.put(IdentityProviderService.PROVIDERS, - ProviderConfigMapper.toJsonValue( - FluentIterable.from(identityProviderService.getIdentityProviders()) - .filter(providerEnabled) - .toList()) - .asList()); + ProviderConfigMapper.toJsonValue(providers).asList()); } } diff --git a/openidm-selfservice/src/test/java/org/forgerock/openidm/selfservice/impl/SelfServiceTest.java b/openidm-selfservice/src/test/java/org/forgerock/openidm/selfservice/impl/SelfServiceTest.java index 6735f0faa..ccc236ac3 100644 --- a/openidm-selfservice/src/test/java/org/forgerock/openidm/selfservice/impl/SelfServiceTest.java +++ b/openidm-selfservice/src/test/java/org/forgerock/openidm/selfservice/impl/SelfServiceTest.java @@ -17,10 +17,14 @@ package org.forgerock.openidm.selfservice.impl; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.fail; import static org.forgerock.json.JsonValue.*; +import static org.mockito.Matchers.any; import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -28,12 +32,17 @@ import com.fasterxml.jackson.databind.DeserializationFeature; import com.fasterxml.jackson.databind.ObjectMapper; import org.forgerock.json.JsonValue; +import org.forgerock.openidm.config.enhanced.EnhancedConfig; import org.forgerock.openidm.idp.config.ProviderConfig; import org.forgerock.openidm.idp.impl.IdentityProviderService; +import org.forgerock.openidm.osgi.ComponentContextUtil; +import org.osgi.service.component.ComponentContext; import org.testng.annotations.BeforeSuite; import org.testng.annotations.Test; +import java.lang.reflect.Field; import java.util.ArrayList; +import java.util.Hashtable; import java.util.List; import java.util.Map; @@ -101,6 +110,120 @@ public void identityProviderConfigChangedShouldIgnoreChangeWithoutConfiguration( // a provider change that arrives before activate or after deactivate has nothing to rebuild selfService.identityProviderConfigChanged(); - verify(identityProviderService, never()).registerIdentityProviderListener(selfService); + // the first call past the guard is the debug log argument + verify(identityProviderService, never()).getIdentityProviders(); } -} \ No newline at end of file + + @Test + public void identityProviderServiceBindAndUnbindShouldRebuild() throws Exception { + final SelfService selfService = spy(new SelfService()); + doNothing().when(selfService).identityProviderConfigChanged(); + final IdentityProviderService first = mock(IdentityProviderService.class); + final IdentityProviderService second = mock(IdentityProviderService.class); + + selfService.bindIdentityProviderService(first); + verify(selfService, times(1)).identityProviderConfigChanged(); + + // DS replaces a dynamic 0..1 reference by binding the new service before unbinding the old one + selfService.bindIdentityProviderService(second); + selfService.unbindIdentityProviderService(first); + verify(selfService, times(2)).identityProviderConfigChanged(); + + selfService.unbindIdentityProviderService(second); + verify(selfService, times(3)).identityProviderConfigChanged(); + } + + @Test + public void unbindIdentityProviderServiceShouldUnregisterListener() throws Exception { + final IdentityProviderService identityProviderService = mock(IdentityProviderService.class); + final SelfService selfService = new SelfService(); + final Hashtable properties = new Hashtable<>(); + properties.put(ComponentContextUtil.COMPONENT_NAME, SelfService.PID); + final ComponentContext context = mock(ComponentContext.class); + when(context.getProperties()).thenReturn(properties); + final Field contextField = SelfService.class.getDeclaredField("context"); + contextField.setAccessible(true); + contextField.set(selfService, context); + + selfService.bindIdentityProviderService(identityProviderService); + selfService.unbindIdentityProviderService(identityProviderService); + + verify(identityProviderService).unregisterIdentityProviderListener(selfService); + } + + @Test + public void amendConfigShouldDropProvidersOfUnboundService() throws Exception { + final IdentityProviderService identityProviderService = mock(IdentityProviderService.class); + final List providerConfigs = new ArrayList<>(); + providerConfigs.add(googleIdentityProvider); + when(identityProviderService.getIdentityProviders()).thenReturn(providerConfigs); + final JsonValue registration = selfServiceRegistration.copy(); + final SelfService selfService = new SelfService(); + + selfService.bindIdentityProviderService(identityProviderService); + selfService.amendConfig(registration); + assertThat(registration.get("stageConfigs").get(0).get("providers").size()).isEqualTo(1); + + // the config is amended in place, so the providers of the unbound service must be replaced + selfService.unbindIdentityProviderService(identityProviderService); + // nothing was registered without a component context, so nothing may be unregistered + verify(identityProviderService, never()).unregisterIdentityProviderListener(selfService); + selfService.amendConfig(registration); + assertThat(registration.get("stageConfigs").get(0).get("providers").size()).isEqualTo(0); + } + + @Test + public void failedActivationShouldLeaveNoConfigurationToRebuild() throws Exception { + final SelfService selfService = spy(new SelfService()); + final EnhancedConfig enhancedConfig = mock(EnhancedConfig.class); + when(enhancedConfig.getConfigurationAsJson(any(ComponentContext.class))).thenReturn(json(object())); + // a blank factory PID fails activate after the configuration is read + when(enhancedConfig.getConfigurationFactoryPid(any(ComponentContext.class))).thenReturn(""); + setField(selfService, "enhancedConfig", enhancedConfig); + try { + selfService.activate(mock(ComponentContext.class)); + fail("Expected IllegalArgumentException"); + } catch (IllegalArgumentException e) { + // expected + } + + // DS binds and unbinds the references of a component whose activation failed + selfService.bindIdentityProviderService(mock(IdentityProviderService.class)); + + verify(selfService, never()).amendConfig(any(JsonValue.class)); + } + + @Test + public void failedActivationShouldUnregisterTheListener() throws Exception { + final IdentityProviderService identityProviderService = mock(IdentityProviderService.class); + final SelfService selfService = new SelfService(); + final EnhancedConfig enhancedConfig = mock(EnhancedConfig.class); + when(enhancedConfig.getConfigurationAsJson(any(ComponentContext.class))) + .thenReturn(selfServiceRegistration.copy()); + when(enhancedConfig.getConfigurationFactoryPid(any(ComponentContext.class))).thenReturn("registration"); + setField(selfService, "enhancedConfig", enhancedConfig); + final Hashtable properties = new Hashtable<>(); + properties.put(ComponentContextUtil.COMPONENT_NAME, SelfService.PID); + // no bundle context: activate fails after amendConfig has registered the listener + final ComponentContext context = mock(ComponentContext.class); + when(context.getProperties()).thenReturn(properties); + + selfService.bindIdentityProviderService(identityProviderService); + try { + selfService.activate(context); + fail("Expected activate to fail"); + } catch (Exception e) { + // expected + } + + verify(identityProviderService).registerIdentityProviderListener(selfService); + verify(identityProviderService).unregisterIdentityProviderListener(selfService); + } + + private static void setField(final SelfService selfService, final String name, final Object value) + throws Exception { + final Field field = SelfService.class.getDeclaredField(name); + field.setAccessible(true); + field.set(selfService, value); + } +}