fix: end DS activation race that broke system-realm auth at startup (#1882)

pnnl.goss.core/build.gradle

@@ -20,7 +20,15 @@ dependencies {
 
 test {
     useJUnitPlatform()
-
+
+    // The activation-order test (GADP-012) asserts the generated Declarative
+    // Services descriptors, which are the real runtime contract. DS annotations
+    // are CLASS-retention and invisible to runtime reflection, so the bnd-emitted
+    // OSGI-INF XML is the authoritative artifact to assert against. Build the
+    // bundles first and hand the test their output directory.
+    dependsOn 'jar'
+    systemProperty 'goss.generated.dir', project.file('generated').absolutePath
+
     testLogging {
         events "passed", "skipped", "failed"
         exceptionFormat "full"

pnnl.goss.core/src/pnnl/goss/core/security/GossRealm.java

@@ -12,6 +12,31 @@
  */
 public interface GossRealm extends Realm, PermissionAdapter {
 
+    /**
+     * Service property name used to mark the system-authenticating realm
+     * (SystemBasedRealm) so ordering-sensitive consumers can select it with a
+     * target filter rather than accepting any bound GossRealm. See
+     * {@link #SYSTEM_REALM_TYPE} and GADP-012 / issue #1882.
+     */
+    String REALM_TYPE_PROPERTY = "realm.type";
+
+    /**
+     * Service property value published by the system-authenticating realm
+     * (SystemBasedRealm). Consumers gate activation on this realm with
+     * {@code @Reference(target = SYSTEM_REALM_TARGET_FILTER)}.
+     */
+    String SYSTEM_REALM_TYPE = "system";
+
+    /**
+     * The literal target filter string consumers pass to
+     * {@code @Reference(target = ...)} to select the system realm. Kept as its own
+     * constant (rather than assembled from the two constants above at runtime)
+     * because {@code @Reference(target = ...)} requires a compile-time constant
+     * expression, and bnd is strict about resolving that expression to a literal
+     * when it emits the OSGI-INF descriptor.
+     */
+    String SYSTEM_REALM_TARGET_FILTER = "(" + REALM_TYPE_PROPERTY + "=" + SYSTEM_REALM_TYPE + ")";
+
     Set<String> getPermissions(String identifier);
 
     boolean hasIdentifier(String identifier);

pnnl.goss.core/src/pnnl/goss/core/security/impl/Activator.java

@@ -29,13 +29,17 @@
  * for authentication and authorization.
  *
  * This component collects all GossRealm services and registers them with the
- * SecurityManager before exposing the SecurityManager service. This ensures
- * that authentication can work immediately when the broker starts.
+ * SecurityManager before exposing the SecurityManager service. It is the SINGLE
+ * writer of the SecurityManager's realm set (SecurityManagerRealmHandler no
+ * longer writes it), so the realm set cannot be clobbered mid-startup.
  *
- * IMPORTANT: This component requires at least one GossRealm to be available
- * (cardinality = AT_LEAST_ONE) before the SecurityManager service is
- * registered. This prevents the race condition where GridOpticsServer tries to
- * authenticate before any realms are configured.
+ * IMPORTANT: The SecurityManager service is not published until BOTH the
+ * AT_LEAST_ONE realmAdded binder has a realm AND the mandatory, target-filtered
+ * systemRealm reference is bound. The AT_LEAST_ONE guard alone was insufficient
+ * (GADP-012 / issue #1882): it guaranteed some realm, not the
+ * system-authenticating one, so GridOpticsServer could connect as
+ * system/manager against a realm set that returned null for "system". The
+ * systemRealm reference closes that gap.
  */
 @Component(service = SecurityManager.class, immediate = true)
 public class Activator extends DefaultActiveMqSecurityManager {
@@ -44,6 +48,34 @@ public class Activator extends DefaultActiveMqSecurityManager {
 
     private final Map<ServiceReference<GossRealm>, GossRealm> realmMap = new ConcurrentHashMap<>();
 
+    /**
+     * Mandatory, target-filtered dependency on the system-authenticating realm
+     * (SystemBasedRealm, marked with the realm.type=system service property).
+     *
+     * This reference is the ordering guarantee for GADP-012 / issue #1882.
+     * Declarative Services will not invoke this component's @Activate, and thus
+     * will not publish the SecurityManager service, until the realm that
+     * authenticates the system/manager principal with the "*" permission is bound.
+     * Because GridOpticsServer holds a mandatory reference on the SecurityManager
+     * service, gating the service publication here transitively gates
+     * GridOpticsServer's broker-connect (createConnection("system", "manager")) on
+     * the system realm being present.
+     *
+     * Mandatory cardinality (the DS default 1..1) is what forces the ordering: DS
+     * holds activation until the reference is bound. Binding does not deadlock,
+     * because SystemBasedRealm depends only on SecurityConfig and
+     * GossPermissionResolver, neither of which depends back on the SecurityManager.
+     * The same realm instance is also collected by the dynamic realmAdded binder
+     * below, so it is included in the realm set.
+     *
+     * Never read directly in Java: the field's sole purpose is to give DS a
+     * mandatory reference to gate @Activate on, so it is unused from this class's
+     * own code and the compiler's unused-field warning is suppressed.
+     */
+    @Reference(target = GossRealm.SYSTEM_REALM_TARGET_FILTER)
+    @SuppressWarnings("unused")
+    private volatile GossRealm systemRealm;
+
     @Activate
     public void activate() {
         log.info("Activating SecurityManager service");

pnnl.goss.core/src/pnnl/goss/core/security/impl/SecurityManagerRealmHandler.java

@@ -10,9 +10,6 @@
 import org.osgi.service.component.annotations.Reference;
 import org.osgi.service.component.annotations.ReferenceCardinality;
 import org.osgi.service.component.annotations.ReferencePolicy;
-import org.apache.shiro.mgt.DefaultSecurityManager;
-import org.apache.shiro.mgt.SecurityManager;
-import org.apache.shiro.realm.Realm;
 import org.osgi.framework.ServiceReference;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -25,52 +22,36 @@ public class SecurityManagerRealmHandler implements PermissionAdapter {
 
     private static final Logger log = LoggerFactory.getLogger(SecurityManagerRealmHandler.class);
 
-    @Reference
-    private volatile SecurityManager securityManager;
     private final Map<ServiceReference<GossRealm>, GossRealm> realmMap = new ConcurrentHashMap<>();
 
     @Activate
     public void activate() {
-        log.info("SecurityManagerRealmHandler activated with {} pending realms", realmMap.size());
-        // Register any realms that were added before the SecurityManager was available
-        if (!realmMap.isEmpty()) {
-            registerAllRealms();
-        }
+        log.info("SecurityManagerRealmHandler activated with {} known realms", realmMap.size());
     }
 
+    /**
+     * Tracks GossRealm services so this component can answer getPermissions()
+     * across every realm (its PermissionAdapter role).
+     *
+     * This binder deliberately does NOT call setRealms() on the SecurityManager.
+     * The SecurityManager component (Activator) is the single writer of the
+     * SecurityManager's realm set; it collects the same GossRealm services and owns
+     * setRealms(). Having two independently-populated maps both write the shared
+     * SecurityManager was the double-writer race in GADP-012 / issue #1882: a
+     * partial write here could transiently drop the system realm from the set
+     * GridOpticsServer authenticates against. Realm wiring is now owned solely by
+     * the Activator; this component only reads realms for permissions.
+     */
     @Reference(cardinality = ReferenceCardinality.MULTIPLE, policy = ReferencePolicy.DYNAMIC, unbind = "realmRemoved")
     public void realmAdded(ServiceReference<GossRealm> ref, GossRealm handler) {
         realmMap.put(ref, handler);
-        log.debug("Realm added: {}", handler.getClass().getName());
-
-        // Only register if the SecurityManager is available
-        if (securityManager != null) {
-            registerAllRealms();
-        }
-    }
-
-    private synchronized void registerAllRealms() {
-        if (securityManager == null) {
-            log.warn("Cannot register realms - SecurityManager is null");
-            return;
-        }
-
-        DefaultSecurityManager defaultInstance = (DefaultSecurityManager) securityManager;
-        Set<Realm> realms = new HashSet<>();
-        for (GossRealm r : realmMap.values()) {
-            realms.add((Realm) r);
-        }
-        defaultInstance.setRealms(realms);
-        log.info("Registered {} realms with SecurityManager", realms.size());
+        log.debug("Realm tracked for permissions: {}", handler.getClass().getName());
     }
 
     public void realmRemoved(ServiceReference<GossRealm> ref) {
         GossRealm removed = realmMap.remove(ref);
-        if (removed != null && securityManager != null) {
-            DefaultSecurityManager defaultInstance = (DefaultSecurityManager) securityManager;
-            if (defaultInstance.getRealms() != null) {
-                defaultInstance.getRealms().remove(removed);
-            }
+        if (removed != null) {
+            log.debug("Realm untracked for permissions: {}", removed.getClass().getName());
         }
     }
 

pnnl.goss.core/src/pnnl/goss/core/security/jwt/UnauthTokenBasedRealm.java

@@ -113,7 +113,7 @@ protected AuthenticationInfo doGetAuthenticationInfo(
                     // look up permissions based on roles and add them
                     Set<String> permissions = new HashSet<String>();
                     JWTAuthenticationToken tokenObj = securityConfig.parseToken(username);
-                    log.info("Has token roles count: {}", 
+                    log.info("Has token roles count: {}",
                             tokenObj.getRoles() != null ? tokenObj.getRoles().size() : 0);
 
                     if (roleManager != null) {

pnnl.goss.core/src/pnnl/goss/core/security/system/SystemBasedRealm.java

@@ -39,10 +39,19 @@
  *
  * NOTE: This class assumes uniqueness of username in the properties file.
  *
+ * The realm.type=system service property is the distinguishing marker that lets
+ * ordering-sensitive consumers (the SecurityManager Activator and
+ * GridOpticsServer) select this realm with a target filter, so Declarative
+ * Services can gate their activation on the system-authenticating realm rather
+ * than on "some realm". See GADP-012 / issue #1882: without this marker the
+ * AT_LEAST_ONE realm guard let GridOpticsServer connect as system/manager
+ * before this realm was wired.
+ *
  * @author Craig Allwardt
  *
  */
-@Component(service = GossRealm.class, configurationPid = "pnnl.goss.core.security.systemrealm", configurationPolicy = ConfigurationPolicy.REQUIRE)
+@Component(service = GossRealm.class, configurationPid = "pnnl.goss.core.security.systemrealm", configurationPolicy = ConfigurationPolicy.REQUIRE, property = {
+        GossRealm.REALM_TYPE_PROPERTY + "=" + GossRealm.SYSTEM_REALM_TYPE})
 public class SystemBasedRealm extends AuthorizingRealm implements GossRealm {
 
     private static final Logger log = LoggerFactory.getLogger(SystemBasedRealm.class);

pnnl.goss.core/src/pnnl/goss/core/server/impl/GridOpticsServer.java

@@ -170,8 +170,27 @@ public class GridOpticsServer implements ServerControl {
     @Reference
     private volatile RequestHandlerRegistry handlerRegistry;
 
-    @Reference
-    private volatile GossRealm permissionAdapter;
+    /**
+     * Mandatory, target-filtered dependency on the system-authenticating realm
+     * (SystemBasedRealm, marked with the realm.type=system service property).
+     *
+     * This is the explicit activation-order gate for GADP-012 / issue #1882.
+     * Declarative Services will not invoke this component's @Activate (start(Map),
+     * which reaches createConnection("system", "manager") below) until the realm
+     * that authenticates the system principal with the "*" permission is bound. The
+     * SecurityManager reference above provides the same ordering transitively (its
+     * own component gates publication on this realm); holding the reference here as
+     * well makes the contract local, explicit, and independent of the
+     * SecurityManager component's internals. The field was previously declared as
+     * an unused permissionAdapter reference; it is retargeted, not added.
+     *
+     * Never read directly in Java: the field's sole purpose is to give DS a
+     * mandatory reference to gate @Activate on, so it is unused from this class's
+     * own code and the compiler's unused-field warning is suppressed.
+     */
+    @Reference(target = GossRealm.SYSTEM_REALM_TARGET_FILTER)
+    @SuppressWarnings("unused")
+    private volatile GossRealm systemRealm;
 
     private final ScheduledExecutorService scheduler = Executors.newScheduledThreadPool(1);