feat: add audit logs for dormancy events #15298
Conversation
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.
There was 10000 a problem hiding this comment.
We're dropping a login audit log when a user logs in via OIDC which I suppose captures the diff of a dormant user going to active. But we're not dropping an audit log event specifically for when their user status is being updated from dormant to active that I can tell. Seems like we should be dropping an extra one with a write action to be consistent with the builtin logs you added. It's also consistent with us dropping extra audit logs for special cases like when we convert the user type.
| @@ -197,6 +197,7 @@ func (r *RootCmd) newCreateAdminUserCommand() *serpent.Command { | |||
| UpdatedAt: dbtime.Now(), | |||
| RBACRoles: []string{rbac.RoleOwner().String()}, | |||
| LoginType: database.LoginTypePassword, | |||
| Status: "", | |||
There was a problem hiding this comment.
why do we pass "" here? Should we just pass active?
There was a problem hiding this comment.
Ideally I would omit it but the linter doesn't like leaving it out. It doesn't really matter what this is created as, so "" just does the default behavior which is dormant.
| ($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 *; |
There was a problem hiding this comment.
why do we handle this as a special case? Shouldn't we expect the status to be provided?
There was a problem hiding this comment.
I didn't really want to leak the previous default behavior out into the API code. I think it makes more sense to do the default behavior here, which is what was happening before when it wasn't being inserted.
coderd/userauth.go
Outdated
| wriBytes = []byte("{}") | ||
| } | ||
|
|
||
| audit.BackgroundAudit(ctx, &audit.BackgroundAuditParams[database.User]{ |
There was a problem hiding this comment.
This doesn't audit if we fail to update the user status. Also why is this a background audit if it's part of an API request?
There was a problem hiding this comment.
Personally I don't really think we get anything valuable from generating an audit log if this fails. The main reason for failed audit logs is for logging users attempting to take an action for which they don't have permission, or similar. The status failing to update here doesn't really matter, it can only ever be a 500 error. Plus, it'll be captured by the main login audit anyways.
I think BackgroundAudit is poorly named, it's really just for anything not specifically tied to a request. I don't think it make sense to use a request audit here because the status could successfully be changed while the api request as a whole fails. Also, since dormancy is really something Coder just does in the background, it maintains consistency. The user isn't really intentionally modifying their status.
| status := "" | ||
| if req.UserStatus != nil { | ||
| status = string(*req.UserStatus) | ||
| } |
There was a problem hiding this comment.
similarly when would we expect the status to be empty?
There was a problem hiding this comment.
Empty just means use the default behavior, which is dormant. It would be a breaking change if this was required
coderd/httpmw/apikey.go
Outdated
| @@ -82,6 +82,7 @@ const ( | |||
|
|
|||
| type ExtractAPIKeyConfig struct { | |||
| DB database.Store | |||
| HandleDormancy func(ctx context.Context, u database.User) database.User | |||
There was a problem hiding this comment.
I generally dislike these passthrough functions and given that we pass the same implementation in the enterprise path, is it reasonable to move this function off this config (and the API struct) and just have it as a package level function somewhere?
There was a problem hiding this comment.
Yeah the passthrough is necessary because we can't import audit in httpmw since audit already imports httpmw itself, so I have to result to this jank
| @@ -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() { | |||
There was a problem hiding this comment.
I don't even understand the point of this function if the other is also exported
There was a problem hiding this comment.
Seems like it's just to hardcode the dormancy period
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.
Closes #15277