SalesforceLabs / survey-force

85 stars 61 forks source link

SFDCAccessController is unnecessary for standard controllers #106

Closed dschach closed 3 years ago

dschach commented 3 years ago

Now that the code uses standard controllers for Visualforce, access control is automatically enforced, so we can remove the ESAPI SFDCAccessController code. Does the community endorse this?

jrattanpal commented 3 years ago

@dschach Which class are you referring to?

I need to check Security Review guidelines for this and get confirmation from that team.

jrattanpal commented 3 years ago

There are still classes (like https://github.com/SalesforceLabs/survey-force/blob/master/force-app/main/default/classes/GettingStartedController.cls) where we use this to check CRUD/FLS permissions.

I believe TakeSurvey class is same

dschach commented 3 years ago

I'm working on a pull request so that all Visualforce controllers will use a standard controller. Plus, all have "with sharing." And all queries are done with security enforced. The ESAPI class is doing nothing to enhance security in this package, now that the platform has these newer features.

jrattanpal commented 3 years ago

That's great!

What about DML? We can use that security enforced with SOQL

dschach commented 3 years ago

If the controller is a standard controller, then DML is run in user context. I'm submitting a large pull request that cleans up a lot of unnecessary code and makes everything run a bit faster. It still uses ESAPI. Once that's pulled, we can look at where ESAPI is unnecessary.

jrattanpal commented 3 years ago

Sounds good

jrattanpal commented 3 years ago

Thank you for working on this! That's a huge list of changes.

I will promote it now and then test it. Once all is good, I will update the package version on AppEx listing.

Thanks again for your amazing help with this!

jrattanpal commented 3 years ago

@dschach

When I submit the survey, I get an error. I believe the issue is in https://github.com/SalesforceLabs/survey-force/blob/master/force-app/main/default/classes/ViewSurveyController.cls

We need to add "Thank_You_Linkc, Thank_You_Textc" to Standard controller fields. If you have the code open and ready, can you add it?

https://sandbox-inspiration-customizati418-d-178d1c224ed.cs94.force.com/sf/TakeSurvey?id=a030R0000080zvBQAQ&cId=none&caId=none&

FATAL_ERROR System.SObjectException: SObject row was retrieved via SOQL without querying the requested field: Surveyc.Thank_You_Linkc

apex-07L0R00000DHB4JUAX.log

jrattanpal commented 3 years ago

In my test scratch org, I added the following above line#43. That worked

stdController.addFields(new List{'Thank_You_Linkc', 'Thank_You_Textc'});

jrattanpal commented 3 years ago

IN addition to error above,

You have changed notification for THank you page. It now disappears. It leaves an empty screen. I was wondering why until I tested again and realized that it was disappearing.

Not a big deal but wanted to share.

Other than that, all looks good

dschach commented 3 years ago

Ah, thanks. I hadn't thought of that. I will create a new way to show the thank-you text instead. Perhaps a nice SLDS modal that redirects to the URL when it's closed.

jrattanpal commented 3 years ago

Great, thanks!

And I'll keep an eye out for your PR to fix that first issue. Once all tested, I will publish new version of AppEx app

jrattanpal commented 3 years ago

@dschach

Let me know when you can submit the fix. OR do you want me to go ahead and submit the fix? I want to make sure master branch is fixed and ready to go

dschach commented 3 years ago

I'm on it. I got distracted trying to add help tooltips to the LexInputComponent. Will submit functional code shortly.

jrattanpal commented 3 years ago

@dschach

Thanks for the changes.

  1. I had to make a small change to object metadata fil: https://github.com/SalesforceLabs/survey-force/commit/513fc96bcc26e6df858885b505a5801a68cc9740. (SFDX push also failed here)

  2. I know you fixed thanks page but now I don't see any message. TEST: https://sandbox-platform-flow-1996-dev-ed-178d6be3fce.cs2.force.com/sf/TakeSurvey?id=a03R000000HDBd1IAH&cId=none&caId=none

You may know which new changes are causing this

dschach commented 3 years ago

Odd. The modal was working properly. Thanks for updating the object xml - not sure why it didn't go with the pull request. I'll get back on the modal now.

jrattanpal commented 3 years ago

Thanks!

I am checking and it's returning thankYouRendered as false. It should be true. Apex is all working

jrattanpal commented 3 years ago

I believe

has to be an apex tag like

In addition, the model settings may need to be changed as it shows all grey (it works after change to pageBlock from div)

jrattanpal commented 3 years ago

It all seems to be working. I will publish latest version on AppExchange.

jrattanpal commented 3 years ago

@dschach

I may have to remove New Contact and New case action from Survey Taken Record Page. otherwise I get following error when trying to install the package.

ERROR: Encountered errors installing the package!,Installation errors: 1) Duplicate Name, Details: The name "NewContact" is already used on component type: Action. Please rename existing component. 2) Duplicate Name, Details: The name "NewCase" is already used on component type: Action. Please rename existing component.

dschach commented 3 years ago

Unexpected. I will override them and will submit that update with a new pull request that sets the User field properly via the public site. Give me a few minutes...

jrattanpal commented 3 years ago

@dschach

  1. Can you check that flexipage in your org? I can push it to scratch org because of following error and others as I change it.

Error force-app/main/default/flexipages/Survey_Taken_Record_Page.flexipage-meta.xml The 'sidebar' region (type Region) doesn't exist.

It's easier to change it if you have it in your org to make sure all is working.

  1. I pushed a fix for object metadata file. Please sync that to your repo before submitting any changes.

https://github.com/SalesforceLabs/survey-force/commit/e241516f512c2716c08a6b1ff652b0174064df1a

jrattanpal commented 3 years ago

@dschach

I think I found the issue. It seems that it wasn't using proper template in the metadata (not sure why). I fixed it for now (and testing it)

https://github.com/SalesforceLabs/survey-force/commit/52eef2526b0031819bfe7cde4965bf401d4a4d34

jrattanpal commented 3 years ago

It all looks good. I was able to generate new package version, install and test it.

Thanks again for all the changes!