From 6b4ff48bef041848074dc26ec25a9ec5c9e2fedd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20V=C3=A9lez?= Date: Wed, 20 Aug 2025 17:13:01 +0200 Subject: [PATCH] Mm 64925 - prevent slack import email auto validation for non admin users (#33609) * MM-64925 - slack import issue autoverifying emails * system admins imports auto verify emails * pass just the isAdmin instead of the entire user struct * enhance documentation and handle mattermost cmd import --------- Co-authored-by: Mattermost Build --- server/channels/app/slack.go | 19 +- .../services/slackimport/slackimport.go | 29 +- .../services/slackimport/slackimport_test.go | 301 ++++++++++++++++++ 3 files changed, 343 insertions(+), 6 deletions(-) diff --git a/server/channels/app/slack.go b/server/channels/app/slack.go index f4ef300e5c5..ff9f42a0fd6 100644 --- a/server/channels/app/slack.go +++ b/server/channels/app/slack.go @@ -49,7 +49,24 @@ func (a *App) SlackImport(c request.CTX, fileData multipart.File, fileSize int64 }, } - importer := slackimport.New(a.Srv().Store(), actions, a.Config()) + // Determine if this is an Admin import: + // mattermost cmd imports (no session) are treated as admin imports since only server admins can run them + // Web imports (include mmctl calls) check the actual user's role + isAdminImport := false + + if c.Session() == nil { + // no session means it's being run directly on the server and only + // server admins can run CLI commands, so treat as admin import + isAdminImport = true + c.Logger().Info("Slack import initiated via CLI, treating as admin import") + } else if c.Session().UserId != "" { + // Web API + mmctl import - check if the user is a system admin + if user, err := a.GetUser(c.Session().UserId); err == nil { + isAdminImport = user.IsSystemAdmin() + } + } + + importer := slackimport.NewWithAdminFlag(a.Srv().Store(), actions, a.Config(), isAdminImport) return importer.SlackImport(c, fileData, fileSize, teamID) } diff --git a/server/platform/services/slackimport/slackimport.go b/server/platform/services/slackimport/slackimport.go index 98cae638c00..c67345c431f 100644 --- a/server/platform/services/slackimport/slackimport.go +++ b/server/platform/services/slackimport/slackimport.go @@ -100,9 +100,10 @@ type Actions struct { // SlackImporter is a service that allows to import slack dumps into mattermost type SlackImporter struct { - store store.Store - actions Actions - config *model.Config + store store.Store + actions Actions + config *model.Config + isAdminImport bool } // New creates a new SlackImporter service instance. It receive a store, a set of actions and the current config. @@ -115,6 +116,17 @@ func New(store store.Store, actions Actions, config *model.Config) *SlackImporte } } +// NewWithAdminFlag creates a new SlackImporter service instance with information about whether this is an admin import. +// This allows for enhanced security controls based on the importing user's role. +func NewWithAdminFlag(store store.Store, actions Actions, config *model.Config, isAdminImport bool) *SlackImporter { + return &SlackImporter{ + store: store, + actions: actions, + config: config, + isAdminImport: isAdminImport, + } +} + func (si *SlackImporter) SlackImport(rctx request.CTX, fileData multipart.File, fileSize int64, teamID string) (*model.AppError, *bytes.Buffer) { // Create log file log := bytes.NewBufferString(i18n.T("api.slackimport.slack_import.log")) @@ -718,8 +730,15 @@ func (si *SlackImporter) oldImportUser(rctx request.CTX, team *model.Team, user return nil } - if _, err := si.store.User().VerifyEmail(ruser.Id, ruser.Email); err != nil { - rctx.Logger().Warn("Failed to set email verified.", mlog.Err(err)) + // Only system admins can automatically verify emails during import + if si.isAdminImport { + if _, err := si.store.User().VerifyEmail(ruser.Id, ruser.Email); err != nil { + rctx.Logger().Warn("Failed to set email verified for admin import.", mlog.Err(err)) + } + } else { + // Non-admin users: emails remain unverified + rctx.Logger().Debug("Email verification skipped for non-admin import.", + mlog.String("user_email", ruser.Email)) } if _, err := si.actions.JoinUserToTeam(team, user, ""); err != nil { diff --git a/server/platform/services/slackimport/slackimport_test.go b/server/platform/services/slackimport/slackimport_test.go index 1de12743ccf..30e82e56c3e 100644 --- a/server/platform/services/slackimport/slackimport_test.go +++ b/server/platform/services/slackimport/slackimport_test.go @@ -13,6 +13,7 @@ import ( "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "github.com/mattermost/mattermost/server/public/model" @@ -459,3 +460,303 @@ func TestSlackUploadFile(t *testing.T) { require.False(t, ok) }) } + +func TestOldImportUserEmailVerificationIsNotAutomatic(t *testing.T) { + rctx := request.TestContext(t) + + store := &mocks.Store{} + userStore := &mocks.UserStore{} + store.On("User").Return(userStore) + + // Track if VerifyEmail is called (it should NOT be called) + verifyEmailCalled := false + userStore.On("VerifyEmail", mock.AnythingOfType("string"), mock.AnythingOfType("string")).Return("user-id", nil).Run(func(args mock.Arguments) { + verifyEmailCalled = true + }) + + savedUser := &model.User{ + Id: "test-user-id", + Username: "testuser", + Email: "testuser@restricted-domain.com", + EmailVerified: false, // Must remain false after import + Roles: model.SystemUserRoleId, + } + userStore.On("Save", mock.AnythingOfType("*request.Context"), mock.AnythingOfType("*model.User")).Return(savedUser, nil) + + joinTeamCalled := false + actions := Actions{ + JoinUserToTeam: func(team *model.Team, user *model.User, userRequestorId string) (*model.TeamMember, *model.AppError) { + joinTeamCalled = true + return &model.TeamMember{}, nil + }, + } + + config := &model.Config{} + config.SetDefaults() + + importer := New(store, actions, config) + + team := &model.Team{ + Id: "test-team-id", + Name: "test-team", + } + + user := &model.User{ + Username: "testuser", + Email: "testuser@restricted-domain.com", + FirstName: "Test", + LastName: "User", + } + + result := importer.oldImportUser(rctx, team, user) + + require.NotNil(t, result, "User import should succeed") + assert.Equal(t, "test-user-id", result.Id, "Should return the saved user") + assert.False(t, verifyEmailCalled, "SECURITY: VerifyEmail should NOT be called - this prevents domain bypass vulnerability") + assert.True(t, joinTeamCalled, "User should still be joined to the team") + + // Verify the user was saved with unverified email (VerifyEmail should not have been called) + userStore.AssertCalled(t, "Save", mock.AnythingOfType("*request.Context"), mock.MatchedBy(func(u *model.User) bool { + return u.Email == "testuser@restricted-domain.com" && !u.EmailVerified + })) +} + +// TestSlackImportEnhancedSecurityAdminCanVerifyEmails tests that system admins can automatically verify emails +func TestSlackImportEnhancedSecurityAdminCanVerifyEmails(t *testing.T) { + rctx := request.TestContext(t) + + store := &mocks.Store{} + userStore := &mocks.UserStore{} + store.On("User").Return(userStore) + + // Track if VerifyEmail is called (it SHOULD be called for admin imports) + verifyEmailCalled := false + userStore.On("VerifyEmail", mock.AnythingOfType("string"), mock.AnythingOfType("string")).Return("user-id", nil).Run(func(args mock.Arguments) { + verifyEmailCalled = true + }) + + savedUser := &model.User{ + Id: "test-user-id", + Username: "testuser", + Email: "testuser@restricted-domain.com", + EmailVerified: false, // Will be verified by admin import + Roles: model.SystemUserRoleId, + } + userStore.On("Save", mock.AnythingOfType("*request.Context"), mock.AnythingOfType("*model.User")).Return(savedUser, nil) + + joinTeamCalled := false + actions := Actions{ + JoinUserToTeam: func(team *model.Team, user *model.User, userRequestorId string) (*model.TeamMember, *model.AppError) { + joinTeamCalled = true + return &model.TeamMember{}, nil + }, + } + + config := &model.Config{} + config.SetDefaults() + + // Pass true to indicate this is an admin import + importer := NewWithAdminFlag(store, actions, config, true) + + team := &model.Team{ + Id: "test-team-id", + Name: "test-team", + } + + user := &model.User{ + Username: "testuser", + Email: "testuser@restricted-domain.com", + FirstName: "Test", + LastName: "User", + } + + result := importer.oldImportUser(rctx, team, user) + + require.NotNil(t, result, "User import should succeed") + assert.Equal(t, "test-user-id", result.Id, "Should return the saved user") + assert.True(t, verifyEmailCalled, "ADMIN IMPORT: VerifyEmail SHOULD be called for system admin imports") + assert.True(t, joinTeamCalled, "User should still be joined to the team") + + // Verify VerifyEmail was called with correct parameters + userStore.AssertCalled(t, "VerifyEmail", "test-user-id", "testuser@restricted-domain.com") +} + +// TestSlackImportEnhancedSecurityNonAdminCannotVerifyEmails tests that non-admin users cannot automatically verify emails +func TestSlackImportEnhancedSecurityNonAdminCannotVerifyEmails(t *testing.T) { + rctx := request.TestContext(t) + + store := &mocks.Store{} + userStore := &mocks.UserStore{} + store.On("User").Return(userStore) + + // Track if VerifyEmail is called (it should NOT be called for non-admin imports) + verifyEmailCalled := false + userStore.On("VerifyEmail", mock.AnythingOfType("string"), mock.AnythingOfType("string")).Return("user-id", nil).Run(func(args mock.Arguments) { + verifyEmailCalled = true + }) + + savedUser := &model.User{ + Id: "test-user-id", + Username: "testuser", + Email: "testuser@restricted-domain.com", + EmailVerified: false, // Should remain false for non-admin import + Roles: model.SystemUserRoleId, + } + userStore.On("Save", mock.AnythingOfType("*request.Context"), mock.AnythingOfType("*model.User")).Return(savedUser, nil) + + joinTeamCalled := false + actions := Actions{ + JoinUserToTeam: func(team *model.Team, user *model.User, userRequestorId string) (*model.TeamMember, *model.AppError) { + joinTeamCalled = true + return &model.TeamMember{}, nil + }, + } + + config := &model.Config{} + config.SetDefaults() + + // Pass false to indicate this is NOT an admin import + importer := NewWithAdminFlag(store, actions, config, false) + + team := &model.Team{ + Id: "test-team-id", + Name: "test-team", + } + + user := &model.User{ + Username: "testuser", + Email: "testuser@restricted-domain.com", + FirstName: "Test", + LastName: "User", + } + + result := importer.oldImportUser(rctx, team, user) + + require.NotNil(t, result, "User import should succeed") + assert.Equal(t, "test-user-id", result.Id, "Should return the saved user") + assert.False(t, verifyEmailCalled, "NON-ADMIN IMPORT: VerifyEmail should NOT be called for non-admin imports") + assert.True(t, joinTeamCalled, "User should still be joined to the team") + + // Verify VerifyEmail was NOT called + userStore.AssertNotCalled(t, "VerifyEmail") +} + +// TestSlackImportEnhancedSecurityNoImportingUser tests behavior when no importing user is provided +func TestSlackImportEnhancedSecurityNoImportingUser(t *testing.T) { + rctx := request.TestContext(t) + + store := &mocks.Store{} + userStore := &mocks.UserStore{} + store.On("User").Return(userStore) + + // Track if VerifyEmail is called (it should NOT be called when no importing user) + verifyEmailCalled := false + userStore.On("VerifyEmail", mock.AnythingOfType("string"), mock.AnythingOfType("string")).Return("user-id", nil).Run(func(args mock.Arguments) { + verifyEmailCalled = true + }) + + savedUser := &model.User{ + Id: "test-user-id", + Username: "testuser", + Email: "testuser@restricted-domain.com", + EmailVerified: false, // Should remain false when no importing user + Roles: model.SystemUserRoleId, + } + userStore.On("Save", mock.AnythingOfType("*request.Context"), mock.AnythingOfType("*model.User")).Return(savedUser, nil) + + joinTeamCalled := false + actions := Actions{ + JoinUserToTeam: func(team *model.Team, user *model.User, userRequestorId string) (*model.TeamMember, *model.AppError) { + joinTeamCalled = true + return &model.TeamMember{}, nil + }, + } + + config := &model.Config{} + config.SetDefaults() + + // Pass false to indicate no admin privileges (default secure behavior) + importer := NewWithAdminFlag(store, actions, config, false) + + team := &model.Team{ + Id: "test-team-id", + Name: "test-team", + } + + user := &model.User{ + Username: "testuser", + Email: "testuser@restricted-domain.com", + FirstName: "Test", + LastName: "User", + } + + result := importer.oldImportUser(rctx, team, user) + + require.NotNil(t, result, "User import should succeed") + assert.Equal(t, "test-user-id", result.Id, "Should return the saved user") + assert.False(t, verifyEmailCalled, "NO IMPORTING USER: VerifyEmail should NOT be called when no importing user is provided") + assert.True(t, joinTeamCalled, "User should still be joined to the team") + + // Verify VerifyEmail was NOT called + userStore.AssertNotCalled(t, "VerifyEmail") +} + +// TestSlackImportEnhancedSecurityBackwardsCompatibility tests that the old New() constructor still works +func TestSlackImportEnhancedSecurityBackwardsCompatibility(t *testing.T) { + rctx := request.TestContext(t) + + store := &mocks.Store{} + userStore := &mocks.UserStore{} + store.On("User").Return(userStore) + + // Track if VerifyEmail is called (it should NOT be called with old constructor) + verifyEmailCalled := false + userStore.On("VerifyEmail", mock.AnythingOfType("string"), mock.AnythingOfType("string")).Return("user-id", nil).Run(func(args mock.Arguments) { + verifyEmailCalled = true + }) + + savedUser := &model.User{ + Id: "test-user-id", + Username: "testuser", + Email: "testuser@restricted-domain.com", + EmailVerified: false, // Should remain false with old constructor + Roles: model.SystemUserRoleId, + } + userStore.On("Save", mock.AnythingOfType("*request.Context"), mock.AnythingOfType("*model.User")).Return(savedUser, nil) + + joinTeamCalled := false + actions := Actions{ + JoinUserToTeam: func(team *model.Team, user *model.User, userRequestorId string) (*model.TeamMember, *model.AppError) { + joinTeamCalled = true + return &model.TeamMember{}, nil + }, + } + + config := &model.Config{} + config.SetDefaults() + + // Use the old constructor (backwards compatibility) + importer := New(store, actions, config) + + team := &model.Team{ + Id: "test-team-id", + Name: "test-team", + } + + user := &model.User{ + Username: "testuser", + Email: "testuser@restricted-domain.com", + FirstName: "Test", + LastName: "User", + } + + result := importer.oldImportUser(rctx, team, user) + + require.NotNil(t, result, "User import should succeed") + assert.Equal(t, "test-user-id", result.Id, "Should return the saved user") + assert.False(t, verifyEmailCalled, "BACKWARDS COMPATIBILITY: VerifyEmail should NOT be called with old constructor") + assert.True(t, joinTeamCalled, "User should still be joined to the team") + + // Verify VerifyEmail was NOT called + userStore.AssertNotCalled(t, "VerifyEmail") +}