Add comprehensive email validation for blocked users

[hussam789]

User description

PR #3

---

PR Type

Enhancement, Tests

---

Description

• Implement comprehensive email blocking system with BlockedEmail model

• Extract email validation logic into reusable EmailValidator class

• Track blocked email statistics (match count, last match time)

• Return validation errors and form values to frontend for UX improvements

• Add client-side rejected emails tracking to prevent resubmission

---

Diagram Walkthrough

flowchart LR
  A["User Signup"] --> B["EmailValidator"]
  B --> C["BlockedEmail Check"]
  C --> D{Email Blocked?}
  D -->|Yes| E["Return Errors & Values"]
  D -->|No| F["Create User"]
  E --> G["Frontend Tracks Rejected Emails"]
  G --> H["Show Validation Error"]

Loading

File Walkthrough

	Relevant files
Enhancement	5 files blocked_email.rb New model for managing blocked emails                                        +25/-0    email_validator.rb Extracted email validation logic into validator                    +24/-0    user.rb Refactored email validation to use EmailValidator                +2/-21    users_controller.rb Return validation errors and values in response                    +3/-1      create_account_controller.js Track rejected emails and prevent resubmission                      +13/-1
Configuration changes	1 files 20130724201552_create_blocked_emails.rb Database migration for blocked emails table                            +12/-0
Tests	3 files blocked_email_spec.rb Comprehensive tests for BlockedEmail model                              +48/-0    email_validator_spec.rb Tests for EmailValidator blocked email logic                          +23/-0    blocked_email_fabricator.rb Test fabricator for BlockedEmail records                                  +4/-0
Documentation	1 files server.en.yml Add blocked email error message translation                            +1/-0

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Missing audit logs: The new blocked email checks and statistic updates occur without any logging of the blocking decision or updates, making it unclear who/when actions occurred for audit purposes. Referred Code 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
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Edge cases unhandled: The validator assumes a non-nil email value and does not guard against nil/blank input or invalid formats before regex/blocked checks, relying on other layers and risking unexpected behavior. Referred Code 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 end
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Detailed errors exposed: User-facing JSON includes errors hash and echoes back values.email, which may reveal validation details and account existence to end users. Referred Code render json: { success: false, message: I18n.t("login.errors", errors: user.errors.full_messages.join("\n")), errors: user.errors.to_hash, values: user.attributes.slice("name", "username", "email") }
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: PII echoed client: The frontend stores rejected email addresses in rejectedEmails, retaining PII client-side and auto-failing future attempts, which may have privacy implications and persistence concerns. Referred Code if (this.get('rejectedEmails').contains(email)) { return Discourse.InputValidation.create({ failed: true, reason: I18n.t('user.email.invalid') }); } if ((this.get('authOptions.email') === email) && this.get('authOptions.email_valid')) {

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Separate validation query from statistics update Refactor the BlockedEmail.should_block? method to separate the validation query from the statistics update. This avoids database modifications during validation, adhering to the Command-Query Separation principle and preventing potential data integrity issues. Examples: app/models/blocked_email.rb [11-19] 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 lib/validators/email_validator.rb [13-15] if record.errors[attribute].blank? and BlockedEmail.should_block?(value) record.errors.add(attribute, I18n.t(:'user.email.blocked')) end Solution Walkthrough: Before: # app/models/blocked_email.rb class BlockedEmail def self.should_block?(email) record = BlockedEmail.where(email: email).first if record # Side-effect: DB write inside a query method record.match_count += 1 record.last_match_at = Time.zone.now record.save end record && record.action_type == actions[:block] end end # lib/validators/email_validator.rb class EmailValidator def validate_each(record, attribute, value) # ... if BlockedEmail.should_block?(value) # This triggers a DB write record.errors.add(attribute, :blocked) end end end After: # app/models/blocked_email.rb class BlockedEmail def self.is_blocked?(email) # Query only, no side-effects exists?(email: email, action_type: actions[:block]) end def self.log_match!(email) # Command only, handles update record = BlockedEmail.where(email: email).first record&.increment!(:match_count) record&.touch(:last_match_at) end end # lib/validators/email_validator.rb class EmailValidator def validate_each(record, attribute, value) if BlockedEmail.is_blocked?(value) # Query only record.errors.add(attribute, :blocked) end end end Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a critical design flaw violating the Command-Query Separation principle in BlockedEmail.should_block?, which can lead to data inconsistency due to operations being outside the main database transaction.	High
Security	Anchor regex to end of string Anchor the domain validation regex to the end of the string with \z to prevent users from bypassing domain blacklists or whitelists. lib/validators/email_validator.rb [18-22] def email_in_restriction_setting?(setting, value) domains = setting.gsub('.', '\.') - regexp = Regexp.new("@(#{domains})", true) + regexp = Regexp.new("@(#{domains})\\z", true) value =~ regexp end Apply / Chat Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a security flaw where the domain validation regex is unanchored, allowing users to bypass domain restrictions, and provides the correct fix.	High
Possible issue	Use atomic SQL update for stats Fix a race condition in should_block? by using an atomic SQL update with update_all to increment match_count, preventing lost updates under concurrent requests. app/models/blocked_email.rb [11-19] 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 + 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] end Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a race condition in the match_count update and proposes a valid solution using an atomic SQL operation, which improves data integrity.	Medium
More