fix(audit): map identity_unlinked and reauthenticate to a log_type - #2683
Open
thanderoy wants to merge 1 commit into
Open
fix(audit): map identity_unlinked and reauthenticate to a log_type#2683thanderoy wants to merge 1 commit into
thanderoy wants to merge 1 commit into
Conversation
NewAuditLogEntry builds every payload with ActionLogTypeMap[action]. A missing key yields the zero value, so an unmapped action is persisted with an empty log_type rather than failing loudly. IdentityUnlinkAction and UserReauthenticateAction were both declared and both in use (internal/api/identity.go, internal/api/reauthenticate.go) but absent from the map, so their audit entries were written with log_type "" and could not be filtered alongside every other action. Map both to user, consistent with the passkey and user_modified actions. Add a test asserting the map covers every declared action, and that it holds no actions the package does not declare. The failure mode here is silent by construction, so it needs a test to stay fixed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
NewAuditLogEntrybuilds every audit payload with"log_type": ActionLogTypeMap[action](audit_log_entry.go#L110). Indexing a Go map with an absent key returns the zero value, so any action missing fromActionLogTypeMapis persisted withlog_type: ""instead of failing loudly.Two declared actions were missing from the map, and both are on live code paths:
identity_unlinkedaudit_log_entry.go#L44identity.go#L56user_reauthenticate_requestedaudit_log_entry.go#L32reauthenticate.go#L41The unlink and reauthenticate requests themselves complete correctly — only the audit record is affected. But those entries can't be filtered or attributed by
log_typethe way every other action can.Fixes #2679, which reports the
identity_unlinkedcase.user_reauthenticate_requestedhas the same root cause and is fixed here too.How
Both actions map to
user, consistent with how the other user-facing actions are already classified (user_modified,user_updated_password,passkey_created/passkey_updated/passkey_deleted).The remaining risk is recurrence: the failure mode is silent by construction, and nothing today connects declaring an action to mapping it. So this adds
internal/models/audit_log_entry_test.gowith two tests — every declared action resolves to a non-emptylog_type, and the map holds no action the package doesn't declare (which catches a stale entry after a rename). Both are pure unit tests; no database, no fixtures.Verified the test detects the original defect: reverting just the two map entries fails it on exactly
identity_unlinkedanduser_reauthenticate_requested, and it passes with them restored.Notes
log_type: "user"where they previously carried"". This restores the intended behaviour rather than changing an interface, but flagging it since anything filtering audit logs on the empty value will see different results.log_type: "". Happy to add one if you'd prefer historical entries corrected — it seemed out of scope for a fix this size, and I didn't want to assume a rewrite of existing audit records is wanted.allAuditActionsis hand-maintained. A new action has to be added to that slice as well as the map, so the test does not catch an action that's declared but never listed. The airtight alternative is a single exported registry the map and tests both derive from — a larger change to this file's shape, so I kept this PR minimal. Glad to follow up with it if you want that direction.make static,gosec,go vet, andgofmtagainstinternal/models— all clean. Pre-existing staticcheck hints elsewhere in the file (interface{}→any,QF1012) are untouched to keep the diff scoped.