Add comprehensive email validation for blocked users#3
Conversation
… many times each email address is blocked, and last time it was blocked. Move email validation out of User model and into EmailValidator. Signup form remembers which email addresses have failed and shows validation error on email field.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis change introduces a comprehensive email blocking system for account creation. A new BlockedEmail model with validation logic tracks rejected emails, which are persisted and checked during account creation. The account creation flow now returns additional error details, and old email validation methods have been consolidated into a new EmailValidator class with whitelist/blacklist support. Changes
Sequence DiagramsequenceDiagram
actor User
participant Browser as Browser / Client
participant Server as Rails Server
participant DB as Database
User->>Browser: Submit account creation form
Browser->>Browser: Validate email against rejectedEmails
alt Email in rejected list
Browser->>User: Show validation error
else Email not rejected
Browser->>Server: POST /users (with email)
Server->>DB: Check BlockedEmail.should_block?(email)
DB->>DB: Find record, increment match_count
DB-->>Server: Return blocking decision
alt Should block
Server-->>Browser: Error response (email in values)
Browser->>Browser: Add email to rejectedEmails
Browser->>User: Show blocked email error
else Allow creation
Server->>DB: Create user record
DB-->>Server: Success
Server-->>Browser: Success response
Browser->>User: Account created
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @app/assets/javascripts/discourse/controllers/create_account_controller.js:
- Around line 274-276: The check that pushes to
createAccountController.get('rejectedEmails') can still push undefined because
it only verifies result.values exists; update the conditional to ensure
result.values.email is present and non-null/undefined before calling pushObject
(e.g. change the if to check result.values && result.values.email or use
optional chaining/result.values?.email), and only push result.values.email when
that value is truthy to avoid inserting undefined into rejectedEmails.
- Line 17: CreateAccountController's rejectedEmails array persists across modal
sessions; to fix, clear it when the modal opens or when the form is reset by
calling this.rejectedEmails.clear() (or reassigning Em.A([])) inside the
modal-open/reset handler rather than only at initialization. Add that clear call
to the controller method that runs when the modal is shown (e.g., the modal
open/show handler or the controller's resetForm/initializeForm routine) so
previously rejected addresses don't block subsequent submissions; also consider
clearing it just before running the email validation logic to ensure fresh
state.
In @app/models/blocked_email.rb:
- Around line 11-19: In should_block?(email) on BlockedEmail, perform a
case-insensitive lookup by downcasing the incoming email (e.g., use
Email.downcase(email) or email.downcase) when querying instead of where(email:
email), increment match_count and update last_match_at as before but replace
record.save with record.save! wrapped in a begin/rescue to catch
ActiveRecord::RecordInvalid (or StandardError) and log the failure so statistics
updates aren’t silently lost; keep the final return checking record &&
record.action_type == actions[:block].
In @lib/validators/email_validator.rb:
- Around line 13-15: Validation currently calls BlockedEmail.should_block?,
which has side effects (increments match_count and sets last_match_at); change
the validator (email_validator.rb) to call a new side-effect-free method like
BlockedEmail.blocked? (or similar) that performs only the check, and implement
that method in BlockedEmail model reusing the same matching logic but without
updating match_count/last_match_at; reserve should_block? (which updates stats)
to be called only during real signup attempts (e.g., in the signup/create flow)
so that repeated validations do not inflate statistics; update/add tests to
cover both the pure check method and the stats-updating method.
🧹 Nitpick comments (3)
spec/components/validators/email_validator_spec.rb (2)
3-23: Consider removing extra empty lines for consistency with RuboCop conventions.The test logic is correct and properly covers the blocked email validation scenarios. However, static analysis flagged extra empty lines at the block body beginning (after line 4) and end (before line 23).
♻️ Optional: Remove extra empty lines
describe EmailValidator do - let(:record) { Fabricate.build(:user, email: "bad@spamclub.com") } let(:validator) { described_class.new({attributes: :email}) } subject(:validate) { validator.validate_each(record,:email,record.email) } context "blocked email" do it "doesn't add an error when email doesn't match a blocked email" do BlockedEmail.stubs(:should_block?).with(record.email).returns(false) validate record.errors[:email].should_not be_present end it "adds an error when email matches a blocked email" do BlockedEmail.stubs(:should_block?).with(record.email).returns(true) validate record.errors[:email].should be_present end end - end
9-21: Consider adding test coverage for whitelist/blacklist interaction.The blocked email tests are well-structured, but the
EmailValidatoralso checks whitelist and blacklist settings before checkingBlockedEmail.should_block?. The validator only callsshould_block?when there are no prior errors. Consider adding tests to verify blocked email checks are skipped when whitelist/blacklist validation already failed.spec/models/blocked_email_spec.rb (1)
9-9: Consider updating to modern RSpec syntax.The tests use the deprecated
shouldsyntax (e.g.,should ==,should be_true,should be_false). While this works, modern RSpec prefersexpect(...).to:
should ==→expect(...).to eq(...)should be_true→expect(...).to be trueshould be_false→expect(...).to be falseshould be_nil→expect(...).to be_nilThis is a minor style consistency issue and can be addressed later if the codebase already uses the older syntax elsewhere.
Also applies to: 15-15, 23-23, 30-30, 37-37, 43-43
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/assets/javascripts/discourse/controllers/create_account_controller.jsapp/controllers/users_controller.rbapp/models/blocked_email.rbapp/models/user.rbconfig/locales/server.en.ymldb/migrate/20130724201552_create_blocked_emails.rblib/validators/email_validator.rbspec/components/validators/email_validator_spec.rbspec/fabricators/blocked_email_fabricator.rbspec/models/blocked_email_spec.rb
🧰 Additional context used
🧬 Code graph analysis (4)
spec/fabricators/blocked_email_fabricator.rb (1)
app/models/blocked_email.rb (1)
actions(7-9)
lib/validators/email_validator.rb (1)
app/models/blocked_email.rb (1)
should_block?(11-19)
spec/components/validators/email_validator_spec.rb (1)
lib/validators/email_validator.rb (2)
validate_each(1-24)validate_each(3-16)
spec/models/blocked_email_spec.rb (1)
app/models/blocked_email.rb (2)
actions(7-9)should_block?(11-19)
🪛 RuboCop (1.82.1)
spec/components/validators/email_validator_spec.rb
[convention] 4-5: Extra empty line detected at block body beginning.
(Layout/EmptyLinesAroundBlockBody)
[convention] 22-23: Extra empty line detected at block body end.
(Layout/EmptyLinesAroundBlockBody)
spec/models/blocked_email_spec.rb
[convention] 4-5: Extra empty line detected at block body beginning.
(Layout/EmptyLinesAroundBlockBody)
[convention] 47-48: Extra empty line detected at block body end.
(Layout/EmptyLinesAroundBlockBody)
🔇 Additional comments (10)
db/migrate/20130724201552_create_blocked_emails.rb (1)
1-12: LGTM!The migration correctly creates the
blocked_emailstable with appropriate columns and constraints. The unique index onconfig/locales/server.en.yml (1)
734-734: LGTM!The new translation key
blockedis correctly placed underen.user.emailand aligns with theI18n.t(:'user.email.blocked')call inEmailValidator.spec/fabricators/blocked_email_fabricator.rb (1)
1-4: LGTM!The fabricator correctly generates unique emails using a sequence and sets the default
action_typeto:blockviaBlockedEmail.actions[:block], consistent with the model's enum definition.app/controllers/users_controller.rb (1)
195-200: LGTM!The enhanced error response structure correctly provides:
errorsas a hash for field-specific error handlingvaluescontaining user-submitted input to enable client-side persistence of rejected emailsThis aligns with the client-side
rejectedEmailshandling mentioned in the PR objectives.app/assets/javascripts/discourse/controllers/create_account_controller.js (1)
68-74: Email rejection validation logic looks correct.The validation properly checks if the current email is in the
rejectedEmailsarray and returns an appropriate validation failure. The dependency onrejectedEmails.@eachensures the computed property re-evaluates when the array contents change.Also applies to: 96-96
lib/validators/email_validator.rb (2)
1-16: Overall validation structure looks good.The validator correctly handles:
- Whitelist mode (only allow specific domains)
- Blacklist mode (deny specific domains)
- Blocked email check (per-email blocking)
The precedence is also correct: whitelist takes priority over blacklist.
18-22: The domain parsing implementation is correct for the actual format used in the codebase. Tests confirm thatemail_domains_whitelistandemail_domains_blacklistsettings use only pipe character (|) as a delimiter for multiple domains, not commas, spaces, or newlines. The existinggsub('.', '\.')correctly escapes literal periods to prevent wildcard matching. No changes needed.Likely an incorrect or invalid review comment.
app/models/user.rb (1)
46-47: Good refactoring of email validation.Consolidating email validation into two clear lines improves maintainability:
- Line 46: Handles presence and uniqueness (database-level checks)
- Line 47: Delegates domain/blocking validation to the new
EmailValidatorclass, only when email changesThe
if: :email_changed?guard is appropriate to avoid unnecessary validation on updates that don't touch the email field.app/models/blocked_email.rb (1)
1-9: Model structure and defaults look good.The model correctly:
- Uses
before_validationto set defaults before validation runs- Validates email presence and uniqueness
- Provides a type-safe Enum for action types via
Enum.new(:block, :do_nothing)Also applies to: 21-23
spec/models/blocked_email_spec.rb (1)
26-33: Good use of shared examples for DRY test code.The shared example
"when a BlockedEmail record matches"effectively reduces duplication between the:blockand:do_nothingaction type contexts. The custom matcherbe_within_one_second_ofis properly defined and used correctly.
| accountPasswordConfirm: 0, | ||
| accountChallenge: 0, | ||
| formSubmitted: false, | ||
| rejectedEmails: Em.A([]), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "CreateAccountController" --type js -C 3Repository: AI-Code-Review-Evals/coderabbit-discourse
Length of output: 1444
🏁 Script executed:
# Also check the file itself to see the full context
wc -l app/assets/javascripts/discourse/controllers/create_account_controller.jsRepository: AI-Code-Review-Evals/coderabbit-discourse
Length of output: 162
🏁 Script executed:
# Check if this is a modal controller and how it's instantiated
rg -n "ModalFunctionality|new.*CreateAccount|CreateAccountController" --type js | head -30Repository: AI-Code-Review-Evals/coderabbit-discourse
Length of output: 4140
🏁 Script executed:
# Look for similar controller patterns with mutable defaults in the same directory
fd -e js . app/assets/javascripts/discourse/controllers/ | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && head -30 "{}"'Repository: AI-Code-Review-Evals/coderabbit-discourse
Length of output: 4152
🏁 Script executed:
cat -n app/assets/javascripts/discourse/controllers/create_account_controller.js | head -100Repository: AI-Code-Review-Evals/coderabbit-discourse
Length of output: 3734
🏁 Script executed:
# Check if there are any methods that clear or reset rejectedEmails
rg -n "rejectedEmails" app/assets/javascripts/discourse/controllers/create_account_controller.jsRepository: AI-Code-Review-Evals/coderabbit-discourse
Length of output: 322
🏁 Script executed:
# Check for init or reset methods in the controller
rg -n "init:|reset:|onOpen:|onShow:" app/assets/javascripts/discourse/controllers/create_account_controller.jsRepository: AI-Code-Review-Evals/coderabbit-discourse
Length of output: 67
🏁 Script executed:
# Look at ModalFunctionality to see if it handles cleanup
cat -n app/assets/javascripts/discourse/mixins/modal_functionality.jsRepository: AI-Code-Review-Evals/coderabbit-discourse
Length of output: 852
🏁 Script executed:
cat -n app/assets/javascripts/discourse/controllers/create_account_controller.js | tail -200Repository: AI-Code-Review-Evals/coderabbit-discourse
Length of output: 8447
🏁 Script executed:
# Specifically look for any clearing/reset logic around rejectedEmails
rg -n -B5 -A5 "rejectedEmails.*pushObject|pushObject.*rejectedEmails" app/assets/javascripts/discourse/controllers/create_account_controller.jsRepository: AI-Code-Review-Evals/coderabbit-discourse
Length of output: 657
Rejected emails array should be cleared when the modal is opened, not when initialized.
The rejectedEmails array is populated when email validation fails (line 275) but is never cleared. When the modal is closed and reopened, previously rejected emails remain in the array and continue to block form submission. This causes state pollution across modal sessions.
Since CreateAccountController is a singleton modal controller, the issue is not multiple instances sharing state, but rather persistent state across separate modal interactions. Clear the array when the modal opens or when form inputs are reset.
🤖 Prompt for AI Agents
In @app/assets/javascripts/discourse/controllers/create_account_controller.js at
line 17, CreateAccountController's rejectedEmails array persists across modal
sessions; to fix, clear it when the modal opens or when the form is reset by
calling this.rejectedEmails.clear() (or reassigning Em.A([])) inside the
modal-open/reset handler rather than only at initialization. Add that clear call
to the controller method that runs when the modal is shown (e.g., the modal
open/show handler or the controller's resetForm/initializeForm routine) so
previously rejected addresses don't block subsequent submissions; also consider
clearing it just before running the email validation logic to ensure fresh
state.
| if (result.errors && result.errors.email && result.values) { | ||
| createAccountController.get('rejectedEmails').pushObject(result.values.email); | ||
| } |
There was a problem hiding this comment.
Verify null safety when accessing nested properties.
The condition checks result.errors && result.errors.email && result.values, but then accesses result.values.email. If result.values exists but result.values.email is undefined/null, pushObject would add an undefined value to the array.
Suggested fix
- if (result.errors && result.errors.email && result.values) {
+ if (result.errors && result.errors.email && result.values && result.values.email) {
createAccountController.get('rejectedEmails').pushObject(result.values.email);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (result.errors && result.errors.email && result.values) { | |
| createAccountController.get('rejectedEmails').pushObject(result.values.email); | |
| } | |
| if (result.errors && result.errors.email && result.values && result.values.email) { | |
| createAccountController.get('rejectedEmails').pushObject(result.values.email); | |
| } |
🤖 Prompt for AI Agents
In @app/assets/javascripts/discourse/controllers/create_account_controller.js
around lines 274 - 276, The check that pushes to
createAccountController.get('rejectedEmails') can still push undefined because
it only verifies result.values exists; update the conditional to ensure
result.values.email is present and non-null/undefined before calling pushObject
(e.g. change the if to check result.values && result.values.email or use
optional chaining/result.values?.email), and only push result.values.email when
that value is truthy to avoid inserting undefined into rejectedEmails.
| def self.should_block?(email) | ||
| record = BlockedEmail.where(email: email).first | ||
| if record | ||
| record.match_count += 1 | ||
| record.last_match_at = Time.zone.now | ||
| record.save | ||
| end | ||
| record && record.action_type == actions[:block] | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "blocked_email.rb" -type fRepository: AI-Code-Review-Evals/coderabbit-discourse
Length of output: 114
🏁 Script executed:
cat -n ./app/models/blocked_email.rbRepository: AI-Code-Review-Evals/coderabbit-discourse
Length of output: 802
🏁 Script executed:
# Check for email normalization patterns in the codebase
rg -n "downcase.*email|email.*downcase|email.*lower|lower.*email" --type ruby -C 2Repository: AI-Code-Review-Evals/coderabbit-discourse
Length of output: 7078
🏁 Script executed:
# Check if BlockedEmail is created/updated elsewhere with email normalization
rg -n "BlockedEmail" --type ruby -B 3 -A 3Repository: AI-Code-Review-Evals/coderabbit-discourse
Length of output: 6419
🏁 Script executed:
# Check how the email validator is used and if emails are normalized before validation
rg -n "email_validator|EmailValidator" --type ruby -B 5 -A 5Repository: AI-Code-Review-Evals/coderabbit-discourse
Length of output: 1909
🏁 Script executed:
# Check how User model handles email normalization during creation/validation
sed -n '1,300p' ./app/models/user.rb | cat -nRepository: AI-Code-Review-Evals/coderabbit-discourse
Length of output: 10703
🏁 Script executed:
# Check if User model normalizes email on assignment or before_save
rg -n "email.*=" ./app/models/user.rb | head -20Repository: AI-Code-Review-Evals/coderabbit-discourse
Length of output: 487
🏁 Script executed:
# Check for before_save and email normalization in User model
rg -n "before_save|def email" ./app/models/user.rb -A 5Repository: AI-Code-Review-Evals/coderabbit-discourse
Length of output: 774
🏁 Script executed:
# Verify the Email.downcase utility and confirm it handles case-insensitive matching properly
cat -n lib/email.rbRepository: AI-Code-Review-Evals/coderabbit-discourse
Length of output: 942
🏁 Script executed:
# Check if there's any test that validates case-insensitive email matching for BlockedEmail
cat -n spec/models/blocked_email_spec.rbRepository: AI-Code-Review-Evals/coderabbit-discourse
Length of output: 1984
Address case-insensitive email matching and silent save failure in should_block?
Two issues in the should_block? method:
-
Case-sensitive email lookup (Line 12): The query
where(email: email)is case-sensitive. A blocked email stored as "bad@spam.com" won't match lookups for "Bad@Spam.com", allowing users to bypass the block by varying case. The codebase establishes a pattern of usingEmail.downcase()for case-insensitive email handling; apply it here. -
Silent save failure (Line 16):
record.savefails silently if validation errors occur. Statistics updates (match_count, last_match_at) are lost without indication. Userecord.save!and handle exceptions to ensure failures are logged.
Suggested improvements
def self.should_block?(email)
- record = BlockedEmail.where(email: email).first
+ record = BlockedEmail.where("lower(email) = ?", email.to_s.downcase).first
if record
record.match_count += 1
record.last_match_at = Time.zone.now
- record.save
+ record.save!
end
record && record.action_type == actions[:block]
+rescue ActiveRecord::RecordInvalid => e
+ Rails.logger.error("Failed to update BlockedEmail stats: #{e.message}")
+ record && record.action_type == actions[:block]
end🤖 Prompt for AI Agents
In @app/models/blocked_email.rb around lines 11 - 19, In should_block?(email) on
BlockedEmail, perform a case-insensitive lookup by downcasing the incoming email
(e.g., use Email.downcase(email) or email.downcase) when querying instead of
where(email: email), increment match_count and update last_match_at as before
but replace record.save with record.save! wrapped in a begin/rescue to catch
ActiveRecord::RecordInvalid (or StandardError) and log the failure so statistics
updates aren’t silently lost; keep the final return checking record &&
record.action_type == actions[:block].
| if record.errors[attribute].blank? and BlockedEmail.should_block?(value) | ||
| record.errors.add(attribute, I18n.t(:'user.email.blocked')) | ||
| end |
There was a problem hiding this comment.
BlockedEmail.should_block? has a side effect during validation.
Calling should_block? increments match_count and updates last_match_at (as seen in app/models/blocked_email.rb lines 14-16). This means statistics are updated every time validation runs, not just on actual signup attempts. If a user corrects other form fields and resubmits, the blocked email counter would be inflated.
Consider whether this is the intended behavior. If not, you may want a separate method that only checks without updating statistics, reserving the stat update for actual signup attempts.
🤖 Prompt for AI Agents
In @lib/validators/email_validator.rb around lines 13 - 15, Validation currently
calls BlockedEmail.should_block?, which has side effects (increments match_count
and sets last_match_at); change the validator (email_validator.rb) to call a new
side-effect-free method like BlockedEmail.blocked? (or similar) that performs
only the check, and implement that method in BlockedEmail model reusing the same
matching logic but without updating match_count/last_match_at; reserve
should_block? (which updates stats) to be called only during real signup
attempts (e.g., in the signup/create flow) so that repeated validations do not
inflate statistics; update/add tests to cover both the pure check method and the
stats-updating method.
2 participants
Test 3
Summary by CodeRabbit
Release Notes
New Features
Database
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/discourse-coderabbit#3