Skip to content

Commit d9d161c

Browse files
mbhavembhave
committed
Allow previously authorized users to access the error page
Prior to this commit, the `ErrorPageSecurityFilter` verified if access to the error page was allowed by invoking the `WebInvocationPrivilegeEvaluator` with the Authentication from the `SecurityContextHolder`. This meant that access to the error page was denied for a `null` Authentication or `AnonymousAuthenticationToken` in cases where the error page required authenticated access. This prevented authorized users from accessing the error page in case the Authentication wasn't retrievable for the error dispatch, which is the case for `@Transient` authentication or stateless session policy. This commit updates the `ErrorPageSecurityFilter` to check access to the error page only if the error is an authn or authz error in cases where an authentication object is not found in the SecurityContextHolder. This makes the error response consistent when bad credentials or no credentials are used while also allowing access to previously authorized users. Fixes gh-28953
1 parent c077ebe commit d9d161c

File tree

9 files changed

+381
-99
lines changed

9 files changed

+381
-99
lines changed

‎spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/servlet/filter/ErrorPageSecurityFilter.java

+17-10
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131

3232
import org.springframework.beans.factory.NoSuchBeanDefinitionException;
3333
import org.springframework.context.ApplicationContext;
34+
import org.springframework.security.authentication.AnonymousAuthenticationToken;
3435
import org.springframework.security.core.Authentication;
3536
import org.springframework.security.core.context.SecurityContextHolder;
3637
import org.springframework.security.web.access.WebInvocationPrivilegeEvaluator;
@@ -67,17 +68,28 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
6768

6869
private void doFilter(HttpServletRequest request, HttpServletResponse response, FilterChain chain)
6970
throws IOException, ServletException {
70-
if (DispatcherType.ERROR.equals(request.getDispatcherType()) && !isAllowed(request)) {
71-
sendError(request, response);
71+
Integer errorCode = (Integer) request.getAttribute(RequestDispatcher.ERROR_STATUS_CODE);
72+
if (DispatcherType.ERROR.equals(request.getDispatcherType()) && !isAllowed(request, errorCode)) {
73+
response.sendError((errorCode != null) ? errorCode : 401);
7274
return;
7375
}
7476
chain.doFilter(request, response);
7577
}
7678

77-
private boolean isAllowed(HttpServletRequest request) {
78-
String uri = request.getRequestURI();
79+
private boolean isAllowed(HttpServletRequest request, Integer errorCode) {
7980
Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
80-
return getPrivilegeEvaluator().isAllowed(uri, authentication);
81+
if (isUnauthenticated(authentication) && isNotAuthenticationError(errorCode)) {
82+
return true;
83+
}
84+
return getPrivilegeEvaluator().isAllowed(request.getRequestURI(), authentication);
85+
}
86+
87+
private boolean isUnauthenticated(Authentication authentication) {
88+
return (authentication == null || authentication instanceof AnonymousAuthenticationToken);
89+
}
90+
91+
private boolean isNotAuthenticationError(Integer errorCode) {
92+
return (errorCode == null || (errorCode != 401 && errorCode != 403));
8193
}
8294

8395
private WebInvocationPrivilegeEvaluator getPrivilegeEvaluator() {
@@ -98,11 +110,6 @@ private WebInvocationPrivilegeEvaluator getPrivilegeEvaluatorBean() {
98110
}
99111
}
100112

101-
private void sendError(HttpServletRequest request, HttpServletResponse response) throws IOException {
102-
Integer errorCode = (Integer) request.getAttribute(RequestDispatcher.ERROR_STATUS_CODE);
103-
response.sendError((errorCode != null) ? errorCode : 401);
104-
}
105-
106113
/**
107114
* {@link WebInvocationPrivilegeEvaluator} that always allows access.
108115
*/

‎spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/filter/ErrorPageSecurityFilterTests.java

+12
Original file line numberDiff line numberDiff line change
@@ -20,13 +20,17 @@
2020
import javax.servlet.FilterChain;
2121
import javax.servlet.RequestDispatcher;
2222

23+
import org.junit.jupiter.api.AfterEach;
2324
import org.junit.jupiter.api.BeforeEach;
2425
import org.junit.jupiter.api.Test;
2526

2627
import org.springframework.beans.factory.NoSuchBeanDefinitionException;
2728
import org.springframework.context.ApplicationContext;
2829
import org.springframework.mock.web.MockHttpServletRequest;
2930
import org.springframework.mock.web.MockHttpServletResponse;
31+
import org.springframework.security.core.Authentication;
32+
import org.springframework.security.core.context.SecurityContext;
33+
import org.springframework.security.core.context.SecurityContextHolder;
3034
import org.springframework.security.web.access.WebInvocationPrivilegeEvaluator;
3135

3236
import static org.assertj.core.api.Assertions.assertThat;
@@ -64,6 +68,11 @@ void setup() {
6468
this.securityFilter = new ErrorPageSecurityFilter(this.context);
6569
}
6670

71+
@AfterEach
72+
void tearDown() {
73+
SecurityContextHolder.clearContext();
74+
}
75+
6776
@Test
6877
void whenAccessIsAllowedShouldContinueDownFilterChain() throws Exception {
6978
given(this.privilegeEvaluator.isAllowed(anyString(), any())).willReturn(true);
@@ -83,6 +92,9 @@ void whenAccessIsDeniedShouldCallSendError() throws Exception {
8392
@Test
8493
void whenAccessIsDeniedAndNoErrorCodeAttributeOnRequest() throws Exception {
8594
given(this.privilegeEvaluator.isAllowed(anyString(), any())).willReturn(false);
95+
SecurityContext securityContext = mock(SecurityContext.class);
96+
SecurityContextHolder.setContext(securityContext);
97+
given(securityContext.getAuthentication()).willReturn(mock(Authentication.class));
8698
this.securityFilter.doFilter(this.request, this.response, this.filterChain);
8799
verifyNoInteractions(this.filterChain);
88100
assertThat(this.response.getStatus()).isEqualTo(401);

‎spring-boot-tests/spring-boot-smoke-tests/spring-boot-smoke-test-actuator-custom-security/src/test/java/smoketest/actuator/customsecurity/SampleActuatorCustomSecurityApplicationTests.java

+2-1
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,8 @@ Environment getEnvironment() {
6666
void testInsecureApplicationPath() {
6767
ResponseEntity<Map> entity = restTemplate().getForEntity(getPath() + "/foo", Map.class);
6868
assertThat(entity.getStatusCode()).isEqualTo(HttpStatus.INTERNAL_SERVER_ERROR);
69-
assertThat(entity.getBody()).isNull();
69+
Map<String, Object> body = entity.getBody();
70+
assertThat((String) body.get("message")).contains("Expected exception in controller");
7071
}
7172

7273
@Test

‎spring-boot-tests/spring-boot-smoke-tests/spring-boot-smoke-test-web-secure/src/main/java/smoketest/web/secure/SampleWebSecureApplication.java

-21
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,6 @@
1818

1919
import org.springframework.boot.autoconfigure.SpringBootApplication;
2020
import org.springframework.boot.builder.SpringApplicationBuilder;
21-
import org.springframework.context.annotation.Bean;
22-
import org.springframework.context.annotation.Configuration;
23-
import org.springframework.security.config.annotation.web.builders.HttpSecurity;
24-
import org.springframework.security.web.SecurityFilterChain;
2521
import org.springframework.web.servlet.config.annotation.ViewControllerRegistry;
2622
import org.springframework.web.servlet.config.annotation.WebMvcConfigurer;
2723

@@ -38,21 +34,4 @@ public static void main(String[] args) {
3834
new SpringApplicationBuilder(SampleWebSecureApplication.class).run(args);
3935
}
4036

41-
@Configuration(proxyBeanMethods = false)
42-
protected static class ApplicationSecurity {
43-
44-
@Bean
45-
SecurityFilterChain configure(HttpSecurity http) throws Exception {
46-
http.csrf().disable();
47-
http.authorizeRequests((requests) -> {
48-
requests.antMatchers("/public/**").permitAll();
49-
requests.anyRequest().fullyAuthenticated();
50-
});
51-
http.httpBasic();
52-
http.formLogin((form) -> form.loginPage("/login").permitAll());
53-
return http.build();
54-
}
55-
56-
}
57-
5837
}

‎spring-boot-tests/spring-boot-smoke-tests/spring-boot-smoke-test-web-secure/src/test/java/smoketest/web/secure/AbstractErrorPageTests.java

+127
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
/*
2+
* Copyright 2012-2021 the original author or authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* https://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package smoketest.web.secure;
18+
19+
import com.fasterxml.jackson.databind.JsonNode;
20+
import org.junit.jupiter.api.Test;
21+
22+
import org.springframework.beans.factory.annotation.Autowired;
23+
import org.springframework.boot.test.web.client.TestRestTemplate;
24+
import org.springframework.context.annotation.Configuration;
25+
import org.springframework.http.HttpMethod;
26+
import org.springframework.http.HttpStatus;
27+
import org.springframework.http.ResponseEntity;
28+
import org.springframework.web.bind.annotation.GetMapping;
29+
import org.springframework.web.bind.annotation.RestController;
30+
31+
import static org.assertj.core.api.Assertions.assertThat;
32+
33+
/**
34+
* Abstract base class for tests to ensure that the error page is accessible only to
35+
* authorized users.
36+
*
37+
* @author Madhura Bhave
38+
*/
39+
abstract class AbstractErrorPageTests {
40+
41+
@Autowired
42+
private TestRestTemplate testRestTemplate;
43+
44+
@Test
45+
void testBadCredentials() {
46+
final ResponseEntity<JsonNode> response = this.testRestTemplate.withBasicAuth("username", "wrongpassword")
47+
.exchange("/test", HttpMethod.GET, null, JsonNode.class);
48+
assertThat(response.getStatusCode()).isEqualTo(HttpStatus.UNAUTHORIZED);
49+
JsonNode jsonResponse = response.getBody();
50+
assertThat(jsonResponse).isNull();
51+
}
52+
53+
@Test
54+
void testNoCredentials() {
55+
final ResponseEntity<JsonNode> response = this.testRestTemplate.exchange("/test", HttpMethod.GET, null,
56+
JsonNode.class);
57+
assertThat(response.getStatusCode()).isEqualTo(HttpStatus.UNAUTHORIZED);
58+
JsonNode jsonResponse = response.getBody();
59+
assertThat(jsonResponse).isNull();
60+
}
61+
62+
@Test
63+
void testPublicNotFoundPage() {
64+
final ResponseEntity<JsonNode> response = this.testRestTemplate.exchange("/public/notfound", HttpMethod.GET,
65+
null, JsonNode.class);
66+
assertThat(response.getStatusCode()).isEqualTo(HttpStatus.NOT_FOUND);
67+
JsonNode jsonResponse = response.getBody();
68+
assertThat(jsonResponse.get("error").asText()).isEqualTo("Not Found");
69+
}
70+
71+
@Test
72+
void testPublicNotFoundPageWithCorrectCredentials() {
73+
final ResponseEntity<JsonNode> response = this.testRestTemplate.withBasicAuth("username", "password")
74+
.exchange("/public/notfound", HttpMethod.GET, null, JsonNode.class);
75+
assertThat(response.getStatusCode()).isEqualTo(HttpStatus.NOT_FOUND);
76+
JsonNode jsonResponse = response.getBody();
77+
assertThat(jsonResponse.get("error").asText()).isEqualTo("Not Found");
78+
}
79+
80+
@Test
81+
void testPublicNotFoundPageWithBadCredentials() {
82+
final ResponseEntity<JsonNode> response = this.testRestTemplate.withBasicAuth("username", "wrong")
83+
.exchange("/public/notfound", HttpMethod.GET, null, JsonNode.class);
84+
assertThat(response.getStatusCode()).isEqualTo(HttpStatus.UNAUTHORIZED);
85+
JsonNode jsonResponse = response.getBody();
86+
assertThat(jsonResponse).isNull();
87+
}
88+
89+
@Test
90+
void testCorrectCredentialsWithControllerException() {
91+
final ResponseEntity<JsonNode> response = this.testRestTemplate.withBasicAuth("username", "password")
92+
.exchange("/fail", HttpMethod.GET, null, JsonNode.class);
93+
assertThat(response.getStatusCode()).isEqualTo(HttpStatus.INTERNAL_SERVER_ERROR);
94+
JsonNode jsonResponse = response.getBody();
95+
assertThat(jsonResponse.get("error").asText()).isEqualTo("Internal Server Error");
96+
}
97+
98+
@Test
99+
void testCorrectCredentials() {
100+
final ResponseEntity<String> response = this.testRestTemplate.withBasicAuth("username", "password")
101+
.exchange("/test", HttpMethod.GET, null, String.class);
102+
assertThat(response.getStatusCode()).isEqualTo(HttpStatus.OK);
103+
response.getBody();
104+
assertThat(response.getBody()).isEqualTo("test");
105+
}
106+
107+
@Configuration(proxyBeanMethods = false)
108+
static class TestConfiguration {
109+
110+
@RestController
111+
static class TestController {
112+
113+
@GetMapping("/test")
114+
String test() {
115+
return "test";
116+
}
117+
118+
@GetMapping("/fail")
119+
String fail() {
120+
throw new RuntimeException();
121+
}
122+
123+
}
124+
125+
}
126+
127+
}

‎spring-boot-tests/spring-boot-smoke-tests/spring-boot-smoke-test-web-secure/src/test/java/smoketest/web/secure/ErrorPageTests.java

+19-66
Original file line numberDiff line numberDiff line change
@@ -16,83 +16,36 @@
1616

1717
package smoketest.web.secure;
1818

19-
import com.fasterxml.jackson.databind.JsonNode;
20-
import org.junit.jupiter.api.Test;
21-
22-
import org.springframework.beans.factory.annotation.Autowired;
2319
import org.springframework.boot.test.context.SpringBootTest;
2420
import org.springframework.boot.test.context.SpringBootTest.WebEnvironment;
25-
import org.springframework.boot.test.web.client.TestRestTemplate;
26-
import org.springframework.context.annotation.Configuration;
27-
import org.springframework.http.HttpMethod;
28-
import org.springframework.http.HttpStatus;
29-
import org.springframework.http.ResponseEntity;
30-
import org.springframework.web.bind.annotation.GetMapping;
31-
import org.springframework.web.bind.annotation.RestController;
32-
33-
import static org.assertj.core.api.Assertions.assertThat;
21+
import org.springframework.context.annotation.Bean;
22+
import org.springframework.security.config.annotation.web.builders.HttpSecurity;
23+
import org.springframework.security.web.SecurityFilterChain;
3424

3525
/**
3626
* Tests to ensure that the error page is accessible only to authorized users.
3727
*
3828
* @author Madhura Bhave
3929
*/
4030
@SpringBootTest(webEnvironment = WebEnvironment.RANDOM_PORT,
41-
classes = { ErrorPageTests.TestConfiguration.class, SampleWebSecureApplication.class },
31+
classes = { AbstractErrorPageTests.TestConfiguration.class, ErrorPageTests.SecurityConfiguration.class,
32+
SampleWebSecureApplication.class },
4233
properties = { "server.error.include-message=always", "spring.security.user.name=username",
4334
"spring.security.user.password=password" })
44-
class ErrorPageTests {
45-
46-
@Autowired
47-
private TestRestTemplate testRestTemplate;
48-
49-
@Test
50-
void testBadCredentials() {
51-
final ResponseEntity<JsonNode> response = this.testRestTemplate.withBasicAuth("username", "wrongpassword")
52-
.exchange("/test", HttpMethod.GET, null, JsonNode.class);
53-
assertThat(response.getStatusCode()).isEqualTo(HttpStatus.UNAUTHORIZED);
54-
JsonNode jsonResponse = response.getBody();
55-
assertThat(jsonResponse).isNull();
56-
}
57-
58-
@Test
59-
void testNoCredentials() {
60-
final ResponseEntity<JsonNode> response = this.testRestTemplate.exchange("/test", HttpMethod.GET, null,
61-
JsonNode.class);
62-
assertThat(response.getStatusCode()).isEqualTo(HttpStatus.UNAUTHORIZED);
63-
JsonNode jsonResponse = response.getBody();
64-
assertThat(jsonResponse).isNull();
65-
}
66-
67-
@Test
68-
void testPublicNotFoundPage() {
69-
final ResponseEntity<JsonNode> response = this.testRestTemplate.exchange("/public/notfound", HttpMethod.GET,
70-
null, JsonNode.class);
71-
assertThat(response.getStatusCode()).isEqualTo(HttpStatus.NOT_FOUND);
72-
JsonNode jsonResponse = response.getBody();
73-
assertThat(jsonResponse).isNull();
74-
}
75-
76-
@Test
77-
void testCorrectCredentials() {
78-
final ResponseEntity<String> response = this.testRestTemplate.withBasicAuth("username", "password")
79-
.exchange("/test", HttpMethod.GET, null, String.class);
80-
assertThat(response.getStatusCode()).isEqualTo(HttpStatus.OK);
81-
response.getBody();
82-
assertThat(response.getBody()).isEqualTo("test");
83-
}
84-
85-
@Configuration(proxyBeanMethods = false)
86-
static class TestConfiguration {
87-
88-
@RestController
89-
static class TestController {
90-
91-
@GetMapping("/test")
92-
String test() {
93-
return "test";
94-
}
95-
35+
class ErrorPageTests extends AbstractErrorPageTests {
36+
37+
@org.springframework.boot.test.context.TestConfiguration(proxyBeanMethods = false)
38+
static class SecurityConfiguration {
39+
40+
@Bean
41+
SecurityFilterChain configure(HttpSecurity http) throws Exception {
42+
http.authorizeRequests((requests) -> {
43+
requests.antMatchers("/public/**").permitAll();
44+
requests.anyRequest().fullyAuthenticated();
45+
});
46+
http.httpBasic();
47+
http.formLogin((form) -> form.loginPage("/login").permitAll());
48+
return http.build();
9649
}
9750

9851
}

0 commit comments

Comments
 (0)