From 89b38dfb821308d5fee647d5d06ca3aadd727e5b Mon Sep 17 00:00:00 2001 From: Robert Baillie Date: Thu, 9 Dec 2021 10:14:26 +0000 Subject: [PATCH] Documentation in the SecureDml class Improved stack trace point in security violation exceptions Improved naming of the violation handlers Added ability to disable CUD settings checking --- .../classes/exceptions/ortoo_Exception.cls | 14 +- .../default/classes/utils/Contract.cls | 1 - framework/main/default/classes/SecureDml.cls | 328 ++++++++++++++---- 3 files changed, 270 insertions(+), 73 deletions(-) diff --git a/framework/default/ortoo-core/default/classes/exceptions/ortoo_Exception.cls b/framework/default/ortoo-core/default/classes/exceptions/ortoo_Exception.cls index 8d67e5d33b7..e4f7b588977 100644 --- a/framework/default/ortoo-core/default/classes/exceptions/ortoo_Exception.cls +++ b/framework/default/ortoo-core/default/classes/exceptions/ortoo_Exception.cls @@ -175,11 +175,19 @@ public virtual class ortoo_Exception extends Exception implements IRenderableMes return returnList; } + // TODO: document + // TODO: test + public ortoo_Exception regenerateStackTraceString( Integer levelsToSkip ) + { + stackTraceString = createStackTraceString( levelsToSkip + 1 ); + return this; + } + private String createStackTraceString( Integer levelsToSkip ) { - return new StackTrace( levelsToSkip+1 ).getFullStackTraceString(); // Since custom exceptions have some problems getting their stack trace strings set, - // we need to get the Stack Trace string from the generic utility, stating that the top - // level method (this one) should be ignored. + return new StackTrace( levelsToSkip + 1 ).getFullStackTraceString(); // Since custom exceptions have some problems getting their stack trace strings set, + // we need to get the Stack Trace string from the generic utility, stating that the top + // level method (this one) should be ignored. } /** diff --git a/framework/default/ortoo-core/default/classes/utils/Contract.cls b/framework/default/ortoo-core/default/classes/utils/Contract.cls index 5651b25973a..58ed45d8d78 100644 --- a/framework/default/ortoo-core/default/classes/utils/Contract.cls +++ b/framework/default/ortoo-core/default/classes/utils/Contract.cls @@ -51,7 +51,6 @@ public class Contract * @param Boolean The condition that must be true * @param String The message to emit when the condition is not true */ - public static void ensures( Boolean condition, String message ) { if ( !condition ) diff --git a/framework/main/default/classes/SecureDml.cls b/framework/main/default/classes/SecureDml.cls index 1f6934c3464..ce4518544d8 100644 --- a/framework/main/default/classes/SecureDml.cls +++ b/framework/main/default/classes/SecureDml.cls @@ -1,10 +1,26 @@ -// TODO: document // TODO: test +// TODO: error codes +// TODO: labels +/** + * Is an implementation of the IDml interface used to manage the DML operations in an SObject Unit of Work. + * + * Implementation is secure by default, ensuring that FLS and CUD (CRUD without the Read) security is adhered to. + * + * Allows ther turning off of security at multiple levels: + * FLS checking for a given field + * FLS checking for a given SObject Type + * FLS checking for all SObjects + * CUD checking for a given SObject Type + * CUD checking for all SObjects + */ public inherited sharing class SecureDml extends fflib_SobjectUnitOfWork.SimpleDML implements fflib_SobjectUnitOfWork.IDml { public inherited sharing class SecureDmlException extends ortoo_Exception {} // TODO: add error code prefix - public interface UnableToDmlHandler + /** + * Interface that defines a handler for if and when a CUD violation occurs. + */ + public interface CudViolationHandler { void handleUnableToInsertRecords( List objList ); void handleUnableToUpdateRecords( List objList ); @@ -12,13 +28,16 @@ public inherited sharing class SecureDml extends fflib_SobjectUnitOfWork.SimpleD void handleUnableToPublishEvents( List objList ); } - public interface InaccessibleFieldsHandler + /** + * Interface that defines a handler for if and when an FLS violation occurs. + */ + public interface FlsViolationHandler { - void handleInaccessibleFields( String operation, String sobjectTypeName, Set removedFields ); + void handleInaccessibleFields( AccessType mode, SobjectType sobjectType, Set fieldsInViolation ); } - InaccessibleFieldsHandler inaccessibleFieldsHandler; - UnableToDmlHandler unableToDmlHandler; + FlsViolationHandler flsViolationHandler; + CudViolationHandler cudViolationHandler; Boolean ignoreCud = false; Set ignoreCudForSobjectTypes = new Set(); @@ -27,30 +46,56 @@ public inherited sharing class SecureDml extends fflib_SobjectUnitOfWork.SimpleD Set ignoreFlsForSobjectTypes = new Set(); Map> ignoreFlsForFields = new Map>(); + /** + * Default constructor that ensures that security violations result in Exceptions being thrown + */ public SecureDml() { - setInaccessibleFieldsHandler( new ErrorOnInaccessibleFieldsHandler() ); - setUnableToDmlHandler( new ErrorOnUnableToDmlHandler() ); + setFlsViolationHandler( new ErrorOnFlsViolationHandler() ); + setCudViolationHandler( new ErrorOnCudViolationHandler() ); } - public SecureDml setInaccessibleFieldsHandler( InaccessibleFieldsHandler inaccessibleFieldsHandler ) + /** + * Set the Handler that should be used when an FLS violoation occurs + * + * @param FlsViolationHandler The FLS violation handler to use + * @return SecureDml Itself, allowing for a fluent interface + */ + public SecureDml setFlsViolationHandler( FlsViolationHandler flsViolationHandler ) { - this.inaccessibleFieldsHandler = inaccessibleFieldsHandler; + this.flsViolationHandler = flsViolationHandler; return this; } - public SecureDml setUnableToDmlHandler( UnableToDmlHandler unableToDmlHandler ) + /** + * Set the Handler that should be used when an CUD violation occurs + * + * @param FlsViolationHandler The FLS violation handler to use + * @return SecureDml Itself, allowing for a fluent interface + */ + public SecureDml setCudViolationHandler( CudViolationHandler cudViolationHandler ) { - this.unableToDmlHandler = unableToDmlHandler; + this.cudViolationHandler = cudViolationHandler; return this; } + /** + * Disables FLS checking for this instance + * + * @return SecureDml Itself, allowing for a fluent interface + */ public SecureDml ignoreFls() { ignoreFls = true; return this; } + /** + * Disables FLS checking for all records of the given SObject Type + * + * @param SobjectType The Sobject Type for which to disable FLS + * @return SecureDml Itself, allowing for a fluent interface + */ public SecureDml ignoreFlsFor( SobjectType type ) { Contract.requires( type != null, 'ignoreFlsFor called with a null type' ); @@ -59,6 +104,13 @@ public inherited sharing class SecureDml extends fflib_SobjectUnitOfWork.SimpleD return this; } + /** + * Disables FLS checking for all records of the given SObject Field + * + * @param SobjectType The Sobject Type for which to disable FLS + * @param SobjectField The Sobject Field for which to disable FLS + * @return SecureDml Itself, allowing for a fluent interface + */ public SecureDml ignoreFlsFor( SobjectType type, SobjectField field ) { Contract.requires( type != null, 'ignoreFlsFor called with a null type' ); @@ -72,6 +124,36 @@ public inherited sharing class SecureDml extends fflib_SobjectUnitOfWork.SimpleD return this; } + /** + * Disables CUD settings checking for this instance + * + * @return SecureDml Itself, allowing for a fluent interface + */ + public SecureDml ignoreCudSettings() + { + ignoreCud = true; + return this; + } + + /** + * Disables CUD settings checking for all records of the given SObject Type + * + * @param SobjectType The Sobject Type for which to disable CUD checking + * @return SecureDml Itself, allowing for a fluent interface + */ + public SecureDml ignoreCudSettingsFor( SobjectType type ) + { + Contract.requires( type != null, 'ignoreCudSettingsFor called with a null type' ); + + ignoreCudForSobjectTypes.add( type ); + return this; + } + + /** + * Performs the DML Insert, whilst also checking CUD and FLS rights, based on the configuration. + * + * @param List The list of records to insert + */ public override void dmlInsert( List objList ) { if ( objList.isEmpty() ) @@ -79,20 +161,27 @@ public inherited sharing class SecureDml extends fflib_SobjectUnitOfWork.SimpleD return; } - if ( shouldCheckCud( objList[0] ) && ! SobjectUtils.isCreateable( objList[0] ) ) // TODO: probably don't need this + SobjectType type = SobjectUtils.getSobjectType( objList[0] ); + + if ( shouldCheckCud( type ) && ! SobjectUtils.isCreateable( objList[0] ) ) { - unableToDmlHandler.handleUnableToInsertRecords( objList ); + cudViolationHandler.handleUnableToInsertRecords( objList ); return; } - if ( shouldCheckFls( objList[0] ) ) + if ( shouldCheckFls( type ) ) { - checkFls( objList, AccessType.CREATABLE, 'insert' ); + checkFls( objList, AccessType.CREATABLE ); } insert objList; } + /** + * Performs the DML Update, whilst also checking CUD and FLS rights, based on the configuration. + * + * @param List The list of records to update + */ public override void dmlUpdate( List objList ) { if ( objList.isEmpty() ) @@ -100,19 +189,26 @@ public inherited sharing class SecureDml extends fflib_SobjectUnitOfWork.SimpleD return; } - if ( shouldCheckCud( objList[0] ) && ! SobjectUtils.isUpdateable( objList[0] ) ) + SobjectType type = SobjectUtils.getSobjectType( objList[0] ); + + if ( shouldCheckCud( type ) && ! SobjectUtils.isUpdateable( objList[0] ) ) { - unableToDmlHandler.handleUnableToUpdateRecords( objList ); + cudViolationHandler.handleUnableToUpdateRecords( objList ); return; } - if ( shouldCheckFls( objList[0] ) ) + if ( shouldCheckFls( type ) ) { - checkFls( objList, AccessType.UPDATABLE, 'update' ); + checkFls( objList, AccessType.UPDATABLE ); } update objList; } + /** + * Performs the DML Delete, whilst also checking CUD and FLS rights, based on the configuration. + * + * @param List The list of records to insert + */ public override void dmlDelete( List objList ) { if ( objList.isEmpty() ) @@ -120,15 +216,21 @@ public inherited sharing class SecureDml extends fflib_SobjectUnitOfWork.SimpleD return; } - if ( shouldCheckCud( objList[0] ) && ! SobjectUtils.isDeletable( objList[0] ) ) + SobjectType type = SobjectUtils.getSobjectType( objList[0] ); + + if ( shouldCheckCud( type ) && ! SobjectUtils.isDeletable( objList[0] ) ) { - unableToDmlHandler.handleUnableToDeleteRecords( objList ); + cudViolationHandler.handleUnableToDeleteRecords( objList ); return; } delete objList; } - // TODO: is this needed? + /** + * Performs the DML Publish, whilst also checking CUD and FLS rights, based on the configuration. + * + * @param List The list of records to insert + */ public override void eventPublish( List objList ) { if ( objList.isEmpty() ) @@ -136,21 +238,30 @@ public inherited sharing class SecureDml extends fflib_SobjectUnitOfWork.SimpleD return; } - if ( shouldCheckCud( objList[0] ) && ! SobjectUtils.isCreateable( objList[0] ) ) // SobjectUtils.isCreateable( objList[0] ) + SobjectType type = SobjectUtils.getSobjectType( objList[0] ); + + if ( shouldCheckCud( type ) && ! SobjectUtils.isCreateable( objList[0] ) ) // SobjectUtils.isCreateable( objList[0] ) { - unableToDmlHandler.handleUnableToPublishEvents( objList ); + cudViolationHandler.handleUnableToPublishEvents( objList ); return; } - if ( shouldCheckFls( objList[0] ) ) + if ( shouldCheckFls( type ) ) { - checkFls( objList, AccessType.CREATABLE, 'piblish' ); + checkFls( objList, AccessType.CREATABLE ); } EventBus.publish( objList ); } - private void checkFls( List objList, AccessType mode, String modeName ) + /** + * Checks the FLS for the given Sobject, in the given mode. + * In the case of violation, report it to the flsViolationHandler. + * + * @param List The list of records for which to to check the FLS + * @param AccessType The access type that needs to be checked + */ + private void checkFls( List objList, AccessType mode ) { String sobjectTypeName = SobjectUtils.getSobjectName( objList[0] ); SObjectAccessDecision securityDecision = Security.stripInaccessible( mode, objList ); @@ -163,35 +274,35 @@ public inherited sharing class SecureDml extends fflib_SobjectUnitOfWork.SimpleD if ( ! removedFields.isEmpty() ) { - inaccessibleFieldsHandler.handleInaccessibleFields( modeName, sobjectTypeName, removedFields ); + flsViolationHandler.handleInaccessibleFields( mode, SobjectUtils.getSobjectType( objList[0] ), removedFields ); replaceList( objList, strippedRecords ); } } } - private static void replaceList( List originalList, List newList ) + /** + * Reviews the configured 'ignoreFlsFor' fields for the given records and puts back any populated fields that should have been ignored. + * + * Is needed since we can't tell stripInaccessible to skip checking of certain fields. + * + * Will mutate the stripped records so that they now contain the specified field values again. + * + * @param Set The fields that were previously removed from the records + * @param List The original list of records, prior to field values being stripped + * @param List The new list of records, after the field values were stripped + * @return Set The new, potentially reduced list of 'removed fields + */ + private Set unstripAccessible( Set removedFields, List originalRecords, List strippedRecords ) { - Contract.requires( originalList != null, 'replaceList called with a null originalList' ); - 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++ ) - { - originalList[i] = newList[i]; - } - } + SobjectType type = SobjectUtils.getSobjectType( originalRecords[0] ); - private Set unstripAccessible(Set removedFields, List originalRecords, List strippedRecords ) - { - Set ignoredFlsFields = getIgnoredFlsFieldsForSobject( originalRecords[0] ); + Set ignoredFlsFields = getIgnoredFlsFields( type ); Set fieldsToUnstrip = removedFields.clone(); fieldsToUnstrip.retainAll( ignoredFlsFields ); - System.debug( 'fieldsToUnstrip: ' + fieldsToUnstrip ); Set remainingRemovedFields = removedFields.clone(); remainingRemovedFields.removeAll( fieldsToUnstrip ); - System.debug( 'remainingRemovedFields: ' + remainingRemovedFields ); if ( remainingRemovedFields.isEmpty() ) // nothing should have been stripped, so the original records are OK { @@ -204,16 +315,13 @@ public inherited sharing class SecureDml extends fflib_SobjectUnitOfWork.SimpleD return remainingRemovedFields; } - // We need to put the ignored, but stripped, fields back + // 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]; Sobject thisStrippedRecord = strippedRecords[i]; - System.debug( 'thisOriginalRecord: ' + thisOriginalRecord ); - System.debug( 'thisStrippedRecord: ' + thisStrippedRecord ); - Map populatedFields = thisOriginalRecord.getPopulatedFieldsAsMap(); for ( String thisFieldToUnstrip : fieldsToUnstrip ) @@ -223,8 +331,6 @@ public inherited sharing class SecureDml extends fflib_SobjectUnitOfWork.SimpleD thisStrippedRecord.put( thisFieldToUnstrip, thisOriginalRecord.get( thisFieldToUnstrip ) ); } } - System.debug( 'unstripped: ' + thisStrippedRecord ); - unstrippedRecords.add( thisStrippedRecord ); } @@ -232,7 +338,13 @@ public inherited sharing class SecureDml extends fflib_SobjectUnitOfWork.SimpleD return remainingRemovedFields; } - private Boolean shouldCheckCud( Sobject record ) + /** + * States if CUD settings should be checked for the given SObject type + * + * @param SobjectType The type for which to ascertain if CUD should be checked + * @return Boolean Should CUD be checked + */ + private Boolean shouldCheckCud( SobjectType type ) { if ( ignoreCud ) { @@ -242,10 +354,16 @@ public inherited sharing class SecureDml extends fflib_SobjectUnitOfWork.SimpleD { return true; } - return ! ignoreCudForSobjectTypes.contains( SobjectUtils.getSobjectType( record ) ); + return ! ignoreCudForSobjectTypes.contains( type ); } - private Boolean shouldCheckFls( Sobject record ) + /** + * States if FLS settings should be checked for the given SObject type + * + * @param SobjectType The type for which to ascertain if FLS should be checked + * @return Boolean Should FLS be checked + */ + private Boolean shouldCheckFls( SobjectType type ) { if ( ignoreFls ) { @@ -255,15 +373,19 @@ public inherited sharing class SecureDml extends fflib_SobjectUnitOfWork.SimpleD { return true; } - return ! ignoreFlsForSobjectTypes.contains( SobjectUtils.getSobjectType( record ) ); + return ! ignoreFlsForSobjectTypes.contains( type ); } - private Set getIgnoredFlsFieldsForSobject( Sobject record ) + /** + * Returns the Set of fields that should have FLS ignored, for the given SObject type + * + * @param SobjectType The type for which the ignored fields should be returned + * @return Set The fields to ignore the FLS of + */ + private Set getIgnoredFlsFields( SobjectType type ) { - System.debug( 'record: ' + record ); + Set fieldsToIgnore = ignoreFlsForFields.get( type ); - Set fieldsToIgnore = ignoreFlsForFields.get( SobjectUtils.getSobjectType( record ) ); - System.debug( 'fieldsToIgnore: ' + fieldsToIgnore ); if ( fieldsToIgnore == null ) { fieldsToIgnore = new Set(); @@ -271,23 +393,71 @@ public inherited sharing class SecureDml extends fflib_SobjectUnitOfWork.SimpleD return fieldsToIgnore; } - public inherited sharing virtual class ErrorOnUnableToDmlHandler implements UnableToDmlHandler + /** + * Given two lists, will update the first so that is contains the values in the second list. + * + * Allows parameters to be mutated with new values easily. + * + * Requires that the lists be of the same length. + * + * @param List The original list to be replaced + * @param List The new list to replace the original list with + */ + private static void replaceList( List originalList, List newList ) { + Contract.requires( originalList != null, 'replaceList called with a null originalList' ); + 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++ ) + { + originalList[i] = newList[i]; + } + } + + /** + * CudViolationHandler that ensures that exceptions are thrown when CUD violations occur + */ + public inherited sharing virtual class ErrorOnCudViolationHandler implements CudViolationHandler + { + /** + * Throws an exception describing the insert CUD violation + * + * @param List The list of SObjects that caused the violation. + */ public void handleUnableToInsertRecords( List objList ) { - throwUnableException( '00001', 'Attempted to insert {0} records without the required permission', objList ); + throwUnableException( '00001', 'Attempted to insert {0} records without the required permission', objList ); // TODO: label } + + /** + * Throws an exception describing the update CUD violation + * + * @param List The list of SObjects that caused the violation. + */ public void handleUnableToUpdateRecords( List objList ) { - throwUnableException( '00002', 'Attempted to update {0} records without the required permission', objList ); + throwUnableException( '00002', 'Attempted to update {0} records without the required permission', objList ); // TODO: label } + + /** + * Throws an exception describing the delete CUD violation + * + * @param List The list of SObjects that caused the violation. + */ public void handleUnableToDeleteRecords( List objList ) { - throwUnableException( '00003', 'Attempted to delete {0} records without the required permission', objList ); + throwUnableException( '00003', 'Attempted to delete {0} records without the required permission', objList ); // TODO: label } + + /** + * Throws an exception describing the publish CUD violation + * + * @param List The list of SObjects that caused the violation. + */ public void handleUnableToPublishEvents( List objList ) { - throwUnableException( '00004', 'Attempted to publish {0} events without the required permission', objList ); + throwUnableException( '00004', 'Attempted to publish {0} events without the required permission', objList ); // TODO: label } private void throwUnableException( String errorCode, String label, List objList ) @@ -296,20 +466,40 @@ public inherited sharing class SecureDml extends fflib_SobjectUnitOfWork.SimpleD throw new SecureDmlException( StringUtils.formatLabel( label, new List{ sobjectTypeName } ) ) .setErrorCode( '0000' ) .addContext( 'sobjectTypeName', sobjectTypeName ) - .addContext( 'records', objList ); + .addContext( 'records', objList ) + .regenerateStackTraceString( 2 ); // push the stack trace string into the point that called the hander, rather than the handler itself } } - public inherited sharing virtual class ErrorOnInaccessibleFieldsHandler implements InaccessibleFieldsHandler + /** + * FlsViolationHandler that ensures that exceptions are thrown when FLS violations occur + */ + public inherited sharing virtual class ErrorOnFlsViolationHandler implements FlsViolationHandler { - public void handleInaccessibleFields( String operation, String sobjectTypeName, Set removedFields ) + /** + * Throws an exception describing the FLS violation + * + * @param AccessType The mode of the operation that was being performed when the violation occurred + * @param SobjectType The Sobject Type that the violation occurred against + * @param Set The names of the fields that violated FLS + */ + public void handleInaccessibleFields( AccessType mode, SobjectType sobjectType, Set fieldsInViolation ) { - String label = 'Attempted to {0} {1} with fields that are not accessible: {2}'; + Map descriptionByMode = new Map{ + AccessType.CREATABLE => 'insert', // TODO: label + AccessType.UPDATABLE => 'update', // TODO: label + AccessType.UPSERTABLE => 'upsert' // TODO: label + }; + + String label = 'Attempted to {0} {1} with fields that are not accessible: {2}'; // TODO: label + String modeDescription = descriptionByMode.get( mode ); + String sobjectTypeName = SobjectUtils.getSobjectName( sobjectType ); - throw new SecureDmlException( StringUtils.formatLabel( label, new List{ operation, sobjectTypeName, removedFields.toString() } ) ) + throw new SecureDmlException( StringUtils.formatLabel( label, new List{ modeDescription, sobjectTypeName, fieldsInViolation.toString() } ) ) .setErrorCode( '0000' ) .addContext( 'sobjectTypeName', sobjectTypeName ) - .addContext( 'removedFields', removedFields ); + .addContext( 'fieldsInViolation', fieldsInViolation ) + .regenerateStackTraceString( 2 ); // push the stack trace string into the point that called the hander, rather than the handler itself } } } \ No newline at end of file