Merge branch 'georgekoltsov/fix-group-members-owner-access-level' into 'master'

Fix Group Import members' owner access level

See merge request gitlab-org/gitlab!25595

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,167 +4,191 @@ require 'spec_helper'
 
 describe Gitlab::ImportExport::MembersMapper do
   describe 'map 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
-      [{
-         "id" => 2,
-         "access_level" => 40,
-         "source_id" => 14,
-         "source_type" => "Project",
-         "notification_level" => 3,
-         "created_at" => "2016-03-11T10:21:44.822Z",
-         "updated_at" => "2016-03-11T10:21:44.822Z",
-         "created_by_id" => nil,
-         "invite_email" => nil,
-         "invite_token" => nil,
-         "invite_accepted_at" => nil,
-         "user" =>
-           {
-             "id" => exported_user_id,
-             "email" => user2.email,
-             "username" => 'test'
-           },
-         "user_id" => 19
-       },
-       {
-         "id" => 3,
-         "access_level" => 40,
-         "source_id" => 14,
-         "source_type" => "Project",
-         "user_id" => nil,
-         "notification_level" => 3,
-         "created_at" => "2016-03-11T10:21:44.822Z",
-         "updated_at" => "2016-03-11T10:21:44.822Z",
-         "created_by_id" => 1,
-         "invite_email" => 'invite@test.com',
-         "invite_token" => 'token',
-         "invite_accepted_at" => nil
-       }]
-    end
-
-    let(:members_mapper) do
-      described_class.new(
-        exported_members: exported_members, user: user, importable: project)
-    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
-      expect(members_mapper.map[exported_user_id]).to eq(user2.id)
-    end
-
-    it 'defaults to importer project 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)
-    end
-
-    it 'removes old user_id from member_hash to avoid conflict with user key' do
-      expect(ProjectMember)
-        .to receive(:create)
-        .twice
-        .with(hash_excluding('user_id'))
-        .and_call_original
-
-      members_mapper.map
-    end
-
-    context 'user is not an admin' do
-      let(:user) { create(:user) }
-
-      it 'does not map a project member' do
-        expect(members_mapper.map[exported_user_id]).to eq(user.id)
+    shared_examples 'imports exported members' do
+      let(:user) { create(:admin) }
+      let(:user2) { create(:user) }
+      let(:exported_user_id) { 99 }
+      let(:exported_members) do
+        [{
+           "id" => 2,
+           "access_level" => 40,
+           "source_id" => 14,
+           "source_type" => source_type,
+           "notification_level" => 3,
+           "created_at" => "2016-03-11T10:21:44.822Z",
+           "updated_at" => "2016-03-11T10:21:44.822Z",
+           "created_by_id" => nil,
+           "invite_email" => nil,
+           "invite_token" => nil,
+           "invite_accepted_at" => nil,
+           "user" =>
+             {
+               "id" => exported_user_id,
+               "email" => user2.email,
+               "username" => 'test'
+             },
+           "user_id" => 19
+         },
+         {
+           "id" => 3,
+           "access_level" => 40,
+           "source_id" => 14,
+           "source_type" => source_type,
+           "user_id" => nil,
+           "notification_level" => 3,
+           "created_at" => "2016-03-11T10:21:44.822Z",
+           "updated_at" => "2016-03-11T10:21:44.822Z",
+           "created_by_id" => 1,
+           "invite_email" => 'invite@test.com',
+           "invite_token" => 'token',
+           "invite_accepted_at" => nil
+         }]
       end
 
-      it 'defaults to importer project member if it does not exist' do
-        expect(members_mapper.map[-1]).to eq(user.id)
+      let(:members_mapper) do
+        described_class.new(
+          exported_members: exported_members, user: user, importable: importable)
       end
-    end
 
-    context 'chooses the one with an email first' do
-      let(:user3) { create(:user, username: 'test') }
+      it 'includes the exported user ID in the map' do
+        expect(members_mapper.map.keys).to include(exported_user_id)
+      end
 
-      it 'maps the project member that has a matching email first' do
+      it 'maps a member' do
         expect(members_mapper.map[exported_user_id]).to eq(user2.id)
       end
-    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(:members_mapper) do
-        described_class.new(
-          exported_members: exported_members, user: user2, importable: project)
+      it 'defaults to importer member if it does not exist' do
+        expect(members_mapper.map[-1]).to eq(user.id)
       end
 
-      before do
-        group.add_users([user, user2], GroupMember::DEVELOPER)
-      end
+      it 'has invited members with no user' do
+        members_mapper.map
 
-      it 'maps the project member' do
-        expect(members_mapper.map[exported_user_id]).to eq(user2.id)
+        expect(member_class.find_by_invite_email('invite@test.com')).not_to be_nil
       end
 
-      it 'maps the project member if it already exists' do
-        project.add_maintainer(user2)
+      it 'removes old user_id from member_hash to avoid conflict with user key' do
+        expect(member_class)
+          .to receive(:create)
+                .twice
+                .with(hash_excluding('user_id'))
+                .and_call_original
 
-        expect(members_mapper.map[exported_user_id]).to eq(user2.id)
+        members_mapper.map
       end
-    end
 
-    context 'importing group members' do
-      let(:group) { create(:group) }
-      let(:project) { create(:project, namespace: group) }
-      let(:members_mapper) do
-        described_class.new(
-          exported_members: exported_members, user: user, importable: project)
-      end
+      context 'user is not an admin' do
+        let(:user) { create(:user) }
 
-      before do
-        group.add_users([user, user2], GroupMember::DEVELOPER)
-        user.update(email: 'invite@test.com')
+        it 'does not map a member' do
+          expect(members_mapper.map[exported_user_id]).to eq(user.id)
+        end
+
+        it 'defaults to importer member if it does not exist' do
+          expect(members_mapper.map[-1]).to eq(user.id)
+        end
       end
 
-      it 'maps the importer' do
-        expect(members_mapper.map[-1]).to eq(user.id)
+      context 'chooses the one with an email' do
+        let(:user3) { create(:user, username: 'test') }
+
+        it 'maps the member that has a matching email' do
+          expect(members_mapper.map[exported_user_id]).to eq(user2.id)
+        end
       end
+    end
 
-      it 'maps the group member' do
-        expect(members_mapper.map[exported_user_id]).to eq(user2.id)
+    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(:importable) { create(:project, :public, name: 'searchable_project', namespace: group) }
+          let(:members_mapper) do
+            described_class.new(
+              exported_members: exported_members, user: user2, importable: importable)
+          end
+
+          before do
+            group.add_users([user, user2], GroupMember::DEVELOPER)
+          end
+
+          it 'maps the project member' do
+            expect(members_mapper.map[exported_user_id]).to eq(user2.id)
+          end
+
+          it 'maps the project member if it already exists' do
+            importable.add_maintainer(user2)
+
+            expect(members_mapper.map[exported_user_id]).to eq(user2.id)
+          end
+        end
+
+        context 'importing group members' do
+          let(:group) { create(:group) }
+          let(:importable) { create(:project, namespace: group) }
+          let(:members_mapper) do
+            described_class.new(
+              exported_members: exported_members, user: user, importable: importable)
+          end
+
+          before do
+            group.add_users([user, user2], GroupMember::DEVELOPER)
+            user.update(email: 'invite@test.com')
+          end
+
+          it 'maps the importer' do
+            expect(members_mapper.map[-1]).to eq(user.id)
+          end
+
+          it 'maps the group member' do
+            expect(members_mapper.map[exported_user_id]).to eq(user2.id)
+          end
+        end
+
+        context 'when importer mapping fails' do
+          let(:exception_message) { 'Something went wrong' }
+
+          it 'includes importer specific error message' do
+            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 importer mapping fails' do
-      let(:exception_message) { 'Something went wrong' }
+    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 'includes importer specific error message' do
-        expect(ProjectMember).to receive(:create!).and_raise(StandardError.new(exception_message))
+        it 'does not lower owner access level' do
+          exported_members.first['access_level'] = member_class::OWNER
 
-        expect { members_mapper.map }.to raise_error(StandardError, "Error adding importer user to Project members. #{exception_message}")
+          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