Merge branch 'fix/link-group-permissions' into 'master'

Check permissions when sharing project with group

## Summary

Unprivileged user was able to share project with group he didn't have access to, and therefore gain partial access to that group, which opened possibilities for further actions like listing private projects in that group.

See https://gitlab.com/gitlab-org/gitlab-ce/issues/15330

## Fix

This change introduces additional check for group read access.

## Further work

We can think about preventing such problems in the future (this is quite common problem) by moving permissions checks to another layer of abstraction (TBD).

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/15330

See merge request !1949

3 files changed, 59 insertions, 4 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 3ced5aa3f89..8c29499a773 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -91,6 +91,9 @@ v 8.7.0 (unreleased)
   - Execute system web hooks on push to the project
   - Allow enable/disable push events for system hooks
 
+v 8.6.7 (unreleased)
+  - Fix vulnerability that made it possible to enumerate private projects belonging to group
+
 v 8.6.6
   - Expire the exists cache before deletion to ensure project dir actually exists (Stan Hu). !3413
   - Fix error on language detection when repository has no HEAD (e.g., master branch) (Jeroen Bobbeldijk). !3654
diff --git a/app/controllers/projects/group_links_controller.rb b/app/controllers/projects/group_links_controller.rb
index 4159e53bfa9..606552fa853 100644
--- a/app/controllers/projects/group_links_controller.rb
+++ b/app/controllers/projects/group_links_controller.rb
@@ -7,10 +7,12 @@ class Projects::GroupLinksController < Projects::ApplicationController
   end
 
   def create
-    link = project.project_group_links.new
-    link.group_id = params[:link_group_id]
-    link.group_access = params[:link_group_access]
-    link.save
+    group = Group.find(params[:link_group_id])
+    return render_404 unless can?(current_user, :read_group, group)
+
+    project.project_group_links.create(
+      group: group, group_access: params[:link_group_access]
+    )
 
     redirect_to namespace_project_group_links_path(project.namespace, project)
   end
diff --git a/spec/controllers/projects/group_links_controller_spec.rb b/spec/controllers/projects/group_links_controller_spec.rb
new file mode 100644
index 00000000000..40bd83af861
--- /dev/null
+++ b/spec/controllers/projects/group_links_controller_spec.rb
@@ -0,0 +1,50 @@
+require 'spec_helper'
+
+describe Projects::GroupLinksController do
+  let(:project) { create(:project, :private) }
+  let(:group) { create(:group, :private) }
+  let(:user) { create(:user) }
+
+  before do
+    project.team << [user, :master]
+    sign_in(user)
+  end
+
+  describe '#create' do
+    shared_context 'link project to group' do
+      before do
+        post(:create, namespace_id: project.namespace.to_param,
+                      project_id: project.to_param,
+                      link_group_id: group.id,
+                      link_group_access: ProjectGroupLink.default_access)
+      end
+    end
+
+    context 'when user has access to group he want to link project to' do
+      before { group.add_developer(user) }
+      include_context 'link project to group'
+
+      it 'links project with selected group' do
+        expect(group.shared_projects).to include project
+      end
+
+      it 'redirects to project group links page'do
+        expect(response).to redirect_to(
+          namespace_project_group_links_path(project.namespace, project)
+        )
+      end
+    end
+
+    context 'when user doers not have access to group he want to link to' do
+      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).to_not include project
+      end
+    end
+  end
+end