Exclude with groups cannot work with groups containing commas

[PVince81]

Steps to reproduce:

1. Create a group called "comma,group"
2. Exclude that group from sharing

Expected result

Group properly excluded

Actual result

(not tested but guessing) group name is split into two groups "comma" and "group", so exclusion cannot work.

Note that other places like the "enable app for groups" use JSON to store the groups array.

The "exclude sharing for groups" feature need to be fixed to also use JSON.

CC @schiesbn

[PVince81]

@MTRichards might not sound that severe but this is blocking #10968 (comment) which fixes three issues that are in the sprint (about loading the group lists with ajax)
So, adding to current sprint.

[PVince81]

This is likely to happen for the "external storage" groups config as well.

[PVince81]

I decided to go with another approach on that PR, so this issue here is no longer blocking.
Removing milestone and tagging as "triage" for a decision.

The TODOs for this ticket is, whenever a group name has a comma in it:

1. Make it work in the "exclude groups from sharing" by switching to JSON
2. Make it work in the external storage app as well, possibly requires moving to JSON as well

The trouble is that we'd need to change the format how groups are internally stored, so we also need compatibility with the old format.

I'm not sure this is adequate for OC 7 and also not sure how likely it is that someone uses a comma in their group names...

[MTRichards]

Ok. On your recommendation (and agreeing), updated the tags.

[PVince81]

So does it mean we still want this for OC 7 ?

[PVince81]

I mean... soon

[MTRichards]

@MTRichards might not sound that severe but this is blocking #10968 (comment) which fixes three issues that are in the sprint (about loading the group lists with ajax)
So, adding to current sprint.

Sounds like you solved the short term problem, and this is a longer term problem.

[PVince81]

Okay. Let's see how the fix looks like then we can decide whether it's risky or not to backport to OC 7 afterwards.

[MTRichards]

I think this has real bad consequences internally - so I would ask @karlitschek for an opinion on the potential impact. Seems to me changing all this is a real potential problem, with room to break things badly. Ideally, not doing this close to launch is a good idea. BUT, this means this bug remains - where commas are creating separate groups.

Okay. Let's see how the fix looks like then we can decide whether it's risky or not to backport to OC 7 afterwards.

Ok, and worste case a release note on group names wihtout commas is ok for the short term. Adding the release note tag in case we get there.

[PVince81]

Makes sense. Thanks.

[karlitschek]

@PVince81 So do you think it is possible to fix this without this big code change?

[PVince81]

Switching to JSON is a big change with a bigger impact.
The alternative is to switch to yet another format using pipes | but note that LDAP group names can probably also contain there.
And if we change formats, regardless which one we use, we need to make the code backwards compatible.

I didn't have time to look into how many places in the code needed changing.

[DeepDiver1975]

@PVince81 OC8.1?

[PVince81]

Yes, it's not a simple change

[carlaschroder]

added to 8.0 release note

[PVince81]

Now thinking of it, we might not need to keep the code backward compatible because we can auto-fix the configuration in a repair step to make sure it's using the new format.