Mm 64925 - prevent slack import email auto validation for non admin users (#33609) (#33779)

Automatic Merge

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)
 }
 

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
+	store         store.Store
-	actions Actions
+	actions       Actions
-	config  *model.Config
+	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 {
+	// Only system admins can automatically verify emails during import
-		rctx.Logger().Warn("Failed to set email verified.", mlog.Err(err))
+	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 {

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")
+}