From 1a5f29ca9b04686667f542b916659bc1f7e542b5 Mon Sep 17 00:00:00 2001 From: Max Batischev Date: Fri, 11 Oct 2024 17:39:20 +0300 Subject: [PATCH] Add AuthorizationResult support for AuthorizationManager Closes gh-14843 --- .../PrePostMethodSecurityConfigurationTests.java | 4 ++++ .../AuthorizeHttpRequestsConfigurerTests.java | 6 +++++- .../config/http/DefaultFilterChainValidatorTests.java | 3 ++- .../websocket/WebSocketMessageBrokerConfigTests.java | 1 + .../authorization/AuthorizationEventPublisher.java | 7 ++----- .../authorization/AuthorizationObservationContext.java | 8 ++++++-- .../authorization/ObservationAuthorizationManager.java | 4 ++-- .../ObservationReactiveAuthorizationManager.java | 4 ++-- .../AuthorizationChannelInterceptorTests.java | 10 +++++++++- 9 files changed, 33 insertions(+), 14 deletions(-) diff --git a/config/src/test/java/org/springframework/security/config/annotation/method/configuration/PrePostMethodSecurityConfigurationTests.java b/config/src/test/java/org/springframework/security/config/annotation/method/configuration/PrePostMethodSecurityConfigurationTests.java index 73e65c78a7f..17d8f8a3a92 100644 --- a/config/src/test/java/org/springframework/security/config/annotation/method/configuration/PrePostMethodSecurityConfigurationTests.java +++ b/config/src/test/java/org/springframework/security/config/annotation/method/configuration/PrePostMethodSecurityConfigurationTests.java @@ -75,6 +75,7 @@ import org.springframework.security.authorization.AuthorizationDecision; import org.springframework.security.authorization.AuthorizationEventPublisher; import org.springframework.security.authorization.AuthorizationManager; +import org.springframework.security.authorization.AuthorizationResult; import org.springframework.security.authorization.method.AuthorizationAdvisor; import org.springframework.security.authorization.method.AuthorizationAdvisorProxyFactory; import org.springframework.security.authorization.method.AuthorizationAdvisorProxyFactory.TargetVisitor; @@ -110,6 +111,7 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.atLeastOnce; import static org.mockito.Mockito.clearInvocations; +import static org.mockito.Mockito.doCallRealMethod; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.spy; @@ -1293,6 +1295,8 @@ static class AuthorizationEventPublisherConfig { @Bean AuthorizationEventPublisher authorizationEventPublisher() { + doCallRealMethod().when(this.publisher) + .publishAuthorizationEvent(any(), any(), any(AuthorizationResult.class)); return this.publisher; } diff --git a/config/src/test/java/org/springframework/security/config/annotation/web/configurers/AuthorizeHttpRequestsConfigurerTests.java b/config/src/test/java/org/springframework/security/config/annotation/web/configurers/AuthorizeHttpRequestsConfigurerTests.java index 168f89f1374..68c752a3943 100644 --- a/config/src/test/java/org/springframework/security/config/annotation/web/configurers/AuthorizeHttpRequestsConfigurerTests.java +++ b/config/src/test/java/org/springframework/security/config/annotation/web/configurers/AuthorizeHttpRequestsConfigurerTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2023 the original author or authors. + * Copyright 2002-2024 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. @@ -42,6 +42,7 @@ import org.springframework.security.authorization.AuthorizationEventPublisher; import org.springframework.security.authorization.AuthorizationManager; import org.springframework.security.authorization.AuthorizationObservationContext; +import org.springframework.security.authorization.AuthorizationResult; import org.springframework.security.config.ObjectPostProcessor; import org.springframework.security.config.annotation.web.AbstractRequestMatcherRegistry; import org.springframework.security.config.annotation.web.builders.HttpSecurity; @@ -78,6 +79,7 @@ import static org.assertj.core.api.Assertions.assertThatExceptionOfType; import static org.mockito.Mockito.any; import static org.mockito.Mockito.atLeastOnce; +import static org.mockito.Mockito.doCallRealMethod; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.verify; @@ -1241,6 +1243,8 @@ static class AuthorizationEventPublisherConfig { @Bean AuthorizationEventPublisher authorizationEventPublisher() { + doCallRealMethod().when(this.publisher) + .publishAuthorizationEvent(any(), any(), any(AuthorizationResult.class)); return this.publisher; } diff --git a/config/src/test/java/org/springframework/security/config/http/DefaultFilterChainValidatorTests.java b/config/src/test/java/org/springframework/security/config/http/DefaultFilterChainValidatorTests.java index a20fe5397f1..a5b899db48c 100644 --- a/config/src/test/java/org/springframework/security/config/http/DefaultFilterChainValidatorTests.java +++ b/config/src/test/java/org/springframework/security/config/http/DefaultFilterChainValidatorTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2022 the original author or authors. + * Copyright 2002-2024 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. @@ -113,6 +113,7 @@ public void validateCheckLoginPageIsntProtectedThrowsIllegalArgumentException() @Test public void validateCheckLoginPageAllowsAnonymous() { given(this.authorizationManager.check(any(), any())).willReturn(new AuthorizationDecision(false)); + given(this.authorizationManager.authorize(any(), any())).willCallRealMethod(); this.validator.validate(this.chainAuthorizationFilter); verify(this.logger).warn("Anonymous access to the login page doesn't appear to be enabled. " + "This is almost certainly an error. Please check your configuration allows unauthenticated " diff --git a/config/src/test/java/org/springframework/security/config/websocket/WebSocketMessageBrokerConfigTests.java b/config/src/test/java/org/springframework/security/config/websocket/WebSocketMessageBrokerConfigTests.java index 7a5e75e9385..6e999933a28 100644 --- a/config/src/test/java/org/springframework/security/config/websocket/WebSocketMessageBrokerConfigTests.java +++ b/config/src/test/java/org/springframework/security/config/websocket/WebSocketMessageBrokerConfigTests.java @@ -514,6 +514,7 @@ public void sendWhenCustomAuthorizationManagerThenAuthorizesAccordingly() { AuthorizationManager> authorizationManager = this.spring.getContext() .getBean(AuthorizationManager.class); given(authorizationManager.check(any(), any())).willReturn(new AuthorizationDecision(false)); + given(authorizationManager.authorize(any(), any())).willCallRealMethod(); Message message = message("/any"); assertThatExceptionOfType(Exception.class).isThrownBy(send(message)) .withCauseInstanceOf(AccessDeniedException.class); diff --git a/core/src/main/java/org/springframework/security/authorization/AuthorizationEventPublisher.java b/core/src/main/java/org/springframework/security/authorization/AuthorizationEventPublisher.java index 67eb71293a1..56b41e59a09 100644 --- a/core/src/main/java/org/springframework/security/authorization/AuthorizationEventPublisher.java +++ b/core/src/main/java/org/springframework/security/authorization/AuthorizationEventPublisher.java @@ -64,13 +64,10 @@ void publishAuthorizationEvent(Supplier authentication, T ob */ default void publishAuthorizationEvent(Supplier authentication, T object, AuthorizationResult decision) { - if (decision == null) { - return; - } - if (!(decision instanceof AuthorizationDecision result)) { + if (decision != null && !(decision instanceof AuthorizationDecision)) { throw new UnsupportedOperationException("Decision must be of type AuthorizationDecision"); } - publishAuthorizationEvent(authentication, object, result); + publishAuthorizationEvent(authentication, object, (AuthorizationDecision) decision); } } diff --git a/core/src/main/java/org/springframework/security/authorization/AuthorizationObservationContext.java b/core/src/main/java/org/springframework/security/authorization/AuthorizationObservationContext.java index de37d30e089..8374bd6e343 100644 --- a/core/src/main/java/org/springframework/security/authorization/AuthorizationObservationContext.java +++ b/core/src/main/java/org/springframework/security/authorization/AuthorizationObservationContext.java @@ -73,9 +73,11 @@ public T getObject() { /** * Get the observed {@link AuthorizationDecision} * @return the observed {@link AuthorizationDecision} + * @deprecated please use {@link #getAuthorizationResult()} instead */ + @Deprecated public AuthorizationDecision getDecision() { - Assert.isInstanceOf(AuthorizationDecision.class, + Assert.isInstanceOf(AuthorizationDecision.class, this.authorizationResult, "Please call getAuthorizationResult instead. If you must call getDecision, please ensure that the result you provide is of type AuthorizationDecision"); return (AuthorizationDecision) this.authorizationResult; } @@ -83,10 +85,12 @@ public AuthorizationDecision getDecision() { /** * Set the observed {@link AuthorizationDecision} * @param decision the observed {@link AuthorizationDecision} - * @deprecated + * @deprecated please use {@link #setAuthorizationResult(AuthorizationResult)} instead */ @Deprecated public void setDecision(AuthorizationDecision decision) { + Assert.isInstanceOf(AuthorizationDecision.class, decision, + "Please call setAuthorizationResult instead. If you must call getDecision, please ensure that the result you provide is of type AuthorizationDecision"); this.decision = decision; } diff --git a/core/src/main/java/org/springframework/security/authorization/ObservationAuthorizationManager.java b/core/src/main/java/org/springframework/security/authorization/ObservationAuthorizationManager.java index 00deb1c0353..3ee6a1f0e8a 100644 --- a/core/src/main/java/org/springframework/security/authorization/ObservationAuthorizationManager.java +++ b/core/src/main/java/org/springframework/security/authorization/ObservationAuthorizationManager.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2023 the original author or authors. + * Copyright 2002-2024 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. @@ -71,7 +71,7 @@ public AuthorizationDecision check(Supplier authentication, T ob Observation observation = Observation.createNotStarted(this.convention, () -> context, this.registry).start(); try (Observation.Scope scope = observation.openScope()) { AuthorizationDecision decision = this.delegate.check(wrapped, object); - context.setDecision(decision); + context.setAuthorizationResult(decision); if (decision != null && !decision.isGranted()) { observation.error(new AccessDeniedException( this.messages.getMessage("AbstractAccessDecisionManager.accessDenied", "Access Denied"))); diff --git a/core/src/main/java/org/springframework/security/authorization/ObservationReactiveAuthorizationManager.java b/core/src/main/java/org/springframework/security/authorization/ObservationReactiveAuthorizationManager.java index d6e7a2b2c63..75e395f227e 100644 --- a/core/src/main/java/org/springframework/security/authorization/ObservationReactiveAuthorizationManager.java +++ b/core/src/main/java/org/springframework/security/authorization/ObservationReactiveAuthorizationManager.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2023 the original author or authors. + * Copyright 2002-2024 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. @@ -68,7 +68,7 @@ public Mono check(Mono authentication, T .parentObservation(contextView.getOrDefault(ObservationThreadLocalAccessor.KEY, null)) .start(); return this.delegate.check(wrapped, object).doOnSuccess((decision) -> { - context.setDecision(decision); + context.setAuthorizationResult(decision); if (decision == null || !decision.isGranted()) { observation.error(new AccessDeniedException("Access Denied")); } diff --git a/messaging/src/test/java/org/springframework/security/messaging/access/intercept/AuthorizationChannelInterceptorTests.java b/messaging/src/test/java/org/springframework/security/messaging/access/intercept/AuthorizationChannelInterceptorTests.java index fc354ed212c..debb21b7622 100644 --- a/messaging/src/test/java/org/springframework/security/messaging/access/intercept/AuthorizationChannelInterceptorTests.java +++ b/messaging/src/test/java/org/springframework/security/messaging/access/intercept/AuthorizationChannelInterceptorTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2022 the original author or authors. + * Copyright 2002-2024 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. @@ -30,6 +30,7 @@ import org.springframework.security.authorization.AuthorizationDecision; import org.springframework.security.authorization.AuthorizationEventPublisher; import org.springframework.security.authorization.AuthorizationManager; +import org.springframework.security.authorization.AuthorizationResult; import org.springframework.security.core.Authentication; import org.springframework.security.core.context.SecurityContextHolder; @@ -38,6 +39,7 @@ import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; import static org.mockito.ArgumentMatchers.any; import static org.mockito.BDDMockito.given; +import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.verify; /** @@ -82,12 +84,14 @@ public void constructorWhenAuthorizationManagerNullThenIllegalArgument() { @Test public void preSendWhenAllowThenSameMessage() { given(this.authorizationManager.check(any(), any())).willReturn(new AuthorizationDecision(true)); + given(this.authorizationManager.authorize(any(), any())).willCallRealMethod(); assertThat(this.interceptor.preSend(this.message, this.channel)).isSameAs(this.message); } @Test public void preSendWhenDenyThenException() { given(this.authorizationManager.check(any(), any())).willReturn(new AuthorizationDecision(false)); + given(this.authorizationManager.authorize(any(), any())).willCallRealMethod(); assertThatExceptionOfType(AccessDeniedException.class) .isThrownBy(() -> this.interceptor.preSend(this.message, this.channel)); } @@ -102,6 +106,10 @@ public void setEventPublisherWhenNullThenException() { public void preSendWhenAuthorizationEventPublisherThenPublishes() { this.interceptor.setAuthorizationEventPublisher(this.eventPublisher); given(this.authorizationManager.check(any(), any())).willReturn(new AuthorizationDecision(true)); + given(this.authorizationManager.authorize(any(), any())).willCallRealMethod(); + lenient().doCallRealMethod() + .when(this.eventPublisher) + .publishAuthorizationEvent(any(), any(), any(AuthorizationResult.class)); this.interceptor.preSend(this.message, this.channel); verify(this.eventPublisher).publishAuthorizationEvent(any(), any(), any()); }