nextcloud / server

☁️ Nextcloud server, a safe home for all your data
https://nextcloud.com
GNU Affero General Public License v3.0
27.57k stars 4.08k forks source link

Can't share calendar with users / groups if name contains spaces or umlauts #17610

Closed charlessteiner closed 3 years ago

charlessteiner commented 5 years ago

Steps to reproduce

  1. create a user with a space in the username
  2. attempt to share a calendar event with user from step 1
  3. event doesn't create

Expected behaviour

Tell us what should happen Create event with users with space in username or prohibit users with a space in the username from being created. Documentation does state that "Login names may contain letters (a-z, A-Z), numbers (0-9), dashes (-), underscores (_), periods (.) and at signs (@)"

Actual behaviour

Tell us what happens instead spaces in usernames are accepted and shared events with such a user fail

Server configuration

CentOS Linux release 7.5.1804 Operating system: CentOS Linux release 7.5.1804

Web server: Apache/2.4.6

Database:mysql Ver 15.1 Distrib 5.5.60-MariaDB, for Linux (x86_64) using readline 5.1

PHP version: PHP 7.2.11 (cli)

Server version: (see your admin page) Nextcloud 15.0.5

Calendar version: (see the apps page) 1.6.4

Updated from an older installed version or fresh install: Fresh, and duplicated the error on a second fresh Signing status (ownCloud/Nextcloud 9.0 and above):

Login as admin user into your cloud and access
http://example.com/index.php/settings/integrity/failed
paste the results here.

List of activated apps:

If you have access to your command line run e.g.:
sudo -u www-data php occ app:list
from within your instance's installation folder

Nextcloud configuration:

If you have access to your command line run e.g.:
sudo -u www-data php occ config:list system
from within your instance's installation folder

or

Insert your config.php content here
Make sure to remove all sensitive content such as passwords. (e.g. database password, passwordsalt, secret, smtp password, …)

Are you using external storage, if yes which one: local/smb/sftp/...

Are you using encryption: yes/no

Are you using an external user-backend, if yes which one: LDAP/ActiveDirectory/Webdav/...

LDAP configuration (delete this part if not used)

With access to your command line run e.g.:
sudo -u www-data php occ ldap:show-config
from within your instance's installation folder

Without access to your command line download the data/owncloud.db to your local
computer or access your SQL server remotely and run the select query:
SELECT * FROM `oc_appconfig` WHERE `appid` = 'user_ldap';

Be sure to replace sensitive data as the name/IP-address of your LDAP server or groups.

Client configuration

Browser:

Operating system:

CalDAV-clients:

Logs

Web server error log

Insert your webserver log here

Log file (data/nextcloud.log)

Insert your nextcloud.log file here

Browser log

Insert your browser log here, this could for example include:

a) The javascript console log
b) The network log
c) ...
--- Want to back this issue? **[Post a bounty on it!](https://www.bountysource.com/issues/70916258-users-created-with-space-in-username-cannot-have-events-shared-with-them-fresh-duplicated?utm_campaign=plugin&utm_content=tracker%2F45525646&utm_medium=issues&utm_source=github)** We accept bounties via [Bountysource](https://www.bountysource.com/?utm_campaign=plugin&utm_content=tracker%2F45525646&utm_medium=issues&utm_source=github).
stefan-muc commented 5 years ago

Just a question: Do you have this setting enabled? Admin -> Settings -> Sharing -> [x] Restrict users to only share with users in their groups

I thought I have a same problem as you (couldn't share with a group that included a space in the name), but it turned out that this setting caused the problems - I haven't been member of this group, so I couldn't share with this group.

der-daniel commented 4 years ago

I did some debugging. First thing:

https://github.com/nextcloud/server/blob/12bdb95c1a40170652570decef7f466e4b97aa65/apps/dav/lib/DAV/Sharing/Backend.php#L109-L113

compares the group id with the name part of the URI.

Example: group id: Test Group uri: principals/groups/Test%20Group

then Test%20Group is compared to the Test Group.

replacing it with e.g.

($principal[1] === 'groups' && !$this->groupManager->groupExists(str_replace("%20", " ", $principal[2])))) {

fixes the issue of saving the share to the database.

So, in general, we need to e.g. modify groupExists to use the url decoded version of $principal[2].

But still the other users in the group are not able to see the calendar in their view. There must be something similar when retrieving the calendar.

tcitworld commented 4 years ago

@der-Daniel You can use urldecode($principal[2]) instead of simply str_replaceing the %20s.

der-daniel commented 4 years ago

@tcitworld I know I know :smile: This was more about digging into it.

der-daniel commented 4 years ago

Ok I spend like 5 hours in the backend, only to find out that it was a decoding issue in the front end of the calendar.

Check https://github.com/nextcloud/calendar/pull/1985

georgehrke commented 4 years ago

@der-Daniel Please see my comment in https://github.com/nextcloud/calendar/pull/1985#issuecomment-586738183. I'm happy to assist in debugging this in the server.

der-daniel commented 4 years ago

As far as I can tell, my my fix in the front end resolve this bug. I also first looked into the backend. The backend is fine.

I am able to share the cal with groups containing whitespace and umlauts.

What makes you think the backend is buggy?

der-daniel commented 4 years ago

Take a look at the contacts app where this bug is not occurring. I compare the share function in vue and the calendar app is missing the uri decode.

Then it is working properly.

georgehrke commented 4 years ago

Please check the response of the principal property search, as I described in https://github.com/nextcloud/calendar/pull/1985#issuecomment-586738183:

6F172EED-2028-47B5-9BA7-5D219F082D0A

The contents of d:href is /remote.php/dav/principals/groups/group%20with%20space/. We are just taking the value of d:href and putting it into a oc:invite block.

So there are two possible solutions to this:

My point here is just that as a CalDAV client (which the Calendar app is), I'm expecting to just reuse properties i get from the server without having to encode them.

@rullzer @icewind1991 This is the issue I mentioned two weeks ago before my vacation.

der-daniel commented 4 years ago

Ahh that's why you called it hot fix. Nice. :smile:

Tbh, when I digged into it I thought the fix would involve a more reasonable group id. As the group id in the database is an unnormalized string with umlate / : whitespace and so on.

The urlencode problem is a problem of responsibilities. If it is the responsibility of the backend to accept URL encoded and non encoded string, otherwise the front end should stick to what the backend expects.

But you have a valid point. This is just a hot fix.

der-daniel commented 4 years ago

Will this hot fix make it into production anytime soon. As this fix is also used in the contacts app, I would really appreciate if this fix can be used until the backend is reworked.

See here:

https://github.com/nextcloud/contacts/blob/7724fb3c9bdedf7b8fee61967adbd8db78834793/src/components/Settings/SettingsAddressbookShare.vue#L98-103

Because as of now, I am unable to share calendars in the nextcloud installation of my sports club :|

georgehrke commented 4 years ago

Also cc @raimund-schluessler @skjnldsv about what they would consider the expected behaviour here.

raimund-schluessler commented 4 years ago

My point here is just that as a CalDAV client (which the Calendar app is), I'm expecting to just reuse properties i get from the server without having to encode them.

This sounds about right. Tbh. I just adjusted the sharing code from the Contacts app for the Tasks app, that's why it is encoded there as well.

I am fine with fixing it in server. But as soon was we change it in the server, we will break Contacts and Tasks. So we have to release adjusted versions at the same time. Since it will probably take time to release a fixed server version, how about hot-fixing it in Calendar, and adjust Calendar, Contacts and Tasks once the fix is in server?

georgehrke commented 4 years ago

Okay, it probably makes most sense to fix it in the server with 19 and just add a version check in the contacts / calendar / tasks app to see whether to encode it yourself or not.

tcitworld commented 4 years ago

stop encoding the path like that in d:href.

If I read RFC 4918 correctly it should always be encoded.

Note that even though the server may be storing the member resource internally as 'a test', it has to be percent-encoded when used inside a URI reference (see Section 2.1 of [RFC3986]). Also note that a legal URI may still contain characters that need to be escaped within XML character data, such as the ampersand character.


But as soon was we change it in the server, we will break Contacts and Tasks.

Why ? urldecode('group%20with%20space') === urldecode('group with space')

georgehrke commented 4 years ago

Why ? urldecode('group%20with%20space') === urldecode('group with space')

But we also allow % in group names ... So you can't tell if %20 is an encoded space or the result of a decoded %2520

der-daniel commented 4 years ago

This is why I mentioned that the group id needs to be properly normalized in the database. The comparison done in the back end is basically a plain old string equals on any strings.

An integer or a uuid may be a better fit. :thinking:

georgehrke commented 4 years ago

An integer or a uuid may be a better fit. 🤔

Yes, ideally. But that's easier said than done if you already have millions of instances out there where people use all kind of weird characters in userid / groupids. Migrating to numerical user and group ids would involve touching at least half the database tables we have.

der-daniel commented 4 years ago

Yes, this will be very tricky :unamused:

From the top of my hand:

As group names need to be unique maybe, one can transform them into string based uuid like ceda14f0-8508-412b-a794-4d8a9d348b02 using a hash or other deterministic functions during a migration step iff the group id does not match the uuid pattern.

georgehrke commented 4 years ago

We implemented a fix in the calendar for now. Proper fix would still include fixing the issues mentioned further above.

Putting into backlog for now.

caribouW3 commented 4 years ago

Hi, We seem to have a somewhat similar issue when trying to share a calendar with a group (mapped from LDAP) which has a '/' in its name. It seems that string normalization for the field owncloud_name in table oc_ldap_group_mapping could be a solution. Should I open a new issue for this one?

skjnldsv commented 4 years ago

So there are two possible solutions to this:

  • stop encoding the path like that in d:href.

For me this is the way to go

skjnldsv commented 4 years ago

Found it, finally! https://github.com/sabre-io/dav/commit/03fb8ed6d5656c68a7d87f8d07f8c3e15da4812c#diff-143c04617d0235612a5f380189c9b421aca777627b8dbc504179067bf29be16fR125 https://github.com/sabre-io/dav/pull/719

Related https://github.com/owncloud/core/issues/33594 cc @PVince81

julian70400 commented 3 years ago

The problem still occured.

Nextcloud 20.0.6

I am creating a group like this : "Comm Info"

The group name if showed perfectly everywhere, except in the activity log.. it show : You shared Agenda with group Com%2binfo

And, my share doesn't work.

And if I realize the same test with a group with a single word, it works like a charm !

Please, thanks to provide a simple way to fix this.

szaimen commented 3 years ago

Please, thanks to provide a simple way to fix this.

You've mentioned a workaround yourself:

And if I realize the same test with a group with a single word, it works like a charm !

Just don't use spaces in group names and everything will work.

alasserr commented 3 years ago

Just don't use spaces in group names and everything will work.

The last reply is clearly contemptuous ! In my case : everything was working fine (with spaces and "[ ]" in the group names) and since I updated Nextcloud it has stopped working. Should I do say to all my users : "ok folks we have to modify every shares we've made (folders, calendars etc ...) as we have found an easy solution to an issue that should not be happening anyway, and that is to change the names of the existing and previously perfectly up and running group names ?" Can we be serious ?

julian70400 commented 3 years ago

Just don't use spaces in group names and everything will work.

The last reply is clearly contemptuous !

That's why I did not give an answer ;)

Anyway, try this solution, it worked for me : https://github.com/nextcloud/server/issues/25165#issuecomment-762140026

alasserr commented 3 years ago

Thanks @julian70400 , I was just checking on this solution. For people using Nextcloud version 19 (I am using the 19.08 version) : I applied the given answer (#25165) taking the four files from the 19.04 version of Nextcloud (https://download.nextcloud.com/server/releases/nextcloud-19.0.4.zip), and it seems OK (well, at first, the shared calendars came back). Hopefully a patch will be inserted in the next releases of Nextcloud 🤞 🙏

MegaS0ra commented 3 years ago

Same issue here... As it is not yet possible to rename groups, it's a bummer to have to create new groups without spaces, and add all users to the new groups...

I'm using Nextcloud 20, not sure the workaround with replacing the 4 files will work ?

alasserr commented 3 years ago

Same issue here... As it is not yet possible to rename groups, it's a bummer to have to create new groups without spaces, and add all users to the new groups...

I'm using Nextcloud 20, not sure the workaround with replacing the 4 files will work ?

As explained in #25165 :

Just extract the 20.0.4 Nextcloud zip on a folder somewhere, and copy/paste these 4 files on your working copy of Nextcloud :

apps/dav/lib/CalDAV/CalDavBackend.php
apps/dav/lib/CardDAV/CardDavBackend.php
apps/dav/lib/DAV/GroupPrincipalBackend.php
apps/dav/lib/DAV/Sharing/Backend.php

That's it.

Nota : on the #25165 thread they're speaking about a repair/patch on the last 20.0.8 Nextcloud version. They are looking for people to test and verify it..

MegaS0ra commented 3 years ago

Thank you so much, just done this an it work ! I can now share calendars with groups that have spaces in their name.

I have also posted on the #25165 thread Thank you

Slydder commented 3 years ago

This bug is still in version 21.0.0

Will try the same "hot fix" and see if those 4 files will fix it in 21.0.0 as well.

Slydder commented 3 years ago

And have verified that the hot fix still seems to work. will need further testing to ensure that nothing else is broken due to major version bump.

chatlumo commented 3 years ago

Hello, I updated to NC 20.0.8 and that bug is still here.

Booteille commented 3 years ago

Hi. We're using groups and calendar at work and this issue is at this time really annoying. Also, our provider won't use the fix proposed in this issue considering potential unknown bugs. We would prefer an official fix.

Would it be possible to consider upgrading the priority of this issue?

Thanks.

phlegx commented 3 years ago

We have the same problem! It affects contact address book sharing. File sharing and calendar sharing works like expected.

Version: 20.0.8.

ChristophWurst commented 3 years ago

Please see https://github.com/nextcloud/calendar/pull/3232 for a potential fix.