Issue/4466/api access/project-specific-access#4487
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRenames WhitelistUser → UserDataAccess, adds api_access_tier and view_user_data, moves ApiAccessTier to users.constants, updates related migrations, serializers, utils, views, URLs, frontend types/API/UI, admin, and tasks to use "data access" terminology and behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant FE as Frontend (Client)
participant API as Backend API (misc.views)
participant Logic as Business Logic (misc.utils)
participant DB as Database (UserDataAccess)
FE->>API: GET /get-data-access-status?post_id&project_id
API->>Logic: get_data_access_status(user, post_id, project_id)
Logic->>DB: query UserDataAccess (user, project/post/null, view_user_data, api_access_tier)
DB-->>Logic: matching entries / tiers
Logic-->>API: (has_data_access, view_deanonymized_data)
API-->>FE: JSON { has_data_access, view_deanonymized_data }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@comments/services/feed.py`:
- Around line 73-79: The current checks treat author_is_staff as truthy so False
is treated like "not provided"; update the conditional logic to detect presence
and explicit True/False values: use "author_is_staff is not None" to detect a
provided boolean and "author_is_staff is True" / "author_is_staff is False" for
behavior decisions. Concretely, change the branch conditions around author and
author_is_staff (the if that currently reads "if author is not None and
author_is_staff", the "elif author_is_staff", and related qs.filter calls) to
explicitly check for is not None and compare to True/False, and implement the
corresponding filters (author_id, author__is_staff=True, author__is_staff=False,
and parent=None where needed).
In `@users/serializers.py`:
- Around line 138-142: get_reduced_api_restriction_projects is returning
duplicate project IDs and loads full WhitelistUser objects; change the query on
user.whitelists to select only project_id and deduplicate in the DB by using
values_list('project_id', flat=True).distinct() combined with the existing
project_id__isnull=False filter so the method returns a lean, unique list of
project IDs without instantiating full model instances.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 912be90c-bc22-4669-8060-485dd631298d
📒 Files selected for processing (8)
comments/serializers/common.pycomments/services/feed.pymisc/migrations/0008_whitelistuser_view_forecaster_data.pymisc/models.pymisc/utils.pyusers/migrations/0016_user_api_access_tier.pyusers/models.pyusers/serializers.py
Contributor
Cleanup: Preview Environment RemovedThe preview environment for this PR has been destroyed.
Cleanup triggered by PR close at 2026-03-26T19:37:30Z |
addresses main site parts of primary spec of #4466 add bot_benchmarking to api access tiers add author_is_staff optional param to comments endpoint
…/api-access/comments-and-bot_benchmarking
lsabor
force-pushed
the
issue/4466/api-access/endpoint-updates
branch
from
2e0916b to
55a2c5f
Compare
lsabor
changed the base branch from
main
to
issue/4466/api-access/comments-and-bot_benchmarking
lsabor
changed the title
Base automatically changed from
issue/4466/api-access/comments-and-bot_benchmarking
to
main
March 14, 2026 21:45
…/api-access/endpoint-updates
…from user data permissions - Rename WhitelistUser model to UserDataAccess across backend, frontend, and migrations - Replace view_forecaster_data field with view_user_data (default False) to explicitly gate user-level data access, separate from API tier grants - Add api_access_tier field to UserDataAccess for project/post-scoped API tier overrides - Extract ApiAccessTier enum to users/constants.py - Rename all related identifiers: whitelists -> data_accesses, is_whitelisted -> has_data_access, get-whitelist-status -> get-data-access-status - Update /users/me endpoint: rename reduced_api_restriction_projects to project_data_access, only return it when ?with_data_access=true is passed - Migration backfills view_user_data=True for all existing rows
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
misc/utils.py (1)
27-45:⚠️ Potential issue | 🟠 MajorCheck all projects attached to the post, not just
default_project.
utils/views.py:156-172now treats anypost.projectsmembership as sufficient forhas_data_access. Here, Lines 29-31 only matchpost.default_project, and Lines 37-45 reuse that same narrowed project for the admin shortcut. Users whose access is granted through a non-default project on the post will get a false negative from this helper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@misc/utils.py` around lines 27 - 45, The helper narrows checks to post.default_project which misses permissions granted via other projects on a post; update the logic in the has_data_access helper to iterate over post.projects (or use post.projects.all()) instead of using post.default_project: when post_id is set, collect data_access_entries for every project in post.projects and also check ProjectUserPermission.objects.filter(user=user, project__in=post.projects.all(), permission=ObjectPermission.ADMIN).exists() so the admin shortcut and data_access_entries include all projects attached to the post rather than only the default_project.utils/csv_utils.py (1)
214-236:⚠️ Potential issue | 🔴 CriticalTighten score scoping for
only_include_user_idsand anonymous callers.
user_forecastsis re-scoped after Line 146, but the score branch is not. If a non-privileged caller providesonly_include_user_ids, Lines 221-228 will return those users’ score rows. And whenuserisNone, Line 232 collapses toQ(user__isnull=True) | Q(), which matches every score. Reapply the caller restriction after the optional ID filter, and use onlyQ(user__isnull=True)for anonymous exports.Possible tightening
- elif only_include_user_ids: + elif only_include_user_ids: + allowed_user_ids = set(only_include_user_ids) + if not (has_data_access or is_staff): + allowed_user_ids &= {user.id} if user else set() # only include user-specific scores for the given user_ids scores = scores.filter( - Q(user_id__in=only_include_user_ids) | Q(user__isnull=True) + Q(user_id__in=allowed_user_ids) | Q(user__isnull=True) ) archived_scores = archived_scores.filter( - Q(user_id__in=only_include_user_ids) | Q(user__isnull=True) + Q(user_id__in=allowed_user_ids) | Q(user__isnull=True) ) elif not (has_data_access or is_staff): # only include user-specific scores for the logged-in user scores = scores.filter( - Q(user__isnull=True) | (Q(user=user) if user else Q()) + Q(user__isnull=True) + | (Q(user=user) if user else Q(user__isnull=True)) ) archived_scores = archived_scores.filter( - Q(user__isnull=True) | (Q(user=user) if user else Q()) + Q(user__isnull=True) + | (Q(user=user) if user else Q(user__isnull=True)) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/csv_utils.py` around lines 214 - 236, The scores/archived_scores query branch incorrectly allows broader access when only_include_user_ids is set and when user is None; update the logic so that after applying the optional only_include_user_ids filter you reapply the caller restriction when not (has_data_access or is_staff), and for anonymous callers (user is None) use only Q(user__isnull=True) instead of Q(user__isnull=True) | Q(), i.e. ensure scores and archived_scores are additionally filtered to (Q(user__isnull=True) | Q(user=user)) for logged-in callers and to Q(user__isnull=True) for anonymous callers while still honoring only_include_user_ids.
🧹 Nitpick comments (1)
misc/models.py (1)
111-120: Disallow deanonymized-only grants that never take effect.Lines 111-119 allow
view_deanonymized_data=Truewhileview_user_data=False, butmisc/utils.pyfilters entries byview_user_data=Truebefore it checks deanonymization. That makes these rows look more permissive in admin than they are at runtime. A smallCheckConstraintor matching admin validation would keep the permission matrix consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@misc/models.py` around lines 111 - 120, Add a constraint and/or admin validation to prevent rows where view_deanonymized_data is True while view_user_data is False: in the model that defines the fields view_user_data and view_deanonymized_data, add a CheckConstraint enforcing "NOT view_deanonymized_data OR view_user_data" (i.e., view_deanonymized_data implies view_user_data) and add matching clean()/ModelAdmin form validation to reject or warn on such combinations so admin UI and runtime filtering remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@misc/utils.py`:
- Around line 27-45: The helper narrows checks to post.default_project which
misses permissions granted via other projects on a post; update the logic in the
has_data_access helper to iterate over post.projects (or use
post.projects.all()) instead of using post.default_project: when post_id is set,
collect data_access_entries for every project in post.projects and also check
ProjectUserPermission.objects.filter(user=user, project__in=post.projects.all(),
permission=ObjectPermission.ADMIN).exists() so the admin shortcut and
data_access_entries include all projects attached to the post rather than only
the default_project.
In `@utils/csv_utils.py`:
- Around line 214-236: The scores/archived_scores query branch incorrectly
allows broader access when only_include_user_ids is set and when user is None;
update the logic so that after applying the optional only_include_user_ids
filter you reapply the caller restriction when not (has_data_access or
is_staff), and for anonymous callers (user is None) use only
Q(user__isnull=True) instead of Q(user__isnull=True) | Q(), i.e. ensure scores
and archived_scores are additionally filtered to (Q(user__isnull=True) |
Q(user=user)) for logged-in callers and to Q(user__isnull=True) for anonymous
callers while still honoring only_include_user_ids.
---
Nitpick comments:
In `@misc/models.py`:
- Around line 111-120: Add a constraint and/or admin validation to prevent rows
where view_deanonymized_data is True while view_user_data is False: in the model
that defines the fields view_user_data and view_deanonymized_data, add a
CheckConstraint enforcing "NOT view_deanonymized_data OR view_user_data" (i.e.,
view_deanonymized_data implies view_user_data) and add matching
clean()/ModelAdmin form validation to reject or warn on such combinations so
admin UI and runtime filtering remain consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b4f09573-398e-498b-8ce8-e437f1a7a28f
📒 Files selected for processing (17)
front_end/src/app/(main)/questions/[id]/components/download_question_data_modal/index.tsxfront_end/src/services/api/posts/posts.shared.tsfront_end/src/types/utils.tsmisc/admin.pymisc/migrations/0008_whitelistuser_api_access_tier.pymisc/models.pymisc/urls.pymisc/utils.pymisc/views.pyusers/constants.pyusers/models.pyusers/serializers.pyusers/views.pyutils/csv_utils.pyutils/serializers.pyutils/tasks.pyutils/views.py
elisescu
approved these changes
Mar 26, 2026
Contributor
elisescu
left a comment
elisescu
left a comment
There was a problem hiding this comment.
Had one more inline comment, but looks good otherwise.
Contributor
|
I added @hlbmtc to review as well |
…/api-access/endpoint-updates
hlbmtc
reviewed
Mar 26, 2026
hlbmtc
reviewed
Mar 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
addresses main site part of optional feature of #4466
followup to #4488
Summary
Renames WhitelistUser model to UserDataAccess and decouples API access tier grants from user-level data permissions.
Key changes
Model rename: WhitelistUser → UserDataAccess, with related_name updated from whitelists to data_accesses across all FKs
New view_user_data field: Replaces view_forecaster_data. Previously, the existence of a whitelist entry implied user data access. Now entries can exist solely for API tier overrides — only entries with view_user_data=True grant user-level data access. Existing rows are backfilled to True.
ApiAccessTier extracted to users/constants.py; BOT_BENCHMARKING renamed to BENCHMARKING
/users/me endpoint: reduced_api_restriction_projects replaced with project_data_access, only returned when ?with_data_access=true is passed
Renamed API surface: get-whitelist-status/ → get-data-access-status/, response key is_whitelisted → has_data_access
Frontend updated to match all backend renames (types, API client, download modal)
All changes consolidated into the existing 0008 migration
Add database constraints for project or post being null and a unique together constraint for user, project, and post.
Summary by CodeRabbit
New Features
Refactor