Automated Test: blocked-email-validation-post #310
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 pull request refactors email validation logic by introducing a new Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (Browser)
participant Controller as CreateAccountController
participant UsersController as UsersController
participant UserModel as User Model
participant EmailValidator as EmailValidator
participant BlockedEmail as BlockedEmail
User->>Controller: Submit account form (email)
Controller->>Controller: Check accountEmail against rejectedEmails
alt Email in rejectedEmails
Controller-->>User: Validation error (skip server)
else Email not rejected
Controller->>UsersController: POST /users/create
UsersController->>UserModel: Create user
UserModel->>EmailValidator: Validate email attribute
EmailValidator->>EmailValidator: Check domain whitelist/blacklist
EmailValidator->>BlockedEmail: should_block?(email)
BlockedEmail->>BlockedEmail: Lookup email record, increment match_count
alt Email is blocked
BlockedEmail-->>EmailValidator: true
EmailValidator-->>UserModel: Add validation error
else Email allowed
BlockedEmail-->>EmailValidator: false
EmailValidator-->>UserModel: Pass validation
end
alt Validation fails
UserModel-->>UsersController: Validation errors
UsersController-->>User: JSON response (errors, values)
User->>Controller: Receive failed email in response
Controller->>Controller: Add email to rejectedEmails
else Validation passes
UserModel-->>UsersController: User created
UsersController-->>User: Success response
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The changes span multiple cohorts with mixed complexity: the new 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: 3
🤖 Fix all issues with AI agents
In `@app/assets/javascripts/discourse/controllers/create_account_controller.js`:
- Around line 274-276: The code pushes result.values.email into rejectedEmails
without ensuring it exists; modify the block in create_account_controller (the
if handling result.errors && result.errors.email) to first check that
result.values.email is defined/non-falsy before calling
createAccountController.get('rejectedEmails').pushObject(...). Use a guard like
checking result.values && result.values.email (or optional chaining) and only
push when a valid email string is present to avoid inserting undefined into
rejectedEmails.
In `@app/models/blocked_email.rb`:
- Around line 11-19: The lookup in BlockedEmail.should_block? uses
BlockedEmail.where(email: email) which is case-sensitive; change the lookup to
use a downcased email (e.g., normalize the incoming email with
email.to_s.downcase before querying) and update the record logic in
should_block? to use the normalized value; also add a before_validation callback
(e.g., before_validation :normalize_email with a normalize_email method) or
ensure set_defaults downcases self.email on create/update so stored emails are
always lowercased.
In `@lib/validators/email_validator.rb`:
- Around line 18-22: The regex in email_in_restriction_setting?(setting, value)
currently uses Regexp.new("@(#{domains})", true) which can match domains in the
middle of the string; update the pattern to anchor to the absolute end (use \z)
so it only matches the domain at the end of the email (e.g.
Regexp.new("@(#{domains})\\z", true)), keeping the existing domains escaping
logic and case-insensitive flag.
🧹 Nitpick comments (7)
config/locales/server.en.yml (1)
733-734: Consider adding guidance to theblockedmessage.The
not_allowedmessage suggests "Please use another email address," but the newblockedmessage simply says "is not allowed." For UX consistency, consider adding similar guidance so users know what to do next.app/assets/javascripts/discourse/controllers/create_account_controller.js (1)
17-17: Shared mutable default on the class prototype.In classic Ember, object/array defaults on
extend()are shared across all instances. Since Discourse controllers are singletons this is safe in practice, but the idiomatic pattern is to initialize mutable properties ininitto prevent subtle bugs if the controller is ever instantiated more than once.app/models/blocked_email.rb (1)
11-19:should_block?has a side effect (updating statistics) despite its predicate name.This method both queries and mutates state. If this is intentional for analytics, consider documenting it or extracting the tracking into a separate method to avoid surprising callers.
lib/validators/email_validator.rb (1)
13-13: Prefer&&overandfor boolean logic.Ruby's
andhas lower precedence than assignment and can cause subtle bugs in more complex expressions.&&is the idiomatic choice for conditionals.Proposed fix
- if record.errors[attribute].blank? and BlockedEmail.should_block?(value) + if record.errors[attribute].blank? && BlockedEmail.should_block?(value)spec/components/validators/email_validator_spec.rb (3)
3-7: Minor formatting: extra blank lines at block boundaries.RuboCop flags extra empty lines at the
describeblock body beginning (after Line 3) and end (before Line 23).♻️ Suggested fix
describe EmailValidator do - let(:record) { Fabricate.build(:user, email: "bad@spamclub.com") }and at the end:
end end - end
13-13: Old RSpecshouldsyntax used.This uses the deprecated
should/should_notexpectation syntax. However, this project predominantly uses the old syntax throughout the test suite (2467 instances ofshouldvs 294 ofexpect), so refactoring this file in isolation may create inconsistency. Consider this a low-priority modernization if the entire test suite is eventually migrated to modern RSpec conventions.
9-21: Add test coverage for domain whitelist/blacklist validation.The tests cover the
BlockedEmail.should_block?path. However, theEmailValidatorimplementation also validatesemail_domains_whitelistandemail_domains_blacklistsite settings (viaemail_in_restriction_setting?), but these paths have no test coverage. Add contexts for both settings to ensure the validator correctly enforces domain restrictions.
| if (result.errors && result.errors.email && result.values) { | ||
| createAccountController.get('rejectedEmails').pushObject(result.values.email); | ||
| } |
There was a problem hiding this comment.
Guard against result.values.email being undefined.
If result.values is present but result.values.email is falsy/undefined, pushObject(undefined) would pollute the rejectedEmails array.
Proposed fix
- 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 code pushes result.values.email into rejectedEmails
without ensuring it exists; modify the block in create_account_controller (the
if handling result.errors && result.errors.email) to first check that
result.values.email is defined/non-falsy before calling
createAccountController.get('rejectedEmails').pushObject(...). Use a guard like
checking result.values && result.values.email (or optional chaining) and only
push when a valid email string is present 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.
Case-sensitive email lookup can be bypassed with mixed-case input.
where(email: email) performs a case-sensitive match on most PostgreSQL configurations. A user could bypass blocking by registering with a differently-cased variant (e.g., Block@SpamFromHome.org vs. the stored block@spamfromhome.org). Downcase the email before lookup.
Proposed fix
def self.should_block?(email)
- record = BlockedEmail.where(email: email).first
+ record = BlockedEmail.where(email: email.downcase).first
if record
record.match_count += 1
record.last_match_at = Time.zone.now
record.save
end
record && record.action_type == actions[:block]
endAlso ensure emails are downcased on creation (in set_defaults or a separate callback):
before_validation :normalize_email
def normalize_email
self.email = email.downcase if email.present?
end🤖 Prompt for AI Agents
In `@app/models/blocked_email.rb` around lines 11 - 19, The lookup in
BlockedEmail.should_block? uses BlockedEmail.where(email: email) which is
case-sensitive; change the lookup to use a downcased email (e.g., normalize the
incoming email with email.to_s.downcase before querying) and update the record
logic in should_block? to use the normalized value; also add a before_validation
callback (e.g., before_validation :normalize_email with a normalize_email
method) or ensure set_defaults downcases self.email on create/update so stored
emails are always lowercased.
| 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 lacks end-of-string anchor — domain matching can produce false positives/negatives.
The pattern @(#{domains}) matches the domain anywhere after @, so user@blacklisted.com.legit.com would match blacklisted.com, and conversely user@whitelisted.com.evil.com would pass the whitelist. Anchor the regex to the end of the string.
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📝 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.
| def email_in_restriction_setting?(setting, value) | |
| domains = setting.gsub('.', '\.') | |
| regexp = Regexp.new("@(#{domains})", true) | |
| value =~ regexp | |
| end | |
| def email_in_restriction_setting?(setting, value) | |
| domains = setting.gsub('.', '\.') | |
| regexp = Regexp.new("@(#{domains})\\z", true) | |
| value =~ regexp | |
| end |
🤖 Prompt for AI Agents
In `@lib/validators/email_validator.rb` around lines 18 - 22, The regex in
email_in_restriction_setting?(setting, value) currently uses
Regexp.new("@(#{domains})", true) which can match domains in the middle of the
string; update the pattern to anchor to the absolute end (use \z) so it only
matches the domain at the end of the email (e.g. Regexp.new("@(#{domains})\\z",
true)), keeping the existing domains escaping logic and case-insensitive flag.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit