From d3b7a47ef81960d9400134c5ed0b90ea253ca3a2 Mon Sep 17 00:00:00 2001 From: Joe Grandja Date: Wed, 25 Sep 2019 15:15:24 -0400 Subject: [PATCH] Polish gh-4442 --- .../client/OAuth2ClientConfigurerTests.java | 10 +- ...thorizationCodeAuthenticationProvider.java | 15 +-- ...tionCodeReactiveAuthenticationManager.java | 21 ++-- ...ultOAuth2AuthorizationRequestResolver.java | 36 +++--- ...verOAuth2AuthorizationRequestResolver.java | 36 +++--- ...zationCodeAuthenticationProviderTests.java | 110 +++++------------- ...odeReactiveAuthenticationManagerTests.java | 86 +++++++++++--- .../OidcIdTokenDecoderFactoryTests.java | 1 + ...eactiveOidcIdTokenDecoderFactoryTests.java | 1 + ...uth2AuthorizationRequestResolverTests.java | 81 ++++++++----- ...thorizationRequestRedirectFilterTests.java | 13 +-- ...uth2AuthorizationRequestResolverTests.java | 38 ++++-- .../oidc/endpoint/OidcParameterNames.java | 2 +- 13 files changed, 248 insertions(+), 202 deletions(-) diff --git a/config/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/client/OAuth2ClientConfigurerTests.java b/config/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/client/OAuth2ClientConfigurerTests.java index 4605bf821d..86e4b4e3c4 100644 --- a/config/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/client/OAuth2ClientConfigurerTests.java +++ b/config/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/client/OAuth2ClientConfigurerTests.java @@ -75,7 +75,6 @@ import static org.springframework.test.web.servlet.result.MockMvcResultMatchers. * Tests for {@link OAuth2ClientConfigurer}. * * @author Joe Grandja - * @author Mark Heckler */ public class OAuth2ClientConfigurerTests { private static ClientRegistrationRepository clientRegistrationRepository; @@ -139,8 +138,7 @@ public class OAuth2ClientConfigurerTests { assertThat(mvcResult.getResponse().getRedirectedUrl()).matches("https://provider.com/oauth2/authorize\\?" + "response_type=code&client_id=client-1&" + "scope=user&state=.{15,}&" + - "redirect_uri=http://localhost/client-1&" + - "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); + "redirect_uri=http://localhost/client-1"); } @Test @@ -153,8 +151,7 @@ public class OAuth2ClientConfigurerTests { assertThat(mvcResult.getResponse().getRedirectedUrl()).matches("https://provider.com/oauth2/authorize\\?" + "response_type=code&client_id=client-1&" + "scope=user&state=.{15,}&" + - "redirect_uri=http://localhost/client-1&" + - "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); + "redirect_uri=http://localhost/client-1"); } @Test @@ -206,8 +203,7 @@ public class OAuth2ClientConfigurerTests { assertThat(mvcResult.getResponse().getRedirectedUrl()).matches("https://provider.com/oauth2/authorize\\?" + "response_type=code&client_id=client-1&" + "scope=user&state=.{15,}&" + - "redirect_uri=http://localhost/client-1&" + - "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); + "redirect_uri=http://localhost/client-1"); verify(requestCache).saveRequest(any(HttpServletRequest.class), any(HttpServletResponse.class)); } diff --git a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeAuthenticationProvider.java b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeAuthenticationProvider.java index d61f270ec7..07490b6277 100644 --- a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeAuthenticationProvider.java +++ b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeAuthenticationProvider.java @@ -158,21 +158,22 @@ public class OidcAuthorizationCodeAuthenticationProvider implements Authenticati null); throw new OAuth2AuthenticationException(invalidIdTokenError, invalidIdTokenError.toString()); } - OidcIdToken idToken = createOidcToken(clientRegistration, accessTokenResponse); + OidcIdToken idToken = createOidcToken(clientRegistration, accessTokenResponse); + // Validate nonce String requestNonce = authorizationRequest.getAttribute(OidcParameterNames.NONCE); if (requestNonce != null) { String nonceHash; - try { nonceHash = createHash(requestNonce); } catch (NoSuchAlgorithmException e) { - throw new OAuth2AuthenticationException(new OAuth2Error(INVALID_NONCE_ERROR_CODE)); + OAuth2Error oauth2Error = new OAuth2Error(INVALID_NONCE_ERROR_CODE); + throw new OAuth2AuthenticationException(oauth2Error, oauth2Error.toString()); } - - String nonceHashClaim = idToken.getClaim(OidcParameterNames.NONCE); + String nonceHashClaim = idToken.getNonce(); if (nonceHashClaim == null || !nonceHashClaim.equals(nonceHash)) { - throw new OAuth2AuthenticationException(new OAuth2Error(INVALID_NONCE_ERROR_CODE)); + OAuth2Error oauth2Error = new OAuth2Error(INVALID_NONCE_ERROR_CODE); + throw new OAuth2AuthenticationException(oauth2Error, oauth2Error.toString()); } } @@ -234,7 +235,7 @@ public class OidcAuthorizationCodeAuthenticationProvider implements Authenticati return idToken; } - private String createHash(String nonce) throws NoSuchAlgorithmException { + static String createHash(String nonce) throws NoSuchAlgorithmException { MessageDigest md = MessageDigest.getInstance("SHA-256"); byte[] digest = md.digest(nonce.getBytes(StandardCharsets.US_ASCII)); return Base64.getUrlEncoder().withoutPadding().encodeToString(digest); diff --git a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeReactiveAuthenticationManager.java b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeReactiveAuthenticationManager.java index 5e2c1b8150..e4612df291 100644 --- a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeReactiveAuthenticationManager.java +++ b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeReactiveAuthenticationManager.java @@ -65,6 +65,7 @@ import java.util.Map; * to complete the authentication. * * @author Rob Winch + * @author Mark Heckler * @since 5.1 * @see OAuth2LoginAuthenticationToken * @see ReactiveOAuth2AccessTokenResponseClient @@ -199,30 +200,28 @@ public class OidcAuthorizationCodeReactiveAuthenticationManager implements .map(jwt -> new OidcIdToken(jwt.getTokenValue(), jwt.getIssuedAt(), jwt.getExpiresAt(), jwt.getClaims())); } - private Mono validateNonce(OAuth2AuthorizationCodeAuthenticationToken authorizationCodeAuthentication, OidcIdToken idToken) { - String requestNonce = authorizationCodeAuthentication - .getAuthorizationExchange() - .getAuthorizationRequest() - .getAttribute(OidcParameterNames.NONCE); + private static Mono validateNonce(OAuth2AuthorizationCodeAuthenticationToken authorizationCodeAuthentication, OidcIdToken idToken) { + String requestNonce = authorizationCodeAuthentication.getAuthorizationExchange() + .getAuthorizationRequest().getAttribute(OidcParameterNames.NONCE); if (requestNonce != null) { String nonceHash; - try { nonceHash = createHash(requestNonce); } catch (NoSuchAlgorithmException e) { - throw new OAuth2AuthenticationException(new OAuth2Error(INVALID_NONCE_ERROR_CODE)); + OAuth2Error oauth2Error = new OAuth2Error(INVALID_NONCE_ERROR_CODE); + throw new OAuth2AuthenticationException(oauth2Error, oauth2Error.toString()); } - - String nonceHashClaim = idToken.getClaim(OidcParameterNames.NONCE); + String nonceHashClaim = idToken.getNonce(); if (nonceHashClaim == null || !nonceHashClaim.equals(nonceHash)) { - throw new OAuth2AuthenticationException(new OAuth2Error(INVALID_NONCE_ERROR_CODE)); + OAuth2Error oauth2Error = new OAuth2Error(INVALID_NONCE_ERROR_CODE); + throw new OAuth2AuthenticationException(oauth2Error, oauth2Error.toString()); } } return Mono.just(idToken); } - private String createHash(String nonce) throws NoSuchAlgorithmException { + static String createHash(String nonce) throws NoSuchAlgorithmException { MessageDigest md = MessageDigest.getInstance("SHA-256"); byte[] digest = md.digest(nonce.getBytes(StandardCharsets.US_ASCII)); return Base64.getUrlEncoder().withoutPadding().encodeToString(digest); diff --git a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/DefaultOAuth2AuthorizationRequestResolver.java b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/DefaultOAuth2AuthorizationRequestResolver.java index 533d29c374..b104796687 100644 --- a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/DefaultOAuth2AuthorizationRequestResolver.java +++ b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/DefaultOAuth2AuthorizationRequestResolver.java @@ -24,10 +24,12 @@ import org.springframework.security.oauth2.core.ClientAuthenticationMethod; import org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationRequest; import org.springframework.security.oauth2.core.endpoint.OAuth2ParameterNames; import org.springframework.security.oauth2.core.endpoint.PkceParameterNames; +import org.springframework.security.oauth2.core.oidc.OidcScopes; import org.springframework.security.oauth2.core.oidc.endpoint.OidcParameterNames; import org.springframework.security.web.util.UrlUtils; import org.springframework.security.web.util.matcher.AntPathRequestMatcher; import org.springframework.util.Assert; +import org.springframework.util.CollectionUtils; import org.springframework.util.StringUtils; import org.springframework.web.util.UriComponents; import org.springframework.web.util.UriComponentsBuilder; @@ -63,7 +65,7 @@ public final class DefaultOAuth2AuthorizationRequestResolver implements OAuth2Au private final ClientRegistrationRepository clientRegistrationRepository; private final AntPathRequestMatcher authorizationRequestMatcher; private final StringKeyGenerator stateGenerator = new Base64StringKeyGenerator(Base64.getUrlEncoder()); - private final StringKeyGenerator stringKeyGenerator = new Base64StringKeyGenerator(Base64.getUrlEncoder().withoutPadding(), 96); + private final StringKeyGenerator secureKeyGenerator = new Base64StringKeyGenerator(Base64.getUrlEncoder().withoutPadding(), 96); /** * Constructs a {@code DefaultOAuth2AuthorizationRequestResolver} using the provided parameters. @@ -121,13 +123,16 @@ public final class DefaultOAuth2AuthorizationRequestResolver implements OAuth2Au if (AuthorizationGrantType.AUTHORIZATION_CODE.equals(clientRegistration.getAuthorizationGrantType())) { builder = OAuth2AuthorizationRequest.authorizationCode(); Map additionalParameters = new HashMap<>(); - - addNonceParameters(attributes, additionalParameters); - + if (!CollectionUtils.isEmpty(clientRegistration.getScopes()) && + clientRegistration.getScopes().contains(OidcScopes.OPENID)) { + // Section 3.1.2.1 Authentication Request - https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest + // scope + // REQUIRED. OpenID Connect requests MUST contain the "openid" scope value. + addNonceParameters(attributes, additionalParameters); + } if (ClientAuthenticationMethod.NONE.equals(clientRegistration.getClientAuthenticationMethod())) { addPkceParameters(attributes, additionalParameters); } - builder.additionalParameters(additionalParameters); } else if (AuthorizationGrantType.IMPLICIT.equals(clientRegistration.getAuthorizationGrantType())) { builder = OAuth2AuthorizationRequest.implicit(); @@ -208,24 +213,21 @@ public final class DefaultOAuth2AuthorizationRequestResolver implements OAuth2Au } /** - * Creates nonce and its hash for use in OpenID Connect Authentication Requests + * Creates nonce and its hash for use in OpenID Connect 1.0 Authentication Requests. * - * @param attributes where {@link OidcParameterNames#NONCE} is stored for the token request - * @param additionalParameters where hash of {@link OidcParameterNames#NONCE} is added to the authentication request + * @param attributes where the {@link OidcParameterNames#NONCE} is stored for the authentication request + * @param additionalParameters where the {@link OidcParameterNames#NONCE} hash is added for the authentication request * * @since 5.2 - * @see 15.5.2. Nonce Implementation Notes - * @see 3.1.3.7. ID Token Validation + * @see 3.1.2.1. Authentication Request */ private void addNonceParameters(Map attributes, Map additionalParameters) { try { - String nonce = this.stringKeyGenerator.generateKey(); - attributes.put(OidcParameterNames.NONCE, nonce); - + String nonce = this.secureKeyGenerator.generateKey(); String nonceHash = createHash(nonce); + attributes.put(OidcParameterNames.NONCE, nonce); additionalParameters.put(OidcParameterNames.NONCE, nonceHash); - } catch (NoSuchAlgorithmException ignored) { - } + } catch (NoSuchAlgorithmException e) { } } /** @@ -241,7 +243,7 @@ public final class DefaultOAuth2AuthorizationRequestResolver implements OAuth2Au * @see 4.2. Client Creates the Code Challenge */ private void addPkceParameters(Map attributes, Map additionalParameters) { - String codeVerifier = this.stringKeyGenerator.generateKey(); + String codeVerifier = this.secureKeyGenerator.generateKey(); attributes.put(PkceParameterNames.CODE_VERIFIER, codeVerifier); try { String codeChallenge = createHash(codeVerifier); @@ -252,7 +254,7 @@ public final class DefaultOAuth2AuthorizationRequestResolver implements OAuth2Au } } - private String createHash(String value) throws NoSuchAlgorithmException { + private static String createHash(String value) throws NoSuchAlgorithmException { MessageDigest md = MessageDigest.getInstance("SHA-256"); byte[] digest = md.digest(value.getBytes(StandardCharsets.US_ASCII)); return Base64.getUrlEncoder().withoutPadding().encodeToString(digest); diff --git a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/server/DefaultServerOAuth2AuthorizationRequestResolver.java b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/server/DefaultServerOAuth2AuthorizationRequestResolver.java index 6561c517bf..1410099d93 100644 --- a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/server/DefaultServerOAuth2AuthorizationRequestResolver.java +++ b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/server/DefaultServerOAuth2AuthorizationRequestResolver.java @@ -27,10 +27,12 @@ import org.springframework.security.oauth2.core.ClientAuthenticationMethod; import org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationRequest; import org.springframework.security.oauth2.core.endpoint.OAuth2ParameterNames; import org.springframework.security.oauth2.core.endpoint.PkceParameterNames; +import org.springframework.security.oauth2.core.oidc.OidcScopes; import org.springframework.security.oauth2.core.oidc.endpoint.OidcParameterNames; import org.springframework.security.web.server.util.matcher.PathPatternParserServerWebExchangeMatcher; import org.springframework.security.web.server.util.matcher.ServerWebExchangeMatcher; import org.springframework.util.Assert; +import org.springframework.util.CollectionUtils; import org.springframework.util.StringUtils; import org.springframework.web.server.ResponseStatusException; import org.springframework.web.server.ServerWebExchange; @@ -77,7 +79,7 @@ public class DefaultServerOAuth2AuthorizationRequestResolver private final StringKeyGenerator stateGenerator = new Base64StringKeyGenerator(Base64.getUrlEncoder()); - private final StringKeyGenerator stringKeyGenerator = new Base64StringKeyGenerator(Base64.getUrlEncoder().withoutPadding(), 96); + private final StringKeyGenerator secureKeyGenerator = new Base64StringKeyGenerator(Base64.getUrlEncoder().withoutPadding(), 96); /** * Creates a new instance @@ -135,13 +137,16 @@ public class DefaultServerOAuth2AuthorizationRequestResolver if (AuthorizationGrantType.AUTHORIZATION_CODE.equals(clientRegistration.getAuthorizationGrantType())) { builder = OAuth2AuthorizationRequest.authorizationCode(); Map additionalParameters = new HashMap<>(); - - addNonceParameters(attributes, additionalParameters); - + if (!CollectionUtils.isEmpty(clientRegistration.getScopes()) && + clientRegistration.getScopes().contains(OidcScopes.OPENID)) { + // Section 3.1.2.1 Authentication Request - https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest + // scope + // REQUIRED. OpenID Connect requests MUST contain the "openid" scope value. + addNonceParameters(attributes, additionalParameters); + } if (ClientAuthenticationMethod.NONE.equals(clientRegistration.getClientAuthenticationMethod())) { addPkceParameters(attributes, additionalParameters); } - builder.additionalParameters(additionalParameters); } else if (AuthorizationGrantType.IMPLICIT.equals(clientRegistration.getAuthorizationGrantType())) { builder = OAuth2AuthorizationRequest.implicit(); @@ -212,24 +217,21 @@ public class DefaultServerOAuth2AuthorizationRequestResolver } /** - * Creates nonce and its hash for use in OpenID Connect Authentication Requests + * Creates nonce and its hash for use in OpenID Connect 1.0 Authentication Requests. * - * @param attributes where {@link OidcParameterNames#NONCE} is stored for the token request - * @param additionalParameters where hash of {@link OidcParameterNames#NONCE} is added to the authentication request + * @param attributes where the {@link OidcParameterNames#NONCE} is stored for the authentication request + * @param additionalParameters where the {@link OidcParameterNames#NONCE} hash is added for the authentication request * * @since 5.2 - * @see 15.5.2. Nonce Implementation Notes - * @see 3.1.3.7. ID Token Validation + * @see 3.1.2.1. Authentication Request */ private void addNonceParameters(Map attributes, Map additionalParameters) { try { - String nonce = this.stringKeyGenerator.generateKey(); - attributes.put(OidcParameterNames.NONCE, nonce); - + String nonce = this.secureKeyGenerator.generateKey(); String nonceHash = createHash(nonce); + attributes.put(OidcParameterNames.NONCE, nonce); additionalParameters.put(OidcParameterNames.NONCE, nonceHash); - } catch (NoSuchAlgorithmException ignored) { - } + } catch (NoSuchAlgorithmException e) { } } /** @@ -245,7 +247,7 @@ public class DefaultServerOAuth2AuthorizationRequestResolver * @see 4.2. Client Creates the Code Challenge */ private void addPkceParameters(Map attributes, Map additionalParameters) { - String codeVerifier = this.stringKeyGenerator.generateKey(); + String codeVerifier = this.secureKeyGenerator.generateKey(); attributes.put(PkceParameterNames.CODE_VERIFIER, codeVerifier); try { String codeChallenge = createHash(codeVerifier); @@ -256,7 +258,7 @@ public class DefaultServerOAuth2AuthorizationRequestResolver } } - private String createHash(String value) throws NoSuchAlgorithmException { + private static String createHash(String value) throws NoSuchAlgorithmException { MessageDigest md = MessageDigest.getInstance("SHA-256"); byte[] digest = md.digest(value.getBytes(StandardCharsets.US_ASCII)); return Base64.getUrlEncoder().withoutPadding().encodeToString(digest); diff --git a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeAuthenticationProviderTests.java b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeAuthenticationProviderTests.java index a5776d4842..ded67aa501 100644 --- a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeAuthenticationProviderTests.java +++ b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeAuthenticationProviderTests.java @@ -46,8 +46,6 @@ import org.springframework.security.oauth2.jwt.Jwt; import org.springframework.security.oauth2.jwt.JwtDecoder; import org.springframework.security.oauth2.jwt.JwtException; -import java.nio.charset.StandardCharsets; -import java.security.MessageDigest; import java.security.NoSuchAlgorithmException; import java.time.Instant; import java.util.Arrays; @@ -64,10 +62,12 @@ import static org.hamcrest.CoreMatchers.containsString; import static org.mockito.ArgumentMatchers.*; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; +import static org.springframework.security.oauth2.client.oidc.authentication.OidcAuthorizationCodeAuthenticationProvider.createHash; import static org.springframework.security.oauth2.client.registration.TestClientRegistrations.clientRegistration; import static org.springframework.security.oauth2.core.endpoint.TestOAuth2AuthorizationRequests.request; import static org.springframework.security.oauth2.core.endpoint.TestOAuth2AuthorizationResponses.error; import static org.springframework.security.oauth2.core.endpoint.TestOAuth2AuthorizationResponses.success; +import static org.springframework.security.oauth2.jwt.TestJwts.jwt; /** * Tests for {@link OidcAuthorizationCodeAuthenticationProvider}. @@ -84,8 +84,7 @@ public class OidcAuthorizationCodeAuthenticationProviderTests { private OAuth2AccessTokenResponse accessTokenResponse; private OAuth2UserService userService; private OidcAuthorizationCodeAuthenticationProvider authenticationProvider; - private StringKeyGenerator stringKeyGenerator = new Base64StringKeyGenerator(Base64.getUrlEncoder().withoutPadding(), 96); - private String nonce = this.stringKeyGenerator.generateKey(); + private StringKeyGenerator secureKeyGenerator = new Base64StringKeyGenerator(Base64.getUrlEncoder().withoutPadding(), 96); private String nonceHash; @Rule @@ -94,16 +93,15 @@ public class OidcAuthorizationCodeAuthenticationProviderTests { @Before @SuppressWarnings("unchecked") public void setUp() { - try { - nonceHash = createHash(nonce); - } catch (NoSuchAlgorithmException e) { - e.printStackTrace(); - } + this.clientRegistration = clientRegistration().clientId("client1").build(); Map attributes = new HashMap<>(); Map additionalParameters = new HashMap<>(); - addNonceToRequest(attributes, additionalParameters); - - this.clientRegistration = clientRegistration().clientId("client1").build(); + try { + String nonce = this.secureKeyGenerator.generateKey(); + this.nonceHash = createHash(nonce); + attributes.put(OidcParameterNames.NONCE, nonce); + additionalParameters.put(OidcParameterNames.NONCE, this.nonceHash); + } catch (NoSuchAlgorithmException e) { } this.authorizationRequest = request() .scope("openid", "profile", "email") .attributes(attributes) @@ -240,6 +238,23 @@ public class OidcAuthorizationCodeAuthenticationProviderTests { new OAuth2LoginAuthenticationToken(this.clientRegistration, this.authorizationExchange)); } + @Test + public void authenticateWhenIdTokenInvalidNonceThenThrowOAuth2AuthenticationException() { + this.exception.expect(OAuth2AuthenticationException.class); + this.exception.expectMessage(containsString("[invalid_nonce]")); + + Map claims = new HashMap<>(); + claims.put(IdTokenClaimNames.ISS, "https://provider.com"); + claims.put(IdTokenClaimNames.SUB, "subject1"); + claims.put(IdTokenClaimNames.AUD, Arrays.asList("client1", "client2")); + claims.put(IdTokenClaimNames.AZP, "client1"); + claims.put(IdTokenClaimNames.NONCE, "invalid-nonce-hash"); + this.setUpIdToken(claims); + + this.authenticationProvider.authenticate( + new OAuth2LoginAuthenticationToken(this.clientRegistration, this.authorizationExchange)); + } + @Test public void authenticateWhenLoginSuccessThenReturnAuthentication() { Map claims = new HashMap<>(); @@ -247,7 +262,7 @@ public class OidcAuthorizationCodeAuthenticationProviderTests { claims.put(IdTokenClaimNames.SUB, "subject1"); claims.put(IdTokenClaimNames.AUD, Arrays.asList("client1", "client2")); claims.put(IdTokenClaimNames.AZP, "client1"); - claims.put(IdTokenClaimNames.NONCE, nonceHash); + claims.put(IdTokenClaimNames.NONCE, this.nonceHash); this.setUpIdToken(claims); OidcUser principal = mock(OidcUser.class); @@ -277,7 +292,7 @@ public class OidcAuthorizationCodeAuthenticationProviderTests { claims.put(IdTokenClaimNames.SUB, "subject1"); claims.put(IdTokenClaimNames.AUD, Arrays.asList("client1", "client2")); claims.put(IdTokenClaimNames.AZP, "client1"); - claims.put(IdTokenClaimNames.NONCE, nonceHash); + claims.put(IdTokenClaimNames.NONCE, this.nonceHash); this.setUpIdToken(claims); OidcUser principal = mock(OidcUser.class); @@ -307,7 +322,7 @@ public class OidcAuthorizationCodeAuthenticationProviderTests { claims.put(IdTokenClaimNames.SUB, "subject1"); claims.put(IdTokenClaimNames.AUD, Arrays.asList("client1", "client2")); claims.put(IdTokenClaimNames.AZP, "client1"); - claims.put(IdTokenClaimNames.NONCE, nonceHash); + claims.put(IdTokenClaimNames.NONCE, this.nonceHash); this.setUpIdToken(claims); OidcUser principal = mock(OidcUser.class); @@ -324,49 +339,8 @@ public class OidcAuthorizationCodeAuthenticationProviderTests { this.accessTokenResponse.getAdditionalParameters()); } - // gh-4442 - @Test - public void authenticateWhenTokenSuccessResponseThenAdditionalParametersAddedToUserRequestNoNonce() { - OAuth2AuthorizationRequest authorizationRequestNoNonce = request() - .scope("openid", "profile", "email") - .attributes(new HashMap<>()) - .additionalParameters(new HashMap<>()) - .build(); - OAuth2AuthorizationExchange authorizationExchangeNoNonce = new OAuth2AuthorizationExchange(authorizationRequestNoNonce, this.authorizationResponse); - - Map claims = new HashMap<>(); - claims.put(IdTokenClaimNames.ISS, "https://provider.com"); - claims.put(IdTokenClaimNames.SUB, "subject1"); - claims.put(IdTokenClaimNames.AUD, Arrays.asList("client1", "client2")); - claims.put(IdTokenClaimNames.AZP, "client1"); - this.setUpIdToken(claims); - - OidcUser principal = mock(OidcUser.class); - List authorities = AuthorityUtils.createAuthorityList("ROLE_USER"); - when(principal.getAuthorities()).thenAnswer( - (Answer>) invocation -> authorities); - ArgumentCaptor userRequestArgCaptor = ArgumentCaptor.forClass(OidcUserRequest.class); - when(this.userService.loadUser(userRequestArgCaptor.capture())).thenReturn(principal); - - this.authenticationProvider.authenticate(new OAuth2LoginAuthenticationToken( - this.clientRegistration, authorizationExchangeNoNonce)); - - assertThat(userRequestArgCaptor.getValue().getAdditionalParameters()).containsAllEntriesOf( - this.accessTokenResponse.getAdditionalParameters()); - } - private void setUpIdToken(Map claims) { - Jwt idToken = Jwt.withTokenValue("token") - .header("alg", "none") - .audience(Collections.singletonList("https://audience.example.org")) - .expiresAt(Instant.MAX) - .issuedAt(Instant.MIN) - .issuer("https://issuer.example.org") - .jti("jti") - .notBefore(Instant.MIN) - .subject("mock-test-subject") - .claims(c -> c.putAll(claims)) - .build(); + Jwt idToken = jwt().claims(c -> c.putAll(claims)).build(); JwtDecoder jwtDecoder = mock(JwtDecoder.class); when(jwtDecoder.decode(anyString())).thenReturn(idToken); this.authenticationProvider.setJwtDecoderFactory(registration -> jwtDecoder); @@ -379,7 +353,6 @@ public class OidcAuthorizationCodeAuthenticationProviderTests { additionalParameters.put("param1", "value1"); additionalParameters.put("param2", "value2"); additionalParameters.put(OidcParameterNames.ID_TOKEN, "id-token"); - additionalParameters.put(IdTokenClaimNames.NONCE, nonceHash); return OAuth2AccessTokenResponse .withToken("access-token-1234") @@ -391,25 +364,4 @@ public class OidcAuthorizationCodeAuthenticationProviderTests { .build(); } - - /** - * Adds nonce for use in OpenID Connect Authentication Requests - * - * @param attributes where {@link IdTokenClaimNames#NONCE} is stored for the token request - * @param additionalParameters where the hash of {@link IdTokenClaimNames#NONCE} is added to be used in the authentication request - * - * @since 5.2 - * @see 15.5.2. Nonce Implementation Notes - * @see 3.1.3.7. ID Token Validation - */ - private void addNonceToRequest(Map attributes, Map additionalParameters) { - attributes.put(IdTokenClaimNames.NONCE, nonce); - additionalParameters.put(IdTokenClaimNames.NONCE, nonceHash); - } - - private String createHash(String nonce) throws NoSuchAlgorithmException { - MessageDigest md = MessageDigest.getInstance("SHA-256"); - byte[] digest = md.digest(nonce.getBytes(StandardCharsets.US_ASCII)); - return Base64.getUrlEncoder().withoutPadding().encodeToString(digest); - } } diff --git a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeReactiveAuthenticationManagerTests.java b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeReactiveAuthenticationManagerTests.java index cbeb598a1f..1c71ca4734 100644 --- a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeReactiveAuthenticationManagerTests.java +++ b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeReactiveAuthenticationManagerTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 the original author or authors. + * Copyright 2002-2019 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -16,22 +16,16 @@ package org.springframework.security.oauth2.client.oidc.authentication; -import java.time.Instant; -import java.util.Arrays; -import java.util.Collections; -import java.util.HashMap; -import java.util.Map; - import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.junit.MockitoJUnitRunner; -import reactor.core.publisher.Mono; - import org.springframework.security.authentication.TestingAuthenticationToken; import org.springframework.security.core.authority.AuthorityUtils; +import org.springframework.security.crypto.keygen.Base64StringKeyGenerator; +import org.springframework.security.crypto.keygen.StringKeyGenerator; import org.springframework.security.oauth2.client.authentication.OAuth2AuthorizationCodeAuthenticationToken; import org.springframework.security.oauth2.client.authentication.OAuth2LoginAuthenticationToken; import org.springframework.security.oauth2.client.endpoint.OAuth2AuthorizationCodeGrantRequest; @@ -54,16 +48,25 @@ import org.springframework.security.oauth2.core.oidc.user.OidcUser; import org.springframework.security.oauth2.jwt.Jwt; import org.springframework.security.oauth2.jwt.JwtException; import org.springframework.security.oauth2.jwt.ReactiveJwtDecoder; +import reactor.core.publisher.Mono; -import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatCode; -import static org.assertj.core.api.Assertions.assertThatThrownBy; +import java.security.NoSuchAlgorithmException; +import java.time.Instant; +import java.util.Arrays; +import java.util.Base64; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; + +import static org.assertj.core.api.Assertions.*; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.when; +import static org.springframework.security.oauth2.client.oidc.authentication.OidcAuthorizationCodeReactiveAuthenticationManager.createHash; import static org.springframework.security.oauth2.jwt.TestJwts.jwt; /** * @author Rob Winch + * @author Joe Grandja * @since 5.1 */ @RunWith(MockitoJUnitRunner.class) @@ -89,6 +92,10 @@ public class OidcAuthorizationCodeReactiveAuthenticationManagerTests { private OidcAuthorizationCodeReactiveAuthenticationManager manager; + private StringKeyGenerator secureKeyGenerator = new Base64StringKeyGenerator(Base64.getUrlEncoder().withoutPadding(), 96); + + private String nonceHash; + @Before public void setup() { this.manager = new OidcAuthorizationCodeReactiveAuthenticationManager(this.accessTokenResponseClient, this.userService); @@ -164,6 +171,31 @@ public class OidcAuthorizationCodeReactiveAuthenticationManagerTests { .hasMessageContaining("[invalid_id_token] ID Token Validation Error"); } + @Test + public void authenticateWhenIdTokenInvalidNonceThenOAuth2AuthenticationException() { + OAuth2AccessTokenResponse accessTokenResponse = OAuth2AccessTokenResponse.withToken("foo") + .tokenType(OAuth2AccessToken.TokenType.BEARER) + .additionalParameters(Collections.singletonMap(OidcParameterNames.ID_TOKEN, this.idToken.getTokenValue())) + .build(); + + OAuth2AuthorizationCodeAuthenticationToken authorizationCodeAuthentication = loginToken(); + + Map claims = new HashMap<>(); + claims.put(IdTokenClaimNames.ISS, "https://issuer.example.com"); + claims.put(IdTokenClaimNames.SUB, "sub"); + claims.put(IdTokenClaimNames.AUD, Arrays.asList("client-id")); + claims.put(IdTokenClaimNames.NONCE, "invalid-nonce-hash"); + Jwt idToken = jwt().claims(c -> c.putAll(claims)).build(); + + when(this.accessTokenResponseClient.getTokenResponse(any())).thenReturn(Mono.just(accessTokenResponse)); + when(this.jwtDecoder.decode(any())).thenReturn(Mono.just(idToken)); + this.manager.setJwtDecoderFactory(c -> this.jwtDecoder); + + assertThatThrownBy(() -> this.manager.authenticate(authorizationCodeAuthentication).block()) + .isInstanceOf(OAuth2AuthenticationException.class) + .hasMessageContaining("[invalid_nonce]"); + } + @Test public void authenticationWhenOAuth2UserNotFoundThenEmpty() { OAuth2AccessTokenResponse accessTokenResponse = OAuth2AccessTokenResponse.withToken("foo") @@ -171,17 +203,20 @@ public class OidcAuthorizationCodeReactiveAuthenticationManagerTests { .additionalParameters(Collections.singletonMap(OidcParameterNames.ID_TOKEN, "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.")) .build(); + OAuth2AuthorizationCodeAuthenticationToken authorizationCodeAuthentication = loginToken(); + Map claims = new HashMap<>(); claims.put(IdTokenClaimNames.ISS, "https://issuer.example.com"); claims.put(IdTokenClaimNames.SUB, "rob"); claims.put(IdTokenClaimNames.AUD, Arrays.asList("client-id")); + claims.put(IdTokenClaimNames.NONCE, this.nonceHash); Jwt idToken = jwt().claims(c -> c.putAll(claims)).build(); when(this.accessTokenResponseClient.getTokenResponse(any())).thenReturn(Mono.just(accessTokenResponse)); when(this.userService.loadUser(any())).thenReturn(Mono.empty()); when(this.jwtDecoder.decode(any())).thenReturn(Mono.just(idToken)); this.manager.setJwtDecoderFactory(c -> this.jwtDecoder); - assertThat(this.manager.authenticate(loginToken()).block()).isNull(); + assertThat(this.manager.authenticate(authorizationCodeAuthentication).block()).isNull(); } @Test @@ -191,10 +226,13 @@ public class OidcAuthorizationCodeReactiveAuthenticationManagerTests { .additionalParameters(Collections.singletonMap(OidcParameterNames.ID_TOKEN, this.idToken.getTokenValue())) .build(); + OAuth2AuthorizationCodeAuthenticationToken authorizationCodeAuthentication = loginToken(); + Map claims = new HashMap<>(); claims.put(IdTokenClaimNames.ISS, "https://issuer.example.com"); claims.put(IdTokenClaimNames.SUB, "rob"); claims.put(IdTokenClaimNames.AUD, Arrays.asList("client-id")); + claims.put(IdTokenClaimNames.NONCE, this.nonceHash); Jwt idToken = jwt().claims(c -> c.putAll(claims)).build(); when(this.accessTokenResponseClient.getTokenResponse(any())).thenReturn(Mono.just(accessTokenResponse)); @@ -203,7 +241,7 @@ public class OidcAuthorizationCodeReactiveAuthenticationManagerTests { when(this.jwtDecoder.decode(any())).thenReturn(Mono.just(idToken)); this.manager.setJwtDecoderFactory(c -> this.jwtDecoder); - OAuth2LoginAuthenticationToken result = (OAuth2LoginAuthenticationToken) this.manager.authenticate(loginToken()).block(); + OAuth2LoginAuthenticationToken result = (OAuth2LoginAuthenticationToken) this.manager.authenticate(authorizationCodeAuthentication).block(); assertThat(result.getPrincipal()).isEqualTo(user); assertThat(result.getAuthorities()).containsOnlyElementsOf(user.getAuthorities()); @@ -218,10 +256,13 @@ public class OidcAuthorizationCodeReactiveAuthenticationManagerTests { .refreshToken("refresh-token") .build(); + OAuth2AuthorizationCodeAuthenticationToken authorizationCodeAuthentication = loginToken(); + Map claims = new HashMap<>(); claims.put(IdTokenClaimNames.ISS, "https://issuer.example.com"); claims.put(IdTokenClaimNames.SUB, "rob"); claims.put(IdTokenClaimNames.AUD, Arrays.asList("client-id")); + claims.put(IdTokenClaimNames.NONCE, this.nonceHash); Jwt idToken = jwt().claims(c -> c.putAll(claims)).build(); when(this.accessTokenResponseClient.getTokenResponse(any())).thenReturn(Mono.just(accessTokenResponse)); @@ -230,7 +271,7 @@ public class OidcAuthorizationCodeReactiveAuthenticationManagerTests { when(this.jwtDecoder.decode(any())).thenReturn(Mono.just(idToken)); this.manager.setJwtDecoderFactory(c -> this.jwtDecoder); - OAuth2LoginAuthenticationToken result = (OAuth2LoginAuthenticationToken) this.manager.authenticate(loginToken()).block(); + OAuth2LoginAuthenticationToken result = (OAuth2LoginAuthenticationToken) this.manager.authenticate(authorizationCodeAuthentication).block(); assertThat(result.getPrincipal()).isEqualTo(user); assertThat(result.getAuthorities()).containsOnlyElementsOf(user.getAuthorities()); @@ -251,10 +292,13 @@ public class OidcAuthorizationCodeReactiveAuthenticationManagerTests { .additionalParameters(additionalParameters) .build(); + OAuth2AuthorizationCodeAuthenticationToken authorizationCodeAuthentication = loginToken(); + Map claims = new HashMap<>(); claims.put(IdTokenClaimNames.ISS, "https://issuer.example.com"); claims.put(IdTokenClaimNames.SUB, "rob"); claims.put(IdTokenClaimNames.AUD, Arrays.asList(clientRegistration.getClientId())); + claims.put(IdTokenClaimNames.NONCE, this.nonceHash); Jwt idToken = jwt().claims(c -> c.putAll(claims)).build(); when(this.accessTokenResponseClient.getTokenResponse(any())).thenReturn(Mono.just(accessTokenResponse)); @@ -264,7 +308,7 @@ public class OidcAuthorizationCodeReactiveAuthenticationManagerTests { when(this.jwtDecoder.decode(any())).thenReturn(Mono.just(idToken)); this.manager.setJwtDecoderFactory(c -> this.jwtDecoder); - this.manager.authenticate(loginToken()).block(); + this.manager.authenticate(authorizationCodeAuthentication).block(); assertThat(userRequestArgCaptor.getValue().getAdditionalParameters()) .containsAllEntriesOf(accessTokenResponse.getAdditionalParameters()); @@ -272,6 +316,14 @@ public class OidcAuthorizationCodeReactiveAuthenticationManagerTests { private OAuth2AuthorizationCodeAuthenticationToken loginToken() { ClientRegistration clientRegistration = this.registration.build(); + Map attributes = new HashMap<>(); + Map additionalParameters = new HashMap<>(); + try { + String nonce = this.secureKeyGenerator.generateKey(); + this.nonceHash = createHash(nonce); + attributes.put(OidcParameterNames.NONCE, nonce); + additionalParameters.put(OidcParameterNames.NONCE, this.nonceHash); + } catch (NoSuchAlgorithmException e) { } OAuth2AuthorizationRequest authorizationRequest = OAuth2AuthorizationRequest .authorizationCode() .state("state") @@ -279,6 +331,8 @@ public class OidcAuthorizationCodeReactiveAuthenticationManagerTests { .authorizationUri(clientRegistration.getProviderDetails().getAuthorizationUri()) .redirectUri(clientRegistration.getRedirectUriTemplate()) .scopes(clientRegistration.getScopes()) + .additionalParameters(additionalParameters) + .attributes(attributes) .build(); OAuth2AuthorizationResponse authorizationResponse = this.authorizationResponseBldr .redirectUri(clientRegistration.getRedirectUriTemplate()) diff --git a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/oidc/authentication/OidcIdTokenDecoderFactoryTests.java b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/oidc/authentication/OidcIdTokenDecoderFactoryTests.java index b2f22f2232..0b26a0d03f 100644 --- a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/oidc/authentication/OidcIdTokenDecoderFactoryTests.java +++ b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/oidc/authentication/OidcIdTokenDecoderFactoryTests.java @@ -59,6 +59,7 @@ public class OidcIdTokenDecoderFactoryTests { Map> claimTypeConverters = OidcIdTokenDecoderFactory.createDefaultClaimTypeConverters(); assertThat(claimTypeConverters).containsKey(IdTokenClaimNames.ISS); assertThat(claimTypeConverters).containsKey(IdTokenClaimNames.AUD); + assertThat(claimTypeConverters).containsKey(IdTokenClaimNames.NONCE); assertThat(claimTypeConverters).containsKey(IdTokenClaimNames.EXP); assertThat(claimTypeConverters).containsKey(IdTokenClaimNames.IAT); assertThat(claimTypeConverters).containsKey(IdTokenClaimNames.AUTH_TIME); diff --git a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/oidc/authentication/ReactiveOidcIdTokenDecoderFactoryTests.java b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/oidc/authentication/ReactiveOidcIdTokenDecoderFactoryTests.java index 388ba79c9a..bb2122a5cc 100644 --- a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/oidc/authentication/ReactiveOidcIdTokenDecoderFactoryTests.java +++ b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/oidc/authentication/ReactiveOidcIdTokenDecoderFactoryTests.java @@ -59,6 +59,7 @@ public class ReactiveOidcIdTokenDecoderFactoryTests { Map> claimTypeConverters = ReactiveOidcIdTokenDecoderFactory.createDefaultClaimTypeConverters(); assertThat(claimTypeConverters).containsKey(IdTokenClaimNames.ISS); assertThat(claimTypeConverters).containsKey(IdTokenClaimNames.AUD); + assertThat(claimTypeConverters).containsKey(IdTokenClaimNames.NONCE); assertThat(claimTypeConverters).containsKey(IdTokenClaimNames.EXP); assertThat(claimTypeConverters).containsKey(IdTokenClaimNames.IAT); assertThat(claimTypeConverters).containsKey(IdTokenClaimNames.AUTH_TIME); diff --git a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/DefaultOAuth2AuthorizationRequestResolverTests.java b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/DefaultOAuth2AuthorizationRequestResolverTests.java index 315f485a3b..1fd1ccb34d 100644 --- a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/DefaultOAuth2AuthorizationRequestResolverTests.java +++ b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/DefaultOAuth2AuthorizationRequestResolverTests.java @@ -24,24 +24,26 @@ import org.springframework.security.oauth2.client.registration.InMemoryClientReg import org.springframework.security.oauth2.client.registration.TestClientRegistrations; import org.springframework.security.oauth2.core.AuthorizationGrantType; import org.springframework.security.oauth2.core.ClientAuthenticationMethod; -import org.springframework.security.oauth2.core.endpoint.*; -import org.springframework.security.oauth2.core.oidc.IdTokenClaimNames; +import org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationRequest; +import org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationResponseType; +import org.springframework.security.oauth2.core.endpoint.OAuth2ParameterNames; +import org.springframework.security.oauth2.core.endpoint.PkceParameterNames; +import org.springframework.security.oauth2.core.oidc.OidcScopes; +import org.springframework.security.oauth2.core.oidc.endpoint.OidcParameterNames; -import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatThrownBy; -import static org.assertj.core.api.Assertions.entry; +import static org.assertj.core.api.Assertions.*; /** * Tests for {@link DefaultOAuth2AuthorizationRequestResolver}. * * @author Joe Grandja - * @author Mark Heckler */ public class DefaultOAuth2AuthorizationRequestResolverTests { private ClientRegistration registration1; private ClientRegistration registration2; private ClientRegistration fineRedirectUriTemplateRegistration; private ClientRegistration pkceRegistration; + private ClientRegistration oidcRegistration; private ClientRegistrationRepository clientRegistrationRepository; private final String authorizationRequestBaseUri = "/oauth2/authorization"; private DefaultOAuth2AuthorizationRequestResolver resolver; @@ -57,9 +59,12 @@ public class DefaultOAuth2AuthorizationRequestResolverTests { .clientAuthenticationMethod(ClientAuthenticationMethod.NONE) .clientSecret(null) .build(); - + this.oidcRegistration = TestClientRegistrations.clientRegistration() + .registrationId("oidc-registration-id") + .scope(OidcScopes.OPENID).build(); this.clientRegistrationRepository = new InMemoryClientRegistrationRepository( - this.registration1, this.registration2, this.fineRedirectUriTemplateRegistration, this.pkceRegistration); + this.registration1, this.registration2, this.fineRedirectUriTemplateRegistration, + this.pkceRegistration, this.oidcRegistration); this.resolver = new DefaultOAuth2AuthorizationRequestResolver( this.clientRegistrationRepository, this.authorizationRequestBaseUri); } @@ -118,14 +123,12 @@ public class DefaultOAuth2AuthorizationRequestResolverTests { assertThat(authorizationRequest.getState()).isNotNull(); assertThat(authorizationRequest.getAdditionalParameters()).doesNotContainKey(OAuth2ParameterNames.REGISTRATION_ID); assertThat(authorizationRequest.getAttributes()) - .containsOnlyKeys(OAuth2ParameterNames.REGISTRATION_ID, IdTokenClaimNames.NONCE) - .contains(entry(OAuth2ParameterNames.REGISTRATION_ID, clientRegistration.getRegistrationId())); + .containsExactly(entry(OAuth2ParameterNames.REGISTRATION_ID, clientRegistration.getRegistrationId())); assertThat(authorizationRequest.getAuthorizationRequestUri()) .matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" + "scope=read:user&state=.{15,}&" + - "redirect_uri=http://localhost/login/oauth2/code/registration-id&" + - "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); + "redirect_uri=http://localhost/login/oauth2/code/registration-id"); } @Test @@ -138,8 +141,7 @@ public class DefaultOAuth2AuthorizationRequestResolverTests { OAuth2AuthorizationRequest authorizationRequest = this.resolver.resolve(request, clientRegistration.getRegistrationId()); assertThat(authorizationRequest).isNotNull(); assertThat(authorizationRequest.getAttributes()) - .containsOnlyKeys(OAuth2ParameterNames.REGISTRATION_ID, IdTokenClaimNames.NONCE) - .contains(entry(OAuth2ParameterNames.REGISTRATION_ID, clientRegistration.getRegistrationId())); + .containsExactly(entry(OAuth2ParameterNames.REGISTRATION_ID, clientRegistration.getRegistrationId())); } @Test @@ -261,8 +263,7 @@ public class DefaultOAuth2AuthorizationRequestResolverTests { .matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" + "scope=read:user&state=.{15,}&" + - "redirect_uri=http://localhost/login/oauth2/code/registration-id&" + - "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); + "redirect_uri=http://localhost/login/oauth2/code/registration-id"); } @Test @@ -280,8 +281,7 @@ public class DefaultOAuth2AuthorizationRequestResolverTests { .matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" + "scope=read:user&state=.{15,}&" + - "redirect_uri=https://example.com/login/oauth2/code/registration-id&" + - "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); + "redirect_uri=https://example.com/login/oauth2/code/registration-id"); } @Test @@ -296,8 +296,7 @@ public class DefaultOAuth2AuthorizationRequestResolverTests { .matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" + "scope=read:user&state=.{15,}&" + - "redirect_uri=http://localhost/authorize/oauth2/code/registration-id&" + - "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); + "redirect_uri=http://localhost/authorize/oauth2/code/registration-id"); } @Test @@ -312,8 +311,7 @@ public class DefaultOAuth2AuthorizationRequestResolverTests { .matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id-2&" + "scope=read:user&state=.{15,}&" + - "redirect_uri=http://localhost/login/oauth2/code/registration-id-2&" + - "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); + "redirect_uri=http://localhost/login/oauth2/code/registration-id-2"); } @Test @@ -329,8 +327,7 @@ public class DefaultOAuth2AuthorizationRequestResolverTests { .matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" + "scope=read:user&state=.{15,}&" + - "redirect_uri=http://localhost/authorize/oauth2/code/registration-id&" + - "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); + "redirect_uri=http://localhost/authorize/oauth2/code/registration-id"); } @Test @@ -346,8 +343,7 @@ public class DefaultOAuth2AuthorizationRequestResolverTests { .matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id-2&" + "scope=read:user&state=.{15,}&" + - "redirect_uri=http://localhost/login/oauth2/code/registration-id-2&" + - "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); + "redirect_uri=http://localhost/login/oauth2/code/registration-id-2"); } @Test @@ -383,10 +379,41 @@ public class DefaultOAuth2AuthorizationRequestResolverTests { "scope=read:user&state=.{15,}&" + "redirect_uri=http://localhost/login/oauth2/code/pkce-client-registration-id&" + "code_challenge_method=S256&" + - "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}&" + "code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); } + @Test + public void resolveWhenAuthenticationRequestWithValidOidcClientThenResolves() { + ClientRegistration clientRegistration = this.oidcRegistration; + String requestUri = this.authorizationRequestBaseUri + "/" + clientRegistration.getRegistrationId(); + MockHttpServletRequest request = new MockHttpServletRequest("GET", requestUri); + request.setServletPath(requestUri); + + OAuth2AuthorizationRequest authorizationRequest = this.resolver.resolve(request); + assertThat(authorizationRequest).isNotNull(); + assertThat(authorizationRequest.getAuthorizationUri()).isEqualTo( + clientRegistration.getProviderDetails().getAuthorizationUri()); + assertThat(authorizationRequest.getGrantType()).isEqualTo(AuthorizationGrantType.AUTHORIZATION_CODE); + assertThat(authorizationRequest.getResponseType()).isEqualTo(OAuth2AuthorizationResponseType.CODE); + assertThat(authorizationRequest.getClientId()).isEqualTo(clientRegistration.getClientId()); + assertThat(authorizationRequest.getRedirectUri()) + .isEqualTo("http://localhost/login/oauth2/code/" + clientRegistration.getRegistrationId()); + assertThat(authorizationRequest.getScopes()).isEqualTo(clientRegistration.getScopes()); + assertThat(authorizationRequest.getState()).isNotNull(); + assertThat(authorizationRequest.getAdditionalParameters()).doesNotContainKey(OAuth2ParameterNames.REGISTRATION_ID); + assertThat(authorizationRequest.getAdditionalParameters()).containsKey(OidcParameterNames.NONCE); + assertThat(authorizationRequest.getAttributes()) + .contains(entry(OAuth2ParameterNames.REGISTRATION_ID, clientRegistration.getRegistrationId())); + assertThat(authorizationRequest.getAttributes()).containsKey(OidcParameterNames.NONCE); + assertThat((String) authorizationRequest.getAttribute(OidcParameterNames.NONCE)).matches("^([a-zA-Z0-9\\-\\.\\_\\~]){128}$"); + assertThat(authorizationRequest.getAuthorizationRequestUri()) + .matches("https://example.com/login/oauth/authorize\\?" + + "response_type=code&client_id=client-id&" + + "scope=openid&state=.{15,}&" + + "redirect_uri=http://localhost/login/oauth2/code/oidc-registration-id&" + + "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); + } + private static ClientRegistration.Builder fineRedirectUriTemplateClientRegistration() { return ClientRegistration.withRegistrationId("fine-redirect-uri-template-client-registration") .redirectUriTemplate("{baseScheme}://{baseHost}{basePort}{basePath}/{action}/oauth2/code/{registrationId}") diff --git a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/OAuth2AuthorizationRequestRedirectFilterTests.java b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/OAuth2AuthorizationRequestRedirectFilterTests.java index 4343ec9a00..bbc47c9a56 100644 --- a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/OAuth2AuthorizationRequestRedirectFilterTests.java +++ b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/OAuth2AuthorizationRequestRedirectFilterTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-2018 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -49,7 +49,6 @@ import static org.mockito.Mockito.*; * Tests for {@link OAuth2AuthorizationRequestRedirectFilter}. * * @author Joe Grandja - * @author Mark Heckler */ public class OAuth2AuthorizationRequestRedirectFilterTests { private ClientRegistration registration1; @@ -155,8 +154,7 @@ public class OAuth2AuthorizationRequestRedirectFilterTests { assertThat(response.getRedirectedUrl()).matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" + "scope=read:user&state=.{15,}&" + - "redirect_uri=http://localhost/login/oauth2/code/registration-id&" + - "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); + "redirect_uri=http://localhost/login/oauth2/code/registration-id"); } @Test @@ -236,8 +234,7 @@ public class OAuth2AuthorizationRequestRedirectFilterTests { assertThat(response.getRedirectedUrl()).matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" + "scope=read:user&state=.{15,}&" + - "redirect_uri=http://localhost/login/oauth2/code/registration-id&" + - "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); + "redirect_uri=http://localhost/login/oauth2/code/registration-id"); } @Test @@ -258,8 +255,7 @@ public class OAuth2AuthorizationRequestRedirectFilterTests { assertThat(response.getRedirectedUrl()).matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" + "scope=read:user&state=.{15,}&" + - "redirect_uri=http://localhost/authorize/oauth2/code/registration-id&" + - "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); + "redirect_uri=http://localhost/authorize/oauth2/code/registration-id"); verify(this.requestCache).saveRequest(any(HttpServletRequest.class), any(HttpServletResponse.class)); } @@ -363,7 +359,6 @@ public class OAuth2AuthorizationRequestRedirectFilterTests { "response_type=code&client_id=client-id&" + "scope=read:user&state=.{15,}&" + "redirect_uri=http://localhost/login/oauth2/code/registration-id&" + - "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}&" + "login_hint=user@provider\\.com"); } } diff --git a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/server/DefaultServerOAuth2AuthorizationRequestResolverTests.java b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/server/DefaultServerOAuth2AuthorizationRequestResolverTests.java index 60a797ea30..0febd5025d 100644 --- a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/server/DefaultServerOAuth2AuthorizationRequestResolverTests.java +++ b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/server/DefaultServerOAuth2AuthorizationRequestResolverTests.java @@ -30,6 +30,8 @@ import org.springframework.security.oauth2.client.registration.TestClientRegistr import org.springframework.security.oauth2.core.ClientAuthenticationMethod; import org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationRequest; import org.springframework.security.oauth2.core.endpoint.PkceParameterNames; +import org.springframework.security.oauth2.core.oidc.OidcScopes; +import org.springframework.security.oauth2.core.oidc.endpoint.OidcParameterNames; import org.springframework.web.server.ResponseStatusException; import org.springframework.web.server.ServerWebExchange; import reactor.core.publisher.Mono; @@ -41,7 +43,6 @@ import static org.mockito.Mockito.when; /** * @author Rob Winch - * @author Mark Heckler * @since 5.1 */ @RunWith(MockitoJUnitRunner.class) @@ -83,13 +84,7 @@ public class DefaultServerOAuth2AuthorizationRequestResolverTests { assertThat(request.getAuthorizationRequestUri()).matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" + "scope=read:user&state=.*?&" + - "redirect_uri=/login/oauth2/code/registration-id&" + - "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); - } - - private OAuth2AuthorizationRequest resolve(String path) { - ServerWebExchange exchange = MockServerWebExchange.from(MockServerHttpRequest.get(path)); - return this.resolver.resolve(exchange).block(); + "redirect_uri=/login/oauth2/code/registration-id"); } @Test @@ -103,8 +98,7 @@ public class DefaultServerOAuth2AuthorizationRequestResolverTests { assertThat(request.getAuthorizationRequestUri()).matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" + "scope=read:user&state=.*?&" + - "redirect_uri=/login/oauth2/code/registration-id&" + - "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); + "redirect_uri=/login/oauth2/code/registration-id"); } @Test @@ -124,7 +118,29 @@ public class DefaultServerOAuth2AuthorizationRequestResolverTests { "scope=read:user&state=.*?&" + "redirect_uri=/login/oauth2/code/registration-id&" + "code_challenge_method=S256&" + - "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}&" + "code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); } + + @Test + public void resolveWhenAuthenticationRequestWithValidOidcClientThenResolves() { + when(this.clientRegistrationRepository.findByRegistrationId(any())).thenReturn( + Mono.just(TestClientRegistrations.clientRegistration() + .scope(OidcScopes.OPENID) + .build())); + + OAuth2AuthorizationRequest request = resolve("/oauth2/authorization/registration-id"); + + assertThat((String) request.getAttribute(OidcParameterNames.NONCE)).matches("^([a-zA-Z0-9\\-\\.\\_\\~]){128}$"); + + assertThat(request.getAuthorizationRequestUri()).matches("https://example.com/login/oauth/authorize\\?" + + "response_type=code&client_id=client-id&" + + "scope=openid&state=.*?&" + + "redirect_uri=/login/oauth2/code/registration-id&" + + "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); + } + + private OAuth2AuthorizationRequest resolve(String path) { + ServerWebExchange exchange = MockServerWebExchange.from(MockServerHttpRequest.get(path)); + return this.resolver.resolve(exchange).block(); + } } diff --git a/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/oidc/endpoint/OidcParameterNames.java b/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/oidc/endpoint/OidcParameterNames.java index 77f5dbd7e9..827c4557b2 100644 --- a/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/oidc/endpoint/OidcParameterNames.java +++ b/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/oidc/endpoint/OidcParameterNames.java @@ -32,7 +32,7 @@ public interface OidcParameterNames { String ID_TOKEN = "id_token"; /** - * {@code nonce} - used in the Access Token Request and Response. + * {@code nonce} - used in the Authentication Request. */ String NONCE = "nonce";