Commit a738d031 authored by Yorick Peterse's avatar Yorick Peterse

Merge branch...

Merge branch 'security-add-public-internal-groups-as-members-to-your-project-idor-11-8' into '11-8-stable'

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

See merge request gitlab/gitlabhq!2962
parents 361151f1 d2c83f40
......@@ -13,9 +13,10 @@ class Projects::GroupLinksController < Projects::ApplicationController
group = Group.find(params[:link_group_id]) if params[:link_group_id].present?
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
flash[:alert] = 'Please select a group.'
end
......
......@@ -4,13 +4,19 @@ module Projects
module GroupLinks
class CreateService < BaseService
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_access: params[:link_group_access],
expires_at: params[:expires_at]
)
if link.save
success(link: link)
else
error(link.errors.full_messages.to_sentence, 409)
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
......@@ -436,27 +436,24 @@ module API
end
params do
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'
end
post ":id/share" do
authorize! :admin_project, user_project
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?
break render_api_error!("The project sharing with group is disabled", 400)
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
present link, with: Entities::ProjectGroupLink
if result[:status] == :success
present result[:link], with: Entities::ProjectGroupLink
else
render_api_error!(link.errors.full_messages.first, 409)
render_api_error!(result[:message], result[:http_status])
end
end
......
......@@ -6,6 +6,8 @@ describe Groups::SharedProjectsController do
end
def share_project(project)
group.add_developer(user)
Projects::GroupLinks::CreateService.new(
project,
user,
......
......@@ -65,8 +65,24 @@ describe Projects::GroupLinksController do
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
before do
group2.add_developer(user)
post(:create, params: {
namespace_id: project.namespace,
project_id: project,
......@@ -102,5 +118,26 @@ describe Projects::GroupLinksController do
expect(flash[:alert]).to eq('Please select a group.')
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
......@@ -27,6 +27,7 @@ describe 'Project > Members > Invite group', :js do
before do
project.add_maintainer(maintainer)
group_to_share_with.add_guest(maintainer)
sign_in(maintainer)
end
......@@ -112,6 +113,7 @@ describe 'Project > Members > Invite group', :js do
before do
project.add_maintainer(maintainer)
group.add_guest(maintainer)
sign_in(maintainer)
visit project_settings_members_path(project)
......
......@@ -10,6 +10,7 @@ describe 'Projects > Settings > User manages group links' do
before do
project.add_maintainer(user)
group_market.add_guest(user)
sign_in(user)
share_link = project.project_group_links.new(group_access: Gitlab::Access::MAINTAINER)
......
......@@ -1484,6 +1484,9 @@ describe API::Projects do
describe "POST /projects/:id/share" do
let(:group) { create(:group) }
before do
group.add_developer(user)
end
it "shares project with group" do
expires_at = 10.days.from_now.to_date
......@@ -1534,6 +1537,15 @@ describe API::Projects do
expect(response).to have_gitlab_http_status(400)
expect(json_response['error']).to eq 'group_access does not have a valid value'
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
describe 'DELETE /projects/:id/share/:group_id' do
......
......@@ -12,6 +12,10 @@ describe Projects::GroupLinks::CreateService, '#execute' do
end
let(:subject) { described_class.new(project, user, opts) }
before do
group.add_developer(user)
end
it 'adds group to project' do
expect { subject.execute(group) }.to change { project.project_group_links.count }.from(0).to(1)
end
......@@ -19,4 +23,8 @@ describe Projects::GroupLinks::CreateService, '#execute' do
it 'returns false if group is blank' do
expect { subject.execute(nil) }.not_to change { project.project_group_links.count }
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
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