From c7e17abd269e31a59c41687459eea6382475ab95 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Tue, 5 Sep 2017 10:38:24 -0700 Subject: [PATCH] =?UTF-8?q?Fix=20=E2=80=9CShare=20lock=E2=80=9D=20policy?= =?UTF-8?q?=20for=20deeply=20nested=20groups?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- app/policies/group_policy.rb | 4 +-- spec/policies/group_policy_spec.rb | 42 ++++++++++++++++++++++++------ 2 files changed, 36 insertions(+), 10 deletions(-) diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb index dab27bbf6da..45416bb7149 100644 --- a/app/policies/group_policy.rb +++ b/app/policies/group_policy.rb @@ -16,7 +16,7 @@ class GroupPolicy < BasePolicy condition(:nested_groups_supported, scope: :global) { Group.supports_nested_groups? } condition(:parent_share_locked) { @subject.has_parent? && @subject.parent.share_with_group_lock? } - condition(:parent_owner) { @subject.has_parent? && @subject.parent.has_owner?(@user) } + condition(:can_change_parent_share_with_group_lock) { @subject.has_parent? && can?(:change_share_with_group_lock, @subject.parent) } condition(:has_projects) do GroupProjectsFinder.new(group: @subject, current_user: @user).execute.any? @@ -57,7 +57,7 @@ class GroupPolicy < BasePolicy rule { ~can?(:view_globally) }.prevent :request_access rule { has_access }.prevent :request_access - rule { owner & (~parent_share_locked | parent_owner) }.enable :change_share_with_group_lock + rule { owner & (~parent_share_locked | can_change_parent_share_with_group_lock) }.enable :change_share_with_group_lock def access_level return GroupMember::NO_ACCESS if @user.nil? diff --git a/spec/policies/group_policy_spec.rb b/spec/policies/group_policy_spec.rb index dcc7b7d5b9c..fdf588f6455 100644 --- a/spec/policies/group_policy_spec.rb +++ b/spec/policies/group_policy_spec.rb @@ -248,19 +248,45 @@ describe GroupPolicy do let(:group) { create(:group, parent: parent) } context 'when the parent share_with_group_lock is enabled' do - let(:parent) { create(:group, share_with_group_lock: true) } let(:current_user) { owner } - context 'when current_user owns the parent' do - before do - parent.add_owner(owner) - end + context 'when the group has a grandparent' do + let(:grandparent) { create(:group, share_with_group_lock: true) } + let(:parent) { create(:group, share_with_group_lock: true, parent: grandparent) } - it { expect_allowed(:change_share_with_group_lock) } + context 'and the grandparent share_with_group_lock is enabled' do + context 'when current_user owns the grandparent' do + before do + grandparent.add_owner(owner) + end + + it { expect_allowed(:change_share_with_group_lock) } + end + + context 'when current_user owns the parent but not the grandparent' do + before do + parent.add_owner(owner) + end + + it { expect_disallowed(:change_share_with_group_lock) } + end + end end - context 'when current_user owns the group but not the parent' do - it { expect_disallowed(:change_share_with_group_lock) } + context 'when the group does not have a grandparent' do + let(:parent) { create(:group, share_with_group_lock: true) } + + context 'when current_user owns the parent' do + before do + parent.add_owner(owner) + end + + it { expect_allowed(:change_share_with_group_lock) } + end + + context 'when current_user owns the group but not the parent' do + it { expect_disallowed(:change_share_with_group_lock) } + end end end