Add comprehensive email validation for blocked users #23
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.
Code Review by Qodo (Alpha)Analysis CompletedPublished 4 inline comment(s) |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| 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.
[action_required] [Race Condition in Counter Updates]
• The should_block? method performs a read-modify-write on match_count without atomic operations or locking
• Multiple concurrent requests checking the same email can cause lost updates (both read count=5, both write count=6)
• Statistics tracking will be inaccurate under concurrent load, which defeats the purpose of tracking blocked email attempts
Agent Prompt
Copy this prompt and use it to remediate the issue with your preferred AI generation tools
## Issue Description
The `should_block?` method has a race condition when updating `match_count`. The read-modify-write pattern causes lost updates under concurrent access.
## Issue Context
The method reads a record, increments a counter in memory, then saves. Two concurrent requests can both read the same value and both write the same incremented value.
## Fix Focus Areas
Replace the in-memory increment with an atomic SQL update:
- app/models/blocked_email.rb[11-19]
| 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.
[action_required] [Database Side Effects During Validation]
• The BlockedEmail.should_block? method modifies the database during the validation phase by incrementing counters
• Rails validations can run multiple times (via valid?, AJAX validation, form submission) before an actual save occurs
• This causes inflated match_count statistics that don't reflect actual blocked registration attempts
Agent Prompt
Copy this prompt and use it to remediate the issue with your preferred AI generation tools
## Issue Description
The `should_block?` method modifies the database during validation, causing inflated statistics when validation runs multiple times.
## Issue Context
Validation can run via `valid?`, AJAX calls, and form submission. Each call increments the counter even if the registration is never actually attempted.
## Fix Focus Areas
Separate the check-if-blocked logic from the track-attempt logic:
- app/models/blocked_email.rb[11-19]
- lib/validators/email_validator.rb[13-15]
| def validate_each(record, attribute, value) | ||
| if (setting = SiteSetting.email_domains_whitelist).present? | ||
| unless email_in_restriction_setting?(setting, value) | ||
| record.errors.add(attribute, I18n.t(:'user.email.not_allowed')) | ||
| end | ||
| elsif (setting = SiteSetting.email_domains_blacklist).present? | ||
| if email_in_restriction_setting?(setting, value) | ||
| record.errors.add(attribute, I18n.t(:'user.email.not_allowed')) | ||
| end | ||
| end | ||
| 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.
[remediation_recommended] [Missed Statistics When Domain Blacklist Rejects]
• The BlockedEmail.should_block? check only runs if no prior email validation errors exist
• Emails rejected by domain whitelist/blacklist will never have their blocked email statistics updated
• This creates incomplete tracking data for emails that are in both the domain blacklist and the blocked_emails table
Agent Prompt
Copy this prompt and use it to remediate the issue with your preferred AI generation tools
## Issue Description
Emails rejected by domain blacklist/whitelist never have their BlockedEmail statistics updated because should_block? is only called when there are no prior errors.
## Issue Context
The conditional on line 13 prevents should_block? from running if domain checks already rejected the email.
## Fix Focus Areas
Separate statistics tracking from blocking decision:
- lib/validators/email_validator.rb[13-15]
| record.errors.add(attribute, I18n.t(:'user.email.not_allowed')) | ||
| end | ||
| end | ||
| if record.errors[attribute].blank? and BlockedEmail.should_block?(value) |
There was a problem hiding this comment.
[informational] [Inconsistent Boolean Operator Usage]
• The code uses Ruby's and keyword instead of && in a conditional expression
• While functionally correct in this case, and has lower precedence than && and can cause unexpected behavior in complex expressions
• Violates common Ruby style guidelines recommending && for boolean expressions
Agent Prompt
Copy this prompt and use it to remediate the issue with your preferred AI generation tools
## Issue Description
The code uses 'and' instead of '&&' in a boolean conditional.
## Issue Context
Ruby's 'and' has lower precedence than '&&' and is typically reserved for control flow, not conditionals.
## Fix Focus Areas
Replace 'and' with '&&':
- lib/validators/email_validator.rb[13]
Benchmark PR from qodo-benchmark/discourse-qodo#3