Propagate AccessDeniedException for Authorized Objects Returned from a Controller

Closes gh-16058

Signed-off-by: Evgeniy Cheban <mister.cheban@gmail.com>

config/src/main/java/org/springframework/security/config/annotation/method/configuration/AuthorizationProxyWebConfiguration.java

@@ -16,20 +16,30 @@
 
 package org.springframework.security.config.annotation.method.configuration;
 
+import java.util.List;
 import java.util.Map;
 
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpServletResponse;
+
 import org.springframework.beans.factory.config.BeanDefinition;
 import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
 import org.springframework.context.annotation.Role;
 import org.springframework.http.HttpEntity;
 import org.springframework.http.ResponseEntity;
+import org.springframework.http.converter.HttpMessageNotWritableException;
+import org.springframework.security.access.AccessDeniedException;
 import org.springframework.security.authorization.method.AuthorizationAdvisorProxyFactory;
+import org.springframework.security.web.util.ThrowableAnalyzer;
+import org.springframework.web.servlet.HandlerExceptionResolver;
 import org.springframework.web.servlet.ModelAndView;
 import org.springframework.web.servlet.View;
+import org.springframework.web.servlet.config.annotation.WebMvcConfigurer;
+import org.springframework.web.servlet.mvc.support.DefaultHandlerExceptionResolver;
 
 @Configuration
-class AuthorizationProxyWebConfiguration {
+class AuthorizationProxyWebConfiguration implements WebMvcConfigurer {
 
 	@Bean
 	@Role(BeanDefinition.ROLE_INFRASTRUCTURE)
@@ -37,6 +47,18 @@ class AuthorizationProxyWebConfiguration {
 		return new WebTargetVisitor();
 	}
 
+	@Override
+	public void extendHandlerExceptionResolvers(List<HandlerExceptionResolver> resolvers) {
+		for (int i = 0; i < resolvers.size(); i++) {
+			HandlerExceptionResolver resolver = resolvers.get(i);
+			if (resolver instanceof DefaultHandlerExceptionResolver) {
+				resolvers.add(i, new HttpMessageNotWritableAccessDeniedExceptionResolver());
+				return;
+			}
+		}
+		resolvers.add(new HttpMessageNotWritableAccessDeniedExceptionResolver());
+	}
+
 	static class WebTargetVisitor implements AuthorizationAdvisorProxyFactory.TargetVisitor {
 
 		@Override
@@ -62,4 +84,28 @@ class AuthorizationProxyWebConfiguration {
 
 	}
 
+	static class HttpMessageNotWritableAccessDeniedExceptionResolver implements HandlerExceptionResolver {
+
+		final ThrowableAnalyzer throwableAnalyzer = new ThrowableAnalyzer();
+
+		@Override
+		public ModelAndView resolveException(HttpServletRequest request, HttpServletResponse response, Object handler,
+				Exception ex) {
+			// Only resolves AccessDeniedException if it occurred during serialization,
+			// otherwise lets the user-defined handler deal with it.
+			if (ex instanceof HttpMessageNotWritableException) {
+				Throwable[] causeChain = this.throwableAnalyzer.determineCauseChain(ex);
+				Throwable accessDeniedException = this.throwableAnalyzer
+					.getFirstThrowableOfType(AccessDeniedException.class, causeChain);
+				if (accessDeniedException != null) {
+					return new ModelAndView((model, req, res) -> {
+						throw ex;
+					});
+				}
+			}
+			return null;
+		}
+
+	}
+
 }

config/src/test/java/org/springframework/security/config/annotation/method/configuration/PrePostMethodSecurityConfigurationTests.java

@@ -33,6 +33,7 @@ import io.micrometer.observation.ObservationHandler;
 import io.micrometer.observation.ObservationRegistry;
 import io.micrometer.observation.ObservationTextPublisher;
 import jakarta.annotation.security.DenyAll;
+import org.aopalliance.aop.Advice;
 import org.aopalliance.intercept.MethodInterceptor;
 import org.aopalliance.intercept.MethodInvocation;
 import org.junit.jupiter.api.Test;
@@ -42,6 +43,7 @@ import org.junit.jupiter.params.provider.ValueSource;
 import org.mockito.Mockito;
 
 import org.springframework.aop.Advisor;
+import org.springframework.aop.Pointcut;
 import org.springframework.aop.config.AopConfigUtils;
 import org.springframework.aop.support.DefaultPointcutAdvisor;
 import org.springframework.aop.support.JdkRegexpMethodPointcut;
@@ -62,8 +64,11 @@ import org.springframework.context.event.EventListener;
 import org.springframework.core.annotation.AnnotationAwareOrderComparator;
 import org.springframework.core.annotation.AnnotationConfigurationException;
 import org.springframework.core.annotation.Order;
+import org.springframework.http.HttpStatus;
 import org.springframework.http.HttpStatusCode;
+import org.springframework.http.MediaType;
 import org.springframework.http.ResponseEntity;
+import org.springframework.http.converter.HttpMessageNotWritableException;
 import org.springframework.security.access.AccessDeniedException;
 import org.springframework.security.access.PermissionEvaluator;
 import org.springframework.security.access.annotation.BusinessService;
@@ -95,6 +100,7 @@ import org.springframework.security.authorization.method.MethodAuthorizationDeni
 import org.springframework.security.authorization.method.MethodInvocationResult;
 import org.springframework.security.authorization.method.PrePostTemplateDefaults;
 import org.springframework.security.config.annotation.SecurityContextChangedListenerConfig;
+import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity;
 import org.springframework.security.config.core.GrantedAuthorityDefaults;
 import org.springframework.security.config.observation.SecurityObservationSettings;
 import org.springframework.security.config.test.SpringTestContext;
@@ -106,13 +112,24 @@ import org.springframework.security.core.context.SecurityContextHolderStrategy;
 import org.springframework.security.test.context.support.WithAnonymousUser;
 import org.springframework.security.test.context.support.WithMockUser;
 import org.springframework.security.test.context.support.WithSecurityContextTestExecutionListener;
+import org.springframework.security.web.util.ThrowableAnalyzer;
 import org.springframework.stereotype.Component;
+import org.springframework.stereotype.Service;
 import org.springframework.test.context.ContextConfiguration;
 import org.springframework.test.context.TestExecutionListeners;
 import org.springframework.test.context.junit.jupiter.SpringExtension;
+import org.springframework.test.web.servlet.MockMvc;
+import org.springframework.test.web.servlet.MvcResult;
+import org.springframework.test.web.servlet.request.MockHttpServletRequestBuilder;
+import org.springframework.web.bind.annotation.ControllerAdvice;
+import org.springframework.web.bind.annotation.ExceptionHandler;
+import org.springframework.web.bind.annotation.GetMapping;
+import org.springframework.web.bind.annotation.RequestParam;
+import org.springframework.web.bind.annotation.RestController;
 import org.springframework.web.context.ConfigurableWebApplicationContext;
 import org.springframework.web.context.support.AnnotationConfigWebApplicationContext;
 import org.springframework.web.servlet.ModelAndView;
+import org.springframework.web.servlet.config.annotation.EnableWebMvc;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
@@ -127,6 +144,9 @@ import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.verifyNoInteractions;
+import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.user;
+import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get;
+import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;
 
 /**
  * Tests for {@link PrePostMethodSecurityConfiguration}.
@@ -148,6 +168,9 @@ public class PrePostMethodSecurityConfigurationTests {
 	@Autowired(required = false)
 	BusinessService businessService;
 
+	@Autowired(required = false)
+	MockMvc mvc;
+
 	@WithMockUser
 	@Test
 	public void customMethodSecurityPreAuthorizeAdminWhenRoleUserThenAccessDeniedException() {
@@ -1181,6 +1204,97 @@ public class PrePostMethodSecurityConfigurationTests {
 		}
 	}
 
+	@Test
+	void getWhenPostAuthorizeAuthenticationNameMatchesThenRespondsWithOk() throws Exception {
+		this.spring.register(WebMvcMethodSecurityConfig.class, BasicController.class).autowire();
+		// @formatter:off
+		MockHttpServletRequestBuilder requestWithUser = get("/authorized-person")
+				.param("name", "rob")
+				.with(user("rob"));
+		// @formatter:on
+		this.mvc.perform(requestWithUser).andExpect(status().isOk());
+	}
+
+	@Test
+	void getWhenPostAuthorizeAuthenticationNameNotMatchThenRespondsWithForbidden() throws Exception {
+		this.spring.register(WebMvcMethodSecurityConfig.class, BasicController.class).autowire();
+		// @formatter:off
+		MockHttpServletRequestBuilder requestWithUser = get("/authorized-person")
+				.param("name", "john")
+				.with(user("rob"));
+		// @formatter:on
+		this.mvc.perform(requestWithUser).andExpect(status().isForbidden());
+	}
+
+	@Test
+	void getWhenPostAuthorizeWithinServiceAuthenticationNameMatchesThenRespondsWithOk() throws Exception {
+		this.spring.register(WebMvcMethodSecurityConfig.class, BasicController.class, BasicService.class).autowire();
+		// @formatter:off
+		MockHttpServletRequestBuilder requestWithUser = get("/greetings/authorized-person")
+				.param("name", "rob")
+				.with(user("rob"));
+		// @formatter:on
+		MvcResult mvcResult = this.mvc.perform(requestWithUser).andExpect(status().isOk()).andReturn();
+		assertThat(mvcResult.getResponse().getContentAsString()).isEqualTo("Hello: rob");
+	}
+
+	@Test
+	void getWhenPostAuthorizeWithinServiceAuthenticationNameNotMatchThenCustomHandlerRespondsWithForbidden()
+			throws Exception {
+		this.spring
+			.register(WebMvcMethodSecurityConfig.class, BasicController.class, BasicService.class,
+					BasicControllerAdvice.class)
+			.autowire();
+		// @formatter:off
+		MockHttpServletRequestBuilder requestWithUser = get("/greetings/authorized-person")
+				.param("name", "john")
+				.with(user("rob"));
+		// @formatter:on
+		MvcResult mvcResult = this.mvc.perform(requestWithUser).andExpect(status().isForbidden()).andReturn();
+		assertThat(mvcResult.getResponse().getContentAsString()).isEqualTo("""
+				{"message":"Access Denied"}\
+				""");
+	}
+
+	@Test
+	void getWhenPostAuthorizeAuthenticationNameNotMatchThenCustomHandlerRespondsWithForbidden() throws Exception {
+		this.spring
+			.register(WebMvcMethodSecurityConfig.class, BasicController.class, BasicService.class,
+					BasicControllerAdvice.class)
+			.autowire();
+		// @formatter:off
+		MockHttpServletRequestBuilder requestWithUser = get("/authorized-person")
+				.param("name", "john")
+				.with(user("rob"));
+		// @formatter:on
+		MvcResult mvcResult = this.mvc.perform(requestWithUser).andExpect(status().isForbidden()).andReturn();
+		assertThat(mvcResult.getResponse().getContentAsString()).isEqualTo("""
+				{"message":"Could not write JSON: Access Denied"}\
+				""");
+	}
+
+	@Test
+	void getWhenCustomAdvisorAuthenticationNameMatchesThenRespondsWithOk() throws Exception {
+		this.spring.register(WebMvcMethodSecurityCustomAdvisorConfig.class, BasicController.class).autowire();
+		// @formatter:off
+		MockHttpServletRequestBuilder requestWithUser = get("/authorized-person")
+				.param("name", "rob")
+				.with(user("rob"));
+		// @formatter:on
+		this.mvc.perform(requestWithUser).andExpect(status().isOk());
+	}
+
+	@Test
+	void getWhenCustomAdvisorAuthenticationNameNotMatchThenRespondsWithForbidden() throws Exception {
+		this.spring.register(WebMvcMethodSecurityCustomAdvisorConfig.class, BasicController.class).autowire();
+		// @formatter:off
+		MockHttpServletRequestBuilder requestWithUser = get("/authorized-person")
+				.param("name", "john")
+				.with(user("rob"));
+		// @formatter:on
+		this.mvc.perform(requestWithUser).andExpect(status().isForbidden());
+	}
+
 	private static Consumer<ConfigurableWebApplicationContext> disallowBeanOverriding() {
 		return (context) -> ((AnnotationConfigWebApplicationContext) context).setAllowBeanDefinitionOverriding(false);
 	}
@@ -1919,4 +2033,118 @@ public class PrePostMethodSecurityConfigurationTests {
 
 	}
 
+	@EnableWebMvc
+	@EnableWebSecurity
+	@EnableMethodSecurity
+	static class WebMvcMethodSecurityConfig {
+
+	}
+
+	@EnableWebMvc
+	@EnableWebSecurity
+	@EnableMethodSecurity
+	static class WebMvcMethodSecurityCustomAdvisorConfig {
+
+		@Bean
+		AuthorizationAdvisor customAdvisor(SecurityContextHolderStrategy strategy) {
+			JdkRegexpMethodPointcut pointcut = new JdkRegexpMethodPointcut();
+			pointcut.setPattern(".*AuthorizedPerson.*getName");
+			return new AuthorizationAdvisor() {
+				@Override
+				public Object invoke(MethodInvocation mi) throws Throwable {
+					Authentication auth = strategy.getContext().getAuthentication();
+					Object result = mi.proceed();
+					if (auth.getName().equals(result)) {
+						return result;
+					}
+					throw new AccessDeniedException("Access Denied for User '" + auth.getName() + "'");
+				}
+
+				@Override
+				public Pointcut getPointcut() {
+					return pointcut;
+				}
+
+				@Override
+				public Advice getAdvice() {
+					return this;
+				}
+
+				@Override
+				public int getOrder() {
+					return AuthorizationInterceptorsOrder.POST_FILTER.getOrder() + 1;
+				}
+			};
+		}
+
+	}
+
+	@RestController
+	static class BasicController {
+
+		@Autowired(required = false)
+		BasicService service;
+
+		@GetMapping("/greetings/authorized-person")
+		String getAuthorizedPersonGreeting(@RequestParam String name) {
+			AuthorizedPerson authorizedPerson = this.service.getAuthorizedPerson(name);
+			return "Hello: " + authorizedPerson.getName();
+		}
+
+		@AuthorizeReturnObject
+		@GetMapping(value = "/authorized-person", produces = MediaType.APPLICATION_JSON_VALUE)
+		AuthorizedPerson getAuthorizedPerson(@RequestParam String name) {
+			return new AuthorizedPerson(name);
+		}
+
+	}
+
+	@ControllerAdvice
+	static class BasicControllerAdvice {
+
+		@ExceptionHandler(AccessDeniedException.class)
+		ResponseEntity<Map<String, String>> handleAccessDenied(AccessDeniedException ex) {
+			Map<String, String> responseBody = Map.of("message", ex.getMessage());
+			return ResponseEntity.status(HttpStatus.FORBIDDEN).body(responseBody);
+		}
+
+		@ExceptionHandler(HttpMessageNotWritableException.class)
+		ResponseEntity<Map<String, String>> handleHttpMessageNotWritable(HttpMessageNotWritableException ex) {
+			ThrowableAnalyzer throwableAnalyzer = new ThrowableAnalyzer();
+			Throwable[] causeChain = throwableAnalyzer.determineCauseChain(ex);
+			Throwable t = throwableAnalyzer.getFirstThrowableOfType(AccessDeniedException.class, causeChain);
+			if (t != null) {
+				Map<String, String> responseBody = Map.of("message", ex.getMessage());
+				return ResponseEntity.status(HttpStatus.FORBIDDEN).body(responseBody);
+			}
+			throw ex;
+		}
+
+	}
+
+	@Service
+	static class BasicService {
+
+		@AuthorizeReturnObject
+		AuthorizedPerson getAuthorizedPerson(String name) {
+			return new AuthorizedPerson(name);
+		}
+
+	}
+
+	public static class AuthorizedPerson {
+
+		final String name;
+
+		AuthorizedPerson(String name) {
+			this.name = name;
+		}
+
+		@PostAuthorize("returnObject == authentication.name")
+		public String getName() {
+			return this.name;
+		}
+
+	}
+
 }