From bc71e174806214d279470af604220e653fc86612 Mon Sep 17 00:00:00 2001 From: Alex Buijs Date: Wed, 28 Dec 2022 13:12:54 +0100 Subject: [PATCH 1/4] Move shared hidden logic from Issue to Issuable In order to make it reusable for merge requests --- app/finders/issuable_finder/params.rb | 5 +++++ app/finders/issues_finder.rb | 2 +- app/finders/issues_finder/params.rb | 8 +------- app/helpers/issuables_helper.rb | 8 ++++++++ app/helpers/issues_helper.rb | 4 +--- app/policies/issuable_policy.rb | 3 +++ app/policies/issue_policy.rb | 3 --- locale/gitlab.pot | 3 +++ spec/helpers/issuables_helper_spec.rb | 24 ++++++++++++++++++++++++ 9 files changed, 46 insertions(+), 14 deletions(-) diff --git a/app/finders/issuable_finder/params.rb b/app/finders/issuable_finder/params.rb index 32d50802537cee..e59c2224594ad7 100644 --- a/app/finders/issuable_finder/params.rb +++ b/app/finders/issuable_finder/params.rb @@ -195,6 +195,11 @@ def parent project || group end + def user_can_see_all_issuables? + Ability.allowed?(current_user, :read_all_resources) + end + strong_memoize_attr :user_can_see_all_issuables? + private def projects_public_or_visible_to_user diff --git a/app/finders/issues_finder.rb b/app/finders/issues_finder.rb index e12dce744b5748..bd81f06f93b156 100644 --- a/app/finders/issues_finder.rb +++ b/app/finders/issues_finder.rb @@ -49,7 +49,7 @@ def params_class # rubocop: disable CodeReuse/ActiveRecord def with_confidentiality_access_check - return model_class.all if params.user_can_see_all_issues? + return model_class.all if params.user_can_see_all_issuables? # Only admins can see hidden issues, so for non-admins, we filter out any hidden issues issues = model_class.without_hidden diff --git a/app/finders/issues_finder/params.rb b/app/finders/issues_finder/params.rb index 7f8acb79ed688c..786bfbd4113b84 100644 --- a/app/finders/issues_finder/params.rb +++ b/app/finders/issues_finder/params.rb @@ -44,7 +44,7 @@ def user_can_see_all_confidential_issues? if parent Ability.allowed?(current_user, :read_confidential_issues, parent) else - user_can_see_all_issues? + user_can_see_all_issuables? end end end @@ -54,12 +54,6 @@ def user_cannot_see_confidential_issues? current_user.blank? end - - def user_can_see_all_issues? - strong_memoize(:user_can_see_all_issues) do - Ability.allowed?(current_user, :read_all_resources) - end - end end end diff --git a/app/helpers/issuables_helper.rb b/app/helpers/issuables_helper.rb index 2b21d8c51e6921..910823b89ee5c4 100644 --- a/app/helpers/issuables_helper.rb +++ b/app/helpers/issuables_helper.rb @@ -372,6 +372,14 @@ def state_name_with_icon(issuable) end end + def hidden_issuable_icon(issuable) + title = format(_('This %{issuable} is hidden because its author has been banned'), + issuable: issuable.human_class_name) + content_tag(:span, class: 'has-tooltip', title: title) do + sprite_icon('spam', css_class: 'gl-vertical-align-text-bottom') + end + end + private def sidebar_gutter_collapsed? diff --git a/app/helpers/issues_helper.rb b/app/helpers/issues_helper.rb index 9095129e05ceab..527106de7900f6 100644 --- a/app/helpers/issues_helper.rb +++ b/app/helpers/issues_helper.rb @@ -75,9 +75,7 @@ def issue_hidden?(issue) def hidden_issue_icon(issue) return unless issue_hidden?(issue) - content_tag(:span, class: 'has-tooltip', title: _('This issue is hidden because its author has been banned')) do - sprite_icon('spam', css_class: 'gl-vertical-align-text-bottom') - end + hidden_issuable_icon(issue) end def award_user_list(awards, current_user, limit: 10) diff --git a/app/policies/issuable_policy.rb b/app/policies/issuable_policy.rb index fae66498038cd0..52796ed1a1d842 100644 --- a/app/policies/issuable_policy.rb +++ b/app/policies/issuable_policy.rb @@ -16,6 +16,9 @@ class IssuablePolicy < BasePolicy condition(:is_incident) { @subject.incident? } + desc "Issuable is hidden" + condition(:hidden, scope: :subject) { @subject.hidden? } + rule { can?(:guest_access) & assignee_or_author & ~is_incident }.policy do enable :read_issue enable :update_issue diff --git a/app/policies/issue_policy.rb b/app/policies/issue_policy.rb index 12d83735d225f1..8215bf421f3240 100644 --- a/app/policies/issue_policy.rb +++ b/app/policies/issue_policy.rb @@ -21,9 +21,6 @@ class IssuePolicy < IssuablePolicy desc "Issue is confidential" condition(:confidential, scope: :subject) { @subject.confidential? } - desc "Issue is hidden" - condition(:hidden, scope: :subject) { @subject.hidden? } - desc "Issue is persisted" condition(:persisted, scope: :subject) { @subject.persisted? } diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 87f17ed548dc2b..321c421f45e041 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -42306,6 +42306,9 @@ msgstr "" msgid "This %{issuableDisplayName} is locked. Only project members can comment." msgstr "" +msgid "This %{issuable} is hidden because its author has been banned" +msgstr "" + msgid "This %{issuable} is locked. Only %{strong_open}project members%{strong_close} can comment." msgstr "" diff --git a/spec/helpers/issuables_helper_spec.rb b/spec/helpers/issuables_helper_spec.rb index 15b57a4c9eb034..7768263bd1a60a 100644 --- a/spec/helpers/issuables_helper_spec.rb +++ b/spec/helpers/issuables_helper_spec.rb @@ -629,4 +629,28 @@ expect(helper.sidebar_milestone_tooltip_label(milestone)).to eq('<img onerror=alert(1)>
Milestone') end end + + describe '#hidden_issuable_icon' do + let_it_be(:mock_svg) { ''.html_safe } + + before do + allow(helper).to receive(:sprite_icon).and_return(mock_svg) + end + + context 'when issuable is an issue' do + let_it_be(:issuable) { build(:issue) } + + it 'returns icon with tooltip' do + expect(helper.hidden_issuable_icon(issuable)).to eq("") + end + end + + context 'when issuable is a merge request' do + let_it_be(:issuable) { build(:merge_request) } + + it 'returns icon with tooltip' do + expect(helper.hidden_issuable_icon(issuable)).to eq("") + end + end + end end -- GitLab From 5c1e5571d273c78e325a38c166a395ce3827c13d Mon Sep 17 00:00:00 2001 From: Alex Buijs Date: Mon, 5 Dec 2022 15:53:51 +0100 Subject: [PATCH 2/4] Hide merge requests from banned users And show a hidden icon for admins and auditors --- .../admin/users/components/actions/ban.vue | 4 +- .../components/issuable_header_warnings.vue | 6 ++- .../merge_requests/application_controller.rb | 4 ++ app/finders/issuable_finder.rb | 5 ++- .../concerns/resolves_merge_requests.rb | 2 +- app/helpers/merge_requests_helper.rb | 6 +++ app/models/merge_request.rb | 12 ++++++ app/policies/merge_request_policy.rb | 4 ++ .../merge_requests/_merge_request.html.haml | 1 + .../merge_requests/_mr_title.html.haml | 2 +- .../hide_merge_requests_from_banned_users.yml | 8 ++++ doc/user/admin_area/moderate_users.md | 2 +- ee/spec/finders/merge_requests_finder_spec.rb | 17 ++++++++ locale/gitlab.pot | 2 +- .../admin_views_hidden_merge_request_spec.rb | 26 ++++++++++++ .../admin_views_hidden_merge_requests_spec.rb | 26 ++++++++++++ .../issuable_header_warnings_spec.js | 3 +- spec/helpers/issuables_helper_spec.rb | 2 +- spec/models/merge_request_spec.rb | 42 +++++++++++++++++++ spec/policies/merge_request_policy_spec.rb | 16 +++++++ .../merge_requests_controller_spec.rb | 28 +++++++++---- 21 files changed, 201 insertions(+), 17 deletions(-) create mode 100644 config/feature_flags/development/hide_merge_requests_from_banned_users.yml create mode 100644 spec/features/merge_request/admin_views_hidden_merge_request_spec.rb create mode 100644 spec/features/merge_requests/admin_views_hidden_merge_requests_spec.rb diff --git a/app/assets/javascripts/admin/users/components/actions/ban.vue b/app/assets/javascripts/admin/users/components/actions/ban.vue index 55938832dce5d4..898a688c203f07 100644 --- a/app/assets/javascripts/admin/users/components/actions/ban.vue +++ b/app/assets/javascripts/admin/users/components/actions/ban.vue @@ -11,7 +11,9 @@ const messageHtml = `

${s__('AdminUsers|You can unban their account in the future. Their data remains intact.')}

${sprintf( diff --git a/app/assets/javascripts/issuable/components/issuable_header_warnings.vue b/app/assets/javascripts/issuable/components/issuable_header_warnings.vue index 543dca0afe1f91..a84187ab86b8b4 100644 --- a/app/assets/javascripts/issuable/components/issuable_header_warnings.vue +++ b/app/assets/javascripts/issuable/components/issuable_header_warnings.vue @@ -1,7 +1,7 @@