From a1cde868f89572f523ec1bd1ecba4ebd5f0929d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Tue, 12 Feb 2019 13:29:47 +0100 Subject: [PATCH 1/4] Fix conflict --- .../projects/group_links_controller.rb | 5 +- .../projects/group_links/create_service.rb | 10 +++- ...groups-as-members-to-your-project-idor.yml | 6 +++ lib/api/projects.rb | 15 +++--- .../groups/shared_projects_controller_spec.rb | 2 + .../projects/group_links_controller_spec.rb | 47 +++++++++++++++++-- .../projects/members/invite_group_spec.rb | 2 + .../settings/user_manages_group_links_spec.rb | 1 + spec/requests/api/projects_spec.rb | 12 +++++ .../group_links/create_service_spec.rb | 8 ++++ 10 files changed, 91 insertions(+), 17 deletions(-) create mode 100644 changelogs/unreleased/2802-security-add-public-internal-groups-as-members-to-your-project-idor.yml diff --git a/app/controllers/projects/group_links_controller.rb b/app/controllers/projects/group_links_controller.rb index 7c713c19762..bc942ba9288 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 1392775f805..e3d5bea0852 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 00000000000..27ad151cd06 --- /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 f5d21d8923f..afca2d5bc2d 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 003c8c262e7..bf551833094 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 879aff26deb..634f797efed 100644 --- a/spec/controllers/projects/group_links_controller_spec.rb +++ b/spec/controllers/projects/group_links_controller_spec.rb @@ -63,12 +63,30 @@ 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 - post(:create, namespace_id: project.namespace, - project_id: project, - link_group_id: group2.id, - link_group_access: ProjectGroupLink.default_access) + group2.add_developer(user) + + post(:create, params: { + namespace_id: project.namespace, + project_id: project, + link_group_id: group2.id, + link_group_access: ProjectGroupLink.default_access + }) end it 'does not share project with selected group' do @@ -96,5 +114,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 diff --git a/spec/features/projects/members/invite_group_spec.rb b/spec/features/projects/members/invite_group_spec.rb index fceead0b45e..b2d2dba55f1 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 676659b90c3..e5a58c44e41 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 e40db55cd20..2ec58145731 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), 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 diff --git a/spec/services/projects/group_links/create_service_spec.rb b/spec/services/projects/group_links/create_service_spec.rb index ffb270d277e..68fd82b4cbe 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 -- GitLab From 58fe022c74a9ac586dc6cd96a1086d78a9bf6fe1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Thu, 21 Feb 2019 12:34:15 +0100 Subject: [PATCH 2/4] Change how path is called --- .../projects/group_links_controller_spec.rb | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/spec/controllers/projects/group_links_controller_spec.rb b/spec/controllers/projects/group_links_controller_spec.rb index 634f797efed..3af6480aa55 100644 --- a/spec/controllers/projects/group_links_controller_spec.rb +++ b/spec/controllers/projects/group_links_controller_spec.rb @@ -81,12 +81,10 @@ describe Projects::GroupLinksController do before do group2.add_developer(user) - post(:create, params: { - namespace_id: project.namespace, - project_id: project, - link_group_id: group2.id, - link_group_access: ProjectGroupLink.default_access - }) + post(:create, namespace_id: project.namespace, + project_id: project, + link_group_id: group2.id, + link_group_access: ProjectGroupLink.default_access) end it 'does not share project with selected group' do -- GitLab From d90a243470369521741842a9e96a65a9859511f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Thu, 21 Feb 2019 14:07:38 +0100 Subject: [PATCH 3/4] Update specs --- .../projects/group_links_controller_spec.rb | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/spec/controllers/projects/group_links_controller_spec.rb b/spec/controllers/projects/group_links_controller_spec.rb index 3af6480aa55..6b12a6ae720 100644 --- a/spec/controllers/projects/group_links_controller_spec.rb +++ b/spec/controllers/projects/group_links_controller_spec.rb @@ -118,12 +118,10 @@ describe Projects::GroupLinksController 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 - }) + 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 -- GitLab From fe478ceac99e595640167243174ff422afbd0680 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Thu, 21 Feb 2019 15:12:00 +0100 Subject: [PATCH 4/4] Update specs --- spec/requests/api/projects_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index 2ec58145731..a33b1e2f09e 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -1450,7 +1450,7 @@ describe API::Projects 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 } + 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 -- GitLab