Merge branch 'security-fix-protected-environment-access-cleanup' into 'master'

Unauthorized User Can Trigger Deployment to the Protected Environment

See merge request gitlab-org/security/gitlab!1559

ee/app/models/ee/project_member.rb

@@ -11,6 +11,7 @@ module EE
       validate :gma_enforcement, if: :group, unless: :project_bot
 
       before_destroy :delete_member_branch_protection
+      before_destroy :delete_protected_environment_acceses
     end
 
     def group
@@ -28,6 +29,12 @@ module EE
       end
     end
 
+    def delete_protected_environment_acceses
+      return unless user.present? && project.present?
+
+      project.protected_environments.deploy_access_levels_by_user(user).delete_all
+    end
+
     def gma_enforcement
       unless ::Gitlab::Auth::GroupSaml::GmaMembershipEnforcer.new(project).can_add_user?(user)
         errors.add(:user, _('is not in the group enforcing Group Managed Account'))

ee/app/models/protected_environment.rb

@@ -22,12 +22,19 @@ class ProtectedEnvironment < ApplicationRecord
              ' AND protected_environments.project_id = environments.project_id')
   end
 
-  scope :deploy_access_levels_by_group, -> (group) do
-    ProtectedEnvironment::DeployAccessLevel
-      .joins(:protected_environment).where(group: group)
-  end
-
   class << self
+    def deploy_access_levels_by_user(user)
+      ProtectedEnvironment::DeployAccessLevel
+        .where(protected_environment_id: select(:id))
+        .where(user: user)
+    end
+
+    def deploy_access_levels_by_group(group)
+      ProtectedEnvironment::DeployAccessLevel
+        .where(protected_environment_id: select(:id))
+        .where(group: group)
+    end
+
     def for_environment(environment)
       raise ArgumentError unless environment.is_a?(::Environment)
 

ee/app/services/protected_environments/base_service.rb

@@ -24,6 +24,11 @@ module ProtectedEnvironments
       keys = attribute.slice(:access_level, :group_id, :user_id).keys
       return false unless keys.count == 1
 
+      # If it's a destroy request, any group/user IDs are allowed to be passed,
+      # so that users who are no longer project members can be removed from the access list.
+      # `has_destroy_flag?` is defined in `ActiveRecord::NestedAttributes`.
+      return true if ProtectedEnvironment::DeployAccessLevel.new.send(:has_destroy_flag?, attribute) # rubocop:disable GitlabSecurity/PublicSend
+
       if attribute[:group_id].present?
         qualified_group_ids.include?(attribute[:group_id])
       elsif attribute[:user_id].present?

ee/spec/models/project_member_spec.rb

@@ -96,4 +96,67 @@ RSpec.describe ProjectMember do
 
     it { is_expected.to eq(false) }
   end
+
+  describe 'delete protected environment acceses cascadingly' do
+    let_it_be(:project) { create(:project) }
+    let_it_be(:user) { create(:user) }
+    let_it_be(:environment) { create(:environment, project: project) }
+    let_it_be(:protected_environment) do
+      create(:protected_environment, project: project, name: environment.name)
+    end
+
+    let!(:member) { create(:project_member, project: project, user: user) }
+
+    let!(:deploy_access) do
+      create(:protected_environment_deploy_access_level, protected_environment: protected_environment, user: user)
+    end
+
+    let!(:deploy_access_for_diffent_user) do
+      create(:protected_environment_deploy_access_level, protected_environment: protected_environment, user: create(:user))
+    end
+
+    let!(:deploy_access_for_group) do
+      create(:protected_environment_deploy_access_level, protected_environment: protected_environment, group: create(:group))
+    end
+
+    let!(:deploy_access_for_maintainer_role) do
+      create(:protected_environment_deploy_access_level, :maintainer_access, protected_environment: protected_environment)
+    end
+
+    it 'deletes associated protected environment access cascadingly' do
+      expect { member.destroy! }
+        .to change { ProtectedEnvironment::DeployAccessLevel.count }.by(-1)
+
+      expect { deploy_access.reload }.to raise_error(ActiveRecord::RecordNotFound)
+      expect(protected_environment.reload.deploy_access_levels)
+        .to include(deploy_access_for_diffent_user, deploy_access_for_group, deploy_access_for_maintainer_role)
+    end
+
+    context 'when the user is assiged to multiple protected environments in the same project' do
+      let!(:other_protected_environment) { create(:protected_environment, project: project, name: 'staging') }
+      let!(:other_deploy_access) { create(:protected_environment_deploy_access_level, protected_environment: other_protected_environment, user: user) }
+
+      it 'deletes all associated protected environment accesses in the project' do
+        expect { member.destroy! }
+          .to change { ProtectedEnvironment::DeployAccessLevel.count }.by(-2)
+
+        expect { deploy_access.reload }.to raise_error(ActiveRecord::RecordNotFound)
+        expect { other_deploy_access.reload }.to raise_error(ActiveRecord::RecordNotFound)
+      end
+    end
+
+    context 'when the user is assiged to multiple protected environments across different projects' do
+      let!(:other_project) { create(:project) }
+      let!(:other_protected_environment) { create(:protected_environment, project: other_project, name: 'staging') }
+      let!(:other_deploy_access) { create(:protected_environment_deploy_access_level, protected_environment: other_protected_environment, user: user) }
+
+      it 'deletes all associated protected environment accesses in the project' do
+        expect { member.destroy! }
+          .to change { ProtectedEnvironment::DeployAccessLevel.count }.by(-1)
+
+        expect { deploy_access.reload }.to raise_error(ActiveRecord::RecordNotFound)
+        expect { other_deploy_access.reload }.not_to raise_error
+      end
+    end
+  end
 end

ee/spec/models/protected_environment_spec.rb

@@ -59,7 +59,7 @@ RSpec.describe ProtectedEnvironment do
 
     context 'when access has been granted to user' do
       before do
-        create_deploy_access_level(user: user)
+        create_deploy_access_level(protected_environment, user: user)
       end
 
       it { is_expected.to be_truthy }
@@ -69,7 +69,7 @@ RSpec.describe ProtectedEnvironment do
       let(:group) { create(:group) }
 
       before do
-        create_deploy_access_level(group: group)
+        create_deploy_access_level(protected_environment, group: group)
       end
 
       it 'allows members of the group' do
@@ -85,7 +85,7 @@ RSpec.describe ProtectedEnvironment do
 
     context 'when access has been granted to maintainers' do
       before do
-        create_deploy_access_level(access_level: Gitlab::Access::MAINTAINER)
+        create_deploy_access_level(protected_environment, access_level: Gitlab::Access::MAINTAINER)
       end
 
       it 'allows maintainers' do
@@ -103,7 +103,7 @@ RSpec.describe ProtectedEnvironment do
 
     context 'when access has been granted to developers' do
       before do
-        create_deploy_access_level(access_level: Gitlab::Access::DEVELOPER)
+        create_deploy_access_level(protected_environment, access_level: Gitlab::Access::DEVELOPER)
       end
 
       it 'allows maintainers' do
@@ -198,18 +198,63 @@ RSpec.describe ProtectedEnvironment do
     end
   end
 
+  describe '.deploy_access_levels_by_user' do
+    let(:user) { create(:user) }
+    let(:project) { create(:project) }
+    let(:environment) { create(:environment, project: project, name: 'production') }
+    let(:protected_environment) { create(:protected_environment, project: project, name: 'production') }
+    let(:deploy_access_level_for_user) { create_deploy_access_level(protected_environment, user: user) }
+
+    before do
+      create_deploy_access_level(protected_environment, user: create(:user))
+      create_deploy_access_level(protected_environment, group: create(:group))
+    end
+
+    it 'returns matching deploy access levels for the given user' do
+      expect(described_class.deploy_access_levels_by_user(user))
+        .to contain_exactly(deploy_access_level_for_user)
+    end
+
+    context 'when user is assigned to protected environment in the other project' do
+      let(:other_project) { create(:project) }
+      let(:other_protected_environment) { create(:protected_environment, project: other_project, name: 'production') }
+      let(:other_deploy_access_level_for_user) { create_deploy_access_level(other_protected_environment, user: user) }
+
+      it 'returns matching deploy access levels for the given user in the specific project' do
+        expect(project.protected_environments.deploy_access_levels_by_user(user))
+          .to contain_exactly(deploy_access_level_for_user)
+        expect(other_project.protected_environments.deploy_access_levels_by_user(user))
+          .to contain_exactly(other_deploy_access_level_for_user)
+      end
+    end
+  end
+
   describe '.deploy_access_levels_by_group' do
     let(:group) { create(:group) }
     let(:project) { create(:project) }
     let(:environment) { create(:environment, project: project, name: 'production') }
     let(:protected_environment) { create(:protected_environment, project: project, name: 'production') }
+    let(:deploy_access_level_for_group) { create_deploy_access_level(protected_environment, group: group) }
 
     it 'returns matching deploy access levels for the given group' do
-      _deploy_access_level_for_different_group = create_deploy_access_level(group: create(:group))
-      _deploy_access_level_for_user = create_deploy_access_level(user: create(:user))
-      deploy_access_level = create_deploy_access_level(group: group)
+      _deploy_access_level_for_different_group = create_deploy_access_level(protected_environment, group: create(:group))
+      _deploy_access_level_for_user = create_deploy_access_level(protected_environment, user: create(:user))
 
-      expect(described_class.deploy_access_levels_by_group(group)).to contain_exactly(deploy_access_level)
+      expect(described_class.deploy_access_levels_by_group(group))
+        .to contain_exactly(deploy_access_level_for_group)
+    end
+
+    context 'when user is assigned to protected environment in the other project' do
+      let(:other_project) { create(:project) }
+      let(:other_protected_environment) { create(:protected_environment, project: other_project, name: 'production') }
+      let(:other_deploy_access_level_for_group) { create_deploy_access_level(other_protected_environment, group: group) }
+
+      it 'returns matching deploy access levels for the given group in the specific project' do
+        expect(project.protected_environments.deploy_access_levels_by_group(group))
+          .to contain_exactly(deploy_access_level_for_group)
+        expect(other_project.protected_environments.deploy_access_levels_by_group(group))
+          .to contain_exactly(other_deploy_access_level_for_group)
+      end
     end
   end
 
@@ -280,7 +325,7 @@ RSpec.describe ProtectedEnvironment do
     end
   end
 
-  def create_deploy_access_level(**opts)
+  def create_deploy_access_level(protected_environment, **opts)
     protected_environment.deploy_access_levels.create(**opts)
   end
 end

ee/spec/services/protected_environments/base_service_spec.rb

@@ -38,6 +38,28 @@ RSpec.describe ProtectedEnvironments::BaseService, '#execute' do
             ]
           )
         end
+
+        context 'with delete flag' do
+          let(:params) do
+            {
+              deploy_access_levels_attributes: [
+                { group_id: group.id },
+                { group_id: other_group.id, '_destroy' => 1 },
+                { group_id: child_group.id }
+              ]
+            }
+          end
+
+          it 'contains inappropriate group id for deleting it' do
+            is_expected.to eq(
+              deploy_access_levels_attributes: [
+                { group_id: group.id },
+                { group_id: other_group.id, '_destroy' => 1 },
+                { group_id: child_group.id }
+              ]
+            )
+          end
+        end
       end
 
       context 'with user-based access control' do
@@ -71,6 +93,23 @@ RSpec.describe ProtectedEnvironments::BaseService, '#execute' do
             ]
           )
         end
+
+        context 'with delte flag' do
+          let(:params) do
+            {
+              deploy_access_levels_attributes: [
+                { user_id: group_maintainer.id },
+                { user_id: group_developer.id, '_destroy' => 1 },
+                { user_id: other_group_maintainer.id, '_destroy' => 1 },
+                { user_id: child_group_maintainer.id, '_destroy' => 1 }
+              ]
+            }
+          end
+
+          it 'contains inappropriate user ids for deleting it' do
+            is_expected.to eq(params)
+          end
+        end
       end
     end
   end