auth: only enforce password length for built-in auth (#8162)

* conf: add AuthMinPasswordLength to handle default value

* auth: only enforce password length for built in auth

* db: add more unit tests

* graphqlbackend: revert enforce for admin-created accounts

* db: fix typo

* db: adjust unit tests

* buildkite/e2e: change test user password to be compliant with minPasswordLength

.buildkite/pipeline.e2e.yml

@@ -5,7 +5,7 @@ env:
   FORCE_COLOR: "3"
   GO111MODULE: "on"
   IMAGE: us.gcr.io/sourcegraph-dev/server:$TAG
-  TEST_USER_PASSWORD: "test"
+  TEST_USER_PASSWORD: "SuperSecurePassword"
 
 steps:
 - command:

cmd/frontend/db/users.go

@@ -102,6 +102,10 @@ type NewUser struct {
 	// user if at least one of the following is true: (1) the site has already been initialized or
 	// (2) any other user account already exists.
 	FailIfNotInitialUser bool `json:"-"` // forbid this field being set by JSON, just in case
+
+	// EnforcePasswordLength is whether should enforce minimum and maximum password length requirement.
+	// Users created by non-builtin auth providers do not have a password thus no need to check.
+	EnforcePasswordLength bool `json:"-"` // forbid this field being set by JSON, just in case
 }
 
 // Create creates a new user in the database.
@@ -151,7 +155,7 @@ const maxPasswordRunes = 256
 // checkPasswordLength returns an error if the password is too long.
 func checkPasswordLength(pw string) error {
 	pwLen := utf8.RuneCountInString(pw)
-	minPasswordRunes := conf.Get().AuthMinPasswordLength
+	minPasswordRunes := conf.AuthMinPasswordLength()
 	if pwLen < minPasswordRunes ||
 		pwLen > maxPasswordRunes {
 		return errcode.NewPresentationError(fmt.Sprintf("Passwords may not be less than %d or be more than %d characters.", minPasswordRunes, maxPasswordRunes))
@@ -166,8 +170,10 @@ func (u *users) create(ctx context.Context, tx *sql.Tx, info NewUser) (newUser *
 		return Mocks.Users.Create(ctx, info)
 	}
 
-	if err := checkPasswordLength(info.Password); err != nil {
-		return nil, err
+	if info.EnforcePasswordLength {
+		if err := checkPasswordLength(info.Password); err != nil {
+			return nil, err
+		}
 	}
 
 	if info.Email != "" && info.EmailVerificationCode == "" && !info.EmailIsVerified {

cmd/frontend/db/users_test.go

@@ -98,41 +98,67 @@ func TestUsers_Create_checkPasswordLength(t *testing.T) {
 	dbtesting.SetupGlobalTestDB(t)
 	ctx := context.Background()
 
-	minPasswordRunes := 12
-	cfg := conf.Get()
-	cfg.AuthMinPasswordLength = minPasswordRunes
-	conf.Mock(cfg)
-	defer func() {
-		cfg.AuthMinPasswordLength = 0
-		conf.Mock(cfg)
-	}()
-
+	minPasswordRunes := conf.AuthMinPasswordLength()
 	expErr := fmt.Sprintf("Passwords may not be less than %d or be more than %d characters.", minPasswordRunes, maxPasswordRunes)
 	tests := []struct {
 		name     string
+		username string
 		password string
+		enforce  bool
 		expErr   string
 	}{
 		{
-			name:     "exceeds maximum",
-			password: strings.Repeat("x", maxPasswordRunes+1),
+			name:     "below minimum",
+			username: "user1",
+			password: strings.Repeat("x", minPasswordRunes-1),
+			enforce:  true,
 			expErr:   expErr,
 		},
 		{
-			name:     "below minimum",
-			password: strings.Repeat("x", minPasswordRunes-1),
+			name:     "exceeds maximum",
+			username: "user2",
+			password: strings.Repeat("x", maxPasswordRunes+1),
+			enforce:  true,
 			expErr:   expErr,
 		},
 
 		{
-			name:     "no problem",
+			name:     "no problem at exact minimum",
+			username: "user3",
 			password: strings.Repeat("x", minPasswordRunes),
+			enforce:  true,
+			expErr:   "",
+		},
+		{
+			name:     "no problem at exact maximum",
+			username: "user4",
+			password: strings.Repeat("x", maxPasswordRunes),
+			enforce:  true,
+			expErr:   "",
+		},
+
+		{
+			name:     "does not enforce and below minimum",
+			username: "user5",
+			password: strings.Repeat("x", minPasswordRunes-1),
+			enforce:  false,
+			expErr:   "",
+		},
+		{
+			name:     "does not enforce and exceeds maximum",
+			username: "user6",
+			password: strings.Repeat("x", maxPasswordRunes+1),
+			enforce:  false,
 			expErr:   "",
 		},
 	}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			_, err := Users.Create(ctx, NewUser{Username: "test", Password: test.password})
+			_, err := Users.Create(ctx, NewUser{
+				Username:              test.username,
+				Password:              test.password,
+				EnforcePasswordLength: test.enforce,
+			})
 			if pm := errcode.PresentationMessage(err); pm != test.expErr {
 				t.Fatalf("err: want %q but got %q", test.expErr, pm)
 			}

cmd/frontend/internal/auth/userpasswd/handlers.go

@@ -98,10 +98,11 @@ func handleSignUp(w http.ResponseWriter, r *http.Request, failIfNewUserIsNotInit
 	// of doServeSignUp checks it, or else that failIfNewUserIsNotInitialSiteAdmin == true (in which
 	// case the only signup allowed is that of the initial site admin).
 	newUserData := db.NewUser{
-		Email:                creds.Email,
-		Username:             creds.Username,
-		Password:             creds.Password,
-		FailIfNotInitialUser: failIfNewUserIsNotInitialSiteAdmin,
+		Email:                 creds.Email,
+		Username:              creds.Username,
+		Password:              creds.Password,
+		FailIfNotInitialUser:  failIfNewUserIsNotInitialSiteAdmin,
+		EnforcePasswordLength: true,
 	}
 	if failIfNewUserIsNotInitialSiteAdmin {
 		// The email of the initial site admin is considered to be verified.

internal/conf/computed.go

@@ -301,3 +301,13 @@ func ExperimentalFeatures() schema.ExperimentalFeatures {
 	}
 	return *val
 }
+
+// AuthMinPasswordLength returns the value of minimum password length requirement.
+// If not set, it returns the default value 12.
+func AuthMinPasswordLength() int {
+	val := Get().AuthMinPasswordLength
+	if val <= 0 {
+		return 12
+	}
+	return val
+}