Merge branch '340716-confidential-issue-2' into 'master'

Validate anonymous search access in issuable API endpoints

See merge request gitlab-org/gitlab!70290

app/graphql/resolvers/concerns/issue_resolver_arguments.rb

@@ -4,6 +4,7 @@ module IssueResolverArguments
   extend ActiveSupport::Concern
 
   prepended do
+    include SearchArguments
     include LooksAhead
 
     argument :iid, GraphQL::Types::String,
@@ -49,9 +50,6 @@ module IssueResolverArguments
     argument :closed_after, Types::TimeType,
              required: false,
              description: 'Issues closed after this date.'
-    argument :search, GraphQL::Types::String,
-             required: false,
-             description: 'Search query for issue title or description.'
     argument :types, [Types::IssueTypeEnum],
              as: :issue_types,
              description: 'Filter issues by the given issue types.',
@@ -91,6 +89,7 @@ module IssueResolverArguments
     params_not_mutually_exclusive(args, mutually_exclusive_assignee_username_args)
     params_not_mutually_exclusive(args, mutually_exclusive_milestone_args)
     params_not_mutually_exclusive(args.fetch(:not, {}), mutually_exclusive_milestone_args)
+    validate_anonymous_search_access! if args[:search].present?
 
     super
   end

app/graphql/resolvers/concerns/search_arguments.rb

+# frozen_string_literal: true
+
+module SearchArguments
+  extend ActiveSupport::Concern
+
+  included do
+    argument :search, GraphQL::Types::String,
+             required: false,
+             description: 'Search query for title or description.'
+  end
+
+  def validate_anonymous_search_access!
+    return if current_user.present? || Feature.disabled?(:disable_anonymous_search, type: :ops)
+
+    raise ::Gitlab::Graphql::Errors::ArgumentError,
+      "User must be authenticated to include the `search` argument."
+  end
+end

ee/app/graphql/resolvers/epics_resolver.rb

@@ -3,6 +3,7 @@
 module Resolvers
   class EpicsResolver < BaseResolver
     include TimeFrameArguments
+    include SearchArguments
     include LooksAhead
     include SetsMaxPageSize
 
@@ -18,10 +19,6 @@ module Resolvers
              required: false,
              description: 'Filter epics by state.'
 
-    argument :search, GraphQL::Types::String,
-             required: false,
-             description: 'Search query for epic title or description.'
-
     argument :in, [Types::IssuableSearchableFieldEnum],
              required: false,
              description: 'Specify the fields to perform the search in. Defaults to `[TITLE, DESCRIPTION]`. Requires the `search` argument.'
@@ -74,6 +71,7 @@ module Resolvers
       validate_timeframe_params!(args)
       validate_starts_with_iid!(args)
       validate_search_in_params!(args)
+      validate_anonymous_search_access! if args[:search].present?
 
       super(**args)
     end

ee/lib/api/epics.rb

@@ -44,6 +44,7 @@ module API
       end
       [':id/epics', ':id/-/epics'].each do |path|
         get path do
+          validate_anonymous_search_access! if declared_params[:search].present?
           epics = paginate(find_epics(finder_params: { group_id: user_group.id })).with_api_entity_associations
 
           # issuable_metadata has to be set because `Entities::Epic` doesn't inherit from `Entities::IssuableEntity`

ee/spec/graphql/resolvers/epics_resolver_spec.rb

@@ -144,6 +144,35 @@ RSpec.describe Resolvers::EpicsResolver do
             expect(epics).to match_array([epic2, epic3, epic4])
           end
         end
+
+        context 'with anonymous user' do
+          let_it_be(:current_user) { nil }
+
+          context 'with disable_anonymous_search enabled' do
+            before do
+              stub_feature_flags(disable_anonymous_search: true)
+            end
+
+            it 'returns an error' do
+              error_message = "User must be authenticated to include the `search` argument."
+
+              expect { resolve_epics(search: 'created') }
+                .to raise_error(Gitlab::Graphql::Errors::ArgumentError, error_message)
+            end
+          end
+
+          context 'with disable_anonymous_search disabled' do
+            before do
+              stub_feature_flags(disable_anonymous_search: false)
+            end
+
+            it 'filters epics by search term' do
+              epics = resolve_epics(search: 'created')
+
+              expect(epics).to match_array([epic1, epic2])
+            end
+          end
+        end
       end
 
       context 'with author_username' do

ee/spec/requests/api/epics_spec.rb

@@ -199,18 +199,6 @@ RSpec.describe API::Epics do
         expect_paginated_array_response([epic.id])
       end
 
-      it 'returns epics matching given search string for title' do
-        get api(url), params: { search: epic2.title }
-
-        expect_paginated_array_response([epic2.id])
-      end
-
-      it 'returns epics matching given search string for description' do
-        get api(url), params: { search: epic2.description }
-
-        expect_paginated_array_response([epic2.id])
-      end
-
       it 'returns epics matching given status' do
         get api(url, user), params: { state: :opened }
 
@@ -379,6 +367,25 @@ RSpec.describe API::Epics do
         expect_paginated_array_response(epic.id)
       end
 
+      context 'with search param' do
+        it 'returns issues matching given search string for title' do
+          get api(url, user), params: { search: epic2.title }
+
+          expect_paginated_array_response(epic2.id)
+        end
+
+        it 'returns issues matching given search string for description' do
+          get api(url, user), params: { search: epic2.description }
+
+          expect_paginated_array_response(epic2.id)
+        end
+
+        it_behaves_like 'issuable anonymous search' do
+          let(:issuable) { epic2 }
+          let(:result) { issuable.id }
+        end
+      end
+
       describe "#to_reference" do
         it 'exposes reference path' do
           get api(url)

lib/api/helpers.rb

@@ -624,6 +624,12 @@ module API
       {}
     end
 
+    def validate_anonymous_search_access!
+      return if current_user.present? || Feature.disabled?(:disable_anonymous_search, type: :ops)
+
+      unprocessable_entity!('User must be authenticated to use search')
+    end
+
     private
 
     # rubocop:disable Gitlab/ModuleWithInstanceVariables

lib/api/issues.rb

@@ -114,6 +114,7 @@ module API
     end
     get '/issues_statistics' do
       authenticate! unless params[:scope] == 'all'
+      validate_anonymous_search_access! if params[:search].present?
 
       present issues_statistics, with: Grape::Presenters::Presenter
     end
@@ -131,6 +132,7 @@ module API
       end
       get do
         authenticate! unless params[:scope] == 'all'
+        validate_anonymous_search_access! if params[:search].present?
         issues = paginate(find_issues)
 
         options = {
@@ -169,6 +171,7 @@ module API
         optional :non_archived, type: Boolean, desc: 'Return issues from non archived projects', default: true
       end
       get ":id/issues" do
+        validate_anonymous_search_access! if declared_params[:search].present?
         issues = paginate(find_issues(group_id: user_group.id, include_subgroups: true))
 
         options = {
@@ -187,6 +190,8 @@ module API
         use :issues_stats_params
       end
       get ":id/issues_statistics" do
+        validate_anonymous_search_access! if declared_params[:search].present?
+
         present issues_statistics(group_id: user_group.id, include_subgroups: true), with: Grape::Presenters::Presenter
       end
     end
@@ -204,6 +209,7 @@ module API
         use :issues_params
       end
       get ":id/issues" do
+        validate_anonymous_search_access! if declared_params[:search].present?
         issues = paginate(find_issues(project_id: user_project.id))
 
         options = {
@@ -222,6 +228,8 @@ module API
         use :issues_stats_params
       end
       get ":id/issues_statistics" do
+        validate_anonymous_search_access! if declared_params[:search].present?
+
         present issues_statistics(project_id: user_project.id), with: Grape::Presenters::Presenter
       end
 

lib/api/merge_requests.rb

@@ -136,6 +136,7 @@ module API
       end
       get feature_category: :code_review do
         authenticate! unless params[:scope] == 'all'
+        validate_anonymous_search_access! if params[:search].present?
         merge_requests = find_merge_requests
 
         present merge_requests, serializer_options_for(merge_requests)
@@ -155,6 +156,7 @@ module API
         default: true
       end
       get ":id/merge_requests", feature_category: :code_review do
+        validate_anonymous_search_access! if declared_params[:search].present?
         merge_requests = find_merge_requests(group_id: user_group.id, include_subgroups: true)
 
         present merge_requests, serializer_options_for(merge_requests).merge(group: user_group)
@@ -195,6 +197,7 @@ module API
       end
       get ":id/merge_requests", feature_category: :code_review do
         authorize! :read_merge_request, user_project
+        validate_anonymous_search_access! if declared_params[:search].present?
 
         merge_requests = find_merge_requests(project_id: user_project.id)
 

spec/graphql/resolvers/issues_resolver_spec.rb

@@ -236,6 +236,36 @@ RSpec.describe Resolvers::IssuesResolver do
 
           resolve_issues(search: 'foo')
         end
+
+        context 'with anonymous user' do
+          let_it_be(:public_project) { create(:project, :public) }
+          let_it_be(:public_issue) { create(:issue, project: public_project, title: 'Test issue') }
+
+          context 'with disable_anonymous_search enabled' do
+            before do
+              stub_feature_flags(disable_anonymous_search: true)
+            end
+
+            it 'returns an error' do
+              error_message = "User must be authenticated to include the `search` argument."
+
+              expect { resolve(described_class, obj: public_project, args: { search: 'test' }, ctx: { current_user: nil }) }
+                .to raise_error(Gitlab::Graphql::Errors::ArgumentError, error_message)
+            end
+          end
+
+          context 'with disable_anonymous_search disabled' do
+            before do
+              stub_feature_flags(disable_anonymous_search: false)
+            end
+
+            it 'returns correct issues' do
+              expect(
+                resolve(described_class, obj: public_project, args: { search: 'test' }, ctx: { current_user: nil })
+              ).to contain_exactly(public_issue)
+            end
+          end
+        end
       end
 
       describe 'filters by negated params' do

spec/requests/api/issues/issues_spec.rb

@@ -138,6 +138,12 @@ RSpec.describe API::Issues do
         expect(json_response).to be_an Array
       end
 
+      it_behaves_like 'issuable anonymous search' do
+        let(:url) { '/issues' }
+        let(:issuable) { issue }
+        let(:result) { issuable.id }
+      end
+
       it 'returns authentication error without any scope' do
         get api('/issues')
 
@@ -256,6 +262,38 @@ RSpec.describe API::Issues do
 
           it_behaves_like 'issues statistics'
         end
+
+        context 'with search param' do
+          let(:params) { { scope: 'all', search: 'foo' } }
+          let(:counts) { { all: 1, closed: 0, opened: 1 } }
+
+          it_behaves_like 'issues statistics'
+
+          context 'with anonymous user' do
+            let(:user) { nil }
+
+            context 'with disable_anonymous_search disabled' do
+              before do
+                stub_feature_flags(disable_anonymous_search: false)
+              end
+
+              it_behaves_like 'issues statistics'
+            end
+
+            context 'with disable_anonymous_search enabled' do
+              before do
+                stub_feature_flags(disable_anonymous_search: true)
+              end
+
+              it 'returns a unprocessable entity 422' do
+                get api("/issues_statistics"), params: params
+
+                expect(response).to have_gitlab_http_status(:unprocessable_entity)
+                expect(json_response['message']).to include('User must be authenticated to use search')
+              end
+            end
+          end
+        end
       end
     end
 

spec/requests/api/merge_requests_spec.rb

@@ -49,6 +49,12 @@ RSpec.describe API::MergeRequests do
 
         expect_successful_response_with_paginated_array
       end
+
+      it_behaves_like 'issuable anonymous search' do
+        let(:url) { endpoint_path }
+        let(:issuable) { merge_request }
+        let(:result) { [merge_request_merged.id, merge_request_locked.id, merge_request_closed.id, merge_request.id] }
+      end
     end
 
     context 'when authenticated' do
@@ -613,6 +619,12 @@ RSpec.describe API::MergeRequests do
         )
       end
 
+      it_behaves_like 'issuable anonymous search' do
+        let(:url) { '/merge_requests' }
+        let(:issuable) { merge_request }
+        let(:result) { [merge_request_merged.id, merge_request_locked.id, merge_request_closed.id, merge_request.id] }
+      end
+
       it "returns authentication error without any scope" do
         get api("/merge_requests")
 

spec/support/shared_examples/requests/api/issuable_search_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'issuable anonymous search' do
+  context 'with anonymous user' do
+    context 'with disable_anonymous_search disabled' do
+      before do
+        stub_feature_flags(disable_anonymous_search: false)
+      end
+
+      it 'returns issuables matching given search string for title' do
+        get api(url), params: { scope: 'all', search: issuable.title }
+
+        expect_paginated_array_response(result)
+      end
+
+      it 'returns issuables matching given search string for description' do
+        get api(url), params: { scope: 'all', search: issuable.description }
+
+        expect_paginated_array_response(result)
+      end
+    end
+
+    context 'with disable_anonymous_search enabled' do
+      before do
+        stub_feature_flags(disable_anonymous_search: true)
+      end
+
+      it "returns 422 error" do
+        get api(url), params: { scope: 'all', search: issuable.title }
+
+        expect(response).to have_gitlab_http_status(:unprocessable_entity)
+        expect(json_response['message']).to eq('User must be authenticated to use search')
+      end
+    end
+  end
+end