diff --git a/server/service/http_auth.go b/server/service/http_auth.go index c10a89f60d..703835dc32 100644 --- a/server/service/http_auth.go +++ b/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 { diff --git a/server/service/service_errors.go b/server/service/service_errors.go new file mode 100644 index 0000000000..ecc4aaca59 --- /dev/null +++ b/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 + +} diff --git a/server/service/service_users.go b/server/service/service_users.go index f4e0c8cbea..97463f42c6 100644 --- a/server/service/service_users.go +++ b/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 { diff --git a/server/service/service_users_test.go b/server/service/service_users_test.go index 82bf709a78..65233bbad9 100644 --- a/server/service/service_users_test.go +++ b/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) + } + }) + } +} diff --git a/server/service/util_test.go b/server/service/util_test.go index ba3467b755..eef063f2e1 100644 --- a/server/service/util_test.go +++ b/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, }, diff --git a/server/service/validation_users.go b/server/service/validation_users.go index 2817670aa2..f518a8c6d5 100644 --- a/server/service/validation_users.go +++ b/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") }