Expand activity logs #446
Expand activity logs #446
Conversation
MaciekBaron
force-pushed
the
feature/CMS-906-audit-logging
branch
from
e34734b to
b194bb0
Compare
pyproject.toml
Outdated
| cryptography = "^45.0.4" | ||
| django-pglock = "^1.7.2" | ||
| wagtail-schema-org = "^4.1.1" | ||
| django-crum = "^0.7.9" |
There was a problem hiding this comment.
Just a quick note, while I do a quick pass, as you called out in stand-up, I will come back to this. Personally, I'm not a fan of this, especially using thread locals for such behaviour, and the fact that it hasn't been maintained for over six years. I think we should revisit the approach. What options have been considered?.
There was a problem hiding this comment.
Currently, images and documents do create log entries, but only in specific scenarios - specifically, when using the non-multi version of the forms.
This means that the form at /admin/images/multiple/add/ does not create a log entry when adding an image. Meanwhile, /admin/images/add/ will (albeit you can only upload one image there). The default form you are taken to is the multi one.
Likewise if you mass delete images, that won't trigger the deletion log entries. It will however create the entries if you delete one image using the delete dropdown option.
I have removed django-crud for now and added tests to show the existing behaviour in: 58c6bb1
I have consulted this with the Wagtail team and they seem to believe that the fact that the log entries are created in the first place is because of a refactor done in 2014, following the work done on wagtail/wagtail#7550 and performing a full audit log of all image/document actions wasn't part of a feature plan.
We have several things we could do here:
- Drop
django-crudand instead write our own thread-local approach which we maintain (shouldn't be too complicated code-wide) - Raise an issue in the Wagtail repo to look into implementing full logging for images and documents, so that we don't just have a partial implementation
- Force users to go through a single image upload form, and disable mass deletion of images via the admin panel
There was a problem hiding this comment.
Have you considered https://docs.python.org/3/library/contextvars.html? I'm not sure if threading.local is safe or future-proof for us; we might use a non-sync worker in the future or have async calls. Flask, which under the hood uses Werkzeug, moved away from threading.local to ContextVar also. However, even with both options, there could be gaps where we use thread pools.
There was a problem hiding this comment.
As discussed, if we have no other strong reason to add support for it apart from the multi-image/document upload path, we can consider disabling that functionality and revisit it in the future.
There was a problem hiding this comment.
Have done an initial pass (not checked all events we need to log yet), and overall it looks good.
One key gap is the need for a ChooserView mixin or similar mechanism. We should ensure that any chooser displaying sensitive information elsewhere in the system is logged. While I don't have a comprehensive list immediately available, some areas that require attention include:
- Logging chooser activity for headline figures. For example, when headline figures are listed in a chooser on a Topic page, we should log an action since users are viewing unpublished values.
- Listing of images or documents, since their titles may contain sensitive information.
We should also review other choosers to identify any additional instances where sensitive information may be exposed and ensure appropriate auditing is in place.
cms/articles/views.py
Outdated
| action="content.chart_download", | ||
| instance=page, | ||
| data={ | ||
| "chart_title": chart_title, |
There was a problem hiding this comment.
We don't want to log anything that may contain sensitive info.
| chart_title = chart_data.get("title") | ||
|
|
||
| # Log the chart download for audit trail | ||
| log( |
There was a problem hiding this comment.
I need to check whether we need to log the request along with the same event log.
| for team_id in added: | ||
| team = Team.objects.get(id=team_id) | ||
| log(action="bundles.team_added", instance=instance, data={"team_name": team.name}) | ||
|
|
||
| for team_id in removed: | ||
| team = Team.objects.get(id=team_id) | ||
| log(action="bundles.team_removed", instance=instance, data={"team_name": team.name}) |
There was a problem hiding this comment.
One for discussion.
This makes things somewhat nice, but the site history record has a record for each page (and other parts of bundle). Ideally, it would be easier to have a single record for such an event. It also makes tracing this via logs much simpler.
I would like us to consider either:
- One log action for each change, such as one for the team, one for the page, etc.
- One log action per bundle save (essentially a diff of bundles) - easier for tracing via logs.
Any specific reason for the current approach (other than formatting issue in site history view?). Let's discuss before potentially making any changes.
| audit_logger.info( | ||
| instance.action, | ||
| extra={ | ||
| "event": instance.action, |
There was a problem hiding this comment.
This is already the event, so this is essentially a duplicate. Any reason this was done?
"event": "bundles.page_added", "data": {"event": "bundles.page_added"
There was a problem hiding this comment.
This was so that the message and the data both contain the action, for JSON parsing. As this was literally the same (as you rightly pointed out), I have changed that in this commit, but happy to remove it from the data if we feel it is redundant: b712391
cms/core/wagtail_hooks.py
Outdated
| chart_title = log_entry.data.get("chart_title") | ||
| chart_id = log_entry.data.get("chart_id", "unknown") | ||
| message = "Downloaded chart CSV for chart " | ||
|
|
||
| if chart_title: | ||
| message += chart_title | ||
| else: | ||
| message += f"with ID {chart_id}" |
There was a problem hiding this comment.
Title here would have been fine as only approved persons have access to audit events in the CMS, it was just in sys logs, we can't emit these. I guess we pass it in data, which is used here, but also in the log output, unless we had a mechanism to pass sensitive content to be used by here and not used by audit_logger.info. Thoughts?
|
@MebinAbraham logging of viewed headline figures in choosers added here: 081b796 |
What is the context of this PR?
This PR addresses ticket CMS 906 and expands the activity logs to cover more actions and content types.
Note that this change introduces a new dependency, namelydjango-crum, in order to expose user details when using signals.How to review
Perform one of the following actions and confirm that the activity was logged in site history:
Follow-up Actions
Confirm that using the
django-crummodule is allowed for the project.