Security: Require challenge host role for challenge setup mutations#5117
Security: Require challenge host role for challenge setup mutations#5117RishabhJain2018 wants to merge 5 commits into
Conversation
Restrict leaderboard, dataset split, phase split, and tag update APIs to challenge hosts instead of any authenticated user. Co-authored-by: Rishabh Jain <rishabhjain2018@gmail.com>
Co-authored-by: Rishabh Jain <rishabhjain2018@gmail.com>
Co-authored-by: Rishabh Jain <rishabhjain2018@gmail.com>
WalkthroughThis PR adds reusable authorization validation helpers to the challenges utility module and integrates them into multiple view endpoints. It centralizes host-permission checks for leaderboard, dataset split, and challenge phase split operations, and tightens access control on the challenge tags endpoint. ChangesChallenge Host Authorization and Endpoint Integration
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/challenges/utils.py`:
- Around line 42-46: The current check uses
ChallengeHost.objects.filter(user=request.user).exists() which treats any
membership row (including PENDING or DENIED) as authorization; update the
predicate to only consider active/accepted memberships (e.g., filter by
ChallengeHost.status in [ChallengeHost.ACCEPTED, ChallengeHost.SELF]) or reuse
the existing challenge-specific host predicate used elsewhere (the same
filter/QuerySet logic you use for challenge host checks) so that only active
hosts pass the guard and unauthorized PENDING/DENIED rows are excluded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 464a0a59-c519-48f6-b83d-b9eafedad71c
📒 Files selected for processing (2)
apps/challenges/utils.pyapps/challenges/views.py
Only treat ACCEPTED and SELF ChallengeHost rows as authorization for challenge setup mutation endpoints.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/challenges/utils.py (1)
99-100:⚠️ Potential issue | 🟠 MajorMajor authorization gap: PATCH allows any host to modify unlinked (orphan) leaderboards
Inapps/challenges/utils.py:get_leaderboard_modification_error, when noChallengePhaseSplitrows exist for theleaderboard_id,challenge_idsis empty and the function returnsNone:if not challenge_ids: return NoneThat makes the
PATCHendpointapps/challenges/views.py:get_or_update_leaderboard(challenge/create/leaderboard/<leaderboard_pk>/) effectively permit schema/config updates by any activeChallengeHost(the only other check isChallengeHost.objects.filter(user=request.user, status__in=[ACCEPTED, SELF]).exists()).This is reachable because
create_leaderboard(.../leaderboard/step_2/) creates standaloneLeaderboardrecords (onlyschema/config_id) before the leaderboard is linked viaChallengePhaseSplitlater in the multi-step flow—so a different host can modify someone else’s unlinked leaderboard if it learns theleaderboard_pk.Fix: enforce ownership even when unlinked (e.g., add a creator/host-team field on
Leaderboardand check it here), or blockPATCHuntil the leaderboard is linked to at least oneChallengePhaseSplit.Minor:
get_leaderboard_modification_errorduplicates theChallengeHostcheck fromget_challenge_host_required_error; reuse the helper to avoid drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/challenges/utils.py` around lines 99 - 100, get_leaderboard_modification_error currently returns None when no ChallengePhaseSplit rows exist (challenge_ids empty), allowing any ChallengeHost to PATCH an unlinked Leaderboard; update get_leaderboard_modification_error in apps/challenges/utils.py so that when challenge_ids is empty it does not permit modification—either return an authorization error requiring the leaderboard be linked to at least one ChallengePhaseSplit before PATCHing or verify ownership by a newly added creator/host-team field on Leaderboard (choose one consistent approach across the app), and remove the duplicated ChallengeHost logic by calling the existing get_challenge_host_required_error helper to determine host permissions.
🧹 Nitpick comments (1)
apps/challenges/utils.py (1)
82-91: 💤 Low valueConsider reusing
get_challenge_host_required_errorto avoid duplication.The host membership check duplicates lines 42-45. Reusing the existing helper improves maintainability.
♻️ Suggested refactor
def get_leaderboard_modification_error(request, leaderboard_pk): """ Return a 401 Response if the user cannot modify the given leaderboard. """ - from hosts.models import ChallengeHost from hosts.utils import is_user_a_host_of_challenge - from rest_framework import status - from rest_framework.response import Response - if not ChallengeHost.objects.filter( - user=request.user, - status__in=[ChallengeHost.ACCEPTED, ChallengeHost.SELF], - ).exists(): - return Response( - { - "error": "Sorry, you are not authorized to perform this operation!" - }, - status=status.HTTP_401_UNAUTHORIZED, - ) + host_error = get_challenge_host_required_error(request) + if host_error: + return host_error🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/challenges/utils.py` around lines 82 - 91, The authorization block performing ChallengeHost.objects.filter(...).exists() duplicates existing logic; replace the manual Response construction with a call to the helper get_challenge_host_required_error() so the same error payload is reused. Locate the check that uses ChallengeHost.objects.filter(user=request.user, status__in=[ChallengeHost.ACCEPTED, ChallengeHost.SELF]).exists() and, when it fails, return get_challenge_host_required_error() instead of constructing a new Response; ensure imports/reference to get_challenge_host_required_error are present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/challenges/utils.py`:
- Around line 99-100: get_leaderboard_modification_error currently returns None
when no ChallengePhaseSplit rows exist (challenge_ids empty), allowing any
ChallengeHost to PATCH an unlinked Leaderboard; update
get_leaderboard_modification_error in apps/challenges/utils.py so that when
challenge_ids is empty it does not permit modification—either return an
authorization error requiring the leaderboard be linked to at least one
ChallengePhaseSplit before PATCHing or verify ownership by a newly added
creator/host-team field on Leaderboard (choose one consistent approach across
the app), and remove the duplicated ChallengeHost logic by calling the existing
get_challenge_host_required_error helper to determine host permissions.
---
Nitpick comments:
In `@apps/challenges/utils.py`:
- Around line 82-91: The authorization block performing
ChallengeHost.objects.filter(...).exists() duplicates existing logic; replace
the manual Response construction with a call to the helper
get_challenge_host_required_error() so the same error payload is reused. Locate
the check that uses ChallengeHost.objects.filter(user=request.user,
status__in=[ChallengeHost.ACCEPTED, ChallengeHost.SELF]).exists() and, when it
fails, return get_challenge_host_required_error() instead of constructing a new
Response; ensure imports/reference to get_challenge_host_required_error are
present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fc21ad32-4cc4-4132-9e24-c7f8df7ff2fa
📒 Files selected for processing (2)
apps/challenges/utils.pyapps/challenges/views.py
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/challenges/views.py
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (50.00%) is below the target coverage (85.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #5117 +/- ##
==========================================
- Coverage 91.84% 91.75% -0.10%
==========================================
Files 96 96
Lines 8207 8225 +18
==========================================
+ Hits 7538 7547 +9
- Misses 669 678 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
2 participants
Summary
Adds authorization checks to challenge setup APIs that previously allowed any authenticated user to mutate leaderboards, splits, and tags.
Changes
apps/challenges/utils.py.Security impact
Limits challenge structure mutations to users who belong to a challenge host team (or the specific challenge host for tag updates).
Slack Thread
Summary by CodeRabbit