diff --git a/app/controllers/projects/group_links_controller.rb b/app/controllers/projects/group_links_controller.rb index 7c713c197626e79d5c6174c6bb2d67d8525a5426..bc942ba9288107f862c9fa068f30ea883f176e1a 100644 --- a/app/controllers/projects/group_links_controller.rb +++ b/app/controllers/projects/group_links_controller.rb @@ -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 diff --git a/app/services/projects/group_links/create_service.rb b/app/services/projects/group_links/create_service.rb index 1392775f8053c7a84e951437a570494410b8eca2..e3d5bea085226761e4bc882579b18966025c39ea 100644 --- a/app/services/projects/group_links/create_service.rb +++ b/app/services/projects/group_links/create_service.rb @@ -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 diff --git a/changelogs/unreleased/2802-security-add-public-internal-groups-as-members-to-your-project-idor.yml b/changelogs/unreleased/2802-security-add-public-internal-groups-as-members-to-your-project-idor.yml new file mode 100644 index 0000000000000000000000000000000000000000..27ad151cd061f49612df1363ba0b3ca8bbc775ee --- /dev/null +++ b/changelogs/unreleased/2802-security-add-public-internal-groups-as-members-to-your-project-idor.yml @@ -0,0 +1,6 @@ +--- +title: Remove the possibility to share a project with a group that a user is not a member + of +merge_request: +author: +type: security diff --git a/lib/api/projects.rb b/lib/api/projects.rb index f5d21d8923fcf1b5975cf229775de1f32e3ca917..afca2d5bc2df1817294ef2c2e3ec4427db1f7c08 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -432,27 +432,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 diff --git a/spec/controllers/groups/shared_projects_controller_spec.rb b/spec/controllers/groups/shared_projects_controller_spec.rb index 003c8c262e76f2121b628f8996b6a6dc7b53b238..bf551833094b62d44a5ce8187500463bc6e1cd0a 100644 --- a/spec/controllers/groups/shared_projects_controller_spec.rb +++ b/spec/controllers/groups/shared_projects_controller_spec.rb @@ -6,6 +6,8 @@ describe Groups::SharedProjectsController do end def share_project(project) + group.add_developer(user) + Projects::GroupLinks::CreateService.new( project, user, diff --git a/spec/controllers/projects/group_links_controller_spec.rb b/spec/controllers/projects/group_links_controller_spec.rb index 879aff26debc55c99eeb43a1087356a55ec8307b..6b12a6ae720f00d62482df8f342b2751a0cea885 100644 --- a/spec/controllers/projects/group_links_controller_spec.rb +++ b/spec/controllers/projects/group_links_controller_spec.rb @@ -63,8 +63,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, namespace_id: project.namespace, project_id: project, link_group_id: group2.id, @@ -96,5 +112,24 @@ 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, 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 diff --git a/spec/features/projects/members/invite_group_spec.rb b/spec/features/projects/members/invite_group_spec.rb index fceead0b45eb2f5f36fca9209018e958dab9f8f6..b2d2dba55f16b3ed84d6947b40dc093377419f7c 100644 --- a/spec/features/projects/members/invite_group_spec.rb +++ b/spec/features/projects/members/invite_group_spec.rb @@ -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) diff --git a/spec/features/projects/settings/user_manages_group_links_spec.rb b/spec/features/projects/settings/user_manages_group_links_spec.rb index 676659b90c318a483c5a5bc2f82501de15c08cc1..e5a58c44e4125bd5a81d0843129827470c6ef2d2 100644 --- a/spec/features/projects/settings/user_manages_group_links_spec.rb +++ b/spec/features/projects/settings/user_manages_group_links_spec.rb @@ -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) diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index e40db55cd204323853b00e57ee304872f132864e..a33b1e2f09ecdd7a80a625b5ead561cd51bc749d 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -1392,6 +1392,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 @@ -1442,6 +1445,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), 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 diff --git a/spec/services/projects/group_links/create_service_spec.rb b/spec/services/projects/group_links/create_service_spec.rb index ffb270d277eecd7ed3842fcd07375319c09ec932..68fd82b4cbe6c452a656248cdf5d0c74e3696a4c 100644 --- a/spec/services/projects/group_links/create_service_spec.rb +++ b/spec/services/projects/group_links/create_service_spec.rb @@ -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