From 64c11f104ceffdb7699686445ddc16c894dbe0c5 Mon Sep 17 00:00:00 2001
From: James Lopez <james@gitlab.com>
Date: Thu, 6 Dec 2018 13:15:29 +0000
Subject: [PATCH] Resolve "Can add an existing group member into a group
 project with new permissions but permissions are not overridden"

---
 app/models/member.rb                          | 19 +++++
 app/models/project.rb                         |  2 +
 app/presenters/member_presenter.rb            |  8 ++
 app/views/shared/members/_member.html.haml    |  2 +-
 ...ions-but-permissions-are-not-overridde.yml |  5 ++
 locale/gitlab.pot                             |  3 +
 spec/finders/group_members_finder_spec.rb     |  2 +-
 spec/models/group_spec.rb                     |  4 +-
 spec/models/member_spec.rb                    | 23 ++++++
 spec/models/members/group_member_spec.rb      | 22 ++++++
 spec/models/members/project_member_spec.rb    | 15 ++++
 spec/models/namespace_spec.rb                 |  2 +-
 spec/models/user_spec.rb                      | 10 +--
 .../presenters/group_member_presenter_spec.rb |  8 ++
 .../project_member_presenter_spec.rb          |  6 ++
 spec/requests/api/members_spec.rb             | 31 ++++++++
 spec/requests/api/projects_spec.rb            |  2 +-
 .../models/member_shared_examples.rb          | 77 +++++++++++++++++++
 18 files changed, 230 insertions(+), 11 deletions(-)
 create mode 100644 changelogs/unreleased/51101-can-add-an-existing-group-member-into-a-group-project-with-new-permissions-but-permissions-are-not-overridde.yml
 create mode 100644 spec/support/shared_examples/models/member_shared_examples.rb

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