Skip to content

Improves admin and course-based permission checks #161

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
happylittle7 wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Improves admin and course-based permission checks #161

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 21 additions & 6 deletions copycat/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,11 @@
# ===================================================================
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
)
Comment on lines 18 to +23

Copilot AI Dec 27, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.


class CopycatView(APIView):
authentication_classes = [SessionAuthentication, JWTAuthentication, ApiTokenAuthentication]
Expand All @@ -27,8 +30,21 @@ class CopycatView(APIView):
def _has_problem_edit_permission(self, user, problem_id):
"""
檢查使用者是否有該題目的編輯權限
必須是該題所屬課程的老師或助教
- Admin (is_staff/is_superuser/identity='admin') 可以操作所有題目
- 其他使用者必須是該題所屬課程的老師或助教
"""
# 1. Admin 權限檢查：可以操作所有題目
if (
getattr(user, 'is_superuser', False)
or getattr(user, 'is_staff', False)
or getattr(user, 'identity', None) == 'admin'
):
Comment on lines +37 to +41

Copilot AI Dec 27, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
# 仍需確認題目存在
if not Problems.objects.filter(pk=problem_id).exists():
return False, "題目不存在"
return True, None
Comment on lines +36 to +45

Copilot AI Dec 27, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment on lines +36 to +45

Copilot AI Dec 27, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

# 2. 非 Admin：檢查課程權限
try:
problem = Problems.objects.select_related('course_id').get(pk=problem_id)
except Problems.DoesNotExist:
Expand Down Expand Up @@ -69,8 +85,7 @@ def post(self, request):
except (ValueError, TypeError):
return api_response(None, "problem_id 必須是整數", status_code=400)

# 2. 權限檢查：必須是該題所屬課程的老師或助教
# (此方法內部會檢查題目是否存在，避免重複查詢)
# 2. 權限檢查：Admin 可操作所有題目，其他使用者須為課程老師或助教
has_permission, error_msg = self._has_problem_edit_permission(request.user, problem_id)
if not has_permission:
# 根據錯誤訊息決定回傳的 status code
Expand Down Expand Up @@ -124,7 +139,7 @@ def get(self, request):
except (ValueError, TypeError):
return api_response(None, "problem_id 必須是整數", status_code=400)

# 權限檢查：必須是該題所屬課程的老師或助教
# 權限檢查：Admin 可操作所有題目，其他使用者須為課程老師或助教
has_permission, error_msg = self._has_problem_edit_permission(request.user, problem_id)
if not has_permission:
if "不存在" in error_msg:
Expand Down