Skip to content

Commit 23a1541

Browse files
jzheauxjzheaux
committed
Improvements
1 parent f5214c0 commit 23a1541

File tree

15 files changed

+180
-95
lines changed

15 files changed

+180
-95
lines changed

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

Lines changed: 15 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -18,18 +18,17 @@
1818

1919
import org.springframework.context.ApplicationContext;
2020
import org.springframework.context.ApplicationContextAware;
21-
import org.springframework.security.authentication.password.ChangePasswordAdvice;
2221
import org.springframework.security.authentication.password.ChangePasswordAdvisor;
2322
import org.springframework.security.authentication.password.ChangePasswordServiceAdvisor;
2423
import org.springframework.security.authentication.password.DelegatingChangePasswordAdvisor;
2524
import org.springframework.security.authentication.password.UserDetailsPasswordManager;
2625
import org.springframework.security.config.annotation.web.HttpSecurityBuilder;
27-
import org.springframework.security.core.userdetails.UserDetails;
2826
import org.springframework.security.web.RequestMatcherRedirectFilter;
2927
import org.springframework.security.web.authentication.UsernamePasswordAuthenticationFilter;
3028
import org.springframework.security.web.authentication.password.ChangeCompromisedPasswordAdvisor;
3129
import org.springframework.security.web.authentication.password.ChangePasswordAdviceHandler;
3230
import org.springframework.security.web.authentication.password.ChangePasswordAdviceRepository;
31+
import org.springframework.security.web.authentication.password.ChangePasswordAdviceSessionAuthenticationStrategy;
3332
import org.springframework.security.web.authentication.password.ChangePasswordAdvisingFilter;
3433
import org.springframework.security.web.authentication.password.HttpSessionChangePasswordAdviceRepository;
3534
import org.springframework.security.web.authentication.password.SimpleChangePasswordAdviceHandler;
@@ -82,8 +81,7 @@ public PasswordManagementConfigurer<B> changePasswordAdviceRepository(
8281
return this;
8382
}
8483

85-
public PasswordManagementConfigurer<B> changePasswordAdvisor(
86-
ChangePasswordAdvisor changePasswordAdvisor) {
84+
public PasswordManagementConfigurer<B> changePasswordAdvisor(ChangePasswordAdvisor changePasswordAdvisor) {
8785
this.changePasswordAdvisor = changePasswordAdvisor;
8886
return this;
8987
}
@@ -115,24 +113,25 @@ public void init(B http) throws Exception {
115113
: this.context.getBeanProvider(ChangePasswordAdviceRepository.class)
116114
.getIfUnique(HttpSessionChangePasswordAdviceRepository::new);
117115

118-
ChangePasswordAdvisor changePasswordAdvisor = (this.changePasswordAdvisor != null)
119-
? this.changePasswordAdvisor
116+
ChangePasswordAdvisor changePasswordAdvisor = (this.changePasswordAdvisor != null) ? this.changePasswordAdvisor
120117
: this.context.getBeanProvider(ChangePasswordAdvisor.class)
121-
.getIfUnique(() -> DelegatingChangePasswordAdvisor.of(
122-
new ChangePasswordServiceAdvisor(passwordManager), new ChangeCompromisedPasswordAdvisor()));
118+
.getIfUnique(() -> DelegatingChangePasswordAdvisor
119+
.of(new ChangePasswordServiceAdvisor(passwordManager), new ChangeCompromisedPasswordAdvisor()));
123120

124121
http.setSharedObject(ChangePasswordAdviceRepository.class, changePasswordAdviceRepository);
125122
http.setSharedObject(UserDetailsPasswordManager.class, passwordManager);
126123

127-
FormLoginConfigurer form = http.getConfigurer(FormLoginConfigurer.class);
128-
String passwordParameter = (form != null) ? form.getPasswordParameter() : "password";
124+
String passwordParameter = "password";
125+
FormLoginConfigurer<B> form = http.getConfigurer(FormLoginConfigurer.class);
126+
if (form != null) {
127+
passwordParameter = form.getPasswordParameter();
128+
}
129+
ChangePasswordAdviceSessionAuthenticationStrategy sessionAuthenticationStrategy = new ChangePasswordAdviceSessionAuthenticationStrategy(
130+
passwordParameter);
131+
sessionAuthenticationStrategy.setChangePasswordAdviceRepository(changePasswordAdviceRepository);
132+
sessionAuthenticationStrategy.setChangePasswordAdvisor(changePasswordAdvisor);
129133
http.getConfigurer(SessionManagementConfigurer.class)
130-
.addSessionAuthenticationStrategy((authentication, request, response) -> {
131-
UserDetails user = (UserDetails) authentication.getPrincipal();
132-
String password = request.getParameter(passwordParameter);
133-
ChangePasswordAdvice advice = changePasswordAdvisor.advise(user, password);
134-
changePasswordAdviceRepository.savePasswordAdvice(request, response, advice);
135-
});
134+
.addSessionAuthenticationStrategy(sessionAuthenticationStrategy);
136135
}
137136

138137
/**

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

Lines changed: 48 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@
1919
import java.net.URI;
2020
import java.util.UUID;
2121

22+
import jakarta.servlet.http.HttpServletRequest;
23+
import jakarta.servlet.http.HttpServletResponse;
2224
import org.junit.jupiter.api.Test;
2325
import org.junit.jupiter.api.extension.ExtendWith;
2426

@@ -28,7 +30,8 @@
2830
import org.springframework.http.ResponseEntity;
2931
import org.springframework.mock.web.MockHttpSession;
3032
import org.springframework.security.authentication.password.ChangePasswordAdvice;
31-
import org.springframework.security.authentication.password.ChangePasswordReason;
33+
import org.springframework.security.authentication.password.ChangePasswordAdvisor;
34+
import org.springframework.security.authentication.password.ChangePasswordReasons;
3235
import org.springframework.security.authentication.password.SimpleChangePasswordAdvice;
3336
import org.springframework.security.authentication.password.UserDetailsPasswordManager;
3437
import org.springframework.security.config.Customizer;
@@ -37,13 +40,16 @@
3740
import org.springframework.security.config.test.SpringTestContext;
3841
import org.springframework.security.config.test.SpringTestContextExtension;
3942
import org.springframework.security.core.annotation.AuthenticationPrincipal;
40-
import org.springframework.security.core.userdetails.User;
43+
import org.springframework.security.core.userdetails.PasswordEncodedUser;
4144
import org.springframework.security.core.userdetails.UserDetails;
4245
import org.springframework.security.core.userdetails.UserDetailsService;
4346
import org.springframework.security.crypto.factory.PasswordEncoderFactories;
4447
import org.springframework.security.crypto.password.PasswordEncoder;
4548
import org.springframework.security.provisioning.InMemoryUserDetailsManager;
4649
import org.springframework.security.web.SecurityFilterChain;
50+
import org.springframework.security.web.authentication.password.ChangeCompromisedPasswordAdvisor;
51+
import org.springframework.security.web.authentication.password.ChangePasswordAdviceRepository;
52+
import org.springframework.security.web.authentication.password.HttpSessionChangePasswordAdviceRepository;
4753
import org.springframework.test.web.servlet.MockMvc;
4854
import org.springframework.test.web.servlet.MvcResult;
4955
import org.springframework.web.bind.annotation.GetMapping;
@@ -125,9 +131,8 @@ void whenAdminSetsExpiredAdviceThenUserLoginRedirectsToResetPassword() throws Ex
125131
this.mvc.perform(get("/").with(user(admin))).andExpect(status().isOk());
126132
// change the password to a test value
127133
String random = UUID.randomUUID().toString();
128-
this.mvc.perform(post("/change-password").with(csrf()).with(user(admin)).param("newPassword", random))
129-
.andExpect(status().isFound())
130-
.andExpect(redirectedUrl("/"));
134+
this.mvc.perform(post("/change-password").with(csrf()).with(user(admin)).param("password", random))
135+
.andExpect(status().isOk());
131136
// admin "expires" their own password
132137
this.mvc.perform(post("/admin/passwords/expire/admin").with(csrf()).with(user(admin)))
133138
.andExpect(status().isCreated());
@@ -144,9 +149,8 @@ void whenAdminSetsExpiredAdviceThenUserLoginRedirectsToResetPassword() throws Ex
144149
.andExpect(redirectedUrl("/change-password"));
145150
// reset the password to update
146151
random = UUID.randomUUID().toString();
147-
this.mvc.perform(post("/change-password").with(csrf()).session(session).param("newPassword", random))
148-
.andExpect(status().isFound())
149-
.andExpect(redirectedUrl("/"));
152+
this.mvc.perform(post("/change-password").with(csrf()).session(session).param("password", random))
153+
.andExpect(status().isOk());
150154
// now we're good
151155
this.mvc.perform(get("/").session(session)).andExpect(status().isOk());
152156
}
@@ -155,14 +159,14 @@ void whenAdminSetsExpiredAdviceThenUserLoginRedirectsToResetPassword() throws Ex
155159
void whenCompromisedThenUserLoginAllowed() throws Exception {
156160
this.spring.register(PasswordManagementConfig.class, AdminController.class, HomeController.class).autowire();
157161
MvcResult result = this.mvc
158-
.perform(post("/login").with(csrf()).param("username", "compromised").param("password", "password"))
162+
.perform(post("/login").with(csrf()).param("username", "user").param("password", "password"))
159163
.andExpect(status().isFound())
160164
.andExpect(redirectedUrl("/"))
161165
.andReturn();
162166
MockHttpSession session = (MockHttpSession) result.getRequest().getSession();
163167
this.mvc.perform(get("/").session(session))
164168
.andExpect(status().isOk())
165-
.andExpect(content().string(containsString("COMPROMISED")));
169+
.andExpect(content().string(containsString("compromised")));
166170
}
167171

168172
@Configuration
@@ -207,8 +211,8 @@ SecurityFilterChain securityFilterChain(HttpSecurity http) throws Exception {
207211
// @formatter:off
208212
http
209213
.authorizeHttpRequests((authz) -> authz
210-
.requestMatchers("/admin/**").hasRole("ADMIN")
211-
.anyRequest().authenticated()
214+
.requestMatchers("/admin/**").hasRole("ADMIN")
215+
.anyRequest().authenticated()
212216
)
213217
.formLogin(Customizer.withDefaults())
214218
.passwordManagement(Customizer.withDefaults());
@@ -219,8 +223,9 @@ SecurityFilterChain securityFilterChain(HttpSecurity http) throws Exception {
219223
@Bean
220224
UserDetailsService users() {
221225
String adminPassword = UUID.randomUUID().toString();
222-
UserDetails compromised = User.withUsername("compromised").password("{noop}password").roles("USER").build();
223-
UserDetails admin = User.withUsername("admin").password("{noop}" + adminPassword).roles("ADMIN").build();
226+
UserDetails compromised = PasswordEncodedUser.user();
227+
UserDetails admin = PasswordEncodedUser.withUserDetails(PasswordEncodedUser.admin())
228+
.password(adminPassword).build();
224229
return new InMemoryUserDetailsManager(compromised, admin);
225230
}
226231

@@ -234,11 +239,9 @@ static class AdminController {
234239

235240
private final UserDetailsPasswordManager passwords;
236241

237-
private final PasswordEncoder encoder = PasswordEncoderFactories.createDelegatingPasswordEncoder();
238-
239-
AdminController(UserDetailsService users) {
242+
AdminController(UserDetailsService users, UserDetailsPasswordManager passwords) {
240243
this.users = users;
241-
this.passwords = (UserDetailsPasswordManager) users;
244+
this.passwords = passwords;
242245
}
243246

244247
@GetMapping("/advice/{username}")
@@ -258,29 +261,48 @@ ResponseEntity<ChangePasswordAdvice> expirePassword(@PathVariable("username") St
258261
return ResponseEntity.notFound().build();
259262
}
260263
ChangePasswordAdvice advice = new SimpleChangePasswordAdvice(ChangePasswordAdvice.Action.MUST_CHANGE,
261-
ChangePasswordReason.EXPIRED);
264+
ChangePasswordReasons.EXPIRED);
262265
this.passwords.savePasswordAdvice(user, advice);
263266
URI uri = URI.create("/admin/passwords/advice/" + username);
264267
return ResponseEntity.created(uri).body(advice);
265268
}
266269

267-
@PostMapping("/change")
268-
ResponseEntity<?> changePassword(@AuthenticationPrincipal UserDetails user,
269-
@RequestParam("password") String password) {
270-
this.passwords.updatePassword(user, this.encoder.encode(password));
271-
return ResponseEntity.ok().build();
272-
}
273-
274270
}
275271

276272
@RestController
277273
static class HomeController {
278274

275+
private final UserDetailsPasswordManager passwords;
276+
277+
private final ChangePasswordAdvisor changePasswordAdvisor =
278+
new ChangeCompromisedPasswordAdvisor();
279+
280+
private final ChangePasswordAdviceRepository changePasswordAdviceRepository =
281+
new HttpSessionChangePasswordAdviceRepository();
282+
283+
private final PasswordEncoder encoder = PasswordEncoderFactories.createDelegatingPasswordEncoder();
284+
285+
HomeController(UserDetailsPasswordManager passwords) {
286+
this.passwords = passwords;
287+
}
288+
279289
@GetMapping
280290
ChangePasswordAdvice index(ChangePasswordAdvice advice) {
281291
return advice;
282292
}
283293

294+
@PostMapping("/change-password")
295+
ResponseEntity<?> changePassword(@AuthenticationPrincipal UserDetails user,
296+
@RequestParam("password") String password, HttpServletRequest request, HttpServletResponse response) {
297+
ChangePasswordAdvice advice = this.changePasswordAdvisor.advise(user, password);
298+
if (advice.getAction() != ChangePasswordAdvice.Action.ABSTAIN) {
299+
return ResponseEntity.badRequest().body(advice);
300+
}
301+
this.passwords.updatePassword(user, this.encoder.encode(password));
302+
this.passwords.removePasswordAdvice(user);
303+
this.changePasswordAdviceRepository.removePasswordAdvice(request, response);
304+
return ResponseEntity.ok().build();
305+
}
284306
}
285307

286308
}

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -41,12 +41,12 @@ public ChangeLengthPasswordAdvisor(int minLength, int maxLength) {
4141
@Override
4242
public ChangePasswordAdvice advise(UserDetails user, String password) {
4343
if (password.length() < this.minLength) {
44-
return new SimpleChangePasswordAdvice(this.tooShortAction, ChangePasswordReason.TOO_SHORT);
44+
return new SimpleChangePasswordAdvice(this.tooShortAction, ChangePasswordReasons.TOO_SHORT);
4545
}
4646
if (password.length() > this.maxLength) {
47-
return new SimpleChangePasswordAdvice(this.tooLongAction, ChangePasswordReason.TOO_LONG);
47+
return new SimpleChangePasswordAdvice(this.tooLongAction, ChangePasswordReasons.TOO_LONG);
4848
}
49-
return ChangePasswordAdvice.keep();
49+
return ChangePasswordAdvice.abstain();
5050
}
5151

5252
public void setTooShortAction(Action tooShortAction) {

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,15 +22,15 @@ public interface ChangePasswordAdvice {
2222

2323
Action getAction();
2424

25-
Collection<ChangePasswordReason> getReasons();
25+
Collection<String> getReasons();
2626

27-
static ChangePasswordAdvice keep() {
28-
return SimpleChangePasswordAdvice.KEEP;
27+
static ChangePasswordAdvice abstain() {
28+
return SimpleChangePasswordAdvice.ABSTAIN;
2929
}
3030

3131
enum Action {
3232

33-
KEEP, SHOULD_CHANGE, MUST_CHANGE
33+
ABSTAIN, SHOULD_CHANGE, MUST_CHANGE
3434

3535
}
3636

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

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
package org.springframework.security.authentication.password;
2+
3+
public interface ChangePasswordReasons {
4+
5+
String COMPROMISED = "compromised";
6+
7+
String EXPIRED = "expired";
8+
9+
String MISSING_CHARACTERS = "missing_characters";
10+
11+
String REPEATED = "repeated";
12+
13+
String TOO_LONG = "too_long";
14+
15+
String TOO_SHORT = "too_short";
16+
17+
String UNSUPPORTED_CHARACTERS = "unsupported_characters";
18+
}

‎core/src/main/java/org/springframework/security/authentication/password/ChangePasswordServiceAdvisor.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,8 @@ public ChangePasswordServiceAdvisor(UserDetailsPasswordManager passwordManager)
2828

2929
@Override
3030
public ChangePasswordAdvice advise(UserDetails user, String password) {
31-
return this.passwordManager.loadPasswordAdvice(user);
31+
ChangePasswordAdvice advice = this.passwordManager.loadPasswordAdvice(user);
32+
return (advice != null) ? advice : ChangePasswordAdvice.abstain();
3233
}
3334

3435
}

‎core/src/main/java/org/springframework/security/authentication/password/DelegatingChangePasswordAdvisor.java

Lines changed: 13 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -20,58 +20,46 @@
2020
import java.util.Collection;
2121
import java.util.Collections;
2222
import java.util.List;
23-
import java.util.Objects;
24-
import java.util.function.BiFunction;
25-
import java.util.stream.Stream;
2623

2724
import org.springframework.security.core.userdetails.UserDetails;
2825

29-
public final class DelegatingChangePasswordAdvisor
30-
implements ChangePasswordAdvisor {
26+
public final class DelegatingChangePasswordAdvisor implements ChangePasswordAdvisor {
3127

32-
private final List<BiFunction<UserDetails, String, ChangePasswordAdvice>> advisors;
28+
private final List<ChangePasswordAdvisor> advisors;
3329

34-
private DelegatingChangePasswordAdvisor(List<BiFunction<UserDetails, String, ChangePasswordAdvice>> advisors) {
30+
private DelegatingChangePasswordAdvisor(List<ChangePasswordAdvisor> advisors) {
3531
this.advisors = Collections.unmodifiableList(advisors);
3632
}
3733

3834
public static ChangePasswordAdvisor of(ChangePasswordAdvisor... advisors) {
39-
return new DelegatingChangePasswordAdvisor(Stream.of(advisors)
40-
.map((advisor) -> (BiFunction<UserDetails, String, ChangePasswordAdvice>) advisor::advise)
41-
.toList());
35+
return new DelegatingChangePasswordAdvisor(List.of(advisors));
4236
}
4337

4438
@Override
4539
public ChangePasswordAdvice advise(UserDetails user, String password) {
4640
Collection<ChangePasswordAdvice> advice = this.advisors.stream()
47-
.map((advisor) -> advisor.apply(user, password))
48-
.filter(Objects::nonNull)
41+
.map((advisor) -> advisor.advise(user, password))
42+
.filter((a) -> a.getAction() != ChangePasswordAdvice.Action.ABSTAIN)
4943
.toList();
5044
return new CompositeChangePasswordAdvice(advice);
5145
}
5246

5347
private static final class CompositeChangePasswordAdvice implements ChangePasswordAdvice {
5448

55-
private final Collection<ChangePasswordAdvice> advice;
56-
5749
private final Action action;
5850

59-
private final Collection<ChangePasswordReason> reasons;
51+
private final Collection<String> reasons;
6052

6153
private CompositeChangePasswordAdvice(Collection<ChangePasswordAdvice> advice) {
62-
this.advice = advice;
63-
Action action = Action.KEEP;
64-
Collection<ChangePasswordReason> reasons = new ArrayList<>();
54+
Action mostUrgentAction = Action.ABSTAIN;
55+
Collection<String> reasons = new ArrayList<>();
6556
for (ChangePasswordAdvice a : advice) {
66-
if (a.getAction() == Action.KEEP) {
67-
continue;
68-
}
69-
if (action.ordinal() < a.getAction().ordinal()) {
70-
action = a.getAction();
57+
if (mostUrgentAction.ordinal() < a.getAction().ordinal()) {
58+
mostUrgentAction = a.getAction();
7159
}
7260
reasons.addAll(a.getReasons());
7361
}
74-
this.action = action;
62+
this.action = mostUrgentAction;
7563
this.reasons = reasons;
7664
}
7765

@@ -81,7 +69,7 @@ public Action getAction() {
8169
}
8270

8371
@Override
84-
public Collection<ChangePasswordReason> getReasons() {
72+
public Collection<String> getReasons() {
8573
return this.reasons;
8674
}
8775

0 commit comments

Comments
 (0)