feat: audit log - backend capability #1607
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces an audit logging feature in the Django project by adding a new application module. The changes include new files for administration, configuration, middleware, models, migrations, mixins, registry, signals, utilities, views, and tests. Additionally, the settings are updated to integrate the audit log application and the custom user middleware. Overall, the modifications enable logging of CRUD operations on models through signal handlers and thread-local user storage. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant MW as UserMiddleware
participant M as Model Instance
participant S as Signal Handler (log_save)
participant AL as AuditLog Model
U->>MW: Sends HTTP Request
MW-->>MW: Store current user in thread-local
M->>S: Triggers post_save signal (save event)
S->>MW: Retrieve current user from thread-local
S->>AL: Create AuditLog entry with operation details
sequenceDiagram
participant U as User
participant MW as UserMiddleware
participant M as Model Instance
participant S as Signal Handler (log_delete)
participant AL as AuditLog Model
U->>MW: Sends HTTP Request (delete)
MW-->>MW: Store current user in thread-local
M->>S: Triggers post_delete signal (delete event)
S->>MW: Retrieve current user from thread-local
S->>AL: Create AuditLog entry for deletion
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
backend/ca_audit_log/views.py (1)
1-1: Unused Import Warning: Therenderfunction imported fromdjango.shortcutsis currently not used in the file. If this import is intended for future view implementations, consider adding a clarifying comment to indicate its planned use. Otherwise, it is advisable to remove the unused import to keep the codebase clean and avoid static analysis warnings.🧰 Tools
🪛 Ruff (0.8.2)
1-1:
django.shortcuts.renderimported but unusedRemove unused import:
django.shortcuts.render(F401)
backend/ca_audit_log/admin.py (1)
1-3: Consider registering the AuditLog model in this fileThis file is currently a placeholder with an unused import. Since this appears to be a work in progress, you'll likely want to register the AuditLog model here once it's implemented.
from django.contrib import admin +from ca_audit_log.models import AuditLog # Register your models here. +@admin.register(AuditLog) +class AuditLogAdmin(admin.ModelAdmin): + list_display = ['timestamp', 'user', 'action', 'content_type', 'object_id'] + list_filter = ['action', 'timestamp', 'content_type'] + search_fields = ['user__username', 'object_representation'] + readonly_fields = ['timestamp', 'user', 'action', 'content_type', 'object_id', 'object_representation', 'changes']🧰 Tools
🪛 Ruff (0.8.2)
1-1:
django.contrib.adminimported but unusedRemove unused import:
django.contrib.admin(F401)
backend/ca_audit_log/tests.py (1)
1-3: Add tests for the audit log functionalityThis is currently a placeholder file with no test implementation. As this appears to be a work in progress, consider adding comprehensive tests for the audit log functionality to ensure it works as expected.
Would you like me to help generate test cases for:
- Verifying that model operations are properly logged
- Testing the UserMiddleware
- Testing signal handling for different CRUD operations
🧰 Tools
🪛 Ruff (0.8.2)
1-1:
django.test.TestCaseimported but unusedRemove unused import:
django.test.TestCase(F401)
backend/ca_audit_log/middleware_request.py (1)
11-12: Consider handling AnonymousUser consistentlyThe current implementation doesn't differentiate between authenticated and anonymous users. You might want to standardize how anonymous users are handled in your audit logs.
def process_request(self, request): - _thread_locals.user = getattr(request, "user", None) + user = getattr(request, "user", None) + # Only store authenticated users + if user and user.is_authenticated: + _thread_locals.user = user + else: + _thread_locals.user = Nonebackend/ca_audit_log/utils.py (1)
5-23: Utility function properly implements custom audit logging.The
log_custom_eventfunction provides a clean interface for manually logging audit events with appropriate documentation.However, consider adding validation for the
operationparameter to ensure it's one of the valid choices ('C', 'U', 'D').def log_custom_event(user, model_class, object_id, operation, event_data=None): """ Utility function to manually log events when needed. Args: user: The user performing the action model_class: The model class of the object object_id: The primary key of the object operation: One of 'C', 'U', 'D' event_data: Dictionary with additional event data """ + if operation not in ('C', 'U', 'D'): + raise ValueError(f"Invalid operation: {operation}. Must be one of 'C', 'U', 'D'") + content_type = ContentType.objects.get_for_model(model_class) return AuditLog.objects.create( user=user, operation=operation, content_type=content_type, object_id=str(object_id), event_data=event_data, )backend/ca_audit_log/mixins.py (1)
1-29: Field change tracking mixin implementation is robust.The
TrackFieldChangesmixin provides an effective way to track field changes in model instances. The implementation correctly handles initialization, field comparison, and reset after saving.Consider a few improvements to make the mixin more robust:
class TrackFieldChanges: """ Mixin to track which fields have changed in a model instance. Add this to models you want to track the changed fields for. """ def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) self._original_state = self._get_field_values() self._changed_fields = [] def _get_field_values(self): - return {f.name: getattr(self, f.name) for f in self._meta.fields} + return {f.name: getattr(self, f.name) for f in self._meta.fields if hasattr(self, f.name)} def save(self, *args, **kwargs): if self.pk: # If this is an update, determine what changed current_state = self._get_field_values() self._changed_fields = [ field for field, value in current_state.items() if field in self._original_state and self._original_state[field] != value ] + # Add a property to access the changed fields from outside + self.get_changed_fields = lambda: self._changed_fields.copy() if hasattr(self, '_changed_fields') else [] super().save(*args, **kwargs) # Update original state after save self._original_state = self._get_field_values() self._changed_fields = []These changes:
- Add a check for attribute existence in
_get_field_valuesto prevent AttributeError- Add a method to safely access the changed fields from outside the class
backend/ca_audit_log/models.py (2)
5-5: Remove unused import.The
jsonmodule is imported but not used in this file.from django.db import models from django.conf import settings from django.contrib.contenttypes.models import ContentType from django.contrib.contenttypes.fields import GenericForeignKey -import json🧰 Tools
🪛 Ruff (0.8.2)
5-5:
jsonimported but unusedRemove unused import:
json(F401)
8-57: AuditLog model implementation is well-structured.The
AuditLogmodel is well-designed for tracking CRUD operations with:
- Appropriate field types and constraints
- Good use of the Django ContentType framework for generic relations
- Clear documentation and string representation
- Important indexes for query optimization
Two suggestions for improvement:
- Consider adding a database-level constraint for the operation field:
# Operation type (CRUD) - operation = models.CharField(max_length=1, choices=OPERATION_CHOICES) + operation = models.CharField(max_length=1, choices=OPERATION_CHOICES, db_index=True)This is more efficient than creating a separate index for this field.
- Consider adding a method to retrieve event data in a more structured way:
def get_event_data(self): """ Safely retrieve event data, returning an empty dict if None. """ return self.event_data or {}backend/ca_audit_log/registry.py (1)
9-21: Consider using sets for operations.Using lists for operations can impede membership checks as the registry grows. Swapping the list for a set can reduce complexity from O(n) to O(1) for membership checks.
-operations = ["C", "U", "D"] # Default to all except READ +self._registry[model_class] = set(operations or ["C", "U", "D"]) ... -return operation in self._registry[model_class] +return operation in self._registry[model_class]backend/ca_audit_log/signals.py (1)
56-66: Duplicate logic betweenlog_saveandlog_delete.Both functions perform nearly identical steps in retrieving the user, content type, and creating an audit record. Consider refactoring common logic into a shared helper function to improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
backend/ca_audit_log/admin.py(1 hunks)backend/ca_audit_log/apps.py(1 hunks)backend/ca_audit_log/middleware_request.py(1 hunks)backend/ca_audit_log/migrations/0001_initial.py(1 hunks)backend/ca_audit_log/mixins.py(1 hunks)backend/ca_audit_log/models.py(1 hunks)backend/ca_audit_log/registry.py(1 hunks)backend/ca_audit_log/signals.py(1 hunks)backend/ca_audit_log/tests.py(1 hunks)backend/ca_audit_log/utils.py(1 hunks)backend/ca_audit_log/views.py(1 hunks)backend/ciso_assistant/settings.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/ca_audit_log/apps.py
10-10: ca_audit_log.signals imported but unused
Remove unused import: ca_audit_log.signals
(F401)
backend/ca_audit_log/models.py
5-5: json imported but unused
Remove unused import: json
(F401)
backend/ca_audit_log/admin.py
1-1: django.contrib.admin imported but unused
Remove unused import: django.contrib.admin
(F401)
backend/ca_audit_log/tests.py
1-1: django.test.TestCase imported but unused
Remove unused import: django.test.TestCase
(F401)
backend/ca_audit_log/views.py
1-1: django.shortcuts.render imported but unused
Remove unused import: django.shortcuts.render
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: startup-docker-compose-test
- GitHub Check: build (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: test (3.12)
- GitHub Check: ruff (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (10)
backend/ca_audit_log/views.py (1)
2-3: Scaffolding Placeholder: The comment# Create your views here.serves as a useful placeholder during early development. As you progress with the audit log backend, ensure that you add the necessary view functions or class-based views to handle HTTP requests appropriately.backend/ca_audit_log/apps.py (1)
4-10: The AppConfig class looks goodThe AppConfig class is properly configured with the appropriate application name and default auto field. The
ready()method correctly imports the signals module to connect signal handlers when the application is initialized.The static analysis flag about the unused import of
ca_audit_log.signalscan be safely ignored as this import is intentional for its side effects (connecting signal handlers) rather than for direct usage.🧰 Tools
🪛 Ruff (0.8.2)
10-10:
ca_audit_log.signalsimported but unusedRemove unused import:
ca_audit_log.signals(F401)
backend/ca_audit_log/middleware_request.py (2)
4-5: Thread-local storage is appropriate for this use caseUsing thread-local storage is a good approach for maintaining user context across different parts of the request lifecycle, especially for audit logging purposes.
14-17: The cleanup in process_response is correctly implementedThe cleanup in the
process_responsemethod is important to prevent memory leaks in thread-local storage. This implementation correctly checks if the attribute exists before attempting to delete it.backend/ciso_assistant/settings.py (1)
166-166: LGTM: Audit log application added to INSTALLED_APPS.The
ca_audit_logapplication has been properly added to theINSTALLED_APPSlist, making it available for the Django project.backend/ca_audit_log/models.py (1)
37-41:❓ Verification inconclusive
Consider adding database-level constraints for the Generic Foreign Key.
To maintain data integrity, it's important to ensure that the
object_idreferences valid objects. While Django's GFK doesn't enforce this at the database level, you might want to handle this in your application logic.
🏁 Script executed:
#!/bin/bash # Check if there are any signal handlers or validators that ensure referential integrity grep -r "pre_delete" --include="*.py" backend/ca_audit_log/ grep -r "pre_save" --include="*.py" backend/ca_audit_log/Length of output: 117
Action Required: Verify Referential Integrity Enforcement for Generic FK
- In
backend/ca_audit_log/models.py(lines 37–41), theGenericForeignKeyis defined using a standardForeignKeyandCharFieldwithout any additional validators.- Grep checks for signal handlers (e.g.,
pre_save,pre_delete) did not reveal any custom logic to enforce referential integrity.- Recommendation: Please verify whether there is any other application-level mechanism (such as a custom
cleanmethod, explicit validators, or other signal handlers elsewhere) that ensures theobject_idreferences valid objects. If not, consider enhancing the logic by adding a custom validation method or similar safeguards to further maintain data integrity.backend/ca_audit_log/migrations/0001_initial.py (2)
29-29: Verify that 255 characters is sufficient for storing object IDs.If you anticipate object IDs that may exceed 255 characters, consider increasing the field length or using a text field.
Would you like a script to locate any model PK definitions across the codebase and check possible maximum lengths?
1-68: Schema design and indexing approach look good.The migration meets standard best practices for an audit log schema. The combination of indexes on
content_type,object_id,timestamp,user, andoperationhelps optimize queries for auditing tasks.backend/ca_audit_log/registry.py (1)
23-26: Warn about overwriting existing registrations.The current
registermethod overwrites any prior registration for the same model. If you need to combine or extend registered operations in separate calls, consider checking and merging existing entries.backend/ca_audit_log/signals.py (1)
8-14: Skipping logs forAuditLogprevents recursion effectively.This preventive measure is helpful to avoid infinite loops. No concerns here.
| class UserMiddleware(MiddlewareMixin): | ||
| """Middleware to store the current user in thread local storage.""" | ||
|
|
||
| def process_request(self, request): | ||
| _thread_locals.user = getattr(request, "user", None) | ||
|
|
||
| def process_response(self, request, response): | ||
| if hasattr(_thread_locals, "user"): | ||
| del _thread_locals.user | ||
| return response |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add a utility function to access the current user
The middleware correctly stores and cleans up the user in thread-local storage, but there's no utility function provided to retrieve the current user. This would be useful for other parts of the application that need access to the current user.
from threading import local
from django.utils.deprecation import MiddlewareMixin
# Thread local storage
_thread_locals = local()
+def get_current_user():
+ """
+ Returns the current user from thread local storage.
+ Returns None if no user is set.
+ """
+ return getattr(_thread_locals, 'user', None)
+
class UserMiddleware(MiddlewareMixin):
"""Middleware to store the current user in thread local storage."""
def process_request(self, request):
_thread_locals.user = getattr(request, "user", None)
def process_response(self, request, response):
if hasattr(_thread_locals, "user"):
del _thread_locals.user
return response📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class UserMiddleware(MiddlewareMixin): | |
| """Middleware to store the current user in thread local storage.""" | |
| def process_request(self, request): | |
| _thread_locals.user = getattr(request, "user", None) | |
| def process_response(self, request, response): | |
| if hasattr(_thread_locals, "user"): | |
| del _thread_locals.user | |
| return response | |
| from threading import local | |
| from django.utils.deprecation import MiddlewareMixin | |
| # Thread local storage | |
| _thread_locals = local() | |
| def get_current_user(): | |
| """ | |
| Returns the current user from thread local storage. | |
| Returns None if no user is set. | |
| """ | |
| return getattr(_thread_locals, 'user', None) | |
| class UserMiddleware(MiddlewareMixin): | |
| """Middleware to store the current user in thread local storage.""" | |
| def process_request(self, request): | |
| _thread_locals.user = getattr(request, "user", None) | |
| def process_response(self, request, response): | |
| if hasattr(_thread_locals, "user"): | |
| del _thread_locals.user | |
| return response |
backend/ciso_assistant/settings.py
Outdated
| @@ -171,6 +172,7 @@ def set_ciso_assistant_url(_, __, event_dict): | |||
| "django.middleware.locale.LocaleMiddleware", | |||
| "django.middleware.common.CommonMiddleware", | |||
| "django.middleware.csrf.CsrfViewMiddleware", | |||
| "ca_audit_log.middleware_request.UserMiddleware", | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify middleware positioning in the stack.
The UserMiddleware has been placed before Django's AuthenticationMiddleware. This position indicates that the audit middleware will run before the user is authenticated, which may lead to incomplete audit logs if it relies on authenticated user information.
🏁 Script executed:
#!/bin/bash
# Check the implementation of UserMiddleware to verify if it handles unauthenticated requests properly
cat backend/ca_audit_log/middleware_request.pyLength of output: 549
Action Required: Reposition Middleware for Accurate Audit Logging
The current configuration in backend/ciso_assistant/settings.py places "ca_audit_log.middleware_request.UserMiddleware" before Django’s AuthenticationMiddleware. Since the UserMiddleware immediately captures request.user in its process_request method, it can record an unauthenticated or default state rather than the authenticated user. This can lead to incomplete or inaccurate audit logs if your logging relies on the final authenticated state.
- Location:
backend/ciso_assistant/settings.py(Line 175) - Concern:
UserMiddlewareruns beforeAuthenticationMiddleware, meaning the authentication hasn’t been applied yet. - Recommendation: Reorder the middleware so that
UserMiddlewareis placed after Django’sAuthenticationMiddlewarein theMIDDLEWARElist to ensure that it logs the properly authenticated user.
backend/ca_audit_log/signals.py
Outdated
| # Note: This requires adding user to local thread in middleware or request processor | ||
| from threading import local | ||
|
|
||
| _thread_locals = local() | ||
| user = getattr(_thread_locals, "user", None) | ||
|
|
There was a problem hiding this comment.
Potentially incorrect user retrieval via thread local.
Each time you call local(), you get a new local object, which may always yield None for user. Typically, you would define and populate a single thread-local instance in middleware or a global scope, then import and retrieve it here.
One possible fix is to import the shared thread-local instance rather than creating a new one:
-from threading import local
-_thread_locals = local()
-user = getattr(_thread_locals, "user", None)
+from .middleware_request import _thread_locals
+user = getattr(_thread_locals, "user", None)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Note: This requires adding user to local thread in middleware or request processor | |
| from threading import local | |
| _thread_locals = local() | |
| user = getattr(_thread_locals, "user", None) | |
| # Note: This requires adding user to local thread in middleware or request processor | |
| - from threading import local | |
| - | |
| - _thread_locals = local() | |
| - user = getattr(_thread_locals, "user", None) | |
| + from .middleware_request import _thread_locals | |
| + user = getattr(_thread_locals, "user", None) |
|
abandoned in favor of #1612 |
Summary by CodeRabbit
New Features
Tests