From 59acda04cf659798141b0d109dea87812167a304 Mon Sep 17 00:00:00 2001 From: Maksim Vinogradov Date: Sun, 21 Apr 2019 00:23:15 +0300 Subject: [PATCH] Fix NPE ExpressionBasedPreInvocationAdviceTests Getting NPE if @PreFilter argument filterType is not provided and method accept more then one argument. Add related exception message. fixes gh-6803 --- .../ExpressionBasedPreInvocationAdvice.java | 5 +- ...pressionBasedPreInvocationAdviceTests.java | 162 ++++++++++++++++++ .../method/MockMethodInvocation.java | 15 +- 3 files changed, 177 insertions(+), 5 deletions(-) create mode 100644 core/src/test/java/org/springframework/security/access/expression/method/ExpressionBasedPreInvocationAdviceTests.java diff --git a/core/src/main/java/org/springframework/security/access/expression/method/ExpressionBasedPreInvocationAdvice.java b/core/src/main/java/org/springframework/security/access/expression/method/ExpressionBasedPreInvocationAdvice.java index 1e933328c4..c4f11aa6f8 100644 --- a/core/src/main/java/org/springframework/security/access/expression/method/ExpressionBasedPreInvocationAdvice.java +++ b/core/src/main/java/org/springframework/security/access/expression/method/ExpressionBasedPreInvocationAdvice.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2016 the original author or authors. + * Copyright 2002-2019 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. @@ -81,6 +81,9 @@ public class ExpressionBasedPreInvocationAdvice implements "A PreFilter expression was set but the method argument type" + arg.getClass() + " is not filterable"); } + } else if (mi.getArguments().length > 1) { + throw new IllegalArgumentException( + "Unable to determine the method argument for filtering. Specify the filter target."); } if (filterTarget.getClass().isArray()) { diff --git a/core/src/test/java/org/springframework/security/access/expression/method/ExpressionBasedPreInvocationAdviceTests.java b/core/src/test/java/org/springframework/security/access/expression/method/ExpressionBasedPreInvocationAdviceTests.java new file mode 100644 index 0000000000..08591c6f62 --- /dev/null +++ b/core/src/test/java/org/springframework/security/access/expression/method/ExpressionBasedPreInvocationAdviceTests.java @@ -0,0 +1,162 @@ +/* + * Copyright 2002-2019 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.access.expression.method; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnitRunner; +import org.springframework.security.access.intercept.method.MockMethodInvocation; +import org.springframework.security.access.prepost.PreInvocationAttribute; +import org.springframework.security.core.Authentication; + +import java.util.ArrayList; +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Tests {@link ExpressionBasedPreInvocationAdvice} + * + * @author Maksim Vinogradov + * @since 5.2 + */ +@RunWith(MockitoJUnitRunner.class) +public class ExpressionBasedPreInvocationAdviceTests { + + @Mock + private Authentication authentication; + + private ExpressionBasedPreInvocationAdvice expressionBasedPreInvocationAdvice; + + @Before + public void setUp() throws Exception { + expressionBasedPreInvocationAdvice = new ExpressionBasedPreInvocationAdvice(); + } + + @Test(expected = IllegalArgumentException.class) + public void findFilterTargetNameProvidedButNotMatch() throws Exception { + //given + PreInvocationAttribute attribute = new PreInvocationExpressionAttribute("true", + "filterTargetDoesNotMatch", + null); + MockMethodInvocation methodInvocation = new MockMethodInvocation(new TestClass(), TestClass.class, + "doSomethingCollection", + new Class[]{List.class}, + new Object[]{new ArrayList<>()}); + //when - then + expressionBasedPreInvocationAdvice.before(authentication, methodInvocation, attribute); + } + + @Test(expected = IllegalArgumentException.class) + public void findFilterTargetNameProvidedArrayUnsupported() throws Exception { + //given + PreInvocationAttribute attribute = new PreInvocationExpressionAttribute("true", + "param", null); + MockMethodInvocation methodInvocation = new MockMethodInvocation(new TestClass(), TestClass.class, + "doSomethingArray", + new Class[]{String[].class}, + new Object[]{new String[0]}); + //when - then + expressionBasedPreInvocationAdvice.before(authentication, methodInvocation, attribute); + } + + @Test + public void findFilterTargetNameProvided() throws Exception { + //given + PreInvocationAttribute attribute = new PreInvocationExpressionAttribute("true", "param", null); + MockMethodInvocation methodInvocation = new MockMethodInvocation(new TestClass(), TestClass.class, + "doSomethingCollection", + new Class[]{List.class}, + new Object[]{new ArrayList<>()}); + + //when + boolean result = expressionBasedPreInvocationAdvice + .before(authentication, methodInvocation, attribute); + //then + assertThat(result).isTrue(); + } + + @Test(expected = IllegalArgumentException.class) + public void findFilterTargetNameNotProvidedArrayUnsupported() throws Exception { + //given + PreInvocationAttribute attribute = new PreInvocationExpressionAttribute("true", "", null); + MockMethodInvocation methodInvocation = new MockMethodInvocation(new TestClass(), TestClass.class, + "doSomethingArray", + new Class[]{String[].class}, + new Object[]{new String[0]}); + //when - then + expressionBasedPreInvocationAdvice.before(authentication, methodInvocation, attribute); + } + + @Test + public void findFilterTargetNameNotProvided() throws Exception { + //given + PreInvocationAttribute attribute = new PreInvocationExpressionAttribute("true", "", null); + MockMethodInvocation methodInvocation = new MockMethodInvocation(new TestClass(), TestClass.class, + "doSomethingCollection", + new Class[]{List.class}, + new Object[]{new ArrayList<>()}); + //when + boolean result = expressionBasedPreInvocationAdvice.before(authentication, methodInvocation, attribute); + //then + assertThat(result).isTrue(); + } + + @Test(expected = IllegalArgumentException.class) + public void findFilterTargetNameNotProvidedTypeNotSupported() throws Exception { + //given + PreInvocationAttribute attribute = new PreInvocationExpressionAttribute("true", "", null); + MockMethodInvocation methodInvocation = new MockMethodInvocation(new TestClass(), TestClass.class, + "doSomethingString", + new Class[]{String.class}, + new Object[]{"param"}); + //when - then + expressionBasedPreInvocationAdvice.before(authentication, methodInvocation, attribute); + } + + @Test(expected = IllegalArgumentException.class) + public void findFilterTargetNameNotProvidedMethodAcceptMoreThenOneArgument() throws Exception { + //given + PreInvocationAttribute attribute = new PreInvocationExpressionAttribute("true", "", null); + MockMethodInvocation methodInvocation = new MockMethodInvocation(new TestClass(), TestClass.class, + "doSomethingTwoArgs", + new Class[]{String.class, List.class}, + new Object[]{"param", new ArrayList<>()}); + //when - then + expressionBasedPreInvocationAdvice.before(authentication, methodInvocation, attribute); + } + + private class TestClass { + + public Boolean doSomethingCollection(List param) { + return Boolean.TRUE; + } + + public Boolean doSomethingArray(String[] param) { + return Boolean.TRUE; + } + + public Boolean doSomethingString(String param) { + return Boolean.TRUE; + } + + public Boolean doSomethingTwoArgs(String param, List list) { + return Boolean.TRUE; + } + } +} diff --git a/core/src/test/java/org/springframework/security/access/intercept/method/MockMethodInvocation.java b/core/src/test/java/org/springframework/security/access/intercept/method/MockMethodInvocation.java index c17c5854aa..3c4704fb3a 100644 --- a/core/src/test/java/org/springframework/security/access/intercept/method/MockMethodInvocation.java +++ b/core/src/test/java/org/springframework/security/access/intercept/method/MockMethodInvocation.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2016 the original author or authors. + * Copyright 2002-2019 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. @@ -15,15 +15,22 @@ */ package org.springframework.security.access.intercept.method; +import org.aopalliance.intercept.MethodInvocation; + import java.lang.reflect.AccessibleObject; import java.lang.reflect.Method; -import org.aopalliance.intercept.MethodInvocation; - @SuppressWarnings("unchecked") public class MockMethodInvocation implements MethodInvocation { private Method method; private Object targetObject; + private Object[] arguments = new Object[0]; + + public MockMethodInvocation(Object targetObject, Class clazz, String methodName, Class[] parameterTypes, + Object[] arguments) throws NoSuchMethodException { + this(targetObject, clazz, methodName, parameterTypes); + this.arguments = arguments; + } public MockMethodInvocation(Object targetObject, Class clazz, String methodName, Class... parameterTypes) throws NoSuchMethodException { @@ -32,7 +39,7 @@ public class MockMethodInvocation implements MethodInvocation { } public Object[] getArguments() { - return null; + return arguments; } public Method getMethod() {