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.

Sign up for GitHub

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

Jump to bottom

fix: add access control check for draft events #7367

Merged
merged 10 commits into from
Oct 20, 2020
Merged

fix: add access control check for draft events #7367

merged 10 commits into from
Oct 20, 2020

Conversation

manav1403
Copy link
Contributor

@manav1403 manav1403 commented Oct 20, 2020 •
edited
Loading

Fixes #7343

Short description of what this resolves:

Earlier the events with status Draft were visible to any user at the event details endpoint using the id.
The expected behavior was that only admin and registrars should be able to see the drafted events only.

Changes proposed in this pull request:

  • Added check for requesting user whether he/she is a registrar or not

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
Copy link

codecov bot commented Oct 20, 2020 •
edited
Loading

Codecov Report

Merging #7367 into development will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #7367      +/-   ##
===============================================
+ Coverage        64.18%   64.23%   +0.04%     
===============================================
  Files              259      259              
  Lines            13126    13130       +4     
===============================================
+ Hits              8425     8434       +9     
+ Misses            4701     4696       -5
Impacted Files Coverage Δ
app/api/events.py 38.91% <100.00%> (+1.60%) ⬆️
app/models/event.py 78.73% <100.00%> (ø)
app/api/helpers/permission_manager.py 61.48% <0.00%> (+0.33%) ⬆️

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 94182b0...76b7c13. Read the comment docs.

@manav1403 manav1403 changed the title added check in event view usinf identifier added check in event details view for draft events Oct 20, 2020
iamareebjamal
iamareebjamal reviewed Oct 20, 2020
View reviewed changes
app/api/events.py Outdated
if 'Authorization' not in request.headers and not has_access(
'is_registrar', event_id=event.id
):
view_kwargs['id'] = None
Copy link
Member

Choose a reason for hiding this comment

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

Use after get object

Copy link
Contributor Author

Choose a reason for hiding this comment

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

made changes @iamareebjamal pls review

iamareebjamal
iamareebjamal reviewed Oct 20, 2020
View reviewed changes
app/api/events.py Outdated
@@ -659,6 +659,13 @@ def before_get_object(self, view_kwargs):
else:
view_kwargs['id'] = None

def after_get_object(self, event, view_kwargs):
if event.state == 'Draft':
Copy link
Member

Choose a reason for hiding this comment

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

There is no Draft state in the event

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default state is this only..

open-event-server/app/models/event.py

Line 76 in 27588e5

state = db.Column(db.String, default="Draft")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iamareebjamal changing to

Suggested change
if event.state == 'Draft':
if event.state == 'draft':

is causing logic to fail and I found that it's because "state": "Draft" in response
** I have created the event using the default configurations

Copy link
Member

Choose a reason for hiding this comment

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

+-----------+---------+
| state     | count   |
|-----------+---------|
| draft     | 664     |
| published | 528     |
+-----------+---------+

Production DB

Copy link
Member

Choose a reason for hiding this comment

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

The default state is this only..

open-event-server/app/models/event.py

Line 76 in 27588e5

state = db.Column(db.String, default="Draft")

Change it here as well

iamareebjamal
iamareebjamal reviewed Oct 20, 2020
View reviewed changes
@manav1403
Copy link
Contributor Author

made changes @iamareebjamal pls review

iamareebjamal
iamareebjamal reviewed Oct 20, 2020
View reviewed changes
app/api/events.py Outdated
@@ -659,6 +659,13 @@ def before_get_object(self, view_kwargs):
else:
view_kwargs['id'] = None

def after_get_object(self, event, view_kwargs):
if event.state == "draft":
if 'Authorization' not in request.headers and not has_access(
Copy link
Member

Choose a reason for hiding this comment

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

This should be or

@iamareebjamal
Copy link
Member

Great. Now please add unit tests for the conditions

  1. Unauthenticated user cannot access draft event
  2. Normal user cannot access draft event
  3. Event owner can access draft event
  4. Admin can access draft event

@iamareebjamal
Copy link
Member

Check this file for reference https://github.com/fossasia/open-event-server/pull/7360/files#diff-2a313974a7aa182365db51b0d3ebdaa7924c91318a741c8237510c3efe1edd06

houndci-bot
houndci-bot reviewed Oct 20, 2020
View reviewed changes
tests/all/integration/api/event/test_eventdetails_view_access.py Outdated
def get_event(db, user=None):
event = EventFactoryBasic()
if user:
role, _ = get_or_create(Role, name='owner', title_name='Owner')
Copy link

Choose a reason for hiding this comment

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

Black would make changes.

@manav1403
Copy link
Contributor Author

@iamareebjamal pls review

iamareebjamal
iamareebjamal reviewed Oct 20, 2020
View reviewed changes
tests/all/integration/api/event/test_eventdetails_view_access.py Outdated
@@ -0,0 +1,61 @@
from tests.factories.event import EventFactoryBasic
Copy link
Member

Choose a reason for hiding this comment

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

rename file to test_event_view_access.py

iamareebjamal
iamareebjamal reviewed Oct 20, 2020
View reviewed changes
tests/all/integration/api/event/test_eventdetails_view_access.py Outdated
return event


def test_eventdetails_draft_get_unauthenticateduser_error(db, client):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def test_eventdetails_draft_get_unauthenticateduser_error(db, client):
def test_event_draft_get_unauthenticated_error(db, client):

iamareebjamal
iamareebjamal reviewed Oct 20, 2020
View reviewed changes
tests/all/integration/api/event/test_eventdetails_view_access.py Outdated
assert response.status_code == 404


def test_eventdetails_draft_get_normaluser_error(db, client, jwt):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def test_eventdetails_draft_get_normaluser_error(db, client, jwt):
def test_event_draft_get_normal_error(db, client, jwt):

iamareebjamal
iamareebjamal reviewed Oct 20, 2020
View reviewed changes
tests/all/integration/api/event/test_eventdetails_view_access.py Outdated
assert response.status_code == 404


def test_eventdetails_draft_get_owner(db, client, user, jwt):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def test_eventdetails_draft_get_owner(db, client, user, jwt):
def test_event_draft_get_owner(db, client, user, jwt):

iamareebjamal
iamareebjamal reviewed Oct 20, 2020
View reviewed changes
tests/all/integration/api/event/test_eventdetails_view_access.py Outdated
assert response.status_code == 200


def test_eventdetails_draft_get_admin(db, client, admin_jwt):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def test_eventdetails_draft_get_admin(db, client, admin_jwt):
def test_event_draft_get_admin(db, client, admin_jwt):

iamareebjamal
iamareebjamal reviewed Oct 20, 2020
View reviewed changes
tests/all/integration/api/event/test_eventdetails_view_access.py Outdated


def get_event(db, user=None):
event = EventFactoryBasic()
Copy link
Member

Choose a reason for hiding this comment

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

Pass in state='draft' explicitly

@iamareebjamal
Copy link
Member

@manav1403 Thank you. Great job. I have suggested a few final changes in the PR

houndci-bot
houndci-bot reviewed Oct 20, 2020
View reviewed changes
tests/all/integration/api/event/test_event_view_access.py


def get_event(db, user=None):
event = EventFactoryBasic(state='draft')
Copy link

Choose a reason for hiding this comment

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

Black would make changes.

@iamareebjamal iamareebjamal changed the title added check in event details view for draft events fix: add access control check for draft event Oct 20, 2020
@iamareebjamal iamareebjamal changed the title fix: add access control check for draft event fix: add access control check for draft events Oct 20, 2020
@auto-label auto-label bot added the fix label Oct 20, 2020
@iamareebjamal iamareebjamal merged commit 315b1bd into fossasia:development Oct 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@houndci-bot houndci-bot houndci-bot left review comments

@iamareebjamal iamareebjamal iamareebjamal approved these changes

Assignees
No one assigned
Labels
fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

User can see draft events
3 participants
@manav1403 @iamareebjamal @houndci-bot