Add new api class for projects events

Refactor api events class to use external helper

Move specs from old class

Add changelog and magic string

Refactor events class to be more explicit

Remove blank line

changelogs/unreleased/secure-disallow-read-user-scope-to-read-project-events.yml

+---
+title: Allow to see project events only with api scope token
+merge_request:
+author:
+type: security

lib/api/api.rb

@@ -134,6 +134,7 @@ module API
     mount ::API::Pipelines
     mount ::API::PipelineSchedules
     mount ::API::ProjectClusters
+    mount ::API::ProjectEvents
     mount ::API::ProjectExport
     mount ::API::ProjectImport
     mount ::API::ProjectHooks

lib/api/events.rb

@@ -4,34 +4,11 @@ module API
   class Events < Grape::API
     include PaginationParams
     include APIGuard
+    helpers ::API::Helpers::EventsHelpers
 
-    helpers do
-      params :event_filter_params do
-        optional :action, type: String, values: Event.actions, desc: 'Event action to filter on'
-        optional :target_type, type: String, values: Event.target_types, desc: 'Event target type to filter on'
-        optional :before, type: Date, desc: 'Include only events created before this date'
-        optional :after, type: Date, desc: 'Include only events created after this date'
-      end
-
-      params :sort_params do
-        optional :sort, type: String, values: %w[asc desc], default: 'desc',
-                        desc: 'Return events sorted in ascending and descending order'
-      end
-
-      def present_events(events)
-        events = paginate(events)
-
-        present events, with: Entities::Event
-      end
-
-      def find_events(source)
-        EventsFinder.new(params.merge(source: source, current_user: current_user, with_associations: true)).execute
-      end
-    end
-
-    resource :events do
     allow_access_with_scope :read_user, if: -> (request) { request.get? }
 
+    resource :events do
       desc "List currently authenticated user's events" do
         detail 'This feature was introduced in GitLab 9.3.'
         success Entities::Event
@@ -55,8 +32,6 @@ module API
       requires :id, type: String, desc: 'The ID or Username of the user'
     end
     resource :users do
-      allow_access_with_scope :read_user, if: -> (request) { request.get? }
-
       desc 'Get the contribution events of a specified user' do
         detail 'This feature was introduced in GitLab 8.13.'
         success Entities::Event
@@ -76,25 +51,5 @@ module API
         present_events(events)
       end
     end
-
-    params do
-      requires :id, type: String, desc: 'The ID of a project'
-    end
-    resource :projects, requirements: API::NAMESPACE_OR_PROJECT_REQUIREMENTS do
-      desc "List a Project's visible events" do
-        success Entities::Event
-      end
-      params do
-        use :pagination
-        use :event_filter_params
-        use :sort_params
-      end
-
-      get ":id/events" do
-        events = find_events(user_project)
-
-        present_events(events)
-      end
-    end
   end
 end

lib/api/helpers/events_helpers.rb

+# frozen_string_literal: true
+
+module API
+  module Helpers
+    module EventsHelpers
+      extend Grape::API::Helpers
+
+      params :event_filter_params do
+        optional :action, type: String, values: Event.actions, desc: 'Event action to filter on'
+        optional :target_type, type: String, values: Event.target_types, desc: 'Event target type to filter on'
+        optional :before, type: Date, desc: 'Include only events created before this date'
+        optional :after, type: Date, desc: 'Include only events created after this date'
+      end
+
+      params :sort_params do
+        optional :sort, type: String, values: %w[asc desc], default: 'desc',
+                        desc: 'Return events sorted in ascending and descending order'
+      end
+
+      def present_events(events)
+        events = paginate(events)
+
+        present events, with: Entities::Event
+      end
+
+      def find_events(source)
+        EventsFinder.new(params.merge(source: source, current_user: current_user, with_associations: true)).execute
+      end
+    end
+  end
+end

lib/api/project_events.rb

+# frozen_string_literal: true
+
+module API
+  class ProjectEvents < Grape::API
+    include PaginationParams
+    include APIGuard
+    helpers ::API::Helpers::EventsHelpers
+
+    params do
+      requires :id, type: String, desc: 'The ID of a project'
+    end
+    resource :projects, requirements: API::NAMESPACE_OR_PROJECT_REQUIREMENTS do
+      desc "List a Project's visible events" do
+        success Entities::Event
+      end
+      params do
+        use :pagination
+        use :event_filter_params
+        use :sort_params
+      end
+
+      get ":id/events" do
+        events = find_events(user_project)
+
+        present_events(events)
+      end
+    end
+  end
+end

spec/requests/api/events_spec.rb

@@ -164,139 +164,4 @@ describe API::Events do
       expect(json_response['message']).to eq('404 User Not Found')
     end
   end
-
-  describe 'GET /projects/:id/events' do
-    context 'when unauthenticated ' do
-      it 'returns 404 for private project' do
-        get api("/projects/#{private_project.id}/events")
-
-        expect(response).to have_gitlab_http_status(404)
-      end
-
-      it 'returns 200 status for a public project' do
-        public_project = create(:project, :public)
-
-        get api("/projects/#{public_project.id}/events")
-
-        expect(response).to have_gitlab_http_status(200)
-      end
-    end
-
-    context 'with inaccessible events' do
-      let(:public_project) { create(:project, :public, creator_id: user.id, namespace: user.namespace) }
-      let(:confidential_issue) { create(:closed_issue, confidential: true, project: public_project, author: user) }
-      let!(:confidential_event) { create(:event, project: public_project, author: user, target: confidential_issue, action: Event::CLOSED) }
-      let(:public_issue) { create(:closed_issue, project: public_project, author: user) }
-      let!(:public_event) { create(:event, project: public_project, author: user, target: public_issue, action: Event::CLOSED) }
-
-      it 'returns only accessible events' do
-        get api("/projects/#{public_project.id}/events", non_member)
-
-        expect(response).to have_gitlab_http_status(200)
-        expect(json_response.size).to eq(1)
-      end
-
-      it 'returns all events when the user has access' do
-        get api("/projects/#{public_project.id}/events", user)
-
-        expect(response).to have_gitlab_http_status(200)
-        expect(json_response.size).to eq(2)
-      end
-    end
-
-    context 'pagination' do
-      let(:public_project) { create(:project, :public) }
-
-      before do
-        create(:event,
-               project: public_project,
-               target: create(:issue, project: public_project, title: 'Issue 1'),
-               action: Event::CLOSED,
-               created_at: Date.parse('2018-12-10'))
-        create(:event,
-               project: public_project,
-               target: create(:issue, confidential: true, project: public_project, title: 'Confidential event'),
-               action: Event::CLOSED,
-               created_at: Date.parse('2018-12-11'))
-        create(:event,
-               project: public_project,
-               target: create(:issue, project: public_project, title: 'Issue 2'),
-               action: Event::CLOSED,
-               created_at: Date.parse('2018-12-12'))
-      end
-
-      it 'correctly returns the second page without inaccessible events' do
-        get api("/projects/#{public_project.id}/events", user), params: { per_page: 2, page: 2 }
-
-        titles = json_response.map { |event| event['target_title'] }
-
-        expect(titles.first).to eq('Issue 1')
-        expect(titles).not_to include('Confidential event')
-      end
-
-      it 'correctly returns the first page without inaccessible events' do
-        get api("/projects/#{public_project.id}/events", user), params: { per_page: 2, page: 1 }
-
-        titles = json_response.map { |event| event['target_title'] }
-
-        expect(titles.first).to eq('Issue 2')
-        expect(titles).not_to include('Confidential event')
-      end
-    end
-
-    context 'when not permitted to read' do
-      it 'returns 404' do
-        get api("/projects/#{private_project.id}/events", non_member)
-
-        expect(response).to have_gitlab_http_status(404)
-      end
-    end
-
-    context 'when authenticated' do
-      it 'returns project events' do
-        get api("/projects/#{private_project.id}/events?action=closed&target_type=issue&after=2016-12-1&before=2016-12-31", user)
-
-        expect(response).to have_gitlab_http_status(200)
-        expect(response).to include_pagination_headers
-        expect(json_response).to be_an Array
-        expect(json_response.size).to eq(1)
-      end
-
-      it 'returns 404 if project does not exist' do
-        get api("/projects/1234/events", user)
-
-        expect(response).to have_gitlab_http_status(404)
-      end
-    end
-
-    context 'when exists some events' do
-      let(:merge_request1) { create(:merge_request, :closed, author: user, assignees: [user], source_project: private_project, title: 'Test') }
-      let(:merge_request2) { create(:merge_request, :closed, author: user, assignees: [user], source_project: private_project, title: 'Test') }
-
-      before do
-        create_event(merge_request1)
-      end
-
-      it 'avoids N+1 queries' do
-        control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) do
-          get api("/projects/#{private_project.id}/events", user), params: { target_type: :merge_request }
-        end.count
-
-        create_event(merge_request2)
-
-        expect do
-          get api("/projects/#{private_project.id}/events", user), params: { target_type: :merge_request }
-        end.not_to exceed_all_query_limit(control_count)
-
-        expect(response).to have_gitlab_http_status(200)
-        expect(response).to include_pagination_headers
-        expect(json_response.size).to eq(2)
-        expect(json_response.map { |r| r['target_id'] }).to match_array([merge_request1.id, merge_request2.id])
-      end
-
-      def create_event(target)
-        create(:event, project: private_project, author: user, target: target)
-      end
-    end
-  end
 end

spec/requests/api/project_events_spec.rb

+require 'spec_helper'
+
+describe API::ProjectEvents do
+  include ApiHelpers
+
+  let(:user) { create(:user) }
+  let(:non_member) { create(:user) }
+  let(:private_project) { create(:project, :private, creator_id: user.id, namespace: user.namespace) }
+  let(:closed_issue) { create(:closed_issue, project: private_project, author: user) }
+  let!(:closed_issue_event) { create(:event, project: private_project, author: user, target: closed_issue, action: Event::CLOSED, created_at: Date.new(2016, 12, 30)) }
+
+  describe 'GET /projects/:id/events' do
+    context 'when unauthenticated ' do
+      it 'returns 404 for private project' do
+        get api("/projects/#{private_project.id}/events")
+
+        expect(response).to have_gitlab_http_status(404)
+      end
+
+      it 'returns 200 status for a public project' do
+        public_project = create(:project, :public)
+
+        get api("/projects/#{public_project.id}/events")
+
+        expect(response).to have_gitlab_http_status(200)
+      end
+    end
+
+    context 'with inaccessible events' do
+      let(:public_project) { create(:project, :public, creator_id: user.id, namespace: user.namespace) }
+      let(:confidential_issue) { create(:closed_issue, confidential: true, project: public_project, author: user) }
+      let!(:confidential_event) { create(:event, project: public_project, author: user, target: confidential_issue, action: Event::CLOSED) }
+      let(:public_issue) { create(:closed_issue, project: public_project, author: user) }
+      let!(:public_event) { create(:event, project: public_project, author: user, target: public_issue, action: Event::CLOSED) }
+
+      it 'returns only accessible events' do
+        get api("/projects/#{public_project.id}/events", non_member)
+
+        expect(response).to have_gitlab_http_status(200)
+        expect(json_response.size).to eq(1)
+      end
+
+      it 'returns all events when the user has access' do
+        get api("/projects/#{public_project.id}/events", user)
+
+        expect(response).to have_gitlab_http_status(200)
+        expect(json_response.size).to eq(2)
+      end
+    end
+
+    context 'pagination' do
+      let(:public_project) { create(:project, :public) }
+
+      before do
+        create(:event,
+               project: public_project,
+               target: create(:issue, project: public_project, title: 'Issue 1'),
+               action: Event::CLOSED,
+               created_at: Date.parse('2018-12-10'))
+        create(:event,
+               project: public_project,
+               target: create(:issue, confidential: true, project: public_project, title: 'Confidential event'),
+               action: Event::CLOSED,
+               created_at: Date.parse('2018-12-11'))
+        create(:event,
+               project: public_project,
+               target: create(:issue, project: public_project, title: 'Issue 2'),
+               action: Event::CLOSED,
+               created_at: Date.parse('2018-12-12'))
+      end
+
+      it 'correctly returns the second page without inaccessible events' do
+        get api("/projects/#{public_project.id}/events", user), params: { per_page: 2, page: 2 }
+
+        titles = json_response.map { |event| event['target_title'] }
+
+        expect(titles.first).to eq('Issue 1')
+        expect(titles).not_to include('Confidential event')
+      end
+
+      it 'correctly returns the first page without inaccessible events' do
+        get api("/projects/#{public_project.id}/events", user), params: { per_page: 2, page: 1 }
+
+        titles = json_response.map { |event| event['target_title'] }
+
+        expect(titles.first).to eq('Issue 2')
+        expect(titles).not_to include('Confidential event')
+      end
+    end
+
+    context 'when not permitted to read' do
+      it 'returns 404' do
+        get api("/projects/#{private_project.id}/events", non_member)
+
+        expect(response).to have_gitlab_http_status(404)
+      end
+    end
+
+    context 'when authenticated' do
+      it 'returns project events' do
+        get api("/projects/#{private_project.id}/events?action=closed&target_type=issue&after=2016-12-1&before=2016-12-31", user)
+
+        expect(response).to have_gitlab_http_status(200)
+        expect(response).to include_pagination_headers
+        expect(json_response).to be_an Array
+        expect(json_response.size).to eq(1)
+      end
+
+      it 'returns 404 if project does not exist' do
+        get api("/projects/1234/events", user)
+
+        expect(response).to have_gitlab_http_status(404)
+      end
+
+      context 'when the requesting token does not have "api" scope' do
+        let(:token) { create(:personal_access_token, scopes: ['read_user'], user: user) }
+
+        it 'returns a "403" response' do
+          get api("/projects/#{private_project.id}/events", personal_access_token: token)
+
+          expect(response).to have_gitlab_http_status(403)
+        end
+      end
+    end
+
+    context 'when exists some events' do
+      let(:merge_request1) { create(:merge_request, :closed, author: user, assignees: [user], source_project: private_project, title: 'Test') }
+      let(:merge_request2) { create(:merge_request, :closed, author: user, assignees: [user], source_project: private_project, title: 'Test') }
+
+      before do
+        create_event(merge_request1)
+      end
+
+      it 'avoids N+1 queries' do
+        control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) do
+          get api("/projects/#{private_project.id}/events", user), params: { target_type: :merge_request }
+        end.count
+
+        create_event(merge_request2)
+
+        expect do
+          get api("/projects/#{private_project.id}/events", user), params: { target_type: :merge_request }
+        end.not_to exceed_all_query_limit(control_count)
+
+        expect(response).to have_gitlab_http_status(200)
+        expect(response).to include_pagination_headers
+        expect(json_response.size).to eq(2)
+        expect(json_response.map { |r| r['target_id'] }).to match_array([merge_request1.id, merge_request2.id])
+      end
+
+      def create_event(target)
+        create(:event, project: private_project, author: user, target: target)
+      end
+    end
+  end
+end