Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(security): log user when access is blocked #16558

Merged

Conversation

crenshaw-dev
Copy link
Member

We currently log access errors, but we don't log who attempted the disallowed access. This just adds the username/email to the logs.

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
@crenshaw-dev crenshaw-dev requested a review from a team as a code owner December 6, 2023 18:19
Copy link

codecov bot commented Dec 6, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (86f79ec) 49.48% compared to head (d2cde51) 49.50%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #16558      +/-   ##
==========================================
+ Coverage   49.48%   49.50%   +0.02%     
==========================================
  Files         270      270              
  Lines       47489    47494       +5     
==========================================
+ Hits        23502    23514      +12     
+ Misses      21675    21670       -5     
+ Partials     2312     2310       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@reversearrow reversearrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be a configurable option instead of logging user details by default?

@crenshaw-dev
Copy link
Member Author

@reversearrow user info is already logged in the grpc claims on the request log. Both should be optional, but this PR doesn't introduce logged PII that wasn't otherwise there.

Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@gdsoumya gdsoumya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@crenshaw-dev crenshaw-dev merged commit 23e0d52 into argoproj:master Dec 18, 2023
25 checks passed
@crenshaw-dev crenshaw-dev deleted the log-user-on-enforcement-error branch December 18, 2023 14:41
JulienFuix pushed a commit to JulienFuix/argo-cd that referenced this pull request Feb 6, 2024
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
penglongli pushed a commit to penglongli/argo-cd that referenced this pull request Mar 6, 2024
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Signed-off-by: penglongli <pelenli@tencent.com>
lyda pushed a commit to lyda/argo-cd that referenced this pull request Mar 28, 2024
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Signed-off-by: Kevin Lyda <kevin@lyda.ie>
Hariharasuthan99 pushed a commit to AmadeusITGroup/argo-cd that referenced this pull request Jun 16, 2024
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants