diff --git a/java/google/registry/model/server/ServerSecret.java b/java/google/registry/model/server/ServerSecret.java index 6917dde80..88f90cad3 100644 --- a/java/google/registry/model/server/ServerSecret.java +++ b/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. + * + *

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, ServerSecret> CACHE = + CacheBuilder.newBuilder().build( + new CacheLoader, ServerSecret>() { + @Override + public ServerSecret load(Class 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() { + @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. - * - *

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(); } } diff --git a/java/google/registry/security/XsrfTokenManager.java b/java/google/registry/security/XsrfTokenManager.java index 653069fea..0e0418a88 100644 --- a/java/google/registry/security/XsrfTokenManager.java +++ b/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) diff --git a/javatests/google/registry/model/server/ServerSecretTest.java b/javatests/google/registry/model/server/ServerSecretTest.java new file mode 100644 index 000000000..8e701a679 --- /dev/null +++ b/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}); + } +}