Skip to content
Jump to bottom

Automated Test: blocked-email-validation-post #318

Closed
wants to merge 1 commit into from
+155 −23
Closed

Automated Test: blocked-email-validation-post #318

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 13 additions & 1 deletion app/assets/javascripts/discourse/controllers/create_account_controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ Discourse.CreateAccountController = Discourse.Controller.extend(Discourse.ModalF
accountPasswordConfirm: 0,
accountChallenge: 0,
formSubmitted: false,
rejectedEmails: Em.A([]),

submitDisabled: function() {
if (this.get('formSubmitted')) return true;
Expand Down Expand Up @@ -64,6 +65,14 @@ Discourse.CreateAccountController = Discourse.Controller.extend(Discourse.ModalF
}

email = this.get("accountEmail");

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')) {
return Discourse.InputValidation.create({
ok: true,
Expand All @@ -84,7 +93,7 @@ Discourse.CreateAccountController = Discourse.Controller.extend(Discourse.ModalF
failed: true,
reason: I18n.t('user.email.invalid')
});
}.property('accountEmail'),
}.property('accountEmail', 'rejectedEmails.@each'),

usernameMatch: function() {
if (this.usernameNeedsToBeValidatedWithEmail()) {
Expand Down Expand Up @@ -262,6 +271,9 @@ Discourse.CreateAccountController = Discourse.Controller.extend(Discourse.ModalF
createAccountController.set('complete', true);
} else {
createAccountController.flash(result.message || I18n.t('create_account.failed'), 'error');
if (result.errors && result.errors.email && result.values) {
createAccountController.get('rejectedEmails').pushObject(result.values.email);
}
createAccountController.set('formSubmitted', false);
}
if (result.active) {
Expand Down
4 changes: 3 additions & 1 deletion app/controllers/users_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,9 @@ def create
else
render json: {
success: false,
message: I18n.t("login.errors", errors: user.errors.full_messages.join("\n"))
message: I18n.t("login.errors", errors: user.errors.full_messages.join("\n")),
errors: user.errors.to_hash,
values: user.attributes.slice("name", "username", "email")
}
end
rescue ActiveRecord::StatementInvalid
Expand Down
25 changes: 25 additions & 0 deletions app/models/blocked_email.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
class BlockedEmail < ActiveRecord::Base

before_validation :set_defaults

validates :email, presence: true, uniqueness: true

def self.actions
@actions ||= Enum.new(:block, :do_nothing)
end

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
Comment on lines +11 to +17
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Check where match_count is used to understand the impact
rg -n "match_count" --glob '*.rb'

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 374

🏁 Script executed:

# Look at the full BlockedEmail model to understand context and any existing concurrency controls
find . -name "blocked_email.rb" -type f | head -5

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 117

🏁 Script executed:

# Read the full BlockedEmail model
cat -n app/models/blocked_email.rb

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 805

🏁 Script executed:

# Read the spec to understand expected behavior
cat -n spec/models/blocked_email_spec.rb

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 1987

🏁 Script executed:

# Check if there are any transaction blocks or locking mechanisms in the Rails app
rg -n "transaction|with_lock" --glob '*.rb' app/models/ | head -20

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 1299

🏁 Script executed:

# Check all callers of should_block? to ensure the refactor won't break anything
rg -n "should_block\?" --glob '*.rb'

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 676

🏁 Script executed:

# Verify the suggested update_all syntax is complete
rg -n "update_all" --glob '*.rb' app/models/ | head -10

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 1354

Use atomic update to prevent lost match_count increments under concurrent access.

The read/modify/write cycle loses increments when multiple requests fetch the same record concurrently. Additionally, each match unnecessarily triggers validations and callbacks.

Suggested fix 
-    if record
-      record.match_count += 1
-      record.last_match_at = Time.zone.now
-      record.save
-    end
+    if record
+      BlockedEmail.where(id: record.id)
+        .update_all(["match_count = match_count + 1, last_match_at = ?", Time.zone.now])
+    end
🤖 Prompt for AI Agents 
In `@app/models/blocked_email.rb` around lines 11 - 17, The should_block? method
is doing a read/modify/write which loses increments under concurrency and
triggers validations/callbacks; replace the record fetch + record.match_count +=
1 + record.save with a single atomic DB update such as BlockedEmail.where(email:
email).update_all(["match_count = COALESCE(match_count,0) + 1, last_match_at =
?", Time.zone.now]) (or use ActiveRecord::Base.increment_counter if you only
need to increment) so the update occurs atomically and bypasses
validations/callbacks while still updating match_count and last_match_at.

record && record.action_type == actions[:block]
end

def set_defaults
self.action_type ||= BlockedEmail.actions[:block]
end

end