From 38216f6f0b4459d86aefe8daa35c350809514ebc Mon Sep 17 00:00:00 2001 From: David Taylor Date: Mon, 16 May 2022 14:21:33 +0100 Subject: [PATCH] DEV: Make user field validation more specific (#16746) - Only validate if custom_fields are loaded, so that we don't trigger a db query - Only validate public user fields, not all custom_fields This commit also reverts the unrelated spec changes in ba148e08, which were required to work around these issues --- app/models/user.rb | 11 ++++++----- app/models/user_field.rb | 2 ++ spec/jobs/activation_reminder_emails_spec.rb | 7 +++++-- spec/jobs/bulk_invite_spec.rb | 2 +- spec/models/user_spec.rb | 18 +++++++++++++----- spec/services/user_anonymizer_spec.rb | 4 ++-- 6 files changed, 29 insertions(+), 15 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 9a171a67ff2..3020796494b 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -114,7 +114,7 @@ class User < ActiveRecord::Base validates :name, user_full_name: true, if: :will_save_change_to_name?, length: { maximum: 255 } validates :ip_address, allowed_ip_address: { on: :create, message: :signup_not_allowed } validates :primary_email, presence: true - validates :custom_fields_values, watched_words: true + validates :public_user_field_values, watched_words: true, unless: :custom_fields_clean? validates_associated :primary_email, message: -> (_, user_email) { user_email[:value]&.errors[:email]&.first } after_initialize :add_trust_level @@ -1245,6 +1245,11 @@ class User < ActiveRecord::Base end end + def public_user_field_values + @public_user_field_ids ||= UserField.public_fields.pluck(:id) + user_fields(@public_user_field_ids).values.join(" ") + end + def set_user_field(field_id, value) custom_fields["#{USER_FIELD_PREFIX}#{field_id}"] = value end @@ -1792,10 +1797,6 @@ class User < ActiveRecord::Base ) SQL end - - def custom_fields_values - custom_fields.values.join(" ") - end end # == Schema Information diff --git a/app/models/user_field.rb b/app/models/user_field.rb index e5c3caf86d3..ea8330cc69b 100644 --- a/app/models/user_field.rb +++ b/app/models/user_field.rb @@ -14,6 +14,8 @@ class UserField < ActiveRecord::Base before_save :sanitize_description after_save :queue_index_search + scope :public_fields, -> { where(show_on_profile: true).or(where(show_on_user_card: true)) } + def self.max_length 2048 end diff --git a/spec/jobs/activation_reminder_emails_spec.rb b/spec/jobs/activation_reminder_emails_spec.rb index 4b451c28215..3179c8bc119 100644 --- a/spec/jobs/activation_reminder_emails_spec.rb +++ b/spec/jobs/activation_reminder_emails_spec.rb @@ -12,9 +12,12 @@ describe Jobs::ActivationReminderEmails do expect { described_class.new.execute({}) } .to change { ActionMailer::Base.deliveries.size }.by(1) .and change { user.email_tokens.count }.by(1) - .and change { user.reload.custom_fields[:activation_reminder] }.to "t" + + expect(user.custom_fields['activation_reminder']).to eq("t") expect { described_class.new.execute({}) }.to change { ActionMailer::Base.deliveries.size }.by(0) - expect { user.activate }.to change { user.reload.custom_fields[:activation_reminder] }.to nil + + user.activate + expect(user.reload.custom_fields['activation_reminder']).to eq(nil) end it 'should not email active users' do diff --git a/spec/jobs/bulk_invite_spec.rb b/spec/jobs/bulk_invite_spec.rb index dd983a9e246..66c1cf03772 100644 --- a/spec/jobs/bulk_invite_spec.rb +++ b/spec/jobs/bulk_invite_spec.rb @@ -100,7 +100,7 @@ describe Jobs::BulkInvite do expect(User.where(staged: true).find_by_email('test@discourse.org')).to eq(nil) expect(user.user_fields[user_field.id.to_s]).to eq('value 1') expect(user.user_fields[user_field_color.id.to_s]).to eq('Blue') - expect(staged_user.reload.user_fields[user_field.id.to_s]).to eq('value 2') + expect(staged_user.user_fields[user_field.id.to_s]).to eq('value 2') expect(staged_user.user_fields[user_field_color.id.to_s]).to eq(nil) new_staged_user = User.where(staged: true).find_by_email('test2@discourse.org') expect(new_staged_user.user_fields[user_field.id.to_s]).to eq('value 3') diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index e8824059bc9..08b5630b1e1 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -138,7 +138,7 @@ RSpec.describe User do end describe "#user_fields" do - fab!(:user_field) { Fabricate(:user_field) } + fab!(:user_field) { Fabricate(:user_field, show_on_profile: true) } fab!(:watched_word) { Fabricate(:watched_word, word: "bad") } before { user.set_user_field(user_field.id, value) } @@ -146,10 +146,18 @@ RSpec.describe User do context "when user fields contain watched words" do let(:value) { "bad user field value" } - it "is not valid" do - user.valid? - expect(user.errors[:base].size).to eq(1) - expect(user.errors.messages[:base]).to include(/you can't post the word/) + context "when user field is public" do + it "is not valid" do + user.valid? + expect(user.errors[:base].size).to eq(1) + expect(user.errors.messages[:base]).to include(/you can't post the word/) + end + end + + context "when user field is private" do + before { user_field.update(show_on_profile: false) } + + it { is_expected.to be_valid } end end diff --git a/spec/services/user_anonymizer_spec.rb b/spec/services/user_anonymizer_spec.rb index 9156b59fa26..0f7ccc2b74e 100644 --- a/spec/services/user_anonymizer_spec.rb +++ b/spec/services/user_anonymizer_spec.rb @@ -289,9 +289,9 @@ describe UserAnonymizer do "user_field_#{field2.id}": "bar", "another_field": "456" } - user.save - expect { make_anonymous }.to change { user.reload.custom_fields }.to "some_field" => "123", "another_field" => "456" + expect { make_anonymous }.to change { user.custom_fields } + expect(user.reload.custom_fields).to eq("some_field" => "123", "another_field" => "456") end end end