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

12 files changed, 192 insertions, 10 deletions

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