Pmd: [apex] False positives with ApexCRUDViolation

[trentchilders]

Affects PMD Version:
All
Rule:
ApexCrudViolation
Description:
Hello, I have encountered what I believe to be a false positive in the ApexCRUDViolation rule. Specifically the READ portion of that rule (or an isAccessible() check in Apex). I created a helper method so that I could dynamically do this check rather than hardcode every single field that I'm querying for. However, this still causes ApexCRUDViolation to fail its read check.
Code Sample demonstrating the issue:
Helper method:

public with sharing class Utilities {
  public static void checkReadAccess(Map<String,Set<String>> fieldMap){
        Set<String> keySet = fieldMap.keySet();

        for(String key : keySet){

            Schema.SObjectType objectType = Schema.getGlobalDescribe().get(key);

            Schema.DescribeSObjectResult describeSObject = objectType.getDescribe(); 
            if(!describeSObject.isAccessible()){
                throw new System.NoAccessException();
            }
            Map<String,Schema.SObjectField> schemaFieldMap = describeSObject.fields.getMap();
            for(String fieldName : fieldMap.get(key)){
                Schema.DescribeFieldResult describeField = schemaFieldMap.get(fieldName).getDescribe();
                if(!describeField.isAccessible()){
                    throw new System.NoAccessException();
                }
            }
        }
}

And then to call it:

Set<String> fieldSet;
fieldSet = new Set<String>{'Id','Name'};
Map<String,Set<String>> fieldMap;
fieldMap = new Map<String,Set<String>>{'Account'=>fieldSet};
Utilities.checkReadAccess(fieldMap);
List<Account> templateList = [SELECT Id, Name FROM Account];

Running PMD through: Codacy

[jsotuyod]

@trentchilders thanks for the report.

This is indeed a false positive, but it's one that PMD can't currently sort out. PMD is currently limited to same-file analysis. So, when it checks the code:

Set<String> fieldSet;
fieldSet = new Set<String>{'Id','Name'};
Map<String,Set<String>> fieldMap;
fieldMap = new Map<String,Set<String>>{'Account'=>fieldSet};
Utilities.checkReadAccess(fieldMap);
List<Account> templateList = [SELECT Id, Name FROM Account];

PMD has no idea what Utilities.checkReadAccess is, or what it does, and is therefore unable to waive this usage.

[trentchilders]

Thanks for the reply!

[trentchilders]

So, even by adding the method to the file, I still get the false positive. Is this expected behavior?

public static void Foo(){
   Set<String> fieldSet;
   fieldSet = new Set<String>{'Id','Name'};
   Map<String,Set<String>> fieldMap;
   fieldMap = new Map<String,Set<String>>{'Account'=>fieldSet};
   checkReadAccess(fieldMap);

   List<Account> templateList = [SELECT Id, Name FROM Account];
}

public static void checkReadAccess(Map<String,Set<String>> fieldMap){
        Set<String> keySet = fieldMap.keySet();

        for(String key : keySet){

            Schema.SObjectType objectType = Schema.getGlobalDescribe().get(key);

            Schema.DescribeSObjectResult describeSObject = objectType.getDescribe(); 
            if(!describeSObject.isAccessible()){
                throw new System.NoAccessException();
            }
            Map<String,Schema.SObjectField> schemaFieldMap = describeSObject.fields.getMap();
            for(String fieldName : fieldMap.get(key)){
                Schema.DescribeFieldResult describeField = schemaFieldMap.get(fieldName).getDescribe();
                if(!describeField.isAccessible()){
                    throw new System.NoAccessException();
                }
            }
        }

[jsotuyod]

@trentchilders yes, this is expected. As you use a Map and Set, working this out would require not only having access to checkReadAccess but understanding how the map / set works (what get(key) does, etc.) and following through the code which values are in the map and set.

The one thing we support is a List of fields when using the ESAPI:

List<String> fieldsToView = new List<String> { 'Id', 'Name' };
if (ESAPI.accessController().isAuthorizedToView(Account.sObject, fieldsToView)) {
    …
}

but support for this is very tailor made.

Moreover, the check is currently very naive, as Apex has no real control flow support (unlike Java). This means, we can understand:

Account.sObjectType.getDescribe().isAccessible()

But we don't follow through a split statement

Schema.DescribeFieldResult describeField = Account.sObjectType.getDescribe()
describeField.isAccessible()

There is plenty of room for improvement, shall anyone be willing to take on this. Adding full support for control flow on Apex is a major task, but one that would greatly help many rules besides this one.