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

fixes sharing to group ids with characters that are being url encoded #24515

Merged
merged 3 commits into from
Dec 18, 2020

Conversation

blizzz
Copy link
Member

@blizzz blizzz commented Dec 2, 2020

When you try to share a calendar to (local) group "Route#44" it will fail, because internally the group name was not decoded.

What also was and still is not the case that on failed sharing a proper error was being reported. I.e. at the moment you do not see that it was failing, only with a reload (or a complaint by one of the members :)). That might be a follow up PR.

@@ -107,6 +107,7 @@ private function shareWith($shareable, $element) {
return;
}

$principal[2] = urldecode($principal[2]);
Copy link
Member

Choose a reason for hiding this comment

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

This breaks groups with a plus as it replaces them with even if it might not be needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Did you try this? Groups containing plusses should suffer the exact same issue.

But I think it reveals a different problem here … with and without this change groups containing plusses are not being offerend in the calendar share dialogue.

Copy link
Member Author

@blizzz blizzz Dec 3, 2020

Choose a reason for hiding this comment

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

Ok, i see the issue now.

It only works # because it does not decode into something else. But, as said above, this is a different problem, only unveiled here.

Copy link
Member Author

@blizzz blizzz Dec 3, 2020

Choose a reason for hiding this comment

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

@nickvergessen I added another commit fixing it. But as said, also without the first commit and the calendar changes, a search for groups containing a plus would end up with

    <?xml version="1.0" encoding="utf-8"?>
    <d:error xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">
      <s:exception>Sabre\DAV\Exception\NotFound</s:exception>
      <s:message>Principal with name POW+WOW not found</s:message>
    </d:error>

(I have not found a similar piece of code for other types like users or circles, i may know to little about the related dav bits)

@blizzz blizzz added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Dec 10, 2020
@rullzer rullzer mentioned this pull request Dec 14, 2020
59 tasks
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz blizzz force-pushed the fix/noid/dav-share-groups-special-chars branch from fe59be1 to 622d028 Compare December 16, 2020 21:43
@blizzz
Copy link
Member Author

blizzz commented Dec 16, 2020

I was finally having another look into this and had to do two more changes (also for carddav) essentially reverting #5298 which ironically was a fix for the same problem. I suppose it broke somewhere else.

For me it works now with both contacts as well as addressbooks, and I tested with several groups:

Screenshot_20201216_224709

I shared addressbook as well as calendars to these groups.

Screenshot_20201216_224804

Screenshot_20201216_224832

(here you also see that contacts has presentation issues - needs a fix like calendar got)

and all were received:

Screenshot_20201216_224928

Screenshot_20201216_225009

In the database URIs are saved with encoded name (as they should):

Screenshot_20201216_225038

My changes do not! change the behaviour of how the URIs are stored. What changes is the group search (Sharing/Backend changes) and the return of URIs in an properly encoded manner (other changes). Therefore I do not expect any effects on old shares, even though I do not have deep knowledge of the changes in between in these components or these components in itself.

@blizzz blizzz added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Dec 16, 2020
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

🐘

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.

Code looks good

@ChristophWurst ChristophWurst merged commit fbf25e1 into master Dec 18, 2020
@ChristophWurst ChristophWurst deleted the fix/noid/dav-share-groups-special-chars branch December 18, 2020 11:01
@blizzz
Copy link
Member Author

blizzz commented Dec 18, 2020

/backport to stable20

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants