-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
feat: add iMip Request Handling #47826
Conversation
0cb241a
to
6f435ca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CS
2815cd9
to
a25d57a
Compare
lib/public/Calendar/ICalendar.php
Outdated
* | ||
* @since 31.0.0 | ||
*/ | ||
public function isWritable(): bool; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if ICalendar is only implemented in the server. There can be apps that implement \OCP\Calendar\ICalendarProvider
and return their own implementations of ICalendar. This change would break them until they add isWritable and isShared. See ICreateFromString
and IHandleImipMessage
. The same trick might be needed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally I left these to methods out of the Interface and only put them in the implementation but that made psalm grumpy.
ERROR: UndefinedInterfaceMethod - lib/private/Calendar/Manager.php:263:46 - Method OCP\Calendar\ICreateFromString::isWritable does not exist (see https://psalm.dev/181)
ERROR: UndefinedInterfaceMethod - lib/private/Calendar/Manager.php:263:73 - Method OCP\Calendar\ICreateFromString::isShared does not exist (see https://psalm.dev/181)
What if I added these to methods to the psalm ignore list? And left them commented out in the interface with a note for later, that way we can give developers a version or two to implement them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a hack. Psalm complains rightfully. App developers have the interface as contract. Anything that's not in the interface must not be called, so it would also not be usable in Mail.
If you want to learn more about this, read up on the paradigm program to an interface, not an implementation :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
a25d57a
to
0504968
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to test it locally but it did not work. I sent an invite from another external service, added it to my calendar and then moved the event in the external service. A REQUEST
with the new dates was sent and received but the event was not updated.
The problem is that no recipient is added to iMIP messages with method REQUEST
in
server/apps/dav/lib/CalDAV/CalendarImpl.php
Lines 237 to 247 in 7a2fe4a
if ($iTipMessage->method === 'REPLY') { | |
if ($server->isExternalAttendee($vEvent->{'ATTENDEE'}->getValue())) { | |
$iTipMessage->recipient = $organizer; | |
} else { | |
$iTipMessage->recipient = $attendee; | |
} | |
$iTipMessage->sender = $attendee; | |
} elseif ($iTipMessage->method === 'CANCEL') { | |
$iTipMessage->recipient = $attendee; | |
$iTipMessage->sender = $organizer; | |
} |
This causes the local delivery to fail in https://github.com/nextcloud/3rdparty/blob/b3cc57b68388b68e4d4dcf0ddcd8c7ba455dd840/sabre/dav/lib/CalDAV/Schedule/Plugin.php#L418-L423 because no recipient is set and thus no principal is available.
I think another if/else branch needs to be added to the statement above.
Please let me know in case I'm doing something wrong.
Okay, I lets talk about it quickly tomorrow. It worked for the two services I used. Might be just the way the external service is sending the message. |
Nope, this was my blunder, I somehow overwrote or left out the code changes in CalendarImpl::handleIMipMessage(), I don't remember at this point, its been over a month. Logic is fixed. The fun of working on multiple issues in the same file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and works beautifully now. Thanks!
91f2ba0
to
4bc3d19
Compare
Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>
4bc3d19
to
7ebeed4
Compare
@ChristophWurst should we back port this to 30? |
This PR adds a new API. We generally don't backport these. So it's a no here. |
Summary
Checklist