From 0735abdaad4d02c145b63a6ee8c2ffde24107a42 Mon Sep 17 00:00:00 2001 From: Joe Grandja Date: Thu, 23 Sep 2021 08:25:58 -0400 Subject: [PATCH] Polish gh-411 --- .../OAuth2AuthorizationEndpointFilter.java | 2 +- .../OAuth2AuthorizationCodeGrantTests.java | 2 +- ...Auth2AuthorizationEndpointFilterTests.java | 2 +- .../config/AuthorizationServerConfig.java | 3 +- ...uthorizationServerCustomConsentTests.java} | 54 +++++++++---------- ...OAuth2AuthorizationServerConsentTests.java | 39 ++++++++------ 6 files changed, 52 insertions(+), 50 deletions(-) rename samples/boot/oauth2-integration/authorizationserver-custom-consent-page/src/test/java/sample/{OAuth2AuthorizationServerCustomConsentPageApplicationTests.java => OAuth2AuthorizationServerCustomConsentTests.java} (66%) diff --git a/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/web/OAuth2AuthorizationEndpointFilter.java b/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/web/OAuth2AuthorizationEndpointFilter.java index 1790d8fd..3efb79fd 100644 --- a/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/web/OAuth2AuthorizationEndpointFilter.java +++ b/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/web/OAuth2AuthorizationEndpointFilter.java @@ -363,7 +363,7 @@ public final class OAuth2AuthorizationEndpointFilter extends OncePerRequestFilte for (String scope : scopesToAuthorize) { builder.append("
"); - builder.append(" "); + builder.append(" "); builder.append(" "); builder.append("
"); } diff --git a/oauth2-authorization-server/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/server/authorization/OAuth2AuthorizationCodeGrantTests.java b/oauth2-authorization-server/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/server/authorization/OAuth2AuthorizationCodeGrantTests.java index 760847d8..7c4b3e8c 100644 --- a/oauth2-authorization-server/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/server/authorization/OAuth2AuthorizationCodeGrantTests.java +++ b/oauth2-authorization-server/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/server/authorization/OAuth2AuthorizationCodeGrantTests.java @@ -557,7 +557,7 @@ public class OAuth2AuthorizationCodeGrantTests { private static String scopeCheckbox(String scope) { return MessageFormat.format( - "", + "", scope ); } diff --git a/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/web/OAuth2AuthorizationEndpointFilterTests.java b/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/web/OAuth2AuthorizationEndpointFilterTests.java index 26304eeb..f8f3cf09 100644 --- a/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/web/OAuth2AuthorizationEndpointFilterTests.java +++ b/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/web/OAuth2AuthorizationEndpointFilterTests.java @@ -575,7 +575,7 @@ public class OAuth2AuthorizationEndpointFilterTests { private static String scopeCheckbox(String scope) { return MessageFormat.format( - "", + "", scope ); } diff --git a/samples/boot/oauth2-integration/authorizationserver-custom-consent-page/src/main/java/sample/config/AuthorizationServerConfig.java b/samples/boot/oauth2-integration/authorizationserver-custom-consent-page/src/main/java/sample/config/AuthorizationServerConfig.java index f4f27ccb..d7064bb4 100644 --- a/samples/boot/oauth2-integration/authorizationserver-custom-consent-page/src/main/java/sample/config/AuthorizationServerConfig.java +++ b/samples/boot/oauth2-integration/authorizationserver-custom-consent-page/src/main/java/sample/config/AuthorizationServerConfig.java @@ -21,6 +21,8 @@ import com.nimbusds.jose.jwk.JWKSet; import com.nimbusds.jose.jwk.RSAKey; import com.nimbusds.jose.jwk.source.JWKSource; import com.nimbusds.jose.proc.SecurityContext; +import sample.jose.Jwks; + import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.core.Ordered; @@ -40,7 +42,6 @@ import org.springframework.security.oauth2.server.authorization.config.ClientSet import org.springframework.security.oauth2.server.authorization.config.ProviderSettings; import org.springframework.security.web.SecurityFilterChain; import org.springframework.security.web.util.matcher.RequestMatcher; -import sample.jose.Jwks; /** * @author Joe Grandja diff --git a/samples/boot/oauth2-integration/authorizationserver-custom-consent-page/src/test/java/sample/OAuth2AuthorizationServerCustomConsentPageApplicationTests.java b/samples/boot/oauth2-integration/authorizationserver-custom-consent-page/src/test/java/sample/OAuth2AuthorizationServerCustomConsentTests.java similarity index 66% rename from samples/boot/oauth2-integration/authorizationserver-custom-consent-page/src/test/java/sample/OAuth2AuthorizationServerCustomConsentPageApplicationTests.java rename to samples/boot/oauth2-integration/authorizationserver-custom-consent-page/src/test/java/sample/OAuth2AuthorizationServerCustomConsentTests.java index 55bb805e..f9905950 100644 --- a/samples/boot/oauth2-integration/authorizationserver-custom-consent-page/src/test/java/sample/OAuth2AuthorizationServerCustomConsentPageApplicationTests.java +++ b/samples/boot/oauth2-integration/authorizationserver-custom-consent-page/src/test/java/sample/OAuth2AuthorizationServerCustomConsentTests.java @@ -16,8 +16,8 @@ package sample; import java.io.IOException; +import java.util.ArrayList; import java.util.List; -import java.util.stream.Collectors; import com.gargoylesoftware.htmlunit.WebClient; import com.gargoylesoftware.htmlunit.WebResponse; @@ -27,17 +27,13 @@ import com.gargoylesoftware.htmlunit.html.HtmlPage; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; + import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc; import org.springframework.boot.test.context.SpringBootTest; -import org.springframework.boot.test.context.TestConfiguration; import org.springframework.boot.test.mock.mockito.MockBean; -import org.springframework.context.annotation.Bean; -import org.springframework.context.annotation.Import; import org.springframework.http.HttpStatus; -import org.springframework.security.oauth2.server.authorization.InMemoryOAuth2AuthorizationService; import org.springframework.security.oauth2.server.authorization.OAuth2AuthorizationConsentService; -import org.springframework.security.oauth2.server.authorization.OAuth2AuthorizationService; import org.springframework.security.test.context.support.WithMockUser; import org.springframework.test.context.junit4.SpringRunner; import org.springframework.web.util.UriComponentsBuilder; @@ -47,30 +43,30 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.when; /** - * Consent page integration tests for the sample Authorization Server serving a custom Consent page + * Consent page integration tests for the sample Authorization Server serving a custom Consent page. * * @author Dmitriy Dubson */ @RunWith(SpringRunner.class) @SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT) -@Import({OAuth2AuthorizationServerCustomConsentPageApplicationTests.InMemoryOAuth2AuthorizationServiceTestConfiguration.class}) @AutoConfigureMockMvc -public class OAuth2AuthorizationServerCustomConsentPageApplicationTests { +public class OAuth2AuthorizationServerCustomConsentTests { + @Autowired private WebClient webClient; @MockBean - private OAuth2AuthorizationConsentService mockAuthorizationConsentService; + private OAuth2AuthorizationConsentService authorizationConsentService; - private final String testConsentResultEndpoint = "http://127.0.0.1/login/oauth2/code/messaging-client-oidc"; + private final String redirectUri = "http://127.0.0.1/login/oauth2/code/messaging-client-oidc"; - private final String authorizeEndpoint = UriComponentsBuilder + private final String authorizationRequestUri = UriComponentsBuilder .fromPath("/oauth2/authorize") .queryParam("response_type", "code") .queryParam("client_id", "messaging-client") .queryParam("scope", "openid message.read message.write") .queryParam("state", "state") - .queryParam("redirect_uri", testConsentResultEndpoint) + .queryParam("redirect_uri", this.redirectUri) .toUriString(); @Before @@ -78,24 +74,28 @@ public class OAuth2AuthorizationServerCustomConsentPageApplicationTests { this.webClient.getOptions().setThrowExceptionOnFailingStatusCode(false); this.webClient.getOptions().setRedirectEnabled(true); this.webClient.getCookieManager().clearCookies(); - when(mockAuthorizationConsentService.findById(any(), any())).thenReturn(null); + when(this.authorizationConsentService.findById(any(), any())).thenReturn(null); } @Test @WithMockUser("user1") public void whenUserConsentsToAllScopesThenReturnAuthorizationCode() throws IOException { - final HtmlPage consentPage = webClient.getPage(authorizeEndpoint); + final HtmlPage consentPage = this.webClient.getPage(this.authorizationRequestUri); assertThat(consentPage.getTitleText()).isEqualTo("Custom consent page - Consent required"); - List scopes = consentPage.querySelectorAll("input[name='scope']").stream() - .map(scope -> (HtmlCheckBoxInput) scope).collect(Collectors.toList()); + List scopes = new ArrayList<>(); + consentPage.querySelectorAll("input[name='scope']").forEach(scope -> + scopes.add((HtmlCheckBoxInput) scope)); for (HtmlCheckBoxInput scope : scopes) { scope.click(); } - assertThat(scopes.stream().map(DomElement::getId).collect(Collectors.toList())) - .containsExactlyInAnyOrder("openid", "message.read", "message.write"); - assertThat(scopes.stream().allMatch(HtmlCheckBoxInput::isChecked)).isTrue(); + List scopeIds = new ArrayList<>(); + scopes.forEach(scope -> { + assertThat(scope.isChecked()).isTrue(); + scopeIds.add(scope.getId()); + }); + assertThat(scopeIds).containsExactlyInAnyOrder("openid", "message.read", "message.write"); DomElement submitConsentButton = consentPage.querySelector("button[id='submit-consent']"); this.webClient.getOptions().setRedirectEnabled(false); @@ -103,14 +103,14 @@ public class OAuth2AuthorizationServerCustomConsentPageApplicationTests { WebResponse approveConsentResponse = submitConsentButton.click().getWebResponse(); assertThat(approveConsentResponse.getStatusCode()).isEqualTo(HttpStatus.MOVED_PERMANENTLY.value()); String location = approveConsentResponse.getResponseHeaderValue("location"); - assertThat(location).startsWith(testConsentResultEndpoint); + assertThat(location).startsWith(this.redirectUri); assertThat(location).contains("code="); } @Test @WithMockUser("user1") - public void whenUserCancelsApprovingConsentThenReturnAnAccessDeniedError() throws IOException { - final HtmlPage consentPage = webClient.getPage(authorizeEndpoint); + public void whenUserCancelsConsentThenReturnAccessDeniedError() throws IOException { + final HtmlPage consentPage = this.webClient.getPage(this.authorizationRequestUri); assertThat(consentPage.getTitleText()).isEqualTo("Custom consent page - Consent required"); DomElement cancelConsentButton = consentPage.querySelector("button[id='cancel-consent']"); @@ -119,14 +119,8 @@ public class OAuth2AuthorizationServerCustomConsentPageApplicationTests { WebResponse cancelConsentResponse = cancelConsentButton.click().getWebResponse(); assertThat(cancelConsentResponse.getStatusCode()).isEqualTo(HttpStatus.MOVED_PERMANENTLY.value()); String location = cancelConsentResponse.getResponseHeaderValue("location"); + assertThat(location).startsWith(this.redirectUri); assertThat(location).contains("error=access_denied"); } - @TestConfiguration - static class InMemoryOAuth2AuthorizationServiceTestConfiguration { - @Bean - public OAuth2AuthorizationService authorizationService() { - return new InMemoryOAuth2AuthorizationService(); - } - } } diff --git a/samples/boot/oauth2-integration/authorizationserver/src/test/java/sample/OAuth2AuthorizationServerConsentTests.java b/samples/boot/oauth2-integration/authorizationserver/src/test/java/sample/OAuth2AuthorizationServerConsentTests.java index 7e90d057..9099cbba 100644 --- a/samples/boot/oauth2-integration/authorizationserver/src/test/java/sample/OAuth2AuthorizationServerConsentTests.java +++ b/samples/boot/oauth2-integration/authorizationserver/src/test/java/sample/OAuth2AuthorizationServerConsentTests.java @@ -16,8 +16,8 @@ package sample; import java.io.IOException; +import java.util.ArrayList; import java.util.List; -import java.util.stream.Collectors; import com.gargoylesoftware.htmlunit.WebClient; import com.gargoylesoftware.htmlunit.WebResponse; @@ -27,6 +27,7 @@ import com.gargoylesoftware.htmlunit.html.HtmlPage; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; + import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc; import org.springframework.boot.test.context.SpringBootTest; @@ -42,7 +43,7 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.when; /** - * Consent screen integration tests for the sample Authorization Server + * Consent screen integration tests for the sample Authorization Server. * * @author Dmitriy Dubson */ @@ -50,21 +51,22 @@ import static org.mockito.Mockito.when; @SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT) @AutoConfigureMockMvc public class OAuth2AuthorizationServerConsentTests { + @Autowired private WebClient webClient; @MockBean - private OAuth2AuthorizationConsentService mockAuthorizationConsentService; + private OAuth2AuthorizationConsentService authorizationConsentService; - private final String testConsentResultEndpoint = "http://127.0.0.1/login/oauth2/code/messaging-client-oidc"; + private final String redirectUri = "http://127.0.0.1/login/oauth2/code/messaging-client-oidc"; - private final String authorizeEndpoint = UriComponentsBuilder + private final String authorizationRequestUri = UriComponentsBuilder .fromPath("/oauth2/authorize") .queryParam("response_type", "code") .queryParam("client_id", "messaging-client") .queryParam("scope", "openid message.read message.write") .queryParam("state", "state") - .queryParam("redirect_uri", testConsentResultEndpoint) + .queryParam("redirect_uri", this.redirectUri) .toUriString(); @Before @@ -72,24 +74,28 @@ public class OAuth2AuthorizationServerConsentTests { this.webClient.getOptions().setThrowExceptionOnFailingStatusCode(false); this.webClient.getOptions().setRedirectEnabled(true); this.webClient.getCookieManager().clearCookies(); - when(mockAuthorizationConsentService.findById(any(), any())).thenReturn(null); + when(this.authorizationConsentService.findById(any(), any())).thenReturn(null); } @Test @WithMockUser("user1") public void whenUserConsentsToAllScopesThenReturnAuthorizationCode() throws IOException { - final HtmlPage consentPage = webClient.getPage(authorizeEndpoint); + final HtmlPage consentPage = this.webClient.getPage(this.authorizationRequestUri); assertThat(consentPage.getTitleText()).isEqualTo("Consent required"); - List scopes = consentPage.querySelectorAll("input[name='scope']").stream() - .map(scope -> (HtmlCheckBoxInput) scope).collect(Collectors.toList()); + List scopes = new ArrayList<>(); + consentPage.querySelectorAll("input[name='scope']").forEach(scope -> + scopes.add((HtmlCheckBoxInput) scope)); for (HtmlCheckBoxInput scope : scopes) { scope.click(); } - assertThat(scopes.stream().map(DomElement::getId).collect(Collectors.toList())) - .containsExactlyInAnyOrder("message.read", "message.write"); - assertThat(scopes.stream().allMatch(HtmlCheckBoxInput::isChecked)).isTrue(); + List scopeIds = new ArrayList<>(); + scopes.forEach(scope -> { + assertThat(scope.isChecked()).isTrue(); + scopeIds.add(scope.getId()); + }); + assertThat(scopeIds).containsExactlyInAnyOrder("message.read", "message.write"); DomElement submitConsentButton = consentPage.querySelector("button[id='submit-consent']"); this.webClient.getOptions().setRedirectEnabled(false); @@ -97,14 +103,14 @@ public class OAuth2AuthorizationServerConsentTests { WebResponse approveConsentResponse = submitConsentButton.click().getWebResponse(); assertThat(approveConsentResponse.getStatusCode()).isEqualTo(HttpStatus.MOVED_PERMANENTLY.value()); String location = approveConsentResponse.getResponseHeaderValue("location"); - assertThat(location).startsWith(testConsentResultEndpoint); + assertThat(location).startsWith(this.redirectUri); assertThat(location).contains("code="); } @Test @WithMockUser("user1") - public void whenUserCancelsApprovingConsentThenReturnAnAccessDeniedError() throws IOException { - final HtmlPage consentPage = webClient.getPage(authorizeEndpoint); + public void whenUserCancelsConsentThenReturnAccessDeniedError() throws IOException { + final HtmlPage consentPage = this.webClient.getPage(this.authorizationRequestUri); assertThat(consentPage.getTitleText()).isEqualTo("Consent required"); DomElement cancelConsentButton = consentPage.querySelector("button[id='cancel-consent']"); @@ -113,6 +119,7 @@ public class OAuth2AuthorizationServerConsentTests { WebResponse cancelConsentResponse = cancelConsentButton.click().getWebResponse(); assertThat(cancelConsentResponse.getStatusCode()).isEqualTo(HttpStatus.MOVED_PERMANENTLY.value()); String location = cancelConsentResponse.getResponseHeaderValue("location"); + assertThat(location).startsWith(this.redirectUri); assertThat(location).contains("error=access_denied"); }