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.
WalkthroughThis change introduces a new system for blocking specific email addresses during user account creation. It adds a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant Backend
participant BlockedEmailModel
participant EmailValidator
User->>Frontend: Submit account creation with email
Frontend->>Backend: POST /users with email
Backend->>EmailValidator: Validate email
EmailValidator->>BlockedEmailModel: should_block?(email)
BlockedEmailModel-->>EmailValidator: Return block status
EmailValidator-->>Backend: Validation result
alt Email blocked or invalid
Backend-->>Frontend: Respond with error, include email in error values
Frontend->>Frontend: Add email to rejectedEmails
Frontend-->>User: Show error, prevent reuse
else Email valid
Backend-->>Frontend: Respond with success
Frontend-->>User: Proceed with account creation
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
spec/components/validators/email_validator_spec.rb (1)
3-23: Good test coverage with minor style improvements needed.The tests properly cover both blocked and unblocked email scenarios using appropriate stubs. However, there are some style issues to address:
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 - endspec/models/blocked_email_spec.rb (2)
1-49: Comprehensive test coverage with minor syntax improvements needed.The test structure effectively covers the BlockedEmail model functionality. However, consider modernizing the RSpec syntax for better maintainability.
Apply this diff to modernize the RSpec syntax:
- subject.should be_false + expect(subject).to be_falsey- it { should be_true } + it { is_expected.to be_truthy }- it { should be_false } + it { is_expected.to be_falsey }
4-5: Remove extra empty lines for consistent formatting.The static analysis correctly identifies unnecessary empty lines at block boundaries.
Apply this diff to fix the formatting:
describe BlockedEmail do - let(:email) { 'block@spamfromhome.org' }end - endAlso applies to: 47-48
lib/validators/email_validator.rb (1)
18-22: Consider enhancing domain matching robustness.The current domain matching implementation works but could be made more robust to handle edge cases and improve security.
Apply this diff to improve the domain matching logic:
def email_in_restriction_setting?(setting, value) - domains = setting.gsub('.', '\.') - regexp = Regexp.new("@(#{domains})", true) - value =~ regexp + return false if value.blank? + email_domain = value.split('@').last&.downcase + return false if email_domain.blank? + + allowed_domains = setting.split('|').map(&:strip).map(&:downcase) + allowed_domains.any? { |domain| email_domain == domain || email_domain.end_with?(".#{domain}") } endThis approach:
- Handles subdomain matching more explicitly
- Avoids regex injection risks
- Provides clearer domain comparison logic
- Handles edge cases like missing @ symbol
app/models/blocked_email.rb (2)
11-19: Consider performance and reliability improvements.The should_block? method works correctly but could be optimized and made more robust.
Apply this diff to improve the method:
def self.should_block?(email) - record = BlockedEmail.where(email: email).first + record = BlockedEmail.find_by(email: email) if record record.match_count += 1 record.last_match_at = Time.zone.now - record.save + record.save! end record && record.action_type == actions[:block] endImprovements:
find_byis more idiomatic thanwhere().firstsave!will raise an exception if the save fails, making debugging easier- Consider adding error handling if statistics updates should be non-blocking
11-19: Evaluate statistics tracking performance impact.The current implementation updates match statistics on every call to should_block?, which could create database load if this method is called frequently. Consider if this level of tracking granularity is necessary.
For high-frequency validation scenarios, consider:
- Batching statistics updates
- Using background jobs for statistics
- Caching frequently accessed blocked emails
- Adding database indexes on the email column for performance
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/assets/javascripts/discourse/controllers/create_account_controller.js(4 hunks)app/controllers/users_controller.rb(1 hunks)app/models/blocked_email.rb(1 hunks)app/models/user.rb(1 hunks)config/locales/server.en.yml(1 hunks)db/migrate/20130724201552_create_blocked_emails.rb(1 hunks)lib/validators/email_validator.rb(1 hunks)spec/components/validators/email_validator_spec.rb(1 hunks)spec/fabricators/blocked_email_fabricator.rb(1 hunks)spec/models/blocked_email_spec.rb(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
spec/fabricators/blocked_email_fabricator.rb (1)
app/models/blocked_email.rb (1)
actions(7-9)
app/controllers/users_controller.rb (1)
app/serializers/user_serializer.rb (1)
attributes(1-76)
spec/components/validators/email_validator_spec.rb (2)
spec/spec_helper.rb (1)
build(130-132)lib/validators/email_validator.rb (2)
validate_each(1-24)validate_each(3-16)
lib/validators/email_validator.rb (1)
app/models/blocked_email.rb (1)
should_block?(11-19)
🪛 RuboCop (1.76.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 (11)
config/locales/server.en.yml (1)
734-734: LGTM!The new locale string is appropriately placed and follows the existing naming conventions. The message "is not allowed." is clear and consistent with the existing email validation messages.
spec/fabricators/blocked_email_fabricator.rb (1)
1-4: LGTM!The fabricator is well-designed with unique sequential email generation and appropriate test data. The email format clearly indicates test nature and the action_type correctly references the BlockedEmail enum.
db/migrate/20130724201552_create_blocked_emails.rb (1)
1-12: LGTM!The migration is well-designed with appropriate column types, constraints, and indexing. The unique index on email prevents duplicates, and the match_count/last_match_at columns provide useful tracking capabilities.
app/controllers/users_controller.rb (1)
197-199: Excellent enhancement to error response structure.The addition of structured
errorshash andvalueshash greatly improves the user experience by:
- Providing detailed validation errors for programmatic handling
- Preserving submitted values so users don't need to re-enter valid fields
- Maintaining backward compatibility with the existing
messagefieldlib/validators/email_validator.rb (1)
1-16: Well-structured email validator with proper error handling.The validator correctly implements the validation flow: whitelist → blacklist → blocked email check. The conditional logic ensures appropriate precedence and the integration with BlockedEmail is well-designed.
app/assets/javascripts/discourse/controllers/create_account_controller.js (4)
17-17: Good addition of client-side email tracking.The
rejectedEmailsarray properly uses Ember.A() for reactive array functionality.
69-75: Verify the validation flow order for user experience.The rejected email check occurs before other email validations, which provides immediate feedback. However, consider whether this might confuse users if the email format is invalid but they see a "rejected" message instead of "invalid format".
The current order prioritizes rejected email feedback over format validation. Consider if this provides the best user experience or if format validation should come first.
96-96: Correct computed property dependency.The addition of
rejectedEmails.@eachas a dependency ensures the validation updates when emails are added to the rejected list.
274-276: Effective error handling integration.The server response parsing correctly identifies email validation failures and captures the submitted email for future rejection. This creates a good feedback loop between server-side validation and client-side prevention.
app/models/blocked_email.rb (1)
1-9: Well-structured model with proper validations.The BlockedEmail model follows Rails conventions with appropriate validations and the enum-like actions pattern provides good type safety.
app/models/user.rb (1)
46-47: Excellent refactoring to use dedicated email validator.The migration from custom validation methods to Rails standard validations with the new EmailValidator improves code organization and maintainability. The conditional validation
if: :email_changed?appropriately prevents unnecessary validation runs.This change successfully:
- Separates email validation concerns into a dedicated validator
- Maintains existing validation requirements (presence, uniqueness)
- Integrates domain restrictions and blocked email checking
- Follows Rails conventions for cleaner, more maintainable code
|
This pull request has been automatically marked as stale because it has been open for 60 days with no activity. To keep it open, remove the stale tag, push code, or add a comment. Otherwise, it will be closed in 14 days. |
2 participants
Test 3
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores