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

Conversation

npsp-reedestockton
Copy link
Contributor

Critical Changes

Changes

Issues Closed

Community Ideas Delivered

Features Intended for Future Release

Features for Elevate Customers

New Metadata

Deleted Metadata

@npsp-reedestockton
Copy link
Contributor Author

@andrewyu-salesforce @callen-sfdo Here is the combined security bug fix PR.

@npsp-reedestockton
Copy link
Contributor Author

private static Boolean hasFieldReadAccess(Set<String> queryFields) {
String nameSpacedField;

for (String queryField:queryFields) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

CommitmentId__c is one of the fields being checked for access. I think we should probably remove that field from the access check. What do you think??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need access to it, but I think it falls into the category of something that is system-maintained, not something that needs to be accessed by a user.

Copy link
Contributor

Choose a reason for hiding this comment

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

CommitmentId definitely falls into the System-maintained side and should not be checked for access.

Copy link
Contributor

Choose a reason for hiding this comment

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

On top of CommitmentId, I think we should also remove the following fields from FLS check because those aren't related to Pause functionality and it's probably used for querying/updating the schedules. In the UI we only display Installment Date, Amount, and Payment Method.

  • npe03__Recurring_Donation_Campaign__r.Name
  • npe03__Installments__c
  • npe03__Total_Paid_Installments__c
  • npe03__Contact__c
  • npe03__Organization__c

What do you think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds right to me!

'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

Copy link
Contributor

@callen-sfdo callen-sfdo left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@@ -49,6 +49,7 @@ public with sharing class PSC_ManageSoftCredits_CTRL {
@TestVisible private String currencySymbol;

/** @description Set to true if the user has the appropriate permissions to access the page */
@TestVisible
Copy link
Contributor

Choose a reason for hiding this comment

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

The TestVisible is not needed or the variable should be private

private static Boolean hasFieldReadAccess(Set<String> queryFields) {
String nameSpacedField;

for (String queryField:queryFields) {
Copy link
Contributor

Choose a reason for hiding this comment

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

CommitmentId definitely falls into the System-maintained side and should not be checked for access.

private static Boolean hasFieldReadAccess(Set<String> queryFields) {
String nameSpacedField;

for (String queryField:queryFields) {
Copy link
Contributor

Choose a reason for hiding this comment

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

On top of CommitmentId, I think we should also remove the following fields from FLS check because those aren't related to Pause functionality and it's probably used for querying/updating the schedules. In the UI we only display Installment Date, Amount, and Payment Method.

  • npe03__Recurring_Donation_Campaign__r.Name
  • npe03__Installments__c
  • npe03__Total_Paid_Installments__c
  • npe03__Contact__c
  • npe03__Organization__c

What do you think

@npsp-reedestockton
Copy link
Contributor Author

@andrewyu-salesforce PR feedback has been handled.

…ity-bug-fixes__security-bug-fixes-2

Feature/248  security bug fixes  security bug fixes 2
@npsp-reedestockton npsp-reedestockton merged commit bd0b2e9 into feature/248 Nov 16, 2023
7 of 11 checks passed
@npsp-reedestockton npsp-reedestockton deleted the feature/248__security-bug-fixes branch November 16, 2023 04:10
@salesforce-org-metaci salesforce-org-metaci bot mentioned this pull request Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants