Revert "Merge pull request #713 from spotify/more-auth-ITs"

This reverts commit 4ff70b4, reversing
changes made to 542be42.

circle.sh

@@ -28,7 +28,7 @@ case "$1" in
     ;;
 
   dependencies)
-    mvn clean install -T 2 -Dmaven.javadoc.skip=true -DskipTests=true -Dinvoker.skip=true -B -V
+    mvn clean install -T 2 -Dmaven.javadoc.skip=true -DskipTests=true -B -V
 
     ;;
 

helios-authentication/src/main/java/com/spotify/helios/auth/AuthenticationPlugin.java

@@ -17,9 +17,6 @@
 
 package com.spotify.helios.auth;
 
-import java.util.Map;
-import java.util.Set;
-
 import io.dropwizard.jersey.setup.JerseyEnvironment;
 
 /**
@@ -42,13 +39,7 @@ public interface AuthenticationPlugin<C> {
    */
   String schemeName();
 
-  /**
-   * Create the ServerAuthentication instance to use with this plugin.
-   *
-   * @param environment the environment variables that the Helios Master was started with. Provided
-   *                    as a method parameter rather than System.getenv() for ease of testing.
-   */
-  ServerAuthentication<C> serverAuthentication(Map<String, String> environment);
+  ServerAuthentication<C> serverAuthentication();
 
   ClientAuthentication<C> clientAuthentication();
 
@@ -76,17 +67,6 @@ interface ServerAuthentication<C> {
      * for multi-stepped authentication handshakes.
      */
     void registerAdditionalJerseyComponents(JerseyEnvironment env);
-
-    /**
-     * Returns a Set of URI paths (such as <code>auth</code>) that should not require
-     * authentication. Note that the path as presented by Jersey will not have the leading
-     * <code>/</code>.
-     * <p>
-     * If the plugin registers any endpoints for authentication handshakes in {@link
-     * #registerAdditionalJerseyComponents(JerseyEnvironment)} then it must also return the path(s)
-     * to those endpoint(s) in this method.</p>
-     */
-    Set<String> unauthenticatedPaths();
   }
 
   interface ClientAuthentication<C> {

helios-authentication/src/main/java/com/spotify/helios/auth/basic/BasicAuthenticationPlugin.java

@@ -18,20 +18,11 @@
 package com.spotify.helios.auth.basic;
 
 import com.google.auto.service.AutoService;
-import com.google.common.base.Throwables;
 
-import com.fasterxml.jackson.core.type.TypeReference;
-import com.fasterxml.jackson.databind.ObjectMapper;
 import com.spotify.helios.auth.AuthenticationPlugin;
 
-import java.io.File;
-import java.io.IOException;
-import java.util.Map;
-
 import io.dropwizard.auth.basic.BasicCredentials;
 
-import static com.google.common.base.Preconditions.checkNotNull;
-
 /** Proof of concept for the authentication plugin framework using HTTP Basic Auth. */
 @AutoService(AuthenticationPlugin.class)
 public class BasicAuthenticationPlugin implements AuthenticationPlugin<BasicCredentials> {
@@ -42,28 +33,8 @@ public String schemeName() {
   }
 
   @Override
-  public ServerAuthentication<BasicCredentials> serverAuthentication(
-      Map<String, String> environment) {
-
-    final String path = environment.get("AUTH_BASIC_USERDB");
-    checkNotNull(path,
-        "Environment variable AUTH_BASIC_USERDB not defined, required for "
-        + BasicAuthenticationPlugin.class.getSimpleName());
-
-    return new BasicServerAuthentication(readFileOfUsers(path));
-  }
-
-  private Map<String, String> readFileOfUsers(final String path) {
-    final File file = new File(path);
-    final ObjectMapper objectMapper = new ObjectMapper();
-    final TypeReference<Map<String, String>> typeToken = new TypeReference<Map<String, String>>() {
-    };
-
-    try {
-      return  objectMapper.readValue(file, typeToken);
-    } catch (IOException e) {
-      throw Throwables.propagate(e);
-    }
+  public ServerAuthentication<BasicCredentials> serverAuthentication() {
+    return new BasicServerAuthentication();
   }
 
   @Override

helios-authentication/src/main/java/com/spotify/helios/auth/basic/BasicServerAuthentication.java

@@ -17,22 +17,48 @@
 
 package com.spotify.helios.auth.basic;
 
-import com.spotify.helios.auth.SimpleServerAuthentication;
+import com.google.common.base.Throwables;
 
+import com.fasterxml.jackson.core.type.TypeReference;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.spotify.helios.auth.AuthenticationPlugin.ServerAuthentication;
+
+import java.io.File;
+import java.io.IOException;
 import java.util.Map;
 
 import io.dropwizard.auth.basic.BasicCredentials;
+import io.dropwizard.jersey.setup.JerseyEnvironment;
+
+import static com.google.common.base.Preconditions.checkNotNull;
 
 /**
  * A very simple implementation of ServerAuthentication to demonstrate how to implement an
  * authentication plugin. This version reads in a user "database" from a JSON file store at a path
  * configured by an environment variable, making it likely far too simple to be used for anything
  * but demonstrations.
  */
-public class BasicServerAuthentication extends SimpleServerAuthentication<BasicCredentials> {
+public class BasicServerAuthentication implements ServerAuthentication<BasicCredentials> {
 
   private final Map<String, String> users;
 
+  public BasicServerAuthentication() {
+    final String path = System.getenv("AUTH_BASIC_USERDB");
+    checkNotNull(path,
+        "Environment variable AUTH_BASIC_USERDB not defined, required for "
+        + BasicAuthenticationPlugin.class.getSimpleName());
+
+    File file = new File(path);
+    final ObjectMapper objectMapper = new ObjectMapper();
+
+    try {
+      this.users = objectMapper.readValue(file, new TypeReference<Map<String, String>>() {
+      });
+    } catch (IOException e) {
+      throw Throwables.propagate(e);
+    }
+  }
+
   public BasicServerAuthentication(Map<String, String> users) {
     this.users = users;
   }
@@ -42,4 +68,8 @@ public com.spotify.helios.auth.Authenticator<BasicCredentials> authenticator() {
     return new BasicAuthenticator(users);
   }
 
+  @Override
+  public void registerAdditionalJerseyComponents(JerseyEnvironment env) {
+    // nothing to add
+  }
 }

helios-authentication/src/test/java/com/spotify/helios/auth/AuthenticationPluginLoaderTest.java

@@ -23,8 +23,6 @@
 
 import org.junit.Test;
 
-import java.util.Map;
-
 import static org.hamcrest.CoreMatchers.instanceOf;
 import static org.junit.Assert.assertThat;
 
@@ -55,7 +53,7 @@ public String schemeName() {
     }
 
     @Override
-    public ServerAuthentication<String> serverAuthentication(Map<String, String> environment) {
+    public ServerAuthentication<String> serverAuthentication() {
       return null;
     }
 

helios-crtauth/src/main/java/com/spotify/helios/auth/crt/CrtAuthenticationPlugin.java

@@ -18,6 +18,7 @@
 package com.spotify.helios.auth.crt;
 
 import com.google.auto.service.AutoService;
+import com.google.common.annotations.VisibleForTesting;
 
 import com.spotify.crtauth.CrtAuthServer;
 import com.spotify.crtauth.keyprovider.KeyProvider;
@@ -31,24 +32,32 @@
 @AutoService(AuthenticationPlugin.class)
 public class CrtAuthenticationPlugin implements AuthenticationPlugin<CrtAccessToken> {
 
+  private final Map<String, String> environment;
+
+  public CrtAuthenticationPlugin() {
+    this(System.getenv());
+  }
+
+  @VisibleForTesting
+  protected CrtAuthenticationPlugin(Map<String, String> environment) {
+    this.environment = environment;
+  }
+
   @Override
   public String schemeName() {
     return "crtauth";
   }
 
   @Override
-  public ServerAuthentication<CrtAccessToken> serverAuthentication(
-      Map<String, String> environment) {
-
+  public ServerAuthentication<CrtAccessToken> serverAuthentication() {
     // only validate the presence of environment variables when this method is called, as opposed to
     // in the constructor, as the client-side code will not use the same environment variables
-    final String ldapUrl = getRequired(environment, "CRTAUTH_LDAP_URL");
-    final String ldapSearchPath = getRequired(environment, "CRTAUTH_LDAP_SEARCH_PATH");
-    final String serverName = getRequired(environment, "CRTAUTH_SERVERNAME");
-    final String secret = getRequired(environment, "CRTAUTH_SECRET");
-    final String ldapFieldNameOfKey =
-        getOptional(environment, "CRTAUTH_LDAP_KEY_FIELDNAME", "sshPublicKey");
-    final int tokenLifetimeSecs = getOptional(environment, "CRTAUTH_TOKEN_LIFETIME_SECS", 540);
+    final String ldapUrl =  getRequiredEnv("CRTAUTH_LDAP_URL");
+    final String ldapSearchPath = getRequiredEnv("CRTAUTH_LDAP_SEARCH_PATH");
+    final String serverName = getRequiredEnv("CRTAUTH_SERVERNAME");
+    final String secret = getRequiredEnv("CRTAUTH_SECRET");
+    final String ldapFieldNameOfKey = getOptionalEnv("CRTAUTH_LDAP_KEY_FIELDNAME", "sshPublicKey");
+    final int tokenLifetimeSecs = getOptionalEnv("CRTAUTH_TOKEN_LIFETIME_SECS", 540);
 
     final LdapContextSource contextSource = new LdapContextSource();
     contextSource.setUrl(ldapUrl);
@@ -71,25 +80,24 @@ public ServerAuthentication<CrtAccessToken> serverAuthentication(
     return new CrtServerAuthentication(new CrtTokenAuthenticator(authServer), authServer);
   }
 
-  private static String getEnv(Map<String, String> environment, String name, boolean required) {
+  private String getEnv(String name, boolean required) {
     if (required && !environment.containsKey(name)) {
       throw new IllegalArgumentException("Environment variable " + name + " is required");
     }
     return environment.get(name);
   }
 
-  private static String getRequired(Map<String, String> environment, String name) {
-    return getEnv(environment, name, true);
+  private String getRequiredEnv(String name) {
+    return getEnv(name, true);
   }
 
-  private static String getOptional(Map<String, String> environment, String name,
-                                    String defaultValue) {
-    final String defined = getEnv(environment, name, false);
+  private String getOptionalEnv(String name, String defaultValue) {
+    final String defined = getEnv(name, false);
     return defined != null ? defined : defaultValue;
   }
 
-  private static int getOptional(Map<String, String> environment, String name, int defaultValue) {
-    final String defined = getEnv(environment, name, false);
+  private int getOptionalEnv(String name, int defaultValue) {
+    final String defined = getEnv(name, false);
     if (defined == null) {
       return defaultValue;
     }

helios-crtauth/src/main/java/com/spotify/helios/auth/crt/CrtHandshakeResource.java

@@ -31,10 +31,9 @@
 import javax.ws.rs.core.Response;
 import javax.ws.rs.core.Response.Status;
 
-@Path(CrtHandshakeResource.PATH)
+@Path("/_auth")
 public class CrtHandshakeResource {
 
-  public static final String PATH = "_auth";
   private static final Logger log = LoggerFactory.getLogger(CrtHandshakeResource.class);
 
   private final CrtAuthServer authServer;

helios-crtauth/src/main/java/com/spotify/helios/auth/crt/CrtServerAuthentication.java

@@ -17,14 +17,10 @@
 
 package com.spotify.helios.auth.crt;
 
-import com.google.common.collect.ImmutableSet;
-
 import com.spotify.crtauth.CrtAuthServer;
 import com.spotify.helios.auth.AuthenticationPlugin.ServerAuthentication;
 import com.spotify.helios.auth.Authenticator;
 
-import java.util.Set;
-
 import io.dropwizard.jersey.setup.JerseyEnvironment;
 
 public class CrtServerAuthentication implements ServerAuthentication<CrtAccessToken> {
@@ -47,8 +43,4 @@ public void registerAdditionalJerseyComponents(JerseyEnvironment env) {
     env.register(new CrtHandshakeResource(this.authServer));
   }
 
-  @Override
-  public Set<String> unauthenticatedPaths() {
-    return ImmutableSet.of(CrtHandshakeResource.PATH);
-  }
 }

helios-crtauth/src/test/java/com/spotify/helios/auth/crt/CrtAuthenticationPluginTest.java

@@ -49,14 +49,14 @@ public void serverAuthentication_NoEnvironmentVariables() {
 
     final Map<String, String> env = ImmutableMap.of();
 
-    final CrtAuthenticationPlugin plugin = new CrtAuthenticationPlugin();
-    plugin.serverAuthentication(env);
+    final CrtAuthenticationPlugin plugin = new CrtAuthenticationPlugin(env);
+    plugin.serverAuthentication();
   }
 
   @Test
   public void serverAuthentication_AllRequiredArgs() {
-    final CrtAuthenticationPlugin plugin = new CrtAuthenticationPlugin();
-    plugin.serverAuthentication(requiredEnvArgs);
+    final CrtAuthenticationPlugin plugin = new CrtAuthenticationPlugin(requiredEnvArgs);
+    plugin.serverAuthentication();
   }
 
   @Test
@@ -66,7 +66,7 @@ public void serverAuthentication_BadlyFormattedTokenLifetime() {
     final Map<String, String> env = new HashMap<>(requiredEnvArgs);
     env.put("CRTAUTH_TOKEN_LIFETIME_SECS", "asdf");
 
-    final CrtAuthenticationPlugin plugin = new CrtAuthenticationPlugin();
-    plugin.serverAuthentication(env);
+    final CrtAuthenticationPlugin plugin = new CrtAuthenticationPlugin(env);
+    plugin.serverAuthentication();
   }
 }