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

MH-12989 Add missing roles for actions->edit scheduled #338

Conversation

krinnewitz
Copy link

@krinnewitz krinnewitz commented Jul 10, 2018

For Actions -> Edit Scheduled Events, a POST request to
event/scheduling.json is sent. Even though a POST request is used, this
is a read-only action. So an _VIEW role is used.

The same applies for event/bulk/conflicts and event/bulk/update
(needs write-access).

This work is sponsored by SWITCH.

@krinnewitz krinnewitz force-pushed the t/MH-12989-non-admin-users-cannot-use-actions-edit-scheduled-events branch 2 times, most recently from 23ac074 to 92e31d5 Compare July 10, 2018 14:02
@krinnewitz krinnewitz changed the title MH-12989 Add missing role for POST event/scheduling.json MH-12989 Add missing roles for actions->edit scheduled Jul 10, 2018
@lkiesow lkiesow self-requested a review July 10, 2018 23:26
@gregorydlogan gregorydlogan added enhancement maintenance This pull request is addressing maintenance issues and removed enhancement labels Jul 23, 2018
@lkiesow lkiesow self-assigned this Aug 15, 2018
@@ -125,10 +126,12 @@
<sec:intercept-url pattern="/services/sanitize" method="POST" access="ROLE_ADMIN, ROLE_UI_SERVICES_STATUS_EDIT" />
<sec:intercept-url pattern="/staticfiles" method="POST" access="ROLE_ADMIN, ROLE_UI_THEMES_CREATE, ROLE_UI_THEMES_EDIT" />
<sec:intercept-url pattern="/admin-ng/acl" method="POST" access="ROLE_ADMIN, ROLE_UI_ACLS_CREATE" />
<sec:intercept-url pattern="/admin-ng/event/bulk/conflicts" method="POST" access="ROLE_ADMIN, ROLE_UI_EVENTS_DETAILS_SCHEDULING_VIEW" />
Copy link
Member

Choose a reason for hiding this comment

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

Where would you need a conflict check when you only have read access to that dialog. Should this maybe be ROLE_UI_EVENTS_DETAILS_SCHEDULING_EDIT?

Copy link
Contributor

@staubesv staubesv Aug 18, 2018

Choose a reason for hiding this comment

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

It is a read request that uses HTTP POST because a parameter is a potentially large list of event IDs that could cause problems then using a GET request because of length limitations of query parameters.

I agree that this requires somebody looking at this line to know about the implementation which should not be necessary...

Should I add a comment? I could also just change it to a *_EDIT role.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

@krinnewitz Could you please change to ROLE_UI_EVENTS_DETAILS_SCHEDULING_EDIT here? @lkiesow seems right as conflict check will only be needed if you can edit scheduled events.

For Actions -> Edit Scheduled Events, a POST request to
event/scheduling.json is sent. Even though a POST request is used, this
is a read-only action. So an _VIEW role is used.

The same applies for event/bulk/conflicts and event/bulk/update
(needs write-access).
@krinnewitz krinnewitz force-pushed the t/MH-12989-non-admin-users-cannot-use-actions-edit-scheduled-events branch from 92e31d5 to 9aee2ea Compare August 21, 2018 10:26
@krinnewitz
Copy link
Author

@staubesv @lkiesow done

@lkiesow lkiesow merged commit 9aee2ea into opencast:develop Aug 22, 2018
lkiesow added a commit that referenced this pull request Aug 22, 2018
…duled-events' of plapadoo/opencast into develop

Pull request #338
  MH-12989 Add missing roles for actions->edit scheduled
@lkiesow
Copy link
Member

lkiesow commented Aug 22, 2018

Thanks for fixing this → merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance This pull request is addressing maintenance issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants