Fix: incorrect queryset assignment and invalid field lookup in product_meta_value#2742
Fix: incorrect queryset assignment and invalid field lookup in product_meta_value#2742keshviagrawal wants to merge 8 commits intofossasia:devfrom
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRestricts product_meta_values results for non-admin users by correctly scoping both defaults and matches querysets to the user’s permitted event IDs, fixing an incorrect field lookup and a queryset mix-up that caused 500s. Sequence diagram for non-admin product_meta_values filteringsequenceDiagram
actor User
participant Browser
participant DjangoView as product_meta_values
participant TeamQS as UserTeams
participant DefaultsQS as DefaultsQueryset
participant MatchesQS as MatchesQueryset
User->>Browser: Type in product metadata field
Browser->>DjangoView: AJAX request to product_meta_values
DjangoView->>DjangoView: Compute all_access flag
alt all_access is false (non-admin or limited access)
DjangoView->>TeamQS: filter(can_change_items=True)
TeamQS-->>DjangoView: teams_queryset
DjangoView->>TeamQS: values_list(limit_events__id, flat=True)
TeamQS-->>DjangoView: user_event_ids
DjangoView->>DefaultsQS: defaults.filter(event__id__in=user_event_ids)
DefaultsQS-->>DjangoView: scoped_defaults
DjangoView->>MatchesQS: matches.filter(product__event__id__in=user_event_ids)
MatchesQS-->>DjangoView: scoped_matches
else all_access is true (admin or all_events)
DjangoView->>DjangoView: Use unfiltered defaults and matches
end
DjangoView-->>Browser: JsonResponse with defaults and matches
Browser-->>User: Render filtered typeahead suggestions
Flow diagram for product_meta_values access and filtering logicflowchart TD
A[Start product_meta_values view] --> B[Compute all_access
- user.is_superuser or user.is_staff
- or team with all_events and can_change_items for organizer]
B -->|all_access is true| C[Use existing defaults and matches querysets]
B -->|all_access is false| D[Query user teams with can_change_items=True]
D --> E[Extract user_event_ids via values_list limit_events__id flat True]
E --> F[Filter defaults by event__id__in user_event_ids]
E --> G[Filter matches by product__event__id__in user_event_ids]
C --> H[Return JsonResponse]
F --> H
G --> H
H[End: return filtered JSON results]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider materializing
user_event_ids(e.g.,list(user_event_ids)) before using it in bothdefaults.filter(...)andmatches.filter(...)to avoid evaluating the queryset twice and issuing duplicate database queries.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider materializing `user_event_ids` (e.g., `list(user_event_ids)`) before using it in both `defaults.filter(...)` and `matches.filter(...)` to avoid evaluating the queryset twice and issuing duplicate database queries.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
Fixes a permission-path bug in the control-panel typeahead endpoint for product meta values that caused 500 errors for non-admin users by correcting queryset filtering and preserving queryset types.
Changes:
- Corrects access-restriction filtering by applying event scoping on the proper model path (
product__event__idforProductMetaValue). - Prevents accidental reassignment of
defaultsto a queryset of the wrong model by filteringdefaultsandmatchesindependently. - Reuses a single
user_event_idssubquery for both filters.
You can also share your feedback on Copilot code review. Take the survey.
| if not all_access: | ||
| defaults = matches.filter( | ||
| event__id__in=request.user.teams.filter(can_change_items=True).values_list('limit_events__id', flat=True) | ||
| ) | ||
| matches = matches.filter( | ||
| product__event__id__in=request.user.teams.filter(can_change_items=True).values_list( | ||
| 'limit_events__id', flat=True | ||
| ) | ||
| ) | ||
| user_event_ids = request.user.teams.filter(can_change_items=True).values_list('limit_events__id', flat=True) | ||
| defaults = defaults.filter(event__id__in=user_event_ids) | ||
| matches = matches.filter(product__event__id__in=user_event_ids) |
There was a problem hiding this comment.
Add a regression test that exercises the non-all_access branch (user has organizer permission but only limited event permissions) and asserts this endpoint returns 200 (no FieldError/500) and only returns values for events in the user's limit_events. This would prevent the original queryset/lookup bug from being reintroduced.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
You can also share your feedback on Copilot code review. Take the survey.
| if not all_access: | ||
| defaults = matches.filter( | ||
| event__id__in=request.user.teams.filter(can_change_items=True).values_list('limit_events__id', flat=True) | ||
| ) | ||
| matches = matches.filter( | ||
| product__event__id__in=request.user.teams.filter(can_change_items=True).values_list( | ||
| 'limit_events__id', flat=True | ||
| ) | ||
| ) | ||
| user_event_ids = request.user.teams.filter( | ||
| can_change_items=True, | ||
| organizer=organizer, | ||
| ).values_list('limit_events__id', flat=True) | ||
| defaults = defaults.filter(event__id__in=user_event_ids) | ||
| matches = matches.filter(product__event__id__in=user_event_ids) |
There was a problem hiding this comment.
Consider adding a regression test for the non-admin path (not all_access) to assert this endpoint returns 200 and only includes meta values from events in limit_events (and does not raise FieldError). This would prevent reintroducing the 500 error reported in #2741.
|
Hi @keshviagrawa include the related issue number in the PR description and not in the PR title itslef Maintainers usually recommend linking the issue,Just a small suggestion to align with the contribution guidelines |
keshviagrawal
changed the title
|
Thanks for the suggestion @Rachit7168! I've updated the title to follow the contribution guidelines. |
|
Good fix on the One thing worth highlighting explicitly: the new code adds |
|
Thanks @shivam-pawar-7217 for pointing that out! I've clarified the organizer-level filtering and its impact on data isolation here. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
mariobehling
left a comment
mariobehling
left a comment
There was a problem hiding this comment.
Please provide a screencast showing that the fix works on the system itself. Thanks.
Demo.mp4Added a screencast demonstrating the fix. The video shows: Non-admin user accessing the product page |
Found a bug in product_meta_values where non-admin users were getting a 500 error.
The main issues were:
The filter was using event__id on a model that doesn't have it (needs to go through the product).
defaults was being overwritten by a different queryset type, which broke things later in the function.
I've separated the filters and used the correct product__event__id path. Also cleaned it up by putting the team's event IDs into a single variable to avoid hitting the DB twice.
Additionally, I added organizer-level filtering (organizer=organizer) to ensure that users only access product meta values belonging to their respective organizer.
Previously, team members associated with multiple organizers could potentially access data across events from different organizers when filtering by limit_events. This change improves data isolation and security alongside fixing the 500 error.
Fixes #2741
Summary by Sourcery
Fix access-controlled product meta value lookups to avoid server errors for non-admin users.
Bug Fixes:
Enhancements: