Merge branch 'issue-hidden-attribute' into 'master'

Hidden issues

See merge request gitlab-org/gitlab!66687

app/assets/javascripts/admin/users/components/actions/ban.vue

@@ -6,10 +6,11 @@ import { I18N_USER_ACTIONS } from '../../constants';
 
 // TODO: To be replaced with <template> content in https://gitlab.com/gitlab-org/gitlab/-/issues/320922
 const messageHtml = `
-  <p>${s__('AdminUsers|When banned, users:')}</p>
+  <p>${s__('AdminUsers|When banned:')}</p>
   <ul>
-    <li>${s__("AdminUsers|Can't log in.")}</li>
-    <li>${s__("AdminUsers|Can't access Git repositories.")}</li>
+    <li>${s__("AdminUsers|The user can't log in.")}</li>
+    <li>${s__("AdminUsers|The user can't access git repositories.")}</li>
+    <li>${s__('AdminUsers|Issues authored by this user are hidden from other users.')}</li>
   </ul>
   <p>${s__('AdminUsers|You can unban their account in the future. Their data remains intact.')}</p>
   <p>${sprintf(

app/finders/issues_finder.rb

@@ -47,17 +47,22 @@ class IssuesFinder < IssuableFinder
 
   # rubocop: disable CodeReuse/ActiveRecord
   def with_confidentiality_access_check
-    return Issue.all if params.user_can_see_all_confidential_issues?
+    return Issue.all if params.user_can_see_all_issues?
+
+    # Only admins can see hidden issues, so for non-admins, we filter out any hidden issues
+    issues = Issue.without_hidden
+
+    return issues.all if params.user_can_see_all_confidential_issues?
 
     # If already filtering by assignee we can skip confidentiality since a user
     # can always see confidential issues assigned to them. This is just an
     # optimization since a very common usecase of this Finder is to load the
     # count of issues assigned to the user for the header bar.
-    return Issue.all if current_user && assignee_filter.includes_user?(current_user)
+    return issues.all if current_user && assignee_filter.includes_user?(current_user)
 
-    return Issue.where('issues.confidential IS NOT TRUE') if params.user_cannot_see_confidential_issues?
+    return issues.where('issues.confidential IS NOT TRUE') if params.user_cannot_see_confidential_issues?
 
-    Issue.where('
+    issues.where('
       issues.confidential IS NOT TRUE
       OR (issues.confidential = TRUE
         AND (issues.author_id = :user_id

app/finders/issues_finder/params.rb

@@ -32,7 +32,7 @@ class IssuesFinder
         if parent
           Ability.allowed?(current_user, :read_confidential_issues, parent)
         else
-          Ability.allowed?(current_user, :read_all_resources)
+          user_can_see_all_issues?
         end
       end
     end
@@ -42,6 +42,12 @@ class IssuesFinder
 
       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
 

app/graphql/resolvers/issues_resolver.rb

@@ -37,7 +37,8 @@ module Resolvers
       [
         {
           project: [:project_feature]
-        }
+        },
+        :author
       ]
     end
 

app/helpers/issues_helper.rb

@@ -52,6 +52,10 @@ module IssuesHelper
     sprite_icon('eye-slash', css_class: 'gl-vertical-align-text-bottom') if issue.confidential?
   end
 
+  def hidden_issue_icon(issue)
+    sprite_icon('spam', css_class: 'gl-vertical-align-text-bottom') if issue.hidden?
+  end
+
   def award_user_list(awards, current_user, limit: 10)
     names = awards.map do |award|
       award.user == current_user ? 'You' : award.user.name

app/models/issue.rb

@@ -130,6 +130,15 @@ class Issue < ApplicationRecord
   scope :public_only, -> { where(confidential: false) }
   scope :confidential_only, -> { where(confidential: true) }
 
+  scope :without_hidden, -> {
+    if Feature.enabled?(:ban_user_feature_flag)
+      where(id: joins('LEFT JOIN banned_users ON banned_users.user_id = issues.author_id WHERE banned_users.user_id IS NULL')
+      .select('issues.id'))
+    else
+      all
+    end
+  }
+
   scope :counts_by_state, -> { reorder(nil).group(:state_id).count }
 
   scope :service_desk, -> { where(author: ::User.support_bot) }
@@ -551,6 +560,8 @@ class Issue < ApplicationRecord
       true
     elsif confidential? && !assignee_or_author?(user)
       project.team.member?(user, Gitlab::Access::REPORTER)
+    elsif hidden?
+      false
     else
       project.public? ||
         project.internal? && !user.external? ||
@@ -558,6 +569,10 @@ class Issue < ApplicationRecord
     end
   end
 
+  def hidden?
+    author&.banned?
+  end
+
   private
 
   def spammable_attribute_changed?
@@ -585,7 +600,7 @@ class Issue < ApplicationRecord
 
   # Returns `true` if this Issue is visible to everybody.
   def publicly_visible?
-    project.public? && !confidential? && !::Gitlab::ExternalAuthorization.enabled?
+    project.public? && !confidential? && !hidden? && !::Gitlab::ExternalAuthorization.enabled?
   end
 
   def expire_etag_cache

app/policies/issue_policy.rb

@@ -15,6 +15,9 @@ 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? }
 
@@ -23,6 +26,10 @@ class IssuePolicy < IssuablePolicy
     prevent :read_issue_iid
   end
 
+  rule { hidden & ~admin }.policy do
+    prevent :read_issue
+  end
+
   rule { ~can?(:read_issue) }.prevent :create_note
 
   rule { locked }.policy do

app/views/projects/issues/_issue.html.haml

@@ -12,6 +12,9 @@
           - if issue.confidential?
             %span.has-tooltip{ title: _('Confidential') }
               = confidential_icon(issue)
+          - if Feature.enabled?(:ban_user_feature_flag) && issue.hidden?
+            %span.has-tooltip{ title: _('This issue is hidden because its author has been banned') }
+              = hidden_issue_icon(issue)
           = link_to issue.title, issue_path(issue)
         = render_if_exists 'projects/issues/subepic_flag', issue: issue
         - if issue.tasks?

doc/user/admin_area/moderate_users.md

@@ -195,12 +195,12 @@ Users can also be activated using the [GitLab API](../../api/users.md#activate-u
 
 > - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/327353) in GitLab 14.2.
 
-GitLab administrators can ban and unban users. The banned user's issues are still displayed. Hiding
-a banned user's issues is a [work in progress](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/66687).
+GitLab administrators can ban and unban users. Banned users are blocked, and their issues are hidden. 
+The banned user's comments are still displayed. Hiding a banned user's comments is [tracked in this issue](https://gitlab.com/gitlab-org/gitlab/-/issues/327356).
 
 ### Ban a user
 
-To completely block a user, administrators can choose to ban the user.
+To block a user and hide their contributions, administrators can ban the user.
 
 Users can be banned using the Admin Area. To do this:
 

ee/spec/finders/issues_finder_spec.rb

@@ -237,9 +237,11 @@ RSpec.describe IssuesFinder do
     let_it_be(:guest) { create(:user) }
 
     let_it_be(:authorized_user) { create(:user) }
+    let_it_be(:banned_user) { create(:user, :banned) }
     let_it_be(:project) { create(:project, namespace: authorized_user.namespace) }
     let_it_be(:public_issue) { create(:issue, project: project) }
     let_it_be(:confidential_issue) { create(:issue, project: project, confidential: true) }
+    let_it_be(:hidden_issue) { create(:issue, project: project, author: banned_user) }
 
     context 'when no project filter is given' do
       let(:params) { {} }
@@ -250,7 +252,7 @@ RSpec.describe IssuesFinder do
         subject { described_class.new(auditor_user, params).with_confidentiality_access_check }
 
         it 'returns all issues' do
-          expect(subject).to include(public_issue, confidential_issue)
+          expect(subject).to include(public_issue, confidential_issue, hidden_issue)
         end
       end
     end
@@ -264,7 +266,7 @@ RSpec.describe IssuesFinder do
         subject { described_class.new(auditor_user, params).with_confidentiality_access_check }
 
         it 'returns all issues' do
-          expect(subject).to include(public_issue, confidential_issue)
+          expect(subject).to include(public_issue, confidential_issue, hidden_issue)
         end
       end
     end

locale/gitlab.pot

@@ -2571,12 +2571,6 @@ msgstr ""
 msgid "AdminUsers|Blocking user has the following effects:"
 msgstr ""
 
-msgid "AdminUsers|Can't access Git repositories."
-msgstr ""
-
-msgid "AdminUsers|Can't log in."
-msgstr ""
-
 msgid "AdminUsers|Cannot sign in or access instance information"
 msgstr ""
 
@@ -2643,6 +2637,9 @@ msgstr ""
 msgid "AdminUsers|Is using seat"
 msgstr ""
 
+msgid "AdminUsers|Issues authored by this user are hidden from other users."
+msgstr ""
+
 msgid "AdminUsers|It's you!"
 msgstr ""
 
@@ -2706,6 +2703,12 @@ msgstr ""
 msgid "AdminUsers|Sort by"
 msgstr ""
 
+msgid "AdminUsers|The user can't access git repositories."
+msgstr ""
+
+msgid "AdminUsers|The user can't log in."
+msgstr ""
+
 msgid "AdminUsers|The user will be logged out"
 msgstr ""
 
@@ -2772,7 +2775,7 @@ msgstr ""
 msgid "AdminUsers|What does this mean?"
 msgstr ""
 
-msgid "AdminUsers|When banned, users:"
+msgid "AdminUsers|When banned:"
 msgstr ""
 
 msgid "AdminUsers|When the user logs back in, their account will reactivate as a fully active account"
@@ -34029,6 +34032,9 @@ msgstr ""
 msgid "This issue is currently blocked by the following issues:"
 msgstr ""
 
+msgid "This issue is hidden because its author has been banned"
+msgstr ""
+
 msgid "This issue is in a child epic of the filtered epic"
 msgstr ""
 

spec/graphql/mutations/design_management/delete_spec.rb

@@ -86,9 +86,9 @@ RSpec.describe Mutations::DesignManagement::Delete do
           end
         end
 
-        it 'runs no more than 28 queries' do
+        it 'runs no more than 29 queries' do
           filenames.each(&:present?) # ignore setup
-          # Queries: as of 2019-08-28
+          # Queries: as of 2021-07-22
           # -------------
           # 01. routing query
           # 02. find project by id
@@ -100,25 +100,26 @@ RSpec.describe Mutations::DesignManagement::Delete do
           # 09. find namespace by id
           # 10. find group namespace by id
           # 11. project.authorizations for user (same query as 5)
-          # 12. project.project_features (same query as 3)
-          # 13. project.authorizations for user (same query as 5)
-          # 14. current designs by filename and issue
-          # 15, 16 project.authorizations for user (same query as 5)
-          # 17. find route by id and source_type
+          # 12. find user by id
+          # 13. project.project_features (same query as 3)
+          # 14. project.authorizations for user (same query as 5)
+          # 15. current designs by filename and issue
+          # 16, 17 project.authorizations for user (same query as 5)
+          # 18. find route by id and source_type
           # ------------- our queries are below:
-          # 18. start transaction 1
-          # 19.   start transaction 2
-          # 20.     find version by sha and issue
-          # 21.     exists version with sha and issue?
-          # 22.   leave transaction 2
-          # 23.   create version with sha and issue
-          # 24.   create design-version links
-          # 25.   validate version.actions.present?
-          # 26.   validate version.issue.present?
-          # 27.   validate version.sha is unique
-          # 28. leave transaction 1
+          # 19. start transaction 1
+          # 20.   start transaction 2
+          # 21.     find version by sha and issue
+          # 22.     exists version with sha and issue?
+          # 23.   leave transaction 2
+          # 24.   create version with sha and issue
+          # 25.   create design-version links
+          # 26.   validate version.actions.present?
+          # 27.   validate version.issue.present?
+          # 28.   validate version.sha is unique
+          # 29. leave transaction 1
           #
-          expect { run_mutation }.not_to exceed_query_limit(28)
+          expect { run_mutation }.not_to exceed_query_limit(29)
         end
       end
 

spec/graphql/resolvers/issues_resolver_spec.rb

@@ -393,13 +393,13 @@ RSpec.describe Resolvers::IssuesResolver do
       end
 
       it 'finds a specific issue with iid', :request_store do
-        result = batch_sync(max_queries: 4) { resolve_issues(iid: issue1.iid).to_a }
+        result = batch_sync(max_queries: 5) { resolve_issues(iid: issue1.iid).to_a }
 
         expect(result).to contain_exactly(issue1)
       end
 
       it 'batches queries that only include IIDs', :request_store do
-        result = batch_sync(max_queries: 4) do
+        result = batch_sync(max_queries: 5) do
           [issue1, issue2]
             .map { |issue| resolve_issues(iid: issue.iid.to_s) }
             .flat_map(&:to_a)
@@ -409,7 +409,7 @@ RSpec.describe Resolvers::IssuesResolver do
       end
 
       it 'finds a specific issue with iids', :request_store do
-        result = batch_sync(max_queries: 4) do
+        result = batch_sync(max_queries: 5) do
           resolve_issues(iids: [issue1.iid]).to_a
         end
 

spec/models/issue_spec.rb

@@ -796,17 +796,47 @@ RSpec.describe Issue do
         end
       end
 
+      shared_examples 'hidden issue readable by user' do
+        before do
+          issue.author.ban!
+        end
+
+        specify do
+          is_expected.to eq(true)
+        end
+
+        after do
+          issue.author.activate!
+        end
+      end
+
+      shared_examples 'hidden issue not readable by user' do
+        before do
+          issue.author.ban!
+        end
+
+        specify do
+          is_expected.to eq(false)
+        end
+
+        after do
+          issue.author.activate!
+        end
+      end
+
       context 'with an admin user' do
         let(:user) { build(:admin) }
 
         context 'when admin mode is enabled', :enable_admin_mode do
           it_behaves_like 'issue readable by user'
           it_behaves_like 'confidential issue readable by user'
+          it_behaves_like 'hidden issue readable by user'
         end
 
         context 'when admin mode is disabled' do
           it_behaves_like 'issue not readable by user'
           it_behaves_like 'confidential issue not readable by user'
+          it_behaves_like 'hidden issue not readable by user'
         end
       end
 
@@ -817,6 +847,7 @@ RSpec.describe Issue do
 
         it_behaves_like 'issue readable by user'
         it_behaves_like 'confidential issue readable by user'
+        it_behaves_like 'hidden issue not readable by user'
       end
 
       context 'with a reporter user' do
@@ -826,6 +857,7 @@ RSpec.describe Issue do
 
         it_behaves_like 'issue readable by user'
         it_behaves_like 'confidential issue readable by user'
+        it_behaves_like 'hidden issue not readable by user'
       end
 
       context 'with a guest user' do
@@ -835,6 +867,7 @@ RSpec.describe Issue do
 
         it_behaves_like 'issue readable by user'
         it_behaves_like 'confidential issue not readable by user'
+        it_behaves_like 'hidden issue not readable by user'
 
         context 'when user is an assignee' do
           before do
@@ -843,6 +876,7 @@ RSpec.describe Issue do
 
           it_behaves_like 'issue readable by user'
           it_behaves_like 'confidential issue readable by user'
+          it_behaves_like 'hidden issue not readable by user'
         end
 
         context 'when user is the author' do
@@ -852,6 +886,7 @@ RSpec.describe Issue do
 
           it_behaves_like 'issue readable by user'
           it_behaves_like 'confidential issue readable by user'
+          it_behaves_like 'hidden issue not readable by user'
         end
       end
 
@@ -861,6 +896,7 @@ RSpec.describe Issue do
 
           it_behaves_like 'issue readable by user'
           it_behaves_like 'confidential issue not readable by user'
+          it_behaves_like 'hidden issue not readable by user'
         end
 
         context 'using an internal project' do
@@ -873,6 +909,7 @@ RSpec.describe Issue do
 
             it_behaves_like 'issue readable by user'
             it_behaves_like 'confidential issue not readable by user'
+            it_behaves_like 'hidden issue not readable by user'
           end
 
           context 'using an external user' do
@@ -882,6 +919,7 @@ RSpec.describe Issue do
 
             it_behaves_like 'issue not readable by user'
             it_behaves_like 'confidential issue not readable by user'
+            it_behaves_like 'hidden issue not readable by user'
           end
         end
 
@@ -892,6 +930,7 @@ RSpec.describe Issue do
 
           it_behaves_like 'issue not readable by user'
           it_behaves_like 'confidential issue not readable by user'
+          it_behaves_like 'hidden issue not readable by user'
         end
       end
 
@@ -1160,6 +1199,26 @@ RSpec.describe Issue do
     end
   end
 
+  describe '.without_hidden' do
+    let_it_be(:banned_user) { create(:user, :banned) }
+    let_it_be(:public_issue) { create(:issue, project: reusable_project) }
+    let_it_be(:hidden_issue) { create(:issue, project: reusable_project, author: banned_user) }
+
+    it 'only returns without_hidden issues' do
+      expect(described_class.without_hidden).to eq([public_issue])
+    end
+
+    context 'when feature flag is disabled' do
+      before do
+        stub_feature_flags(ban_user_feature_flag: false)
+      end
+
+      it 'returns public and hidden issues' do
+        expect(described_class.without_hidden).to eq([public_issue, hidden_issue])
+      end
+    end
+  end
+
   describe '.by_project_id_and_iid' do
     let_it_be(:issue_a) { create(:issue, project: reusable_project) }
     let_it_be(:issue_b) { create(:issue, iid: issue_a.iid) }

spec/policies/issue_policy_spec.rb

@@ -360,6 +360,21 @@ RSpec.describe IssuePolicy do
         expect(permissions(assignee, confidential_issue_no_assignee)).to be_disallowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue, :set_issue_metadata)
       end
     end
+
+    context 'with a hidden issue' do
+      let(:user) { create(:user) }
+      let(:banned_user) { create(:user, :banned) }
+      let(:admin) { create(:user, :admin)}
+      let(:hidden_issue) { create(:issue, project: project, author: banned_user) }
+
+      it 'does not allow non-admin user to read the issue' do
+        expect(permissions(user, hidden_issue)).not_to be_allowed(:read_issue)
+      end
+
+      it 'allows admin to read the issue', :enable_admin_mode do
+        expect(permissions(admin, hidden_issue)).to be_allowed(:read_issue)
+      end
+    end
   end
 
   context 'with external authorization enabled' do