Add auditlog for JWT and legacy tokens#3673
Conversation
Test results 27 files 27 suites 44m 59s ⏱️ Results for commit b1f2dd6. ♻️ This comment has been updated with latest results. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3673 +/- ##
==========================================
+ Coverage 63.04% 63.07% +0.03%
==========================================
Files 612 612
Lines 45230 45237 +7
Branches 43 43
==========================================
+ Hits 28513 28531 +18
+ Misses 16707 16696 -11
Partials 10 10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1b961ca to
2114e14
Compare
There was a problem hiding this comment.
This looks OK to me, but I would like @hmpf to sign off on it too.
Two things hit, me though, and the first of them is a biggie, IMHO:
- 🔴 Legacy opaque tokens are identified by their actual token string in the audit log, meaning the actual, usable tokens are leaking everywhere in the logs. This is just plain stupid and a borderline security issue.
- I don't really understand #1727. The
LogEntrymodel uses aLegacyGenericForeignKeyto link to objects, and does not specify any sort of cascading delete rule (which would be hard, since generic foreign keys do not enforce integrity at the database level anyway). Deleting a target object from NAV only results in the foreign key of theLogEntryto become an invalid foreign key. When attempting to resolve it, you will hit a dead end. We haven't really lost anything - but we can know that the target is referring to some object that no longer exists, and we can be explicit about that to the user (i.e. instead of a blank in the target column, there would be a message that "target X has been deleted" from NAV).
None of these are issues with this PR, however, but we should have a discussion of them, nonetheless, @hmpf.
|
Point 1 needs an issue, bugfix and/or security. It's probably because APIToken's "str" is just the token itself. Changing away from that should be done in general, it is not safe. It is better to only access the actual token explicitly. We cannot special case this in LogEntry itself, that way madness lies. We could actually rewrite the existing auditlog for apitokens to use object.pk instead. Talking about auditlog, it should be possible to filter by object type and target type and maybe search in description. |
|
Point 2, lets try deleting any of the objects pointed to from the auditlog and if the specific entry survives, close #1727. Agreed that when an object pointed to is deleted it should be shown as "deleted" or similar. |
|
.. it would be possible, for point 1, to in
Override object.token with a safe string, that would fix the Or we could fix |
|
.. we could also add a |
7b05136 to
5bbdb24
Compare
johannaengland
left a comment
There was a problem hiding this comment.
Very clean!
I noticed that you added tests for the JWT token creation/editing/etc, but the changes in the classic token creation are uncovered, so either in this PR or as a follow up issue you could add tests for that as well
tests/integration/web/jwt_test.py
Outdated
| logentry = LogEntry.objects.filter( | ||
| object_pk=token.pk, verb='create-jwtrefreshtoken' | ||
| ) | ||
| assert logentry.exists() |
There was a problem hiding this comment.
Any reason you're doing it here in two lines and then in the next test in one line?
There was a problem hiding this comment.
I assure you there is no good reason for that
There was a problem hiding this comment.
I noticed that you added tests for the JWT token creation/editing/etc, but the changes in the classic token creation are uncovered, so either in this PR or as a follow up issue you could add tests for that as well
yeah I dont think the old token system has tests to begin with, so I neglected it. Its supposed to be removed at some point so not the most important thing in the world. Could potentially do it in a different PR and base the tests off the JWT tests here. Not sure when we're planning on removing it so if it sticks around the tests will prob be worthwhile
This feature is not called expiry, its called recreate
the `delete` function for generic DeleteViews apparantly is not called, you need to override form_valid instead.
2114e14 to
b6c6b58
Compare
b6c6b58 to
b1f2dd6
Compare
|
Scope and purpose
Fixes #3405 . Based on #3674
Adds Auditlog for legacy and JWT tokens. Alot of the auditlogging was there already, it just needed a few adjustments and actually showing the auditlog table in the frontend
Auditlog entries for tokens get messed up when you delete the token, it seems like part of the auditlog requires the referred object to exist for it to have all the information, so when an object is deleted you lose info. This has a related issue #1727, but I'm not sure I've experienced entire entries being deleted, only that the entries are missing the
objectvalueContributor 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.