diff --git a/server/kolide/users.go b/server/kolide/users.go index 668fb38cd5..9b00a09c01 100644 --- a/server/kolide/users.go +++ b/server/kolide/users.go @@ -38,6 +38,10 @@ type UserService interface { // Users returns all users ListUsers(ctx context.Context, opt ListOptions) (users []*User, err error) + // ChangePassword validates the existing password, and sets the new + // password. User is retrieved from the viewer context. + ChangePassword(ctx context.Context, oldPass, newPass string) error + // RequestPasswordReset generates a password reset request for // a user. The request results in a token emailed to the user. // If the person making the request is an admin the AdminForcedPasswordReset diff --git a/server/service/endpoint_users.go b/server/service/endpoint_users.go index 491dec5719..41f3314f01 100644 --- a/server/service/endpoint_users.go +++ b/server/service/endpoint_users.go @@ -101,6 +101,29 @@ func makeListUsersEndpoint(svc kolide.Service) endpoint.Endpoint { } } +//////////////////////////////////////////////////////////////////////////////// +// Change Password +//////////////////////////////////////////////////////////////////////////////// + +type changePasswordRequest struct { + OldPassword string `json:"old_password"` + NewPassword string `json:"new_password"` +} + +type changePasswordResponse struct { + Err error `json:"error,omitempty"` +} + +func (r changePasswordResponse) error() error { return r.Err } + +func makeChangePasswordEndpoint(svc kolide.Service) endpoint.Endpoint { + return func(ctx context.Context, request interface{}) (interface{}, error) { + req := request.(changePasswordRequest) + err := svc.ChangePassword(ctx, req.OldPassword, req.NewPassword) + return changePasswordResponse{Err: err}, nil + } +} + //////////////////////////////////////////////////////////////////////////////// // Reset Password //////////////////////////////////////////////////////////////////////////////// diff --git a/server/service/handler.go b/server/service/handler.go index 78363bd7be..a1c5223778 100644 --- a/server/service/handler.go +++ b/server/service/handler.go @@ -19,6 +19,7 @@ type KolideEndpoints struct { ForgotPassword endpoint.Endpoint ResetPassword endpoint.Endpoint Me endpoint.Endpoint + ChangePassword endpoint.Endpoint CreateUser endpoint.Endpoint GetUser endpoint.Endpoint ListUsers endpoint.Endpoint @@ -84,6 +85,7 @@ func MakeKolideServerEndpoints(svc kolide.Service, jwtKey string) KolideEndpoint // canPerformActions (these other checks should also call // canPerformActions if that is appropriate). Me: authenticatedUser(jwtKey, svc, canPerformActions(makeGetSessionUserEndpoint(svc))), + ChangePassword: authenticatedUser(jwtKey, svc, canPerformActions(makeChangePasswordEndpoint(svc))), GetUser: authenticatedUser(jwtKey, svc, canReadUser(makeGetUserEndpoint(svc))), ListUsers: authenticatedUser(jwtKey, svc, canPerformActions(makeListUsersEndpoint(svc))), ModifyUser: authenticatedUser(jwtKey, svc, validateModifyUserRequest(makeModifyUserEndpoint(svc))), @@ -140,6 +142,7 @@ type kolideHandlers struct { ForgotPassword *kithttp.Server ResetPassword *kithttp.Server Me *kithttp.Server + ChangePassword *kithttp.Server CreateUser *kithttp.Server GetUser *kithttp.Server ListUsers *kithttp.Server @@ -198,6 +201,7 @@ func makeKolideKitHandlers(ctx context.Context, e KolideEndpoints, opts []kithtt ForgotPassword: newServer(e.ForgotPassword, decodeForgotPasswordRequest), ResetPassword: newServer(e.ResetPassword, decodeResetPasswordRequest), Me: newServer(e.Me, decodeNoParamsRequest), + ChangePassword: newServer(e.ChangePassword, decodeChangePasswordRequest), CreateUser: newServer(e.CreateUser, decodeCreateUserRequest), GetUser: newServer(e.GetUser, decodeGetUserRequest), ListUsers: newServer(e.ListUsers, decodeListUsersRequest), @@ -278,6 +282,7 @@ func attachKolideAPIRoutes(r *mux.Router, h kolideHandlers) { r.Handle("/api/v1/kolide/forgot_password", h.ForgotPassword).Methods("POST") r.Handle("/api/v1/kolide/reset_password", h.ResetPassword).Methods("POST") r.Handle("/api/v1/kolide/me", h.Me).Methods("GET") + r.Handle("/api/v1/kolide/change_password", h.ChangePassword).Methods("POST") r.Handle("/api/v1/kolide/users", h.ListUsers).Methods("GET") r.Handle("/api/v1/kolide/users", h.CreateUser).Methods("POST") diff --git a/server/service/logging_users.go b/server/service/logging_users.go index 6327f04863..d9b5ed1163 100644 --- a/server/service/logging_users.go +++ b/server/service/logging_users.go @@ -118,12 +118,35 @@ func (mw loggingMiddleware) AuthenticatedUser(ctx context.Context) (*kolide.User return user, err } +func (mw loggingMiddleware) ChangePassword(ctx context.Context, oldPass, newPass string) error { + var ( + requestedBy = "unauthenticated" + err error + ) + vc, ok := viewer.FromContext(ctx) + if ok { + requestedBy = vc.Username() + } + + defer func(begin time.Time) { + _ = mw.logger.Log( + "method", "ChangePassword", + "err", err, + "requested_by", requestedBy, + "took", time.Since(begin), + ) + }(time.Now()) + + err = mw.Service.ChangePassword(ctx, oldPass, newPass) + return err +} + func (mw loggingMiddleware) ResetPassword(ctx context.Context, token, password string) error { var err error defer func(begin time.Time) { _ = mw.logger.Log( - "method", "ChangePassword", + "method", "ResetPassword", "err", err, "took", time.Since(begin), ) diff --git a/server/service/metrics_users.go b/server/service/metrics_users.go index fb4f274801..4e87b2b1bb 100644 --- a/server/service/metrics_users.go +++ b/server/service/metrics_users.go @@ -87,6 +87,19 @@ func (mw metricsMiddleware) AuthenticatedUser(ctx context.Context) (*kolide.User return user, err } +func (mw metricsMiddleware) ChangePassword(ctx context.Context, oldPass, newPass string) error { + var err error + + defer func(begin time.Time) { + lvs := []string{"method", "ChangePassword", "error", fmt.Sprint(err != nil)} + mw.requestCount.With(lvs...).Add(1) + mw.requestLatency.With(lvs...).Observe(time.Since(begin).Seconds()) + }(time.Now()) + + err = mw.Service.ChangePassword(ctx, oldPass, newPass) + return err +} + func (mw metricsMiddleware) ResetPassword(ctx context.Context, token, password string) error { var err error diff --git a/server/service/service_users.go b/server/service/service_users.go index 63f59282ac..63edc47969 100644 --- a/server/service/service_users.go +++ b/server/service/service_users.go @@ -7,6 +7,7 @@ import ( "github.com/kolide/kolide-ose/server/contexts/viewer" "github.com/kolide/kolide-ose/server/kolide" + "github.com/pkg/errors" "golang.org/x/net/context" ) @@ -130,33 +131,60 @@ func (svc service) ListUsers(ctx context.Context, opt kolide.ListOptions) ([]*ko return svc.ds.ListUsers(opt) } +// setNewPassword is a helper for changing a user's password. It should be +// called to set the new password after proper authorization has been +// performed. +func (svc service) setNewPassword(ctx context.Context, user *kolide.User, password string) error { + err := user.SetPassword(password, svc.config.Auth.SaltKeySize, svc.config.Auth.BcryptCost) + if err != nil { + return errors.Wrap(err, "setting new password") + } + + err = svc.saveUser(user) + if err != nil { + return errors.Wrap(err, "saving changed password") + } + + return nil +} + +func (svc service) ChangePassword(ctx context.Context, oldPass, newPass string) error { + vc, ok := viewer.FromContext(ctx) + if !ok { + return errNoContext + } + + if err := vc.User.ValidatePassword(oldPass); err != nil { + return errors.Wrap(err, "password validation failed") + } + + return errors.Wrap(svc.setNewPassword(ctx, vc.User, newPass), "setting new password") +} + func (svc service) ResetPassword(ctx context.Context, token, password string) error { reset, err := svc.ds.FindPassswordResetByToken(token) if err != nil { - return err + return errors.Wrap(err, "looking up reset by token") } user, err := svc.User(ctx, reset.UserID) if err != nil { - return err + return errors.Wrap(err, "retrieving user") } - err = user.SetPassword(password, svc.config.Auth.SaltKeySize, svc.config.Auth.BcryptCost) + err = svc.setNewPassword(ctx, user, password) if err != nil { - return err - } - if err := svc.saveUser(user); err != nil { - return err + return errors.Wrap(err, "setting new password") } // delete password reset tokens for user if err := svc.ds.DeletePasswordResetRequestsForUser(user.ID); err != nil { - return err + return errors.Wrap(err, "deleting password reset requests") } // Clear sessions so that any other browsers will have to log in with // the new password if err := svc.DeleteSessionsForUser(ctx, user.ID); err != nil { - return err + return errors.Wrap(err, "deleting user sessions") } return nil diff --git a/server/service/service_users_test.go b/server/service/service_users_test.go index 4202538d78..51342ee5a7 100644 --- a/server/service/service_users_test.go +++ b/server/service/service_users_test.go @@ -5,13 +5,14 @@ import ( "testing" "time" - "github.com/WatchBeam/clock" "github.com/kolide/kolide-ose/server/config" "github.com/kolide/kolide-ose/server/contexts/viewer" "github.com/kolide/kolide-ose/server/datastore/inmem" kolide_errors "github.com/kolide/kolide-ose/server/errors" "github.com/kolide/kolide-ose/server/kolide" + "github.com/WatchBeam/clock" + pkg_errors "github.com/pkg/errors" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/net/context" @@ -244,11 +245,71 @@ func setupInvites(t *testing.T, ds kolide.Datastore, emails []string) map[string return invites } -func TestChangeUserPassword(t *testing.T) { +func TestChangePassword(t *testing.T) { + ds, _ := inmem.New(config.TestConfig()) + svc, _ := newTestService(ds, nil) + users := createTestUsers(t, ds) + var passwordChangeTests = []struct { + user kolide.User + oldPassword string + newPassword string + anyErr bool + wantErr error + }{ + { // all good + user: users["admin1"], + oldPassword: "foobar", + newPassword: "123cat!", + }, + { // all good + user: users["user1"], + oldPassword: "foobar", + newPassword: "newpass", + }, + { // bad old password + user: users["user1"], + oldPassword: "wrong_password", + newPassword: "123cat!", + anyErr: true, + }, + { // missing old password + newPassword: "123cat!", + wantErr: &invalidArgumentError{invalidArgument{name: "old_password", reason: "cannot be empty field"}}, + }, + { // missing new password + oldPassword: "abcd", + wantErr: &invalidArgumentError{invalidArgument{name: "new_password", reason: "cannot be empty field"}}, + }, + } + + for _, tt := range passwordChangeTests { + t.Run("", func(t *testing.T) { + ctx := context.Background() + ctx = viewer.NewContext(ctx, viewer.Viewer{User: &tt.user}) + + err := svc.ChangePassword(ctx, tt.oldPassword, tt.newPassword) + if tt.anyErr { + require.NotNil(t, err) + } else { + require.Equal(t, tt.wantErr, pkg_errors.Cause(err)) + } + + if err != nil { + return + } + + // Attempt login after successful change + _, _, err = svc.Login(context.Background(), tt.user.Username, tt.newPassword) + require.Nil(t, err, "should be able to login with new password") + }) + } +} + +func TestResetPassword(t *testing.T) { ds, _ := inmem.New(config.TestConfig()) svc, _ := newTestService(ds, nil) createTestUsers(t, ds) - var passwordChangeTests = []struct { + var passwordResetTests = []struct { token string newPassword string wantErr error @@ -272,7 +333,7 @@ func TestChangeUserPassword(t *testing.T) { }, } - for _, tt := range passwordChangeTests { + for _, tt := range passwordResetTests { t.Run("", func(t *testing.T) { ctx := context.Background() request := &kolide.PasswordResetRequest{ @@ -292,7 +353,7 @@ func TestChangeUserPassword(t *testing.T) { assert.Nil(t, err) serr := svc.ResetPassword(ctx, tt.token, tt.newPassword) - assert.Equal(t, tt.wantErr, serr) + assert.Equal(t, tt.wantErr, pkg_errors.Cause(serr)) }) } } diff --git a/server/service/transport_users.go b/server/service/transport_users.go index f41121680f..5a952baa9c 100644 --- a/server/service/transport_users.go +++ b/server/service/transport_users.go @@ -32,14 +32,6 @@ func decodeListUsersRequest(ctx context.Context, r *http.Request) (interface{}, return listUsersRequest{ListOptions: opt}, nil } -func decodeChangePasswordRequest(ctx context.Context, r *http.Request) (interface{}, error) { - var req resetPasswordRequest - if err := json.NewDecoder(r.Body).Decode(&req); err != nil { - return nil, err - } - return req, nil -} - func decodeModifyUserRequest(ctx context.Context, r *http.Request) (interface{}, error) { id, err := idFromRequest(r, "id") if err != nil { @@ -53,6 +45,14 @@ func decodeModifyUserRequest(ctx context.Context, r *http.Request) (interface{}, return req, nil } +func decodeChangePasswordRequest(ctx context.Context, r *http.Request) (interface{}, error) { + var req changePasswordRequest + if err := json.NewDecoder(r.Body).Decode(&req); err != nil { + return nil, err + } + return req, nil +} + func decodeForgotPasswordRequest(ctx context.Context, r *http.Request) (interface{}, error) { var req forgotPasswordRequest if err := json.NewDecoder(r.Body).Decode(&req); err != nil { diff --git a/server/service/transport_users_test.go b/server/service/transport_users_test.go index 0f18380dcf..b35a9c28ad 100644 --- a/server/service/transport_users_test.go +++ b/server/service/transport_users_test.go @@ -2,13 +2,13 @@ package service import ( "bytes" - "golang.org/x/net/context" "net/http" "net/http/httptest" "testing" "github.com/gorilla/mux" "github.com/stretchr/testify/assert" + "golang.org/x/net/context" ) func TestDecodeCreateUserRequest(t *testing.T) { @@ -52,10 +52,33 @@ func TestDecodeGetUserRequest(t *testing.T) { func TestDecodeChangePasswordRequest(t *testing.T) { router := mux.NewRouter() - router.HandleFunc("/api/v1/kolide/users/{id}/password", func(writer http.ResponseWriter, request *http.Request) { + router.HandleFunc("/api/v1/kolide/change_password", func(writer http.ResponseWriter, request *http.Request) { r, err := decodeChangePasswordRequest(context.Background(), request) assert.Nil(t, err) + params := r.(changePasswordRequest) + assert.Equal(t, "foo", params.OldPassword) + assert.Equal(t, "bar", params.NewPassword) + }).Methods("POST") + + var body bytes.Buffer + body.Write([]byte(`{ + "old_password": "foo", + "new_password": "bar" + }`)) + + router.ServeHTTP( + httptest.NewRecorder(), + httptest.NewRequest("POST", "/api/v1/kolide/change_password", &body), + ) +} + +func TestDecodeResetPasswordRequest(t *testing.T) { + router := mux.NewRouter() + router.HandleFunc("/api/v1/kolide/users/{id}/password", func(writer http.ResponseWriter, request *http.Request) { + r, err := decodeResetPasswordRequest(context.Background(), request) + assert.Nil(t, err) + params := r.(resetPasswordRequest) assert.Equal(t, "bar", params.NewPassword) assert.Equal(t, "baz", params.PasswordResetToken) diff --git a/server/service/validation_users.go b/server/service/validation_users.go index 4ceeb89f8f..19761d10ea 100644 --- a/server/service/validation_users.go +++ b/server/service/validation_users.go @@ -37,6 +37,20 @@ func (mw validationMiddleware) NewUser(ctx context.Context, p kolide.UserPayload return mw.Service.NewUser(ctx, p) } +func (mw validationMiddleware) ChangePassword(ctx context.Context, oldPass, newPass string) error { + invalid := &invalidArgumentError{} + if oldPass == "" { + invalid.Append("old_password", "cannot be empty field") + } + if newPass == "" { + invalid.Append("new_password", "cannot be empty field") + } + if invalid.HasErrors() { + return invalid + } + return mw.Service.ChangePassword(ctx, oldPass, newPass) +} + func (mw validationMiddleware) ResetPassword(ctx context.Context, token, password string) error { invalid := &invalidArgumentError{} if token == "" {