Conversation
Test results 14 files 14 suites 25m 19s ⏱️ Results for commit 7caa7a8. ♻️ This comment has been updated with latest results. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3739 +/- ##
==========================================
+ Coverage 63.14% 63.58% +0.43%
==========================================
Files 614 614
Lines 45502 45764 +262
Branches 43 43
==========================================
+ Hits 28734 29097 +363
+ Misses 16758 16657 -101
Partials 10 10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
0da6f66 to
3ffaec4
Compare
154c77e to
bf15a09
Compare
johannaengland
left a comment
There was a problem hiding this comment.
When I try to run python tools/audit_auditlog.py fix -p delete-account with a copy of our production database I get the following error:
Traceback (most recent call last):
File "/workspaces/nav/tools/audit_auditlog.py", line 173, in <module>
main()
File "/workspaces/nav/tools/audit_auditlog.py", line 50, in main
repair_delete_account()
File "/workspaces/nav/tools/audit_auditlog.py", line 164, in repair_delete_account
free_ids = _find_unused_ids()
^^^^^^^^^^^^^^^^^^
File "/workspaces/nav/tools/audit_auditlog.py", line 134, in _find_unused_ids
max_id = max(used_ids)
^^^^^^^^^^^^^
TypeError: '>' not supported between instances of 'str' and 'int'
that happens because actor_ids are strings and account_ids are int
223198b to
6d8125b
Compare
.. and make it easy to mass-produce fixes and reports.
7c2a610 to
f305d27
Compare
|
johannaengland
left a comment
There was a problem hiding this comment.
When I try to run python tools/audit_auditlog.py fix -r delete-account-fix-object then I get the following traceback:
Traceback (most recent call last):
File "/workspaces/nav/tools/audit_auditlog.py", line 267, in <module>
main()
File "/workspaces/nav/tools/audit_auditlog.py", line 74, in main
elif args.repair in KNOWN_REPAIRS:
^^^^^^^^^^^
AttributeError: 'Namespace' object has no attribute 'repair'
|
|
||
|
|
||
| def get_lurkers(): | ||
| "Get a list of current accounts that have no actor entries in the audit log" |
There was a problem hiding this comment.
| "Get a list of current accounts that have no actor entries in the audit log" | |
| "Get a list of current accounts that have not been recorded as actors in the audit log" |
tools/audit_auditlog.py
Outdated
|
|
||
| def list_available_fixes(): | ||
| "List avaliable fixes" | ||
| print("delete-account: Attempts to set the pk of the object if it is missing") |
There was a problem hiding this comment.
| print("delete-account: Attempts to set the pk of the object if it is missing") | |
| print("delete-account: Attempts to set the pk of the object if it is missing to malformed delete-account entries") |
tools/audit_auditlog.py
Outdated
| ), | ||
| "-r", | ||
| "--report", | ||
| help="Generate and print the named report", |
There was a problem hiding this comment.
Just to be consistent - all should either start with a lowercase or uppercase letter
| help="Generate and print the named report", | |
| help="generate and print the named report", |
tools/audit_auditlog.py
Outdated
| "zombies", | ||
| ( | ||
| "List currently existing accounts that according to the audit log " | ||
| "should no longer exist." |
There was a problem hiding this comment.
| "should no longer exist." | |
| "should no longer exist" |
|
If this is a tool you intend for end-users to run manually to clean up broken auditlog history, then the answer is no. If it is purely meant as a tool for developers to manipulate data in their development installations, then the answer is yes. The point being: |
Absolutely not. I don't want to see something that automatically deletes user accounts without user intervention on upgrades. We cannot know the admin's current intent from looking at old, potentially broken or incorrect audit log entries. |
lunkwill42
left a comment
There was a problem hiding this comment.
I found this tool a bit confusing (it doesn't help that the PR includes commits that just refactor previous commits from the same PR, I did spend time commenting on stuff that was removed later).
If this is a tool that we want to make available to actual end users, I think we need some better documentation about what the actual issues are that this script is trying help fix. If it is not a tool for end users, what good is it really?
|
|
||
|
|
||
| def get_all_historical_actors(): | ||
| "List all recorded actors (need not be accounts!), including deleted ones" |
There was a problem hiding this comment.
What does this mean? What kind of actors will the audit log refer to that are not accounts?
And why only single quotes? PEP-257 (which we try to adopt in NAV) suggest to always use triple quotes, for consistency. I know there is some old code in NAV that doesn't follow it properly, but that's no excuse to drop it from new code :)
| actor = tuple( | ||
| filter(lambda x: x is not None, (entry.actor_model, entry.actor_pk)) | ||
| ) | ||
| obj = tuple( | ||
| filter(lambda x: x is not None, (entry.object_model, entry.object_pk)) | ||
| ) | ||
| target = tuple( | ||
| filter(lambda x: x is not None, (entry.target_model, entry.target_pk)) | ||
| ) |
There was a problem hiding this comment.
Why not just
| actor = tuple( | |
| filter(lambda x: x is not None, (entry.actor_model, entry.actor_pk)) | |
| ) | |
| obj = tuple( | |
| filter(lambda x: x is not None, (entry.object_model, entry.object_pk)) | |
| ) | |
| target = tuple( | |
| filter(lambda x: x is not None, (entry.target_model, entry.target_pk)) | |
| ) | |
| actor = tuple(x for x in (entry.actor_model, entry.actor_pk) if x is not None) | |
| obj = tuple(x for x in (entry.object_model, entry.object_pk) if x is not None) | |
| target = tuple(x for x in (entry.target_model, entry.target_pk) if x is not None) |
? (plus ruff-formatting, of course)
|
|
||
|
|
||
| def get_lurkers(): | ||
| "Get a list of current accounts that have no actor entries in the audit log" |
|
|
||
|
|
||
| def view_dump(): | ||
| "Print dump suitable for further processing" |
|
|
||
|
|
||
| def view_lurkers(): | ||
| "Print list of current accounts never recorded as actors in the audit log" |
|
|
||
|
|
||
| def delete_account_remove_zombies(): | ||
| "Delete accounts that have an entry in the object column of delete-account" |
| "List currently existing accounts that have not done anything " | ||
| "in this NAV instance" |
There was a problem hiding this comment.
| "List currently existing accounts that have not done anything " | |
| "in this NAV instance" | |
| "List currently existing accounts that have no logged activity in this NAV " | |
| "instance" |
| "Lurkers" are accounts that have no entries in the actor-column. | ||
| Their pk's are faked because there is no way to recover them. |
There was a problem hiding this comment.
I thought lurkers were accounts that exist but have never been logged as having had any activity?
Scope and purpose
Fixes #3729
Also adds some other useful tools for examining and using the auditlog.
Open questions:
Contributor Checklist
Every pull request should have this checklist filled out, no matter how small it is.
More information about contributing to NAV can be found in the
Hacker's guide to NAV.
<major>.<minor>.x). For a new feature or other additions, it should be based onmaster.