diff --git a/core/src/main/java/org/springframework/security/core/context/ConstantSupplier.java b/core/src/main/java/org/springframework/security/core/context/ConstantSupplier.java new file mode 100644 index 00000000000..b695bfd4121 --- /dev/null +++ b/core/src/main/java/org/springframework/security/core/context/ConstantSupplier.java @@ -0,0 +1,42 @@ +/* + * Copyright 2004-present 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. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.security.core.context; + +import java.util.function.Supplier; + +import org.springframework.util.Assert; + +/** + * A {@link Supplier} of an already-materialized {@link SecurityContext}. Marker class so + * {@link SecurityContextHolderThreadLocalAccessor} can inspect the value without + * triggering deferred materialization. + */ +final class ConstantSupplier implements Supplier { + + private final SecurityContext context; + + ConstantSupplier(SecurityContext context) { + Assert.notNull(context, "context cannot be null"); + this.context = context; + } + + @Override + public SecurityContext get() { + return this.context; + } + +} diff --git a/core/src/main/java/org/springframework/security/core/context/GlobalSecurityContextHolderStrategy.java b/core/src/main/java/org/springframework/security/core/context/GlobalSecurityContextHolderStrategy.java index ded1a611b24..e477eeb36c5 100644 --- a/core/src/main/java/org/springframework/security/core/context/GlobalSecurityContextHolderStrategy.java +++ b/core/src/main/java/org/springframework/security/core/context/GlobalSecurityContextHolderStrategy.java @@ -16,6 +16,8 @@ package org.springframework.security.core.context; +import java.util.function.Supplier; + import org.jspecify.annotations.Nullable; import org.springframework.util.Assert; @@ -52,6 +54,12 @@ public void setContext(SecurityContext context) { contextHolder = context; } + @Override + public @Nullable Supplier peekDeferredContext() { + SecurityContext context = contextHolder; + return (context != null) ? new ConstantSupplier(context) : null; + } + @Override public SecurityContext createEmptyContext() { return new SecurityContextImpl(); diff --git a/core/src/main/java/org/springframework/security/core/context/InheritableThreadLocalSecurityContextHolderStrategy.java b/core/src/main/java/org/springframework/security/core/context/InheritableThreadLocalSecurityContextHolderStrategy.java index e64d9058d1f..1403bdb9e63 100644 --- a/core/src/main/java/org/springframework/security/core/context/InheritableThreadLocalSecurityContextHolderStrategy.java +++ b/core/src/main/java/org/springframework/security/core/context/InheritableThreadLocalSecurityContextHolderStrategy.java @@ -18,6 +18,8 @@ import java.util.function.Supplier; +import org.jspecify.annotations.Nullable; + import org.springframework.util.Assert; /** @@ -46,8 +48,7 @@ public SecurityContext getContext() { public Supplier getDeferredContext() { Supplier result = contextHolder.get(); if (result == null) { - SecurityContext context = createEmptyContext(); - result = () -> context; + result = new ConstantSupplier(createEmptyContext()); contextHolder.set(result); } return result; @@ -56,17 +57,15 @@ public Supplier getDeferredContext() { @Override public void setContext(SecurityContext context) { Assert.notNull(context, "Only non-null SecurityContext instances are permitted"); - contextHolder.set(() -> context); + contextHolder.set(new ConstantSupplier(context)); } @Override public void setDeferredContext(Supplier deferredContext) { Assert.notNull(deferredContext, "Only non-null Supplier instances are permitted"); - Supplier notNullDeferredContext = () -> { - SecurityContext result = deferredContext.get(); - Assert.notNull(result, "A Supplier returned null and is not allowed."); - return result; - }; + Supplier notNullDeferredContext = (deferredContext instanceof NotNullSupplier + || deferredContext instanceof ConstantSupplier) ? deferredContext + : new NotNullSupplier(deferredContext); contextHolder.set(notNullDeferredContext); } @@ -75,4 +74,9 @@ public SecurityContext createEmptyContext() { return new SecurityContextImpl(); } + @Override + public @Nullable Supplier peekDeferredContext() { + return contextHolder.get(); + } + } diff --git a/core/src/main/java/org/springframework/security/core/context/ListeningSecurityContextHolderStrategy.java b/core/src/main/java/org/springframework/security/core/context/ListeningSecurityContextHolderStrategy.java index 771684c786d..e3c36faa47f 100644 --- a/core/src/main/java/org/springframework/security/core/context/ListeningSecurityContextHolderStrategy.java +++ b/core/src/main/java/org/springframework/security/core/context/ListeningSecurityContextHolderStrategy.java @@ -21,6 +21,8 @@ import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Supplier; +import org.jspecify.annotations.Nullable; + import org.springframework.util.Assert; /** @@ -150,6 +152,14 @@ public Supplier getDeferredContext() { return this.delegate.getDeferredContext(); } + /** + * {@inheritDoc} + */ + @Override + public @Nullable Supplier peekDeferredContext() { + return this.delegate.peekDeferredContext(); + } + /** * {@inheritDoc} */ diff --git a/core/src/main/java/org/springframework/security/core/context/NotNullSupplier.java b/core/src/main/java/org/springframework/security/core/context/NotNullSupplier.java new file mode 100644 index 00000000000..a1e73e6e099 --- /dev/null +++ b/core/src/main/java/org/springframework/security/core/context/NotNullSupplier.java @@ -0,0 +1,44 @@ +/* + * Copyright 2004-present 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. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.security.core.context; + +import java.util.function.Supplier; + +import org.springframework.util.Assert; + +/** + * Wraps a {@link Supplier} to reject {@code null} results. Marker class so + * {@link SecurityContextHolderStrategy} can skip re-wrapping on round-trip — without it, + * {@code setDeferredContext(getDeferredContext())} would accumulate one wrapper per call. + */ +final class NotNullSupplier implements Supplier { + + private final Supplier delegate; + + NotNullSupplier(Supplier delegate) { + Assert.notNull(delegate, "delegate cannot be null"); + this.delegate = delegate; + } + + @Override + public SecurityContext get() { + SecurityContext result = this.delegate.get(); + Assert.notNull(result, "A Supplier returned null and is not allowed."); + return result; + } + +} diff --git a/core/src/main/java/org/springframework/security/core/context/SecurityContextHolderStrategy.java b/core/src/main/java/org/springframework/security/core/context/SecurityContextHolderStrategy.java index 6b23733667e..fbf3a999347 100644 --- a/core/src/main/java/org/springframework/security/core/context/SecurityContextHolderStrategy.java +++ b/core/src/main/java/org/springframework/security/core/context/SecurityContextHolderStrategy.java @@ -18,6 +18,8 @@ import java.util.function.Supplier; +import org.jspecify.annotations.Nullable; + /** * A strategy for storing security context information against a thread. * @@ -51,6 +53,23 @@ default Supplier getDeferredContext() { return this::getContext; } + /** + * Obtains the current context as a {@link Supplier} if one is associated with the + * current thread, or {@code null} otherwise. Used by Micrometer Context Propagation + * during snapshot capture, so implementations SHOULD return the stored + * {@link Supplier} without invoking it and without auto-creating or storing a default + * context as a side effect. + *

+ * The default implementation preserves the pre-7.1 propagation behavior: it + * materializes the current context eagerly and returns a {@link Supplier} of the + * captured context. + * @return the current context's {@link Supplier}, or {@code null} if none is set + * @since 7.1.1 + */ + default @Nullable Supplier peekDeferredContext() { + return new ConstantSupplier(getContext()); + } + /** * Sets the current context. * @param context to the new argument (should never be null, although diff --git a/core/src/main/java/org/springframework/security/core/context/SecurityContextHolderThreadLocalAccessor.java b/core/src/main/java/org/springframework/security/core/context/SecurityContextHolderThreadLocalAccessor.java index 170f260c19a..acbffd7f04a 100644 --- a/core/src/main/java/org/springframework/security/core/context/SecurityContextHolderThreadLocalAccessor.java +++ b/core/src/main/java/org/springframework/security/core/context/SecurityContextHolderThreadLocalAccessor.java @@ -16,6 +16,8 @@ package org.springframework.security.core.context; +import java.util.function.Supplier; + import io.micrometer.context.ThreadLocalAccessor; import org.jspecify.annotations.Nullable; @@ -30,12 +32,17 @@ * {@link SecurityContext} in Servlet applications. It is automatically registered with * the {@link io.micrometer.context.ContextRegistry} through the * {@link java.util.ServiceLoader} mechanism when context-propagation is on the classpath. + *

+ * The propagated value is the deferred {@link Supplier}, so capture does not materialize + * the {@link SecurityContext}; the supplier is invoked lazily on the consuming thread. An + * already-materialized {@link SecurityContext} that equals the empty context is treated + * as absent and is not propagated. * * @author Steve Riesenberg * @since 6.5 * @see io.micrometer.context.ContextRegistry */ -public final class SecurityContextHolderThreadLocalAccessor implements ThreadLocalAccessor { +public final class SecurityContextHolderThreadLocalAccessor implements ThreadLocalAccessor> { @Override public Object key() { @@ -43,17 +50,21 @@ public Object key() { } @Override - public @Nullable SecurityContext getValue() { - SecurityContext securityContext = SecurityContextHolder.getContext(); - SecurityContext emptyContext = SecurityContextHolder.createEmptyContext(); - - return !securityContext.equals(emptyContext) ? securityContext : null; + public @Nullable Supplier getValue() { + Supplier deferred = SecurityContextHolder.getContextHolderStrategy().peekDeferredContext(); + // The empty check is only possible when the context is already materialized; + // invoking a deferred supplier here would reintroduce gh-18059. + if (deferred instanceof ConstantSupplier constant + && constant.get().equals(SecurityContextHolder.createEmptyContext())) { + return null; + } + return deferred; } @Override - public void setValue(SecurityContext securityContext) { + public void setValue(Supplier securityContext) { Assert.notNull(securityContext, "securityContext cannot be null"); - SecurityContextHolder.setContext(securityContext); + SecurityContextHolder.getContextHolderStrategy().setDeferredContext(securityContext); } @Override diff --git a/core/src/main/java/org/springframework/security/core/context/ThreadLocalSecurityContextHolderStrategy.java b/core/src/main/java/org/springframework/security/core/context/ThreadLocalSecurityContextHolderStrategy.java index f4fb5689c02..27ba2a6ddc2 100644 --- a/core/src/main/java/org/springframework/security/core/context/ThreadLocalSecurityContextHolderStrategy.java +++ b/core/src/main/java/org/springframework/security/core/context/ThreadLocalSecurityContextHolderStrategy.java @@ -18,6 +18,8 @@ import java.util.function.Supplier; +import org.jspecify.annotations.Nullable; + import org.springframework.util.Assert; /** @@ -47,8 +49,7 @@ public SecurityContext getContext() { public Supplier getDeferredContext() { Supplier result = contextHolder.get(); if (result == null) { - SecurityContext context = createEmptyContext(); - result = () -> context; + result = new ConstantSupplier(createEmptyContext()); contextHolder.set(result); } return result; @@ -57,17 +58,15 @@ public Supplier getDeferredContext() { @Override public void setContext(SecurityContext context) { Assert.notNull(context, "Only non-null SecurityContext instances are permitted"); - contextHolder.set(() -> context); + contextHolder.set(new ConstantSupplier(context)); } @Override public void setDeferredContext(Supplier deferredContext) { Assert.notNull(deferredContext, "Only non-null Supplier instances are permitted"); - Supplier notNullDeferredContext = () -> { - SecurityContext result = deferredContext.get(); - Assert.notNull(result, "A Supplier returned null and is not allowed."); - return result; - }; + Supplier notNullDeferredContext = (deferredContext instanceof NotNullSupplier + || deferredContext instanceof ConstantSupplier) ? deferredContext + : new NotNullSupplier(deferredContext); contextHolder.set(notNullDeferredContext); } @@ -76,4 +75,9 @@ public SecurityContext createEmptyContext() { return new SecurityContextImpl(); } + @Override + public @Nullable Supplier peekDeferredContext() { + return contextHolder.get(); + } + } diff --git a/core/src/test/java/org/springframework/security/core/context/GlobalSecurityContextHolderStrategyTests.java b/core/src/test/java/org/springframework/security/core/context/GlobalSecurityContextHolderStrategyTests.java new file mode 100644 index 00000000000..a3bca8f3b41 --- /dev/null +++ b/core/src/test/java/org/springframework/security/core/context/GlobalSecurityContextHolderStrategyTests.java @@ -0,0 +1,64 @@ +/* + * Copyright 2004-present 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. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.security.core.context; + +import java.util.function.Supplier; + +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; + +import org.springframework.security.core.Authentication; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; + +class GlobalSecurityContextHolderStrategyTests { + + GlobalSecurityContextHolderStrategy strategy = new GlobalSecurityContextHolderStrategy(); + + @AfterEach + void clearContext() { + this.strategy.clearContext(); + } + + // gh-18059 + @Test + void peekDeferredContextWhenEmptyThenReturnsNull() { + assertThat(this.strategy.peekDeferredContext()).isNull(); + } + + // gh-18059 + @Test + void peekDeferredContextWhenSetThenReturnsSupplierOfSameContext() { + Authentication authentication = mock(Authentication.class); + SecurityContext context = new SecurityContextImpl(authentication); + this.strategy.setContext(context); + Supplier deferred = this.strategy.peekDeferredContext(); + assertThat(deferred).isNotNull(); + assertThat(deferred.get()).isSameAs(context); + } + + // gh-18059 + @Test + void peekDeferredContextWhenContextAutoCreatedThenReturnsSupplierOfSameContext() { + SecurityContext context = this.strategy.getContext(); + Supplier deferred = this.strategy.peekDeferredContext(); + assertThat(deferred).isNotNull(); + assertThat(deferred.get()).isSameAs(context); + } + +} diff --git a/core/src/test/java/org/springframework/security/core/context/InheritableThreadLocalSecurityContextHolderStrategyTests.java b/core/src/test/java/org/springframework/security/core/context/InheritableThreadLocalSecurityContextHolderStrategyTests.java index e1961240de6..0cb45a20bce 100644 --- a/core/src/test/java/org/springframework/security/core/context/InheritableThreadLocalSecurityContextHolderStrategyTests.java +++ b/core/src/test/java/org/springframework/security/core/context/InheritableThreadLocalSecurityContextHolderStrategyTests.java @@ -81,4 +81,58 @@ void getContextWhenEmptyThenReturnsSameInstance() { assertThat(this.strategy.getContext().getAuthentication()).isEqualTo(authentication); } + @Test + void setDeferredContextWhenAlreadyWrappedThenDoesNotRewrap() { + Authentication authentication = mock(Authentication.class); + this.strategy.setDeferredContext(() -> new SecurityContextImpl(authentication)); + Supplier wrapped = this.strategy.getDeferredContext(); + this.strategy.setDeferredContext(wrapped); + assertThat(this.strategy.getDeferredContext()).isSameAs(wrapped); + } + + // gh-18059 + @Test + void setDeferredContextWhenAlreadyMaterializedThenDoesNotRewrap() { + Authentication authentication = mock(Authentication.class); + this.strategy.setContext(new SecurityContextImpl(authentication)); + Supplier materialized = this.strategy.getDeferredContext(); + this.strategy.setDeferredContext(materialized); + assertThat(this.strategy.getDeferredContext()).isSameAs(materialized); + } + + // gh-18059 + @Test + void peekDeferredContextWhenNotSetThenReturnsNull() { + assertThat(this.strategy.peekDeferredContext()).isNull(); + } + + // gh-18059 + @Test + void peekDeferredContextWhenContextAutoCreatedThenReturnsSupplierOfSameContext() { + SecurityContext context = this.strategy.getContext(); + Supplier deferred = this.strategy.peekDeferredContext(); + assertThat(deferred).isNotNull(); + assertThat(deferred.get()).isSameAs(context); + } + + // gh-18059 + @Test + void peekDeferredContextWhenContextSetThenReturnsSupplierOfSameContext() { + Authentication authentication = mock(Authentication.class); + SecurityContext context = new SecurityContextImpl(authentication); + this.strategy.setContext(context); + Supplier deferred = this.strategy.peekDeferredContext(); + assertThat(deferred).isNotNull(); + assertThat(deferred.get()).isSameAs(context); + } + + // gh-18059 + @Test + void peekDeferredContextWhenDeferredContextSetThenReturnsSameSupplierWithoutInvoking() { + Supplier deferredContext = mock(Supplier.class); + this.strategy.setDeferredContext(deferredContext); + assertThat(this.strategy.peekDeferredContext()).isSameAs(this.strategy.getDeferredContext()); + verifyNoInteractions(deferredContext); + } + } diff --git a/core/src/test/java/org/springframework/security/core/context/ListeningSecurityContextHolderStrategyTests.java b/core/src/test/java/org/springframework/security/core/context/ListeningSecurityContextHolderStrategyTests.java index 22a9e6a596e..733972624b2 100644 --- a/core/src/test/java/org/springframework/security/core/context/ListeningSecurityContextHolderStrategyTests.java +++ b/core/src/test/java/org/springframework/security/core/context/ListeningSecurityContextHolderStrategyTests.java @@ -134,4 +134,19 @@ public void constructorWhenNullListenerThenIllegalArgument() { (SecurityContextChangedListener) null)); } + // gh-18059 + @Test + public void peekDeferredContextDelegatesToUnderlyingStrategy() { + SecurityContextHolderStrategy delegate = mock(SecurityContextHolderStrategy.class); + SecurityContextChangedListener listener = mock(SecurityContextChangedListener.class); + SecurityContextHolderStrategy strategy = new ListeningSecurityContextHolderStrategy(delegate, listener); + + given(delegate.peekDeferredContext()).willReturn(null); + assertThat(strategy.peekDeferredContext()).isNull(); + + Supplier supplier = () -> new SecurityContextImpl(); + given(delegate.peekDeferredContext()).willReturn(supplier); + assertThat(strategy.peekDeferredContext()).isSameAs(supplier); + } + } diff --git a/core/src/test/java/org/springframework/security/core/context/SecurityContextHolderStrategyTests.java b/core/src/test/java/org/springframework/security/core/context/SecurityContextHolderStrategyTests.java new file mode 100644 index 00000000000..9e271541fae --- /dev/null +++ b/core/src/test/java/org/springframework/security/core/context/SecurityContextHolderStrategyTests.java @@ -0,0 +1,89 @@ +/* + * Copyright 2004-present 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. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.security.core.context; + +import java.util.function.Supplier; + +import org.jspecify.annotations.Nullable; +import org.junit.jupiter.api.Test; + +import org.springframework.security.core.Authentication; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; + +/** + * Tests for the {@link SecurityContextHolderStrategy} default methods. + */ +class SecurityContextHolderStrategyTests { + + LegacyStrategy strategy = new LegacyStrategy(); + + // gh-18059 + @Test + void peekDeferredContextWhenNotSetThenReturnsSupplierOfMaterializedContext() { + Supplier deferred = this.strategy.peekDeferredContext(); + assertThat(deferred).isNotNull(); + assertThat(deferred.get()).isSameAs(this.strategy.getContext()); + } + + // gh-18059 + @Test + void peekDeferredContextWhenContextSetThenReturnsSupplierOfCapturedContext() { + Authentication authentication = mock(Authentication.class); + SecurityContext context = new SecurityContextImpl(authentication); + this.strategy.setContext(context); + Supplier deferred = this.strategy.peekDeferredContext(); + assertThat(deferred).isNotNull(); + // the default captures the value eagerly, so it is not a live view + this.strategy.clearContext(); + assertThat(deferred.get()).isSameAs(context); + } + + /** + * A pre-5.8 style implementation relying on the interface default methods. + */ + static final class LegacyStrategy implements SecurityContextHolderStrategy { + + private @Nullable SecurityContext context; + + @Override + public void clearContext() { + this.context = null; + } + + @Override + public SecurityContext getContext() { + if (this.context == null) { + this.context = createEmptyContext(); + } + return this.context; + } + + @Override + public void setContext(SecurityContext context) { + this.context = context; + } + + @Override + public SecurityContext createEmptyContext() { + return new SecurityContextImpl(); + } + + } + +} diff --git a/core/src/test/java/org/springframework/security/core/context/SecurityContextHolderThreadLocalAccessorTests.java b/core/src/test/java/org/springframework/security/core/context/SecurityContextHolderThreadLocalAccessorTests.java index e04d0277ff5..7bcbdb189e3 100644 --- a/core/src/test/java/org/springframework/security/core/context/SecurityContextHolderThreadLocalAccessorTests.java +++ b/core/src/test/java/org/springframework/security/core/context/SecurityContextHolderThreadLocalAccessorTests.java @@ -16,14 +16,29 @@ package org.springframework.security.core.context; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Supplier; + +import io.micrometer.context.ContextExecutorService; +import io.micrometer.context.ContextSnapshot; +import io.micrometer.context.ContextSnapshotFactory; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import reactor.util.context.Context; import org.springframework.security.authentication.TestingAuthenticationToken; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verifyNoInteractions; /** * Tests for {@link SecurityContextHolderThreadLocalAccessor}. @@ -55,21 +70,67 @@ public void getValueWhenSecurityContextHolderNotSetThenReturnsNull() { } @Test - public void getValueWhenSecurityContextHolderSetThenReturnsSecurityContext() { + public void getValueWhenSecurityContextHolderSetThenReturnsSupplierOfSecurityContext() { SecurityContext securityContext = SecurityContextHolder.createEmptyContext(); securityContext.setAuthentication(new TestingAuthenticationToken("user", "password")); SecurityContextHolder.setContext(securityContext); - assertThat(this.threadLocalAccessor.getValue()).isSameAs(securityContext); + Supplier deferred = this.threadLocalAccessor.getValue(); + assertThat(deferred).isNotNull(); + assertThat(deferred.get()).isSameAs(securityContext); + } + + // gh-18059 + @Test + public void getValueWhenContextAutoCreatedThenReturnsNull() { + SecurityContextHolder.getContext(); + assertThat(this.threadLocalAccessor.getValue()).isNull(); + } + + // gh-18059 + @Test + public void getValueWhenEmptyContextSetThenReturnsNull() { + SecurityContextHolder.setContext(SecurityContextHolder.createEmptyContext()); + assertThat(this.threadLocalAccessor.getValue()).isNull(); + } + + // gh-18059 + @Test + public void getValueWhenAutoCreatedContextModifiedThenReturnsSupplier() { + SecurityContext securityContext = SecurityContextHolder.getContext(); + securityContext.setAuthentication(new TestingAuthenticationToken("user", "password")); + Supplier deferred = this.threadLocalAccessor.getValue(); + assertThat(deferred).isNotNull(); + assertThat(deferred.get()).isSameAs(securityContext); + } + + // gh-18059 + @Test + @SuppressWarnings("unchecked") + public void getValueWhenDeferredContextSetThenDoesNotInvokeSupplier() { + Supplier deferredContext = mock(Supplier.class); + SecurityContextHolder.setDeferredContext(deferredContext); + Supplier result = this.threadLocalAccessor.getValue(); + assertThat(result).isNotNull(); + verifyNoInteractions(deferredContext); } @Test - public void setValueWhenSecurityContextThenSetsSecurityContextHolder() { + public void setValueWhenSupplierThenSetsSecurityContextHolder() { SecurityContext securityContext = SecurityContextHolder.createEmptyContext(); securityContext.setAuthentication(new TestingAuthenticationToken("user", "password")); - this.threadLocalAccessor.setValue(securityContext); + this.threadLocalAccessor.setValue(() -> securityContext); assertThat(SecurityContextHolder.getContext()).isSameAs(securityContext); } + // gh-18059 + @Test + @SuppressWarnings("unchecked") + public void setValueWhenSupplierThenDoesNotInvokeSupplier() { + Supplier deferredContext = mock(Supplier.class); + this.threadLocalAccessor.setValue(deferredContext); + verifyNoInteractions(deferredContext); + } + @Test public void setValueWhenNullThenThrowsIllegalArgumentException() { // @formatter:off @@ -90,4 +151,151 @@ public void setValueWhenSecurityContextSetThenClearsSecurityContextHolder() { assertThat(SecurityContextHolder.getContext()).isEqualTo(emptyContext); } + // gh-18059 + @Test + public void captureAllWhenDeferredContextSetThenDoesNotInvokeSupplier() { + SecurityContext securityContext = new SecurityContextImpl(new TestingAuthenticationToken("user", "password")); + AtomicInteger invocationCount = new AtomicInteger(); + SecurityContextHolder.setDeferredContext(() -> { + invocationCount.incrementAndGet(); + return securityContext; + }); + + ContextSnapshotFactory factory = ContextSnapshotFactory.builder().build(); + factory.captureAll(); + + assertThat(invocationCount.get()).as("snapshot capture must not invoke the deferred supplier").isZero(); + } + + // gh-18059 + @Test + public void setThreadLocalsFromWhenContextMissingKeyThenDoesNotInvokeSupplier() { + // The branch from the issue's stack trace: clearMissing=true and no + // SecurityContext key in the Reactor Context invokes accessor.getValue() + // to save the previous value before clearing. + SecurityContext securityContext = new SecurityContextImpl(new TestingAuthenticationToken("user", "password")); + AtomicInteger invocationCount = new AtomicInteger(); + SecurityContextHolder.setDeferredContext(() -> { + invocationCount.incrementAndGet(); + return securityContext; + }); + Supplier before = this.threadLocalAccessor.getValue(); + + ContextSnapshotFactory factory = ContextSnapshotFactory.builder().clearMissing(true).build(); + try (ContextSnapshot.Scope scope = factory.setThreadLocalsFrom(Context.empty())) { + assertThat(this.threadLocalAccessor.getValue()).as("cleared inside the scope").isNull(); + } + + assertThat(this.threadLocalAccessor.getValue()).as("restored on scope close, not re-wrapped").isSameAs(before); + assertThat(invocationCount.get()).as("leaving the scope must not invoke the deferred supplier").isZero(); + } + + // gh-18059 + @Test + public void captureAllWhenDeferredContextReentersPropagationThenDoesNotRecurse() { + // Models the issue's Lettuce/Redis flow: materializing the deferred context + // itself triggers Mono.subscribe -> context propagation -> accessor.getValue(). + // Bounded so a regression fails the assertion instead of a StackOverflowError. + SecurityContext securityContext = new SecurityContextImpl(new TestingAuthenticationToken("user", "password")); + ContextSnapshotFactory factory = ContextSnapshotFactory.builder().build(); + AtomicInteger invocationCount = new AtomicInteger(); + SecurityContextHolder.setDeferredContext(() -> { + if (invocationCount.incrementAndGet() <= 3) { + factory.captureAll(); + } + return securityContext; + }); + + factory.captureAll(); + + assertThat(invocationCount.get()).as("snapshot capture must not invoke the deferred supplier, even reentrantly") + .isZero(); + } + + // gh-18059 + @Test + public void getValueWhenDeferredContextReentersAccessorThenDoesNotRecurseAndReturnsCallableSupplier() { + SecurityContext securityContext = new SecurityContextImpl(new TestingAuthenticationToken("user", "password")); + AtomicInteger invocationCount = new AtomicInteger(); + Supplier reentrantSupplier = () -> { + // Bound recursion so a regression fails the assertion instead of + // crashing the JVM with StackOverflowError. + if (invocationCount.incrementAndGet() > 1) { + return securityContext; + } + Supplier nested = this.threadLocalAccessor.getValue(); + if (nested != null) { + nested.get(); + } + return securityContext; + }; + SecurityContextHolder.setDeferredContext(reentrantSupplier); + + Supplier result = this.threadLocalAccessor.getValue(); + + assertThat(result).isNotNull(); + assertThat(invocationCount.get()).isZero(); + assertThat(result.get()).isSameAs(securityContext); + } + + // gh-18059 + @Test + public void contextPropagationToWorkerThreadDoesNotMaterializeDeferredSupplier() + throws ExecutionException, InterruptedException, TimeoutException { + // Cross-thread propagation via ContextExecutorService: micrometer captures on + // submit() and applies the snapshot on the worker thread. + SecurityContext securityContext = new SecurityContextImpl(new TestingAuthenticationToken("user", "password")); + AtomicInteger invocationCount = new AtomicInteger(); + Supplier deferredSupplier = () -> { + invocationCount.incrementAndGet(); + return securityContext; + }; + + ExecutorService raw = Executors.newSingleThreadExecutor(); + ContextSnapshotFactory factory = ContextSnapshotFactory.builder().build(); + ExecutorService propagating = ContextExecutorService.wrap(raw, factory); + try { + assertThat(raw.submit(this.threadLocalAccessor::getValue).get(5, TimeUnit.SECONDS)) + .as("worker thread initially has no SecurityContext") + .isNull(); + + SecurityContextHolder.setDeferredContext(deferredSupplier); + Supplier captured = this.threadLocalAccessor.getValue(); + + AtomicInteger invocationsAtTaskStart = new AtomicInteger(-1); + AtomicReference> workerSupplier = new AtomicReference<>(); + AtomicReference workerContext = new AtomicReference<>(); + AtomicInteger invocationsAfterMaterialization = new AtomicInteger(-1); + + propagating.submit(() -> { + invocationsAtTaskStart.set(invocationCount.get()); + workerSupplier.set(this.threadLocalAccessor.getValue()); + workerContext.set(SecurityContextHolder.getContext()); + invocationsAfterMaterialization.set(invocationCount.get()); + }).get(5, TimeUnit.SECONDS); + + assertThat(invocationsAtTaskStart.get()) + .as("micrometer's capture+apply path must not invoke the deferred supplier") + .isZero(); + assertThat(workerSupplier.get()) + .as("worker thread receives the same supplier instance captured on the submitter") + .isSameAs(captured); + assertThat(workerContext.get()) + .as("SecurityContextHolder.getContext() on the worker materializes the supplier") + .isSameAs(securityContext); + assertThat(invocationsAfterMaterialization.get()) + .as("materialization on the worker invokes the supplier exactly once") + .isOne(); + + // Probe via the unwrapped executor so this submission does not propagate. + assertThat(raw.submit(this.threadLocalAccessor::getValue).get(5, TimeUnit.SECONDS)) + .as("worker thread's prior (empty) state must be restored after the task") + .isNull(); + } + finally { + propagating.shutdown(); + raw.shutdownNow(); + } + } + } diff --git a/core/src/test/java/org/springframework/security/core/context/ThreadLocalSecurityContextHolderStrategyTests.java b/core/src/test/java/org/springframework/security/core/context/ThreadLocalSecurityContextHolderStrategyTests.java index 23c238ac732..79de7897909 100644 --- a/core/src/test/java/org/springframework/security/core/context/ThreadLocalSecurityContextHolderStrategyTests.java +++ b/core/src/test/java/org/springframework/security/core/context/ThreadLocalSecurityContextHolderStrategyTests.java @@ -81,4 +81,58 @@ void getContextWhenEmptyThenReturnsSameInstance() { assertThat(this.strategy.getContext().getAuthentication()).isEqualTo(authentication); } + @Test + void setDeferredContextWhenAlreadyWrappedThenDoesNotRewrap() { + Authentication authentication = mock(Authentication.class); + this.strategy.setDeferredContext(() -> new SecurityContextImpl(authentication)); + Supplier wrapped = this.strategy.getDeferredContext(); + this.strategy.setDeferredContext(wrapped); + assertThat(this.strategy.getDeferredContext()).isSameAs(wrapped); + } + + // gh-18059 + @Test + void setDeferredContextWhenAlreadyMaterializedThenDoesNotRewrap() { + Authentication authentication = mock(Authentication.class); + this.strategy.setContext(new SecurityContextImpl(authentication)); + Supplier materialized = this.strategy.getDeferredContext(); + this.strategy.setDeferredContext(materialized); + assertThat(this.strategy.getDeferredContext()).isSameAs(materialized); + } + + // gh-18059 + @Test + void peekDeferredContextWhenNotSetThenReturnsNull() { + assertThat(this.strategy.peekDeferredContext()).isNull(); + } + + // gh-18059 + @Test + void peekDeferredContextWhenContextAutoCreatedThenReturnsSupplierOfSameContext() { + SecurityContext context = this.strategy.getContext(); + Supplier deferred = this.strategy.peekDeferredContext(); + assertThat(deferred).isNotNull(); + assertThat(deferred.get()).isSameAs(context); + } + + // gh-18059 + @Test + void peekDeferredContextWhenContextSetThenReturnsSupplierOfSameContext() { + Authentication authentication = mock(Authentication.class); + SecurityContext context = new SecurityContextImpl(authentication); + this.strategy.setContext(context); + Supplier deferred = this.strategy.peekDeferredContext(); + assertThat(deferred).isNotNull(); + assertThat(deferred.get()).isSameAs(context); + } + + // gh-18059 + @Test + void peekDeferredContextWhenDeferredContextSetThenReturnsSameSupplierWithoutInvoking() { + Supplier deferredContext = mock(Supplier.class); + this.strategy.setDeferredContext(deferredContext); + assertThat(this.strategy.peekDeferredContext()).isSameAs(this.strategy.getDeferredContext()); + verifyNoInteractions(deferredContext); + } + } diff --git a/docs/modules/ROOT/pages/whats-new.adoc b/docs/modules/ROOT/pages/whats-new.adoc index 76314d2c876..7931fdcea2e 100644 --- a/docs/modules/ROOT/pages/whats-new.adoc +++ b/docs/modules/ROOT/pages/whats-new.adoc @@ -5,6 +5,7 @@ * https://github.com/spring-projects/spring-security/pull/18634[gh-18634] - Added javadoc:org.springframework.security.util.matcher.InetAddressMatcher[] * https://github.com/spring-projects/spring-security/issues/18960[gh-18960] - Added xref:servlet/authentication/mfa.adoc#all-factors-anyof[AllRequiredFactorsAuthorizationManager.anyOf] +* https://github.com/spring-projects/spring-security/issues/18059[gh-18059] - `SecurityContextHolderThreadLocalAccessor` now propagates the deferred `SecurityContext` supplier without materializing it during context snapshot capture == Web * https://github.com/spring-projects/spring-security/issues/18755[gh-18755] - Include `charset` in `WWW-Authenticate` header