THREESCALE-765: Brute force protection for the admin login screen #3841

jlledom · 2024-07-01T10:12:36Z

What this PR does / why we need it:

Add a Recaptcha to the admin portal login screen.

This PR includes a new screen under Account Settings -> Integrate -> Bot Protection to enable/disable the Recaptcha.

Which issue(s) this PR fixes

https://issues.redhat.com/browse/THREESCALE-765

Verification steps

Go to Account Settings -> Integrate -> Bot Protection
Select reCAPTCHA
Go to the provider login screen
You should see the text

This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.

The login screen should work as usual: return an error when the credentials are invalid and log in when the credentials are valid
Set RECAPTCHA_MIN_BOT_SCORE to 0.99 and restart the server
You should always be detected as a bot, no matter whether the credentials are valid or not.

Special notes for your reviewer:

Some references I took to write the React component:

Please, @josemigallas @lvillen review the UI part, it's probably not aligned with design guidelines or not well integrated with the rest of the UI code base.

jlledom · 2024-07-02T10:32:51Z

spec/javascripts/utilities/ReCaptchaV3.spec.tsx

+
+describe('ReCaptchaV3', () => {
+  it('should render itself correctly', () => {
+    const wrapper = mount(<ReCaptchaV3 enabled action="test/action" siteKey="fakeKey" />)


This component loads a JS script from google. @josemigallas do you know whether this test would actually download the script every time it runs?

If you use a real browser in test (@javascript in cucumbers), then it will try. But we try to kill internet in these tests so it should fail in circleci.

Surprisingly I did exactly that and the cuke doesn't fail: https://github.com/3scale/porta/blob/1cbf07e991bec976bb8b9a753c5cda740410be85/features/provider/login.feature

We probably mock kit somewhere, somehow. For javascript tests you can create a mock like like the ones for Braintree and Stripe: https://github.com/3scale/porta/blob/a25da8553ddd1b1f20f85a8a8e3988c299596f8f/spec/javascripts/__mocks__/braintree-web.js

josemigallas · 2024-07-03T14:41:33Z

app/javascript/src/utilities/ReCaptchaV3.tsx

+}
+
+const ReCaptchaV3 = ({ enabled = false, siteKey, action }: Props): ReactElement => {
+  if (!enabled) return (<div>{null}</div>)


Suggested change

if (!enabled) return (<div>{null}</div>)

if (!enabled) {

return <div>{null}</div>

}

Also, why <div>{null}</div> and not null?

Because the linter fails with:

TS2322: Type 'null' is not assignable to type 'ReactElement<any, string | JSXElementConstructor<any>>'.

josemigallas · 2024-07-03T14:42:22Z

app/javascript/src/utilities/ReCaptchaV3.tsx

+  action: string;
+}
+
+const ReCaptchaV3 = ({ enabled = false, siteKey, action }: Props): ReactElement => {


Suggested change

const ReCaptchaV3 = ({ enabled = false, siteKey, action }: Props): ReactElement => {

const ReCaptchaV3: FunctionComponent<Props> = ({ enabled = false, siteKey, action }) => {

josemigallas · 2024-07-03T14:42:55Z

app/javascript/src/utilities/ReCaptchaV3.tsx

+
+import { useScript } from 'utilities/useScript'
+
+import type { ReactElement } from 'react'


Suggested change

import type { ReactElement } from 'react'

import type { FunctionComponent } from 'react'

josemigallas · 2024-07-03T14:44:40Z

app/javascript/src/utilities/ReCaptchaV3.tsx

+  const [token, setToken] = useState('')
+
+  useScript(`https://www.google.com/recaptcha/api.js?render=${siteKey}`, () => {
+    const grecaptcha = window.grecaptcha


Suggested change

const grecaptcha = window.grecaptcha

const { grecaptcha } = window

josemigallas · 2024-07-03T14:50:40Z

spec/javascripts/utilities/ReCaptchaV3.spec.tsx

+
+const ReCaptchaV3 = jest.requireActual('utilities/ReCaptchaV3').ReCaptchaV3 as FunctionComponent<Props>
+
+describe('ReCaptchaV3', () => {


IMHO there's no need to "describe" the element you're testing. The file is already "ReCaptchaV3". I would use describe to wrap different scenarios, for instance:

describe("when recaptcha is disabled", () => { it('should do that...', () => {}) }) describe("when recaptcha is enabled", () => { it('should do this...', () => {}) })

josemigallas · 2024-07-03T14:53:55Z

features/step_definitions/provider_steps.rb

+Given "{provider} tries to log in" do |provider|
+  username = provider.admins.first.username
+  set_current_domain provider.external_admin_domain
+  visit provider_login_path
+  fill_in 'Email or Username', with:  username
+  fill_in 'Password', with: "supersecret"
+  click_button 'Sign in'
+end


If I'm not missing anything, this step is the same as

porta/features/step_definitions/session_steps.rb

Lines 3 to 6 in 3898a60

Given "{provider} logs in" do |provider|

set_current_domain(provider.external_admin_domain)

try_provider_login(provider.admins.first.username, 'supersecret')

end

josemigallas · 2024-07-03T14:54:42Z

features/step_definitions/provider_steps.rb

+When "{provider} wants to log in" do |provider|
+  set_current_domain provider.external_admin_domain
+  visit provider_login_path
+end


Instead of this use:

When they go to the provider login page

josemigallas · 2024-07-03T14:56:23Z

features/step_definitions/provider_steps.rb

+Then "the provider login attempt fails" do
+  assert_current_path "/p/sessions"
+  assert find_field('Email or Username')
+  assert find_field('Password')
+  assert find_button('Sign in', disabled: true)
+end


I would simply use:

And the current page is the provider login page

And also assert the error message (if any)

Then they should see "Login failed"

Also, there are steps for those assertions:

And there is a field "Email or Username" And there is a field "Password" And there should be a button to "Sign in" # or And I should see button "Sign in" disabled

But I honestly think it doesn't have to be that meticulous.

features/provider/login.feature

josemigallas · 2024-07-03T15:11:58Z

app/javascript/src/utilities/ReCaptchaV3.tsx

+  const inputName = `g-recaptcha-response-data[${action}]`
+  const [token, setToken] = useState('')
+
+  useScript(`https://www.google.com/recaptcha/api.js?render=${siteKey}`, () => {


Why do you need to useEffect this? Is siteKey ever going to change dynamically? Maybe I am mistaken but to me it looks like siteKey is passed once to the component, at first render. If so, there's no need to use an effect, simply render the script.

I wonder if it would make sense to do it in the template (new.html.slim) and use a defer:

<script id="recaptcha_script" src="https://www.google.com/recaptcha/api.js?render=#{siteKey}" defer />

defer will load the script but wait for the HTML to be parsed before executing the script, but before DOMContentLoaded is triggered.

I did it because...

I think I'm going to keep the useScript component but remove the call to useEffect. I think that's better than just rendering <script /> because this way I can run he callback after the script has been loaded. I'm not sure it would work the same way with defer

josemigallas · 2024-07-03T15:13:28Z

app/javascript/src/Login/components/LoginForm.tsx

@@ -114,6 +121,7 @@ const LoginForm: FunctionComponent<Props> = ({
          onChange={handleOnChange('password')}
        />
      </FormGroup>
+      <ReCaptchaV3 action={recaptcha.action} enabled={recaptcha.enabled} siteKey={recaptcha.siteKey} />


I don't think it is correct to pass enabled. Either render ReCaptchaV3 or don't:

Suggested change

<ReCaptchaV3 action={recaptcha.action} enabled={recaptcha.enabled} siteKey={recaptcha.siteKey} />

{recaptcha.enabled && <ReCaptchaV3 action={recaptcha.action} siteKey={recaptcha.siteKey} />}

This is a React patttern called "conditional rendering".

mayorova · 2024-07-05T09:07:35Z

Hm, strange, it doesn't work for me as described, i.e. I don't see any Google message on the login screen, and I don't see recaptcha errors with min score 0.99 (after restarting the server).

Also, I think it's confusing that the setting in Audience > Developer Portal > Settings > Bot Protection controls the admin portal login 🤔

I think we probably need another setting for the admin portal (and its own UI for this setting). The user may decide to use recaptcha for the developer portal, and not for admin portal, and vice versa... I know that's more work, but I think it would be better.

UPDATE: I missed the part "on the master portal" 😬 When switching the setting there, it actually works. The place of the setting is still very confusing though, because we don't have a developer portal as such in master 🫠

jlledom · 2024-07-05T12:30:59Z

@josemigallas I addressed all your comments on the React components. But I couldn't do the same on cucumbers. Some of your suggestions broke some other cukes, and I failed to find a way to fix it. After two days I give up.

jlledom · 2024-07-16T07:32:12Z

Based on this comment on the issue, I created a new screen under Account Settings -> Integrate -> Bot Protection to enable the login protection for the current provider.

mayorova · 2024-07-17T10:07:19Z

db/migrate/20240715075900_add_admin_bot_protection_to_settings.rb

+    add_column :settings, :admin_bot_protection_level, :string
+    change_column_default :settings, :admin_bot_protection_level, "none"


Suggested change

add_column :settings, :admin_bot_protection_level, :string

change_column_default :settings, :admin_bot_protection_level, "none"

add_column :settings, :admin_bot_protection_level, :string, default: "none"

Not sure if there was a specific reason to separate the two operations? If not, I think they can be merged into one.

Yes, I did that at the beginning, but I received a warning from strong_migrations because it's an unsafe operation, only for Oracle.

=== Dangerous operation detected #strong_migrations === Adding a column with a non-null default causes the entire table to be rewritten. Instead, add the column without a default value, then change the default. class AddAdminBotProtectionToSettings < ActiveRecord::Migration[6.1] def up add_column :settings, :admin_bot_protection_level, :string change_column_default :settings, :admin_bot_protection_level, "none" end def down remove_column :settings, :admin_bot_protection_level end end Then backfill the existing rows in the Rails console or a separate migration with disable_ddl_transaction!. class BackfillAddAdminBotProtectionToSettings < ActiveRecord::Migration[6.1] disable_ddl_transaction! def up Setting.unscoped.in_batches do |relation| relation.update_all admin_bot_protection_level: "none" sleep(0.01) end end end Then add the NOT NULL constraint in separate migrations. /home/jlledom/redhat/porta/db/migrate/20240711145208_add_admin_bot_protection_to_settings.rb:3:in `up'

So I updated the migration to:

Split into two operations

Don't backfill

Don't make the column NOT NULL

Adapt the code to interpret NULL as :none

It's much simpler, less risky and works for all DB engines.

OK, thanks for the explanation @jlledom

mayorova · 2024-07-17T10:35:01Z

The feature works well, although I think that the config screen could be maybe improved a bit. It is currently not clear, what it protects, and it's even more confusing given that there is another screen in the admin portal which is identical.

The location of the dev portal one (Developer Portal > Settings > Bot Protection) kind of suggests that it's bot protection for the developer portal, but for this new screen it's not clear, and I think it can be easily confused.
Maybe it's worth adding some text that says that this bot protection is for the admin portal specifically? Not sure if it's worth mentioning that it's for login page, but probably not, as we might add more pages in future.

jlledom · 2024-07-22T11:57:24Z

@mayorova I added some descriptions in 6454b02

I took the text from this comment in the ticket:
https://issues.redhat.com/browse/THREESCALE-765?focusedId=25146833&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-25146833

josemigallas · 2024-07-23T10:36:55Z

app/views/provider/admin/bot_protections/edit.html.slim

This and sites/spam_protections/edit look the same, can we use a partial?

Done in 2cc9c9b

mayorova · 2024-07-31T08:00:25Z

app/views/provider/admin/bot_protections/_form.html.slim

+            input_html: { disabled: !Recaptcha.captcha_configured? }
+
+    = form.actions do
+      = form.commit_button 'Update Settings'


no new line?

Unacceptable

mayorova · 2024-07-31T08:01:55Z

app/views/provider/admin/bot_protections/_form.html.slim

@@ -0,0 +1,11 @@
+= semantic_form_for settings, url: url, html: {:id => 'bot-protection-settings' } do |form|
+  = form.inputs 'Protection against bots' do


With this refactor we don't keep the "admin portal" and "developer portal" label in this title. Or it was considered redundant?

Actually, we probably can move this and the commit button labels to I18n, and reuse the complete view, not only the form.

With this refactor we don't keep the "admin portal" and "developer portal" label in this title. Or it was considered redundant?

I don't remember 🤕

Actually, we probably can move this and the commit button labels to I18n, and reuse the complete view, not only the form.

I don't like the idea of reuse the complete view, because then we should set some parameters like url or field from the controller.

OK, but we can still use I18n for Protection against bots and set it to different values for admin and dev portal.

Here you go: 1f9ef72

app/views/provider/admin/bot_protections/_form.html.slim

mayorova · 2024-08-01T19:22:50Z

@jlledom I've updated this PR with more suggestions 😬
https://github.com/jlledom/porta/pull/2/files

I just find it easier to follow and update.

But it's OK to keep as it is, the only real suggestion is to update the description.

And also looks like this branch needs some conflict resolution.

jlledom · 2024-08-05T11:32:35Z

@mayorova I applied your suggestions and resolved the conflict

config/locales/en.yml

Based on: - https://w11i.me/recaptcha-v3-react - https://stackoverflow.com/questions/53832882/react-and-recaptcha-v3 - https://developers.google.com/recaptcha/docs/v3#programmatically_invoke_the_challenge

It showed errors twice in the admin login screen.

🥒

jlledom requested a review from a team July 1, 2024 10:14

jlledom marked this pull request as ready for review July 1, 2024 10:15

jlledom commented Jul 2, 2024

View reviewed changes

jlledom self-assigned this Jul 3, 2024

josemigallas reviewed Jul 3, 2024

View reviewed changes

features/provider/login.feature Outdated Show resolved Hide resolved

josemigallas reviewed Jul 3, 2024

View reviewed changes

jlledom force-pushed the THREESCALE-765-brute-force-admin-login branch from f94eff1 to debebc6 Compare July 5, 2024 11:28

jlledom requested a review from josemigallas July 5, 2024 12:31

mayorova reviewed Jul 17, 2024

View reviewed changes

lvillen mentioned this pull request Jul 19, 2024

🥒 Fix "should see the following details" errors #3849

Merged

jlledom requested a review from mayorova July 22, 2024 11:57

josemigallas reviewed Jul 23, 2024

View reviewed changes

jlledom requested a review from josemigallas July 23, 2024 15:34

mayorova reviewed Jul 31, 2024

View reviewed changes

jlledom requested a review from mayorova August 1, 2024 15:44

mayorova reviewed Aug 1, 2024

View reviewed changes

app/views/provider/admin/bot_protections/_form.html.slim Outdated Show resolved Hide resolved

jlledom force-pushed the THREESCALE-765-brute-force-admin-login branch from 50a917f to ad41e23 Compare August 5, 2024 11:31

jlledom requested a review from mayorova August 5, 2024 11:32

mayorova reviewed Aug 5, 2024

View reviewed changes

config/locales/en.yml Outdated Show resolved Hide resolved

mayorova approved these changes Aug 5, 2024

View reviewed changes

jlledom added 9 commits August 5, 2024 14:17

Migration, new setting: :admin_bot_protection_level

4720e47

New React component to load external scripts

a3d9470

New React component for Google Recaptcha V3

b1f0669

Based on: - https://w11i.me/recaptcha-v3-react - https://stackoverflow.com/questions/53832882/react-and-recaptcha-v3 - https://developers.google.com/recaptcha/docs/v3#programmatically_invoke_the_challenge

New screen to set :admin_bot_protection_level

76507a6

Add a Recaptcha to the Admin portal login screen

08a7752

Move form to its own partial

77d54f9

Fix recaptcha flash errors

f350d2f

It showed errors twice in the admin login screen.

Add/Update/Fix tests

2c2a50b

Add/Update/Fix cucumbers

e9d88d9

🥒

jlledom force-pushed the THREESCALE-765-brute-force-admin-login branch from 8f45824 to e9d88d9 Compare August 5, 2024 12:26

jlledom merged commit b17a18a into 3scale:master Aug 5, 2024
16 of 21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

THREESCALE-765: Brute force protection for the admin login screen #3841

THREESCALE-765: Brute force protection for the admin login screen #3841

jlledom commented Jul 1, 2024 •

edited

Loading

jlledom Jul 2, 2024

akostadinov Jul 2, 2024

jlledom Jul 2, 2024

josemigallas Jul 3, 2024

josemigallas Jul 3, 2024

jlledom Jul 3, 2024

josemigallas Jul 3, 2024

josemigallas Jul 3, 2024

josemigallas Jul 3, 2024

josemigallas Jul 3, 2024 •

edited

Loading

josemigallas Jul 3, 2024

josemigallas Jul 3, 2024

josemigallas Jul 3, 2024 •

edited

Loading

josemigallas Jul 3, 2024

jlledom Jul 4, 2024

josemigallas Jul 3, 2024

mayorova commented Jul 5, 2024 •

edited

Loading

jlledom commented Jul 5, 2024

jlledom commented Jul 16, 2024

mayorova Jul 17, 2024

jlledom Jul 17, 2024 •

edited

Loading

mayorova Jul 17, 2024

mayorova commented Jul 17, 2024

jlledom commented Jul 22, 2024

josemigallas Jul 23, 2024

jlledom Jul 23, 2024

mayorova Jul 31, 2024

jlledom Jul 31, 2024

mayorova Jul 31, 2024

jlledom Jul 31, 2024 •

edited

Loading

mayorova Aug 1, 2024

jlledom Aug 1, 2024

mayorova commented Aug 1, 2024

jlledom commented Aug 5, 2024

-  if (!enabled) return (<div>{null}</div>)
+  if (!enabled) {
+    return <div>{null}</div>
+  }

	const ReCaptchaV3 = ({ enabled = false, siteKey, action }: Props): ReactElement => {
	const ReCaptchaV3: FunctionComponent<Props> = ({ enabled = false, siteKey, action }) => {


		import { useScript } from 'utilities/useScript'

		import type { ReactElement } from 'react'

	import type { ReactElement } from 'react'
	import type { FunctionComponent } from 'react'

	const grecaptcha = window.grecaptcha
	const { grecaptcha } = window


		const ReCaptchaV3 = jest.requireActual('utilities/ReCaptchaV3').ReCaptchaV3 as FunctionComponent<Props>

		describe('ReCaptchaV3', () => {

	Given "{provider} logs in" do \|provider\|
	set_current_domain(provider.external_admin_domain)
	try_provider_login(provider.admins.first.username, 'supersecret')
	end

	<ReCaptchaV3 action={recaptcha.action} enabled={recaptcha.enabled} siteKey={recaptcha.siteKey} />
	{recaptcha.enabled && <ReCaptchaV3 action={recaptcha.action} siteKey={recaptcha.siteKey} />}

		add_column :settings, :admin_bot_protection_level, :string
		change_column_default :settings, :admin_bot_protection_level, "none"

		@@ -0,0 +1,11 @@
		= semantic_form_for settings, url: url, html: {:id => 'bot-protection-settings' } do \|form\|
		= form.inputs 'Protection against bots' do

THREESCALE-765: Brute force protection for the admin login screen #3841

THREESCALE-765: Brute force protection for the admin login screen #3841

Conversation

jlledom commented Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josemigallas Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josemigallas Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayorova commented Jul 5, 2024 • edited Loading

jlledom commented Jul 5, 2024

jlledom commented Jul 16, 2024

Choose a reason for hiding this comment

jlledom Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayorova commented Jul 17, 2024

jlledom commented Jul 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlledom Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayorova commented Aug 1, 2024

jlledom commented Aug 5, 2024

jlledom commented Jul 1, 2024 •

edited

Loading

josemigallas Jul 3, 2024 •

edited

Loading

josemigallas Jul 3, 2024 •

edited

Loading

mayorova commented Jul 5, 2024 •

edited

Loading

jlledom Jul 17, 2024 •

edited

Loading

jlledom Jul 31, 2024 •

edited

Loading