feat(ignore): adding event logger for ignored comments#178
feat(ignore): adding event logger for ignored comments#178Bill Li (billxinli) wants to merge 2 commits intomainfrom
Conversation
|
🚀 Preview package published! Install with: pip install --index-url https://test.pypi.org/simple/ --extra-index-url https://pypi.org/simple socketsecurity==2.2.83.dev1Docker image: |
0e20dd0 to
09e3172
Compare
|
bugbot run |
Bradley Farias (bmeck)
left a comment
There was a problem hiding this comment.
seems ok, would slightly pref v1 events but this is fine since v1 would be a slightly bigger change
| now = datetime.now(timezone.utc).isoformat() | ||
| shared_fields = { | ||
| "event_kind": "user-action", | ||
| "client_action": "ignore_alerts", |
There was a problem hiding this comment.
action fields should match ResolvedIssueAction, and this is missing the current action from policy (alert_action)
enum ResolvedIssueActionEnum {
error = 'error',
warn = 'warn',
monitor = 'monitor',
ignore = 'ignore',
}
aebda0d to
29e2c61
Compare
6e31308 to
ea4f245
Compare
John Hiesey (jhiesey)
left a comment
There was a problem hiding this comment.
A couple questions but otherwise looks good!
| log.debug(f"Flow decision: scm={scm is not None}, force_diff_mode={force_diff_mode}, force_api_mode={force_api_mode}, enable_diff={config.enable_diff}") | ||
|
|
||
| def _is_unprocessed(c): | ||
| """Check if an ignore comment has not yet been marked with '+1' reaction. |
There was a problem hiding this comment.
Is this the best way of marking comments processed? Seems a little easy for other human users to mess up. It's fine as a best-effort approach though
There was a problem hiding this comment.
This is mainly used to dedup the number of events logged.
first ignore foo message will cause the workflow to rerun with the cli, and it will log foo is being ignored
second ignore bar message will cause foo and bar to be logged, the +1 emoji attempts to deduplicate these logs, however, at the same time, it also means the user could immediately add the +1 emoji
| if not config.disable_ignore: | ||
| log.debug("Removing comment alerts") | ||
| alerts_before = list(diff.new_alerts) | ||
| diff.new_alerts = Comments.remove_alerts(comments, diff.new_alerts) |
There was a problem hiding this comment.
Why does this use Comments.remove_alerts but the above code path uses scm.remove_comment_alerts(comments)?
There was a problem hiding this comment.
My understanding of this is that there are two different flows.
Comment flow: Security comment already exists on the PR. User posts Socket (@SocketSecurity) ignore foo. The CLI re-processes the existing comment to remove foo rows.
Push flow: A new push happened. The CLI is about to create a new security comment. It filters alerts before generating the comment so ignored packages never appear.
one scm.remove_comment_alerts actually modify the comment as part of the PR/MR, while Comments.remove_alerts is just in memory filter
POST /v0/orgs/{slug}/telemetrywhen users suppress alerts via@SocketSecurity ignorePR/MR commentseyesemoji reaction as a dedup marker — only unprocessed comments (withouteyes) trigger events; after sending, theeyesreaction is addedsender_name/sender_idreactions.eyesfrom API response) and GitLab (lazyhas_eyes_reaction()API call per comment, best-effortpost_eyes_reaction()via Award Emoji API)artifact_input(raw user text) since the ignore command is user input and may not be a valid PURLartifact_purlfrom actual alert objects withalert_actionderived from the alert's resolved policy actionget_ignore_options—@socketSecurity ignore(lowercase s) now worksChanges
socketsecurity/core/cli_client.pypost_telemetry_events()— sends events individually toPOST /v0/orgs/{slug}/telemetrysocketsecurity/core/scm/github.pyis_commenter_authorized()— checks collaborator permission level (admin/write)post_eyes_reaction()— postseyesreaction to mark comments as processedpost_negative_reaction()— posts-1reaction for unauthorized usershandle_ignore_reactions()— only adds+1for authorized users,-1for unauthorizedsocketsecurity/core/scm/gitlab.pyis_commenter_authorized()— checks project member access level (Developer+)has_eyes_reaction()— best-effort check foreyesaward emoji on MR notespost_eyes_reaction()— best-effort addeyesaward emoji withContent-Type: application/jsonsocketsecurity/core/scm_comments.pyget_ignore_options()case-insensitive (fixes@socketSecurityvs@SocketSecurity)socketsecurity/socketcli.py_is_commenter_authorized()— checks user permissions with caching per user_is_unprocessed()— checks inlinereactions.eyes, falls back toscm.has_eyes_reaction(), then checks authorization_filter_authorized_ignore_comments()— filters ignore comments to authorized users only, applied before suppression and telemetry in both flowsalert_actionfrom alert's resolved policy flagsTests
tests/unit/test_client.py— 2 tests: individual event sending, continues on failureEvent attributes
{ "event_kind": "user-action", "client_action": "ignore", "alert_action": "error", "event_id": "<uuid>", "event_sender_created_at": "<iso8601>", "vcs_provider": "github|gitlab", "owner": "<repo_owner>", "repo": "<owner/repo>", "pr_number": 123, "ignore_all": true|false, "sender_name": "<comment_author_login>", "sender_id": "<comment_author_id>", "artifact_input": "<raw_user_text>" (comment flow), "artifact_purl": "<valid_purl>" (push flow) }Public Changelog
N/A