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/4] [#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/4] [#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 From ff6bf2f666f8053a8e1d43f603cd0595ace116eb Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Tue, 29 Sep 2026 11:39:26 +0300 Subject: [PATCH 3/4] [#230] Restore the identity provider catalogue and fix the social provider docs conf/identityProviders.json, the catalogue of supported social providers, was dropped from the distribution in 95c573c78, so IdentityProviderService (ConfigurationPolicy.REQUIRE) never started and Configure > Social ID Providers offered nothing. Bring it back with current endpoints: Google's token and userinfo endpoints from its discovery document, Facebook's unversioned Graph endpoints without the discontinued locale field, and LinkedIn's OpenID Connect userinfo in place of the retired v1 API. The Integrator's Guide placed provider configuration in identityProviders.json. A provider is configured in its own identityProvider-.json, which the Admin UI writes, SOCIAL_PROVIDERS reads and the configuration service encrypts client_secret in; identityProviders.json is only the catalogue. Fixes #230 --- .../appendix-auth-modules.adoc | 4 +- .../asciidoc/integrators-guide/chap-auth.adoc | 43 +++++- .../resources/conf/identityProviders.json | 142 ++++++++++++++++++ 3 files changed, 184 insertions(+), 5 deletions(-) create mode 100644 openidm-zip/src/main/resources/conf/identityProviders.json diff --git a/openidm-doc/src/main/asciidoc/integrators-guide/appendix-auth-modules.adoc b/openidm-doc/src/main/asciidoc/integrators-guide/appendix-auth-modules.adoc index cccac0dd8a..b86d9e6fcb 100644 --- a/openidm-doc/src/main/asciidoc/integrators-guide/appendix-auth-modules.adoc +++ b/openidm-doc/src/main/asciidoc/integrators-guide/appendix-auth-modules.adoc @@ -294,11 +294,11 @@ In general, if you add a custom property, the Admin UI writes changes to the `au [#social-providers-module-details] === SOCIAL_PROVIDERS Module Configuration Options -The `SOCIAL_PROVIDERS` module is a meta-module (template) that dynamically generates `OPENID_CONNECT` and `OAUTH` authentication modules at startup for supported providers registered in the `IdentityProviderService`. The identity provider configurations themselves (client IDs, client secrets, authorization endpoints, etc.) are defined in `conf/identityProviders.json`, not inside the `SOCIAL_PROVIDERS` module entry. +The `SOCIAL_PROVIDERS` module is a meta-module (template) that dynamically generates `OPENID_CONNECT` and `OAUTH` authentication modules at startup for supported providers registered in the `IdentityProviderService`. The identity provider configurations themselves (client IDs, client secrets, authorization endpoints, etc.) are defined in one `conf/identityProvider-.json` file per provider, not inside the `SOCIAL_PROVIDERS` module entry. `conf/identityProviders.json` only holds the catalog of providers that the Admin UI offers; see xref:chap-auth.adoc#social-providers-module["SOCIAL_PROVIDERS"]. [NOTE] ====== -The `SOCIAL_PROVIDERS` entry is removed from the active authentication module list at startup — it is never initialized as an authenticator itself. Each `OPENID_CONNECT` or `OAUTH` provider entry in `conf/identityProviders.json` results in exactly one generated authentication module. +The `SOCIAL_PROVIDERS` entry is removed from the active authentication module list at startup — it is never initialized as an authenticator itself. Each `conf/identityProvider-.json` file results in exactly one generated `OPENID_CONNECT` or `OAUTH` authentication module, which carries the `enabled` flag of that provider. ====== [#social-providers-module-prop-basic] diff --git a/openidm-doc/src/main/asciidoc/integrators-guide/chap-auth.adoc b/openidm-doc/src/main/asciidoc/integrators-guide/chap-auth.adoc index c67d7ec701..352801dfd4 100644 --- a/openidm-doc/src/main/asciidoc/integrators-guide/chap-auth.adoc +++ b/openidm-doc/src/main/asciidoc/integrators-guide/chap-auth.adoc @@ -481,17 +481,54 @@ IWA:: The IWA module enables users to authenticate by using Integrated Windows Authentication (IWA), rather than by providing a username and password. For information about configuring the IWA module with OpenIDM, see xref:#openidm-auth-kerberos["Configuring IWA Authentication"]. [[social-providers-module]]SOCIAL_PROVIDERS:: -The `SOCIAL_PROVIDERS` module is a __meta-module__ (template) that bridges the social identity provider configuration in `conf/identityProviders.json` and the OpenIDM authentication filter. It is not an authenticator itself — instead, at startup it scans all providers registered with the `IdentityProviderService`, removes itself from the active module list, and dynamically generates the corresponding `OPENID_CONNECT` or `OAUTH` authentication modules: +The `SOCIAL_PROVIDERS` module is a __meta-module__ (template) that bridges the social identity providers configured in `conf/identityProvider-.json` files and the OpenIDM authentication filter. It is not an authenticator itself — instead, at startup it scans all providers registered with the `IdentityProviderService`, removes itself from the active module list, and dynamically generates the corresponding `OPENID_CONNECT` or `OAUTH` authentication modules: + * For each provider of type `OPENID_CONNECT`, an `OPENID_CONNECT` auth module is generated with `openIdConnectHeader: "authToken"`. * For each provider of type `OAUTH`, an `OAUTH` auth module is generated with `authTokenHeader: "authToken"` and `authResolverHeader: "provider"`. + -The generated modules inherit the `augmentSecurityContext`, `propertyMapping`, and `defaultUserRoles` values from the `SOCIAL_PROVIDERS` template entry. +The generated modules inherit the `augmentSecurityContext`, `propertyMapping`, and `defaultUserRoles` values from the `SOCIAL_PROVIDERS` template entry. Each generated module also carries the `enabled` flag of its provider, so a provider with `"enabled" : false` produces no active module. + -Providers are configured separately in `conf/identityProviders.json`, or via the Admin UI under *Configure > Social ID Providers*. The `SOCIAL_PROVIDERS` module acts as a single configuration point so that operators do not need to add individual `OPENID_CONNECT` or `OAUTH` entries to `authentication.json` for every social provider. +Each provider is configured in a file of its own, `conf/identityProvider-.json`, for example `conf/identityProvider-google.json`. The provider name is taken from the file name. You can write the file by hand, or enable the provider in the Admin UI under *Configure > Social ID Providers*, which writes the same file. OpenIDM stores the `client_secret` of these files encrypted. The `SOCIAL_PROVIDERS` module acts as a single configuration point so that operators do not need to add individual `OPENID_CONNECT` or `OAUTH` entries to `authentication.json` for every social provider. + ++ +A sample `conf/identityProvider-google.json` is as follows: ++ +[source, json] +---- +{ + "name" : "google", + "type" : "OPENID_CONNECT", + "enabled" : true, + "authorization_endpoint" : "https://accounts.google.com/o/oauth2/v2/auth", + "token_endpoint" : "https://oauth2.googleapis.com/token", + "userinfo_endpoint" : "https://openidconnect.googleapis.com/v1/userinfo", + "well-known" : "https://accounts.google.com/.well-known/openid-configuration", + "client_id" : "your-client-id", + "client_secret" : "your-client-secret", + "scope" : [ + "openid", + "profile", + "email" + ], + "authenticationId" : "sub", + "propertyMap" : [ + { + "source" : "sub", + "target" : "id" + }, + { + "source" : "email", + "target" : "username" + } + ] +} +---- + ++ +`conf/identityProviders.json` is the configuration of the identity provider service itself. Its `providers` list is the catalog of supported providers (Google, Facebook, and LinkedIn by default): the templates that the Admin UI offers under *Configure > Social ID Providers*, and that `POST /openidm/identityProviders?_action=availableProviders` returns. An entry in the catalog is not an active provider, and client credentials placed there are neither used nor encrypted. Keep the file in place: without it the identity provider service does not start, and no social provider can be configured. + [NOTE] diff --git a/openidm-zip/src/main/resources/conf/identityProviders.json b/openidm-zip/src/main/resources/conf/identityProviders.json new file mode 100644 index 0000000000..d961f130c6 --- /dev/null +++ b/openidm-zip/src/main/resources/conf/identityProviders.json @@ -0,0 +1,142 @@ +{ + "providers" : [ + { + "name" : "google", + "type" : "OPENID_CONNECT", + "icon" : "", + "authorization_endpoint" : "https://accounts.google.com/o/oauth2/v2/auth", + "token_endpoint" : "https://oauth2.googleapis.com/token", + "userinfo_endpoint" : "https://openidconnect.googleapis.com/v1/userinfo", + "well-known" : "https://accounts.google.com/.well-known/openid-configuration", + "client_id" : "", + "client_secret" : "", + "scope" : [ + "openid", + "profile", + "email" + ], + "authenticationId" : "sub", + "propertyMap" : [ + { + "source" : "sub", + "target" : "id" + }, + { + "source" : "name", + "target" : "displayName" + }, + { + "source" : "given_name", + "target" : "givenName" + }, + { + "source" : "family_name", + "target" : "familyName" + }, + { + "source" : "picture", + "target" : "photoUrl" + }, + { + "source" : "email", + "target" : "email" + }, + { + "source" : "email", + "target" : "username" + } + ] + }, + { + "name" : "facebook", + "type" : "OAUTH", + "icon" : "", + "authorization_endpoint" : "https://www.facebook.com/dialog/oauth", + "token_endpoint" : "https://graph.facebook.com/oauth/access_token", + "userinfo_endpoint" : "https://graph.facebook.com/me?fields=id,name,email,first_name,last_name", + "client_id" : "", + "client_secret" : "", + "scope" : [ + "public_profile", + "email" + ], + "authenticationId" : "id", + "propertyMap" : [ + { + "source" : "id", + "target" : "id" + }, + { + "source" : "name", + "target" : "displayName" + }, + { + "source" : "first_name", + "target" : "givenName" + }, + { + "source" : "last_name", + "target" : "familyName" + }, + { + "source" : "email", + "target" : "email" + }, + { + "source" : "email", + "target" : "username" + } + ] + }, + { + "name" : "linkedIn", + "type" : "OAUTH", + "icon" : "", + "authorization_endpoint" : "https://www.linkedin.com/oauth/v2/authorization", + "token_endpoint" : "https://www.linkedin.com/oauth/v2/accessToken", + "userinfo_endpoint" : "https://api.linkedin.com/v2/userinfo", + "client_id" : "", + "client_secret" : "", + "scope" : [ + "openid", + "profile", + "email" + ], + "authenticationId" : "sub", + "propertyMap" : [ + { + "source" : "sub", + "target" : "id" + }, + { + "source" : "name", + "target" : "displayName" + }, + { + "source" : "given_name", + "target" : "givenName" + }, + { + "source" : "family_name", + "target" : "familyName" + }, + { + "source" : "picture", + "target" : "photoUrl" + }, + { + "source" : "email", + "target" : "email" + }, + { + "source" : "email", + "target" : "username" + }, + { + "source" : "locale", + "target" : "locale" + } + ] + } + ] +} From f238c17d6c0fc94c38fc581c551bf1f119ac6721 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Sun, 4 Oct 2026 12:53:54 +0300 Subject: [PATCH 4/4] [#230] Require icon and scope in hand-written provider files and test the catalogue The login, registration and provider-link views compile the provider's icon and join its scope, so an enabled provider file without either stops the login page from rendering. Add the icon to the sample identityProvider-google.json and say that a hand-written file needs both. Add IdentityProvidersJsonTest: the shipped catalogue must exist, keep the google/facebook/linkedIn names the Admin UI keys on, and give each entry an icon, a scope and https endpoints. Unknown keys fail the test, since IdentityProviderService drops them silently. --- .../asciidoc/integrators-guide/chap-auth.adoc | 4 ++ .../script/IdentityProvidersJsonTest.java | 58 +++++++++++++++++++ 2 files changed, 62 insertions(+) create mode 100644 openidm-zip/src/test/java/org/forgerock/openidm/bin/defaults/script/IdentityProvidersJsonTest.java diff --git a/openidm-doc/src/main/asciidoc/integrators-guide/chap-auth.adoc b/openidm-doc/src/main/asciidoc/integrators-guide/chap-auth.adoc index 352801dfd4..38cd6b5fbe 100644 --- a/openidm-doc/src/main/asciidoc/integrators-guide/chap-auth.adoc +++ b/openidm-doc/src/main/asciidoc/integrators-guide/chap-auth.adoc @@ -502,6 +502,7 @@ A sample `conf/identityProvider-google.json` is as follows: "name" : "google", "type" : "OPENID_CONNECT", "enabled" : true, + "icon" : "", "authorization_endpoint" : "https://accounts.google.com/o/oauth2/v2/auth", "token_endpoint" : "https://oauth2.googleapis.com/token", "userinfo_endpoint" : "https://openidconnect.googleapis.com/v1/userinfo", @@ -527,6 +528,9 @@ A sample `conf/identityProvider-google.json` is as follows: } ---- ++ +A hand-written file must include `icon` and `scope`: the login and registration pages render the provider's button from `icon` and build the authorization request from `scope`, and they fail to render while an enabled provider lacks either. The Admin UI copies both from the catalog entry. + + `conf/identityProviders.json` is the configuration of the identity provider service itself. Its `providers` list is the catalog of supported providers (Google, Facebook, and LinkedIn by default): the templates that the Admin UI offers under *Configure > Social ID Providers*, and that `POST /openidm/identityProviders?_action=availableProviders` returns. An entry in the catalog is not an active provider, and client credentials placed there are neither used nor encrypted. Keep the file in place: without it the identity provider service does not start, and no social provider can be configured. diff --git a/openidm-zip/src/test/java/org/forgerock/openidm/bin/defaults/script/IdentityProvidersJsonTest.java b/openidm-zip/src/test/java/org/forgerock/openidm/bin/defaults/script/IdentityProvidersJsonTest.java new file mode 100644 index 0000000000..6953f3f542 --- /dev/null +++ b/openidm-zip/src/test/java/org/forgerock/openidm/bin/defaults/script/IdentityProvidersJsonTest.java @@ -0,0 +1,58 @@ +/* + * The contents of this file are subject to the terms of the Common Development and + * Distribution License (the License). You may not use this file except in compliance with the + * License. + * + * You can obtain a copy of the License at legal/CDDLv1.0.txt. See the License for the + * specific language governing permission and limitations under the License. + * + * When distributing Covered Software, include this CDDL Header Notice in each file and include + * the License file at legal/CDDLv1.0.txt. If applicable, add the following below the CDDL + * Header, with the fields enclosed by brackets [] replaced by your own identifying + * information: "Portions copyright [year] [name of copyright owner]". + * + * Copyright 2026 3A Systems, LLC. + */ +package org.forgerock.openidm.bin.defaults.script; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.fasterxml.jackson.databind.ObjectMapper; +import org.forgerock.json.JsonValue; +import org.forgerock.openidm.idp.config.ProviderConfig; +import org.testng.annotations.Test; + +import java.io.InputStream; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; + +/** + * Checks the catalog of supported identity providers shipped in conf/identityProviders.json. + * + * IdentityProviderService does not start without this file, and the Admin UI offers its entries as + * templates, keyed on the provider name, and copies their icon into the provider's own configuration. + */ +public class IdentityProvidersJsonTest { + @Test + public void testCatalogEntries() throws Exception { + // unlike ProviderConfigMapper, fail on unknown keys: the service would silently drop them + final ObjectMapper mapper = new ObjectMapper(); + final JsonValue catalog; + try (final InputStream configStream = getClass().getResourceAsStream("/conf/identityProviders.json")) { + assertThat(configStream).isNotNull(); + catalog = new JsonValue(mapper.readValue(configStream, Map.class)); + } + final List names = new ArrayList<>(); + for (JsonValue entry : catalog.get("providers")) { + final ProviderConfig config = mapper.convertValue(entry.getObject(), ProviderConfig.class); + names.add(config.getName()); + assertThat(config.getIcon()).isNotEmpty(); + assertThat(config.getScope()).isNotEmpty(); + assertThat(config.getAuthorizationEndpoint()).startsWith("https://"); + assertThat(config.getTokenEndpoint()).startsWith("https://"); + assertThat(config.getUserInfoEndpoint()).startsWith("https://"); + } + assertThat(names).containsExactly("google", "facebook", "linkedIn"); + } +}