-
Notifications
You must be signed in to change notification settings - Fork 1
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
Apply reCaptcha to Public Forms #477
base: master
Are you sure you want to change the base?
Conversation
…oundation/Volunteers-for-Salesforce into feature/sw-authflow__captcha
**lurch: attach W-037947 |
Tracking W-037947 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added comments from our discussion earlier today.
src/classes/VOL_SharedCode.cls
Outdated
* @return Boolean value for captcha check | ||
********************************************************************************************************/ | ||
public static Boolean verifyCaptcha() { | ||
// If captcha is disabled, pass the check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if Captcha is enabled but they did not supply the settings necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note from earlier conversation, should we move this into its own class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently this shows the authorization VF screen and would be considered a misconfiguration. We can adjust to make it act disabled when fields are missing though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Callout method will stay in this class. We can move the captcha specific method and a new enabled check to a separate class.
src/classes/VOL_SharedCode_TEST.cls
Outdated
Test.setCurrentPage(lookupPage); | ||
System.currentPageReference().getParameters().put('g-recaptcha-response', '1234567890'); | ||
|
||
HttpMockFactory mock = new HttpMockFactory(200, 'OK', '{"success":true}', new Map<String,String>()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to worry about handling other http statuses?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now if the verification returns any status / body value without the success parameter, the verification is treated as a failure and displayed as such. I will add an additional test that mocks an unexpected response to cover an additional use case.
…oundation/Volunteers-for-Salesforce into feature/sw-authflow__captcha
@@ -104,6 +148,8 @@ private with sharing class VOL_CTRL_PersonalSiteContactLookup_TEST { | |||
} | |||
|
|||
private static void TestPersonalSiteContactLookup() { | |||
//disable captcha | |||
UTIL_UnitTest.setupCaptchaSettings(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is disabled by default, why do we need to disable it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Safeguards the test against unexpected changes elsewhere by being explicit here.
Critical Changes
Changes
Added reCaptcha option to public forms.
Issues Closed
New Metadata
Captcha.component
HttpMockFactory.cls
Deleted Metadata