From dbe8a390f543afb37e82efb91bd9210e8fada19a Mon Sep 17 00:00:00 2001 From: Robert Baillie Date: Thu, 9 Dec 2021 17:29:13 +0000 Subject: [PATCH] Fixed bug with recognising when fields have been removed Fixed bug with replication of stripped records not happening Improvements to performance and memory usage Added test for large numbers of records. --- .../classes/fflib-extension/SecureDml.cls | 17 ++-- .../fflib-extension/tests/SecureDmlTest.cls | 90 ++++++++++++++----- 2 files changed, 74 insertions(+), 33 deletions(-) diff --git a/framework/default/ortoo-core/default/classes/fflib-extension/SecureDml.cls b/framework/default/ortoo-core/default/classes/fflib-extension/SecureDml.cls index be0a848f148..8213fb109b2 100644 --- a/framework/default/ortoo-core/default/classes/fflib-extension/SecureDml.cls +++ b/framework/default/ortoo-core/default/classes/fflib-extension/SecureDml.cls @@ -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 removedFields = securityDecision.getRemovedFieldsFor( sobjectTypeName ); List strippedRecords = securityDecision.getRecords(); @@ -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 unstrippedRecords = new List(); for ( Integer i=0; i < originalRecords.size(); i++ ) { Sobject thisOriginalRecord = originalRecords[i]; @@ -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; } @@ -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]; } @@ -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 ); } /** diff --git a/framework/default/ortoo-core/default/classes/fflib-extension/tests/SecureDmlTest.cls b/framework/default/ortoo-core/default/classes/fflib-extension/tests/SecureDmlTest.cls index 5bbe44ca896..abb0c37ae7e 100644 --- a/framework/default/ortoo-core/default/classes/fflib-extension/tests/SecureDmlTest.cls +++ b/framework/default/ortoo-core/default/classes/fflib-extension/tests/SecureDmlTest.cls @@ -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 { @@ -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 accounts = new List(); + 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 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 { @@ -493,11 +516,38 @@ private without sharing class SecureDmlTest System.assertEquals( new Set{ '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 accounts = new List + { + new Account( Name = 'Account1', NumberOfEmployees = 1 ), + new Account( Name = 'Account2', NumberOfEmployees = 2 ) + }; + + List strippedAccounts = new List + { + new Account(), + new Account() + }; + + Set accountFieldsBlockedByFls = new Set{ 'Name', 'NumberOfEmployees' }; + Map> removedFields = new Map>{ '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 getAccountsInserted() { @@ -510,7 +560,8 @@ private without sharing class SecureDmlTest private static List 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; @@ -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 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 objList ) {} // NOPMD: intentionally left blank + public void handleUnableToUpdateRecords( List objList ) {} // NOPMD: intentionally left blank + public void handleUnableToDeleteRecords( List objList ) {} // NOPMD: intentionally left blank + public void handleUnableToPublishEvents( List objList ) {} // NOPMD: intentionally left blank } + } \ No newline at end of file