Merge branch 'security-idor-protected-environment-groups' into 'master'

Prevent IDOR when adding groups to protected environments

See merge request gitlab/gitlab-ee!1184

ee/app/services/protected_environments/base_service.rb

+# frozen_string_literal: true
+module ProtectedEnvironments
+  class BaseService < ::BaseService
+    include Gitlab::Utils::StrongMemoize
+
+    protected
+
+    def sanitized_params
+      params.dup.tap do |sanitized_params|
+        sanitized_params[:deploy_access_levels_attributes] =
+          filter_valid_groups(sanitized_params[:deploy_access_levels_attributes])
+      end
+    end
+
+    private
+
+    def filter_valid_groups(attributes)
+      return unless attributes
+
+      attributes.select do |attribute|
+        attribute[:group_id].nil? || invited_group_ids.include?(attribute[:group_id])
+      end
+    end
+
+    def invited_group_ids
+      strong_memoize(:invited_group_ids) do
+        project.invited_groups.pluck_primary_key.to_set
+      end
+    end
+  end
+end

ee/app/services/protected_environments/create_service.rb

 # frozen_string_literal: true
 module ProtectedEnvironments
-  class CreateService < BaseService
+  class CreateService < ProtectedEnvironments::BaseService
     def execute
-      project.protected_environments.create(params)
+      project.protected_environments.create(sanitized_params)
     end
   end
 end

ee/app/services/protected_environments/update_service.rb

 # frozen_string_literal: true
 module ProtectedEnvironments
-  class UpdateService < BaseService
+  class UpdateService < ProtectedEnvironments::BaseService
     def execute(protected_environment)
-      protected_environment.update(params)
+      protected_environment.update(sanitized_params)
     end
   end
 end

ee/changelogs/unreleased/security-idor-protected-environment-groups.yml

+---
+title: Prevent IDOR when adding groups to protected environments
+merge_request:
+author:
+type: security

ee/spec/services/protected_environments/create_service_spec.rb

@@ -32,4 +32,27 @@ describe ProtectedEnvironments::CreateService, '#execute' do
       expect(subject.persisted?).to be_falsy
     end
   end
+
+  context 'deploy access level by group' do
+    let(:params) do
+      attributes_for(:protected_environment,
+                     deploy_access_levels_attributes: [{ group_id: group.id }])
+    end
+
+    context 'invalid group' do
+      it_behaves_like 'invalid protected environment group' do
+        it 'does not create protected environment' do
+          expect { subject }.not_to change(ProtectedEnvironment, :count)
+        end
+      end
+    end
+
+    context 'valid group' do
+      it_behaves_like 'valid protected environment group' do
+        it 'creates protected environment' do
+          expect { subject }.to change(ProtectedEnvironment, :count).by(1)
+        end
+      end
+    end
+  end
 end

ee/spec/services/protected_environments/update_service_spec.rb

@@ -44,4 +44,16 @@ describe ProtectedEnvironments::UpdateService, '#execute' do
       end.not_to change { ProtectedEnvironment::DeployAccessLevel.count }
     end
   end
+
+  context 'deploy access level by group' do
+    let(:params) { { deploy_access_levels_attributes: [{ group_id: group.id }] } }
+
+    context 'invalid group' do
+      it_behaves_like 'invalid protected environment group'
+    end
+
+    context 'valid group' do
+      it_behaves_like 'valid protected environment group'
+    end
+  end
 end

ee/spec/support/shared_examples/services/protected_environments_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'invalid protected environment group' do
+  let(:group) { create(:group, :private) }
+
+  it 'does not create deploy access level' do
+    expect { subject }.not_to change(ProtectedEnvironment::DeployAccessLevel, :count)
+  end
+end
+
+RSpec.shared_examples 'valid protected environment group' do
+  let(:project_group_link) { create(:project_group_link) }
+  let(:group) { project_group_link.group }
+  let(:project) { project_group_link.project }
+
+  it 'creates deploy access level' do
+    expect { subject }.to change(ProtectedEnvironment::DeployAccessLevel, :count).by(1)
+  end
+end