From 6773d13deee47a437db07e74b0675992cfa912dd Mon Sep 17 00:00:00 2001 From: Doug Lauder Date: Tue, 25 Jun 2024 09:26:08 -0400 Subject: [PATCH] MM-58255 Ensure remote users do not get valid email addresses (#27421) * remote users don't get valid email addresses; remote users cannot have access tokens * block notification emails for remote users --------- Co-authored-by: Mattermost Build --- server/channels/api4/user.go | 6 ++++++ server/channels/api4/user_test.go | 21 +++++++++++++++++++ server/channels/app/notification.go | 4 ++-- .../services/sharedchannel/sync_recv.go | 4 ++-- .../platform/services/sharedchannel/util.go | 9 -------- server/public/model/user.go | 2 +- server/public/model/user_test.go | 10 ++++++++- 7 files changed, 41 insertions(+), 15 deletions(-) diff --git a/server/channels/api4/user.go b/server/channels/api4/user.go index af0c66b9df..3616a624dd 100644 --- a/server/channels/api4/user.go +++ b/server/channels/api4/user.go @@ -2415,6 +2415,12 @@ func createUserAccessToken(c *Context, w http.ResponseWriter, r *http.Request) { audit.AddEventParameterAuditable(auditRec, "user", user) + if user.IsRemote() { + // remote/synthetic users cannot have access tokens + c.SetPermissionError(model.PermissionCreateUserAccessToken) + return + } + if c.AppContext.Session().IsOAuth { c.SetPermissionError(model.PermissionCreateUserAccessToken) c.Err.DetailedError += ", attempted access by oauth app" diff --git a/server/channels/api4/user_test.go b/server/channels/api4/user_test.go index d6c2f013a0..81678c88d2 100644 --- a/server/channels/api4/user_test.go +++ b/server/channels/api4/user_test.go @@ -22,6 +22,7 @@ import ( "github.com/stretchr/testify/require" "github.com/mattermost/mattermost/server/public/model" + "github.com/mattermost/mattermost/server/public/shared/request" "github.com/mattermost/mattermost/server/v8/channels/app" "github.com/mattermost/mattermost/server/v8/channels/utils/testutils" "github.com/mattermost/mattermost/server/v8/einterfaces/mocks" @@ -4634,6 +4635,26 @@ func TestCreateUserAccessToken(t *testing.T) { assertToken(t, th, rtoken, th.BasicUser.Id) }) + t.Run("create user access token for remote user as a system admin", func(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableUserAccessTokens = true }) + + // make a remote user + remoteUser, appErr := th.App.CreateUser(request.TestContext(t), &model.User{ + Username: "remoteuser", + RemoteId: model.NewString(model.NewId()), + Password: model.NewId(), + Email: "remoteuser@example.com", + }) + require.Nil(t, appErr) + + _, resp, err := th.SystemAdminClient.CreateUserAccessToken(context.Background(), remoteUser.Id, "test token") + require.Error(t, err) + CheckForbiddenStatus(t, resp) // remote users are not allowed to have access tokens + }) + t.Run("create access token as oauth session", func(t *testing.T) { th := Setup(t).InitBasic() defer th.TearDown() diff --git a/server/channels/app/notification.go b/server/channels/app/notification.go index 9b243ca05f..f59739f22d 100644 --- a/server/channels/app/notification.go +++ b/server/channels/app/notification.go @@ -1102,8 +1102,8 @@ func max(a, b int64) int64 { } func (a *App) userAllowsEmail(c request.CTX, user *model.User, channelMemberNotificationProps model.StringMap, post *model.Post) bool { - // if user is a bot account, then we do not send email - if user.IsBot { + // if user is a bot account or remote, then we do not send email + if user.IsBot || user.IsRemote() { return false } diff --git a/server/platform/services/sharedchannel/sync_recv.go b/server/platform/services/sharedchannel/sync_recv.go index 1b5f7ec532..25898445c8 100644 --- a/server/platform/services/sharedchannel/sync_recv.go +++ b/server/platform/services/sharedchannel/sync_recv.go @@ -267,7 +267,7 @@ func (scs *Service) insertSyncUser(rctx request.CTX, user *model.User, _ *model. } user.Username = mungUsername(user.Username, rc.Name, suffix, model.UserNameMaxLength) - user.Email = mungEmail(rc.Name, model.UserEmailMaxLength) + user.Email = model.NewId() if userSaved, err = scs.server.GetStore().User().Save(rctx, user); err != nil { field, ok := isConflictError(err) @@ -321,7 +321,7 @@ func (scs *Service) updateSyncUser(rctx request.CTX, patch *model.UserPatch, use suffix = strconv.FormatInt(int64(i), 10) } user.Username = mungUsername(user.Username, rc.Name, suffix, model.UserNameMaxLength) - user.Email = mungEmail(rc.Name, model.UserEmailMaxLength) + user.Email = model.NewId() if update, err = scs.server.GetStore().User().Update(rctx, user, false); err != nil { field, ok := isConflictError(err) diff --git a/server/platform/services/sharedchannel/util.go b/server/platform/services/sharedchannel/util.go index f5a76fc91e..ae42e68868 100644 --- a/server/platform/services/sharedchannel/util.go +++ b/server/platform/services/sharedchannel/util.go @@ -95,15 +95,6 @@ func mungUsername(username string, remotename string, suffix string, maxLen int) return fmt.Sprintf("%s%s%s:%s%s", username, suffix, userEllipses, remotename, remoteEllipses) } -// mungEmail creates a unique email address using a UID and remote name. -func mungEmail(remotename string, maxLen int) string { - s := fmt.Sprintf("%s@%s", model.NewId(), remotename) - if len(s) > maxLen { - s = s[:maxLen] - } - return s -} - func isConflictError(err error) (string, bool) { if err == nil { return "", false diff --git a/server/public/model/user.go b/server/public/model/user.go index 78e8b1503e..8b1b0cfc1e 100644 --- a/server/public/model/user.go +++ b/server/public/model/user.go @@ -346,7 +346,7 @@ func (u *User) IsValid() *AppError { } } - if len(u.Email) > UserEmailMaxLength || u.Email == "" || !IsValidEmail(u.Email) { + if len(u.Email) > UserEmailMaxLength || u.Email == "" || (!IsValidEmail(u.Email) && !u.IsRemote()) { return InvalidUserError("email", u.Id, u.Email) } diff --git a/server/public/model/user_test.go b/server/public/model/user_test.go index 5b18971fbf..2141a52cb0 100644 --- a/server/public/model/user_test.go +++ b/server/public/model/user_test.go @@ -9,9 +9,10 @@ import ( "strings" "testing" - "github.com/mattermost/mattermost/server/public/shared/mlog" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/mattermost/mattermost/server/public/shared/mlog" ) func TestUserDeepCopy(t *testing.T) { @@ -117,6 +118,13 @@ func TestUserIsValid(t *testing.T) { user.LastName = "" require.Nil(t, user.IsValid()) + user.Email = NewId() + appErr = user.IsValid() + require.True(t, HasExpectedUserIsValidError(appErr, "email", user.Id, user.Email), "expected user is valid error: %s", appErr.Error()) + + user.RemoteId = NewString(NewId()) + require.Nil(t, user.IsValid()) + user.FirstName = strings.Repeat("a", 65) appErr = user.IsValid() require.True(t, HasExpectedUserIsValidError(appErr, "first_name", user.Id, user.FirstName), "expected user is valid error: %s", appErr.Error())