Commit c80a74d8 authored by Douglas Barbosa Alexandre's avatar Douglas Barbosa Alexandre

Merge branch...

Merge branch '51101-can-add-an-existing-group-member-into-a-group-project-with-new-permissions-but-permissions-are-not-overridden' into 'master'

Resolve "Can add an existing group member into a group project with new permissions but permissions are not overridden"

Closes #51101

See merge request gitlab-org/gitlab-ce!23226
parents 39c769ae 64c11f10
...@@ -7,6 +7,7 @@ class Member < ActiveRecord::Base ...@@ -7,6 +7,7 @@ class Member < ActiveRecord::Base
include Expirable include Expirable
include Gitlab::Access include Gitlab::Access
include Presentable include Presentable
include Gitlab::Utils::StrongMemoize
attr_accessor :raw_invite_token attr_accessor :raw_invite_token
...@@ -22,6 +23,7 @@ class Member < ActiveRecord::Base ...@@ -22,6 +23,7 @@ class Member < ActiveRecord::Base
message: "already exists in source", message: "already exists in source",
allow_nil: true } allow_nil: true }
validates :access_level, inclusion: { in: Gitlab::Access.all_values }, presence: true validates :access_level, inclusion: { in: Gitlab::Access.all_values }, presence: true
validate :higher_access_level_than_group, unless: :importing?
validates :invite_email, validates :invite_email,
presence: { presence: {
if: :invite? if: :invite?
...@@ -364,6 +366,15 @@ class Member < ActiveRecord::Base ...@@ -364,6 +366,15 @@ class Member < ActiveRecord::Base
end end
# rubocop: enable CodeReuse/ServiceClass # 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 private
def send_invite def send_invite
...@@ -430,4 +441,12 @@ class Member < ActiveRecord::Base ...@@ -430,4 +441,12 @@ class Member < ActiveRecord::Base
def notifiable_options def notifiable_options
{} {}
end 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 end
...@@ -570,6 +570,8 @@ class Project < ActiveRecord::Base ...@@ -570,6 +570,8 @@ class Project < ActiveRecord::Base
.base_and_ancestors(upto: top, hierarchy_order: hierarchy_order) .base_and_ancestors(upto: top, hierarchy_order: hierarchy_order)
end end
alias_method :ancestors, :ancestors_upto
def lfs_enabled? def lfs_enabled?
return namespace.lfs_enabled? if self[:lfs_enabled].nil? return namespace.lfs_enabled? if self[:lfs_enabled].nil?
......
...@@ -7,6 +7,14 @@ class MemberPresenter < Gitlab::View::Presenter::Delegated ...@@ -7,6 +7,14 @@ class MemberPresenter < Gitlab::View::Presenter::Delegated
member.class.access_level_roles member.class.access_level_roles
end 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? def can_resend_invite?
invite? && invite? &&
can?(current_user, admin_member_permission, source) can?(current_user, admin_member_permission, source)
......
...@@ -75,7 +75,7 @@ ...@@ -75,7 +75,7 @@
= dropdown_title(_("Change permissions")) = dropdown_title(_("Change permissions"))
.dropdown-content .dropdown-content
%ul %ul
- member.access_level_roles.each do |role, role_id| - member.valid_level_roles.each do |role, role_id|
%li %li
= link_to role, "javascript:void(0)", = link_to role, "javascript:void(0)",
class: ("is-active" if member.access_level == role_id), 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" ...@@ -7932,6 +7932,9 @@ msgid_plural "replies"
msgstr[0] "" msgstr[0] ""
msgstr[1] "" msgstr[1] ""
msgid "should be higher than %{access} inherited membership from group %{group_name}"
msgstr ""
msgid "source" msgid "source"
msgstr "" msgstr ""
......
...@@ -19,7 +19,7 @@ describe GroupMembersFinder, '#execute' do ...@@ -19,7 +19,7 @@ describe GroupMembersFinder, '#execute' do
end end
it 'returns members for nested group', :nested_groups do it 'returns members for nested group', :nested_groups do
group.add_maintainer(user2) group.add_developer(user2)
nested_group.request_access(user4) nested_group.request_access(user4)
member1 = group.add_maintainer(user1) member1 = group.add_maintainer(user1)
member3 = nested_group.add_maintainer(user2) member3 = nested_group.add_maintainer(user2)
......
...@@ -76,7 +76,7 @@ describe Group do ...@@ -76,7 +76,7 @@ describe Group do
before do before do
group.add_developer(user) group.add_developer(user)
sub_group.add_developer(user) sub_group.add_maintainer(user)
end end
it 'also gets notification settings from parent groups' do it 'also gets notification settings from parent groups' do
...@@ -498,7 +498,7 @@ describe Group do ...@@ -498,7 +498,7 @@ describe Group do
it 'returns member users on every nest level without duplication' do it 'returns member users on every nest level without duplication' do
group.add_developer(user_a) group.add_developer(user_a)
nested_group.add_developer(user_b) 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(group.users_with_descendants).to contain_exactly(user_a, user_b)
expect(nested_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 @@ describe Member do ...@@ -53,6 +53,29 @@ describe Member do
expect(member).to be_valid expect(member).to be_valid
end end
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 end
describe 'Scopes & finders' do describe 'Scopes & finders' do
......
...@@ -50,4 +50,26 @@ describe GroupMember do ...@@ -50,4 +50,26 @@ describe GroupMember do
group_member.destroy group_member.destroy
end end
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 end
...@@ -124,4 +124,19 @@ describe ProjectMember do ...@@ -124,4 +124,19 @@ describe ProjectMember do
end end
it_behaves_like 'members notifications', :project 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 end
...@@ -538,7 +538,7 @@ describe Namespace do ...@@ -538,7 +538,7 @@ describe Namespace do
it 'returns member users on every nest level without duplication' do it 'returns member users on every nest level without duplication' do
group.add_developer(user_a) group.add_developer(user_a)
nested_group.add_developer(user_b) 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(group.users_with_descendants).to contain_exactly(user_a, user_b)
expect(nested_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 @@ describe User do ...@@ -2325,11 +2325,11 @@ describe User do
context 'user is member of all groups' do context 'user is member of all groups' do
before do before do
group.add_owner(user) group.add_reporter(user)
nested_group_1.add_owner(user) nested_group_1.add_developer(user)
nested_group_1_1.add_owner(user) nested_group_1_1.add_maintainer(user)
nested_group_2.add_owner(user) nested_group_2.add_developer(user)
nested_group_2_1.add_owner(user) nested_group_2_1.add_maintainer(user)
end end
it 'returns all groups' do it 'returns all groups' do
......
...@@ -135,4 +135,12 @@ describe GroupMemberPresenter do ...@@ -135,4 +135,12 @@ describe GroupMemberPresenter do
end end
end 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 end
...@@ -135,4 +135,10 @@ describe ProjectMemberPresenter do ...@@ -135,4 +135,10 @@ describe ProjectMemberPresenter do
end end
end end
end end
it_behaves_like '#valid_level_roles', :project do
before do
entity.group = group
end
end
end end
...@@ -224,6 +224,37 @@ describe API::Members do ...@@ -224,6 +224,37 @@ describe API::Members do
end end
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 it "returns 409 if member already exists" do
post api("/#{source_type.pluralize}/#{source.id}/members", maintainer), post api("/#{source_type.pluralize}/#{source.id}/members", maintainer),
user_id: maintainer.id, access_level: Member::MAINTAINER user_id: maintainer.id, access_level: Member::MAINTAINER
......
...@@ -1906,7 +1906,7 @@ describe API::Projects do ...@@ -1906,7 +1906,7 @@ describe API::Projects do
let(:group) { create(:group) } let(:group) { create(:group) }
let(:group2) do let(:group2) do
group = create(:group, name: 'group2_name') group = create(:group, name: 'group2_name')
group.add_owner(user2) group.add_maintainer(user2)
group group
end 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