User groups with "/" character in the name can be deleted, but reappear

[bpcurse]

Steps to reproduce

1. Log in as admin user
2. Go to the user management
3. Create a group with "/" in the name, e.g. "testgroup r/w"
4. Delete the group
5. Change somewhere, e.g. to files
6. Go back to the user management
7. Deleted group has reappeared

Expected behaviour

Group should be permanently deleted

Actual behaviour

Deleted Group reappears

Server configuration

Operating system:
Linux dd42018 4.4.0-87-generic #110-Ubuntu SMP Tue Jul 18 12:55:35 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux

Web server:
Shared Webhosting

Database:
mySQL 5.7.15

PHP version:
5.6.30

Nextcloud version: (see Nextcloud admin page)
12.0.1 stable

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

I hope this is not already filed as an issue. At least I could not find it.

[bpcurse]

Will this solve the problem?
#4885

[MorrisJobke]

Will this solve the problem?
#4885

It should already be in 12.0.1: #5299 and it only affects the caldav and carddav backend and not the group management.

This looks more like stuff in #2997, but needs to be done for the user management as well.

[bpcurse]

As long as this bug persists, is there a workaround to delete an empty user group manually? Can it be done directly in the database without harm?

[bpcurse]

Should anyone else want to get rid of a user group with a "/" in the name you might try this at your own risk: Remove all users from the group and delete it in the database table "xxxxx_groups".
It worked for me and doesn't seem to do any harm to the nextcloud instance.

[skjnldsv]

Our route matcher is decoding our url.
Therefore unless we double-encode them, we can't get rid of this behaviour.
https://github.com/symfony/Routing/blob/master/Matcher/UrlMatcher.php#L87

Nonetheless, we could also do like the users: have id and name as separate data. And then we could add a check to make sure the groupid always contains alphabetical characters :)

@rullzer @MorrisJobke @blizzz? Thoughts?

[blizzz]

@skjnldsv this should not have been allowed since the beginning, but sigh, we need to live with it. and whatever we do, we need to be backwards compatible, as we cannot change existing group ids. The logic for displaynames in groups is already there, it just does not have any use in the UI so far, afaik. I am afraid double-encoding is what we should do know, if that works. Also more of an ugly hack :(

[skjnldsv]

@blizzz yeah it works, but it's a terrible alternative! :/
We could do both? Detect if groupid contains forbidden char (and if so, double urlencode) and forbid creation of new groups with special chars?

[blizzz]

forbid creation of new groups with special chars?

This only makes sense if we decide to change some fundamentals (at least group id renaming) including a way that requires apps to support this mechanism. Otherwise you can never assume groups with "/" in their id are gone. There's no quick way to accomplish this reasonably. Also, I doubt the case is worth the huge effort.

[skjnldsv]

We should then change our api.
Currently we have a PUT on the /groups endpoint and a DELETE on the /groups/groupName endpoint

If we switch to a DELETE to /groups endpoint and pass the groupId as data value (like the PUT creation) it should be ok. But currently, there is no way of deleting a group with a / in it :/

[blizzz]

We cannot just change it, but adding this other route additionally (and phasing the other out over time) would work. Although it would uglify the API. Well, something's got to give. Then i am favouring avoiding escaping multiple times and go with the API modification. Other opinons? @MorrisJobke @rullzer

[skjnldsv]

We can deprecate old route and in a few new versions completely remove it. I already did some similar changes for 14. :)
Nonetheless, whatever we choose will result in something ugly ! :/

[blizzz]

that's what i mean :)

[rullzer]

See #9814 for my current solution ;)