Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fail deployment if multiple security annotations are present #10568

Merged
merged 1 commit into from
Jul 9, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -222,15 +222,16 @@ private Map<MethodInfo, Function<BytecodeCreator, ResultHandle>> gatherSecurityA
Map<DotName, ClassInfo> additionalSecuredClasses, boolean denyUnannotated) {

Map<MethodInfo, AnnotationInstance> methodToInstanceCollector = new HashMap<>();
Map<ClassInfo, AnnotationInstance> classAnnotations = new HashMap<>();
Map<MethodInfo, Function<BytecodeCreator, ResultHandle>> result = new HashMap<>(gatherSecurityAnnotations(
index, DotNames.ROLES_ALLOWED, methodToInstanceCollector,
index, DotNames.ROLES_ALLOWED, methodToInstanceCollector, classAnnotations,
(instance -> rolesAllowedSecurityCheck(instance.value().asStringArray()))));
result.putAll(gatherSecurityAnnotations(index, DotNames.PERMIT_ALL, methodToInstanceCollector,
result.putAll(gatherSecurityAnnotations(index, DotNames.PERMIT_ALL, methodToInstanceCollector, classAnnotations,
(instance -> permitAllSecurityCheck())));
result.putAll(gatherSecurityAnnotations(index, DotNames.AUTHENTICATED, methodToInstanceCollector,
result.putAll(gatherSecurityAnnotations(index, DotNames.AUTHENTICATED, methodToInstanceCollector, classAnnotations,
(instance -> authenticatedSecurityCheck())));

result.putAll(gatherSecurityAnnotations(index, DotNames.DENY_ALL, methodToInstanceCollector,
result.putAll(gatherSecurityAnnotations(index, DotNames.DENY_ALL, methodToInstanceCollector, classAnnotations,
(instance -> denyAllSecurityCheck())));

/*
Expand Down Expand Up @@ -285,6 +286,7 @@ private boolean isPublicNonStaticNonConstructor(MethodInfo methodInfo) {
private Map<MethodInfo, Function<BytecodeCreator, ResultHandle>> gatherSecurityAnnotations(
IndexView index, DotName dotName,
Map<MethodInfo, AnnotationInstance> alreadyCheckedMethods,
Map<ClassInfo, AnnotationInstance> classLevelAnnotations,
Function<AnnotationInstance, Function<BytecodeCreator, ResultHandle>> securityCheckInstanceCreator) {

Map<MethodInfo, Function<BytecodeCreator, ResultHandle>> result = new HashMap<>();
Expand All @@ -308,16 +310,22 @@ private Map<MethodInfo, Function<BytecodeCreator, ResultHandle>> gatherSecurityA
AnnotationTarget target = instance.target();
if (target.kind() == AnnotationTarget.Kind.CLASS) {
List<MethodInfo> methods = target.asClass().methods();
for (MethodInfo methodInfo : methods) {
AnnotationInstance alreadyExistingInstance = alreadyCheckedMethods.get(methodInfo);
if ((alreadyExistingInstance == null)) {
result.put(methodInfo, securityCheckInstanceCreator.apply(instance));
} else if (alreadyExistingInstance.target().kind() == AnnotationTarget.Kind.CLASS) {
throw new IllegalStateException(
"Class " + methodInfo.declaringClass() + " is annotated with multiple security annotations");
AnnotationInstance existingClassInstance = classLevelAnnotations.get(target.asClass());
if (existingClassInstance == null) {
classLevelAnnotations.put(target.asClass(), instance);
for (MethodInfo methodInfo : methods) {
AnnotationInstance alreadyExistingInstance = alreadyCheckedMethods.get(methodInfo);
if ((alreadyExistingInstance == null)) {
result.put(methodInfo, securityCheckInstanceCreator.apply(instance));
}
}
} else {
throw new IllegalStateException(
"Class " + target.asClass() + " is annotated with multiple security annotations " + instance.name()
+ " and " + existingClassInstance.name());
}
}

}

return result;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package io.quarkus.security.test.cdi;

import javax.annotation.security.RolesAllowed;
import javax.enterprise.context.ApplicationScoped;

import io.quarkus.security.Authenticated;

@ApplicationScoped
@Authenticated
@RolesAllowed("admin")
public class InvalidBean {
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package io.quarkus.security.test.cdi;

import java.util.function.Consumer;
import java.util.function.Supplier;

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

import io.quarkus.test.QuarkusUnitTest;

public class InvalidClassBeanTestCase {
@RegisterExtension
static QuarkusUnitTest test = new QuarkusUnitTest()
.setArchiveProducer(new Supplier<JavaArchive>() {
@Override
public JavaArchive get() {
return ShrinkWrap.create(JavaArchive.class)
.addClass(InvalidBean.class);
}
}).assertException(new Consumer<Throwable>() {
@Override
public void accept(Throwable throwable) {
Assertions.assertEquals(IllegalStateException.class, throwable.getClass());
}
});

@Test
public void testRejected() {
Assertions.fail();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ public class QuarkusUnitTest
implements BeforeAllCallback, AfterAllCallback, BeforeEachCallback, AfterEachCallback,
InvocationInterceptor {

public static final String THE_BUILD_WAS_EXPECTED_TO_FAIL = "The build was expected to fail";

static {
System.setProperty("java.util.logging.manager", "org.jboss.logmanager.LogManager");
}
Expand Down Expand Up @@ -405,7 +407,7 @@ public void execute(BuildContext context) {
//we restore the CL at the end of the test
Thread.currentThread().setContextClassLoader(runningQuarkusApplication.getClassLoader());
if (assertException != null) {
fail("The build was expected to fail");
fail(THE_BUILD_WAS_EXPECTED_TO_FAIL);
}
started = true;
System.setProperty("test.url", TestHTTPResourceManager.getUri(runningQuarkusApplication));
Expand All @@ -424,6 +426,10 @@ public void execute(BuildContext context) {
} catch (Throwable e) {
started = false;
if (assertException != null) {
if (e instanceof AssertionError && e.getMessage().equals(THE_BUILD_WAS_EXPECTED_TO_FAIL)) {
//don't pass the 'no failure' assertion into the assert exception function
throw e;
}
if (e instanceof RuntimeException) {
Throwable cause = e.getCause();
if (cause != null && cause instanceof BuildException) {
Expand Down