✨(backend) Audit logging [WIP]#1415
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
|
|
||
|
|
||
| def getLogger( | ||
| name: str | None = None, custom_serializers: list[tuple | None] = None |
There was a problem hiding this comment.
?
| name: str | None = None, custom_serializers: list[tuple | None] = None | |
| name: str | None = None, custom_serializers: list[tuple] | None = None |
| self, | ||
| level, | ||
| event_type, | ||
| request: HttpRequest | None, # Intentionnaly mandatory |
There was a problem hiding this comment.
The comment is contradicting the typing, why?
*intentionaly
There was a problem hiding this comment.
There is no = None so it's valid from my pov
| # One public method per standard logging level, all sharing ``_emit``. | ||
| # The ``TYPE_CHECKING`` declarations expose the real signature to editors; | ||
| # the ``else`` branch is what runs, binding the level via ``partialmethod``. | ||
| if TYPE_CHECKING: | ||
| debug: _AuditEmit | ||
| info: _AuditEmit | ||
| warning: _AuditEmit | ||
| error: _AuditEmit | ||
| critical: _AuditEmit | ||
| # ``exception`` mirrors stdlib: ERROR level with the active traceback. | ||
| exception: _AuditEmit | ||
| else: | ||
| debug = partialmethod(_emit, logging.DEBUG) | ||
| info = partialmethod(_emit, logging.INFO) | ||
| warning = partialmethod(_emit, logging.WARNING) | ||
| error = partialmethod(_emit, logging.ERROR) | ||
| critical = partialmethod(_emit, logging.CRITICAL) | ||
| exception = partialmethod(_emit, logging.ERROR, exc_info=True) |
There was a problem hiding this comment.
What would you think about just spelling these out as plain methods, like CPython does for Logger.debug/info/warning/..., thin wrappers that each delegate to self._log(LEVEL, ...)?
My worry is that the partialmethod + Protocol + TYPE_CHECKING approach doesn't really remove the duplication, just moves it into the typing layer where it's harder to follow, the method names still appear twice, and only the body is actually deduplicated (which a plain delegating method does too, since _emit holds the real work).
Since this is headed for django-lasuite, I'd lean toward keeping it stupid-simple: plain defs give honest signatures and tracebacks for free (the thing the Protocol is there to recover) and sidestep stub/runtime drift. The levels are fixed by stdlib, so it's about as stable as duplication gets. Curious what drove the current choice though, is there a constraint I'm missing?
| application.client_id, | ||
| email, | ||
| audit_logger.warning( | ||
| "Application denied delegation", |
There was a problem hiding this comment.
Looks like we're mixing up event message and event type here. Bit confused tbh, is passing just the event type (snake_case) what's expected? And should we list allowed event types in a data structure somewhere, or is that overkill?
| def __init__( | ||
| self, | ||
| logger_name=AUDIT_LOGGER_NAME, | ||
| custom_serializers: list[tuple] | None = None, |
There was a problem hiding this comment.
Could this be typed better or documented ? list[tuple] ?
| exception = partialmethod(_emit, logging.ERROR, exc_info=True) | ||
|
|
||
|
|
||
| def getLogger( |
There was a problem hiding this comment.
I'd rename this to get_audit_logger, and maybe make the AuditLogger class private ?
| return AuditLogger(AUDIT_LOGGER_NAME, custom_serializers=custom_serializers) | ||
| return AuditLogger( | ||
| f"{AUDIT_LOGGER_NAME}.{name}", custom_serializers=custom_serializers | ||
| ) |
There was a problem hiding this comment.
You should add an __ALL__ = ... to this module
| self, | ||
| level, | ||
| event_type, | ||
| request: HttpRequest | None, # Intentionnaly mandatory |
There was a problem hiding this comment.
There is no = None so it's valid from my pov
| self._logger = logging.getLogger(logger_name) | ||
| self._serializers = [] | ||
| if custom_serializers is not None: | ||
| self._serializers = custom_serializers |
There was a problem hiding this comment.
You should build a mapping of class -> serializer method, so that later you can make direct lookups in 0(1) based on the type of the item. (and fallback to 0(n) if it's a subclass of a class you have handling for).
It might also be possible to type this in a generic way, so that if we try to pass something that is not supported we get typehint feedback.
You should crash early if you have multiple sirializers for the same kind of object.
| new_extra = {} | ||
| for key, value in extra.items(): | ||
| for cls, serializer in self._serializers: | ||
| if isinstance(value, cls): | ||
| new_extra = new_extra | { | ||
| f"{key}.{ser_key}": ser_field | ||
| for ser_key, ser_field in serializer(value).items() | ||
| } | ||
| break | ||
| else: | ||
| new_extra[key] = value |
There was a problem hiding this comment.
This is not very optimal perf wise, see my comment above.
3 participants
WIP: Open for feedback
Purpose
Description and CSIRT recommendations
Proposal
src/backend/core/audit.pymeant to be ported todjango-lasuitewhen approvedsrc/backend/core/audit.pyfrom which you can import a logger (getLogger) and a json formatter (AuditJsonFormatter)requestvariable (may be None). When passed, multiple information are derived and added to logs (source_ip,source_portetc.)src/backend/core/audit_meet.py). For example, we can passlogger.info(..., original_room=room), androom.slug,room.nameare automatically added to the log (prefixed with the variable name, e.g.original_room.slug)TODO: