Polish Core Authentication Builders

Issue spring-projectsgh-17861

Author: jzheaux

core/src/main/java/org/springframework/security/authentication/AbstractAuthenticationToken.java

@@ -197,14 +197,22 @@ public String toString() {
 		return sb.toString();
 	}
 
+	/**
+	 * A common abstract implementation of {@link Authentication.Builder}. It implements
+	 * the builder methods that correspond to the {@link Authentication} methods that
+	 * {@link AbstractAuthenticationToken} implements
+	 *
+	 * @param <B>
+	 * @since 7.0
+	 */
 	protected abstract static class AbstractAuthenticationBuilder<B extends AbstractAuthenticationBuilder<B>>
 			implements Authentication.Builder<B> {
 
-		protected boolean authenticated;
+		private boolean authenticated;
 
-		protected @Nullable Object details;
+		private @Nullable Object details;
 
-		protected final Collection<GrantedAuthority> authorities;
+		private final Collection<GrantedAuthority> authorities;
 
 		protected AbstractAuthenticationBuilder(AbstractAuthenticationToken token) {
 			this.authorities = new LinkedHashSet<>(token.getAuthorities());

core/src/main/java/org/springframework/security/authentication/RememberMeAuthenticationToken.java

@@ -20,7 +20,6 @@
 
 import org.jspecify.annotations.Nullable;
 
-import org.springframework.security.core.Authentication;
 import org.springframework.security.core.GrantedAuthority;
 import org.springframework.util.Assert;
 
@@ -99,8 +98,8 @@ public Object getPrincipal() {
 	}
 
 	@Override
-	public Builder toBuilder() {
-		return new Builder(this);
+	public Builder<?> toBuilder() {
+		return new Builder<>(this);
 	}
 
 	@Override
@@ -122,7 +121,7 @@ public int hashCode() {
 	}
 
 	/**
-	 * A builder preserving the concrete {@link Authentication} type
+	 * A builder of {@link RememberMeAuthenticationToken} instances
 	 *
 	 * @since 7.0
 	 */
@@ -145,8 +144,13 @@ public B principal(@Nullable Object principal) {
 			return (B) this;
 		}
 
-		public B keyHash(int keyHash) {
-			this.keyHash = keyHash;
+		/**
+		 * Use this key
+		 * @param key the key to use
+		 * @return the {@link Builder} for further configurations
+		 */
+		public B key(String key) {
+			this.keyHash = key.hashCode();
 			return (B) this;
 		}
 

core/src/main/java/org/springframework/security/authentication/TestingAuthenticationToken.java

@@ -21,7 +21,6 @@
 
 import org.jspecify.annotations.Nullable;
 
-import org.springframework.security.core.Authentication;
 import org.springframework.security.core.GrantedAuthority;
 import org.springframework.security.core.authority.AuthorityUtils;
 import org.springframework.util.Assert;
@@ -87,7 +86,7 @@ public Builder<?> toBuilder() {
 	}
 
 	/**
-	 * A builder preserving the concrete {@link Authentication} type
+	 * A builder of {@link TestingAuthenticationToken} instances
 	 *
 	 * @since 7.0
 	 */

core/src/main/java/org/springframework/security/authentication/UsernamePasswordAuthenticationToken.java

@@ -20,7 +20,6 @@
 
 import org.jspecify.annotations.Nullable;
 
-import org.springframework.security.core.Authentication;
 import org.springframework.security.core.GrantedAuthority;
 import org.springframework.util.Assert;
 
@@ -137,15 +136,15 @@ public Builder<?> toBuilder() {
 	}
 
 	/**
-	 * A builder preserving the concrete {@link Authentication} type
+	 * A builder of {@link UsernamePasswordAuthenticationToken} instances
 	 *
 	 * @since 7.0
 	 */
 	public static class Builder<B extends Builder<B>> extends AbstractAuthenticationBuilder<B> {
 
-		protected @Nullable Object principal;
+		private @Nullable Object principal;
 
-		protected @Nullable Object credentials;
+		private @Nullable Object credentials;
 
 		protected Builder(UsernamePasswordAuthenticationToken token) {
 			super(token);

core/src/main/java/org/springframework/security/authentication/jaas/JaasAuthenticationToken.java

@@ -23,9 +23,7 @@
 import org.jspecify.annotations.Nullable;
 
 import org.springframework.security.authentication.UsernamePasswordAuthenticationToken;
-import org.springframework.security.core.Authentication;
 import org.springframework.security.core.GrantedAuthority;
-import org.springframework.util.Assert;
 
 /**
  * UsernamePasswordAuthenticationToken extension to carry the Jaas LoginContext that the
@@ -65,7 +63,7 @@ public Builder<?> toBuilder() {
 	}
 
 	/**
-	 * A builder preserving the concrete {@link Authentication} type
+	 * A builder of {@link JaasAuthenticationToken} instances
 	 *
 	 * @since 7.0
 	 */
@@ -81,7 +79,7 @@ protected Builder(JaasAuthenticationToken token) {
 		/**
 		 * Use this {@link LoginContext}
 		 * @param loginContext the {@link LoginContext} to use
-		 * @return the {@link Builder} for further configuration
+		 * @return the {@link Builder} for further configurations
 		 */
 		public B loginContext(LoginContext loginContext) {
 			this.loginContext = loginContext;
@@ -90,7 +88,6 @@ public B loginContext(LoginContext loginContext) {
 
 		@Override
 		public JaasAuthenticationToken build() {
-			Assert.notNull(this.principal, "principal cannot be null");
 			return new JaasAuthenticationToken(this);
 		}
 

core/src/main/java/org/springframework/security/authentication/ott/OneTimeTokenAuthentication.java

@@ -65,7 +65,9 @@ public Builder<?> toBuilder() {
 	}
 
 	/**
-	 * A builder for constructing a {@link OneTimeTokenAuthentication} instance
+	 * A builder of {@link OneTimeTokenAuthentication} instances
+	 *
+	 * @since 7.0
 	 */
 	public static class Builder<B extends Builder<B>> extends AbstractAuthenticationBuilder<B> {
 
@@ -77,7 +79,7 @@ protected Builder(OneTimeTokenAuthentication token) {
 		}
 
 		/**
-		 * Use this principal
+		 * Use this principal.
 		 * @return the {@link Builder} for further configuration
 		 */
 		@Override

core/src/main/java/org/springframework/security/core/Authentication.java

@@ -138,7 +138,19 @@ public interface Authentication extends Principal, Serializable {
 	void setAuthenticated(boolean isAuthenticated) throws IllegalArgumentException;
 
 	/**
-	 * Return an {@link Builder} based on this instance
+	 * Return an {@link Builder} based on this instance. By default, returns a builder
+	 * that builds a {@link SimpleAuthentication}.
+	 * <p>
+	 * Although a {@code default} method, all {@link Authentication} implementations
+	 * should implement this. The reason is to ensure that the {@link Authentication} type
+	 * is preserved when {@link Builder#build} is invoked. This is especially important in
+	 * the event that your authentication implementation contains custom fields.
+	 * </p>
+	 * <p>
+	 * This isn't strictly necessary since it is recommended that applications code to the
+	 * {@link Authentication} interface and that custom information is often contained in
+	 * the {@link Authentication#getPrincipal} value.
+	 * </p>
 	 * @return an {@link Builder} for building a new {@link Authentication} based on this
 	 * instance
 	 * @since 7.0
@@ -155,17 +167,72 @@ default Builder<?> toBuilder() {
 	 */
 	interface Builder<B extends Builder<B>> {
 
+		/**
+		 * Mutate the authorities with this {@link Consumer}.
+		 * <p>
+		 * Note that since a non-empty set of authorities implies an
+		 * {@link Authentication} is authenticated, this method also marks the
+		 * authentication as {@link #authenticated} by default.
+		 * </p>
+		 * @param authorities a consumer that receives the full set of authorities
+		 * @return the {@link Builder} for additional configuration
+		 * @see Authentication#getAuthorities
+		 */
 		B authorities(Consumer<Collection<GrantedAuthority>> authorities);
 
+		/**
+		 * Use this credential.
+		 * <p>
+		 * Note that since some credentials are insecure to store, this method is
+		 * implemented as unsupported by default. Only implement or use this method if you
+		 * support secure storage of the credential or if your implementation also
+		 * implements {@link CredentialsContainer} and the credentials are thereby erased.
+		 * </p>
+		 * @param credentials the credentials to use
+		 * @return the {@link Builder} for additional configuration
+		 * @see Authentication#getCredentials
+		 */
 		default B credentials(@Nullable Object credentials) {
 			throw new UnsupportedOperationException(
 					String.format("%s does not store credentials", this.getClass().getSimpleName()));
 		}
 
+		/**
+		 * Use this details object.
+		 * <p>
+		 * Implementations may choose to use these {@code details} in combination with any
+		 * principal from the pre-existing {@link Authentication} instance.
+		 * </p>
+		 * @param details the details to use
+		 * @return the {@link Builder} for additional configuration
+		 * @see Authentication#getDetails
+		 */
 		B details(@Nullable Object details);
 
+		/**
+		 * Use this principal.
+		 * <p>
+		 * Note that in many cases, the principal is strongly-typed. Implementations may
+		 * choose to do a type check and are not necessarily expected to allow any object
+		 * as a principal.
+		 * </p>
+		 * <p>
+		 * Implementations may choose to use this {@code principal} in combination with
+		 * any principal from the pre-existing {@link Authentication} instance.
+		 * </p>
+		 * @param principal the principal to use
+		 * @return the {@link Builder} for additional configuration
+		 * @see Authentication#getPrincipal
+		 */
 		B principal(@Nullable Object principal);
 
+		/**
+		 * Mark this authentication as authenticated or not
+		 * @param authenticated whether this is an authenticated {@link Authentication}
+		 * instance
+		 * @return the {@link Builder} for additional configuration
+		 * @see Authentication#isAuthenticated
+		 */
 		B authenticated(boolean authenticated);
 
 		/**

core/src/test/java/org/springframework/security/authentication/AbstractAuthenticationBuilderTests.java

@@ -18,10 +18,8 @@
 
 import java.util.Set;
 
-import org.jspecify.annotations.Nullable;
 import org.junit.jupiter.api.Test;
 
-import org.springframework.security.authentication.AbstractAuthenticationToken.AbstractAuthenticationBuilder;
 import org.springframework.security.core.Authentication;
 import org.springframework.security.core.authority.AuthorityUtils;
 
@@ -40,20 +38,15 @@ void applyWhenAuthoritiesThenAdds() {
 	}
 
 	private static final class TestAbstractAuthenticationBuilder
-			extends AbstractAuthenticationBuilder<TestAbstractAuthenticationBuilder> {
+			extends TestingAuthenticationToken.Builder<TestAbstractAuthenticationBuilder> {
 
 		private TestAbstractAuthenticationBuilder(TestingAuthenticationToken token) {
 			super(token);
 		}
 
-		@Override
-		public TestAbstractAuthenticationBuilder principal(@Nullable Object principal) {
-			return this;
-		}
-
 		@Override
 		public TestingAuthenticationToken build() {
-			return new TestingAuthenticationToken("user", "password", this.authorities);
+			return new TestingAuthenticationToken(this);
 		}
 
 	}

core/src/test/java/org/springframework/security/authentication/rememberme/RememberMeAuthenticationTokenTests.java

@@ -18,13 +18,15 @@
 
 import java.util.Arrays;
 import java.util.List;
+import java.util.Set;
 
 import org.junit.jupiter.api.Test;
 
 import org.springframework.security.authentication.RememberMeAuthenticationToken;
 import org.springframework.security.authentication.UsernamePasswordAuthenticationToken;
 import org.springframework.security.core.GrantedAuthority;
 import org.springframework.security.core.authority.AuthorityUtils;
+import org.springframework.security.core.userdetails.PasswordEncodedUser;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
@@ -96,4 +98,21 @@ public void testSetAuthenticatedIgnored() {
 		assertThat(!token.isAuthenticated()).isTrue();
 	}
 
+	@Test
+	public void toBuilderWhenApplyThenCopies() {
+		RememberMeAuthenticationToken factorOne = new RememberMeAuthenticationToken("key", PasswordEncodedUser.user(),
+				AuthorityUtils.createAuthorityList("FACTOR_ONE"));
+		RememberMeAuthenticationToken factorTwo = new RememberMeAuthenticationToken("yek", PasswordEncodedUser.admin(),
+				AuthorityUtils.createAuthorityList("FACTOR_TWO"));
+		RememberMeAuthenticationToken authentication = factorOne.toBuilder()
+			.authorities((a) -> a.addAll(factorTwo.getAuthorities()))
+			.key("yek")
+			.principal(factorTwo.getPrincipal())
+			.build();
+		Set<String> authorities = AuthorityUtils.authorityListToSet(authentication.getAuthorities());
+		assertThat(authentication.getKeyHash()).isEqualTo(factorTwo.getKeyHash());
+		assertThat(authentication.getPrincipal()).isEqualTo(factorTwo.getPrincipal());
+		assertThat(authorities).containsExactlyInAnyOrder("FACTOR_ONE", "FACTOR_TWO");
+	}
+
 }