-
Notifications
You must be signed in to change notification settings - Fork 96
Shared-event permissions editing fix #264
base: master
Are you sure you want to change the base?
Shared-event permissions editing fix #264
Conversation
@libasys can you test if this solves #217 cc @simonruber @sabrina12 @skadyrov does this solve #263 for you? |
Also @georgehrke how does this look? |
@jo-tud please check if this solves #217 for you. |
@ben-denham unfortunately seems to not work correctly. When the other person edits the event and saves, the event gets duplicated (because the »Calendar« dropdown« does not contain the correct calendar – actually the person getting the event should not be allowed to change the calendar, or have the info displayed at all) And worst – the event is taken from the person who originally shared it so they can not access it anymore! |
@jancborchardt is that a 👎 ? |
For now yes, but I’m very much interested in having this issue fixed. :) @tanghus do you have the same issue as me when trying this pull request? @ben-denham can you look into it? |
@jancborchardt Haven't had time to test it. Will do after @ben-denham checks if he can reproduce the issue. |
@jancborchardt Sorry I took so long, but I've managed to reproduce your issue, and have fixed it by making a few changes to the permissions that are granted for shared events and how they apply. Since you say that a user who an event is shared with should not be able to take that event away from the original sharer, I have made ticking the "edit" checkbox for events only grant the UPDATE permission, and not the DELETE permission. I have also made moving an event from calendar A to calendar B require the DELETE permission for calendar A or the event, and the CREATE permission for calendar B. Finally, I have adjusted the conditions under which the delete button and calendar picker are displayed to match how the permissions now work. |
@georgehrke can you review the code? |
What is the state of this issue? It has been around quite some time. Did anyone review the PR? |
@raimund-schluessler it needs to ideally be rebased before we should test because there might be conflicts. @ben-denham are you around to do that? |
@jancborchardt rebase to master completed. |
@georgehrke @raimund-schluessler @owncloud/designers let’s test this then! :) |
Code looks good to me, have no time for testing this right now |
I noticed several issues with this PR:
I will try to have a look into why this happens. |
The reason for the second part of the problem are the following lines: https://github.com/owncloud/calendar/blob/master/ajax/event/edit.php#L41-L48 I guess it was intended to be able to move an object from one calendar to the other, but in this case the user with whom the event was shared with, should not be able to alter the calendarID? |
Hi there,
I've been looking into solving a few issues for Owncloud's calendar app over the last few days, and this is my first fix.
The changes in this commit allow edit/reshare-permissions to be saved for shared events, which solves: owncloud/calendar#217 and owncloud/calendar#263 (which appears to be a duplicate).
As suggested in owncloud/calendar#217 (comment), I have altered permissions checking in lib/object.php so that users who only have update permissions for a single event (but not for the whole calendar) are able to edit the event. However, perhaps OC_Calendar_App::getPermissions() should be used instead to check event/calendar permissions in lib/object.php?
One problem that has not been addressed is that shared-event 'resharing' permissions always override shared-calendar 'resharing' permissions.
So in the rare case that a user has reshare-permissions for a shared-calendar, but not for a shared-event on that calendar, they will be denied access if they attempt to reshare that event.
This problem seems to be with the permissions-checking in OCP\Share::put().
Please let me know if I haven't made anything clear or if you would like me to make any changes, and thanks for all your hard work on Owncloud :)
Cheers,
Ben