feat(attachment): implement attachment workflow for user and admin #575
feat(attachment): implement attachment workflow for user and admin #575ZephyrNova47 wants to merge 49 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new UserFile/FileUsage model and supporting UI/API endpoints to enable an attachment workflow (including shareable inline-view URLs and permission-aware access), and refactors markdown image uploads to store uploads as UserFile records with scoped authorization.
Changes:
- Added
UserFile+FileUsagemodels, migrations, access-authorization chain, and a backfill command for existing Martor uploads. - Added user-facing pages + API v2 endpoints for listing/uploading/editing/downloading user files.
- Updated Martor image upload handling to create
UserFilerecords and track usage context (problem/contest) for access control.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| templates/user/user-tabs.html | Adds a “My Files” tab gated by permission. |
| templates/user/files/file_upload.html | New upload form UI for user files. |
| templates/user/files/file_list.html | New list page UI (with pagination) for user files. |
| templates/user/files/file_edit.html | New edit-metadata UI for user files. |
| templates/user/files/file_detail.html | New detail page UI for user files (incl. usage display). |
| templates/user/files/file_delete.html | New delete-confirm UI for user files. |
| judge/views/widgets.py | Refactors Martor uploader to create UserFile + FileUsage and return access URL. |
| judge/views/user_files.py | Adds HTML views for CRUD + download/inline access with authorization. |
| judge/views/api/user_files.py | Adds API endpoints for listing/uploading/downloading/deleting user files. |
| judge/views/api/init.py | Exposes the new user_files API module. |
| judge/utils/user_file_access.py | Implements Chain-of-Responsibility authorization for file access. |
| judge/models/user_file.py | Defines UserFile storage behavior, access checks, and FileUsage model. |
| judge/models/tests/test_user_file.py | Adds unit tests for access rules and authorization chain. |
| judge/models/init.py | Exports UserFile, FileUsage, and storage router from the models package. |
| judge/migrations/0221_alter_contest_authors_alter_contest_curators_and_more.py | Introduces UserFile/FileUsage tables and related contest/profile field alterations. |
| judge/migrations/0222_fileusage_contest_id_userfile_storage_scope_and_more.py | Adds storage_scope and contest usage fields + indexes. |
| judge/migrations/0223_merge_20260426_1739.py | Merge migration for the new schema changes. |
| judge/management/commands/backfill_martor_user_files.py | Adds command to backfill UserFile entries for existing Martor-referenced uploads. |
| judge/forms.py | Adds upload/edit forms for UserFile. |
| judge/admin/init.py | Minor formatting-only change. |
| dmoj/urls.py | Wires up new HTML and API endpoints; serves media under DEBUG. |
| dmoj/settings.py | Adds MEDIA_URL/MEDIA_ROOT defaults for uploads. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except Exception as e: | ||
| return self._error_response('unknown', str(e), 500) | ||
|
|
There was a problem hiding this comment.
The broad except Exception as e returns str(e) to clients with a 500. This can leak internal details (paths, settings, stack-relevant messages) and makes error handling inconsistent. Prefer catching expected exceptions and returning a generic message for unexpected failures (while logging the real exception server-side).
|
|
||
| base_url = url_map.get(scope) | ||
| if not base_url: | ||
| base_url = os.path.join(default_url.rstrip('/'), USER_FILE_STORAGE_PREFIX, scope) + '/' |
There was a problem hiding this comment.
os.path.join is used to build base_url. Because this is a URL (not a filesystem path), os.path.join can produce backslashes on Windows and can mangle leading slashes. Prefer URL-safe joining (e.g. manual '/' concatenation or urllib.parse.urljoin) when constructing base_url.
| base_url = os.path.join(default_url.rstrip('/'), USER_FILE_STORAGE_PREFIX, scope) + '/' | |
| base_url = '/'.join((default_url.rstrip('/'), USER_FILE_STORAGE_PREFIX, scope, '')) |
| # Media files (user uploads) | ||
| MEDIA_URL = '/media/' | ||
| MEDIA_ROOT = os.path.join(BASE_DIR, 'media') |
There was a problem hiding this comment.
we shouldn't set it here, it's belong to local_settings.py
add admin dashboard CRUD for user files add public URL for viewing files close/revoke public access when file permission is set to private known issue: normal user file dashboard view is still not working
… but super upload still has issues
… scope - Add can_list_by/can_upload_by permission helpers; default users only view files, admins grant judge.add_userfile to enable uploading at /files - Hide the upload button for users without upload permission - Store user files under per-scope subfolders (martor/problem/contest) so the source of each file is traceable on disk - Remove the dropped file_type kwarg that broke martor/problem/contest uploads - Make UserFileStorage deconstructible so migrations stay environment-stable and fix the migration storage drift - Trim redundant comments/docstrings across the feature
- IDOR: validate file ownership in FileAttachmentForm.clean() so users cannot attach files they don't own - File type: add USER_FILE_ATTACHMENT_SAFE_EXTS to settings and reject disallowed extensions (e.g. .html, .svg) in clean_new_file() - Empty form guard: raise ValidationError when neither file nor new_file is provided; clear empty attachment rows via JS on submit (matching the contest problem formset pattern) - Scope attachments context to ContestDetail only (was ContestMixin), filtered by can_view_attachment_by() so names aren't leaked - Admin: register UserFile and FileAttachment models - Tests: IDOR ownership check, dangerous file type rejection, formset IDOR via crafted POST, contest participant attachment access (issue 14)
1381fe4 to
1961df4
Compare
Description
This PR adds an admin-driven attachment workflow with secure file access control, and refactors file authorization using the Chain of Responsibility design pattern.
Known limitation: normal user file dashboard still needs follow-up work.
Type of change: new feature
What
Limitations: My code haven't optimize yet. Need create checkbox for use delete multiple files
Why
Why this PR is needed?
Fixes # (567)
How Has This Been Tested?
Manual test (CRUD done)
Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.By submitting this pull request, I confirm that my contribution is made under the terms of the AGPL-3.0 License.