diff --git a/server/channels/app/import_functions_test.go b/server/channels/app/import_functions_test.go index bc36bb42f33..36b9261efb2 100644 --- a/server/channels/app/import_functions_test.go +++ b/server/channels/app/import_functions_test.go @@ -1738,6 +1738,28 @@ func TestImportImportUser(t *testing.T) { assert.True(t, teamMember.SchemeGuest) assert.Equal(t, "", channelMember.ExplicitRoles) }) + + t.Run("import guest user without any team or channel memberships", func(t *testing.T) { + username := model.NewUsername() + guestData := &imports.UserImportData{ + Username: &username, + Email: model.NewPointer(model.NewId() + "@example.com"), + Roles: model.NewPointer("system_guest"), + } + + appErr := th.App.importUser(th.Context, guestData, false) + require.Nil(t, appErr, "Failed to import guest user without memberships") + + user, appErr := th.App.GetUserByUsername(*guestData.Username) + require.Nil(t, appErr, "Failed to get user from database.") + + assert.True(t, user.IsGuest(), "User should be a guest") + assert.Equal(t, "system_guest", user.Roles) + + teams, appErr := th.App.GetTeamsForUser(user.Id) + require.Nil(t, appErr) + assert.Empty(t, teams, "Guest user should have no team memberships") + }) } func TestImportUserTeams(t *testing.T) { diff --git a/server/channels/app/imports/import_validators.go b/server/channels/app/imports/import_validators.go index 2a3d2a680f1..fac52335d63 100644 --- a/server/channels/app/imports/import_validators.go +++ b/server/channels/app/imports/import_validators.go @@ -267,8 +267,8 @@ 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 err := validateGuestRoles(*data); err != nil { + return err } if data.NotifyProps != nil { @@ -758,49 +758,77 @@ func isValidEmailBatchingInterval(emailInterval string) bool { 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 { +// validateGuestRoles checks if the user has guest roles consistently across system, team, and channel levels. +// At this point, we assume that the user has a valid role scheme. +func validateGuestRoles(data UserImportData) *model.AppError { if data.Roles == nil { - return true + return nil } 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 user has no teams, they can still be a system guest without issue + if data.Teams == nil || len(*data.Teams) == 0 { + return nil + } + + var isTeamGuest, isChannelGuest bool + var hasChannels bool // hasChannels indicates if the user has any channels within their teams + + var teamGuestCount, channelGuestCount int + var totalTeams, totalChannels int + + totalTeams = len(*data.Teams) + for _, team := range *data.Teams { + if team.Roles != nil && model.IsInRole(*team.Roles, model.TeamGuestRoleId) { + teamGuestCount++ + } + + if len(model.SafeDereference(team.Channels)) > 0 { + hasChannels = true + totalChannels += len(*team.Channels) - if team.Channels == nil || len(*team.Channels) == 0 { - continue - } for _, channel := range *team.Channels { if channel.Roles != nil && model.IsInRole(*channel.Roles, model.ChannelGuestRoleId) { - ctc++ + channelGuestCount++ } } - - 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 + // Set flags based on whether all available teams/channels have guest roles + if totalTeams > 0 && teamGuestCount == totalTeams { + isTeamGuest = true } - return true + if hasChannels && channelGuestCount == totalChannels { + isChannelGuest = true + } + + // If the user is a system guest, they must have consistent guest roles in any teams/channels they belong to + if isSystemGuest { + // If they have teams, they must be a team guest in all teams + if totalTeams > 0 && !isTeamGuest { + return model.NewAppError("BulkImport", "app.import.validate_user_import_data.system_guest_missing_team_guest_roles.error", map[string]any{"TeamGuestCount": teamGuestCount, "TotalTeams": totalTeams}, "", http.StatusBadRequest) + } + + // If they have channels, they must be a channel guest in all channels + if hasChannels && !isChannelGuest { + return model.NewAppError("BulkImport", "app.import.validate_user_import_data.system_guest_missing_channel_guest_roles.error", map[string]any{"ChannelGuestCount": channelGuestCount, "TotalChannels": totalChannels}, "", http.StatusBadRequest) + } + + return nil + } + + // If not a system guest, ensure consistency in the other direction + // If they're a team or channel guest, they must be a system guest + if (isTeamGuest || isChannelGuest) && !isSystemGuest { + if isTeamGuest { + return model.NewAppError("BulkImport", "app.import.validate_user_import_data.team_guest_missing_system_guest_role.error", nil, "", http.StatusBadRequest) + } + return model.NewAppError("BulkImport", "app.import.validate_user_import_data.channel_guest_missing_system_guest_role.error", nil, "", http.StatusBadRequest) + } + + return nil } // ValidateAttachmentPathForImport joins 'path' to 'basePath' (defaulting to "." if empty) and ensures diff --git a/server/channels/app/imports/import_validators_test.go b/server/channels/app/imports/import_validators_test.go index 123444ab4fd..a046d0db4a5 100644 --- a/server/channels/app/imports/import_validators_test.go +++ b/server/channels/app/imports/import_validators_test.go @@ -1601,17 +1601,16 @@ func checkNoError(t *testing.T, err *model.AppError) { require.Nil(t, err, "Unexpected Error: %v", err) } -func TestIsValidGuestRoles(t *testing.T) { +func TestValidateGuestRoles(t *testing.T) { testCases := []struct { - name string - input UserImportData - expected bool + name string + input UserImportData + expectError bool }{ { name: "Valid case: User is a guest in all places", input: UserImportData{ - Username: model.NewPointer("guest1"), - Roles: model.NewPointer(model.SystemGuestRoleId), + Roles: model.NewPointer(model.SystemGuestRoleId), Teams: &[]UserTeamImportData{ { Roles: model.NewPointer(model.TeamGuestRoleId), @@ -1621,13 +1620,12 @@ func TestIsValidGuestRoles(t *testing.T) { }, }, }, - expected: true, + expectError: false, }, { name: "Invalid case: User is a guest in a team but not in another team", input: UserImportData{ - Username: model.NewPointer("mixeduser1"), - Roles: model.NewPointer(model.SystemGuestRoleId), + Roles: model.NewPointer(model.SystemGuestRoleId), Teams: &[]UserTeamImportData{ { Roles: model.NewPointer(model.TeamGuestRoleId), @@ -1643,13 +1641,12 @@ func TestIsValidGuestRoles(t *testing.T) { }, }, }, - expected: false, + expectError: true, }, { name: "Invalid case: User is a guest in a team but not in another team and has no channel membership", input: UserImportData{ - Username: model.NewPointer("mixeduser2"), - Roles: model.NewPointer(model.SystemGuestRoleId), + Roles: model.NewPointer(model.SystemGuestRoleId), Teams: &[]UserTeamImportData{ { Roles: model.NewPointer(model.TeamGuestRoleId), @@ -1663,21 +1660,27 @@ func TestIsValidGuestRoles(t *testing.T) { }, }, }, - expected: false, + expectError: true, }, { - name: "Invalid case: User is system guest but not guest in team and channel", + name: "Valid case: User is system guest with no teams", input: UserImportData{ - Username: model.NewPointer("systemguestonly"), - Roles: model.NewPointer(model.SystemGuestRoleId), + Roles: model.NewPointer(model.SystemGuestRoleId), }, - expected: false, + expectError: false, + }, + { + name: "Valid case: User is system guest with empty teams array", + input: UserImportData{ + Roles: model.NewPointer(model.SystemGuestRoleId), + Teams: &[]UserTeamImportData{}, + }, + expectError: false, }, { name: "Invalid case: User has mixed roles", input: UserImportData{ - Username: model.NewPointer("mixeduser3"), - Roles: model.NewPointer(model.SystemGuestRoleId), + Roles: model.NewPointer(model.SystemGuestRoleId), Teams: &[]UserTeamImportData{ { Roles: model.NewPointer(model.TeamUserRoleId), @@ -1687,20 +1690,42 @@ func TestIsValidGuestRoles(t *testing.T) { }, }, }, - expected: false, + expectError: true, }, { - name: "Valid case: User does not have any role defined in any place", + name: "Valid case: User is system guest with team guest role but no channels", input: UserImportData{ - Username: model.NewPointer("noroleuser"), + Roles: model.NewPointer(model.SystemGuestRoleId), + Teams: &[]UserTeamImportData{ + { + Roles: model.NewPointer(model.TeamGuestRoleId), + Channels: &[]UserChannelImportData{}, + }, + }, }, - expected: true, + expectError: false, + }, + { + name: "Valid case: User is system guest with team guest role and nil channels", + input: UserImportData{ + Roles: model.NewPointer(model.SystemGuestRoleId), + Teams: &[]UserTeamImportData{ + { + Roles: model.NewPointer(model.TeamGuestRoleId), + }, + }, + }, + expectError: false, + }, + { + name: "Valid case: User does not have any role defined in any place", + input: UserImportData{}, + expectError: false, }, { name: "Valid case: User is not a guest in any place", input: UserImportData{ - Username: model.NewPointer("normaluser"), - Roles: model.NewPointer(model.SystemUserRoleId), + Roles: model.NewPointer(model.SystemUserRoleId), Teams: &[]UserTeamImportData{ { Roles: model.NewPointer(model.TeamAdminRoleId), @@ -1710,81 +1735,18 @@ func TestIsValidGuestRoles(t *testing.T) { }, }, }, - expected: true, - }, - { - name: "Valid case: User with team but nil channels array", - input: UserImportData{ - Username: model.NewPointer("nilchannelsuser"), - Roles: model.NewPointer(model.SystemUserRoleId), - Teams: &[]UserTeamImportData{ - { - Roles: model.NewPointer(model.TeamUserRoleId), - Channels: nil, - }, - }, - }, - expected: true, - }, - { - name: "Invalid case: User is guest in channels but not in system or team", - input: UserImportData{ - Username: model.NewPointer("testuser3"), - Roles: model.NewPointer(model.SystemUserRoleId), - Teams: &[]UserTeamImportData{ - { - Roles: model.NewPointer(model.TeamUserRoleId), - Channels: &[]UserChannelImportData{ - {Roles: model.NewPointer(model.ChannelGuestRoleId)}, - }, - }, - }, - }, - expected: false, - }, - { - name: "Invalid case: User is system guest and team guest but has no channels", - input: UserImportData{ - Username: model.NewPointer("testuser4"), - Roles: model.NewPointer(model.SystemGuestRoleId), - Teams: &[]UserTeamImportData{ - { - Roles: model.NewPointer(model.TeamGuestRoleId), - Channels: &[]UserChannelImportData{}, - }, - }, - }, - expected: false, - }, - { - name: "Valid case: User is guest in all places with multiple teams and channels", - input: UserImportData{ - Username: model.NewPointer("testuser5"), - Roles: model.NewPointer(model.SystemGuestRoleId), - Teams: &[]UserTeamImportData{ - { - Roles: model.NewPointer(model.TeamGuestRoleId), - Channels: &[]UserChannelImportData{ - {Roles: model.NewPointer(model.ChannelGuestRoleId)}, - {Roles: model.NewPointer(model.ChannelGuestRoleId)}, - }, - }, - { - Roles: model.NewPointer(model.TeamGuestRoleId), - Channels: &[]UserChannelImportData{ - {Roles: model.NewPointer(model.ChannelGuestRoleId)}, - }, - }, - }, - }, - expected: true, + expectError: false, }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - result := isValidGuestRoles(tc.input) - assert.Equal(t, tc.expected, result, tc.name) + err := validateGuestRoles(tc.input) + if tc.expectError { + assert.NotNil(t, err, tc.name) + } else { + assert.Nil(t, err, tc.name) + } }) } } diff --git a/server/i18n/en.json b/server/i18n/en.json index 64cd42726fc..0e7d5853cc1 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -6534,6 +6534,10 @@ "id": "app.import.validate_user_import_data.auth_data_length.error", "translation": "User AuthData is too long." }, + { + "id": "app.import.validate_user_import_data.channel_guest_missing_system_guest_role.error", + "translation": "User has channel guest roles but is not a system guest." + }, { "id": "app.import.validate_user_import_data.email_length.error", "translation": "User email has an invalid length." @@ -6546,10 +6550,6 @@ "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.invalid_image_path.error", "translation": "User profile image path is invalid: \"{{.Path}}\"" @@ -6606,6 +6606,18 @@ "id": "app.import.validate_user_import_data.roles_invalid.error", "translation": "User roles are not valid." }, + { + "id": "app.import.validate_user_import_data.system_guest_missing_channel_guest_roles.error", + "translation": "User is a system guest but does not have channel guest roles in all channels. Channel guest count: {{.ChannelGuestCount}}, Total channels: {{.TotalChannels}}." + }, + { + "id": "app.import.validate_user_import_data.system_guest_missing_team_guest_roles.error", + "translation": "User is a system guest but does not have team guest roles in all teams. Team guest count: {{.TeamGuestCount}}, Total teams: {{.TotalTeams}}." + }, + { + "id": "app.import.validate_user_import_data.team_guest_missing_system_guest_role.error", + "translation": "User has team guest roles but is not a system guest." + }, { "id": "app.import.validate_user_import_data.username_invalid.error", "translation": "Username is not valid."