Skip to content
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

Default behavior of including Standard Fields in ApplicationSObjectSelector tries to add inaccessible fields #57

Open
goto-dev-null opened this issue Jul 1, 2021 · 4 comments

Comments

@goto-dev-null
Copy link

This is more of a question than anything but what is the reasoning behind having getStandardFields()? I'm experiencing fflib_SecurityUtils exceptions due to some of the standard fields not being readable when it comes time to execute the query. It seems to me that it is making the assumption that users will have access to all standard fields for any object they have read access to, but I'm not sure if I'm missing something. At the very least, should it be its own thing separate from includeFieldSetFields since it's not even a field set?

If I do understand the problem correctly, a solution I'd like to propose is -if enforcingFLS- ignore fields that are inaccessible.

            Schema.DescribeFieldResult fieldDescribe = field.getDescribe();

            if ( ! fieldDescribe.isCustom() )
            {
                if ( fieldDescribe.isAccessible() || ! isEnforcingFLS() )
                {
                    standardFields.add( field );
                    system.debug( LoggingLevel.FINEST, 'Standard field : ' + field + ' added');
                }
                else
                {
                    system.debug( LoggingLevel.FINEST, 'Standard field : ' + field + ' NOT added');
                }
            }
@ClayChipps
Copy link
Contributor

@jonnypetraglia Can you share your selector class implementation? Sorry, I know this is not a direct answer but want to understand your implementation before I comment further.

@goto-dev-null
Copy link
Author

Thanks for the response. As you'll see it's about as barebones as it can be. For completeness sake I will attach the atdx_AppSObjectSelector but I just forked that recently so it should be unchanged aside from namespacing.

public class GHExample extends atdx_AppSObjectSelector{
    public static GHExample newInstance(){
        return (GHExample) atdx_App.Selector.newInstance(Account.SObjectType);
    }
    public Schema.SObjectType getSObjectType(){
        return Account.sObjectType;
    }
    public List<Schema.SObjectField> getAdditionalSObjectFieldList(){
        return new List<Schema.SObjectField> {
            Account.Id,
            Account.Name
        };
    }
    public GHExample(){
        this(true, true, true);
    }
    public GHExample(Boolean includeFieldSetFields, Boolean enforceCRUD, Boolean enforceFLS){
        super(includeFieldSetFields, enforceCRUD, enforceFLS);
    }
    public List<Account> selectByName(Set<String> lookupValues){
        fflib_QueryFactory queryFactory = newQueryFactory(true);
        return (List<Account>)Database.query(queryFactory.setCondition('Name IN :lookupValues').toSOQL());
    }
}

If I try to execute (new GHExample()).selectByName(new Set<String>{'Ahhhhhhhh'}); I get an unhandled Exception:
"Line: 261, Column: 1 - fflib_SecurityUtils.FlsException: You do not have permission to read the field Fax on Account"

Please note that this has no FieldSet inclusions. Fax is referenced nowhere in the entire package, but AT4DX presumes I should have access to it and all standard fields.

For my own curiosity, what's the reasoning behind including standard fields? You could just as easily do it for all fields on the SObject for every Selector implementation instead of specifying Fields and FieldSets. I thought that the reason for the latter was for performance reasons but pulling in Standard Fields seems to contradict that. And while it doesn't happen often, it seems like this would break horribly if a new Standard Field was added on an Object without permissions for every user.

@ClayChipps
Copy link
Contributor

Originally, the reasoning was to prevent the boilerplate to include the standard fields for every object. Thinking more along the lines of, why wouldn't we just include Id, CreatedDate, LastModifiedBy, etc as part of the selector by default. This line of thinking was interesting, but as you have found, ran into the permissions issues and other associated problems.

John or John can jump in and probably provide you more historical context as to why the functionality is still present/plans to improve it in the future, but I can provide you the "workaround" to avoid the issue entirely.

public override List<Schema.SObjectField> getSObjectFieldList() {
     return new List<Schema.SObjectField>{
         Account.Id,
         Account.Name
    };
}

private List<Schema.SObjectField> getAdditionalSObjectFieldList() {
    return new List<Schema.SObjectField>{};
}

This will override the virtual getSObjectFieldList method and completely negate all of the code that pulls in the standard fields. You can then flip the getAdditionalSObjectFieldList method to just return an empty list of Schema.SObjectFields, and you should be good to go, never worrying about standard fields being pulled in again.

@goto-dev-null
Copy link
Author

Ah, that makes a ton of sense, thanks for the explanation.

The workaround works for sure but requires putting it in in every selector implementation across multiple packages. I think the original code block solution I posted (adding a check for FLS) is the way I'll deal with it for now. That implementation also makes more sense to me as it is not a breaking change; if anyone was depending on Standard Fields being pulled in before nothing will change. But I very much don't want to have any difference from upstream so I'm still invested in finding a solution for this repo, if I can help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants