Check access rights when creating/updating ProtectedRefs

At the moment, when we create a ProtectedBranch or ProtectedTag
when don't check that the user/group is already a member of the
project. That means that when creating/updating the params
can be tampered and add a user/group outside the project.

app/services/protected_branches/api_service.rb

@@ -7,8 +7,6 @@ module ProtectedBranches
       @merge_params = AccessLevelParams.new(:merge, params)
       @unprotect_params = AccessLevelParams.new(:unprotect, params)
 
-      verify_params!
-
       protected_branch_params = {
         name: params[:name],
         push_access_levels_attributes: @push_params.access_levels,
@@ -18,13 +16,5 @@ module ProtectedBranches
 
       ::ProtectedBranches::CreateService.new(@project, @current_user, protected_branch_params).execute
     end
-
-    private
-
-    def verify_params!
-      # EE-only
-    end
   end
 end
-
-ProtectedBranches::ApiService.prepend(EE::ProtectedBranches::ApiService)

ee/app/models/concerns/ee/protected_ref_access.rb

@@ -27,6 +27,9 @@ module EE
         validates :group, :user,
                   absence: true,
                   unless: :protected_refs_for_users_required_and_available
+
+        validate :validate_group_membership, if: :protected_refs_for_users_required_and_available
+        validate :validate_user_membership, if: :protected_refs_for_users_required_and_available
       end
     end
 
@@ -81,5 +84,21 @@ module EE
     def protected_refs_for_users_required_and_available
       type != :role && project.feature_available?(:protected_refs_for_users)
     end
+
+    def validate_group_membership
+      return unless group
+
+      unless project.project_group_links.where(group: group).exists?
+        self.errors.add(:group, 'does not have access to the project')
+      end
+    end
+
+    def validate_user_membership
+      return unless user
+
+      unless project.members.where(user: user).exists?
+        self.errors.add(:user, 'is not a member of the project')
+      end
+    end
   end
 end

ee/app/services/ee/protected_branches/access_level_params.rb

@@ -10,14 +10,6 @@ module EE
         ce_style_access_level + ee_style_access_levels
       end
 
-      def group_ids
-        ids_for('group_id')
-      end
-
-      def user_ids
-        ids_for('user_id')
-      end
-
       private
 
       override :use_default_access_level?
@@ -28,10 +20,6 @@ module EE
       def ee_style_access_levels
         params[:"allowed_to_#{type}"] || []
       end
-
-      def ids_for(key)
-        ee_style_access_levels.select { |level| level[key] }.flat_map(&:values)
-      end
     end
   end
 end

ee/app/services/ee/protected_branches/api_service.rb

-# frozen_string_literal: true
-
-module EE
-  module ProtectedBranches
-    module ApiService
-      extend ::Gitlab::Utils::Override
-
-      GroupsNotAccessibleError = Class.new(StandardError)
-      UsersNotAccessibleError = Class.new(StandardError)
-
-      override :create
-      def create
-        super
-      rescue ::EE::ProtectedBranches::ApiService::GroupsNotAccessibleError,
-             ::EE::ProtectedBranches::ApiService::UsersNotAccessibleError
-        ::ProtectedBranch.new.tap do |protected_branch|
-          message = 'Cannot add users or groups unless they have access to the project'
-          protected_branch.errors.add(:base, message)
-        end
-      end
-
-      private
-
-      override :verify_params!
-      def verify_params!
-        raise GroupsNotAccessibleError.new unless groups_accessible?
-        raise UsersNotAccessibleError.new unless users_accessible?
-      end
-
-      # rubocop: disable CodeReuse/ActiveRecord
-      def groups_accessible?
-        group_ids = @merge_params.group_ids + @push_params.group_ids + @unprotect_params.group_ids # rubocop:disable Gitlab/ModuleWithInstanceVariables
-        allowed_groups = @project.invited_groups.where(id: group_ids) # rubocop:disable Gitlab/ModuleWithInstanceVariables
-
-        group_ids.uniq.count == allowed_groups.count
-      end
-      # rubocop: enable CodeReuse/ActiveRecord
-
-      # rubocop: disable CodeReuse/ActiveRecord
-      def users_accessible?
-        user_ids = @merge_params.user_ids + @push_params.user_ids + @unprotect_params.user_ids # rubocop:disable Gitlab/ModuleWithInstanceVariables
-        allowed_users = @project.team.users.where(id: user_ids) # rubocop:disable Gitlab/ModuleWithInstanceVariables
-
-        user_ids.uniq.count == allowed_users.count
-      end
-      # rubocop: enable CodeReuse/ActiveRecord
-    end
-  end
-end

ee/changelogs/unreleased/security-fix-protected-branches-creation-access-rights.yml

+---
+title: Check access rights when creating/updating ProtectedRefs
+merge_request:
+author:
+type: security

ee/spec/features/projects/members/member_is_removed_from_project_spec.rb

@@ -3,9 +3,11 @@ require 'spec_helper'
 describe 'Projects > Members > Member is removed from project' do
   let(:user) { create(:user) }
   let(:project) { create(:project) }
+  let(:other_user) { create(:user) }
 
   before do
     project.add_maintainer(user)
+    project.add_maintainer(other_user)
     sign_in(user)
     visit project_project_members_path(project)
   end
@@ -17,7 +19,6 @@ describe 'Projects > Members > Member is removed from project' do
   end
 
   context 'when the user has been specifically allowed to access a protected branch' do
-    let(:other_user) { create(:user) }
     let!(:matching_protected_branch) { create(:protected_branch, authorize_user_to_push: user, authorize_user_to_merge: user, project: project) }
     let!(:non_matching_protected_branch) { create(:protected_branch, authorize_user_to_push: other_user, authorize_user_to_merge: other_user, project: project) }
 

ee/spec/features/projects/members/member_leaves_project_spec.rb

@@ -6,6 +6,7 @@ describe 'Projects > Members > Member leaves project' do
 
   before do
     project.add_developer(user)
+    project.add_developer(other_user)
     sign_in(user)
     visit project_path(project)
   end

ee/spec/models/ee/protected_branch_spec.rb

@@ -30,6 +30,7 @@ describe ProtectedBranch do
 
       it "does not count a user-based #{human_association_name} with an `access_level` set" do
         protected_branch = create(:protected_branch, default_access_level: false)
+        protected_branch.project.add_developer(user)
 
         protected_branch.send(association_name) << build(factory_name, user: user, access_level: Gitlab::Access::MASTER)
         protected_branch.send(association_name) << build(factory_name, access_level: Gitlab::Access::MASTER)
@@ -40,6 +41,7 @@ describe ProtectedBranch do
       it "does not count a group-based #{human_association_name} with an `access_level` set" do
         group = create(:group)
         protected_branch = create(:protected_branch, default_access_level: false)
+        protected_branch.project.project_group_links.create(group: group)
 
         protected_branch.send(association_name) << build(factory_name, group: group, access_level: Gitlab::Access::MASTER)
         protected_branch.send(association_name) << build(factory_name, access_level: Gitlab::Access::MASTER)
@@ -53,6 +55,9 @@ describe ProtectedBranch do
         first_protected_branch = create(:protected_branch, default_access_level: false)
         second_protected_branch = create(:protected_branch, default_access_level: false)
 
+        first_protected_branch.project.add_developer(user)
+        second_protected_branch.project.add_developer(user)
+
         first_protected_branch.send(association_name) << build(factory_name, user: user)
         second_protected_branch.send(association_name) << build(factory_name, user: user)
 
@@ -66,6 +71,7 @@ describe ProtectedBranch do
 
       it "ignores the `access_level` while validating a user-based #{human_association_name}" do
         protected_branch = create(:protected_branch, default_access_level: false)
+        protected_branch.project.add_developer(user)
 
         protected_branch.send(association_name) << build(factory_name, access_level: Gitlab::Access::MASTER)
         protected_branch.send(association_name) << build(factory_name, user: user, access_level: Gitlab::Access::MASTER)
@@ -81,6 +87,9 @@ describe ProtectedBranch do
         first_protected_branch = create(:protected_branch, default_access_level: false)
         second_protected_branch = create(:protected_branch, default_access_level: false)
 
+        first_protected_branch.project.project_group_links.create(group: group)
+        second_protected_branch.project.project_group_links.create(group: group)
+
         first_protected_branch.send(association_name) << build(factory_name, group: group)
         second_protected_branch.send(association_name) << build(factory_name, group: group)
 
@@ -94,6 +103,7 @@ describe ProtectedBranch do
 
       it "ignores the `access_level` while validating a group-based #{human_association_name}" do
         protected_branch = create(:protected_branch, default_access_level: false)
+        protected_branch.project.project_group_links.create(group: group)
 
         protected_branch.send(association_name) << build(factory_name, access_level: Gitlab::Access::MASTER)
         protected_branch.send(association_name) << build(factory_name, group: group, access_level: Gitlab::Access::MASTER)
@@ -146,6 +156,7 @@ describe ProtectedBranch do
 
     context 'multiple access levels' do
       before do
+        project.add_developer(user)
         subject.unprotect_access_levels.create!(user: maintainer)
         subject.unprotect_access_levels.create!(user: user)
       end

ee/spec/models/ee/protected_ref_access_spec.rb

@@ -10,6 +10,14 @@ describe EE::ProtectedRefAccess do
     context "in #{included_in_class}" do
       let(:factory_name) { included_in_class.name.underscore.tr('/', '_') }
       let(:access_level) { build(factory_name) }
+      let(:project) { access_level.project }
+      let(:user) { create(:user) }
+      let(:group) { create(:group) }
+
+      before do
+        project.add_developer(user)
+        project.project_group_links.create(group: group)
+      end
 
       it "#{included_in_class} includes {described_class}" do
         expect(included_in_class.included_modules).to include(described_class)
@@ -20,17 +28,19 @@ describe EE::ProtectedRefAccess do
           stub_licensed_features(protected_refs_for_users: false)
         end
 
-        it "allows creating an #{included_in_class} with a group" do
-          access_level.group = create(:group)
+        it "does not allow to create an #{included_in_class} with a group" do
+          access_level.group = group
 
           expect(access_level).not_to be_valid
+          expect(access_level.errors.count).to eq 1
           expect(access_level.errors).to include(:group)
         end
 
-        it "allows creating an #{included_in_class} with a user" do
-          access_level.user = create(:user)
+        it "does not allow to create an #{included_in_class} with a user" do
+          access_level.user = user
 
           expect(access_level).not_to be_valid
+          expect(access_level.errors.count).to eq 1
           expect(access_level.errors).to include(:user)
         end
       end
@@ -41,16 +51,32 @@ describe EE::ProtectedRefAccess do
         end
 
         it "allows creating an #{included_in_class} with a group" do
-          access_level.group = create(:group)
+          access_level.group = group
 
           expect(access_level).to be_valid
         end
 
+        it 'does not allow to add non member groups' do
+          access_level.group = create(:group)
+
+          expect(access_level).not_to be_valid
+          expect(access_level.errors.count).to eq 1
+          expect(access_level.errors[:group].first).to eq 'does not have access to the project'
+        end
+
         it "allows creating an #{included_in_class} with a user" do
-          access_level.user = create(:user)
+          access_level.user = user
 
           expect(access_level).to be_valid
         end
+
+        it 'does not allow to add non member users' do
+          access_level.user = create(:user)
+
+          expect(access_level).not_to be_valid
+          expect(access_level.errors.count).to eq 1
+          expect(access_level.errors[:user].first).to eq 'is not a member of the project'
+        end
       end
 
       it 'requires access_level if no user or group is specified' do
@@ -60,13 +86,15 @@ describe EE::ProtectedRefAccess do
       end
 
       it "doesn't require access_level if user specified" do
-        subject = build(factory_name, access_level: nil, user: create(:user))
+        subject = build(factory_name, access_level: nil, user: user)
+        subject.project.add_developer(subject.user)
 
         expect(subject).to be_valid
       end
 
       it "doesn't require access_level if group specified" do
         subject = build(factory_name, access_level: nil, group: create(:group))
+        subject.project.project_group_links.create(group: subject.group)
 
         expect(subject).to be_valid
       end

ee/spec/models/ee/protected_ref_spec.rb

@@ -4,8 +4,18 @@ describe EE::ProtectedRef do
   context 'for protected branches' do
     it 'deletes all related access levels' do
       protected_branch = create(:protected_branch)
-      2.times { protected_branch.merge_access_levels.create!(group: create(:group)) }
-      2.times { protected_branch.push_access_levels.create!(user: create(:user)) }
+
+      2.times do
+        group = create(:group)
+        protected_branch.project.project_group_links.create(group: group)
+        protected_branch.merge_access_levels.create!(group: group)
+      end
+
+      2.times do
+        user = create(:user)
+        protected_branch.project.add_developer(user)
+        protected_branch.push_access_levels.create!(user: user)
+      end
 
       protected_branch.destroy
 

ee/spec/policies/protected_branch_policy_spec.rb

@@ -11,6 +11,7 @@ describe ProtectedBranchPolicy do
 
   before do
     project.add_maintainer(user)
+    project.project_group_links.create(group: allowed_group)
   end
 
   context 'when unprotection is limited by access levels' do

spec/lib/gitlab/git_access_spec.rb

@@ -776,10 +776,13 @@ describe Gitlab::GitAccess do
         it "has the correct permissions for #{role}s" do
           if role == :admin
             user.update_attribute(:admin, true)
+            project.add_guest(user)
           else
             project.add_role(user, role)
           end
 
+          protected_branch.save
+
           aggregate_failures do
             matrix.each do |action, allowed|
               check = -> { push_changes(changes[action]) }
@@ -805,6 +808,8 @@ describe Gitlab::GitAccess do
             .project_group_links
             .create(group: group, group_access: Gitlab::Access.sym_options[role])
 
+          protected_branch.save
+
           aggregate_failures do
             matrix.each do |action, allowed|
               check = -> { push_changes(changes[action]) }
@@ -886,25 +891,19 @@ describe Gitlab::GitAccess do
 
     [%w(feature exact), ['feat*', 'wildcard']].each do |protected_branch_name, protected_branch_type|
       context do
-        before do
-          create(:protected_branch, :maintainers_can_push, name: protected_branch_name, project: project)
-        end
+        let(:protected_branch) { create(:protected_branch, :maintainers_can_push, name: protected_branch_name, project: project) }
 
         run_permission_checks(permissions_matrix)
       end
 
       context "when developers are allowed to push into the #{protected_branch_type} protected branch" do
-        before do
-          create(:protected_branch, :maintainers_can_push, :developers_can_push, name: protected_branch_name, project: project)
-        end
+        let(:protected_branch) { create(:protected_branch, :developers_can_push, name: protected_branch_name, project: project) }
 
         run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true }))
       end
 
       context "developers are allowed to merge into the #{protected_branch_type} protected branch" do
-        before do
-          create(:protected_branch, :maintainers_can_push, :developers_can_merge, name: protected_branch_name, project: project)
-        end
+        let(:protected_branch) { create(:protected_branch, :developers_can_merge, name: protected_branch_name, project: project) }
 
         context "when a merge request exists for the given source/target branch" do
           context "when the merge request is in progress" do
@@ -931,20 +930,16 @@ describe Gitlab::GitAccess do
       end
 
       context "when developers are allowed to push and merge into the #{protected_branch_type} protected branch" do
-        before do
-          create(:protected_branch, :maintainers_can_push, :developers_can_merge, :developers_can_push, name: protected_branch_name, project: project)
-        end
+        let(:protected_branch) { create(:protected_branch, :developers_can_merge, :developers_can_push, name: protected_branch_name, project: project) }
 
         run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true }))
       end
 
       context "user-specific access control" do
-        context "when a specific user is allowed to push into the #{protected_branch_type} protected branch" do
         let(:user) { create(:user) }
 
-          before do
-            create(:protected_branch, authorize_user_to_push: user, name: protected_branch_name, project: project)
-          end
+        context "when a specific user is allowed to push into the #{protected_branch_type} protected branch" do
+          let(:protected_branch) { build(:protected_branch, authorize_user_to_push: user, name: protected_branch_name, project: project) }
 
           run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true },
                                                               guest: { push_protected_branch: false, merge_into_protected_branch: false },
@@ -952,11 +947,10 @@ describe Gitlab::GitAccess do
         end
 
         context "when a specific user is allowed to merge into the #{protected_branch_type} protected branch" do
-          let(:user) { create(:user) }
+          let(:protected_branch) { build(:protected_branch, authorize_user_to_merge: user, name: protected_branch_name, project: project) }
 
           before do
             create(:merge_request, source_project: project, source_branch: unprotected_branch, target_branch: 'feature', state: 'locked', in_progress_merge_commit_sha: merge_into_protected_branch)
-            create(:protected_branch, authorize_user_to_merge: user, name: protected_branch_name, project: project)
           end
 
           run_permission_checks(permissions_matrix.deep_merge(admin: { push_protected_branch: false, push_all: false, merge_into_protected_branch: true },
@@ -967,11 +961,10 @@ describe Gitlab::GitAccess do
         end
 
         context "when a specific user is allowed to push & merge into the #{protected_branch_type} protected branch" do
-          let(:user) { create(:user) }
+          let(:protected_branch) { build(:protected_branch, authorize_user_to_push: user, authorize_user_to_merge: user, name: protected_branch_name, project: project) }
 
           before do
             create(:merge_request, source_project: project, source_branch: unprotected_branch, target_branch: 'feature', state: 'locked', in_progress_merge_commit_sha: merge_into_protected_branch)
-            create(:protected_branch, authorize_user_to_push: user, authorize_user_to_merge: user, name: protected_branch_name, project: project)
           end
 
           run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true },
@@ -981,15 +974,16 @@ describe Gitlab::GitAccess do
       end
 
       context "group-specific access control" do
-        context "when a specific group is allowed to push into the #{protected_branch_type} protected branch" do
         let(:user) { create(:user) }
         let(:group) { create(:group) }
 
         before do
           group.add_maintainer(user)
-            create(:protected_branch, authorize_group_to_push: group, name: protected_branch_name, project: project)
         end
 
+        context "when a specific group is allowed to push into the #{protected_branch_type} protected branch" do
+          let(:protected_branch) { build(:protected_branch, authorize_group_to_push: group, name: protected_branch_name, project: project) }
+
           permissions = permissions_matrix.except(:admin).deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true },
                                                                      guest: { push_protected_branch: false, merge_into_protected_branch: false },
                                                                      reporter: { push_protected_branch: false, merge_into_protected_branch: false })
@@ -998,13 +992,10 @@ describe Gitlab::GitAccess do
         end
 
         context "when a specific group is allowed to merge into the #{protected_branch_type} protected branch" do
-          let(:user) { create(:user) }
-          let(:group) { create(:group) }
+          let(:protected_branch) { build(:protected_branch, authorize_group_to_merge: group, name: protected_branch_name, project: project) }
 
           before do
-            group.add_maintainer(user)
             create(:merge_request, source_project: project, source_branch: unprotected_branch, target_branch: 'feature', state: 'locked', in_progress_merge_commit_sha: merge_into_protected_branch)
-            create(:protected_branch, authorize_group_to_merge: group, name: protected_branch_name, project: project)
           end
 
           permissions = permissions_matrix.except(:admin).deep_merge(maintainer: { push_protected_branch: false, push_all: false, merge_into_protected_branch: true },
@@ -1016,13 +1007,10 @@ describe Gitlab::GitAccess do
         end
 
         context "when a specific group is allowed to push & merge into the #{protected_branch_type} protected branch" do
-          let(:user) { create(:user) }
-          let(:group) { create(:group) }
+          let(:protected_branch) { build(:protected_branch, authorize_group_to_push: group, authorize_group_to_merge: group, name: protected_branch_name, project: project) }
 
           before do
-            group.add_maintainer(user)
             create(:merge_request, source_project: project, source_branch: unprotected_branch, target_branch: 'feature', state: 'locked', in_progress_merge_commit_sha: merge_into_protected_branch)
-            create(:protected_branch, authorize_group_to_push: group, authorize_group_to_merge: group, name: protected_branch_name, project: project)
           end
 
           permissions = permissions_matrix.except(:admin).deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true },
@@ -1034,9 +1022,7 @@ describe Gitlab::GitAccess do
       end
 
       context "when no one is allowed to push to the #{protected_branch_name} protected branch" do
-        before do
-          create(:protected_branch, :no_one_can_push, name: protected_branch_name, project: project)
-        end
+        let(:protected_branch) { build(:protected_branch, :no_one_can_push, name: protected_branch_name, project: project) }
 
         run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: false, push_all: false, merge_into_protected_branch: false },
                                                             maintainer: { push_protected_branch: false, push_all: false, merge_into_protected_branch: false },

spec/requests/api/protected_branches_spec.rb

@@ -74,6 +74,9 @@ describe API::ProtectedBranches do
         let(:unprotect_group) { create(:group) }
 
         before do
+          project.add_developer(push_user)
+          project.project_group_links.create(group: merge_group)
+          project.project_group_links.create(group: unprotect_group)
           protected_branch.push_access_levels.create!(user: push_user)
           protected_branch.merge_access_levels.create!(group: merge_group)
           protected_branch.unprotect_access_levels.create!(group: unprotect_group)
@@ -282,7 +285,7 @@ describe API::ProtectedBranches do
           post post_endpoint, params: { name: branch_name, allowed_to_merge: [{ user_id: push_user.id }] }
 
           expect(response).to have_gitlab_http_status(422)
-          expect(json_response['message'][0]).to match(/Cannot add users or groups/)
+          expect(json_response['message'][0]).to match(/is not a member of the project/)
         end
 
         it "fails if groups aren't all invited to the project" do
@@ -291,7 +294,7 @@ describe API::ProtectedBranches do
           post post_endpoint, params: { name: branch_name, allowed_to_merge: [{ group_id: merge_group.id }] }
 
           expect(response).to have_gitlab_http_status(422)
-          expect(json_response['message'][0]).to match(/Cannot add users or groups/)
+          expect(json_response['message'][0]).to match(/does not have access to the project/)
         end
 
         it 'avoids creating default access levels unless necessary' do