Fix Group Import owner access level

changelogs/unreleased/georgekoltsov-fix-group-members-owner-access-level.yml

+---
+title: Fix an issue with Group Import members with Owner access level being imported with Maintainer access level. Owner access level is now preserved
+merge_request: 25595
+author:
+type: fixed

lib/gitlab/import_export/members_mapper.rb

@@ -51,7 +51,7 @@ module Gitlab
 
         @importable.members.destroy_all # rubocop: disable DestroyAll
 
-        relation_class.create!(user: @user, access_level: relation_class::MAINTAINER, source_id: @importable.id, importing: true)
+        relation_class.create!(user: @user, access_level: highest_access_level, source_id: @importable.id, importing: true)
       rescue => e
         raise e, "Error adding importer user to #{@importable.class} members. #{e.message}"
       end
@@ -59,7 +59,7 @@ module Gitlab
       def user_already_member?
         member = @importable.members&.first
 
-        member&.user == @user && member.access_level >= relation_class::MAINTAINER
+        member&.user == @user && member.access_level >= highest_access_level
       end
 
       def add_team_member(member, existing_user = nil)
@@ -72,7 +72,7 @@ module Gitlab
         parsed_hash(member).merge(
           'source_id' => @importable.id,
           'importing' => true,
-          'access_level' => [member['access_level'], relation_class::MAINTAINER].min
+          'access_level' => [member['access_level'], highest_access_level].min
         ).except('user_id')
       end
 
@@ -97,6 +97,12 @@ module Gitlab
           GroupMember
         end
       end
+
+      def highest_access_level
+        return relation_class::OWNER if relation_class == GroupMember
+
+        relation_class::MAINTAINER
+      end
     end
   end
 end

spec/lib/gitlab/import_export/members_mapper_spec.rb

@@ -4,8 +4,8 @@ require 'spec_helper'
 
 describe Gitlab::ImportExport::MembersMapper do
   describe 'map members' do
+    shared_examples 'imports exported members' do
       let(:user) { create(:admin) }
-    let(:project) { create(:project, :public, name: 'searchable_project') }
       let(:user2) { create(:user) }
       let(:exported_user_id) { 99 }
       let(:exported_members) do
@@ -13,7 +13,7 @@ describe Gitlab::ImportExport::MembersMapper do
            "id" => 2,
            "access_level" => 40,
            "source_id" => 14,
-         "source_type" => "Project",
+           "source_type" => source_type,
            "notification_level" => 3,
            "created_at" => "2016-03-11T10:21:44.822Z",
            "updated_at" => "2016-03-11T10:21:44.822Z",
@@ -33,7 +33,7 @@ describe Gitlab::ImportExport::MembersMapper do
            "id" => 3,
            "access_level" => 40,
            "source_id" => 14,
-         "source_type" => "Project",
+           "source_type" => source_type,
            "user_id" => nil,
            "notification_level" => 3,
            "created_at" => "2016-03-11T10:21:44.822Z",
@@ -47,43 +47,29 @@ describe Gitlab::ImportExport::MembersMapper do
 
       let(:members_mapper) do
         described_class.new(
-        exported_members: exported_members, user: user, importable: project)
+          exported_members: exported_members, user: user, importable: importable)
       end
 
       it 'includes the exported user ID in the map' do
         expect(members_mapper.map.keys).to include(exported_user_id)
       end
 
-    it 'maps a project member' do
+      it 'maps a member' do
         expect(members_mapper.map[exported_user_id]).to eq(user2.id)
       end
 
-    it 'defaults to importer project member if it does not exist' do
+      it 'defaults to importer member if it does not exist' do
         expect(members_mapper.map[-1]).to eq(user.id)
       end
 
       it 'has invited members with no user' do
         members_mapper.map
 
-      expect(ProjectMember.find_by_invite_email('invite@test.com')).not_to be_nil
-    end
-
-    it 'authorizes the users to the project' do
-      members_mapper.map
-
-      expect(user.authorized_project?(project)).to be true
-      expect(user2.authorized_project?(project)).to be true
-    end
-
-    it 'maps an owner as a maintainer' do
-      exported_members.first['access_level'] = ProjectMember::OWNER
-
-      expect(members_mapper.map[exported_user_id]).to eq(user2.id)
-      expect(ProjectMember.find_by_user_id(user2.id).access_level).to eq(ProjectMember::MAINTAINER)
+        expect(member_class.find_by_invite_email('invite@test.com')).not_to be_nil
       end
 
       it 'removes old user_id from member_hash to avoid conflict with user key' do
-      expect(ProjectMember)
+        expect(member_class)
           .to receive(:create)
                 .twice
                 .with(hash_excluding('user_id'))
@@ -95,30 +81,51 @@ describe Gitlab::ImportExport::MembersMapper do
       context 'user is not an admin' do
         let(:user) { create(:user) }
 
-      it 'does not map a project member' do
+        it 'does not map a member' do
           expect(members_mapper.map[exported_user_id]).to eq(user.id)
         end
 
-      it 'defaults to importer project member if it does not exist' do
+        it 'defaults to importer member if it does not exist' do
           expect(members_mapper.map[-1]).to eq(user.id)
         end
       end
 
-    context 'chooses the one with an email first' do
+      context 'chooses the one with an email' do
         let(:user3) { create(:user, username: 'test') }
 
-      it 'maps the project member that has a matching email first' do
+        it 'maps the member that has a matching email' do
           expect(members_mapper.map[exported_user_id]).to eq(user2.id)
         end
       end
+    end
+
+    context 'when importable is Project' do
+      include_examples 'imports exported members' do
+        let(:source_type) { 'Project' }
+        let(:member_class) { ProjectMember }
+        let(:importable) { create(:project, :public, name: 'searchable_project') }
+
+        it 'authorizes the users to the project' do
+          members_mapper.map
+
+          expect(user.authorized_project?(importable)).to be true
+          expect(user2.authorized_project?(importable)).to be true
+        end
+
+        it 'maps an owner as a maintainer' do
+          exported_members.first['access_level'] = ProjectMember::OWNER
+
+          expect(members_mapper.map[exported_user_id]).to eq(user2.id)
+          expect(member_class.find_by_user_id(user2.id).access_level).to eq(ProjectMember::MAINTAINER)
+        end
 
         context 'importer same as group member' do
           let(:user2) { create(:admin) }
           let(:group) { create(:group) }
-      let(:project) { create(:project, :public, name: 'searchable_project', namespace: group) }
+          let(:importable) { create(:project, :public, name: 'searchable_project', namespace: group) }
           let(:members_mapper) do
             described_class.new(
-          exported_members: exported_members, user: user2, importable: project)
+              exported_members: exported_members, user: user2, importable: importable)
           end
 
           before do
@@ -130,7 +137,7 @@ describe Gitlab::ImportExport::MembersMapper do
           end
 
           it 'maps the project member if it already exists' do
-        project.add_maintainer(user2)
+            importable.add_maintainer(user2)
 
             expect(members_mapper.map[exported_user_id]).to eq(user2.id)
           end
@@ -138,10 +145,10 @@ describe Gitlab::ImportExport::MembersMapper do
 
         context 'importing group members' do
           let(:group) { create(:group) }
-      let(:project) { create(:project, namespace: group) }
+          let(:importable) { create(:project, namespace: group) }
           let(:members_mapper) do
             described_class.new(
-          exported_members: exported_members, user: user, importable: project)
+              exported_members: exported_members, user: user, importable: importable)
           end
 
           before do
@@ -162,10 +169,27 @@ describe Gitlab::ImportExport::MembersMapper do
           let(:exception_message) { 'Something went wrong' }
 
           it 'includes importer specific error message' do
-        expect(ProjectMember).to receive(:create!).and_raise(StandardError.new(exception_message))
+            expect(member_class).to receive(:create!).and_raise(StandardError.new(exception_message))
 
             expect { members_mapper.map }.to raise_error(StandardError, "Error adding importer user to Project members. #{exception_message}")
           end
         end
       end
+    end
+
+    context 'when importable is Group' do
+      include_examples 'imports exported members' do
+        let(:source_type) { 'Namespace' }
+        let(:member_class) { GroupMember }
+        let(:importable) { create(:group) }
+
+        it 'does not lower owner access level' do
+          exported_members.first['access_level'] = member_class::OWNER
+
+          expect(members_mapper.map[exported_user_id]).to eq(user2.id)
+          expect(member_class.find_by_user_id(user2.id).access_level).to eq(member_class::OWNER)
+        end
+      end
+    end
+  end
 end