Skip to content

Commit

Permalink
Fixed bug with recognising when fields have been removed
Browse files Browse the repository at this point in the history 
Fixed bug with replication of stripped records not happening
Improvements to performance and memory usage
Added test for large numbers of records.
  • Loading branch information
@rob-baillie-ortoo
rob-baillie-ortoo committed Dec 9, 2021
1 parent 6ba0371 commit dbe8a39
Show file tree
Hide file tree
Showing 2 changed files with 74 additions and 33 deletions.
17 changes: 7 additions & 10 deletions framework/default/ortoo-core/default/classes/fflib-extension/SecureDml.cls
View file
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@ public inherited sharing virtual class SecureDml extends fflib_SobjectUnitOfWork
String sobjectTypeName = SobjectUtils.getSobjectName( objList[0] );
SecurityDecision securityDecision = stripInaccessible( mode, objList );

if ( securityDecision.fieldsWereRemoved() )
if ( securityDecision.fieldsWereRemoved( sobjectTypeName ) )
{
Set<String> removedFields = securityDecision.getRemovedFieldsFor( sobjectTypeName );
List<Sobject> strippedRecords = securityDecision.getRecords();
Expand Down Expand Up @@ -340,8 +340,6 @@ public inherited sharing virtual class SecureDml extends fflib_SobjectUnitOfWork
return remainingRemovedFields;
}

// We need to put the ignored, but previously stripped, fields back
List<Sobject> unstrippedRecords = new List<Sobject>();
for ( Integer i=0; i < originalRecords.size(); i++ )
{
Sobject thisOriginalRecord = originalRecords[i];
Expand All @@ -356,10 +354,8 @@ public inherited sharing virtual class SecureDml extends fflib_SobjectUnitOfWork
thisStrippedRecord.put( thisFieldToUnstrip, thisOriginalRecord.get( thisFieldToUnstrip ) );
}
}
unstrippedRecords.add( thisStrippedRecord );
}

replaceList( strippedRecords, unstrippedRecords );
return remainingRemovedFields;
}

Expand Down Expand Up @@ -434,7 +430,7 @@ public inherited sharing virtual class SecureDml extends fflib_SobjectUnitOfWork
Contract.requires( newList != null, 'replaceList called with a null newList' );
Contract.requires( originalList.size() == newList.size(), 'replaceList called with lists that are different sizes' );

for ( Integer i; i < originalList.size(); i++ )
for ( Integer i=0; i < originalList.size(); i++ )
{
originalList[i] = newList[i];
}
Expand Down Expand Up @@ -504,13 +500,14 @@ public inherited sharing virtual class SecureDml extends fflib_SobjectUnitOfWork
}

/**
* States if fields were removed from the sobjects
* States if fields were removed from the given Sobject Type
*
* @return Boolean Where fields removed?
* @param String The Name of the Sobject Type to check
* @return Boolean Were fields removed
*/
public Boolean fieldsWereRemoved()
public Boolean fieldsWereRemoved( String sobjectTypeName )
{
return !records.isEmpty();
return !removedFields.isEmpty() && removedFields.containsKey( sobjectTypeName );
}

/**
Expand Down
90 changes: 67 additions & 23 deletions framework/default/ortoo-core/default/classes/fflib-extension/tests/SecureDmlTest.cls
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
@isTest
private without sharing class SecureDmlTest
{
private static final Integer LOTS_OF_RECORDS = 10000;

@isTest
private static void dmlInsert_whenTheUserCanCreateTheRecords_willInsertTheRecords() // NOPMD: Test method name format
{
Expand All @@ -26,6 +28,27 @@ private without sharing class SecureDmlTest
System.assertEquals( 'Account2', createdAccounts[1].Name, 'dmlInsert, when the user can create the records, will insert the records, setting the fields on the records that are created (1)' );
}

@isTest
private static void dmlInsert_whenAskedToCreateALotOfRecords_willInsertTheRecords() // NOPMD: Test method name format
{
List<Account> accounts = new List<Account>();
for ( Integer i=0; i < LOTS_OF_RECORDS; i++ )
{
accounts.add( new Account( Name = 'Account' + i ) );
}

Test.startTest();

SecureDml dml = new TestableSecureDml();
dml.dmlInsert( accounts );

Test.stopTest();

List<Account> createdAccounts = getAccountsInserted();

System.assertEquals( LOTS_OF_RECORDS, createdAccounts.size(), 'dmlInsert, when the user can create the records, will insert the records without blowing CPU or Heap size limits' );
}

@isTest
private static void dmlInsert_whenTheUserCannotCreateRecords_willThrowAnException() // NOPMD: Test method name format
{
Expand Down Expand Up @@ -493,11 +516,38 @@ private without sharing class SecureDmlTest
System.assertEquals( new Set<String>{ 'Name' }, context.getValue(), 'dmlInsert, when the user cannot create records because of FLS and not all fields are switched off, will throw an exception with a context named fieldsInViolation set to the fields that were in violation minus those switched off' );
}

// when exception is supressed, will strip the fields
/*
System.assertEquals( null, accounts[0].Name, 'dmlInsert, when the user cannot create records because of FLS, will update the records to remove the fields in violation' );
System.assertEquals( 1, accounts[0].NumberOfEmployees, 'dmlInsert, when the user cannot create records because of FLS, will ensure the supressed fields keep their values' );
*/
@isTest
private static void dmlInsert_whenTheUserCannotCreateRecordsDueToFlsAndNullHandler_willStripFields() // NOPMD: Test method name format
{
List<Account> accounts = new List<Account>
{
new Account( Name = 'Account1', NumberOfEmployees = 1 ),
new Account( Name = 'Account2', NumberOfEmployees = 2 )
};

List<Account> strippedAccounts = new List<Account>
{
new Account(),
new Account()
};

Set<String> accountFieldsBlockedByFls = new Set<String>{ 'Name', 'NumberOfEmployees' };
Map<String,Set<String>> removedFields = new Map<String,Set<String>>{ 'Account' => accountFieldsBlockedByFls };
SecureDml.SecurityDecision stripsNumberOfEmployees = new SecureDml.SecurityDecision( removedFields, strippedAccounts );

Test.startTest();
SecureDml dml = new TestableSecureDml()
.ignoreFlsFor( Account.sobjectType, Account.NumberOfEmployees )
.setFlsViolationHandler( new SwallowOnFlsViolationHandler() );

((TestableSecureDml)dml).stripInaccessibleSecurityDecision = stripsNumberOfEmployees;

dml.dmlInsert( accounts );
Test.stopTest();

System.assertEquals( null, accounts[0].Name, 'dmlInsert, when the user cannot create records because of FLS and handler does not throw exception, will update the records to remove the fields in violation' );
System.assertEquals( 1, accounts[0].NumberOfEmployees, 'dmlInsert, when the user cannot create records because of FLS and handler does not throw exception, will ensure the supressed fields keep their values' );
}

private static List<Account> getAccountsInserted()
{
Expand All @@ -510,7 +560,8 @@ private without sharing class SecureDmlTest
private static List<Sobject> recordsPublished;

// version of SecureDml that allows us to override the checks on whether
// the current user can create / update / delete
// the current user can create / update / delete as well as the actual DML
// Overriding the DML allows the tests to run in any environment
private inherited sharing class TestableSecureDml extends SecureDml
{
public Boolean canCreate = true;
Expand Down Expand Up @@ -576,24 +627,17 @@ private without sharing class SecureDmlTest
}
}

private inherited sharing class FAccount extends sfab_FabricatedSobject
private inherited sharing class SwallowOnFlsViolationHandler implements SecureDml.FlsViolationHandler
{
public FAccount()
{
super( Account.class );
name( 'Default Name' );
}

public FAccount name( String name )
{
set( Account.Name, name );
return this;
}
public void handleInaccessibleFields( AccessType mode, SobjectType sobjectType, Set<String> fieldsInViolation ) {} // NOPMD: intentionally left blank
}

public FAccount numberOfEmployees( Integer numberOfEmployees )
{
set( Account.NumberOfEmployees, numberOfEmployees );
return this;
}
private inherited sharing class SwallowOnCudViolationHandler implements SecureDml.CudViolationHandler
{
public void handleUnableToInsertRecords( List<SObject> objList ) {} // NOPMD: intentionally left blank
public void handleUnableToUpdateRecords( List<SObject> objList ) {} // NOPMD: intentionally left blank
public void handleUnableToDeleteRecords( List<SObject> objList ) {} // NOPMD: intentionally left blank
public void handleUnableToPublishEvents( List<SObject> objList ) {} // NOPMD: intentionally left blank
}

}

0 comments on commit dbe8a39

Please sign in to comment.