diff options
author | James Lopez <james@gitlab.com> | 2018-12-06 16:15:29 +0300 |
---|---|---|
committer | Douglas Barbosa Alexandre <dbalexandre@gmail.com> | 2018-12-06 16:15:29 +0300 |
commit | 64c11f104ceffdb7699686445ddc16c894dbe0c5 (patch) | |
tree | d4331a41db06511c3f4daaa6ec853110f31b4260 | |
parent | 39c769aee8af82cd755a4c666a22eb5d6bec808e (diff) |
Resolve "Can add an existing group member into a group project with new permissions but permissions are not overridden"
-rw-r--r-- | app/models/member.rb | 19 | ||||
-rw-r--r-- | app/models/project.rb | 2 | ||||
-rw-r--r-- | app/presenters/member_presenter.rb | 8 | ||||
-rw-r--r-- | app/views/shared/members/_member.html.haml | 2 | ||||
-rw-r--r-- | changelogs/unreleased/51101-can-add-an-existing-group-member-into-a-group-project-with-new-permissions-but-permissions-are-not-overridde.yml | 5 | ||||
-rw-r--r-- | locale/gitlab.pot | 3 | ||||
-rw-r--r-- | spec/finders/group_members_finder_spec.rb | 2 | ||||
-rw-r--r-- | spec/models/group_spec.rb | 4 | ||||
-rw-r--r-- | spec/models/member_spec.rb | 23 | ||||
-rw-r--r-- | spec/models/members/group_member_spec.rb | 22 | ||||
-rw-r--r-- | spec/models/members/project_member_spec.rb | 15 | ||||
-rw-r--r-- | spec/models/namespace_spec.rb | 2 | ||||
-rw-r--r-- | spec/models/user_spec.rb | 10 | ||||
-rw-r--r-- | spec/presenters/group_member_presenter_spec.rb | 8 | ||||
-rw-r--r-- | spec/presenters/project_member_presenter_spec.rb | 6 | ||||
-rw-r--r-- | spec/requests/api/members_spec.rb | 31 | ||||
-rw-r--r-- | spec/requests/api/projects_spec.rb | 2 | ||||
-rw-r--r-- | spec/support/shared_examples/models/member_shared_examples.rb | 77 |
18 files changed, 230 insertions, 11 deletions
diff --git a/app/models/member.rb b/app/models/member.rb index bc8ac14d148..9fc95ea00c3 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -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 @@ class Member < ActiveRecord::Base 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 @@ class Member < ActiveRecord::Base 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 diff --git a/app/models/project.rb b/app/models/project.rb index 587bada469e..1adcb73806d 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -570,6 +570,8 @@ class Project < ActiveRecord::Base .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? diff --git a/app/presenters/member_presenter.rb b/app/presenters/member_presenter.rb index 2497bea4aff..9e9b6973b8e 100644 --- a/app/presenters/member_presenter.rb +++ b/app/presenters/member_presenter.rb @@ -7,6 +7,14 @@ class MemberPresenter < Gitlab::View::Presenter::Delegated 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) diff --git a/app/views/shared/members/_member.html.haml b/app/views/shared/members/_member.html.haml index a7fd75d85d7..6b3841ebbc4 100644 --- a/app/views/shared/members/_member.html.haml +++ b/app/views/shared/members/_member.html.haml @@ -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), diff --git a/changelogs/unreleased/51101-can-add-an-existing-group-member-into-a-group-project-with-new-permissions-but-permissions-are-not-overridde.yml b/changelogs/unreleased/51101-can-add-an-existing-group-member-into-a-group-project-with-new-permissions-but-permissions-are-not-overridde.yml new file mode 100644 index 00000000000..96f33a72cc5 --- /dev/null +++ b/changelogs/unreleased/51101-can-add-an-existing-group-member-into-a-group-project-with-new-permissions-but-permissions-are-not-overridde.yml @@ -0,0 +1,5 @@ +--- +title: Restrict member access level to be higher than that of any parent group +merge_request: 23226 +author: +type: fixed diff --git a/locale/gitlab.pot b/locale/gitlab.pot index f023a9be3eb..23ee90ff0dd 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -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 "" diff --git a/spec/finders/group_members_finder_spec.rb b/spec/finders/group_members_finder_spec.rb index f545da3aee4..8975ea0f063 100644 --- a/spec/finders/group_members_finder_spec.rb +++ b/spec/finders/group_members_finder_spec.rb @@ -19,7 +19,7 @@ describe GroupMembersFinder, '#execute' do 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) diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 0c3a49cd0f2..87aa5a46c21 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -76,7 +76,7 @@ describe Group do 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 @@ describe Group do 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) diff --git a/spec/models/member_spec.rb b/spec/models/member_spec.rb index fca1b1f90d9..188beac1582 100644 --- a/spec/models/member_spec.rb +++ b/spec/models/member_spec.rb @@ -53,6 +53,29 @@ describe Member do 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 diff --git a/spec/models/members/group_member_spec.rb b/spec/models/members/group_member_spec.rb index 97959ed4304..a3451c67bd8 100644 --- a/spec/models/members/group_member_spec.rb +++ b/spec/models/members/group_member_spec.rb @@ -50,4 +50,26 @@ describe GroupMember do 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 diff --git a/spec/models/members/project_member_spec.rb b/spec/models/members/project_member_spec.rb index 334d4f95f53..097b1bb30dc 100644 --- a/spec/models/members/project_member_spec.rb +++ b/spec/models/members/project_member_spec.rb @@ -124,4 +124,19 @@ describe ProjectMember do 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 diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb index 6ee19c0ddf4..96561dab1c9 100644 --- a/spec/models/namespace_spec.rb +++ b/spec/models/namespace_spec.rb @@ -538,7 +538,7 @@ describe Namespace do 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) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index e5490e0a156..6cb27246f06 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -2325,11 +2325,11 @@ describe User do 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 diff --git a/spec/presenters/group_member_presenter_spec.rb b/spec/presenters/group_member_presenter_spec.rb index c00e41725d9..bb66523a83d 100644 --- a/spec/presenters/group_member_presenter_spec.rb +++ b/spec/presenters/group_member_presenter_spec.rb @@ -135,4 +135,12 @@ describe GroupMemberPresenter do 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 diff --git a/spec/presenters/project_member_presenter_spec.rb b/spec/presenters/project_member_presenter_spec.rb index 83db5c56cdf..73ef113a1c5 100644 --- a/spec/presenters/project_member_presenter_spec.rb +++ b/spec/presenters/project_member_presenter_spec.rb @@ -135,4 +135,10 @@ describe ProjectMemberPresenter do end end end + + it_behaves_like '#valid_level_roles', :project do + before do + entity.group = group + end + end end diff --git a/spec/requests/api/members_spec.rb b/spec/requests/api/members_spec.rb index 93e1c3a2294..bb32d581176 100644 --- a/spec/requests/api/members_spec.rb +++ b/spec/requests/api/members_spec.rb @@ -224,6 +224,37 @@ describe API::Members do 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 diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index 62b6a3ce42e..e40db55cd20 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -1906,7 +1906,7 @@ describe API::Projects do let(:group) { create(:group) } let(:group2) do group = create(:group, name: 'group2_name') - group.add_owner(user2) + group.add_maintainer(user2) group end diff --git a/spec/support/shared_examples/models/member_shared_examples.rb b/spec/support/shared_examples/models/member_shared_examples.rb new file mode 100644 index 00000000000..77376496854 --- /dev/null +++ b/spec/support/shared_examples/models/member_shared_examples.rb @@ -0,0 +1,77 @@ +# 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 |