diff --git a/changelogs/unreleased/security-id-restricted-access-to-private-repo.yml b/changelogs/unreleased/security-id-restricted-access-to-private-repo.yml new file mode 100644 index 0000000000000000000000000000000000000000..7d7478d297b8322b8c6efaf92c4cdd29f00066bc --- /dev/null +++ b/changelogs/unreleased/security-id-restricted-access-to-private-repo.yml @@ -0,0 +1,5 @@ +--- +title: Forbid creating discussions for users with restricted access +merge_request: +author: +type: security diff --git a/lib/api/helpers/notes_helpers.rb b/lib/api/helpers/notes_helpers.rb index 216b2c457419a338b974e5f9f43b99493f75de3d..795dca5cf0300ddfd8c36ac82796e8aa03e6eb39 100644 --- a/lib/api/helpers/notes_helpers.rb +++ b/lib/api/helpers/notes_helpers.rb @@ -70,14 +70,7 @@ def noteable_read_ability_name(noteable) def find_noteable(parent, noteables_str, noteable_id) noteable = public_send("find_#{parent}_#{noteables_str.singularize}", noteable_id) # rubocop:disable GitlabSecurity/PublicSend - readable = - if noteable.is_a?(Commit) - # for commits there is not :read_commit policy, check if user - # has :read_note permission on the commit's project - can?(current_user, :read_note, user_project) - else - can?(current_user, noteable_read_ability_name(noteable), noteable) - end + readable = can?(current_user, noteable_read_ability_name(noteable), noteable) return not_found!(noteables_str) unless readable @@ -89,12 +82,11 @@ def noteable_parent(noteable) end def create_note(noteable, opts) - policy_object = noteable.is_a?(Commit) ? user_project : noteable - authorize!(:create_note, policy_object) + authorize!(:create_note, noteable) parent = noteable_parent(noteable) - opts.delete(:created_at) unless current_user.can?(:set_note_created_at, policy_object) + opts.delete(:created_at) unless current_user.can?(:set_note_created_at, noteable) opts[:updated_at] = opts[:created_at] if opts[:created_at] diff --git a/spec/policies/commit_policy_spec.rb b/spec/policies/commit_policy_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..2259693cf013e00ec490c0908753bb4fc05d362b --- /dev/null +++ b/spec/policies/commit_policy_spec.rb @@ -0,0 +1,53 @@ +require 'spec_helper' + +describe CommitPolicy do + describe '#rules' do + let(:user) { create(:user) } + let(:commit) { project.repository.head_commit } + let(:policy) { described_class.new(user, commit) } + + context 'when project is public' do + let(:project) { create(:project, :public, :repository) } + + it 'can read commit and create a note' do + expect(policy).to be_allowed(:read_commit) + end + + context 'when repository access level is private' do + let(:project) { create(:project, :public, :repository, :repository_private) } + + it 'can not read commit and create a note' do + expect(policy).to be_disallowed(:read_commit) + end + + context 'when the user is a project member' do + before do + project.add_developer(user) + end + + it 'can read commit and create a note' do + expect(policy).to be_allowed(:read_commit) + end + end + end + end + + context 'when project is private' do + let(:project) { create(:project, :private, :repository) } + + it 'can not read commit and create a note' do + expect(policy).to be_disallowed(:read_commit) + end + + context 'when the user is a project member' do + before do + project.add_developer(user) + end + + it 'can read commit and create a note' do + expect(policy).to be_allowed(:read_commit) + end + end + end + end +end diff --git a/spec/policies/note_policy_spec.rb b/spec/policies/note_policy_spec.rb index 0e848c74659f75f8c8e5efc2eefa86ca0a25de21..4be7a0266d1a318e6351a8343d99bcdab05a392a 100644 --- a/spec/policies/note_policy_spec.rb +++ b/spec/policies/note_policy_spec.rb @@ -1,28 +1,15 @@ require 'spec_helper' -describe NotePolicy, mdoels: true do +describe NotePolicy do describe '#rules' do let(:user) { create(:user) } let(:project) { create(:project, :public) } let(:issue) { create(:issue, project: project) } - - def policies(noteable = nil) - return @policies if @policies - - noteable ||= issue - note = if noteable.is_a?(Commit) - create(:note_on_commit, commit_id: noteable.id, author: user, project: project) - else - create(:note, noteable: noteable, author: user, project: project) - end - - @policies = described_class.new(user, note) - end + let(:noteable) { issue } + let(:policy) { described_class.new(user, note) } + let(:note) { create(:note, noteable: noteable, author: user, project: project) } shared_examples_for 'a discussion with a private noteable' do - let(:noteable) { issue } - let(:policy) { policies(noteable) } - context 'when the note author can no longer see the noteable' do it 'can not edit nor read the note' do expect(policy).to be_disallowed(:admin_note) @@ -46,12 +33,21 @@ def policies(noteable = nil) end end - context 'when the project is private' do - let(:project) { create(:project, :private, :repository) } + context 'when the noteable is a commit' do + let(:commit) { project.repository.head_commit } + let(:note) { create(:note_on_commit, commit_id: commit.id, author: user, project: project) } + + context 'when the project is private' do + let(:project) { create(:project, :private, :repository) } + + it_behaves_like 'a discussion with a private noteable' + end - context 'when the noteable is a commit' do - it_behaves_like 'a discussion with a private noteable' do - let(:noteable) { project.repository.head_commit } + context 'when the project is public' do + context 'when repository access level is private' do + let(:project) { create(:project, :public, :repository, :repository_private) } + + it_behaves_like 'a discussion with a private noteable' end end end @@ -59,44 +55,44 @@ def policies(noteable = nil) context 'when the project is public' do context 'when the note author is not a project member' do it 'can edit a note' do - expect(policies).to be_allowed(:admin_note) - expect(policies).to be_allowed(:resolve_note) - expect(policies).to be_allowed(:read_note) + expect(policy).to be_allowed(:admin_note) + expect(policy).to be_allowed(:resolve_note) + expect(policy).to be_allowed(:read_note) end end context 'when the noteable is a project snippet' do - it 'can edit note' do - policies = policies(create(:project_snippet, :public, project: project)) + let(:noteable) { create(:project_snippet, :public, project: project) } - expect(policies).to be_allowed(:admin_note) - expect(policies).to be_allowed(:resolve_note) - expect(policies).to be_allowed(:read_note) + it 'can edit note' do + expect(policy).to be_allowed(:admin_note) + expect(policy).to be_allowed(:resolve_note) + expect(policy).to be_allowed(:read_note) end context 'when it is private' do - it_behaves_like 'a discussion with a private noteable' do - let(:noteable) { create(:project_snippet, :private, project: project) } - end + let(:noteable) { create(:project_snippet, :private, project: project) } + + it_behaves_like 'a discussion with a private noteable' end end context 'when the noteable is a personal snippet' do - it 'can edit note' do - policies = policies(create(:personal_snippet, :public)) + let(:noteable) { create(:personal_snippet, :public) } - expect(policies).to be_allowed(:admin_note) - expect(policies).to be_allowed(:resolve_note) - expect(policies).to be_allowed(:read_note) + it 'can edit note' do + expect(policy).to be_allowed(:admin_note) + expect(policy).to be_allowed(:resolve_note) + expect(policy).to be_allowed(:read_note) end context 'when it is private' do - it 'can not edit nor read the note' do - policies = policies(create(:personal_snippet, :private)) + let(:noteable) { create(:personal_snippet, :private) } - expect(policies).to be_disallowed(:admin_note) - expect(policies).to be_disallowed(:resolve_note) - expect(policies).to be_disallowed(:read_note) + it 'can not edit nor read the note' do + expect(policy).to be_disallowed(:admin_note) + expect(policy).to be_disallowed(:resolve_note) + expect(policy).to be_disallowed(:read_note) end end end @@ -120,20 +116,20 @@ def policies(noteable = nil) end it 'can edit a note' do - expect(policies).to be_allowed(:admin_note) - expect(policies).to be_allowed(:resolve_note) - expect(policies).to be_allowed(:read_note) + expect(policy).to be_allowed(:admin_note) + expect(policy).to be_allowed(:resolve_note) + expect(policy).to be_allowed(:read_note) end end context 'when the note author is not a project member' do it 'can not edit a note' do - expect(policies).to be_disallowed(:admin_note) - expect(policies).to be_disallowed(:resolve_note) + expect(policy).to be_disallowed(:admin_note) + expect(policy).to be_disallowed(:resolve_note) end it 'can read a note' do - expect(policies).to be_allowed(:read_note) + expect(policy).to be_allowed(:read_note) end end end diff --git a/spec/support/shared_examples/requests/api/discussions.rb b/spec/support/shared_examples/requests/api/discussions.rb index e44da4faa5af10b5951b17a45f37c235d2d336bb..eff8e401bad4c8715c787d907b56d508216ed31e 100644 --- a/spec/support/shared_examples/requests/api/discussions.rb +++ b/spec/support/shared_examples/requests/api/discussions.rb @@ -86,6 +86,37 @@ expect(response).to have_gitlab_http_status(404) end end + + context 'when a project is public with private repo access' do + let!(:parent) { create(:project, :public, :repository, :repository_private, :snippets_private) } + let!(:user_without_access) { create(:user) } + + context 'when user is not a team member of private repo' do + before do + project.team.truncate + end + + context "creating a new note" do + before do + post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/discussions", user_without_access), params: { body: 'hi!' } + end + + it 'raises 404 error' do + expect(response).to have_gitlab_http_status(404) + end + end + + context "fetching a discussion" do + before do + get api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/discussions/#{note.discussion_id}", user_without_access) + end + + it 'raises 404 error' do + expect(response).to have_gitlab_http_status(404) + end + end + end + end end describe "POST /#{parent_type}/:id/#{noteable_type}/:noteable_id/discussions/:discussion_id/notes" do