233 list out of bounds query factory

[wimvelzeboer]

#233 Fix for list out of bounds error in QueryFactory when executed without sharing for a community user on an object that the user doesn't have access to.

When a query with a relationship fields in the select is generated for a community user in a without sharing context on an object that is not accessible (CRUD), a list out of bounds error is returned.

The method Schema.getGlobalDescribe().get('NonAccessibleObjectName__c').getDescribe().fields.getMap().get('MyRelationShipFieldToAnotherNonAccessibleObject__c').getDescribe().getReferenceTo() will return null when executed as community user.

While if we get the describe of that SObject for that relationship field via Schema.getGlobalDescribe().get('NonAccessibleRelationshipObjectName__c').getDescribe(), it will just return the expected describe.

So we have access to both SObjects, as expected because we run without sharing, but we do not have access to get the getReferenceTo on the relationship field between both SObjects.

As the purpose of this getFieldPath method is to validate the field level security for individual fields in the referred relationship, we can bypass this whole method when enforceFLS is disabled.

---

This change is 

[foxysolutions]

@wimvelzeboer I'm not fully sure whether I agree with this change. In my perspective getFieldPath() performs more than just a FLS check. It also validates the field existence and, with that, ensures a distinct list of fields. By excluding that when FLS should not be asserted, one could end up having duplicate fields in the query (caused by typos).

fflib_QueryFactory qf = new fflib_QueryFactory(Contact.SObjectType);
qf.selectField('NAMe');
qf.selectFields( new Set<String>{'naMe', 'email'});
System.debug( '*** '+ qf.getSelectedFields() );

Result original code: {Email, Name}
Result modified code when FLS is false : {NAMe, email, naMe}

A suggestion could be to update the token validation, saying:

if( token == null && enforceFLS ){
   throw new InvalidFieldException( fieldName, this.table );
}

Although this would still come across odd to be. The GetDescribe in without sharing mode should not return null for a field that does exist right?

[daveespo]

When I saw the code for this PR, I wanted to do the same test that @foxcreation did -- thanks for supplying the test case that illustrates why circumventing getFieldPath() has undesirable side effects

I agree that this PR can't be merged as-is

The test case illustrated in #233 may be flawed: You can't change the sharing model of a class using inheritance. From the Apex docs: https://developer.salesforce.com/docs/atlas.en-us.apexcode.meta/apexcode/apex_classes_keywords_sharing.htm

Classes inherit this setting from a parent class when one class extends or implements another.

So I think the root cause of getReferenceTo() returning null is that fflib_SObjectSelector is running with sharing regardless of the ElevatedSelector's sharing model

This is related to the discussion over on #199 -- I think we need to resolve that PR first before tackling this one

@afawcett -- keep me honest on the sharing model of subclasses

cc @ImJohnMDaniel

[daveespo]

I need to walk some of my previous comment back based on further experimentation we did.

In summary: The Apex docs are wrong. The sharing model used for executing SOQL is the sharing model defined on the class where the query actually runs. Inheritance doesn't come in to play at all.

Consider the following base class example:

public virtual with sharing class BaseClass {
	public List<Contact> getContactsWithSharing(){
		return [SELECT Id, FirstName, LastName, Email FROM Contact];
	}
	
	public virtual List<Contact> getContactsUnclearSharing(){
		return [SELECT Id, FirstName, LastName, Email FROM Contact];
	}
}

and a class that derives from it:

public inherited sharing class OverrideClass extends BaseClass{
	public List<Contact> getContactsInhertedSharing(){
		return [SELECT Id, FirstName, LastName, Email FROM Contact];
	}
	
	public List<Contact> getContactsDelegateSharing(){
		return super.getContactsUnclearSharing();
	}
}

This is the result of calling methods from a WITHOUT SHARING context:

new OverrideClass().getContactsWithSharing(); //Returns zero rows (this is expected, according to the docs)
new OverrideClass().getContactsInheritedSharing(); //Returns all rows (see Note 1 below)
new OverrideClass().getContactsDelegateSharing(); //Returns zero rows (see Note 2 below)

Note 1: This is unexpected -- it directly contradicts the Apex Doc that says that "Classes inherit this setting from a parent class when one class extends or implements another." -- We're empirically observing that sharing model of the base class is irrelevant to the sharing model of methods defined in the subclass

Note 2: This is unexpected -- We're calling a method defined in a superclass and the sharing model is changing to the sharing model of the superclass

Based on the example that @wimvelzeboer provided on #233, he should not run afoul of a sharing-related restriction during construction of the QueryFactory so I have no explanation as to why the getReferenceTo() is coming back empty

@afawcett and @capeterson -- what's the process for getting a doc bug filed? And maybe an example to go with it :-)

[wimvelzeboer]

@foxcreation Thank you for you insight in the usecases of the getFieldPath() method, and indeed I think we should make some changes to this PR.
@daveespo I think the issue is caused by something unforeseen by Salesforce, so I would think this is an actual bug in Salesforce.

@ImJohnMDaniel @afawcett would it be an idea to validate the output of tokenDescribe.getReferenceTo(). If it returns a null value and enforceFLS is disabled that we only in that scenario return the string 'fieldName'?

@afawcett Do you also agree that this is a bug in Salesforce?

[capeterson]

@daveespo thanks for flagging. You can generally get a doc bug filed by just using the inline doc feedback form - that goes into a triage queue where the vast majority end up filed as bugs.

But since you flagged me down, no reason to trouble the doc writer triage queue, I'll make sure there's no exceptions to what you're showing and log the bug directly.

[daveespo]

Super, thanks @capeterson

[JAertgeerts]

@capeterson / @daveespo - has this bug been taken up or slated for future releases?

[JAertgeerts]

This has forced us to apply the fix to the library in this commit. But would rather love salesforce to obviously resolve the internal.

[cropredyHelix]

@ImJohnMDaniel @afawcett would it be an idea to validate the output of tokenDescribe.getReferenceTo(). If it returns a null value and enforceFLS is disabled that we only in that scenario return the string 'fieldName'?

N.B. tokenDescribe.getReferenceTo() returns empty list, not null when running user doesn't have access to the object. There are other use cases besides the Community one; e.g. querying PermissionSetAssignment field Assignee.Profile.Name when user doesn't have View All Data

[daveespo]

Per the comment on #309 and with Spring '21 and the #317 API upgrade, this should be resolved. Please reopen the issue if you find differently