Polishing.

Fetch SecretId if no secretId is configured but an initial token is provided instead of relying on a configured role name. Use configured AppRole mount path instead of static literal. Reorder methods, add since and author tags. Reduce tests to AppRoleAuthenticationOptions code. Add further test cases. Add integration tests. Formatting, fix typos.

Original pull request: gh-133.
Related ticket: gh-132.

spring-vault-core/src/main/java/org/springframework/vault/authentication/AppRoleAuthentication.java

@@ -17,8 +17,10 @@ package org.springframework.vault.authentication;
 
 import java.util.HashMap;
 import java.util.Map;
+
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
+
 import org.springframework.http.HttpEntity;
 import org.springframework.http.HttpHeaders;
 import org.springframework.http.HttpMethod;
@@ -42,6 +44,7 @@ import org.springframework.web.client.RestOperations;
  * {@link AppRoleAuthenticationOptions#getSecretId()}.
  *
  * @author Mark Paluch
+ * @author Vincent Le Nair
  * @see AppRoleAuthenticationOptions
  * @see RestOperations
  * @see <a href="https://www.vaultproject.io/docs/auth/approle.html">Auth Backend:
@@ -91,7 +94,8 @@ public class AppRoleAuthentication implements ClientAuthentication,
 				.login("auth/{mount}/login", options.getPath());
 	}
 
-	@Override public VaultToken login() {
+	@Override
+	public VaultToken login() {
 		return createTokenUsingAppRole();
 	}
 
@@ -108,9 +112,8 @@ public class AppRoleAuthentication implements ClientAuthentication,
 		Map<String, String> login = getAppRoleLogin(roleId, secretId);
 
 		try {
-			VaultResponse response = restOperations
-				.postForObject("auth/{mount}/login", login, VaultResponse.class,
-					options.getPath());
+			VaultResponse response = restOperations.postForObject("auth/{mount}/login",
+					login, VaultResponse.class, options.getPath());
 
 			Assert.state(response != null && response.getAuth() != null,
 					"Auth field must not be null");
@@ -121,55 +124,59 @@ public class AppRoleAuthentication implements ClientAuthentication,
 		}
 		catch (HttpStatusCodeException e) {
 			throw new VaultException(String.format("Cannot login using AppRole: %s",
-				VaultResponses.getError(e.getResponseBodyAsString())));
+					VaultResponses.getError(e.getResponseBodyAsString())));
 		}
 	}
 
 	private String getRoleId() {
+
 		String roleId = options.getRoleId();
-		if (StringUtils.isEmpty(roleId) && !StringUtils.isEmpty(options.getAppRole())) {
+
+		if (StringUtils.isEmpty(roleId)) {
+
 			try {
-				ResponseEntity<VaultResponse> response = restOperations
-					.exchange("auth/approle/role/{role}/role-id", HttpMethod.GET,
+				ResponseEntity<VaultResponse> response = restOperations.exchange(
+						"auth/{mount}/role/{role}/role-id", HttpMethod.GET,
 						createHttpEntityWithToken(), VaultResponse.class,
-						options.getAppRole());
+						options.getPath(), options.getAppRole());
 				roleId = (String) response.getBody().getData().get("role_id");
 			}
 			catch (HttpStatusCodeException e) {
-				throw new VaultException(String
-					.format("Cannot get Role id using AppRole: %s",
+				throw new VaultException(String.format(
+						"Cannot get Role id using AppRole: %s",
 						VaultResponses.getError(e.getResponseBodyAsString())));
 			}
 		}
-		return roleId;
 
+		return roleId;
 	}
 
 	private String getSecretId() {
+
 		String secretId = options.getSecretId();
-		if (StringUtils.isEmpty(secretId) && !StringUtils.isEmpty(options.getAppRole())) {
+
+		if (StringUtils.isEmpty(secretId) && options.getInitialToken() != null) {
 			try {
-				VaultResponse response = restOperations
-					.postForObject("auth/approle/role/{role}/secret-id",
+				VaultResponse response = restOperations.postForObject(
+						"auth/{mount}/role/{role}/secret-id",
 						createHttpEntityWithToken(), VaultResponse.class,
-						options.getAppRole());
+						options.getPath(), options.getAppRole());
 				secretId = (String) response.getData().get("secret_id");
 			}
 			catch (HttpStatusCodeException e) {
-				throw new VaultException(String
-					.format("Cannot get Secret id using AppRole: %s",
+				throw new VaultException(String.format(
+						"Cannot get Secret id using AppRole: %s",
 						VaultResponses.getError(e.getResponseBodyAsString())));
 			}
 		}
-		return secretId;
 
+		return secretId;
 	}
 
 	private HttpEntity createHttpEntityWithToken() {
+
 		HttpHeaders headers = new HttpHeaders();
-		if (options.getInitialToken() != null) {
-			headers.set("X-Vault-Token", options.getInitialToken());
-		}
+		headers.set("X-Vault-Token", options.getInitialToken().getToken());
 		return new HttpEntity<String>(null, headers);
 	}
 
@@ -179,6 +186,7 @@ public class AppRoleAuthentication implements ClientAuthentication,
 		Map<String, String> login = new HashMap<>();
 
 		login.put("role_id", roleId);
+
 		if (secretId != null) {
 			login.put("secret_id", secretId);
 		}

spring-vault-core/src/main/java/org/springframework/vault/authentication/AppRoleAuthenticationOptions.java

@@ -18,6 +18,7 @@ package org.springframework.vault.authentication;
 import org.springframework.lang.Nullable;
 import org.springframework.util.Assert;
 import org.springframework.util.StringUtils;
+import org.springframework.vault.support.VaultToken;
 
 /**
  * Authentication options for {@link AppRoleAuthentication}.
@@ -27,6 +28,7 @@ import org.springframework.util.StringUtils;
  * this class are immutable once constructed.
  *
  * @author Mark Paluch
+ * @author Vincent Le Nair
  * @see AppRoleAuthentication
  * @see #builder()
  */
@@ -35,13 +37,14 @@ public class AppRoleAuthenticationOptions {
 	public static final String DEFAULT_APPROLE_AUTHENTICATION_PATH = "approle";
 
 	/**
-	 * Path of the apprile authentication backend mount.
+	 * Path of the approle authentication backend mount.
 	 */
 	private final String path;
 
 	/**
 	 * The RoleId.
 	 */
+	@Nullable
 	private final String roleId;
 
 	/**
@@ -53,15 +56,18 @@ public class AppRoleAuthenticationOptions {
 	/**
 	 * Role name used to get roleId and secretID
 	 */
+	@Nullable
 	private final String appRole;
 
 	/**
-	 * Token associated to the roleName.
+	 * Token associated for pull mode (retrieval of secretId/roleId).
 	 */
-	private final String initialToken;
+	@Nullable
+	private final VaultToken initialToken;
 
-	private AppRoleAuthenticationOptions(String path, String roleId,
-			@Nullable String secretId, String appRole, String initialToken) {
+	private AppRoleAuthenticationOptions(String path, @Nullable String roleId,
+			@Nullable String secretId, @Nullable String appRole,
+			@Nullable VaultToken initialToken) {
 
 		this.path = path;
 		this.roleId = roleId;
@@ -87,6 +93,7 @@ public class AppRoleAuthenticationOptions {
 	/**
 	 * @return the RoleId.
 	 */
+	@Nullable
 	public String getRoleId() {
 		return roleId;
 	}
@@ -101,15 +108,19 @@ public class AppRoleAuthenticationOptions {
 
 	/**
 	 * @return the bound AppRole.
+	 * @since 1.1
 	 */
+	@Nullable
 	public String getAppRole() {
 		return appRole;
 	}
 
 	/**
-	 * @return the bound InitialToken.
+	 * @return the initial token for roleId/secretId retrieval in pull mode.
+	 * @since 1.1
 	 */
-	public String getInitialToken() {
+	@Nullable
+	public VaultToken getInitialToken() {
 		return initialToken;
 	}
 
@@ -120,16 +131,16 @@ public class AppRoleAuthenticationOptions {
 
 		private String path = DEFAULT_APPROLE_AUTHENTICATION_PATH;
 
-		private String appRole;
-
-		private String initialToken;
-
 		@Nullable
 		private String roleId;
 
 		@Nullable
 		private String secretId;
 
+		private String appRole;
+
+		private VaultToken initialToken;
+
 		AppRoleAuthenticationOptionsBuilder() {
 		}
 
@@ -148,34 +159,6 @@ public class AppRoleAuthenticationOptions {
 			return this;
 		}
 
-		/**
-		 * Configure a {@code appRole}.
-		 *
-		 * @param appRole must not be empty or {@literal null}.
-		 * @return {@code this} {@link AppRoleAuthenticationOptionsBuilder}.
-		 */
-		public AppRoleAuthenticationOptionsBuilder appRole(String appRole) {
-
-			Assert.hasText(appRole, "AppRole must not be empty");
-
-			this.appRole = appRole;
-			return this;
-		}
-
-		/**
-		 * Configure a {@code initialToken}.
-		 *
-		 * @param initialToken must not be empty or {@literal null}.
-		 * @return {@code this} {@link AppRoleAuthenticationOptionsBuilder}.
-		 */
-		public AppRoleAuthenticationOptionsBuilder initialToken(String initialToken) {
-
-			Assert.hasText(initialToken, "InitialToken must not be empty");
-
-			this.initialToken = initialToken;
-			return this;
-		}
-
 		/**
 		 * Configure the RoleId.
 		 *
@@ -204,10 +187,40 @@ public class AppRoleAuthenticationOptions {
 			return this;
 		}
 
+		/**
+		 * Configure a {@code appRole}.
+		 *
+		 * @param appRole must not be empty or {@literal null}.
+		 * @return {@code this} {@link AppRoleAuthenticationOptionsBuilder}.
+		 * @since 1.1
+		 */
+		public AppRoleAuthenticationOptionsBuilder appRole(String appRole) {
+
+			Assert.hasText(appRole, "AppRole must not be empty");
+
+			this.appRole = appRole;
+			return this;
+		}
+
+		/**
+		 * Configure a {@code initialToken}.
+		 *
+		 * @param initialToken must not be empty or {@literal null}.
+		 * @return {@code this} {@link AppRoleAuthenticationOptionsBuilder}.
+		 * @since 1.1
+		 */
+		public AppRoleAuthenticationOptionsBuilder initialToken(VaultToken initialToken) {
+
+			Assert.notNull(initialToken, "InitialToken must not be null");
+
+			this.initialToken = initialToken;
+			return this;
+		}
+
 		/**
 		 * Build a new {@link AppRoleAuthenticationOptions} instance. Requires
-		 * {@link #roleId(String)} for Push Mode or {@link #appRole(String)} and
-		 * {@link #initialToken(String)} for pull Mode to be configured.
+		 * {@link #roleId(String)} for push mode or {@link #appRole(String)} and
+		 * {@link #initialToken(VaultToken)} for pull mode to be configured.
 		 *
 		 * @return a new {@link AppRoleAuthenticationOptions}.
 		 */
@@ -215,19 +228,26 @@ public class AppRoleAuthenticationOptions {
 
 			Assert.hasText(path, "Path must not be empty");
 
-			//Role ID is required in order to use push mode (no appRole and initialToken)
-			if (StringUtils.isEmpty(appRole) && StringUtils.isEmpty(initialToken)) {
-				Assert.notNull(roleId, "RoleId must not be null");
+			// Role ID is required in order to use push mode (no appRole and initialToken)
+
+			if (StringUtils.isEmpty(roleId) && StringUtils.isEmpty(appRole)
+					&& initialToken == null) {
+				throw new IllegalArgumentException(
+						"Either roleId (push mode) or appRole/initialToken (pull mode) must be configured for AppRole authentication");
 			}
 
-			//AppRole and InitialToken are required in order to use pull mode (no roleId)
+			// AppRole and InitialToken are required in order to use pull mode (no roleId)
 			if (StringUtils.isEmpty(roleId)) {
-				Assert.notNull(appRole, "AppRole must not be null");
-				Assert.notNull(initialToken, "InitialToken must not be null");
+
+				Assert.notNull(appRole,
+						"AppRole authentication configured for pull mode. AppRole must not be null.");
+				Assert.notNull(
+						initialToken,
+						"AppRole authentication configured for pull mode. InitialToken must not be null (pull mode)");
 			}
 
 			return new AppRoleAuthenticationOptions(path, roleId, secretId, appRole,
-				initialToken);
+					initialToken);
 		}
 	}
 }

spring-vault-core/src/test/java/org/springframework/vault/authentication/AppRoleAuthenticationIntegrationTests.java

@@ -26,6 +26,7 @@ import org.springframework.vault.VaultException;
 import org.springframework.vault.core.VaultOperations;
 import org.springframework.vault.support.VaultResponse;
 import org.springframework.vault.util.IntegrationTestSupport;
+import org.springframework.vault.util.Settings;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.hamcrest.CoreMatchers.anyOf;
@@ -88,6 +89,29 @@ public class AppRoleAuthenticationIntegrationTests extends IntegrationTestSuppor
 		assertThat(authentication.login()).isNotNull();
 	}
 
+	@Test
+	public void shouldAuthenticateWithFullPullMode() {
+
+		AppRoleAuthenticationOptions options = AppRoleAuthenticationOptions.builder()
+				.appRole("with-secret-id").initialToken(Settings.token()).build();
+		AppRoleAuthentication authentication = new AppRoleAuthentication(options,
+				prepare().getRestTemplate());
+
+		assertThat(authentication.login()).isNotNull();
+	}
+
+	@Test
+	public void shouldAuthenticateWithPullMode() {
+
+		AppRoleAuthenticationOptions options = AppRoleAuthenticationOptions.builder()
+				.roleId(getRoleId("with-secret-id")).appRole("with-secret-id")
+				.initialToken(Settings.token()).build();
+		AppRoleAuthentication authentication = new AppRoleAuthentication(options,
+				prepare().getRestTemplate());
+
+		assertThat(authentication.login()).isNotNull();
+	}
+
 	@Test
 	public void shouldAuthenticatePullModeWithGeneratedSecretId() {
 

spring-vault-core/src/test/java/org/springframework/vault/authentication/AppRoleAuthenticationUnitTests.java

@@ -30,7 +30,10 @@ import org.springframework.vault.support.VaultToken;
 import org.springframework.web.client.RestTemplate;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.springframework.test.web.client.match.MockRestRequestMatchers.*;
+import static org.springframework.test.web.client.match.MockRestRequestMatchers.header;
+import static org.springframework.test.web.client.match.MockRestRequestMatchers.jsonPath;
+import static org.springframework.test.web.client.match.MockRestRequestMatchers.method;
+import static org.springframework.test.web.client.match.MockRestRequestMatchers.requestTo;
 import static org.springframework.test.web.client.response.MockRestResponseCreators.withServerError;
 import static org.springframework.test.web.client.response.MockRestResponseCreators.withSuccess;
 
@@ -38,6 +41,7 @@ import static org.springframework.test.web.client.response.MockRestResponseCreat
  * Unit tests for {@link AppRoleAuthentication}.
  *
  * @author Mark Paluch
+ * @author Vincent Le Nair
  */
 public class AppRoleAuthenticationUnitTests {
 
@@ -82,23 +86,21 @@ public class AppRoleAuthenticationUnitTests {
 	public void loginShouldPullRoleIdAndSecretId() throws Exception {
 
 		AppRoleAuthenticationOptions options = AppRoleAuthenticationOptions.builder()
-				.appRole("app_role")
-				.initialToken("initial_token")
-				.build();
+				.appRole("app_role").initialToken(VaultToken.of("initial_token")).build();
 
 		mockRest.expect(requestTo("/auth/approle/role/app_role/role-id"))
 				.andExpect(method(HttpMethod.GET))
 				.andExpect(header("X-Vault-token", "initial_token"))
-				.andRespond(withSuccess().contentType(MediaType.APPLICATION_JSON).body(
-						"{\"data\": {\"role_id\": \"hello\"}}"
-				));
+				.andRespond(
+						withSuccess().contentType(MediaType.APPLICATION_JSON).body(
+								"{\"data\": {\"role_id\": \"hello\"}}"));
 
 		mockRest.expect(requestTo("/auth/approle/role/app_role/secret-id"))
 				.andExpect(method(HttpMethod.POST))
 				.andExpect(header("X-Vault-token", "initial_token"))
-				.andRespond(withSuccess().contentType(MediaType.APPLICATION_JSON).body(
-						"{\"data\": {\"secret_id\": \"world\"}}"
-				));
+				.andRespond(
+						withSuccess().contentType(MediaType.APPLICATION_JSON).body(
+								"{\"data\": {\"secret_id\": \"world\"}}"));
 
 		mockRest.expect(requestTo("/auth/approle/login"))
 				.andExpect(method(HttpMethod.POST))
@@ -117,16 +119,18 @@ public class AppRoleAuthenticationUnitTests {
 	}
 
 	@Test(expected = IllegalArgumentException.class)
-	public void loginShouldFailIfPullModeButNoToken() throws Exception {
+	public void optionsShouldRequireTokenOrRoleIdIfNothingIsSet() {
+		AppRoleAuthenticationOptions.builder().build();
+	}
 
-		AppRoleAuthenticationOptions options = AppRoleAuthenticationOptions.builder()
-				.appRole("app_role")
-				.build();
-
-		AppRoleAuthentication sut = new AppRoleAuthentication(options, restTemplate);
-
-		sut.login();
+	@Test(expected = IllegalArgumentException.class)
+	public void optionsShouldRequireTokenOrRoleIdIfTokenIsSet() {
+		AppRoleAuthenticationOptions.builder().initialToken(VaultToken.of("foo")).build();
+	}
 
+	@Test(expected = IllegalArgumentException.class)
+	public void optionsShouldRequireTokenOrRoleIdIfAppRoleIdIsSet() {
+		AppRoleAuthenticationOptions.builder().appRole("app_role").build();
 	}
 
 	@Test

src/main/asciidoc/reference/authentication.adoc

@@ -196,7 +196,8 @@ authentication, like the deprecated (since Vault 0.6.1) <<vault.authentication.a
 AppRole authentication consists of two hard to guess (secret) tokens: RoleId and SecretId.
 
 Spring Vault supports AppRole authentication by providing either RoleId only
-or together with a provided SecretId.
+or together with a provided SecretId and fetching RoleId/SecretId from Vault
+(push and pull modes).
 
 ====
 [source,java]
@@ -222,7 +223,9 @@ class AppConfig extends AbstractVaultConfiguration {
 ----
 ====
 
-Spring Vault also support a full pull mode: if RoleId and SecretId are not provided, Spring Vault will retreive them usind AppRole and RoleToken
+Spring Vault also support full pull mode: If RoleId and SecretId are not provided,
+Spring Vault will retrieve them using the role name and an initial token. The
+initial token may be associated with a TTL and usage limit.
 
 ====
 [source,java]
@@ -237,7 +240,7 @@ class AppConfig extends AbstractVaultConfiguration {
 
         AppRoleAuthenticationOptions options = AppRoleAuthenticationOptions.builder()
                 .appRole("…")
-                .roleToken("…")
+                .initialToken(VaultToken.of("…"))
                 .build();
 
         return new AppRoleAuthentication(options, restOperations());