Skip to content

Add registration rate limit#1659

Merged
Oaphi merged 11 commits intodevelopfrom
art/signup-rate-limiting
Jul 1, 2025
Merged

Add registration rate limit#1659
Oaphi merged 11 commits intodevelopfrom
art/signup-rate-limiting

Conversation

@ArtOfCode-
Copy link
Member

@ArtOfCode- ArtOfCode- commented Jun 24, 2025

Adds a configurable rate limit for how quickly accounts can be created from the same IP. Related to the ongoing email issues.

@ArtOfCode- ArtOfCode- requested a review from a team June 24, 2025 21:01
@codecov
Copy link

codecov bot commented Jun 24, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.36%. Comparing base (51cbe3e) to head (64846ef).
Report is 12 commits behind head on develop.

Additional details and impacted files
Components Coverage Δ
controllers 61.81% <100.00%> (+0.15%) ⬆️
helpers 69.17% <ø> (ø)
jobs 28.00% <ø> (ø)
models 81.92% <ø> (-0.02%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Oaphi
Oaphi requested changes Jul 1, 2025
View reviewed changes
Copy link
Member

@Oaphi Oaphi left a comment

Choose a reason for hiding this comment

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

You cannot call create unless the parent is saved

Unfortunately, send_welcome_tour_message causes this error as it was supposed to be an after_create callback. The same problem is likely to apply to the ensure_websites callback

@ArtOfCode-
Copy link
Member Author

ArtOfCode- commented Jul 1, 2025

Where are you seeing that? I haven't hit that error anywhere

@Oaphi
Copy link
Member

Oaphi commented Jul 1, 2025 •
edited
Loading

Where are you seeing that? I haven't hit that error anywhere

Any attempt to manually (via the UI) sign up a user with the PR checked out - do you not get this error, @ArtOfCode-?

@ArtOfCode-
Copy link
Member Author

ArtOfCode- commented Jul 1, 2025

Nope, no error in manual testing or the automated tests

@Oaphi
Copy link
Member

Oaphi commented Jul 1, 2025 •
edited
Loading

Nope, no error in manual testing or the automated tests

Hmm... Tests pass for me, but not the manual registration - not sure what's going on.

Nevermind, I think I know what's going on: there were validation errors that prevented the user from being saved. save in Devise::RegistrationsController fails, but as its return value is not handled, the user is passed back in the "dirty" state. Let's check whether the user has any errors before even trying to check the limit?

@Oaphi Oaphi self-requested a review July 1, 2025 15:12
Oaphi
Oaphi approved these changes Jul 1, 2025
View reviewed changes
Copy link
Member

@Oaphi Oaphi left a comment

Choose a reason for hiding this comment

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

LGTM

@Oaphi Oaphi merged commit 4ed85f4 into develop Jul 1, 2025
10 checks passed
@Oaphi Oaphi deleted the art/signup-rate-limiting branch July 1, 2025 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Oaphi Oaphi Oaphi approved these changes

Assignees

No one assigned

Labels

priority: high

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ArtOfCode- @Oaphi @cellio