Skip to content

Commit 1637727

Browse files
jzheauxjzheaux
committed
Merge branch '6.5.x' into 7.0.x
2 parents a2358bb + 6e5f8f2 commit 1637727

8 files changed

Lines changed: 174 additions & 19 deletions

File tree

‎core/src/main/java/org/springframework/security/authentication/dao/AbstractUserDetailsAuthenticationProvider.java‎

Lines changed: 39 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,8 @@ public abstract class AbstractUserDetailsAuthenticationProvider
9797

9898
private UserDetailsChecker postAuthenticationChecks = new DefaultPostAuthenticationChecks();
9999

100+
private boolean alwaysPerformAdditionalChecksOnUser = true;
101+
100102
private GrantedAuthoritiesMapper authoritiesMapper = new NullAuthoritiesMapper();
101103

102104
private static final String AUTHORITY = FactorGrantedAuthority.PASSWORD_AUTHORITY;
@@ -154,8 +156,7 @@ public Authentication authenticate(Authentication authentication) throws Authent
154156
Assert.notNull(user, "retrieveUser returned null - a violation of the interface contract");
155157
}
156158
try {
157-
this.preAuthenticationChecks.check(user);
158-
additionalAuthenticationChecks(user, (UsernamePasswordAuthenticationToken) authentication);
159+
performPreCheck(user, (UsernamePasswordAuthenticationToken) authentication);
159160
}
160161
catch (AuthenticationException ex) {
161162
if (!cacheWasUsed) {
@@ -165,8 +166,7 @@ public Authentication authenticate(Authentication authentication) throws Authent
165166
// we're using latest data (i.e. not from the cache)
166167
cacheWasUsed = false;
167168
user = retrieveUser(username, (UsernamePasswordAuthenticationToken) authentication);
168-
this.preAuthenticationChecks.check(user);
169-
additionalAuthenticationChecks(user, (UsernamePasswordAuthenticationToken) authentication);
169+
performPreCheck(user, (UsernamePasswordAuthenticationToken) authentication);
170170
}
171171
this.postAuthenticationChecks.check(user);
172172
if (!cacheWasUsed) {
@@ -179,6 +179,25 @@ public Authentication authenticate(Authentication authentication) throws Authent
179179
return createSuccessAuthentication(principalToReturn, authentication, user);
180180
}
181181

182+
private void performPreCheck(UserDetails user, UsernamePasswordAuthenticationToken authentication) {
183+
try {
184+
this.preAuthenticationChecks.check(user);
185+
}
186+
catch (AuthenticationException ex) {
187+
if (!this.alwaysPerformAdditionalChecksOnUser) {
188+
throw ex;
189+
}
190+
try {
191+
additionalAuthenticationChecks(user, authentication);
192+
}
193+
catch (AuthenticationException ignored) {
194+
// preserve the original failed check
195+
}
196+
throw ex;
197+
}
198+
additionalAuthenticationChecks(user, authentication);
199+
}
200+
182201
private String determineUsername(Authentication authentication) {
183202
return (authentication.getPrincipal() == null) ? "NONE_PROVIDED" : authentication.getName();
184203
}
@@ -324,6 +343,22 @@ public void setPostAuthenticationChecks(UserDetailsChecker postAuthenticationChe
324343
this.postAuthenticationChecks = postAuthenticationChecks;
325344
}
326345

346+
/**
347+
* Set whether to always perform the additional checks on the user, even if the
348+
* pre-authentication checks fail. This is useful to ensure that regardless of the
349+
* state of the user account, authentication takes the same amount of time to
350+
* complete.
351+
*
352+
* <p>
353+
* For applications that rely on the additional checks running only once should set
354+
* this value to {@code false}
355+
* @param alwaysPerformAdditionalChecksOnUser
356+
* @since 5.7.23
357+
*/
358+
public void setAlwaysPerformAdditionalChecksOnUser(boolean alwaysPerformAdditionalChecksOnUser) {
359+
this.alwaysPerformAdditionalChecksOnUser = alwaysPerformAdditionalChecksOnUser;
360+
}
361+
327362
public void setAuthoritiesMapper(GrantedAuthoritiesMapper authoritiesMapper) {
328363
this.authoritiesMapper = authoritiesMapper;
329364
}

‎core/src/main/java/org/springframework/security/authentication/ott/JdbcOneTimeTokenService.java‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,9 @@ private void insertOneTimeToken(OneTimeToken oneTimeToken) {
153153
return null;
154154
}
155155
OneTimeToken token = tokens.get(0);
156-
deleteOneTimeToken(token);
156+
if (deleteOneTimeToken(token) == 0) {
157+
return null;
158+
}
157159
if (isExpired(token)) {
158160
return null;
159161
}
@@ -171,11 +173,11 @@ private List<OneTimeToken> selectOneTimeToken(OneTimeTokenAuthenticationToken au
171173
return this.jdbcOperations.query(SELECT_ONE_TIME_TOKEN_SQL, pss, this.oneTimeTokenRowMapper);
172174
}
173175

174-
private void deleteOneTimeToken(OneTimeToken oneTimeToken) {
176+
private int deleteOneTimeToken(OneTimeToken oneTimeToken) {
175177
List<SqlParameterValue> parameters = List
176178
.of(new SqlParameterValue(Types.VARCHAR, oneTimeToken.getTokenValue()));
177179
PreparedStatementSetter pss = new ArgumentPreparedStatementSetter(parameters.toArray());
178-
this.jdbcOperations.update(DELETE_ONE_TIME_TOKEN_SQL, pss);
180+
return this.jdbcOperations.update(DELETE_ONE_TIME_TOKEN_SQL, pss);
179181
}
180182

181183
private @Nullable ThreadPoolTaskScheduler createTaskScheduler(String cleanupCron) {

‎core/src/test/java/org/springframework/security/authentication/dao/DaoAuthenticationProviderTests.java‎

Lines changed: 42 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import java.util.List;
2222

2323
import org.junit.jupiter.api.Test;
24+
import org.junit.jupiter.api.condition.EnabledIfSystemProperty;
2425

2526
import org.springframework.cache.Cache;
2627
import org.springframework.dao.DataRetrievalFailureException;
@@ -44,6 +45,7 @@
4445
import org.springframework.security.core.userdetails.PasswordEncodedUser;
4546
import org.springframework.security.core.userdetails.User;
4647
import org.springframework.security.core.userdetails.UserDetails;
48+
import org.springframework.security.core.userdetails.UserDetailsChecker;
4749
import org.springframework.security.core.userdetails.UserDetailsPasswordService;
4850
import org.springframework.security.core.userdetails.UserDetailsService;
4951
import org.springframework.security.core.userdetails.UsernameNotFoundException;
@@ -64,6 +66,7 @@
6466
import static org.mockito.ArgumentMatchers.eq;
6567
import static org.mockito.ArgumentMatchers.isA;
6668
import static org.mockito.BDDMockito.given;
69+
import static org.mockito.BDDMockito.willThrow;
6770
import static org.mockito.Mockito.mock;
6871
import static org.mockito.Mockito.times;
6972
import static org.mockito.Mockito.verify;
@@ -422,12 +425,10 @@ public void constructWhenPasswordEncoderProvidedThenSets() {
422425
assertThat(daoAuthenticationProvider.getPasswordEncoder()).isSameAs(NoOpPasswordEncoder.getInstance());
423426
}
424427

425-
/**
426-
* This is an explicit test for SEC-2056. It is intentionally ignored since this test
427-
* is not deterministic and {@link #testUserNotFoundEncodesPassword()} ensures that
428-
* SEC-2056 is fixed.
429-
*/
430-
public void IGNOREtestSec2056() {
428+
// SEC-2056
429+
@Test
430+
@EnabledIfSystemProperty(named = "spring.security.timing-tests", matches = "true")
431+
public void testSec2056() {
431432
UsernamePasswordAuthenticationToken foundUser = UsernamePasswordAuthenticationToken.unauthenticated("rod",
432433
"koala");
433434
UsernamePasswordAuthenticationToken notFoundUser = UsernamePasswordAuthenticationToken
@@ -460,6 +461,41 @@ public void IGNOREtestSec2056() {
460461
.isTrue();
461462
}
462463

464+
// related to SEC-2056
465+
@Test
466+
@EnabledIfSystemProperty(named = "spring.security.timing-tests", matches = "true")
467+
public void testDisabledUserTiming() {
468+
UsernamePasswordAuthenticationToken user = UsernamePasswordAuthenticationToken.unauthenticated("rod", "koala");
469+
PasswordEncoder encoder = new BCryptPasswordEncoder();
470+
DaoAuthenticationProvider provider = new DaoAuthenticationProvider();
471+
provider.setPasswordEncoder(encoder);
472+
MockUserDetailsServiceUserRod users = new MockUserDetailsServiceUserRod();
473+
users.password = encoder.encode((CharSequence) user.getCredentials());
474+
provider.setUserDetailsService(users);
475+
int sampleSize = 100;
476+
List<Long> enabledTimes = new ArrayList<>(sampleSize);
477+
for (int i = 0; i < sampleSize; i++) {
478+
long start = System.currentTimeMillis();
479+
provider.authenticate(user);
480+
enabledTimes.add(System.currentTimeMillis() - start);
481+
}
482+
UserDetailsChecker preChecks = mock(UserDetailsChecker.class);
483+
willThrow(new DisabledException("User is disabled")).given(preChecks).check(any(UserDetails.class));
484+
provider.setPreAuthenticationChecks(preChecks);
485+
List<Long> disabledTimes = new ArrayList<>(sampleSize);
486+
for (int i = 0; i < sampleSize; i++) {
487+
long start = System.currentTimeMillis();
488+
assertThatExceptionOfType(DisabledException.class).isThrownBy(() -> provider.authenticate(user));
489+
disabledTimes.add(System.currentTimeMillis() - start);
490+
}
491+
double enabledAvg = avg(enabledTimes);
492+
double disabledAvg = avg(disabledTimes);
493+
assertThat(Math.abs(disabledAvg - enabledAvg) <= 3)
494+
.withFailMessage("Disabled user average " + disabledAvg + " should be within 3ms of enabled user average "
495+
+ enabledAvg)
496+
.isTrue();
497+
}
498+
463499
private double avg(List<Long> counts) {
464500
return counts.stream().mapToLong(Long::longValue).average().orElse(0);
465501
}

‎core/src/test/java/org/springframework/security/authentication/ott/JdbcOneTimeTokenServiceTests.java‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,19 +21,24 @@
2121
import java.time.Instant;
2222
import java.time.ZoneOffset;
2323
import java.time.temporal.ChronoUnit;
24+
import java.util.List;
2425

2526
import org.junit.jupiter.api.AfterEach;
2627
import org.junit.jupiter.api.BeforeEach;
2728
import org.junit.jupiter.api.Test;
29+
import org.mockito.ArgumentMatchers;
2830

2931
import org.springframework.jdbc.core.JdbcOperations;
3032
import org.springframework.jdbc.core.JdbcTemplate;
33+
import org.springframework.jdbc.core.PreparedStatementSetter;
34+
import org.springframework.jdbc.core.RowMapper;
3135
import org.springframework.jdbc.datasource.embedded.EmbeddedDatabase;
3236
import org.springframework.jdbc.datasource.embedded.EmbeddedDatabaseBuilder;
3337
import org.springframework.jdbc.datasource.embedded.EmbeddedDatabaseType;
3438

3539
import static org.assertj.core.api.Assertions.assertThat;
3640
import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
41+
import static org.mockito.ArgumentMatchers.any;
3742
import static org.mockito.BDDMockito.given;
3843
import static org.mockito.Mockito.mock;
3944

@@ -145,6 +150,27 @@ void consumeWhenTokenDoesNotExistsThenReturnNull() {
145150
assertThat(consumedOneTimeToken).isNull();
146151
}
147152

153+
@Test
154+
void consumeWhenTokenIsDeletedConcurrentlyThenReturnNull() throws Exception {
155+
// Simulates a concurrent consume: SELECT finds the token but DELETE affects
156+
// 0 rows because another caller already consumed it.
157+
JdbcOperations jdbcOperations = mock(JdbcOperations.class);
158+
Instant notExpired = Instant.now().plus(5, ChronoUnit.MINUTES);
159+
OneTimeToken token = new DefaultOneTimeToken(TOKEN_VALUE, USERNAME, notExpired);
160+
given(jdbcOperations.query(any(String.class), any(PreparedStatementSetter.class),
161+
ArgumentMatchers.<RowMapper<OneTimeToken>>any()))
162+
.willReturn(List.of(token));
163+
given(jdbcOperations.update(any(String.class), any(PreparedStatementSetter.class))).willReturn(0);
164+
JdbcOneTimeTokenService service = new JdbcOneTimeTokenService(jdbcOperations);
165+
try {
166+
OneTimeToken consumed = service.consume(new OneTimeTokenAuthenticationToken(TOKEN_VALUE));
167+
assertThat(consumed).isNull();
168+
}
169+
finally {
170+
service.destroy();
171+
}
172+
}
173+
148174
@Test
149175
void consumeWhenTokenIsExpiredThenReturnNull() {
150176
GenerateOneTimeTokenRequest request = new GenerateOneTimeTokenRequest(USERNAME);

‎oauth2/oauth2-jose/src/main/java/org/springframework/security/oauth2/jwt/NimbusJwtDecoder.java‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -232,7 +232,8 @@ public static JwkSetUriJwtDecoderBuilder withIssuerLocation(String issuer) {
232232
.getConfigurationForIssuerLocation(issuer, rest);
233233
JwtDecoderProviderConfigurationUtils.validateIssuer(configuration, issuer);
234234
return configuration.get("jwks_uri").toString();
235-
}, JwtDecoderProviderConfigurationUtils::getJWSAlgorithms);
235+
}, JwtDecoderProviderConfigurationUtils::getJWSAlgorithms)
236+
.validator(JwtValidators.createDefaultWithIssuer(issuer));
236237
}
237238

238239
/**
@@ -301,6 +302,8 @@ public static final class JwkSetUriJwtDecoderBuilder {
301302

302303
private Consumer<ConfigurableJWTProcessor<SecurityContext>> jwtProcessorCustomizer;
303304

305+
private OAuth2TokenValidator<Jwt> validator = JwtValidators.createDefault();
306+
304307
private JwkSetUriJwtDecoderBuilder(String jwkSetUri) {
305308
Assert.hasText(jwkSetUri, "jwkSetUri cannot be empty");
306309
this.jwkSetUri = (rest) -> jwkSetUri;
@@ -441,6 +444,12 @@ public JwkSetUriJwtDecoderBuilder jwtProcessorCustomizer(
441444
return this;
442445
}
443446

447+
JwkSetUriJwtDecoderBuilder validator(OAuth2TokenValidator<Jwt> validator) {
448+
Assert.notNull(validator, "validator cannot be null");
449+
this.validator = validator;
450+
return this;
451+
}
452+
444453
JWSKeySelector<SecurityContext> jwsKeySelector(JWKSource<SecurityContext> jwkSource) {
445454
if (this.signatureAlgorithms.isEmpty()) {
446455
return new JWSVerificationKeySelector<>(this.defaultAlgorithms.apply(jwkSource), jwkSource);
@@ -479,7 +488,9 @@ JWTProcessor<SecurityContext> processor() {
479488
* @return the configured {@link NimbusJwtDecoder}
480489
*/
481490
public NimbusJwtDecoder build() {
482-
return new NimbusJwtDecoder(processor());
491+
NimbusJwtDecoder decoder = new NimbusJwtDecoder(processor());
492+
decoder.setJwtValidator(this.validator);
493+
return decoder;
483494
}
484495

485496
private static final class SpringJWKSource<C extends SecurityContext> implements JWKSetSource<C> {

‎oauth2/oauth2-jose/src/main/java/org/springframework/security/oauth2/jwt/NimbusReactiveJwtDecoder.java‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -241,7 +241,8 @@ public static JwkSetUriReactiveJwtDecoderBuilder withIssuerLocation(String issue
241241
}
242242
return Mono.just(configuration.get("jwks_uri").toString());
243243
}),
244-
ReactiveJwtDecoderProviderConfigurationUtils::getJWSAlgorithms);
244+
ReactiveJwtDecoderProviderConfigurationUtils::getJWSAlgorithms)
245+
.validator(JwtValidators.createDefaultWithIssuer(issuer));
245246
}
246247

247248
/**
@@ -332,6 +333,8 @@ public static final class JwkSetUriReactiveJwtDecoderBuilder {
332333

333334
private BiFunction<ReactiveRemoteJWKSource, ConfigurableJWTProcessor<JWKSecurityContext>, Mono<ConfigurableJWTProcessor<JWKSecurityContext>>> jwtProcessorCustomizer;
334335

336+
private OAuth2TokenValidator<Jwt> validator = JwtValidators.createDefault();
337+
335338
private JwkSetUriReactiveJwtDecoderBuilder(String jwkSetUri) {
336339
Assert.hasText(jwkSetUri, "jwkSetUri cannot be empty");
337340
this.jwkSetUri = (web) -> Mono.just(jwkSetUri);
@@ -456,6 +459,11 @@ public JwkSetUriReactiveJwtDecoderBuilder jwtProcessorCustomizer(
456459
return this;
457460
}
458461

462+
JwkSetUriReactiveJwtDecoderBuilder validator(OAuth2TokenValidator<Jwt> validator) {
463+
this.validator = validator;
464+
return this;
465+
}
466+
459467
JwkSetUriReactiveJwtDecoderBuilder jwtProcessorCustomizer(
460468
BiFunction<ReactiveRemoteJWKSource, ConfigurableJWTProcessor<JWKSecurityContext>, Mono<ConfigurableJWTProcessor<JWKSecurityContext>>> jwtProcessorCustomizer) {
461469
Assert.notNull(jwtProcessorCustomizer, "jwtProcessorCustomizer cannot be null");
@@ -468,7 +476,9 @@ JwkSetUriReactiveJwtDecoderBuilder jwtProcessorCustomizer(
468476
* @return the configured {@link NimbusReactiveJwtDecoder}
469477
*/
470478
public NimbusReactiveJwtDecoder build() {
471-
return new NimbusReactiveJwtDecoder(processor());
479+
NimbusReactiveJwtDecoder decoder = new NimbusReactiveJwtDecoder(processor());
480+
decoder.setJwtValidator(this.validator);
481+
return decoder;
472482
}
473483

474484
Mono<JWSKeySelector<JWKSecurityContext>> jwsKeySelector(ReactiveRemoteJWKSource source) {

‎oauth2/oauth2-jose/src/test/java/org/springframework/security/oauth2/jwt/NimbusJwtDecoderTests.java‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -332,7 +332,10 @@ public void decodeWhenIssuerLocationThenOk() {
332332
.willReturn(new ResponseEntity<>(Map.of("issuer", issuer, "jwks_uri", issuer + "/jwks"), HttpStatus.OK));
333333
given(restOperations.exchange(any(RequestEntity.class), eq(String.class)))
334334
.willReturn(new ResponseEntity<>(JWK_SET, HttpStatus.OK));
335-
JwtDecoder jwtDecoder = NimbusJwtDecoder.withIssuerLocation(issuer).restOperations(restOperations).build();
335+
NimbusJwtDecoder jwtDecoder = NimbusJwtDecoder.withIssuerLocation(issuer)
336+
.restOperations(restOperations)
337+
.build();
338+
jwtDecoder.setJwtValidator(JwtValidators.createDefault());
336339
Jwt jwt = jwtDecoder.decode(SIGNED_JWT);
337340
assertThat(jwt.hasClaim(JwtClaimNames.EXP)).isNotNull();
338341
}
@@ -350,6 +353,18 @@ public void decodeWhenDiscoverJwsAlgorithmsThenOk() {
350353
assertThat(jwt.hasClaim(JwtClaimNames.EXP)).isNotNull();
351354
}
352355

356+
@Test
357+
public void decodeWhenIssuerLocationThenRejectsMismatchingIssuers() {
358+
String issuer = "https://example.org/wrong-issuer";
359+
RestOperations restOperations = mock(RestOperations.class);
360+
given(restOperations.exchange(any(RequestEntity.class), any(ParameterizedTypeReference.class)))
361+
.willReturn(new ResponseEntity<>(Map.of("issuer", issuer, "jwks_uri", issuer + "/jwks"), HttpStatus.OK));
362+
given(restOperations.exchange(any(RequestEntity.class), eq(String.class)))
363+
.willReturn(new ResponseEntity<>(JWK_SET, HttpStatus.OK));
364+
JwtDecoder jwtDecoder = NimbusJwtDecoder.withIssuerLocation(issuer).restOperations(restOperations).build();
365+
assertThatExceptionOfType(JwtValidationException.class).isThrownBy(() -> jwtDecoder.decode(SIGNED_JWT));
366+
}
367+
353368
@Test
354369
public void withJwkSetUriWhenNullOrEmptyThenThrowsException() {
355370
// @formatter:off

‎oauth2/oauth2-jose/src/test/java/org/springframework/security/oauth2/jwt/NimbusReactiveJwtDecoderTests.java‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -617,13 +617,33 @@ public void decodeWhenIssuerLocationThenOk() {
617617
given(responseSpec.bodyToMono(any(ParameterizedTypeReference.class)))
618618
.willReturn(Mono.just(Map.of("issuer", issuer, "jwks_uri", issuer + "/jwks")));
619619
given(spec.retrieve()).willReturn(responseSpec);
620-
ReactiveJwtDecoder jwtDecoder = NimbusReactiveJwtDecoder.withIssuerLocation(issuer)
620+
NimbusReactiveJwtDecoder jwtDecoder = NimbusReactiveJwtDecoder.withIssuerLocation(issuer)
621621
.webClient(webClient)
622622
.build();
623+
jwtDecoder.setJwtValidator(JwtValidators.createDefault());
623624
Jwt jwt = jwtDecoder.decode(this.messageReadToken).block();
624625
assertThat(jwt.hasClaim(JwtClaimNames.EXP)).isNotNull();
625626
}
626627

628+
@Test
629+
public void decodeWhenIssuerLocationThenRejectsMismatchingIssuers() {
630+
String issuer = "https://example.org/wrong-issuer";
631+
WebClient real = WebClient.builder().build();
632+
WebClient.RequestHeadersUriSpec spec = spy(real.get());
633+
WebClient webClient = spy(WebClient.class);
634+
given(webClient.get()).willReturn(spec);
635+
WebClient.ResponseSpec responseSpec = mock(WebClient.ResponseSpec.class);
636+
given(responseSpec.bodyToMono(String.class)).willReturn(Mono.just(this.jwkSet));
637+
given(responseSpec.bodyToMono(any(ParameterizedTypeReference.class)))
638+
.willReturn(Mono.just(Map.of("issuer", issuer, "jwks_uri", issuer + "/jwks")));
639+
given(spec.retrieve()).willReturn(responseSpec);
640+
ReactiveJwtDecoder jwtDecoder = NimbusReactiveJwtDecoder.withIssuerLocation(issuer)
641+
.webClient(webClient)
642+
.build();
643+
assertThatExceptionOfType(JwtValidationException.class)
644+
.isThrownBy(() -> jwtDecoder.decode(this.messageReadToken).block());
645+
}
646+
627647
@Test
628648
public void jwsKeySelectorWhenNoAlgorithmThenReturnsRS256Selector() {
629649
ReactiveRemoteJWKSource jwkSource = mock(ReactiveRemoteJWKSource.class);

0 commit comments

Comments
 (0)