Commit b50ad884 authored by Yorick Peterse's avatar Yorick Peterse

Merge branch...

Merge branch '2802-security-add-public-internal-groups-as-members-to-your-project-idor' into 'master'

Add public/internal groups as members to your Project(IDOR)

See merge request gitlab/gitlabhq!2898
parents 03340f09 211c4e59
...@@ -13,9 +13,10 @@ class Projects::GroupLinksController < Projects::ApplicationController ...@@ -13,9 +13,10 @@ class Projects::GroupLinksController < Projects::ApplicationController
group = Group.find(params[:link_group_id]) if params[:link_group_id].present? group = Group.find(params[:link_group_id]) if params[:link_group_id].present?
if group if group
return render_404 unless can?(current_user, :read_group, group) result = Projects::GroupLinks::CreateService.new(project, current_user, group_link_create_params).execute(group)
return render_404 if result[:http_status] == 404
Projects::GroupLinks::CreateService.new(project, current_user, group_link_create_params).execute(group) flash[:alert] = result[:message] if result[:http_status] == 409
else else
flash[:alert] = 'Please select a group.' flash[:alert] = 'Please select a group.'
end end
......
...@@ -4,13 +4,19 @@ module Projects ...@@ -4,13 +4,19 @@ module Projects
module GroupLinks module GroupLinks
class CreateService < BaseService class CreateService < BaseService
def execute(group) def execute(group)
return false unless group return error('Not Found', 404) unless group && can?(current_user, :read_namespace, group)
project.project_group_links.create( link = project.project_group_links.new(
group: group, group: group,
group_access: params[:link_group_access], group_access: params[:link_group_access],
expires_at: params[:expires_at] expires_at: params[:expires_at]
) )
if link.save
success(link: link)
else
error(link.errors.full_messages.to_sentence, 409)
end
end end
end end
end end
......
---
title: Remove the possibility to share a project with a group that a user is not a member
of
merge_request:
author:
type: security
...@@ -442,27 +442,24 @@ module API ...@@ -442,27 +442,24 @@ module API
end end
params do params do
requires :group_id, type: Integer, desc: 'The ID of a group' requires :group_id, type: Integer, desc: 'The ID of a group'
requires :group_access, type: Integer, values: Gitlab::Access.values, desc: 'The group access level' requires :group_access, type: Integer, values: Gitlab::Access.values, as: :link_group_access, desc: 'The group access level'
optional :expires_at, type: Date, desc: 'Share expiration date' optional :expires_at, type: Date, desc: 'Share expiration date'
end end
post ":id/share" do post ":id/share" do
authorize! :admin_project, user_project authorize! :admin_project, user_project
group = Group.find_by_id(params[:group_id]) group = Group.find_by_id(params[:group_id])
unless group && can?(current_user, :read_group, group)
not_found!('Group')
end
unless user_project.allowed_to_share_with_group? unless user_project.allowed_to_share_with_group?
break render_api_error!("The project sharing with group is disabled", 400) break render_api_error!("The project sharing with group is disabled", 400)
end end
link = user_project.project_group_links.new(declared_params(include_missing: false)) result = ::Projects::GroupLinks::CreateService.new(user_project, current_user, declared_params(include_missing: false))
.execute(group)
if link.save if result[:status] == :success
present link, with: Entities::ProjectGroupLink present result[:link], with: Entities::ProjectGroupLink
else else
render_api_error!(link.errors.full_messages.first, 409) render_api_error!(result[:message], result[:http_status])
end end
end end
......
...@@ -6,6 +6,8 @@ describe Groups::SharedProjectsController do ...@@ -6,6 +6,8 @@ describe Groups::SharedProjectsController do
end end
def share_project(project) def share_project(project)
group.add_developer(user)
Projects::GroupLinks::CreateService.new( Projects::GroupLinks::CreateService.new(
project, project,
user, user,
......
...@@ -65,8 +65,24 @@ describe Projects::GroupLinksController do ...@@ -65,8 +65,24 @@ describe Projects::GroupLinksController do
end end
end end
context 'when user does not have access to the public group' do
let(:group) { create(:group, :public) }
include_context 'link project to group'
it 'renders 404' do
expect(response.status).to eq 404
end
it 'does not share project with that group' do
expect(group.shared_projects).not_to include project
end
end
context 'when project group id equal link group id' do context 'when project group id equal link group id' do
before do before do
group2.add_developer(user)
post(:create, params: { post(:create, params: {
namespace_id: project.namespace, namespace_id: project.namespace,
project_id: project, project_id: project,
...@@ -102,5 +118,26 @@ describe Projects::GroupLinksController do ...@@ -102,5 +118,26 @@ describe Projects::GroupLinksController do
expect(flash[:alert]).to eq('Please select a group.') expect(flash[:alert]).to eq('Please select a group.')
end end
end end
context 'when link is not persisted in the database' do
before do
allow(::Projects::GroupLinks::CreateService).to receive_message_chain(:new, :execute)
.and_return({ status: :error, http_status: 409, message: 'error' })
post(:create, params: {
namespace_id: project.namespace,
project_id: project,
link_group_id: group.id,
link_group_access: ProjectGroupLink.default_access
})
end
it 'redirects to project group links page' do
expect(response).to redirect_to(
project_project_members_path(project)
)
expect(flash[:alert]).to eq('error')
end
end
end end
end end
...@@ -27,6 +27,7 @@ describe 'Project > Members > Invite group', :js do ...@@ -27,6 +27,7 @@ describe 'Project > Members > Invite group', :js do
before do before do
project.add_maintainer(maintainer) project.add_maintainer(maintainer)
group_to_share_with.add_guest(maintainer)
sign_in(maintainer) sign_in(maintainer)
end end
...@@ -112,6 +113,7 @@ describe 'Project > Members > Invite group', :js do ...@@ -112,6 +113,7 @@ describe 'Project > Members > Invite group', :js do
before do before do
project.add_maintainer(maintainer) project.add_maintainer(maintainer)
group.add_guest(maintainer)
sign_in(maintainer) sign_in(maintainer)
visit project_settings_members_path(project) visit project_settings_members_path(project)
......
...@@ -10,6 +10,7 @@ describe 'Projects > Settings > User manages group links' do ...@@ -10,6 +10,7 @@ describe 'Projects > Settings > User manages group links' do
before do before do
project.add_maintainer(user) project.add_maintainer(user)
group_market.add_guest(user)
sign_in(user) sign_in(user)
share_link = project.project_group_links.new(group_access: Gitlab::Access::MAINTAINER) share_link = project.project_group_links.new(group_access: Gitlab::Access::MAINTAINER)
......
...@@ -1510,6 +1510,9 @@ describe API::Projects do ...@@ -1510,6 +1510,9 @@ describe API::Projects do
describe "POST /projects/:id/share" do describe "POST /projects/:id/share" do
let(:group) { create(:group) } let(:group) { create(:group) }
before do
group.add_developer(user)
end
it "shares project with group" do it "shares project with group" do
expires_at = 10.days.from_now.to_date expires_at = 10.days.from_now.to_date
...@@ -1560,6 +1563,15 @@ describe API::Projects do ...@@ -1560,6 +1563,15 @@ describe API::Projects do
expect(response).to have_gitlab_http_status(400) expect(response).to have_gitlab_http_status(400)
expect(json_response['error']).to eq 'group_access does not have a valid value' expect(json_response['error']).to eq 'group_access does not have a valid value'
end end
it "returns a 409 error when link is not saved" do
allow(::Projects::GroupLinks::CreateService).to receive_message_chain(:new, :execute)
.and_return({ status: :error, http_status: 409, message: 'error' })
post api("/projects/#{project.id}/share", user), params: { group_id: group.id, group_access: Gitlab::Access::DEVELOPER }
expect(response).to have_gitlab_http_status(409)
end
end end
describe 'DELETE /projects/:id/share/:group_id' do describe 'DELETE /projects/:id/share/:group_id' do
......
...@@ -12,6 +12,10 @@ describe Projects::GroupLinks::CreateService, '#execute' do ...@@ -12,6 +12,10 @@ describe Projects::GroupLinks::CreateService, '#execute' do
end end
let(:subject) { described_class.new(project, user, opts) } let(:subject) { described_class.new(project, user, opts) }
before do
group.add_developer(user)
end
it 'adds group to project' do it 'adds group to project' do
expect { subject.execute(group) }.to change { project.project_group_links.count }.from(0).to(1) expect { subject.execute(group) }.to change { project.project_group_links.count }.from(0).to(1)
end end
...@@ -19,4 +23,8 @@ describe Projects::GroupLinks::CreateService, '#execute' do ...@@ -19,4 +23,8 @@ describe Projects::GroupLinks::CreateService, '#execute' do
it 'returns false if group is blank' do it 'returns false if group is blank' do
expect { subject.execute(nil) }.not_to change { project.project_group_links.count } expect { subject.execute(nil) }.not_to change { project.project_group_links.count }
end end
it 'returns error if user is not allowed to share with a group' do
expect { subject.execute(create :group) }.not_to change { project.project_group_links.count }
end
end end
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