perf: speed-up policy evaluation

src/main/java/com/aws/greengrass/clientdevices/auth/PermissionEvaluationUtils.java

@@ -14,28 +14,27 @@
 import com.aws.greengrass.util.Utils;
 import lombok.Builder;
 import lombok.Value;
+import org.apache.commons.lang3.StringUtils;
 
 import java.util.Map;
+import java.util.Objects;
 import java.util.Set;
-import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 import javax.inject.Inject;
 
 public final class PermissionEvaluationUtils {
     private static final Logger logger = LogManager.getLogger(PermissionEvaluationUtils.class);
-    private static final String ANY_REGEX = "*";
-    private static final String SERVICE_PATTERN_STRING = "([a-zA-Z]+)";
-    private static final String SERVICE_OPERATION_PATTERN_STRING = "([a-zA-Z0-9-_]+)";
-    private static final String SERVICE_RESOURCE_TYPE_PATTERN_STRING = "([a-zA-Z]+)";
-    // Characters, digits, special chars, space (Allowed in MQTT topics)
-    private static final String SERVICE_RESOURCE_NAME_PATTERN_STRING = "([\\w -\\/:-@\\[-\\`{-~]+)";
-    private static final String SERVICE_OPERATION_FORMAT = "%s:%s";
-    private static final String SERVICE_RESOURCE_FORMAT = "%s:%s:%s";
-    private static final Pattern SERVICE_OPERATION_PATTERN = Pattern.compile(
-            String.format(SERVICE_OPERATION_FORMAT, SERVICE_PATTERN_STRING, SERVICE_OPERATION_PATTERN_STRING));
-    private static final Pattern SERVICE_RESOURCE_PATTERN = Pattern.compile(
-            String.format(SERVICE_RESOURCE_FORMAT, SERVICE_PATTERN_STRING, SERVICE_RESOURCE_TYPE_PATTERN_STRING,
-                    SERVICE_RESOURCE_NAME_PATTERN_STRING), Pattern.UNICODE_CHARACTER_CLASS);
+    private static final String WILDCARD = "*";
+    private static final String DELIM = ":";
+    private static final Pattern RESOURCE_NAME_PATTERN =
+            Pattern.compile("([\\w -\\/:-@\\[-\\`{-~]+)", Pattern.UNICODE_CHARACTER_CLASS);
+    private static final String EXCEPTION_MALFORMED_OPERATION =
+            "Operation is malformed, must be of the form: "
+            + "([a-zA-Z]+):([a-zA-Z0-9-_]+)";
+    private static final String EXCEPTION_MALFORMED_RESOURCE =
+            "Resource is malformed, must be of the form: "
+            + "([a-zA-Z]+):([a-zA-Z]+):" + RESOURCE_NAME_PATTERN.pattern();
+
     private final GroupManager groupManager;
 
     /**
@@ -56,118 +55,170 @@ public PermissionEvaluationUtils(GroupManager groupManager) {
      *
      * @return boolean indicating if the operation requested is authorized
      */
+    @SuppressWarnings("PMD.AvoidBranchingStatementAsLastInLoop")
     public boolean isAuthorized(AuthorizationRequest request, Session session) {
-        Operation op = parseOperation(request.getOperation());
-        Resource rsc = parseResource(request.getResource());
+        Operation op;
+        Resource rsc;
+        try {
+            op = parseOperation(request.getOperation());
+            rsc = parseResource(request.getResource());
+        } catch (PolicyException e) {
+            logger.atError().setCause(e).log();
+            return false;
+        }
 
         if (!rsc.getService().equals(op.getService())) {
             throw new IllegalArgumentException(
                     String.format("Operation %s service is not same as resource %s service", op, rsc));
-
         }
 
-        Map<String, Set<Permission>> groupToPermissionsMap = groupManager.getApplicablePolicyPermissions(session);
+        Map<String, Set<Permission>> groupPermissions = groupManager.getApplicablePolicyPermissions(session);
 
-        if (groupToPermissionsMap == null || groupToPermissionsMap.isEmpty()) {
+        if (groupPermissions == null || groupPermissions.isEmpty()) {
             logger.atDebug().kv("operation", request.getOperation()).kv("resource", request.getResource())
                     .log("No authorization group matches, " + "deny the request");
             return false;
         }
 
-        for (Map.Entry<String, Set<Permission>> entry : groupToPermissionsMap.entrySet()) {
+        for (Map.Entry<String, Set<Permission>> entry : groupPermissions.entrySet()) {
             String principal = entry.getKey();
-            Set<Permission> permissions = entry.getValue();
-            if (Utils.isEmpty(permissions)) {
-                continue;
-            }
 
             // Find the first matching permission since we don't support 'deny' operation yet.
             //TODO add support of 'deny' operation
-            Permission permission = permissions.stream().filter(e -> {
-                if (!comparePrincipal(principal, e.getPrincipal())) {
-                    return false;
+            for (Permission permission : entry.getValue()) {
+                if (!comparePrincipal(principal, permission.getPrincipal())) {
+                    continue;
                 }
-                if (!compareOperation(op, e.getOperation())) {
-                    return false;
+                if (!compareOperation(op, permission.getOperation())) {
+                    continue;
                 }
+
+                String resource;
                 try {
-                    return compareResource(rsc, e.getResource(session));
+                    resource = permission.getResource(session);
                 } catch (PolicyException er) {
                     logger.atError().setCause(er).log();
-                    return false;
+                    continue;
+                }
+
+                if (!compareResource(rsc, resource)) {
+                    continue;
                 }
-            }).findFirst().orElse(null);
 
-            if (permission != null) {
                 logger.atDebug().log("Hit policy with permission {}", permission);
                 return true;
             }
         }
-
         return false;
     }
 
     private boolean comparePrincipal(String requestPrincipal, String policyPrincipal) {
-        if (requestPrincipal.equals(policyPrincipal)) {
+        if (Objects.equals(requestPrincipal, policyPrincipal)) {
             return true;
         }
-
-        return ANY_REGEX.equals(policyPrincipal);
+        return Objects.equals(WILDCARD, policyPrincipal);
     }
 
     private boolean compareOperation(Operation requestOperation, String policyOperation) {
-        if (requestOperation.getOperationStr().equals(policyOperation)) {
+        if (Objects.equals(requestOperation.getOperationStr(), policyOperation)) {
             return true;
         }
-        if (String.format(SERVICE_OPERATION_FORMAT, requestOperation.getService(), ANY_REGEX).equals(policyOperation)) {
+        if (Objects.equals(requestOperation.getService() + DELIM + WILDCARD, policyOperation)) {
             return true;
         }
-        return ANY_REGEX.equals(policyOperation);
+        return Objects.equals(WILDCARD, policyOperation);
     }
 
     private boolean compareResource(Resource requestResource, String policyResource) {
-        if (requestResource.getResourceStr().equals(policyResource)) {
+        if (Objects.equals(requestResource.getResourceStr(), policyResource)) {
             return true;
         }
-
-        if (String.format(SERVICE_RESOURCE_FORMAT, requestResource.getService(), requestResource.getResourceType(),
-                ANY_REGEX).equals(policyResource)) {
+        if (Objects.equals(
+                requestResource.getService() + DELIM + requestResource.getResourceType() + DELIM + WILDCARD,
+                policyResource)) {
             return true;
         }
-
-        return ANY_REGEX.equals(policyResource);
+        return Objects.equals(WILDCARD, policyResource);
     }
 
-    private Operation parseOperation(String operationStr) {
+    private Operation parseOperation(String operationStr) throws PolicyException {
         if (Utils.isEmpty(operationStr)) {
-            throw new IllegalArgumentException("Operation can't be empty");
+            throw new PolicyException("Operation can't be empty");
+        }
+
+        int split = operationStr.indexOf(':');
+        if (split == -1 || split == operationStr.length() - 1) {
+            throw new PolicyException(EXCEPTION_MALFORMED_OPERATION);
+        }
+
+        String service = operationStr.substring(0, split);
+        if (service.isEmpty() || !StringUtils.isAlpha(service)) {
+            throw new PolicyException(EXCEPTION_MALFORMED_OPERATION);
         }
 
-        Matcher matcher = SERVICE_OPERATION_PATTERN.matcher(operationStr);
-        if (matcher.matches()) {
-            return Operation.builder().operationStr(operationStr).service(matcher.group(1)).action(matcher.group(2))
-                    .build();
+        String action = operationStr.substring(split + 1);
+        if (action.isEmpty() || !isAlphanumericWithExtraChars(action, "-_")) {
+            throw new PolicyException(EXCEPTION_MALFORMED_OPERATION);
         }
-        throw new IllegalArgumentException(String.format("Operation %s is not in the form of %s", operationStr,
-                SERVICE_OPERATION_PATTERN.pattern()));
+
+        return Operation.builder()
+                .operationStr(operationStr)
+                .service(service)
+                .action(action)
+                .build();
     }
 
-    private Resource parseResource(String resourceStr) {
+    private Resource parseResource(String resourceStr) throws PolicyException  {
         if (Utils.isEmpty(resourceStr)) {
-            throw new IllegalArgumentException("Resource can't be empty");
+            throw new PolicyException("Resource can't be empty");
+        }
+
+        int split = resourceStr.indexOf(':');
+        if (split == -1 || split == resourceStr.length() - 1) {
+            throw new PolicyException(EXCEPTION_MALFORMED_RESOURCE);
+        }
+
+        String service = resourceStr.substring(0, split);
+        if (service.isEmpty() || !StringUtils.isAlpha(service)) {
+            throw new PolicyException(EXCEPTION_MALFORMED_RESOURCE);
+        }
+
+        String typeAndName = resourceStr.substring(split + 1);
+        split = typeAndName.indexOf(':');
+        if (split == -1 || split == resourceStr.length() - 1) {
+            throw new PolicyException(EXCEPTION_MALFORMED_RESOURCE);
         }
 
-        Matcher matcher = SERVICE_RESOURCE_PATTERN.matcher(resourceStr);
-        if (matcher.matches()) {
-            return Resource.builder().resourceStr(resourceStr)
-                    .service(matcher.group(1))
-                    .resourceType(matcher.group(2))
-                    .resourceName(matcher.group(3))
-                    .build();
+        String resourceType = typeAndName.substring(0, split);
+        if (resourceType.isEmpty() || !StringUtils.isAlpha(resourceType)) {
+            throw new PolicyException(EXCEPTION_MALFORMED_RESOURCE);
         }
 
-        throw new IllegalArgumentException(
-                String.format("Resource %s is not in the form of %s", resourceStr, SERVICE_RESOURCE_PATTERN.pattern()));
+        String resourceName = typeAndName.substring(split + 1); // TODO
+        // still using regex because Pattern.UNICODE_CHARACTER_CLASS is complicated
+        if (!RESOURCE_NAME_PATTERN.matcher(resourceName).matches()) {
+            throw new PolicyException(EXCEPTION_MALFORMED_RESOURCE);
+        }
+
+        return Resource.builder()
+                .resourceStr(resourceStr)
+                .service(service)
+                .resourceType(resourceType)
+                .resourceName(resourceName)
+                .build();
+    }
+
+    private static boolean isAlphanumericWithExtraChars(CharSequence cs, String extra) {
+        if (Utils.isEmpty(cs)) {
+            return false;
+        }
+        for (int i = 0; i < cs.length(); i++) {
+            char curr = cs.charAt(i);
+            if (!Character.isLetterOrDigit(curr) && extra.indexOf(curr) == -1) {
+                return false;
+            }
+        }
+        return true;
     }
 
     @Value

src/main/java/com/aws/greengrass/clientdevices/auth/configuration/GroupManager.java

@@ -8,11 +8,10 @@
 import com.aws.greengrass.clientdevices.auth.session.Session;
 
 import java.util.Collections;
-import java.util.HashSet;
+import java.util.HashMap;
 import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicReference;
-import java.util.stream.Collectors;
 
 /**
  * Singleton manager class for managing device group roles and retrieving permissions associated with Sessions. To
@@ -37,21 +36,15 @@ public Map<String, Set<Permission>> getApplicablePolicyPermissions(Session sessi
         if (config == null) {
             return Collections.emptyMap();
         }
-        Set<String> matchingGroups = findMatchingGroups(config.getDefinitions(), session);
-        return matchingGroups.stream()
-                .collect(Collectors.toMap(group -> group, group -> config.getGroupToPermissionsMap().get(group)));
-    }
-
-    private Set<String> findMatchingGroups(Map<String, GroupDefinition> groupDefinitionMap, Session session) {
-        Set<String> matchingGroups = new HashSet<>();
-
-        for (String groupName : groupDefinitionMap.keySet()) {
-            GroupDefinition group = groupDefinitionMap.get(groupName);
-            if (group.containsClientDevice(session)) {
-                matchingGroups.add(groupName);
+        Map<String, Set<Permission>> groupPermissions = new HashMap<>();
+        for (Map.Entry<String, GroupDefinition> entry : config.getDefinitions().entrySet()) {
+            if (entry.getValue().containsClientDevice(session)) {
+                Set<Permission> permissions = config.getGroupToPermissionsMap().get(entry.getKey());
+                if (permissions != null) {
+                    groupPermissions.put(entry.getKey(), permissions);
+                }
             }
         }
-
-        return matchingGroups;
+        return groupPermissions;
     }
 }

src/main/java/com/aws/greengrass/clientdevices/auth/exception/PolicyException.java

@@ -5,7 +5,7 @@
 
 package com.aws.greengrass.clientdevices.auth.exception;
 
-public class PolicyException extends Exception {
+public class PolicyException extends AuthorizationException {
     private static final long serialVersionUID = -1L;
 
     public PolicyException(String message) {

src/test/java/com/aws/greengrass/clientdevices/auth/PermissionEvaluationUtilsTest.java

@@ -18,6 +18,10 @@
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.extension.ExtensionContext;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
 import org.mockito.Mock;
 import org.mockito.junit.jupiter.MockitoExtension;
 
@@ -26,9 +30,12 @@
 import java.util.HashSet;
 import java.util.Map;
 import java.util.Set;
+import java.util.stream.Stream;
 
+import static com.aws.greengrass.testcommons.testutilities.ExceptionLogProtector.ignoreExceptionOfType;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.is;
+import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.when;
 
@@ -129,6 +136,57 @@ void GIVEN_single_permission_with_multiple_invalid_policy_variables_WHEN_get_per
         assertThat(policyVariablePermission.getResource(session).equals(expectedPermission.getResource()), is(true));
     }
 
+    public static Stream<Arguments> invalidAuthRequests() {
+        return Stream.of( // operation, resource
+
+                // bad resources
+                Arguments.of("mqtt:publish", ""),
+                Arguments.of("mqtt:publish", ":"),
+                Arguments.of("mqtt:publish", "::"),
+                Arguments.of("mqtt:publish", "mqtt:topic:"),
+                Arguments.of("mqtt:publish", "mqtt::myTopic"),
+                Arguments.of("mqtt:publish", ":topic:myTopic"),
+                Arguments.of("mqtt:publish", "mqtt::"),
+                Arguments.of("mqtt:publish", "mqtt:"),
+                Arguments.of("mqtt:publish", "mqtt: "),
+                Arguments.of("mqtt:publish", ":topic:"),
+                Arguments.of("mqtt:publish", "::myTopic"),
+                Arguments.of("mqtt:publish", "mqtt:topic"),
+                Arguments.of("mqtt:publish", "mqtt"),
+                Arguments.of("mqtt:publish", "mqtt:topic:myTopic:"),
+                Arguments.of("mqtt:publish", "mqtt::topic:myTopic"),
+                Arguments.of("mqtt:publish", "mqtt:topic::myTopic"),
+                Arguments.of("mqtt:publish", ":mqtt:topic:myTopic"),
+                Arguments.of("mqtt:publish", " :topic:myTopic"),
+                Arguments.of("mqtt:publish", "mqtt: :myTopic"),
+
+                // bad operations
+                Arguments.of("", "mqtt:topic:myTopic"),
+                Arguments.of(":", "mqtt:topic:myTopic"),
+                Arguments.of("mqtt", "mqtt:topic:myTopic"),
+                Arguments.of("mqtt:", "mqtt:topic:myTopic"),
+                Arguments.of("mqtt: ", "mqtt:topic:myTopic"),
+                Arguments.of(":publish", "mqtt:topic:myTopic"),
+                Arguments.of(" :publish", "mqtt:topic:myTopic"),
+                Arguments.of("mqtt:publish:", "mqtt:topic:myTopic"),
+                Arguments.of("mqtt::publish", "mqtt:topic:myTopic"),
+                Arguments.of(":mqtt:publish", "mqtt:topic:myTopic")
+        );
+    }
+
+    @MethodSource("invalidAuthRequests")
+    @ParameterizedTest
+    void GIVEN_invalid_auth_request_WHEN_authN_performed_THEN_exception_thrown(String operation, String resource, ExtensionContext context) {
+        ignoreExceptionOfType(context, PolicyException.class);
+        assertFalse(permissionEvaluationUtils.isAuthorized(
+                AuthorizationRequest.builder()
+                        .sessionId(SESSION_ID)
+                        .operation(operation)
+                        .resource(resource)
+                        .build(),
+                session));
+    }
+
     @Test
     void GIVEN_single_group_permission_with_variable_WHEN_evaluate_operation_permission_THEN_return_decision() {
         when(groupManager.getApplicablePolicyPermissions(any(Session.class)))