nextcloud / server

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

Group shared calendars missing (20.0.5) #25165

Closed derBobby closed 3 years ago

derBobby commented 3 years ago

Steps to reproduce

0.) User A and User B are member of Group G 1.) User A has shared a calendar with group G 1a.) The shared calendar name contains a SPACE. 2.) User B can't see the shared calendar in the calendar web app 3.) User B can't sync the shared calendar on his devices

Expected behaviour

Group sharing of calendar should work Group shared calendars should be visible for users shared with

Actual behaviour

Group shared calendars are not visible / syncable

Server configuration detail

Operating system: Linux 4.19.0-13-amd64 #1 SMP Debian 4.19.160-2 (2020-11-28) x86_64

Webserver: Apache/2.4.38 (Debian) (apache2handler)

Database: hub.docker.com: mariadb:10.4.10-bionic

PHP version:

7.4.14 Modules loaded: Core, date, libxml, openssl, pcre, sqlite3, zlib, ctype, curl, dom, fileinfo, filter, ftp, hash, iconv, json, mbstring, SPL, PDO, session, posix, Reflection, standard, SimpleXML, pdo_sqlite, Phar, tokenizer, xml, xmlreader, xmlwriter, mysqlnd, apache2handler, apcu, bcmath, exif, gd, gmp, imagick, intl, ldap, memcached, pcntl, pdo_mysql, pdo_pgsql, redis, sodium, zip, Zend OPcache

Nextcloud version: 20.0.5 - 20.0.5.2

Updated from an older Nextcloud/ownCloud or fresh install: 20.0.4

Where did you install Nextcloud from: hub.docker.com: nextcloud:20.0.5

Signing status Array ( )
List of activated apps ``` Enabled: - accessibility: 1.6.0 - activity: 2.13.4 - admin_audit: 1.10.0 - bruteforcesettings: 2.0.1 - calendar: 2.1.3 - cloud_federation_api: 1.3.0 - comments: 1.10.0 - contacts: 3.4.3 - contactsinteraction: 1.1.0 - dashboard: 7.0.0 - dav: 1.16.2 - deck: 1.2.3 - documentserver_community: 0.1.8 - drawio: 0.9.8 - event_update_notification: 1.2.0 - federatedfilesharing: 1.10.2 - federation: 1.10.1 - files: 1.15.0 - files_pdfviewer: 2.0.1 - files_rightclick: 0.17.0 - files_sharing: 1.12.2 - files_trashbin: 1.10.1 - files_versions: 1.13.0 - files_videoplayer: 1.9.0 - firstrunwizard: 2.9.0 - forms: 2.1.0 - groupfolders: 8.2.0 - impersonate: 1.7.0 - issuetemplate: 0.7.0 - keeweb: 0.6.4 - logreader: 2.5.0 - lookup_server_connector: 1.8.0 - nextcloud_announcements: 1.9.0 - notes: 4.0.2 - notifications: 2.8.0 - oauth2: 1.8.0 - onlyoffice: 6.2.0 - password_policy: 1.10.1 - photos: 1.2.3 - previewgenerator: 3.1.0 - privacy: 1.4.0 - provisioning_api: 1.10.0 - recommendations: 0.8.0 - registration: 0.6.0 - serverinfo: 1.10.0 - settings: 1.2.0 - sharebymail: 1.10.0 - spreed: 10.0.5 - support: 1.3.0 - survey_client: 1.8.0 - systemtags: 1.10.0 - tasks: 0.13.6 - text: 3.1.0 - theming: 1.11.0 - twofactor_backupcodes: 1.9.0 - updatenotification: 1.10.0 - user_status: 1.0.1 - viewer: 1.4.0 - weather_status: 1.0.0 - workflowengine: 2.2.0 Disabled: - encryption - files_external - user_ldap ```
Configuration (config/config.php) ``` { "memcache.local": "\\OC\\Memcache\\APCu", "apps_paths": [ { "path": "\/var\/www\/html\/apps", "url": "\/apps", "writable": false }, { "path": "\/var\/www\/html\/custom_apps", "url": "\/custom_apps", "writable": true } ], "passwordsalt": "***REMOVED SENSITIVE VALUE***", "secret": "***REMOVED SENSITIVE VALUE***", "trusted_domains": [ "removed", "removed" ], "datadirectory": "***REMOVED SENSITIVE VALUE***", "dbtype": "mysql", "version": "20.0.5.2", "overwrite.cli.url": "https:\/\/removed", "overwriteprotocol": "https", "dbname": "***REMOVED SENSITIVE VALUE***", "dbhost": "***REMOVED SENSITIVE VALUE***", "dbport": "", "dbtableprefix": "", "mysql.utf8mb4": true, "dbuser": "***REMOVED SENSITIVE VALUE***", "dbpassword": "***REMOVED SENSITIVE VALUE***", "installed": true, "loglevel": 0, "mail_sendmailmode": "smtp", "mail_smtpmode": "smtp", "mail_smtpauthtype": "PLAIN", "mail_smtpsecure": "tls", "mail_from_address": "***REMOVED SENSITIVE VALUE***", "mail_domain": "***REMOVED SENSITIVE VALUE***", "mail_smtpauth": 1, "mail_smtphost": "***REMOVED SENSITIVE VALUE***", "mail_smtpport": "587", "mail_smtpname": "***REMOVED SENSITIVE VALUE***", "mail_smtppassword": "***REMOVED SENSITIVE VALUE***", "mail_smtpdebug": false, "mail_smtptimeout": 10, "maintenance": false, "allow_user_to_change_display_name": false, "auth.bruteforce.protection.enabled": true, "trashbin_retention_obligation": "730, auto", "trusted_proxies": "***REMOVED SENSITIVE VALUE***", "forwarded_for_headers": [ "HTTP_X_FORWARDED_FOR", "X-Real-IP" ], "instanceid": "***REMOVED SENSITIVE VALUE***", "theme": "", "app_install_overwrite": [ "keeweb" ] } ```

Are you using external storage, if yes which one: NO

Are you using encryption: NO

Are you using an external user-backend, if yes which one: NO

Client configuration

Browser: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:84.0) Gecko/20100101 Firefox/84.0

Operating system: Windows 10

Logs

Web server error log ``` Insert your web server log here ```
Nextcloud log ``` ```
Browser log
derBobby commented 3 years ago

might be related to #25164

nursoda commented 3 years ago

Same issue here. No changes except for the core update.

In one case I could work around the issue by unsharing and resharing the calendar.

In another case on the same inscance when doing so it seemed (from the result in the actual sharing dialog) that it succeeded but a reload of calendar showed that the shared-to group vanishes from the list. So again: When I enter the sharing user's calendar, I see that the calendar is shared to the group. I delete that sharing. I re-add it (and see that it "is"). Then reload the web page and it's gone.

I can reliably share to users though. So in some cases it may be a (cumbersome) workaround to temporarily share to affected users directly.

nursoda commented 3 years ago

might be related to #25165

self-ref – I guess you meant another issue?

derBobby commented 3 years ago

Thank you. Updated.

jmechnich commented 3 years ago

In our case this issue is restricted to groups containing a whitespace in their name. Another user reports similar problems when an umlaut is used (see German Nextcloud forum).

Shared addressbooks are affected as well.

I can confirm that this issue exists since we upgraded to 20.0.5 on Friday evening.

Our setup is on Debian Buster with apache 2.4.38, PHP 7.3 and MariaDB 10.3.27. The logs don't show any obvious errors (except 404 errors for the missing DAV shares).

derBobby commented 3 years ago

Thank you @jmechnich !

Do you know if it is enough to rename to group in the UI or would it be necessary to rename the gid?

MariaDB [nextcloud]> select * from groups;
+------------------+------------------+
| gid              | displayname      |
+------------------+------------------+
...
+------------------+------------------+

It is probably not a good idea to change the gid and displayname via database, right?

jmechnich commented 3 years ago

Do you know if it is enough to rename to group in the UI or would it be necessary to rename the gid?

Is this even possible via the UI? :D I have only seen a trash symbol so far...

At this point I am still trying to avoid modifying the database directly. The groups table is not the only one where the gid seems to come into play (e.g. dav_shares).

jmechnich commented 3 years ago

There was a merged pull request related to DAV shares and URL de-/encoding which might be related: https://github.com/nextcloud/server/commit/25143fc87b814df93405e6de6ce18434de4f8bde

jmechnich commented 3 years ago

There was a merged pull request related to DAV shares and URL de-/encoding which might be related: 25143fc

After reverting those changes things seem to be working properly on my test server. Still a bit reluctant to implement them on our production machine as there might be side effects.

Edit: this would also fix https://github.com/nextcloud/server/issues/25164

jmechnich commented 3 years ago

I can confirm that reverting https://github.com/nextcloud/server/commit/25143fc87b814df93405e6de6ce18434de4f8bde also fixes the issue on our production machine.

tflidd commented 3 years ago

So, this is related to PR https://github.com/nextcloud/server/pull/24515

@blizzz worked on that...

derBobby commented 3 years ago

Updated the whitespace hint to my initial post.

@jmechnich Thank you, I appreciate your analysis! :-) Is the rollback to the old file safe?

regenpfeifer commented 3 years ago

Same issue here, c.f.: https://help.nextcloud.com/t/probleme-mit-kalender-freigabe/104490 Calendars are not shareable to groups containing a SPACE. Existing shares to such groups have disappeared.

jmechnich commented 3 years ago

@jmechnich Thank you, I appreciate your analysis! :-) Is the rollback to the old file safe?

@derBobby It should be quite safe in my opinion but would still advise you to create a backup of your cloud... ;)

JB1985 commented 3 years ago

Does this patch fix the problem? Has anyone tested that?

simonspa commented 3 years ago

@JB1985 if I understand the issue correctly then the patch you referring to is the cause, not the remedy of this. The patch you linked has been included in 20.0.5, which is the version from which on users experience this problem.

JB1985 commented 3 years ago

Then the solution should be to take the files from 2.0.4?!

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

Has anyone tested that?

jmechnich commented 3 years ago

@JB1985 https://github.com/nextcloud/server/issues/25165#issuecomment-761617302

k00ni commented 3 years ago

Is the root cause maybe also responsible for https://github.com/nextcloud/contacts/issues/2023?

I have the same behavior for address books. If target group contains a white space, a share can be created but users of the group don't see it.

jmechnich commented 3 years ago

Is the root cause maybe also responsible for nextcloud/contacts#2023?

I have the same behavior for address books. If target group contains a white space, a share can be created but users of the group don't see it.

@k00ni That is very likely the case.

kevinkk525 commented 3 years ago

fix a bug and create 2 more.. How about releasing a hotfix version?

manu-p commented 3 years ago

Same issue for us without any space characters in group names: "salariés" and "bénévoles"...

nursoda commented 3 years ago

Same issue for us without any space characters in group names: "salariés" and "bénévoles"...

In my case "…ä…" I was able to un-share and re-share, that fixed it. As long as there are no space chars. If there are, it's not saved or displayed/used after re-sharing. Might be a quick and easy workaround.

etr999 commented 3 years ago

I do not recommend this workaround for unexperienced or very scared people! Anyway, it worked in our case ;) Make a backup of your database (at least the tables mentioned below) Enable nextcloud maintainance mode.

If you have access to your nextcloud database, you can rename/transliterate the critical field contents.

AFAIK the DAV-sharing stuff is spread over 3 tables:

oc_groups oc_dav_shares oc_group_user

Replacing the spaces with underscores solved the problem:

update oc_groups SET gid = REPLACE(gid,' ','_');
update oc_dav_shares SET principaluri = REPLACE(principaluri,' ','_');
update oc_group_user SET gid = REPLACE(gid,' ','_');

Replace space with any arbitrary "critical" character.

Watch out and have fun!

blizzz commented 3 years ago

How old where those calendar shares, i.e. when did you create them? I failed to do so in 20.0.4.

nursoda commented 3 years ago

How old where those calendar shares, i.e. when did you create them? I failed to do so in 20.0.4.

In my case NC19.0.x or NC20.0.x

jmechnich commented 3 years ago

How old where those calendar shares, i.e. when did you create them? I failed to do so in 20.0.4.

@blizzz Not very recently (some of them are older than two years).

manu-p commented 3 years ago

@blizzz same as @jmechnich for us, at least in 18.x

kevinkk525 commented 3 years ago

my instance is so old, it was owncloud once.. does that mean I have to be careful and test each minor update before updating my production instance?? Nextcloud should learn how to do proper automated testing of their code to detect errors like that.

mklemme1 commented 3 years ago

@kevinkk525 We also recently migrated from OC.

I fixed the issue by replacing the 4 files as described by @JB1985 yesterday. I would not bother changing the database.

Testing always help. I did a test - but I did not see this issue... :-(

blizzz commented 3 years ago

Sounds like there were changes on the way. I'll investigate this.

JB1985 commented 3 years ago

@kevinkk525 We also recently migrated from OC.

I fixed the issue by replacing the 4 files as described by @JB1985 yesterday.

Yes it works!

blizzz commented 3 years ago

Sounds like there were changes on the way. I'll investigate this.

Okay, I tested with group names containing semicolon, question mark, slash, ampersand, hash, and arrow brackets. Kind of mean to have boring whitespaces as party pooper 🤦‍

The issue is that before 20.0.5 the uri of the shared calendar was saved unencoded. Spaces stay spaces and umlauts stay umlauts, while it failed to create the share of the group id contained one of the afore mentioned characters.

The underlying problem her is that principal uris in the dav share table were saved un-uri-encoded (if they made it this far). There was an old "fix" that just compared them in this style.

The solution is going to be to repair these entries. Delicate.

k00ni commented 3 years ago

For what its worth, Nextcloud showed me group names with white spaces like Foo%2bBar in the share setting (whereas %2b is the white space).

jmechnich commented 3 years ago

For what its worth, Nextcloud showed me group names with white spaces like Foo%2bBar in the share setting (whereas %2b is the white space).

But that's kind of weird, as %2b is a + sign. Could it be that urlencode (" " -> "+") and rawurlencode (" " -> "%20") are used inconsistently?

Edit: probably in this case, double encoding with urlencode (" " -> "+" -> "%2b").

JB1985 commented 3 years ago

The solution is going to be to repair these entries. Delicate.

How shall this be understood? @blizzz

Does that mean users have to find a solution or will there be a fix?

blizzz commented 3 years ago

The solution is going to be to repair these entries. Delicate.

How shall this be understood? @blizzz

Does that mean users have to find a solution or will there be a fix?

Of course we'll have to fix it. I looked into this extensively today but the whole topic is a little bit bigger than this. But this issue uncovered it. Finding a good way to repair the entries is what I am working on.

blizzz commented 3 years ago

For now as a workaround you can apply this patch:

diff --git a/apps/dav/lib/CalDAV/CalDavBackend.php b/apps/dav/lib/CalDAV/CalDavBackend.php
index ea1a30c629..21f570ffce 100644
--- a/apps/dav/lib/CalDAV/CalDavBackend.php
+++ b/apps/dav/lib/CalDAV/CalDavBackend.php
@@ -346,6 +346,9 @@ class CalDavBackend extends AbstractBackend implements SyncSupport, Subscription
        $principals = $this->principalBackend->getGroupMembership($principalUriOriginal, true);
        $principals = array_merge($principals, $this->principalBackend->getCircleMembership($principalUriOriginal));

+       foreach ($principals as $principal) {
+           $principals[] = urldecode($principal);
+       }
        $principals[] = $principalUri;

        $fields = array_values($this->propertyMap);
diff --git a/apps/dav/lib/CardDAV/CardDavBackend.php b/apps/dav/lib/CardDAV/CardDavBackend.php
index d5c3609695..2946b9dd34 100644
--- a/apps/dav/lib/CardDAV/CardDavBackend.php
+++ b/apps/dav/lib/CardDAV/CardDavBackend.php
@@ -192,6 +192,9 @@ class CardDavBackend implements BackendInterface, SyncSupport {
        $principals = $this->principalBackend->getGroupMembership($principalUriOriginal, true);
        $principals = array_merge($principals, $this->principalBackend->getCircleMembership($principalUriOriginal));

+       foreach ($principals as $principal) {
+           $principals[] = urldecode($principal);
+       }
        $principals[] = $principalUri;

        $query = $this->db->getQueryBuilder();

Basically it adds those three lines in two files each. No changes in the database.

The principal is then tested against group principals as they are fetched and decoded. There is the theoretical danger that a user will get access to shares to a group they are not member of – when you have group names at the same time that would be identical after decoding. For example:

Both must exists and have different users.

k00ni commented 3 years ago

Thank you @blizzz. You wrote:

The principal is then tested against group principals as they are fetched and decoded. There is the theoretical danger that a user will get access to shares to a group they are not member of – when you have group names at the same time that would be identical after decoding.

Does that mean, that there is more code to come or does your code fix the problem entirely?

blizzz commented 3 years ago

Thank you @blizzz. You wrote:

The principal is then tested against group principals as they are fetched and decoded. There is the theoretical danger that a user will get access to shares to a group they are not member of – when you have group names at the same time that would be identical after decoding.

Does that mean, that there is more code to come or does your code fix the problem entirely?

This is rather a hotfix you can apply right now.

RafalLukawiecki commented 3 years ago

I can add that in addition to calendars, none of our users can see any group-shared reminders/task lists either. This happened after update to 20.0.5.

All our group names have a space in them. Group names were created when the initial Nextcloud was installed, the last 18 release. Server was then regularly updated, through 19 and 20, and everything worked well till 20.0.4.

This is a major inconvenience as it prevents collaboration since no one can see each other’s tasks and calendars, something essential to our daily workflow.

wscheicher commented 3 years ago

For now as a workaround you can apply this patch: [...]

Hmm ... i'm on 20.0.5 and this change didn't seem to help. Is this supposed to fix existing shares? Would this also cover if the group in question is a ldap group? ("Domain Users")

RafalLukawiecki commented 3 years ago

The comment from the workaround above is important, thank you for sharing it @blizzz

There is the theoretical danger that a user will get access to shares to a group they are not member of – when you have group names at the same time that would be identical after decoding. For example:

  • Friends & Family
  • Friends+%26+Family

Both must exists and have different users.

I would be concerned that this creates opportunity for intentional attacks. Someone could arrange to have multiple groups with specially chosen characters, and all would be granted access whilst only one of them should have been. This would be a notifiable vulnerability, and it needs to be avoided. A security principal must not resolve to more identities than the one it is defined for. I hope a fix to this problem is possible without causing such issues.

I suspect any solution would need to take care of older principals that have spaces etc in their names and either force a conversion with the significant negative side-effect of breaking any other apps that relied on them, or deal with the "version" of the principal by checking its creation date. In general, this is a case for versioning of the security principal to avoid exactly such issues in the future, and to reconsider using display names as the underlying principals—but it may be too late for such a NC-wide decision...

RafalLukawiecki commented 3 years ago

The workaround from @JB1985 has worked for me.

Then the solution should be to take the files from 2.0.4?!

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

Has anyone tested that?

However, if you have tried re-sharing a calendar (or task list) whilst in v20.0.5, perhaps while troubleshooting, then that calendar will not appear for the group members. You need to share it again, using the same name etc, and then it will reappear for the group members. Calendars which I have not touched while troubleshooting have just reappeared after putting those files back without any further action. Thank you @JB1985.

blizzz commented 3 years ago

The comment from the workaround above is important, thank you for sharing it @blizzz

There is the theoretical danger that a user will get access to shares to a group they are not member of – when you have group names at the same time that would be identical after decoding. For example:

  • Friends & Family
  • Friends+%26+Family

Both must exists and have different users.

I would be concerned that this creates opportunity for intentional attacks. Someone could arrange to have multiple groups with specially chosen characters, and all would be granted access whilst only one of them should have been. This would be a notifiable vulnerability, and it needs to be avoided. A security principal must not resolve to more identities than the one it is defined for. I hope a fix to this problem is possible without causing such issues.

The groups have to exist and the calendar (or address book) have to be shared with it. All in all I think it is more a hypothetical risk. Group creation is limited to admins. Admins don't need hacks like these.

But anyways, that's why I pointed it out.

I suspect any solution would need to take care of older principals that have spaces etc in their names and either force a conversion with the significant negative side-effect of breaking any other apps that relied on them

That's goes all via the DAV app and API, no direct DB access. It should not lead to breakage of other apps.

or deal with the "version" of the principal by checking its creation date. In general, this is a case for versioning of the security principal to avoid exactly such issues in the future, and to reconsider using display names as the underlying principals—but it may be too late for such a NC-wide decision...

Currently it's only stored which resources was shared to which principal.

blizzz commented 3 years ago

For now as a workaround you can apply this patch: [...]

Hmm ... i'm on 20.0.5 and this change didn't seem to help. Is this supposed to fix existing shares? Would this also cover if the group in question is a ldap group? ("Domain Users")

Yes. How does its principaluri in SELECT * FROM oc_dav_shares look like? principals/groups/Domain Users?

wscheicher commented 3 years ago

Yes. How does its principaluri in SELECT * FROM oc_dav_shares look like? principals/groups/Domain Users?

Hmm - no, this is missing. And the share is now not in the webinterface anymore (and recreating the entry vanishes on page reload)

blizzz commented 3 years ago

Yes. How does its principaluri in SELECT * FROM oc_dav_shares look like? principals/groups/Domain Users?

Hmm - no, this is missing. And the share is now not in the webinterface anymore (and recreating the entry vanishes on page reload)

None of this code would remove existing shares.

tims-adb commented 3 years ago

Last night came the update to NC 20.0.6 and I can confirm the problem persists.

Temporary workaround: Manually replacing the files from 20.0.4 again did it for us: 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

JB1985 commented 3 years ago

Last night came the update to NC 20.0.6 and I can confirm the problem persists.

Why is a new version released and this issue not resolved? This bug affects a lot of users.

I think this will be a never-ending story. I don't think the problem will be fixed.

@blizzz pls replace the 4 Files for 20.0.7 until a solution to the problem is found.