Skip to content

@W-10778096@ Minor error message fixes #770

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

Merged
merged 2 commits into from
Jul 22, 2022
Merged

@W-10778096@ Minor error message fixes #770

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
37a0b99
@W-10778096@ Minor error message fixes
rmohan20 Jul 22, 2022
bcdbed4
Fixing test issues + reformatted unresolved violation template
rmohan20 Jul 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions sfge/src/main/java/com/salesforce/Main.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
Expand Down
15 changes: 8 additions & 7 deletions sfge/src/main/java/com/salesforce/config/UserFacingMessages.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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. "
Copy link
Contributor Author

@rmohan20 rmohan20 Jul 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jfeingold35 I've changed the format based on our conversation.

+ "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]";
}
7 changes: 5 additions & 2 deletions sfge/src/main/java/com/salesforce/rules/fls/apex/operations/FlsValidationCentral.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
3 changes: 1 addition & 2 deletions sfge/src/main/java/com/salesforce/rules/fls/apex/operations/FlsViolationCreatorUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
13 changes: 4 additions & 9 deletions sfge/src/main/java/com/salesforce/rules/fls/apex/operations/FlsViolationMessageUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -99,7 +91,10 @@ private static String getValidationInformation(
messageTemplate,
violationType,
flsViolationInfo.getValidationType().name(),
flsViolationInfo.getObjectName());
flsViolationInfo.getObjectName(),
fieldInformation);
Copy link
Contributor Author

@rmohan20 rmohan20 Jul 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now passing in fieldInformation as another parameter.


// Return the full validation message
return validationInformation;
}

Expand Down
77 changes: 70 additions & 7 deletions sfge/src/test/java/com/salesforce/rules/fls/apex/operations/FlsViolationUtilsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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(
Copy link
Contributor Author

@rmohan20 rmohan20 Jul 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified failing tests to use String formats instead of hardcoding the message. Will clean up the remaining tests at another iteration while applying Teresa's feedback.

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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down
3 changes: 2 additions & 1 deletion sfge/src/test/java/com/salesforce/testutils/ViolationWrapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<ViolationWrapper.FlsViolationBuilder, FlsViolationInfo> instanceSupplier;

Expand Down