Address feedback

Author: aulme

src/main/java/com/uid2/admin/Main.java

@@ -76,7 +76,6 @@ public class Main {
 
     private final Vertx vertx;
     private final JsonObject config;
-
     public Main(Vertx vertx, JsonObject config) {
         this.vertx = vertx;
         this.config = config;

src/main/java/com/uid2/admin/cloudEncryption/CloudKeyRotationStrategy.java

@@ -48,8 +48,9 @@ private Stream<CloudEncryptionKey> desiredKeysForSite(
                 KeyIdGenerator idGenerator,
                 Set<CloudEncryptionKey> existingKeys
         ) {
-                var withNewKey = Streams.concat(existingKeys.stream(), Stream.of(makeNewKey(siteId, idGenerator)));
-                return keyRetentionStrategy.selectKeysToRetain(withNewKey.collect(Collectors.toSet())).stream();
+                var existingKeysToRetain = keyRetentionStrategy.selectKeysToRetain(existingKeys);
+                var newKey = makeNewKey(siteId, idGenerator);
+                return Streams.concat(existingKeysToRetain.stream(), Stream.of(newKey));
         }
 
         private CloudEncryptionKey makeNewKey(Integer siteId, KeyIdGenerator idGenerator) {

src/main/java/com/uid2/admin/cloudEncryption/ExpiredKeyCountRetentionStrategy.java

@@ -3,13 +3,12 @@
 import com.uid2.admin.store.Clock;
 import com.uid2.shared.model.CloudEncryptionKey;
 
-import java.util.ArrayList;
 import java.util.Comparator;
 import java.util.Set;
 import java.util.stream.Collectors;
-import java.util.stream.Stream;
 
-// Keep up to `expiredKeysToRetain` most recent expired keys
+// Only keep keys past activation time - no future keys allowed
+// Keep up to `expiredKeysToRetain` most recent keys
 // Considers all keys passed, doesn't do grouping by site (handled upstream)
 public class ExpiredKeyCountRetentionStrategy implements CloudKeyRetentionStrategy {
     private final Clock clock;
@@ -22,37 +21,10 @@ public ExpiredKeyCountRetentionStrategy(Clock clock, int expiredKeysToRetain) {
 
     @Override
     public Set<CloudEncryptionKey> selectKeysToRetain(Set<CloudEncryptionKey> keysForSite) {
-        var activeKey = findActiveKey(keysForSite);
-        if (activeKey == null) {
-            return keysForSite;
-        }
-
-        var expiredKeys = new ArrayList<CloudEncryptionKey>();
-        var nonExpiredKeys = new ArrayList<CloudEncryptionKey>();
-        for (var key : keysForSite) {
-            if (key.getActivates() < activeKey.getActivates()) {
-                expiredKeys.add(key);
-            } else {
-                nonExpiredKeys.add(key);
-            }
-        }
-
-        var retainedExpiredKeys = pickRetainedExpiredKeys(expiredKeys);
-        return Stream.concat(retainedExpiredKeys, nonExpiredKeys.stream()).collect(Collectors.toSet());
-
-    }
-
-    private Stream<CloudEncryptionKey> pickRetainedExpiredKeys(ArrayList<CloudEncryptionKey> expiredKeys) {
-        return expiredKeys
-                .stream()
+        return keysForSite.stream()
+                .filter(key -> key.getActivates() <= clock.getEpochSecond())
                 .sorted(Comparator.comparingLong(CloudEncryptionKey::getActivates).reversed())
-                .limit(expiredKeysToRetain);
-    }
-
-    private CloudEncryptionKey findActiveKey(Set<CloudEncryptionKey> keys) {
-        var keysPastActivation = keys.stream().filter(key -> key.getActivates() <= clock.getEpochSecond());
-        return keysPastActivation
-                .max(Comparator.comparingLong(CloudEncryptionKey::getActivates))
-                .orElse(null);
+                .limit(expiredKeysToRetain)
+                .collect(Collectors.toSet());
     }
 }

src/main/java/com/uid2/admin/vertx/service/CloudEncryptionKeyService.java

@@ -50,7 +50,7 @@ public void setupRoutes(Router router) {
         );
 
         router.post(Endpoints.CLOUD_ENCRYPTION_KEY_ROTATE.toString()).handler(
-                auth.handle(this::handleRotate, Role.MAINTAINER)
+                auth.handle(this::handleRotate, Role.MAINTAINER, Role.SECRET_ROTATION)
         );
     }
 

src/test/java/com/uid2/admin/cloudEncryption/ExpiredKeyCountRetentionStrategyTest.java

@@ -16,15 +16,13 @@
 class ExpiredKeyCountRetentionStrategyTest {
     private final long past1 = 100L;
     private final long past2 = 200L;
-    private final long past3 = 300L;
-    private final long now = 400L;
-    private final long future = 500L;
+    private final long now = 300L;
+    private final long future = 400L;
     private ExpiredKeyCountRetentionStrategy strategy;
-    private Clock clock;
 
     @BeforeEach
     void setUp() {
-        clock = mock(Clock.class);
+        Clock clock = mock(Clock.class);
         strategy = new ExpiredKeyCountRetentionStrategy(clock, 2);
         when(clock.getEpochSecond()).thenReturn(now);
     }
@@ -50,29 +48,27 @@ void selectKeysToRetain_withLessThanNKeys() {
     }
 
     @Test
-    void selectKeysToRetain_withMoreThanNNonExpiredKeys() {
+    void selectKeysToRetain_dropsFutureKeys() {
         var activeKey = new CloudEncryptionKey(1, 1, now, now, "secret 1");
         var futureKey1 = new CloudEncryptionKey(2, 1, future, now, "secret 2");
-        var futureKey2 = new CloudEncryptionKey(3, 1, future, now, "secret 3");
-        var futureKey3 = new CloudEncryptionKey(4, 1, future, now, "secret 4");
-        var expiredKey1 = new CloudEncryptionKey(5, 1, past1, now, "secret 5");
-        var originalKeys = Set.of(activeKey, futureKey1, futureKey2, futureKey3, expiredKey1);
+        var expiredKey1 = new CloudEncryptionKey(3, 1, past1, now, "secret 3");
+        var originalKeys = Set.of(activeKey, futureKey1, expiredKey1);
+
+        var expected = Set.of(activeKey, expiredKey1);
 
         var actual = strategy.selectKeysToRetain(originalKeys);
 
-        assertThat(actual).isEqualTo(originalKeys);
+        assertThat(actual).isEqualTo(expected);
     }
 
     @Test
-    void selectKeysToRetain_withMoreThanNExpiredKeys() {
+    void selectKeysToRetain_keepsNMostRecentNonFutureKeys() {
         var oldestExpiredKey = new CloudEncryptionKey(1, 1, past1, past1, "secret 1"); // Don't retain
         var expiredKey2 = new CloudEncryptionKey(2, 1, past2, past1, "secret 2");
-        var expiredKey3 = new CloudEncryptionKey(3, 1, past3, past1, "secret 3");
-        var activeKey = new CloudEncryptionKey(3, 1, now, past1, "secret 4");
-        var futureKey = new CloudEncryptionKey(3, 1, now, past1, "secret 5");
-        var originalKeys = Set.of(oldestExpiredKey, expiredKey2, expiredKey3, activeKey, futureKey);
+        var activeKey = new CloudEncryptionKey(3, 1, now, past1, "secret 3");
+        var originalKeys = Set.of(oldestExpiredKey, expiredKey2, activeKey);
 
-        var expected = Set.of(expiredKey2, expiredKey3, activeKey, futureKey);
+        var expected = Set.of(expiredKey2, activeKey);
 
         var actual = strategy.selectKeysToRetain(originalKeys);
 

src/test/java/com/uid2/admin/vertx/CloudEncryptionKeyServiceTest.java

@@ -111,6 +111,16 @@ public void testRotate_noAccess(Vertx vertx, VertxTestContext testContext) {
         });
     }
 
+    @Test
+    public void testRotate_canBeRotatedBySecretRotationJob(Vertx vertx, VertxTestContext testContext) {
+        fakeAuth(Role.SECRET_ROTATION);
+        post(vertx, testContext, Endpoints.CLOUD_ENCRYPTION_KEY_ROTATE, null, response -> {
+            assertEquals(200, response.statusCode());
+
+            testContext.completeNow();
+        });
+    }
+
     @Test
     public void testRotate_noSitesDoesNothing(Vertx vertx, VertxTestContext testContext) {
         fakeAuth(Role.MAINTAINER);