-
Notifications
You must be signed in to change notification settings - Fork 54
NEW (GraphEngine): @W-12408352@: Classifies "as user" DML operations as safe. #1080
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,86 @@ | ||
| package com.salesforce.graph.vertex; | ||
|
|
||
| import com.salesforce.apex.jorje.ASTConstants; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import org.apache.logging.log4j.LogManager; | ||
| import org.apache.logging.log4j.Logger; | ||
|
|
||
| /** Represents a vertex that can perform DML through Apex. */ | ||
| public abstract class DmlStatementVertex extends BaseSFVertex { | ||
| protected static final Logger LOGGER = LogManager.getLogger(DmlStatementVertex.class); | ||
|
|
||
| private static final String ACCESS_LEVEL_REFERENCE = "AccessLevel"; | ||
|
|
||
| private enum AccessLevel { | ||
| USER_MODE, | ||
| SYSTEM_MODE | ||
| } | ||
|
|
||
| private final AccessLevel accessLevel; | ||
|
|
||
| DmlStatementVertex(Map<Object, Object> properties) { | ||
| super(properties); | ||
| accessLevel = calculateAccessLevel(this); | ||
| } | ||
|
|
||
| private static AccessLevel calculateAccessLevel(DmlStatementVertex vertex) { | ||
| // Default to System mode to begin with. | ||
| AccessLevel accessLevel = AccessLevel.SYSTEM_MODE; | ||
|
|
||
| // If AccessLevel is included in the syntax, it's usually the last child. It shows up in the | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "Usually the last child" are there known cases where it won't be? Will those cases break here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When "as user" or "as system" is not included, the variable that contains the value for DML is the last child. If "as user" or "as system" is included, it'll be the last child. The conditionals below confirm other things in the code that follows. |
||
| // form of a VariableExpression with a ReferenceExpression child. | ||
|
|
||
| // spotless: off | ||
| // Example: | ||
| //<DmlUpdateStatement BeginColumn="9" BeginLine="6" DefiningType="MyClass" DefiningType_CaseSafe="myclass" EndLine="6" EndScopes="[BlockStatement]" FirstChild="false" LastChild="true" childIdx="2"> | ||
| // <VariableExpression BeginColumn="24" BeginLine="6" DefiningType="MyClass" DefiningType_CaseSafe="myclass" EndLine="6" FirstChild="true" LastChild="false" Name="a" Name_CaseSafe="a" childIdx="0"> | ||
| // <EmptyReferenceExpression BeginColumn="24" BeginLine="6" DefiningType="MyClass" DefiningType_CaseSafe="myclass" EndLine="6" FirstChild="true" LastChild="true" childIdx="0"/> | ||
| // </VariableExpression> | ||
| // <VariableExpression BeginColumn="30" BeginLine="-1" DefiningType="MyClass" DefiningType_CaseSafe="myclass" EndLine="-1" FirstChild="false" LastChild="true" Name="USER_MODE" Name_CaseSafe="user_mode" childIdx="1"> | ||
| // <ReferenceExpression BeginColumn="19" BeginLine="6" DefiningType="MyClass" DefiningType_CaseSafe="myclass" EndLine="6" FirstChild="true" LastChild="true" Name="AccessLevel" Name_CaseSafe="accesslevel" Names="[AccessLevel]" ReferenceType="LOAD" childIdx="0"/> | ||
| // </VariableExpression> | ||
| //</DmlUpdateStatement> | ||
|
|
||
| // spotless: on | ||
|
|
||
| final List<VariableExpressionVertex> children = | ||
| vertex.getChildren(ASTConstants.NodeType.VARIABLE_EXPRESSION); | ||
| if (children.size() > 0) { | ||
| final VariableExpressionVertex lastChild = children.get(children.size() - 1); | ||
| ReferenceExpressionVertex referenceExpression = | ||
| lastChild.getOnlyChildOrNull(ASTConstants.NodeType.REFERENCE_EXPRESSION); | ||
| if (referenceExpression != null) { | ||
| if (ACCESS_LEVEL_REFERENCE.equalsIgnoreCase(referenceExpression.getName())) { | ||
| // lastChild's name holds AccessLevel value | ||
| final String accessLevelValueString = lastChild.getName(); | ||
| final AccessLevel accessLevelValue = | ||
| AccessLevel.valueOf(accessLevelValueString); | ||
| if (accessLevelValue != null) { | ||
| accessLevel = accessLevelValue; | ||
| } else { | ||
| if (LOGGER.isInfoEnabled()) { | ||
| LOGGER.info( | ||
| "AccessLevel is unknown. accessLevelValueString=" | ||
| + accessLevelValueString); | ||
| } | ||
| } | ||
| } else { | ||
| if (LOGGER.isInfoEnabled()) { | ||
| LOGGER.info( | ||
| "Unknown ReferenceExpression name. referenceExpression=" | ||
| + referenceExpression); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return accessLevel; | ||
| } | ||
|
|
||
| /** | ||
| * @return true if the DML statement has System access level. | ||
| */ | ||
| public boolean isSystemMode() { | ||
| return accessLevel == AccessLevel.SYSTEM_MODE; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -142,10 +142,6 @@ public void createExpectedValidations(DmlStatementVertex vertex, SymbolProvider | |
| } | ||
|
|
||
| final List<BaseSFVertex> dmlStatementVertexChildren = vertex.getChildren(); | ||
| if (dmlStatementVertexChildren.size() != validationType.parameterCount) { | ||
| throw new UnexpectedException( | ||
| "Unexpected count of parameters: " + dmlStatementVertexChildren.size()); | ||
| } | ||
|
Comment on lines
-145
to
-148
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This check was me being extra cautious when I didn't understand ASTs better. This doesn't make sense anymore since ASTs will be standard based on compilation. |
||
|
|
||
| // Create expected validations based on the first parameter. | ||
| // Even though MERGE operation takes two parameters, both of them need to be of the same | ||
|
|
@@ -166,7 +162,6 @@ public void createExpectedValidations(DmlStatementVertex vertex, SymbolProvider | |
| if (LOGGER.isWarnEnabled()) { | ||
| LOGGER.warn( | ||
| "TODO: Apex value not detected for dml's child vertex: " + childVertex); | ||
| // TODO: add telemetry | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Todos were taken care of earlier. |
||
| } | ||
| violations.add( | ||
| FlsViolationCreatorUtil.createUnresolvedCrudFlsViolation( | ||
|
|
@@ -175,7 +170,6 @@ public void createExpectedValidations(DmlStatementVertex vertex, SymbolProvider | |
| } else { | ||
| if (LOGGER.isWarnEnabled()) { | ||
| LOGGER.warn("TODO: Child vertex of DML is not a chained vertex: " + childVertex); | ||
| // TODO: add telemetry | ||
| } | ||
| violations.add( | ||
| FlsViolationCreatorUtil.createUnresolvedCrudFlsViolation( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| package com.salesforce.rules.fls.apex; | ||
|
|
||
| import com.salesforce.rules.AbstractPathBasedRule; | ||
| import com.salesforce.rules.ApexFlsViolationRule; | ||
| import com.salesforce.testutils.BaseFlsTest; | ||
| import java.util.stream.Stream; | ||
| import org.junit.jupiter.params.ParameterizedTest; | ||
| import org.junit.jupiter.params.provider.Arguments; | ||
| import org.junit.jupiter.params.provider.MethodSource; | ||
|
|
||
| /** | ||
| * Tests to verify that "as user" invocation on DML does not cause violations on | ||
| * ApexFlsViolationRule. | ||
| */ | ||
| public class DmlAsUserTest extends BaseFlsTest { | ||
| private static final AbstractPathBasedRule RULE = ApexFlsViolationRule.getInstance(); | ||
|
|
||
| public static Stream<Arguments> input() { | ||
| return Stream.of( | ||
| Arguments.of( | ||
| "Insert_KeyValue1", | ||
| "Account a = new Account();\n" | ||
| + "a.name = 'Acme Inc.'\n;" | ||
| + "insert %s a;\n"), | ||
| Arguments.of( | ||
| "Insert_KeyValue2", | ||
| "Account a = new Account(Name = 'Acme Inc.');\n" + "insert %s a;\n"), | ||
| Arguments.of( | ||
| "Update", | ||
| "/* sfge-disable-next-line ApexFlsViolationRule */\n" | ||
| + "Account a = [SELECT Id, Name FROM Account];\n" | ||
| + "a.Name = 'Acme Inc.';\n" | ||
| + "update %s a;\n"), | ||
| Arguments.of( | ||
| "Delete", | ||
| "Account a = new Account(Id = '001abc000000001', Name = 'Acme Inc.');\n" | ||
| + "delete %s a;\n"), | ||
| Arguments.of( | ||
| "Merge", | ||
| "Account a1 = new Account(Name = 'Acme Inc.');\n" | ||
| + "Account a2 = new Account(Name = 'Acme');\n" | ||
| + "merge %s a1 a2;\n"), | ||
| Arguments.of( | ||
| "Undelete", | ||
| "/* sfge-disable-next-line ApexFlsViolationRule */\n" | ||
| + "Account a = [SELECT Id, Name FROM Account WHERE Name = 'Acme Inc' ALL ROWS];\n" | ||
| + "undelete %s a;\n")); | ||
| } | ||
|
|
||
| @MethodSource("input") | ||
| @ParameterizedTest(name = "{displayName}: {0}") | ||
| public void testDmlIsSafe(String testName, String dmlStatement) { | ||
| // spotless: off | ||
| String sourceCode = | ||
| "public class MyClass {\n" | ||
| + " public void foo() {\n" | ||
| + String.format(dmlStatement, "as user") | ||
| + " }\n" | ||
| + "}\n"; | ||
| // spotless: on | ||
|
|
||
| assertNoViolation(RULE, sourceCode); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic to parse "as user" and "as system". A DML statement with neither is considered as executing in system mode.