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

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

Merged
merged 12 commits into from
Jul 3, 2024

Conversation

Rutvikrj26
Copy link
Contributor

#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.

@Rutvikrj26 Rutvikrj26 requested review from tompollard and bemoody May 28, 2024 12:56
@bemoody
Copy link
Collaborator

bemoody commented May 28, 2024

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

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

bemoody commented May 30, 2024

  • In console/views.py:

@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).

  • 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.

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

@Rutvikrj26
Copy link
Contributor Author

@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.

Copy link
Member

@tompollard tompollard left a comment

Choose a reason for hiding this comment

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

@Rutvikrj26 I'm a little confused about this PR. The changes to the code don't seem to match the goal that you set out in the title and description ("Adding cohost permissions to view/respond to participation requests").

Side note but I don't remember why "invite co-host" form and notifications ended up being added to the "view event" page. We should definitely move them to http://localhost:8000/events/ (following a similar pattern to http://localhost:8000/projects/) at some point.

@@ -8,7 +8,7 @@


class PublishedProjectManager(Manager):
def accessible_by(self, user):
def accessible_by(self, user, include_event_datatsets=True):
Copy link
Member

Choose a reason for hiding this comment

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

typo include_event_datatsets should be include_event_datasets

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i've ran over it at least half a dozen times, but all three seem to be the exact same to me. Not sure what you're pointing to.

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 a spelling mistake, I think? Shouldn't "datatsets" be "datasets"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, Thanks!!
I couldn't spot it😅

physionet-django/console/views.py Show resolved Hide resolved
physionet-django/events/forms.py Show resolved Hide resolved
physionet-django/events/views.py Show resolved Hide resolved
physionet-django/project/managers/publishedproject.py Outdated Show resolved Hide resolved
@tompollard tompollard merged commit c29fc7b into MIT-LCP:dev Jul 3, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants