GH zalando#47:

No "CREDENTIALS_DIR" exception if it is not configured.
vadeg · Jun 11, 2017 · 651257a · 651257a
1 parent eecc0e6
commit 651257a
Show file tree

Hide file tree

Showing 4 changed files with 54 additions and 44 deletions.
diff --git a/src/main/java/org/zalando/stups/tokens/AccessTokensBuilder.java b/src/main/java/org/zalando/stups/tokens/AccessTokensBuilder.java
@@ -15,10 +15,11 @@
  */
 package org.zalando.stups.tokens;
 
-import static org.zalando.stups.tokens.EndsWithFilenameFilter.forSuffix;
-import static org.zalando.stups.tokens.FileSupplier.getCredentialsDir;
-import static org.zalando.stups.tokens.util.Objects.notNull;
+import org.zalando.stups.tokens.fs.FilesystemSecretRefresher;
+import org.zalando.stups.tokens.fs.FilesystemSecretsRefresherConfiguration;
+import org.zalando.stups.tokens.mcb.MCBConfig;
 
+import java.io.File;
 import java.net.URI;
 import java.util.Collections;
 import java.util.HashSet;
@@ -27,9 +28,8 @@
 import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.TimeUnit;
 
-import org.zalando.stups.tokens.fs.FilesystemSecretRefresher;
-import org.zalando.stups.tokens.fs.FilesystemSecretsRefresherConfiguration;
-import org.zalando.stups.tokens.mcb.MCBConfig;
+import static org.zalando.stups.tokens.EndsWithFilenameFilter.forSuffix;
+import static org.zalando.stups.tokens.util.Objects.notNull;
 
 /**
  * Use the <i>AccessTokensBuilder</i> obtained via
@@ -464,7 +464,7 @@ public AccessTokensBuilder metricsListener(MetricsListener metricsListener) {
         return this;
     }
 
-    public FilesystemSecretsRefresherConfiguration whenUsingFilesystemSecrets(){
+    public FilesystemSecretsRefresherConfiguration whenUsingFilesystemSecrets() {
         return this.filesystemSecretsRefresherConfiguration;
     }
 
@@ -557,8 +557,8 @@ public Set<AccessTokenConfiguration> getAccessTokenConfigurations() {
      *         for any of your configured <i>tokenIds</i>.
      */
     public AccessTokens start() {
-        if (accessTokenConfigurations.size() == 0) {
-            throw new IllegalArgumentException("no scopes defined");
+        if (accessTokenConfigurations.isEmpty()) {
+            throw new IllegalArgumentException("No tokens configured");
         }
 
         locked = true;
@@ -596,11 +596,13 @@ protected AbstractAccessTokenRefresher getAccessTokenRefresher() {
     }
 
     private boolean isFilesystemSecretsLayout() {
-        try {
-            return getCredentialsDir().list(forSuffix("-token-secret")).length > 0;
-        } catch (Exception e) {
-            return false;
-        }
+        return FileSupplier
+                .credentialsDir()
+                .map(dir -> {
+                    String[] files = new File(dir).list(forSuffix("-token-secret"));
+                    return files != null && files.length > 0;
+                })
+                .orElse(Boolean.FALSE);
     }
 
     @Override

diff --git a/src/main/java/org/zalando/stups/tokens/FileSupplier.java b/src/main/java/org/zalando/stups/tokens/FileSupplier.java
@@ -15,10 +15,11 @@
  */
 package org.zalando.stups.tokens;
 
-import java.io.File;
-
 import org.apache.http.util.Args;
 
+import java.io.File;
+import java.util.Optional;
+
 /**
  * Replacement to make it compatible with Java7.
  *
@@ -48,17 +49,16 @@ public synchronized File get() {
     }
 
     public static File getCredentialsDir() {
-        String dir = System.getenv("CREDENTIALS_DIR");
-        if (dir == null) {
-
-            // this for testing
-            dir = System.getProperty("CREDENTIALS_DIR");
-            if (dir == null) {
-                throw new IllegalStateException("environment variable CREDENTIALS_DIR not set");
-            }
-        }
+        return credentialsDir()
+                .map(File::new)
+                .orElseThrow(() -> new IllegalStateException(
+                        "environment variable or application property CREDENTIALS_DIR not set"
+                ));
+    }
 
-        return new File(dir);
+    public static Optional<String> credentialsDir() {
+        Optional<String> optionalDir = Optional.ofNullable(System.getenv("CREDENTIALS_DIR"));
+        return optionalDir.isPresent() ? optionalDir : Optional.ofNullable(System.getProperty("CREDENTIALS_DIR"));
     }
 
 }
diff --git a/src/test/java/org/zalando/stups/tokens/AccessTokenBuilderTest.java b/src/test/java/org/zalando/stups/tokens/AccessTokenBuilderTest.java
@@ -15,13 +15,6 @@
  */
 package org.zalando.stups.tokens;
 
-import static org.assertj.core.api.Assertions.assertThat;
-
-import java.io.File;
-import java.io.IOException;
-import java.net.URI;
-import java.util.concurrent.ScheduledExecutorService;
-
 import org.assertj.core.api.Assertions;
 import org.junit.After;
 import org.junit.Before;
@@ -31,6 +24,14 @@
 import org.junit.rules.TemporaryFolder;
 import org.mockito.Mockito;
 import org.mockito.internal.util.io.IOUtil;
+import org.zalando.stups.tokens.fs.FilesystemSecretRefresher;
+
+import java.io.File;
+import java.io.IOException;
+import java.net.URI;
+import java.util.concurrent.ScheduledExecutorService;
+
+import static org.assertj.core.api.Assertions.assertThat;
 
 /**
  * 
@@ -119,7 +120,7 @@ public void httpConfigShouldBeNotNull() {
 	}
 
 	@Test(expected = IllegalArgumentException.class)
-	public void accessTokenConfigurationWithouScopesShouldFail() {
+	public void accessTokenConfigurationWithoutScopesShouldFail() {
 		Tokens.createAccessTokensWithUri(uri).start();
 	}
 
@@ -167,4 +168,14 @@ public void usinEnvCreatesBuilder() {
         Assertions.assertThat(builder).isNotNull();
     }
 
+	@Test
+	public void checkCorrectRefresherUsed() throws Exception {
+		environmentVariables.set("OAUTH2_ACCESS_TOKEN_URL", "https://somwhere.test/tokens");
+		AccessTokensBuilder builder = Tokens.createAccessTokens();
+		AbstractAccessTokenRefresher refresher = builder.getAccessTokenRefresher();
+		assertThat(refresher instanceof FilesystemSecretRefresher);
+
+		System.getProperties().remove(CREDENTIALS_DIR);
+		assertThat(refresher instanceof AccessTokenRefresher);
+	}
 }
diff --git a/src/test/java/org/zalando/stups/tokens/FileSupplierTest.java b/src/test/java/org/zalando/stups/tokens/FileSupplierTest.java
@@ -15,14 +15,14 @@
  */
 package org.zalando.stups.tokens;
 
-import java.io.File;
-
 import org.assertj.core.api.Assertions;
 import org.junit.Assert;
 import org.junit.Test;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import java.io.File;
+
 /**
  * @author  jbellmann
  */
@@ -70,19 +70,16 @@ public void testFilenameParameterIsNotNull() {
         supplier.get();
     }
 
-    @Test
+    @Test(expected = IllegalStateException.class)
     public void testNoCredentialsDirSet() {
         if (System.getenv("CREDENTIALS_DIR") != null) {
             LOG.warn("ENV 'CREDENTIALS_DIR' was set so we skip");
             return;
         }
-        try {
-            FileSupplier supplier = new FileSupplier("notExistent");
-            Assert.assertNotNull(supplier);
-            supplier.get();
-            Assertions.fail("expect an exception, when 'CREDENTIALS_DIR' not set in environment");
-        } catch (IllegalStateException t) {
-        }
+        FileSupplier supplier = new FileSupplier("notExistent");
+        Assert.assertNotNull(supplier);
+        supplier.get();
+        Assertions.fail("expect an exception, when 'CREDENTIALS_DIR' not set in environment");
     }
 
 }