From 7b4b3d5f268534c028f55ef1014a84fe6a916cb0 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 13 Dec 2016 20:59:39 +0200 Subject: [PATCH 1/2] Include group parents into read access for project and group Signed-off-by: Dmitriy Zaporozhets --- app/models/group.rb | 18 ++++++-- app/policies/group_policy.rb | 2 +- app/policies/project_policy.rb | 2 +- spec/models/group_spec.rb | 11 +++++ spec/policies/group_policy_spec.rb | 66 ++++++++++++++++++++++++++++++ 5 files changed, 93 insertions(+), 6 deletions(-) diff --git a/app/models/group.rb b/app/models/group.rb index ac8a82c8c1e2..50c949d84aab 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -155,15 +155,17 @@ def add_owner(user, current_user = nil) end def has_owner?(user) - owners.include?(user) + members_with_parents.owners.where(user_id: user).any? end def has_master?(user) - members.masters.where(user_id: user).any? + members_with_parents.masters.where(user_id: user).any? end + # Check if user is a last owner of the group. + # Parent owners are ignored for nested groups. def last_owner?(user) - has_owner?(user) && owners.size == 1 + owners.include?(user) && owners.size == 1 end def avatar_type @@ -189,6 +191,14 @@ def system_hook_service end def refresh_members_authorized_projects - UserProjectAccessChangedService.new(users.pluck(:id)).execute + UserProjectAccessChangedService.new(users_with_parents.pluck(:id)).execute + end + + def members_with_parents + GroupMember.where(requested_at: nil, source_id: parents.map(&:id).push(id)) + end + + def users_with_parents + User.where(id: members_with_parents.pluck(:user_id)) end end diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb index 6f943feb2a72..0be6e1136558 100644 --- a/app/policies/group_policy.rb +++ b/app/policies/group_policy.rb @@ -4,7 +4,7 @@ def rules return unless @user globally_viewable = @subject.public? || (@subject.internal? && !@user.external?) - member = @subject.users.include?(@user) + member = @subject.users_with_parents.include?(@user) owner = @user.admin? || @subject.has_owner?(@user) master = owner || @subject.has_master?(@user) diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index b5db9c126229..eaf3035dfe18 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -245,7 +245,7 @@ def anonymous_rules def project_group_member?(user) project.group && ( - project.group.members.exists?(user_id: user.id) || + project.group.members_with_parents.exists?(user_id: user.id) || project.group.requesters.exists?(user_id: user.id) ) end diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 7d5ecfbaa64a..45fe927202b9 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -277,4 +277,15 @@ def setup_group_members(group) it { is_expected.to be_valid } it { expect(subject.parent).to be_kind_of(Group) } end + + describe '#members_with_parents' do + let!(:group) { create(:group, :nested) } + let!(:master) { group.parent.add_user(create(:user), GroupMember::MASTER) } + let!(:developer) { group.add_user(create(:user), GroupMember::DEVELOPER) } + + it 'returns parents members' do + expect(group.members_with_parents).to include(developer) + expect(group.members_with_parents).to include(master) + end + end end diff --git a/spec/policies/group_policy_spec.rb b/spec/policies/group_policy_spec.rb index a20ac303a538..5c34ff041529 100644 --- a/spec/policies/group_policy_spec.rb +++ b/spec/policies/group_policy_spec.rb @@ -105,4 +105,70 @@ is_expected.to include(*owner_permissions) end end + + describe 'private nested group inherit permissions' do + let(:nested_group) { create(:group, :private, parent: group) } + + subject { described_class.abilities(current_user, nested_group).to_set } + + context 'with no user' do + let(:current_user) { nil } + + it do + is_expected.not_to include(:read_group) + is_expected.not_to include(*master_permissions) + is_expected.not_to include(*owner_permissions) + end + end + + context 'guests' do + let(:current_user) { guest } + + it do + is_expected.to include(:read_group) + is_expected.not_to include(*master_permissions) + is_expected.not_to include(*owner_permissions) + end + end + + context 'reporter' do + let(:current_user) { reporter } + + it do + is_expected.to include(:read_group) + is_expected.not_to include(*master_permissions) + is_expected.not_to include(*owner_permissions) + end + end + + context 'developer' do + let(:current_user) { developer } + + it do + is_expected.to include(:read_group) + is_expected.not_to include(*master_permissions) + is_expected.not_to include(*owner_permissions) + end + end + + context 'master' do + let(:current_user) { master } + + it do + is_expected.to include(:read_group) + is_expected.to include(*master_permissions) + is_expected.not_to include(*owner_permissions) + end + end + + context 'owner' do + let(:current_user) { owner } + + it do + is_expected.to include(:read_group) + is_expected.to include(*master_permissions) + is_expected.to include(*owner_permissions) + end + end + end end -- GitLab From 9f39953eaf5568eb75bd2ecf1bab230bbf13f330 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Thu, 29 Dec 2016 00:29:33 +0200 Subject: [PATCH 2/2] Improve Group#users_with_parents method Signed-off-by: Dmitriy Zaporozhets --- app/models/group.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/group.rb b/app/models/group.rb index 50c949d84aab..21c9047c98a0 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -199,6 +199,6 @@ def members_with_parents end def users_with_parents - User.where(id: members_with_parents.pluck(:user_id)) + User.where(id: members_with_parents.select(:user_id)) end end -- GitLab