[#225] Bind identity provider configs through the DS bind methods

[vharseko]

Fixes #225

Note: #231, merged on 2026-10-06, was stacked on this PR's first two commits and brought them to master: the DS bind methods in IdentityProviderService and AuthenticationService, the atomic publishing of the rebuilt auth state, notifyListeners() notifying every listener, the skipping of unsupported provider types and the Check DS descriptors CI step. The branch is rebased onto that master, so the diff now holds the rest: SelfService bound through its DS bind methods, the resolver type set once during the rebuild, failed activations leaving nothing to rebuild, provider configs without a type, synchronized bind/unbind, and the tests below. The description still covers the whole fix.

Problem

IdentityProviderService declared its @Reference to IdentityProviderConfig on a Map<String, List<IdentityProviderConfig>> field. Declarative Services cannot inject that type, so SCR rejected the reference (Field identityProviders ... has unsupported type java.util.Map) and bindIdentityProviderConfig / unbindIdentityProviderConfig were never called. As a result /identityProviders always answered {"providers":[]}, no OPENID_CONNECT / OAUTH auth module was generated, and the social self-service stage received no providers.

The annotations were moved onto the fields in 5aa45c8 (the Felix SCR → OSGi DS migration). The same thing happened in AuthenticationService: its @Reference to IdentityProviderService is on the field, and because the field type is valid, SCR injects it without logging anything. bindIdentityProviderService, which registers the provider listener, was never called either, so provider changes never reached the authentication filter. SelfService has the same field-level reference. It registered as a listener only if IdentityProviderService was already bound when it activated, so a service that arrived later or replaced the bound one never reached the social self-service stage.

Changes

• IdentityProviderService: the reference is declared on bindIdentityProviderConfig, with an explicit unbind and the same reference name identityProviders. Providers are added atomically (computeIfAbsent + CopyOnWriteArrayList). A provider config without a type is ignored with a warning instead of failing with an NPE. getIdentityProviderByType now returns an empty list for a type with no bound provider. Before this change that path could not be reached; with providers bound it would have thrown an NPE.
• AuthenticationService: the reference is declared on bindIdentityProviderService, and unbindIdentityProviderService now takes the service argument that DS requires. Both methods are synchronized and both rebuild the social auth modules. The rebuild does nothing before activation, and covers IdentityProviderService arriving after AuthenticationService has already activated.
• SelfService: the same change. The reference is declared on bindIdentityProviderService under the same name identityProviderService, with an explicit unbindIdentityProviderService. Both are synchronized and both rebuild, and the rebuild registers the listener on the new service. Without a bound service, the socialUserDetails stage gets an empty provider list instead of keeping the previous one.

Generated descriptors:

<reference name="identityProviders" cardinality="0..n" policy="dynamic" interface="org.forgerock.openidm.idp.impl.IdentityProviderConfig" bind="bindIdentityProviderConfig" unbind="unbindIdentityProviderConfig"/>
<reference name="identityProviderService" cardinality="0..1" policy="dynamic" interface="org.forgerock.openidm.idp.impl.IdentityProviderService" bind="bindIdentityProviderService" unbind="unbindIdentityProviderService"/>

The second line is the same in org.forgerock.openidm.authentication.xml and org.forgerock.openidm.selfservice.xml.

Binding the providers makes the listener rebuilds live: they now run on DS bind threads, concurrently with requests. The rest of the PR hardens that path:

• AuthenticationService.identityProviderConfigChanged() builds the amended config and the authenticators locally. It sets the resolver type of the OPENID_CONNECT / OAUTH modules there, once, and publishes both through volatile fields only after setFilter succeeds. A module whose resolvers is not a non-empty list starting with a map keeps no type, because the loop also covers disabled modules and must not fail the rebuild. The loop selects these modules with a null-safe name check, so a module configured by className alone does not fail it either. The rebuild, activate and deactivate are synchronized. Before this, a reauthenticate request could hit a ConcurrentModificationException or an empty authenticator list, readInstance could see a half-amended config, and readInstance / getIdentityProviderConfig wrote type into the shared config from request threads.
• A failed activate clears the configuration in AuthenticationService and SelfService, and SelfService also unregisters its listener. DS calls no deactivate after a failed activate, but it still unbinds the references. Without this, the unbind rebuild of a dead instance could install an authentication filter or register a self-service handler.
• SelfService.identityProviderConfigChanged() is synchronized as well, so its unregister/register pair cannot leak a registration. It also ignores a change that arrives without configuration.
• IdentityProviderService.notifyListeners() notifies every listener and rethrows the first failure, instead of leaving the remaining listeners with the old provider set.
• AuthenticationService.amendAuthConfig() skips providers whose type is neither OPENID_CONNECT nor OAUTH, with a warning. Otherwise one such provider would fail the whole authentication configuration, including AuthenticationService activation.
• CI: a Check DS descriptors step in build.yml checks the three descriptors above in the built jars.

Tests

• IdentityProviderServiceTest: added tests for:

  • a lookup by a type with no bound providers;
  • unbind (the provider is removed and listeners are notified);
  • a failing listener not stopping the others, with each listener taking the failing role in turn, since the listener map iterates in key-hash order;
  • a provider without a type being ignored.

• AuthenticationServiceTest: added tests for:

  • bind registering the listener;
  • unbind unregistering it and no longer injecting providers;
  • bind/unbind rebuilding the auth modules, including the DS replacement order;
  • providers of an unsupported type being skipped;
  • a rebuild publishing its config only after the filter is set: the resolver type is already in the published config before any read, and a second rebuild leaves the values the first one published untouched;
  • a failed activation leaving nothing for the unbind to rebuild, both for an IdentityProviderServiceException and for a RuntimeException;
  • a disabled OPENID_CONNECT module with malformed resolvers ([], {}, a non-map entry) not failing the activation;
  • a module configured by className alone, after an enabled SOCIAL_PROVIDERS template, failing neither the activation nor the read;
  • a read leaving the resolvers of the published config unchanged.

  The existing tests now call setConfig after bind, which is the order SCR uses (bind before activate).

• SelfServiceTest: added tests for:

  • a provider change without configuration being ignored;
  • bind/unbind rebuilding, including the replacement order;
  • unbind unregistering the listener;
  • the providers of an unbound service being dropped, with nothing unregistered when there is no component context;
  • a failed activation leaving no configuration to rebuild, and unregistering the listener that amendConfig already registered.

• The unit tests call bind* directly and so do not exercise the SCR wiring; the new CI step checks the descriptors in the built jars.

[maximthomas]

praise: The fix goes where the bug is, and the built bundles show it.

• A HEAD build generates bind="bindIdentityProviderConfig" unbind="unbindIdentityProviderConfig" (OSGI-INF/org.forgerock.openidm.identityProviders.xml) and bind="bindIdentityProviderService" unbind="unbindIdentityProviderService" (OSGI-INF/org.forgerock.openidm.authentication.xml). The pre-PR jars had field="identityProviders" field-option="update" and field="identityProviderService".
• unbindIdentityProviderService clears the field only when the departing service is the bound one (AuthenticationService.java:264), which is correct for the DS replacement order: bind the new service, then unbind the old one.
• getIdentityProviderByType falls back to getOrDefault (IdentityProviderService.java:205), which removes the NPE on a type with no bound provider.

---

issue (non-blocking): identityProviderConfigChanged() rebuilds authenticators and amendedConfig in place without a lock, and it now runs on DS bind threads while request threads read both fields.

openidm-authnfilter/src/main/java/org/forgerock/openidm/auth/AuthenticationService.java:497-523, :217-220, :734, :792, :822

At BASE this method ran only from activate(). With this PR it also runs from bind/unbindIdentityProviderService and from IdentityProviderService.notifyListeners() on every IdentityProviderConfig bind or unbind. If a provider is added or removed while a reauthenticate action is iterating authenticators (L734), clear()/addAll() on the plain ArrayList either throws ConcurrentModificationException or the loop sees an empty list and returns 403. readInstance (L822) can also read amendedConfig half-amended: SOCIAL_PROVIDERS already removed, provider modules not yet added. This is the same shape as before 5aa45c8, so it is narrow, but it is reachable again. The change below compiles at the head, and AuthenticationServiceTest stays 9/9.

    private volatile JsonValue amendedConfig;

    /** The authenticators to delegate to.*/
    private volatile List<Authenticator> authenticators = new ArrayList<>();

    @Override
    public synchronized void identityProviderConfigChanged() throws IdentityProviderServiceException {
        if (config == null) {
            logger.debug("No configuration for Authentication Service");
            return;
        }
        final JsonValue newAmendedConfig = config.copy();
        // the auth module list config lives under at /serverAuthConfig/authModule
        final JsonValue authModuleConfig = newAmendedConfig.get(SERVER_AUTH_CONTEXT_KEY).get(AUTH_MODULES_KEY);
        amendAuthConfig(authModuleConfig);

        try {
            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);
        }

        // publish complete values only; request threads read both fields without a lock
        amendedConfig = newAmendedConfig;
        authenticators = FluentIterable.from(authModuleConfig)
                .filter(enabledAuthModules)
                .transform(toModuleProperties)
                .filter(authModulesThatHaveValidAuthenticatorProperties)
                .transform(toAuthenticatorFromProperties)
                .toList();
    }

    // in deactivate(): replaces authenticators.clear()
        authenticators = new ArrayList<>();

---

issue (non-blocking): notifyListeners() stops at the first listener that throws, so the remaining listeners keep the old provider set.

openidm-identity-provider/src/main/java/org/forgerock/openidm/idp/impl/IdentityProviderService.java:359-363

This loop was dead at BASE and is live now. By the time it runs, the provider has already been added to or removed from the map. Example: an identityProvider-*.json whose type is not in IDMAuthModule. IdentityProviderConfig.activate does not check the type, so IDMAuthModule.valueOf throws IllegalArgumentException inside AuthenticationService's rebuild (AuthenticationService.java:459) when SOCIAL_PROVIDERS is enabled. If AuthenticationService comes first in ConcurrentHashMap order, the self-service social stage never receives this change or any later one. The change below compiles at the head, and IdentityProviderServiceTest stays 3/3.

    public void notifyListeners() throws IdentityProviderServiceException {
        IdentityProviderServiceException failure = null;
        for (IdentityProviderListener listener : identityProviderListeners.values()) {
            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;
        }
    }

---

suggestion (non-blocking): No test covers the rebuild calls in bindIdentityProviderService/unbindIdentityProviderService, or the replacement guard.

openidm-authnfilter/src/main/java/org/forgerock/openidm/auth/AuthenticationService.java:258, :264-266

Every AuthenticationServiceTest case binds or unbinds while config is still null, so identityProviderConfigChanged() returns at L498. The only unbind test unbinds the same mock it bound. If L258 or L266 is deleted, or the guard is dropped, the suite stays 8/8 green. The case below passes at the head (9/9) and fails on each of the three mutants: L258 gives "Wanted but not invoked", L266 gives TooLittleActualInvocations, the dropped guard gives TooManyActualInvocations. It needs static imports of doNothing, spy and times.

    @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();
    }

---

suggestion (non-blocking): Nothing pins the DS descriptors this PR fixes. Moving @Reference back onto the field keeps every check green.

openidm-identity-provider/src/main/java/org/forgerock/openidm/idp/impl/IdentityProviderService.java:150-156, openidm-authnfilter/src/main/java/org/forgerock/openidm/auth/AuthenticationService.java:248-253

As the description says, the unit tests call bind* directly. bnd writes OSGI-INF only into the jar, so no test sees it. The BASE descriptor passed every CI leg, because no sample configures an identity provider and no e2e spec reads /identityProviders. A check after the Maven build exits 0 on the HEAD jars and 1 on the pre-PR jars:

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"'

[vharseko]

Thanks for the review. All four points are addressed in 2202ea9 (the branch is also rebased onto the current master):

1. identityProviderConfigChanged() without a lock: taken as proposed. The amended config and the authenticators are built locally and published through volatile fields, and the method is synchronized. activate/deactivate are synchronized as well, so a rebuild can neither race deactivate nor read config while it changes. The same unsynchronized rebuild had become live in SelfService.identityProviderConfigChanged() (unregisterServiceRegistration() + registerService(...), which could leak a registration). It is synchronized there too and ignores a change that arrives without configuration.
2. notifyListeners() stops at the first failing listener: taken as proposed. In addition, amendAuthConfig() now skips providers whose type is neither OPENID_CONNECT nor OAUTH, with a warning. With providers bound before activate, such a provider would otherwise fail the AuthenticationService activation entirely, and a valid non-social IDMAuthModule name (e.g. MANAGED_USER) hit the default -> null branch of SocialAuthModuleConfigFactory.
3. Test for the bind/unbind rebuilds: your test is added as is. New tests also cover skipping unsupported provider types, notifying every listener when one fails, and SelfService ignoring a change without configuration.
4. DS descriptor check: added as a Check DS descriptors step in build.yml right after the Maven build (Unix runners).

Locally: IdentityProviderServiceTest 4/4, openidm-authnfilter 41/41 (AuthenticationServiceTest 10/10), SelfServiceTest 2/2. The descriptor check passes on the built jars.

[maximthomas]

praise: The second commit fixes both round-1 issues where they are.

• IdentityProviderService.notifyListeners() now catches each listener's failure, logs it with the listener name and rethrows the first one after the loop (IdentityProviderService.java:359-376).
• AuthenticationService.identityProviderConfigChanged() builds newAmendedConfig and the authenticators locally and publishes them only after setFilter succeeds (AuthenticationService.java:521-544). After a failed rebuild, readInstance therefore keeps describing the filter that is actually in force.
• The Check DS descriptors step in build.yml checks bind=/unbind= in the built jars, and it passed on the green legs.

---

issue (non-blocking): notifyListenersShouldNotifyEveryListenerWhenOneFails cannot fail on the stop-at-first-failure regression it was written for.

openidm-identity-provider/src/test/java/org/forgerock/openidm/idp/impl/IdentityProviderServiceTest.java:137-157, openidm-identity-provider/src/main/java/org/forgerock/openidm/idp/impl/IdentityProviderService.java:359-376

The listeners live in a default-capacity ConcurrentHashMap, which iterates in key-hash order: "healthy" falls in bin 6 and "failing" in bin 8 whatever the insertion order. The healthy listener is therefore always notified before the failing one throws. Replacing the catch body with throw new IdentityProviderServiceException(e.getMessage(), e); leaves the class green (4/4), even though the comment at L154 and the description ("a failing listener not stopping the others") both say this case is covered. The version below gives the failing role to each name in turn, so one of the two runs puts the failing listener first. Measured: green at the head, and the same mutant then fails it with "Wanted but not invoked" on verify(healthy).

    @Test
    public void notifyListenersShouldNotifyEveryListenerWhenOneFails() throws Exception {
        // 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();
        }
    }

---

question (non-blocking): Did you leave SelfService's late-arrival and replacement case out on purpose, or should it get the same bind/unbind treatment as AuthenticationService?

openidm-selfservice/src/main/java/org/forgerock/openidm/selfservice/impl/SelfService.java:137-141, :186-189

identityProviderService is still a field-level @Reference. DS never calls bindIdentityProviderService, and there is no unbind. SelfService registers as a listener only inside amendConfig, and only when the field is non-null at that moment. So if IdentityProviderService activates after SelfService, or is replaced (it has a REQUIRE config and no @Modified, and the old instance's deactivate clears its listeners), SelfService never registers on the live instance. The social stage then keeps its last provider list, empty or stale, while AuthenticationService follows every change. At this head no shipped conf or sample boots both services, so this is Minor; if the PR is meant to close the self-service half of #225 under any start-up order, it is Major. A possible fix (not compiled):

    private volatile 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; amendConfig registers this listener on the new service
        identityProviderConfigChanged();
    }

    synchronized void unbindIdentityProviderService(IdentityProviderService identityProviderService)
            throws IdentityProviderServiceException {
        identityProviderService.unregisterIdentityProviderListener(this);
        if (this.identityProviderService == identityProviderService) {
            this.identityProviderService = null;
            identityProviderConfigChanged();
        }
    }

---

suggestion (non-blocking): No test runs AuthenticationService.identityProviderConfigChanged() past its config == null guard, so none of the publish-after-success hardening is pinned.

openidm-authnfilter/src/main/java/org/forgerock/openidm/auth/AuthenticationService.java:515-544, :569, openidm-authnfilter/src/test/java/org/forgerock/openidm/auth/AuthenticationServiceTest.java:273-288

In AuthenticationServiceTest, every bind and unbind runs before setConfig on a fresh instance, the spy test stubs the method with doNothing(), and no test calls activate or deactivate. Each of the following keeps the suite green: publishing amendedConfig before setFilter, going back to authenticators.clear()/addAll (on the ImmutableList, that throws UnsupportedOperationException from the second rebuild on), or dropping synchronized/volatile.

Pin: set the config, bind a mock IdentityProviderService that returns one provider, put a mock AuthFilterWrapper on the private field, and call identityProviderConfigChanged() twice; this kills the clear()/addAll revert. Then make setFilter throw a RuntimeException on a third call with a changed provider list, and assert that readInstance still shows the second call's modules; this kills publish-before-setFilter.

---

issue (non-blocking): A SelfService activation that fails after amendConfig leaves the instance registered as a listener with config set, so the new guard does not stop it.

openidm-selfservice/src/main/java/org/forgerock/openidm/selfservice/impl/SelfService.java:154-178, :189, :347, :363-373

activate sets config (L158). amendConfig then registers the listener (L189), and only after that do build() and registerService run. If either of them throws, DS calls no deactivate. A later provider change then runs the dead instance's rebuild past the config == null guard, and that rebuild can register a RequestHandler that nothing unregisters. Not reproduced: this needs a transient or provider-dependent failure after L189. Registering the listener after a successful build, or unregistering it and clearing config in activate's catch, would close it.

---

issue (non-blocking): readInstance and getIdentityProviderConfig still write into the published amendedConfig through setType.

openidm-authnfilter/src/main/java/org/forgerock/openidm/auth/AuthenticationService.java:380, :346-350, :811-827, :842-861

setType calls resolvers.get(0).put("type", ...) on the shared config from request threads. For a static OPENID_CONNECT/OAUTH module whose resolver has no type, the first put inserts a new key, and that insert can race with another request copying the same map. Publishing only complete values does not make the published config read-only. The code is unchanged from BASE. Generated modules carry type, so for them the put only replaces a value. Not run. Applying the resolvers to amendedConfig.copy(), or setting type once during the rebuild, removes the write.

---

suggestion (non-blocking): Make bindIdentityProviderService and unbindIdentityProviderService synchronized.

openidm-authnfilter/src/main/java/org/forgerock/openidm/auth/AuthenticationService.java:252-268

unbind's this.identityProviderService == identityProviderService check and its write of null run outside the monitor that identityProviderConfigChanged() takes. If a bind of the new service on another thread runs between the check and the write, its effect is lost: the field ends up null while the new service holds the listener. I have not shown that SCR lets these calls overlap. The rebuild is already synchronized and the monitor is reentrant, so adding the keyword to both methods is enough.

---

suggestion (non-blocking): Reject a provider config without type with a message instead of a raw NPE from ConcurrentHashMap.

openidm-identity-provider/src/main/java/org/forgerock/openidm/idp/impl/IdentityProviderService.java:158-159, :165-167

IdentityProviderConfig.activate does not check type, so a hand-written identityProvider-x.json without one fails computeIfAbsent(null, ...) on the DS bind thread. No valid provider is lost; only the error is poor. unbindIdentityProviderConfig needs the same early return before identityProviders.get(type).

    protected void bindIdentityProviderConfig(final IdentityProviderConfig config)
            throws IdentityProviderServiceException {
        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();
    }

---

suggestion (non-blocking): The assertion in identityProviderConfigChangedShouldIgnoreChangeWithoutConfiguration cannot fail; the guard is pinned only by the NPE it prevents.

openidm-selfservice/src/test/java/org/forgerock/openidm/selfservice/impl/SelfServiceTest.java:96-105, openidm-selfservice/src/main/java/org/forgerock/openidm/selfservice/impl/SelfService.java:347

With config == null, registerIdentityProviderListener is unreachable whether or not the guard is there, because without the guard amendConfig(null) throws first, at L182. Asserting on the first call past the guard makes the test fail through the assertion. That call is the eagerly evaluated debug-log argument, so the assertion also catches a guard moved below it.

        verify(identityProviderService, never()).getIdentityProviders();

[vharseko]

All points of the second round are addressed in c3cc6d8 and 177284d. The branch is also rebased onto the current master.

1. notifyListenersShouldNotifyEveryListenerWhenOneFails cannot fail: confirmed. With the default capacity, "healthy" falls in bin 6 and "failing" in bin 8, so the healthy listener always ran first. I took your version, which gives each name the failing role in turn. With the catch body replaced by a throw, it now fails on verify(healthy).

2. SelfService late arrival and replacement: I did not leave it out on purpose, so it now gets the same treatment, in a separate commit (177284d). The reference is declared on bindIdentityProviderService under the same name identityProviderService, with an explicit unbindIdentityProviderService. Both are synchronized and both rebuild, and the rebuild registers the listener on the new service. Your sketch needed two changes:

   • unbind unregisters only when there is a component context. getListenerName() is ComponentContextUtil.getFullPid(context), which throws an NPE on a null context.
   • amendConfig amends config in place. If the service is gone, the old provider list would therefore stay in the socialUserDetails stage, so the stage now gets an empty list in that case. A side effect: without a service the stage used to get no providers at all, and SocialUserDetailsStage calls config.getProviders().size().

   The Check DS descriptors step now covers org.forgerock.openidm.selfservice.xml as well. New tests cover the bind/unbind rebuilds (including the replacement order), the unregistering, and the dropped providers.

3. Nothing pins the publish-after-success hardening: identityProviderConfigChangedShouldPublishOnlyAfterTheFilterIsSet follows your outline. configureAuthenticationFilter is now package-private so that the test can stub it, and the test sets authFilterWrapper by reflection. The test kills both "publish amendedConfig before setFilter" and "no resolver type set during the rebuild" (see 5). One limit: reverting to clear()/addAll on a mutable list keeps it green. That revert is a race, and a single-threaded test cannot show it. The test kills only the variant that mutates an already published ImmutableList.

4. A failed SelfService activation leaves the listener registered: confirmed. Felix SCR 2.1.20 calls no deactivate after a failed activate, but it does unbind the references (SingleComponentManager, the 112.5.8 branch). The activate catch now clears config and unregisters the listener. The same check turned up the matching gap in AuthenticationService, which this PR introduced. There, the unbind after a failed activation rebuilds, and if that rebuild succeeds, a dead instance calls authFilterWrapper.setFilter(...). activate now clears config before rethrowing. failedActivationShouldLeaveNothingForUnbindToRebuild pins this: the first filter fails and a later one would succeed.

5. setType writes into the published amendedConfig: the resolver type of every OPENID_CONNECT/OAUTH module is now set once, during the rebuild, before publishing. resolvers no longer calls setType. The .transform(setType) in getIdentityProviderConfig ran on resolvers rather than modules and changed nothing, so it is removed. Request threads now only read the published config.

6. synchronized bind/unbind in AuthenticationService: done.

7. Provider config without type: done as proposed. Bind ignores it with a warning, and unbind returns early. providerWithoutTypeShouldBeIgnored covers both.

8. The SelfService guard assertion cannot fail: it now asserts verify(identityProviderService, never()).getIdentityProviders().

Locally: openidm-identity-provider 5/5, openidm-authnfilter 43/43, openidm-selfservice 5/5. All three descriptors check out in the built jars.

[maximthomas]

praise: The second-round items are all fixed, and the auth rebuild now publishes only finished state.

• AuthenticationService.identityProviderConfigChanged builds newAmendedConfig and the authenticators locally and publishes both volatile fields only after authFilterWrapper.setFilter (:527 → :549-550). The resolver type is written there once (:543), so readInstance no longer writes into the published config.
• SelfService.bindIdentityProviderService / unbindIdentityProviderService (:140-163) are synchronized DS bind methods, and unbind checks which instance is bound, which handles DS's bind-new-then-unbind-old replacement. A late or replaced IdentityProviderService now reaches self-service.
• IdentityProviderService now skips a provider without type with a WARN instead of failing with a raw NPE from ConcurrentHashMap.

---

issue (non-blocking): assertProviders cannot tell a resolver type set by the rebuild from one written by the read, so the test does not pin "the resolver type set during the rebuild".

openidm-authnfilter/src/test/java/org/forgerock/openidm/auth/AuthenticationServiceTest.java:395, AuthenticationService.java:349, :543-545, :868

Every type assertion goes through readInstance, and readInstance applies the resolvers function. Take a mutant that fully reverts the move: it puts setType.apply(jsonValue) back into resolvers and deletes the rebuild loop. That brings back the request-thread write into the published amendedConfig, and AuthenticationServiceTest still passes 12/12. Only deleting the loop alone fails. The PR description lists this test as covering the change, so it is worth an assertion on the published field before any read.

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);
}

// in identityProviderConfigChangedShouldPublishOnlyAfterTheFilterIsSet, right after the first rebuild:
// the type is in the published config before any request reads it
final JsonValue published = (JsonValue) getField(service, "amendedConfig");
for (final JsonValue module : published.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);
    }
}

Pin: with this assertion the revert mutant fails on explicit-oidc, whose type the test removes.

---

issue (non-blocking): The rebuild's setType loop runs on disabled and malformed OIDC/OAUTH modules after setFilter. A config that activated before this PR now fails activation and leaves the new filter installed.

openidm-authnfilter/src/main/java/org/forgerock/openidm/auth/AuthenticationService.java:543-545, :311-318, :374-385, :562-568

The loop has no enabledAuthModules filter. setType calls resolvers.get(0).put(...) whenever resolvers is not null, so "resolvers": [] or {} on a disabled OPENID_CONNECT module throws JsonValueException: Expecting a Map or List. Before this PR that module activated fine and only the social-login read failed. Now activate nulls config and rethrows. No deactivate follows, so the filter set at :527 stays in AuthFilterWrapper. A module with only a className, which processModuleConfiguration accepts, reaches the same road through oidcAndOauth2Modules' get(name).asString().equals(...) and fails with an NPE. No shipped config has either shape, so this is not blocking. Keep the loop over all modules, since getIdentityProviderConfig (:828) reads disabled ones too, and make both helpers tolerant:

public static final Predicate<JsonValue> oidcAndOauth2Modules =
        new Predicate<JsonValue>() {
            @Override
            public boolean apply(JsonValue jsonValue) {
                final String name = jsonValue.get(AUTH_MODULE_NAME_KEY).asString();
                return IDMAuthModule.OPENID_CONNECT.name().equals(name)
                        || IDMAuthModule.OAUTH.name().equals(name);
            }
        };

private static final Function<JsonValue, JsonValue> setType = new Function<JsonValue, JsonValue>() {
    @Override
    public JsonValue apply(JsonValue jsonValue) {
        final JsonValue resolvers = jsonValue.get(AUTH_MODULE_PROPERTIES_KEY).get(AUTH_MODULE_RESOLVERS_KEY);
        // currently we only support one resolver per auth module; a malformed one must not fail the rebuild
        if (resolvers.isList() && resolvers.size() > 0 && resolvers.get(0).isMap()) {
            return resolvers.get(0).put("type", jsonValue.get(AUTH_MODULE_NAME_KEY).asString());
        }
        return jsonValue;
    }
};

Pin: activate with a config that adds {"name": "OPENID_CONNECT", "enabled": false, "properties": {"resolvers": []}} must succeed.

---

suggestion (non-blocking): Nothing checks "a second rebuild must not modify the values the first one published".

openidm-authnfilter/src/test/java/org/forgerock/openidm/auth/AuthenticationServiceTest.java:342, AuthenticationService.java:520

The test asserts only the end state after both rebuilds. With final JsonValue newAmendedConfig = config; (no copy) it still passes 12/12, because setType is idempotent and amendAuthConfig returns early once SOCIAL_PROVIDERS is gone.

service.identityProviderConfigChanged();
final JsonValue first = (JsonValue) getField(service, "amendedConfig");
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);

Pin: the no-copy mutant publishes config twice and fails isNotSameAs.

---

suggestion (non-blocking): SelfService.activate's failed-activation catch is untested.

openidm-selfservice/src/main/java/org/forgerock/openidm/selfservice/impl/SelfService.java:197-205

SelfServiceTest never calls activate(). Deleting config = null; passes 5/5, and so does deleting the unregisterIdentityProviderListener block. AuthenticationService has the equivalent pin; SelfService does not.

@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("");
    final Field enhancedConfigField = SelfService.class.getDeclaredField("enhancedConfig");
    enhancedConfigField.setAccessible(true);
    enhancedConfigField.set(selfService, enhancedConfig);
    try {
        selfService.activate(mock(ComponentContext.class));
        fail("Expected IllegalArgumentException");
    } catch (IllegalArgumentException e) {
        // expected
    }

    selfService.bindIdentityProviderService(mock(IdentityProviderService.class));

    verify(selfService, never()).amendConfig(any(JsonValue.class));
}

Pin: the case above kills the config = null deletion. The unregister deletion needs a failure after amendConfig has registered the listener: a socialUserDetails stage and a ComponentContext whose getBundleContext() returns null, then verify(identityProviderService).unregisterIdentityProviderListener(selfService).

---

suggestion (non-blocking): The context != null guard in unbindIdentityProviderService is unpinned.

openidm-selfservice/src/main/java/org/forgerock/openidm/selfservice/impl/SelfService.java:156-158

The real IdentityProviderService.unregisterIdentityProviderListener calls getListenerName(), which calls ComponentContextUtil.getFullPid(null) and throws an NPE. The null-context unbinds in the tests use mock(IdentityProviderService.class), and nothing verifies them. Making the unregister unconditional still passes 5/5.

// in amendConfigShouldDropProvidersOfUnboundService, after the unbind without a ComponentContext:
// nothing was registered, so nothing may be unregistered
verify(identityProviderService, never()).unregisterIdentityProviderListener(selfService);

---

suggestion (non-blocking): The | RuntimeException arm of AuthenticationService.activate's catch is unpinned.

openidm-authnfilter/src/main/java/org/forgerock/openidm/auth/AuthenticationService.java:565

failedActivationShouldLeaveNothingForUnbindToRebuild throws only AuthenticationException, which the rebuild wraps as IdentityProviderServiceException. Narrowing the catch to catch (IdentityProviderServiceException e) passes 12/12. The RuntimeException arm handles a JsonValueException from setType and an IllegalStateException from setFilter.

@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");
    }
    service.unbindIdentityProviderService(identityProviderService);

    verify(authFilterWrapper, never()).setFilter(any(Filter.class));
}

Pin: with the narrowed catch, config survives, the unbind rebuilds, and setFilter is called.

[vharseko]

The third round is addressed in f035bab. The branch is rebased onto the current master first. #231, merged today, already carried this PR's first two commits (1d835ae06, 3df071f28) into master, so they are gone from the branch and the PR diff now shows only the second-round commits plus this one.

1. assertProviders cannot tell the rebuild's resolver type from one written by the read: confirmed. identityProviderConfigChangedShouldPublishOnlyAfterTheFilterIsSet now checks the type on the published amendedConfig right after the first rebuild, before any read. The revert mutant (setType back in resolvers, rebuild loop deleted) now fails on explicit-oidc.
2. setType fails the activation on a disabled module with malformed resolvers: confirmed, and introduced in the second round. setType now writes the type only when resolvers is a non-empty list whose first entry is a map. The loop still covers all OPENID_CONNECT/OAUTH modules, since getIdentityProviderConfig reads disabled ones too. activationShouldTolerateMalformedResolversOfDisabledModules activates with [], {} and ["not a resolver"] on a disabled OPENID_CONNECT module. It fails on the previous setType.
   The module with only a className is not a regression of this PR: getSocialAuthTemplate() calls .asString().equals(...) on every module's name at each activation, unchanged from master, so that config already failed activation there, before setFilter. I left oidcAndOauth2Modules as is.
3. Nothing checks that a second rebuild leaves the first one's values alone: added as proposed (isNotSameAs plus the snapshot comparison). The no-copy mutant fails isNotSameAs.
4. SelfService.activate's failed-activation catch is untested: failedActivationShouldLeaveNoConfigurationToRebuild is your case and kills the config = null deletion. failedActivationShouldUnregisterTheListener covers the other half: activation fails after amendConfig has registered the listener (a socialUserDetails stage, no bundle context), and the test verifies the unregister. Deleting the unregister block fails it.
5. The context != null guard in unbindIdentityProviderService is unpinned: amendConfigShouldDropProvidersOfUnboundService now verifies that nothing is unregistered after an unbind without a component context. Making the unregister unconditional fails it.
6. The | RuntimeException arm of AuthenticationService.activate is unpinned: failedActivationOnARuntimeExceptionShouldLeaveNothingForUnbindToRebuild is added as proposed. Narrowing the catch to IdentityProviderServiceException fails it.

Locally: openidm-identity-provider 5/5, openidm-authnfilter 45/45, openidm-selfservice 7/7. Each mutant named above fails exactly the test written for it.

[vharseko]

Rebased onto the current master (6d183a1d9) to pick up #249. CI on the previous head f035bab8f failed only because master itself did not compile at that point: openidm-repo-orientdb stopped at DocumentUtil.java:[106,33] cannot find symbol: variable topLevel after #245, in every build-maven job, and both Docker jobs then had no artifact to download.

The three commits are unchanged: git range-diff marks all of them =. The new head is 0441ed0f5, and CI is running on it. The third-round fixes are the ones described in my previous comment.

[maximthomas]

praise: Round 4 closes the malformed-resolver half of the activation regression and takes the published-config pin.

• setType writes the type only when resolvers.isList() && resolvers.size() > 0 && resolvers.get(0).isMap() (AuthenticationService.java:380), so a disabled module with [], {} or a non-map entry no longer fails activation; activationShouldTolerateMalformedResolversOfDisabledModules covers the three shapes.
• identityProviderConfigChangedShouldPublishOnlyAfterTheFilterIsSet now reads the published amendedConfig (AuthenticationServiceTest.java:343-349), so a deleted rebuild loop fails it on explicit-oidc.

---

issue (non-blocking): A className-only auth module after an enabled SOCIAL_PROVIDERS module still fails the activation and leaves the new filter installed.

openidm-authnfilter/src/main/java/org/forgerock/openidm/auth/AuthenticationService.java:544, :315-316

The loop at :544 runs every module through oidcAndOauth2Modules, and its jsonValue.get(AUTH_MODULE_NAME_KEY).asString().equals(...) throws an NPE on a module that has className and no name, which processModuleConfiguration accepts. With authModules = [{name: SOCIAL_PROVIDERS, enabled: true}, {className: "com.example.Custom"}] the NPE comes after setFilter (:528). Activation fails, config is cleared, and the new filter stays in AuthFilterWrapper with nothing published. A probe at the head confirmed this: NPE in AuthenticationService$5.apply:315, setFilter recorded. The same config activated at the base, where only the social-login read failed. No shipped authentication.json uses className. A className-only module placed before SOCIAL_PROVIDERS, or in a config without it, already failed at the base in getSocialAuthTemplate (:396).

public boolean apply(JsonValue jsonValue) {
    final String name = jsonValue.get(AUTH_MODULE_NAME_KEY).asString();
    return IDMAuthModule.OPENID_CONNECT.name().equals(name) || IDMAuthModule.OAUTH.name().equals(name);
}

This also makes readInstance (:869) and getIdentityProviderConfig (:830) null-safe on such a module.

---

suggestion (non-blocking): Nothing pins that a read no longer writes the resolver type into the published config.

openidm-authnfilter/src/test/java/org/forgerock/openidm/auth/AuthenticationServiceTest.java:457, openidm-authnfilter/src/main/java/org/forgerock/openidm/auth/AuthenticationService.java:349, :833

The published-field check (:343-349) catches a deleted rebuild loop but not a restored read-path write. Put setType.apply(jsonValue) back into resolvers (:349) and .transform(setType) back into getIdentityProviderConfig (:833), keep the loop, and AuthenticationServiceTest stays green 14/14 (measured twice). The read writes the same value the rebuild already wrote, so no assertion can see the difference. The comment at :457 ("the type is set during the rebuild, not by the read") claims more than the case checks.

@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();
    }
}

Pin: under the mutant above, the read puts type back on the enabled oauth and oidc resolvers of config/amendedAuthentication.json and the case fails. At the head readInstance (:857-876) writes nothing and it passes. The case was traced by reading and has not been run.

[vharseko]

Both points of the fourth round are addressed in a93e4e2. The branch is rebased onto the current master (9d4b5a123) first.

1. A className-only module after an enabled SOCIAL_PROVIDERS fails the activation: confirmed, and my answer in the third round was wrong. I wrote that getSocialAuthTemplate() reads the name of every module, but it returns at the first enabled SOCIAL_PROVIDERS, so a className-only module after it activated fine at the base. The rebuild loop added in the second round is what reaches it now, after setFilter. oidcAndOauth2Modules now compares with IDMAuthModule.X.name().equals(name) as you proposed, which also keeps readInstance and getIdentityProviderConfig from failing on such a module. activationShouldTolerateAModuleConfiguredByClassNameOnly activates with [SOCIAL_PROVIDERS (enabled), {className: ...}] and reads /authentication. The previous predicate fails it with the NPE.
2. Nothing pins that a read no longer writes the resolver type: readInstanceShouldNotWriteTheResolverTypeIntoThePublishedConfig is added as proposed. With setType.apply(jsonValue) put back into resolvers, it fails. The comment in assertProviders now points to this test instead of claiming it.

Locally: openidm-authnfilter 47/47 (AuthenticationServiceTest 16/16). Each mutant named above fails the test written for it.

[maximthomas]

praise: Round 5 closes the className-only half of the activation regression, and the new tests are real pins.

• oidcAndOauth2Modules reads the name once and compares constant-first (AuthenticationService.java:315-318). With the old asString().equals(...) restored, activationShouldTolerateAModuleConfiguredByClassNameOnly fails with the NPE (16 run, 1 failed).
• readInstanceShouldNotWriteTheResolverTypeIntoThePublishedConfig (AuthenticationServiceTest.java:472) pins the read side of the setType move. Putting setType.apply(jsonValue) back into resolvers turns it red.
• The earlier pins hold under mutation as well. Reverting the setType guard fails activationShouldTolerateMalformedResolversOfDisabledModules. Dropping config.copy() fails identityProviderConfigChangedShouldPublishOnlyAfterTheFilterIsSet. Removing RuntimeException from activate's catch fails failedActivationOnARuntimeExceptionShouldLeaveNothingForUnbindToRebuild.