diff options
author | GitLab Release Tools Bot <robert+release-tools@gitlab.com> | 2019-09-26 16:52:54 +0300 |
---|---|---|
committer | GitLab Release Tools Bot <robert+release-tools@gitlab.com> | 2019-09-26 16:52:54 +0300 |
commit | bbab7b62706eeaad91e60643e673a31678887dbc (patch) | |
tree | ecb709229028878693304d9e2d3fae0079682b2a | |
parent | 4be19d5f71e5ab912b9c09a46f356c996e9e0b0b (diff) | |
parent | 2daab029885e01f0c79994f86b01c212e4f7ae88 (diff) |
Merge branch 'security-cross-reference-fix-ce-12-2' into '12-2-stable'
Filter not accessible label events
See merge request gitlab/gitlabhq!3441
-rw-r--r-- | app/finders/resource_label_event_finder.rb | 41 | ||||
-rw-r--r-- | app/models/resource_label_event.rb | 10 | ||||
-rw-r--r-- | app/policies/resource_label_event_policy.rb | 14 | ||||
-rw-r--r-- | changelogs/unreleased/security-cross-reference-fix.yml | 5 | ||||
-rw-r--r-- | lib/api/resource_label_events.rb | 8 | ||||
-rw-r--r-- | spec/finders/resource_label_event_finder_spec.rb | 61 | ||||
-rw-r--r-- | spec/policies/resource_label_event_policy_spec.rb | 67 | ||||
-rw-r--r-- | spec/requests/api/resource_label_events_spec.rb | 11 | ||||
-rw-r--r-- | spec/support/shared_examples/resource_label_events_api.rb | 99 |
9 files changed, 283 insertions, 33 deletions
diff --git a/app/finders/resource_label_event_finder.rb b/app/finders/resource_label_event_finder.rb new file mode 100644 index 00000000000..9aafd6e91b9 --- /dev/null +++ b/app/finders/resource_label_event_finder.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +class ResourceLabelEventFinder + include FinderMethods + + MAX_PER_PAGE = 100 + + attr_reader :params, :current_user, :eventable + + def initialize(current_user, eventable, params = {}) + @current_user = current_user + @eventable = eventable + @params = params + end + + def execute + events = eventable.resource_label_events.inc_relations + events = events.page(page).per(per_page) + events = visible_to_user(events) + + Kaminari.paginate_array(events) + end + + private + + def visible_to_user(events) + ResourceLabelEvent.preload_label_subjects(events) + + events.select do |event| + Ability.allowed?(current_user, :read_label, event) + end + end + + def per_page + [params[:per_page], MAX_PER_PAGE].compact.min + end + + def page + params[:page] || 1 + end +end diff --git a/app/models/resource_label_event.rb b/app/models/resource_label_event.rb index ad08f4763ae..21a167e3888 100644 --- a/app/models/resource_label_event.rb +++ b/app/models/resource_label_event.rb @@ -15,6 +15,7 @@ class ResourceLabelEvent < ApplicationRecord belongs_to :label scope :created_after, ->(time) { where('created_at > ?', time) } + scope :inc_relations, -> { includes(:label, :user) } validates :user, presence: { unless: :importing? }, on: :create validates :label, presence: { unless: :importing? }, on: :create @@ -32,6 +33,15 @@ class ResourceLabelEvent < ApplicationRecord %i(issue merge_request).freeze end + def self.preload_label_subjects(events) + labels = events.map(&:label).compact + project_labels, group_labels = labels.partition { |label| label.is_a? ProjectLabel } + + preloader = ActiveRecord::Associations::Preloader.new + preloader.preload(project_labels, { project: :project_feature }) + preloader.preload(group_labels, :group) + end + def issuable issue || merge_request end diff --git a/app/policies/resource_label_event_policy.rb b/app/policies/resource_label_event_policy.rb new file mode 100644 index 00000000000..de4748d9890 --- /dev/null +++ b/app/policies/resource_label_event_policy.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +class ResourceLabelEventPolicy < BasePolicy + condition(:can_read_label) { @subject.label_id.nil? || can?(:read_label, @subject.label) } + condition(:can_read_issuable) { can?(:"read_#{@subject.issuable.to_ability_name}", @subject.issuable) } + + rule { can_read_label }.policy do + enable :read_label + end + + rule { can_read_label & can_read_issuable }.policy do + enable :read_resource_label_event + end +end diff --git a/changelogs/unreleased/security-cross-reference-fix.yml b/changelogs/unreleased/security-cross-reference-fix.yml new file mode 100644 index 00000000000..15d6509fd63 --- /dev/null +++ b/changelogs/unreleased/security-cross-reference-fix.yml @@ -0,0 +1,5 @@ +--- +title: Do not show resource label events referencing not accessible labels. +merge_request: +author: +type: security diff --git a/lib/api/resource_label_events.rb b/lib/api/resource_label_events.rb index 505a6c68c9c..062115c5103 100644 --- a/lib/api/resource_label_events.rb +++ b/lib/api/resource_label_events.rb @@ -24,14 +24,14 @@ module API use :pagination end - # rubocop: disable CodeReuse/ActiveRecord get ":id/#{eventables_str}/:eventable_id/resource_label_events" do eventable = find_noteable(parent_type, params[:id], eventable_type, params[:eventable_id]) - events = eventable.resource_label_events.includes(:label, :user) + + opts = { page: params[:page], per_page: params[:per_page] } + events = ResourceLabelEventFinder.new(current_user, eventable, opts).execute present paginate(events), with: Entities::ResourceLabelEvent end - # rubocop: enable CodeReuse/ActiveRecord desc "Get a single #{eventable_type.to_s.downcase} resource label event" do success Entities::ResourceLabelEvent @@ -45,6 +45,8 @@ module API eventable = find_noteable(parent_type, params[:id], eventable_type, params[:eventable_id]) event = eventable.resource_label_events.find(params[:event_id]) + not_found!('ResourceLabelEvent') unless can?(current_user, :read_resource_label_event, event) + present event, with: Entities::ResourceLabelEvent end end diff --git a/spec/finders/resource_label_event_finder_spec.rb b/spec/finders/resource_label_event_finder_spec.rb new file mode 100644 index 00000000000..c894387100d --- /dev/null +++ b/spec/finders/resource_label_event_finder_spec.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe ResourceLabelEventFinder do + set(:user) { create(:user) } + set(:issue_project) { create(:project) } + set(:issue) { create(:issue, project: issue_project) } + + describe '#execute' do + subject { described_class.new(user, issue).execute } + + it 'returns events with labels accessible by user' do + label = create(:label, project: issue_project) + event = create_event(label) + issue_project.add_guest(user) + + expect(subject).to eq [event] + end + + it 'filters events with public project labels if issues and MRs are private' do + project = create(:project, :public, :issues_private, :merge_requests_private) + label = create(:label, project: project) + create_event(label) + + expect(subject).to be_empty + end + + it 'filters events with project labels not accessible by user' do + project = create(:project, :private) + label = create(:label, project: project) + create_event(label) + + expect(subject).to be_empty + end + + it 'filters events with group labels not accessible by user' do + group = create(:group, :private) + label = create(:group_label, group: group) + create_event(label) + + expect(subject).to be_empty + end + + it 'paginates results' do + label = create(:label, project: issue_project) + create_event(label) + create_event(label) + issue_project.add_guest(user) + + paginated = described_class.new(user, issue, per_page: 1).execute + + expect(subject.count).to eq 2 + expect(paginated.count).to eq 1 + end + + def create_event(label) + create(:resource_label_event, issue: issue, label: label) + end + end +end diff --git a/spec/policies/resource_label_event_policy_spec.rb b/spec/policies/resource_label_event_policy_spec.rb new file mode 100644 index 00000000000..9206640ea00 --- /dev/null +++ b/spec/policies/resource_label_event_policy_spec.rb @@ -0,0 +1,67 @@ +require 'spec_helper' + +describe ResourceLabelEventPolicy do + set(:user) { create(:user) } + set(:project) { create(:project, :private) } + set(:issue) { create(:issue, project: project) } + set(:private_project) { create(:project, :private) } + + describe '#read_resource_label_event' do + context 'with non-member user' do + it 'does not allow to read event' do + event = build_event(project) + + expect(permissions(user, event)).to be_disallowed(:read_resource_label_event) + end + end + + context 'with member user' do + before do + project.add_guest(user) + end + + it 'allows to read event for accessible label' do + event = build_event(project) + + expect(permissions(user, event)).to be_allowed(:read_resource_label_event) + end + + it 'does not allow to read event for not accessible label' do + event = build_event(private_project) + + expect(permissions(user, event)).to be_disallowed(:read_resource_label_event) + end + end + end + + describe '#read_label' do + it 'allows to read deleted label' do + event = build(:resource_label_event, issue: issue, label: nil) + + expect(permissions(user, event)).to be_allowed(:read_label) + end + + it 'allows to read accessible label' do + project.add_guest(user) + event = build_event(project) + + expect(permissions(user, event)).to be_allowed(:read_label) + end + + it 'does not allow to read not accessible label' do + event = build_event(private_project) + + expect(permissions(user, event)).to be_disallowed(:read_label) + end + end + + def build_event(label_project) + label = create(:label, project: label_project) + + build(:resource_label_event, issue: issue, label: label) + end + + def permissions(user, issue) + described_class.new(user, issue) + end +end diff --git a/spec/requests/api/resource_label_events_spec.rb b/spec/requests/api/resource_label_events_spec.rb index 25bea627b0c..8bac378787c 100644 --- a/spec/requests/api/resource_label_events_spec.rb +++ b/spec/requests/api/resource_label_events_spec.rb @@ -5,28 +5,23 @@ require 'spec_helper' describe API::ResourceLabelEvents do set(:user) { create(:user) } set(:project) { create(:project, :public, namespace: user.namespace) } + set(:label) { create(:label, project: project) } before do project.add_developer(user) end context 'when eventable is an Issue' do - let(:issue) { create(:issue, project: project, author: user) } - it_behaves_like 'resource_label_events API', 'projects', 'issues', 'iid' do let(:parent) { project } - let(:eventable) { issue } - let!(:event) { create(:resource_label_event, issue: issue) } + let(:eventable) { create(:issue, project: project, author: user) } end end context 'when eventable is a Merge Request' do - let(:merge_request) { create(:merge_request, source_project: project, target_project: project, author: user) } - it_behaves_like 'resource_label_events API', 'projects', 'merge_requests', 'iid' do let(:parent) { project } - let(:eventable) { merge_request } - let!(:event) { create(:resource_label_event, merge_request: merge_request) } + let(:eventable) { create(:merge_request, source_project: project, target_project: project, author: user) } end end end diff --git a/spec/support/shared_examples/resource_label_events_api.rb b/spec/support/shared_examples/resource_label_events_api.rb index 945cb8d9f2c..6622df78ee2 100644 --- a/spec/support/shared_examples/resource_label_events_api.rb +++ b/spec/support/shared_examples/resource_label_events_api.rb @@ -2,43 +2,98 @@ shared_examples 'resource_label_events API' do |parent_type, eventable_type, id_name| describe "GET /#{parent_type}/:id/#{eventable_type}/:noteable_id/resource_label_events" do - it "returns an array of resource label events" do - get api("/#{parent_type}/#{parent.id}/#{eventable_type}/#{eventable[id_name]}/resource_label_events", user) + context "with local label reference" do + let!(:event) { create_event(label) } - expect(response).to have_gitlab_http_status(200) - expect(response).to include_pagination_headers - expect(json_response).to be_an Array - expect(json_response.first['id']).to eq(event.id) - end + it "returns an array of resource label events" do + get api("/#{parent_type}/#{parent.id}/#{eventable_type}/#{eventable[id_name]}/resource_label_events", 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.first['id']).to eq(event.id) + end + + it "returns a 404 error when eventable id not found" do + get api("/#{parent_type}/#{parent.id}/#{eventable_type}/12345/resource_label_events", user) + + expect(response).to have_gitlab_http_status(404) + end + + it "returns 404 when not authorized" do + parent.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE) + private_user = create(:user) - it "returns a 404 error when eventable id not found" do - get api("/#{parent_type}/#{parent.id}/#{eventable_type}/12345/resource_label_events", user) + get api("/#{parent_type}/#{parent.id}/#{eventable_type}/#{eventable[id_name]}/resource_label_events", private_user) - expect(response).to have_gitlab_http_status(404) + expect(response).to have_gitlab_http_status(404) + end end - it "returns 404 when not authorized" do - parent.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE) - private_user = create(:user) + context "with cross-project label reference" do + let(:private_project) { create(:project, :private) } + let(:project_label) { create(:label, project: private_project) } + let!(:event) { create_event(project_label) } - get api("/#{parent_type}/#{parent.id}/#{eventable_type}/#{eventable[id_name]}/resource_label_events", private_user) + it "returns cross references accessible by user" do + private_project.add_guest(user) - expect(response).to have_gitlab_http_status(404) + get api("/#{parent_type}/#{parent.id}/#{eventable_type}/#{eventable[id_name]}/resource_label_events", user) + + expect(json_response).to be_an Array + expect(json_response.first['id']).to eq(event.id) + end + + it "does not return cross references not accessible by user" do + get api("/#{parent_type}/#{parent.id}/#{eventable_type}/#{eventable[id_name]}/resource_label_events", user) + + expect(json_response).to be_an Array + expect(json_response).to eq [] + end end end describe "GET /#{parent_type}/:id/#{eventable_type}/:noteable_id/resource_label_events/:event_id" do - it "returns a resource label event by id" do - get api("/#{parent_type}/#{parent.id}/#{eventable_type}/#{eventable[id_name]}/resource_label_events/#{event.id}", user) + context "with local label reference" do + let!(:event) { create_event(label) } + + it "returns a resource label event by id" do + get api("/#{parent_type}/#{parent.id}/#{eventable_type}/#{eventable[id_name]}/resource_label_events/#{event.id}", user) - expect(response).to have_gitlab_http_status(200) - expect(json_response['id']).to eq(event.id) + expect(response).to have_gitlab_http_status(200) + expect(json_response['id']).to eq(event.id) + end + + it "returns 404 when not authorized" do + parent.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE) + private_user = create(:user) + + get api("/#{parent_type}/#{parent.id}/#{eventable_type}/#{eventable[id_name]}/resource_label_events/#{event.id}", private_user) + + expect(response).to have_gitlab_http_status(404) + end + + it "returns a 404 error if resource label event not found" do + get api("/#{parent_type}/#{parent.id}/#{eventable_type}/#{eventable[id_name]}/resource_label_events/12345", user) + + expect(response).to have_gitlab_http_status(404) + end end - it "returns a 404 error if resource label event not found" do - get api("/#{parent_type}/#{parent.id}/#{eventable_type}/#{eventable[id_name]}/resource_label_events/12345", user) + context "with cross-project label reference" do + let(:private_project) { create(:project, :private) } + let(:project_label) { create(:label, project: private_project) } + let!(:event) { create_event(project_label) } + + it "returns a 404 error if cross-reference project is not accessible" do + get api("/#{parent_type}/#{parent.id}/#{eventable_type}/#{eventable[id_name]}/resource_label_events/#{event.id}", user) - expect(response).to have_gitlab_http_status(404) + expect(response).to have_gitlab_http_status(404) + end end end + + def create_event(label) + create(:resource_label_event, eventable.class.name.underscore => eventable, label: label) + end end |