Skip to content

Fix spam protection in the reset password screen#3215

Merged
jlledom merged 6 commits into
3scale:masterfrom
jlledom:THREESCALE-7431-recaptcha-forgot-page
Feb 21, 2023
Merged

Fix spam protection in the reset password screen#3215
jlledom merged 6 commits into
3scale:masterfrom
jlledom:THREESCALE-7431-recaptcha-forgot-page

Conversation

@jlledom
Copy link
Copy Markdown
Contributor

@jlledom jlledom commented Feb 16, 2023

What this PR does / why we need it:

THREESCALE-7431 was not only about fixing the spam protection in the signup screen: the reset password screen had a broken recaptcha as well. The same fix should be good for both screens but the controller for the reset password form wasn't taking the spam check params correctly, so the checks always failed and the activity was always being marked as suspicious. Fixed in https://github.com/3scale/porta/pull/3215/files#diff-811e0b68328f92343b613fa4b352676067bc7e9f71fe951f627de512d2a21b62R49-R51

I also added some cukes to the reset password screen to test this behavior.

Which issue(s) this PR fixes

THREESCALE-7431

Verification steps

  1. Fill in the reset password form in a non-suspicious way: Javascript enabled, correct email, and take a few seconds to fill it in.
  2. Click "Send instructions"
  3. The spam check should pass, so you shouldn't see the captcha.

@jlledom jlledom requested a review from a team February 16, 2023 13:32
@jlledom jlledom self-assigned this Feb 16, 2023
Copy link
Copy Markdown
Contributor

@thalesmiguel thalesmiguel left a comment

Choose a reason for hiding this comment

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

Just a question about the test.

Comment thread features/step_definitions/capcha_steps.rb
When the buyer wants to reset their password
Then 15 seconds pass
Then the buyer doesn't need to pass the captcha after reset password form is filled wrong
But the buyer will need to pass the captcha after reset password form is filled in too quickly No newline at end of file
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

OCD-related-comment: We're missing an empty line by the end of this file.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You are right, and I am sorry.

params.require(:user).permit(:password, :password_confirmation)
end

def spam_protection_params
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

To me the name is a bit confusing, as it's fetching all the :account params, whether they are spam protection-related or not. At this point all of them are spam protection-related indeed.

I think I'd prefer to just call it account_params, and add a comment that fetching these params is for the spam protection mechanism. But well, maybe it's just me 🙃

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK!


def buyer
@buyer ||= @provider.buyers.build
@buyer ||= @provider.buyers.build.tap do |account|
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is .tap needed here? I think it would work just with .build do |account|

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Indeed we can remove tap as build already returns newly built object.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed!

@jlledom jlledom changed the title Fix spam foprtection in the reset password screen Fix spam protection in the reset password screen Feb 20, 2023
Copy link
Copy Markdown
Contributor

@thalesmiguel thalesmiguel left a comment

Choose a reason for hiding this comment

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

🚀

@jlledom jlledom merged commit 7bdf1bd into 3scale:master Feb 21, 2023
@jlledom jlledom deleted the THREESCALE-7431-recaptcha-forgot-page branch March 13, 2023 08:19
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.

3 participants