[MM-33794] Improve password generation during bulk import (#17147)

Automatic Merge

app/import_functions.go

@@ -337,8 +337,12 @@ func (a *App) importUser(data *UserImportData, dryRun bool) *model.AppError {
 		password = *data.Password
 		authData = nil
 	} else {
+		var err error
 		// If no AuthData or Password is specified, we must generate a password.
-		password = model.GeneratePassword(*a.Config().PasswordSettings.MinimumLength)
+		password, err = generatePassword(*a.Config().PasswordSettings.MinimumLength)
+		if err != nil {
+			return model.NewAppError("importUser", "app.import.generate_password.app_error", nil, err.Error(), http.StatusInternalServerError)
+		}
 		authData = nil
 	}
 

app/import_utils.go

@@ -0,0 +1,60 @@
+// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
+// See LICENSE.txt for license information.
+
+package app
+
+import (
+	"crypto/rand"
+	"math/big"
+)
+
+const (
+	passwordSpecialChars     = "!$%^&*(),."
+	passwordNumbers          = "0123456789"
+	passwordUpperCaseLetters = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
+	passwordLowerCaseLetters = "abcdefghijklmnopqrstuvwxyz"
+	passwordAllChars         = passwordSpecialChars + passwordNumbers + passwordUpperCaseLetters + passwordLowerCaseLetters
+)
+
+func randInt(max int) (int, error) {
+	val, err := rand.Int(rand.Reader, big.NewInt(int64(max)))
+	if err != nil {
+		return 0, err
+	}
+	return int(val.Int64()), nil
+}
+
+func generatePassword(minimumLength int) (string, error) {
+	upperIdx, err := randInt(len(passwordUpperCaseLetters))
+	if err != nil {
+		return "", err
+	}
+	numberIdx, err := randInt(len(passwordNumbers))
+	if err != nil {
+		return "", err
+	}
+	lowerIdx, err := randInt(len(passwordLowerCaseLetters))
+	if err != nil {
+		return "", err
+	}
+	specialIdx, err := randInt(len(passwordSpecialChars))
+	if err != nil {
+		return "", err
+	}
+
+	// Make sure we are guaranteed at least one of each type to meet any possible password complexity requirements.
+	password := string([]rune(passwordUpperCaseLetters)[upperIdx]) +
+		string([]rune(passwordNumbers)[numberIdx]) +
+		string([]rune(passwordLowerCaseLetters)[lowerIdx]) +
+		string([]rune(passwordSpecialChars)[specialIdx])
+
+	for len(password) < minimumLength {
+		i, err := randInt(len(passwordAllChars))
+		if err != nil {
+			return "", err
+		}
+		password = password + string([]rune(passwordAllChars)[i])
+	}
+
+	return password, nil
+}

app/import_utils_test.go

@@ -0,0 +1,41 @@
+// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
+// See LICENSE.txt for license information.
+
+package app
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestGeneratePassword(t *testing.T) {
+	t.Run("Should be the minimum length or 4, whichever is less", func(t *testing.T) {
+		password1, err := generatePassword(5)
+		require.NoError(t, err)
+		assert.Len(t, password1, 5)
+		password2, err := generatePassword(10)
+		require.NoError(t, err)
+		assert.Len(t, password2, 10)
+		password3, err := generatePassword(1)
+		require.NoError(t, err)
+		assert.Len(t, password3, 4)
+	})
+
+	t.Run("Should contain at least one of symbols, upper case, lower case and numbers", func(t *testing.T) {
+		password, err := generatePassword(4)
+		require.NoError(t, err)
+		require.Len(t, password, 4)
+		assert.Contains(t, []rune(passwordUpperCaseLetters), []rune(password)[0])
+		assert.Contains(t, []rune(passwordNumbers), []rune(password)[1])
+		assert.Contains(t, []rune(passwordLowerCaseLetters), []rune(password)[2])
+		assert.Contains(t, []rune(passwordSpecialChars), []rune(password)[3])
+	})
+
+	t.Run("Should not fail on concurrent calls", func(t *testing.T) {
+		for i := 0; i < 10; i++ {
+			go generatePassword(10)
+		}
+	})
+}

i18n/en.json

@@ -4422,6 +4422,10 @@
     "id": "app.import.emoji.bad_file.error",
     "translation": "Error reading import emoji image file. Emoji with name: \"{{.EmojiName}}\""
   },
+  {
+    "id": "app.import.generate_password.app_error",
+    "translation": "Error generating password."
+  },
   {
     "id": "app.import.get_teams_by_names.some_teams_not_found.error",
     "translation": "Some teams not found"

model/user.go

@@ -9,13 +9,10 @@ import (
 	"fmt"
 	"io"
 	"io/ioutil"
-	"math/rand"
 	"net/http"
 	"regexp"
 	"sort"
 	"strings"
-	"sync"
-	"time"
 	"unicode/utf8"
 
 	"golang.org/x/crypto/bcrypt"
@@ -940,41 +937,3 @@ func UsersWithGroupsAndCountFromJson(data io.Reader) *UsersWithGroupsAndCount {
 	json.Unmarshal(bodyBytes, uwg)
 	return uwg
 }
-
-//msgp:ignore lockedRand
-type lockedRand struct {
-	mu sync.Mutex
-	rn *rand.Rand
-}
-
-func (r *lockedRand) Intn(n int) int {
-	r.mu.Lock()
-	m := r.rn.Intn(n)
-	r.mu.Unlock()
-	return m
-}
-
-var passwordRandom = lockedRand{
-	rn: rand.New(rand.NewSource(time.Now().Unix())),
-}
-
-var passwordSpecialChars = "!$%^&*(),."
-var passwordNumbers = "0123456789"
-var passwordUpperCaseLetters = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
-var passwordLowerCaseLetters = "abcdefghijklmnopqrstuvwxyz"
-var passwordAllChars = passwordSpecialChars + passwordNumbers + passwordUpperCaseLetters + passwordLowerCaseLetters
-
-func GeneratePassword(minimumLength int) string {
-	// Make sure we are guaranteed at least one of each type to meet any possible password complexity requirements.
-	password := string([]rune(passwordUpperCaseLetters)[passwordRandom.Intn(len(passwordUpperCaseLetters))]) +
-		string([]rune(passwordNumbers)[passwordRandom.Intn(len(passwordNumbers))]) +
-		string([]rune(passwordLowerCaseLetters)[passwordRandom.Intn(len(passwordLowerCaseLetters))]) +
-		string([]rune(passwordSpecialChars)[passwordRandom.Intn(len(passwordSpecialChars))])
-
-	for len(password) < minimumLength {
-		i := passwordRandom.Intn(len(passwordAllChars))
-		password = password + string([]rune(passwordAllChars)[i])
-	}
-
-	return password
-}

model/user_test.go

@@ -5,7 +5,6 @@ package model
 
 import (
 	"fmt"
-	"math/rand"
 	"net/http"
 	"strings"
 	"testing"
@@ -351,33 +350,3 @@ func TestUserSlice(t *testing.T) {
 		assert.Equal(t, 1, len(nonBotUsers))
 	})
 }
-
-func TestGeneratePassword(t *testing.T) {
-	passwordRandom = lockedRand{
-		rn: rand.New(rand.NewSource(12345)),
-	}
-
-	t.Run("Should be the minimum length or 4, whichever is less", func(t *testing.T) {
-		password1 := GeneratePassword(5)
-		assert.Len(t, password1, 5)
-		password2 := GeneratePassword(10)
-		assert.Len(t, password2, 10)
-		password3 := GeneratePassword(1)
-		assert.Len(t, password3, 4)
-	})
-
-	t.Run("Should contain at least one of symbols, upper case, lower case and numbers", func(t *testing.T) {
-		password := GeneratePassword(4)
-		require.Len(t, password, 4)
-		assert.Contains(t, []rune(passwordUpperCaseLetters), []rune(password)[0])
-		assert.Contains(t, []rune(passwordNumbers), []rune(password)[1])
-		assert.Contains(t, []rune(passwordLowerCaseLetters), []rune(password)[2])
-		assert.Contains(t, []rune(passwordSpecialChars), []rune(password)[3])
-	})
-
-	t.Run("Should not fail on concurrent calls", func(t *testing.T) {
-		for i := 0; i < 10; i++ {
-			go GeneratePassword(10)
-		}
-	})
-}