Clean up the tattered shreds of SessionMetadata

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

java/google/registry/flows/HttpSessionMetadata.java

@@ -14,42 +14,77 @@
 
 package google.registry.flows;
 
-import static com.google.common.base.Preconditions.checkState;
+import static com.google.common.base.MoreObjects.toStringHelper;
+import static google.registry.util.CollectionUtils.nullToEmpty;
+
+import com.google.common.base.Joiner;
+import com.google.common.base.Optional;
+
+import java.util.Set;
 
 import javax.servlet.http.HttpSession;
 
 /** A metadata class that is a wrapper around {@link HttpSession}. */
-public class HttpSessionMetadata extends SessionMetadata {
+public class HttpSessionMetadata implements SessionMetadata {
+
+  private static final String CLIENT_ID = "CLIENT_ID";
+  private static final String SERVICE_EXTENSIONS = "SERVICE_EXTENSIONS";
+  private static final String FAILED_LOGIN_ATTEMPTS = "FAILED_LOGIN_ATTEMPTS";
 
   private final HttpSession session;
-  private boolean isValid = true;
 
   public HttpSessionMetadata(HttpSession session) {
     this.session = session;
   }
 
-  @Override
-  protected void checkValid() {
-    checkState(isValid, "This session has been invalidated.");
-  }
-
   @Override
   public void invalidate() {
     session.invalidate();
-    isValid = false;
   }
 
   @Override
-  protected void setProperty(String key, Object value) {
-    if (value == null) {
-      session.removeAttribute(key);
-    } else {
-      session.setAttribute(key, value);
-    }
+  public String getClientId() {
+    return (String) session.getAttribute(CLIENT_ID);
   }
 
   @Override
-  protected Object getProperty(String key) {
-    return session.getAttribute(key);
+  @SuppressWarnings("unchecked")
+  public Set<String> getServiceExtensionUris() {
+    return (Set<String>) session.getAttribute(SERVICE_EXTENSIONS);
+  }
+
+  @Override
+  public void setClientId(String clientId) {
+    session.setAttribute(CLIENT_ID, clientId);
+  }
+
+  @Override
+  public void setServiceExtensionUris(Set<String> serviceExtensionUris) {
+    session.setAttribute(SERVICE_EXTENSIONS, serviceExtensionUris);
+  }
+
+  @Override
+  public int getFailedLoginAttempts() {
+    return Optional.fromNullable((Integer) session.getAttribute(FAILED_LOGIN_ATTEMPTS)).or(0);
+  }
+
+  @Override
+  public void incrementFailedLoginAttempts() {
+    session.setAttribute(FAILED_LOGIN_ATTEMPTS, getFailedLoginAttempts() + 1);
+  }
+
+  @Override
+  public void resetFailedLoginAttempts() {
+    session.removeAttribute(FAILED_LOGIN_ATTEMPTS);
+  }
+
+  @Override
+  public String toString() {
+    return toStringHelper(getClass())
+        .add("system hash code", System.identityHashCode(this))
+        .add("clientId", getClientId())
+        .add("failedLoginAttempts", getFailedLoginAttempts())
+        .add("serviceExtensionUris", Joiner.on('.').join(nullToEmpty(getServiceExtensionUris())))
+        .toString();
   }
 }

java/google/registry/flows/SessionMetadata.java

@@ -14,95 +14,29 @@
 
 package google.registry.flows;
 
-import static com.google.common.base.MoreObjects.toStringHelper;
-import static com.google.common.base.Preconditions.checkNotNull;
-import static google.registry.util.CollectionUtils.nullToEmpty;
-
-import com.google.common.base.Joiner;
-import com.google.common.base.Optional;
-
 import java.util.Set;
 
-/** Class to allow setting and retrieving session information in flows. */
-public abstract class SessionMetadata {
-
-  /** The key used for looking up the current client id on the session object. */
-  protected static final String CLIENT_ID_KEY = "CLIENT_ID";
-
-  /** The key used for looking up the service extensions on the session object. */
-  protected static final String SERVICE_EXTENSIONS_KEY = "SERVICE_EXTENSIONS";
-
-  /** The key used for looking up the number of failed login attempts. */
-  protected static final String FAILED_LOGIN_ATTEMPTS_KEY = "FAILED_LOGIN_ATTEMPTS";
-
-  protected abstract void setProperty(String key, Object value);
-
-  protected abstract Object getProperty(String key);
+/** Object to allow setting and retrieving session information in flows. */
+public interface SessionMetadata {
 
   /**
    * Invalidates the session. A new instance must be created after this for future sessions.
    * Attempts to invoke methods of this class after this method has been called will throw
    * {@code IllegalStateException}.
    */
-  public abstract void invalidate();
+  void invalidate();
 
-  /** Subclasses can override this to verify that this is a valid session. */
-  protected void checkValid() {}
+  String getClientId();
 
-  /** Check that the session is valid and set a property. */
-  private void setPropertyChecked(String key, Object value) {
-    checkValid();
-    setProperty(key, value);
-  }
+  Set<String> getServiceExtensionUris();
 
-  /**
-   * Check that the session is valid and get a property as a given type.
-   *
-   * @param clazz type to return, specified as a param to enforce typesafe generics
-   * @see "http://errorprone.info/bugpattern/TypeParameterUnusedInFormals"
-   */
-  private <T> T getProperty(Class<T> clazz, String key) {
-    checkValid();
-    return clazz.cast(getProperty(key));
-  }
+  void setClientId(String clientId);
 
-  public String getClientId() {
-    return getProperty(String.class, CLIENT_ID_KEY);
-  }
+  void setServiceExtensionUris(Set<String> serviceExtensionUris);
 
-  @SuppressWarnings("unchecked")
-  public Set<String> getServiceExtensionUris() {
-    return getProperty(Set.class, SERVICE_EXTENSIONS_KEY);
-  }
+  int getFailedLoginAttempts();
 
-  public void setClientId(String clientId) {
-    setPropertyChecked(CLIENT_ID_KEY, clientId);
-  }
+  void incrementFailedLoginAttempts();
 
-  public void setServiceExtensionUris(Set<String> serviceExtensionUris) {
-    setPropertyChecked(SERVICE_EXTENSIONS_KEY, checkNotNull(serviceExtensionUris));
-  }
-
-  public int getFailedLoginAttempts() {
-    return Optional.fromNullable(getProperty(Integer.class, FAILED_LOGIN_ATTEMPTS_KEY))
-        .or(0);
-  }
-
-  public void incrementFailedLoginAttempts() {
-    setPropertyChecked(FAILED_LOGIN_ATTEMPTS_KEY, getFailedLoginAttempts() + 1);
-  }
-
-  public void resetFailedLoginAttempts() {
-    setPropertyChecked(FAILED_LOGIN_ATTEMPTS_KEY, null);
-  }
-
-  @Override
-  public String toString() {
-    return toStringHelper(getClass())
-        .add("system hash code", System.identityHashCode(this))
-        .add("clientId", getClientId())
-        .add("failedLoginAttempts", getFailedLoginAttempts())
-        .add("serviceExtensionUris", Joiner.on('.').join(nullToEmpty(getServiceExtensionUris())))
-        .toString();
-  }
+  void resetFailedLoginAttempts();
 }

java/google/registry/flows/StatelessRequestSessionMetadata.java

@@ -14,19 +14,25 @@
 
 package google.registry.flows;
 
+import static com.google.common.base.MoreObjects.toStringHelper;
+import static dagger.internal.Preconditions.checkNotNull;
+import static google.registry.util.CollectionUtils.nullToEmpty;
+
+import com.google.common.base.Joiner;
+import com.google.common.collect.ImmutableSet;
+
 import java.util.Set;
 
 /** A read-only {@link SessionMetadata} that doesn't support login/logout. */
-public class StatelessRequestSessionMetadata extends SessionMetadata {
+public class StatelessRequestSessionMetadata implements SessionMetadata {
 
   private final String clientId;
-  private final Set<String> serviceExtensionUris;
+  private final ImmutableSet<String> serviceExtensionUris;
 
   public StatelessRequestSessionMetadata(
-      String clientId,
-      Set<String> serviceExtensionUris) {
-    this.clientId = clientId;
-    this.serviceExtensionUris = serviceExtensionUris;
+      String clientId, ImmutableSet<String> serviceExtensionUris) {
+    this.clientId = checkNotNull(clientId);
+    this.serviceExtensionUris = checkNotNull(serviceExtensionUris);
   }
 
   @Override
@@ -45,14 +51,38 @@ public class StatelessRequestSessionMetadata extends SessionMetadata {
   }
 
   @Override
-  protected void setProperty(String key, Object value) {
+  public void setClientId(String clientId) {
     throw new UnsupportedOperationException();
   }
 
   @Override
-  protected Object getProperty(String key) {
-    // We've overridden the getters of all of the properties that we care about. Return null for
-    // everything else so that toString() continues to work.
-    return null;
+  public void setServiceExtensionUris(Set<String> serviceExtensionUris) {
+    throw new UnsupportedOperationException();
+  }
+
+  @Override
+  public int getFailedLoginAttempts() {
+    throw new UnsupportedOperationException();
+  }
+
+  @Override
+  public void incrementFailedLoginAttempts() {
+    throw new UnsupportedOperationException();
+  }
+
+  @Override
+  public void resetFailedLoginAttempts() {
+    throw new UnsupportedOperationException();
+  }
+
+
+  @Override
+  public String toString() {
+    return toStringHelper(getClass())
+        .add("system hash code", System.identityHashCode(this))
+        .add("clientId", getClientId())
+        .add("serviceExtensionUris", Joiner.on('.').join(nullToEmpty(getServiceExtensionUris())))
+        .toString();
   }
 }
+

java/google/registry/tools/ValidateLoginCredentialsCommand.java

@@ -33,7 +33,7 @@ import com.beust.jcommander.Parameters;
 
 import google.registry.flows.EppRequestSource;
 import google.registry.flows.FlowRunner;
-import google.registry.flows.HttpSessionMetadata;
+import google.registry.flows.SessionMetadata;
 import google.registry.flows.TlsCredentials;
 import google.registry.flows.session.LoginFlow;
 import google.registry.model.eppcommon.Trid;
@@ -42,11 +42,11 @@ import google.registry.tools.Command.GtechCommand;
 import google.registry.tools.Command.RemoteApiCommand;
 import google.registry.tools.params.PathParameter;
 import google.registry.tools.soy.LoginSoyInfo;
-import google.registry.util.BasicHttpSession;
 import google.registry.util.SystemClock;
 
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.util.Set;
 
 import javax.annotation.Nullable;
 
@@ -100,12 +100,13 @@ final class ValidateLoginCredentialsCommand implements RemoteApiCommand, GtechCo
         .setData(new SoyMapData("clientIdentifier", clientIdentifier, "password", password))
         .render()
         .getBytes(UTF_8);
+
     System.out.println(new String(marshalWithLenientRetry(
         new FlowRunner(
             LoginFlow.class,
             unmarshal(EppInput.class, inputXmlBytes),
             Trid.create(null),
-            new HttpSessionMetadata(new BasicHttpSession()),
+            new StubSessionMetadata(),
             new TlsCredentials(
                 clientCertificateHash,
                 Optional.of(clientIpAddress),
@@ -117,4 +118,38 @@ final class ValidateLoginCredentialsCommand implements RemoteApiCommand, GtechCo
             null,
             new SystemClock()).run()), UTF_8));
   }
+
+  /** A {@link SessionMetadata} that ignores setters rather than throwing exceptions. */
+  private static class StubSessionMetadata implements SessionMetadata {
+
+    @Override
+    public void setClientId(String clientId) {}
+
+    @Override
+    public void setServiceExtensionUris(Set<String> serviceExtensionUris) {}
+
+    @Override
+    public void incrementFailedLoginAttempts() {}
+
+    @Override
+    public void resetFailedLoginAttempts() {}
+
+    @Override
+    public void invalidate() {}
+
+    @Override
+    public String getClientId() {
+      return null;
+    }
+
+    @Override
+    public Set<String> getServiceExtensionUris() {
+      return null;
+    }
+
+    @Override
+    public int getFailedLoginAttempts() {
+      return 0;
+    }
+  }
 }

javatests/google/registry/flows/EppConsoleActionTest.java

@@ -22,9 +22,9 @@ import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
 
 import google.registry.testing.AppEngineRule;
+import google.registry.testing.FakeHttpSession;
 import google.registry.testing.ShardableTestCase;
 import google.registry.testing.UserInfo;
-import google.registry.util.BasicHttpSession;
 
 import org.junit.Rule;
 import org.junit.Test;
@@ -47,7 +47,7 @@ public class EppConsoleActionTest extends ShardableTestCase {
   public void doTest() {
     EppConsoleAction action = new EppConsoleAction();
     action.inputXmlBytes = INPUT_XML_BYTES;
-    action.session = new BasicHttpSession();
+    action.session = new FakeHttpSession();
     action.session.setAttribute("CLIENT_ID", "ClientIdentifier");
     action.eppRequestHandler = mock(EppRequestHandler.class);
     action.run();

javatests/google/registry/flows/EppTestCase.java

@@ -28,10 +28,10 @@ import com.google.common.net.MediaType;
 import google.registry.model.ofy.Ofy;
 import google.registry.monitoring.whitebox.EppMetrics;
 import google.registry.testing.FakeClock;
+import google.registry.testing.FakeHttpSession;
 import google.registry.testing.FakeResponse;
 import google.registry.testing.InjectRule;
 import google.registry.testing.ShardableTestCase;
-import google.registry.testing.TestSessionMetadata;
 
 import org.joda.time.DateTime;
 import org.junit.Before;
@@ -49,7 +49,7 @@ public class EppTestCase extends ShardableTestCase {
 
   private final FakeClock clock = new FakeClock();
 
-  private TestSessionMetadata sessionMetadata;
+  private SessionMetadata sessionMetadata;
   private TransportCredentials credentials = new PasswordOnlyTransportCredentials();
   private boolean isSuperuser;
 
@@ -94,12 +94,16 @@ public class EppTestCase extends ShardableTestCase {
     String expectedOutput =
         loadFileWithSubstitutions(getClass(), outputFilename, outputSubstitutions);
     if (sessionMetadata == null) {
-      sessionMetadata = new TestSessionMetadata();
+      sessionMetadata = new HttpSessionMetadata(new FakeHttpSession()) {
+        @Override
+        public void invalidate() {
+          // When a session is invalidated, reset the sessionMetadata field.
+          super.invalidate();
+          EppTestCase.this.sessionMetadata = null;
+        }
+      };
     }
     String actualOutput = executeXmlCommand(input);
-    if (!sessionMetadata.isValid()) {
-      sessionMetadata = null;
-    }
     assertXmlEqualsWithMessage(
         expectedOutput,
         actualOutput,

javatests/google/registry/flows/EppTlsActionTest.java

@@ -23,8 +23,8 @@ import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
+import google.registry.testing.FakeHttpSession;
 import google.registry.testing.ShardableTestCase;
-import google.registry.util.BasicHttpSession;
 
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -43,7 +43,7 @@ public class EppTlsActionTest extends ShardableTestCase {
     action.inputXmlBytes = INPUT_XML_BYTES;
     action.tlsCredentials = mock(TlsCredentials.class);
     when(action.tlsCredentials.hasSni()).thenReturn(true);
-    action.session = new BasicHttpSession();
+    action.session = new FakeHttpSession();
     action.session.setAttribute("CLIENT_ID", "ClientIdentifier");
     action.eppRequestHandler = mock(EppRequestHandler.class);
     action.run();

javatests/google/registry/flows/FlowTestCase.java

@@ -49,8 +49,8 @@ import google.registry.model.tmch.ClaimsListShard.ClaimsListSingleton;
 import google.registry.testing.AppEngineRule;
 import google.registry.testing.EppLoader;
 import google.registry.testing.FakeClock;
+import google.registry.testing.FakeHttpSession;
 import google.registry.testing.InjectRule;
-import google.registry.testing.TestSessionMetadata;
 import google.registry.util.TypeUtils.TypeInstantiator;
 import google.registry.xml.ValidationMode;
 
@@ -95,7 +95,7 @@ public abstract class FlowTestCase<F extends Flow> {
 
   @Before
   public void init() throws Exception {
-    sessionMetadata = new TestSessionMetadata();
+    sessionMetadata = new HttpSessionMetadata(new FakeHttpSession());
     sessionMetadata.setClientId("TheRegistrar");
     sessionMetadata.setServiceExtensionUris(ProtocolDefinition.getVisibleServiceExtensionUris());
     ofy().saveWithoutBackup().entity(new ClaimsListSingleton()).now();

javatests/google/registry/model/EppResourceUtilsTest.java

@@ -32,6 +32,7 @@ import com.googlecode.objectify.Key;
 
 import google.registry.flows.EppRequestSource;
 import google.registry.flows.FlowRunner;
+import google.registry.flows.HttpSessionMetadata;
 import google.registry.flows.PasswordOnlyTransportCredentials;
 import google.registry.flows.SessionMetadata;
 import google.registry.model.domain.DomainResource;
@@ -42,8 +43,8 @@ import google.registry.testing.AppEngineRule;
 import google.registry.testing.EppLoader;
 import google.registry.testing.ExceptionRule;
 import google.registry.testing.FakeClock;
+import google.registry.testing.FakeHttpSession;
 import google.registry.testing.InjectRule;
-import google.registry.testing.TestSessionMetadata;
 
 import org.joda.time.DateTime;
 import org.joda.time.Duration;
@@ -79,7 +80,7 @@ public class EppResourceUtilsTest {
   }
 
   private void runFlow() throws Exception {
-    SessionMetadata sessionMetadata = new TestSessionMetadata();
+    SessionMetadata sessionMetadata = new HttpSessionMetadata(new FakeHttpSession());
     sessionMetadata.setClientId("TheRegistrar");
     new FlowRunner(
         getFlowClass(eppLoader.getEpp()),

javatests/google/registry/testing/FakeHttpSession.java

@@ -12,7 +12,9 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package google.registry.util;
+package google.registry.testing;
+
+import static com.google.common.base.Preconditions.checkState;
 
 import java.util.Enumeration;
 import java.util.HashMap;
@@ -23,11 +25,12 @@ import javax.servlet.ServletContext;
 import javax.servlet.http.HttpSession;
 import javax.servlet.http.HttpSessionContext;
 
-/** An {@link HttpSession} that only provides support for getting/setting attributes. */
+/** A fake {@link HttpSession} that only provides support for getting/setting attributes. */
 @SuppressWarnings("deprecation")
-public class BasicHttpSession implements HttpSession {
+public class FakeHttpSession implements HttpSession {
 
   private final Map<String, Object> map = new HashMap<>();
+
   boolean isValid = true;
 
   @Override
@@ -67,7 +70,7 @@ public class BasicHttpSession implements HttpSession {
 
   @Override
   public Object getAttribute(@Nullable String name) {
-    checkValid();
+    checkState(isValid, "This session has been invalidated.");
     return map.get(name);
   }
 
@@ -88,7 +91,7 @@ public class BasicHttpSession implements HttpSession {
 
   @Override
   public void setAttribute(@Nullable String name, @Nullable Object value) {
-    checkValid();
+    checkState(isValid, "This session has been invalidated.");
     map.put(name, value);
   }
 
@@ -99,7 +102,7 @@ public class BasicHttpSession implements HttpSession {
 
   @Override
   public void removeAttribute(@Nullable String name) {
-    checkValid();
+    checkState(isValid, "This session has been invalidated.");
     map.remove(name);
   }
 
@@ -118,10 +121,4 @@ public class BasicHttpSession implements HttpSession {
   public boolean isNew() {
     throw new UnsupportedOperationException();
   }
-
-  private void checkValid() {
-    if (!isValid) {
-      throw new IllegalStateException("This session has been invalidated.");
-    }
-  }
 }

javatests/google/registry/testing/TestSessionMetadata.java

@@ -1,52 +0,0 @@
-// Copyright 2016 The Domain Registry 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.testing;
-
-import static com.google.common.base.Preconditions.checkState;
-
-import google.registry.flows.SessionMetadata;
-
-import java.util.HashMap;
-import java.util.Map;
-
-public class TestSessionMetadata extends SessionMetadata {
-
-  private final Map<String, Object> properties = new HashMap<>();
-  private boolean isValid = true;
-
-  @Override
-  protected void setProperty(String key, Object value) {
-    properties.put(key, value);
-  }
-
-  @Override
-  protected Object getProperty(String key) {
-    return properties.get(key);
-  }
-
-  @Override
-  public void checkValid() {
-    checkState(isValid, "Session was invalidated");
-  }
-
-  @Override
-  public void invalidate() {
-    isValid = false;
-  }
-
-  public boolean isValid() {
-    return isValid;
-  }
-}