Skip to content
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.

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

feat: Serving event invoices through a protected route #6145

Merged
merged 4 commits into from Jul 12, 2019

Conversation

mrsaicharan1
Copy link
Member

@mrsaicharan1 mrsaicharan1 commented Jul 5, 2019

Fixes #6144

Short description of what this resolves:

This route serves event invoices in the same way the order invoices and tickets are served. It allows event organizers to access their respective event invoices.
Allows staff members to download/view them.

Changes proposed in this pull request:

  • Added route to serve event invoices
  • Added security checks to enable access-control for these invoices & only allow authenticated event organisers & staff members to access them.

Checklist

  • I have read the Contribution & Best practices Guide and my PR follows them.
  • My branch is up-to-date with the Upstream development branch.
  • The unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • All the functions created/modified in this PR contain relevant docstrings

app/api/auth.py Outdated Show resolved Hide resolved
app/api/auth.py Outdated Show resolved Hide resolved

################

@ticket_blueprint.route('/events/invoices/<string:invoice_identifier>')

Choose a reason for hiding this comment

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

expected 2 blank lines, found 5

app/api/auth.py Outdated Show resolved Hide resolved
app/api/auth.py Outdated Show resolved Hide resolved
app/api/auth.py Outdated Show resolved Hide resolved
return ForbiddenError({'source': ''}, 'Authentication Required to access Invoice').respond()


@ticket_blueprint.route('/events/invoices/<string:invoice_identifier>')

Choose a reason for hiding this comment

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

redefinition of unused 'event_invoices' from line 349

app/api/auth.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 5, 2019

Codecov Report

Merging #6145 into development will decrease coverage by 0.03%.
The diff coverage is 23.8%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #6145      +/-   ##
===============================================
- Coverage         66.1%   66.07%   -0.04%     
===============================================
  Files              288      288              
  Lines            14484    14503      +19     
===============================================
+ Hits              9575     9583       +8     
- Misses            4909     4920      +11
Impacted Files Coverage Δ
app/api/auth.py 23.88% <23.8%> (-0.24%) ⬇️
app/api/helpers/scheduled_jobs.py 24.8% <0%> (+3.19%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f352525...f602c4d. Read the comment docs.

@mrsaicharan1 mrsaicharan1 changed the title [WIP]feat: Serving event invoices over a secure route feat: Serving event invoices over a secure route Jul 7, 2019
@auto-label auto-label bot added the feature label Jul 7, 2019
@mrsaicharan1 mrsaicharan1 force-pushed the route-invoice branch 2 times, most recently from 65fbdfc to 7686a12 Compare July 9, 2019 04:53
@mrsaicharan1
Copy link
Member Author

@iamareebjamal @uds5501 @shreyanshdwivedi Please have a look at this as it is ready now.
Also, @shreyanshdwivedi , I think you can test your basic design layout for the event invoice once this is merged.

@poush
Copy link
Member

poush commented Jul 10, 2019

The title of this PR seems misleading xD. Instead of "secure", it can be simply "protected" or "restricted"

app/api/auth.py Outdated Show resolved Hide resolved
app/api/auth.py Outdated
@ticket_blueprint.route('/events/invoices/<string:invoice_identifier>')
@jwt_required()
def event_invoices(invoice_identifier):
if current_user:
Copy link
Member

Choose a reason for hiding this comment

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

There are better design methods that you can consider. For eg:

if not current_user:
    raise Forbidden error

There will be no unnecessary if-else ladder

Copy link
Member Author

Choose a reason for hiding this comment

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

@poush This if-else ladder is required because at each stage, we are validating the user & the corresponding permissions. If there is any violation at any point, we throw the appropriate exception.

The example given by you is more or less the equivalent of my logic. If we use

if not current_user:
    raise Forbidden error

we would then have to proceed with the checks in the else block (similar to what I've implemented)

else if current_user.is_staff and current_user.is_verified:
                ...

Copy link
Member

Choose a reason for hiding this comment

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

we would then have to proceed with the checks in the else block

No you won't
You can assume from the next line that current_user is present, removing the need of nested conditions

Copy link
Member Author

Choose a reason for hiding this comment

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

we would then have to proceed with the checks in the else block

No you won't
You can assume from the next line that current_user is present, removing the need of nested conditions

Won't the nesting still be present to check for current_user.is_verified & other cases?

Copy link
Member

Choose a reason for hiding this comment

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

if current_user:
  # 1 level nesting
  if current_user.is_verified:
    # 2 level
  else:
    # 2 level
    throw
else:
  # 1 level
  throw

Max Level 2
Conditions 4

if not current_user:
  throw

if current_user.is_verified:
  throw

# continue

Max level nesting: 0
Conditions 2

Copy link
Member Author

Choose a reason for hiding this comment

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

@iamareebjamal @poush Understood! Will be updating this.

@mrsaicharan1 mrsaicharan1 changed the title feat: Serving event invoices over a secure route feat: Serving event invoices over a protected route Jul 10, 2019
app/api/auth.py Outdated Show resolved Hide resolved
app/api/auth.py Outdated Show resolved Hide resolved
@mrsaicharan1 mrsaicharan1 force-pushed the route-invoice branch 2 times, most recently from 9b8a190 to 26e4503 Compare July 10, 2019 10:31
app/api/auth.py Outdated Show resolved Hide resolved
@mrsaicharan1 mrsaicharan1 force-pushed the route-invoice branch 3 times, most recently from 184735d to 2b3002b Compare July 10, 2019 10:33
@mrsaicharan1 mrsaicharan1 changed the title feat: Serving event invoices over a protected route feat: Serving event invoices through a protected route Jul 10, 2019
app/api/auth.py Outdated Show resolved Hide resolved
app/api/auth.py Outdated Show resolved Hide resolved
app/api/auth.py Outdated Show resolved Hide resolved
Refactored code to reduce nesting
@mrsaicharan1
Copy link
Member Author

@iamareebjamal @poush THis is ready for another review

@iamareebjamal iamareebjamal merged commit dcc365e into fossasia:development Jul 12, 2019
iamareebjamal pushed a commit to iamareebjamal/open-event-server that referenced this pull request Aug 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implementation of a route to serve Event Invoices
6 participants