From 48075c77c6172ef7935c925d8e84133fbd61729a Mon Sep 17 00:00:00 2001 From: Jay Swain Date: Fri, 24 Feb 2023 17:59:06 -0300 Subject: [PATCH 1/3] Hide notes from banned users This MR hides comments from banned users. Admins are not affected. part of: https://gitlab.com/gitlab-org/gitlab/-/issues/327356 --- app/finders/notes_finder.rb | 5 +++ app/models/note.rb | 8 +++++ .../development/hidden_notes.yml | 8 +++++ spec/finders/notes_finder_spec.rb | 33 +++++++++++++++++++ spec/models/note_spec.rb | 26 +++++++++++++++ 5 files changed, 80 insertions(+) create mode 100644 config/feature_flags/development/hidden_notes.yml diff --git a/app/finders/notes_finder.rb b/app/finders/notes_finder.rb index 81017290f12af1..5a764ae7000cae 100644 --- a/app/finders/notes_finder.rb +++ b/app/finders/notes_finder.rb @@ -31,6 +31,7 @@ def execute notes = since_fetch_at(notes) notes = notes.with_notes_filter(@params[:notes_filter]) if notes_filter? notes = redact_internal(notes) + notes = notes.without_hidden if without_hidden_notes? sort(notes) end @@ -189,6 +190,10 @@ def redact_internal(notes) notes.not_internal end + + def without_hidden_notes? + Feature.enabled?(:hidden_notes) && !@current_user&.can_admin_all_resources? + end end NotesFinder.prepend_mod_with('NotesFinder') diff --git a/app/models/note.rb b/app/models/note.rb index b9b884b88c5ccb..13fff9520b7ab5 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -173,6 +173,14 @@ class Note < ApplicationRecord end scope :with_metadata, -> { includes(:system_note_metadata) } + scope :without_hidden, -> { + if Feature.enabled?(:hidden_notes) + where_not_exists(Users::BannedUser.where('notes.author_id = banned_users.user_id')) + else + all + end + } + scope :for_note_or_capitalized_note, ->(text) { where(note: [text, text.capitalize]) } scope :like_note_or_capitalized_note, ->(text) { where('(note LIKE ? OR note LIKE ?)', text, text.capitalize) } diff --git a/config/feature_flags/development/hidden_notes.yml b/config/feature_flags/development/hidden_notes.yml new file mode 100644 index 00000000000000..239234d696037d --- /dev/null +++ b/config/feature_flags/development/hidden_notes.yml @@ -0,0 +1,8 @@ +--- +name: hidden_notes +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/112973 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/405148 +milestone: '15.11' +type: development +group: group::anti-abuse +default_enabled: false diff --git a/spec/finders/notes_finder_spec.rb b/spec/finders/notes_finder_spec.rb index 1df0aa411d69e1..7a0383e465fab6 100644 --- a/spec/finders/notes_finder_spec.rb +++ b/spec/finders/notes_finder_spec.rb @@ -128,6 +128,39 @@ end end + context 'for notes from users who have been banned', :enable_admin_mode, feature_category: :instance_resiliency do + subject(:finder) { described_class.new(user, project: project).execute } + + let_it_be(:banned_user) { create(:banned_user).user } + let!(:banned_note) { create(:note_on_issue, project: project, author: banned_user) } + + context 'when :hidden_notes feature is not enabled' do + before do + stub_feature_flags(hidden_notes: false) + end + + context 'when user is not an admin' do + it { is_expected.to include(banned_note) } + end + end + + context 'when :hidden_notes feature is enabled' do + before do + stub_feature_flags(hidden_notes: true) + end + + context 'when user is an admin' do + let(:user) { create(:admin) } + + it { is_expected.to include(banned_note) } + end + + context 'when user is not an admin' do + it { is_expected.not_to include(banned_note) } + end + end + end + context 'for target type' do let(:project) { create(:project, :repository) } let!(:note1) { create :note_on_issue, project: project } diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index c1de8125c0d313..86d515cd55ef6f 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -1666,6 +1666,32 @@ def expect_expiration(noteable) end end end + + describe 'without_hidden' do + subject { described_class.without_hidden } + + context 'when a note with a banned author exists' do + let_it_be(:banned_user) { create(:banned_user).user } + let_it_be(:banned_note) { create(:note, author: banned_user) } + + context 'when the :hidden_notes feature is disabled' do + before do + stub_feature_flags(hidden_notes: false) + end + + it { is_expected.to include(banned_note, note1) } + end + + context 'when the :hidden_notes feature is enabled' do + before do + stub_feature_flags(hidden_notes: true) + end + + it { is_expected.not_to include(banned_note) } + it { is_expected.to include(note1) } + end + end + end end describe 'banzai_render_context' do -- GitLab From 340326907ac1f6a2d14d5797d90d4c6061efd23d Mon Sep 17 00:00:00 2001 From: Jay Swain Date: Mon, 10 Apr 2023 14:32:33 -0700 Subject: [PATCH 2/3] Feedback from BE review * make code more readable * add nil @current_user test case --- app/finders/notes_finder.rb | 5 ++++- spec/finders/notes_finder_spec.rb | 12 ++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/app/finders/notes_finder.rb b/app/finders/notes_finder.rb index 5a764ae7000cae..3d764f67990615 100644 --- a/app/finders/notes_finder.rb +++ b/app/finders/notes_finder.rb @@ -192,7 +192,10 @@ def redact_internal(notes) end def without_hidden_notes? - Feature.enabled?(:hidden_notes) && !@current_user&.can_admin_all_resources? + return false unless Feature.enabled?(:hidden_notes) + return false if @current_user&.can_admin_all_resources? + + true end end diff --git a/spec/finders/notes_finder_spec.rb b/spec/finders/notes_finder_spec.rb index 7a0383e465fab6..e93c0c790c2a79 100644 --- a/spec/finders/notes_finder_spec.rb +++ b/spec/finders/notes_finder_spec.rb @@ -142,6 +142,12 @@ context 'when user is not an admin' do it { is_expected.to include(banned_note) } end + + context 'when @current_user is nil' do + let(:user) { nil } + + it { is_expected.to be_empty } + end end context 'when :hidden_notes feature is enabled' do @@ -158,6 +164,12 @@ context 'when user is not an admin' do it { is_expected.not_to include(banned_note) } end + + context 'when @current_user is nil' do + let(:user) { nil } + + it { is_expected.to be_empty } + end end end -- GitLab From 76d36424318a0ad1f2e464d6023a677d31e97926 Mon Sep 17 00:00:00 2001 From: Doug Stull Date: Mon, 10 Apr 2023 21:36:37 +0000 Subject: [PATCH 3/3] Apply 1 suggestion(s) to 1 file(s) --- spec/models/note_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 86d515cd55ef6f..ed2bf26e1297a5 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -1667,7 +1667,7 @@ def expect_expiration(noteable) end end - describe 'without_hidden' do + describe '.without_hidden' do subject { described_class.without_hidden } context 'when a note with a banned author exists' do -- GitLab