From 2e3ccb05c7466a78e07b4bb17311f2e8938749ca Mon Sep 17 00:00:00 2001 From: Patrick Cyiza Date: Thu, 14 Sep 2023 11:31:37 +0200 Subject: [PATCH 1/2] Prevent existing undeleted user access to return true Some revoked inherited members of groups with personalized protected branch rules could still push to branches Changelog: fixed --- app/models/concerns/protected_ref_access.rb | 1 + .../concerns/protected_ref_access_examples.rb | 32 +++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/app/models/concerns/protected_ref_access.rb b/app/models/concerns/protected_ref_access.rb index f0bb1cc359b699..c504effeaa4f4e 100644 --- a/app/models/concerns/protected_ref_access.rb +++ b/app/models/concerns/protected_ref_access.rb @@ -70,6 +70,7 @@ def role? def check_access(current_user) return false if current_user.nil? || no_access? return current_user.admin? if admin_access? + return false unless project.member?(current_user) yield if block_given? diff --git a/spec/support/shared_examples/models/concerns/protected_ref_access_examples.rb b/spec/support/shared_examples/models/concerns/protected_ref_access_examples.rb index 0e9200f1fd9346..801400d19377b4 100644 --- a/spec/support/shared_examples/models/concerns/protected_ref_access_examples.rb +++ b/spec/support/shared_examples/models/concerns/protected_ref_access_examples.rb @@ -52,7 +52,11 @@ end describe '#check_access' 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) } let_it_be(:current_user) { create(:user) } + let_it_be(:protected_ref) { create(association, project: project) } let(:access_level) { ::Gitlab::Access::DEVELOPER } @@ -71,6 +75,34 @@ it { expect(subject.check_access(nil)).to eq(false) } end + context 'when current_user access exist 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) + + it do + expect(user_access.check_access(other_user)).to be_falsey + end + + context 'when user has inherited membership' do + let!(:inherited_membership) { create(:group_member, group: group, user: other_user) } + + it do + expect(user_access.check_access(other_user)).to be_truthy + end + end + end + context 'when access_level is NO_ACCESS' do let(:access_level) { ::Gitlab::Access::NO_ACCESS } -- GitLab From 91b928f3d5327788eeccde5ea729f34361b020d0 Mon Sep 17 00:00:00 2001 From: Patrick Cyiza Date: Wed, 20 Sep 2023 16:15:51 +0200 Subject: [PATCH 2/2] Feature flag project membership check Changelog: security EE: false --- app/models/concerns/protected_ref_access.rb | 3 ++- ...eck_membership_in_protected_ref_access.yml | 8 ++++++ .../concerns/protected_ref_access_examples.rb | 27 ++++++++++++++----- 3 files changed, 30 insertions(+), 8 deletions(-) create mode 100644 config/feature_flags/development/check_membership_in_protected_ref_access.yml diff --git a/app/models/concerns/protected_ref_access.rb b/app/models/concerns/protected_ref_access.rb index c504effeaa4f4e..a5994b538ce570 100644 --- a/app/models/concerns/protected_ref_access.rb +++ b/app/models/concerns/protected_ref_access.rb @@ -70,7 +70,8 @@ def role? def check_access(current_user) return false if current_user.nil? || no_access? return current_user.admin? if admin_access? - return false unless project.member?(current_user) + + return false if Feature.enabled?(:check_membership_in_protected_ref_access) && !project.member?(current_user) yield if block_given? diff --git a/config/feature_flags/development/check_membership_in_protected_ref_access.yml b/config/feature_flags/development/check_membership_in_protected_ref_access.yml new file mode 100644 index 00000000000000..87e3fc9fcaf10f --- /dev/null +++ b/config/feature_flags/development/check_membership_in_protected_ref_access.yml @@ -0,0 +1,8 @@ +--- +name: check_membership_in_protected_ref_access +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/132380 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/425926 +milestone: '16.5' +type: development +group: group::source code +default_enabled: false diff --git a/spec/support/shared_examples/models/concerns/protected_ref_access_examples.rb b/spec/support/shared_examples/models/concerns/protected_ref_access_examples.rb index 801400d19377b4..bb438b0082fd4f 100644 --- a/spec/support/shared_examples/models/concerns/protected_ref_access_examples.rb +++ b/spec/support/shared_examples/models/concerns/protected_ref_access_examples.rb @@ -75,7 +75,7 @@ it { expect(subject.check_access(nil)).to eq(false) } end - context 'when current_user access exist without membership' do + 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) @@ -90,15 +90,28 @@ 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) - it do - expect(user_access.check_access(other_user)).to be_falsey + user_access.check_access(other_user) end - context 'when user has inherited membership' do - let!(:inherited_membership) { create(:group_member, group: group, user: other_user) } + 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 + + context 'when user has inherited membership' do + let!(:inherited_membership) { create(:group_member, group: group, user: other_user) } - it do - expect(user_access.check_access(other_user)).to be_truthy + it do + expect(user_access.check_access(other_user)).to be_truthy + end end end end -- GitLab