apex-enterprise-patterns / fflib-apex-common

Common Apex Library supporting Apex Enterprise Patterns and much more!
BSD 3-Clause "New" or "Revised" License
907 stars 515 forks source link

fflib_SObjectSelector - How to override record level security #135

Closed daveerickson closed 7 years ago

daveerickson commented 8 years ago

If you define a class as without sharing you are able to let the apex code run in system mode, without care for CRUD or FLS requirements. This is useful for a process that a user may kick off, but may not have access to records/fields needed during the process.

My use case specifically is building out a way on Asset to assign the Account via a value (Firm CRD) that is on the Account record and can be entered on the Asset record to lookup and find the Account record and assign the Asset's Account field to that Account.

I have this logic in an Assets domain class. I have an inner class SetAccount defined without sharing with the logic needed to query and set the value of Account on the Asset record. If I write out the SOQL, [SELECT Id, Name, Firm_CRD__c FROM Account WHERE Firm_CRD__c IN :firmCRDs], everything works fine, but If I call my AccountsSelector class with a method to do the same thing if the record is not visible to the running user it is not returned.

I've tried instantiating the selector class with enforcingCRUD to false, and verified within the method via System.debug that isEnforcingCRUD() is false. Still the record is not being returned by the user who otherwise does not have access to the record.

I looked through the fflib_SObjectSelector, fflib_QueryFactory, and fflib_SecurityUtils classes to see what is happening. It appears that the SObjectSelector calls to QueryFactor which calls SecurityUtils class if enforcing CRUD is true. It can check if the user has access to the object, but it does not have a way to verify access to a record. since the SObjectSelector class is defined with sharing it is weeding out any records the running user does not have access to.

For now I can just leave the written out SOQL to make this work, but it seems like there should be a way to run the Selector in a without sharing context. Maybe I'm missing something and it's in there already.

afawcett commented 8 years ago

CRUD and FLS does not control record visibility so these will have no effect.

In terms of Sharing, Apex runs by default in system mode (unless you are in immediate code in Execute Anonymous). As per my thoughts in the blog below your Service layer should switch to user mode via "with sharing" keyword on the service class. Other classes, such as domain and selector should not specify it, and as such inherit from the caller. Only when you need to see all records should you elevate to the "without sharing" context.

Apex Sharing and applying to Apex Enterprise Patterns

To me, despite it being a class level thing in Apex, sharing is a query filter concern, and belongs in the Selector layer, which of course manages all your queries. So if you have a Selector method that runs a query that needs this, either implicitly elevate (within the method implementation only) or let the caller specify when they want to see all records or not, as a method parameter. Much as you would any other criteria the Selector method may want to expose to pass into the query it runs.

Either way requirement to control visibility of records is encapsulated in the selector implementation code and the caller does not need to themselves override the default "with sharing" context the service layer entry point has established.

Let me know if this makes sense, great question! 👍

daveerickson commented 8 years ago

Wow, I feel sort of silly to have built the Domain class to use an inner class without sharing, but didn't think to do the same thing on the Selector. Thank you for all the details and link to the blog post. I think I have a working solution that allows the caller to specify whether or not to call the selector without sharing. Here is my solution which is slightly different than yours but works. Do you see any issues with this approach?

public List<Account> selectByFirmCRD(Set<String> firmCRDs)
{
    selectByFirmCRD(firmCRDs, TRUE);
}

public List<Account> selectByFirmCRD(Set<String> firmCRDs, Boolean withSharing)
{
    if (withSharing) return (List<Account>) Database.query(newQueryFactory().setCondition('Firm_CRD__c IN :firmCRDs).toSOQL());
    else return new WithoutSharing().selectByFirmCRD(this, firmCRDs);
}

private without sharing class WithoutSharing
{
    private List<Account> selectByFirmCRD(AccountsSelector selector, Set<String> firmCRDs)
    {
        return List<Account> Database.query(selector.newQueryFactory().setCondition('Firm_CRD__c IN :firmCRDs).toSOQL());
    }
}

This approach ends up being a bit redundant, especially if you have subselects, or other modifications to the query factory. Still worth it to allow tighter visibility settings. Thanks!

afawcett commented 8 years ago

Yep spot on, you got it! 👍

Only improve would be to reuse the query code, like so...

public List<Account> selectByFirmCRD(Set<String> firmCRDs)
{
    selectByFirmCRD(firmCRDs, TRUE);
}

public List<Account> selectByFirmCRD(Set<String> firmCRDs, Boolean withSharing)
{
    if (withSharing) return internalSelectByFirmCRD(firmCRDs);
    else return new WithoutSharing().selectByFirmCRD(this, firmCRDs);
}

private List<Account> internalSelectByFirmCRD(Set<String> firmCRDs) 
{
    return (List<Account>) Database.query(newQueryFactory().setCondition('Firm_CRD__c IN :firmCRDs).toSOQL());
} 

private without sharing class WithoutSharing
{
    private List<Account> selectByFirmCRD(AccountsSelector selector, Set<String> firmCRDs)
    {
        return selector.internalSelectByFirmCRD(firmCRDs);
    }
}
afawcett commented 8 years ago

There is a probably another variation of the above using inheritance with the inner classes. Might be tidier if you have many methods on the selector where you want to provide the 'withSharing' parameter.

afawcett commented 8 years ago

Also i know some devs i speak to prefer to have method name clarity over using a parameter e.g.

selector.selectByFirmCRDWithoutSharing(firmCRDS);

vs

selector.selectByFirmCRD(firmCRDS, false);

Second approach might be clearer to new eyes.

Side note, i miss named parameters in .Net... ;-)

daveerickson commented 8 years ago

Sorry, I was on vacation. Just getting back to this project.

What I found with your method is that visibility was still limited when calling the private internal method through the passed in selector. I'm guessing the Database.query call is respecting the sharing of the outer class since that is where the method resides. Instead I was able to get it to work by passing along the dynamic soql string into the without sharing inner class and calling the Database.query from there. It looks like this:

public List<Account> selectByFirmCRD(Set<String> firmCRDs)
{
    return selectByFirmCRD(firmCRDs, TRUE);
}

public List<Account> selectByFirmCRD(Set<String> firmCRDs, Boolean withSharing)
{
    String query = newQueryFactory().setCondition('Firm_CRD__c IN :firmCRDs').toSOQL());
    if (withSharing) return (List<Account>) Database.query(query);
    else return new WithoutSharing().selectByFirmCRD(firmCRDs, query);
}

private without sharing class WithoutSharing
{
    private List<Account> selectByFirmCRD(Set<String> firmCRDs, String query)
    {
        return (List<Account>) Database.query(query);
    }
}

Maybe I'm missing something about your approach. I'd love to hear your take on my observations.

Side note, named parameters sound awesome. I've never developed in .net, but man does that look like a great way to solve some issues with passing numerous parameters. I wonder if that sort of solve is something that could be baked into apex in the future. Here's to hoping.

afawcett commented 7 years ago

Sorry for the delay @daveerickson, yeah that's likely it. Take a look at this variant i used on the rollup tool. Might be another way to consider?