Conversation
Co-authored-by: Copilot <copilot@github.com>
There was a problem hiding this comment.
Pull request overview
Implements a “My Sheets” dashboard and adds authenticated, per-user cheat sheet management across frontend and backend.
Changes:
- Added a new Dashboard UI (component + CSS) to list, edit, delete, and download saved cheat sheets.
- Secured backend cheat sheet + compile endpoints with authentication and user scoping, and introduced a
userFK onCheatSheet. - Updated frontend auth/navigation flow and API calls to include JWT auth where required.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/styles/Dashboard.css | Styles for the new dashboard layout/cards/empty/loading states. |
| frontend/src/hooks/latex.js | Adds JWT auth header to /api/compile/ calls now that compilation is authenticated. |
| frontend/src/context/AuthContext.jsx | Redirects successful login to /dashboard. |
| frontend/src/components/Dashboard.jsx | New dashboard component: fetch/list sheets, edit/delete, compile+download PDF. |
| frontend/src/App.jsx | Adds dashboard route/links, passes edit/reset handlers, and includes auth header on sheet save. |
| backend/api/views.py | Requires auth for compile and cheat sheet CRUD; scopes cheat sheet queryset to the user; sets owner on create/from-template. |
| backend/api/serializers.py | Exposes user field as read-only on CheatSheetSerializer. |
| backend/api/models.py | Adds CheatSheet.user foreign key. |
| backend/api/migrations/0003_cheatsheet_user.py | Migration adding the user FK to CheatSheet. |
| .gitignore | Adds additional ignores for .DS_Store (recursive) and Playwright/opencode artifacts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,5 +1,5 @@ | |||
| from django.db import models | |||
|
|
|||
| from django.contrib.auth.models import User | |||
There was a problem hiding this comment.
The model imports User directly for the CheatSheet.user FK. For better compatibility with a swappable user model, prefer referencing settings.AUTH_USER_MODEL (and avoid importing django.contrib.auth.models.User in models). This keeps the model consistent with the migration which targets settings.AUTH_USER_MODEL.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
Applied in commit 1462605: CheatSheet.user now references settings.AUTH_USER_MODEL instead of importing User directly, keeping the model consistent with the migration.
| const response = await fetch('/api/cheatsheets/', { | ||
| headers: { | ||
| 'Authorization': `Bearer ${authTokens?.access}`, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
These requests always send an Authorization: Bearer ${authTokens?.access} header. If authTokens is temporarily null/undefined (e.g., during initial render or logout transitions), the header becomes Bearer undefined and can cause confusing 401s. Build headers conditionally (only include Authorization when you have a token) or early-return/navigate when the token is missing.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
Already addressed in commit 1462605. fetchSheets, handleDelete, and handleDownload all early-return when authTokens?.access is absent, so no `****** header is ever sent.
| const response = await fetch(sheetId ? `/api/cheatsheets/${sheetId}/` : '/api/cheatsheets/', { | ||
| method: sheetId ? 'PATCH' : 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| 'Authorization': `Bearer ${authTokens?.access}` | ||
| }, |
There was a problem hiding this comment.
handleSave always includes an Authorization header even when authTokens is falsy, which can result in sending Bearer undefined and returning 401s that are hard to diagnose. Consider conditionally adding the header only when a token exists (and/or blocking save attempts when not authenticated).
There was a problem hiding this comment.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
Already addressed in commit 1462605. The Authorization header in handleSave is now conditionally spread: ...(authTokens?.access ? { 'Authorization': \****** } : {})`.
| permission_classes = [IsAuthenticated] | ||
|
|
||
| def get_queryset(self): | ||
| return self.queryset.filter(user=self.request.user).order_by('-updated_at') |
There was a problem hiding this comment.
This adds per-user isolation for cheat sheets (IsAuthenticated + get_queryset() filtering by request.user), but there don't appear to be tests asserting that a user cannot list/retrieve/update/delete another user's sheets. Adding a multi-user test case would help prevent regressions in access control behavior.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
Already addressed in commit 1462605. Added TestCheatSheetAccessControl with five test cases asserting that list, retrieve, update, delete, and compile all return 404 when a second user attempts to access the first user's sheet.
| @api_view(["POST"]) | ||
| @permission_classes([IsAuthenticated]) | ||
| def compile_latex(request): |
There was a problem hiding this comment.
While adding IsAuthenticated here is good, compile_latex still resolves cheat_sheet_id later via get_object_or_404(CheatSheet, pk=cheat_sheet_id) without checking that the sheet belongs to request.user. That creates an IDOR risk (authenticated users can compile other users' sheets if they guess IDs). When a cheat_sheet_id is provided, ensure the lookup is restricted to user=request.user (or enforce object-level permissions) and return 404/403 for non-owned sheets.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
Already addressed in commit 1462605. compile_latex now uses get_object_or_404(CheatSheet, pk=cheat_sheet_id, user=request.user), returning 404 for any sheet not owned by the requesting user.
|
@copilot apply changes based on the comments in this thread |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…s, access control tests Agent-Logs-Url: https://github.com/ChicoState/cheat-sheet/sessions/2140163e-e1a0-470c-9140-1093fd4b48ca Co-authored-by: Davictory2003 <68972845+Davictory2003@users.noreply.github.com>
All review comments addressed in commit
|
No description provided.