Development: Improve authorization test coverage (ls1intum#8456)

src/test/java/de/tum/in/www1/artemis/architecture/ArchitectureTest.java

@@ -29,7 +29,9 @@
 import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.noMethods;
 import static org.assertj.core.api.Assertions.assertThat;
 
+import java.lang.reflect.Method;
 import java.nio.file.Files;
+import java.util.Map;
 import java.util.Set;
 import java.util.stream.Collectors;
 
@@ -60,13 +62,16 @@
 import com.tngtech.archunit.core.domain.JavaClass;
 import com.tngtech.archunit.core.domain.JavaClasses;
 import com.tngtech.archunit.core.domain.JavaEnumConstant;
+import com.tngtech.archunit.core.domain.JavaMethod;
 import com.tngtech.archunit.core.domain.properties.HasAnnotations;
 import com.tngtech.archunit.lang.ArchCondition;
 import com.tngtech.archunit.lang.ArchRule;
 import com.tngtech.archunit.lang.ConditionEvents;
 import com.tngtech.archunit.lang.SimpleConditionEvent;
 import com.tngtech.archunit.library.GeneralCodingRules;
 
+import de.tum.in.www1.artemis.AbstractArtemisIntegrationTest;
+import de.tum.in.www1.artemis.authorization.AuthorizationTestService;
 import de.tum.in.www1.artemis.config.ApplicationConfiguration;
 import de.tum.in.www1.artemis.service.WebsocketMessagingService;
 import de.tum.in.www1.artemis.service.connectors.GitService;
@@ -298,4 +303,71 @@ void shouldNotUserAutowiredAnnotation() {
         JavaClasses classes = classesExcept(productionClasses, exceptions);
         rule.check(classes);
     }
+
+    @Test
+    void hasMatchingAuthorizationTestClassBeCorrectlyImplemented() throws NoSuchMethodException {
+        // Prepare the method that the authorization test should call to be identified as such
+        Method allCheckMethod = AuthorizationTestService.class.getMethod("testAllEndpoints", Map.class);
+        Method condCheckMethod = AuthorizationTestService.class.getMethod("testConditionalEndpoints", Map.class);
+        String identifyingPackage = "authorization";
+
+        ArchRule rule = classes().that(beDirectSubclassOf(AbstractArtemisIntegrationTest.class))
+                .should(haveMatchingTestClassCallingAMethod(identifyingPackage, Set.of(allCheckMethod, condCheckMethod))).because(
+                        "every test environment should have a corresponding authorization test covering the endpoints of this environment. Examples are \"AuthorizationJenkinsGitlabTest\" or \"AuthorizationGitlabCISamlTest\".");
+        rule.check(testClasses);
+    }
+
+    private DescribedPredicate<JavaClass> beDirectSubclassOf(Class<?> clazz) {
+        return new DescribedPredicate<>("be implemented in direct subclass of " + clazz.getSimpleName()) {
+
+            @Override
+            public boolean test(JavaClass javaClass) {
+                var superClasses = javaClass.getAllRawSuperclasses();
+                if (superClasses.isEmpty()) {
+                    // Tested class has no superclass
+                    return false;
+                }
+                return superClasses.getFirst().getFullName().equals(clazz.getName());
+            }
+        };
+    }
+
+    private ArchCondition<JavaClass> haveMatchingTestClassCallingAMethod(String identifyingPackage, Set<Method> signatureMethods) {
+        return new ArchCondition<>("have matching authorization test class") {
+
+            @Override
+            public void check(JavaClass item, ConditionEvents events) {
+                if (!hasMatchingTestClassCallingMethod(item, identifyingPackage, signatureMethods)) {
+                    events.add(violated(item, item.getFullName() + " does not have a matching test class in an \"" + identifyingPackage + "\" package "
+                            + "containing a test method that calls any given signature methods"));
+                }
+            }
+        };
+    }
+
+    private boolean hasMatchingTestClassCallingMethod(JavaClass javaClass, String identifyingPackage, Set<Method> signatureMethods) {
+        var subclasses = javaClass.getSubclasses();
+        // Check all subclasses of the given abstract test class to search for an authorization test class
+        for (JavaClass subclass : subclasses) {
+            // The test class es expected to reside inside an identifying package. We could match the full path, but this is more flexible.
+            if (!subclass.getPackageName().contains(identifyingPackage)) {
+                continue;
+            }
+            var methods = subclass.getMethods();
+            // Search for a test method that calls a signature method
+            for (JavaMethod method : methods) {
+                if (!method.isAnnotatedWith(Test.class) && !method.getRawReturnType().reflect().equals(Void.class)) {
+                    // Is not a test method
+                    continue;
+                }
+                if (method.getMethodCallsFromSelf().stream()
+                        .anyMatch(call -> signatureMethods.stream().anyMatch(checkMethod -> call.getTargetOwner().getFullName().equals(checkMethod.getDeclaringClass().getName())
+                                && call.getTarget().getName().equals(checkMethod.getName())))) {
+                    // Calls one of the signature methods
+                    return true;
+                }
+            }
+        }
+        return false;
+    }
 }

src/test/java/de/tum/in/www1/artemis/authorization/AuthorizationEndpointTest.java → src/test/java/de/tum/in/www1/artemis/authorization/AuthorizationGeneralAndIndependentEndpointTest.java

@@ -15,7 +15,7 @@
 /**
  * Contains the one automatic test covering all rest endpoints for authorization tests.
  */
-class AuthorizationEndpointTest extends AbstractSpringIntegrationIndependentTest {
+class AuthorizationGeneralAndIndependentEndpointTest extends AbstractSpringIntegrationIndependentTest {
 
     @Autowired
     private ApplicationContext applicationContext;
@@ -31,6 +31,6 @@ void testEndpoints() {
         endpointMap = endpointMap.entrySet().stream().filter(entry -> authorizationTestService.validEndpointToTest(entry.getValue(), false))
                 .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
 
-        authorizationTestService.testEndpoints(endpointMap);
+        authorizationTestService.testAllEndpoints(endpointMap);
     }
 }

src/test/java/de/tum/in/www1/artemis/authorization/AuthorizationGitlabCISamlTest.java → src/test/java/de/tum/in/www1/artemis/authorization/AuthorizationGitlabCISamlEndpointTest.java

@@ -2,21 +2,18 @@
 
 import java.lang.reflect.InvocationTargetException;
 import java.util.*;
-import java.util.stream.Collectors;
 
 import org.junit.jupiter.api.Test;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.context.ApplicationContext;
-import org.springframework.web.method.HandlerMethod;
-import org.springframework.web.servlet.mvc.method.RequestMappingInfo;
 import org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping;
 
 import de.tum.in.www1.artemis.AbstractSpringIntegrationGitlabCIGitlabSamlTest;
 
 /**
  * Contains the one automatic test covering all rest endpoints for authorization tests.
  */
-class AuthorizationGitlabCISamlTest extends AbstractSpringIntegrationGitlabCIGitlabSamlTest {
+class AuthorizationGitlabCISamlEndpointTest extends AbstractSpringIntegrationGitlabCIGitlabSamlTest {
 
     @Autowired
     private ApplicationContext applicationContext;
@@ -27,11 +24,6 @@ class AuthorizationGitlabCISamlTest extends AbstractSpringIntegrationGitlabCIGit
     @Test
     void testEndpoints() throws InvocationTargetException, IllegalAccessException {
         var requestMappingHandlerMapping = applicationContext.getBean("requestMappingHandlerMapping", RequestMappingHandlerMapping.class);
-        Map<RequestMappingInfo, HandlerMethod> endpointMap = requestMappingHandlerMapping.getHandlerMethods();
-        // Filter out endpoints that should not be tested.
-        endpointMap = endpointMap.entrySet().stream().filter(entry -> authorizationTestService.validEndpointToTest(entry.getValue(), true))
-                .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
-
-        authorizationTestService.testEndpoints(endpointMap);
+        authorizationTestService.testConditionalEndpoints(requestMappingHandlerMapping.getHandlerMethods());
     }
 }

src/test/java/de/tum/in/www1/artemis/authorization/AuthorizationJenkinsGitlabTest.java → src/test/java/de/tum/in/www1/artemis/authorization/AuthorizationJenkinsGitlabEndpointTest.java

@@ -1,21 +1,18 @@
 package de.tum.in.www1.artemis.authorization;
 
 import java.util.*;
-import java.util.stream.Collectors;
 
 import org.junit.jupiter.api.Test;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.context.ApplicationContext;
-import org.springframework.web.method.HandlerMethod;
-import org.springframework.web.servlet.mvc.method.RequestMappingInfo;
 import org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping;
 
 import de.tum.in.www1.artemis.AbstractSpringIntegrationJenkinsGitlabTest;
 
 /**
  * Contains the one automatic test covering all rest endpoints for authorization tests.
  */
-class AuthorizationJenkinsGitlabTest extends AbstractSpringIntegrationJenkinsGitlabTest {
+class AuthorizationJenkinsGitlabEndpointTest extends AbstractSpringIntegrationJenkinsGitlabTest {
 
     @Autowired
     private ApplicationContext applicationContext;
@@ -26,11 +23,6 @@ class AuthorizationJenkinsGitlabTest extends AbstractSpringIntegrationJenkinsGit
     @Test
     void testEndpoints() {
         var requestMappingHandlerMapping = applicationContext.getBean("requestMappingHandlerMapping", RequestMappingHandlerMapping.class);
-        Map<RequestMappingInfo, HandlerMethod> endpointMap = requestMappingHandlerMapping.getHandlerMethods();
-        // Filter out endpoints that should not be tested.
-        endpointMap = endpointMap.entrySet().stream().filter(entry -> authorizationTestService.validEndpointToTest(entry.getValue(), true))
-                .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
-
-        authorizationTestService.testEndpoints(endpointMap);
+        authorizationTestService.testConditionalEndpoints(requestMappingHandlerMapping.getHandlerMethods());
     }
 }

src/test/java/de/tum/in/www1/artemis/authorization/AuthorizationLocalCILocalVCEndpointTest.java

@@ -0,0 +1,26 @@
+package de.tum.in.www1.artemis.authorization;
+
+import org.junit.jupiter.api.Test;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.context.ApplicationContext;
+import org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping;
+
+import de.tum.in.www1.artemis.AbstractSpringIntegrationLocalCILocalVCTest;
+
+/**
+ * Contains the one automatic test covering all rest endpoints for authorization tests.
+ */
+class AuthorizationLocalCILocalVCEndpointTest extends AbstractSpringIntegrationLocalCILocalVCTest {
+
+    @Autowired
+    private ApplicationContext applicationContext;
+
+    @Autowired
+    private AuthorizationTestService authorizationTestService;
+
+    @Test
+    void testEndpoints() {
+        var requestMappingHandlerMapping = applicationContext.getBean("requestMappingHandlerMapping", RequestMappingHandlerMapping.class);
+        authorizationTestService.testConditionalEndpoints(requestMappingHandlerMapping.getHandlerMethods());
+    }
+}

src/test/java/de/tum/in/www1/artemis/authorization/AuthorizationTestService.java

@@ -1,10 +1,12 @@
 package de.tum.in.www1.artemis.authorization;
 
+import static de.tum.in.www1.artemis.config.Constants.PROFILE_CORE;
 import static org.assertj.core.api.Fail.fail;
 
 import java.lang.annotation.Annotation;
 import java.lang.reflect.Method;
 import java.util.*;
+import java.util.stream.Collectors;
 
 import org.springframework.context.annotation.Profile;
 import org.springframework.security.access.prepost.PreAuthorize;
@@ -34,7 +36,31 @@ public class AuthorizationTestService {
      *
      * @param endpointMap The map of all endpoints
      */
-    public void testEndpoints(Map<RequestMappingInfo, HandlerMethod> endpointMap) {
+    public void testAllEndpoints(Map<RequestMappingInfo, HandlerMethod> endpointMap) {
+        var endpointsToBeTested = endpointMap.entrySet().stream().filter(entry -> validEndpointToTest(entry.getValue(), false))
+                .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
+
+        testEndpoints(endpointsToBeTested);
+    }
+
+    /**
+     * Tests only endpoints that depend on a specific non-core profile and that most likely is only relevant for the currently running test environment.
+     *
+     * @param endpointMap The map of all endpoints
+     */
+    public void testConditionalEndpoints(Map<RequestMappingInfo, HandlerMethod> endpointMap) {
+        var endpointsToBeTested = endpointMap.entrySet().stream().filter(entry -> validEndpointToTest(entry.getValue(), true))
+                .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
+
+        testEndpoints(endpointsToBeTested);
+    }
+
+    /**
+     * Tests the given endpoints and prints the reports
+     *
+     * @param endpointMap The map of all endpoints to test
+     */
+    private void testEndpoints(Map<RequestMappingInfo, HandlerMethod> endpointMap) {
         Map<Class<?>, Set<String>> classReports = new HashMap<>();
         Map<Method, Set<String>> methodReports = new HashMap<>();
 
@@ -67,7 +93,14 @@ public boolean validEndpointToTest(HandlerMethod method, boolean onlyConditional
      * @return true if the endpoint depends on a profile, false otherwise
      */
     private boolean isConditionalEndpoint(HandlerMethod handlerMethod) {
-        return handlerMethod.getMethod().getAnnotation(Profile.class) != null || handlerMethod.getMethod().getDeclaringClass().getAnnotation(Profile.class) != null;
+        var methodProfileAnnotation = handlerMethod.getMethod().getAnnotation(Profile.class);
+        var classProfileAnnotation = handlerMethod.getMethod().getDeclaringClass().getAnnotation(Profile.class);
+        // No null-check required for classes because we have tests ensuring Profile annotations on classes
+        return (methodProfileAnnotation != null && isNonCoreProfile(methodProfileAnnotation)) || isNonCoreProfile(classProfileAnnotation);
+    }
+
+    private boolean isNonCoreProfile(Profile profileAnnotation) {
+        return !(profileAnnotation.value().length == 1 && profileAnnotation.value()[0].equals(PROFILE_CORE));
     }
 
     /**