Skip to content

Add formatting check and fix formatting #1734

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add formatting check and fix formatting #1734

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
32 changes: 32 additions & 0 deletions .github/workflows/code-formatting.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
name: Code Formatting Check

on:
pull_request:
branches: [ main ]
paths:
- '**.java'
- 'pom.xml'
- 'codestyle/**'

jobs:
formatting-check:
runs-on: ubuntu-latest

steps:
- uses: actions/checkout@v4

- name: Set up JDK 11
uses: actions/setup-java@v4
with:
java-version: '11'
distribution: 'corretto'

- name: Cache Maven dependencies
uses: actions/cache@v3
with:
path: ~/.m2
key: ${{ runner.os }}-m2-${{ hashFiles('**/pom.xml') }}
restore-keys: ${{ runner.os }}-m2

- name: Check code formatting
run: mvn spotless:check
256 changes: 256 additions & 0 deletions codestyle/eclipse-formatter.xml
View file
Open in desktop

Large diffs are not rendered by default.

41 changes: 13 additions & 28 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -445,34 +445,6 @@
</execution>
</executions>
</plugin>
<plugin>
<artifactId>maven-checkstyle-plugin</artifactId>
<version>3.1.0</version>
<dependencies>
<dependency>
<groupId>com.puppycrawl.tools</groupId>
<artifactId>checkstyle</artifactId>
<version>8.29</version>
</dependency>
</dependencies>
<configuration>
<logViolationsToConsole>true</logViolationsToConsole>
<configLocation>codestyle/checkstyle.xml</configLocation>
<violationSeverity>warning</violationSeverity>
<maxAllowedViolations>0</maxAllowedViolations>
<excludes>**/awssdk/**, **/eventstream/**, **/vendored/**</excludes>
<skip>${skipTests}</skip>
</configuration>
<executions>
<execution>
<id>validate</id>
<phase>validate</phase>
<goals>
<goal>check</goal>
</goals>
</execution>
</executions>
</plugin>
<plugin>
<artifactId>maven-surefire-plugin</artifactId>
<version>3.0.0-M7</version>
Expand Down Expand Up @@ -846,6 +818,19 @@
</execution>
</executions>
</plugin>
<plugin>
<groupId>com.diffplug.spotless</groupId>
<artifactId>spotless-maven-plugin</artifactId>
<version>2.40.0</version>
<configuration>
<java>
<eclipse>
<file>codestyle/eclipse-formatter.xml</file>
</eclipse>
<removeUnusedImports />
</java>
</configuration>
</plugin>
</plugins>
</build>
<properties>
Expand Down
235 changes: 114 additions & 121 deletions src/main/java/com/aws/greengrass/authorization/AuthorizationHandler.java
View file
Open in desktop

Large diffs are not rendered by default.

18 changes: 11 additions & 7 deletions src/main/java/com/aws/greengrass/authorization/AuthorizationIPCAgent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ public ValidateAuthorizationTokenOperationHandler getValidateAuthorizationTokenO

@SuppressWarnings("PMD.PreserveStackTrace")
class ValidateAuthorizationTokenOperationHandler
extends GeneratedAbstractValidateAuthorizationTokenOperationHandler {
extends
GeneratedAbstractValidateAuthorizationTokenOperationHandler {
private final String serviceName;

protected ValidateAuthorizationTokenOperationHandler(OperationContinuationHandlerContext context) {
Expand All @@ -65,21 +66,24 @@ public void handleStreamEvent(EventStreamJsonMessage streamRequestEvent) {
@Override
public ValidateAuthorizationTokenResponse handleRequest(ValidateAuthorizationTokenRequest request) {
if (!AUTHORIZED_COMPONENTS.contains(serviceName)) {
logger.atDebug("service-unauthorized-error").log("{} is not authorized to perform {}",
serviceName, this.getOperationModelContext().getOperationName());
logger.atDebug("service-unauthorized-error")
.log("{} is not authorized to perform {}", serviceName,
this.getOperationModelContext().getOperationName());
throw new UnauthorizedError(String.format("%s is not authorized to perform %s", serviceName,
this.getOperationModelContext().getOperationName()));
}
ValidateAuthorizationTokenResponse response = new ValidateAuthorizationTokenResponse();
try {
authenticationHandler.doAuthentication(request.getToken());
response.setIsValid(true);
logger.atDebug("authorization-validated").log("Authorization validated for {} for {}",
serviceName, this.getOperationModelContext().getOperationName());
logger.atDebug("authorization-validated")
.log("Authorization validated for {} for {}", serviceName,
this.getOperationModelContext().getOperationName());
return response;
} catch (UnauthenticatedException e) {
logger.atDebug("invalid-token-error").log("Invalid token used when trying to authorize {} "
+ "to perform {}", serviceName, this.getOperationModelContext().getOperationName());
logger.atDebug("invalid-token-error")
.log("Invalid token used when trying to authorize {} " + "to perform {}", serviceName,
this.getOperationModelContext().getOperationName());
throw new InvalidTokenError(e.getMessage());
}
}
Expand Down
59 changes: 30 additions & 29 deletions src/main/java/com/aws/greengrass/authorization/AuthorizationModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,42 +23,43 @@
import static com.aws.greengrass.authorization.WildcardTrie.wildcardChar;

/**
* Simple permission table which stores permissions. A permission is a
* 4 value set of destination,principal,operation,resource.
* Simple permission table which stores permissions. A permission is a 4 value set of
* destination,principal,operation,resource.
*/
public class AuthorizationModule {
// Destination, Principal, Operation, Resource
Map<String, Map<String, Map<String, WildcardTrie>>> resourceAuthZCompleteMap =
new DefaultConcurrentHashMap<>(() -> new DefaultConcurrentHashMap<>(() ->
new DefaultConcurrentHashMap<>(WildcardTrie::new)));
Map<String, Map<String, Map<String, WildcardTrie>>> resourceAuthZCompleteMap = new DefaultConcurrentHashMap<>(
() -> new DefaultConcurrentHashMap<>(() -> new DefaultConcurrentHashMap<>(WildcardTrie::new)));
Map<String, Map<String, Map<String, Set<String>>>> rawResourceList = new DefaultConcurrentHashMap<>(
() -> new DefaultConcurrentHashMap<>(() -> new DefaultConcurrentHashMap<>(CopyOnWriteArraySet::new)));

/**
* Add permission for the given input set.
*
* @param destination destination entity
* @param permission set of principal, operation, resource.
* @throws AuthorizationException when arguments are invalid
*/
public void addPermission(String destination, Permission permission) throws AuthorizationException {
// resource is allowed to be null
if (Utils.isEmpty(permission.getPrincipal())
|| Utils.isEmpty(destination)
if (Utils.isEmpty(permission.getPrincipal()) || Utils.isEmpty(destination)
|| Utils.isEmpty(permission.getOperation())) {
throw new AuthorizationException("Invalid arguments");
}
String resource = permission.getResource();
validateResource(resource);
resourceAuthZCompleteMap.get(destination).get(permission.getPrincipal()).get(permission.getOperation()).add(
resource);
rawResourceList.get(destination).get(permission.getPrincipal()).get(permission.getOperation()).add(
resource);
resourceAuthZCompleteMap.get(destination)
.get(permission.getPrincipal())
.get(permission.getOperation())
.add(resource);
rawResourceList.get(destination).get(permission.getPrincipal()).get(permission.getOperation()).add(resource);
}

/**
* Only allow '?' if it's escaped. You can only escape special characters ('*', '$', '?').
* Any occurrence of '${' is only valid if it holds a single valid special character ('*', '$', '?') inside it
* and ends with '}'. (eg: "${*}" is valid, "${c}" is invalid, "${c" is invalid, ${*bc} is invalid)
* Only allow '?' if it's escaped. You can only escape special characters ('*', '$', '?'). Any occurrence of '${' is
* only valid if it holds a single valid special character ('*', '$', '?') inside it and ends with '}'. (eg: "${*}"
* is valid, "${c}" is invalid, "${c" is invalid, ${*bc} is invalid)
*
* @param resource resource to be validated
*/
private void validateResource(String resource) throws AuthorizationException {
Expand All @@ -75,12 +76,12 @@ private void validateResource(String resource) throws AuthorizationException {
if (currentChar == escapeChar && i + 1 < length && resource.charAt(i + 1) == '{') {
char actualChar = getActualChar(resource.substring(i));
if (actualChar == nullChar) {
throw new AuthorizationException("Resource contains an invalid escape sequence. "
+ "You can use ${*}, ${$}, or ${?}");
throw new AuthorizationException(
"Resource contains an invalid escape sequence. " + "You can use ${*}, ${$}, or ${?}");
}
if (!isSpecialChar(actualChar)) {
throw new AuthorizationException("Resource contains an invalid escape "
+ "sequence: ${" + actualChar + "}. You can use ${*}, ${$}, or ${?}");
throw new AuthorizationException("Resource contains an invalid escape " + "sequence: ${"
+ actualChar + "}. You can use ${*}, ${$}, or ${?}");
}
// skip next 3 characters as they are accounted for in escape sequence
i = i + 3;
Expand All @@ -96,9 +97,9 @@ boolean isSpecialChar(char actualChar) {
return actualChar == wildcardChar || actualChar == escapeChar || actualChar == singleCharWildcard;
}


/**
* Clear the permission list for a given destination. This is used when updating policies for a component.
*
* @param destination destination value
*/
public void deletePermissionsWithDestination(String destination) {
Expand All @@ -108,6 +109,7 @@ public void deletePermissionsWithDestination(String destination) {

/**
* Check if the combination of destination,principal,operation,resource exists in the table.
*
* @param destination destination value
* @param permission set of principal, operation and resource.
* @param resourceLookupPolicy whether to match MQTT wildcards or not.
Expand All @@ -117,8 +119,7 @@ public void deletePermissionsWithDestination(String destination) {
@SuppressWarnings("PMD.AvoidDeeplyNestedIfStmts")
public boolean isPresent(String destination, Permission permission, ResourceLookupPolicy resourceLookupPolicy)
throws AuthorizationException {
if (Utils.isEmpty(permission.getPrincipal())
|| Utils.isEmpty(destination)
if (Utils.isEmpty(permission.getPrincipal()) || Utils.isEmpty(destination)
|| Utils.isEmpty(permission.getOperation())) {
throw new AuthorizationException("Invalid arguments");
}
Expand All @@ -132,8 +133,8 @@ public boolean isPresent(String destination, Permission permission, ResourceLook
if (destMap.containsKey(permission.getPrincipal())) {
Map<String, WildcardTrie> principalMap = destMap.get(permission.getPrincipal());
if (principalMap.containsKey(permission.getOperation())) {
return principalMap.get(permission.getOperation()).matches(permission.getResource(),
resourceLookupPolicy);
return principalMap.get(permission.getOperation())
.matches(permission.getResource(), resourceLookupPolicy);
}
}
}
Expand All @@ -145,19 +146,19 @@ public boolean isPresent(String destination, Permission permission) throws Autho
}

/**
* Get resources for combination of destination, principal and operation.
* Also returns resources covered by permissions with * operation/principal.
* Get resources for combination of destination, principal and operation. Also returns resources covered by
* permissions with * operation/principal.
*
* @param destination destination
* @param principal principal (cannot be *)
* @param operation operation (cannot be *)
* @param principal principal (cannot be *)
* @param operation operation (cannot be *)
* @return list of allowed resources
* @throws AuthorizationException when arguments are invalid
*/
public Set<String> getResources(String destination, String principal, String operation)
throws AuthorizationException {
if (Utils.isEmpty(destination) || Utils.isEmpty(principal) || Utils.isEmpty(operation) || principal
.equals(ANY_REGEX) || operation.equals(ANY_REGEX)) {
if (Utils.isEmpty(destination) || Utils.isEmpty(principal) || Utils.isEmpty(operation)
|| principal.equals(ANY_REGEX) || operation.equals(ANY_REGEX)) {
throw new AuthorizationException("Invalid arguments");
}

Expand Down
9 changes: 6 additions & 3 deletions src/main/java/com/aws/greengrass/authorization/AuthorizationPolicy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,13 @@
@AllArgsConstructor
@NoArgsConstructor
public class AuthorizationPolicy implements Comparable<AuthorizationPolicy> {
@NonNull String policyId;
@NonNull
String policyId;
String policyDescription;
@NonNull Set<String> principals;
@NonNull Set<String> operations;
@NonNull
Set<String> principals;
@NonNull
Set<String> operations;
Set<String> resources;

@Override
Expand Down
9 changes: 6 additions & 3 deletions src/main/java/com/aws/greengrass/authorization/AuthorizationPolicyConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,10 @@
@AllArgsConstructor
@NoArgsConstructor
public class AuthorizationPolicyConfig {
@NonNull String policyDescription;
@NonNull Set<String> operations;
@NonNull Set<String> resources;
@NonNull
String policyDescription;
@NonNull
Set<String> operations;
@NonNull
Set<String> resources;
}
Loading
Loading