Remove Reasons

- These aren't super helpful in the end. Applications
can inspect the type and get a richer set of information.

Author: jzheaux

config/src/main/java/org/springframework/security/config/annotation/web/configurers/PasswordManagementConfigurer.java

@@ -19,19 +19,17 @@
 import org.springframework.context.ApplicationContext;
 import org.springframework.context.ApplicationContextAware;
 import org.springframework.security.authentication.password.ChangePasswordAdvisor;
-import org.springframework.security.authentication.password.ChangePasswordServiceAdvisor;
-import org.springframework.security.authentication.password.DelegatingChangePasswordAdvisor;
+import org.springframework.security.authentication.password.CompositeChangePasswordAdvisor;
+import org.springframework.security.authentication.password.UserDetailsChangePasswordAdvisor;
 import org.springframework.security.authentication.password.UserDetailsPasswordManager;
 import org.springframework.security.config.annotation.web.HttpSecurityBuilder;
 import org.springframework.security.web.RequestMatcherRedirectFilter;
 import org.springframework.security.web.authentication.UsernamePasswordAuthenticationFilter;
 import org.springframework.security.web.authentication.password.ChangeCompromisedPasswordAdvisor;
-import org.springframework.security.web.authentication.password.ChangePasswordAdviceHandler;
 import org.springframework.security.web.authentication.password.ChangePasswordAdviceRepository;
 import org.springframework.security.web.authentication.password.ChangePasswordAdviceSessionAuthenticationStrategy;
 import org.springframework.security.web.authentication.password.ChangePasswordAdvisingFilter;
 import org.springframework.security.web.authentication.password.HttpSessionChangePasswordAdviceRepository;
-import org.springframework.security.web.authentication.password.RedirectingChangePasswordAdviceHandler;
 import org.springframework.security.web.savedrequest.RequestCache;
 import org.springframework.security.web.savedrequest.RequestCacheAwareFilter;
 import org.springframework.util.Assert;
@@ -59,8 +57,6 @@ public final class PasswordManagementConfigurer<B extends HttpSecurityBuilder<B>
 
 	private ChangePasswordAdvisor changePasswordAdvisor;
 
-	private ChangePasswordAdviceHandler changePasswordAdviceHandler;
-
 	private UserDetailsPasswordManager userDetailsPasswordManager;
 
 	/**
@@ -87,12 +83,6 @@ public PasswordManagementConfigurer<B> changePasswordAdvisor(ChangePasswordAdvis
 		return this;
 	}
 
-	public PasswordManagementConfigurer<B> changePasswordAdviceHandler(
-			ChangePasswordAdviceHandler changePasswordAdviceHandler) {
-		this.changePasswordAdviceHandler = changePasswordAdviceHandler;
-		return this;
-	}
-
 	public PasswordManagementConfigurer<B> userDetailsPasswordManager(
 			UserDetailsPasswordManager userDetailsPasswordManager) {
 		this.userDetailsPasswordManager = userDetailsPasswordManager;
@@ -116,8 +106,8 @@ public void init(B http) throws Exception {
 
 		ChangePasswordAdvisor changePasswordAdvisor = (this.changePasswordAdvisor != null) ? this.changePasswordAdvisor
 				: this.context.getBeanProvider(ChangePasswordAdvisor.class)
-					.getIfUnique(() -> DelegatingChangePasswordAdvisor
-						.of(new ChangePasswordServiceAdvisor(passwordManager), new ChangeCompromisedPasswordAdvisor()));
+					.getIfUnique(() -> CompositeChangePasswordAdvisor
+						.of(new UserDetailsChangePasswordAdvisor(), new ChangeCompromisedPasswordAdvisor()));
 
 		http.setSharedObject(ChangePasswordAdviceRepository.class, changePasswordAdviceRepository);
 		http.setSharedObject(UserDetailsPasswordManager.class, passwordManager);

config/src/test/java/org/springframework/security/config/annotation/web/configurers/PasswordManagementConfigurerTests.java

@@ -31,8 +31,6 @@
 import org.springframework.mock.web.MockHttpSession;
 import org.springframework.security.authentication.password.ChangePasswordAdvice;
 import org.springframework.security.authentication.password.ChangePasswordAdvisor;
-import org.springframework.security.authentication.password.ChangePasswordReasons;
-import org.springframework.security.authentication.password.SimpleChangePasswordAdvice;
 import org.springframework.security.authentication.password.UserDetailsPasswordManager;
 import org.springframework.security.config.Customizer;
 import org.springframework.security.config.annotation.web.builders.HttpSecurity;
@@ -250,8 +248,7 @@ ResponseEntity<ChangePasswordAdvice> requireChangePassword(@PathVariable("userna
 			if (user == null) {
 				return ResponseEntity.notFound().build();
 			}
-			ChangePasswordAdvice advice = this.passwords.loadPasswordAdvice(user);
-			return ResponseEntity.ok(advice);
+			return ResponseEntity.ok(user.getChangePasswordAdvice());
 		}
 
 		@PostMapping("/expire/{username}")
@@ -260,8 +257,7 @@ ResponseEntity<ChangePasswordAdvice> expirePassword(@PathVariable("username") St
 			if (user == null) {
 				return ResponseEntity.notFound().build();
 			}
-			ChangePasswordAdvice advice = new SimpleChangePasswordAdvice(ChangePasswordAdvice.Action.MUST_CHANGE,
-					ChangePasswordReasons.EXPIRED);
+			ChangePasswordAdvice advice = () -> ChangePasswordAdvice.Action.MUST_CHANGE;
 			this.passwords.savePasswordAdvice(user, advice);
 			URI uri = URI.create("/admin/passwords/advice/" + username);
 			return ResponseEntity.created(uri).body(advice);
@@ -299,7 +295,6 @@ ResponseEntity<?> changePassword(@AuthenticationPrincipal UserDetails user,
 				return ResponseEntity.badRequest().body(advice);
 			}
 			this.passwords.updatePassword(user, this.encoder.encode(password));
-			this.passwords.removePasswordAdvice(user);
 			this.changePasswordAdviceRepository.removePasswordAdvice(request, response);
 			return ResponseEntity.ok().build();
 		}

core/src/main/java/org/springframework/security/authentication/password/ChangeLengthPasswordAdvisor.java

@@ -41,12 +41,12 @@ public ChangeLengthPasswordAdvisor(int minLength, int maxLength) {
 	@Override
 	public ChangePasswordAdvice advise(UserDetails user, String password) {
 		if (password.length() < this.minLength) {
-			return new SimpleChangePasswordAdvice(this.tooShortAction, ChangePasswordReasons.TOO_SHORT);
+			return new TooShortAdvice(this.tooShortAction, this.minLength, password.length());
 		}
 		if (password.length() > this.maxLength) {
-			return new SimpleChangePasswordAdvice(this.tooLongAction, ChangePasswordReasons.TOO_LONG);
+			return new TooLongAdvice(this.tooLongAction, this.maxLength, password.length());
 		}
-		return ChangePasswordAdvice.abstain();
+		return ChangePasswordAdvice.ABSTAIN;
 	}
 
 	public void setTooShortAction(Action tooShortAction) {
@@ -57,4 +57,76 @@ public void setTooLongAction(Action tooLongAction) {
 		this.tooLongAction = tooLongAction;
 	}
 
+	public static final class TooShortAdvice implements ChangePasswordAdvice {
+		private final Action action;
+
+		private final int minLength;
+
+		private final int actualLength;
+
+		private TooShortAdvice(Action action, int minLength, int actualLength) {
+			this.action = action;
+			this.minLength = minLength;
+			this.actualLength = actualLength;
+		}
+
+		@Override
+		public Action getAction() {
+			return this.action;
+		}
+
+		public int getMinLength() {
+			return this.minLength;
+		}
+
+		public int getActualLength() {
+			return this.actualLength;
+		}
+
+		@Override
+		public String toString() {
+			return "TooShort [" +
+					"action=" + this.action +
+					", minLength=" + this.minLength +
+					", actualLength=" + this.actualLength +
+					"]";
+		}
+
+	}
+
+	public static final class TooLongAdvice implements ChangePasswordAdvice {
+		private final Action action;
+
+		private final int maxLength;
+
+		private final int actualLength;
+
+		private TooLongAdvice(Action action, int maxLength, int actualLength) {
+			this.action = action;
+			this.maxLength = maxLength;
+			this.actualLength = actualLength;
+		}
+
+		@Override
+		public Action getAction() {
+			return this.action;
+		}
+
+		public int getMaxLength() {
+			return this.maxLength;
+		}
+
+		public int getActualLength() {
+			return this.actualLength;
+		}
+
+		@Override
+		public String toString() {
+			return "TooLong [" +
+					"action=" + this.action +
+					", maxLength=" + this.maxLength +
+					", actualLength=" + this.actualLength +
+					"]";
+		}
+	}
 }

core/src/main/java/org/springframework/security/authentication/password/ChangePasswordAdvice.java

@@ -16,17 +16,11 @@
 
 package org.springframework.security.authentication.password;
 
-import java.util.Collection;
-
 public interface ChangePasswordAdvice {
 
-	Action getAction();
-
-	Collection<String> getReasons();
+	ChangePasswordAdvice ABSTAIN = () -> Action.ABSTAIN;
 
-	static ChangePasswordAdvice abstain() {
-		return SimpleChangePasswordAdvice.ABSTAIN;
-	}
+	Action getAction();
 
 	enum Action {
 

core/src/main/java/org/springframework/security/authentication/password/ChangePasswordReasons.java

@@ -16,63 +16,69 @@
 
 package org.springframework.security.authentication.password;
 
-import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
 
 import org.springframework.security.core.userdetails.UserDetails;
 
-public final class DelegatingChangePasswordAdvisor implements ChangePasswordAdvisor {
+public final class CompositeChangePasswordAdvisor implements ChangePasswordAdvisor {
 
 	private final List<ChangePasswordAdvisor> advisors;
 
-	private DelegatingChangePasswordAdvisor(List<ChangePasswordAdvisor> advisors) {
+	private CompositeChangePasswordAdvisor(List<ChangePasswordAdvisor> advisors) {
 		this.advisors = Collections.unmodifiableList(advisors);
 	}
 
 	public static ChangePasswordAdvisor of(ChangePasswordAdvisor... advisors) {
-		return new DelegatingChangePasswordAdvisor(List.of(advisors));
+		return new CompositeChangePasswordAdvisor(List.of(advisors));
 	}
 
 	@Override
 	public ChangePasswordAdvice advise(UserDetails user, String password) {
 		Collection<ChangePasswordAdvice> advice = this.advisors.stream()
 			.map((advisor) -> advisor.advise(user, password))
-			.filter((a) -> a.getAction() != ChangePasswordAdvice.Action.ABSTAIN)
 			.toList();
-		return new CompositeChangePasswordAdvice(advice);
+		return new Advice(advice);
 	}
 
-	private static final class CompositeChangePasswordAdvice implements ChangePasswordAdvice {
+	public static final class Advice implements ChangePasswordAdvice {
 
 		private final Action action;
 
-		private final Collection<String> reasons;
+		private final Collection<ChangePasswordAdvice> advice;
 
-		private CompositeChangePasswordAdvice(Collection<ChangePasswordAdvice> advice) {
+		private Advice(Collection<ChangePasswordAdvice> advice) {
+			this.action = findMostUrgentAction(advice);
+			this.advice = advice;
+		}
+
+		private Action findMostUrgentAction(Collection<ChangePasswordAdvice> advice) {
 			Action mostUrgentAction = Action.ABSTAIN;
-			Collection<String> reasons = new ArrayList<>();
 			for (ChangePasswordAdvice a : advice) {
 				if (mostUrgentAction.ordinal() < a.getAction().ordinal()) {
 					mostUrgentAction = a.getAction();
 				}
-				reasons.addAll(a.getReasons());
 			}
-			this.action = mostUrgentAction;
-			this.reasons = reasons;
+			return mostUrgentAction;
 		}
 
 		@Override
 		public Action getAction() {
 			return this.action;
 		}
 
-		@Override
-		public Collection<String> getReasons() {
-			return this.reasons;
+		public Collection<ChangePasswordAdvice> getAdvice() {
+			return this.advice;
 		}
 
+		@Override
+		public String toString() {
+			return "Composite [" +
+					"action=" + this.action +
+					", advice=" + this.advice +
+					"]";
+		}
 	}
 
 }