Repository: aurora Updated Branches: refs/heads/master b2cc604a6 -> a2c7ccc17
Allowing dual authorizing params to account for thrift API deprecations. Also, added missing test coverage. Reviewed at https://reviews.apache.org/r/42614/ Project: http://git-wip-us.apache.org/repos/asf/aurora/repo Commit: http://git-wip-us.apache.org/repos/asf/aurora/commit/a2c7ccc1 Tree: http://git-wip-us.apache.org/repos/asf/aurora/tree/a2c7ccc1 Diff: http://git-wip-us.apache.org/repos/asf/aurora/diff/a2c7ccc1 Branch: refs/heads/master Commit: a2c7ccc17e8bc2e0a2aaef6c90db949d5885d60b Parents: b2cc604 Author: Maxim Khutornenko <[email protected]> Authored: Thu Jan 21 15:06:07 2016 -0800 Committer: Maxim Khutornenko <[email protected]> Committed: Thu Jan 21 15:06:07 2016 -0800 ---------------------------------------------------------------------- config/legacy_untested_classes.txt | 1 - .../http/api/security/AuthorizingParam.java | 11 ++- .../ShiroAuthorizingParamInterceptor.java | 94 +++++++++++--------- .../ShiroAuthorizingParamInterceptorTest.java | 56 ++++++++++++ 4 files changed, 115 insertions(+), 47 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/aurora/blob/a2c7ccc1/config/legacy_untested_classes.txt ---------------------------------------------------------------------- diff --git a/config/legacy_untested_classes.txt b/config/legacy_untested_classes.txt index 6b71fd2..144b258 100644 --- a/config/legacy_untested_classes.txt +++ b/config/legacy_untested_classes.txt @@ -54,7 +54,6 @@ org/apache/aurora/scheduler/http/Utilization$4 org/apache/aurora/scheduler/http/Utilization$5 org/apache/aurora/scheduler/http/Utilization$Display org/apache/aurora/scheduler/http/Utilization$DisplayMetric -org/apache/aurora/scheduler/http/api/security/FieldGetter$IdentityFieldGetter org/apache/aurora/scheduler/http/api/security/Kerberos5Realm org/apache/aurora/scheduler/log/mesos/MesosLogStreamModule org/apache/aurora/scheduler/log/mesos/MesosLogStreamModule$3 http://git-wip-us.apache.org/repos/asf/aurora/blob/a2c7ccc1/src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java b/src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java index 11d7e46..73c539b 100644 --- a/src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java +++ b/src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java @@ -26,9 +26,14 @@ import java.lang.annotation.Target; * parameter, otherwise a privilege escalation vulnerability exists. * * <p> - * A method intercepted by this interceptor that does not contain an AuthorizingParam or with - * multiple AuthorizingParams will result in an {@link java.lang.IllegalStateException} from the - * interceptor. + * A method intercepted by this interceptor that does not contain an AuthorizingParam will result + * in an {@link java.lang.IllegalStateException} from the interceptor. + * + * <p> + * It is possible to have more than one AuthorizingParam annotations applied to a method. This may + * be needed to ensure graceful deprecation cycle of the old annotated parameter for backwards + * compatibility reasons. In such cases, only one of the annotated arguments must be non-null or + * an {@link java.lang.IllegalStateException} is thrown. * * <p> * If the parameter type is not known to the interceptor an {@link java.lang.IllegalStateException} http://git-wip-us.apache.org/repos/asf/aurora/blob/a2c7ccc1/src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java b/src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java index 6905662..21e565e 100644 --- a/src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java +++ b/src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java @@ -14,7 +14,7 @@ package org.apache.aurora.scheduler.http.api.security; import java.lang.reflect.Method; -import java.util.List; +import java.lang.reflect.Parameter; import java.util.Map; import java.util.Set; import java.util.concurrent.atomic.AtomicLong; @@ -33,10 +33,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; -import com.google.common.collect.Lists; import com.google.common.collect.Maps; -import com.google.common.reflect.Invokable; -import com.google.common.reflect.Parameter; import org.aopalliance.intercept.MethodInterceptor; import org.aopalliance.intercept.MethodInvocation; @@ -62,7 +59,6 @@ import org.apache.aurora.scheduler.storage.entities.IJobKey; import org.apache.aurora.scheduler.thrift.Responses; import org.apache.shiro.authz.Permission; import org.apache.shiro.subject.Subject; -import org.apache.thrift.TBase; import static java.util.Objects.requireNonNull; @@ -106,6 +102,16 @@ class ShiroAuthorizingParamInterceptor implements MethodInterceptor { } }; + private static class JobKeyGetter { + private final int index; + private final Function<Object, Optional<JobKey>> func; + + JobKeyGetter(int index, Function<Object, Optional<JobKey>> func) { + this.index = index; + this.func = func; + } + } + private static final FieldGetter<JobUpdateRequest, TaskConfig> UPDATE_REQUEST_GETTER = new ThriftFieldGetter<>( JobUpdateRequest.class, @@ -195,26 +201,33 @@ class ShiroAuthorizingParamInterceptor implements MethodInterceptor { }; } - private static int annotatedParameterIndex(Method method) { + private static Iterable<JobKeyGetter> annotatedParameterGetters(Method method) { for (Method candidateMethod : getCandidateMethods(method)) { - List<Parameter> parameters = Invokable.from(candidateMethod).getParameters(); - List<Integer> parameterIndicies = Lists.newArrayList(); - for (int i = 0; i < parameters.size(); i++) { - if (parameters.get(i).isAnnotationPresent(AuthorizingParam.class)) { - parameterIndicies.add(i); + Parameter[] parameters = candidateMethod.getParameters(); + ImmutableList.Builder<JobKeyGetter> jobKeyGetters = ImmutableList.builder(); + for (int i = 0; i < parameters.length; i++) { + Parameter param = parameters[i]; + if (param.isAnnotationPresent(AuthorizingParam.class)) { + Class<?> parameterType = param.getType(); + @SuppressWarnings("unchecked") + Optional<Function<Object, Optional<JobKey>>> jobKeyGetter = + Optional.fromNullable( + (Function<Object, Optional<JobKey>>) FIELD_GETTERS_BY_TYPE.get(parameterType)); + if (!jobKeyGetter.isPresent()) { + throw new UnsupportedOperationException( + "No " + + JobKey.class.getName() + + " field getter was supplied for " + + parameterType.getName()); + } + + jobKeyGetters.add(new JobKeyGetter(i, jobKeyGetter.get())); } } - if (parameterIndicies.size() == 1) { - return Iterables.getOnlyElement(parameterIndicies); - } else if (parameterIndicies.size() > 1) { - throw new UnsupportedOperationException( - "Too many parameters annotated with " - + AuthorizingParam.class.getName() - + " found on method " - + method.getName() - + " of class " - + method.getDeclaringClass().getName()); + ImmutableList<JobKeyGetter> getters = jobKeyGetters.build(); + if (!Iterables.isEmpty(getters)) { + return getters; } } @@ -242,30 +255,25 @@ class ShiroAuthorizingParamInterceptor implements MethodInterceptor { + Response.class.getName()); } - final int index = annotatedParameterIndex(method); - ImmutableList<Parameter> parameters = Invokable.from(method).getParameters(); - Class<?> parameterType = parameters.get(index).getType().getRawType(); - if (!TBase.class.isAssignableFrom(parameterType)) { - throw new UnsupportedOperationException( - "Annotated parameter must be a thrift struct."); - } - @SuppressWarnings("unchecked") - final Optional<Function<Object, Optional<JobKey>>> jobKeyGetter = - Optional.fromNullable( - (Function<Object, Optional<JobKey>>) FIELD_GETTERS_BY_TYPE.get(parameterType)); - if (!jobKeyGetter.isPresent()) { - throw new UnsupportedOperationException( - "No " - + JobKey.class.getName() - + " field getter was supplied for " - + parameterType.getName()); - } + Iterable<JobKeyGetter> getters = annotatedParameterGetters(method); return arguments -> { - Optional<Object> argument = Optional.fromNullable(arguments[index]); - if (argument.isPresent()) { - return jobKeyGetter.get().apply(argument.get()); - } else { + Iterable<JobKeyGetter> nonNullArgGetters = + Iterables.filter(getters, getter -> arguments[getter.index] != null); + if (Iterables.isEmpty(nonNullArgGetters)) { return Optional.absent(); + } else { + if (Iterables.size(nonNullArgGetters) > 1) { + throw new IllegalStateException( + "Too many non-null arguments annotated with " + + AuthorizingParam.class.getName() + + " passed to " + + method.getName() + + " of " + + method.getDeclaringClass().getName()); + } + + JobKeyGetter getter = Iterables.getOnlyElement(nonNullArgGetters); + return getter.func.apply(arguments[getter.index]); } }; } http://git-wip-us.apache.org/repos/asf/aurora/blob/a2c7ccc1/src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptorTest.java ---------------------------------------------------------------------- diff --git a/src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptorTest.java b/src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptorTest.java index 16a3a3b..79e70fd 100644 --- a/src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptorTest.java +++ b/src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptorTest.java @@ -16,7 +16,10 @@ package org.apache.aurora.scheduler.http.api.security; import java.lang.reflect.Method; import java.util.concurrent.atomic.AtomicLong; +import com.google.common.base.Function; +import com.google.common.base.Optional; import com.google.common.collect.ImmutableSet; +import com.google.common.util.concurrent.UncheckedExecutionException; import com.google.inject.AbstractModule; import com.google.inject.Guice; import com.google.inject.matcher.Matchers; @@ -24,6 +27,8 @@ import com.google.inject.matcher.Matchers; import org.apache.aurora.common.stats.StatsProvider; import org.apache.aurora.common.testing.easymock.EasyMockTest; import org.apache.aurora.gen.JobConfiguration; +import org.apache.aurora.gen.JobKey; +import org.apache.aurora.gen.JobUpdateRequest; import org.apache.aurora.gen.Response; import org.apache.aurora.gen.ResponseCode; import org.apache.aurora.gen.TaskQuery; @@ -187,4 +192,55 @@ public class ShiroAuthorizingParamInterceptorTest extends EasyMockTest { ImmutableSet.of(JOB_KEY.newBuilder(), JOB_KEY.newBuilder().setName("other")))) .orNull()); } + + @Test + public void testHandlesMultipleAnnotations() { + control.replay(); + + Function<Object[], Optional<JobKey>> func = + interceptor.getAuthorizingParamGetters().getUnchecked(Params.class.getMethods()[0]); + + func.apply(new Object[]{new TaskQuery(), null, null}); + func.apply(new Object[]{null, new JobKey(), null}); + func.apply(new Object[]{null, null, new JobUpdateRequest()}); + } + + @Test(expected = IllegalStateException.class) + public void testThrowsOnMultipleNonNullArguments() { + control.replay(); + + Function<Object[], Optional<JobKey>> func = + interceptor.getAuthorizingParamGetters().getUnchecked(Params.class.getMethods()[0]); + + func.apply(new Object[]{new TaskQuery(), new JobKey(), null}); + } + + @Test(expected = UncheckedExecutionException.class) + public void testThrowsNoAuthParams() { + control.replay(); + + interceptor.getAuthorizingParamGetters().getUnchecked(NoParams.class.getMethods()[0]); + } + + @Test(expected = UncheckedExecutionException.class) + public void testThrowsNoResponseReturned() { + control.replay(); + + interceptor.getAuthorizingParamGetters().getUnchecked(NoResponse.class.getMethods()[0]); + } + + private interface NoResponse { + void test(@AuthorizingParam TaskQuery query); + } + + private interface NoParams { + Response test(TaskQuery query); + } + + private interface Params { + Response test( + @AuthorizingParam TaskQuery query, + @AuthorizingParam JobKey job, + @AuthorizingParam JobUpdateRequest request); + } }
