Switched to using a SecretVersion to represent a particular version o…

…f a secret

api/src/main/java/keywhiz/api/automation/v2/SecretDetailResponseV2.java

@@ -4,19 +4,16 @@
 import com.fasterxml.jackson.annotation.JsonProperty;
 import com.google.auto.value.AutoValue;
 import com.google.common.base.MoreObjects;
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.primitives.UnsignedLong;
 import java.util.Base64;
-import java.util.List;
 import java.util.Map;
 import javax.annotation.Nullable;
 import keywhiz.api.model.Secret;
-import keywhiz.api.model.SecretContent;
 import keywhiz.api.model.SecretSeries;
+import keywhiz.api.model.SecretVersion;
 
 import static com.google.common.base.Strings.nullToEmpty;
-import static java.nio.charset.StandardCharsets.UTF_8;
 import static keywhiz.api.model.Secret.decodedLength;
 
 @AutoValue public abstract class SecretDetailResponseV2 {
@@ -72,13 +69,16 @@ public Builder secret(Secret secret) {
           .metadata(secret.getMetadata());
     }
 
-    public Builder secretContent(SecretContent secretContent) {
+    public Builder secretVersion(SecretVersion secretVersion) {
       return this
-          .version(secretContent.id())
-          .createdAtSeconds(secretContent.createdAt().toEpochSecond())
-          .createdBy(secretContent.createdBy())
-          .metadata(secretContent.metadata())
-          .expiry(secretContent.expiry());
+          .name(secretVersion.name())
+          .version(secretVersion.versionId())
+          .description(secretVersion.description())
+          .createdAtSeconds(secretVersion.createdAt().toEpochSecond())
+          .createdBy(secretVersion.createdBy())
+          .type(secretVersion.type())
+          .expiry(secretVersion.expiry())
+          .metadata(secretVersion.metadata());
 
     }
 
@@ -147,3 +147,4 @@ public SecretDetailResponseV2 build() {
         .toString();
   }
 }
+

api/src/main/java/keywhiz/api/model/SecretVersion.java

@@ -0,0 +1,52 @@
+package keywhiz.api.model;
+
+import com.fasterxml.jackson.annotation.JsonAnyGetter;
+import com.google.auto.value.AutoValue;
+import com.google.common.base.MoreObjects;
+import com.google.common.collect.ImmutableMap;
+import javax.annotation.Nullable;
+import keywhiz.api.ApiDate;
+
+import static com.google.common.base.Strings.nullToEmpty;
+
+/**
+ * Contains all relevant information on a version of a secret (note that this does
+ * not contain the contents of a secret).
+ */
+@AutoValue
+public abstract class SecretVersion {
+  public static SecretVersion of(long secretId, long versionId, String name, @Nullable String description, ApiDate createdAt,
+      @Nullable String createdBy, ApiDate updatedAt, @Nullable String updatedBy,
+      ImmutableMap<String, String> metadata, @Nullable String type, long expiry) {
+    return new AutoValue_SecretVersion(secretId, versionId, name, nullToEmpty(description),
+        createdAt, nullToEmpty(createdBy), updatedAt,
+        nullToEmpty(updatedBy), metadata, type, expiry);
+  }
+
+  public abstract long secretId();
+  public abstract long versionId();
+  public abstract String name();
+  public abstract String description();
+  public abstract ApiDate createdAt();
+  public abstract String createdBy();
+  public abstract ApiDate updatedAt();
+  public abstract String updatedBy();
+  @JsonAnyGetter public abstract  ImmutableMap<String, String> metadata();
+  public abstract String type();
+  public abstract long expiry();
+
+  @Override public String toString() {
+    return MoreObjects.toStringHelper(this)
+        .add("secretId", secretId())
+        .add("name", name())
+        .add("description", description())
+        .add("createdAt", createdAt())
+        .add("createdBy", createdBy())
+        .add("updatedAt", updatedAt())
+        .add("updatedBy", updatedBy())
+        .add("metadata", metadata())
+        .add("type", type())
+        .add("expiry", expiry())
+        .omitNullValues().toString();
+  }
+}

api/src/test/java/keywhiz/api/automation/v2/SecretDetailResponseV2Test.java

@@ -23,6 +23,7 @@
 import keywhiz.api.model.Secret;
 import keywhiz.api.model.SecretContent;
 import keywhiz.api.model.SecretSeries;
+import keywhiz.api.model.SecretVersion;
 import org.junit.Test;
 
 import static keywhiz.testing.JsonHelpers.asJson;
@@ -79,17 +80,14 @@ public class SecretDetailResponseV2Test {
         .isEqualTo(jsonFixture("fixtures/v2/secretDetailResponse.json"));
   }
 
-  @Test public void formsCorrectlyFromSecretContents() throws Exception {
-    SecretContent secretContent = SecretContent.of(1, 1, "",
+  @Test public void formsCorrectlyFromSecretVersion() throws Exception {
+    SecretVersion version = SecretVersion.of(10, 1, "secret-name", "secret-description",
         ApiDate.parse("2013-03-28T21:23:04.159Z"), "creator-user",
         ApiDate.parse("2013-03-28T21:23:04.159Z"), "creator-user",
-        ImmutableMap.of("owner", "root"), 1136214245);
+        ImmutableMap.of("owner", "root"), "text/plain", 1136214245);
     SecretDetailResponseV2 secretDetailResponse = SecretDetailResponseV2.builder()
-        .secretContent(secretContent)
-        .name("secret-name")
-        .description("secret-description")
+        .secretVersion(version)
         .content("YXNkZGFz")
-        .type("text/plain")
         .build();
 
     assertThat(asJson(secretDetailResponse))

server/src/main/java/keywhiz/service/daos/SecretDAO.java

@@ -22,8 +22,10 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
+import java.util.stream.Collectors;
 import javax.annotation.Nullable;
 import javax.inject.Inject;
+import javax.ws.rs.NotFoundException;
 import keywhiz.api.model.*;
 import keywhiz.jooq.tables.Secrets;
 import keywhiz.service.config.Readonly;
@@ -37,6 +39,7 @@
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
 import static java.lang.String.format;
+import static java.util.stream.Collectors.toList;
 import static keywhiz.jooq.tables.Secrets.SECRETS;
 
 /**
@@ -204,7 +207,7 @@ public ImmutableList<SimpleEntry<Long, String>> getSecretsNameOnly() {
    * @param numVersions the number of versions after versionIdx to select in the list of versions
    * @return Versions of a secret matching input parameters or Optional.absent().
    */
-  public Optional<SecretSeriesAndVersions> getSecretVersionsByName(String name,
+  public Optional<ImmutableList<SecretVersion>> getSecretVersionsByName(String name,
       int versionIdx,
       int numVersions) {
     checkArgument(!name.isEmpty());
@@ -216,32 +219,39 @@ public Optional<SecretSeriesAndVersions> getSecretVersionsByName(String name,
 
     Optional<SecretSeries> series = secretSeriesDAO.getSecretSeriesByName(name);
     if (series.isPresent()) {
-      long secretId = series.get().id();
+      SecretSeries s = series.get();
+      long secretId = s.id();
       Optional<ImmutableList<SecretContent>> contents =
           secretContentDAO.getSecretVersionsBySecretId(secretId, versionIdx, numVersions);
       if (contents.isPresent()) {
-        return Optional.of(SecretSeriesAndVersions.of(series.get(), contents.get()));
+        ImmutableList.Builder<SecretVersion> b = new ImmutableList.Builder<>();
+        b.addAll(contents.get()
+            .stream()
+            .map(c -> SecretVersion.of(s.id(), c.id(), s.name(), s.description(), c.createdAt(),
+                c.createdBy(), c.updatedAt(), c.updatedBy(), c.metadata(), s.type().orElse(""),
+                c.expiry()))
+            .collect(toList()));
+
+        return Optional.of(b.build());
       }
     }
+
     return Optional.empty();
   }
 
   /**
    * @param name of secret series for which to reset secret version
    * @param versionId The identifier for the desired current version
-   * @return 0 for success, non-zero for failure (secret series not found)
+   * @throws NotFoundException if secret not found
    */
-  public int setCurrentSecretVersionByName(String name, long versionId) {
+  public void setCurrentSecretVersionByName(String name, long versionId) {
     checkArgument(!name.isEmpty());
     checkArgument(versionId >= 0);
 
     SecretSeriesDAO secretSeriesDAO = secretSeriesDAOFactory.using(dslContext.configuration());
-    Optional<SecretSeries> series = secretSeriesDAO.getSecretSeriesByName(name);
-    if (series.isPresent()) {
-      secretSeriesDAO.setCurrentVersion(series.get().id(), versionId);
-      return 0;
-    }
-    return 1; // Secret series not found
+    SecretSeries series = secretSeriesDAO.getSecretSeriesByName(name).orElseThrow(
+        NotFoundException::new);
+    secretSeriesDAO.setCurrentVersion(series.id(), versionId);
   }
 
 

server/src/main/java/keywhiz/service/resources/automation/v2/SecretResource.java

@@ -2,6 +2,7 @@
 
 import com.codahale.metrics.annotation.Timed;
 import com.codahale.metrics.annotation.ExceptionMetered;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Sets;
 import io.dropwizard.auth.Auth;
 import java.util.List;
@@ -237,16 +238,13 @@ public SecretDetailResponseV2 secretInfo(@Auth AutomationClient automationClient
   public Iterable<SecretDetailResponseV2> secretVersions(@Auth AutomationClient automationClient,
       @PathParam("name") String name, @PathParam("versionIdx") int versionIdx,
       @PathParam("numVersions") int numVersions) {
-    SecretSeriesAndVersions secret =
+    ImmutableList<SecretVersion> versions =
         secretDAO.getSecretVersionsByName(name, versionIdx, numVersions)
             .orElseThrow(NotFoundException::new);
 
-    // Note it's necessary to call "series" before "secretContent", as "secretContent"
-    // resets some fields with version-specific information
-    return secret.content().stream()
-        .map(c -> SecretDetailResponseV2.builder()
-            .series(secret.series())
-            .secretContent(c)
+    return versions.stream()
+        .map(v -> SecretDetailResponseV2.builder()
+            .secretVersion(v)
             .build())
         .collect(toList());
   }
@@ -266,10 +264,9 @@ public Iterable<SecretDetailResponseV2> secretVersions(@Auth AutomationClient au
   @POST
   public Response resetSecretVersion(@Auth AutomationClient automationClient,
       @PathParam("name") String name, @PathParam("versionId") long versionId) {
-    int found = secretDAO.setCurrentSecretVersionByName(name, versionId);
-    if (found != 0) {
-      throw new NotFoundException();
-    }
+    secretDAO.setCurrentSecretVersionByName(name, versionId);
+
+    // If the secret wasn't found, setCurrentSecretVersionByName already threw a NotFoundException
     return Response.status(Response.Status.OK).build();
   }
 

server/src/test/java/keywhiz/service/resources/automation/v2/SecretResourceTest.java

@@ -294,7 +294,8 @@ public class SecretResourceTest {
     // get current time to calculate timestamps off for expiry
     long now = System.currentTimeMillis() / 1000L;
 
-    // Create secrets 1 second apart, so their version order is deterministic
+    // Create secrets 1 second apart, so that the order of the versions, which
+    // will be listed by creation time, is fixed
     for (int i = 0; i < totalVersions; i++) {
       createOrUpdate(CreateOrUpdateSecretRequestV2.builder()
           .content(encoder.encodeToString(format("supa secret20_v%d", i).getBytes(UTF_8)))