Improve Method Security logging

Closes gh-10247

core/src/main/java/org/springframework/security/authorization/AuthorityAuthorizationDecision.java

@@ -18,22 +18,24 @@ package org.springframework.security.authorization;
 
 import java.util.Collection;
 
+import org.springframework.security.core.GrantedAuthority;
+
 /**
  * Represents an {@link AuthorizationDecision} based on a collection of authorities
  *
  * @author Marcus Da Coregio
  * @since 5.6
  */
-class AuthorityAuthorizationDecision extends AuthorizationDecision {
+public class AuthorityAuthorizationDecision extends AuthorizationDecision {
 
-	private final Collection<String> authorities;
+	private final Collection<GrantedAuthority> authorities;
 
-	AuthorityAuthorizationDecision(boolean granted, Collection<String> authorities) {
+	public AuthorityAuthorizationDecision(boolean granted, Collection<GrantedAuthority> authorities) {
 		super(granted);
 		this.authorities = authorities;
 	}
 
-	Collection<String> getAuthorities() {
+	public Collection<GrantedAuthority> getAuthorities() {
 		return this.authorities;
 	}
 

core/src/main/java/org/springframework/security/authorization/AuthorityAuthorizationManager.java

@@ -16,13 +16,13 @@
 
 package org.springframework.security.authorization;
 
-import java.util.Arrays;
 import java.util.HashSet;
 import java.util.Set;
 import java.util.function.Supplier;
 
 import org.springframework.security.core.Authentication;
 import org.springframework.security.core.GrantedAuthority;
+import org.springframework.security.core.authority.AuthorityUtils;
 import org.springframework.util.Assert;
 
 /**
@@ -37,10 +37,10 @@ public final class AuthorityAuthorizationManager<T> implements AuthorizationMana
 
 	private static final String ROLE_PREFIX = "ROLE_";
 
-	private final Set<String> authorities;
+	private final Set<GrantedAuthority> authorities;
 
 	private AuthorityAuthorizationManager(String... authorities) {
-		this.authorities = new HashSet<>(Arrays.asList(authorities));
+		this.authorities = new HashSet<>(AuthorityUtils.createAuthorityList(authorities));
 	}
 
 	/**
@@ -133,8 +133,7 @@ public final class AuthorityAuthorizationManager<T> implements AuthorizationMana
 
 	private boolean isAuthorized(Authentication authentication) {
 		for (GrantedAuthority grantedAuthority : authentication.getAuthorities()) {
-			String authority = grantedAuthority.getAuthority();
+			if (this.authorities.contains(grantedAuthority)) {
-			if (this.authorities.contains(authority)) {
 				return true;
 			}
 		}

core/src/main/java/org/springframework/security/authorization/AuthorityReactiveAuthorizationManager.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2018 the original author or authors.
+ * Copyright 2002-2021 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.
@@ -16,13 +16,13 @@
 
 package org.springframework.security.authorization;
 
-import java.util.Arrays;
 import java.util.List;
 
 import reactor.core.publisher.Mono;
 
 import org.springframework.security.core.Authentication;
 import org.springframework.security.core.GrantedAuthority;
+import org.springframework.security.core.authority.AuthorityUtils;
 import org.springframework.util.Assert;
 
 /**
@@ -35,10 +35,10 @@ import org.springframework.util.Assert;
  */
 public class AuthorityReactiveAuthorizationManager<T> implements ReactiveAuthorizationManager<T> {
 
-	private final List<String> authorities;
+	private final List<GrantedAuthority> authorities;
 
 	AuthorityReactiveAuthorizationManager(String... authorities) {
-		this.authorities = Arrays.asList(authorities);
+		this.authorities = AuthorityUtils.createAuthorityList(authorities);
 	}
 
 	@Override
@@ -46,7 +46,6 @@ public class AuthorityReactiveAuthorizationManager<T> implements ReactiveAuthori
 		// @formatter:off
 		return authentication.filter((a) -> a.isAuthenticated())
 				.flatMapIterable(Authentication::getAuthorities)
-				.map(GrantedAuthority::getAuthority)
 				.any(this.authorities::contains)
 				.map((granted) -> ((AuthorizationDecision) new AuthorityAuthorizationDecision(granted, this.authorities)))
 				.defaultIfEmpty(new AuthorityAuthorizationDecision(false, this.authorities));

core/src/main/java/org/springframework/security/authorization/method/AuthorizationManagerAfterMethodInterceptor.java

@@ -21,14 +21,18 @@ import java.util.function.Supplier;
 import org.aopalliance.aop.Advice;
 import org.aopalliance.intercept.MethodInterceptor;
 import org.aopalliance.intercept.MethodInvocation;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
 
 import org.springframework.aop.Pointcut;
 import org.springframework.aop.PointcutAdvisor;
 import org.springframework.aop.framework.AopInfrastructureBean;
 import org.springframework.core.Ordered;
+import org.springframework.core.log.LogMessage;
 import org.springframework.security.access.AccessDeniedException;
 import org.springframework.security.access.prepost.PostAuthorize;
 import org.springframework.security.authentication.AuthenticationCredentialsNotFoundException;
+import org.springframework.security.authorization.AuthorizationDecision;
 import org.springframework.security.authorization.AuthorizationManager;
 import org.springframework.security.core.Authentication;
 import org.springframework.security.core.context.SecurityContextHolder;
@@ -54,6 +58,8 @@ public final class AuthorizationManagerAfterMethodInterceptor
 		return authentication;
 	};
 
+	private final Log logger = LogFactory.getLog(this.getClass());
+
 	private final Pointcut pointcut;
 
 	private final AuthorizationManager<MethodInvocationResult> authorizationManager;
@@ -103,7 +109,7 @@ public final class AuthorizationManagerAfterMethodInterceptor
 	@Override
 	public Object invoke(MethodInvocation mi) throws Throwable {
 		Object result = mi.proceed();
-		this.authorizationManager.verify(AUTHENTICATION_SUPPLIER, new MethodInvocationResult(mi, result));
+		attemptAuthorization(mi, result);
 		return result;
 	}
 
@@ -134,4 +140,16 @@ public final class AuthorizationManagerAfterMethodInterceptor
 		return true;
 	}
 
+	private void attemptAuthorization(MethodInvocation mi, Object result) {
+		this.logger.debug(LogMessage.of(() -> "Authorizing method invocation " + mi));
+		AuthorizationDecision decision = this.authorizationManager.check(AUTHENTICATION_SUPPLIER,
+				new MethodInvocationResult(mi, result));
+		if (decision != null && !decision.isGranted()) {
+			this.logger.debug(LogMessage.of(() -> "Failed to authorize " + mi + " with authorization manager "
+					+ this.authorizationManager + " and decision " + decision));
+			throw new AccessDeniedException("Access Denied");
+		}
+		this.logger.debug(LogMessage.of(() -> "Authorized method invocation " + mi));
+	}
+
 }

core/src/main/java/org/springframework/security/authorization/method/AuthorizationManagerBeforeMethodInterceptor.java

@@ -25,15 +25,19 @@ import javax.annotation.security.RolesAllowed;
 import org.aopalliance.aop.Advice;
 import org.aopalliance.intercept.MethodInterceptor;
 import org.aopalliance.intercept.MethodInvocation;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
 
 import org.springframework.aop.Pointcut;
 import org.springframework.aop.PointcutAdvisor;
 import org.springframework.aop.framework.AopInfrastructureBean;
 import org.springframework.core.Ordered;
+import org.springframework.core.log.LogMessage;
 import org.springframework.security.access.AccessDeniedException;
 import org.springframework.security.access.annotation.Secured;
 import org.springframework.security.access.prepost.PreAuthorize;
 import org.springframework.security.authentication.AuthenticationCredentialsNotFoundException;
+import org.springframework.security.authorization.AuthorizationDecision;
 import org.springframework.security.authorization.AuthorizationManager;
 import org.springframework.security.core.Authentication;
 import org.springframework.security.core.context.SecurityContextHolder;
@@ -59,6 +63,8 @@ public final class AuthorizationManagerBeforeMethodInterceptor
 		return authentication;
 	};
 
+	private final Log logger = LogFactory.getLog(this.getClass());
+
 	private final Pointcut pointcut;
 
 	private final AuthorizationManager<MethodInvocation> authorizationManager;
@@ -149,7 +155,7 @@ public final class AuthorizationManagerBeforeMethodInterceptor
 	 */
 	@Override
 	public Object invoke(MethodInvocation mi) throws Throwable {
-		this.authorizationManager.verify(AUTHENTICATION_SUPPLIER, mi);
+		attemptAuthorization(mi);
 		return mi.proceed();
 	}
 
@@ -180,4 +186,15 @@ public final class AuthorizationManagerBeforeMethodInterceptor
 		return true;
 	}
 
+	private void attemptAuthorization(MethodInvocation mi) {
+		this.logger.debug(LogMessage.of(() -> "Authorizing method invocation " + mi));
+		AuthorizationDecision decision = this.authorizationManager.check(AUTHENTICATION_SUPPLIER, mi);
+		if (decision != null && !decision.isGranted()) {
+			this.logger.debug(LogMessage.of(() -> "Failed to authorize " + mi + " with authorization manager "
+					+ this.authorizationManager + " and decision " + decision));
+			throw new AccessDeniedException("Access Denied");
+		}
+		this.logger.debug(LogMessage.of(() -> "Authorized method invocation " + mi));
+	}
+
 }

core/src/main/java/org/springframework/security/authorization/method/ExpressionAttributeAuthorizationDecision.java

@@ -24,16 +24,16 @@ import org.springframework.security.authorization.AuthorizationDecision;
  * @author Marcus Da Coregio
  * @since 5.6
  */
-class ExpressionAttributeAuthorizationDecision extends AuthorizationDecision {
+public class ExpressionAttributeAuthorizationDecision extends AuthorizationDecision {
 
 	private final ExpressionAttribute expressionAttribute;
 
-	ExpressionAttributeAuthorizationDecision(boolean granted, ExpressionAttribute expressionAttribute) {
+	public ExpressionAttributeAuthorizationDecision(boolean granted, ExpressionAttribute expressionAttribute) {
 		super(granted);
 		this.expressionAttribute = expressionAttribute;
 	}
 
-	ExpressionAttribute getExpressionAttribute() {
+	public ExpressionAttribute getExpressionAttribute() {
 		return this.expressionAttribute;
 	}
 

core/src/test/java/org/springframework/security/authorization/method/AuthorizationManagerAfterMethodInterceptorTests.java

@@ -53,7 +53,7 @@ public class AuthorizationManagerAfterMethodInterceptorTests {
 	}
 
 	@Test
-	public void beforeWhenMockAuthorizationManagerThenVerifyAndReturnedObject() throws Throwable {
+	public void beforeWhenMockAuthorizationManagerThenCheckAndReturnedObject() throws Throwable {
 		MethodInvocation mockMethodInvocation = mock(MethodInvocation.class);
 		MethodInvocationResult result = new MethodInvocationResult(mockMethodInvocation, new Object());
 		given(mockMethodInvocation.proceed()).willReturn(result.getResult());
@@ -62,7 +62,7 @@ public class AuthorizationManagerAfterMethodInterceptorTests {
 				Pointcut.TRUE, mockAuthorizationManager);
 		Object returnedObject = advice.invoke(mockMethodInvocation);
 		assertThat(returnedObject).isEqualTo(result.getResult());
-		verify(mockAuthorizationManager).verify(eq(AuthorizationManagerAfterMethodInterceptor.AUTHENTICATION_SUPPLIER),
+		verify(mockAuthorizationManager).check(eq(AuthorizationManagerAfterMethodInterceptor.AUTHENTICATION_SUPPLIER),
 				any(MethodInvocationResult.class));
 	}
 

core/src/test/java/org/springframework/security/authorization/method/AuthorizationManagerBeforeMethodInterceptorTests.java

@@ -49,13 +49,13 @@ public class AuthorizationManagerBeforeMethodInterceptorTests {
 	}
 
 	@Test
-	public void beforeWhenMockAuthorizationManagerThenVerify() throws Throwable {
+	public void beforeWhenMockAuthorizationManagerThenCheck() throws Throwable {
 		MethodInvocation mockMethodInvocation = mock(MethodInvocation.class);
 		AuthorizationManager<MethodInvocation> mockAuthorizationManager = mock(AuthorizationManager.class);
 		AuthorizationManagerBeforeMethodInterceptor advice = new AuthorizationManagerBeforeMethodInterceptor(
 				Pointcut.TRUE, mockAuthorizationManager);
 		advice.invoke(mockMethodInvocation);
-		verify(mockAuthorizationManager).verify(AuthorizationManagerBeforeMethodInterceptor.AUTHENTICATION_SUPPLIER,
+		verify(mockAuthorizationManager).check(AuthorizationManagerBeforeMethodInterceptor.AUTHENTICATION_SUPPLIER,
 				mockMethodInvocation);
 	}