From b59819da68c89d6fefc46238883c4df0fbd9d10f Mon Sep 17 00:00:00 2001 From: Ibrahim Serdar Acikgoz Date: Tue, 17 Dec 2024 12:49:55 +0100 Subject: [PATCH] [MM-56986] imports/import_validators: add valid guest role check (#28658) --- server/channels/app/import_functions_test.go | 72 +++++------ .../channels/app/imports/import_validators.go | 48 ++++++++ .../app/imports/import_validators_test.go | 113 ++++++++++++++++++ server/i18n/en.json | 4 + 4 files changed, 201 insertions(+), 36 deletions(-) diff --git a/server/channels/app/import_functions_test.go b/server/channels/app/import_functions_test.go index 4790289785..373e422445 100644 --- a/server/channels/app/import_functions_test.go +++ b/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() - // deleteAt = model.GetMillis() - // deletedGuestData := &imports.UserImportData{ - // Username: &username, - // DeleteAt: &deleteAt, - // Email: model.NewPointer(model.NewId() + "@example.com"), - // Teams: &[]imports.UserTeamImportData{ - // { - // Name: &team.Name, - // Roles: model.NewPointer("team_guest"), - // Channels: &[]imports.UserChannelImportData{ - // { - // Name: &channel.Name, - // Roles: model.NewPointer("channel_guest"), - // }, - // }, - // }, - // }, - // } - // appErr = th.App.importUser(th.Context, deletedGuestData, false) - // assert.Nil(t, appErr) + username = model.NewUsername() + deleteAt = model.GetMillis() + deletedGuestData := &imports.UserImportData{ + Username: &username, + DeleteAt: &deleteAt, + Email: model.NewPointer(model.NewId() + "@example.com"), + Roles: model.NewPointer("system_guest"), + Teams: &[]imports.UserTeamImportData{ + { + Name: &team.Name, + Roles: model.NewPointer("team_guest"), + Channels: &[]imports.UserChannelImportData{ + { + Name: &channel.Name, + Roles: model.NewPointer("channel_guest"), + }, + }, + }, + }, + } + appErr = th.App.importUser(th.Context, deletedGuestData, false) + assert.Nil(t, appErr) - // user, appErr = th.App.GetUserByUsername(*deletedGuestData.Username) - // require.Nil(t, appErr, "Failed to get user from database.") + user, appErr = th.App.GetUserByUsername(*deletedGuestData.Username) + require.Nil(t, appErr, "Failed to get user from database.") - // teamMember, appErr = th.App.GetTeamMember(th.Context, team.Id, user.Id) - // require.Nil(t, appErr, "Failed to get the team member") + teamMember, appErr = th.App.GetTeamMember(th.Context, team.Id, user.Id) + require.Nil(t, appErr, "Failed to get the team member") - // assert.False(t, teamMember.SchemeAdmin) - // assert.False(t, teamMember.SchemeUser) - // assert.True(t, teamMember.SchemeGuest) - // assert.Equal(t, "", teamMember.ExplicitRoles) + assert.False(t, teamMember.SchemeAdmin) + assert.False(t, teamMember.SchemeUser) + assert.True(t, teamMember.SchemeGuest) + assert.Equal(t, "", teamMember.ExplicitRoles) - // channelMember, appErr = th.App.GetChannelMember(th.Context, channel.Id, user.Id) - // require.Nil(t, appErr, "Failed to get the channel member") + channelMember, appErr = th.App.GetChannelMember(th.Context, channel.Id, user.Id) + require.Nil(t, appErr, "Failed to get the channel member") - // assert.False(t, teamMember.SchemeAdmin) - // assert.False(t, channelMember.SchemeUser) - // assert.True(t, teamMember.SchemeGuest) - // assert.Equal(t, "", channelMember.ExplicitRoles) + assert.False(t, teamMember.SchemeAdmin) + assert.False(t, channelMember.SchemeUser) + assert.True(t, teamMember.SchemeGuest) + assert.Equal(t, "", channelMember.ExplicitRoles) } func TestImportUserTeams(t *testing.T) { diff --git a/server/channels/app/imports/import_validators.go b/server/channels/app/imports/import_validators.go index 40748ff0af..f4b3339523 100644 --- a/server/channels/app/imports/import_validators.go +++ b/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 +} diff --git a/server/channels/app/imports/import_validators_test.go b/server/channels/app/imports/import_validators_test.go index 4d6495250a..74943ff139 100644 --- a/server/channels/app/imports/import_validators_test.go +++ b/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) + }) + } +} diff --git a/server/i18n/en.json b/server/i18n/en.json index 0987478696..e292190f97 100644 --- a/server/i18n/en.json +++ b/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."