fix: Check user permission before exporting #6581

prateekj117 · 2019-11-06T07:37:49Z

Short description of what this resolves:

Export event has some information which should not be available to a normal user. So check user permissions before exporting an event.

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.

codecov · 2019-11-06T07:45:51Z

Codecov Report

Merging #6581 into development will increase coverage by 0.27%.
The diff coverage is 83.63%.

@@               Coverage Diff               @@
##           development    #6581      +/-   ##
===============================================
+ Coverage        65.03%   65.31%   +0.27%     
===============================================
  Files              296      297       +1     
  Lines            15247    15227      -20     
===============================================
+ Hits              9916     9945      +29     
+ Misses            5331     5282      -49

Impacted Files	Coverage Δ
app/api/exports.py	`42.1% <100%> (+18.1%)`	⬆️
app/api/helpers/permissions.py	`35.89% <35.71%> (+2.25%)`	⬆️
app/api/schema/tickets.py	`84.81% <0%> (-2.04%)`	⬇️
app/api/auth.py	`24.15% <0%> (-0.16%)`	⬇️
app/api/helpers/query.py	`44% <0%> (ø)`	⬆️
app/api/custom/orders.py	`31.63% <0%> (ø)`
app/__init__.py	`87.75% <0%> (+0.06%)`	⬆️
app/api/orders.py	`26.81% <0%> (+0.13%)`	⬆️
app/api/helpers/scheduled_jobs.py	`24.39% <0%> (+0.95%)`	⬆️

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 006110f...ba24a32. Read the comment docs.

app/api/exports.py

prateekj117 · 2019-11-06T13:56:21Z

@iamareebjamal Please review. This will require some changes. I don't get the purpose of passing an argument.

app/api/helpers/exceptions.py

prateekj117 · 2019-11-07T11:27:04Z

@iamareebjamal Done. Please review.

app/api/exports.py

houndci-bot · 2019-11-07T16:40:51Z

app/api/helpers/permissions.py

-                    user.is_moderator(kwargs['event_id']) or
-                    user.has_event_access(kwargs['event_id'])):
+            user.is_moderator(kwargs['event_id']) or
+            user.has_event_access(kwargs['event_id'])):


continuation line with same indent as next logical line

houndci-bot · 2019-11-07T16:40:51Z

app/api/helpers/permissions.py

-                    user.is_track_organizer(kwargs['event_id']) or
-                    user.has_event_access(kwargs['event_id'])):
+            user.is_track_organizer(kwargs['event_id']) or
+            user.has_event_access(kwargs['event_id'])):


continuation line with same indent as next logical line

houndci-bot · 2019-11-07T16:40:51Z

app/api/helpers/permissions.py

-                    user.is_registrar(kwargs['event_id']) or
-                    user.has_event_access(kwargs['event_id'])):
+            user.is_registrar(kwargs['event_id']) or
+            user.has_event_access(kwargs['event_id'])):


continuation line with same indent as next logical line

houndci-bot · 2019-11-07T16:40:52Z

app/api/helpers/exceptions.py

@@ -1,4 +1,6 @@
 from flask_rest_jsonapi.exceptions import JsonApiException
+from flask_rest_jsonapi.errors import jsonapi_errors
+from flask import make_response, json, abort


'flask.abort' imported but unused
'flask.json' imported but unused
'flask.make_response' imported but unused

houndci-bot · 2019-11-07T16:40:52Z

app/api/helpers/exceptions.py

@@ -1,4 +1,6 @@
 from flask_rest_jsonapi.exceptions import JsonApiException
+from flask_rest_jsonapi.errors import jsonapi_errors


'flask_rest_jsonapi.errors.jsonapi_errors' imported but unused

prateekj117 · 2019-11-07T16:41:36Z

@iamareebjamal Done. Please review.

iamareebjamal · 2019-11-07T17:52:15Z

app/api/exports.py

@@ -6,7 +6,7 @@

 from app.api.helpers.export_helpers import export_event_json, create_export_job
 from app.api.helpers.permission_manager import has_access
-from app.api.helpers.exceptions import assert_forbidden
+from app.api.helpers.permissions import assert_forbidden


Should be in exceptions

iamareebjamal · 2019-11-07T17:54:09Z

app/api/helpers/permissions.py

@@ -257,3 +260,10 @@ def decorated_function(*args, **kwargs):
        return f(*args, **kwargs)

    return decorated_function
+
+
+def assert_forbidden(access_level):


Suggested change

def assert_forbidden(access_level):

def assert_forbidden(has_access):

houndci-bot · 2019-11-08T13:33:51Z

app/api/helpers/permissions.py

 from flask_jwt_extended import verify_jwt_in_request, current_user

 from app.api.helpers.db import save_to_db
 from app.api.helpers.errors import ForbiddenError
+from app.api.helpers.exceptions import ForbiddenException


'app.api.helpers.exceptions.ForbiddenException' imported but unused

prateekj117 · 2019-11-08T13:38:57Z

@iamareebjamal Done. Please review now.

iamareebjamal · 2019-11-08T13:40:54Z

app/api/helpers/permissions.py

-                    user.is_moderator(kwargs['event_id']) or
-                    user.has_event_access(kwargs['event_id'])):
+            user.is_moderator(kwargs['event_id']) or
+                user.has_event_access(kwargs['event_id'])):


Revert changes in this file. And create a decorator in this file similar to others and apply that to the functions

@iamareebjamal But these are the simple indentation corrections, I don't see the point of reverting those changes.

Formatting changes should be done in separate PRs. Someone will be trying to debug something and see the blame and blame you for last modifying and breaking something in a PR for fixing permissions whereas you only indented files

prateekj117 · 2019-11-12T06:14:49Z

@iamareebjamal Please review.

iamareebjamal · 2019-11-12T06:53:28Z

Everything is obviously broken. Please test before asking for review

app/api/exports.py

app/api/helpers/permissions.py

iamareebjamal · 2019-11-18T18:48:13Z

app/api/exports.py

@@ -264,6 +275,7 @@ def export_speakers_csv(event_identifier):

 @export_routes.route('/events/<string:event_identifier>/export/sessions/pdf', methods=['GET'],
                     endpoint='export_sessions_pdf')
+@get_event_id
 @is_coorganizer
 def export_sessions_pdf(event_identifier):


Now this should be event_id and conversion in each function should be removed

I will edit the logic accordingly.

prateekj117 · 2019-11-21T21:34:48Z

@iamareebjamal Please review.

iamareebjamal · 2019-11-24T07:39:23Z

@kushthedude Review. There are a lot of PRs open, but not reviewed. We need to move forward with new Order API and can't until the deck is cleared

iamareebjamal · 2019-11-24T07:40:16Z

app/api/helpers/permissions.py

-        if 'event_id' in kwargs and user.has_event_access(kwargs['event_id']):
+        if user.is_staff or ('event_id' in kwargs and user.has_event_access(kwargs['event_id'])):
+            if 'event_identifier' in kwargs:
+                kwargs.pop('event_identifier', None)


Why's this needed?

Because then we will also have to pass an additional parameter of event_identifier in the function after using this decorator which is not used anywhere.

But this can break a function where they actually are passing event_identifier

@iamareebjamal Already considered this point. Check the implementation of the older is_coorganizer function, it always used to check using event_id only.

That's not what I'm talking about. Consider a function where event_identifier is being received as a parameter and this decorator is applied there

@iamareebjamal Considered this as well. It isn't the case anywhere right now.

Right now isn't good enough, try to create a method with this decorator and see if it fails

@iamareebjamal It will fail in case if the function wants event_identifier.

prateekj117 · 2019-11-25T13:26:26Z

Depends on #6610

prateekj117 · 2019-11-25T13:26:53Z

@iamareebjamal Please merge this.

iamareebjamal · 2019-11-25T15:42:28Z

How can 6581 depend on 6610?

iamareebjamal · 2019-11-25T15:43:02Z

Besides, this has no relation to 6610

prateekj117 · 2019-11-25T15:44:31Z

@iamareebjamal My bad. Wanted to comment this on a other PR.

app/api/helpers/permissions.py

iamareebjamal · 2019-11-26T12:32:38Z

Not much time to move ahead and be perfect with everything. So, will merge after requested changes are addressed @prateekj117

Co-Authored-By: Areeb Jamal <[email protected]>

houndci-bot · 2019-11-26T22:21:07Z

app/api/helpers/permissions.py

+            else:
+                kwargs['event_id'] = kwargs['event_identifier']
+
+        return f(*args, **kwargs)


undefined name 'f'

houndci-bot · 2019-11-26T22:21:07Z

app/api/helpers/permissions.py

+    :return:
+    """
+
+    @wraps(f)


undefined name 'f'

Co-Authored-By: Areeb Jamal <[email protected]>

prateekj117 · 2019-11-26T22:22:55Z

@iamareebjamal Done.

iamareebjamal · 2019-11-26T22:49:03Z

Everything is broken

iamareebjamal · 2019-11-27T16:59:40Z

@prateekj117 When can this be finalized

prateekj117 · 2019-11-27T17:21:45Z

@iamareebjamal Done.

niranjan94 · 2019-11-27T17:28:06Z

Here is an overview of what got changed by this pull request:

Issues
======
+ Solved 2
           

Clones removed
==============
+ app/api/exports.py  -11

See the complete overview on Codacy

iamareebjamal · 2019-11-27T17:33:38Z

Create an issue to fix is_coorganizer which doesn't mutilate method arguments

auto-label bot added the fix label Nov 6, 2019

prateekj117 requested a review from iamareebjamal November 6, 2019 07:39

prateekj117 changed the title ~~fix: Check user permission before exporting~~ [WIP] fix: Check user permission before exporting Nov 6, 2019

auto-label bot removed the fix label Nov 6, 2019

prateekj117 changed the title ~~[WIP] fix: Check user permission before exporting~~ fix: Check user permission before exporting Nov 6, 2019

auto-label bot added the fix label Nov 6, 2019

iamareebjamal reviewed Nov 6, 2019

View reviewed changes

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

fossasia deleted a comment Nov 6, 2019

iamareebjamal reviewed Nov 6, 2019

View reviewed changes

app/api/helpers/exceptions.py Outdated Show resolved Hide resolved

fossasia deleted a comment Nov 7, 2019

iamareebjamal reviewed Nov 7, 2019

View reviewed changes

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

iamareebjamal reviewed Nov 7, 2019

View reviewed changes

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

houndci-bot reviewed Nov 7, 2019

View reviewed changes

fossasia deleted a comment Nov 7, 2019

iamareebjamal reviewed Nov 7, 2019

View reviewed changes

houndci-bot reviewed Nov 8, 2019

View reviewed changes

iamareebjamal reviewed Nov 8, 2019

View reviewed changes

iamareebjamal mentioned this pull request Nov 10, 2019

fix: Correcting the routes for my-session fossasia/open-event-frontend#3606

Merged

prateekj117 force-pushed the event-export-permissions branch from 500e9ad to 677c138 Compare November 12, 2019 06:13

iamareebjamal reviewed Nov 12, 2019

View reviewed changes

app/api/exports.py Show resolved Hide resolved

iamareebjamal reviewed Nov 12, 2019

View reviewed changes

app/api/helpers/permissions.py Outdated Show resolved Hide resolved

iamareebjamal reviewed Nov 18, 2019

View reviewed changes

Required changes.

5f3ab66

iamareebjamal previously approved these changes Nov 24, 2019

View reviewed changes

iamareebjamal reviewed Nov 24, 2019

View reviewed changes

iamareebjamal reviewed Nov 26, 2019

View reviewed changes

app/api/helpers/permissions.py Outdated Show resolved Hide resolved

iamareebjamal reviewed Nov 26, 2019

View reviewed changes

app/api/helpers/permissions.py Outdated Show resolved Hide resolved

iamareebjamal reviewed Nov 26, 2019

View reviewed changes

app/api/helpers/permissions.py Outdated Show resolved Hide resolved

Required changes.

dc326f9

Co-Authored-By: Areeb Jamal <[email protected]>

prateekj117 dismissed iamareebjamal’s stale review via dc326f9 November 26, 2019 22:21

houndci-bot reviewed Nov 26, 2019

View reviewed changes

prateekj117 and others added 2 commits November 27, 2019 03:51

Required changes

81c6553

Co-Authored-By: Areeb Jamal <[email protected]>

Required changes.

b7b0676

Co-Authored-By: Areeb Jamal <[email protected]>

Required changes

ba24a32

iamareebjamal approved these changes Nov 27, 2019

View reviewed changes

iamareebjamal merged commit 9d8a848 into fossasia:development Nov 27, 2019

codedsun pushed a commit to codedsun/open-event-server that referenced this pull request Dec 22, 2019

fix: Check user permission before exporting (fossasia#6581)

1a6d819

		@@ -1,4 +1,6 @@
		from flask_rest_jsonapi.exceptions import JsonApiException
		from flask_rest_jsonapi.errors import jsonapi_errors

	def assert_forbidden(access_level):
	def assert_forbidden(has_access):

fix: Check user permission before exporting #6581

fix: Check user permission before exporting #6581

Conversation

prateekj117 commented Nov 6, 2019

Short description of what this resolves:

Checklist

codecov bot commented Nov 6, 2019 • edited Loading

Codecov Report

prateekj117 commented Nov 6, 2019

prateekj117 commented Nov 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prateekj117 commented Nov 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prateekj117 commented Nov 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamareebjamal Nov 8, 2019 • edited Loading

Choose a reason for hiding this comment

prateekj117 commented Nov 12, 2019

iamareebjamal commented Nov 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prateekj117 commented Nov 21, 2019

iamareebjamal commented Nov 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prateekj117 commented Nov 25, 2019

prateekj117 commented Nov 25, 2019

iamareebjamal commented Nov 25, 2019

iamareebjamal commented Nov 25, 2019

prateekj117 commented Nov 25, 2019

iamareebjamal commented Nov 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prateekj117 commented Nov 26, 2019

iamareebjamal commented Nov 26, 2019

iamareebjamal commented Nov 27, 2019

prateekj117 commented Nov 27, 2019

niranjan94 commented Nov 27, 2019

iamareebjamal commented Nov 27, 2019

codecov bot commented Nov 6, 2019 •

edited

Loading

iamareebjamal Nov 8, 2019 •

edited

Loading

iamareebjamal commented Nov 26, 2019 •

edited

Loading