Improves admin and course-based permission checks#161
Improves admin and course-based permission checks#161happylittle7 wants to merge 1 commit intodevfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refines permission logic in the copycat views to clearly distinguish admin users from regular course-based users. Admins with elevated privileges can now manage plagiarism checks for all problems regardless of course affiliation, while regular users must still be teachers or TAs of the problem's associated course. The changes also standardize API response formatting for improved code consistency.
Key Changes
- Added admin privilege checks allowing superusers, staff, and admin-identity users to access all problems
- Refactored
api_responsefunction for better readability with multi-line formatting - Updated permission check logic to validate problem existence for both admin and non-admin users
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ( | ||
| getattr(user, 'is_superuser', False) | ||
| or getattr(user, 'is_staff', False) | ||
| or getattr(user, 'identity', None) == 'admin' | ||
| ): |
There was a problem hiding this comment.
There's inconsistent handling of admin permissions. The code checks three different attributes for admin status (is_superuser, is_staff, and identity == 'admin'), but the logic treats them all as equivalent. Consider consolidating this into a single method or property on the User model to avoid having to repeat this complex boolean expression throughout the codebase. This would improve maintainability and reduce the risk of inconsistently checking admin status in different parts of the application.
| # 1. Admin 權限檢查：可以操作所有題目 | ||
| if ( | ||
| getattr(user, 'is_superuser', False) | ||
| or getattr(user, 'is_staff', False) | ||
| or getattr(user, 'identity', None) == 'admin' | ||
| ): | ||
| # 仍需確認題目存在 | ||
| if not Problems.objects.filter(pk=problem_id).exists(): | ||
| return False, "題目不存在" | ||
| return True, None |
There was a problem hiding this comment.
The new admin permission logic introduced in this PR lacks test coverage. There are no tests verifying that admins (users with is_superuser, is_staff, or identity='admin') can access problems across all courses, nor tests ensuring the problem existence check works correctly for admin users. Consider adding test cases for admin users to verify they can operate on problems regardless of course affiliation, and that they receive appropriate errors when problems don't exist.
| def api_response(data=None, message="OK", status_code=200): | ||
| status_str = "ok" if 200 <= status_code < 400 else "error" | ||
| if data is None: data = {} | ||
| return Response({"data": data, "message": message, "status": status_str}, status=status_code) | ||
| return Response( | ||
| {"data": data, "message": message, "status": status_str}, | ||
| status=status_code | ||
| ) |
There was a problem hiding this comment.
The removal of the if data is None: data = {} line causes a bug. When api_response is called with data=None (which happens in several places in this file, e.g., line 86, 93, 140, 146), the response will include "data": null instead of "data": {}. This changes the API response format and may break clients expecting an empty object rather than null.
| # 1. Admin 權限檢查：可以操作所有題目 | ||
| if ( | ||
| getattr(user, 'is_superuser', False) | ||
| or getattr(user, 'is_staff', False) | ||
| or getattr(user, 'identity', None) == 'admin' | ||
| ): | ||
| # 仍需確認題目存在 | ||
| if not Problems.objects.filter(pk=problem_id).exists(): | ||
| return False, "題目不存在" | ||
| return True, None |
There was a problem hiding this comment.
The admin permission check should verify that the problem exists before allowing admin access. Currently, there's a race condition: admins bypass the course-based permission checks at lines 48-72, but the problem existence check at lines 43-44 occurs after the admin privileges are already confirmed. If a malicious admin attempts to access a problem between deletion and this check, unexpected behavior could occur. Consider moving the problem existence check before the admin privilege evaluation to fail fast on non-existent problems.
Overview
Rationale
These updates help prevent unauthorized access and possible errors by ensuring only users with appropriate privileges can perform sensitive actions. The changes also improve code clarity, making future updates to permission logic easier and less error-prone.
Relates to improved security and code maintainability.