From 3bf7ac9f410c9ba64da8085e7b863f658459a293 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Wed, 23 Sep 2026 17:00:53 +0300 Subject: [PATCH 1/2] [#225] Bind identity provider configs through the DS bind methods The @Reference to IdentityProviderConfig sat on a Map field, a type Declarative Services cannot inject, so SCR rejected it and bindIdentityProviderConfig was never called: /identityProviders stayed empty and no social auth module was generated. Move the annotation to the bind method, and do the same for AuthenticationService, whose bindIdentityProviderService (which registers the provider listener) was bypassed by field injection too. Also make getIdentityProviderByType return an empty list for a type with no bound provider, and add providers atomically. Fixes #225 --- .../openidm/auth/AuthenticationService.java | 21 +++++++++--- .../auth/AuthenticationServiceTest.java | 34 ++++++++++++++++++- .../idp/impl/IdentityProviderService.java | 29 +++++++--------- .../idp/impl/IdentityProviderServiceTest.java | 34 +++++++++++++++++++ 4 files changed, 96 insertions(+), 22 deletions(-) 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 567e6fd8ee..47a6106a82 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 @@ -12,7 +12,7 @@ * information: "Portions copyright [year] [name of copyright owner]". * * Copyright 2013-2016 ForgeRock AS - * Portions copyright 2024-2025 3A Systems LLC. + * Portions copyright 2024-2026 3A Systems LLC. */ package org.forgerock.openidm.auth; @@ -243,17 +243,28 @@ public class AuthenticationService implements SingletonResourceProvider, Identit @Reference(policy = ReferencePolicy.DYNAMIC, target="(service.pid=org.forgerock.openidm.auth.config)") private volatile AuthFilterWrapper authFilterWrapper; - @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") + void bindIdentityProviderService(IdentityProviderService identityProviderService) + throws IdentityProviderServiceException { this.identityProviderService = identityProviderService; identityProviderService.registerIdentityProviderListener(this); + // no-op until activated; rebuilds the social auth modules if the service arrives later + identityProviderConfigChanged(); } - void unbindIdentityProviderService() { + void unbindIdentityProviderService(IdentityProviderService identityProviderService) + throws IdentityProviderServiceException { identityProviderService.unregisterIdentityProviderListener(this); - identityProviderService = null; + if (this.identityProviderService == identityProviderService) { + this.identityProviderService = null; + identityProviderConfigChanged(); + } } /** An on-demand Provider for the ConnectionFactory */ 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 19f001e772..954b469c5a 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 @@ -22,6 +22,7 @@ import static org.forgerock.json.resource.Requests.newReadRequest; import static org.forgerock.openidm.auth.AuthenticationService.Action; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import javax.security.auth.message.MessageInfo; @@ -86,7 +87,6 @@ public void setUp() throws Exception { OBJECT_MAPPER.readValue(getClass().getResource("/config/authentication.json"), Map.class)); // Instantiate the object to be used with proper mocked IdentityProviderService authenticationService = new AuthenticationService(); - authenticationService.setConfig(authenticationJson); } @AfterMethod @@ -111,6 +111,8 @@ public void testAmendAuthConfig() throws Exception { // Instantiate the object to be used with proper mocked IdentityProviderService authenticationService.bindIdentityProviderService(identityProviderService); + // the reference is bound before the component is activated with its configuration + authenticationService.setConfig(authenticationJson); // Call the amendAuthConfig to see the configuration of authentication.json be modified with // the injected identityProvider config from the IdentityProviderService @@ -154,6 +156,8 @@ public void testAmendAuthConfigWithTwoAuthTypes() throws Exception { // Instantiate the object to be used with proper mocked IdentityProviderService authenticationService.bindIdentityProviderService(identityProviderService); + // the reference is bound before the component is activated with its configuration + authenticationService.setConfig(authenticationJson); // Call the amendAuthConfig to see the configuration of authentication.json be modified with // the injected identityProvider config from the IdentityProviderService @@ -183,6 +187,8 @@ public void testNoProviderConfigsToInject() throws Exception { when(identityProviderService.getIdentityProviders()).thenReturn(providerConfigs); authenticationService.bindIdentityProviderService(identityProviderService); + // the reference is bound before the component is activated with its configuration + authenticationService.setConfig(authenticationJson); // Call the amendAuthConfig to see the configuration of authentication.json be modified with // the injected identityProvider config from the IdentityProviderService; in this test case @@ -234,6 +240,32 @@ public void amendAuthConfigShouldRemoveSocialProvidersModuleWhenIdentityProvider assertThat(authenticationJson.get(AUTH_MODULES).size()).isEqualTo(1); } + @Test + public void bindIdentityProviderServiceShouldRegisterListener() throws Exception { + final IdentityProviderService identityProviderService = mock(IdentityProviderService.class); + + authenticationService.bindIdentityProviderService(identityProviderService); + + verify(identityProviderService).registerIdentityProviderListener(authenticationService); + } + + @Test + public void unbindIdentityProviderServiceShouldUnregisterListenerAndStopInjectingProviders() throws Exception { + final IdentityProviderService identityProviderService = mock(IdentityProviderService.class); + final List openIdProviderConfigs = new ArrayList<>(); + openIdProviderConfigs.add(ProviderConfigMapper.toProviderConfig(googleIdentityProvider)); + when(identityProviderService.getIdentityProviders()).thenReturn(openIdProviderConfigs); + + authenticationService.bindIdentityProviderService(identityProviderService); + authenticationService.unbindIdentityProviderService(identityProviderService); + authenticationService.setConfig(authenticationJson); + authenticationService.amendAuthConfig(authenticationJson.get(AUTH_MODULES)); + + verify(identityProviderService).unregisterIdentityProviderListener(authenticationService); + // only the stand-alone OPENID_CONNECT module is left, no module was generated from the provider + assertThat(authenticationJson.get(AUTH_MODULES).size()).isEqualTo(1); + } + /** * 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 469a88da00..6be063eb4c 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 @@ -12,14 +12,16 @@ * information: "Portions copyright [year] [name of copyright owner]". * * Copyright 2016 ForgeRock AS. - * Portions Copyrighted 2024 3A Systems LLC. + * Portions Copyrighted 2024-2026 3A Systems LLC. */ package org.forgerock.openidm.idp.impl; import java.util.ArrayList; +import java.util.Collections; import java.util.List; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CopyOnWriteArrayList; import static org.forgerock.http.handler.HttpClientHandler.OPTION_LOADER; import static org.forgerock.json.JsonValue.field; @@ -143,24 +145,18 @@ private enum Action { availableProviders, getProfile } * The String param in Map is referring to the * type of auth the identity provider supports. */ + private final Map> identityProviders = new ConcurrentHashMap<>(); + @Reference( + name = "identityProviders", service = IdentityProviderConfig.class, cardinality = ReferenceCardinality.MULTIPLE, - policy = ReferencePolicy.DYNAMIC) - private final Map> identityProviders = new ConcurrentHashMap<>(); - + policy = ReferencePolicy.DYNAMIC, + unbind = "unbindIdentityProviderConfig") protected void bindIdentityProviderConfig(final IdentityProviderConfig config) throws IdentityProviderServiceException { - // for this to be true, we do not have any identityProviders of this type - if (!identityProviders.containsKey(config.getIdentityProviderConfig().getType())) { - // initialize new array list to store providers of this type - List providers = new ArrayList<>(); - providers.add(config); - identityProviders.put(config.getIdentityProviderConfig().getType(), providers); - } else { - // we currently have existing configs of this type, just add to it - identityProviders.get(config.getIdentityProviderConfig().getType()).add(config); - } + identityProviders.computeIfAbsent(config.getIdentityProviderConfig().getType(), + type -> new CopyOnWriteArrayList<>()).add(config); notifyListeners(); } @@ -201,11 +197,12 @@ public void deactivate(ComponentContext context) { */ public List getIdentityProviderByType(final String type) { final List providers = new ArrayList<>(); - if (identityProviders == null || identityProviders.size() == 0) { + if (identityProviders.isEmpty()) { logger.debug("No Identity Providers have been configured."); return providers; } - for (final IdentityProviderConfig config : identityProviders.get(type)) { + for (final IdentityProviderConfig config + : identityProviders.getOrDefault(type, Collections.emptyList())) { providers.add(config.getIdentityProviderConfig()); } return providers; 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 28b988b9a5..6f6519fd3f 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 @@ -21,6 +21,8 @@ import static org.forgerock.json.resource.Requests.newReadRequest; import static org.forgerock.json.test.assertj.AssertJJsonValueAssert.assertThat; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import java.util.Map; @@ -96,4 +98,36 @@ public void testReadInstance() throws Exception { assertThat(google).doesNotContain("client_secret"); // it should be removed by readInstance assertThat(google.isEqualTo(expected)).isTrue(); } + + @Test + public void testGetIdentityProviderByType() throws Exception { + IdentityProviderConfig idpConfig = mock(IdentityProviderConfig.class); + when(idpConfig.getIdentityProviderConfig()).thenReturn(googleIdentityProvider); + + IdentityProviderService service = new IdentityProviderService(); + service.bindIdentityProviderConfig(idpConfig); + + assertThat(service.getIdentityProviderByType("OPENID_CONNECT")).containsExactly(googleIdentityProvider); + // a type with no bound provider yields an empty list rather than failing + assertThat(service.getIdentityProviderByType("OAUTH")).isEmpty(); + } + + @Test + public void testUnbindIdentityProviderConfig() throws Exception { + IdentityProviderConfig idpConfig = mock(IdentityProviderConfig.class); + when(idpConfig.getIdentityProviderConfig()).thenReturn(googleIdentityProvider); + IdentityProviderListener listener = mock(IdentityProviderListener.class); + when(listener.getListenerName()).thenReturn("listener"); + + IdentityProviderService service = new IdentityProviderService(); + service.registerIdentityProviderListener(listener); + service.bindIdentityProviderConfig(idpConfig); + assertThat(service.getIdentityProvider("google")).isSameAs(googleIdentityProvider); + + service.unbindIdentityProviderConfig(idpConfig); + + assertThat(service.getIdentityProviders()).isEmpty(); + assertThat(service.getIdentityProvider("google")).isNull(); + verify(listener, times(2)).identityProviderConfigChanged(); + } } \ No newline at end of file From 2202ea991e05e20952d7c51137536fac9573371a Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Sat, 3 Oct 2026 09:16:07 +0300 Subject: [PATCH 2/2] [#225] Publish rebuilt auth state atomically and keep notifying listeners Binding identity providers through the DS bind methods makes the listener rebuilds run on DS bind threads, concurrently with requests: - AuthenticationService builds the amended config and authenticators locally and publishes them through volatile fields; the rebuild, activate and deactivate are synchronized. - SelfService serializes its unregister/register rebuild and ignores a change that arrives without configuration. - notifyListeners() notifies every listener and rethrows the first failure instead of stopping at it. - amendAuthConfig() skips providers whose type is neither OPENID_CONNECT nor OAUTH instead of failing the whole configuration. Also check the generated DS descriptors in CI. --- .github/workflows/build.yml | 10 +++++ .../openidm/auth/AuthenticationService.java | 45 ++++++++++++++----- .../auth/AuthenticationServiceTest.java | 44 ++++++++++++++++++ .../idp/impl/IdentityProviderService.java | 15 ++++++- .../idp/impl/IdentityProviderServiceTest.java | 25 +++++++++++ .../openidm/selfservice/impl/SelfService.java | 14 ++++-- .../selfservice/impl/SelfServiceTest.java | 15 +++++++ 7 files changed, 151 insertions(+), 17 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index bfda1ce5fd..e0d29e0329 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -60,6 +60,16 @@ jobs: env: MAVEN_OPTS: -Dhttps.protocols=TLSv1.2 -Dmaven.wagon.httpconnectionManager.ttlSeconds=120 -Dmaven.wagon.http.retryHandler.requestSentEnabled=true -Dmaven.wagon.http.retryHandler.count=10 run: mvn --batch-mode --errors --update-snapshots verify --file pom.xml + - name: Check DS descriptors + if: runner.os != 'Windows' + # The identity provider references must be bound through their bind methods (#225). The descriptors + # only exist in the jars and no shipped sample configures an identity provider, so nothing else notices + # a @Reference moved back onto the field. + run: | + unzip -p openidm-identity-provider/target/openidm-identity-provider-*[0-9T].jar \ + 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"' - 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 47a6106a82..cd10913761 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 @@ -214,10 +214,10 @@ public class AuthenticationService implements SingletonResourceProvider, Identit * all the associated auth modules (OAUTH and OPENID_CONNECT) and remove the SOCIAL_PROVIDERS * authentication module in memory only so that it does not get initialized. */ - private JsonValue amendedConfig; + private volatile JsonValue amendedConfig; /** The authenticators to delegate to.*/ - private List authenticators = new ArrayList<>(); + private volatile List authenticators = new ArrayList<>(); // ----- Declarative Service Implementation @@ -424,11 +424,30 @@ void amendAuthConfig(final JsonValue authModuleConfig) { if (identityProviderService != null) { authModuleConfig.asList().addAll( FluentIterable.from(identityProviderService.getIdentityProviders()) + .filter(socialAuthModuleTypes) .transform(new SocialAuthModuleConfigFactory(socialAuthTemplate)) .toList()); } } + /** + * A {@link Predicate} that keeps the identity providers an auth module can be generated for, so that one + * provider with an unsupported type does not fail the whole authentication configuration. + */ + private static final Predicate socialAuthModuleTypes = + new Predicate() { + @Override + public boolean apply(ProviderConfig providerConfig) { + if (IDMAuthModule.OPENID_CONNECT.name().equals(providerConfig.getType()) + || IDMAuthModule.OAUTH.name().equals(providerConfig.getType())) { + return true; + } + logger.warn("Identity provider {} has unsupported type {}, no auth module is generated for it", + providerConfig.getName(), providerConfig.getType()); + return false; + } + }; + /** * Factory used to create OPENID_CONNECT and OAUTH auth module configurations. */ @@ -494,32 +513,34 @@ public String getListenerName() { } @Override - public void identityProviderConfigChanged() throws IdentityProviderServiceException { + public synchronized void identityProviderConfigChanged() throws IdentityProviderServiceException { if (config == null) { logger.debug("No configuration for Authentication Service"); return; } - amendedConfig = config.copy(); + final JsonValue newAmendedConfig = config.copy(); // the auth module list config lives under at /serverAuthConfig/authModule - final JsonValue authModuleConfig = amendedConfig.get(SERVER_AUTH_CONTEXT_KEY).get(AUTH_MODULES_KEY); + final JsonValue authModuleConfig = newAmendedConfig.get(SERVER_AUTH_CONTEXT_KEY).get(AUTH_MODULES_KEY); amendAuthConfig(authModuleConfig); try { - authFilterWrapper.setFilter(configureAuthenticationFilter(amendedConfig)); + authFilterWrapper.setFilter(configureAuthenticationFilter(newAmendedConfig)); } catch (AuthenticationException e) { logger.debug("Error in configuration for Authentication Service. Filter not set.", e); 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.clear(); - authenticators.addAll(FluentIterable.from(authModuleConfig) + authenticators = FluentIterable.from(authModuleConfig) .filter(enabledAuthModules) .transform(toModuleProperties) .filter(authModulesThatHaveValidAuthenticatorProperties) .transform(toAuthenticatorFromProperties) - .toList()); + .toList(); } /** @@ -528,7 +549,7 @@ public void identityProviderConfigChanged() throws IdentityProviderServiceExcept * @param context The ComponentContext */ @Activate - public void activate(final ComponentContext context) + public synchronized void activate(final ComponentContext context) throws AuthenticationException, IdentityProviderServiceException { logger.info("Activating Authentication Service with configuration {}", context.getProperties()); config = enhancedConfig.getConfigurationAsJson(context); @@ -542,10 +563,10 @@ public void activate(final ComponentContext context) * @param context The ComponentContext. */ @Deactivate - public void deactivate(ComponentContext context) { + public synchronized void deactivate(ComponentContext context) { logger.debug("OpenIDM Config for Authentication {} is deactivated.", config.get(Constants.SERVICE_PID)); config = null; - authenticators.clear(); + authenticators = new ArrayList<>(); // remove CAF filter from CHF filter wrapper if (authFilterWrapper != null) { 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 954b469c5a..5ba518975a 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,7 +21,10 @@ 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.mockito.Mockito.doNothing; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -266,6 +269,47 @@ public void unbindIdentityProviderServiceShouldUnregisterListenerAndStopInjectin assertThat(authenticationJson.get(AUTH_MODULES).size()).isEqualTo(1); } + @Test + public void identityProviderServiceBindAndUnbindShouldRebuildAuthModules() throws Exception { + final AuthenticationService service = spy(new AuthenticationService()); + doNothing().when(service).identityProviderConfigChanged(); + final IdentityProviderService first = mock(IdentityProviderService.class); + final IdentityProviderService second = mock(IdentityProviderService.class); + + service.bindIdentityProviderService(first); + verify(service, times(1)).identityProviderConfigChanged(); + + // DS replaces a dynamic 0..1 reference by binding the new service before unbinding the old one + service.bindIdentityProviderService(second); + service.unbindIdentityProviderService(first); + verify(service, times(2)).identityProviderConfigChanged(); + + service.unbindIdentityProviderService(second); + verify(service, times(3)).identityProviderConfigChanged(); + } + + @Test + public void amendAuthConfigShouldSkipProvidersOfUnsupportedType() throws Exception { + final IdentityProviderService identityProviderService = mock(IdentityProviderService.class); + final List providerConfigs = new ArrayList<>(); + providerConfigs.add(ProviderConfigMapper.toProviderConfig(googleIdentityProvider)); + // not an IDMAuthModule name at all + providerConfigs.add(ProviderConfigMapper.toProviderConfig( + googleIdentityProvider.copy().put("name", "unknown").put("type", "UNKNOWN"))); + // an IDMAuthModule name, but not one a social auth module can be generated for + providerConfigs.add(ProviderConfigMapper.toProviderConfig( + googleIdentityProvider.copy().put("name", "managed").put("type", "MANAGED_USER"))); + when(identityProviderService.getIdentityProviders()).thenReturn(providerConfigs); + + authenticationService.bindIdentityProviderService(identityProviderService); + authenticationService.setConfig(authenticationJson); + authenticationService.amendAuthConfig(authenticationJson.get(AUTH_MODULES)); + + // the stand-alone OPENID_CONNECT module plus the one generated from the supported provider + assertThat(authenticationJson.get(AUTH_MODULES).size()).isEqualTo(2); + assertThat(authenticationJson.get(AUTH_MODULES).get(1).get("name").asString()).isEqualTo(OPENID_CONNECT); + } + /** * 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 6be063eb4c..7107515368 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 @@ -357,8 +357,21 @@ public void unregisterIdentityProviderListener(IdentityProviderListener listener * on any identity provider configuration. */ public void notifyListeners() throws IdentityProviderServiceException { + IdentityProviderServiceException failure = null; for (IdentityProviderListener listener : identityProviderListeners.values()) { - listener.identityProviderConfigChanged(); + try { + listener.identityProviderConfigChanged(); + } catch (IdentityProviderServiceException | RuntimeException e) { + // keep notifying the other listeners; one failing listener must not leave them stale + logger.warn("Listener {} failed to apply the identity provider change", + listener.getListenerName(), e); + if (failure == null) { + failure = new IdentityProviderServiceException(e.getMessage(), e); + } + } + } + if (failure != null) { + throw failure; } } 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 6f6519fd3f..43fa6d6c0e 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 @@ -17,9 +17,11 @@ package org.forgerock.openidm.idp.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.forgerock.json.resource.Requests.newReadRequest; 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.times; import static org.mockito.Mockito.verify; @@ -130,4 +132,27 @@ public void testUnbindIdentityProviderConfig() throws Exception { assertThat(service.getIdentityProvider("google")).isNull(); verify(listener, times(2)).identityProviderConfigChanged(); } + + @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"); + + 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(); + } } \ 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 d7c0e256a4..0f073f3ebf 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 @@ -12,7 +12,7 @@ * information: "Portions copyright [year] [name of copyright owner]". * * Copyright 2015-2016 ForgeRock AS. - * Portions Copyrighted 2024 3A Systems LLC. + * Portions Copyrighted 2024-2026 3A Systems LLC. */ package org.forgerock.openidm.selfservice.impl; @@ -151,7 +151,7 @@ void bindIdentityProviderService(IdentityProviderService identityProviderService private ProgressStageProvider progressStageProvider; @Activate - void activate(ComponentContext context) throws Exception { + synchronized void activate(ComponentContext context) throws Exception { this.context = context; LOGGER.debug("Activating Service with configuration {}", context.getProperties()); try { @@ -308,7 +308,7 @@ public JsonValue remove(String s) { } @Deactivate - void deactivate(ComponentContext compContext) { + synchronized void deactivate(ComponentContext compContext) { LOGGER.debug("Deactivating Service {}", compContext.getProperties()); try { unregisterServiceRegistration(); @@ -340,8 +340,14 @@ public String getListenerName() { } @Override - public void identityProviderConfigChanged() + public synchronized void identityProviderConfigChanged() throws IdentityProviderServiceException { + // runs on DS bind threads too: serialize the unregister/register pair below, + // and ignore a change that arrives after deactivate + if (config == null) { + LOGGER.debug("No configuration for {}", PID); + return; + } LOGGER.debug("Configuring {} with changes from IdentityProviderConfig {}", PID, identityProviderService != null ? identityProviderService.getIdentityProviders() 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 3ea7be2cf6..6735f0faac 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 @@ -12,6 +12,7 @@ * information: "Portions copyright [year] [name of copyright owner]". * * Copyright 2016 ForgeRock AS. + * Portions copyright 2026 3A Systems LLC */ package org.forgerock.openidm.selfservice.impl; @@ -19,6 +20,8 @@ import static org.forgerock.json.JsonValue.*; import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import com.fasterxml.jackson.core.JsonParser; @@ -88,4 +91,16 @@ public void testAmendConfig() throws Exception { assertThat(selfServiceRegistration.isEqualTo(amendedSelfServiceRegistration)).isTrue(); } + + @Test + public void identityProviderConfigChangedShouldIgnoreChangeWithoutConfiguration() throws Exception { + final IdentityProviderService identityProviderService = mock(IdentityProviderService.class); + final SelfService selfService = new SelfService(); + selfService.bindIdentityProviderService(identityProviderService); + + // a provider change that arrives before activate or after deactivate has nothing to rebuild + selfService.identityProviderConfigChanged(); + + verify(identityProviderService, never()).registerIdentityProviderListener(selfService); + } } \ No newline at end of file