From 873f3527b292aca1f524d9241bdcd1ca45cac4d3 Mon Sep 17 00:00:00 2001
From: Scott Wells <scott@illuminatedcloud.com>
Date: Thu, 21 Oct 2021 16:47:16 -0500
Subject: [PATCH] Added information about an observed bug with authorization
 checks against sub-relations in static SOQL queries. Added a test case that
 reproduces the bug. Once the bug is fixed, the unit test expectation can be
 changed to provide both verification and a check against regressions.

---
 .../rule/security/ApexCRUDViolationRule.java  |  3 ++
 .../rule/security/xml/ApexCRUDViolation.xml   | 28 +++++++++++++++++++
 2 files changed, 31 insertions(+)

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 afe668a4ef..6462a70886 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
@@ -660,6 +660,9 @@ public class ApexCRUDViolationRule extends AbstractApexRule {
     }
 
     private void checkForAccessibility(final ASTSoqlExpression node, Object data) {
+        // TODO: This includes sub-relation queries which are incorrectly flagged because you authorize the type
+        //  and not the sub-relation name. Should we (optionally) exclude sub-relations until/unless they can be
+        //  resolved to the proper SObject type?
         final Set<String> typesFromSOQL = getTypesFromSOQLQuery(node);
 
         final Set<ASTMethodCallExpression> prevCalls = getPreviousMethodCalls(node);
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 7d7e4ab192..b814c5ce61 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
@@ -1489,6 +1489,34 @@ public class Foo {
             merge masterAccount mergeAccount;
         }
     }
+}
+        ]]></code>
+    </test-code>
+
+    <!-- Demonstrate that authorization for sub-relation queries doesn't work properly; see checkForAccessibility() -->
+
+    <test-code>
+        <description>Demonstrate that authorization for sub-relation queries doesn't work properly</description>
+        <!-- This is what we should get if things were working properly -->
+        <!--<expected-problems>0</expected-problems>-->
+        <!-- But this is what we actually get -->
+        <expected-problems>1</expected-problems>
+        <code><![CDATA[
+public class Foo {
+    public void foo() {
+        if (Account.SObjectType.getDescribe().isAccessible() &&
+            // This check isn't working because it's checking against "Contacts" due to the nested FROM clause
+            // instead of "Contact" given that "Contacts" is of type "List<Contact>"
+            Contact.SObjectType.getDescribe().isAccessible()) {
+            List<Account> accounts = [
+                SELECT Id, (
+                    SELECT Id
+                    FROM Contacts
+                )
+                FROM Account
+            ];
+        }
+    }
 }
         ]]></code>
     </test-code>