Skip to content

Make permission checks namespace safe #154

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 75 additions & 0 deletions trigger-actions-framework/main/default/classes/ActionUtility.cls
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
/**
* Utilities for managing actions.
*
* Allows for the retrieval of the current user's permissions only once during the entire transaction
* Consolidates code that is shared between MetadataTriggerHandler and FinalizerHandler classes
*/

public with sharing class ActionUtility {
@TestVisible
public static Set<String> userPermissions {
get {
if (userPermissions == null) {
userPermissions = new Set<String>();

List<PermissionSetAssignment> permSets = [
SELECT AssigneeId, Assignee.FirstName, PermissionSetId, PermissionSet.Name
FROM PermissionSetAssignment
WHERE AssigneeId = :UserInfo.getUserId()
];
Set<Id> permSetIds = new Set<Id>();
for (PermissionSetAssignment psa : permSets) {
permSetIds.add(psa.PermissionSetId);
}
List<CustomPermission> customPerms = [
SELECT DeveloperName, MasterLabel, NamespacePrefix
FROM CustomPermission
WHERE Id IN (SELECT SetupEntityId FROM SetupEntityAccess WHERE ParentId IN :permSetIds)
];
for (CustomPermission cp : customPerms) {
String qualifiedPerm = String.isBlank(cp.NamespacePrefix)
? cp.DeveloperName
: cp.NamespacePrefix + '__' + cp.DeveloperName;
userPermissions.add(qualifiedPerm);
}
}
return userPermissions;
}
private set;
}

public static boolean isNotBypassed(String requiredPermission, String bypassPermission) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious why using isNOTBypassed - it can be confusing to the user/dev if Booleans are expressed negatively.

Gentle suggestion: Update to isBypassed and return the negative of the output (which is already a negative, I see).

return !((requiredPermission != null && userPermissions.contains(requiredPermission)) ||
(bypassPermission != null && !userPermissions.contains(bypassPermission)));
}

public static Object getActionInstance(String className) {
String namespace = '';
System.Type actionType = Type.forName(className);
/** Type.forName(fullyQualifiedName) allowed some messyness and ambiguity in dealing with namespace
* If config does not provide the correct namespace (likely if upgrading from older versions of this framework) we need to fallback in two scenarios
* - package and class namespaced but namespace wasn't specified
* - namespace is actually in the class field in the form namespace.classname
*/
// try shared Namespace
if (actionType == null) {
// Get the namespace of the current class.
String[] parts = String.valueOf(ActionUtility.class).split('\\.', 2);
namespace = parts.size() == 2 ? parts[0] : '';

// try again with the new namespace
actionType = Type.forName(namespace, className);
}
// try namespace in Class_Name field
if (actionType == null) {
String[] parts = className.split('\\.', 2);
if (parts.size() == 2) {
namespace = parts[0];
className = parts[1];
actionType = Type.forName(namespace, className);
}
}
Object dynamicInstance = actionType.newInstance();
return dynamicInstance;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<?xml version="1.0" encoding="UTF-8"?>
<ApexClass xmlns="http://soap.sforce.com/2006/04/metadata">
<apiVersion>61.0</apiVersion>
<status>Active</status>
</ApexClass>
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,6 @@ public with sharing virtual class FinalizerHandler {
@TestVisible
private static Set<String> bypassedFinalizers = new Set<String>();

@TestVisible
private static Map<String, Boolean> permissionMap = new Map<String, Boolean>();

/**
* @description Bypass the execution of a specific finalizer.
* @param finalizer The name of the finalizer to bypass.
Expand Down Expand Up @@ -103,10 +100,8 @@ public with sharing virtual class FinalizerHandler {
if (finalizerMetadata.Bypass_Execution__c) {
return;
}
populatePermissionMap(finalizerMetadata.Bypass_Permission__c);
populatePermissionMap(finalizerMetadata.Required_Permission__c);
if (
isNotBypassed(
ActionUtility.isNotBypassed(
finalizerMetadata.Bypass_Permission__c,
finalizerMetadata.Required_Permission__c
)
Expand All @@ -131,7 +126,7 @@ public with sharing virtual class FinalizerHandler {
return;
}
try {
dynamicInstance = Type.forName(className).newInstance();
dynamicInstance = ActionUtility.getActionInstance(className);
} catch (System.NullPointerException e) {
handleFinalizerException(INVALID_CLASS_ERROR_FINALIZER, className);
}
Expand Down Expand Up @@ -171,34 +166,6 @@ public with sharing virtual class FinalizerHandler {
);
}

/**
* @description Check if the finalizer is not bypassed.
* @param requiredPermission The required permission.
* @param bypassPermission The bypass permission.
* @return True if the finalizer is not bypassed, false otherwise.
*/
private boolean isNotBypassed(
String requiredPermission,
String bypassPermission
) {
return !((requiredPermission != null &&
permissionMap.get(requiredPermission)) ||
(bypassPermission != null && !permissionMap.get(bypassPermission)));
}

/**
* @description Populate the permission map.
* @param permissionName The permission name.
*/
private void populatePermissionMap(String permissionName) {
if (permissionName != null && !permissionMap.containsKey(permissionName)) {
permissionMap.put(
permissionName,
FeatureManagement.checkPermission(permissionName)
);
}
}

@TestVisible
private List<DML_Finalizer__mdt> allFinalizers {
get {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ private with sharing class FinalizerHandlerTest {
)
};

FinalizerHandler.permissionMap.put(BYPASS_PERMISSION, true);
ActionUtility.userPermissions.add(BYPASS_PERMISSION);
handler.handleDynamicFinalizers();

System.Assert.isTrue(
Expand All @@ -138,7 +138,7 @@ private with sharing class FinalizerHandlerTest {
)
};

FinalizerHandler.permissionMap.put(REQUIRED_PERMISSION, false);
ActionUtility.userPermissions = new Set<String>();
handler.handleDynamicFinalizers();

System.Assert.isTrue(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,6 @@ public inherited sharing class MetadataTriggerHandler extends TriggerBase implem
private static Set<String> bypassedActions = new Set<String>();
@TestVisible
private static Selector selector = new Selector();
@TestVisible
private static Map<String, Boolean> permissionMap = new Map<String, Boolean>();
private static Map<String, Map<String, List<Trigger_Action__mdt>>> sObjectToContextToActions = new Map<String, Map<String, List<Trigger_Action__mdt>>>();
@TestVisible
private static FinalizerHandler finalizerHandler = new FinalizerHandler();
Expand Down Expand Up @@ -225,20 +223,6 @@ public inherited sharing class MetadataTriggerHandler extends TriggerBase implem
finalizerHandler.handleDynamicFinalizers();
}

/**
* @description Populate the permission map.
*
* @param permissionName The name of the permission to check.
*/
private void populatePermissionMap(String permissionName) {
if (permissionName != null && !permissionMap.containsKey(permissionName)) {
permissionMap.put(
permissionName,
FeatureManagement.checkPermission(permissionName)
);
}
}

/**
* @description Get the Trigger Action metadata.
*
Expand Down Expand Up @@ -311,38 +295,12 @@ public inherited sharing class MetadataTriggerHandler extends TriggerBase implem
relationshipName + RELATIONSHIP_SUFFIX
))
.get(sObject_Trigger_Setting__mdt.Required_Permission__c);
for (
String permissionName : new List<String>{
actionMetadata.Bypass_Permission__c,
actionMetadata.Required_Permission__c,
sObjectBypassPermissionName,
sObjectRequiredPermissionName
}
) {
populatePermissionMap(permissionName);
}

return isNotBypassed(
return ActionUtility.isNotBypassed(
actionMetadata.Bypass_Permission__c,
actionMetadata.Required_Permission__c
) &&
isNotBypassed(sObjectBypassPermissionName, sObjectRequiredPermissionName);
}

/**
* @description Check if the Trigger Action is not bypassed.
*
* @param requiredPermission The required permission for the Trigger Action.
* @param bypassPermission The bypass permission for the Trigger Action.
* @return True if the Trigger Action is not bypassed, false otherwise.
*/
private static boolean isNotBypassed(
String requiredPermission,
String bypassPermission
) {
return !((requiredPermission != null &&
permissionMap.get(requiredPermission)) ||
(bypassPermission != null && !permissionMap.get(bypassPermission)));
ActionUtility.isNotBypassed(sObjectBypassPermissionName, sObjectRequiredPermissionName);
}

/**
Expand Down Expand Up @@ -385,8 +343,7 @@ public inherited sharing class MetadataTriggerHandler extends TriggerBase implem
for (Trigger_Action__mdt triggerMetadata : actionMetadata) {
Object triggerAction;
try {
triggerAction = Type.forName(triggerMetadata.Apex_Class_Name__c)
.newInstance();
triggerAction = ActionUtility.getActionInstance(triggerMetadata.Apex_Class_Name__c);
if (triggerMetadata.Flow_Name__c != null) {
((TriggerActionFlow) triggerAction)
.flowName = triggerMetadata.Flow_Name__c;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -626,7 +626,7 @@ private class MetadataTriggerHandlerTest {
new List<SObject>{ action }
);

MetadataTriggerHandler.permissionMap.clear();
ActionUtility.userPermissions = new Set<String>();

handler.beforeInsert(handler.triggerNew);

Expand All @@ -641,7 +641,7 @@ private class MetadataTriggerHandlerTest {
new List<SObject>{ action }
);

MetadataTriggerHandler.permissionMap.clear();
ActionUtility.userPermissions = new Set<String>();

handler.beforeInsert(handler.triggerNew);

Expand All @@ -654,9 +654,7 @@ private class MetadataTriggerHandlerTest {
MetadataTriggerHandler.selector = new FakeSelector(
new List<SObject>{ action }
);
MetadataTriggerHandler.permissionMap = new Map<String, Boolean>{
REQUIRED_PERMISSION => false
};
ActionUtility.userPermissions = new Set<String>();

handler.beforeInsert(handler.triggerNew);

Expand All @@ -669,9 +667,7 @@ private class MetadataTriggerHandlerTest {
MetadataTriggerHandler.selector = new FakeSelector(
new List<SObject>{ action }
);
MetadataTriggerHandler.permissionMap = new Map<String, Boolean>{
REQUIRED_PERMISSION => false
};
ActionUtility.userPermissions = new Set<String>();

handler.beforeInsert(handler.triggerNew);

Expand All @@ -684,9 +680,7 @@ private class MetadataTriggerHandlerTest {
MetadataTriggerHandler.selector = new FakeSelector(
new List<SObject>{ action }
);
MetadataTriggerHandler.permissionMap = new Map<String, Boolean>{
BYPASS_PERMISSION => true
};
ActionUtility.userPermissions = new Set<String>{BYPASS_PERMISSION};

handler.beforeInsert(handler.triggerNew);

Expand All @@ -699,9 +693,7 @@ private class MetadataTriggerHandlerTest {
MetadataTriggerHandler.selector = new FakeSelector(
new List<SObject>{ action }
);
MetadataTriggerHandler.permissionMap = new Map<String, Boolean>{
BYPASS_PERMISSION => true
};
ActionUtility.userPermissions = new Set<String>{BYPASS_PERMISSION};

handler.beforeInsert(handler.triggerNew);

Expand Down