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

Add impersonation to django admin log #44

Merged
merged 1 commit into from
Aug 10, 2023

Conversation

joseph-sentry
Copy link
Contributor

Purpose/Motivation

Links to relevant tickets

codecov/engineering-team#116

What does this PR do?

  • Create helper function to log to admin log entry
  • Log to admin log in impersonation middleware
  • Add __str__ method to User

Notes to Reviewer

Anything to note to the team? Any tips on how to review, or where to start?

Legal Boilerplate

Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.

@joseph-sentry joseph-sentry force-pushed the joseph/admin-log-impersonation branch from 0de2db8 to 58c0dca Compare July 28, 2023 15:15
@hootener
Copy link

hootener commented Jul 28, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (a649222) 95.20% compared to head (4ec4ed1) 95.21%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #44   +/-   ##
=======================================
  Coverage   95.20%   95.21%           
=======================================
  Files         578      578           
  Lines       14455    14474   +19     
=======================================
+ Hits        13762    13781   +19     
  Misses        693      693           
Flag Coverage Δ
unit 95.21% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
codecov_auth/middleware.py 100.00% <ø> (ø)
codecov_auth/admin.py 93.20% <100.00%> (+0.13%) ⬆️
codecov_auth/helpers.py 75.67% <100.00%> (+20.67%) ⬆️

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

@codecov
Copy link

codecov bot commented Jul 28, 2023

Codecov Report

Merging #44 (4ec4ed1) into main (a649222) will increase coverage by 0.01%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##            main     #44     +/-   ##
=======================================
+ Coverage   95.26   95.27   +0.01     
=======================================
  Files        691     691             
  Lines      14626   14645     +19     
=======================================
+ Hits       13933   13952     +19     
  Misses       693     693             
Flag Coverage Δ
unit 95.21% <100.00%> (+<0.01%) ⬆️
unit-latest-uploader 95.21% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
codecov_auth/middleware.py 100.00% <ø> (ø)
codecov_auth/admin.py 93.20% <100.00%> (+0.13%) ⬆️
codecov_auth/helpers.py 75.67% <100.00%> (+20.67%) ⬆️

@joseph-sentry joseph-sentry force-pushed the joseph/admin-log-impersonation branch 3 times, most recently from b960169 to 7f6bc18 Compare July 28, 2023 20:36
@joseph-sentry joseph-sentry linked an issue Jul 31, 2023 that may be closed by this pull request
@joseph-sentry joseph-sentry force-pushed the joseph/admin-log-impersonation branch 2 times, most recently from 2d0f8e3 to 04b7750 Compare August 2, 2023 14:45
@joseph-sentry joseph-sentry marked this pull request as ready for review August 2, 2023 15:09
Copy link
Contributor

@matt-codecov matt-codecov left a comment

Choose a reason for hiding this comment

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

accepting with feedback - please add comments or more test cases to illustrate what the arguments to log() are supposed to represent conceptually

Comment on lines 39 to 41
add = ADDITION
change = CHANGE
delete = DELETION
Copy link
Contributor

Choose a reason for hiding this comment

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

i think you may as well just use the pre-existing constants


class History:
@staticmethod
def log(objects, message, user, action_flag=None, add_traceback=False):
Copy link
Contributor

Choose a reason for hiding this comment

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

could you add a comment describing what these arguments are meant to be? particularly objects and action_flag.

looking at the tests, you are logging that orig_owner made a change related to impersonated_user, but that could also be a list of impersonated users? what are some other use cases of this that would want a list of objects?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This helper is just being used in the case where we're impersonating users, but in the future we might use this helper to log other actions in the admin log, some of those actions may involve multiple objects instead of just one.

@joseph-sentry joseph-sentry force-pushed the joseph/admin-log-impersonation branch 4 times, most recently from e15225a to b21ac0f Compare August 4, 2023 14:32
- Create History helper class to add logs to admin log
- Add History.log to log in impersonate_owner
- Add tests for History.log()

Signed-off-by: joseph-sentry <joseph.sawaya@sentry.io>
@joseph-sentry joseph-sentry merged commit 9e4eb01 into main Aug 10, 2023
11 of 12 checks passed
@joseph-sentry joseph-sentry deleted the joseph/admin-log-impersonation branch August 10, 2023 13:50
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.

Add impersonation to django_admin_log
3 participants