From b36eb415d1d3eb1f6deb03ae631f99d88fcc5d4c Mon Sep 17 00:00:00 2001 From: Miguel de la Cruz Date: Wed, 14 Aug 2019 16:07:58 +0200 Subject: [PATCH] [MM-17213] Fix domain check when inviting guests and update user checks (#11793) * Add update user check for guests and tests * [MM-17213] Fix domain check when inviting guests * Fix i18n strings * Fix tests * Add missing translation * Adds test case for non guest user --- api4/team_test.go | 47 +++++++++++++++-------------------------------- app/team.go | 2 +- app/user.go | 21 ++++++++++++++++----- app/user_test.go | 36 ++++++++++++++++++++++++++++++++++-- i18n/en.json | 8 ++++++++ 5 files changed, 74 insertions(+), 40 deletions(-) diff --git a/api4/team_test.go b/api4/team_test.go index e69b6604b5..ff5b0d84eb 100644 --- a/api4/team_test.go +++ b/api4/team_test.go @@ -2305,10 +2305,12 @@ func TestInviteGuestsToTeam(t *testing.T) { enableEmailInvitations := *th.App.Config().ServiceSettings.EnableEmailInvitations restrictCreationToDomains := th.App.Config().TeamSettings.RestrictCreationToDomains + guestRestrictCreationToDomains := th.App.Config().GuestAccountsSettings.RestrictCreationToDomains enableGuestAccounts := *th.App.Config().GuestAccountsSettings.Enable defer func() { th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.EnableEmailInvitations = &enableEmailInvitations }) th.App.UpdateConfig(func(cfg *model.Config) { cfg.TeamSettings.RestrictCreationToDomains = restrictCreationToDomains }) + th.App.UpdateConfig(func(cfg *model.Config) { cfg.GuestAccountsSettings.RestrictCreationToDomains = guestRestrictCreationToDomains }) th.App.UpdateConfig(func(cfg *model.Config) { cfg.GuestAccountsSettings.Enable = &enableGuestAccounts }) }() @@ -2378,45 +2380,26 @@ func TestInviteGuestsToTeam(t *testing.T) { th.App.UpdateConfig(func(cfg *model.Config) { *cfg.TeamSettings.RestrictCreationToDomains = "@global.com,@common.com" }) - t.Run("restricted domains", func(t *testing.T) { + t.Run("team domain restrictions should not affect inviting guests", func(t *testing.T) { err := th.App.InviteGuestsToChannels(th.BasicTeam.Id, &model.GuestsInvite{Emails: emailList, Channels: []string{th.BasicChannel.Id}, Message: "test message"}, th.BasicUser.Id) - - if err == nil { - t.Fatal("Adding users with non-restricted domains was allowed") - } - if err.Where != "InviteGuestsToChannels" || err.Id != "api.team.invite_members.invalid_email.app_error" { - t.Log(err) - t.Fatal("Got wrong error message!") - } + require.Nil(t, err, "guest user invites should not be affected by team restrictions") }) - t.Run("override restricted domains", func(t *testing.T) { - th.BasicTeam.AllowedDomains = "invalid.com,common.com" - if _, err := th.App.UpdateTeam(th.BasicTeam); err == nil { - t.Fatal("Should not update the team") - } + t.Run("guest restrictions should affect guest users", func(t *testing.T) { + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.GuestAccountsSettings.RestrictCreationToDomains = "@guest.com" }) - th.BasicTeam.AllowedDomains = "common.com" - if _, err := th.App.UpdateTeam(th.BasicTeam); err != nil { - t.Log(err) - t.Fatal("Should update the team") - } + err := th.App.InviteGuestsToChannels(th.BasicTeam.Id, &model.GuestsInvite{Emails: []string{"guest1@invalid.com"}, Channels: []string{th.BasicChannel.Id}, Message: "test message"}, th.BasicUser.Id) + require.NotNil(t, err, "guest user invites should be affected by the guest domain restrictions") - if err := th.App.InviteGuestsToChannels(th.BasicTeam.Id, &model.GuestsInvite{Emails: []string{"test@global.com"}, Channels: []string{th.BasicChannel.Id}, Message: "test message"}, th.BasicUser.Id); err == nil || err.Where != "InviteGuestsToChannels" { - t.Log(err) - t.Fatal("Per team restriction should take precedence over the global restriction") - } + err = th.App.InviteGuestsToChannels(th.BasicTeam.Id, &model.GuestsInvite{Emails: []string{"guest1@guest.com"}, Channels: []string{th.BasicChannel.Id}, Message: "test message"}, th.BasicUser.Id) + require.Nil(t, err, "whitelisted guest user email should be allowed by the guest domain restrictions") + }) - if err := th.App.InviteGuestsToChannels(th.BasicTeam.Id, &model.GuestsInvite{Emails: []string{"test@common.com"}, Channels: []string{th.BasicChannel.Id}, Message: "test message"}, th.BasicUser.Id); err != nil { - t.Log(err) - t.Fatal("Failed to invite user which was common between team and global domain restriction") - } - - if err := th.App.InviteGuestsToChannels(th.BasicTeam.Id, &model.GuestsInvite{Emails: []string{"test@invalid.com"}, Channels: []string{th.BasicChannel.Id}, Message: "test message"}, th.BasicUser.Id); err == nil { - t.Log(err) - t.Fatal("Should not invite user") - } + t.Run("guest restrictions should not affect inviting new team members", func(t *testing.T) { + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.GuestAccountsSettings.RestrictCreationToDomains = "@guest.com" }) + err := th.App.InviteNewUsersToTeam([]string{"user@global.com"}, th.BasicTeam.Id, th.BasicUser.Id) + require.Nil(t, err, "non guest user invites should not be affected by the guest domain restrictions") }) } diff --git a/app/team.go b/app/team.go index a9bf8ac05b..cfb39d7cc4 100644 --- a/app/team.go +++ b/app/team.go @@ -1103,7 +1103,7 @@ func (a *App) InviteGuestsToChannels(teamId string, guestsInvite *model.GuestsIn var invalidEmailList []string for _, email := range guestsInvite.Emails { - if !a.isTeamEmailAddressAllowed(email, team.AllowedDomains) { + if !CheckEmailDomain(email, *a.Config().GuestAccountsSettings.RestrictCreationToDomains) { invalidEmailList = append(invalidEmailList, email) } } diff --git a/app/user.go b/app/user.go index 4df91b577a..0f7f03caed 100644 --- a/app/user.go +++ b/app/user.go @@ -404,8 +404,8 @@ func (a *App) CreateOAuthUser(service string, userData io.Reader, teamId string) return ruser, nil } -// CheckUserDomain checks that a user's email domain matches a list of space-delimited domains as a string. -func CheckUserDomain(user *model.User, domains string) bool { +// CheckEmailDomain checks that an email domain matches a list of space-delimited domains as a string. +func CheckEmailDomain(email string, domains string) bool { if len(domains) == 0 { return true } @@ -413,7 +413,7 @@ func CheckUserDomain(user *model.User, domains string) bool { domainArray := strings.Fields(strings.TrimSpace(strings.ToLower(strings.Replace(strings.Replace(domains, "@", " ", -1), ",", " ", -1)))) for _, d := range domainArray { - if strings.HasSuffix(strings.ToLower(user.Email), "@"+d) { + if strings.HasSuffix(strings.ToLower(email), "@"+d) { return true } } @@ -421,6 +421,11 @@ func CheckUserDomain(user *model.User, domains string) bool { return false } +// CheckUserDomain checks that a user's email domain matches a list of space-delimited domains as a string. +func CheckUserDomain(user *model.User, domains string) bool { + return CheckEmailDomain(user.Email, domains) +} + // IsUsernameTaken checks if the username is already used by another user. Return false if the username is invalid. func (a *App) IsUsernameTaken(name string) bool { if !model.IsValidUsername(name) { @@ -1089,8 +1094,14 @@ func (a *App) UpdateUser(user *model.User, sendNotifications bool) (*model.User, } if !CheckUserDomain(user, *a.Config().TeamSettings.RestrictCreationToDomains) { - if !prev.IsLDAPUser() && !prev.IsSAMLUser() && user.Email != prev.Email { - return nil, model.NewAppError("UpdateUser", "api.user.create_user.accepted_domain.app_error", nil, "", http.StatusBadRequest) + if !prev.IsGuest() && !prev.IsLDAPUser() && !prev.IsSAMLUser() && user.Email != prev.Email { + return nil, model.NewAppError("UpdateUser", "api.user.update_user.accepted_domain.app_error", nil, "", http.StatusBadRequest) + } + } + + if !CheckUserDomain(user, *a.Config().GuestAccountsSettings.RestrictCreationToDomains) { + if prev.IsGuest() && !prev.IsLDAPUser() && !prev.IsSAMLUser() && user.Email != prev.Email { + return nil, model.NewAppError("UpdateUser", "api.user.update_user.accepted_guest_domain.app_error", nil, "", http.StatusBadRequest) } } diff --git a/app/user_test.go b/app/user_test.go index 9755c95818..30fa84b273 100644 --- a/app/user_test.go +++ b/app/user_test.go @@ -147,11 +147,43 @@ func TestUpdateUserToRestrictedDomain(t *testing.T) { }) _, err := th.App.UpdateUser(user, false) - assert.True(t, err == nil) + assert.Nil(t, err) user.Email = "asdf@ghjk.l" _, err = th.App.UpdateUser(user, false) - assert.False(t, err == nil) + assert.NotNil(t, err) + + t.Run("Restricted Domains must be ignored for guest users", func(t *testing.T) { + guest := th.CreateGuest() + defer th.App.PermanentDeleteUser(guest) + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.TeamSettings.RestrictCreationToDomains = "foo.com" + }) + + guest.Email = "asdf@bar.com" + updatedGuest, err := th.App.UpdateUser(guest, false) + require.Nil(t, err) + require.Equal(t, updatedGuest.Email, guest.Email) + }) + + t.Run("Guest users should be affected by guest restricted domains", func(t *testing.T) { + guest := th.CreateGuest() + defer th.App.PermanentDeleteUser(guest) + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.GuestAccountsSettings.RestrictCreationToDomains = "foo.com" + }) + + guest.Email = "asdf@bar.com" + _, err := th.App.UpdateUser(guest, false) + require.NotNil(t, err) + + guest.Email = "asdf@foo.com" + updatedGuest, err := th.App.UpdateUser(guest, false) + require.Nil(t, err) + require.Equal(t, updatedGuest.Email, guest.Email) + }) } func TestUpdateUserActive(t *testing.T) { diff --git a/i18n/en.json b/i18n/en.json index b208ea95c1..f223f309f9 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -2646,6 +2646,14 @@ "id": "api.user.update_password.valid_account.app_error", "translation": "Update password failed because we couldn't find a valid account" }, + { + "id": "api.user.update_user.accepted_domain.app_error", + "translation": "The email you provided does not belong to an accepted domain. Please contact your administrator or sign up with a different email." + }, + { + "id": "api.user.update_user.accepted_guest_domain.app_error", + "translation": "The email you provided does not belong to an accepted domain for guest accounts. Please contact your administrator or sign up with a different email." + }, { "id": "api.user.upload_profile_user.array.app_error", "translation": "Empty array under 'image' in request"