Adding reports - #649
Open
BillingServ wants to merge 4 commits into
Open
Conversation
|
nice addition for admins. |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an integration access report for administrators with filtering, pagination, CSV export, and asynchronous refresh support.
Changes:
- Adds report navigation, routes, views, and UI.
- Implements access matrices and CSV downloads.
- Adds background refresh tasks and nested filter-path support.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Summary | Findings |
|---|---|---|
back/users/templates/admin_base.html |
Adds Reports navigation. | None noted. |
back/back/settings.py |
Configures report pagination. | None noted. |
back/admin/integrations/views.py |
Implements report, CSV, and refresh views. | Critical CSV formula injection; moderate employee filtering issue; tests needed. |
back/admin/integrations/utils.py |
Supports nested filter paths. | None noted. |
back/admin/integrations/urls.py |
Registers report routes. | None noted. |
back/admin/integrations/tests.py |
Tests filter behavior. | None noted. |
back/admin/integrations/templates/access_report.html |
Renders report controls and access matrix. | Moderate authorization issue with generated links. |
back/admin/integrations/tasks.py |
Queues integration refreshes. | Moderate inactive-employee refresh issue. |
Suppressed comments (7)
back/admin/integrations/tasks.py:92
- A failed
user_existslookup returnsNonewithout changing theIntegrationUserrow, and this task only logs the failure. The report reads that row'srevokedvalue, so a timeout for a previously active account is still exported/rendered as “Active” with no indication that the refresh failed. Persist an unknown/error result or expose refresh freshness before presenting the value as current audit data.
integration.user_exists(user, save_result=True)
except Exception as e:
back/admin/integrations/tasks.py:127
- Every refresh enqueues one task per integration/user pair, and there is no in-progress or duplicate guard. Repeated clicks or a large tenant can therefore create many duplicate HTTP lookups and consume the shared django-q worker pool; the worker pool does not provide per-integration rate limiting. Deduplicate refreshes or batch/rate-limit work per integration before exposing this as a repeatable action.
for user_id in user_ids:
async_task(
"admin.integrations.tasks.refresh_access_for_user_integration",
integration.id,
user_id,
task_name=f"Refresh access: {integration.name} #{user_id}",
)
back/admin/integrations/tasks.py:91
Integration.user_exists()ultimately callsrun_request()with an HTTP timeout of 120 seconds, while Django-Q is configured to terminate tasks after 90 seconds (back/back/settings.py:358-362). A lookup taking 90–120 seconds will be killed before this task can save its result, leaving the report stale and causing task failure/retry. Align the request and task timeouts (and their retry policy) for this refresh path.
integration.user_exists(user, save_result=True)
back/admin/integrations/utils.py:62
candidate is not Nonechanges the existing behavior for one-segment filters: the olditem.get(field, "")matched a missing field forfield=and matched JSONnullforfield=None, but both now get skipped and raiseKeyError. That contradicts the comment that single-segment paths preserve the old behavior; keep the old top-level comparison and use a separate missing-path branch for dotted fields.
if candidate is not None and str(candidate) == expected:
back/admin/integrations/views.py:343
AdminOrManagerPermMixingrants managers this endpoint, but_filtered_users_querysetis not scoped to their managed new hires, so it exposes every active employee's third-party access. That conflicts with the existing manager permission contract (users/models.py:170-171) and the per-user access views. Since the report is described as admin-only, useAdminPermMixinor explicitly scope the matrix to permitted users.
class IntegrationAccessReportView(AdminOrManagerPermMixin, TemplateView):
back/admin/integrations/views.py:398
- The CSV endpoint independently uses
AdminOrManagerPermMixin, so a manager can bypass any restriction added to the HTML report and export all employees' integration access. Apply the same admin-only authorization (or manager-specific user scoping) to this endpoint.
class IntegrationAccessReportCSVView(AdminOrManagerPermMixin, View):
back/users/templates/admin_base.html:156
- This menu item is rendered for every user using
admin_base.html, including managers. If the report is restricted to administrators as described, managers will still see a Reports link that leads to a forbidden page; gate this menu block withrequest.user.is_adminalongside the endpoint authorization.
<li class="nav-item dropdown {% if 'access-report' in request.path %}active{% endif %}">
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| Integration.objects.account_provision_options().filter(is_active=True) | ||
| ) | ||
| user_ids = list( | ||
| User.objects.filter(is_active=True) |
| {% for row in rows %} | ||
| <tr> | ||
| <td style="position: sticky; left: 0; background: var(--tblr-card-bg, #fff);"> | ||
| <a href="{% if row.user.role == 0 %}{% url 'people:new_hire_access' row.user.id %}{% else %}{% url 'people:colleague_access' row.user.id %}{% endif %}"> |
|
|
||
| def _filtered_users_queryset(role_filter=None, search=None): | ||
| User = get_user_model() | ||
| users_qs = User.objects.filter(is_active=True).order_by("first_name", "last_name") |
Comment on lines
+359
to
+363
| integrations, rows = _build_access_matrix( | ||
| role_filter=role_filter, | ||
| search=search, | ||
| users=list(page_obj.object_list), | ||
| ) |
Comment on lines
+421
to
+423
| writer.writerow( | ||
| [user.full_name, user.email] | ||
| + [cell_labels[c] for c in row["cells"]] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Added a new section for admins to check which employees have access to what, refresh connections and download a CSV for any auditing or compliance needs.