- Notifications
You must be signed in to change notification settings - Fork927
feat: add audit logs for dormancy events#15298
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
An audit log will now be generated when Coder automatically modifiesuser statuses to dormant, or when a user logs in and are automaticallyconverted to active.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We're dropping alogin 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 fromdormant toactive that I can tell. Seems like we should be dropping an extra one with awrite action to be consistent with thebuiltin 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
why do we pass"" here? Should we just passactive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 thinkBackgroundAudit 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.
Uh oh!
There was an error while loading.Please reload this page.
| status := "" | ||
| if req.UserStatus != nil { | ||
| status = string(*req.UserStatus) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
similarly when would we expect the status to be empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Empty just means use the default behavior, which isdormant. It would be a breaking change if this was required
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I generally dislike these passthrough functions and given that we pass the same implementation in theenterprise 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
reasonable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah the passthrough is necessary because we can't importaudit inhttpmw sinceaudit already importshttpmw 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't even understand the point of this function if the other is also exported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Seems like it's just to hardcode the dormancy period
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
088f219 intomainUh oh!
There was an error while loading.Please reload this page.
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