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 fcdad71b1c9..0284710ccf2 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 @@ 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 boolean isPerInstance() { * @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 @@ private void attemptAuthorization(MethodInvocation mi, Object result) { 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 36ccdfd6a6c..239c723cea5 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 @@ 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 boolean isPerInstance() { * @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 @@ private void attemptAuthorization(MethodInvocation mi) { 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 4e58611b6d2..99630298a23 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 @@ 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 boolean isPerInstance() { * @since 5.8 */ public void setSecurityContextHolderStrategy(SecurityContextHolderStrategy strategy) { - this.authentication = getAuthentication(strategy); + this.securityContextHolderStrategy = () -> strategy; } /** @@ -125,19 +124,17 @@ public Object invoke(MethodInvocation mi) throws Throwable { 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 a0a9e30171b..bde5f1ee9d2 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 @@ 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 boolean isPerInstance() { * @since 5.8 */ public void setSecurityContextHolderStrategy(SecurityContextHolderStrategy strategy) { - this.authentication = getAuthentication(strategy); + this.securityContextHolderStrategy = () -> strategy; } /** @@ -124,7 +123,7 @@ public Object invoke(MethodInvocation mi) throws Throwable { 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 @@ private Object findFilterTarget(String filterTargetName, EvaluationContext ctx, 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 adbfb9d6e12..b82785f1017 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.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 void afterWhenMockSecurityContextHolderStrategyThenUses() throws Throwabl 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 af922fc4e86..210a70e0b4e 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 void beforeWhenMockSecurityContextHolderStrategyThenUses() throws Throwab 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 aad3db83ceb..00f2aed42dd 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 Object proceed() { 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 6d51063544c..4f1d56fb146 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 void preFilterWhenMockSecurityContextHolderStrategyThenUses() throws Thro 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 {