fix: Add user_id check before error in event invoice API #7403
Conversation
This file contains 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
app/api/event_invoices.py
Outdated
| @@ -30,7 +30,7 @@ def query(self, view_kwargs): | |||
| """ | |||
| user = current_user | |||
| user_id = view_kwargs.get('user_id') | |||
| if user_id != user.id and not user.is_staff: | |||
| if user_id and user_id != user.id and not user.is_staff: | |||
There was a problem hiding this comment.
Good work. It should throw error if /v1/event-invoices is accessed
|
@iamareebjamal pls review |
Codecov Report
@@ Coverage Diff @@
## development #7403 +/- ##
============================================
Coverage 64.74% 64.74%
============================================
Files 262 262
Lines 13235 13236 +1
============================================
+ Hits 8569 8570 +1
Misses 4666 4666
Continue to review full report at Codecov.
|
app/api/event_invoices.py
Outdated
| @@ -30,6 +30,8 @@ def query(self, view_kwargs): | |||
| """ | |||
| user = current_user | |||
| user_id = view_kwargs.get('user_id') | |||
| if '/v1/event-invoices' in request.url and not user.is_staff: | |||
There was a problem hiding this comment.
Is the way I opted now is correct or still go for args?
iamareebjamal
changed the title
iamareebjamal
changed the title
app/api/event_invoices.py
Outdated
| or view_kwargs.get('event_identifier') | ||
| if not params and not user.is_staff: | ||
| raise ForbiddenError({'source': ''}, 'Admin access is required') | ||
| if user_id and user_id != user.id and not user.is_staff: |
There was a problem hiding this comment.
Both the conditions can be combined
app/api/event_invoices.py
Outdated
| if '/v1/event-invoices' in request.url and not user.is_staff: | ||
| params = user_id or view_kwargs.get('event_id') \ | ||
| or view_kwargs.get('event_identifier') | ||
| if not params and not user.is_staff: |
There was a problem hiding this comment.
I think checking for an empty dict is much easier
iamareebjamal
approved these changes
Nov 3, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #7402
Short description of what this resolves:
added user_id check before Forbidden error is raised
Changes proposed in this pull request:
Checklist
developmentbranch.