Commit 6a065074 authored by Timothy Andrew's avatar Timothy Andrew

Fix a bug with the User#abuse_report association.

Introduction
------------

1. The foreign key was not explicitly specified on the association.
2. The `AbuseReport` model contains two references to user - `reporter_id` and
   `user_id`
3. `user.abuse_report` is supposed to return the single abuse report where
   `user_id` refers to the given user.

Bug Description
---------------

1. `user.abuse_report` would return an abuse report where `reporter_id` referred
   to the current user, if such an abuse report was present.

2. This implies a slightly more serious bug as well:

   - Assume User A filed an abuse report against User B
   - We have an abuse report where `reporter_id` is User A and `user_id` is User B
   - If User A is updated (`user_a.block`, for example), the abuse report would
     also be updated, such that both `reporter_id` _and_ `user_id` point to User A.

Fix
---

Explicitly declare the foreign key `user_id` in the `has_one` declaration
parent 97cbf7c2
......@@ -89,7 +89,7 @@ class User < ActiveRecord::Base
has_many :subscriptions, dependent: :destroy
has_many :recent_events, -> { order "id DESC" }, foreign_key: :author_id, class_name: "Event"
has_many :oauth_applications, class_name: 'Doorkeeper::Application', as: :owner, dependent: :destroy
has_one :abuse_report, dependent: :destroy
has_one :abuse_report, dependent: :destroy, foreign_key: :user_id
has_many :spam_logs, dependent: :destroy
has_many :builds, dependent: :nullify, class_name: 'Ci::Build'
has_many :pipelines, dependent: :nullify, class_name: 'Ci::Pipeline'
......
......@@ -28,7 +28,6 @@ describe User, models: true do
it { is_expected.to have_many(:merge_requests).dependent(:destroy) }
it { is_expected.to have_many(:assigned_merge_requests).dependent(:nullify) }
it { is_expected.to have_many(:identities).dependent(:destroy) }
it { is_expected.to have_one(:abuse_report) }
it { is_expected.to have_many(:spam_logs).dependent(:destroy) }
it { is_expected.to have_many(:todos).dependent(:destroy) }
it { is_expected.to have_many(:award_emoji).dependent(:destroy) }
......@@ -38,6 +37,33 @@ describe User, models: true do
it { is_expected.to have_many(:chat_names).dependent(:destroy) }
it { is_expected.to have_many(:uploads).dependent(:destroy) }
describe "#abuse_report" do
let(:current_user) { create(:user) }
let(:other_user) { create(:user) }
it { is_expected.to have_one(:abuse_report) }
it "refers to the abuse report whose user_id is the current user" do
abuse_report = create(:abuse_report, reporter: other_user, user: current_user)
expect(current_user.abuse_report).to eq(abuse_report)
end
it "does not refer to the abuse report whose reporter_id is the current user" do
create(:abuse_report, reporter: current_user, user: other_user)
expect(current_user.abuse_report).to be_nil
end
it "does not update the user_id of an abuse report when the user is updated" do
abuse_report = create(:abuse_report, reporter: current_user, user: other_user)
current_user.block
expect(abuse_report.reload.user).to eq(other_user)
end
end
describe '#group_members' do
it 'does not include group memberships for which user is a requester' do
user = create(:user)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment