Merge branch 'secure-disallow-read-user-scope-to-read-project-events' into 'master'

Disallow read user scope to read project events

See merge request gitlab/gitlabhq!3067

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