feat(auth): Add structured logging for AuthIdentity updates and deletes#108688
feat(auth): Add structured logging for AuthIdentity updates and deletes#108688michelletran-sentry merged 2 commits intomasterfrom
Conversation
Security-relevant changes to SSO identities (user reassignment, ident changes, data updates, deletions) were happening silently. Override `update()` and `delete()` on `AuthIdentity` to emit `logger.info` for meaningful field changes, filtering out timestamp-only updates. Co-Authored-By: Claude <[email protected]>
github-actions
bot
added
the
Scope: Backend
|
|
||
| logger = logging.getLogger("sentry.auth.identity") | ||
|
|
||
| _MEANINGFUL_UPDATE_FIELDS = frozenset({"user", "user_id", "ident", "data"}) |
There was a problem hiding this comment.
@geoffg-sentry Is there anything else that you think we should log here?
There was a problem hiding this comment.
We don't expose other meta for these. Maybe token_type and expiry but hard to image that would be helpful, since it's almost always type bearer with an expected short expiry anyway. Looks good!
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Capture user_id before super().update() so the log always reflects the prior owner. When user reassignment occurs, a new_user_id field is added to identify the target user. Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| if self.user_id != old_user_id: | ||
| extra["new_user_id"] = self.user_id | ||
| logger.info("auth_identity.update", extra=extra) | ||
| return result |
There was a problem hiding this comment.
Update logs even when values unchanged
Medium Severity
AuthIdentity.update() logs whenever a “meaningful” field appears in kwds, even if the new value equals the existing value (e.g., re-saving identical data/ident). This can generate misleading security/audit logs that claim an identity change occurred when nothing actually changed.
There was a problem hiding this comment.
Hmm... we can see how noisy it is before we do more to tune the logs.
| if self.user_id != old_user_id: | ||
| extra["new_user_id"] = self.user_id | ||
| logger.info("auth_identity.update", extra=extra) | ||
| return result |
There was a problem hiding this comment.
Update logs despite failed DB update
Low Severity
AuthIdentity.update() can log an update even when super().update() affects 0 rows (e.g., the row was deleted concurrently), because logging is gated only on changed_fields, not on the returned affected-row count.
…es (#108688) Security-relevant changes to SSO identities (user reassignment, ident changes, data updates, deletions) were happening silently. Override `update()` and `delete()` on `AuthIdentity` to emit `logger.info` for meaningful field changes, filtering out timestamp-only updates. I'm a bit unsure if this will be too noisy, but I suspect that identity changes aren't common(?) --------- Co-authored-by: Claude <[email protected]>
…es (#108688) Security-relevant changes to SSO identities (user reassignment, ident changes, data updates, deletions) were happening silently. Override `update()` and `delete()` on `AuthIdentity` to emit `logger.info` for meaningful field changes, filtering out timestamp-only updates. I'm a bit unsure if this will be too noisy, but I suspect that identity changes aren't common(?) --------- Co-authored-by: Claude <[email protected]>
2 participants
Security-relevant changes to SSO identities (user reassignment, ident changes, data updates, deletions) were happening silently. Override
update()anddelete()onAuthIdentityto emitlogger.infofor meaningful field changes, filtering out timestamp-only updates.I'm a bit unsure if this will be too noisy, but I suspect that identity changes aren't common(?)