From 11a21896dddd43e09f71a349c915c36e0ec89ba2 Mon Sep 17 00:00:00 2001 From: Josh Cummings Date: Mon, 6 Nov 2023 17:08:54 -0700 Subject: [PATCH] Defer SecurityContextHolderStrategy Lookup Due to how early method interceptors are loaded during startup it's reasonable to consider scenarios where applications are changing the global security context holder strategy during startup. Closes gh-12877 --- ...rizationManagerAfterMethodInterceptor.java | 27 +++++++++---------- ...izationManagerBeforeMethodInterceptor.java | 27 +++++++++---------- ...tFilterAuthorizationMethodInterceptor.java | 25 ++++++++--------- ...eFilterAuthorizationMethodInterceptor.java | 25 ++++++++--------- ...ionManagerAfterMethodInterceptorTests.java | 22 ++++++++++++++- ...onManagerBeforeMethodInterceptorTests.java | 20 +++++++++++++- ...erAuthorizationMethodInterceptorTests.java | 25 ++++++++++++++++- ...erAuthorizationMethodInterceptorTests.java | 22 ++++++++++++++- 8 files changed, 131 insertions(+), 62 deletions(-) diff --git a/core/src/main/java/org/springframework/security/authorization/method/AuthorizationManagerAfterMethodInterceptor.java b/core/src/main/java/org/springframework/security/authorization/method/AuthorizationManagerAfterMethodInterceptor.java index fcdad71b1c..0284710ccf 100644 --- a/core/src/main/java/org/springframework/security/authorization/method/AuthorizationManagerAfterMethodInterceptor.java +++ b/core/src/main/java/org/springframework/security/authorization/method/AuthorizationManagerAfterMethodInterceptor.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2022 the original author or authors. + * Copyright 2002-2023 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. @@ -51,8 +51,7 @@ import org.springframework.util.Assert; public final class AuthorizationManagerAfterMethodInterceptor implements Ordered, MethodInterceptor, PointcutAdvisor, AopInfrastructureBean { - private Supplier authentication = getAuthentication( - SecurityContextHolder.getContextHolderStrategy()); + private Supplier securityContextHolderStrategy = SecurityContextHolder::getContextHolderStrategy; private final Log logger = LogFactory.getLog(this.getClass()); @@ -156,14 +155,14 @@ public final class AuthorizationManagerAfterMethodInterceptor * @since 5.8 */ public void setSecurityContextHolderStrategy(SecurityContextHolderStrategy strategy) { - this.authentication = getAuthentication(strategy); + this.securityContextHolderStrategy = () -> strategy; } private void attemptAuthorization(MethodInvocation mi, Object result) { this.logger.debug(LogMessage.of(() -> "Authorizing method invocation " + mi)); MethodInvocationResult object = new MethodInvocationResult(mi, result); - AuthorizationDecision decision = this.authorizationManager.check(this.authentication, object); - this.eventPublisher.publishAuthorizationEvent(this.authentication, object, decision); + AuthorizationDecision decision = this.authorizationManager.check(this::getAuthentication, object); + this.eventPublisher.publishAuthorizationEvent(this::getAuthentication, object, decision); if (decision != null && !decision.isGranted()) { this.logger.debug(LogMessage.of(() -> "Failed to authorize " + mi + " with authorization manager " + this.authorizationManager + " and decision " + decision)); @@ -172,15 +171,13 @@ public final class AuthorizationManagerAfterMethodInterceptor this.logger.debug(LogMessage.of(() -> "Authorized method invocation " + mi)); } - private Supplier getAuthentication(SecurityContextHolderStrategy strategy) { - return () -> { - Authentication authentication = strategy.getContext().getAuthentication(); - if (authentication == null) { - throw new AuthenticationCredentialsNotFoundException( - "An Authentication object was not found in the SecurityContext"); - } - return authentication; - }; + private Authentication getAuthentication() { + Authentication authentication = this.securityContextHolderStrategy.get().getContext().getAuthentication(); + if (authentication == null) { + throw new AuthenticationCredentialsNotFoundException( + "An Authentication object was not found in the SecurityContext"); + } + return authentication; } private static void noPublish(Supplier authentication, T object, diff --git a/core/src/main/java/org/springframework/security/authorization/method/AuthorizationManagerBeforeMethodInterceptor.java b/core/src/main/java/org/springframework/security/authorization/method/AuthorizationManagerBeforeMethodInterceptor.java index 36ccdfd6a6..239c723cea 100644 --- a/core/src/main/java/org/springframework/security/authorization/method/AuthorizationManagerBeforeMethodInterceptor.java +++ b/core/src/main/java/org/springframework/security/authorization/method/AuthorizationManagerBeforeMethodInterceptor.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2022 the original author or authors. + * Copyright 2002-2023 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. @@ -56,8 +56,7 @@ import org.springframework.util.Assert; public final class AuthorizationManagerBeforeMethodInterceptor implements Ordered, MethodInterceptor, PointcutAdvisor, AopInfrastructureBean { - private Supplier authentication = getAuthentication( - SecurityContextHolder.getContextHolderStrategy()); + private Supplier securityContextHolderStrategy = SecurityContextHolder::getContextHolderStrategy; private final Log logger = LogFactory.getLog(this.getClass()); @@ -202,13 +201,13 @@ public final class AuthorizationManagerBeforeMethodInterceptor * @since 5.8 */ public void setSecurityContextHolderStrategy(SecurityContextHolderStrategy securityContextHolderStrategy) { - this.authentication = getAuthentication(securityContextHolderStrategy); + this.securityContextHolderStrategy = () -> securityContextHolderStrategy; } private void attemptAuthorization(MethodInvocation mi) { this.logger.debug(LogMessage.of(() -> "Authorizing method invocation " + mi)); - AuthorizationDecision decision = this.authorizationManager.check(this.authentication, mi); - this.eventPublisher.publishAuthorizationEvent(this.authentication, mi, decision); + AuthorizationDecision decision = this.authorizationManager.check(this::getAuthentication, mi); + this.eventPublisher.publishAuthorizationEvent(this::getAuthentication, mi, decision); if (decision != null && !decision.isGranted()) { this.logger.debug(LogMessage.of(() -> "Failed to authorize " + mi + " with authorization manager " + this.authorizationManager + " and decision " + decision)); @@ -217,15 +216,13 @@ public final class AuthorizationManagerBeforeMethodInterceptor this.logger.debug(LogMessage.of(() -> "Authorized method invocation " + mi)); } - private Supplier getAuthentication(SecurityContextHolderStrategy strategy) { - return () -> { - Authentication authentication = strategy.getContext().getAuthentication(); - if (authentication == null) { - throw new AuthenticationCredentialsNotFoundException( - "An Authentication object was not found in the SecurityContext"); - } - return authentication; - }; + private Authentication getAuthentication() { + Authentication authentication = this.securityContextHolderStrategy.get().getContext().getAuthentication(); + if (authentication == null) { + throw new AuthenticationCredentialsNotFoundException( + "An Authentication object was not found in the SecurityContext"); + } + return authentication; } private static void noPublish(Supplier authentication, T object, diff --git a/core/src/main/java/org/springframework/security/authorization/method/PostFilterAuthorizationMethodInterceptor.java b/core/src/main/java/org/springframework/security/authorization/method/PostFilterAuthorizationMethodInterceptor.java index 4e58611b6d..99630298a2 100644 --- a/core/src/main/java/org/springframework/security/authorization/method/PostFilterAuthorizationMethodInterceptor.java +++ b/core/src/main/java/org/springframework/security/authorization/method/PostFilterAuthorizationMethodInterceptor.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2022 the original author or authors. + * Copyright 2002-2023 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. @@ -46,8 +46,7 @@ import org.springframework.security.core.context.SecurityContextHolderStrategy; public final class PostFilterAuthorizationMethodInterceptor implements Ordered, MethodInterceptor, PointcutAdvisor, AopInfrastructureBean { - private Supplier authentication = getAuthentication( - SecurityContextHolder.getContextHolderStrategy()); + private Supplier securityContextHolderStrategy = SecurityContextHolder::getContextHolderStrategy; private PostFilterExpressionAttributeRegistry registry = new PostFilterExpressionAttributeRegistry(); @@ -108,7 +107,7 @@ public final class PostFilterAuthorizationMethodInterceptor * @since 5.8 */ public void setSecurityContextHolderStrategy(SecurityContextHolderStrategy strategy) { - this.authentication = getAuthentication(strategy); + this.securityContextHolderStrategy = () -> strategy; } /** @@ -125,19 +124,17 @@ public final class PostFilterAuthorizationMethodInterceptor return returnedObject; } MethodSecurityExpressionHandler expressionHandler = this.registry.getExpressionHandler(); - EvaluationContext ctx = expressionHandler.createEvaluationContext(this.authentication, mi); + EvaluationContext ctx = expressionHandler.createEvaluationContext(this::getAuthentication, mi); return expressionHandler.filter(returnedObject, attribute.getExpression(), ctx); } - private Supplier getAuthentication(SecurityContextHolderStrategy strategy) { - return () -> { - Authentication authentication = strategy.getContext().getAuthentication(); - if (authentication == null) { - throw new AuthenticationCredentialsNotFoundException( - "An Authentication object was not found in the SecurityContext"); - } - return authentication; - }; + private Authentication getAuthentication() { + Authentication authentication = this.securityContextHolderStrategy.get().getContext().getAuthentication(); + if (authentication == null) { + throw new AuthenticationCredentialsNotFoundException( + "An Authentication object was not found in the SecurityContext"); + } + return authentication; } } diff --git a/core/src/main/java/org/springframework/security/authorization/method/PreFilterAuthorizationMethodInterceptor.java b/core/src/main/java/org/springframework/security/authorization/method/PreFilterAuthorizationMethodInterceptor.java index a0a9e30171..bde5f1ee9d 100644 --- a/core/src/main/java/org/springframework/security/authorization/method/PreFilterAuthorizationMethodInterceptor.java +++ b/core/src/main/java/org/springframework/security/authorization/method/PreFilterAuthorizationMethodInterceptor.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2022 the original author or authors. + * Copyright 2002-2023 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. @@ -47,8 +47,7 @@ import org.springframework.util.StringUtils; public final class PreFilterAuthorizationMethodInterceptor implements Ordered, MethodInterceptor, PointcutAdvisor, AopInfrastructureBean { - private Supplier authentication = getAuthentication( - SecurityContextHolder.getContextHolderStrategy()); + private Supplier securityContextHolderStrategy = SecurityContextHolder::getContextHolderStrategy; private PreFilterExpressionAttributeRegistry registry = new PreFilterExpressionAttributeRegistry(); @@ -109,7 +108,7 @@ public final class PreFilterAuthorizationMethodInterceptor * @since 5.8 */ public void setSecurityContextHolderStrategy(SecurityContextHolderStrategy strategy) { - this.authentication = getAuthentication(strategy); + this.securityContextHolderStrategy = () -> strategy; } /** @@ -124,7 +123,7 @@ public final class PreFilterAuthorizationMethodInterceptor return mi.proceed(); } MethodSecurityExpressionHandler expressionHandler = this.registry.getExpressionHandler(); - EvaluationContext ctx = expressionHandler.createEvaluationContext(this.authentication, mi); + EvaluationContext ctx = expressionHandler.createEvaluationContext(this::getAuthentication, mi); Object filterTarget = findFilterTarget(attribute.getFilterTarget(), ctx, mi); expressionHandler.filter(filterTarget, attribute.getExpression(), ctx); return mi.proceed(); @@ -150,15 +149,13 @@ public final class PreFilterAuthorizationMethodInterceptor return filterTarget; } - private Supplier getAuthentication(SecurityContextHolderStrategy strategy) { - return () -> { - Authentication authentication = strategy.getContext().getAuthentication(); - if (authentication == null) { - throw new AuthenticationCredentialsNotFoundException( - "An Authentication object was not found in the SecurityContext"); - } - return authentication; - }; + private Authentication getAuthentication() { + Authentication authentication = this.securityContextHolderStrategy.get().getContext().getAuthentication(); + if (authentication == null) { + throw new AuthenticationCredentialsNotFoundException( + "An Authentication object was not found in the SecurityContext"); + } + return authentication; } } diff --git a/core/src/test/java/org/springframework/security/authorization/method/AuthorizationManagerAfterMethodInterceptorTests.java b/core/src/test/java/org/springframework/security/authorization/method/AuthorizationManagerAfterMethodInterceptorTests.java index adbfb9d6e1..b82785f101 100644 --- a/core/src/test/java/org/springframework/security/authorization/method/AuthorizationManagerAfterMethodInterceptorTests.java +++ b/core/src/test/java/org/springframework/security/authorization/method/AuthorizationManagerAfterMethodInterceptorTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2022 the original author or authors. + * Copyright 2002-2023 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. @@ -29,6 +29,7 @@ import org.springframework.security.authorization.AuthorizationDecision; import org.springframework.security.authorization.AuthorizationEventPublisher; import org.springframework.security.authorization.AuthorizationManager; import org.springframework.security.core.Authentication; +import org.springframework.security.core.authority.AuthorityUtils; import org.springframework.security.core.context.SecurityContext; import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.security.core.context.SecurityContextHolderStrategy; @@ -91,6 +92,25 @@ public class AuthorizationManagerAfterMethodInterceptorTests { verify(strategy).getContext(); } + // gh-12877 + @Test + public void afterWhenStaticSecurityContextHolderStrategyAfterConstructorThenUses() throws Throwable { + SecurityContextHolderStrategy strategy = mock(SecurityContextHolderStrategy.class); + Authentication authentication = new TestingAuthenticationToken("john", "password", + AuthorityUtils.createAuthorityList("authority")); + given(strategy.getContext()).willReturn(new SecurityContextImpl(authentication)); + MethodInvocation invocation = mock(MethodInvocation.class); + AuthorizationManager authorizationManager = AuthenticatedAuthorizationManager + .authenticated(); + AuthorizationManagerAfterMethodInterceptor advice = new AuthorizationManagerAfterMethodInterceptor( + Pointcut.TRUE, authorizationManager); + SecurityContextHolderStrategy saved = SecurityContextHolder.getContextHolderStrategy(); + SecurityContextHolder.setContextHolderStrategy(strategy); + advice.invoke(invocation); + verify(strategy).getContext(); + SecurityContextHolder.setContextHolderStrategy(saved); + } + @Test public void configureWhenAuthorizationEventPublisherIsNullThenIllegalArgument() { AuthorizationManagerAfterMethodInterceptor advice = new AuthorizationManagerAfterMethodInterceptor( diff --git a/core/src/test/java/org/springframework/security/authorization/method/AuthorizationManagerBeforeMethodInterceptorTests.java b/core/src/test/java/org/springframework/security/authorization/method/AuthorizationManagerBeforeMethodInterceptorTests.java index af922fc4e8..210a70e0b4 100644 --- a/core/src/test/java/org/springframework/security/authorization/method/AuthorizationManagerBeforeMethodInterceptorTests.java +++ b/core/src/test/java/org/springframework/security/authorization/method/AuthorizationManagerBeforeMethodInterceptorTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2022 the original author or authors. + * Copyright 2002-2023 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. @@ -87,6 +87,24 @@ public class AuthorizationManagerBeforeMethodInterceptorTests { verify(strategy).getContext(); } + // gh-12877 + @Test + public void beforeWhenStaticSecurityContextHolderStrategyAfterConstructorThenUses() throws Throwable { + SecurityContextHolderStrategy strategy = mock(SecurityContextHolderStrategy.class); + Authentication authentication = new TestingAuthenticationToken("john", "password", + AuthorityUtils.createAuthorityList("authority")); + given(strategy.getContext()).willReturn(new SecurityContextImpl(authentication)); + MethodInvocation invocation = mock(MethodInvocation.class); + AuthorizationManager authorizationManager = AuthenticatedAuthorizationManager.authenticated(); + AuthorizationManagerBeforeMethodInterceptor advice = new AuthorizationManagerBeforeMethodInterceptor( + Pointcut.TRUE, authorizationManager); + SecurityContextHolderStrategy saved = SecurityContextHolder.getContextHolderStrategy(); + SecurityContextHolder.setContextHolderStrategy(strategy); + advice.invoke(invocation); + verify(strategy).getContext(); + SecurityContextHolder.setContextHolderStrategy(saved); + } + @Test public void configureWhenAuthorizationEventPublisherIsNullThenIllegalArgument() { AuthorizationManagerBeforeMethodInterceptor advice = new AuthorizationManagerBeforeMethodInterceptor( diff --git a/core/src/test/java/org/springframework/security/authorization/method/PostFilterAuthorizationMethodInterceptorTests.java b/core/src/test/java/org/springframework/security/authorization/method/PostFilterAuthorizationMethodInterceptorTests.java index aad3db83ce..00f2aed42d 100644 --- a/core/src/test/java/org/springframework/security/authorization/method/PostFilterAuthorizationMethodInterceptorTests.java +++ b/core/src/test/java/org/springframework/security/authorization/method/PostFilterAuthorizationMethodInterceptorTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2022 the original author or authors. + * Copyright 2002-2023 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. @@ -147,6 +147,29 @@ public class PostFilterAuthorizationMethodInterceptorTests { verify(strategy).getContext(); } + // gh-12877 + @Test + public void postFilterWhenStaticSecurityContextHolderStrategyAfterConstructorThenUses() throws Throwable { + SecurityContextHolderStrategy strategy = mock(SecurityContextHolderStrategy.class); + Authentication authentication = new TestingAuthenticationToken("john", "password", + AuthorityUtils.createAuthorityList("authority")); + given(strategy.getContext()).willReturn(new SecurityContextImpl(authentication)); + String[] array = { "john", "bob" }; + MockMethodInvocation invocation = new MockMethodInvocation(new TestClass(), TestClass.class, + "doSomethingArrayAuthentication", new Class[] { String[].class }, new Object[] { array }) { + @Override + public Object proceed() { + return array; + } + }; + PostFilterAuthorizationMethodInterceptor advice = new PostFilterAuthorizationMethodInterceptor(); + SecurityContextHolderStrategy saved = SecurityContextHolder.getContextHolderStrategy(); + SecurityContextHolder.setContextHolderStrategy(strategy); + advice.invoke(invocation); + verify(strategy).getContext(); + SecurityContextHolder.setContextHolderStrategy(saved); + } + @PostFilter("filterObject == 'john'") public static class TestClass implements InterfaceAnnotationsOne, InterfaceAnnotationsTwo { diff --git a/core/src/test/java/org/springframework/security/authorization/method/PreFilterAuthorizationMethodInterceptorTests.java b/core/src/test/java/org/springframework/security/authorization/method/PreFilterAuthorizationMethodInterceptorTests.java index 6d51063544..4f1d56fb14 100644 --- a/core/src/test/java/org/springframework/security/authorization/method/PreFilterAuthorizationMethodInterceptorTests.java +++ b/core/src/test/java/org/springframework/security/authorization/method/PreFilterAuthorizationMethodInterceptorTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2022 the original author or authors. + * Copyright 2002-2023 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. @@ -204,6 +204,26 @@ public class PreFilterAuthorizationMethodInterceptorTests { verify(strategy).getContext(); } + // gh-12877 + @Test + public void preFilterWhenStaticSecurityContextHolderStrategyAfterConstructorThenUses() throws Throwable { + SecurityContextHolderStrategy strategy = mock(SecurityContextHolderStrategy.class); + Authentication authentication = new TestingAuthenticationToken("john", "password", + AuthorityUtils.createAuthorityList("authority")); + given(strategy.getContext()).willReturn(new SecurityContextImpl(authentication)); + List list = new ArrayList<>(); + list.add("john"); + list.add("bob"); + MockMethodInvocation invocation = new MockMethodInvocation(new TestClass(), TestClass.class, + "doSomethingArrayFilterAuthentication", new Class[] { List.class }, new Object[] { list }); + PreFilterAuthorizationMethodInterceptor advice = new PreFilterAuthorizationMethodInterceptor(); + SecurityContextHolderStrategy saved = SecurityContextHolder.getContextHolderStrategy(); + SecurityContextHolder.setContextHolderStrategy(strategy); + advice.invoke(invocation); + verify(strategy).getContext(); + SecurityContextHolder.setContextHolderStrategy(saved); + } + @PreFilter("filterObject == 'john'") public static class TestClass implements InterfaceAnnotationsOne, InterfaceAnnotationsTwo {