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

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

See merge request gitlab/gitlabhq!2963

10 files changed, 85 insertions, 13 deletions

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 9f3a1699146..79b3582fcba 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 dab7700cf64..b0c20fb5a90 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 675eeff8d12..ce021b2f085 100644
--- a/spec/controllers/projects/group_links_controller_spec.rb
+++ b/spec/controllers/projects/group_links_controller_spec.rb
@@ -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
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 ffe4512fa6f..90b2fc33ba0 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