Address Review Comments

- update config scan period for local-config.json - remove unnecessary config values - make ConfigRetrieverFactory create method static - extract runtime config creation to helper method in Main - Added token config validation to UIDOperatorService
IABTechLab · Jan 31, 2025 · d153181 · d153181
1 parent 12fd4b3
commit d153181
Show file tree

Hide file tree

Showing 10 changed files with 73 additions and 39 deletions.
diff --git a/conf/default-config.json b/conf/default-config.json
@@ -34,6 +34,5 @@
   "optout_inmem_cache": false,
   "enclave_platform": null,
   "failure_shutdown_wait_hours": 120,
-  "sharing_token_expiry_seconds": 2592000,
   "operator_type": "public"
 }
diff --git a/conf/local-config.json b/conf/local-config.json
@@ -9,14 +9,10 @@
   "salts_metadata_path": "/com.uid2.core/test/salts/metadata.json",
   "services_metadata_path": "/com.uid2.core/test/services/metadata.json",
   "service_links_metadata_path": "/com.uid2.core/test/service_links/metadata.json",
-  "identity_token_expires_after_seconds": 3600,
-  "refresh_token_expires_after_seconds": 86400,
-  "refresh_identity_token_after_seconds": 900,
   "refresh_token_v3": false,
   "identity_v3": false,
   "identity_scope": "uid2",
   "enable_v2_encryption": false,
-  "sharing_token_expiry_seconds": 2592000,
   "cloud_download_threads": 8,
   "cloud_upload_threads": 2,
   "cloud_refresh_interval": 60,
@@ -43,6 +39,6 @@
       "path": "conf/runtime-config-defaults.json",
       "format": "json"
     },
-    "config_scan_period_ms": -1
+    "config_scan_period_ms": 5000
   }
 }
diff --git a/conf/local-e2e-docker-public-config.json b/conf/local-e2e-docker-public-config.json
@@ -13,9 +13,6 @@
   "salts_metadata_path": "http://core:8088/salt/refresh",
   "services_metadata_path": "http://core:8088/services/refresh",
   "service_links_metadata_path": "http://core:8088/service_links/refresh",
-  "identity_token_expires_after_seconds": 3600,
-  "refresh_token_expires_after_seconds": 86400,
-  "refresh_identity_token_after_seconds": 900,
   "refresh_token_v3": true,
   "identity_v3": false,
   "identity_scope": "uid2",

diff --git a/src/main/java/com/uid2/operator/Main.java b/src/main/java/com/uid2/operator/Main.java
@@ -267,43 +267,73 @@ private ICloudStorage wrapCloudStorageForOptOut(ICloudStorage cloudStorage) {
         }
     }
 
-    private void run() throws Exception {
-        this.createVertxInstancesMetric();
-        this.createVertxEventLoopsMetric();
+    private Future<IConfigService> initialiseConfigService() throws Exception {
+        Promise<IConfigService> promise = Promise.promise();
 
-        ConfigRetrieverFactory configRetrieverFactory = new ConfigRetrieverFactory();
-        ConfigRetriever dynamicConfigRetriever = configRetrieverFactory.create(vertx, config.getJsonObject("runtime_config_store"), this.createOperatorKeyRetriever().retrieve());
+        ConfigRetriever dynamicConfigRetriever = ConfigRetrieverFactory.create(
+                vertx,
+                config.getJsonObject("runtime_config_store"),
+                this.createOperatorKeyRetriever().retrieve()
+        );
         Future<ConfigService> dynamicConfigFuture = ConfigService.create(dynamicConfigRetriever);
 
-        ConfigRetriever featureFlagConfigRetriever = configRetrieverFactory.create(
+        ConfigRetriever featureFlagConfigRetriever = ConfigRetrieverFactory.create(
                 vertx,
                 new JsonObject()
                         .put("type", "file")
                         .put("config", new JsonObject()
                                 .put("path", "conf/feat-flag/feat-flag.json")
                                 .put("format", "json"))
-                        .put(ConfigScanPeriodMsProp, 60000),
+                        .put(ConfigScanPeriodMsProp, 10000),
                 ""
         );
 
-        featureFlagConfigRetriever.getConfig().compose(featureFlagConfig -> {
+        featureFlagConfigRetriever.getConfig()
+                .compose(featureFlagConfig -> {
+                    if (featureFlagConfig == null) {
+                        return Future.failedFuture(new RuntimeException("Feature flag config is null"));
+                    }
+
                     JsonObject remoteConfigJson = featureFlagConfig.getJsonObject("remote_config");
                     JsonObject featureFlagBootstrapConfig = remoteConfigJson.getJsonObject("runtime_config_store");
-                    ConfigRetriever staticConfigRetriever = configRetrieverFactory.create(vertx, featureFlagBootstrapConfig, "");
-                    return Future.all(dynamicConfigFuture, ConfigService.create(staticConfigRetriever));
-                })
-                .compose(configServiceManagerCompositeFuture -> {
-                    ConfigService dynamicConfigService = configServiceManagerCompositeFuture.resultAt(0);
-                    ConfigService staticConfigService = configServiceManagerCompositeFuture.resultAt(1);
 
-                    boolean featureFlag = featureFlagConfigRetriever.getCachedConfig().getJsonObject("remote_config").getBoolean(Const.Config.RemoteConfigFeatureFlagProp, false);
+                    ConfigRetriever staticConfigRetriever = ConfigRetrieverFactory.create(vertx, featureFlagBootstrapConfig, "");
+                    Future<ConfigService> staticConfigFuture = ConfigService.create(staticConfigRetriever);
 
-                    ConfigServiceManager configServiceManager = new ConfigServiceManager(
-                            vertx, dynamicConfigService, staticConfigService, featureFlag);
+                    return Future.all(dynamicConfigFuture, staticConfigFuture);
+                })
+                .onComplete(ar -> {
+                    if (ar.succeeded()) {
+                        CompositeFuture configServiceManagerCompositeFuture = ar.result();
+                        IConfigService dynamicConfigService = configServiceManagerCompositeFuture.resultAt(0);
+                        IConfigService staticConfigService = configServiceManagerCompositeFuture.resultAt(1);
+
+                        boolean remoteConfigFeatureFlag = featureFlagConfigRetriever.getCachedConfig()
+                                .getJsonObject("remote_config")
+                                .getBoolean(Const.Config.RemoteConfigFeatureFlagProp, false);
+
+                        ConfigServiceManager configServiceManager = new ConfigServiceManager(
+                                vertx, dynamicConfigService, staticConfigService, remoteConfigFeatureFlag);
+
+                        setupFeatureFlagListener(configServiceManager, featureFlagConfigRetriever);
+
+                        IConfigService configService = configServiceManager.getDelegatingConfigService();
+                        promise.complete(configService);
+                    } else {
+                        LOGGER.error("Failed to initialise ConfigService: ", ar.cause());
+                        promise.fail(ar.cause());
+                    }
+                });
+        return promise.future();
+    }
+
+    private void run() throws Exception {
+        this.createVertxInstancesMetric();
+        this.createVertxEventLoopsMetric();
 
-                    setupFeatureFlagListener(configServiceManager, featureFlagConfigRetriever);
+        this.initialiseConfigService()
+                .compose(configService -> {
 
-                    IConfigService configService = configServiceManager.getDelegatingConfigService();
                     Supplier<Verticle> operatorVerticleSupplier = () -> {
                         UIDOperatorVerticle verticle = new UIDOperatorVerticle(configService, config, this.clientSideTokenGenerate, siteProvider, clientKeyProvider, clientSideKeypairProvider, getKeyManager(), saltProvider, optOutStore, Clock.systemUTC(), _statsCollectorQueue, new SecureLinkValidatorService(this.serviceLinkProvider, this.serviceProvider), this.shutdownHandler::handleSaltRetrievalResponse);
                         return verticle;

diff --git a/src/main/java/com/uid2/operator/service/ConfigRetrieverFactory.java b/src/main/java/com/uid2/operator/service/ConfigRetrieverFactory.java
@@ -9,7 +9,7 @@
 import static com.uid2.operator.Const.Config.ConfigScanPeriodMsProp;
 
 public class ConfigRetrieverFactory {
-    public ConfigRetriever create(Vertx vertx, JsonObject bootstrapConfig, String operatorKey) {
+    public static ConfigRetriever create(Vertx vertx, JsonObject bootstrapConfig, String operatorKey) {
         String type = bootstrapConfig.getString("type");
         JsonObject storeConfig = bootstrapConfig.getJsonObject("config");
         if (type.equals("http")) {

diff --git a/src/main/java/com/uid2/operator/service/ConfigService.java b/src/main/java/com/uid2/operator/service/ConfigService.java
@@ -26,6 +26,7 @@ public static Future<ConfigService> create(ConfigRetriever configRetriever) {
 
         ConfigService instance = new ConfigService(configRetriever);
 
+        // Prevent dependent classes from attempting to access configuration before it has been retrieved.
         configRetriever.getConfig(ar -> {
             if (ar.succeeded()) {
                 logger.info("Successfully loaded config");

diff --git a/src/main/java/com/uid2/operator/service/DelegatingConfigService.java b/src/main/java/com/uid2/operator/service/DelegatingConfigService.java
@@ -4,7 +4,7 @@
 
 import java.util.concurrent.atomic.AtomicReference;
 
-public class DelegatingConfigService implements IConfigService{
+public class DelegatingConfigService implements IConfigService {
     private final AtomicReference<IConfigService> activeConfigService;
 
     public DelegatingConfigService(IConfigService initialConfigService) {

diff --git a/src/main/java/com/uid2/operator/service/UIDOperatorService.java b/src/main/java/com/uid2/operator/service/UIDOperatorService.java
@@ -49,7 +49,7 @@ public class UIDOperatorService implements IUIDOperatorService {
     private final Handler<Boolean> saltRetrievalResponseHandler;
 
     public UIDOperatorService(IOptOutStore optOutStore, ISaltProvider saltProvider, ITokenEncoder encoder, Clock clock,
-                              IdentityScope identityScope, Handler<Boolean> saltRetrievalResponseHandler, Boolean identityV3Enabled) {
+                              IdentityScope identityScope, Handler<Boolean> saltRetrievalResponseHandler, boolean identityV3Enabled) {
         this.saltProvider = saltProvider;
         this.encoder = encoder;
         this.optOutStore = optOutStore;
@@ -76,8 +76,21 @@ public UIDOperatorService(IOptOutStore optOutStore, ISaltProvider saltProvider,
         this.rawUidV3Enabled = identityV3Enabled;
     }
 
+    private void validateTokenDurations(Duration refreshIdentityAfter, Duration refreshExpiresAfter, Duration identityExpiresAfter) {
+        if (identityExpiresAfter.compareTo(refreshExpiresAfter) > 0) {
+            throw new IllegalStateException(REFRESH_TOKEN_EXPIRES_AFTER_SECONDS + " must be >= " + IDENTITY_TOKEN_EXPIRES_AFTER_SECONDS);
+        }
+        if (refreshIdentityAfter.compareTo(identityExpiresAfter) > 0) {
+            throw new IllegalStateException(IDENTITY_TOKEN_EXPIRES_AFTER_SECONDS + " must be >= " + REFRESH_IDENTITY_TOKEN_AFTER_SECONDS);
+        }
+        if (refreshIdentityAfter.compareTo(refreshExpiresAfter) > 0) {
+            throw new IllegalStateException(REFRESH_TOKEN_EXPIRES_AFTER_SECONDS + " must be >= " + REFRESH_IDENTITY_TOKEN_AFTER_SECONDS);
+        }
+    }
+
     @Override
     public IdentityTokens generateIdentity(IdentityRequest request, Duration refreshIdentityAfter, Duration refreshExpiresAfter, Duration identityExpiresAfter) {
+        this.validateTokenDurations(refreshIdentityAfter, refreshExpiresAfter, identityExpiresAfter);
         final Instant now = EncodingUtils.NowUTCMillis(this.clock);
         final byte[] firstLevelHash = getFirstLevelHash(request.userIdentity.id, now);
         final UserIdentity firstLevelHashIdentity = new UserIdentity(
@@ -93,6 +106,7 @@ public IdentityTokens generateIdentity(IdentityRequest request, Duration refresh
 
     @Override
     public RefreshResponse refreshIdentity(RefreshToken token, Duration refreshIdentityAfter, Duration refreshExpiresAfter, Duration identityExpiresAfter) {
+        this.validateTokenDurations(refreshIdentityAfter, refreshExpiresAfter, identityExpiresAfter);
         // should not be possible as different scopes should be using different keys, but just in case
         if (token.userIdentity.identityScope != this.identityScope) {
             return RefreshResponse.Invalid;

diff --git a/src/main/java/com/uid2/operator/vertx/UIDOperatorVerticle.java b/src/main/java/com/uid2/operator/vertx/UIDOperatorVerticle.java
@@ -96,8 +96,8 @@ public class UIDOperatorVerticle extends AbstractVerticle {
     private final IOptOutStore optOutStore;
     private final IClientKeyProvider clientKeyProvider;
     private final Clock clock;
-    private final Boolean allowLegacyAPI;
-    private final Boolean identityV3Enabled;
+    private final boolean allowLegacyAPI;
+    private final boolean identityV3Enabled;
     protected IUIDOperatorService idService;
     private final Map<String, DistributionSummary> _identityMapMetricSummaries = new HashMap<>();
     private final Map<Tuple.Tuple2<String, Boolean>, DistributionSummary> _refreshDurationMetricSummaries = new HashMap<>();

diff --git a/src/test/java/com/uid2/operator/ConfigServiceTest.java b/src/test/java/com/uid2/operator/ConfigServiceTest.java
@@ -27,7 +27,6 @@ class ConfigServiceTest {
     private JsonObject bootstrapConfig;
     private JsonObject runtimeConfig;
     private HttpServer server;
-    private ConfigRetrieverFactory configRetrieverFactory;
 
     @BeforeEach
     void setUp() {
@@ -47,8 +46,6 @@ void setUp() {
                 .put(MaxBidstreamLifetimeSecondsProp, 7200)
                 .put(SharingTokenExpiryProp, 3600);
 
-        configRetrieverFactory = new ConfigRetrieverFactory();
-
     }
 
     @AfterEach
@@ -87,7 +84,7 @@ private Future<Void> startMockServer(JsonObject config) {
 
     @Test
     void testGetConfig(VertxTestContext testContext) {
-        ConfigRetriever configRetriever = configRetrieverFactory.create(vertx, bootstrapConfig, "");
+        ConfigRetriever configRetriever = ConfigRetrieverFactory.create(vertx, bootstrapConfig, "");
         JsonObject httpStoreConfig = runtimeConfig;
         startMockServer(httpStoreConfig)
                 .compose(v -> ConfigService.create(configRetriever))
@@ -110,7 +107,7 @@ void testInvalidConfigRevertsToPrevious(VertxTestContext testContext) {
                 .put("type", "json")
                 .put("config", invalidConfig)
                 .put(ConfigScanPeriodMsProp, -1);
-        ConfigRetriever spyRetriever = spy(configRetrieverFactory.create(vertx, jsonBootstrapConfig, ""));
+        ConfigRetriever spyRetriever = spy(ConfigRetrieverFactory.create(vertx, jsonBootstrapConfig, ""));
         when(spyRetriever.getCachedConfig()).thenReturn(lastConfig);
         ConfigService.create(spyRetriever)
                 .compose(configService -> {
@@ -130,7 +127,7 @@ void testFirstInvalidConfigThrowsRuntimeException(VertxTestContext testContext)
                 .put("type", "json")
                 .put("config", invalidConfig)
                 .put(ConfigScanPeriodMsProp, -1);
-        ConfigRetriever configRetriever = configRetrieverFactory.create(vertx, jsonBootstrapConfig, "");
+        ConfigRetriever configRetriever = ConfigRetrieverFactory.create(vertx, jsonBootstrapConfig, "");
         ConfigService.create(configRetriever)
                 .onComplete(testContext.failing(throwable -> {
                    assertThrows(RuntimeException.class, () -> {