fix: safeguard dashboard audit logging#259
Conversation
2-Coatl
deleted the
feature/analyze-and-implement-api-according-to-docs/backend-13-23-31
branch
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @staticmethod | ||
| def _serialize_widgets(widget_keys: List[str]) -> List[Dict[str, str]]: | ||
| """Convierte identificadores de widgets a diccionarios serializables.""" | ||
|
|
||
| return [asdict(WIDGET_REGISTRY[widget]) for widget in widget_keys if widget in WIDGET_REGISTRY] |
There was a problem hiding this comment.
The new _serialize_widgets assumes each entry in widget_keys is a widget identifier string, but configurations saved via the existing personalizar endpoint store configuracion['widgets'] as dictionaries (e.g., {'type': 'total_calls', 'position': ...}). When such a saved dashboard is exported, widget in WIDGET_REGISTRY receives a dict and raises TypeError: unhashable type: 'dict', so ver_dashboard/exportar_dashboard fail for personalized dashboards. Normalize the entries (e.g., use widget['type']) or validate the expected type before the membership check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors dashboard audit logging to use a safe, reusable helper method that skips logging when the audit table is unavailable, and updates dashboard service methods to use proper AuditoriaPermiso model fields.
Key Changes:
- Introduces
_registrar_auditoriahelper that checks table existence before logging - Updates
exportar,personalizar, andcompartirmethods to use supported audit fields (contexto_adicional) instead of non-existent fields - Adds three new service methods:
personalizar_dashboard,compartir_dashboard, andexportar_dashboardwith different APIs than their legacy counterparts
| @staticmethod | ||
| def compartir_dashboard( | ||
| usuario_origen_id: int, | ||
| usuario_destino_id: int, | ||
| ) -> DashboardConfiguracion: | ||
| """Copia la configuración del dashboard del usuario origen al destino. | ||
|
|
||
| Args: | ||
| usuario_origen_id: ID del usuario que comparte su dashboard. | ||
| usuario_destino_id: ID del usuario que recibirá la configuración. | ||
|
|
||
| Returns: | ||
| Objeto ``DashboardConfiguracion`` del usuario destino actualizado. | ||
|
|
||
| Raises: | ||
| ObjectDoesNotExist: Si el usuario destino no existe. | ||
| """ | ||
|
|
||
| try: | ||
| usuario_destino = User.objects.get(id=usuario_destino_id, is_deleted=False) | ||
| except User.DoesNotExist as exc: | ||
| raise ObjectDoesNotExist( | ||
| f"Usuario destino no encontrado: {usuario_destino_id}" | ||
| ) from exc | ||
|
|
||
| configuracion_origen = DashboardConfiguracion.objects.filter( | ||
| usuario_id=usuario_origen_id | ||
| ).first() | ||
|
|
||
| if configuracion_origen is None: | ||
| configuracion_origen = DashboardConfiguracion.objects.create( | ||
| usuario_id=usuario_origen_id, | ||
| configuracion={"widgets": list(WIDGET_REGISTRY.keys())}, | ||
| ) | ||
|
|
||
| configuracion_destino, _ = DashboardConfiguracion.objects.update_or_create( | ||
| usuario_id=usuario_destino.id, | ||
| defaults={ | ||
| "configuracion": deepcopy(configuracion_origen.configuracion), | ||
| }, | ||
| ) | ||
|
|
||
| return configuracion_destino |
There was a problem hiding this comment.
The compartir_dashboard method lacks permission verification. Unlike the legacy compartir method (lines 307-392), this new method doesn't check if the user has the 'sistema.vistas.dashboards.compartir' permission before sharing dashboard configurations. This creates a security vulnerability where any user could share dashboards without authorization.
| def personalizar_dashboard(usuario_id: int, widgets: List[str]) -> DashboardConfiguracion: | ||
| """Guarda la lista de widgets habilitados para el usuario. | ||
|
|
||
| Args: | ||
| usuario_id: ID del usuario dueño de la configuración. | ||
| widgets: Lista de identificadores de widgets. | ||
|
|
||
| Raises: | ||
| ValidationError: Si la lista está vacía o contiene widgets inexistentes. | ||
| """ | ||
| if not widgets: | ||
| raise ValidationError('Debe proporcionar al menos un widget para personalizar el dashboard') | ||
|
|
||
| widgets_invalidos = [widget for widget in widgets if widget not in WIDGET_REGISTRY] | ||
| if widgets_invalidos: | ||
| raise ValidationError(f"Widget invalido: {', '.join(widgets_invalidos)}") | ||
|
|
||
| configuracion, _ = DashboardConfiguracion.objects.update_or_create( | ||
| usuario_id=usuario_id, | ||
| defaults={"configuracion": {"widgets": widgets}}, | ||
| ) | ||
|
|
||
| return configuracion | ||
|
|
||
| @staticmethod | ||
| def compartir_dashboard( | ||
| usuario_origen_id: int, | ||
| usuario_destino_id: int, | ||
| ) -> DashboardConfiguracion: | ||
| """Copia la configuración del dashboard del usuario origen al destino. | ||
|
|
||
| Args: | ||
| usuario_origen_id: ID del usuario que comparte su dashboard. | ||
| usuario_destino_id: ID del usuario que recibirá la configuración. | ||
|
|
||
| Returns: | ||
| Objeto ``DashboardConfiguracion`` del usuario destino actualizado. | ||
|
|
||
| Raises: | ||
| ObjectDoesNotExist: Si el usuario destino no existe. | ||
| """ | ||
|
|
||
| try: | ||
| usuario_destino = User.objects.get(id=usuario_destino_id, is_deleted=False) | ||
| except User.DoesNotExist as exc: | ||
| raise ObjectDoesNotExist( | ||
| f"Usuario destino no encontrado: {usuario_destino_id}" | ||
| ) from exc | ||
|
|
||
| configuracion_origen = DashboardConfiguracion.objects.filter( | ||
| usuario_id=usuario_origen_id | ||
| ).first() | ||
|
|
||
| if configuracion_origen is None: | ||
| configuracion_origen = DashboardConfiguracion.objects.create( | ||
| usuario_id=usuario_origen_id, | ||
| configuracion={"widgets": list(WIDGET_REGISTRY.keys())}, | ||
| ) | ||
|
|
||
| configuracion_destino, _ = DashboardConfiguracion.objects.update_or_create( | ||
| usuario_id=usuario_destino.id, | ||
| defaults={ | ||
| "configuracion": deepcopy(configuracion_origen.configuracion), | ||
| }, | ||
| ) | ||
|
|
||
| return configuracion_destino | ||
|
|
||
| @staticmethod | ||
| def exportar_dashboard(usuario_id: int, formato: str = 'csv') -> Union[str, bytes]: | ||
| """Exporta el dashboard del usuario en formato CSV o PDF. | ||
|
|
||
| Args: | ||
| usuario_id: ID del usuario. | ||
| formato: Formato solicitado (``csv`` o ``pdf``). | ||
|
|
||
| Returns: | ||
| Cadena CSV cuando ``formato`` es ``csv`` o bytes que representan un | ||
| PDF cuando ``formato`` es ``pdf``. | ||
|
|
||
| Raises: | ||
| ValidationError: Si el formato es inválido. | ||
| """ | ||
| if formato not in {"csv", "pdf"}: | ||
| raise ValidationError("Formato invalido. Use csv o pdf") | ||
|
|
||
| dashboard = DashboardService.ver_dashboard(usuario_id=usuario_id) | ||
| widgets = dashboard.get("widgets", []) | ||
|
|
||
| if formato == "csv": | ||
| output = StringIO() | ||
| writer = csv.DictWriter(output, fieldnames=["type", "title", "value", "change", "period"]) | ||
| writer.writeheader() | ||
| for widget in widgets: | ||
| writer.writerow(widget) | ||
| return output.getvalue() | ||
|
|
||
| buffer = BytesIO() | ||
| buffer.write(b"%PDF-1.4\n%\xe2\xe3\xcf\xd3\n") | ||
| buffer.write(b"1 0 obj<< /Type /Catalog /Pages 2 0 R >>endobj\n") | ||
| buffer.write(b"2 0 obj<< /Type /Pages /Kids[3 0 R] /Count 1 >>endobj\n") | ||
| buffer.write(b"3 0 obj<< /Type /Page /Parent 2 0 R /MediaBox[0 0 300 144] /Contents 4 0 R >>endobj\n") | ||
| buffer.write(b"4 0 obj<< /Length 44 >>stream\nBT /F1 12 Tf 72 700 Td (Dashboard Export) Tj ET\nendstream endobj\n") | ||
| buffer.write(b"xref\n0 5\n0000000000 65535 f \n0000000010 00000 n \n0000000060 00000 n \n0000000113 00000 n \n0000000200 00000 n \ntrail\n<< /Size 5 /Root 1 0 R >>\nstartxref\n280\n%%EOF") | ||
| return buffer.getvalue() |
There was a problem hiding this comment.
The methods personalizar_dashboard, compartir_dashboard, and exportar_dashboard lack audit logging. While the refactored methods (exportar, personalizar, compartir) now use _registrar_auditoria, these new methods don't log any audit trail. This is inconsistent with the PR's stated goal of improving audit logging and creates gaps in the audit trail.
| buffer = BytesIO() | ||
| buffer.write(b"%PDF-1.4\n%\xe2\xe3\xcf\xd3\n") | ||
| buffer.write(b"1 0 obj<< /Type /Catalog /Pages 2 0 R >>endobj\n") | ||
| buffer.write(b"2 0 obj<< /Type /Pages /Kids[3 0 R] /Count 1 >>endobj\n") | ||
| buffer.write(b"3 0 obj<< /Type /Page /Parent 2 0 R /MediaBox[0 0 300 144] /Contents 4 0 R >>endobj\n") | ||
| buffer.write(b"4 0 obj<< /Length 44 >>stream\nBT /F1 12 Tf 72 700 Td (Dashboard Export) Tj ET\nendstream endobj\n") | ||
| buffer.write(b"xref\n0 5\n0000000000 65535 f \n0000000010 00000 n \n0000000060 00000 n \n0000000113 00000 n \n0000000200 00000 n \ntrail\n<< /Size 5 /Root 1 0 R >>\nstartxref\n280\n%%EOF") |
There was a problem hiding this comment.
The hardcoded PDF generation in lines 246-252 is fragile and may produce invalid PDF files. The xref table and trailer offsets appear incorrect (e.g., 0000000010 00000 n should have proper byte offsets). Consider using a proper PDF library like ReportLab (which was removed in the imports) or document that this is a placeholder implementation.
| writer = csv.DictWriter(output, fieldnames=["type", "title", "value", "change", "period"]) | ||
| writer.writeheader() | ||
| for widget in widgets: | ||
| writer.writerow(widget) |
There was a problem hiding this comment.
The CSV export (lines 238-244) assumes all widgets have the fields "type", "title", "value", "change", and "period". If widgets are missing any of these fields, the writer.writerow(widget) call will fail. Add validation or use writer.writerow({k: widget.get(k, '') for k in fieldnames}) to handle missing fields gracefully.
| writer = csv.DictWriter(output, fieldnames=["type", "title", "value", "change", "period"]) | |
| writer.writeheader() | |
| for widget in widgets: | |
| writer.writerow(widget) | |
| fieldnames = ["type", "title", "value", "change", "period"] | |
| writer = csv.DictWriter(output, fieldnames=fieldnames) | |
| writer.writeheader() | |
| for widget in widgets: | |
| writer.writerow({k: widget.get(k, '') for k in fieldnames}) |
|
|
||
| widgets_invalidos = [widget for widget in widgets if widget not in WIDGET_REGISTRY] | ||
| if widgets_invalidos: | ||
| raise ValidationError(f"Widget invalido: {', '.join(widgets_invalidos)}") |
There was a problem hiding this comment.
Spelling error: "invalido" should be "inválido" (missing accent mark). This affects user-facing error messages.
| raise ValidationError(f"Widget invalido: {', '.join(widgets_invalidos)}") | |
| raise ValidationError(f"Widget inválido: {', '.join(widgets_invalidos)}") |
| @staticmethod | ||
| def exportar_dashboard(usuario_id: int, formato: str = 'csv') -> Union[str, bytes]: | ||
| """Exporta el dashboard del usuario en formato CSV o PDF. | ||
|
|
||
| Args: | ||
| usuario_id: ID del usuario. | ||
| formato: Formato solicitado (``csv`` o ``pdf``). | ||
|
|
||
| Returns: | ||
| Cadena CSV cuando ``formato`` es ``csv`` o bytes que representan un | ||
| PDF cuando ``formato`` es ``pdf``. | ||
|
|
||
| Raises: | ||
| ValidationError: Si el formato es inválido. | ||
| """ | ||
| if formato not in {"csv", "pdf"}: | ||
| raise ValidationError("Formato invalido. Use csv o pdf") | ||
|
|
||
| dashboard = DashboardService.ver_dashboard(usuario_id=usuario_id) | ||
| widgets = dashboard.get("widgets", []) | ||
|
|
||
| if formato == "csv": | ||
| output = StringIO() | ||
| writer = csv.DictWriter(output, fieldnames=["type", "title", "value", "change", "period"]) | ||
| writer.writeheader() | ||
| for widget in widgets: | ||
| writer.writerow(widget) | ||
| return output.getvalue() | ||
|
|
||
| buffer = BytesIO() | ||
| buffer.write(b"%PDF-1.4\n%\xe2\xe3\xcf\xd3\n") | ||
| buffer.write(b"1 0 obj<< /Type /Catalog /Pages 2 0 R >>endobj\n") | ||
| buffer.write(b"2 0 obj<< /Type /Pages /Kids[3 0 R] /Count 1 >>endobj\n") | ||
| buffer.write(b"3 0 obj<< /Type /Page /Parent 2 0 R /MediaBox[0 0 300 144] /Contents 4 0 R >>endobj\n") | ||
| buffer.write(b"4 0 obj<< /Length 44 >>stream\nBT /F1 12 Tf 72 700 Td (Dashboard Export) Tj ET\nendstream endobj\n") | ||
| buffer.write(b"xref\n0 5\n0000000000 65535 f \n0000000010 00000 n \n0000000060 00000 n \n0000000113 00000 n \n0000000200 00000 n \ntrail\n<< /Size 5 /Root 1 0 R >>\nstartxref\n280\n%%EOF") | ||
| return buffer.getvalue() |
There was a problem hiding this comment.
There's potential duplication between exportar_dashboard (line 218, returns CSV/PDF data) and exportar (line 94, returns metadata with file path). Both serve export functionality but with different signatures and return types. Consider consolidating these methods or clearly documenting their different purposes to avoid confusion.
| def personalizar_dashboard(usuario_id: int, widgets: List[str]) -> DashboardConfiguracion: | ||
| """Guarda la lista de widgets habilitados para el usuario. | ||
|
|
||
| Args: | ||
| usuario_id: ID del usuario dueño de la configuración. | ||
| widgets: Lista de identificadores de widgets. | ||
|
|
||
| Raises: | ||
| ValidationError: Si la lista está vacía o contiene widgets inexistentes. | ||
| """ | ||
| if not widgets: | ||
| raise ValidationError('Debe proporcionar al menos un widget para personalizar el dashboard') | ||
|
|
||
| widgets_invalidos = [widget for widget in widgets if widget not in WIDGET_REGISTRY] | ||
| if widgets_invalidos: | ||
| raise ValidationError(f"Widget invalido: {', '.join(widgets_invalidos)}") | ||
|
|
||
| configuracion, _ = DashboardConfiguracion.objects.update_or_create( | ||
| usuario_id=usuario_id, | ||
| defaults={"configuracion": {"widgets": widgets}}, | ||
| ) | ||
|
|
||
| return configuracion |
There was a problem hiding this comment.
There's duplication between personalizar_dashboard (line 149, accepts list of widget IDs) and personalizar (line 256, accepts dict configuration). Both methods save dashboard configuration but with different APIs. Consider consolidating or clearly documenting their relationship to avoid confusion and potential inconsistencies in how dashboard personalization is handled.
| if AuditoriaPermiso._meta.db_table not in connection.introspection.table_names(): | ||
| return |
There was a problem hiding this comment.
The connection.introspection.table_names() call can cause performance issues in production as it queries the database schema on every audit call. Consider caching the result at module level or using a try-except approach around the AuditoriaPermiso.objects.create() call instead, which will be faster in the common case where the table exists.
| ValidationError: Si el formato es inválido. | ||
| """ | ||
| if formato not in {"csv", "pdf"}: | ||
| raise ValidationError("Formato invalido. Use csv o pdf") |
There was a problem hiding this comment.
Spelling error: "invalido" should be "inválido" (missing accent mark). This affects user-facing error messages.
| raise ValidationError("Formato invalido. Use csv o pdf") | |
| raise ValidationError("Formato inválido. Use csv o pdf") |
| def personalizar_dashboard(usuario_id: int, widgets: List[str]) -> DashboardConfiguracion: | ||
| """Guarda la lista de widgets habilitados para el usuario. | ||
|
|
||
| Args: | ||
| usuario_id: ID del usuario dueño de la configuración. | ||
| widgets: Lista de identificadores de widgets. | ||
|
|
||
| Raises: | ||
| ValidationError: Si la lista está vacía o contiene widgets inexistentes. | ||
| """ | ||
| if not widgets: | ||
| raise ValidationError('Debe proporcionar al menos un widget para personalizar el dashboard') | ||
|
|
||
| widgets_invalidos = [widget for widget in widgets if widget not in WIDGET_REGISTRY] | ||
| if widgets_invalidos: | ||
| raise ValidationError(f"Widget invalido: {', '.join(widgets_invalidos)}") | ||
|
|
||
| configuracion, _ = DashboardConfiguracion.objects.update_or_create( | ||
| usuario_id=usuario_id, | ||
| defaults={"configuracion": {"widgets": widgets}}, | ||
| ) | ||
|
|
||
| return configuracion |
There was a problem hiding this comment.
The personalizar_dashboard method lacks permission verification. Unlike the exportar, personalizar, and compartir methods, this new method doesn't check if the user has the necessary permission before modifying the dashboard configuration. This creates an inconsistent security pattern and potential unauthorized access vulnerability.
Summary
Testing
Codex Task