Commit a43fd6ac authored by Yorick Peterse's avatar Yorick Peterse

Merge branch 'security-id-restricted-access-to-private-repo' into 'master'

Forbid creating discussions for users with restricted access

Closes #2788

See merge request gitlab/gitlabhq!2868
parents 9faf957b 5169dafc
---
title: Forbid creating discussions for users with restricted access
merge_request:
author:
type: security
...@@ -70,14 +70,7 @@ module API ...@@ -70,14 +70,7 @@ module API
def find_noteable(parent, noteables_str, noteable_id) def find_noteable(parent, noteables_str, noteable_id)
noteable = public_send("find_#{parent}_#{noteables_str.singularize}", noteable_id) # rubocop:disable GitlabSecurity/PublicSend noteable = public_send("find_#{parent}_#{noteables_str.singularize}", noteable_id) # rubocop:disable GitlabSecurity/PublicSend
readable = readable = can?(current_user, noteable_read_ability_name(noteable), noteable)
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
return not_found!(noteables_str) unless readable return not_found!(noteables_str) unless readable
...@@ -89,12 +82,11 @@ module API ...@@ -89,12 +82,11 @@ module API
end end
def create_note(noteable, opts) def create_note(noteable, opts)
policy_object = noteable.is_a?(Commit) ? user_project : noteable authorize!(:create_note, noteable)
authorize!(:create_note, policy_object)
parent = noteable_parent(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] opts[:updated_at] = opts[:created_at] if opts[:created_at]
......
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
require 'spec_helper' require 'spec_helper'
describe NotePolicy, mdoels: true do describe NotePolicy do
describe '#rules' do describe '#rules' do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:project) { create(:project, :public) } let(:project) { create(:project, :public) }
let(:issue) { create(:issue, project: project) } 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
shared_examples_for 'a discussion with a private noteable' do
let(:noteable) { issue } let(:noteable) { issue }
let(:policy) { policies(noteable) } 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
context 'when the note author can no longer see the noteable' do context 'when the note author can no longer see the noteable' do
it 'can not edit nor read the note' do it 'can not edit nor read the note' do
expect(policy).to be_disallowed(:admin_note) expect(policy).to be_disallowed(:admin_note)
...@@ -46,12 +33,21 @@ describe NotePolicy, mdoels: true do ...@@ -46,12 +33,21 @@ describe NotePolicy, mdoels: true do
end end
end end
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 context 'when the project is private' do
let(:project) { create(:project, :private, :repository) } let(:project) { create(:project, :private, :repository) }
context 'when the noteable is a commit' do it_behaves_like 'a discussion with a private noteable'
it_behaves_like 'a discussion with a private noteable' do end
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 end
end end
...@@ -59,44 +55,44 @@ describe NotePolicy, mdoels: true do ...@@ -59,44 +55,44 @@ describe NotePolicy, mdoels: true do
context 'when the project is public' do context 'when the project is public' do
context 'when the note author is not a project member' do context 'when the note author is not a project member' do
it 'can edit a note' do it 'can edit a note' do
expect(policies).to be_allowed(:admin_note) expect(policy).to be_allowed(:admin_note)
expect(policies).to be_allowed(:resolve_note) expect(policy).to be_allowed(:resolve_note)
expect(policies).to be_allowed(:read_note) expect(policy).to be_allowed(:read_note)
end end
end end
context 'when the noteable is a project snippet' do context 'when the noteable is a project snippet' do
it 'can edit note' do let(:noteable) { create(:project_snippet, :public, project: project) }
policies = policies(create(:project_snippet, :public, project: project))
expect(policies).to be_allowed(:admin_note) it 'can edit note' do
expect(policies).to be_allowed(:resolve_note) expect(policy).to be_allowed(:admin_note)
expect(policies).to be_allowed(:read_note) expect(policy).to be_allowed(:resolve_note)
expect(policy).to be_allowed(:read_note)
end end
context 'when it is private' do context 'when it is private' do
it_behaves_like 'a discussion with a private noteable' do
let(:noteable) { create(:project_snippet, :private, project: project) } let(:noteable) { create(:project_snippet, :private, project: project) }
end
it_behaves_like 'a discussion with a private noteable'
end end
end end
context 'when the noteable is a personal snippet' do context 'when the noteable is a personal snippet' do
it 'can edit note' do let(:noteable) { create(:personal_snippet, :public) }
policies = policies(create(:personal_snippet, :public))
expect(policies).to be_allowed(:admin_note) it 'can edit note' do
expect(policies).to be_allowed(:resolve_note) expect(policy).to be_allowed(:admin_note)
expect(policies).to be_allowed(:read_note) expect(policy).to be_allowed(:resolve_note)
expect(policy).to be_allowed(:read_note)
end end
context 'when it is private' do context 'when it is private' do
it 'can not edit nor read the note' do let(:noteable) { create(:personal_snippet, :private) }
policies = policies(create(:personal_snippet, :private))
expect(policies).to be_disallowed(:admin_note) it 'can not edit nor read the note' do
expect(policies).to be_disallowed(:resolve_note) expect(policy).to be_disallowed(:admin_note)
expect(policies).to be_disallowed(:read_note) expect(policy).to be_disallowed(:resolve_note)
expect(policy).to be_disallowed(:read_note)
end end
end end
end end
...@@ -120,20 +116,20 @@ describe NotePolicy, mdoels: true do ...@@ -120,20 +116,20 @@ describe NotePolicy, mdoels: true do
end end
it 'can edit a note' do it 'can edit a note' do
expect(policies).to be_allowed(:admin_note) expect(policy).to be_allowed(:admin_note)
expect(policies).to be_allowed(:resolve_note) expect(policy).to be_allowed(:resolve_note)
expect(policies).to be_allowed(:read_note) expect(policy).to be_allowed(:read_note)
end end
end end
context 'when the note author is not a project member' do context 'when the note author is not a project member' do
it 'can not edit a note' do it 'can not edit a note' do
expect(policies).to be_disallowed(:admin_note) expect(policy).to be_disallowed(:admin_note)
expect(policies).to be_disallowed(:resolve_note) expect(policy).to be_disallowed(:resolve_note)
end end
it 'can read a note' do it 'can read a note' do
expect(policies).to be_allowed(:read_note) expect(policy).to be_allowed(:read_note)
end end
end end
end end
......
...@@ -86,6 +86,37 @@ shared_examples 'discussions API' do |parent_type, noteable_type, id_name| ...@@ -86,6 +86,37 @@ shared_examples 'discussions API' do |parent_type, noteable_type, id_name|
expect(response).to have_gitlab_http_status(404) expect(response).to have_gitlab_http_status(404)
end end
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 end
describe "POST /#{parent_type}/:id/#{noteable_type}/:noteable_id/discussions/:discussion_id/notes" do describe "POST /#{parent_type}/:id/#{noteable_type}/:noteable_id/discussions/:discussion_id/notes" do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment