From 96475866988ad744a8632b7d388f5c3a852e3b85 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Staffan=20Gim=C3=A5ker?= Date: Wed, 28 Oct 2015 18:27:59 -0400 Subject: [PATCH] Revert "Merge pull request #713 from spotify/more-auth-ITs" This reverts commit 4ff70b4ad8c919c3f04c7cc43fa2d87068f7ed39, reversing changes made to 542be42d827832e122c04b01a945a1851bc346c7. --- circle.sh | 2 +- .../helios/auth/AuthenticationPlugin.java | 22 +--- .../auth/SimpleServerAuthentication.java | 43 -------- .../auth/basic/BasicAuthenticationPlugin.java | 33 +----- .../auth/basic/BasicServerAuthentication.java | 34 +++++- .../auth/AuthenticationPluginLoaderTest.java | 4 +- .../auth/crt/CrtAuthenticationPlugin.java | 44 +++++--- .../helios/auth/crt/CrtHandshakeResource.java | 3 +- .../auth/crt/CrtServerAuthentication.java | 8 -- .../auth/crt/CrtAuthenticationPluginTest.java | 12 +- .../com/spotify/helios/master/MasterMain.java | 35 ++---- .../spotify/helios/master/MasterService.java | 45 ++------ .../system/MasterAuthenticationTest.java | 103 +++--------------- .../spotify/helios/system/SystemTestBase.java | 20 +--- 14 files changed, 106 insertions(+), 302 deletions(-) delete mode 100644 helios-authentication/src/main/java/com/spotify/helios/auth/SimpleServerAuthentication.java diff --git a/circle.sh b/circle.sh index 027ba7dc2..c0947b3e4 100755 --- a/circle.sh +++ b/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 ;; diff --git a/helios-authentication/src/main/java/com/spotify/helios/auth/AuthenticationPlugin.java b/helios-authentication/src/main/java/com/spotify/helios/auth/AuthenticationPlugin.java index 298aa05e4..41e328f77 100644 --- a/helios-authentication/src/main/java/com/spotify/helios/auth/AuthenticationPlugin.java +++ b/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 { */ 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 serverAuthentication(Map environment); + ServerAuthentication serverAuthentication(); ClientAuthentication clientAuthentication(); @@ -76,17 +67,6 @@ interface ServerAuthentication { * for multi-stepped authentication handshakes. */ void registerAdditionalJerseyComponents(JerseyEnvironment env); - - /** - * Returns a Set of URI paths (such as auth) that should not require - * authentication. Note that the path as presented by Jersey will not have the leading - * /. - *

- * 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.

- */ - Set unauthenticatedPaths(); } interface ClientAuthentication { diff --git a/helios-authentication/src/main/java/com/spotify/helios/auth/SimpleServerAuthentication.java b/helios-authentication/src/main/java/com/spotify/helios/auth/SimpleServerAuthentication.java deleted file mode 100644 index 2c7efb314..000000000 --- a/helios-authentication/src/main/java/com/spotify/helios/auth/SimpleServerAuthentication.java +++ /dev/null @@ -1,43 +0,0 @@ -/* - * Copyright (c) 2014 Spotify AB. - * - * 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 com.spotify.helios.auth; - -import com.google.common.collect.ImmutableSet; - -import com.spotify.helios.auth.AuthenticationPlugin.ServerAuthentication; - -import java.util.Set; - -import io.dropwizard.jersey.setup.JerseyEnvironment; - -/** - * A base authentication plugin class that can be used if the plugin does not need to add any - * additional Jersey components. - */ -public abstract class SimpleServerAuthentication implements ServerAuthentication { - - @Override - public final void registerAdditionalJerseyComponents(final JerseyEnvironment env) { - // do nothing - } - - @Override - public Set unauthenticatedPaths() { - return ImmutableSet.of(); - } -} diff --git a/helios-authentication/src/main/java/com/spotify/helios/auth/basic/BasicAuthenticationPlugin.java b/helios-authentication/src/main/java/com/spotify/helios/auth/basic/BasicAuthenticationPlugin.java index fcac8fccc..803c672a1 100644 --- a/helios-authentication/src/main/java/com/spotify/helios/auth/basic/BasicAuthenticationPlugin.java +++ b/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 { @@ -42,28 +33,8 @@ public String schemeName() { } @Override - public ServerAuthentication serverAuthentication( - Map 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 readFileOfUsers(final String path) { - final File file = new File(path); - final ObjectMapper objectMapper = new ObjectMapper(); - final TypeReference> typeToken = new TypeReference>() { - }; - - try { - return objectMapper.readValue(file, typeToken); - } catch (IOException e) { - throw Throwables.propagate(e); - } + public ServerAuthentication serverAuthentication() { + return new BasicServerAuthentication(); } @Override diff --git a/helios-authentication/src/main/java/com/spotify/helios/auth/basic/BasicServerAuthentication.java b/helios-authentication/src/main/java/com/spotify/helios/auth/basic/BasicServerAuthentication.java index c00b0a293..a76df9ca8 100644 --- a/helios-authentication/src/main/java/com/spotify/helios/auth/basic/BasicServerAuthentication.java +++ b/helios-authentication/src/main/java/com/spotify/helios/auth/basic/BasicServerAuthentication.java @@ -17,11 +17,20 @@ 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 @@ -29,10 +38,27 @@ * configured by an environment variable, making it likely far too simple to be used for anything * but demonstrations. */ -public class BasicServerAuthentication extends SimpleServerAuthentication { +public class BasicServerAuthentication implements ServerAuthentication { private final Map 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>() { + }); + } catch (IOException e) { + throw Throwables.propagate(e); + } + } + public BasicServerAuthentication(Map users) { this.users = users; } @@ -42,4 +68,8 @@ public com.spotify.helios.auth.Authenticator authenticator() { return new BasicAuthenticator(users); } + @Override + public void registerAdditionalJerseyComponents(JerseyEnvironment env) { + // nothing to add + } } diff --git a/helios-authentication/src/test/java/com/spotify/helios/auth/AuthenticationPluginLoaderTest.java b/helios-authentication/src/test/java/com/spotify/helios/auth/AuthenticationPluginLoaderTest.java index 908bf1d4c..2970cb09b 100644 --- a/helios-authentication/src/test/java/com/spotify/helios/auth/AuthenticationPluginLoaderTest.java +++ b/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 serverAuthentication(Map environment) { + public ServerAuthentication serverAuthentication() { return null; } diff --git a/helios-crtauth/src/main/java/com/spotify/helios/auth/crt/CrtAuthenticationPlugin.java b/helios-crtauth/src/main/java/com/spotify/helios/auth/crt/CrtAuthenticationPlugin.java index f6001c524..1269d4f90 100644 --- a/helios-crtauth/src/main/java/com/spotify/helios/auth/crt/CrtAuthenticationPlugin.java +++ b/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 { + private final Map environment; + + public CrtAuthenticationPlugin() { + this(System.getenv()); + } + + @VisibleForTesting + protected CrtAuthenticationPlugin(Map environment) { + this.environment = environment; + } + @Override public String schemeName() { return "crtauth"; } @Override - public ServerAuthentication serverAuthentication( - Map environment) { - + public ServerAuthentication 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 serverAuthentication( return new CrtServerAuthentication(new CrtTokenAuthenticator(authServer), authServer); } - private static String getEnv(Map 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 environment, String name) { - return getEnv(environment, name, true); + private String getRequiredEnv(String name) { + return getEnv(name, true); } - private static String getOptional(Map 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 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; } diff --git a/helios-crtauth/src/main/java/com/spotify/helios/auth/crt/CrtHandshakeResource.java b/helios-crtauth/src/main/java/com/spotify/helios/auth/crt/CrtHandshakeResource.java index 8a9554b02..0fa4d30ac 100644 --- a/helios-crtauth/src/main/java/com/spotify/helios/auth/crt/CrtHandshakeResource.java +++ b/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; diff --git a/helios-crtauth/src/main/java/com/spotify/helios/auth/crt/CrtServerAuthentication.java b/helios-crtauth/src/main/java/com/spotify/helios/auth/crt/CrtServerAuthentication.java index 90eff37bc..0d8ebdf1d 100644 --- a/helios-crtauth/src/main/java/com/spotify/helios/auth/crt/CrtServerAuthentication.java +++ b/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 { @@ -47,8 +43,4 @@ public void registerAdditionalJerseyComponents(JerseyEnvironment env) { env.register(new CrtHandshakeResource(this.authServer)); } - @Override - public Set unauthenticatedPaths() { - return ImmutableSet.of(CrtHandshakeResource.PATH); - } } diff --git a/helios-crtauth/src/test/java/com/spotify/helios/auth/crt/CrtAuthenticationPluginTest.java b/helios-crtauth/src/test/java/com/spotify/helios/auth/crt/CrtAuthenticationPluginTest.java index 796f35b17..1a522feb0 100644 --- a/helios-crtauth/src/test/java/com/spotify/helios/auth/crt/CrtAuthenticationPluginTest.java +++ b/helios-crtauth/src/test/java/com/spotify/helios/auth/crt/CrtAuthenticationPluginTest.java @@ -49,14 +49,14 @@ public void serverAuthentication_NoEnvironmentVariables() { final Map 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 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(); } } \ No newline at end of file diff --git a/helios-services/src/main/java/com/spotify/helios/master/MasterMain.java b/helios-services/src/main/java/com/spotify/helios/master/MasterMain.java index 88ae8ca74..1da83b865 100644 --- a/helios-services/src/main/java/com/spotify/helios/master/MasterMain.java +++ b/helios-services/src/main/java/com/spotify/helios/master/MasterMain.java @@ -17,8 +17,6 @@ package com.spotify.helios.master; -import com.google.common.annotations.VisibleForTesting; - import com.spotify.helios.common.LoggingConfig; import com.spotify.helios.servicescommon.ServiceMain; import com.spotify.helios.servicescommon.coordination.CuratorClientFactory; @@ -29,8 +27,6 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import java.util.Map; - /** * Instantiates and runs the helios master. We do our own bootstrapping instead of using * {@link io.dropwizard.setup.Bootstrap} because we want more control over logging etc. @@ -41,51 +37,34 @@ public class MasterMain extends ServiceMain { private final MasterConfig masterConfig; private final CuratorClientFactory curatorClientFactory; - private final Map environmentVariables; private MasterService service; public MasterMain(final String[] args) throws ArgumentParserException { - this(new CuratorClientFactoryImpl(), new MasterParser(args), System.getenv()); - } - - /** Allows mock environment variables to be passed in for testing purposes. */ - @VisibleForTesting - public MasterMain(Map environmentVariables, final String[] args) - throws ArgumentParserException { - - this(new CuratorClientFactoryImpl(), new MasterParser(args), environmentVariables); + this(new CuratorClientFactoryImpl(), new MasterParser(args)); } public MasterMain(final CuratorClientFactory curatorClientFactory, final String[] args) throws ArgumentParserException { - this(curatorClientFactory, new MasterParser(args), System.getenv()); + this(curatorClientFactory, new MasterParser(args)); } public MasterMain(final CuratorClientFactory curatorClientFactory, - final MasterParser parser, - final Map environmentVariables) { - this(curatorClientFactory, - parser.getMasterConfig(), - parser.getLoggingConfig(), - environmentVariables); + final MasterParser parser) { + this(curatorClientFactory, parser.getMasterConfig(), parser.getLoggingConfig()); } public MasterMain(final CuratorClientFactory curatorClientFactory, final MasterConfig masterConfig, - final LoggingConfig loggingConfig, - final Map environmentVariables) { + final LoggingConfig loggingConfig) { super(loggingConfig, masterConfig.getSentryDsn()); this.masterConfig = masterConfig; this.curatorClientFactory = curatorClientFactory; - this.environmentVariables = environmentVariables; } @Override protected void startUp() throws Exception { - service = new MasterService(masterConfig, - createEnvironment("helios-master"), - curatorClientFactory, - environmentVariables); + service = new MasterService(masterConfig, createEnvironment("helios-master"), + curatorClientFactory); service.startAsync().awaitRunning(); } diff --git a/helios-services/src/main/java/com/spotify/helios/master/MasterService.java b/helios-services/src/main/java/com/spotify/helios/master/MasterService.java index 82f9b00ee..29517df01 100644 --- a/helios-services/src/main/java/com/spotify/helios/master/MasterService.java +++ b/helios-services/src/main/java/com/spotify/helios/master/MasterService.java @@ -83,7 +83,6 @@ import java.nio.file.Files; import java.nio.file.Path; import java.util.EnumSet; -import java.util.Map; import java.util.concurrent.TimeUnit; import javax.servlet.DispatcherType; @@ -117,7 +116,6 @@ public class MasterService extends AbstractIdleService { private final ExpiredJobReaper expiredJobReaper; private final CuratorClientFactory curatorClientFactory; private final RollingUpdateService rollingUpdateService; - private final Map environmentVariables; private ZooKeeperRegistrarService zkRegistrar; @@ -129,14 +127,11 @@ public class MasterService extends AbstractIdleService { * @param curatorClientFactory The zookeeper curator factory. * @throws ConfigurationException If there is a problem with the DropWizard configuration. */ - public MasterService(final MasterConfig config, - final Environment environment, - final CuratorClientFactory curatorClientFactory, - final Map environmentVariables) + public MasterService(final MasterConfig config, final Environment environment, + final CuratorClientFactory curatorClientFactory) throws ConfigurationException, IOException, InterruptedException { this.config = config; this.curatorClientFactory = curatorClientFactory; - this.environmentVariables = environmentVariables; // Configure metrics // TODO (dano): do something with the riemann facade @@ -261,14 +256,12 @@ private void setupAuthentication(final Environment environment, final MasterConf authPlugin.getClass().getName(), authConfig.getEnabledScheme()); - final ServerAuthentication authentication = - authPlugin.serverAuthentication(this.environmentVariables); + final ServerAuthentication authentication = authPlugin.serverAuthentication(); + final Authenticator authenticator = authPlugin.serverAuthentication().authenticator(); + final Predicate isRequired = authenticationRequired(authConfig); - final Authenticator authenticator = authentication.authenticator(); - - final AuthenticationRequestFilter filter = new AuthenticationRequestFilter(authenticator, - authPlugin.schemeName(), - authenticationRequired(authentication, authConfig)); + final AuthenticationRequestFilter filter = + new AuthenticationRequestFilter(authenticator, authPlugin.schemeName(), isRequired); // setting up filters in Jersey 1.x is convoluted: environment.jersey().getResourceConfig().getContainerRequestFilters().add(filter); @@ -277,31 +270,11 @@ private void setupAuthentication(final Environment environment, final MasterConf authentication.registerAdditionalJerseyComponents(environment.jersey()); } - private Predicate authenticationRequired( - final ServerAuthentication authentication, final ServerAuthenticationConfig config) { - - // evaluate if the request is for the actual authentication endpoint - Predicate pathRequiresAuth = new Predicate() { - @Override - public boolean apply(final HttpRequestContext input) { - final String requestPath = input.getPath(); - return !authentication.unauthenticatedPaths().contains(requestPath); - } - }; - - // if authentication is enabled for all versions then we just need to check that the request - // is not for an unauthenticated path. - // Otherwise we also check that the clientversion header is >= the minimum version that needs - // authentication - + private Predicate authenticationRequired(ServerAuthenticationConfig config) { if (config.isEnabledForAllVersions()) { - return pathRequiresAuth; + return Predicates.alwaysTrue(); } - return Predicates.and(pathRequiresAuth, clientVersionRequiresAuth(config)); - } - private Predicate clientVersionRequiresAuth( - final ServerAuthenticationConfig config) { final PomVersion minVersion = PomVersion.parse(config.getMinimumRequiredVersion()); return new Predicate() { diff --git a/helios-system-tests/src/main/java/com/spotify/helios/system/MasterAuthenticationTest.java b/helios-system-tests/src/main/java/com/spotify/helios/system/MasterAuthenticationTest.java index 4fc37b484..5076ec6d3 100644 --- a/helios-system-tests/src/main/java/com/spotify/helios/system/MasterAuthenticationTest.java +++ b/helios-system-tests/src/main/java/com/spotify/helios/system/MasterAuthenticationTest.java @@ -19,14 +19,10 @@ import com.google.auto.service.AutoService; import com.google.common.base.Optional; -import com.google.common.collect.ImmutableMap; -import com.google.common.io.BaseEncoding; -import com.spotify.docker.client.shaded.com.fasterxml.jackson.databind.ObjectMapper; import com.spotify.helios.auth.AuthenticationPlugin; import com.spotify.helios.auth.Authenticator; import com.spotify.helios.auth.HeliosUser; -import com.spotify.helios.auth.SimpleServerAuthentication; import com.sun.jersey.api.core.HttpRequestContext; import org.apache.http.Header; @@ -35,54 +31,39 @@ import org.apache.http.client.methods.HttpGet; import org.junit.Test; -import java.io.File; -import java.io.IOException; -import java.util.Map; - import javax.ws.rs.core.HttpHeaders; import io.dropwizard.auth.AuthenticationException; +import io.dropwizard.jersey.setup.JerseyEnvironment; import static org.hamcrest.Matchers.arrayWithSize; import static org.hamcrest.Matchers.is; -import static org.hamcrest.Matchers.not; import static org.junit.Assert.assertThat; /** Tests of authentication within Helios masters. */ public class MasterAuthenticationTest extends SystemTestBase { - /** - * Simple test that ensures that a HTTP request to /masters returns 401 Unauthorized when the - * request has no Authorization headers, and that a second request containing the expected - * Authorization header succeeds. - */ @Test public void authenticationEnabled() throws Exception { // use a new authentication scheme since BasicAuth requires setting up some environment // variables, which isn't simple to pass through startDefaultMaster startDefaultMaster("--auth-scheme", "fixed-password"); - verifyNormalRequestIsUnauthorized("/masters", "fixed-password"); - - final HttpGet authorizedGet = new HttpGet(masterEndpoint() + "/masters"); - authorizedGet.addHeader(HttpHeaders.AUTHORIZATION, "secret123"); - - try (CloseableHttpResponse response = httpClient.execute(authorizedGet)) { - assertThat(response.getStatusLine().getStatusCode(), is(HttpStatus.SC_OK)); - } - } - - private void verifyNormalRequestIsUnauthorized(String path, String expectedScheme) - throws IOException { - - final String uri = masterEndpoint() + path; + final String uri = masterEndpoint() + "/masters"; // expect an unauthenticated GET to return 401 Unauthorized try (CloseableHttpResponse response = httpClient.execute(new HttpGet(uri))) { assertThat(response.getStatusLine().getStatusCode(), is(HttpStatus.SC_UNAUTHORIZED)); final Header[] headers = response.getHeaders(HttpHeaders.WWW_AUTHENTICATE); assertThat(headers, arrayWithSize(1)); - assertThat(headers[0].getValue(), is(expectedScheme)); + assertThat(headers[0].getValue(), is("fixed-password")); + } + + final HttpGet authorizedGet = new HttpGet(uri); + authorizedGet.addHeader(HttpHeaders.AUTHORIZATION, "secret123"); + + try (CloseableHttpResponse response = httpClient.execute(authorizedGet)) { + assertThat(response.getStatusLine().getStatusCode(), is(HttpStatus.SC_OK)); } } @@ -95,8 +76,8 @@ public String schemeName() { } @Override - public ServerAuthentication serverAuthentication(Map environment) { - return new SimpleServerAuthentication() { + public ServerAuthentication serverAuthentication() { + return new ServerAuthentication() { @Override public Authenticator authenticator() { return new Authenticator() { @@ -111,10 +92,15 @@ public Optional authenticate(final String credentials) if ("secret123".equals(credentials)) { return Optional.of(new HeliosUser("the-user")); } + ; return Optional.absent(); } }; } + + @Override + public void registerAdditionalJerseyComponents(final JerseyEnvironment env) { + } }; } @@ -122,60 +108,5 @@ public Optional authenticate(final String credentials) public ClientAuthentication clientAuthentication() { return null; } - - } - - @Test - public void basicAuthEnabled() throws Exception { - final Map users = ImmutableMap.of("user1", "password2"); - - final File tempFile = File.createTempFile("users", "json"); - tempFile.deleteOnExit(); - - final ObjectMapper objectMapper = new ObjectMapper(); - objectMapper.writeValue(tempFile, users); - - final Map env = ImmutableMap.of( - "AUTH_BASIC_USERDB", tempFile.getAbsolutePath() - ); - - startDefaultMaster(env, "--auth-scheme", "http-basic"); - - // sanity check that the auth plugin is loaded and working - verifyNormalRequestIsUnauthorized("/masters", "http-basic"); - - // and now with a token - final HttpGet request = new HttpGet(masterEndpoint() + "/masters"); - final String encoded = BaseEncoding.base64().encode("user1:password2".getBytes()); - request.addHeader(HttpHeaders.AUTHORIZATION, "Basic " + encoded); - - try (CloseableHttpResponse response = httpClient.execute(request)) { - assertThat(response.getStatusLine().getStatusCode(), is(HttpStatus.SC_OK)); - } - } - - /** - * Make sure that when crtauth is enabled, that the /_auth endpoint it registers does not itself - * require an Authorization header, otherwise no one would ever be able to obtain a token in the - * first place. - */ - @Test - public void crtAuthEnabled_AuthEndpointDoesNotRequireAuthentication() throws Exception { - final Map env = ImmutableMap.of( - "CRTAUTH_SECRET", "sekret", - "CRTAUTH_SERVERNAME", "foo", - "CRTAUTH_LDAP_URL", "foo", - "CRTAUTH_LDAP_SEARCH_PATH", "foo"); - - startDefaultMaster(env, "--auth-scheme", "crtauth"); - - // sanity check that the auth plugin is loaded and working - verifyNormalRequestIsUnauthorized("/masters", "crtauth"); - - // the /_auth endpoint added by crtauth should not require authentication itself - final HttpGet request = new HttpGet(masterEndpoint() + "/_auth"); - try (CloseableHttpResponse response = httpClient.execute(request)) { - assertThat(response.getStatusLine().getStatusCode(), is(not(HttpStatus.SC_UNAUTHORIZED))); - } } } diff --git a/helios-system-tests/src/main/java/com/spotify/helios/system/SystemTestBase.java b/helios-system-tests/src/main/java/com/spotify/helios/system/SystemTestBase.java index ab548f3c3..321bc18ab 100644 --- a/helios-system-tests/src/main/java/com/spotify/helios/system/SystemTestBase.java +++ b/helios-system-tests/src/main/java/com/spotify/helios/system/SystemTestBase.java @@ -569,27 +569,14 @@ protected MasterMain startDefaultMaster(String... args) throws Exception { return startDefaultMaster(0, args); } - protected MasterMain startDefaultMaster(Map environmentVariables, String... args) - throws Exception { - return startDefaultMaster(0, environmentVariables, args); - } - protected MasterMain startDefaultMaster(final int offset, String... args) throws Exception { - return startDefaultMaster(offset, ImmutableMap.of(), args); - } - - protected MasterMain startDefaultMaster(final int offset, - final Map environmentVariables, - final String... args) throws Exception { final List argsList = setupDefaultMaster(offset, args); if (argsList == null) { return null; } - final MasterMain master = - startMaster(environmentVariables, argsList.toArray(new String[argsList.size()])); - + final MasterMain master = startMaster(argsList.toArray(new String[argsList.size()])); waitForMasterToBeFullyUp(); return master; @@ -671,9 +658,8 @@ protected AgentMain startDefaultAgent(final String host, final String... args) return startAgent(argsList.toArray(new String[argsList.size()])); } - protected MasterMain startMaster(final Map environmentVariables, - final String... args) throws Exception { - final MasterMain main = new MasterMain(environmentVariables, args); + protected MasterMain startMaster(final String... args) throws Exception { + final MasterMain main = new MasterMain(args); main.startAsync().awaitRunning(); services.add(main); return main;