MIT-LCP / physionet-build

The new PhysioNet platform.
https://physionet.org/
BSD 3-Clause "New" or "Revised" License
56 stars 20 forks source link

Adding cohost permissions to view/respond to participation requests #2243

Closed Rutvikrj26 closed 3 months ago

Rutvikrj26 commented 4 months ago

2192 Implements the ability for an event host to add cohosts to the event. Currently cohosts don't have permissions to do anything on the platform.

This PR implements the change in the events functionality for the cohost to be able to view the participant lists, edit the event, view/respond to participation requests.

A scenario: Considering the event is held across the nation with multiple hubs, with each hub having an admin/lead. The admin for each hub is a cohost that can individually verify and approve the participant requests for the hub.

bemoody commented 4 months ago

The key question here is what this means in terms of access control for event datasets.

I realize that for now, event datasets don't actually make a difference to PhysioNet, but I would like to support them eventually, and there are a few things about them that that still make me uneasy.

In issue #2244, I've tried to lay out what I think is needed to make this system reasonably secure and auditable - changes that I hope should be easy to implement and not cause problems for HDN either. Could you please take a look?

Rutvikrj26 commented 4 months ago

Thanks for adding these @bemoody These are major security issues and need addressing before the next event happens. I'll be updating this PR to also solve the issues mentioned in #2244

bemoody commented 4 months ago

@console_permission_required('user.add_event_dataset')

This should be events, not user - and the other lines referring to user.view_all_events should be events.view_all_events instead. Oops!

user.view_all_events and user.view_event_menu are obsolete permissions that should have been removed (issue #1818).

{% if user == event.host or user.id in event.get_cohost_ids %}

Couldn't this be {% if user == event.host or user in event.get_cohosts %} ?

It might be cleaner, instead, to define another function in events/templatetags/participation_status.py.

Rutvikrj26 commented 4 months ago

@bemoody I've implemented these suggestion:

This should be events, not user - and the other lines referring to user.view_all_events should be events.view_all_events instead. Oops!

user.view_all_events and user.view_event_menu are obsolete permissions that should have been removed (issue #1818).

Both these permissions were actually a part of Event model, I mistakenly put them to user.

  • In events/templates/events/event_home.html:

{% if user == event.host or user.id in event.get_cohost_ids %}

Couldn't this be {% if user == event.host or user in event.get_cohosts %} ?

It might be cleaner, instead, to define another function in events/templatetags/participation_status.py.

That is a much cleaner implementation - I implemented a filter in the templatetags that addresses this and removed the redundant method for cohost_ids.

  • In events/templates/events/event_notifications.html: the added text should be wrapped in {% if participation_response_form.instance.event.datasets.exists %}.

Wrapped the text in the condition to make it more secure.