From 03f45faa96722c7b5ac1d70409efc525c889569f Mon Sep 17 00:00:00 2001
From: Joe Woodward <j@joewoodward.me>
Date: Thu, 20 Jun 2024 16:19:30 +0100
Subject: [PATCH] Move membership check when authenticated with deploy key

We fixed a bug in
https://gitlab.com/gitlab-org/gitlab/-/merge_requests/132380+ which
allowed owners of a deploy key to access projects that they were no
longer members of.

This exploit consisted of gaining membership to a
project, uploading a deploy key, removing the membership, then pushing
to the project using the deploy key.

This exploit was possible because we only checked that the deploy key
was active and not that the owner was a member during the checks flow.

To fix this we have added a condition in the
ProtectedRefAccess#check_access method to check if the user is a project
member.

While this does fix the bug, the issue is specific to deploy keys so
doesn't really make sense to check membership for role, user, or group
based access levels.

- For role based levels we check the users max membership within the
project's team so we are already checking they are a member.
- For user based levels we don't want to check if the user is a member as
non-members should also be allowed to perform the actions. We exit early
so this doesn't affect those access levels.
- For groups we don't want check if the user is a member of the
project, we check if the group has access to the project and if the user
is a member of the group. Again we exit early so this doesn't affect
group access levels.

This change moves the membership check to the correct location. Now we
will only check if the user is a project member when we are checking
deploy_key based access levels.

Related to https://gitlab.com/gitlab-org/gitlab/-/issues/461213+
---
 app/models/concerns/protected_ref_access.rb   |   3 -
 .../protected_ref_deploy_key_access.rb        |   7 ++
 .../concerns/ee/protected_ref_access.rb       |  12 +-
 .../protected_ref_access_shared_examples.rb   |  62 ++++++----
 .../protected_ref_access_shared_examples.rb   |  77 ++++--------
 ...d_ref_deploy_key_access_shared_examples.rb | 111 ++++++++++++------
 6 files changed, 154 insertions(+), 118 deletions(-)

diff --git a/app/models/concerns/protected_ref_access.rb b/app/models/concerns/protected_ref_access.rb
index 19d8c359a3f0ff..583e93b51c1c1f 100644
--- a/app/models/concerns/protected_ref_access.rb
+++ b/app/models/concerns/protected_ref_access.rb
@@ -72,9 +72,6 @@ def check_access(current_user, current_project = project)
     return false if current_user.nil? || no_access?
     return current_user.admin? if admin_access?
 
-    return false if Feature.enabled?(:check_membership_in_protected_ref_access) &&
-      (current_project && !current_project.member?(current_user))
-
     yield if block_given?
 
     user_can_access?(current_user, current_project)
diff --git a/app/models/concerns/protected_ref_deploy_key_access.rb b/app/models/concerns/protected_ref_deploy_key_access.rb
index 6a45d6d274f3cf..986e621131ab29 100644
--- a/app/models/concerns/protected_ref_deploy_key_access.rb
+++ b/app/models/concerns/protected_ref_deploy_key_access.rb
@@ -59,10 +59,17 @@ def validate_deploy_key_membership
   def enabled_deploy_key_for_user?(current_user)
     current_user.can?(:read_project, project) &&
       deploy_key.user_id == current_user.id &&
+      active_project_member?(current_user) &&
       deploy_key_has_write_access_to_project?
   end
 
   def deploy_key_has_write_access_to_project?
     DeployKey.with_write_access_for_project(project, deploy_key: deploy_key).exists?
   end
+
+  def active_project_member?(current_user)
+    return true if Feature.disabled?(:check_membership_in_protected_ref_access) # rubocop:disable Gitlab/FeatureFlagWithoutActor -- This flag already exists, I don't want to add an actor and break it.
+
+    project.member?(current_user)
+  end
 end
diff --git a/ee/app/models/concerns/ee/protected_ref_access.rb b/ee/app/models/concerns/ee/protected_ref_access.rb
index 623effca930813..1f0b73ab841cd3 100644
--- a/ee/app/models/concerns/ee/protected_ref_access.rb
+++ b/ee/app/models/concerns/ee/protected_ref_access.rb
@@ -69,7 +69,7 @@ def humanize
     override :check_access
     def check_access(current_user, current_project = project)
       super do
-        break current_user.id == user_id if user?
+        break user_access_allowed?(current_user) if user?
         break group_access_allowed?(current_user) if group?
 
         yield if block_given?
@@ -86,6 +86,10 @@ def humanize_group
       group&.name || 'Group'
     end
 
+    def user_access_allowed?(current_user)
+      current_user.id == user_id && active_project_member?(current_user)
+    end
+
     def group_access_allowed?(current_user)
       group.has_user?(current_user)
     end
@@ -121,5 +125,11 @@ def validate_user_membership
 
       errors.add(:user, 'is not a member of the project')
     end
+
+    def active_project_member?(current_user)
+      return true if ::Feature.disabled?(:check_membership_in_protected_ref_access) # rubocop:disable Gitlab/FeatureFlagWithoutActor -- This flag already exists, I don't want to add an actor and break it.
+
+      project.member?(current_user)
+    end
   end
 end
diff --git a/ee/spec/support/shared_examples/models/concerns/protected_ref_access_shared_examples.rb b/ee/spec/support/shared_examples/models/concerns/protected_ref_access_shared_examples.rb
index 567072a3492dda..6769ce8f66753e 100644
--- a/ee/spec/support/shared_examples/models/concerns/protected_ref_access_shared_examples.rb
+++ b/ee/spec/support/shared_examples/models/concerns/protected_ref_access_shared_examples.rb
@@ -248,9 +248,9 @@
       :group,            :group_id, :user,            :user_id, :expectation
     ) do
       ref(:test_group) | nil      | nil             | nil     | :group
-      nil              | 1        | nil             | nil     | :group
+      nil              | 0        | nil             | nil     | :group
       nil              | nil      | ref(:test_user) | nil     | :user
-      nil              | nil      | nil             | 1       | :user
+      nil              | nil      | nil             | 0       | :user
     end
 
     with_them do
@@ -274,9 +274,9 @@
       :group,            :group_id, :user,            :user_id, :expectation
     ) do
       ref(:test_group) | nil      | nil             | nil     | lazy { test_group.name }
-      nil              | 1        | nil             | nil     | 'Group'
+      nil              | 0        | nil             | nil     | 'Group'
       nil              | nil      | ref(:test_user) | nil     | lazy { test_user.name }
-      nil              | nil      | nil             | 1       | 'User'
+      nil              | nil      | nil             | 0       | 'User'
     end
 
     with_them do
@@ -298,37 +298,49 @@
   let_it_be(:project) { create(:project) }
   let_it_be(:protected_ref) { create(association, project: project) }
 
-  describe '#check_access' do
+  describe '#check_access(current_user, current_project)' do
     let_it_be(:current_user) { create(:user) }
 
-    let(:access_level) { nil }
     let(:user) { nil }
     let(:group) { nil }
+    let(:current_project) { project }
+    let(:described_instance) do
+      described_class.new(
+        association => protected_ref,
+        user: user,
+        group: group
+      )
+    end
 
     before_all do
       project.add_maintainer(current_user)
     end
 
     subject do
-      described_class.new(
-        association => protected_ref,
-        user: user,
-        group: group,
-        access_level: access_level
-      )
+      described_instance.check_access(current_user, current_project)
     end
 
     context 'when user is assigned' do
       context 'when current_user is the user' do
         let(:user) { current_user }
 
-        it { expect(subject.check_access(current_user)).to eq(true) }
+        context 'when user is a project member' do
+          it { is_expected.to eq(true) }
+        end
+
+        context 'when user is not a project member' do
+          before do
+            allow(project).to receive(:member?).with(user).and_return(false)
+          end
+
+          it { is_expected.to eq(false) }
+        end
       end
 
       context 'when current_user is another user' do
         let(:user) { create(:user) }
 
-        it { expect(subject.check_access(current_user)).to eq(false) }
+        it { is_expected.to eq(false) }
       end
     end
   end
@@ -338,24 +350,26 @@
   let_it_be(:project) { create(:project) }
   let_it_be(:protected_ref) { create(association, project: project) }
 
-  describe '#check_access' do
+  describe '#check_access(current_user, current_project)' do
     let_it_be(:current_user) { create(:user) }
 
-    let(:access_level) { nil }
     let(:user) { nil }
     let(:group) { nil }
+    let(:current_project) { project }
+    let(:described_instance) do
+      described_class.new(
+        association => protected_ref,
+        user: user,
+        group: group
+      )
+    end
 
     before_all do
       project.add_maintainer(current_user)
     end
 
     subject do
-      described_class.new(
-        association => protected_ref,
-        user: user,
-        group: group,
-        access_level: access_level
-      )
+      described_instance.check_access(current_user, current_project)
     end
 
     context 'when group is assigned' do
@@ -366,11 +380,11 @@
           group.add_developer(current_user)
         end
 
-        it { expect(subject.check_access(current_user)).to eq(true) }
+        it { is_expected.to eq(true) }
       end
 
       context 'when current_user is not in the group' do
-        it { expect(subject.check_access(current_user)).to eq(false) }
+        it { is_expected.to eq(false) }
       end
     end
   end
diff --git a/spec/support/shared_examples/models/concerns/protected_ref_access_shared_examples.rb b/spec/support/shared_examples/models/concerns/protected_ref_access_shared_examples.rb
index 9e2a51822d528c..b11bd4a008b4da 100644
--- a/spec/support/shared_examples/models/concerns/protected_ref_access_shared_examples.rb
+++ b/spec/support/shared_examples/models/concerns/protected_ref_access_shared_examples.rb
@@ -51,7 +51,7 @@
     it { is_expected.to eq(levels) }
   end
 
-  describe '#check_access' do
+  describe '#check_access(user, current_project)' do
     let_it_be(:group) { create(:group) }
     # Making a project public to avoid false positives tests
     let_it_be(:project) { create(:project, :public, group: group) }
@@ -59,74 +59,43 @@
     let_it_be(:protected_ref) { create(association, project: project) }
 
     let(:access_level) { ::Gitlab::Access::DEVELOPER }
-
-    before_all do
-      project.add_developer(current_user)
-    end
-
-    subject do
+    let(:current_project) { project }
+    let(:described_instance) do
       described_class.new(
         association => protected_ref,
         access_level: access_level
       )
     end
 
-    context 'when current_user is nil' do
-      it { expect(subject.check_access(nil)).to eq(false) }
+    before_all do
+      project.add_developer(current_user)
     end
 
-    context 'when current_user access exists without membership' do
-      let(:other_user) { create(:user) }
-      let(:user_access) do
-        described_class.new(association => protected_ref, access_level: access_level, user_id: other_user.id)
-      end
-
-      let(:enable_ff) { false }
-
-      before do
-        stub_feature_flags(check_membership_in_protected_ref_access: enable_ff)
-      end
-
-      it 'does not check membership if check_membership_in_protected_ref_access FF is disabled' do
-        expect(project).not_to receive(:member?).with(other_user)
-
-        user_access.check_access(other_user)
-      end
-
-      context 'when check_membership_in_protected_ref_access FF is enabled' do
-        let(:enable_ff) { true }
-
-        it 'does check membership' do
-          expect(project).to receive(:member?).with(other_user)
-
-          user_access.check_access(other_user)
-        end
-
-        it 'returns false' do
-          expect(user_access.check_access(other_user)).to be_falsey
-        end
+    subject do
+      described_instance.check_access(current_user, current_project)
+    end
 
-        context 'when user has inherited membership' do
-          let!(:inherited_membership) { create(:group_member, group: group, user: other_user) }
+    context 'when current_user is nil' do
+      let(:current_user) { nil }
 
-          it do
-            expect(user_access.check_access(other_user)).to be_truthy
-          end
-        end
-      end
+      it { is_expected.to eq false }
     end
 
     context 'when access_level is NO_ACCESS' do
       let(:access_level) { ::Gitlab::Access::NO_ACCESS }
 
-      it { expect(subject.check_access(current_user)).to eq(false) }
+      it { is_expected.to eq false }
     end
 
     context 'when instance admin access is configured' do
       let(:access_level) { Gitlab::Access::ADMIN }
 
       context 'when current_user is a maintainer' do
-        it { expect(subject.check_access(current_user)).to eq(false) }
+        before_all do
+          project.add_maintainer(current_user)
+        end
+
+        it { is_expected.to eq false }
       end
 
       context 'when current_user is admin' do
@@ -134,36 +103,36 @@
           allow(current_user).to receive(:admin?).and_return(true)
         end
 
-        it { expect(subject.check_access(current_user)).to eq(true) }
+        it { is_expected.to eq true }
       end
     end
 
     context 'when current_user can push_code to project' do
       context 'and member access is high enough' do
-        it { expect(subject.check_access(current_user)).to eq(true) }
+        it { is_expected.to eq true }
 
         context 'when external authorization denies access' do
           before do
             external_service_deny_access(current_user, project)
           end
 
-          it { expect(subject.check_access(current_user)).to be_falsey }
+          it { is_expected.to eq false }
         end
       end
 
       context 'and member access is too low' do
         let(:access_level) { ::Gitlab::Access::MAINTAINER }
 
-        it { expect(subject.check_access(current_user)).to eq(false) }
+        it { is_expected.to eq false }
       end
     end
 
     context 'when current_user cannot push_code to project' do
       before do
-        allow(current_user).to receive(:can?).with(:push_code, project).and_return(false)
+        allow(current_user).to receive(:can?).with(:push_code, project).and_return(false) # rubocop:disable CodeReuse/ActiveRecord -- false positive flag on `with`
       end
 
-      it { expect(subject.check_access(current_user)).to eq(false) }
+      it { is_expected.to eq false }
     end
   end
 end
diff --git a/spec/support/shared_examples/models/concerns/protected_ref_deploy_key_access_shared_examples.rb b/spec/support/shared_examples/models/concerns/protected_ref_deploy_key_access_shared_examples.rb
index 988d2f6d417fe6..f841c6baaa0839 100644
--- a/spec/support/shared_examples/models/concerns/protected_ref_deploy_key_access_shared_examples.rb
+++ b/spec/support/shared_examples/models/concerns/protected_ref_deploy_key_access_shared_examples.rb
@@ -3,7 +3,7 @@
 RSpec.shared_examples 'protected ref deploy_key access' do
   let_it_be(:described_instance) { described_class.model_name.singular }
   let_it_be(:protected_ref_name) { described_class.module_parent.model_name.singular }
-  let_it_be(:project) { create(:project) }
+  let_it_be(:project) { create(:project, :in_group) }
   let_it_be(:protected_ref) { create(protected_ref_name, project: project) } # rubocop:disable Rails/SaveBang -- False positive because factory name is dynamic
 
   describe 'associations' do
@@ -84,85 +84,124 @@
       create(:deploy_keys_project, :write_access, project: project, deploy_key: deploy_key)
     end
 
-    before_all do
-      project.add_maintainer(user)
-    end
+    subject(:perform_access_check) { access_level.check_access(user) }
 
     context "when this #{described_class.model_name.singular} is tied to a deploy key" do
       let!(:access_level) do
         create(described_instance, protected_ref_name => protected_ref, deploy_key: deploy_key)
       end
 
-      context 'when the deploy key is among the active keys for this project' do
-        it { expect(access_level.check_access(user)).to be_truthy }
-      end
+      context 'and user is not a project member' do
+        before do
+          allow(project).to receive(:public?).and_return(true)
+          stub_feature_flags(check_membership_in_protected_ref_access: enable_ff)
+        end
 
-      context 'when user is missing' do
-        it { expect(access_level.check_access(nil)).to be_falsey }
-      end
+        context 'when check_membership_in_protected_ref_access is enabled' do
+          let(:enable_ff) { true }
 
-      context 'when deploy key does not belong to the user' do
-        let(:another_user) { create(:user) }
+          it 'does check membership' do
+            expect(project).to receive(:member?).with(user)
 
-        it { expect(access_level.check_access(another_user)).to be_falsey }
-      end
+            perform_access_check
+          end
 
-      context 'when user cannot access the project' do
-        before do
-          allow(user).to receive(:can?).with(:read_project, project).and_return(false)
+          it { is_expected.to eq(false) }
+
+          context 'when user has inherited membership' do
+            let!(:inherited_membership) { create(:group_member, group: project.group, user: user) }
+
+            it { is_expected.to eq(true) }
+          end
         end
 
-        it { expect(access_level.check_access(user)).to be_falsey }
+        context 'when check_membership_in_protected_ref_access is disabled' do
+          let(:enable_ff) { false }
+
+          it 'does not check membership' do
+            expect(project).not_to receive(:member?).with(user)
+
+            perform_access_check
+          end
+        end
       end
 
-      context 'when the deploy key is not among the active keys of this project' do
-        before do
-          deploy_keys_project.update!(can_push: false)
+      context 'when the user is a project maintainer' do
+        before_all do
+          project.add_maintainer(user)
         end
 
-        after do
-          deploy_keys_project.update!(can_push: true)
+        context 'when the deploy key is among the active keys for this project' do
+          it { is_expected.to eq(true) }
         end
 
-        it { expect(access_level.check_access(user)).to be_falsey }
+        context 'when the deploy key is not among the active keys of this project' do
+          before do
+            deploy_keys_project.update!(can_push: false)
+          end
+
+          after do
+            deploy_keys_project.update!(can_push: true)
+          end
+
+          it { is_expected.to eq(false) }
+        end
+
+        context 'when user cannot access the project' do
+          before do
+            allow(user).to receive(:can?).with(:read_project, project).and_return(false)
+          end
+
+          it { is_expected.to eq(false) }
+        end
+
+        context 'when deploy key does not belong to the user' do
+          let_it_be(:deploy_key) do
+            create(:deploy_keys_project, :write_access, project: project).deploy_key
+          end
+
+          it { is_expected.to eq(false) }
+        end
+      end
+
+      context 'when user is nil' do
+        let(:user) { nil }
+
+        it { is_expected.to eq(false) }
       end
     end
   end
 
   describe '#type' do
+    subject { access_level.type }
+
     context 'when deploy_key is present and deploy_key_id is nil' do
       let(:access_level) { build(described_instance, deploy_key: build(:deploy_key)) }
 
-      it 'returns :deploy_key' do
-        expect(access_level.type).to eq(:deploy_key)
-      end
+      it { is_expected.to eq(:deploy_key) }
     end
 
     context 'when deploy_key_id is present and deploy_key is nil' do
       let(:access_level) { build(described_instance, deploy_key_id: 0) }
 
-      it 'returns :deploy_key' do
-        expect(access_level.type).to eq(:deploy_key)
-      end
+      it { is_expected.to eq(:deploy_key) }
     end
   end
 
   describe '#humanize' do
+    subject { access_level.humanize }
+
     context 'when deploy_key is present' do
       let(:deploy_key) { build(:deploy_key) }
       let(:access_level) { build(described_instance, deploy_key: deploy_key) }
 
-      it 'returns deploy_key.title' do
-        expect(access_level.humanize).to eq(deploy_key.title)
-      end
+      it { is_expected.to eq(deploy_key.title) }
     end
 
     context 'when deploy_key_id is present and deploy_key is nil' do
       let(:access_level) { build(described_instance, deploy_key_id: 0) }
 
-      it 'returns "Deploy key"' do
-        expect(access_level.humanize).to eq('Deploy key')
-      end
+      it { is_expected.to eq('Deploy key') }
     end
   end
 end
-- 
GitLab