Automated Test: blocked-email-validation-post #326
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.
📝 WalkthroughWalkthroughThis PR introduces an email blocking system for user account creation. It adds a BlockedEmail model with tracking, creates an EmailValidator for domain restrictions and blocked email enforcement, updates the user controller to return rejection details, and modifies the signup controller to track and prevent rejected email re-submission. Changes
Sequence DiagramsequenceDiagram
actor User
participant Client as Create Account<br/>Controller
participant Server as Users<br/>Controller
participant UserModel as User Model<br/>& Validators
participant BlockedEmail as BlockedEmail<br/>Model
User->>Client: Submit account form
Client->>Server: POST /users (email in payload)
Server->>UserModel: user.save
UserModel->>UserModel: Validate email presence,<br/>uniqueness, format
UserModel->>BlockedEmail: BlockedEmail.should_block?<br/>(email)
BlockedEmail->>BlockedEmail: Check if email<br/>matches blocked record
alt Email Blocked
BlockedEmail-->>UserModel: true
UserModel->>UserModel: Validation fails
UserModel-->>Server: Errors with email
end
alt Email Not Blocked
BlockedEmail-->>UserModel: false
UserModel-->>Server: Validation success
Server-->>Client: Account created
Client->>User: Success
end
alt Validation Failed
Server->>Server: Construct error response<br/>(errors, values)
Server-->>Client: HTTP 422 with errors<br/>& rejected email
Client->>Client: Add email to<br/>rejectedEmails array
Client->>User: Show error,<br/>block email field
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@app/models/blocked_email.rb`:
- Around line 7-9: Add a require_dependency 'enum' at the top of
blocked_email.rb so the Enum constant is loaded before self.actions calls
Enum.new(:block, :do_nothing); specifically, open blocked_email.rb and insert
require_dependency 'enum' near other requires (matching how post.rb,
notification.rb, post_action_type.rb do it) to ensure the Enum class is
available for the actions method.
- Around line 11-19: should_block? performs a case-sensitive lookup and
non-atomic read-modify-save on match_count; normalize the incoming email (strip
and downcase) before querying so BlockedEmail.where(email:
normalized_email).first finds stored entries, and replace the read-modify-save
with an atomic DB update that increments match_count and sets last_match_at in
one statement (e.g. an update_all or increment counter with a where clause on
the record id) to avoid lost updates; also add/ensure a before_validation (or
set_defaults) callback to normalize stored emails (self.email =
email.strip.downcase if present) so saved records are consistently lowercased.
In `@lib/validators/email_validator.rb`:
- Around line 13-15: The validator is calling BlockedEmail.should_block? which
performs DB writes (incrementing match_count and updating last_match_at) during
validate_each, causing duplicate increments when user.valid? is called before
save; change this by extracting a pure predicate (e.g.,
BlockedEmail.blocked?(email) or BlockedEmail.match_for(email, persist: false))
that only reads DB and does not update counters, and have the validator call
that pure method; move the increment/update side-effect into a single place that
runs once after a successful create (for example call BlockedEmail.record_match!
from UsersController#create after user.save, or implement an after_create
callback) so match_count/last_match_at are updated exactly once per actual
creation.
- Around line 18-22: The current email_in_restriction_setting? method builds a
domain pattern but its regexp is unanchored so emails like
user@mailinator.com.evil.com match; update the regexp to anchor the domain to
the end of the string (use end-of-string anchor such as \z) so the pattern
matches only the exact domain after the @ (keep the case-insensitive flag),
e.g., modify the Regexp.new call that currently uses "@(#{domains})" to include
the end anchor.
🧹 Nitpick comments (5)
config/locales/server.en.yml (1)
733-734: Theblockedmessage is very similar to the existingnot_allowedmessage.
not_allowed: "is not allowed from that email provider. Please use another email address."
blocked: "is not allowed."Consider making the
blockedmessage more distinct so users (and support staff reviewing logs) can differentiate between a domain-restriction rejection and a specific blocked-email rejection.spec/components/validators/email_validator_spec.rb (2)
7-7: Missing spaces after commas in method arguments.Line 7 has
validate_each(record,:email,record.email)— add spaces after the commas for readability.- subject(:validate) { validator.validate_each(record,:email,record.email) } + subject(:validate) { validator.validate_each(record, :email, record.email) }
3-23: Remove extra blank lines at block body boundaries (RuboCop).RuboCop flags the extra blank line after
describe EmailValidator do(Line 4) and before the closingend(Line 22). Remove them to satisfyLayout/EmptyLinesAroundBlockBody.♻️ Proposed fix
describe EmailValidator do - let(:record) { Fabricate.build(:user, email: "bad@spamclub.com") }end - endapp/assets/javascripts/discourse/controllers/create_account_controller.js (2)
17-17: Mutable array default on the class prototype.
Em.A([])defined at the class body level is evaluated once and shared across all instances. In Ember Classic, the safe pattern is to initialize mutable defaults ininit. Since Discourse controllers are singletons this is unlikely to bite in practice, but it's still a latent risk if the lifecycle ever changes.♻️ Safer initialization
Add an
initmethod to set up the array per-instance:init: function() { this._super(); this.set('rejectedEmails', Em.A([])); },And remove the class-level
rejectedEmails: Em.A([]),line.
69-74: Consider using a more specific error message for blocked emails.This uses
I18n.t('user.email.invalid')for emails that were previously rejected by the server. Since ablockedtranslation key was added on the server side, a corresponding client-side key (e.g.,'user.email.blocked') would give users a more accurate signal than "invalid" for an email that is syntactically valid but policy-rejected.
| def self.actions | ||
| @actions ||= Enum.new(:block, :do_nothing) | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the Enum class definition
rg -n 'class Enum' --type=rubyRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 119
🏁 Script executed:
#!/bin/bash
# Check if blocked_email.rb has any explicit requires
echo "=== Checking blocked_email.rb for requires ==="
cat -n app/models/blocked_email.rb | head -20
echo -e "\n=== Examining the Enum class implementation ==="
cat -n lib/enum.rb
echo -e "\n=== Checking if lib is in Rails autoload paths ==="
rg -n 'config\.autoload' --type=ruby config/Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 2477
🏁 Script executed:
#!/bin/bash
# Search for any requires of 'enum' in the codebase
echo "=== Searching for require statements for Enum ==="
rg -n "require.*enum" --type=ruby -i
echo -e "\n=== Checking for initializers that might load lib/enum ==="
ls -la config/initializers/ 2>/dev/null | head -20
rg -n "require.*lib" --type=ruby config/initializers/
echo -e "\n=== Checking if lib directory is in autoload_paths ==="
cat config/application.rb | grep -A 10 -B 5 "autoload_paths"Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 2629
Add require_dependency 'enum' at the top of blocked_email.rb.
The Enum class is defined in lib/enum.rb, but blocked_email.rb lacks the required require_dependency 'enum' statement. Other files using Enum in the codebase (e.g., post.rb, notification.rb, post_action_type.rb) include this require statement. Without it, calling Enum.new(:block, :do_nothing) will fail at runtime with a NameError.
🤖 Prompt for AI Agents
In `@app/models/blocked_email.rb` around lines 7 - 9, Add a require_dependency
'enum' at the top of blocked_email.rb so the Enum constant is loaded before
self.actions calls Enum.new(:block, :do_nothing); specifically, open
blocked_email.rb and insert require_dependency 'enum' near other requires
(matching how post.rb, notification.rb, post_action_type.rb do it) to ensure the
Enum class is available for the actions method.
| 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:
# First, let's explore the repository structure
git ls-files | head -20Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 348
🏁 Script executed:
# Check the BlockedEmail model and related models
fd -t f -e rb 'blocked_email|user' app/models/ | head -20Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 615
🏁 Script executed:
# Search for BlockedEmail references and email normalization patterns
rg -n 'BlockedEmail|should_block' --type=rb -g '!spec/**' | head -30Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 114
🏁 Script executed:
# Check the User model for email handling/normalization
fd -t f 'user.rb' app/models/ -x cat -n {}Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 39687
🏁 Script executed:
# Look for email normalization in the codebase
rg -n 'downcase|normalize.*email|email.*downcase' --type=rb -g '!spec/**' | head -30Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 114
🏁 Script executed:
cat -n app/models/blocked_email.rbRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 805
🏁 Script executed:
# Search for how BlockedEmail is used in the codebase
rg "BlockedEmail" --type-list | head -5Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 477
🏁 Script executed:
# Try searching for BlockedEmail references without type specification
rg "BlockedEmail\|should_block" --max-count=20Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 70
🏁 Script executed:
# Search for Email class and downcase method
rg "class Email|def downcase" --max-count=20Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 685
🏁 Script executed:
# Find the Email class definition
find . -name 'email.rb' -type f | grep -v node_modules | head -10Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 102
🏁 Script executed:
# Search for where BlockedEmail.should_block? is called
rg "should_block" --max-count=20Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 658
🏁 Script executed:
# Search for email normalization in validators
rg "downcase|strip|normalize" lib/validators/email_validator.rbRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 70
🏁 Script executed:
# Check if there's any email normalization in database callbacks
rg "before_save|before_validation.*email" --max-count=20Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 459
🏁 Script executed:
cat -n lib/email.rbRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 945
🏁 Script executed:
cat -n lib/validators/email_validator.rbRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 1095
🏁 Script executed:
cat -n spec/models/blocked_email_spec.rb | head -80Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 1987
🏁 Script executed:
# Check if email is normalized before being passed to BlockedEmail.should_block?
rg -B5 -A5 "BlockedEmail.should_block" lib/validators/email_validator.rbRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 509
🏁 Script executed:
# Verify that Email.downcase is not called in the EmailValidator path
rg -B10 -A10 "Email.downcase" lib/validators/email_validator.rbRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 70
🏁 Script executed:
# Check User model to confirm email is not normalized before validation
rg -B5 -A5 "validates.*email" app/models/user.rb | head -30Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 418
🏁 Script executed:
# Look at how User creation/validation handles email
rg -B5 -A5 "new_from_params|create_for_email" app/models/user.rbRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 650
should_block? has no email normalization — case-sensitive lookup may miss blocked entries.
If an admin blocks spam@example.com but a user registers with Spam@Example.com, the where(email: email) lookup will miss the record (case-sensitive comparison in SQL). Although Email.downcase() is available in the codebase, it is not used in the validation flow. The email should be normalized before lookup to ensure consistent blocking.
Additionally, the read-modify-save pattern on match_count is susceptible to lost updates under concurrent requests. Using an atomic SQL update would be more robust for statistics tracking.
Proposed fix for normalization and atomic update
def self.should_block?(email)
- record = BlockedEmail.where(email: email).first
+ record = BlockedEmail.where(email: email.strip.downcase).first
if record
- record.match_count += 1
- record.last_match_at = Time.zone.now
- record.save
+ BlockedEmail.where(id: record.id).update_all(
+ ['match_count = match_count + 1, last_match_at = ?', Time.zone.now]
+ )
end
record && record.action_type == actions[:block]
endYou may also want to normalize the email in set_defaults or via a before_validation callback so stored emails are always lowercase:
before_validation { self.email = email.strip.downcase if email.present? }🤖 Prompt for AI Agents
In `@app/models/blocked_email.rb` around lines 11 - 19, should_block? performs a
case-sensitive lookup and non-atomic read-modify-save on match_count; normalize
the incoming email (strip and downcase) before querying so
BlockedEmail.where(email: normalized_email).first finds stored entries, and
replace the read-modify-save with an atomic DB update that increments
match_count and sets last_match_at in one statement (e.g. an update_all or
increment counter with a where clause on the record id) to avoid lost updates;
also add/ensure a before_validation (or set_defaults) callback to normalize
stored emails (self.email = email.strip.downcase if present) so saved records
are consistently lowercased.
| 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 DB write side-effect inside validation.
Every call to validate_each for a blocked email will increment match_count and update last_match_at. If valid? is called explicitly before save (which internally calls valid? again), the counter gets incremented twice. This is a minor accuracy concern for the statistics, but worth being aware of given the current UsersController#create flow calls user.valid? on Line 168 before user.save on Line 172.
🤖 Prompt for AI Agents
In `@lib/validators/email_validator.rb` around lines 13 - 15, The validator is
calling BlockedEmail.should_block? which performs DB writes (incrementing
match_count and updating last_match_at) during validate_each, causing duplicate
increments when user.valid? is called before save; change this by extracting a
pure predicate (e.g., BlockedEmail.blocked?(email) or
BlockedEmail.match_for(email, persist: false)) that only reads DB and does not
update counters, and have the validator call that pure method; move the
increment/update side-effect into a single place that runs once after a
successful create (for example call BlockedEmail.record_match! from
UsersController#create after user.save, or implement an after_create callback)
so match_count/last_match_at are updated exactly once per actual creation.
| def email_in_restriction_setting?(setting, value) | ||
| domains = setting.gsub('.', '\.') | ||
| regexp = Regexp.new("@(#{domains})", true) | ||
| value =~ regexp | ||
| end |
There was a problem hiding this comment.
Regex is not anchored — domain restrictions can be bypassed.
The regex @(mailinator\.com|trashmail\.net) matches anywhere after the @, so an email like user@mailinator.com.evil.com would pass the blacklist check (it matches the substring) and also incorrectly pass the whitelist check. Anchor the regex to the end of the string to ensure it matches the full domain.
Proposed fix
def email_in_restriction_setting?(setting, value)
domains = setting.gsub('.', '\.')
- regexp = Regexp.new("@(#{domains})", true)
+ regexp = Regexp.new("@(#{domains})\\z", true)
value =~ regexp
end🤖 Prompt for AI Agents
In `@lib/validators/email_validator.rb` around lines 18 - 22, The current
email_in_restriction_setting? method builds a domain pattern but its regexp is
unanchored so emails like user@mailinator.com.evil.com match; update the regexp
to anchor the domain to the end of the string (use end-of-string anchor such as
\z) so the pattern matches only the exact domain after the @ (keep the
case-insensitive flag), e.g., modify the Regexp.new call that currently uses
"@(#{domains})" to include the end anchor.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
New Features
Tests