Validate password requirements (#962)

Add validation for user password creation/reset
 - at least 7 chars
 - 1 number
 - 1 symbol 

consolidated service errors to a single file.

server/service/http_auth.go

@@ -1,7 +1,6 @@
 package service
 
 import (
-	"fmt"
 	"net/http"
 	"strings"
 
@@ -12,67 +11,6 @@ import (
 	"golang.org/x/net/context"
 )
 
-// authentication error
-type authError struct {
-	reason string
-	// client reason is used to provide
-	// a different error message to the client
-	// when security is a concern
-	clientReason string
-}
-
-func (e authError) Error() string {
-	return e.reason
-}
-
-func (e authError) AuthError() string {
-	if e.clientReason != "" {
-		return e.clientReason
-	}
-	return "username or email and password do not match"
-}
-
-// permissionError, set when user is authenticated, but not allowed to perform action
-type permissionError struct {
-	message string
-	badArgs []invalidArgument
-}
-
-func (e permissionError) Error() string {
-	switch len(e.badArgs) {
-	case 0:
-	case 1:
-		e.message = fmt.Sprintf("unauthorized: %s",
-			e.badArgs[0].reason,
-		)
-	default:
-		e.message = fmt.Sprintf("unauthorized: %s and %d other errors",
-			e.badArgs[0].reason,
-			len(e.badArgs),
-		)
-	}
-	if e.message == "" {
-		return "unauthorized"
-	}
-	return e.message
-}
-
-func (e permissionError) PermissionError() []map[string]string {
-	var forbidden []map[string]string
-	if len(e.badArgs) == 0 {
-		forbidden = append(forbidden, map[string]string{"reason": e.Error()})
-		return forbidden
-	}
-	for _, arg := range e.badArgs {
-		forbidden = append(forbidden, map[string]string{
-			"name":   arg.name,
-			"reason": arg.reason,
-		})
-	}
-	return forbidden
-
-}
-
 // setRequestsContexts updates the request with necessary context values for a request
 func setRequestsContexts(svc kolide.Service, jwtKey string) kithttp.RequestFunc {
 	return func(ctx context.Context, r *http.Request) context.Context {

server/service/service_errors.go

@@ -0,0 +1,119 @@
+package service
+
+import "fmt"
+
+type invalidArgumentError []invalidArgument
+type invalidArgument struct {
+	name   string
+	reason string
+}
+
+// newInvalidArgumentError returns a invalidArgumentError with at least
+// one error.
+func newInvalidArgumentError(name, reason string) *invalidArgumentError {
+	var invalid invalidArgumentError
+	invalid = append(invalid, invalidArgument{
+		name:   name,
+		reason: reason,
+	})
+	return &invalid
+}
+
+func (e *invalidArgumentError) Append(name, reason string) {
+	*e = append(*e, invalidArgument{
+		name:   name,
+		reason: reason,
+	})
+}
+func (e *invalidArgumentError) Appendf(name, reasonFmt string, args ...interface{}) {
+	*e = append(*e, invalidArgument{
+		name:   name,
+		reason: fmt.Sprintf(reasonFmt, args...),
+	})
+}
+
+func (e *invalidArgumentError) HasErrors() bool {
+	return len(*e) != 0
+}
+
+// invalidArgumentError is returned when one or more arguments are invalid.
+func (e invalidArgumentError) Error() string {
+	switch len(e) {
+	case 0:
+		return "validation failed"
+	case 1:
+		return fmt.Sprintf("validation failed: %s %s", e[0].name, e[0].reason)
+	default:
+		return fmt.Sprintf("validation failed: %s %s and %d other errors", e[0].name, e[0].reason,
+			len(e))
+	}
+}
+
+func (e invalidArgumentError) Invalid() []map[string]string {
+	var invalid []map[string]string
+	for _, i := range e {
+		invalid = append(invalid, map[string]string{"name": i.name, "reason": i.reason})
+	}
+	return invalid
+}
+
+// authentication error
+type authError struct {
+	reason string
+	// client reason is used to provide
+	// a different error message to the client
+	// when security is a concern
+	clientReason string
+}
+
+func (e authError) Error() string {
+	return e.reason
+}
+
+func (e authError) AuthError() string {
+	if e.clientReason != "" {
+		return e.clientReason
+	}
+	return "username or email and password do not match"
+}
+
+// permissionError, set when user is authenticated, but not allowed to perform action
+type permissionError struct {
+	message string
+	badArgs []invalidArgument
+}
+
+func (e permissionError) Error() string {
+	switch len(e.badArgs) {
+	case 0:
+	case 1:
+		e.message = fmt.Sprintf("unauthorized: %s",
+			e.badArgs[0].reason,
+		)
+	default:
+		e.message = fmt.Sprintf("unauthorized: %s and %d other errors",
+			e.badArgs[0].reason,
+			len(e.badArgs),
+		)
+	}
+	if e.message == "" {
+		return "unauthorized"
+	}
+	return e.message
+}
+
+func (e permissionError) PermissionError() []map[string]string {
+	var forbidden []map[string]string
+	if len(e.badArgs) == 0 {
+		forbidden = append(forbidden, map[string]string{"reason": e.Error()})
+		return forbidden
+	}
+	for _, arg := range e.badArgs {
+		forbidden = append(forbidden, map[string]string{
+			"name":   arg.name,
+			"reason": arg.reason,
+		})
+	}
+	return forbidden
+
+}

server/service/service_users.go

@@ -143,7 +143,10 @@ func (svc service) ChangePassword(ctx context.Context, oldPass, newPass string)
 		return newInvalidArgumentError("old_password", "old password does not match")
 	}
 
-	return errors.Wrap(svc.setNewPassword(ctx, vc.User, newPass), "setting new password")
+	if err := svc.setNewPassword(ctx, vc.User, newPass); err != nil {
+		return errors.Wrap(err, "setting new password")
+	}
+	return nil
 }
 
 func (svc service) ResetPassword(ctx context.Context, token, password string) error {

server/service/service_users_test.go

@@ -130,19 +130,19 @@ func TestCreateUser(t *testing.T) {
 	}{
 		{
 			Username:    stringPtr("admin2"),
-			Password:    stringPtr("foobar"),
+			Password:    stringPtr("foobarbaz1234!"),
 			InviteToken: &invites["admin2@example.com"].Token,
 			wantErr:     &invalidArgumentError{invalidArgument{name: "email", reason: "missing required argument"}},
 		},
 		{
 			Username: stringPtr("admin2"),
-			Password: stringPtr("foobar"),
+			Password: stringPtr("foobarbaz1234!"),
 			Email:    stringPtr("admin2@example.com"),
 			wantErr:  &invalidArgumentError{invalidArgument{name: "invite_token", reason: "missing required argument"}},
 		},
 		{
 			Username:           stringPtr("admin2"),
-			Password:           stringPtr("foobar"),
+			Password:           stringPtr("foobarbaz1234!"),
 			Email:              stringPtr("admin2@example.com"),
 			NeedsPasswordReset: boolPtr(true),
 			Admin:              boolPtr(false),
@@ -151,7 +151,7 @@ func TestCreateUser(t *testing.T) {
 		{ // should return ErrNotFound because the invite is deleted
 			// after a user signs up
 			Username:           stringPtr("admin2"),
-			Password:           stringPtr("foobar"),
+			Password:           stringPtr("foobarbaz1234!"),
 			Email:              stringPtr("admin2@example.com"),
 			NeedsPasswordReset: boolPtr(true),
 			Admin:              boolPtr(false),
@@ -160,7 +160,7 @@ func TestCreateUser(t *testing.T) {
 		},
 		{
 			Username:           stringPtr("admin3"),
-			Password:           stringPtr("foobar"),
+			Password:           stringPtr("foobarbaz1234!"),
 			Email:              &invites["expired"].Email,
 			NeedsPasswordReset: boolPtr(true),
 			Admin:              boolPtr(false),
@@ -169,7 +169,7 @@ func TestCreateUser(t *testing.T) {
 		},
 		{
 			Username:           stringPtr("@admin2"),
-			Password:           stringPtr("foobar"),
+			Password:           stringPtr("foobarbaz1234!"),
 			Email:              stringPtr("admin2@example.com"),
 			NeedsPasswordReset: boolPtr(true),
 			Admin:              boolPtr(false),
@@ -257,33 +257,36 @@ func TestChangePassword(t *testing.T) {
 	}{
 		{ // all good
 			user:        users["admin1"],
-			oldPassword: "foobar",
-			newPassword: "123cat!",
+			oldPassword: "foobarbaz1234!",
+			newPassword: "12345cat!",
 		},
 		{ // prevent password reuse
 			user:        users["admin1"],
-			oldPassword: "foobar",
-			newPassword: "foobar",
+			oldPassword: "12345cat!",
+			newPassword: "foobarbaz1234!",
 			wantErr:     &invalidArgumentError{invalidArgument{name: "new_password", reason: "cannot reuse old password"}},
 		},
 		{ // all good
 			user:        users["user1"],
-			oldPassword: "foobar",
-			newPassword: "newpass",
+			oldPassword: "foobarbaz1234!",
+			newPassword: "newpassa1234!",
 		},
 		{ // bad old password
 			user:        users["user1"],
 			oldPassword: "wrong_password",
-			newPassword: "123cat!",
+			newPassword: "12345cat!",
 			anyErr:      true,
 		},
 		{ // missing old password
-			newPassword: "123cat!",
+			newPassword: "123cataaa!",
 			wantErr:     &invalidArgumentError{invalidArgument{name: "old_password", reason: "cannot be empty"}},
 		},
 		{ // missing new password
 			oldPassword: "abcd",
-			wantErr:     &invalidArgumentError{invalidArgument{name: "new_password", reason: "cannot be empty"}},
+			wantErr: &invalidArgumentError{
+				{name: "new_password", reason: "cannot be empty"},
+				{name: "new_password", reason: "password does not meet validation requirements"},
+			},
 		},
 	}
 
@@ -295,8 +298,10 @@ func TestChangePassword(t *testing.T) {
 			err := svc.ChangePassword(ctx, tt.oldPassword, tt.newPassword)
 			if tt.anyErr {
 				require.NotNil(t, err)
-			} else {
+			} else if tt.wantErr != nil {
 				require.Equal(t, tt.wantErr, pkg_errors.Cause(err))
+			} else {
+				require.Nil(t, err)
 			}
 
 			if err != nil {
@@ -338,8 +343,11 @@ func TestResetPassword(t *testing.T) {
 			wantErr:     &invalidArgumentError{invalidArgument{name: "token", reason: "cannot be empty field"}},
 		},
 		{ // missing password
-			token:   "abcd",
-			wantErr: &invalidArgumentError{invalidArgument{name: "new_password", reason: "cannot be empty field"}},
+			token: "abcd",
+			wantErr: &invalidArgumentError{
+				{name: "new_password", reason: "cannot be empty field"},
+				{name: "new_password", reason: "password does not meet validation requirements"},
+			},
 		},
 	}
 
@@ -478,3 +486,37 @@ func TestPerformRequiredPasswordReset(t *testing.T) {
 		})
 	}
 }
+
+func TestUserPasswordRequirements(t *testing.T) {
+	var passwordTests = []struct {
+		password string
+		wantErr  bool
+	}{
+		{
+			password: "foobar",
+			wantErr:  true,
+		},
+		{
+			password: "foobarbaz",
+			wantErr:  true,
+		},
+		{
+			password: "foobarbaz!",
+			wantErr:  true,
+		},
+		{
+			password: "foobarbaz!3",
+		},
+	}
+
+	for _, tt := range passwordTests {
+		t.Run(tt.password, func(t *testing.T) {
+			err := validatePasswordRequirements(tt.password)
+			if tt.wantErr {
+				assert.NotNil(t, err)
+			} else {
+				assert.Nil(t, err)
+			}
+		})
+	}
+}

server/service/util_test.go

@@ -69,26 +69,26 @@ var testUsers = map[string]struct {
 }{
 	"admin1": {
 		Username:          "admin1",
-		PlaintextPassword: "foobar",
+		PlaintextPassword: "foobarbaz1234!",
 		Email:             "admin1@example.com",
 		IsAdmin:           true,
 		Enabled:           true,
 	},
 	"user1": {
 		Username:          "user1",
-		PlaintextPassword: "foobar",
+		PlaintextPassword: "foobarbaz1234!",
 		Email:             "user1@example.com",
 		Enabled:           true,
 	},
 	"user2": {
 		Username:          "user2",
-		PlaintextPassword: "bazfoo",
+		PlaintextPassword: "bazfoo1234!",
 		Email:             "user2@example.com",
 		Enabled:           true,
 	},
 	"disabled1": {
 		Username:          "disabled1",
-		PlaintextPassword: "bazfoo",
+		PlaintextPassword: "bazfoo1234!",
 		Email:             "disabled1@example.com",
 		Enabled:           false,
 	},

server/service/validation_users.go

@@ -1,8 +1,9 @@
 package service
 
 import (
-	"fmt"
+	"errors"
 	"strings"
+	"unicode"
 
 	"github.com/kolide/kolide-ose/server/kolide"
 	"golang.org/x/net/context"
@@ -33,6 +34,9 @@ func (mw validationMiddleware) NewUser(ctx context.Context, p kolide.UserPayload
 		if *p.Password == "" {
 			invalid.Append("password", "cannot be empty")
 		}
+		if err := validatePasswordRequirements(*p.Password); err != nil {
+			invalid.Append("password", err.Error())
+		}
 	}
 
 	if p.Email == nil {
@@ -95,6 +99,11 @@ func (mw validationMiddleware) ChangePassword(ctx context.Context, oldPass, newP
 	if newPass == "" {
 		invalid.Append("new_password", "cannot be empty")
 	}
+
+	if err := validatePasswordRequirements(newPass); err != nil {
+		invalid.Append("new_password", err.Error())
+	}
+
 	if invalid.HasErrors() {
 		return invalid
 	}
@@ -109,63 +118,39 @@ func (mw validationMiddleware) ResetPassword(ctx context.Context, token, passwor
 	if password == "" {
 		invalid.Append("new_password", "cannot be empty field")
 	}
+	if err := validatePasswordRequirements(password); err != nil {
+		invalid.Append("new_password", err.Error())
+	}
 	if invalid.HasErrors() {
 		return invalid
 	}
 	return mw.Service.ResetPassword(ctx, token, password)
 }
 
-type invalidArgumentError []invalidArgument
-type invalidArgument struct {
-	name   string
-	reason string
-}
+// Requirements for user password:
+// at least 7 character length
+// at least 1 symbol
+// at least 1 number
+func validatePasswordRequirements(password string) error {
+	var (
+		number bool
+		symbol bool
+	)
 
-// newInvalidArgumentError returns a invalidArgumentError with at least
-// one error.
-func newInvalidArgumentError(name, reason string) *invalidArgumentError {
-	var invalid invalidArgumentError
-	invalid = append(invalid, invalidArgument{
-		name:   name,
-		reason: reason,
-	})
-	return &invalid
-}
-
-func (e *invalidArgumentError) Append(name, reason string) {
-	*e = append(*e, invalidArgument{
-		name:   name,
-		reason: reason,
-	})
-}
-func (e *invalidArgumentError) Appendf(name, reasonFmt string, args ...interface{}) {
-	*e = append(*e, invalidArgument{
-		name:   name,
-		reason: fmt.Sprintf(reasonFmt, args...),
-	})
-}
-
-func (e *invalidArgumentError) HasErrors() bool {
-	return len(*e) != 0
-}
-
-// invalidArgumentError is returned when one or more arguments are invalid.
-func (e invalidArgumentError) Error() string {
-	switch len(e) {
-	case 0:
-		return "validation failed"
-	case 1:
-		return fmt.Sprintf("validation failed: %s %s", e[0].name, e[0].reason)
-	default:
-		return fmt.Sprintf("validation failed: %s %s and %d other errors", e[0].name, e[0].reason,
-			len(e))
+	for _, s := range password {
+		switch {
+		case unicode.IsNumber(s):
+			number = true
+		case unicode.IsPunct(s) || unicode.IsSymbol(s):
+			symbol = true
+		}
 	}
-}
 
-func (e invalidArgumentError) Invalid() []map[string]string {
-	var invalid []map[string]string
-	for _, i := range e {
-		invalid = append(invalid, map[string]string{"name": i.name, "reason": i.reason})
+	if len(password) >= 7 &&
+		number &&
+		symbol {
+		return nil
 	}
-	return invalid
+
+	return errors.New("password does not meet validation requirements")
 }