From b44adff71e913ed58d08e5692cf571c042baae3a Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Mon, 30 Sep 2024 18:47:36 +0000 Subject: [PATCH 01/24] feat: add api for forgotten password flow --- coderd/apidoc/docs.go | 88 +++++++ coderd/apidoc/swagger.json | 74 ++++++ coderd/coderd.go | 2 + coderd/coderdtest/swaggerparser.go | 4 +- coderd/database/dbauthz/dbauthz.go | 8 + coderd/database/dbauthz/dbauthz_test.go | 6 + coderd/database/dbmem/dbmem.go | 21 ++ coderd/database/dbmetrics/dbmetrics.go | 7 + coderd/database/dbmock/dbmock.go | 14 ++ ...260_notifications_forgot_password.down.sql | 1 + ...00260_notifications_forgot_password.up.sql | 4 + coderd/database/querier.go | 1 + coderd/database/queries.sql.go | 25 +- coderd/database/queries/users.sql | 14 +- coderd/notifications/events.go | 2 + coderd/notifications/notifications_test.go | 10 + ...serRequestedOneTimePasscode-body.md.golden | 7 + ...erRequestedOneTimePasscode-title.md.golden | 1 + coderd/userauth.go | 228 ++++++++++++++++++ coderd/userauth_test.go | 43 ++++ codersdk/users.go | 40 +++ docs/reference/api/authorization.md | 66 +++++ docs/reference/api/schemas.md | 32 +++ site/src/api/typesGenerated.ts | 12 + 24 files changed, 707 insertions(+), 3 deletions(-) create mode 100644 coderd/database/migrations/000260_notifications_forgot_password.down.sql create mode 100644 coderd/database/migrations/000260_notifications_forgot_password.up.sql create mode 100644 coderd/notifications/testdata/rendered-templates/TemplateUserRequestedOneTimePasscode-body.md.golden create mode 100644 coderd/notifications/testdata/rendered-templates/TemplateUserRequestedOneTimePasscode-title.md.golden diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index de2bb1e6b91a9..8e7dfe37352ab 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -5094,6 +5094,34 @@ const docTemplate = `{ } } }, + "/users/change-password-with-one-time-passcode": { + "post": { + "consumes": [ + "application/json" + ], + "tags": [ + "Authorization" + ], + "summary": "Change password with a one-time-passcode.", + "operationId": "change-password-with-a-one-time-passcode", + "parameters": [ + { + "description": "Change password request", + "name": "request", + "in": "body", + "required": true, + "schema": { + "$ref": "#/definitions/codersdk.ChangePasswordWithOneTimePasscodeRequest" + } + } + ], + "responses": { + "204": { + "description": "No Content" + } + } + } + }, "/users/first": { "get": { "security": [ @@ -5253,6 +5281,34 @@ const docTemplate = `{ } } }, + "/users/request-one-time-passcode": { + "post": { + "consumes": [ + "application/json" + ], + "tags": [ + "Authorization" + ], + "summary": "Request one-time-passcode.", + "operationId": "request-one-time-passcode", + "parameters": [ + { + "description": "Request one time passcode request", + "name": "request", + "in": "body", + "required": true, + "schema": { + "$ref": "#/definitions/codersdk.RequestOneTimePasscodeRequest" + } + } + ], + "responses": { + "200": { + "description": "OK" + } + } + } + }, "/users/roles": { "get": { "security": [ @@ -9293,6 +9349,26 @@ const docTemplate = `{ "BuildReasonAutostop" ] }, + "codersdk.ChangePasswordWithOneTimePasscodeRequest": { + "type": "object", + "required": [ + "email", + "one_time_passcode", + "password" + ], + "properties": { + "email": { + "type": "string", + "format": "email" + }, + "one_time_passcode": { + "type": "string" + }, + "password": { + "type": "string" + } + } + }, "codersdk.ConnectionLatency": { "type": "object", "properties": { @@ -12271,6 +12347,18 @@ const docTemplate = `{ } } }, + "codersdk.RequestOneTimePasscodeRequest": { + "type": "object", + "required": [ + "email" + ], + "properties": { + "email": { + "type": "string", + "format": "email" + } + } + }, "codersdk.ResolveAutostartResponse": { "type": "object", "properties": { diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index ed640dd50262f..082d793c31634 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -4500,6 +4500,30 @@ } } }, + "/users/change-password-with-one-time-passcode": { + "post": { + "consumes": ["application/json"], + "tags": ["Authorization"], + "summary": "Change password with a one-time-passcode.", + "operationId": "change-password-with-a-one-time-passcode", + "parameters": [ + { + "description": "Change password request", + "name": "request", + "in": "body", + "required": true, + "schema": { + "$ref": "#/definitions/codersdk.ChangePasswordWithOneTimePasscodeRequest" + } + } + ], + "responses": { + "204": { + "description": "No Content" + } + } + } + }, "/users/first": { "get": { "security": [ @@ -4635,6 +4659,30 @@ } } }, + "/users/request-one-time-passcode": { + "post": { + "consumes": ["application/json"], + "tags": ["Authorization"], + "summary": "Request one-time-passcode.", + "operationId": "request-one-time-passcode", + "parameters": [ + { + "description": "Request one time passcode request", + "name": "request", + "in": "body", + "required": true, + "schema": { + "$ref": "#/definitions/codersdk.RequestOneTimePasscodeRequest" + } + } + ], + "responses": { + "200": { + "description": "OK" + } + } + } + }, "/users/roles": { "get": { "security": [ @@ -8264,6 +8312,22 @@ "BuildReasonAutostop" ] }, + "codersdk.ChangePasswordWithOneTimePasscodeRequest": { + "type": "object", + "required": ["email", "one_time_passcode", "password"], + "properties": { + "email": { + "type": "string", + "format": "email" + }, + "one_time_passcode": { + "type": "string" + }, + "password": { + "type": "string" + } + } + }, "codersdk.ConnectionLatency": { "type": "object", "properties": { @@ -11091,6 +11155,16 @@ } } }, + "codersdk.RequestOneTimePasscodeRequest": { + "type": "object", + "required": ["email"], + "properties": { + "email": { + "type": "string", + "format": "email" + } + } + }, "codersdk.ResolveAutostartResponse": { "type": "object", "properties": { diff --git a/coderd/coderd.go b/coderd/coderd.go index 83a780474825b..b798e75880887 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -983,6 +983,8 @@ func New(options *Options) *API { // This value is intentionally increased during tests. r.Use(httpmw.RateLimit(options.LoginRateLimit, time.Minute)) r.Post("/login", api.postLogin) + r.Post("/request-one-time-passcode", api.postRequestOneTimePasscode) + r.Post("/change-password-with-one-time-passcode", api.postChangePasswordWithOneTimePasscode) r.Route("/oauth2", func(r chi.Router) { r.Route("/github", func(r chi.Router) { r.Use( diff --git a/coderd/coderdtest/swaggerparser.go b/coderd/coderdtest/swaggerparser.go index 1b5317e05ff4c..24442fde68b79 100644 --- a/coderd/coderdtest/swaggerparser.go +++ b/coderd/coderdtest/swaggerparser.go @@ -303,7 +303,9 @@ func assertSecurityDefined(t *testing.T, comment SwaggerComment) { if comment.router == "/updatecheck" || comment.router == "/buildinfo" || comment.router == "/" || - comment.router == "/users/login" { + comment.router == "/users/login" || + comment.router == "/users/request-one-time-passcode" || + comment.router == "/users/change-password-with-one-time-passcode" { return // endpoints do not require authorization } assert.Equal(t, "CoderSessionToken", comment.security, "@Security must be equal CoderSessionToken") diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 6436e7c6e3425..88cbf7ad90375 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -3628,6 +3628,14 @@ func (q *querier) UpdateUserGithubComUserID(ctx context.Context, arg database.Up return q.db.UpdateUserGithubComUserID(ctx, arg) } +func (q *querier) UpdateUserHashedOneTimePasscode(ctx context.Context, arg database.UpdateUserHashedOneTimePasscodeParams) error { + if err := q.authorizeContext(ctx, policy.ActionUpdate, rbac.ResourceSystem); err != nil { + return err + } + + return q.db.UpdateUserHashedOneTimePasscode(ctx, arg) +} + func (q *querier) UpdateUserHashedPassword(ctx context.Context, arg database.UpdateUserHashedPasswordParams) error { user, err := q.db.GetUserByID(ctx, arg.ID) if err != nil { diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index f3aec6c9326b0..4dda0bbf8122b 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -1187,6 +1187,12 @@ func (s *MethodTestSuite) TestUser() { ID: u.ID, }).Asserts(u, policy.ActionUpdatePersonal).Returns() })) + s.Run("UpdateUserHashedOneTimePasscode", s.Subtest(func(db database.Store, check *expects) { + u := dbgen.User(s.T(), db, database.User{}) + check.Args(database.UpdateUserHashedOneTimePasscodeParams{ + ID: u.ID, + }).Asserts(rbac.ResourceSystem, policy.ActionUpdate).Returns() + })) s.Run("UpdateUserQuietHoursSchedule", s.Subtest(func(db database.Store, check *expects) { u := dbgen.User(s.T(), db, database.User{}) check.Args(database.UpdateUserQuietHoursScheduleParams{ diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index 09dfa3e7306db..653f54d6d8abe 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -9077,6 +9077,27 @@ func (q *FakeQuerier) UpdateUserGithubComUserID(_ context.Context, arg database. return sql.ErrNoRows } +func (q *FakeQuerier) UpdateUserHashedOneTimePasscode(ctx context.Context, arg database.UpdateUserHashedOneTimePasscodeParams) error { + err := validateDatabaseType(arg) + if err != nil { + return err + } + + q.mutex.Lock() + defer q.mutex.Unlock() + + for i, user := range q.users { + if user.ID != arg.ID { + continue + } + user.HashedOneTimePasscode = arg.HashedOneTimePasscode + user.OneTimePasscodeExpiresAt = arg.OneTimePasscodeExpiresAt + q.users[i] = user + return nil + } + return sql.ErrNoRows +} + func (q *FakeQuerier) UpdateUserHashedPassword(_ context.Context, arg database.UpdateUserHashedPasswordParams) error { if err := validateDatabaseType(arg); err != nil { return err diff --git a/coderd/database/dbmetrics/dbmetrics.go b/coderd/database/dbmetrics/dbmetrics.go index b050a4ce9afc4..93a5e0dabb2a5 100644 --- a/coderd/database/dbmetrics/dbmetrics.go +++ b/coderd/database/dbmetrics/dbmetrics.go @@ -2307,6 +2307,13 @@ func (m metricsStore) UpdateUserGithubComUserID(ctx context.Context, arg databas return r0 } +func (m metricsStore) UpdateUserHashedOneTimePasscode(ctx context.Context, arg database.UpdateUserHashedOneTimePasscodeParams) error { + start := time.Now() + r0 := m.s.UpdateUserHashedOneTimePasscode(ctx, arg) + m.queryLatencies.WithLabelValues("UpdateUserHashedOneTimePasscode").Observe(time.Since(start).Seconds()) + return r0 +} + func (m metricsStore) UpdateUserHashedPassword(ctx context.Context, arg database.UpdateUserHashedPasswordParams) error { start := time.Now() err := m.s.UpdateUserHashedPassword(ctx, arg) diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index 3c7dbd6d9b958..c75e45a6f91db 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -4861,6 +4861,20 @@ func (mr *MockStoreMockRecorder) UpdateUserGithubComUserID(arg0, arg1 any) *gomo return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateUserGithubComUserID", reflect.TypeOf((*MockStore)(nil).UpdateUserGithubComUserID), arg0, arg1) } +// UpdateUserHashedOneTimePasscode mocks base method. +func (m *MockStore) UpdateUserHashedOneTimePasscode(arg0 context.Context, arg1 database.UpdateUserHashedOneTimePasscodeParams) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "UpdateUserHashedOneTimePasscode", arg0, arg1) + ret0, _ := ret[0].(error) + return ret0 +} + +// UpdateUserHashedOneTimePasscode indicates an expected call of UpdateUserHashedOneTimePasscode. +func (mr *MockStoreMockRecorder) UpdateUserHashedOneTimePasscode(arg0, arg1 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateUserHashedOneTimePasscode", reflect.TypeOf((*MockStore)(nil).UpdateUserHashedOneTimePasscode), arg0, arg1) +} + // UpdateUserHashedPassword mocks base method. func (m *MockStore) UpdateUserHashedPassword(arg0 context.Context, arg1 database.UpdateUserHashedPasswordParams) error { m.ctrl.T.Helper() diff --git a/coderd/database/migrations/000260_notifications_forgot_password.down.sql b/coderd/database/migrations/000260_notifications_forgot_password.down.sql new file mode 100644 index 0000000000000..3c85dc3887fbd --- /dev/null +++ b/coderd/database/migrations/000260_notifications_forgot_password.down.sql @@ -0,0 +1 @@ +DELETE FROM notification_templates WHERE id = '62f86a30-2330-4b61-a26d-311ff3b608cf'; diff --git a/coderd/database/migrations/000260_notifications_forgot_password.up.sql b/coderd/database/migrations/000260_notifications_forgot_password.up.sql new file mode 100644 index 0000000000000..27a53d94757ba --- /dev/null +++ b/coderd/database/migrations/000260_notifications_forgot_password.up.sql @@ -0,0 +1,4 @@ +INSERT INTO notification_templates (id, name, title_template, body_template, "group", actions) +VALUES ('62f86a30-2330-4b61-a26d-311ff3b608cf', 'One Time Passcode', E'Your one time passcode is enclosed.', + E'Hi {{.UserName}},\n\nA request to reset the password for your Coder account has been made. Your one time passcode is:\n\n**{{.Labels.one_time_passcode}}**\n\nIf you did not request to reset your password, you can ignore this message.', + 'User Events', '[]'::jsonb); diff --git a/coderd/database/querier.go b/coderd/database/querier.go index d71c54e008350..de0a1a16a7e53 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -457,6 +457,7 @@ type sqlcQuerier interface { UpdateUserAppearanceSettings(ctx context.Context, arg UpdateUserAppearanceSettingsParams) (User, error) UpdateUserDeletedByID(ctx context.Context, id uuid.UUID) error UpdateUserGithubComUserID(ctx context.Context, arg UpdateUserGithubComUserIDParams) error + UpdateUserHashedOneTimePasscode(ctx context.Context, arg UpdateUserHashedOneTimePasscodeParams) error UpdateUserHashedPassword(ctx context.Context, arg UpdateUserHashedPasswordParams) error UpdateUserLastSeenAt(ctx context.Context, arg UpdateUserLastSeenAtParams) (User, error) UpdateUserLink(ctx context.Context, arg UpdateUserLinkParams) (UserLink, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index f5b2943d1fa04..446fa479eeaff 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -10528,11 +10528,34 @@ func (q *sqlQuerier) UpdateUserGithubComUserID(ctx context.Context, arg UpdateUs return err } +const updateUserHashedOneTimePasscode = `-- name: UpdateUserHashedOneTimePasscode :exec +UPDATE + users +SET + hashed_one_time_passcode = $2, + one_time_passcode_expires_at = $3 +WHERE + id = $1 +` + +type UpdateUserHashedOneTimePasscodeParams struct { + ID uuid.UUID `db:"id" json:"id"` + HashedOneTimePasscode []byte `db:"hashed_one_time_passcode" json:"hashed_one_time_passcode"` + OneTimePasscodeExpiresAt sql.NullTime `db:"one_time_passcode_expires_at" json:"one_time_passcode_expires_at"` +} + +func (q *sqlQuerier) UpdateUserHashedOneTimePasscode(ctx context.Context, arg UpdateUserHashedOneTimePasscodeParams) error { + _, err := q.db.ExecContext(ctx, updateUserHashedOneTimePasscode, arg.ID, arg.HashedOneTimePasscode, arg.OneTimePasscodeExpiresAt) + return err +} + const updateUserHashedPassword = `-- name: UpdateUserHashedPassword :exec UPDATE users SET - hashed_password = $2 + hashed_password = $2, + hashed_one_time_passcode = NULL, + one_time_passcode_expires_at = NULL WHERE id = $1 ` diff --git a/coderd/database/queries/users.sql b/coderd/database/queries/users.sql index 44148eb936a33..013e2b8027a45 100644 --- a/coderd/database/queries/users.sql +++ b/coderd/database/queries/users.sql @@ -117,7 +117,9 @@ RETURNING *; UPDATE users SET - hashed_password = $2 + hashed_password = $2, + hashed_one_time_passcode = NULL, + one_time_passcode_expires_at = NULL WHERE id = $1; @@ -289,3 +291,13 @@ RETURNING id, email, last_seen_at; -- AllUserIDs returns all UserIDs regardless of user status or deletion. -- name: AllUserIDs :many SELECT DISTINCT id FROM USERS; + +-- name: UpdateUserHashedOneTimePasscode :exec +UPDATE + users +SET + hashed_one_time_passcode = $2, + one_time_passcode_expires_at = $3 +WHERE + id = $1 +; diff --git a/coderd/notifications/events.go b/coderd/notifications/events.go index 43406c3012317..c2e0f442e0623 100644 --- a/coderd/notifications/events.go +++ b/coderd/notifications/events.go @@ -24,6 +24,8 @@ var ( TemplateUserAccountActivated = uuid.MustParse("9f5af851-8408-4e73-a7a1-c6502ba46689") TemplateYourAccountSuspended = uuid.MustParse("6a2f0609-9b69-4d36-a989-9f5925b6cbff") TemplateYourAccountActivated = uuid.MustParse("1a6a6bea-ee0a-43e2-9e7c-eabdb53730e4") + + TemplateUserRequestedOneTimePasscode = uuid.MustParse("62f86a30-2330-4b61-a26d-311ff3b608cf") ) // Template-related events. diff --git a/coderd/notifications/notifications_test.go b/coderd/notifications/notifications_test.go index 6cc9c9467e9fd..f46b4317f0c0b 100644 --- a/coderd/notifications/notifications_test.go +++ b/coderd/notifications/notifications_test.go @@ -895,6 +895,16 @@ func TestNotificationTemplatesCanRender(t *testing.T) { }, }, }, + { + name: "TemplateUserRequestedOneTimePasscode", + id: notifications.TemplateUserRequestedOneTimePasscode, + payload: types.MessagePayload{ + UserName: "Bobby", + Labels: map[string]string{ + "one_time_passcode": "fad9020b-6562-4cdb-87f1-0486f1bea415", + }, + }, + }, } allTemplates, err := enumerateAllTemplates(t) diff --git a/coderd/notifications/testdata/rendered-templates/TemplateUserRequestedOneTimePasscode-body.md.golden b/coderd/notifications/testdata/rendered-templates/TemplateUserRequestedOneTimePasscode-body.md.golden new file mode 100644 index 0000000000000..adc4e2510bd81 --- /dev/null +++ b/coderd/notifications/testdata/rendered-templates/TemplateUserRequestedOneTimePasscode-body.md.golden @@ -0,0 +1,7 @@ +Hi Bobby, + +A request to reset the password for your Coder account has been made. Your one time passcode is: + +**fad9020b-6562-4cdb-87f1-0486f1bea415** + +If you did not request to reset your password, you can ignore this message. \ No newline at end of file diff --git a/coderd/notifications/testdata/rendered-templates/TemplateUserRequestedOneTimePasscode-title.md.golden b/coderd/notifications/testdata/rendered-templates/TemplateUserRequestedOneTimePasscode-title.md.golden new file mode 100644 index 0000000000000..48382ae544046 --- /dev/null +++ b/coderd/notifications/testdata/rendered-templates/TemplateUserRequestedOneTimePasscode-title.md.golden @@ -0,0 +1 @@ +Your one time passcode is enclosed. \ No newline at end of file diff --git a/coderd/userauth.go b/coderd/userauth.go index b0ef24ad978cf..93fcfd6847469 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -24,6 +24,7 @@ import ( "cdr.dev/slog" "github.com/coder/coder/v2/coderd/idpsync" + "github.com/coder/coder/v2/coderd/notifications" "github.com/coder/coder/v2/coderd/apikey" "github.com/coder/coder/v2/coderd/audit" @@ -201,6 +202,233 @@ func (api *API) postConvertLoginType(rw http.ResponseWriter, r *http.Request) { }) } +// Requests a one-time-passcode for a user. +// +// @Summary Request one-time-passcode. +// @ID request-one-time-passcode +// @Accept json +// @Tags Authorization +// @Param request body codersdk.RequestOneTimePasscodeRequest true "Request one time passcode request" +// @Success 200 +// @Router /users/request-one-time-passcode [post] +func (api *API) postRequestOneTimePasscode(rw http.ResponseWriter, r *http.Request) { + var ( + ctx = r.Context() + auditor = api.Auditor.Load() + logger = api.Logger.Named(userAuthLoggerName) + aReq, commitAudit = audit.InitRequest[database.User](rw, &audit.RequestParams{ + Audit: *auditor, + Log: api.Logger, + Request: r, + Action: database.AuditActionWrite, + }) + ) + defer commitAudit() + + if api.DeploymentValues.DisablePasswordAuth { + httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{ + Message: "Password authentication is disabled.", + }) + return + } + + var req codersdk.RequestOneTimePasscodeRequest + if !httpapi.Read(ctx, rw, r, &req) { + return + } + + defer func() { + // We always send the same response. If we give a more detailed response + // it would open us up to an enumeration attack. + rw.WriteHeader(http.StatusOK) + }() + + //nolint:gocritic // In order to request a one-time-passcode, we need to get the user first! + user, err := api.Database.GetUserByEmailOrUsername(dbauthz.AsSystemRestricted(ctx), database.GetUserByEmailOrUsernameParams{ + Email: req.Email, + }) + if err != nil && !errors.Is(err, sql.ErrNoRows) { + logger.Error(ctx, "unable to get user by email", slog.Error(err)) + return + } + aReq.Old = user + + // TODO: Should we use something different? + passcode := uuid.New() + // TODO: What should the token duration be? + passcodeExpiresAt := dbtime.Now().Add(5 * time.Minute) + + hashedPasscode, err := userpassword.Hash(passcode.String()) + if err != nil { + logger.Error(ctx, "unable to hash passcode", slog.Error(err)) + return + } + + //nolint:gocritic // We need to be able to save the one-time-passcode. + err = api.Database.UpdateUserHashedOneTimePasscode(dbauthz.AsSystemRestricted(ctx), database.UpdateUserHashedOneTimePasscodeParams{ + ID: user.ID, + HashedOneTimePasscode: []byte(hashedPasscode), + OneTimePasscodeExpiresAt: sql.NullTime{Time: passcodeExpiresAt, Valid: true}, + }) + if err != nil { + logger.Error(ctx, "unable to set user hashed one time passcode", slog.Error(err)) + return + } + + newUser := user + newUser.HashedOneTimePasscode = []byte(hashedPasscode) + newUser.OneTimePasscodeExpiresAt = sql.NullTime{Time: passcodeExpiresAt, Valid: true} + aReq.New = newUser + + // Send the one-time-passcode to the user. + api.notifyUserRequestedOneTimePasscode(ctx, user, passcode.String()) +} + +func (api *API) notifyUserRequestedOneTimePasscode(ctx context.Context, user database.User, passcode string) { + _, err := api.NotificationsEnqueuer.Enqueue( + dbauthz.AsSystemRestricted(ctx), + user.ID, + notifications.TemplateUserRequestedOneTimePasscode, + map[string]string{"one_time_passcode": passcode}, + "change-password-with-one-time-passcode", + user.ID, + ) + if err != nil { + api.Logger.Warn(ctx, "unable to notify user about requested one-time-passcode", slog.F("affected_user", user.Username), slog.Error(err)) + } +} + +// Change a users password with a one-time-passcode. +// +// @Summary Change password with a one-time-passcode. +// @ID change-password-with-a-one-time-passcode +// @Accept json +// @Tags Authorization +// @Param request body codersdk.ChangePasswordWithOneTimePasscodeRequest true "Change password request" +// @Success 204 +// @Router /users/change-password-with-one-time-passcode [post] +func (api *API) postChangePasswordWithOneTimePasscode(rw http.ResponseWriter, r *http.Request) { + var ( + err error + ctx = r.Context() + auditor = api.Auditor.Load() + logger = api.Logger.Named(userAuthLoggerName) + aReq, commitAudit = audit.InitRequest[database.User](rw, &audit.RequestParams{ + Audit: *auditor, + Log: api.Logger, + Request: r, + Action: database.AuditActionWrite, + }) + ) + defer commitAudit() + + if api.DeploymentValues.DisablePasswordAuth { + httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{ + Message: "Password authentication is disabled.", + }) + return + } + + var req codersdk.ChangePasswordWithOneTimePasscodeRequest + if !httpapi.Read(ctx, rw, r, &req) { + return + } + + //nolint:gocritic // In order to change a user's password, we need to get the user first! + user, err := api.Database.GetUserByEmailOrUsername(dbauthz.AsSystemRestricted(ctx), database.GetUserByEmailOrUsernameParams{ + Email: req.Email, + }) + if err != nil && !errors.Is(err, sql.ErrNoRows) { + logger.Error(ctx, "unable to fetch user by email", slog.Error(err)) + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error.", + }) + return + } + aReq.Old = user + + equal, err := userpassword.Compare(string(user.HashedOneTimePasscode), req.OneTimePasscode) + if err != nil { + logger.Error(ctx, "unable to compare passwords", slog.Error(err)) + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error.", + }) + return + } + + if !equal { + httpapi.Write(ctx, rw, http.StatusUnauthorized, codersdk.Response{ + Message: "Incorrect email or one-time-passcode.", + }) + return + } + + if err := userpassword.Validate(req.Password); err != nil { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Invalid password.", + Validations: []codersdk.ValidationError{ + { + Field: "password", + Detail: err.Error(), + }, + }, + }) + return + } + + if equal, _ = userpassword.Compare(string(user.HashedPassword), req.Password); equal { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "New password cannot match old password.", + }) + return + } + + newHashedPassword, err := userpassword.Hash(req.Password) + if err != nil { + logger.Error(ctx, "unable to hash new user password", slog.Error(err)) + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error hashing new password.", + Detail: err.Error(), + }) + return + } + + err = api.Database.InTx(func(tx database.Store) error { + //nolint:gocritic // We need to update the user's password. + err = tx.UpdateUserHashedPassword(dbauthz.AsSystemRestricted(ctx), database.UpdateUserHashedPasswordParams{ + ID: user.ID, + HashedPassword: []byte(newHashedPassword), + }) + if err != nil { + return xerrors.Errorf("update user hashed password: %w", err) + } + + //nolint:gocritic // We need to delete all API keys for the user. + err = tx.DeleteAPIKeysByUserID(dbauthz.AsSystemRestricted(ctx), user.ID) + if err != nil { + return xerrors.Errorf("delete api keys for user: %w", err) + } + + return nil + }, nil) + if err != nil { + logger.Error(ctx, "unable to update user's password", slog.Error(err)) + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error updating user's password.", + Detail: err.Error(), + }) + return + } + + newUser := user + newUser.HashedPassword = []byte(newHashedPassword) + newUser.OneTimePasscodeExpiresAt = sql.NullTime{} + newUser.HashedOneTimePasscode = nil + aReq.New = newUser + + rw.WriteHeader(http.StatusNoContent) +} + // Authenticates the user with an email and password. // // @Summary Log in user diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index 6302bee390acd..37aa8bd313ca4 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -31,6 +31,7 @@ import ( "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/database/dbgen" "github.com/coder/coder/v2/coderd/database/dbtestutil" + "github.com/coder/coder/v2/coderd/notifications" "github.com/coder/coder/v2/coderd/promoauth" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/cryptorand" @@ -1654,6 +1655,48 @@ func TestOIDCSkipIssuer(t *testing.T) { require.Equal(t, found.LoginType, codersdk.LoginTypeOIDC) } +func TestUserForgotPassword(t *testing.T) { + t.Parallel() + + t.Run("Can change their password", func(t *testing.T) { + t.Parallel() + + notifyEnq := &testutil.FakeNotificationsEnqueuer{} + + client := coderdtest.New(t, &coderdtest.Options{ + NotificationsEnqueuer: notifyEnq, + }) + user := coderdtest.CreateFirstUser(t, client) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + anotherClient, anotherUser := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) + + err := anotherClient.RequestOneTimePasscode(ctx, codersdk.RequestOneTimePasscodeRequest{ + Email: anotherUser.Email, + }) + require.NoError(t, err) + + require.Equal(t, 2, len(notifyEnq.Sent)) + + notif := notifyEnq.Sent[1] + require.Equal(t, notifications.TemplateUserRequestedOneTimePasscode, notif.TemplateID) + require.Equal(t, anotherUser.ID, notif.UserID) + require.Equal(t, 1, len(notif.Targets)) + require.Equal(t, anotherUser.ID, notif.Targets[0]) + + oneTimePasscode := notif.Labels["one_time_passcode"] + + err = anotherClient.ChangePasswordWithOneTimePasscode(ctx, codersdk.ChangePasswordWithOneTimePasscodeRequest{ + Email: anotherUser.Email, + OneTimePasscode: oneTimePasscode, + Password: "SomeNewSecurePassword!", + }) + require.NoError(t, err) + }) +} + func oauth2Callback(t *testing.T, client *codersdk.Client, opts ...func(*http.Request)) *http.Response { client.HTTPClient.CheckRedirect = func(req *http.Request, via []*http.Request) error { return http.ErrUseLastResponse diff --git a/codersdk/users.go b/codersdk/users.go index e35803abeb15e..d9b80d5730af2 100644 --- a/codersdk/users.go +++ b/codersdk/users.go @@ -243,6 +243,18 @@ type LoginWithPasswordResponse struct { SessionToken string `json:"session_token" validate:"required"` } +// RequestOneTimePasscodeRequest enables callers to request a one-time-passcode to change their password. +type RequestOneTimePasscodeRequest struct { + Email string `json:"email" validate:"required,email" format:"email"` +} + +// ChangePasswordWithOneTimePasscodeRequest enables callers to change their password when they've forgotten it. +type ChangePasswordWithOneTimePasscodeRequest struct { + Email string `json:"email" validate:"required,email" format:"email"` + Password string `json:"password" validate:"required"` + OneTimePasscode string `json:"one_time_passcode" validate:"required"` +} + type OAuthConversionResponse struct { StateString string `json:"state_string"` ExpiresAt time.Time `json:"expires_at" format:"date-time"` @@ -550,6 +562,34 @@ func (c *Client) LoginWithPassword(ctx context.Context, req LoginWithPasswordReq return resp, nil } +func (c *Client) RequestOneTimePasscode(ctx context.Context, req RequestOneTimePasscodeRequest) error { + res, err := c.Request(ctx, http.MethodPost, "/api/v2/users/request-one-time-passcode", req) + if err != nil { + return err + } + defer res.Body.Close() + + if res.StatusCode != http.StatusOK { + return ReadBodyAsError(res) + } + + return nil +} + +func (c *Client) ChangePasswordWithOneTimePasscode(ctx context.Context, req ChangePasswordWithOneTimePasscodeRequest) error { + res, err := c.Request(ctx, http.MethodPost, "/api/v2/users/change-password-with-one-time-passcode", req) + if err != nil { + return err + } + defer res.Body.Close() + + if res.StatusCode != http.StatusNoContent { + return ReadBodyAsError(res) + } + + return nil +} + // ConvertLoginType will send a request to convert the user from password // based authentication to oauth based. The response has the oauth state code // to use in the oauth flow. diff --git a/docs/reference/api/authorization.md b/docs/reference/api/authorization.md index 537d7e6944830..1a2065f033845 100644 --- a/docs/reference/api/authorization.md +++ b/docs/reference/api/authorization.md @@ -68,6 +68,40 @@ curl -X POST http://coder-server:8080/api/v2/authcheck \ To perform this operation, you must be authenticated. [Learn more](authentication.md). +## Change password with a one-time-passcode. + +### Code samples + +```shell +# Example request using curl +curl -X POST http://coder-server:8080/api/v2/users/change-password-with-one-time-passcode \ + -H 'Content-Type: application/json' +``` + +`POST /users/change-password-with-one-time-passcode` + +> Body parameter + +```json +{ + "email": "user@example.com", + "one_time_passcode": "string", + "password": "string" +} +``` + +### Parameters + +| Name | In | Type | Required | Description | +| ------ | ---- | ---------------------------------------------------------------------------------------------------------------- | -------- | ----------------------- | +| `body` | body | [codersdk.ChangePasswordWithOneTimePasscodeRequest](schemas.md#codersdkchangepasswordwithonetimepasscoderequest) | true | Change password request | + +### Responses + +| Status | Meaning | Description | Schema | +| ------ | --------------------------------------------------------------- | ----------- | ------ | +| 204 | [No Content](https://tools.ietf.org/html/rfc7231#section-6.3.5) | No Content | | + ## Log in user ### Code samples @@ -112,6 +146,38 @@ curl -X POST http://coder-server:8080/api/v2/users/login \ | ------ | ------------------------------------------------------------ | ----------- | ---------------------------------------------------------------------------------- | | 201 | [Created](https://tools.ietf.org/html/rfc7231#section-6.3.2) | Created | [codersdk.LoginWithPasswordResponse](schemas.md#codersdkloginwithpasswordresponse) | +## Request one-time-passcode. + +### Code samples + +```shell +# Example request using curl +curl -X POST http://coder-server:8080/api/v2/users/request-one-time-passcode \ + -H 'Content-Type: application/json' +``` + +`POST /users/request-one-time-passcode` + +> Body parameter + +```json +{ + "email": "user@example.com" +} +``` + +### Parameters + +| Name | In | Type | Required | Description | +| ------ | ---- | ------------------------------------------------------------------------------------------ | -------- | --------------------------------- | +| `body` | body | [codersdk.RequestOneTimePasscodeRequest](schemas.md#codersdkrequestonetimepasscoderequest) | true | Request one time passcode request | + +### Responses + +| Status | Meaning | Description | Schema | +| ------ | ------------------------------------------------------- | ----------- | ------ | +| 200 | [OK](https://tools.ietf.org/html/rfc7231#section-6.3.1) | OK | | + ## Convert user from password to oauth authentication ### Code samples diff --git a/docs/reference/api/schemas.md b/docs/reference/api/schemas.md index 00004bb83e74b..03c1d8caf0e4a 100644 --- a/docs/reference/api/schemas.md +++ b/docs/reference/api/schemas.md @@ -930,6 +930,24 @@ AuthorizationObject can represent a "set" of objects, such as: all workspaces in | `autostart` | | `autostop` | +## codersdk.ChangePasswordWithOneTimePasscodeRequest + +```json +{ + "email": "user@example.com", + "one_time_passcode": "string", + "password": "string" +} +``` + +### Properties + +| Name | Type | Required | Restrictions | Description | +| ------------------- | ------ | -------- | ------------ | ----------- | +| `email` | string | true | | | +| `one_time_passcode` | string | true | | | +| `password` | string | true | | | + ## codersdk.ConnectionLatency ```json @@ -4598,6 +4616,20 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o | `region_id` | integer | false | | Region ID is the region of the replica. | | `relay_address` | string | false | | Relay address is the accessible address to relay DERP connections. | +## codersdk.RequestOneTimePasscodeRequest + +```json +{ + "email": "user@example.com" +} +``` + +### Properties + +| Name | Type | Required | Restrictions | Description | +| ------- | ------ | -------- | ------------ | ----------- | +| `email` | string | true | | | + ## codersdk.ResolveAutostartResponse ```json diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index ebc296f57db1b..dcc904081c5f1 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -178,6 +178,13 @@ export interface BuildInfoResponse { readonly deployment_id: string; } +// From codersdk/users.go +export interface ChangePasswordWithOneTimePasscodeRequest { + readonly email: string; + readonly password: string; + readonly one_time_passcode: string; +} + // From codersdk/insights.go export interface ConnectionLatency { readonly p50: number; @@ -1146,6 +1153,11 @@ export interface Replica { readonly database_latency: number; } +// From codersdk/users.go +export interface RequestOneTimePasscodeRequest { + readonly email: string; +} + // From codersdk/workspaces.go export interface ResolveAutostartResponse { readonly parameter_mismatch: boolean; From 60fc32cd095b5c98db08a826747b135ea9a76797 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Tue, 1 Oct 2024 11:24:18 +0000 Subject: [PATCH 02/24] fix: appease linter --- coderd/database/dbmem/dbmem.go | 2 +- coderd/userauth.go | 12 +++++++++--- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index 653f54d6d8abe..026e5f43c4a17 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -9077,7 +9077,7 @@ func (q *FakeQuerier) UpdateUserGithubComUserID(_ context.Context, arg database. return sql.ErrNoRows } -func (q *FakeQuerier) UpdateUserHashedOneTimePasscode(ctx context.Context, arg database.UpdateUserHashedOneTimePasscodeParams) error { +func (q *FakeQuerier) UpdateUserHashedOneTimePasscode(_ context.Context, arg database.UpdateUserHashedOneTimePasscodeParams) error { err := validateDatabaseType(arg) if err != nil { return err diff --git a/coderd/userauth.go b/coderd/userauth.go index 93fcfd6847469..d22f386fd333e 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -281,11 +281,15 @@ func (api *API) postRequestOneTimePasscode(rw http.ResponseWriter, r *http.Reque aReq.New = newUser // Send the one-time-passcode to the user. - api.notifyUserRequestedOneTimePasscode(ctx, user, passcode.String()) + err = api.notifyUserRequestedOneTimePasscode(ctx, user, passcode.String()) + if err != nil { + logger.Error(ctx, "unable to notify user about one time passcode request", slog.Error(err)) + } } -func (api *API) notifyUserRequestedOneTimePasscode(ctx context.Context, user database.User, passcode string) { +func (api *API) notifyUserRequestedOneTimePasscode(ctx context.Context, user database.User, passcode string) error { _, err := api.NotificationsEnqueuer.Enqueue( + //nolint:gocritic // We need to be able to send the user their one time passcode. dbauthz.AsSystemRestricted(ctx), user.ID, notifications.TemplateUserRequestedOneTimePasscode, @@ -294,8 +298,10 @@ func (api *API) notifyUserRequestedOneTimePasscode(ctx context.Context, user dat user.ID, ) if err != nil { - api.Logger.Warn(ctx, "unable to notify user about requested one-time-passcode", slog.F("affected_user", user.Username), slog.Error(err)) + return xerrors.Errorf("enqueue notification: %w", err) } + + return nil } // Change a users password with a one-time-passcode. From 32a8df4b9b09c3aede8fd02a858a8167baaafb66 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Tue, 1 Oct 2024 14:01:05 +0000 Subject: [PATCH 03/24] fix: give 20 minutes for one time passcode --- coderd/userauth.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/coderd/userauth.go b/coderd/userauth.go index d22f386fd333e..37fd60e5055a8 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -253,10 +253,8 @@ func (api *API) postRequestOneTimePasscode(rw http.ResponseWriter, r *http.Reque } aReq.Old = user - // TODO: Should we use something different? passcode := uuid.New() - // TODO: What should the token duration be? - passcodeExpiresAt := dbtime.Now().Add(5 * time.Minute) + passcodeExpiresAt := dbtime.Now().Add(20 * time.Minute) hashedPasscode, err := userpassword.Hash(passcode.String()) if err != nil { From e486923c257ef80cdc4f2023a0e269827de33ad1 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Wed, 2 Oct 2024 08:56:59 +0000 Subject: [PATCH 04/24] chore: changes from feedback --- ...00260_notifications_forgot_password.up.sql | 4 +-- ...serRequestedOneTimePasscode-body.md.golden | 2 +- ...erRequestedOneTimePasscode-title.md.golden | 2 +- coderd/userauth.go | 35 ++++++++++--------- 4 files changed, 22 insertions(+), 21 deletions(-) diff --git a/coderd/database/migrations/000260_notifications_forgot_password.up.sql b/coderd/database/migrations/000260_notifications_forgot_password.up.sql index 27a53d94757ba..2c230b84d58bf 100644 --- a/coderd/database/migrations/000260_notifications_forgot_password.up.sql +++ b/coderd/database/migrations/000260_notifications_forgot_password.up.sql @@ -1,4 +1,4 @@ INSERT INTO notification_templates (id, name, title_template, body_template, "group", actions) -VALUES ('62f86a30-2330-4b61-a26d-311ff3b608cf', 'One Time Passcode', E'Your one time passcode is enclosed.', - E'Hi {{.UserName}},\n\nA request to reset the password for your Coder account has been made. Your one time passcode is:\n\n**{{.Labels.one_time_passcode}}**\n\nIf you did not request to reset your password, you can ignore this message.', +VALUES ('62f86a30-2330-4b61-a26d-311ff3b608cf', 'One-Time Passcode', E'Your one-time passcode is enclosed.', + E'Hi {{.UserName}},\n\nA request to reset the password for your Coder account has been made. Your one-time passcode is:\n\n**{{.Labels.one_time_passcode}}**\n\nIf you did not request to reset your password, you can ignore this message.', 'User Events', '[]'::jsonb); diff --git a/coderd/notifications/testdata/rendered-templates/TemplateUserRequestedOneTimePasscode-body.md.golden b/coderd/notifications/testdata/rendered-templates/TemplateUserRequestedOneTimePasscode-body.md.golden index adc4e2510bd81..6288af33f867e 100644 --- a/coderd/notifications/testdata/rendered-templates/TemplateUserRequestedOneTimePasscode-body.md.golden +++ b/coderd/notifications/testdata/rendered-templates/TemplateUserRequestedOneTimePasscode-body.md.golden @@ -1,6 +1,6 @@ Hi Bobby, -A request to reset the password for your Coder account has been made. Your one time passcode is: +A request to reset the password for your Coder account has been made. Your one-time passcode is: **fad9020b-6562-4cdb-87f1-0486f1bea415** diff --git a/coderd/notifications/testdata/rendered-templates/TemplateUserRequestedOneTimePasscode-title.md.golden b/coderd/notifications/testdata/rendered-templates/TemplateUserRequestedOneTimePasscode-title.md.golden index 48382ae544046..0c1df88eab542 100644 --- a/coderd/notifications/testdata/rendered-templates/TemplateUserRequestedOneTimePasscode-title.md.golden +++ b/coderd/notifications/testdata/rendered-templates/TemplateUserRequestedOneTimePasscode-title.md.golden @@ -1 +1 @@ -Your one time passcode is enclosed. \ No newline at end of file +Your one-time passcode is enclosed. \ No newline at end of file diff --git a/coderd/userauth.go b/coderd/userauth.go index 37fd60e5055a8..f9d6d4c8ac3fe 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -46,6 +46,7 @@ const ( userAuthLoggerName = "userauth" OAuthConvertCookieValue = "coder_oauth_convert_jwt" mergeStateStringPrefix = "convert-" + oneTimePasscodeDuration = 20 * time.Minute ) type OAuthConvertStateClaims struct { @@ -202,13 +203,13 @@ func (api *API) postConvertLoginType(rw http.ResponseWriter, r *http.Request) { }) } -// Requests a one-time-passcode for a user. +// Requests a one-time passcode for a user. // -// @Summary Request one-time-passcode. +// @Summary Request one-time passcode. // @ID request-one-time-passcode // @Accept json // @Tags Authorization -// @Param request body codersdk.RequestOneTimePasscodeRequest true "Request one time passcode request" +// @Param request body codersdk.RequestOneTimePasscodeRequest true "Request one-time passcode request" // @Success 200 // @Router /users/request-one-time-passcode [post] func (api *API) postRequestOneTimePasscode(rw http.ResponseWriter, r *http.Request) { @@ -243,7 +244,7 @@ func (api *API) postRequestOneTimePasscode(rw http.ResponseWriter, r *http.Reque rw.WriteHeader(http.StatusOK) }() - //nolint:gocritic // In order to request a one-time-passcode, we need to get the user first! + //nolint:gocritic // In order to request a one-time passcode, we need to get the user first - and can only do that in the system auth context. user, err := api.Database.GetUserByEmailOrUsername(dbauthz.AsSystemRestricted(ctx), database.GetUserByEmailOrUsernameParams{ Email: req.Email, }) @@ -254,7 +255,7 @@ func (api *API) postRequestOneTimePasscode(rw http.ResponseWriter, r *http.Reque aReq.Old = user passcode := uuid.New() - passcodeExpiresAt := dbtime.Now().Add(20 * time.Minute) + passcodeExpiresAt := dbtime.Now().Add(oneTimePasscodeDuration) hashedPasscode, err := userpassword.Hash(passcode.String()) if err != nil { @@ -262,14 +263,14 @@ func (api *API) postRequestOneTimePasscode(rw http.ResponseWriter, r *http.Reque return } - //nolint:gocritic // We need to be able to save the one-time-passcode. + //nolint:gocritic // We need the system auth context to be able to save the one-time passcode. err = api.Database.UpdateUserHashedOneTimePasscode(dbauthz.AsSystemRestricted(ctx), database.UpdateUserHashedOneTimePasscodeParams{ ID: user.ID, HashedOneTimePasscode: []byte(hashedPasscode), OneTimePasscodeExpiresAt: sql.NullTime{Time: passcodeExpiresAt, Valid: true}, }) if err != nil { - logger.Error(ctx, "unable to set user hashed one time passcode", slog.Error(err)) + logger.Error(ctx, "unable to set user hashed one-time passcode", slog.Error(err)) return } @@ -278,16 +279,16 @@ func (api *API) postRequestOneTimePasscode(rw http.ResponseWriter, r *http.Reque newUser.OneTimePasscodeExpiresAt = sql.NullTime{Time: passcodeExpiresAt, Valid: true} aReq.New = newUser - // Send the one-time-passcode to the user. + // Send the one-time passcode to the user. err = api.notifyUserRequestedOneTimePasscode(ctx, user, passcode.String()) if err != nil { - logger.Error(ctx, "unable to notify user about one time passcode request", slog.Error(err)) + logger.Error(ctx, "unable to notify user about one-time passcode request", slog.Error(err)) } } func (api *API) notifyUserRequestedOneTimePasscode(ctx context.Context, user database.User, passcode string) error { _, err := api.NotificationsEnqueuer.Enqueue( - //nolint:gocritic // We need to be able to send the user their one time passcode. + //nolint:gocritic // We need the system auth context to be able to send the user their one-time passcode. dbauthz.AsSystemRestricted(ctx), user.ID, notifications.TemplateUserRequestedOneTimePasscode, @@ -302,9 +303,9 @@ func (api *API) notifyUserRequestedOneTimePasscode(ctx context.Context, user dat return nil } -// Change a users password with a one-time-passcode. +// Change a users password with a one-time passcode. // -// @Summary Change password with a one-time-passcode. +// @Summary Change password with a one-time passcode. // @ID change-password-with-a-one-time-passcode // @Accept json // @Tags Authorization @@ -338,7 +339,7 @@ func (api *API) postChangePasswordWithOneTimePasscode(rw http.ResponseWriter, r return } - //nolint:gocritic // In order to change a user's password, we need to get the user first! + //nolint:gocritic // In order to change a user's password, we need to get the user first - and can only do that in the system auth context. user, err := api.Database.GetUserByEmailOrUsername(dbauthz.AsSystemRestricted(ctx), database.GetUserByEmailOrUsernameParams{ Email: req.Email, }) @@ -361,7 +362,7 @@ func (api *API) postChangePasswordWithOneTimePasscode(rw http.ResponseWriter, r } if !equal { - httpapi.Write(ctx, rw, http.StatusUnauthorized, codersdk.Response{ + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "Incorrect email or one-time-passcode.", }) return @@ -398,7 +399,7 @@ func (api *API) postChangePasswordWithOneTimePasscode(rw http.ResponseWriter, r } err = api.Database.InTx(func(tx database.Store) error { - //nolint:gocritic // We need to update the user's password. + //nolint:gocritic // We need the system auth context to be able to update the user's password. err = tx.UpdateUserHashedPassword(dbauthz.AsSystemRestricted(ctx), database.UpdateUserHashedPasswordParams{ ID: user.ID, HashedPassword: []byte(newHashedPassword), @@ -407,7 +408,7 @@ func (api *API) postChangePasswordWithOneTimePasscode(rw http.ResponseWriter, r return xerrors.Errorf("update user hashed password: %w", err) } - //nolint:gocritic // We need to delete all API keys for the user. + //nolint:gocritic // We need the system auth context to be able to delete all API keys for the user. err = tx.DeleteAPIKeysByUserID(dbauthz.AsSystemRestricted(ctx), user.ID) if err != nil { return xerrors.Errorf("delete api keys for user: %w", err) @@ -430,7 +431,7 @@ func (api *API) postChangePasswordWithOneTimePasscode(rw http.ResponseWriter, r newUser.HashedOneTimePasscode = nil aReq.New = newUser - rw.WriteHeader(http.StatusNoContent) + rw.WriteHeader(http.StatusOK) } // Authenticates the user with an email and password. From da87e9b7200618f80ddfe192c10f859b5eee082e Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Wed, 2 Oct 2024 09:14:48 +0000 Subject: [PATCH 05/24] fix: make ChangePasswordWithOneTimePassword expect StatusOK --- codersdk/users.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/codersdk/users.go b/codersdk/users.go index d9b80d5730af2..7d23969940e34 100644 --- a/codersdk/users.go +++ b/codersdk/users.go @@ -583,7 +583,7 @@ func (c *Client) ChangePasswordWithOneTimePasscode(ctx context.Context, req Chan } defer res.Body.Close() - if res.StatusCode != http.StatusNoContent { + if res.StatusCode != http.StatusOK { return ReadBodyAsError(res) } From 39fa0f18df2e5978a63c35310c44ed13b3c78b87 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Wed, 2 Oct 2024 09:16:26 +0000 Subject: [PATCH 06/24] docs: run 'make gen' --- coderd/apidoc/docs.go | 10 +++++----- coderd/apidoc/swagger.json | 10 +++++----- coderd/userauth.go | 2 +- docs/reference/api/authorization.md | 12 ++++++------ 4 files changed, 17 insertions(+), 17 deletions(-) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 8e7dfe37352ab..ad653b5e0b047 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -5102,7 +5102,7 @@ const docTemplate = `{ "tags": [ "Authorization" ], - "summary": "Change password with a one-time-passcode.", + "summary": "Change password with a one-time passcode.", "operationId": "change-password-with-a-one-time-passcode", "parameters": [ { @@ -5116,8 +5116,8 @@ const docTemplate = `{ } ], "responses": { - "204": { - "description": "No Content" + "200": { + "description": "OK" } } } @@ -5289,11 +5289,11 @@ const docTemplate = `{ "tags": [ "Authorization" ], - "summary": "Request one-time-passcode.", + "summary": "Request one-time passcode.", "operationId": "request-one-time-passcode", "parameters": [ { - "description": "Request one time passcode request", + "description": "Request one-time passcode request", "name": "request", "in": "body", "required": true, diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 082d793c31634..f35f52b25d1ba 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -4504,7 +4504,7 @@ "post": { "consumes": ["application/json"], "tags": ["Authorization"], - "summary": "Change password with a one-time-passcode.", + "summary": "Change password with a one-time passcode.", "operationId": "change-password-with-a-one-time-passcode", "parameters": [ { @@ -4518,8 +4518,8 @@ } ], "responses": { - "204": { - "description": "No Content" + "200": { + "description": "OK" } } } @@ -4663,11 +4663,11 @@ "post": { "consumes": ["application/json"], "tags": ["Authorization"], - "summary": "Request one-time-passcode.", + "summary": "Request one-time passcode.", "operationId": "request-one-time-passcode", "parameters": [ { - "description": "Request one time passcode request", + "description": "Request one-time passcode request", "name": "request", "in": "body", "required": true, diff --git a/coderd/userauth.go b/coderd/userauth.go index f9d6d4c8ac3fe..8186467644185 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -310,7 +310,7 @@ func (api *API) notifyUserRequestedOneTimePasscode(ctx context.Context, user dat // @Accept json // @Tags Authorization // @Param request body codersdk.ChangePasswordWithOneTimePasscodeRequest true "Change password request" -// @Success 204 +// @Success 200 // @Router /users/change-password-with-one-time-passcode [post] func (api *API) postChangePasswordWithOneTimePasscode(rw http.ResponseWriter, r *http.Request) { var ( diff --git a/docs/reference/api/authorization.md b/docs/reference/api/authorization.md index 1a2065f033845..918a3debd4326 100644 --- a/docs/reference/api/authorization.md +++ b/docs/reference/api/authorization.md @@ -68,7 +68,7 @@ curl -X POST http://coder-server:8080/api/v2/authcheck \ To perform this operation, you must be authenticated. [Learn more](authentication.md). -## Change password with a one-time-passcode. +## Change password with a one-time passcode. ### Code samples @@ -98,9 +98,9 @@ curl -X POST http://coder-server:8080/api/v2/users/change-password-with-one-time ### Responses -| Status | Meaning | Description | Schema | -| ------ | --------------------------------------------------------------- | ----------- | ------ | -| 204 | [No Content](https://tools.ietf.org/html/rfc7231#section-6.3.5) | No Content | | +| Status | Meaning | Description | Schema | +| ------ | ------------------------------------------------------- | ----------- | ------ | +| 200 | [OK](https://tools.ietf.org/html/rfc7231#section-6.3.1) | OK | | ## Log in user @@ -146,7 +146,7 @@ curl -X POST http://coder-server:8080/api/v2/users/login \ | ------ | ------------------------------------------------------------ | ----------- | ---------------------------------------------------------------------------------- | | 201 | [Created](https://tools.ietf.org/html/rfc7231#section-6.3.2) | Created | [codersdk.LoginWithPasswordResponse](schemas.md#codersdkloginwithpasswordresponse) | -## Request one-time-passcode. +## Request one-time passcode. ### Code samples @@ -170,7 +170,7 @@ curl -X POST http://coder-server:8080/api/v2/users/request-one-time-passcode \ | Name | In | Type | Required | Description | | ------ | ---- | ------------------------------------------------------------------------------------------ | -------- | --------------------------------- | -| `body` | body | [codersdk.RequestOneTimePasscodeRequest](schemas.md#codersdkrequestonetimepasscoderequest) | true | Request one time passcode request | +| `body` | body | [codersdk.RequestOneTimePasscodeRequest](schemas.md#codersdkrequestonetimepasscoderequest) | true | Request one-time passcode request | ### Responses From b89dfd7ec6337e3cf0913db7743513f7e66616da Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Wed, 2 Oct 2024 10:41:49 +0000 Subject: [PATCH 07/24] test: add more tests --- coderd/userauth.go | 18 +++--- coderd/userauth_test.go | 124 ++++++++++++++++++++++++++++++++++++++-- 2 files changed, 128 insertions(+), 14 deletions(-) diff --git a/coderd/userauth.go b/coderd/userauth.go index 8186467644185..6839c81db92ec 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -274,10 +274,10 @@ func (api *API) postRequestOneTimePasscode(rw http.ResponseWriter, r *http.Reque return } - newUser := user - newUser.HashedOneTimePasscode = []byte(hashedPasscode) - newUser.OneTimePasscodeExpiresAt = sql.NullTime{Time: passcodeExpiresAt, Valid: true} - aReq.New = newUser + auditUser := user + auditUser.HashedOneTimePasscode = []byte(hashedPasscode) + auditUser.OneTimePasscodeExpiresAt = sql.NullTime{Time: passcodeExpiresAt, Valid: true} + aReq.New = auditUser // Send the one-time passcode to the user. err = api.notifyUserRequestedOneTimePasscode(ctx, user, passcode.String()) @@ -425,11 +425,11 @@ func (api *API) postChangePasswordWithOneTimePasscode(rw http.ResponseWriter, r return } - newUser := user - newUser.HashedPassword = []byte(newHashedPassword) - newUser.OneTimePasscodeExpiresAt = sql.NullTime{} - newUser.HashedOneTimePasscode = nil - aReq.New = newUser + auditUser := user + auditUser.HashedPassword = []byte(newHashedPassword) + auditUser.OneTimePasscodeExpiresAt = sql.NullTime{} + auditUser.HashedOneTimePasscode = nil + aReq.New = auditUser rw.WriteHeader(http.StatusOK) } diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index 37aa8bd313ca4..b14c191a1da38 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -1658,7 +1658,16 @@ func TestOIDCSkipIssuer(t *testing.T) { func TestUserForgotPassword(t *testing.T) { t.Parallel() - t.Run("Can change their password", func(t *testing.T) { + verifyOneTimePasscodeNotification := func(t *testing.T, notif *testutil.Notification, userID uuid.UUID) { + require.Equal(t, notifications.TemplateUserRequestedOneTimePasscode, notif.TemplateID) + require.Equal(t, userID, notif.UserID) + require.Equal(t, 1, len(notif.Targets)) + require.Equal(t, userID, notif.Targets[0]) + } + + t.Run("CanChangeTheirPassword", func(t *testing.T) { + const newPassword = "SomeNewSecurePassword!" + t.Parallel() notifyEnq := &testutil.FakeNotificationsEnqueuer{} @@ -1681,19 +1690,124 @@ func TestUserForgotPassword(t *testing.T) { require.Equal(t, 2, len(notifyEnq.Sent)) notif := notifyEnq.Sent[1] - require.Equal(t, notifications.TemplateUserRequestedOneTimePasscode, notif.TemplateID) - require.Equal(t, anotherUser.ID, notif.UserID) - require.Equal(t, 1, len(notif.Targets)) - require.Equal(t, anotherUser.ID, notif.Targets[0]) + verifyOneTimePasscodeNotification(t, notif, anotherUser.ID) oneTimePasscode := notif.Labels["one_time_passcode"] err = anotherClient.ChangePasswordWithOneTimePasscode(ctx, codersdk.ChangePasswordWithOneTimePasscodeRequest{ Email: anotherUser.Email, OneTimePasscode: oneTimePasscode, + Password: newPassword, + }) + require.NoError(t, err) + + _, err = anotherClient.LoginWithPassword(ctx, codersdk.LoginWithPasswordRequest{ + Email: anotherUser.Email, + Password: newPassword, + }) + require.NoError(t, err) + }) + + t.Run("CannotChangePasswordWithInvalidOneTimePasscode", func(t *testing.T) { + t.Parallel() + + notifyEnq := &testutil.FakeNotificationsEnqueuer{} + + client := coderdtest.New(t, &coderdtest.Options{ + NotificationsEnqueuer: notifyEnq, + }) + user := coderdtest.CreateFirstUser(t, client) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + anotherClient, anotherUser := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) + + err := anotherClient.RequestOneTimePasscode(ctx, codersdk.RequestOneTimePasscodeRequest{ + Email: anotherUser.Email, + }) + require.NoError(t, err) + + require.Equal(t, 2, len(notifyEnq.Sent)) + + notif := notifyEnq.Sent[1] + verifyOneTimePasscodeNotification(t, notif, anotherUser.ID) + + err = anotherClient.ChangePasswordWithOneTimePasscode(ctx, codersdk.ChangePasswordWithOneTimePasscodeRequest{ + Email: anotherUser.Email, + OneTimePasscode: uuid.New().String(), // Use a different UUID to the one expected Password: "SomeNewSecurePassword!", }) + require.Error(t, err) + + var apiErr *codersdk.Error + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusBadRequest, apiErr.StatusCode()) + }) + + t.Run("CannotChangePasswordWithWeakPassword", func(t *testing.T) { + t.Parallel() + + notifyEnq := &testutil.FakeNotificationsEnqueuer{} + + client := coderdtest.New(t, &coderdtest.Options{ + NotificationsEnqueuer: notifyEnq, + }) + user := coderdtest.CreateFirstUser(t, client) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + anotherClient, anotherUser := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) + + err := anotherClient.RequestOneTimePasscode(ctx, codersdk.RequestOneTimePasscodeRequest{ + Email: anotherUser.Email, + }) require.NoError(t, err) + + require.Equal(t, 2, len(notifyEnq.Sent)) + + notif := notifyEnq.Sent[1] + verifyOneTimePasscodeNotification(t, notif, anotherUser.ID) + + oneTimePasscode := notif.Labels["one_time_passcode"] + + err = anotherClient.ChangePasswordWithOneTimePasscode(ctx, codersdk.ChangePasswordWithOneTimePasscodeRequest{ + Email: anotherUser.Email, + OneTimePasscode: oneTimePasscode, + Password: "notstrong", + }) + require.Error(t, err) + + var apiErr *codersdk.Error + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusBadRequest, apiErr.StatusCode()) + }) + + t.Run("NoNotificationIsSentIfEmailInvalid", func(t *testing.T) { + t.Parallel() + + notifyEnq := &testutil.FakeNotificationsEnqueuer{} + + client := coderdtest.New(t, &coderdtest.Options{ + NotificationsEnqueuer: notifyEnq, + }) + user := coderdtest.CreateFirstUser(t, client) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + anotherClient, _ := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) + + err := anotherClient.RequestOneTimePasscode(ctx, codersdk.RequestOneTimePasscodeRequest{ + Email: "not-a-valid-email@coder.com", + }) + require.NoError(t, err) + + require.Equal(t, 1, len(notifyEnq.Sent)) + + notif := notifyEnq.Sent[0] + require.NotEqual(t, notifications.TemplateUserRequestedOneTimePasscode, notif.TemplateID) }) } From 714e31668d533362b2ed526fccb2ef89906913b2 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Wed, 2 Oct 2024 10:49:15 +0000 Subject: [PATCH 08/24] test: ensure login fails before changing password --- coderd/userauth_test.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index b14c191a1da38..e199d6d9eeb12 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -1682,7 +1682,15 @@ func TestUserForgotPassword(t *testing.T) { anotherClient, anotherUser := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) - err := anotherClient.RequestOneTimePasscode(ctx, codersdk.RequestOneTimePasscodeRequest{ + // First try to login before changing our password. We expected this to error + // as we haven't change the password yet. + _, err := anotherClient.LoginWithPassword(ctx, codersdk.LoginWithPasswordRequest{ + Email: anotherUser.Email, + Password: newPassword, + }) + require.Error(t, err) + + err = anotherClient.RequestOneTimePasscode(ctx, codersdk.RequestOneTimePasscodeRequest{ Email: anotherUser.Email, }) require.NoError(t, err) @@ -1701,6 +1709,7 @@ func TestUserForgotPassword(t *testing.T) { }) require.NoError(t, err) + // Now that we have changed the password, this should work. _, err = anotherClient.LoginWithPassword(ctx, codersdk.LoginWithPasswordRequest{ Email: anotherUser.Email, Password: newPassword, From c4ed20520de65a7c5e07f3998475f7a2beaef1f1 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Wed, 2 Oct 2024 12:09:10 +0000 Subject: [PATCH 09/24] refactor: test and dbmem.UpdateUserHashedOneTimePasscode --- coderd/database/dbmem/dbmem.go | 3 +-- coderd/userauth_test.go | 10 +++++----- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index 026e5f43c4a17..d66197334a330 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -9093,9 +9093,8 @@ func (q *FakeQuerier) UpdateUserHashedOneTimePasscode(_ context.Context, arg dat user.HashedOneTimePasscode = arg.HashedOneTimePasscode user.OneTimePasscodeExpiresAt = arg.OneTimePasscodeExpiresAt q.users[i] = user - return nil } - return sql.ErrNoRows + return nil } func (q *FakeQuerier) UpdateUserHashedPassword(_ context.Context, arg database.UpdateUserHashedPasswordParams) error { diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index e199d6d9eeb12..ba8359fe8ee9a 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -1793,7 +1793,7 @@ func TestUserForgotPassword(t *testing.T) { require.Equal(t, http.StatusBadRequest, apiErr.StatusCode()) }) - t.Run("NoNotificationIsSentIfEmailInvalid", func(t *testing.T) { + t.Run("GivenOKResponseWithInvalidEmail", func(t *testing.T) { t.Parallel() notifyEnq := &testutil.FakeNotificationsEnqueuer{} @@ -1809,14 +1809,14 @@ func TestUserForgotPassword(t *testing.T) { anotherClient, _ := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) err := anotherClient.RequestOneTimePasscode(ctx, codersdk.RequestOneTimePasscodeRequest{ - Email: "not-a-valid-email@coder.com", + Email: "not-a-member@coder.com", }) require.NoError(t, err) - require.Equal(t, 1, len(notifyEnq.Sent)) + require.Equal(t, 2, len(notifyEnq.Sent)) - notif := notifyEnq.Sent[0] - require.NotEqual(t, notifications.TemplateUserRequestedOneTimePasscode, notif.TemplateID) + notif := notifyEnq.Sent[1] + verifyOneTimePasscodeNotification(t, notif, uuid.Nil) }) } From fa9d426a74e29433a5db72ba4aef077f5526fea8 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Wed, 2 Oct 2024 13:26:08 +0000 Subject: [PATCH 10/24] fix: do not enqueue notification if user.ID is uuid.Nil --- coderd/userauth.go | 10 ++++++---- coderd/userauth_test.go | 8 ++++---- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/coderd/userauth.go b/coderd/userauth.go index 6839c81db92ec..44378a7060f8d 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -279,10 +279,12 @@ func (api *API) postRequestOneTimePasscode(rw http.ResponseWriter, r *http.Reque auditUser.OneTimePasscodeExpiresAt = sql.NullTime{Time: passcodeExpiresAt, Valid: true} aReq.New = auditUser - // Send the one-time passcode to the user. - err = api.notifyUserRequestedOneTimePasscode(ctx, user, passcode.String()) - if err != nil { - logger.Error(ctx, "unable to notify user about one-time passcode request", slog.Error(err)) + if user.ID != uuid.Nil { + // Send the one-time passcode to the user. + err = api.notifyUserRequestedOneTimePasscode(ctx, user, passcode.String()) + if err != nil { + logger.Error(ctx, "unable to notify user about one-time passcode request", slog.Error(err)) + } } } diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index ba8359fe8ee9a..afd1c88c32594 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -1665,7 +1665,7 @@ func TestUserForgotPassword(t *testing.T) { require.Equal(t, userID, notif.Targets[0]) } - t.Run("CanChangeTheirPassword", func(t *testing.T) { + t.Run("CanChangePassword", func(t *testing.T) { const newPassword = "SomeNewSecurePassword!" t.Parallel() @@ -1813,10 +1813,10 @@ func TestUserForgotPassword(t *testing.T) { }) require.NoError(t, err) - require.Equal(t, 2, len(notifyEnq.Sent)) + require.Equal(t, 1, len(notifyEnq.Sent)) - notif := notifyEnq.Sent[1] - verifyOneTimePasscodeNotification(t, notif, uuid.Nil) + notif := notifyEnq.Sent[0] + require.NotEqual(t, notifications.TemplateUserRequestedOneTimePasscode, notif.TemplateID) }) } From 883a231a7d955510811aaf3a6f9e3cf1c5afcb2c Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Wed, 2 Oct 2024 14:43:41 +0000 Subject: [PATCH 11/24] refactor: wrap GetUserByEmailOrUsername in transaction --- coderd/userauth.go | 113 +++++++++++++++++++++------------------------ 1 file changed, 52 insertions(+), 61 deletions(-) diff --git a/coderd/userauth.go b/coderd/userauth.go index 44378a7060f8d..739a5bb155205 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -341,66 +341,57 @@ func (api *API) postChangePasswordWithOneTimePasscode(rw http.ResponseWriter, r return } - //nolint:gocritic // In order to change a user's password, we need to get the user first - and can only do that in the system auth context. - user, err := api.Database.GetUserByEmailOrUsername(dbauthz.AsSystemRestricted(ctx), database.GetUserByEmailOrUsernameParams{ - Email: req.Email, - }) - if err != nil && !errors.Is(err, sql.ErrNoRows) { - logger.Error(ctx, "unable to fetch user by email", slog.Error(err)) - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error.", + err = api.Database.InTx(func(tx database.Store) error { + //nolint:gocritic // In order to change a user's password, we need to get the user first - and can only do that in the system auth context. + user, err := tx.GetUserByEmailOrUsername(dbauthz.AsSystemRestricted(ctx), database.GetUserByEmailOrUsernameParams{ + Email: req.Email, }) - return - } - aReq.Old = user + if err != nil && !errors.Is(err, sql.ErrNoRows) { + logger.Error(ctx, "unable to fetch user by email", slog.F("email", req.Email), slog.Error(err)) + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error.", + }) + return nil + } + aReq.Old = user - equal, err := userpassword.Compare(string(user.HashedOneTimePasscode), req.OneTimePasscode) - if err != nil { - logger.Error(ctx, "unable to compare passwords", slog.Error(err)) - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error.", - }) - return - } + equal, err := userpassword.Compare(string(user.HashedOneTimePasscode), req.OneTimePasscode) + if err != nil { + return xerrors.Errorf("compare one time passcode: %w", err) + } - if !equal { - httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: "Incorrect email or one-time-passcode.", - }) - return - } + if !equal { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Incorrect email or one-time-passcode.", + }) + return nil + } - if err := userpassword.Validate(req.Password); err != nil { - httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: "Invalid password.", - Validations: []codersdk.ValidationError{ - { - Field: "password", - Detail: err.Error(), + if err := userpassword.Validate(req.Password); err != nil { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Invalid password.", + Validations: []codersdk.ValidationError{ + { + Field: "password", + Detail: err.Error(), + }, }, - }, - }) - return - } + }) + return nil + } - if equal, _ = userpassword.Compare(string(user.HashedPassword), req.Password); equal { - httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: "New password cannot match old password.", - }) - return - } + if equal, _ = userpassword.Compare(string(user.HashedPassword), req.Password); equal { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "New password cannot match old password.", + }) + return nil + } - newHashedPassword, err := userpassword.Hash(req.Password) - if err != nil { - logger.Error(ctx, "unable to hash new user password", slog.Error(err)) - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error hashing new password.", - Detail: err.Error(), - }) - return - } + newHashedPassword, err := userpassword.Hash(req.Password) + if err != nil { + return xerrors.Errorf("hash user password: %w", err) + } - err = api.Database.InTx(func(tx database.Store) error { //nolint:gocritic // We need the system auth context to be able to update the user's password. err = tx.UpdateUserHashedPassword(dbauthz.AsSystemRestricted(ctx), database.UpdateUserHashedPasswordParams{ ID: user.ID, @@ -416,24 +407,24 @@ func (api *API) postChangePasswordWithOneTimePasscode(rw http.ResponseWriter, r return xerrors.Errorf("delete api keys for user: %w", err) } + auditUser := user + auditUser.HashedPassword = []byte(newHashedPassword) + auditUser.OneTimePasscodeExpiresAt = sql.NullTime{} + auditUser.HashedOneTimePasscode = nil + aReq.New = auditUser + + rw.WriteHeader(http.StatusOK) + return nil }, nil) if err != nil { logger.Error(ctx, "unable to update user's password", slog.Error(err)) httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error updating user's password.", + Message: "Internal error.", Detail: err.Error(), }) return } - - auditUser := user - auditUser.HashedPassword = []byte(newHashedPassword) - auditUser.OneTimePasscodeExpiresAt = sql.NullTime{} - auditUser.HashedOneTimePasscode = nil - aReq.New = auditUser - - rw.WriteHeader(http.StatusOK) } // Authenticates the user with an email and password. From bff384b355f56f6bb6a70ef2273d5f17074a5eb1 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Wed, 2 Oct 2024 14:56:47 +0000 Subject: [PATCH 12/24] refactor: error handling --- coderd/userauth.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/coderd/userauth.go b/coderd/userauth.go index 739a5bb155205..eda485480b29b 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -348,15 +348,13 @@ func (api *API) postChangePasswordWithOneTimePasscode(rw http.ResponseWriter, r }) if err != nil && !errors.Is(err, sql.ErrNoRows) { logger.Error(ctx, "unable to fetch user by email", slog.F("email", req.Email), slog.Error(err)) - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error.", - }) - return nil + return xerrors.Errorf("get user by email: %w", err) } aReq.Old = user equal, err := userpassword.Compare(string(user.HashedOneTimePasscode), req.OneTimePasscode) if err != nil { + logger.Error(ctx, "unable to compare one time passcode", slog.Error(err)) return xerrors.Errorf("compare one time passcode: %w", err) } @@ -389,6 +387,7 @@ func (api *API) postChangePasswordWithOneTimePasscode(rw http.ResponseWriter, r newHashedPassword, err := userpassword.Hash(req.Password) if err != nil { + logger.Error(ctx, "unable to hash user's password", slog.Error(err)) return xerrors.Errorf("hash user password: %w", err) } @@ -398,12 +397,14 @@ func (api *API) postChangePasswordWithOneTimePasscode(rw http.ResponseWriter, r HashedPassword: []byte(newHashedPassword), }) if err != nil { + logger.Error(ctx, "unable to delete user's hashed password", slog.Error(err)) return xerrors.Errorf("update user hashed password: %w", err) } //nolint:gocritic // We need the system auth context to be able to delete all API keys for the user. err = tx.DeleteAPIKeysByUserID(dbauthz.AsSystemRestricted(ctx), user.ID) if err != nil { + logger.Error(ctx, "unable to delete user's api keys", slog.Error(err)) return xerrors.Errorf("delete api keys for user: %w", err) } @@ -418,7 +419,6 @@ func (api *API) postChangePasswordWithOneTimePasscode(rw http.ResponseWriter, r return nil }, nil) if err != nil { - logger.Error(ctx, "unable to update user's password", slog.Error(err)) httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error.", Detail: err.Error(), From 26cc758eb8d135d54ab7620292b461536c7b3c86 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Wed, 2 Oct 2024 18:31:57 +0000 Subject: [PATCH 13/24] fix: check one-time passcode expiry --- coderd/apidoc/docs.go | 4 ++-- coderd/apidoc/swagger.json | 4 ++-- coderd/userauth.go | 7 ++++--- docs/reference/api/authorization.md | 4 ++-- 4 files changed, 10 insertions(+), 9 deletions(-) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index ad653b5e0b047..7f68bbf8cd813 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -5102,7 +5102,7 @@ const docTemplate = `{ "tags": [ "Authorization" ], - "summary": "Change password with a one-time passcode.", + "summary": "Change password with a one-time passcode", "operationId": "change-password-with-a-one-time-passcode", "parameters": [ { @@ -5289,7 +5289,7 @@ const docTemplate = `{ "tags": [ "Authorization" ], - "summary": "Request one-time passcode.", + "summary": "Request one-time passcode", "operationId": "request-one-time-passcode", "parameters": [ { diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index f35f52b25d1ba..c7d1e3e4eb6fc 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -4504,7 +4504,7 @@ "post": { "consumes": ["application/json"], "tags": ["Authorization"], - "summary": "Change password with a one-time passcode.", + "summary": "Change password with a one-time passcode", "operationId": "change-password-with-a-one-time-passcode", "parameters": [ { @@ -4663,7 +4663,7 @@ "post": { "consumes": ["application/json"], "tags": ["Authorization"], - "summary": "Request one-time passcode.", + "summary": "Request one-time passcode", "operationId": "request-one-time-passcode", "parameters": [ { diff --git a/coderd/userauth.go b/coderd/userauth.go index eda485480b29b..1da97455c1cfe 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -205,7 +205,7 @@ func (api *API) postConvertLoginType(rw http.ResponseWriter, r *http.Request) { // Requests a one-time passcode for a user. // -// @Summary Request one-time passcode. +// @Summary Request one-time passcode // @ID request-one-time-passcode // @Accept json // @Tags Authorization @@ -307,7 +307,7 @@ func (api *API) notifyUserRequestedOneTimePasscode(ctx context.Context, user dat // Change a users password with a one-time passcode. // -// @Summary Change password with a one-time passcode. +// @Summary Change password with a one-time passcode // @ID change-password-with-a-one-time-passcode // @Accept json // @Tags Authorization @@ -358,7 +358,8 @@ func (api *API) postChangePasswordWithOneTimePasscode(rw http.ResponseWriter, r return xerrors.Errorf("compare one time passcode: %w", err) } - if !equal { + now := dbtime.Now() + if !equal || now.After(user.OneTimePasscodeExpiresAt.Time) { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "Incorrect email or one-time-passcode.", }) diff --git a/docs/reference/api/authorization.md b/docs/reference/api/authorization.md index 918a3debd4326..cd02184b85419 100644 --- a/docs/reference/api/authorization.md +++ b/docs/reference/api/authorization.md @@ -68,7 +68,7 @@ curl -X POST http://coder-server:8080/api/v2/authcheck \ To perform this operation, you must be authenticated. [Learn more](authentication.md). -## Change password with a one-time passcode. +## Change password with a one-time passcode ### Code samples @@ -146,7 +146,7 @@ curl -X POST http://coder-server:8080/api/v2/users/login \ | ------ | ------------------------------------------------------------ | ----------- | ---------------------------------------------------------------------------------- | | 201 | [Created](https://tools.ietf.org/html/rfc7231#section-6.3.2) | Created | [codersdk.LoginWithPasswordResponse](schemas.md#codersdkloginwithpasswordresponse) | -## Request one-time passcode. +## Request one-time passcode ### Code samples From 25581c2a5d499e610be8a9d30cad6ded09f80b47 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Wed, 2 Oct 2024 18:40:54 +0000 Subject: [PATCH 14/24] test: one-time passcode becomes invalid after use --- coderd/database/dbmem/dbmem.go | 2 ++ coderd/userauth_test.go | 45 ++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+) diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index d66197334a330..2b43b80115724 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -9110,6 +9110,8 @@ func (q *FakeQuerier) UpdateUserHashedPassword(_ context.Context, arg database.U continue } user.HashedPassword = arg.HashedPassword + user.HashedOneTimePasscode = nil + user.OneTimePasscodeExpiresAt = sql.NullTime{} q.users[i] = user return nil } diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index afd1c88c32594..977c5d545a2f8 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -1715,6 +1715,14 @@ func TestUserForgotPassword(t *testing.T) { Password: newPassword, }) require.NoError(t, err) + + // We now need to check that the one-time passcode isn't valid. + err = anotherClient.ChangePasswordWithOneTimePasscode(ctx, codersdk.ChangePasswordWithOneTimePasscodeRequest{ + Email: anotherUser.Email, + OneTimePasscode: oneTimePasscode, + Password: "SomeDifferentSecurePassword!", + }) + require.Error(t, err) }) t.Run("CannotChangePasswordWithInvalidOneTimePasscode", func(t *testing.T) { @@ -1754,6 +1762,43 @@ func TestUserForgotPassword(t *testing.T) { require.Equal(t, http.StatusBadRequest, apiErr.StatusCode()) }) + t.Run("CannotChangePasswordWithNoOneTimePasscode", func(t *testing.T) { + t.Parallel() + + notifyEnq := &testutil.FakeNotificationsEnqueuer{} + + client := coderdtest.New(t, &coderdtest.Options{ + NotificationsEnqueuer: notifyEnq, + }) + user := coderdtest.CreateFirstUser(t, client) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + anotherClient, anotherUser := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) + + err := anotherClient.RequestOneTimePasscode(ctx, codersdk.RequestOneTimePasscodeRequest{ + Email: anotherUser.Email, + }) + require.NoError(t, err) + + require.Equal(t, 2, len(notifyEnq.Sent)) + + notif := notifyEnq.Sent[1] + verifyOneTimePasscodeNotification(t, notif, anotherUser.ID) + + err = anotherClient.ChangePasswordWithOneTimePasscode(ctx, codersdk.ChangePasswordWithOneTimePasscodeRequest{ + Email: anotherUser.Email, + OneTimePasscode: "", + Password: "SomeNewSecurePassword!", + }) + require.Error(t, err) + + var apiErr *codersdk.Error + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusBadRequest, apiErr.StatusCode()) + }) + t.Run("CannotChangePasswordWithWeakPassword", func(t *testing.T) { t.Parallel() From 6078409140f294fd49e61b5cb69d16e8ff3f69da Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Thu, 3 Oct 2024 08:17:50 +0000 Subject: [PATCH 15/24] fix: make changes from feedback --- coderd/apidoc/docs.go | 8 ++-- coderd/apidoc/swagger.json | 8 ++-- ...00261_notifications_forgot_password.up.sql | 2 +- ...erRequestedOneTimePasscode-title.md.golden | 2 +- coderd/userauth.go | 38 ++++++++++--------- coderd/userauth_test.go | 26 ++++++++----- codersdk/users.go | 4 +- docs/reference/api/authorization.md | 12 +++--- 8 files changed, 54 insertions(+), 46 deletions(-) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 1ca43e69281c7..fd8b02afd8e49 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -5116,8 +5116,8 @@ const docTemplate = `{ } ], "responses": { - "200": { - "description": "OK" + "204": { + "description": "No Content" } } } @@ -5303,8 +5303,8 @@ const docTemplate = `{ } ], "responses": { - "200": { - "description": "OK" + "204": { + "description": "No Content" } } } diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index eff9607fd635d..52f6e3e2038f2 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -4518,8 +4518,8 @@ } ], "responses": { - "200": { - "description": "OK" + "204": { + "description": "No Content" } } } @@ -4677,8 +4677,8 @@ } ], "responses": { - "200": { - "description": "OK" + "204": { + "description": "No Content" } } } diff --git a/coderd/database/migrations/000261_notifications_forgot_password.up.sql b/coderd/database/migrations/000261_notifications_forgot_password.up.sql index 2c230b84d58bf..a5c1982be3d98 100644 --- a/coderd/database/migrations/000261_notifications_forgot_password.up.sql +++ b/coderd/database/migrations/000261_notifications_forgot_password.up.sql @@ -1,4 +1,4 @@ INSERT INTO notification_templates (id, name, title_template, body_template, "group", actions) -VALUES ('62f86a30-2330-4b61-a26d-311ff3b608cf', 'One-Time Passcode', E'Your one-time passcode is enclosed.', +VALUES ('62f86a30-2330-4b61-a26d-311ff3b608cf', 'One-Time Passcode', E'Your One-Time Passcode for Coder.', E'Hi {{.UserName}},\n\nA request to reset the password for your Coder account has been made. Your one-time passcode is:\n\n**{{.Labels.one_time_passcode}}**\n\nIf you did not request to reset your password, you can ignore this message.', 'User Events', '[]'::jsonb); diff --git a/coderd/notifications/testdata/rendered-templates/TemplateUserRequestedOneTimePasscode-title.md.golden b/coderd/notifications/testdata/rendered-templates/TemplateUserRequestedOneTimePasscode-title.md.golden index 0c1df88eab542..ecf7383911053 100644 --- a/coderd/notifications/testdata/rendered-templates/TemplateUserRequestedOneTimePasscode-title.md.golden +++ b/coderd/notifications/testdata/rendered-templates/TemplateUserRequestedOneTimePasscode-title.md.golden @@ -1 +1 @@ -Your one-time passcode is enclosed. \ No newline at end of file +Your One-Time Passcode for Coder. \ No newline at end of file diff --git a/coderd/userauth.go b/coderd/userauth.go index 1da97455c1cfe..15c31d0cc1020 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -43,10 +43,10 @@ import ( ) const ( - userAuthLoggerName = "userauth" - OAuthConvertCookieValue = "coder_oauth_convert_jwt" - mergeStateStringPrefix = "convert-" - oneTimePasscodeDuration = 20 * time.Minute + userAuthLoggerName = "userauth" + OAuthConvertCookieValue = "coder_oauth_convert_jwt" + mergeStateStringPrefix = "convert-" + oneTimePasscodeValidityPeriod = 20 * time.Minute ) type OAuthConvertStateClaims struct { @@ -210,7 +210,7 @@ func (api *API) postConvertLoginType(rw http.ResponseWriter, r *http.Request) { // @Accept json // @Tags Authorization // @Param request body codersdk.RequestOneTimePasscodeRequest true "Request one-time passcode request" -// @Success 200 +// @Success 204 // @Router /users/request-one-time-passcode [post] func (api *API) postRequestOneTimePasscode(rw http.ResponseWriter, r *http.Request) { var ( @@ -241,7 +241,7 @@ func (api *API) postRequestOneTimePasscode(rw http.ResponseWriter, r *http.Reque defer func() { // We always send the same response. If we give a more detailed response // it would open us up to an enumeration attack. - rw.WriteHeader(http.StatusOK) + rw.WriteHeader(http.StatusNoContent) }() //nolint:gocritic // In order to request a one-time passcode, we need to get the user first - and can only do that in the system auth context. @@ -255,7 +255,7 @@ func (api *API) postRequestOneTimePasscode(rw http.ResponseWriter, r *http.Reque aReq.Old = user passcode := uuid.New() - passcodeExpiresAt := dbtime.Now().Add(oneTimePasscodeDuration) + passcodeExpiresAt := dbtime.Now().Add(oneTimePasscodeValidityPeriod) hashedPasscode, err := userpassword.Hash(passcode.String()) if err != nil { @@ -279,13 +279,15 @@ func (api *API) postRequestOneTimePasscode(rw http.ResponseWriter, r *http.Reque auditUser.OneTimePasscodeExpiresAt = sql.NullTime{Time: passcodeExpiresAt, Valid: true} aReq.New = auditUser - if user.ID != uuid.Nil { - // Send the one-time passcode to the user. - err = api.notifyUserRequestedOneTimePasscode(ctx, user, passcode.String()) - if err != nil { - logger.Error(ctx, "unable to notify user about one-time passcode request", slog.Error(err)) + go func() { + if user.ID != uuid.Nil { + // Send the one-time passcode to the user. + err = api.notifyUserRequestedOneTimePasscode(context.Background(), user, passcode.String()) + if err != nil { + logger.Error(ctx, "unable to notify user about one-time passcode request", slog.Error(err)) + } } - } + }() } func (api *API) notifyUserRequestedOneTimePasscode(ctx context.Context, user database.User, passcode string) error { @@ -312,7 +314,7 @@ func (api *API) notifyUserRequestedOneTimePasscode(ctx context.Context, user dat // @Accept json // @Tags Authorization // @Param request body codersdk.ChangePasswordWithOneTimePasscodeRequest true "Change password request" -// @Success 200 +// @Success 204 // @Router /users/change-password-with-one-time-passcode [post] func (api *API) postChangePasswordWithOneTimePasscode(rw http.ResponseWriter, r *http.Request) { var ( @@ -354,14 +356,14 @@ func (api *API) postChangePasswordWithOneTimePasscode(rw http.ResponseWriter, r equal, err := userpassword.Compare(string(user.HashedOneTimePasscode), req.OneTimePasscode) if err != nil { - logger.Error(ctx, "unable to compare one time passcode", slog.Error(err)) - return xerrors.Errorf("compare one time passcode: %w", err) + logger.Error(ctx, "unable to compare one-time passcode", slog.Error(err)) + return xerrors.Errorf("compare one-time passcode: %w", err) } now := dbtime.Now() if !equal || now.After(user.OneTimePasscodeExpiresAt.Time) { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: "Incorrect email or one-time-passcode.", + Message: "Incorrect email or one-time passcode.", }) return nil } @@ -415,7 +417,7 @@ func (api *API) postChangePasswordWithOneTimePasscode(rw http.ResponseWriter, r auditUser.HashedOneTimePasscode = nil aReq.New = auditUser - rw.WriteHeader(http.StatusOK) + rw.WriteHeader(http.StatusNoContent) return nil }, nil) diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index 977c5d545a2f8..d6043d8ceefd6 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -1666,10 +1666,10 @@ func TestUserForgotPassword(t *testing.T) { } t.Run("CanChangePassword", func(t *testing.T) { - const newPassword = "SomeNewSecurePassword!" - t.Parallel() + const newPassword = "SomeNewSecurePassword!" + notifyEnq := &testutil.FakeNotificationsEnqueuer{} client := coderdtest.New(t, &coderdtest.Options{ @@ -1688,7 +1688,10 @@ func TestUserForgotPassword(t *testing.T) { Email: anotherUser.Email, Password: newPassword, }) - require.Error(t, err) + var apiErr *codersdk.Error + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusUnauthorized, apiErr.StatusCode()) + require.Contains(t, apiErr.Message, "Incorrect email or password.") err = anotherClient.RequestOneTimePasscode(ctx, codersdk.RequestOneTimePasscodeRequest{ Email: anotherUser.Email, @@ -1722,7 +1725,9 @@ func TestUserForgotPassword(t *testing.T) { OneTimePasscode: oneTimePasscode, Password: "SomeDifferentSecurePassword!", }) - require.Error(t, err) + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusBadRequest, apiErr.StatusCode()) + require.Contains(t, apiErr.Message, "Incorrect email or one-time passcode.") }) t.Run("CannotChangePasswordWithInvalidOneTimePasscode", func(t *testing.T) { @@ -1755,11 +1760,10 @@ func TestUserForgotPassword(t *testing.T) { OneTimePasscode: uuid.New().String(), // Use a different UUID to the one expected Password: "SomeNewSecurePassword!", }) - require.Error(t, err) - var apiErr *codersdk.Error require.ErrorAs(t, err, &apiErr) require.Equal(t, http.StatusBadRequest, apiErr.StatusCode()) + require.Contains(t, apiErr.Message, "Incorrect email or one-time passcode") }) t.Run("CannotChangePasswordWithNoOneTimePasscode", func(t *testing.T) { @@ -1792,11 +1796,12 @@ func TestUserForgotPassword(t *testing.T) { OneTimePasscode: "", Password: "SomeNewSecurePassword!", }) - require.Error(t, err) - var apiErr *codersdk.Error require.ErrorAs(t, err, &apiErr) require.Equal(t, http.StatusBadRequest, apiErr.StatusCode()) + require.Contains(t, apiErr.Message, "Validation failed.") + require.Equal(t, 1, len(apiErr.Validations)) + require.Equal(t, "one_time_passcode", apiErr.Validations[0].Field) }) t.Run("CannotChangePasswordWithWeakPassword", func(t *testing.T) { @@ -1831,11 +1836,12 @@ func TestUserForgotPassword(t *testing.T) { OneTimePasscode: oneTimePasscode, Password: "notstrong", }) - require.Error(t, err) - var apiErr *codersdk.Error require.ErrorAs(t, err, &apiErr) require.Equal(t, http.StatusBadRequest, apiErr.StatusCode()) + require.Contains(t, apiErr.Message, "Invalid password.") + require.Equal(t, 1, len(apiErr.Validations)) + require.Equal(t, "password", apiErr.Validations[0].Field) }) t.Run("GivenOKResponseWithInvalidEmail", func(t *testing.T) { diff --git a/codersdk/users.go b/codersdk/users.go index 7d23969940e34..5f6394e7eb894 100644 --- a/codersdk/users.go +++ b/codersdk/users.go @@ -569,7 +569,7 @@ func (c *Client) RequestOneTimePasscode(ctx context.Context, req RequestOneTimeP } defer res.Body.Close() - if res.StatusCode != http.StatusOK { + if res.StatusCode != http.StatusNoContent { return ReadBodyAsError(res) } @@ -583,7 +583,7 @@ func (c *Client) ChangePasswordWithOneTimePasscode(ctx context.Context, req Chan } defer res.Body.Close() - if res.StatusCode != http.StatusOK { + if res.StatusCode != http.StatusNoContent { return ReadBodyAsError(res) } diff --git a/docs/reference/api/authorization.md b/docs/reference/api/authorization.md index cd02184b85419..a2284eb70d1a8 100644 --- a/docs/reference/api/authorization.md +++ b/docs/reference/api/authorization.md @@ -98,9 +98,9 @@ curl -X POST http://coder-server:8080/api/v2/users/change-password-with-one-time ### Responses -| Status | Meaning | Description | Schema | -| ------ | ------------------------------------------------------- | ----------- | ------ | -| 200 | [OK](https://tools.ietf.org/html/rfc7231#section-6.3.1) | OK | | +| Status | Meaning | Description | Schema | +| ------ | --------------------------------------------------------------- | ----------- | ------ | +| 204 | [No Content](https://tools.ietf.org/html/rfc7231#section-6.3.5) | No Content | | ## Log in user @@ -174,9 +174,9 @@ curl -X POST http://coder-server:8080/api/v2/users/request-one-time-passcode \ ### Responses -| Status | Meaning | Description | Schema | -| ------ | ------------------------------------------------------- | ----------- | ------ | -| 200 | [OK](https://tools.ietf.org/html/rfc7231#section-6.3.1) | OK | | +| Status | Meaning | Description | Schema | +| ------ | --------------------------------------------------------------- | ----------- | ------ | +| 204 | [No Content](https://tools.ietf.org/html/rfc7231#section-6.3.5) | No Content | | ## Convert user from password to oauth authentication From 3a0946cda5f202a27219c41edcd2fecd0f961b87 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Thu, 3 Oct 2024 09:42:15 +0000 Subject: [PATCH 16/24] refactor: change endpoints --- coderd/apidoc/docs.go | 60 +++++++++++------------ coderd/apidoc/swagger.json | 52 ++++++++++---------- coderd/coderd.go | 4 +- coderd/userauth.go | 20 ++++---- codersdk/users.go | 4 +- docs/reference/api/authorization.md | 74 ++++++++++++++--------------- 6 files changed, 106 insertions(+), 108 deletions(-) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index fd8b02afd8e49..d2bc239f9310f 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -5094,34 +5094,6 @@ const docTemplate = `{ } } }, - "/users/change-password-with-one-time-passcode": { - "post": { - "consumes": [ - "application/json" - ], - "tags": [ - "Authorization" - ], - "summary": "Change password with a one-time passcode", - "operationId": "change-password-with-a-one-time-passcode", - "parameters": [ - { - "description": "Change password request", - "name": "request", - "in": "body", - "required": true, - "schema": { - "$ref": "#/definitions/codersdk.ChangePasswordWithOneTimePasscodeRequest" - } - } - ], - "responses": { - "204": { - "description": "No Content" - } - } - } - }, "/users/first": { "get": { "security": [ @@ -5281,7 +5253,35 @@ const docTemplate = `{ } } }, - "/users/request-one-time-passcode": { + "/users/otp/change-password": { + "post": { + "consumes": [ + "application/json" + ], + "tags": [ + "Authorization" + ], + "summary": "Change password with a one-time passcode", + "operationId": "change-password-with-a-one-time-passcode", + "parameters": [ + { + "description": "Change password request", + "name": "request", + "in": "body", + "required": true, + "schema": { + "$ref": "#/definitions/codersdk.ChangePasswordWithOneTimePasscodeRequest" + } + } + ], + "responses": { + "204": { + "description": "No Content" + } + } + } + }, + "/users/otp/request": { "post": { "consumes": [ "application/json" @@ -5293,7 +5293,7 @@ const docTemplate = `{ "operationId": "request-one-time-passcode", "parameters": [ { - "description": "Request one-time passcode request", + "description": "One-time passcode request", "name": "request", "in": "body", "required": true, diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 52f6e3e2038f2..260be731b53c2 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -4500,30 +4500,6 @@ } } }, - "/users/change-password-with-one-time-passcode": { - "post": { - "consumes": ["application/json"], - "tags": ["Authorization"], - "summary": "Change password with a one-time passcode", - "operationId": "change-password-with-a-one-time-passcode", - "parameters": [ - { - "description": "Change password request", - "name": "request", - "in": "body", - "required": true, - "schema": { - "$ref": "#/definitions/codersdk.ChangePasswordWithOneTimePasscodeRequest" - } - } - ], - "responses": { - "204": { - "description": "No Content" - } - } - } - }, "/users/first": { "get": { "security": [ @@ -4659,7 +4635,31 @@ } } }, - "/users/request-one-time-passcode": { + "/users/otp/change-password": { + "post": { + "consumes": ["application/json"], + "tags": ["Authorization"], + "summary": "Change password with a one-time passcode", + "operationId": "change-password-with-a-one-time-passcode", + "parameters": [ + { + "description": "Change password request", + "name": "request", + "in": "body", + "required": true, + "schema": { + "$ref": "#/definitions/codersdk.ChangePasswordWithOneTimePasscodeRequest" + } + } + ], + "responses": { + "204": { + "description": "No Content" + } + } + } + }, + "/users/otp/request": { "post": { "consumes": ["application/json"], "tags": ["Authorization"], @@ -4667,7 +4667,7 @@ "operationId": "request-one-time-passcode", "parameters": [ { - "description": "Request one-time passcode request", + "description": "One-time passcode request", "name": "request", "in": "body", "required": true, diff --git a/coderd/coderd.go b/coderd/coderd.go index ed68ec0f6675e..9e714d47ecebd 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -984,8 +984,8 @@ func New(options *Options) *API { // This value is intentionally increased during tests. r.Use(httpmw.RateLimit(options.LoginRateLimit, time.Minute)) r.Post("/login", api.postLogin) - r.Post("/request-one-time-passcode", api.postRequestOneTimePasscode) - r.Post("/change-password-with-one-time-passcode", api.postChangePasswordWithOneTimePasscode) + r.Post("/otp/request", api.postRequestOneTimePasscode) + r.Post("/otp/change-password", api.postChangePasswordWithOneTimePasscode) r.Route("/oauth2", func(r chi.Router) { r.Route("/github", func(r chi.Router) { r.Use( diff --git a/coderd/userauth.go b/coderd/userauth.go index 15c31d0cc1020..963d9d5362ae9 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -209,9 +209,9 @@ func (api *API) postConvertLoginType(rw http.ResponseWriter, r *http.Request) { // @ID request-one-time-passcode // @Accept json // @Tags Authorization -// @Param request body codersdk.RequestOneTimePasscodeRequest true "Request one-time passcode request" +// @Param request body codersdk.RequestOneTimePasscodeRequest true "One-time passcode request" // @Success 204 -// @Router /users/request-one-time-passcode [post] +// @Router /users/otp/request [post] func (api *API) postRequestOneTimePasscode(rw http.ResponseWriter, r *http.Request) { var ( ctx = r.Context() @@ -279,15 +279,13 @@ func (api *API) postRequestOneTimePasscode(rw http.ResponseWriter, r *http.Reque auditUser.OneTimePasscodeExpiresAt = sql.NullTime{Time: passcodeExpiresAt, Valid: true} aReq.New = auditUser - go func() { - if user.ID != uuid.Nil { - // Send the one-time passcode to the user. - err = api.notifyUserRequestedOneTimePasscode(context.Background(), user, passcode.String()) - if err != nil { - logger.Error(ctx, "unable to notify user about one-time passcode request", slog.Error(err)) - } + if user.ID != uuid.Nil { + // Send the one-time passcode to the user. + err = api.notifyUserRequestedOneTimePasscode(ctx, user, passcode.String()) + if err != nil { + logger.Error(ctx, "unable to notify user about one-time passcode request", slog.Error(err)) } - }() + } } func (api *API) notifyUserRequestedOneTimePasscode(ctx context.Context, user database.User, passcode string) error { @@ -315,7 +313,7 @@ func (api *API) notifyUserRequestedOneTimePasscode(ctx context.Context, user dat // @Tags Authorization // @Param request body codersdk.ChangePasswordWithOneTimePasscodeRequest true "Change password request" // @Success 204 -// @Router /users/change-password-with-one-time-passcode [post] +// @Router /users/otp/change-password [post] func (api *API) postChangePasswordWithOneTimePasscode(rw http.ResponseWriter, r *http.Request) { var ( err error diff --git a/codersdk/users.go b/codersdk/users.go index 5f6394e7eb894..f57b8010f9229 100644 --- a/codersdk/users.go +++ b/codersdk/users.go @@ -563,7 +563,7 @@ func (c *Client) LoginWithPassword(ctx context.Context, req LoginWithPasswordReq } func (c *Client) RequestOneTimePasscode(ctx context.Context, req RequestOneTimePasscodeRequest) error { - res, err := c.Request(ctx, http.MethodPost, "/api/v2/users/request-one-time-passcode", req) + res, err := c.Request(ctx, http.MethodPost, "/api/v2/users/otp/request", req) if err != nil { return err } @@ -577,7 +577,7 @@ func (c *Client) RequestOneTimePasscode(ctx context.Context, req RequestOneTimeP } func (c *Client) ChangePasswordWithOneTimePasscode(ctx context.Context, req ChangePasswordWithOneTimePasscodeRequest) error { - res, err := c.Request(ctx, http.MethodPost, "/api/v2/users/change-password-with-one-time-passcode", req) + res, err := c.Request(ctx, http.MethodPost, "/api/v2/users/otp/change-password", req) if err != nil { return err } diff --git a/docs/reference/api/authorization.md b/docs/reference/api/authorization.md index a2284eb70d1a8..86cee5d0fd727 100644 --- a/docs/reference/api/authorization.md +++ b/docs/reference/api/authorization.md @@ -68,83 +68,83 @@ curl -X POST http://coder-server:8080/api/v2/authcheck \ To perform this operation, you must be authenticated. [Learn more](authentication.md). -## Change password with a one-time passcode +## Log in user ### Code samples ```shell # Example request using curl -curl -X POST http://coder-server:8080/api/v2/users/change-password-with-one-time-passcode \ - -H 'Content-Type: application/json' +curl -X POST http://coder-server:8080/api/v2/users/login \ + -H 'Content-Type: application/json' \ + -H 'Accept: application/json' ``` -`POST /users/change-password-with-one-time-passcode` +`POST /users/login` > Body parameter ```json { "email": "user@example.com", - "one_time_passcode": "string", "password": "string" } ``` ### Parameters -| Name | In | Type | Required | Description | -| ------ | ---- | ---------------------------------------------------------------------------------------------------------------- | -------- | ----------------------- | -| `body` | body | [codersdk.ChangePasswordWithOneTimePasscodeRequest](schemas.md#codersdkchangepasswordwithonetimepasscoderequest) | true | Change password request | +| Name | In | Type | Required | Description | +| ------ | ---- | -------------------------------------------------------------------------------- | -------- | ------------- | +| `body` | body | [codersdk.LoginWithPasswordRequest](schemas.md#codersdkloginwithpasswordrequest) | true | Login request | + +### Example responses + +> 201 Response + +```json +{ + "session_token": "string" +} +``` ### Responses -| Status | Meaning | Description | Schema | -| ------ | --------------------------------------------------------------- | ----------- | ------ | -| 204 | [No Content](https://tools.ietf.org/html/rfc7231#section-6.3.5) | No Content | | +| Status | Meaning | Description | Schema | +| ------ | ------------------------------------------------------------ | ----------- | ---------------------------------------------------------------------------------- | +| 201 | [Created](https://tools.ietf.org/html/rfc7231#section-6.3.2) | Created | [codersdk.LoginWithPasswordResponse](schemas.md#codersdkloginwithpasswordresponse) | -## Log in user +## Change password with a one-time passcode ### Code samples ```shell # Example request using curl -curl -X POST http://coder-server:8080/api/v2/users/login \ - -H 'Content-Type: application/json' \ - -H 'Accept: application/json' +curl -X POST http://coder-server:8080/api/v2/users/otp/change-password \ + -H 'Content-Type: application/json' ``` -`POST /users/login` +`POST /users/otp/change-password` > Body parameter ```json { "email": "user@example.com", + "one_time_passcode": "string", "password": "string" } ``` ### Parameters -| Name | In | Type | Required | Description | -| ------ | ---- | -------------------------------------------------------------------------------- | -------- | ------------- | -| `body` | body | [codersdk.LoginWithPasswordRequest](schemas.md#codersdkloginwithpasswordrequest) | true | Login request | - -### Example responses - -> 201 Response - -```json -{ - "session_token": "string" -} -``` +| Name | In | Type | Required | Description | +| ------ | ---- | ---------------------------------------------------------------------------------------------------------------- | -------- | ----------------------- | +| `body` | body | [codersdk.ChangePasswordWithOneTimePasscodeRequest](schemas.md#codersdkchangepasswordwithonetimepasscoderequest) | true | Change password request | ### Responses -| Status | Meaning | Description | Schema | -| ------ | ------------------------------------------------------------ | ----------- | ---------------------------------------------------------------------------------- | -| 201 | [Created](https://tools.ietf.org/html/rfc7231#section-6.3.2) | Created | [codersdk.LoginWithPasswordResponse](schemas.md#codersdkloginwithpasswordresponse) | +| Status | Meaning | Description | Schema | +| ------ | --------------------------------------------------------------- | ----------- | ------ | +| 204 | [No Content](https://tools.ietf.org/html/rfc7231#section-6.3.5) | No Content | | ## Request one-time passcode @@ -152,11 +152,11 @@ curl -X POST http://coder-server:8080/api/v2/users/login \ ```shell # Example request using curl -curl -X POST http://coder-server:8080/api/v2/users/request-one-time-passcode \ +curl -X POST http://coder-server:8080/api/v2/users/otp/request \ -H 'Content-Type: application/json' ``` -`POST /users/request-one-time-passcode` +`POST /users/otp/request` > Body parameter @@ -168,9 +168,9 @@ curl -X POST http://coder-server:8080/api/v2/users/request-one-time-passcode \ ### Parameters -| Name | In | Type | Required | Description | -| ------ | ---- | ------------------------------------------------------------------------------------------ | -------- | --------------------------------- | -| `body` | body | [codersdk.RequestOneTimePasscodeRequest](schemas.md#codersdkrequestonetimepasscoderequest) | true | Request one-time passcode request | +| Name | In | Type | Required | Description | +| ------ | ---- | ------------------------------------------------------------------------------------------ | -------- | ------------------------- | +| `body` | body | [codersdk.RequestOneTimePasscodeRequest](schemas.md#codersdkrequestonetimepasscoderequest) | true | One-time passcode request | ### Responses From 1a7ad7a72b05427a8f13dcf1c6cef28c71256029 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Thu, 3 Oct 2024 09:57:41 +0000 Subject: [PATCH 17/24] fix: update assertSecurityDefined links --- coderd/coderdtest/swaggerparser.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/coderdtest/swaggerparser.go b/coderd/coderdtest/swaggerparser.go index 24442fde68b79..c0cbe54236124 100644 --- a/coderd/coderdtest/swaggerparser.go +++ b/coderd/coderdtest/swaggerparser.go @@ -304,8 +304,8 @@ func assertSecurityDefined(t *testing.T, comment SwaggerComment) { comment.router == "/buildinfo" || comment.router == "/" || comment.router == "/users/login" || - comment.router == "/users/request-one-time-passcode" || - comment.router == "/users/change-password-with-one-time-passcode" { + comment.router == "/users/otp/request" || + comment.router == "/users/otp/change-password" { return // endpoints do not require authorization } assert.Equal(t, "CoderSessionToken", comment.security, "@Security must be equal CoderSessionToken") From 6bbcff2625d67202c82f07374329392c3465e480 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Thu, 3 Oct 2024 11:10:33 +0000 Subject: [PATCH 18/24] refactor: make validity period an option and test it --- coderd/coderd.go | 6 ++++ coderd/coderdtest/coderdtest.go | 8 +++++ coderd/userauth.go | 9 +++--- coderd/userauth_test.go | 55 +++++++++++++++++++++++++++++++++ 4 files changed, 73 insertions(+), 5 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 9e714d47ecebd..dc40554fbabae 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -248,6 +248,9 @@ type Options struct { // IDPSync holds all configured values for syncing external IDP users into Coder. IDPSync idpsync.IDPSync + + // OneTimePasscodeValidityPeriod specifies how long a one time passcode should be valid for. + OneTimePasscodeValidityPeriod time.Duration } // @title Coder API @@ -387,6 +390,9 @@ func New(options *Options) *API { v := schedule.NewAGPLUserQuietHoursScheduleStore() options.UserQuietHoursScheduleStore.Store(&v) } + if options.OneTimePasscodeValidityPeriod == 0 { + options.OneTimePasscodeValidityPeriod = 20 * time.Minute + } if options.StatsBatcher == nil { panic("developer error: options.StatsBatcher is nil") diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index 21104167ad8dd..372d8931207db 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -128,6 +128,9 @@ type Options struct { LoginRateLimit int FilesRateLimit int + // OneTimePasscodeValidityPeriod specifies how long a one time passcode should be valid for. + OneTimePasscodeValidityPeriod time.Duration + // IncludeProvisionerDaemon when true means to start an in-memory provisionerD IncludeProvisionerDaemon bool ProvisionerDaemonTags map[string]string @@ -311,6 +314,10 @@ func NewOptions(t testing.TB, options *Options) (func(http.Handler), context.Can options.NotificationsEnqueuer = &testutil.FakeNotificationsEnqueuer{} } + if options.OneTimePasscodeValidityPeriod == 0 { + options.OneTimePasscodeValidityPeriod = 20 * time.Second + } + var templateScheduleStore atomic.Pointer[schedule.TemplateScheduleStore] if options.TemplateScheduleStore == nil { options.TemplateScheduleStore = schedule.NewAGPLTemplateScheduleStore() @@ -530,6 +537,7 @@ func NewOptions(t testing.TB, options *Options) (func(http.Handler), context.Can DatabaseRolluper: options.DatabaseRolluper, WorkspaceUsageTracker: wuTracker, NotificationsEnqueuer: options.NotificationsEnqueuer, + OneTimePasscodeValidityPeriod: options.OneTimePasscodeValidityPeriod, } } diff --git a/coderd/userauth.go b/coderd/userauth.go index 963d9d5362ae9..8e0608054ba0d 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -43,10 +43,9 @@ import ( ) const ( - userAuthLoggerName = "userauth" - OAuthConvertCookieValue = "coder_oauth_convert_jwt" - mergeStateStringPrefix = "convert-" - oneTimePasscodeValidityPeriod = 20 * time.Minute + userAuthLoggerName = "userauth" + OAuthConvertCookieValue = "coder_oauth_convert_jwt" + mergeStateStringPrefix = "convert-" ) type OAuthConvertStateClaims struct { @@ -255,7 +254,7 @@ func (api *API) postRequestOneTimePasscode(rw http.ResponseWriter, r *http.Reque aReq.Old = user passcode := uuid.New() - passcodeExpiresAt := dbtime.Now().Add(oneTimePasscodeValidityPeriod) + passcodeExpiresAt := dbtime.Now().Add(api.OneTimePasscodeValidityPeriod) hashedPasscode, err := userpassword.Hash(passcode.String()) if err != nil { diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index d6043d8ceefd6..601c897815b14 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -1730,6 +1730,61 @@ func TestUserForgotPassword(t *testing.T) { require.Contains(t, apiErr.Message, "Incorrect email or one-time passcode.") }) + t.Run("OneTimePasscodeExpires", func(t *testing.T) { + t.Parallel() + + const newPassword = "SomeNewSecurePassword!" + const oneTimePasscodeValidityPeriod = 2 * time.Second + + notifyEnq := &testutil.FakeNotificationsEnqueuer{} + + client := coderdtest.New(t, &coderdtest.Options{ + NotificationsEnqueuer: notifyEnq, + OneTimePasscodeValidityPeriod: oneTimePasscodeValidityPeriod, + }) + user := coderdtest.CreateFirstUser(t, client) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + anotherClient, anotherUser := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) + + err := anotherClient.RequestOneTimePasscode(ctx, codersdk.RequestOneTimePasscodeRequest{ + Email: anotherUser.Email, + }) + require.NoError(t, err) + + require.Equal(t, 2, len(notifyEnq.Sent)) + + notif := notifyEnq.Sent[1] + verifyOneTimePasscodeNotification(t, notif, anotherUser.ID) + + oneTimePasscode := notif.Labels["one_time_passcode"] + + // Wait for long enough so that the token expires + time.Sleep(oneTimePasscodeValidityPeriod + 1*time.Second) + + // Try to change password with an expired one time passcode. + err = anotherClient.ChangePasswordWithOneTimePasscode(ctx, codersdk.ChangePasswordWithOneTimePasscodeRequest{ + Email: anotherUser.Email, + OneTimePasscode: oneTimePasscode, + Password: newPassword, + }) + var apiErr *codersdk.Error + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusBadRequest, apiErr.StatusCode()) + require.Contains(t, apiErr.Message, "Incorrect email or one-time passcode.") + + // Ensure that the password was not changed. + _, err = anotherClient.LoginWithPassword(ctx, codersdk.LoginWithPasswordRequest{ + Email: anotherUser.Email, + Password: newPassword, + }) + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusUnauthorized, apiErr.StatusCode()) + require.Contains(t, apiErr.Message, "Incorrect email or password.") + }) + t.Run("CannotChangePasswordWithInvalidOneTimePasscode", func(t *testing.T) { t.Parallel() From dfc32b1aaeb22946eeebe2ed519d3563d2bd69ff Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Thu, 3 Oct 2024 15:44:45 +0000 Subject: [PATCH 19/24] test: refactor tests to reduce duplication --- coderd/userauth_test.go | 162 ++++++++++++++++++++-------------------- 1 file changed, 79 insertions(+), 83 deletions(-) diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index 601c897815b14..77a15ddc3c313 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -1658,13 +1658,52 @@ func TestOIDCSkipIssuer(t *testing.T) { func TestUserForgotPassword(t *testing.T) { t.Parallel() - verifyOneTimePasscodeNotification := func(t *testing.T, notif *testutil.Notification, userID uuid.UUID) { + requireOneTimePasscodeNotification := func(t *testing.T, notif *testutil.Notification, userID uuid.UUID) { require.Equal(t, notifications.TemplateUserRequestedOneTimePasscode, notif.TemplateID) require.Equal(t, userID, notif.UserID) require.Equal(t, 1, len(notif.Targets)) require.Equal(t, userID, notif.Targets[0]) } + requireCanLogin := func(t *testing.T, ctx context.Context, client *codersdk.Client, email string, password string) { + _, err := client.LoginWithPassword(ctx, codersdk.LoginWithPasswordRequest{ + Email: email, + Password: password, + }) + require.NoError(t, err) + } + + requireCannotLogin := func(t *testing.T, ctx context.Context, client *codersdk.Client, email string, password string) { + _, err := client.LoginWithPassword(ctx, codersdk.LoginWithPasswordRequest{ + Email: email, + Password: password, + }) + var apiErr *codersdk.Error + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusUnauthorized, apiErr.StatusCode()) + require.Contains(t, apiErr.Message, "Incorrect email or password.") + } + + requireRequestOneTimePasscode := func(t *testing.T, ctx context.Context, client *codersdk.Client, notifyEnq *testutil.FakeNotificationsEnqueuer, email string, userID uuid.UUID) string { + err := client.RequestOneTimePasscode(ctx, codersdk.RequestOneTimePasscodeRequest{Email: email}) + require.NoError(t, err) + + require.Equal(t, 2, len(notifyEnq.Sent)) + + notif := notifyEnq.Sent[1] + requireOneTimePasscodeNotification(t, notifyEnq.Sent[1], userID) + return notif.Labels["one_time_passcode"] + } + + requireChangePasswordWithOneTimePasscode := func(t *testing.T, ctx context.Context, client *codersdk.Client, email string, passcode string, password string) { + err := client.ChangePasswordWithOneTimePasscode(ctx, codersdk.ChangePasswordWithOneTimePasscodeRequest{ + Email: email, + OneTimePasscode: passcode, + Password: password, + }) + require.NoError(t, err) + } + t.Run("CanChangePassword", func(t *testing.T) { t.Parallel() @@ -1684,47 +1723,20 @@ func TestUserForgotPassword(t *testing.T) { // First try to login before changing our password. We expected this to error // as we haven't change the password yet. - _, err := anotherClient.LoginWithPassword(ctx, codersdk.LoginWithPasswordRequest{ - Email: anotherUser.Email, - Password: newPassword, - }) - var apiErr *codersdk.Error - require.ErrorAs(t, err, &apiErr) - require.Equal(t, http.StatusUnauthorized, apiErr.StatusCode()) - require.Contains(t, apiErr.Message, "Incorrect email or password.") + requireCannotLogin(t, ctx, anotherClient, anotherUser.Email, newPassword) - err = anotherClient.RequestOneTimePasscode(ctx, codersdk.RequestOneTimePasscodeRequest{ - Email: anotherUser.Email, - }) - require.NoError(t, err) + oneTimePasscode := requireRequestOneTimePasscode(t, ctx, anotherClient, notifyEnq, anotherUser.Email, anotherUser.ID) - require.Equal(t, 2, len(notifyEnq.Sent)) - - notif := notifyEnq.Sent[1] - verifyOneTimePasscodeNotification(t, notif, anotherUser.ID) - - oneTimePasscode := notif.Labels["one_time_passcode"] - - err = anotherClient.ChangePasswordWithOneTimePasscode(ctx, codersdk.ChangePasswordWithOneTimePasscodeRequest{ - Email: anotherUser.Email, - OneTimePasscode: oneTimePasscode, - Password: newPassword, - }) - require.NoError(t, err) - - // Now that we have changed the password, this should work. - _, err = anotherClient.LoginWithPassword(ctx, codersdk.LoginWithPasswordRequest{ - Email: anotherUser.Email, - Password: newPassword, - }) - require.NoError(t, err) + requireChangePasswordWithOneTimePasscode(t, ctx, anotherClient, anotherUser.Email, oneTimePasscode, newPassword) + requireCanLogin(t, ctx, anotherClient, anotherUser.Email, newPassword) // We now need to check that the one-time passcode isn't valid. - err = anotherClient.ChangePasswordWithOneTimePasscode(ctx, codersdk.ChangePasswordWithOneTimePasscodeRequest{ + err := anotherClient.ChangePasswordWithOneTimePasscode(ctx, codersdk.ChangePasswordWithOneTimePasscodeRequest{ Email: anotherUser.Email, OneTimePasscode: oneTimePasscode, Password: "SomeDifferentSecurePassword!", }) + var apiErr *codersdk.Error require.ErrorAs(t, err, &apiErr) require.Equal(t, http.StatusBadRequest, apiErr.StatusCode()) require.Contains(t, apiErr.Message, "Incorrect email or one-time passcode.") @@ -1749,23 +1761,13 @@ func TestUserForgotPassword(t *testing.T) { anotherClient, anotherUser := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) - err := anotherClient.RequestOneTimePasscode(ctx, codersdk.RequestOneTimePasscodeRequest{ - Email: anotherUser.Email, - }) - require.NoError(t, err) - - require.Equal(t, 2, len(notifyEnq.Sent)) - - notif := notifyEnq.Sent[1] - verifyOneTimePasscodeNotification(t, notif, anotherUser.ID) - - oneTimePasscode := notif.Labels["one_time_passcode"] + oneTimePasscode := requireRequestOneTimePasscode(t, ctx, anotherClient, notifyEnq, anotherUser.Email, anotherUser.ID) // Wait for long enough so that the token expires time.Sleep(oneTimePasscodeValidityPeriod + 1*time.Second) // Try to change password with an expired one time passcode. - err = anotherClient.ChangePasswordWithOneTimePasscode(ctx, codersdk.ChangePasswordWithOneTimePasscodeRequest{ + err := anotherClient.ChangePasswordWithOneTimePasscode(ctx, codersdk.ChangePasswordWithOneTimePasscodeRequest{ Email: anotherUser.Email, OneTimePasscode: oneTimePasscode, Password: newPassword, @@ -1776,16 +1778,10 @@ func TestUserForgotPassword(t *testing.T) { require.Contains(t, apiErr.Message, "Incorrect email or one-time passcode.") // Ensure that the password was not changed. - _, err = anotherClient.LoginWithPassword(ctx, codersdk.LoginWithPasswordRequest{ - Email: anotherUser.Email, - Password: newPassword, - }) - require.ErrorAs(t, err, &apiErr) - require.Equal(t, http.StatusUnauthorized, apiErr.StatusCode()) - require.Contains(t, apiErr.Message, "Incorrect email or password.") + requireCannotLogin(t, ctx, anotherClient, anotherUser.Email, newPassword) }) - t.Run("CannotChangePasswordWithInvalidOneTimePasscode", func(t *testing.T) { + t.Run("CannotChangePasswordWithoutRequestingOneTimePasscode", func(t *testing.T) { t.Parallel() notifyEnq := &testutil.FakeNotificationsEnqueuer{} @@ -1800,17 +1796,35 @@ func TestUserForgotPassword(t *testing.T) { anotherClient, anotherUser := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) - err := anotherClient.RequestOneTimePasscode(ctx, codersdk.RequestOneTimePasscodeRequest{ - Email: anotherUser.Email, + err := anotherClient.ChangePasswordWithOneTimePasscode(ctx, codersdk.ChangePasswordWithOneTimePasscodeRequest{ + Email: anotherUser.Email, + OneTimePasscode: uuid.New().String(), + Password: "SomeNewSecurePassword!", }) - require.NoError(t, err) + var apiErr *codersdk.Error + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusBadRequest, apiErr.StatusCode()) + require.Contains(t, apiErr.Message, "Incorrect email or one-time passcode") + }) - require.Equal(t, 2, len(notifyEnq.Sent)) + t.Run("CannotChangePasswordWithInvalidOneTimePasscode", func(t *testing.T) { + t.Parallel() - notif := notifyEnq.Sent[1] - verifyOneTimePasscodeNotification(t, notif, anotherUser.ID) + notifyEnq := &testutil.FakeNotificationsEnqueuer{} - err = anotherClient.ChangePasswordWithOneTimePasscode(ctx, codersdk.ChangePasswordWithOneTimePasscodeRequest{ + client := coderdtest.New(t, &coderdtest.Options{ + NotificationsEnqueuer: notifyEnq, + }) + user := coderdtest.CreateFirstUser(t, client) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + anotherClient, anotherUser := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) + + _ = requireRequestOneTimePasscode(t, ctx, anotherClient, notifyEnq, anotherUser.Email, anotherUser.ID) + + err := anotherClient.ChangePasswordWithOneTimePasscode(ctx, codersdk.ChangePasswordWithOneTimePasscodeRequest{ Email: anotherUser.Email, OneTimePasscode: uuid.New().String(), // Use a different UUID to the one expected Password: "SomeNewSecurePassword!", @@ -1836,17 +1850,9 @@ func TestUserForgotPassword(t *testing.T) { anotherClient, anotherUser := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) - err := anotherClient.RequestOneTimePasscode(ctx, codersdk.RequestOneTimePasscodeRequest{ - Email: anotherUser.Email, - }) - require.NoError(t, err) - - require.Equal(t, 2, len(notifyEnq.Sent)) - - notif := notifyEnq.Sent[1] - verifyOneTimePasscodeNotification(t, notif, anotherUser.ID) + _ = requireRequestOneTimePasscode(t, ctx, anotherClient, notifyEnq, anotherUser.Email, anotherUser.ID) - err = anotherClient.ChangePasswordWithOneTimePasscode(ctx, codersdk.ChangePasswordWithOneTimePasscodeRequest{ + err := anotherClient.ChangePasswordWithOneTimePasscode(ctx, codersdk.ChangePasswordWithOneTimePasscodeRequest{ Email: anotherUser.Email, OneTimePasscode: "", Password: "SomeNewSecurePassword!", @@ -1874,19 +1880,9 @@ func TestUserForgotPassword(t *testing.T) { anotherClient, anotherUser := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) - err := anotherClient.RequestOneTimePasscode(ctx, codersdk.RequestOneTimePasscodeRequest{ - Email: anotherUser.Email, - }) - require.NoError(t, err) - - require.Equal(t, 2, len(notifyEnq.Sent)) - - notif := notifyEnq.Sent[1] - verifyOneTimePasscodeNotification(t, notif, anotherUser.ID) - - oneTimePasscode := notif.Labels["one_time_passcode"] + oneTimePasscode := requireRequestOneTimePasscode(t, ctx, anotherClient, notifyEnq, anotherUser.Email, anotherUser.ID) - err = anotherClient.ChangePasswordWithOneTimePasscode(ctx, codersdk.ChangePasswordWithOneTimePasscodeRequest{ + err := anotherClient.ChangePasswordWithOneTimePasscode(ctx, codersdk.ChangePasswordWithOneTimePasscodeRequest{ Email: anotherUser.Email, OneTimePasscode: oneTimePasscode, Password: "notstrong", From 9f80082809bf6342a66595a1413720f6ad6ee1ba Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Fri, 4 Oct 2024 07:51:16 +0000 Subject: [PATCH 20/24] test: ensure cannot login after failed password change --- coderd/userauth_test.go | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index 77a15ddc3c313..15f60937aaa57 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -1658,6 +1658,8 @@ func TestOIDCSkipIssuer(t *testing.T) { func TestUserForgotPassword(t *testing.T) { t.Parallel() + const newPassword = "SomeNewSecurePassword!" + requireOneTimePasscodeNotification := func(t *testing.T, notif *testutil.Notification, userID uuid.UUID) { require.Equal(t, notifications.TemplateUserRequestedOneTimePasscode, notif.TemplateID) require.Equal(t, userID, notif.UserID) @@ -1707,8 +1709,6 @@ func TestUserForgotPassword(t *testing.T) { t.Run("CanChangePassword", func(t *testing.T) { t.Parallel() - const newPassword = "SomeNewSecurePassword!" - notifyEnq := &testutil.FakeNotificationsEnqueuer{} client := coderdtest.New(t, &coderdtest.Options{ @@ -1734,18 +1734,19 @@ func TestUserForgotPassword(t *testing.T) { err := anotherClient.ChangePasswordWithOneTimePasscode(ctx, codersdk.ChangePasswordWithOneTimePasscodeRequest{ Email: anotherUser.Email, OneTimePasscode: oneTimePasscode, - Password: "SomeDifferentSecurePassword!", + Password: newPassword + "!", }) var apiErr *codersdk.Error require.ErrorAs(t, err, &apiErr) require.Equal(t, http.StatusBadRequest, apiErr.StatusCode()) require.Contains(t, apiErr.Message, "Incorrect email or one-time passcode.") + + requireCannotLogin(t, ctx, anotherClient, anotherUser.Email, newPassword+"!") }) t.Run("OneTimePasscodeExpires", func(t *testing.T) { t.Parallel() - const newPassword = "SomeNewSecurePassword!" const oneTimePasscodeValidityPeriod = 2 * time.Second notifyEnq := &testutil.FakeNotificationsEnqueuer{} @@ -1799,12 +1800,14 @@ func TestUserForgotPassword(t *testing.T) { err := anotherClient.ChangePasswordWithOneTimePasscode(ctx, codersdk.ChangePasswordWithOneTimePasscodeRequest{ Email: anotherUser.Email, OneTimePasscode: uuid.New().String(), - Password: "SomeNewSecurePassword!", + Password: newPassword, }) var apiErr *codersdk.Error require.ErrorAs(t, err, &apiErr) require.Equal(t, http.StatusBadRequest, apiErr.StatusCode()) require.Contains(t, apiErr.Message, "Incorrect email or one-time passcode") + + requireCannotLogin(t, ctx, anotherClient, anotherUser.Email, newPassword) }) t.Run("CannotChangePasswordWithInvalidOneTimePasscode", func(t *testing.T) { @@ -1827,12 +1830,14 @@ func TestUserForgotPassword(t *testing.T) { err := anotherClient.ChangePasswordWithOneTimePasscode(ctx, codersdk.ChangePasswordWithOneTimePasscodeRequest{ Email: anotherUser.Email, OneTimePasscode: uuid.New().String(), // Use a different UUID to the one expected - Password: "SomeNewSecurePassword!", + Password: newPassword, }) var apiErr *codersdk.Error require.ErrorAs(t, err, &apiErr) require.Equal(t, http.StatusBadRequest, apiErr.StatusCode()) require.Contains(t, apiErr.Message, "Incorrect email or one-time passcode") + + requireCannotLogin(t, ctx, anotherClient, anotherUser.Email, newPassword) }) t.Run("CannotChangePasswordWithNoOneTimePasscode", func(t *testing.T) { @@ -1855,7 +1860,7 @@ func TestUserForgotPassword(t *testing.T) { err := anotherClient.ChangePasswordWithOneTimePasscode(ctx, codersdk.ChangePasswordWithOneTimePasscodeRequest{ Email: anotherUser.Email, OneTimePasscode: "", - Password: "SomeNewSecurePassword!", + Password: newPassword, }) var apiErr *codersdk.Error require.ErrorAs(t, err, &apiErr) @@ -1863,6 +1868,8 @@ func TestUserForgotPassword(t *testing.T) { require.Contains(t, apiErr.Message, "Validation failed.") require.Equal(t, 1, len(apiErr.Validations)) require.Equal(t, "one_time_passcode", apiErr.Validations[0].Field) + + requireCannotLogin(t, ctx, anotherClient, anotherUser.Email, newPassword) }) t.Run("CannotChangePasswordWithWeakPassword", func(t *testing.T) { @@ -1893,6 +1900,8 @@ func TestUserForgotPassword(t *testing.T) { require.Contains(t, apiErr.Message, "Invalid password.") require.Equal(t, 1, len(apiErr.Validations)) require.Equal(t, "password", apiErr.Validations[0].Field) + + requireCannotLogin(t, ctx, anotherClient, anotherUser.Email, "notstrong") }) t.Run("GivenOKResponseWithInvalidEmail", func(t *testing.T) { From fe9b3f828a0850403a4c45a0a141dd5383c5146b Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Fri, 4 Oct 2024 09:28:14 +0000 Subject: [PATCH 21/24] fix: imports --- coderd/userauth.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/userauth.go b/coderd/userauth.go index 8e0608054ba0d..6604490fb9e67 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -23,8 +23,6 @@ import ( "golang.org/x/xerrors" "cdr.dev/slog" - "github.com/coder/coder/v2/coderd/idpsync" - "github.com/coder/coder/v2/coderd/notifications" "github.com/coder/coder/v2/coderd/apikey" "github.com/coder/coder/v2/coderd/audit" @@ -34,6 +32,8 @@ import ( "github.com/coder/coder/v2/coderd/externalauth" "github.com/coder/coder/v2/coderd/httpapi" "github.com/coder/coder/v2/coderd/httpmw" + "github.com/coder/coder/v2/coderd/idpsync" + "github.com/coder/coder/v2/coderd/notifications" "github.com/coder/coder/v2/coderd/promoauth" "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/coderd/render" From b42b73cff6729ae75851e3d329723ac3f7089f05 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Fri, 4 Oct 2024 10:12:36 +0000 Subject: [PATCH 22/24] test: ensure can login with old credentials --- coderd/coderdtest/coderdtest.go | 2 +- coderd/userauth_test.go | 54 ++++++++++++++++++++++++++++++--- 2 files changed, 50 insertions(+), 6 deletions(-) diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index 372d8931207db..05c31f35bd20a 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -315,7 +315,7 @@ func NewOptions(t testing.TB, options *Options) (func(http.Handler), context.Can } if options.OneTimePasscodeValidityPeriod == 0 { - options.OneTimePasscodeValidityPeriod = 20 * time.Second + options.OneTimePasscodeValidityPeriod = testutil.WaitLong } var templateScheduleStore atomic.Pointer[schedule.TemplateScheduleStore] diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index 15f60937aaa57..20dfe7f723899 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -1658,6 +1658,7 @@ func TestOIDCSkipIssuer(t *testing.T) { func TestUserForgotPassword(t *testing.T) { t.Parallel() + const oldPassword = "SomeSecurePassword!" const newPassword = "SomeNewSecurePassword!" requireOneTimePasscodeNotification := func(t *testing.T, notif *testutil.Notification, userID uuid.UUID) { @@ -1687,13 +1688,15 @@ func TestUserForgotPassword(t *testing.T) { } requireRequestOneTimePasscode := func(t *testing.T, ctx context.Context, client *codersdk.Client, notifyEnq *testutil.FakeNotificationsEnqueuer, email string, userID uuid.UUID) string { + notifsSent := len(notifyEnq.Sent) + err := client.RequestOneTimePasscode(ctx, codersdk.RequestOneTimePasscodeRequest{Email: email}) require.NoError(t, err) - require.Equal(t, 2, len(notifyEnq.Sent)) + require.Equal(t, notifsSent+1, len(notifyEnq.Sent)) - notif := notifyEnq.Sent[1] - requireOneTimePasscodeNotification(t, notifyEnq.Sent[1], userID) + notif := notifyEnq.Sent[notifsSent] + requireOneTimePasscodeNotification(t, notif, userID) return notif.Labels["one_time_passcode"] } @@ -1742,12 +1745,13 @@ func TestUserForgotPassword(t *testing.T) { require.Contains(t, apiErr.Message, "Incorrect email or one-time passcode.") requireCannotLogin(t, ctx, anotherClient, anotherUser.Email, newPassword+"!") + requireCanLogin(t, ctx, anotherClient, anotherUser.Email, newPassword) }) t.Run("OneTimePasscodeExpires", func(t *testing.T) { t.Parallel() - const oneTimePasscodeValidityPeriod = 2 * time.Second + const oneTimePasscodeValidityPeriod = 1 * time.Millisecond notifyEnq := &testutil.FakeNotificationsEnqueuer{} @@ -1765,7 +1769,7 @@ func TestUserForgotPassword(t *testing.T) { oneTimePasscode := requireRequestOneTimePasscode(t, ctx, anotherClient, notifyEnq, anotherUser.Email, anotherUser.ID) // Wait for long enough so that the token expires - time.Sleep(oneTimePasscodeValidityPeriod + 1*time.Second) + time.Sleep(oneTimePasscodeValidityPeriod + 1*time.Millisecond) // Try to change password with an expired one time passcode. err := anotherClient.ChangePasswordWithOneTimePasscode(ctx, codersdk.ChangePasswordWithOneTimePasscodeRequest{ @@ -1780,6 +1784,7 @@ func TestUserForgotPassword(t *testing.T) { // Ensure that the password was not changed. requireCannotLogin(t, ctx, anotherClient, anotherUser.Email, newPassword) + requireCanLogin(t, ctx, anotherClient, anotherUser.Email, oldPassword) }) t.Run("CannotChangePasswordWithoutRequestingOneTimePasscode", func(t *testing.T) { @@ -1808,6 +1813,7 @@ func TestUserForgotPassword(t *testing.T) { require.Contains(t, apiErr.Message, "Incorrect email or one-time passcode") requireCannotLogin(t, ctx, anotherClient, anotherUser.Email, newPassword) + requireCanLogin(t, ctx, anotherClient, anotherUser.Email, oldPassword) }) t.Run("CannotChangePasswordWithInvalidOneTimePasscode", func(t *testing.T) { @@ -1838,6 +1844,7 @@ func TestUserForgotPassword(t *testing.T) { require.Contains(t, apiErr.Message, "Incorrect email or one-time passcode") requireCannotLogin(t, ctx, anotherClient, anotherUser.Email, newPassword) + requireCanLogin(t, ctx, anotherClient, anotherUser.Email, oldPassword) }) t.Run("CannotChangePasswordWithNoOneTimePasscode", func(t *testing.T) { @@ -1870,6 +1877,7 @@ func TestUserForgotPassword(t *testing.T) { require.Equal(t, "one_time_passcode", apiErr.Validations[0].Field) requireCannotLogin(t, ctx, anotherClient, anotherUser.Email, newPassword) + requireCanLogin(t, ctx, anotherClient, anotherUser.Email, oldPassword) }) t.Run("CannotChangePasswordWithWeakPassword", func(t *testing.T) { @@ -1902,6 +1910,42 @@ func TestUserForgotPassword(t *testing.T) { require.Equal(t, "password", apiErr.Validations[0].Field) requireCannotLogin(t, ctx, anotherClient, anotherUser.Email, "notstrong") + requireCanLogin(t, ctx, anotherClient, anotherUser.Email, oldPassword) + }) + + t.Run("CannotChangePasswordOfAnotherUser", func(t *testing.T) { + t.Parallel() + + notifyEnq := &testutil.FakeNotificationsEnqueuer{} + + client := coderdtest.New(t, &coderdtest.Options{ + NotificationsEnqueuer: notifyEnq, + }) + user := coderdtest.CreateFirstUser(t, client) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + anotherClient, anotherUser := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) + thirdClient, thirdUser := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) + + // Request a One-Time Passcode for `anotherUser` + oneTimePasscode := requireRequestOneTimePasscode(t, ctx, anotherClient, notifyEnq, anotherUser.Email, anotherUser.ID) + + // Ensure we cannot change the password for `thirdUser` with `anotherUser`'s One-Time Passcode. + err := thirdClient.ChangePasswordWithOneTimePasscode(ctx, codersdk.ChangePasswordWithOneTimePasscodeRequest{ + Email: thirdUser.Email, + OneTimePasscode: oneTimePasscode, + Password: newPassword, + }) + var apiErr *codersdk.Error + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusBadRequest, apiErr.StatusCode()) + require.Contains(t, apiErr.Message, "Incorrect email or one-time passcode") + + requireCannotLogin(t, ctx, thirdClient, thirdUser.Email, newPassword) + requireCanLogin(t, ctx, thirdClient, thirdUser.Email, oldPassword) + requireCanLogin(t, ctx, anotherClient, anotherUser.Email, oldPassword) }) t.Run("GivenOKResponseWithInvalidEmail", func(t *testing.T) { From 034a7d4aafcb8bb9644c212700265244cb8bf37e Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Fri, 4 Oct 2024 10:29:14 +0000 Subject: [PATCH 23/24] chore: apply changes based on feedback --- coderd/userauth.go | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/coderd/userauth.go b/coderd/userauth.go index 6604490fb9e67..968b16680f8cf 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -340,6 +340,19 @@ func (api *API) postChangePasswordWithOneTimePasscode(rw http.ResponseWriter, r return } + if err := userpassword.Validate(req.Password); err != nil { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Invalid password.", + Validations: []codersdk.ValidationError{ + { + Field: "password", + Detail: err.Error(), + }, + }, + }) + return + } + err = api.Database.InTx(func(tx database.Store) error { //nolint:gocritic // In order to change a user's password, we need to get the user first - and can only do that in the system auth context. user, err := tx.GetUserByEmailOrUsername(dbauthz.AsSystemRestricted(ctx), database.GetUserByEmailOrUsernameParams{ @@ -349,6 +362,7 @@ func (api *API) postChangePasswordWithOneTimePasscode(rw http.ResponseWriter, r logger.Error(ctx, "unable to fetch user by email", slog.F("email", req.Email), slog.Error(err)) return xerrors.Errorf("get user by email: %w", err) } + // We continue if err == sql.ErrNoRows to help prevent a timing-based attack. aReq.Old = user equal, err := userpassword.Compare(string(user.HashedOneTimePasscode), req.OneTimePasscode) @@ -365,20 +379,13 @@ func (api *API) postChangePasswordWithOneTimePasscode(rw http.ResponseWriter, r return nil } - if err := userpassword.Validate(req.Password); err != nil { - httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: "Invalid password.", - Validations: []codersdk.ValidationError{ - { - Field: "password", - Detail: err.Error(), - }, - }, - }) - return nil + equal, err = userpassword.Compare(string(user.HashedPassword), req.Password) + if err != nil { + logger.Error(ctx, "unable to compare password", slog.Error(err)) + return xerrors.Errorf("compare password: %w", err) } - if equal, _ = userpassword.Compare(string(user.HashedPassword), req.Password); equal { + if equal { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "New password cannot match old password.", }) From e14d43b611c8ebc185ae20ab9d6c873541f00a48 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Fri, 4 Oct 2024 10:40:26 +0000 Subject: [PATCH 24/24] chore: add missing comment --- coderd/userauth.go | 1 + 1 file changed, 1 insertion(+) diff --git a/coderd/userauth.go b/coderd/userauth.go index 968b16680f8cf..a1e1252797de3 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -251,6 +251,7 @@ func (api *API) postRequestOneTimePasscode(rw http.ResponseWriter, r *http.Reque logger.Error(ctx, "unable to get user by email", slog.Error(err)) return } + // We continue if err == sql.ErrNoRows to help prevent a timing-based attack. aReq.Old = user passcode := uuid.New()