aurora-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ma...@apache.org
Subject aurora git commit: Allowing dual authorizing params to account for thrift API deprecations.
Date Thu, 21 Jan 2016 23:06:25 GMT
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 <maxim@apache.org>
Authored: Thu Jan 21 15:06:07 2016 -0800
Committer: Maxim Khutornenko <maxim@apache.org>
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);
+  }
 }


Mime
View raw message