Commit c747c642 authored by James Lopez's avatar James Lopez Committed by Douglas Barbosa Alexandre
Browse files

Resolve "Can add an existing group member into a group project with new...

Resolve "Can add an existing group member into a group project with new permissions but permissions are not overridden"
parent 68286a2914f8
......@@ -7,6 +7,7 @@ class Member < ActiveRecord::Base
include Expirable
include Gitlab::Access
include Presentable
include Gitlab::Utils::StrongMemoize
attr_accessor :raw_invite_token
......@@ -22,6 +23,7 @@ class Member < ActiveRecord::Base
message: "already exists in source",
allow_nil: true }
validates :access_level, inclusion: { in: Gitlab::Access.all_values }, presence: true
validate :higher_access_level_than_group, unless: :importing?
validates :invite_email,
presence: {
if: :invite?
......@@ -364,6 +366,15 @@ def notifiable?(type, opts = {})
end
# rubocop: enable CodeReuse/ServiceClass
# Find the user's group member with a highest access level
def highest_group_member
strong_memoize(:highest_group_member) do
next unless user_id && source&.ancestors&.any?
GroupMember.where(source: source.ancestors, user_id: user_id).order(:access_level).last
end
end
private
def send_invite
......@@ -430,4 +441,12 @@ def notification_service
def notifiable_options
{}
end
def higher_access_level_than_group
if highest_group_member && highest_group_member.access_level >= access_level
error_parameters = { access: highest_group_member.human_access, group_name: highest_group_member.group.name }
errors.add(:access_level, s_("should be higher than %{access} inherited membership from group %{group_name}") % error_parameters)
end
end
end
......@@ -570,6 +570,8 @@ def ancestors_upto(top = nil, hierarchy_order: nil)
.base_and_ancestors(upto: top, hierarchy_order: hierarchy_order)
end
alias_method :ancestors, :ancestors_upto
def lfs_enabled?
return namespace.lfs_enabled? if self[:lfs_enabled].nil?
......
......@@ -7,6 +7,14 @@ def access_level_roles
member.class.access_level_roles
end
def valid_level_roles
return access_level_roles unless member.highest_group_member
access_level_roles.reject do |_name, level|
member.highest_group_member.access_level > level
end
end
def can_resend_invite?
invite? &&
can?(current_user, admin_member_permission, source)
......
......@@ -75,7 +75,7 @@
= dropdown_title(_("Change permissions"))
.dropdown-content
%ul
- member.access_level_roles.each do |role, role_id|
- member.valid_level_roles.each do |role, role_id|
%li
= link_to role, "javascript:void(0)",
class: ("is-active" if member.access_level == role_id),
......
---
title: Restrict member access level to be higher than that of any parent group
merge_request: 23226
author:
type: fixed
......@@ -7932,6 +7932,9 @@ msgid_plural "replies"
msgstr[0] ""
msgstr[1] ""
msgid "should be higher than %{access} inherited membership from group %{group_name}"
msgstr ""
msgid "source"
msgstr ""
......
......@@ -19,7 +19,7 @@
end
it 'returns members for nested group', :nested_groups do
group.add_maintainer(user2)
group.add_developer(user2)
nested_group.request_access(user4)
member1 = group.add_maintainer(user1)
member3 = nested_group.add_maintainer(user2)
......
......@@ -76,7 +76,7 @@
before do
group.add_developer(user)
sub_group.add_developer(user)
sub_group.add_maintainer(user)
end
it 'also gets notification settings from parent groups' do
......@@ -498,7 +498,7 @@ def setup_group_members(group)
it 'returns member users on every nest level without duplication' do
group.add_developer(user_a)
nested_group.add_developer(user_b)
deep_nested_group.add_developer(user_a)
deep_nested_group.add_maintainer(user_a)
expect(group.users_with_descendants).to contain_exactly(user_a, user_b)
expect(nested_group.users_with_descendants).to contain_exactly(user_a, user_b)
......
......@@ -53,6 +53,29 @@
expect(member).to be_valid
end
end
context "when a child member inherits its access level" do
let(:user) { create(:user) }
let(:member) { create(:group_member, :developer, user: user) }
let(:child_group) { create(:group, parent: member.group) }
let(:child_member) { build(:group_member, group: child_group, user: user) }
it "requires a higher level" do
child_member.access_level = GroupMember::REPORTER
child_member.validate
expect(child_member).not_to be_valid
end
it "is valid with a higher level" do
child_member.access_level = GroupMember::MAINTAINER
child_member.validate
expect(child_member).to be_valid
end
end
end
describe 'Scopes & finders' do
......
......@@ -50,4 +50,26 @@
group_member.destroy
end
end
context 'access levels', :nested_groups do
context 'with parent group' do
it_behaves_like 'inherited access level as a member of entity' do
let(:entity) { create(:group, parent: parent_entity) }
end
end
context 'with parent group and a sub subgroup' do
it_behaves_like 'inherited access level as a member of entity' do
let(:subgroup) { create(:group, parent: parent_entity) }
let(:entity) { create(:group, parent: subgroup) }
end
context 'when only the subgroup has the member' do
it_behaves_like 'inherited access level as a member of entity' do
let(:parent_entity) { create(:group, parent: create(:group)) }
let(:entity) { create(:group, parent: parent_entity) }
end
end
end
end
end
......@@ -124,4 +124,19 @@
end
it_behaves_like 'members notifications', :project
context 'access levels' do
context 'with parent group' do
it_behaves_like 'inherited access level as a member of entity' do
let(:entity) { create(:project, group: parent_entity) }
end
end
context 'with parent group and a subgroup', :nested_groups do
it_behaves_like 'inherited access level as a member of entity' do
let(:subgroup) { create(:group, parent: parent_entity) }
let(:entity) { create(:project, group: subgroup) }
end
end
end
end
......@@ -538,7 +538,7 @@ def project_rugged(project)
it 'returns member users on every nest level without duplication' do
group.add_developer(user_a)
nested_group.add_developer(user_b)
deep_nested_group.add_developer(user_a)
deep_nested_group.add_maintainer(user_a)
expect(group.users_with_descendants).to contain_exactly(user_a, user_b)
expect(nested_group.users_with_descendants).to contain_exactly(user_a, user_b)
......
......@@ -2325,11 +2325,11 @@ def add_user(access)
context 'user is member of all groups' do
before do
group.add_owner(user)
nested_group_1.add_owner(user)
nested_group_1_1.add_owner(user)
nested_group_2.add_owner(user)
nested_group_2_1.add_owner(user)
group.add_reporter(user)
nested_group_1.add_developer(user)
nested_group_1_1.add_maintainer(user)
nested_group_2.add_developer(user)
nested_group_2_1.add_maintainer(user)
end
it 'returns all groups' do
......
......@@ -135,4 +135,12 @@
end
end
end
it_behaves_like '#valid_level_roles', :group do
let(:expected_roles) { { 'Developer' => 30, 'Maintainer' => 40, 'Owner' => 50, 'Reporter' => 20 } }
before do
entity.parent = group
end
end
end
......@@ -135,4 +135,10 @@
end
end
end
it_behaves_like '#valid_level_roles', :project do
before do
entity.group = group
end
end
end
......@@ -224,6 +224,37 @@
end
end
context 'access levels' do
it 'does not create the member if group level is higher', :nested_groups do
parent = create(:group)
group.update(parent: parent)
project.update(group: group)
parent.add_developer(stranger)
post api("/#{source_type.pluralize}/#{source.id}/members", maintainer),
user_id: stranger.id, access_level: Member::REPORTER
expect(response).to have_gitlab_http_status(400)
expect(json_response['message']['access_level']).to eq(["should be higher than Developer inherited membership from group #{parent.name}"])
end
it 'creates the member if group level is lower', :nested_groups do
parent = create(:group)
group.update(parent: parent)
project.update(group: group)
parent.add_developer(stranger)
post api("/#{source_type.pluralize}/#{source.id}/members", maintainer),
user_id: stranger.id, access_level: Member::MAINTAINER
expect(response).to have_gitlab_http_status(201)
expect(json_response['id']).to eq(stranger.id)
expect(json_response['access_level']).to eq(Member::MAINTAINER)
end
end
it "returns 409 if member already exists" do
post api("/#{source_type.pluralize}/#{source.id}/members", maintainer),
user_id: maintainer.id, access_level: Member::MAINTAINER
......
......@@ -1906,7 +1906,7 @@
let(:group) { create(:group) }
let(:group2) do
group = create(:group, name: 'group2_name')
group.add_owner(user2)
group.add_maintainer(user2)
group
end
......
# frozen_string_literal: true
shared_examples_for 'inherited access level as a member of entity' do
let(:parent_entity) { create(:group) }
let(:user) { create(:user) }
let(:member) { entity.is_a?(Group) ? entity.group_member(user) : entity.project_member(user) }
context 'with root parent_entity developer member' do
before do
parent_entity.add_developer(user)
end
it 'is allowed to be a maintainer of the entity' do
entity.add_maintainer(user)
expect(member.access_level).to eq(Gitlab::Access::MAINTAINER)
end
it 'is not allowed to be a reporter of the entity' do
entity.add_reporter(user)
expect(member).to be_nil
end
it 'is allowed to change to be a developer of the entity' do
entity.add_maintainer(user)
expect { member.update(access_level: Gitlab::Access::DEVELOPER) }
.to change { member.access_level }.to(Gitlab::Access::DEVELOPER)
end
it 'is not allowed to change to be a guest of the entity' do
entity.add_maintainer(user)
expect { member.update(access_level: Gitlab::Access::GUEST) }
.not_to change { member.reload.access_level }
end
it "shows an error if the member can't be updated" do
entity.add_maintainer(user)
member.update(access_level: Gitlab::Access::REPORTER)
expect(member.errors.full_messages).to eq(["Access level should be higher than Developer inherited membership from group #{parent_entity.name}"])
end
it 'allows changing the level from a non existing member' do
non_member_user = create(:user)
entity.add_maintainer(non_member_user)
non_member = entity.is_a?(Group) ? entity.group_member(non_member_user) : entity.project_member(non_member_user)
expect { non_member.update(access_level: Gitlab::Access::GUEST) }
.to change { non_member.reload.access_level }
end
end
end
shared_examples_for '#valid_level_roles' do |entity_name|
let(:member_user) { create(:user) }
let(:group) { create(:group) }
let(:entity) { create(entity_name) }
let(:entity_member) { create("#{entity_name}_member", :developer, source: entity, user: member_user) }
let(:presenter) { described_class.new(entity_member, current_user: member_user) }
let(:expected_roles) { { 'Developer' => 30, 'Maintainer' => 40, 'Reporter' => 20 } }
it 'returns all roles when no parent member is present' do
expect(presenter.valid_level_roles).to eq(entity_member.class.access_level_roles)
end
it 'returns higher roles when a parent member is present' do
group.add_reporter(member_user)
expect(presenter.valid_level_roles).to eq(expected_roles)
end
end
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment