Skip to content

Commit

Permalink
Ensure that Security annotation are always places on non-final methods
Browse files Browse the repository at this point in the history
We need security annotations to result in the creation of an interceptor
which is not possible when a method is final.
The solution we follow is to remove the final modifier from methods
that need intercepting.

Fixes: #5051
  • Loading branch information
geoand committed Nov 1, 2019
1 parent b23d288 commit bba05f9
Show file tree
Hide file tree
Showing 7 changed files with 147 additions and 22 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.function.Consumer;
import java.util.function.Predicate;
import java.util.stream.Collectors;

Expand All @@ -33,6 +34,7 @@
import io.quarkus.arc.processor.BeanInfo;
import io.quarkus.arc.processor.BeanProcessor;
import io.quarkus.arc.processor.BuiltinScope;
import io.quarkus.arc.processor.BytecodeTransformer;
import io.quarkus.arc.processor.ContextConfigurator;
import io.quarkus.arc.processor.ContextRegistrar;
import io.quarkus.arc.processor.ReflectionRegistration;
Expand All @@ -47,6 +49,7 @@
import io.quarkus.deployment.annotations.Record;
import io.quarkus.deployment.builditem.ApplicationArchivesBuildItem;
import io.quarkus.deployment.builditem.ApplicationClassPredicateBuildItem;
import io.quarkus.deployment.builditem.BytecodeTransformerBuildItem;
import io.quarkus.deployment.builditem.ExecutorBuildItem;
import io.quarkus.deployment.builditem.FeatureBuildItem;
import io.quarkus.deployment.builditem.GeneratedClassBuildItem;
Expand Down Expand Up @@ -239,7 +242,8 @@ public BeanRegistrationPhaseBuildItem registerBeans(ContextRegistrationPhaseBuil
// PHASE 3 - initialize and validate the bean deployment
@BuildStep
public ValidationPhaseBuildItem validate(BeanRegistrationPhaseBuildItem beanRegistrationPhase,
List<BeanConfiguratorBuildItem> beanConfigurators) {
List<BeanConfiguratorBuildItem> beanConfigurators,
BuildProducer<BytecodeTransformerBuildItem> bytecodeTransformer) {

for (BeanConfiguratorBuildItem beanConfigurator : beanConfigurators) {
for (BeanConfigurator<?> value : beanConfigurator.getValues()) {
Expand All @@ -248,7 +252,12 @@ public ValidationPhaseBuildItem validate(BeanRegistrationPhaseBuildItem beanRegi
}
}

beanRegistrationPhase.getBeanProcessor().initialize();
beanRegistrationPhase.getBeanProcessor().initialize(new Consumer<BytecodeTransformer>() {
@Override
public void accept(BytecodeTransformer t) {
bytecodeTransformer.produce(new BytecodeTransformerBuildItem(t.getClassToTransform(), t.getVisitorFunction()));
}
});
return new ValidationPhaseBuildItem(beanRegistrationPhase.getBeanProcessor().validate(),
beanRegistrationPhase.getBeanProcessor());
}
Expand Down Expand Up @@ -364,4 +373,4 @@ public boolean test(T t) {
return false;
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
package io.quarkus.security.test.cdi;

import static io.quarkus.security.test.cdi.SecurityTestUtils.assertFailureFor;
import static io.quarkus.security.test.cdi.SecurityTestUtils.assertSuccess;
import static io.quarkus.security.test.utils.IdentityMock.ADMIN;
import static io.quarkus.security.test.utils.IdentityMock.ANONYMOUS;
import static io.quarkus.security.test.utils.IdentityMock.USER;

import javax.annotation.security.DenyAll;
import javax.annotation.security.RolesAllowed;
import javax.inject.Inject;
import javax.inject.Singleton;

import org.jboss.shrinkwrap.api.ShrinkWrap;
import org.jboss.shrinkwrap.api.spec.JavaArchive;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;

import io.quarkus.security.ForbiddenException;
import io.quarkus.security.UnauthorizedException;
import io.quarkus.security.test.utils.AuthData;
import io.quarkus.security.test.utils.IdentityMock;
import io.quarkus.test.QuarkusUnitTest;

public class SecurityAnnotationOnFinalMethodTest {

@RegisterExtension
static final QuarkusUnitTest config = new QuarkusUnitTest()
.setArchiveProducer(() -> ShrinkWrap.create(JavaArchive.class)
.addClasses(BeanWithSecuredFinalMethod.class, IdentityMock.class,
AuthData.class, SecurityTestUtils.class));

@Inject
BeanWithSecuredFinalMethod bean;

@Test
public void shouldRestrictAccessToSpecificRole() {
assertFailureFor(() -> bean.securedMethod(), UnauthorizedException.class, ANONYMOUS);
assertSuccess(() -> bean.securedMethod(), "accessibleForAdminOnly", ADMIN);
}

@Test
public void shouldFailToAccessCompletely() {
assertFailureFor(() -> bean.otherSecuredMethod("whatever"), UnauthorizedException.class, ANONYMOUS);
assertFailureFor(() -> bean.otherSecuredMethod("whatever"), ForbiddenException.class, USER, ADMIN);
}

@Singleton
public static class BeanWithSecuredFinalMethod {

@RolesAllowed("admin")
public final String securedMethod() {
return "accessibleForAdminOnly";
}

@DenyAll
public final String otherSecuredMethod(String input) {
return "denied";
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.CopyOnWriteArrayList;
import java.util.concurrent.CopyOnWriteArraySet;
import java.util.function.Consumer;
import java.util.function.Function;
import java.util.function.Predicate;
import java.util.stream.Collectors;
Expand Down Expand Up @@ -203,19 +204,19 @@ BeanRegistrar.RegistrationContext registerBeans(List<BeanRegistrar> beanRegistra
return registerSyntheticBeans(beanRegistrars, buildContext);
}

void init() {
void init(Consumer<BytecodeTransformer> bytecodeTransformerConsumer) {
long start = System.currentTimeMillis();

// Collect dependency resolution errors
List<Throwable> errors = new ArrayList<>();
for (BeanInfo bean : beans) {
bean.init(errors);
bean.init(errors, bytecodeTransformerConsumer);
}
for (ObserverInfo observer : observers) {
observer.init(errors);
}
for (InterceptorInfo interceptor : interceptors) {
interceptor.init(errors);
interceptor.init(errors, bytecodeTransformerConsumer);
}
processErrors(errors);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -396,7 +396,7 @@ void validate(List<Throwable> errors, List<BeanDeploymentValidator> validators)
}
}

void init(List<Throwable> errors) {
void init(List<Throwable> errors, Consumer<BytecodeTransformer> bytecodeTransformerConsumer) {
for (Injection injection : injections) {
for (InjectionPointInfo injectionPoint : injection.injectionPoints) {
Beans.resolveInjectionPoint(beanDeployment, this, injectionPoint, errors);
Expand All @@ -405,7 +405,7 @@ void init(List<Throwable> errors) {
if (disposer != null) {
disposer.init(errors);
}
interceptedMethods.putAll(initInterceptedMethods());
interceptedMethods.putAll(initInterceptedMethods(bytecodeTransformerConsumer));
lifecycleInterceptors.putAll(initLifecycleInterceptors());
}

Expand All @@ -421,7 +421,8 @@ protected String getType() {
}
}

private Map<MethodInfo, InterceptionInfo> initInterceptedMethods() {
private Map<MethodInfo, InterceptionInfo> initInterceptedMethods(
Consumer<BytecodeTransformer> bytecodeTransformerConsumer) {
if (!isInterceptor() && isClassBean()) {
Map<MethodInfo, InterceptionInfo> interceptedMethods = new HashMap<>();
Map<MethodKey, Set<AnnotationInstance>> candidates = new HashMap<>();
Expand All @@ -434,7 +435,8 @@ private Map<MethodInfo, InterceptionInfo> initInterceptedMethods() {
}
}

Methods.addInterceptedMethodCandidates(beanDeployment, target.get().asClass(), candidates, classLevelBindings);
Methods.addInterceptedMethodCandidates(beanDeployment, target.get().asClass(), candidates, classLevelBindings,
bytecodeTransformerConsumer);

for (Entry<MethodKey, Set<AnnotationInstance>> entry : candidates.entrySet()) {
List<InterceptorInfo> interceptors = beanDeployment.getInterceptorResolver()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import java.util.Map;
import java.util.Objects;
import java.util.concurrent.ConcurrentHashMap;
import java.util.function.Consumer;
import java.util.function.Predicate;
import java.util.stream.Collectors;
import org.jboss.jandex.AnnotationInstance;
Expand All @@ -29,7 +30,7 @@
* <ol>
* <li>{@link #registerCustomContexts()}</li>
* <li>{@link #registerBeans()}</li>
* <li>{@link #initialize()}</li>
* <li>{@link #initialize(Consumer)}</li>
* <li>{@link #validate()}</li>
* <li>{@link #processValidationErrors(io.quarkus.arc.processor.BeanDeploymentValidator.ValidationContext)}</li>
* <li>{@link #generateResources(ReflectionRegistration)}</li>
Expand Down Expand Up @@ -102,8 +103,8 @@ public BeanRegistrar.RegistrationContext registerBeans() {
return beanDeployment.registerBeans(beanRegistrars);
}

public void initialize() {
beanDeployment.init();
public void initialize(Consumer<BytecodeTransformer> bytecodeTransformerConsumer) {
beanDeployment.init(bytecodeTransformerConsumer);
}

public BeanDeploymentValidator.ValidationContext validate() {
Expand Down Expand Up @@ -201,7 +202,12 @@ public BeanDeployment getBeanDeployment() {
public BeanDeployment process() throws IOException {
registerCustomContexts();
registerBeans();
initialize();
initialize(new Consumer<BytecodeTransformer>() {
@Override
public void accept(BytecodeTransformer transformer) {

}
});
ValidationContext validationContext = validate();
processValidationErrors(validationContext);
generateResources(null);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
package io.quarkus.arc.processor;

import java.util.function.BiFunction;
import org.objectweb.asm.ClassVisitor;

public class BytecodeTransformer {

final String classToTransform;
final BiFunction<String, ClassVisitor, ClassVisitor> visitorFunction;

public BytecodeTransformer(String classToTransform,
BiFunction<String, ClassVisitor, ClassVisitor> visitorFunction) {
this.classToTransform = classToTransform;
this.visitorFunction = visitorFunction;
}

public String getClassToTransform() {
return classToTransform;
}

public BiFunction<String, ClassVisitor, ClassVisitor> getVisitorFunction() {
return visitorFunction;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.function.BiFunction;
import java.util.function.Consumer;
import java.util.stream.Collectors;
import org.jboss.jandex.AnnotationInstance;
import org.jboss.jandex.ClassInfo;
Expand All @@ -18,6 +20,9 @@
import org.jboss.jandex.Type;
import org.jboss.jandex.TypeVariable;
import org.jboss.logging.Logger;
import org.objectweb.asm.ClassVisitor;
import org.objectweb.asm.MethodVisitor;
import org.objectweb.asm.Opcodes;

/**
*
Expand Down Expand Up @@ -115,7 +120,9 @@ static boolean isObjectToString(MethodInfo method) {

static void addInterceptedMethodCandidates(BeanDeployment beanDeployment, ClassInfo classInfo,
Map<MethodKey, Set<AnnotationInstance>> candidates,
List<AnnotationInstance> classLevelBindings) {
List<AnnotationInstance> classLevelBindings, Consumer<BytecodeTransformer> bytecodeTransformerConsumer) {

Set<String> methodsFromWhichToRemoveFinal = new HashSet<>();
for (MethodInfo method : classInfo.methods()) {
if (skipForSubclass(method)) {
continue;
Expand All @@ -136,20 +143,35 @@ static void addInterceptedMethodCandidates(BeanDeployment beanDeployment, ClassI
if (Modifier.isFinal(method.flags())) {
String className = method.declaringClass().name().toString();
if (!className.startsWith("java.")) {
LOGGER.warn(
String.format(
"Method %s.%s() is final, skipped during generation of the corresponding intercepted subclass",
className, method.name()));
methodsFromWhichToRemoveFinal.add(method.name());
}
} else {
candidates.computeIfAbsent(new Methods.MethodKey(method), key -> merged);
}
candidates.computeIfAbsent(new Methods.MethodKey(method), key -> merged);
}
}
if (!methodsFromWhichToRemoveFinal.isEmpty()) {
bytecodeTransformerConsumer.accept(
new BytecodeTransformer(classInfo.name().toString(), new BiFunction<String, ClassVisitor, ClassVisitor>() {
@Override
public ClassVisitor apply(String s, ClassVisitor classVisitor) {
return new ClassVisitor(Opcodes.ASM7, classVisitor) {
@Override
public MethodVisitor visitMethod(int access, String name, String descriptor, String signature,
String[] exceptions) {
if (methodsFromWhichToRemoveFinal.contains(name)) {
access = access & (~Opcodes.ACC_FINAL);
}
return super.visitMethod(access, name, descriptor, signature, exceptions);
}
};
}
}));
}
if (classInfo.superClassType() != null) {
ClassInfo superClassInfo = getClassByName(beanDeployment.getIndex(), classInfo.superName());
if (superClassInfo != null) {
addInterceptedMethodCandidates(beanDeployment, superClassInfo, candidates, classLevelBindings);
addInterceptedMethodCandidates(beanDeployment, superClassInfo, candidates, classLevelBindings,
bytecodeTransformerConsumer);
}
}
}
Expand Down

0 comments on commit bba05f9

Please sign in to comment.