Merge branch 'api-fix-project-group-sharing' into 'security'

API: Share projects only with groups current_user can access

Aims to address the issues here: https://gitlab.com/gitlab-org/gitlab-ce/issues/23004

* Projects can be shared with non-existent groups
* Projects can be shared with groups that the current user does not have access to read

Concerns:

The new implementation of the API endpoint allows projects to be shared with a larger range of groups than can be done via the web UI.

The form for sharing a project with a group uses the following API endpoint to index the available groups: https://gitlab.com/gitlab-org/gitlab-ce/blob/494269fc92f61098ee6bd635a0426129ce2c5456/lib/api/groups.rb#L17. The groups indexed in the web form will only be those groups that the user is currently a member of.

The new implementation allows projects to be shared with any group that the authenticated user has access to view. This widens the range of groups to those that are public and internal.

See merge request !2005

Signed-off-by: Rémy Coutable <remy@rymai.me>

5 files changed, 24 insertions, 4 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 7b157d61411..34d1481d28b 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,7 +1,7 @@
 Please view this file on the master branch, on stable branches it's out of date.
 
 v 8.10.12
-  - Don't send Private-Token (API authentication) headers to Sentry
+  - Share projects via the API only with groups the authenticated user can access
 
 v 8.10.11
   - Respect the fork_project permission when forking projects
diff --git a/app/models/project_group_link.rb b/app/models/project_group_link.rb
index e52a6bd7c84..1bc5fa5e4d4 100644
--- a/app/models/project_group_link.rb
+++ b/app/models/project_group_link.rb
@@ -8,7 +8,7 @@ class ProjectGroupLink < ActiveRecord::Base
   belongs_to :group
 
   validates :project_id, presence: true
-  validates :group_id, presence: true
+  validates :group, presence: true
   validates :group_id, uniqueness: { scope: [:project_id], message: "already shared with this group" }
   validates :group_access, presence: true
   validates :group_access, inclusion: { in: Gitlab::Access.values }, presence: true
diff --git a/lib/api/projects.rb b/lib/api/projects.rb
index 6d2a6f3946c..473c705f1d2 100644
--- a/lib/api/projects.rb
+++ b/lib/api/projects.rb
@@ -373,6 +373,12 @@ module API
         authorize! :admin_project, user_project
         required_attributes! [:group_id, :group_access]
 
+        group = Group.find_by_id(attrs[:group_id])
+
+        unless group && can?(current_user, :read_group, group)
+          not_found!('Group')
+        end
+
         unless user_project.allowed_to_share_with_group?
           return render_api_error!("The project sharing with group is disabled", 400)
         end
diff --git a/spec/models/project_group_link_spec.rb b/spec/models/project_group_link_spec.rb
index 2fa6715fcaf..c5ff1941378 100644
--- a/spec/models/project_group_link_spec.rb
+++ b/spec/models/project_group_link_spec.rb
@@ -11,7 +11,7 @@ describe ProjectGroupLink do
 
     it { should validate_presence_of(:project_id) }
     it { should validate_uniqueness_of(:group_id).scoped_to(:project_id).with_message(/already shared/) }
-    it { should validate_presence_of(:group_id) }
+    it { should validate_presence_of(:group) }
     it { should validate_presence_of(:group_access) }
   end
 end
diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb
index 8c6a7e6529d..5b62aef413d 100644
--- a/spec/requests/api/projects_spec.rb
+++ b/spec/requests/api/projects_spec.rb
@@ -823,7 +823,21 @@ describe API::API, api: true  do
       expect(response.status).to eq 400
     end
 
-    it "should return a 409 error when wrong params passed" do
+    it 'returns a 404 error when user cannot read group' do
+      private_group = create(:group, :private)
+
+      post api("/projects/#{project.id}/share", user), group_id: private_group.id, group_access: Gitlab::Access::DEVELOPER
+
+      expect(response.status).to eq 404
+    end
+
+    it 'returns a 404 error when group does not exist' do
+      post api("/projects/#{project.id}/share", user), group_id: 1234, group_access: Gitlab::Access::DEVELOPER
+
+      expect(response.status).to eq 404
+    end
+
+    it "returns a 409 error when wrong params passed" do
       post api("/projects/#{project.id}/share", user), group_id: group.id, group_access: 1234
       expect(response.status).to eq 409
       expect(json_response['message']).to eq 'Group access is not included in the list'