From 7a3ab3d7a18649ad60d8517044e64cbb9bcadccd Mon Sep 17 00:00:00 2001
From: Sergey Gorbaty <sergey.gorbaty@salesforce.com>
Date: Tue, 10 Jan 2017 09:46:10 -0800
Subject: [PATCH] Accessibility checks improved with CF

---
 .../rule/security/ApexCRUDViolationRule.java  | 17 ++++--
 .../rule/security/xml/ApexCRUDViolation.xml   | 60 +++++++++++++++++++
 2 files changed, 71 insertions(+), 6 deletions(-)

diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java
index 059bd441f7..b9591d66fd 100644
--- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java
+++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java
@@ -95,7 +95,7 @@ public class ApexCRUDViolationRule extends AbstractApexRule {
 
     @Override
     public Object visit(ASTMethodCallExpression node, Object data) {
-        performMethodLevelChecks(node);
+        collectCRUDMethodLevelChecks(node);
         return data;
     }
 
@@ -192,7 +192,7 @@ public class ApexCRUDViolationRule extends AbstractApexRule {
 
     }
 
-    private void performMethodLevelChecks(final ASTMethodCallExpression node) {
+    private void collectCRUDMethodLevelChecks(final ASTMethodCallExpression node) {
         final String method = node.getNode().getMethodName();
         final ASTReferenceExpression ref = node.getFirstChildOfType(ASTReferenceExpression.class);
         if (ref == null) {
@@ -270,9 +270,9 @@ public class ApexCRUDViolationRule extends AbstractApexRule {
     }
 
     private void checkForCRUD(final AbstractApexNode<?> node, final Object data, final String crudMethod) {
-        final HashSet<ASTMethodCallExpression> prevCalls = getPreviousCalls(node);
+        final HashSet<ASTMethodCallExpression> prevCalls = getPreviousMethodCalls(node);
         for (ASTMethodCallExpression prevCall : prevCalls) {
-            performMethodLevelChecks(prevCall);
+            collectCRUDMethodLevelChecks(prevCall);
         }
 
         final ASTMethod wrappingMethod = node.getFirstParentOfType(ASTMethod.class);
@@ -301,7 +301,7 @@ public class ApexCRUDViolationRule extends AbstractApexRule {
         }
     }
 
-    private HashSet<ASTMethodCallExpression> getPreviousCalls(final AbstractApexNode<?> self) {
+    private HashSet<ASTMethodCallExpression> getPreviousMethodCalls(final AbstractApexNode<?> self) {
         final HashSet<ASTMethodCallExpression> innerMethodCalls = new HashSet<>();
 
         final ASTBlockStatement blockStatement = self.getFirstParentOfType(ASTBlockStatement.class);
@@ -414,7 +414,12 @@ public class ApexCRUDViolationRule extends AbstractApexRule {
         }
     }
 
-    private void checkForAccessibility(final AbstractApexNode<?> node, Object data) {
+    private void checkForAccessibility(final ASTSoqlExpression node, Object data) {
+        final HashSet<ASTMethodCallExpression> prevCalls = getPreviousMethodCalls(node);
+        for (ASTMethodCallExpression prevCall : prevCalls) {
+            collectCRUDMethodLevelChecks(prevCall);
+        }
+
         boolean isGetter = false;
         String returnType = null;
 
diff --git a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexCRUDViolation.xml b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexCRUDViolation.xml
index 0d3835c8e4..aadadb5398 100644
--- a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexCRUDViolation.xml
+++ b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexCRUDViolation.xml
@@ -542,4 +542,64 @@ public class Foo {
 	
 } ]]></code>
 	</test-code> 
+	
+	<test-code>
+		<description>Control flow accessibility CRUD check</description>
+		<expected-problems>0</expected-problems>
+		<code><![CDATA[ 
+public class Foo { 	
+	public Contact justGiveMeFoo() {
+		checkPerms(); 
+		
+		String tempID = 'someID';
+	 	return [SELECT Name FROM Contact WHERE Id=:tempID];
+	} 
+	
+	private void checkPerms() {
+		if (!Contact.sObjectType.getDescribe().isAccessible()) {
+			throw new NoAccessException();
+		}
+	}
+} 
+]]></code>
+	</test-code> 
+	
+	<test-code>
+		<description>Control flow substitute CRUD check</description>
+		<expected-problems>0</expected-problems>
+		<code><![CDATA[ 
+public class Foo { 	
+	public Contact justGiveMeFoo() {
+		checkPerms();
+		String tempID = 'someID';
+	 	return [SELECT Name FROM Contact WHERE Id=:tempID];
+	} 
+	
+	private void checkPerms() {
+		if (!Contact.sObjectType.getDescribe().isCreateable()) {
+			throw new NoAccessException();
+		}
+	}
+} 
+]]></code>
+	</test-code>
+	
+	<test-code>
+		<description>Forgot to call the CRUD check</description>
+		<expected-problems>1</expected-problems>
+		<code><![CDATA[ 
+public class Foo { 	
+	public Contact justGiveMeFoo() {
+		String tempID = 'someID';
+	 	return [SELECT Name FROM Contact WHERE Id=:tempID];
+	} 
+	
+	private void checkPerms() {
+		if (!Contact.sObjectType.getDescribe().isCreateable()) {
+			throw new NoAccessException();
+		}
+	}
+} 
+]]></code>
+	</test-code>
 </test-data>