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

fix(caldav): Add sharee to address list when calendar is shared #45054

Merged
merged 1 commit into from
Aug 6, 2024

Conversation

SebastianKrupinski
Copy link
Contributor

@SebastianKrupinski SebastianKrupinski commented Apr 26, 2024

Summary

Sabre invitation plugin skip invites if the organizer of the event is not the owner of the calendar. That happens for shared calendars because \OCA\DAV\CalDAV\Calendar::getOwner overwrites \Sabre\CalDAV\Calendar::getOwner. Upstream returns the principaluri of the owner, we return the principaluri of the sharee.
Want to test if this revives the invites in shared calendars. Adjustments for other places that use the getOwner method might be required.

Checklist

@ChristophWurst
Copy link
Member

Setting to draft because we have to test this locally before it's integrated. We do not want to experiment with this on production instances

@ChristophWurst ChristophWurst added the 2. developing Work in progress label Apr 26, 2024
@ChristophWurst ChristophWurst changed the title fix: remove getowner override issue #26668 fix(caldav): Do not overwrite shared calendar owner Apr 26, 2024
@SebastianKrupinski
Copy link
Contributor Author

Setting to draft because we have to test this locally before it's integrated. We do not want to experiment with this on production instances

Agreed.

@kesselb
Copy link
Contributor

kesselb commented May 23, 2024

Test scenario

  • Calendar "A-Team"
  • Owned by Alice
  • Shared editable with Bob, readonly with Jane
  • Reoccurring event created by Alice (Weekly, Mo, 9am, start date in the future - no invitations for events in the past)
  • Attendees are Alice, Bob, Jane, John, veronica@mail.internal, kevin@mail.internal, mandy@mail.internal

Case 1

Steps:

  1. Alice: Create event and add Bob and veronica@mail.internal
  2. Alice: Edit event and add Jane and kevin@mail.internal
  3. Bob: Edit event and add John and mandy@mail.internal

Expected outcome:

  • Alice can invite internal and external attendees
  • and the invitations are sent by email
  • Bob sees the event two times (1x in the personal calendar because he is also an attendee, 1x in the shared calendar)
  • Bob can invite internal and external attendees to the event in the shared calendar
  • and the invitations are sent by email
  • Jane sees the event two times (1x in the personal calendar because he is also an attendee, 1x in the shared calendar)
  • Jane cannot edit the event in the shared calendar
  • Internal users see the event in the calendar
  • Users can accept/decline the event
  • The status is updated properly for every user with a local account (note, it's important to accept the invitation for the right event - the copy in your personal calendar, if you accept the event in shared calendar the status for the copy in your personal calendar does not change until the next update from the event organizer)
  • External users receive an invitation by email
  • and can use the provided link to accept/decline

Case 2

Steps:

  1. Bob: Create event and add John and mandy@mail.internal
  2. Bob: Edit event and add Jane and kevin@mail.internal

Expected outcome:

  • Bob sees the event one time (the one in the shared calendar, there is no copy in bob's personal calendar)
  • Bob can invite internal and external attendees to the event in the shared calendar
  • and the invitations are sent by email
  • Jane sees the event two times (1x in the personal calendar because he is also an attendee, 1x in the shared calendar)
  • Jane cannot edit the event in the shared calendar
  • Internal users see the event in the calendar
  • Users can accept/decline the event
  • The status is updated properly for every user with a local account (the reply sent to bob does not trigger a status update because the event is not found at https://github.com/nextcloud/3rdparty/blob/cbcfacd52639b3201dd2cf507da3d440ea3344fe/sabre/dav/lib/CalDAV/Schedule/Plugin.php#L492)
  • External users receive an invitation by email
  • and can use the provided link to accept/decline

@solracsf solracsf added this to the Nextcloud 30 milestone Jun 18, 2024
@kesselb kesselb removed this from the Nextcloud 30 milestone Jun 25, 2024
@SebastianKrupinski SebastianKrupinski force-pushed the fix/issue-26668 branch 2 times, most recently from a8b4ad7 to 83f6266 Compare July 21, 2024 17:48
apps/dav/lib/CalDAV/Schedule/Plugin.php Fixed Show fixed Hide fixed
apps/dav/lib/CalDAV/Schedule/Plugin.php Fixed Show fixed Hide fixed
apps/dav/lib/CalDAV/Schedule/Plugin.php Fixed Show fixed Hide fixed
apps/dav/lib/CalDAV/Schedule/Plugin.php Fixed Show fixed Hide fixed
apps/dav/lib/CalDAV/Schedule/Plugin.php Fixed Show fixed Hide fixed
@SebastianKrupinski SebastianKrupinski changed the title fix(caldav): Do not overwrite shared calendar owner fix(caldav): Add sharee to address list when calendar is shared Jul 21, 2024
@SebastianKrupinski
Copy link
Contributor Author

Summary

Sabre invitation plugin skip invites if the organizer of the event is not the owner of the calendar. That happens for shared calendars because \OCA\DAV\CalDAV\Calendar::getOwner overwrites \Sabre\CalDAV\Calendar::getOwner. Upstream returns the principaluri of the owner, we return the principaluri of the sharee. Want to test if this revives the invites in shared calendars. Adjustments for other places that use the getOwner method might be required.

Disregard... Initial comment...

The better solution is to supply both the Sharer and Sharee addresses to the iTipBroker... This keeps the calendar owner logic sounds and in tacked and fixes the reason the iTipBroker does not generate proper messages when a Sharee creates an event with attendees in the Sharers calendar.

@SebastianKrupinski
Copy link
Contributor Author

Additional thoughts...

Some front end UI changes that would be beneficial...

  • Drop down on attendee tab to selected the organizer of the event. Use cases...
  • Sharee is an assistant with access to the Managers calendar (Sharer)... The organizer of the event should be the Sharer.
  • Sharee is a employee with access to the Company calendar (Sharer)... The organizer of the event should be the Sharee.

Some back end changes that would be beneficial...

  • Additional calendar property to control access to send on behalf of the Sharer.

Draw backs...

  • The above features would only work in the NC UI. CalDAV dose support the "CALDAV:schedule-send" ACL permission but I doubt that there are many if any clients that actually support this feature.

@SebastianKrupinski SebastianKrupinski added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jul 21, 2024
@SebastianKrupinski SebastianKrupinski marked this pull request as ready for review July 24, 2024 12:44
@SebastianKrupinski
Copy link
Contributor Author

SebastianKrupinski commented Jul 24, 2024

Testing Instructions:

  1. Pull Patch and Calendar patch fix: enable attendee selection on shared calendars calendar#6202 (enables attendee selection from UI)
  2. Create invite as an Sharee (User B) on a shared calendar (from User A) with attendees (User C).
  3. Check email of User C to see if invitation arrived.
  4. Repeat step 2 and 3 by creating invite in personal calendar with attendees

@ChristophWurst ChristophWurst added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jul 24, 2024
@ChristophWurst ChristophWurst marked this pull request as draft July 24, 2024 12:55
@SebastianKrupinski
Copy link
Contributor Author

SebastianKrupinski commented Jul 28, 2024

Notes: Testing Current Shared Calendar Behavior
Configuration: User 2 (Sharer/Owner), User 1 (Sharee), User 3 (Non-Sharee), User External (Mail Address)

Case 1 Test:
User 2 (Owner) Creates event with all other users

Case 1 Outcome:
User 2 (Owner) - SEEs event in shared calendar and receives NO email
User 1 (Sharee) - SEEs two events, one in personal and one in shared calendar and RECEIVES an email
User 3 (Non-Sharee) - SEEs event in personal calendar and RECEIVES an email
User E (External) - RECEIVES an email

Case 2 Test:
User 1 (Sharee) Creates event with all other users

Case 2 Outcome:
User 1 (Sharee) - SEEs event in shared calendar and receives NO email
User 2 (Owner) - SEEs event in shared calendar and receives NO email
User 3 (Non-Sharee) - Dose NOT see event in personal calendar and receives NO email
User E (External) - receives NO email

@SebastianKrupinski
Copy link
Contributor Author

Notes: Testing New Shared Calendar Behavior
Configuration: User 2 (Sharer/Owner), User 1 (Sharee), User 3 (Non-Sharee), User External (Mail Address)

Case 3 Test:
User 2 (Owner) Creates event with all other users

Case 3 Outcome:
User 2 (Owner) - SEEs event in shared calendar and receives NO email
User 1 (Sharee) - SEEs two events, one in personal and one in shared calendar and RECEIVES an email
User 3 (Non-Sharee) - SEEs event in personal calendar and RECEIVES an email
User E (External) - RECEIVES an email

Case 4 Test:
User 1 (Sharee) Creates event with all other users

Case 4 Outcome:
User 1 (Sharee) - SEEs event in shared calendar and receives NO email
User 2 (Owner) - SEEs two events, one in personal and one in shared calendar and RECEIVES an email
User 3 (Non-Sharee) - SEEs event in personal calendar and RECEIVES an email
User E (External) - RECEIVES an email

@miaulalala
Copy link
Contributor

I tried reproducing the 1st use case from Daniel but I can't add an additional attendee from the secondary user Bob with whom the calendar was shared. Am I missing something?

@SebastianKrupinski
Copy link
Contributor Author

SebastianKrupinski commented Jul 31, 2024

@miaulalala

Did you enable the UI selection from the calendar PR?

#45054 (comment)

@miaulalala
Copy link
Contributor

Works, although the following issue might be confusing:

Admin invites Bob
Bob has 2 copies of the VEVENT - one in the shared calendar, one in the personal calendar
Bob deletes the VEVENT from the personal calendar
Bob accepts the invitiation in the shared calendar
The event in the personal calendar is recreated

@SebastianKrupinski
Copy link
Contributor Author

Works, although the following issue might be confusing:

Admin invites Bob Bob has 2 copies of the VEVENT - one in the shared calendar, one in the personal calendar Bob deletes the VEVENT from the personal calendar Bob accepts the invitiation in the shared calendar The event in the personal calendar is recreated

Pretty sure we would need to rewrite the Sabre iTipBroker class to fix this.

apps/dav/lib/CalDAV/Schedule/Plugin.php Fixed Show fixed Hide fixed
apps/dav/lib/CalDAV/Schedule/Plugin.php Fixed Show fixed Hide fixed
apps/dav/lib/CalDAV/Schedule/Plugin.php Fixed Show fixed Hide fixed
apps/dav/lib/CalDAV/Schedule/Plugin.php Fixed Show fixed Hide fixed
apps/dav/lib/CalDAV/Schedule/Plugin.php Dismissed Show dismissed Hide dismissed
@miaulalala miaulalala marked this pull request as ready for review August 5, 2024 11:38
@miaulalala miaulalala marked this pull request as draft August 5, 2024 11:38
Copy link
Contributor

@miaulalala miaulalala left a comment

Choose a reason for hiding this comment

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

Works - mentioned the double ics issue but that's not something essential.

@SebastianKrupinski SebastianKrupinski marked this pull request as ready for review August 5, 2024 15:42
@SebastianKrupinski SebastianKrupinski self-assigned this Aug 5, 2024
@SebastianKrupinski SebastianKrupinski added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Aug 5, 2024
@SebastianKrupinski SebastianKrupinski added this to the Nextcloud 30 milestone Aug 5, 2024
@Altahrim Altahrim mentioned this pull request Aug 6, 2024
Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Tested inviting myself in a calendar shared with me, setting the sharer as organizer

Works 👍

@SebastianKrupinski SebastianKrupinski merged commit 1e2bf36 into master Aug 6, 2024
168 checks passed
@SebastianKrupinski SebastianKrupinski deleted the fix/issue-26668 branch August 6, 2024 15:09
@bpmartin20
Copy link

Thank you all, most especially SebastianKrupinski, for the dedicated work on this.

I determined my skills at present were insufficient to assist in the testing, but I truly appreciate the work and am anxious to try it out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
Status: ☑️ Done
Development

Successfully merging this pull request may close these issues.

Send invitations for shared calendars
6 participants