From e137e9251e41e5f2004f15c3f2544728062d9317 Mon Sep 17 00:00:00 2001 From: Colin Adler Date: Wed, 30 Oct 2024 21:16:46 +0000 Subject: [PATCH 01/14] feat: add audit logs for dormancy events An audit log will now be generated when Coder automatically modifies user statuses to dormant, or when a user logs in and are automatically converted to active. --- coderd/database/queries.sql.go | 10 +++++-- coderd/database/queries/users.sql | 2 +- .../provisionerdserver/provisionerdserver.go | 1 + coderd/userauth.go | 23 ++++++++++++++ enterprise/cli/server.go | 2 +- enterprise/coderd/dormancy/dormantusersjob.go | 30 +++++++++++++++++-- .../coderd/dormancy/dormantusersjob_test.go | 6 +++- .../AuditLogDescription.tsx | 2 +- 8 files changed, 67 insertions(+), 9 deletions(-) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index d00c4ec3bcdef..02b54ba588264 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -10408,7 +10408,7 @@ SET WHERE last_seen_at < $2 :: timestamp AND status = 'active'::user_status -RETURNING id, email, last_seen_at +RETURNING id, email, username, last_seen_at ` type UpdateInactiveUsersToDormantParams struct { @@ -10419,6 +10419,7 @@ type UpdateInactiveUsersToDormantParams struct { type UpdateInactiveUsersToDormantRow struct { ID uuid.UUID `db:"id" json:"id"` Email string `db:"email" json:"email"` + Username string `db:"username" json:"username"` LastSeenAt time.Time `db:"last_seen_at" json:"last_seen_at"` } @@ -10431,7 +10432,12 @@ func (q *sqlQuerier) UpdateInactiveUsersToDormant(ctx context.Context, arg Updat var items []UpdateInactiveUsersToDormantRow for rows.Next() { var i UpdateInactiveUsersToDormantRow - if err := rows.Scan(&i.ID, &i.Email, &i.LastSeenAt); err != nil { + if err := rows.Scan( + &i.ID, + &i.Email, + &i.Username, + &i.LastSeenAt, + ); err != nil { return nil, err } items = append(items, i) diff --git a/coderd/database/queries/users.sql b/coderd/database/queries/users.sql index 013e2b8027a45..74e11d18c72fa 100644 --- a/coderd/database/queries/users.sql +++ b/coderd/database/queries/users.sql @@ -286,7 +286,7 @@ SET WHERE last_seen_at < @last_seen_after :: timestamp AND status = 'active'::user_status -RETURNING id, email, last_seen_at; +RETURNING id, email, username, last_seen_at; -- AllUserIDs returns all UserIDs regardless of user status or deletion. -- name: AllUserIDs :many diff --git a/coderd/provisionerdserver/provisionerdserver.go b/coderd/provisionerdserver/provisionerdserver.go index 1f97863e7802b..6c72ff5831947 100644 --- a/coderd/provisionerdserver/provisionerdserver.go +++ b/coderd/provisionerdserver/provisionerdserver.go @@ -1063,6 +1063,7 @@ func (s *server) FailJob(ctx context.Context, failJob *proto.FailedJob) (*proto. wriBytes, err := json.Marshal(buildResourceInfo) if err != nil { s.Logger.Error(ctx, "marshal workspace resource info for failed job", slog.Error(err)) + wriBytes = []byte("{}") } bag := audit.BaggageFromContext(ctx) diff --git a/coderd/userauth.go b/coderd/userauth.go index 13f9b088d731f..392b9ecbb3b5a 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -566,6 +566,7 @@ func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req co } if user.Status == database.UserStatusDormant { + oldUser := user //nolint:gocritic // System needs to update status of the user account (dormant -> active). user, err = api.Database.UpdateUserStatus(dbauthz.AsSystemRestricted(ctx), database.UpdateUserStatusParams{ ID: user.ID, @@ -579,6 +580,28 @@ func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req co }) return user, rbac.Subject{}, false } + + af := map[string]string{ + "automatic_actor": "coder", + "automatic_subsystem": "dormancy", + } + + wriBytes, err := json.Marshal(af) + if err != nil { + api.Logger.Error(ctx, "marshal additional fields for dormancy audit", slog.Error(err)) + wriBytes = []byte("{}") + } + + audit.BackgroundAudit(ctx, &audit.BackgroundAuditParams[database.User]{ + Audit: *api.Auditor.Load(), + Log: api.Logger, + UserID: user.ID, + Action: database.AuditActionWrite, + Old: oldUser, + New: user, + Status: http.StatusOK, + AdditionalFields: wriBytes, + }) } subject, userStatus, err := httpmw.UserRBACSubject(ctx, api.Database, user.ID, rbac.ScopeAll) diff --git a/enterprise/cli/server.go b/enterprise/cli/server.go index 930a3e4956257..211a3a49deb3b 100644 --- a/enterprise/cli/server.go +++ b/enterprise/cli/server.go @@ -95,7 +95,7 @@ func (r *RootCmd) Server(_ func()) *serpent.Command { DefaultQuietHoursSchedule: options.DeploymentValues.UserQuietHoursSchedule.DefaultSchedule.Value(), ProvisionerDaemonPSK: options.DeploymentValues.Provisioner.DaemonPSK.Value(), - CheckInactiveUsersCancelFunc: dormancy.CheckInactiveUsers(ctx, options.Logger, options.Database), + CheckInactiveUsersCancelFunc: dormancy.CheckInactiveUsers(ctx, options.Logger, options.Database, options.Auditor), } if encKeys := options.DeploymentValues.ExternalTokenEncryptionKeys.Value(); len(encKeys) != 0 { diff --git a/enterprise/coderd/dormancy/dormantusersjob.go b/enterprise/coderd/dormancy/dormantusersjob.go index 8c8e22310c031..343beb77eba8d 100644 --- a/enterprise/coderd/dormancy/dormantusersjob.go +++ b/enterprise/coderd/dormancy/dormantusersjob.go @@ -3,12 +3,15 @@ package dormancy import ( "context" "database/sql" + "encoding/json" + "net/http" "time" "golang.org/x/xerrors" "cdr.dev/slog" + "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbtime" ) @@ -22,13 +25,13 @@ const ( // CheckInactiveUsers function updates status of inactive users from active to dormant // using default parameters. -func CheckInactiveUsers(ctx context.Context, logger slog.Logger, db database.Store) func() { - return CheckInactiveUsersWithOptions(ctx, logger, db, jobInterval, accountDormancyPeriod) +func CheckInactiveUsers(ctx context.Context, logger slog.Logger, db database.Store, auditor audit.Auditor) func() { + return CheckInactiveUsersWithOptions(ctx, logger, db, auditor, jobInterval, accountDormancyPeriod) } // CheckInactiveUsersWithOptions function updates status of inactive users from active to dormant // using provided parameters. -func CheckInactiveUsersWithOptions(ctx context.Context, logger slog.Logger, db database.Store, checkInterval, dormancyPeriod time.Duration) func() { +func CheckInactiveUsersWithOptions(ctx context.Context, logger slog.Logger, db database.Store, auditor audit.Auditor, checkInterval, dormancyPeriod time.Duration) func() { logger = logger.Named("dormancy") ctx, cancelFunc := context.WithCancel(ctx) @@ -57,8 +60,29 @@ func CheckInactiveUsersWithOptions(ctx context.Context, logger slog.Logger, db d continue } + af := map[string]string{ + "automatic_actor": "coder", + "automatic_subsystem": "dormancy", + } + + wriBytes, err := json.Marshal(af) + if err != nil { + logger.Error(ctx, "marshal additional fields", slog.Error(err)) + wriBytes = []byte("{}") + } + for _, u := range updatedUsers { logger.Info(ctx, "account has been marked as dormant", slog.F("email", u.Email), slog.F("last_seen_at", u.LastSeenAt)) + audit.BackgroundAudit(ctx, &audit.BackgroundAuditParams[database.User]{ + Audit: auditor, + Log: logger, + UserID: u.ID, + Action: database.AuditActionWrite, + Old: database.User{ID: u.ID, Username: u.Username, Status: database.UserStatusActive}, + New: database.User{ID: u.ID, Username: u.Username, Status: database.UserStatusDormant}, + Status: http.StatusOK, + AdditionalFields: wriBytes, + }) } logger.Debug(ctx, "checking user accounts is done", slog.F("num_dormant_accounts", len(updatedUsers)), slog.F("execution_time", time.Since(startTime))) } diff --git a/enterprise/coderd/dormancy/dormantusersjob_test.go b/enterprise/coderd/dormancy/dormantusersjob_test.go index c752e84bc1d90..c0663b83f20f2 100644 --- a/enterprise/coderd/dormancy/dormantusersjob_test.go +++ b/enterprise/coderd/dormancy/dormantusersjob_test.go @@ -10,6 +10,7 @@ import ( "cdr.dev/slog/sloggers/slogtest" + "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbmem" "github.com/coder/coder/v2/enterprise/coderd/dormancy" @@ -42,8 +43,9 @@ func TestCheckInactiveUsers(t *testing.T) { suspendedUser2 := setupUser(ctx, t, db, "suspended-user-2@coder.com", database.UserStatusSuspended, time.Now().Add(-dormancyPeriod).Add(-time.Hour)) suspendedUser3 := setupUser(ctx, t, db, "suspended-user-3@coder.com", database.UserStatusSuspended, time.Now().Add(-dormancyPeriod).Add(-6*time.Hour)) + mAudit := audit.NewMock() // Run the periodic job - closeFunc := dormancy.CheckInactiveUsersWithOptions(ctx, logger, db, interval, dormancyPeriod) + closeFunc := dormancy.CheckInactiveUsersWithOptions(ctx, logger, db, mAudit, interval, dormancyPeriod) t.Cleanup(closeFunc) var rows []database.GetUsersRow @@ -66,6 +68,8 @@ func TestCheckInactiveUsers(t *testing.T) { return len(rows) == 9 && dormant == 3 && suspended == 3 }, testutil.WaitShort, testutil.IntervalMedium) + require.Len(t, mAudit.AuditLogs(), 3) + allUsers := ignoreUpdatedAt(database.ConvertUserRows(rows)) // Verify user status diff --git a/site/src/pages/AuditPage/AuditLogRow/AuditLogDescription/AuditLogDescription.tsx b/site/src/pages/AuditPage/AuditLogRow/AuditLogDescription/AuditLogDescription.tsx index dd00129f935eb..51d4e8ec910d9 100644 --- a/site/src/pages/AuditPage/AuditLogRow/AuditLogDescription/AuditLogDescription.tsx +++ b/site/src/pages/AuditPage/AuditLogRow/AuditLogDescription/AuditLogDescription.tsx @@ -23,7 +23,7 @@ export const AuditLogDescription: FC = ({ target = ""; } - // This occurs when SCIM creates a user. + // This occurs when SCIM creates a user, or dormancy changes a users status. if ( auditLog.resource_type === "user" && auditLog.additional_fields?.automatic_actor === "coder" From e84b89388c907b24bc7bd3a45b1276bbf7b378ad Mon Sep 17 00:00:00 2001 From: Colin Adler Date: Wed, 30 Oct 2024 21:40:52 +0000 Subject: [PATCH 02/14] fixup! feat: add audit logs for dormancy events --- coderd/database/dbmem/dbmem.go | 8 +++++++- coderd/database/queries.sql.go | 7 +++++-- coderd/database/queries/users.sql | 5 +++-- coderd/users.go | 2 ++ 4 files changed, 17 insertions(+), 5 deletions(-) diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index 4f54598744dd0..aee63d803e56d 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -7709,6 +7709,11 @@ func (q *FakeQuerier) InsertUser(_ context.Context, arg database.InsertUserParam } } + status := database.UserStatusDormant + if arg.Status != "" { + status = arg.Status + } + user := database.User{ ID: arg.ID, Email: arg.Email, @@ -7717,7 +7722,7 @@ func (q *FakeQuerier) InsertUser(_ context.Context, arg database.InsertUserParam UpdatedAt: arg.UpdatedAt, Username: arg.Username, Name: arg.Name, - Status: database.UserStatusDormant, + Status: status, RBACRoles: arg.RBACRoles, LoginType: arg.LoginType, } @@ -8640,6 +8645,7 @@ func (q *FakeQuerier) UpdateInactiveUsersToDormant(_ context.Context, params dat updated = append(updated, database.UpdateInactiveUsersToDormantRow{ ID: user.ID, Email: user.Email, + Username: user.Username, LastSeenAt: user.LastSeenAt, }) } diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 02b54ba588264..f42b4b01a5805 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -10345,10 +10345,11 @@ INSERT INTO created_at, updated_at, rbac_roles, - login_type + login_type, + status ) VALUES - ($1, $2, $3, $4, $5, $6, $7, $8, $9) RETURNING id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url, deleted, last_seen_at, quiet_hours_schedule, theme_preference, name, github_com_user_id, hashed_one_time_passcode, one_time_passcode_expires_at + ($1, $2, $3, $4, $5, $6, $7, $8, $9, NULLIF($10::user_status, '')) RETURNING id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url, deleted, last_seen_at, quiet_hours_schedule, theme_preference, name, github_com_user_id, hashed_one_time_passcode, one_time_passcode_expires_at ` type InsertUserParams struct { @@ -10361,6 +10362,7 @@ type InsertUserParams struct { UpdatedAt time.Time `db:"updated_at" json:"updated_at"` RBACRoles pq.StringArray `db:"rbac_roles" json:"rbac_roles"` LoginType LoginType `db:"login_type" json:"login_type"` + Status UserStatus `db:"status" json:"status"` } func (q *sqlQuerier) InsertUser(ctx context.Context, arg InsertUserParams) (User, error) { @@ -10374,6 +10376,7 @@ func (q *sqlQuerier) InsertUser(ctx context.Context, arg InsertUserParams) (User arg.UpdatedAt, arg.RBACRoles, arg.LoginType, + arg.Status, ) var i User err := row.Scan( diff --git a/coderd/database/queries/users.sql b/coderd/database/queries/users.sql index 74e11d18c72fa..42da37561638e 100644 --- a/coderd/database/queries/users.sql +++ b/coderd/database/queries/users.sql @@ -67,10 +67,11 @@ INSERT INTO created_at, updated_at, rbac_roles, - login_type + login_type, + status ) VALUES - ($1, $2, $3, $4, $5, $6, $7, $8, $9) RETURNING *; + ($1, $2, $3, $4, $5, $6, $7, $8, $9, NULLIF(@status::user_status, '')) RETURNING *; -- name: UpdateUserProfile :one UPDATE diff --git a/coderd/users.go b/coderd/users.go index 5e521da3a6004..3749ae3c267b6 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -1328,6 +1328,7 @@ func (api *API) organizationByUserAndName(rw http.ResponseWriter, r *http.Reques type CreateUserRequest struct { codersdk.CreateUserRequestWithOrgs LoginType database.LoginType + Status database.UserStatus SkipNotifications bool accountCreatorName string } @@ -1354,6 +1355,7 @@ func (api *API) CreateUser(ctx context.Context, store database.Store, req Create // All new users are defaulted to members of the site. RBACRoles: []string{}, LoginType: req.LoginType, + Status: req.Status, } // If a user signs up with OAuth, they can have no password! if req.Password != "" { From 388111b16b09aba1e7dc0605c4b7a077d0c41216 Mon Sep 17 00:00:00 2001 From: Colin Adler Date: Wed, 30 Oct 2024 21:48:22 +0000 Subject: [PATCH 03/14] fixup! feat: add audit logs for dormancy events --- coderd/database/dbmem/dbmem.go | 2 +- coderd/database/queries.sql.go | 4 ++-- coderd/database/queries/users.sql | 2 +- coderd/users.go | 3 ++- 4 files changed, 6 insertions(+), 5 deletions(-) diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index aee63d803e56d..e949b5be4880d 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -7711,7 +7711,7 @@ func (q *FakeQuerier) InsertUser(_ context.Context, arg database.InsertUserParam status := database.UserStatusDormant if arg.Status != "" { - status = arg.Status + status = database.UserStatus(arg.Status) } user := database.User{ diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index f42b4b01a5805..d9208c6549065 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -10349,7 +10349,7 @@ INSERT INTO status ) VALUES - ($1, $2, $3, $4, $5, $6, $7, $8, $9, NULLIF($10::user_status, '')) RETURNING id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url, deleted, last_seen_at, quiet_hours_schedule, theme_preference, name, github_com_user_id, hashed_one_time_passcode, one_time_passcode_expires_at + ($1, $2, $3, $4, $5, $6, $7, $8, $9, NULLIF($10::text, '')::user_status) RETURNING id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url, deleted, last_seen_at, quiet_hours_schedule, theme_preference, name, github_com_user_id, hashed_one_time_passcode, one_time_passcode_expires_at ` type InsertUserParams struct { @@ -10362,7 +10362,7 @@ type InsertUserParams struct { UpdatedAt time.Time `db:"updated_at" json:"updated_at"` RBACRoles pq.StringArray `db:"rbac_roles" json:"rbac_roles"` LoginType LoginType `db:"login_type" json:"login_type"` - Status UserStatus `db:"status" json:"status"` + Status string `db:"status" json:"status"` } func (q *sqlQuerier) InsertUser(ctx context.Context, arg InsertUserParams) (User, error) { diff --git a/coderd/database/queries/users.sql b/coderd/database/queries/users.sql index 42da37561638e..d1ce4132ac857 100644 --- a/coderd/database/queries/users.sql +++ b/coderd/database/queries/users.sql @@ -71,7 +71,7 @@ INSERT INTO status ) VALUES - ($1, $2, $3, $4, $5, $6, $7, $8, $9, NULLIF(@status::user_status, '')) RETURNING *; + ($1, $2, $3, $4, $5, $6, $7, $8, $9, NULLIF(@status::text, '')::user_status) RETURNING *; -- name: UpdateUserProfile :one UPDATE diff --git a/coderd/users.go b/coderd/users.go index 3749ae3c267b6..1c28add720bbe 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -195,6 +195,7 @@ func (api *API) postFirstUser(rw http.ResponseWriter, r *http.Request) { OrganizationIDs: []uuid.UUID{defaultOrg.ID}, }, LoginType: database.LoginTypePassword, + Status: database.UserStatusActive, accountCreatorName: "coder", }) if err != nil { @@ -1355,7 +1356,7 @@ func (api *API) CreateUser(ctx context.Context, store database.Store, req Create // All new users are defaulted to members of the site. RBACRoles: []string{}, LoginType: req.LoginType, - Status: req.Status, + Status: string(req.Status), } // If a user signs up with OAuth, they can have no password! if req.Password != "" { From 850ee3870eab59240ed845604a8b125b3cc7cd63 Mon Sep 17 00:00:00 2001 From: Colin Adler Date: Wed, 30 Oct 2024 22:50:53 +0000 Subject: [PATCH 04/14] fixup! feat: add audit logs for dormancy events --- cli/server_createadminuser.go | 1 + coderd/apidoc/docs.go | 8 ++++++++ coderd/apidoc/swagger.json | 8 ++++++++ coderd/coderdtest/coderdtest.go | 3 +++ coderd/database/dbgen/dbgen.go | 1 + coderd/users.go | 9 +++++---- coderd/users_test.go | 35 +++++++++++++++++++++++++++++++++ codersdk/users.go | 2 ++ docs/reference/api/schemas.md | 18 +++++++++-------- docs/reference/api/users.md | 1 + site/src/api/typesGenerated.ts | 1 + 11 files changed, 75 insertions(+), 12 deletions(-) diff --git a/cli/server_createadminuser.go b/cli/server_createadminuser.go index 0619688468554..7ef95e7e093e6 100644 --- a/cli/server_createadminuser.go +++ b/cli/server_createadminuser.go @@ -197,6 +197,7 @@ func (r *RootCmd) newCreateAdminUserCommand() *serpent.Command { UpdatedAt: dbtime.Now(), RBACRoles: []string{rbac.RoleOwner().String()}, LoginType: database.LoginTypePassword, + Status: "", }) if err != nil { return xerrors.Errorf("insert user: %w", err) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 83d1fdc2c492a..27aef82123db8 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -9896,6 +9896,14 @@ const docTemplate = `{ "password": { "type": "string" }, + "user_status": { + "description": "UserStatus defaults to UserStatusDormant.", + "allOf": [ + { + "$ref": "#/definitions/codersdk.UserStatus" + } + ] + }, "username": { "type": "string" } diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 9861e195b7a69..2a19c40d979b3 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -8809,6 +8809,14 @@ "password": { "type": "string" }, + "user_status": { + "description": "UserStatus defaults to UserStatusDormant.", + "allOf": [ + { + "$ref": "#/definitions/codersdk.UserStatus" + } + ] + }, "username": { "type": "string" } diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index e287e04b8d0cf..1bb07f07ca65f 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -719,6 +719,9 @@ func createAnotherUserRetry(t testing.TB, client *codersdk.Client, organizationI Name: RandomName(t), Password: "SomeSecurePassword!", OrganizationIDs: organizationIDs, + // Always create users as active in tests to ignore an extra audit log + // when logging in. + UserStatus: codersdk.UserStatusActive, } for _, m := range mutators { m(&req) diff --git a/coderd/database/dbgen/dbgen.go b/coderd/database/dbgen/dbgen.go index 69419b98c79b1..3df873cdb4cbf 100644 --- a/coderd/database/dbgen/dbgen.go +++ b/coderd/database/dbgen/dbgen.go @@ -342,6 +342,7 @@ func User(t testing.TB, db database.Store, orig database.User) database.User { UpdatedAt: takeFirst(orig.UpdatedAt, dbtime.Now()), RBACRoles: takeFirstSlice(orig.RBACRoles, []string{}), LoginType: takeFirst(orig.LoginType, database.LoginTypePassword), + Status: string(takeFirst(orig.Status, database.UserStatusDormant)), }) require.NoError(t, err, "insert user") diff --git a/coderd/users.go b/coderd/users.go index 1c28add720bbe..f3dba95f9d220 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -192,10 +192,12 @@ func (api *API) postFirstUser(rw http.ResponseWriter, r *http.Request) { Username: createUser.Username, Name: createUser.Name, Password: createUser.Password, + UserStatus: codersdk.UserStatusActive, OrganizationIDs: []uuid.UUID{defaultOrg.ID}, }, - LoginType: database.LoginTypePassword, - Status: database.UserStatusActive, + LoginType: database.LoginTypePassword, + // There's no reason to create the first user as dormant, since you have + // to login immediately anyways. accountCreatorName: "coder", }) if err != nil { @@ -1329,7 +1331,6 @@ func (api *API) organizationByUserAndName(rw http.ResponseWriter, r *http.Reques type CreateUserRequest struct { codersdk.CreateUserRequestWithOrgs LoginType database.LoginType - Status database.UserStatus SkipNotifications bool accountCreatorName string } @@ -1356,7 +1357,7 @@ func (api *API) CreateUser(ctx context.Context, store database.Store, req Create // All new users are defaulted to members of the site. RBACRoles: []string{}, LoginType: req.LoginType, - Status: string(req.Status), + Status: string(req.UserStatus), } // If a user signs up with OAuth, they can have no password! if req.Password != "" { diff --git a/coderd/users_test.go b/coderd/users_test.go index c33ca933a9d96..13b18d746bdf6 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -695,6 +695,41 @@ func TestPostUsers(t *testing.T) { }) require.NoError(t, err) + // User should default to dormant. + require.Equal(t, codersdk.UserStatusDormant, user.Status) + + require.Len(t, auditor.AuditLogs(), numLogs) + require.Equal(t, database.AuditActionCreate, auditor.AuditLogs()[numLogs-1].Action) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs()[numLogs-2].Action) + + require.Len(t, user.OrganizationIDs, 1) + assert.Equal(t, firstUser.OrganizationID, user.OrganizationIDs[0]) + }) + + t.Run("CreateWithStatus", func(t *testing.T) { + t.Parallel() + auditor := audit.NewMock() + client := coderdtest.New(t, &coderdtest.Options{Auditor: auditor}) + numLogs := len(auditor.AuditLogs()) + + firstUser := coderdtest.CreateFirstUser(t, client) + numLogs++ // add an audit log for user create + numLogs++ // add an audit log for login + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + user, err := client.CreateUserWithOrgs(ctx, codersdk.CreateUserRequestWithOrgs{ + OrganizationIDs: []uuid.UUID{firstUser.OrganizationID}, + Email: "another@user.org", + Username: "someone-else", + Password: "SomeSecurePassword!", + UserStatus: codersdk.UserStatusActive, + }) + require.NoError(t, err) + + require.Equal(t, codersdk.UserStatusActive, user.Status) + require.Len(t, auditor.AuditLogs(), numLogs) require.Equal(t, database.AuditActionCreate, auditor.AuditLogs()[numLogs-1].Action) require.Equal(t, database.AuditActionLogin, auditor.AuditLogs()[numLogs-2].Action) diff --git a/codersdk/users.go b/codersdk/users.go index f57b8010f9229..53c58bb64613e 100644 --- a/codersdk/users.go +++ b/codersdk/users.go @@ -139,6 +139,8 @@ type CreateUserRequestWithOrgs struct { Password string `json:"password"` // UserLoginType defaults to LoginTypePassword. UserLoginType LoginType `json:"login_type"` + // UserStatus defaults to UserStatusDormant. + UserStatus UserStatus `json:"user_status"` // OrganizationIDs is a list of organization IDs that the user should be a member of. OrganizationIDs []uuid.UUID `json:"organization_ids" validate:"" format:"uuid"` } diff --git a/docs/reference/api/schemas.md b/docs/reference/api/schemas.md index f4e683305029b..64df97a031543 100644 --- a/docs/reference/api/schemas.md +++ b/docs/reference/api/schemas.md @@ -1342,20 +1342,22 @@ AuthorizationObject can represent a "set" of objects, such as: all workspaces in "name": "string", "organization_ids": ["497f6eca-6276-4993-bfeb-53cbbbba6f08"], "password": "string", + "user_status": "active", "username": "string" } ``` ### Properties -| Name | Type | Required | Restrictions | Description | -| ------------------ | ---------------------------------------- | -------- | ------------ | ----------------------------------------------------------------------------------- | -| `email` | string | true | | | -| `login_type` | [codersdk.LoginType](#codersdklogintype) | false | | Login type defaults to LoginTypePassword. | -| `name` | string | false | | | -| `organization_ids` | array of string | false | | Organization ids is a list of organization IDs that the user should be a member of. | -| `password` | string | false | | | -| `username` | string | true | | | +| Name | Type | Required | Restrictions | Description | +| ------------------ | ------------------------------------------ | -------- | ------------ | ----------------------------------------------------------------------------------- | +| `email` | string | true | | | +| `login_type` | [codersdk.LoginType](#codersdklogintype) | false | | Login type defaults to LoginTypePassword. | +| `name` | string | false | | | +| `organization_ids` | array of string | false | | Organization ids is a list of organization IDs that the user should be a member of. | +| `password` | string | false | | | +| `user_status` | [codersdk.UserStatus](#codersdkuserstatus) | false | | User status defaults to UserStatusDormant. | +| `username` | string | true | | | ## codersdk.CreateWorkspaceBuildRequest diff --git a/docs/reference/api/users.md b/docs/reference/api/users.md index 3979f5521b377..5e0ae3c239c04 100644 --- a/docs/reference/api/users.md +++ b/docs/reference/api/users.md @@ -86,6 +86,7 @@ curl -X POST http://coder-server:8080/api/v2/users \ "name": "string", "organization_ids": ["497f6eca-6276-4993-bfeb-53cbbbba6f08"], "password": "string", + "user_status": "active", "username": "string" } ``` diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index d687fb68ec61f..a43c614ae694c 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -328,6 +328,7 @@ export interface CreateUserRequestWithOrgs { readonly name: string; readonly password: string; readonly login_type: LoginType; + readonly user_status: UserStatus; readonly organization_ids: Readonly>; } From 7d2b5323e93720d85a1b7ae3f58e14e520c5121b Mon Sep 17 00:00:00 2001 From: Colin Adler Date: Wed, 30 Oct 2024 23:03:48 +0000 Subject: [PATCH 05/14] fixup! feat: add audit logs for dormancy events --- coderd/database/queries.sql.go | 6 +++++- coderd/database/queries/users.sql | 6 +++++- site/e2e/api.ts | 1 + 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index d9208c6549065..1030f1503b5cd 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -10349,7 +10349,11 @@ INSERT INTO status ) VALUES - ($1, $2, $3, $4, $5, $6, $7, $8, $9, NULLIF($10::text, '')::user_status) RETURNING id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url, deleted, last_seen_at, quiet_hours_schedule, theme_preference, name, github_com_user_id, hashed_one_time_passcode, one_time_passcode_expires_at + ($1, $2, $3, $4, $5, $6, $7, $8, $9, + -- if the status passed in is empty, fallback to dormant, which is what + -- we were doing before. + COALESCE(NULLIF($10::text, '')::user_status, 'dormant'::user_status) + ) RETURNING id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url, deleted, last_seen_at, quiet_hours_schedule, theme_preference, name, github_com_user_id, hashed_one_time_passcode, one_time_passcode_expires_at ` type InsertUserParams struct { diff --git a/coderd/database/queries/users.sql b/coderd/database/queries/users.sql index d1ce4132ac857..a4f8844fd2db5 100644 --- a/coderd/database/queries/users.sql +++ b/coderd/database/queries/users.sql @@ -71,7 +71,11 @@ INSERT INTO status ) VALUES - ($1, $2, $3, $4, $5, $6, $7, $8, $9, NULLIF(@status::text, '')::user_status) RETURNING *; + ($1, $2, $3, $4, $5, $6, $7, $8, $9, + -- if the status passed in is empty, fallback to dormant, which is what + -- we were doing before. + COALESCE(NULLIF(@status::text, '')::user_status, 'dormant'::user_status) + ) RETURNING *; -- name: UpdateUserProfile :one UPDATE diff --git a/site/e2e/api.ts b/site/e2e/api.ts index da5a57dee007d..963045f47230b 100644 --- a/site/e2e/api.ts +++ b/site/e2e/api.ts @@ -37,6 +37,7 @@ export const createUser = async (orgId: string) => { password: "s3cure&password!", login_type: "password", organization_ids: [orgId], + user_status: "dormant" }); return user; }; From 68adf8addc4b788c1c53e96d8c911200c965b163 Mon Sep 17 00:00:00 2001 From: Colin Adler Date: Wed, 30 Oct 2024 23:11:40 +0000 Subject: [PATCH 06/14] fixup! feat: add audit logs for dormancy events --- coderd/coderdtest/coderdtest.go | 2 +- coderd/users.go | 9 +++++++-- coderd/users_test.go | 3 ++- codersdk/users.go | 2 +- site/e2e/api.ts | 1 - site/src/api/typesGenerated.ts | 2 +- 6 files changed, 12 insertions(+), 7 deletions(-) diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index 1bb07f07ca65f..f3868bf14d54b 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -721,7 +721,7 @@ func createAnotherUserRetry(t testing.TB, client *codersdk.Client, organizationI OrganizationIDs: organizationIDs, // Always create users as active in tests to ignore an extra audit log // when logging in. - UserStatus: codersdk.UserStatusActive, + UserStatus: ptr.Ref(codersdk.UserStatusActive), } for _, m := range mutators { m(&req) diff --git a/coderd/users.go b/coderd/users.go index f3dba95f9d220..c62fa29885f10 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -28,6 +28,7 @@ import ( "github.com/coder/coder/v2/coderd/searchquery" "github.com/coder/coder/v2/coderd/telemetry" "github.com/coder/coder/v2/coderd/userpassword" + "github.com/coder/coder/v2/coderd/util/ptr" "github.com/coder/coder/v2/coderd/util/slice" "github.com/coder/coder/v2/codersdk" ) @@ -192,7 +193,7 @@ func (api *API) postFirstUser(rw http.ResponseWriter, r *http.Request) { Username: createUser.Username, Name: createUser.Name, Password: createUser.Password, - UserStatus: codersdk.UserStatusActive, + UserStatus: ptr.Ref(codersdk.UserStatusActive), OrganizationIDs: []uuid.UUID{defaultOrg.ID}, }, LoginType: database.LoginTypePassword, @@ -1346,6 +1347,10 @@ func (api *API) CreateUser(ctx context.Context, store database.Store, req Create err := store.InTx(func(tx database.Store) error { orgRoles := make([]string, 0) + status := "" + if req.UserStatus != nil { + status = string(*req.UserStatus) + } params := database.InsertUserParams{ ID: uuid.New(), Email: req.Email, @@ -1357,7 +1362,7 @@ func (api *API) CreateUser(ctx context.Context, store database.Store, req Create // All new users are defaulted to members of the site. RBACRoles: []string{}, LoginType: req.LoginType, - Status: string(req.UserStatus), + Status: status, } // If a user signs up with OAuth, they can have no password! if req.Password != "" { diff --git a/coderd/users_test.go b/coderd/users_test.go index 13b18d746bdf6..3c88d3e5022ac 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -30,6 +30,7 @@ import ( "github.com/coder/coder/v2/coderd/database/dbgen" "github.com/coder/coder/v2/coderd/database/dbtime" "github.com/coder/coder/v2/coderd/rbac" + "github.com/coder/coder/v2/coderd/util/ptr" "github.com/coder/coder/v2/coderd/util/slice" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/testutil" @@ -724,7 +725,7 @@ func TestPostUsers(t *testing.T) { Email: "another@user.org", Username: "someone-else", Password: "SomeSecurePassword!", - UserStatus: codersdk.UserStatusActive, + UserStatus: ptr.Ref(codersdk.UserStatusActive), }) require.NoError(t, err) diff --git a/codersdk/users.go b/codersdk/users.go index 53c58bb64613e..546fcc99e9fbe 100644 --- a/codersdk/users.go +++ b/codersdk/users.go @@ -140,7 +140,7 @@ type CreateUserRequestWithOrgs struct { // UserLoginType defaults to LoginTypePassword. UserLoginType LoginType `json:"login_type"` // UserStatus defaults to UserStatusDormant. - UserStatus UserStatus `json:"user_status"` + UserStatus *UserStatus `json:"user_status"` // OrganizationIDs is a list of organization IDs that the user should be a member of. OrganizationIDs []uuid.UUID `json:"organization_ids" validate:"" format:"uuid"` } diff --git a/site/e2e/api.ts b/site/e2e/api.ts index 963045f47230b..da5a57dee007d 100644 --- a/site/e2e/api.ts +++ b/site/e2e/api.ts @@ -37,7 +37,6 @@ export const createUser = async (orgId: string) => { password: "s3cure&password!", login_type: "password", organization_ids: [orgId], - user_status: "dormant" }); return user; }; diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index a43c614ae694c..4568948595e7c 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -328,7 +328,7 @@ export interface CreateUserRequestWithOrgs { readonly name: string; readonly password: string; readonly login_type: LoginType; - readonly user_status: UserStatus; + readonly user_status?: UserStatus; readonly organization_ids: Readonly>; } From 764bc250ccb2474d9428a864cec009cfdefbb87e Mon Sep 17 00:00:00 2001 From: Colin Adler Date: Thu, 31 Oct 2024 19:29:56 +0000 Subject: [PATCH 07/14] fixup! feat: add audit logs for dormancy events --- coderd/audit/fields.go | 29 +++++++++++++ coderd/audit/request.go | 13 +++--- coderd/coderd.go | 1 + coderd/httpmw/apikey.go | 21 +++------ coderd/userauth.go | 86 ++++++++++++++----------------------- coderd/users.go | 14 +++--- enterprise/coderd/coderd.go | 1 + 7 files changed, 85 insertions(+), 80 deletions(-) create mode 100644 coderd/audit/fields.go diff --git a/coderd/audit/fields.go b/coderd/audit/fields.go new file mode 100644 index 0000000000000..3f12bc9fa8f8b --- /dev/null +++ b/coderd/audit/fields.go @@ -0,0 +1,29 @@ +package audit + +import ( + "context" + "encoding/json" + + "cdr.dev/slog" +) + +type BackgroundSubsystem string + +const ( + BackgroundSubsystemDormancy BackgroundSubsystem = "dormancy" +) + +func BackgroundTaskFields(ctx context.Context, logger slog.Logger, subsystem BackgroundSubsystem) []byte { + af := map[string]string{ + "automatic_actor": "coder", + "automatic_subsystem": string(subsystem), + } + + wriBytes, err := json.Marshal(af) + if err != nil { + logger.Error(ctx, "marshal additional fields for dormancy audit", slog.Error(err)) + return []byte("{}") + } + + return wriBytes +} diff --git a/coderd/audit/request.go b/coderd/audit/request.go index 88b637384eeda..c8b7bf17b4b96 100644 --- a/coderd/audit/request.go +++ b/coderd/audit/request.go @@ -62,12 +62,13 @@ type BackgroundAuditParams[T Auditable] struct { Audit Auditor Log slog.Logger - UserID uuid.UUID - RequestID uuid.UUID - Status int - Action database.AuditAction - OrganizationID uuid.UUID - IP string + UserID uuid.UUID + RequestID uuid.UUID + Status int + Action database.AuditAction + OrganizationID uuid.UUID + IP string + // todo: this should automatically marshal an interface{} instead of accepting a raw message. AdditionalFields json.RawMessage New T diff --git a/coderd/coderd.go b/coderd/coderd.go index bd844d7ca13c3..b50b50940b0e4 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -702,6 +702,7 @@ func New(options *Options) *API { apiKeyMiddleware := httpmw.ExtractAPIKeyMW(httpmw.ExtractAPIKeyConfig{ DB: options.Database, + HandleDormancy: api.ActivateDormantUser, OAuth2Configs: oauthConfigs, RedirectToLogin: false, DisableSessionExpiryRefresh: options.DeploymentValues.Sessions.DisableExpiryRefresh.Value(), diff --git a/coderd/httpmw/apikey.go b/coderd/httpmw/apikey.go index c4d1c7f202533..f8e3c5a738739 100644 --- a/coderd/httpmw/apikey.go +++ b/coderd/httpmw/apikey.go @@ -82,6 +82,7 @@ const ( type ExtractAPIKeyConfig struct { DB database.Store + HandleDormancy func(ctx context.Context, u database.User) database.User OAuth2Configs *OAuth2Configs RedirectToLogin bool DisableSessionExpiryRefresh bool @@ -414,21 +415,13 @@ func ExtractAPIKey(rw http.ResponseWriter, r *http.Request, cfg ExtractAPIKeyCon }) } - if userStatus == database.UserStatusDormant { - // If coder confirms that the dormant user is valid, it can switch their account to active. - // nolint:gocritic - u, err := cfg.DB.UpdateUserStatus(dbauthz.AsSystemRestricted(ctx), database.UpdateUserStatusParams{ - ID: key.UserID, - Status: database.UserStatusActive, - UpdatedAt: dbtime.Now(), + if userStatus == database.UserStatusDormant && cfg.HandleDormancy != nil { + id, _ := uuid.Parse(actor.ID) + cfg.HandleDormancy(ctx, database.User{ + ID: id, + Username: actor.FriendlyName, + Status: userStatus, }) - if err != nil { - return write(http.StatusInternalServerError, codersdk.Response{ - Message: internalErrorMessage, - Detail: fmt.Sprintf("can't activate a dormant user: %s", err.Error()), - }) - } - userStatus = u.Status } if userStatus != database.UserStatusActive { diff --git a/coderd/userauth.go b/coderd/userauth.go index 392b9ecbb3b5a..4e064adbb80b7 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -565,44 +565,7 @@ func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req co return user, rbac.Subject{}, false } - if user.Status == database.UserStatusDormant { - oldUser := user - //nolint:gocritic // System needs to update status of the user account (dormant -> active). - user, err = api.Database.UpdateUserStatus(dbauthz.AsSystemRestricted(ctx), database.UpdateUserStatusParams{ - ID: user.ID, - Status: database.UserStatusActive, - UpdatedAt: dbtime.Now(), - }) - if err != nil { - logger.Error(ctx, "unable to update user status to active", slog.Error(err)) - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error occurred. Try again later, or contact an admin for assistance.", - }) - return user, rbac.Subject{}, false - } - - af := map[string]string{ - "automatic_actor": "coder", - "automatic_subsystem": "dormancy", - } - - wriBytes, err := json.Marshal(af) - if err != nil { - api.Logger.Error(ctx, "marshal additional fields for dormancy audit", slog.Error(err)) - wriBytes = []byte("{}") - } - - audit.BackgroundAudit(ctx, &audit.BackgroundAuditParams[database.User]{ - Audit: *api.Auditor.Load(), - Log: api.Logger, - UserID: user.ID, - Action: database.AuditActionWrite, - Old: oldUser, - New: user, - Status: http.StatusOK, - AdditionalFields: wriBytes, - }) - } + user = api.ActivateDormantUser(ctx, user) subject, userStatus, err := httpmw.UserRBACSubject(ctx, api.Database, user.ID, rbac.ScopeAll) if err != nil { @@ -624,6 +587,36 @@ func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req co return user, subject, true } +func (api *API) ActivateDormantUser(ctx context.Context, user database.User) database.User { + if user.Status != database.UserStatusDormant { + return user + } + + //nolint:gocritic // System needs to update status of the user account (dormant -> active). + newUser, err := api.Database.UpdateUserStatus(dbauthz.AsSystemRestricted(ctx), database.UpdateUserStatusParams{ + ID: user.ID, + Status: database.UserStatusActive, + UpdatedAt: dbtime.Now(), + }) + if err != nil { + api.Logger.Error(ctx, "unable to update user status to active", slog.Error(err)) + return user + } + + audit.BackgroundAudit(ctx, &audit.BackgroundAuditParams[database.User]{ + Audit: *api.Auditor.Load(), + Log: api.Logger, + UserID: user.ID, + Action: database.AuditActionWrite, + Old: user, + New: newUser, + Status: http.StatusOK, + AdditionalFields: audit.BackgroundTaskFields(ctx, api.Logger, audit.BackgroundSubsystemDormancy), + }) + + return newUser +} + // Clear the user's session cookie. // // @Summary Log out user @@ -1411,9 +1404,10 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C ctx = r.Context() user database.User cookies []*http.Cookie - logger = api.Logger.Named(userAuthLoggerName) ) + params.User = api.ActivateDormantUser(ctx, params.User) + var isConvertLoginType bool err := api.Database.InTx(func(tx database.Store) error { var ( @@ -1522,20 +1516,6 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C } } - // Activate dormant user on sign-in - if user.Status == database.UserStatusDormant { - //nolint:gocritic // System needs to update status of the user account (dormant -> active). - user, err = tx.UpdateUserStatus(dbauthz.AsSystemRestricted(ctx), database.UpdateUserStatusParams{ - ID: user.ID, - Status: database.UserStatusActive, - UpdatedAt: dbtime.Now(), - }) - if err != nil { - logger.Error(ctx, "unable to update user status to active", slog.Error(err)) - return xerrors.Errorf("update user status: %w", err) - } - } - debugContext, err := json.Marshal(params.DebugContext) if err != nil { return xerrors.Errorf("marshal debug context: %w", err) diff --git a/coderd/users.go b/coderd/users.go index c62fa29885f10..445b44f334349 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -189,16 +189,16 @@ func (api *API) postFirstUser(rw http.ResponseWriter, r *http.Request) { //nolint:gocritic // needed to create first user user, err := api.CreateUser(dbauthz.AsSystemRestricted(ctx), api.Database, CreateUserRequest{ CreateUserRequestWithOrgs: codersdk.CreateUserRequestWithOrgs{ - Email: createUser.Email, - Username: createUser.Username, - Name: createUser.Name, - Password: createUser.Password, + Email: createUser.Email, + Username: createUser.Username, + Name: createUser.Name, + Password: createUser.Password, + // There's no reason to create the first user as dormant, since you have + // to login immediately anyways. UserStatus: ptr.Ref(codersdk.UserStatusActive), OrganizationIDs: []uuid.UUID{defaultOrg.ID}, }, - LoginType: database.LoginTypePassword, - // There's no reason to create the first user as dormant, since you have - // to login immediately anyways. + LoginType: database.LoginTypePassword, accountCreatorName: "coder", }) if err != nil { diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index 7e59eb341411f..eb5b6df433c1f 100644 --- a/enterprise/coderd/coderd.go +++ b/enterprise/coderd/coderd.go @@ -172,6 +172,7 @@ func New(ctx context.Context, options *Options) (_ *API, err error) { } apiKeyMiddleware := httpmw.ExtractAPIKeyMW(httpmw.ExtractAPIKeyConfig{ DB: options.Database, + HandleDormancy: api.AGPL.ActivateDormantUser, OAuth2Configs: oauthConfigs, RedirectToLogin: false, DisableSessionExpiryRefresh: options.DeploymentValues.Sessions.DisableExpiryRefresh.Value(), From 7846882a6b2e85d28ead4a73dadacdb63d4ebd71 Mon Sep 17 00:00:00 2001 From: Colin Adler Date: Thu, 31 Oct 2024 19:41:35 +0000 Subject: [PATCH 08/14] fixup! feat: add audit logs for dormancy events --- coderd/userauth.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/coderd/userauth.go b/coderd/userauth.go index 4e064adbb80b7..11ac3ddcb8c4c 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -27,6 +27,7 @@ import ( "github.com/coder/coder/v2/coderd/cryptokeys" "github.com/coder/coder/v2/coderd/idpsync" "github.com/coder/coder/v2/coderd/jwtutils" + "github.com/coder/coder/v2/coderd/util/ptr" "github.com/coder/coder/v2/coderd/apikey" "github.com/coder/coder/v2/coderd/audit" @@ -1507,6 +1508,7 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C Email: params.Email, Username: params.Username, OrganizationIDs: orgIDs, + UserStatus: ptr.Ref(codersdk.UserStatusActive), }, LoginType: params.LoginType, accountCreatorName: "oauth", From cd7740512ef1fd140d6207b9835879910fe28ec8 Mon Sep 17 00:00:00 2001 From: Colin Adler Date: Thu, 31 Oct 2024 19:42:04 +0000 Subject: [PATCH 09/14] fixup! feat: add audit logs for dormancy events --- coderd/userauth.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/userauth.go b/coderd/userauth.go index 11ac3ddcb8c4c..885f2c5a75e54 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -589,7 +589,7 @@ func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req co } func (api *API) ActivateDormantUser(ctx context.Context, user database.User) database.User { - if user.Status != database.UserStatusDormant { + if user.ID == uuid.Nil || user.Status != database.UserStatusDormant { return user } From 95ac7364646d78e6ed80d25a441916112fd793c9 Mon Sep 17 00:00:00 2001 From: Colin Adler Date: Thu, 31 Oct 2024 20:08:19 +0000 Subject: [PATCH 10/14] fixup! feat: add audit logs for dormancy events --- enterprise/cli/server.go | 3 +- enterprise/coderd/dormancy/dormantusersjob.go | 84 +++++++------------ .../coderd/dormancy/dormantusersjob_test.go | 40 ++++----- 3 files changed, 54 insertions(+), 73 deletions(-) diff --git a/enterprise/cli/server.go b/enterprise/cli/server.go index 211a3a49deb3b..1bf4f31a8506b 100644 --- a/enterprise/cli/server.go +++ b/enterprise/cli/server.go @@ -23,6 +23,7 @@ import ( "github.com/coder/coder/v2/enterprise/dbcrypt" "github.com/coder/coder/v2/enterprise/trialer" "github.com/coder/coder/v2/tailnet" + "github.com/coder/quartz" "github.com/coder/serpent" agplcoderd "github.com/coder/coder/v2/coderd" @@ -95,7 +96,7 @@ func (r *RootCmd) Server(_ func()) *serpent.Command { DefaultQuietHoursSchedule: options.DeploymentValues.UserQuietHoursSchedule.DefaultSchedule.Value(), ProvisionerDaemonPSK: options.DeploymentValues.Provisioner.DaemonPSK.Value(), - CheckInactiveUsersCancelFunc: dormancy.CheckInactiveUsers(ctx, options.Logger, options.Database, options.Auditor), + CheckInactiveUsersCancelFunc: dormancy.CheckInactiveUsers(ctx, options.Logger, quartz.NewReal(), options.Database, options.Auditor), } if encKeys := options.DeploymentValues.ExternalTokenEncryptionKeys.Value(); len(encKeys) != 0 { diff --git a/enterprise/coderd/dormancy/dormantusersjob.go b/enterprise/coderd/dormancy/dormantusersjob.go index 343beb77eba8d..dbc4bd9b0b2e8 100644 --- a/enterprise/coderd/dormancy/dormantusersjob.go +++ b/enterprise/coderd/dormancy/dormantusersjob.go @@ -3,7 +3,6 @@ package dormancy import ( "context" "database/sql" - "encoding/json" "net/http" "time" @@ -14,6 +13,7 @@ import ( "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbtime" + "github.com/coder/quartz" ) const ( @@ -25,71 +25,49 @@ const ( // CheckInactiveUsers function updates status of inactive users from active to dormant // using default parameters. -func CheckInactiveUsers(ctx context.Context, logger slog.Logger, db database.Store, auditor audit.Auditor) func() { - return CheckInactiveUsersWithOptions(ctx, logger, db, auditor, jobInterval, accountDormancyPeriod) +func CheckInactiveUsers(ctx context.Context, logger slog.Logger, clk quartz.Clock, db database.Store, auditor audit.Auditor) func() { + return CheckInactiveUsersWithOptions(ctx, logger, clk, db, auditor, jobInterval, accountDormancyPeriod) } // CheckInactiveUsersWithOptions function updates status of inactive users from active to dormant // using provided parameters. -func CheckInactiveUsersWithOptions(ctx context.Context, logger slog.Logger, db database.Store, auditor audit.Auditor, checkInterval, dormancyPeriod time.Duration) func() { +func CheckInactiveUsersWithOptions(ctx context.Context, logger slog.Logger, clk quartz.Clock, db database.Store, auditor audit.Auditor, checkInterval, dormancyPeriod time.Duration) func() { logger = logger.Named("dormancy") ctx, cancelFunc := context.WithCancel(ctx) - done := make(chan struct{}) - ticker := time.NewTicker(checkInterval) - go func() { - defer close(done) - defer ticker.Stop() - for { - select { - case <-ctx.Done(): - return - case <-ticker.C: - } + ticker := clk.TickerFunc(ctx, checkInterval, func() error { + startTime := time.Now() + lastSeenAfter := dbtime.Now().Add(-dormancyPeriod) + logger.Debug(ctx, "check inactive user accounts", slog.F("dormancy_period", dormancyPeriod), slog.F("last_seen_after", lastSeenAfter)) - startTime := time.Now() - lastSeenAfter := dbtime.Now().Add(-dormancyPeriod) - logger.Debug(ctx, "check inactive user accounts", slog.F("dormancy_period", dormancyPeriod), slog.F("last_seen_after", lastSeenAfter)) + updatedUsers, err := db.UpdateInactiveUsersToDormant(ctx, database.UpdateInactiveUsersToDormantParams{ + LastSeenAfter: lastSeenAfter, + UpdatedAt: dbtime.Now(), + }) + if err != nil && !xerrors.Is(err, sql.ErrNoRows) { + logger.Error(ctx, "can't mark inactive users as dormant", slog.Error(err)) + return nil + } - updatedUsers, err := db.UpdateInactiveUsersToDormant(ctx, database.UpdateInactiveUsersToDormantParams{ - LastSeenAfter: lastSeenAfter, - UpdatedAt: dbtime.Now(), + for _, u := range updatedUsers { + logger.Info(ctx, "account has been marked as dormant", slog.F("email", u.Email), slog.F("last_seen_at", u.LastSeenAt)) + audit.BackgroundAudit(ctx, &audit.BackgroundAuditParams[database.User]{ + Audit: auditor, + Log: logger, + UserID: u.ID, + Action: database.AuditActionWrite, + Old: database.User{ID: u.ID, Username: u.Username, Status: database.UserStatusActive}, + New: database.User{ID: u.ID, Username: u.Username, Status: database.UserStatusDormant}, + Status: http.StatusOK, + AdditionalFields: audit.BackgroundTaskFields(ctx, logger, audit.BackgroundSubsystemDormancy), }) - if err != nil && !xerrors.Is(err, sql.ErrNoRows) { - logger.Error(ctx, "can't mark inactive users as dormant", slog.Error(err)) - continue - } - - af := map[string]string{ - "automatic_actor": "coder", - "automatic_subsystem": "dormancy", - } - - wriBytes, err := json.Marshal(af) - if err != nil { - logger.Error(ctx, "marshal additional fields", slog.Error(err)) - wriBytes = []byte("{}") - } - - for _, u := range updatedUsers { - logger.Info(ctx, "account has been marked as dormant", slog.F("email", u.Email), slog.F("last_seen_at", u.LastSeenAt)) - audit.BackgroundAudit(ctx, &audit.BackgroundAuditParams[database.User]{ - Audit: auditor, - Log: logger, - UserID: u.ID, - Action: database.AuditActionWrite, - Old: database.User{ID: u.ID, Username: u.Username, Status: database.UserStatusActive}, - New: database.User{ID: u.ID, Username: u.Username, Status: database.UserStatusDormant}, - Status: http.StatusOK, - AdditionalFields: wriBytes, - }) - } - logger.Debug(ctx, "checking user accounts is done", slog.F("num_dormant_accounts", len(updatedUsers)), slog.F("execution_time", time.Since(startTime))) } - }() + logger.Debug(ctx, "checking user accounts is done", slog.F("num_dormant_accounts", len(updatedUsers)), slog.F("execution_time", time.Since(startTime))) + return nil + }) return func() { cancelFunc() - <-done + ticker.Wait() } } diff --git a/enterprise/coderd/dormancy/dormantusersjob_test.go b/enterprise/coderd/dormancy/dormantusersjob_test.go index c0663b83f20f2..bb3e0b4170baf 100644 --- a/enterprise/coderd/dormancy/dormantusersjob_test.go +++ b/enterprise/coderd/dormancy/dormantusersjob_test.go @@ -14,7 +14,7 @@ import ( "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbmem" "github.com/coder/coder/v2/enterprise/coderd/dormancy" - "github.com/coder/coder/v2/testutil" + "github.com/coder/quartz" ) func TestCheckInactiveUsers(t *testing.T) { @@ -44,29 +44,31 @@ func TestCheckInactiveUsers(t *testing.T) { suspendedUser3 := setupUser(ctx, t, db, "suspended-user-3@coder.com", database.UserStatusSuspended, time.Now().Add(-dormancyPeriod).Add(-6*time.Hour)) mAudit := audit.NewMock() + mClock := quartz.NewMock(t) // Run the periodic job - closeFunc := dormancy.CheckInactiveUsersWithOptions(ctx, logger, db, mAudit, interval, dormancyPeriod) + closeFunc := dormancy.CheckInactiveUsersWithOptions(ctx, logger, mClock, db, mAudit, interval, dormancyPeriod) t.Cleanup(closeFunc) - var rows []database.GetUsersRow - var err error - require.Eventually(t, func() bool { - rows, err = db.GetUsers(ctx, database.GetUsersParams{}) - if err != nil { - return false - } + dur, w := mClock.AdvanceNext() + require.Equal(t, interval, dur) + w.MustWait(ctx) + + rows, err := db.GetUsers(ctx, database.GetUsersParams{}) + require.NoError(t, err) - var dormant, suspended int - for _, row := range rows { - if row.Status == database.UserStatusDormant { - dormant++ - } else if row.Status == database.UserStatusSuspended { - suspended++ - } + var dormant, suspended int + for _, row := range rows { + if row.Status == database.UserStatusDormant { + dormant++ + } else if row.Status == database.UserStatusSuspended { + suspended++ } - // 6 users in total, 3 dormant, 3 suspended - return len(rows) == 9 && dormant == 3 && suspended == 3 - }, testutil.WaitShort, testutil.IntervalMedium) + } + + // 9 users in total, 3 active, 3 dormant, 3 suspended + require.Len(t, rows, 9) + require.Equal(t, 3, dormant) + require.Equal(t, 3, suspended) require.Len(t, mAudit.AuditLogs(), 3) From b6c043dd43d4bb959ce53629e261b7aefff2cc9a Mon Sep 17 00:00:00 2001 From: Colin Adler Date: Thu, 31 Oct 2024 20:15:51 +0000 Subject: [PATCH 11/14] fixup! feat: add audit logs for dormancy events --- enterprise/coderd/dormancy/dormantusersjob.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/enterprise/coderd/dormancy/dormantusersjob.go b/enterprise/coderd/dormancy/dormantusersjob.go index dbc4bd9b0b2e8..21e84cdcae2ed 100644 --- a/enterprise/coderd/dormancy/dormantusersjob.go +++ b/enterprise/coderd/dormancy/dormantusersjob.go @@ -68,6 +68,6 @@ func CheckInactiveUsersWithOptions(ctx context.Context, logger slog.Logger, clk return func() { cancelFunc() - ticker.Wait() + _ = ticker.Wait() } } From b3269d18f14e32be68364bf5a778ca050988854b Mon Sep 17 00:00:00 2001 From: Colin Adler Date: Thu, 31 Oct 2024 21:13:04 +0000 Subject: [PATCH 12/14] fixup! feat: add audit logs for dormancy events --- coderd/coderd.go | 2 +- coderd/httpmw/apikey.go | 6 +- coderd/userauth.go | 95 +++++++++++++------ enterprise/coderd/coderd.go | 2 +- enterprise/coderd/dormancy/dormantusersjob.go | 4 +- 5 files changed, 71 insertions(+), 38 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index b50b50940b0e4..70101b7020890 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -702,7 +702,7 @@ func New(options *Options) *API { apiKeyMiddleware := httpmw.ExtractAPIKeyMW(httpmw.ExtractAPIKeyConfig{ DB: options.Database, - HandleDormancy: api.ActivateDormantUser, + ActivateDormantUser: ActivateDormantUser(options.Logger, &api.Auditor, options.Database), OAuth2Configs: oauthConfigs, RedirectToLogin: false, DisableSessionExpiryRefresh: options.DeploymentValues.Sessions.DisableExpiryRefresh.Value(), diff --git a/coderd/httpmw/apikey.go b/coderd/httpmw/apikey.go index f8e3c5a738739..ae5d79589e277 100644 --- a/coderd/httpmw/apikey.go +++ b/coderd/httpmw/apikey.go @@ -82,7 +82,7 @@ const ( type ExtractAPIKeyConfig struct { DB database.Store - HandleDormancy func(ctx context.Context, u database.User) database.User + ActivateDormantUser func(ctx context.Context, u database.User) database.User OAuth2Configs *OAuth2Configs RedirectToLogin bool DisableSessionExpiryRefresh bool @@ -415,9 +415,9 @@ func ExtractAPIKey(rw http.ResponseWriter, r *http.Request, cfg ExtractAPIKeyCon }) } - if userStatus == database.UserStatusDormant && cfg.HandleDormancy != nil { + if userStatus == database.UserStatusDormant && cfg.ActivateDormantUser != nil { id, _ := uuid.Parse(actor.ID) - cfg.HandleDormancy(ctx, database.User{ + cfg.ActivateDormantUser(ctx, database.User{ ID: id, Username: actor.FriendlyName, Status: userStatus, diff --git a/coderd/userauth.go b/coderd/userauth.go index 885f2c5a75e54..c02e4b1a69baf 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -12,6 +12,7 @@ import ( "strconv" "strings" "sync" + "sync/atomic" "time" "github.com/coreos/go-oidc/v3/oidc" @@ -566,7 +567,7 @@ func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req co return user, rbac.Subject{}, false } - user = api.ActivateDormantUser(ctx, user) + user = ActivateDormantUser(api.Logger, &api.Auditor, api.Database)(ctx, user) subject, userStatus, err := httpmw.UserRBACSubject(ctx, api.Database, user.ID, rbac.ScopeAll) if err != nil { @@ -588,34 +589,36 @@ func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req co return user, subject, true } -func (api *API) ActivateDormantUser(ctx context.Context, user database.User) database.User { - if user.ID == uuid.Nil || user.Status != database.UserStatusDormant { - return user - } +func ActivateDormantUser(logger slog.Logger, auditor *atomic.Pointer[audit.Auditor], db database.Store) func(ctx context.Context, user database.User) database.User { + return func(ctx context.Context, user database.User) database.User { + if user.ID == uuid.Nil || user.Status != database.UserStatusDormant { + return user + } - //nolint:gocritic // System needs to update status of the user account (dormant -> active). - newUser, err := api.Database.UpdateUserStatus(dbauthz.AsSystemRestricted(ctx), database.UpdateUserStatusParams{ - ID: user.ID, - Status: database.UserStatusActive, - UpdatedAt: dbtime.Now(), - }) - if err != nil { - api.Logger.Error(ctx, "unable to update user status to active", slog.Error(err)) - return user - } - - audit.BackgroundAudit(ctx, &audit.BackgroundAuditParams[database.User]{ - Audit: *api.Auditor.Load(), - Log: api.Logger, - UserID: user.ID, - Action: database.AuditActionWrite, - Old: user, - New: newUser, - Status: http.StatusOK, - AdditionalFields: audit.BackgroundTaskFields(ctx, api.Logger, audit.BackgroundSubsystemDormancy), - }) + //nolint:gocritic // System needs to update status of the user account (dormant -> active). + newUser, err := db.UpdateUserStatus(dbauthz.AsSystemRestricted(ctx), database.UpdateUserStatusParams{ + ID: user.ID, + Status: database.UserStatusActive, + UpdatedAt: dbtime.Now(), + }) + if err != nil { + logger.Error(ctx, "unable to update user status to active", slog.Error(err)) + return user + } + + audit.BackgroundAudit(ctx, &audit.BackgroundAuditParams[database.User]{ + Audit: *auditor.Load(), + Log: logger, + UserID: user.ID, + Action: database.AuditActionWrite, + Old: user, + New: newUser, + Status: http.StatusOK, + AdditionalFields: audit.BackgroundTaskFields(ctx, logger, audit.BackgroundSubsystemDormancy), + }) - return newUser + return newUser + } } // Clear the user's session cookie. @@ -1402,12 +1405,23 @@ func (p *oauthLoginParams) CommitAuditLogs() { func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.Cookie, database.User, database.APIKey, error) { var ( - ctx = r.Context() - user database.User - cookies []*http.Cookie + ctx = r.Context() + user database.User + cookies []*http.Cookie + logger = api.Logger.Named(userAuthLoggerName) + auditor = *api.Auditor.Load() + dormantConvertAudit *audit.Request[database.User] + initDormantAuditOnce = sync.OnceFunc(func() { + dormantConvertAudit = params.initAuditRequest(&audit.RequestParams{ + Audit: auditor, + Log: api.Logger, + Request: r, + Action: database.AuditActionWrite, + }) + }) ) - params.User = api.ActivateDormantUser(ctx, params.User) + params.User = ActivateDormantUser(api.Logger, &api.Auditor, api.Database)(ctx, params.User) var isConvertLoginType bool err := api.Database.InTx(func(tx database.Store) error { @@ -1518,6 +1532,25 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C } } + // Activate dormant user on sign-in + if user.Status == database.UserStatusDormant { + // This is necessary because transactions can be retried, and we + // only want to add the audit log a single time. + initDormantAuditOnce() + dormantConvertAudit.Old = user + //nolint:gocritic // System needs to update status of the user account (dormant -> active). + user, err = tx.UpdateUserStatus(dbauthz.AsSystemRestricted(ctx), database.UpdateUserStatusParams{ + ID: user.ID, + Status: database.UserStatusActive, + UpdatedAt: dbtime.Now(), + }) + if err != nil { + logger.Error(ctx, "unable to update user status to active", slog.Error(err)) + return xerrors.Errorf("update user status: %w", err) + } + dormantConvertAudit.New = user + } + debugContext, err := json.Marshal(params.DebugContext) if err != nil { return xerrors.Errorf("marshal debug context: %w", err) diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index eb5b6df433c1f..dddf619b34058 100644 --- a/enterprise/coderd/coderd.go +++ b/enterprise/coderd/coderd.go @@ -172,7 +172,7 @@ func New(ctx context.Context, options *Options) (_ *API, err error) { } apiKeyMiddleware := httpmw.ExtractAPIKeyMW(httpmw.ExtractAPIKeyConfig{ DB: options.Database, - HandleDormancy: api.AGPL.ActivateDormantUser, + ActivateDormantUser: coderd.ActivateDormantUser(options.Logger, &api.AGPL.Auditor, options.Database), OAuth2Configs: oauthConfigs, RedirectToLogin: false, DisableSessionExpiryRefresh: options.DeploymentValues.Sessions.DisableExpiryRefresh.Value(), diff --git a/enterprise/coderd/dormancy/dormantusersjob.go b/enterprise/coderd/dormancy/dormantusersjob.go index 21e84cdcae2ed..c636cdc851481 100644 --- a/enterprise/coderd/dormancy/dormantusersjob.go +++ b/enterprise/coderd/dormancy/dormantusersjob.go @@ -35,7 +35,7 @@ func CheckInactiveUsersWithOptions(ctx context.Context, logger slog.Logger, clk logger = logger.Named("dormancy") ctx, cancelFunc := context.WithCancel(ctx) - ticker := clk.TickerFunc(ctx, checkInterval, func() error { + tf := clk.TickerFunc(ctx, checkInterval, func() error { startTime := time.Now() lastSeenAfter := dbtime.Now().Add(-dormancyPeriod) logger.Debug(ctx, "check inactive user accounts", slog.F("dormancy_period", dormancyPeriod), slog.F("last_seen_after", lastSeenAfter)) @@ -68,6 +68,6 @@ func CheckInactiveUsersWithOptions(ctx context.Context, logger slog.Logger, clk return func() { cancelFunc() - _ = ticker.Wait() + _ = tf.Wait() } } From 3829773acb6de29defe873a2a59e1a0ac563bf54 Mon Sep 17 00:00:00 2001 From: Colin Adler Date: Thu, 31 Oct 2024 21:23:48 +0000 Subject: [PATCH 13/14] fixup! feat: add audit logs for dormancy events --- coderd/httpmw/apikey.go | 11 +++++++++-- coderd/userauth.go | 30 ++++++++++++++++++------------ 2 files changed, 27 insertions(+), 14 deletions(-) diff --git a/coderd/httpmw/apikey.go b/coderd/httpmw/apikey.go index ae5d79589e277..f6746b95eb20e 100644 --- a/coderd/httpmw/apikey.go +++ b/coderd/httpmw/apikey.go @@ -82,7 +82,7 @@ const ( type ExtractAPIKeyConfig struct { DB database.Store - ActivateDormantUser func(ctx context.Context, u database.User) database.User + ActivateDormantUser func(ctx context.Context, u database.User) (database.User, error) OAuth2Configs *OAuth2Configs RedirectToLogin bool DisableSessionExpiryRefresh bool @@ -417,11 +417,18 @@ func ExtractAPIKey(rw http.ResponseWriter, r *http.Request, cfg ExtractAPIKeyCon if userStatus == database.UserStatusDormant && cfg.ActivateDormantUser != nil { id, _ := uuid.Parse(actor.ID) - cfg.ActivateDormantUser(ctx, database.User{ + user, err := cfg.ActivateDormantUser(ctx, database.User{ ID: id, Username: actor.FriendlyName, Status: userStatus, }) + if err != nil { + return write(http.StatusInternalServerError, codersdk.Response{ + Message: internalErrorMessage, + Detail: fmt.Sprintf("update user status: %s", err.Error()), + }) + } + userStatus = user.Status } if userStatus != database.UserStatusActive { diff --git a/coderd/userauth.go b/coderd/userauth.go index c02e4b1a69baf..bc5a45f29ff0e 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -567,7 +567,14 @@ func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req co return user, rbac.Subject{}, false } - user = ActivateDormantUser(api.Logger, &api.Auditor, api.Database)(ctx, user) + user, err = ActivateDormantUser(api.Logger, &api.Auditor, api.Database)(ctx, user) + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error.", + Detail: err.Error(), + }) + return user, rbac.Subject{}, false + } subject, userStatus, err := httpmw.UserRBACSubject(ctx, api.Database, user.ID, rbac.ScopeAll) if err != nil { @@ -589,10 +596,10 @@ func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req co return user, subject, true } -func ActivateDormantUser(logger slog.Logger, auditor *atomic.Pointer[audit.Auditor], db database.Store) func(ctx context.Context, user database.User) database.User { - return func(ctx context.Context, user database.User) database.User { +func ActivateDormantUser(logger slog.Logger, auditor *atomic.Pointer[audit.Auditor], db database.Store) func(ctx context.Context, user database.User) (database.User, error) { + return func(ctx context.Context, user database.User) (database.User, error) { if user.ID == uuid.Nil || user.Status != database.UserStatusDormant { - return user + return user, nil } //nolint:gocritic // System needs to update status of the user account (dormant -> active). @@ -603,7 +610,7 @@ func ActivateDormantUser(logger slog.Logger, auditor *atomic.Pointer[audit.Audit }) if err != nil { logger.Error(ctx, "unable to update user status to active", slog.Error(err)) - return user + return user, xerrors.Errorf("update user status: %w", err) } audit.BackgroundAudit(ctx, &audit.BackgroundAuditParams[database.User]{ @@ -617,7 +624,7 @@ func ActivateDormantUser(logger slog.Logger, auditor *atomic.Pointer[audit.Audit AdditionalFields: audit.BackgroundTaskFields(ctx, logger, audit.BackgroundSubsystemDormancy), }) - return newUser + return newUser, nil } } @@ -1413,16 +1420,15 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C dormantConvertAudit *audit.Request[database.User] initDormantAuditOnce = sync.OnceFunc(func() { dormantConvertAudit = params.initAuditRequest(&audit.RequestParams{ - Audit: auditor, - Log: api.Logger, - Request: r, - Action: database.AuditActionWrite, + Audit: auditor, + Log: api.Logger, + Request: r, + Action: database.AuditActionWrite, + OrganizationID: uuid.Nil, }) }) ) - params.User = ActivateDormantUser(api.Logger, &api.Auditor, api.Database)(ctx, params.User) - var isConvertLoginType bool err := api.Database.InTx(func(tx database.Store) error { var ( From 6fdb233058523c7cee71e0f96e7477326928e40f Mon Sep 17 00:00:00 2001 From: Colin Adler Date: Thu, 31 Oct 2024 22:25:49 +0000 Subject: [PATCH 14/14] fix oidc, add test --- coderd/audit/fields.go | 8 +++- coderd/userauth.go | 22 +++++---- coderd/userauth_test.go | 45 ++++++++++++++++++- enterprise/coderd/dormancy/dormantusersjob.go | 2 +- 4 files changed, 65 insertions(+), 12 deletions(-) diff --git a/coderd/audit/fields.go b/coderd/audit/fields.go index 3f12bc9fa8f8b..db0879730425a 100644 --- a/coderd/audit/fields.go +++ b/coderd/audit/fields.go @@ -13,11 +13,15 @@ const ( BackgroundSubsystemDormancy BackgroundSubsystem = "dormancy" ) -func BackgroundTaskFields(ctx context.Context, logger slog.Logger, subsystem BackgroundSubsystem) []byte { - af := map[string]string{ +func BackgroundTaskFields(subsystem BackgroundSubsystem) map[string]string { + return map[string]string{ "automatic_actor": "coder", "automatic_subsystem": string(subsystem), } +} + +func BackgroundTaskFieldsBytes(ctx context.Context, logger slog.Logger, subsystem BackgroundSubsystem) []byte { + af := BackgroundTaskFields(subsystem) wriBytes, err := json.Marshal(af) if err != nil { diff --git a/coderd/userauth.go b/coderd/userauth.go index bc5a45f29ff0e..1dc399e1ac3b5 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -613,15 +613,19 @@ func ActivateDormantUser(logger slog.Logger, auditor *atomic.Pointer[audit.Audit return user, xerrors.Errorf("update user status: %w", err) } + oldAuditUser := user + newAuditUser := user + newAuditUser.Status = database.UserStatusActive + audit.BackgroundAudit(ctx, &audit.BackgroundAuditParams[database.User]{ Audit: *auditor.Load(), Log: logger, UserID: user.ID, Action: database.AuditActionWrite, - Old: user, - New: newUser, + Old: oldAuditUser, + New: newAuditUser, Status: http.StatusOK, - AdditionalFields: audit.BackgroundTaskFields(ctx, logger, audit.BackgroundSubsystemDormancy), + AdditionalFields: audit.BackgroundTaskFieldsBytes(ctx, logger, audit.BackgroundSubsystemDormancy), }) return newUser, nil @@ -1420,11 +1424,12 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C dormantConvertAudit *audit.Request[database.User] initDormantAuditOnce = sync.OnceFunc(func() { dormantConvertAudit = params.initAuditRequest(&audit.RequestParams{ - Audit: auditor, - Log: api.Logger, - Request: r, - Action: database.AuditActionWrite, - OrganizationID: uuid.Nil, + Audit: auditor, + Log: api.Logger, + Request: r, + Action: database.AuditActionWrite, + OrganizationID: uuid.Nil, + AdditionalFields: audit.BackgroundTaskFields(audit.BackgroundSubsystemDormancy), }) }) ) @@ -1543,6 +1548,7 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C // This is necessary because transactions can be retried, and we // only want to add the audit log a single time. initDormantAuditOnce() + dormantConvertAudit.UserID = user.ID dormantConvertAudit.Old = user //nolint:gocritic // System needs to update status of the user account (dormant -> active). user, err = tx.UpdateUserStatus(dbauthz.AsSystemRestricted(ctx), database.UpdateUserStatusParams{ diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index 6386be7eb8be4..843f8ec753133 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -1285,7 +1285,7 @@ func TestUserOIDC(t *testing.T) { tc.AssertResponse(t, resp) } - ctx := testutil.Context(t, testutil.WaitLong) + ctx := testutil.Context(t, testutil.WaitShort) if tc.AssertUser != nil { user, err := client.User(ctx, "me") @@ -1300,6 +1300,49 @@ func TestUserOIDC(t *testing.T) { }) } + t.Run("OIDCDormancy", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + + auditor := audit.NewMock() + fake := oidctest.NewFakeIDP(t, + oidctest.WithRefresh(func(_ string) error { + return xerrors.New("refreshing token should never occur") + }), + oidctest.WithServing(), + ) + cfg := fake.OIDCConfig(t, nil, func(cfg *coderd.OIDCConfig) { + cfg.AllowSignups = true + }) + + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug) + owner, db := coderdtest.NewWithDatabase(t, &coderdtest.Options{ + Auditor: auditor, + OIDCConfig: cfg, + Logger: &logger, + }) + + user := dbgen.User(t, db, database.User{ + LoginType: database.LoginTypeOIDC, + Status: database.UserStatusDormant, + }) + auditor.ResetLogs() + + client, resp := fake.AttemptLogin(t, owner, jwt.MapClaims{ + "email": user.Email, + }) + require.Equal(t, http.StatusOK, resp.StatusCode) + + auditor.Contains(t, database.AuditLog{ + ResourceType: database.ResourceTypeUser, + AdditionalFields: json.RawMessage(`{"automatic_actor":"coder","automatic_subsystem":"dormancy"}`), + }) + me, err := client.User(ctx, "me") + require.NoError(t, err) + + require.Equal(t, codersdk.UserStatusActive, me.Status) + }) + t.Run("OIDCConvert", func(t *testing.T) { t.Parallel() diff --git a/enterprise/coderd/dormancy/dormantusersjob.go b/enterprise/coderd/dormancy/dormantusersjob.go index c636cdc851481..cae442ce07507 100644 --- a/enterprise/coderd/dormancy/dormantusersjob.go +++ b/enterprise/coderd/dormancy/dormantusersjob.go @@ -59,7 +59,7 @@ func CheckInactiveUsersWithOptions(ctx context.Context, logger slog.Logger, clk Old: database.User{ID: u.ID, Username: u.Username, Status: database.UserStatusActive}, New: database.User{ID: u.ID, Username: u.Username, Status: database.UserStatusDormant}, Status: http.StatusOK, - AdditionalFields: audit.BackgroundTaskFields(ctx, logger, audit.BackgroundSubsystemDormancy), + AdditionalFields: audit.BackgroundTaskFieldsBytes(ctx, logger, audit.BackgroundSubsystemDormancy), }) } logger.Debug(ctx, "checking user accounts is done", slog.F("num_dormant_accounts", len(updatedUsers)), slog.F("execution_time", time.Since(startTime)))