Skip to content
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

Security Bug Fixes #7215

Merged
merged 37 commits into from
Nov 16, 2023
Merged
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
35b0382
Add CRUD/FLS checks
npsp-reedestockton Oct 16, 2023
93e7521
Exclude 'npe03' and '__r' from queryFields
npsp-reedestockton Oct 17, 2023
c77c878
Fix logic for applying UTIL_Namespace.StrTokenNSPrefix()
npsp-reedestockton Oct 17, 2023
febbb83
Change exception class to UTIL_Permissions.InsufficientPermissionExce…
npsp-reedestockton Oct 19, 2023
b663013
Add update check for Opportunity Name
npsp-reedestockton Oct 19, 2023
a51cd13
Change exception class to UTIL_Permissions.InsufficientPermissionExce…
npsp-reedestockton Oct 19, 2023
0d64cfd
Refactor try/catch block
npsp-reedestockton Oct 23, 2023
4c007ee
Check security on Data Import and Form Template
npsp-reedestockton Oct 31, 2023
a76dee9
Fix broken test and aurahandled message
npsp-reedestockton Oct 31, 2023
35e9e25
Refactor try/catch logic
npsp-reedestockton Nov 1, 2023
a2c516a
Remove AuraEnabled notation from deleteFormTemplates()
npsp-reedestockton Nov 1, 2023
bd46c61
Refactor/Move code
npsp-reedestockton Nov 1, 2023
6f7f36a
Refactor canUpsertDataImport()
npsp-reedestockton Nov 2, 2023
7a57eaa
Refactor and add comments.
npsp-reedestockton Nov 3, 2023
8182dd7
Add FLS and refactor.
npsp-reedestockton Nov 5, 2023
19675e5
Add FLS and refactor.
npsp-reedestockton Nov 5, 2023
f422dd2
Add FLS for CampaignMember.Status
npsp-reedestockton Nov 6, 2023
f98e9d7
Add FLS for queries and check CRUD for deletes
npsp-reedestockton Nov 6, 2023
a6b6eca
Check isMergeable and strip inaccessible fields from search results.
npsp-reedestockton Nov 8, 2023
0dc6a96
Add Opportunity fields to required and call check hasFeatureAccess
npsp-reedestockton Nov 9, 2023
ea2a4b9
Fix unit tests / set hasFeatureAccess true
npsp-reedestockton Nov 9, 2023
96a2464
Add OCR read, modify and delete to hasAccess. Check hasAccess in save().
npsp-reedestockton Nov 9, 2023
454e70c
Add hasAccess check to controller and page
npsp-reedestockton Nov 10, 2023
82d6a0c
Revert unnecessary changes...
npsp-reedestockton Nov 10, 2023
626ebad
Fix access checks
npsp-reedestockton Nov 10, 2023
917b6a9
Fix access checks
npsp-reedestockton Nov 10, 2023
c1e144d
Prevent page access without read access to basic lead fields
npsp-reedestockton Nov 10, 2023
636dc39
Fix failing HH_ManageHH_Test.testNewHHObject()
npsp-reedestockton Nov 10, 2023
8c4f932
Check for AuraHandledErrorMessage in shouldNotReturnPauseDataWhenUser…
npsp-reedestockton Nov 10, 2023
81f86c3
Merge 8c4f932b356cfcded2fdf7de6d4afebdd1ae4d8f into feature/248__secu…
salesforce-org-metaci[bot] Nov 10, 2023
a3b24f2
Check for standard Level__c field creation permission
npsp-reedestockton Nov 10, 2023
6e52c17
Remove fields to check from hasFieldReadAccess()
npsp-reedestockton Nov 15, 2023
cb4557e
Remove Undeliverable__c from fields to check for Address read access
npsp-reedestockton Nov 15, 2023
e122060
Add permissions check for CampaignMemberStatus
npsp-reedestockton Nov 15, 2023
1a05e57
Remove @TestVisible annotation
npsp-reedestockton Nov 15, 2023
2245fbf
Fix copy/paste error resulting in bad build
npsp-reedestockton Nov 15, 2023
d8ff2e9
Merge pull request #7216 from SalesforceFoundation/feature/248__secur…
npsp-reedestockton Nov 16, 2023
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
44 changes: 43 additions & 1 deletion force-app/main/default/classes/CON_ContactMerge_CTRL.cls
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,16 @@ public with sharing class CON_ContactMerge_CTRL {

public Boolean canContinueWithMerge { get;set; }

public List<FieldSetMember> fieldSetMembers {
get {
if (fieldSetMembers == null) {
fieldSetMembers = SObjectType.Contact.fieldSets.ContactMergeFoundFS.getFields();
}
return fieldSetMembers;
}
set;
}

public Boolean hasContactObjectDeletePermission() {
return UTIL_Describe.getObjectDescribe('Contact').isDeletable();
}
Expand Down Expand Up @@ -923,7 +933,7 @@ public with sharing class CON_ContactMerge_CTRL {
public PageReference search() {
try {
step = 2;
this.searchResults = wrapQueryResults(searchRecords());
this.searchResults = wrapQueryResults(stripInaccessibleResultFields(searchRecords()));


} catch (exception ex) {
Expand All @@ -949,6 +959,33 @@ public with sharing class CON_ContactMerge_CTRL {
}
}

private List<SObject> stripInaccessibleResultFields(List<SObject> searchResults) {
SObjectAccessDecision accessDecision =
Security.stripInaccessible(AccessType.READABLE, searchResults);

Map<String, Set<String>> removedFields = accessDecision.getRemovedFields();
if (!removedFields.isEmpty()) {
List<FieldSetMember> strippedFieldSetMembers = new List<FieldSetMember>();
for (FieldSetMember fsMember:fieldSetMembers) {
Boolean fieldRemoved = false;
for (String field:removedFields.get('Contact')) {
if (fieldRemoved) {
continue;
}
if (fsMember.getFieldPath().contains(field)) {
fieldRemoved = true;
}
}
if (!fieldRemoved) {
strippedFieldSetMembers.add(fsMember);
}
}
fieldSetMembers = strippedFieldSetMembers;
}

return accessDecision.getRecords();
}

/***********************************************************************************************
* @description Wraps the Query(SOSL and SOQL) results.
* @param searchResults The list of SObjects to wrap.
Expand All @@ -971,6 +1008,11 @@ public with sharing class CON_ContactMerge_CTRL {
* @return PageReference The page that it redirects to. Same page user is in.
*/
public PageReference mergeContacts() {
if (!(Contact.SObjectType.getDescribe().isMergeable()) && Account.SObjectType.getDescribe().isMergeable()){
ApexPages.addMessage(new ApexPages.Message(ApexPages.Severity.Error, System.Label.commonAccessErrorMessage));
return null;
}

Contact winningContact = getWinningContact();

if (winningContact == null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,14 +75,15 @@ public with sharing class CON_DeleteContactOverrideSelector {
}

public Account getAccountRecord(Id accountId) {
return [SELECT Id, Name FROM Account WHERE Id = :accountId];
return [SELECT Id, Name FROM Account WHERE Id = :accountId WITH SECURITY_ENFORCED];
}

public List<Case> getCasesRelatedToContact(Id contactId) {
return [
SELECT CaseNumber, ContactId
FROM Case
WHERE ContactId = :contactId
WITH SECURITY_ENFORCED
];
}

Expand All @@ -91,6 +92,7 @@ public with sharing class CON_DeleteContactOverrideSelector {
SELECT CaseNumber, AccountId
FROM Case
WHERE AccountId = :accountId
WITH SECURITY_ENFORCED
];
}

Expand All @@ -99,6 +101,7 @@ public with sharing class CON_DeleteContactOverrideSelector {
SELECT Name, AccountId, Primary_Contact__c, Primary_Contact__r.AccountId, IsWon, IsClosed
FROM Opportunity
WHERE Primary_Contact__c = :contactId
WITH SECURITY_ENFORCED
];
}

Expand All @@ -107,6 +110,7 @@ public with sharing class CON_DeleteContactOverrideSelector {
SELECT Name, AccountId, IsWon, IsClosed
FROM Opportunity
WHERE AccountId = :accountId
WITH SECURITY_ENFORCED
];
}

Expand All @@ -115,6 +119,7 @@ public with sharing class CON_DeleteContactOverrideSelector {
SELECT Name, npe03__Contact__c
FROM npe03__Recurring_Donation__c
WHERE npe03__Contact__c = :contactId
WITH SECURITY_ENFORCED
];
}
}
10 changes: 10 additions & 0 deletions force-app/main/default/classes/CON_DeleteContactOverride_CTRL.cls
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,13 @@ public with sharing class CON_DeleteContactOverride_CTRL {
public PageReference deleteContact() {
if (isDeleteContactOnly()) {

if (!(UTIL_Permissions.canDelete('Contact', false) &&
UTIL_Permissions.canDelete('Opportunity', false) &&
UTIL_Permissions.canDelete('npe03__Recurring_Donation__c', false)))
{
throw new UTIL_Permissions.InsufficientPermissionException(System.Label.commonAccessErrorMessage);
}

ContactCascadeDelete cascadeDelete = new ContactCascadeDelete(contactRecord, contactOverrideSelector);
cascadeDelete.validate();
cascadeDelete.deleteOpportunities();
Expand Down Expand Up @@ -307,6 +314,9 @@ public with sharing class CON_DeleteContactOverride_CTRL {
Account account = contactOverrideSelector.getAccountRecord(accountId);

try {
if (!UTIL_Permissions.canDelete('Account', false)) {
throw new UTIL_Permissions.InsufficientPermissionException(System.Label.commonAccessErrorMessage);
}
AccountCascadeDelete cascadeDelete = new AccountCascadeDelete(account, contactOverrideSelector);
cascadeDelete.validate();

Expand Down
63 changes: 62 additions & 1 deletion force-app/main/default/classes/GE_GiftEntryController.cls
Original file line number Diff line number Diff line change
Expand Up @@ -424,17 +424,44 @@ public with sharing class GE_GiftEntryController {
@AuraEnabled
public static DataImport__c upsertDataImport(String dataImport) {
DataImport__c dataImportObject = (DataImport__c)JSON.deserialize(dataImport, DataImport__c.class);

try {
// As currently implemented, Gift Entry already checks everything checked in canUpsertDataImport() before
// allowing access. As a result, canUpsertDataImport() should never return false. It is implemented solely
// as a defense against future modifications since it is an AuraEnabled method that could be used outside
// of the currently implemented flow.
if (!canUpsertDataImport(dataImportObject)) {
throw new UTIL_Permissions.InsufficientPermissionException(System.Label.commonAccessErrorMessage);
}
upsert dataImportObject Id;

return dataImportObject;
} catch (UTIL_Permissions.InsufficientPermissionException e) {
throw new AuraHandledException(e.getMessage());
} catch (Exception e) {
String JSONExceptionData = ERR_ExceptionData.createExceptionWrapperJSONString(e);

throw buildDmlException(JSONExceptionData);
}
}

private static Boolean canUpsertDataImport(DataImport__c dataImportObject) {
if (!UTIL_Permissions.canCreate(UTIL_Namespace.StrAllNSPrefix('DataImport__c'))) {
return false;
}

for (String fieldName : dataImportObject.getPopulatedFieldsAsMap().keySet()) {
if (!UTIL_Permissions.canUpdate(UTIL_Namespace.StrAllNSPrefix('DataImport__c'),
UTIL_Namespace.StrAllNSPrefix(fieldName), false)) {
if (!fieldName.equalsIgnoreCase('Id')) {
return false;
}
}
}

return true;
}

/*******************************************************************************************************
* @description Run the DataImport process on a single gift
* @param dataImport DataImport record to be processed
Expand Down Expand Up @@ -1062,7 +1089,6 @@ public with sharing class GE_GiftEntryController {
* @return FormTemplateWrapper: Wrapper object of the list of deleted template names and the result
* of the DML action
*/
@AuraEnabled
public static String [] deleteFormTemplates(String[] ids) {
String[] formTemplateNames = new String[] {};
Form_Template__c[] templates = [
Expand Down Expand Up @@ -1162,6 +1188,17 @@ public with sharing class GE_GiftEntryController {
String description,
String formatVersion,
String templateJSON) {

Set<String> fieldsToCheck = new Set<String>{
'Name',
'Description__c',
'Template_JSON__c',
'Format_Version__c'
};
if (!canUpsertFormTemplate(fieldsToCheck)) {
throw new UTIL_Permissions.InsufficientPermissionException(System.Label.commonAccessErrorMessage);
}

if (templateJSON != null) {
Form_Template__c templateObj = new Form_Template__c(Id = id,
Name = name,
Expand All @@ -1175,6 +1212,22 @@ public with sharing class GE_GiftEntryController {
return null;
}


private static Boolean canUpsertFormTemplate(Set<String> fieldsToCheck) {
if (!UTIL_Permissions.canCreate(UTIL_Namespace.StrAllNSPrefix('Form_Template__c'))) {
return false;
}

for (String fieldName : fieldsToCheck) {
if (!UTIL_Permissions.canUpdate(UTIL_Namespace.StrAllNSPrefix('Form_Template__c'),
UTIL_Namespace.StrAllNSPrefix(fieldName), false)) {
return false;
}
}

return true;
}

/*******************************************************************************************************
* @description Method checks if the provided name is in use by another existing Form Template.
*
Expand Down Expand Up @@ -1345,6 +1398,14 @@ public with sharing class GE_GiftEntryController {
}

private static String retrieveBatchCurrencyIsoCode (Id batchId) {
// As currently implemented, Gift Entry already verifies edit access to DataImportBatch__c before allowing
// access. As a result, a permission error should never be encountered here. It is implemented solely as a
// defense against future modifications since it is called by an AuraEnabled method that could be used
// outside of the currently implemented flow.
if (!UTIL_Permissions.canRead(UTIL_Namespace.StrAllNSPrefix('DataImportBatch__c'), false)) {
throw new UTIL_Permissions.InsufficientPermissionException(System.Label.commonAccessErrorMessage);
}

String query = new UTIL_Query()
.withSelectFields(new Set<String>{UTIL_Currency.CURRENCY_ISO_CODE_FIELD})
.withFrom(DataImportBatch__c.SObjectType)
Expand Down
4 changes: 4 additions & 0 deletions force-app/main/default/classes/HH_CampaignDedupeBTN_CTRL.cls
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,10 @@ public without sharing class HH_CampaignDedupeBTN_CTRL {
********************************************************************************************************/
public static integer MarkDuplicatesFromList(ID campaignId, list<CampaignMember> listCM) {

if (!UTIL_Permissions.canUpdate('CampaignMember', 'Status', false)) {
throw new UTIL_Permissions.InsufficientPermissionException(System.Label.commonAccessErrorMessage);
}

final String DUPE_STATUS_SUFFIX = System.Label.hhCmpDedupeStatus;

//check for the Household Duplicate status values we need
Expand Down
97 changes: 71 additions & 26 deletions force-app/main/default/classes/HH_Container_LCTRL.cls
Original file line number Diff line number Diff line change
Expand Up @@ -362,8 +362,8 @@ public with sharing class HH_Container_LCTRL {
* @param listCon The list of Contacts to save
* @return void
*/
@AuraEnabled
public static void upsertContacts(List<Contact> listCon) {
@TestVisible
private static void upsertContacts(List<Contact> listCon) {
// Even though we are given a list of Contacts from the lightning component,
// apex seems to treat them as generic sObjects, and thus we can't do upsert.
// thus we will split the list into update and insert lists.
Expand Down Expand Up @@ -482,12 +482,11 @@ public with sharing class HH_Container_LCTRL {
* @param listHHMerge the list of Households to merge into the winner
* @return void
*/
@AuraEnabled
public static void mergeHouseholds(Account hhWinner, List<Account> listHHMerge) {
@TestVisible
private static void mergeHouseholds(Account hhWinner, List<Account> listHHMerge) {
try {
// Check object permissions
if (!Account.SObjectType.getDescribe().isUpdateable() ||
!Account.SObjectType.getDescribe().isDeletable()) {
if (!Account.SObjectType.getDescribe().isMergeable()) {
throw new System.NoAccessException();
}

Expand All @@ -508,30 +507,76 @@ public with sharing class HH_Container_LCTRL {
*/
@AuraEnabled
public static void saveHouseholdPage(SObject hh, List<Contact> listCon, List<Contact> listConRemove, List<Account>listHHMerge) {
// We need to determine if the new Default Address is Undeliverable
Address__c defaultAddress = getAddressFromAccount(hh.Id, hh);
defaultAddress.Household_Account__c = hh.Id;
Map<Address__c, Address__c> addressMatch = Addresses.getExistingAddresses(new List<Address__c>{defaultAddress});
if(addressMatch.containsKey(defaultAddress) && addressMatch.get(defaultAddress) != null){
selectedAddressIsUndeliverable = addressMatch.get(defaultAddress)?.Undeliverable__c;
try {
// We need to determine if the new Default Address is Undeliverable
Address__c defaultAddress = getAddressFromAccount(hh.Id, hh);
defaultAddress.Household_Account__c = hh.Id;

if (!canReadAddress()) {
throw new UTIL_Permissions.InsufficientPermissionException(System.Label.commonAccessErrorMessage);
}
Map<Address__c, Address__c> addressMatch = Addresses.getExistingAddresses(new List<Address__c>{
defaultAddress
});
if (addressMatch.containsKey(defaultAddress) && addressMatch.get(defaultAddress) != null) {
selectedAddressIsUndeliverable = addressMatch.get(defaultAddress)?.Undeliverable__c;
}

updateHousehold(hh);

// need to merge any households (Accounts only) before we save contacts
// so we avoid deleting a household if that contact was the last one in the hh.
if (isNotEmpty(listHHMerge)) {
mergeHouseholds((Account) hh, listHHMerge);
updateWinnerHouseholdSustainerAfterMerge((Account) hh);
}

List<Contact> contacts = new List<Contact>(listCon);
contacts.addAll(listConRemove);
upsertContacts(contacts);

if (isNotEmpty(listHHMerge)) {
cleanupAddresses(new List<Id>{
(Id) hh.get('Id')
});
}
} catch (Exception e) {
throw new AuraHandledException(e.getMessage());
}
}

updateHousehold(hh);

// need to merge any households (Accounts only) before we save contacts
// so we avoid deleting a household if that contact was the last one in the hh.
if (isNotEmpty(listHHMerge)) {
mergeHouseholds((Account)hh, listHHMerge);
updateWinnerHouseholdSustainerAfterMerge((Account)hh);
private static Boolean canReadAddress() {
if (Test.isRunningTest()) {
return true;
}

List<Contact> contacts = new List<Contact>(listCon);
contacts.addAll(listConRemove);
upsertContacts(contacts);

if (isNotEmpty(listHHMerge)) {
cleanupAddresses(new List<Id>{ (Id) hh.get('Id') });
}
Set<String> addressFields = new Set<String>{
'Default_Address__c',
'Household_Account__c',
'Address_Type__c',
'MailingStreet__c',
'MailingStreet2__c',
'MailingCity__c',
'MailingState__c',
'MailingPostalCode__c',
'MailingCountry__c',
'Seasonal_Start_Month__c',
'Seasonal_Start_Day__c',
'Seasonal_End_Month__c',
'Seasonal_End_Day__c',
'Geolocation__Latitude__s',
'Geolocation__Longitude__s',
'Undeliverable__c'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Undeliverable__c is relatively new, so I believe we should remove it from the access check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree

};

for (String addressField : addressFields) {
if (!UTIL_Permissions.canRead(UTIL_Namespace.StrAllNSPrefix('Address__c'),
UTIL_Namespace.StrAllNSPrefix(addressField), false)) {
return false;
}
}

return true;
}

private static void updateWinnerHouseholdSustainerAfterMerge(Account winnerHousehold) {
Expand Down
Loading
Loading