[MM-56986] imports/import_validators: add valid guest role check (#28658)

server/channels/app/import_functions_test.go

@@ -1552,48 +1552,48 @@ func TestImportImportUser(t *testing.T) {
 	assert.False(t, channelMember.SchemeGuest)
 	assert.Equal(t, "", channelMember.ExplicitRoles)
 
-	// see https://mattermost.atlassian.net/browse/MM-56986
 	// Test importing deleted guest with a valid team & valid channel name in apply mode.
-	// username = model.NewUsername()
+	username = model.NewUsername()
-	// deleteAt = model.GetMillis()
+	deleteAt = model.GetMillis()
-	// deletedGuestData := &imports.UserImportData{
+	deletedGuestData := &imports.UserImportData{
-	// 	Username: &username,
+		Username: &username,
-	// 	DeleteAt: &deleteAt,
+		DeleteAt: &deleteAt,
-	// 	Email:    model.NewPointer(model.NewId() + "@example.com"),
+		Email:    model.NewPointer(model.NewId() + "@example.com"),
-	// 	Teams: &[]imports.UserTeamImportData{
+		Roles:    model.NewPointer("system_guest"),
-	// 		{
+		Teams: &[]imports.UserTeamImportData{
-	// 			Name:  &team.Name,
+			{
-	// 			Roles: model.NewPointer("team_guest"),
+				Name:  &team.Name,
-	// 			Channels: &[]imports.UserChannelImportData{
+				Roles: model.NewPointer("team_guest"),
-	// 				{
+				Channels: &[]imports.UserChannelImportData{
-	// 					Name:  &channel.Name,
+					{
-	// 					Roles: model.NewPointer("channel_guest"),
+						Name:  &channel.Name,
-	// 				},
+						Roles: model.NewPointer("channel_guest"),
-	// 			},
+					},
-	// 		},
+				},
-	// 	},
+			},
-	// }
+		},
-	// appErr = th.App.importUser(th.Context, deletedGuestData, false)
+	}
-	// assert.Nil(t, appErr)
+	appErr = th.App.importUser(th.Context, deletedGuestData, false)
+	assert.Nil(t, appErr)
 
-	// user, appErr = th.App.GetUserByUsername(*deletedGuestData.Username)
+	user, appErr = th.App.GetUserByUsername(*deletedGuestData.Username)
-	// require.Nil(t, appErr, "Failed to get user from database.")
+	require.Nil(t, appErr, "Failed to get user from database.")
 
-	// teamMember, appErr = th.App.GetTeamMember(th.Context, team.Id, user.Id)
+	teamMember, appErr = th.App.GetTeamMember(th.Context, team.Id, user.Id)
-	// require.Nil(t, appErr, "Failed to get the team member")
+	require.Nil(t, appErr, "Failed to get the team member")
 
-	// assert.False(t, teamMember.SchemeAdmin)
+	assert.False(t, teamMember.SchemeAdmin)
-	// assert.False(t, teamMember.SchemeUser)
+	assert.False(t, teamMember.SchemeUser)
-	// assert.True(t, teamMember.SchemeGuest)
+	assert.True(t, teamMember.SchemeGuest)
-	// assert.Equal(t, "", teamMember.ExplicitRoles)
+	assert.Equal(t, "", teamMember.ExplicitRoles)
 
-	// channelMember, appErr = th.App.GetChannelMember(th.Context, channel.Id, user.Id)
+	channelMember, appErr = th.App.GetChannelMember(th.Context, channel.Id, user.Id)
-	// require.Nil(t, appErr, "Failed to get the channel member")
+	require.Nil(t, appErr, "Failed to get the channel member")
 
-	// assert.False(t, teamMember.SchemeAdmin)
+	assert.False(t, teamMember.SchemeAdmin)
-	// assert.False(t, channelMember.SchemeUser)
+	assert.False(t, channelMember.SchemeUser)
-	// assert.True(t, teamMember.SchemeGuest)
+	assert.True(t, teamMember.SchemeGuest)
-	// assert.Equal(t, "", channelMember.ExplicitRoles)
+	assert.Equal(t, "", channelMember.ExplicitRoles)
 }
 
 func TestImportUserTeams(t *testing.T) {

server/channels/app/imports/import_validators.go

@@ -261,6 +261,10 @@ func ValidateUserImportData(data *UserImportData) *model.AppError {
 		return model.NewAppError("BulkImport", "app.import.validate_user_import_data.roles_invalid.error", nil, "", http.StatusBadRequest)
 	}
 
+	if !isValidGuestRoles(*data) {
+		return model.NewAppError("BulkImport", "app.import.validate_user_import_data.guest_roles_conflict.error", nil, "", http.StatusBadRequest)
+	}
+
 	if data.NotifyProps != nil {
 		if data.NotifyProps.Desktop != nil && !isValidUserNotifyLevel(*data.NotifyProps.Desktop) {
 			return model.NewAppError("BulkImport", "app.import.validate_user_import_data.notify_props_desktop_invalid.error", nil, "", http.StatusBadRequest)
@@ -688,3 +692,47 @@ func isValidEmailBatchingInterval(emailInterval string) bool {
 		emailInterval == model.PreferenceEmailIntervalFifteen ||
 		emailInterval == model.PreferenceEmailIntervalHour
 }
+
+// isValidGuestRoles checks if the user has both guest roles in the same team or channel.
+// at this point we assume that the user has a valid role scheme.
+func isValidGuestRoles(data UserImportData) bool {
+	if data.Roles == nil {
+		return true
+	}
+	isSystemGuest := model.IsInRole(*data.Roles, model.SystemGuestRoleId)
+
+	var isTeamGuest, isChannelGuest bool
+	if data.Teams != nil {
+		// counters for guest roles for teams and channels
+		// we expect the total count of guest roles to be equal to the total count of teams and channels
+		var gtc, ctc int
+		for _, team := range *data.Teams {
+			if team.Roles != nil && model.IsInRole(*team.Roles, model.TeamGuestRoleId) {
+				gtc++
+			}
+
+			if *team.Channels != nil {
+				for _, channel := range *team.Channels {
+					if channel.Roles != nil && model.IsInRole(*channel.Roles, model.ChannelGuestRoleId) {
+						ctc++
+					}
+				}
+
+				if ctc == len(*team.Channels) {
+					isChannelGuest = true
+				}
+			}
+		}
+		if gtc == len(*data.Teams) {
+			isTeamGuest = true
+		}
+	}
+
+	// basically we want to be sure if the user either fully guest in all 3 places or not at all
+	// (a | b | c) & !(a & b & c) -> 3-way XOR?
+	if (isSystemGuest || isTeamGuest || isChannelGuest) && !(isSystemGuest && isTeamGuest && isChannelGuest) {
+		return false
+	}
+
+	return true
+}

server/channels/app/imports/import_validators_test.go

@@ -1469,3 +1469,116 @@ func checkError(t *testing.T, err *model.AppError) {
 func checkNoError(t *testing.T, err *model.AppError) {
 	require.Nil(t, err, "Unexpected Error: %v", err)
 }
+
+func TestIsValidGuestRoles(t *testing.T) {
+	var testCases = []struct {
+		name     string
+		input    UserImportData
+		expected bool
+	}{
+		{
+			name: "Valid case: User is a guest in all places",
+			input: UserImportData{
+				Roles: model.NewPointer(model.SystemGuestRoleId),
+				Teams: &[]UserTeamImportData{
+					{
+						Roles: model.NewPointer(model.TeamGuestRoleId),
+						Channels: &[]UserChannelImportData{
+							{Roles: model.NewPointer(model.ChannelGuestRoleId)},
+						},
+					},
+				},
+			},
+			expected: true,
+		},
+		{
+			name: "Invalid case: User is a guest in a team but not in another team",
+			input: UserImportData{
+				Roles: model.NewPointer(model.SystemGuestRoleId),
+				Teams: &[]UserTeamImportData{
+					{
+						Roles: model.NewPointer(model.TeamGuestRoleId),
+						Channels: &[]UserChannelImportData{
+							{Roles: model.NewPointer(model.ChannelGuestRoleId)},
+						},
+					},
+					{
+						Roles: model.NewPointer(model.TeamUserRoleId),
+						Channels: &[]UserChannelImportData{
+							{Roles: model.NewPointer(model.ChannelUserRoleId)},
+						},
+					},
+				},
+			},
+			expected: false,
+		},
+		{
+			name: "Invalid case: User is a guest in a team but not in another team and has no channel membership",
+			input: UserImportData{
+				Roles: model.NewPointer(model.SystemGuestRoleId),
+				Teams: &[]UserTeamImportData{
+					{
+						Roles: model.NewPointer(model.TeamGuestRoleId),
+						Channels: &[]UserChannelImportData{
+							{Roles: model.NewPointer(model.ChannelGuestRoleId)},
+						},
+					},
+					{
+						Roles:    model.NewPointer(model.TeamUserRoleId),
+						Channels: &[]UserChannelImportData{},
+					},
+				},
+			},
+			expected: false,
+		},
+		{
+			name: "Invalid case: User is system guest but not guest in team and channel",
+			input: UserImportData{
+				Roles: model.NewPointer(model.SystemGuestRoleId),
+			},
+			expected: false,
+		},
+		{
+			name: "Invalid case: User has mixed roles",
+			input: UserImportData{
+				Roles: model.NewPointer(model.SystemGuestRoleId),
+				Teams: &[]UserTeamImportData{
+					{
+						Roles: model.NewPointer(model.TeamUserRoleId),
+						Channels: &[]UserChannelImportData{
+							{Roles: model.NewPointer(model.ChannelGuestRoleId)},
+						},
+					},
+				},
+			},
+			expected: false,
+		},
+		{
+			name:     "Valid case: User does not have any role defined in any place",
+			input:    UserImportData{},
+			expected: true,
+		},
+		{
+			name: "Valid case: User is not a guest in any place",
+			input: UserImportData{
+				Roles: model.NewPointer(model.SystemUserRoleId),
+				Teams: &[]UserTeamImportData{
+					{
+						Roles: model.NewPointer(model.TeamAdminRoleId),
+						Channels: &[]UserChannelImportData{
+							{Roles: model.NewPointer(model.ChannelAdminRoleId)},
+						},
+					},
+				},
+			},
+			expected: true,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			result := isValidGuestRoles(tc.input)
+			assert.Equal(t, tc.expected, result, tc.name)
+		})
+	}
+}

server/i18n/en.json

@@ -5730,6 +5730,10 @@
     "id": "app.import.validate_user_import_data.first_name_length.error",
     "translation": "User First Name is too long."
   },
+  {
+    "id": "app.import.validate_user_import_data.guest_roles_conflict.error",
+    "translation": "User roles are not consistent with guest status."
+  },
   {
     "id": "app.import.validate_user_import_data.last_name_length.error",
     "translation": "User Last Name is too long."