diff --git a/sfge/src/main/java/com/salesforce/Main.java b/sfge/src/main/java/com/salesforce/Main.java index aabe40ea9..3b850f895 100644 --- a/sfge/src/main/java/com/salesforce/Main.java +++ b/sfge/src/main/java/com/salesforce/Main.java @@ -5,6 +5,7 @@ import com.salesforce.exception.SfgeException; import com.salesforce.exception.SfgeRuntimeException; import com.salesforce.exception.UnexpectedException; +import com.salesforce.exception.UserActionException; import com.salesforce.graph.ops.GraphUtil; import com.salesforce.messaging.CliMessager; import com.salesforce.metainfo.MetaInfoCollector; @@ -135,6 +136,10 @@ private int execute(String... args) { System.err.println( "Unexpected exception while loading graph. See logs for more information."); return INTERNAL_ERROR; + } catch (UserActionException ex) { + LOGGER.error("User action expected: ", ex); + System.err.println(formatError(ex)); + return INTERNAL_ERROR; } // Run all of the rules. diff --git a/sfge/src/main/java/com/salesforce/config/UserFacingMessages.java b/sfge/src/main/java/com/salesforce/config/UserFacingMessages.java index d004e3b7d..ce152253f 100644 --- a/sfge/src/main/java/com/salesforce/config/UserFacingMessages.java +++ b/sfge/src/main/java/com/salesforce/config/UserFacingMessages.java @@ -10,22 +10,23 @@ public final class UserFacingMessages { // format: filename,defined type, line number public static final String UNREACHABLE_CODE = - "Please remove unreachable code to proceed with analysis: %s,%s:%d"; + "Remove unreachable code to proceed with analysis: %s,%s:%d"; /** CRUD/FLS Violation messages * */ + // format: "CRUD" or "FLS", DML operation, Object type, Field information public static final String VIOLATION_MESSAGE_TEMPLATE = - "%1$s validation is missing for [%2$s] operation on [%3$s]"; + "%1$s validation is missing for [%2$s] operation on [%3$s]%4$s"; public static final String STRIP_INACCESSIBLE_READ_WARNING_TEMPLATE = "For stripInaccessible checks on READ operation, " - + "SFGE does not have the capability to verify that only sanitized data is used after the check." - + "Please ensure that unsanitized data is discarded for [%2$s]"; + + "SFGE doesn't have the capability to verify that only sanitized data is used after the check." + + "Please confirm that unsanitized data is discarded for [%2$s]"; public static final String UNRESOLVED_CRUD_FLS_TEMPLATE = - "SFGE was unable to resolve the parameter passed to [%2$s] operation. " - + "Ensure manually that this operation has the necessary CRUD/FLS checks."; + "SFGE couldn't resolve the parameter passed to [%2$s] operation%4$s. " + + "Please confirm that this operation has the necessary %1$s checks"; public static final String FIELDS_MESSAGE_TEMPLATE = " with field(s) [%s]"; public static final String FIELD_HANDLING_NOTICE = - " - SFGE may not have parsed some objects/fields correctly. Please confirm if the objects/fields involved in these segments have FLS checks: [%s]"; + " - SFGE may not have parsed some objects/fields correctly. Please confirm that the objects/fields involved in these segments have FLS checks: [%s]"; } diff --git a/sfge/src/main/java/com/salesforce/rules/fls/apex/operations/FlsValidationCentral.java b/sfge/src/main/java/com/salesforce/rules/fls/apex/operations/FlsValidationCentral.java index 02dc93483..af1b01ca7 100644 --- a/sfge/src/main/java/com/salesforce/rules/fls/apex/operations/FlsValidationCentral.java +++ b/sfge/src/main/java/com/salesforce/rules/fls/apex/operations/FlsValidationCentral.java @@ -210,9 +210,12 @@ public void createExpectedValidations( if (!apexValueOptional.isPresent()) { // TODO: add telemetry on missing parameter type that we need to handle in future if (LOGGER.isWarnEnabled()) { - LOGGER.warn("Database operation method has a parameter of unexpected type: " + parameter); + LOGGER.warn( + "Database operation method has a parameter of unexpected type: " + + parameter); } - // Add a violation to let users know that SFGE cannot resolve the parameter in the DML operation + // Add a violation to let users know that SFGE cannot resolve the parameter in the DML + // operation // and the onus of verifying its check is on them. violations.add( FlsViolationCreatorUtil.createUnresolvedCrudFlsViolation( diff --git a/sfge/src/main/java/com/salesforce/rules/fls/apex/operations/FlsViolationCreatorUtil.java b/sfge/src/main/java/com/salesforce/rules/fls/apex/operations/FlsViolationCreatorUtil.java index 28023266a..7276b5d87 100644 --- a/sfge/src/main/java/com/salesforce/rules/fls/apex/operations/FlsViolationCreatorUtil.java +++ b/sfge/src/main/java/com/salesforce/rules/fls/apex/operations/FlsViolationCreatorUtil.java @@ -85,8 +85,7 @@ static FlsViolationInfo getFlsViolationInfo( } static FlsViolationInfo createUnresolvedCrudFlsViolation( - FlsConstants.FlsValidationType validationType, - MethodCallExpressionVertex sinkVertex) { + FlsConstants.FlsValidationType validationType, MethodCallExpressionVertex sinkVertex) { final FlsViolationInfo violationInfo = new UnresolvedCrudFlsViolationInfo(validationType); violationInfo.setSinkVertex(sinkVertex); diff --git a/sfge/src/main/java/com/salesforce/rules/fls/apex/operations/FlsViolationMessageUtil.java b/sfge/src/main/java/com/salesforce/rules/fls/apex/operations/FlsViolationMessageUtil.java index d6c8c3737..a73414184 100644 --- a/sfge/src/main/java/com/salesforce/rules/fls/apex/operations/FlsViolationMessageUtil.java +++ b/sfge/src/main/java/com/salesforce/rules/fls/apex/operations/FlsViolationMessageUtil.java @@ -81,15 +81,7 @@ static String constructMessageInternal(FlsViolationInfo flsViolationInfo) { final String fieldInformation = getFieldInformation(fieldNames, allFields, flsViolationInfo.getObjectName()); - final String validationInformation = - getValidationInformation(flsViolationInfo, violationType); - - // Return the full validation message - return validationInformation + fieldInformation; - } - private static String getValidationInformation( - FlsViolationInfo flsViolationInfo, ViolationType violationType) { // Use the template that corresponds to the instance of flsViolationInfo. // This is necessary since individual subtypes have different message templates. final String messageTemplate = flsViolationInfo.getMessageTemplate(); @@ -99,7 +91,10 @@ private static String getValidationInformation( messageTemplate, violationType, flsViolationInfo.getValidationType().name(), - flsViolationInfo.getObjectName()); + flsViolationInfo.getObjectName(), + fieldInformation); + + // Return the full validation message return validationInformation; } diff --git a/sfge/src/test/java/com/salesforce/rules/fls/apex/operations/FlsViolationUtilsTest.java b/sfge/src/test/java/com/salesforce/rules/fls/apex/operations/FlsViolationUtilsTest.java index c00f06b0b..63337b60f 100644 --- a/sfge/src/test/java/com/salesforce/rules/fls/apex/operations/FlsViolationUtilsTest.java +++ b/sfge/src/test/java/com/salesforce/rules/fls/apex/operations/FlsViolationUtilsTest.java @@ -4,6 +4,7 @@ import static org.hamcrest.Matchers.equalToIgnoringCase; import com.salesforce.collections.CollectionUtil; +import com.salesforce.config.UserFacingMessages; import java.util.TreeSet; import org.junit.jupiter.api.Test; @@ -76,7 +77,17 @@ public void testMessageWithRelationalField() { assertThat( message, equalToIgnoringCase( - "FLS validation is missing for [UPDATE] operation on [My_Obj__c] with field(s) [Name,Status__c] - SFGE may not have parsed some objects/fields correctly. Please confirm if the objects/fields involved in these segments have FLS checks: [Relational_Field__r.Another_field__c]")); + String.format( + UserFacingMessages.VIOLATION_MESSAGE_TEMPLATE, + "FLS", + "UPDATE", + "My_Obj__c", + String.format( + UserFacingMessages.FIELDS_MESSAGE_TEMPLATE, + "Name,Status__c")) + + String.format( + UserFacingMessages.FIELD_HANDLING_NOTICE, + "Relational_Field__r.Another_field__c"))); } @Test @@ -91,7 +102,17 @@ public void testMessageWithRelationalObject() { assertThat( message, equalToIgnoringCase( - "FLS validation is missing for [UPDATE] operation on [My_Relational_Obj__r] with field(s) [Name,Status__c] - SFGE may not have parsed some objects/fields correctly. Please confirm if the objects/fields involved in these segments have FLS checks: [My_Relational_Obj__r]")); + String.format( + UserFacingMessages.VIOLATION_MESSAGE_TEMPLATE, + "FLS", + "UPDATE", + "My_Relational_Obj__r", + String.format( + UserFacingMessages.FIELDS_MESSAGE_TEMPLATE, + "Name,Status__c")) + + String.format( + UserFacingMessages.FIELD_HANDLING_NOTICE, + "My_Relational_Obj__r"))); } @Test @@ -105,7 +126,16 @@ public void testMessageWithIllegibleField() { assertThat( message, equalToIgnoringCase( - "FLS validation is missing for [UPDATE] operation on [My_Obj__c] with field(s) [Name,Status__c] - SFGE may not have parsed some objects/fields correctly. Please confirm if the objects/fields involved in these segments have FLS checks: [{1}{2}{3}]")); + String.format( + UserFacingMessages.VIOLATION_MESSAGE_TEMPLATE, + "FLS", + "UPDATE", + "My_Obj__c", + String.format( + UserFacingMessages.FIELDS_MESSAGE_TEMPLATE, + "Name,Status__c")) + + String.format( + UserFacingMessages.FIELD_HANDLING_NOTICE, "{1}{2}{3}"))); } @Test @@ -119,7 +149,16 @@ public void testMessageWithIllegibleFieldWithAllFields() { assertThat( message, equalToIgnoringCase( - "FLS validation is missing for [UPDATE] operation on [My_Obj__c] with field(s) [ALL_FIELDS] - SFGE may not have parsed some objects/fields correctly. Please confirm if the objects/fields involved in these segments have FLS checks: [{1}{2}{3}]")); + String.format( + UserFacingMessages.VIOLATION_MESSAGE_TEMPLATE, + "FLS", + "UPDATE", + "My_Obj__c", + String.format( + UserFacingMessages.FIELDS_MESSAGE_TEMPLATE, + "ALL_FIELDS")) + + String.format( + UserFacingMessages.FIELD_HANDLING_NOTICE, "{1}{2}{3}"))); } @Test @@ -132,7 +171,14 @@ public void testMessageWithOnlyIllegibleFields() { assertThat( message, equalToIgnoringCase( - "FLS validation is missing for [UPDATE] operation on [My_Obj__c] - SFGE may not have parsed some objects/fields correctly. Please confirm if the objects/fields involved in these segments have FLS checks: [{1}{2}{3}]")); + String.format( + UserFacingMessages.VIOLATION_MESSAGE_TEMPLATE, + "FLS", + "UPDATE", + "My_Obj__c", + "") + + String.format( + UserFacingMessages.FIELD_HANDLING_NOTICE, "{1}{2}{3}"))); } @Test @@ -145,7 +191,16 @@ public void testMessageWithOnlyIllegibleFieldsWithAllFields() { assertThat( message, equalToIgnoringCase( - "FLS validation is missing for [UPDATE] operation on [My_Obj__c] with field(s) [ALL_FIELDS] - SFGE may not have parsed some objects/fields correctly. Please confirm if the objects/fields involved in these segments have FLS checks: [{1}{2}{3}]")); + String.format( + UserFacingMessages.VIOLATION_MESSAGE_TEMPLATE, + "FLS", + "UPDATE", + "My_Obj__c", + String.format( + UserFacingMessages.FIELDS_MESSAGE_TEMPLATE, + "ALL_FIELDS")) + + String.format( + UserFacingMessages.FIELD_HANDLING_NOTICE, "{1}{2}{3}"))); } @Test @@ -158,7 +213,15 @@ public void testMessageWithIllegibleObject() { assertThat( message, equalToIgnoringCase( - "FLS validation is missing for [UPDATE] operation on [{1}] with field(s) [Name,Status__c] - SFGE may not have parsed some objects/fields correctly. Please confirm if the objects/fields involved in these segments have FLS checks: [{1}]")); + String.format( + UserFacingMessages.VIOLATION_MESSAGE_TEMPLATE, + "FLS", + "UPDATE", + "{1}", + String.format( + UserFacingMessages.FIELDS_MESSAGE_TEMPLATE, + "Name,Status__c")) + + String.format(UserFacingMessages.FIELD_HANDLING_NOTICE, "{1}"))); } @Test diff --git a/sfge/src/test/java/com/salesforce/testutils/ViolationWrapper.java b/sfge/src/test/java/com/salesforce/testutils/ViolationWrapper.java index 688bd0029..2984a6110 100644 --- a/sfge/src/test/java/com/salesforce/testutils/ViolationWrapper.java +++ b/sfge/src/test/java/com/salesforce/testutils/ViolationWrapper.java @@ -29,7 +29,8 @@ public enum FlsViolationType { builder.objectName, builder.fieldNames, builder.allFields)), - UNRESOLVED_CRUD_FLS((builder) -> new UnresolvedCrudFlsViolationInfo(builder.validationType)); + UNRESOLVED_CRUD_FLS( + (builder) -> new UnresolvedCrudFlsViolationInfo(builder.validationType)); Function instanceSupplier;