Skip to content

Commit

Permalink
chore: remove substring method, add tests
Browse files Browse the repository at this point in the history
  • Loading branch information
@jcosentino11
jcosentino11 committed Feb 6, 2024
1 parent c05d95a commit 1343ec4
Show file tree
Hide file tree
Showing 3 changed files with 68 additions and 39 deletions.
47 changes: 9 additions & 38 deletions src/main/java/com/aws/greengrass/clientdevices/auth/PermissionEvaluationUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
import com.aws.greengrass.logging.impl.LogManager;
import com.aws.greengrass.util.Utils;
import lombok.Builder;
import lombok.NonNull;
import lombok.Value;
import org.apache.commons.lang3.StringUtils;

Expand Down Expand Up @@ -148,16 +147,16 @@ private Operation parseOperation(String operationStr) throws PolicyException {
}

int split = operationStr.indexOf(':');
if (split == -1) {
if (split == -1 || split == operationStr.length() - 1) {
throw new PolicyException(EXCEPTION_MALFORMED_OPERATION);
}

String service = safeSubstring(operationStr, 0, split);
String service = operationStr.substring(0, split);
if (service.isEmpty() || !StringUtils.isAlpha(service)) {
throw new PolicyException(EXCEPTION_MALFORMED_OPERATION);
}

String action = safeSubstring(operationStr, split + 1);
String action = operationStr.substring(split + 1);
if (action.isEmpty() || !isAlphanumericWithExtraChars(action, "-_")) {
throw new PolicyException(EXCEPTION_MALFORMED_OPERATION);
}
Expand All @@ -175,32 +174,27 @@ private Resource parseResource(String resourceStr) throws PolicyException {
}

int split = resourceStr.indexOf(':');
if (split == -1) {
if (split == -1 || split == resourceStr.length() - 1) {
throw new PolicyException(EXCEPTION_MALFORMED_RESOURCE);
}

String service = safeSubstring(resourceStr, 0, split);
String service = resourceStr.substring(0, split);
if (service.isEmpty() || !StringUtils.isAlpha(service)) {
throw new PolicyException(EXCEPTION_MALFORMED_RESOURCE);
}

String typeAndName = safeSubstring(resourceStr, split + 1);
if (typeAndName.isEmpty()) {
throw new PolicyException(EXCEPTION_MALFORMED_RESOURCE);
}

String typeAndName = resourceStr.substring(split + 1);
split = typeAndName.indexOf(':');
if (split == -1) {
if (split == -1 || split == resourceStr.length() - 1) {
throw new PolicyException(EXCEPTION_MALFORMED_RESOURCE);
}

String resourceType = safeSubstring(typeAndName, 0, split);
String resourceType = typeAndName.substring(0, split);
if (resourceType.isEmpty() || !StringUtils.isAlpha(resourceType)) {
throw new PolicyException(EXCEPTION_MALFORMED_RESOURCE);
}

String resourceName = safeSubstring(typeAndName, split + 1);

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);
Expand All @@ -227,29 +221,6 @@ private static boolean isAlphanumericWithExtraChars(CharSequence cs, String extr
return true;
}

/**
* Like {@link String#substring(int, int)}, except rather than throwing, start/end
* indexes will be clamped to the string's length.
* <ul>
* <li>safeSubstring("a", 0, 0) -> ""</li>
* <li>safeSubstring("a", 0, 1) -> "a"</li>
* <li>safeSubstring("a", 0, 10) -> "a"</li>
* <li>safeSubstring("a", 1, 10) -> ""</li>
* </ul>
*
* @param str input string
* @param start substring start, inclusive. must be greater than zero.
* @param end substring end,
* @return substring
*/
private static String safeSubstring(@NonNull String str, int start, int end) {
return str.substring(Math.min(start, str.length()), Math.min(end, str.length()));
}

private static String safeSubstring(@NonNull String str, int start) {
return safeSubstring(str, start, str.length());
}

@Value
@Builder
private static class Operation {
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/aws/greengrass/clientdevices/auth/exception/PolicyException.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
58 changes: 58 additions & 0 deletions src/test/java/com/aws/greengrass/clientdevices/auth/PermissionEvaluationUtilsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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;

Expand Down Expand Up @@ -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)))
Expand Down

0 comments on commit 1343ec4

Please sign in to comment.