Rewrite ServerSecret caching and accessor logic

I'm working on some changes to XsrfTokenManager (b/35388772) and ServerSecret
was crufty enough that I ended up rewriting it.  Now it uses a LoadingCache
with a transaction instead of needlessly race-condition-y static init logic.

It also now supports retrieving its value as either a UUID (the old format
used by XsrfTokenManager) or a byte[].  The latter is more flexible and can
be directly used with HMAC which the new XsrfTokenManager format will employ.

And lastly, I added tests.  In addition, I tested this code on alpha and
verified appropriate operation (XSRF tokens still work from the console and
from regtool; if you remove ServerSecret from datastore and memcache, it
persists a new one).

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=148931620

java/google/registry/model/server/ServerSecret.java

@@ -17,14 +17,21 @@ package google.registry.model.server;
 import static google.registry.model.ofy.ObjectifyService.ofy;
 import static google.registry.model.ofy.Ofy.RECOMMENDED_MEMCACHE_EXPIRATION;
 
-import com.googlecode.objectify.VoidWork;
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.cache.CacheBuilder;
+import com.google.common.cache.CacheLoader;
+import com.google.common.cache.LoadingCache;
+import com.google.common.primitives.Longs;
+import com.googlecode.objectify.Work;
 import com.googlecode.objectify.annotation.Cache;
 import com.googlecode.objectify.annotation.Entity;
 import com.googlecode.objectify.annotation.Unindex;
 import google.registry.model.annotations.NotBackedUp;
 import google.registry.model.annotations.NotBackedUp.Reason;
 import google.registry.model.common.CrossTldSingleton;
+import java.nio.ByteBuffer;
 import java.util.UUID;
+import java.util.concurrent.ExecutionException;
 
 /** A secret number used for generating tokens (such as XSRF tokens). */
 @Entity
@@ -33,34 +40,76 @@ import java.util.UUID;
 @NotBackedUp(reason = Reason.AUTO_GENERATED)
 public class ServerSecret extends CrossTldSingleton {
 
-  private static ServerSecret secret;
+  /**
+   * Cache of the singleton ServerSecret instance that creates it if not present.
+   *
+   * <p>The key is meaningless since there is only one instance; this is essentially a memoizing
+   * Supplier that can be reset for testing purposes.
+   */
+  private static final LoadingCache<Class<ServerSecret>, ServerSecret> CACHE =
+      CacheBuilder.newBuilder().build(
+          new CacheLoader<Class<ServerSecret>, ServerSecret>() {
+            @Override
+            public ServerSecret load(Class<ServerSecret> unused) {
+              // Fast path - non-transactional load to hit memcache.
+              ServerSecret secret = ofy().load().entity(new ServerSecret()).now();
+              if (secret != null) {
+                return secret;
+              }
+              // Slow path - transactionally create a new ServerSecret (once per app setup).
+              return ofy().transact(new Work<ServerSecret>() {
+                @Override
+                public ServerSecret run() {
+                  // Check again for an existing secret within the transaction to avoid races.
+                  ServerSecret secret = ofy().load().entity(new ServerSecret()).now();
+                  if (secret == null) {
+                    UUID uuid = UUID.randomUUID();
+                    secret = create(uuid.getMostSignificantBits(), uuid.getLeastSignificantBits());
+                    ofy().saveWithoutBackup().entity(secret).now();
+                  }
+                  return secret;
+                }});
+            }
+          });
 
+  /** Returns the global ServerSecret instance, creating it if one isn't already in Datastore. */
+  public static ServerSecret get() {
+    try {
+      return CACHE.get(ServerSecret.class);
+    } catch (ExecutionException e) {
+      throw new RuntimeException(e);
+    }
+  }
+
+  /** Most significant 8 bytes of the UUID value. */
   long mostSignificant;
+
+  /** Least significant 8 bytes of the UUID value. */
   long leastSignificant;
 
-  /**
-   * Get the server secret, creating it if the Datastore doesn't have one already.
-   *
-   * <p>There's a tiny risk of a race here if two calls to this happen simultaneously and create
-   * different keys, in which case one of the calls will end up with an incorrect key. However, this
-   * happens precisely once in the history of the system (after that it's always in Datastore) so
-   * it's not worth worrying about.
-   */
-  public static UUID getServerSecret() {
-    if (secret == null) {
-      secret = ofy().load().entity(new ServerSecret()).now();
-    }
-    if (secret == null) {
-      secret = new ServerSecret();
-      UUID uuid = UUID.randomUUID();
-      secret.mostSignificant = uuid.getMostSignificantBits();
-      secret.leastSignificant = uuid.getLeastSignificantBits();
-      ofy().transact(new VoidWork(){
-        @Override
-        public void vrun() {
-          ofy().saveWithoutBackup().entity(secret);
-        }});
-    }
-    return new UUID(secret.mostSignificant, secret.leastSignificant);
+  @VisibleForTesting
+  static ServerSecret create(long mostSignificant, long leastSignificant) {
+    ServerSecret secret = new ServerSecret();
+    secret.mostSignificant = mostSignificant;
+    secret.leastSignificant = leastSignificant;
+    return secret;
+  }
+
+  /** Returns the value of this ServerSecret as a UUID. */
+  public UUID asUuid() {
+    return new UUID(mostSignificant, leastSignificant);
+  }
+
+  /** Returns the value of this ServerSecret as a byte array. */
+  public byte[] asBytes() {
+    return ByteBuffer.allocate(Longs.BYTES * 2)
+        .putLong(mostSignificant)
+        .putLong(leastSignificant)
+        .array();
+  }
+
+  @VisibleForTesting
+  static void resetCache() {
+    CACHE.invalidateAll();
   }
 }

java/google/registry/security/XsrfTokenManager.java

@@ -15,13 +15,13 @@
 package google.registry.security;
 
 import static com.google.common.io.BaseEncoding.base64Url;
-import static google.registry.model.server.ServerSecret.getServerSecret;
 import static java.nio.charset.StandardCharsets.UTF_8;
 
 import com.google.appengine.api.users.UserService;
 import com.google.common.base.Joiner;
 import com.google.common.base.Splitter;
 import com.google.common.hash.Hashing;
+import google.registry.model.server.ServerSecret;
 import google.registry.util.Clock;
 import google.registry.util.FormattingLogger;
 import java.util.List;
@@ -60,7 +60,9 @@ public final class XsrfTokenManager {
    */
   private static String encodeToken(long creationTime, @Nullable String scope, String userEmail) {
     String token =
-        Joiner.on('\t').skipNulls().join(getServerSecret(), userEmail, scope, creationTime);
+        Joiner.on('\t')
+            .skipNulls()
+            .join(ServerSecret.get().asUuid(), userEmail, scope, creationTime);
     return base64Url().encode(Hashing.sha256()
         .newHasher(token.length())
         .putString(token, UTF_8)

javatests/google/registry/model/server/ServerSecretTest.java

@@ -0,0 +1,80 @@
+// Copyright 2017 The Nomulus Authors. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package google.registry.model.server;
+
+import static com.google.common.truth.Truth.assertThat;
+import static google.registry.model.ofy.ObjectifyService.ofy;
+
+import google.registry.model.ofy.RequestCapturingAsyncDatastoreService;
+import google.registry.testing.AppEngineRule;
+import java.util.UUID;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** Unit tests for {@link ServerSecret}. */
+@RunWith(JUnit4.class)
+public class ServerSecretTest {
+  @Rule
+  public final AppEngineRule appEngine = AppEngineRule.builder()
+      .withDatastore()
+      .build();
+
+  @Before
+  public void before() {
+    ServerSecret.resetCache();
+  }
+
+  @Test
+  public void testGet_bootstrapping_savesSecretToDatastore() throws Exception {
+    ServerSecret secret = ServerSecret.get();
+    assertThat(secret).isNotNull();
+    assertThat(ofy().load().entity(new ServerSecret()).now()).isEqualTo(secret);
+  }
+
+  @Test
+  public void testGet_existingSecret_returned() throws Exception {
+    ServerSecret secret = ServerSecret.create(123, 456);
+    ofy().saveWithoutBackup().entity(secret).now();
+    assertThat(ServerSecret.get()).isEqualTo(secret);
+    assertThat(ofy().load().entity(new ServerSecret()).now()).isEqualTo(secret);
+  }
+
+  @Test
+  public void testGet_cachedSecret_returnedWithoutDatastoreRead() throws Exception {
+    int numInitialReads = RequestCapturingAsyncDatastoreService.getReads().size();
+    ServerSecret secret = ServerSecret.get();
+    int numReads = RequestCapturingAsyncDatastoreService.getReads().size();
+    assertThat(numReads).isGreaterThan(numInitialReads);
+    assertThat(ServerSecret.get()).isEqualTo(secret);
+    assertThat(RequestCapturingAsyncDatastoreService.getReads()).hasSize(numReads);
+  }
+
+  @Test
+  public void testAsUuid() throws Exception {
+    UUID uuid = ServerSecret.create(123, 456).asUuid();
+    assertThat(uuid.getMostSignificantBits()).isEqualTo(123);
+    assertThat(uuid.getLeastSignificantBits()).isEqualTo(456);
+  }
+
+  @Test
+  public void testAsBytes() throws Exception {
+    byte[] bytes = ServerSecret.create(123, 0x456).asBytes();
+    assertThat(bytes)
+        .isEqualTo(new byte[] {0, 0, 0, 0, 0, 0, 0, 123, 0, 0, 0, 0, 0, 0, 0x4, 0x56});
+  }
+}