occ files:scan fix groups with comma, plus tests

[phil-davis]

Description

1. Test that groups containing a comma work OK in the Provisioning API.
2. Test that groups containing a comma work OK in the "excluded from sharing" group list.
3. Adjust the files_scan command so that the groups to scan are not put in a comma-separated list. Instead, list the groups to be scanned like --group=foo --group=bar - this allows specifying group names that contain commas.

Related Issue

#31578
#31835

Motivation and Context

@tomneedham asked if comma is allowed in group names - this demonstrates that it works in the Provisioning API.

How Has This Been Tested?

Local test runs

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Acceptance test enhancement

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation. https://github.com/owncloud/documentation/issues/4215
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.

[codecov]

Codecov Report

Merging #31719 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master   #31719      +/-   ##
============================================
+ Coverage     63.92%   63.93%   +<.01%     
  Complexity    18542    18542              
============================================
  Files          1169     1169              
  Lines         69831    69833       +2     
  Branches       1267     1267              
============================================
+ Hits          44641    44649       +8     
+ Misses        24821    24815       -6     
  Partials        369      369

Flag	Coverage Δ	Complexity Δ
#javascript	52.81% <ø> (ø)	0 <ø> (ø)	⬇️
#phpunit	65.2% <100%> (+0.01%)	18542 <0> (ø)	⬇️

Impacted Files	Coverage Δ	Complexity Δ
apps/files/lib/Command/Scan.php	71.47% <100%> (ø)	73 <0> (-1)	⬇️
lib/private/legacy/app.php	64.82% <0%> (+0.06%)	179% <0%> (-1%)	⬇️
lib/private/App/AppManager.php	86.3% <0%> (+0.25%)	86% <0%> (+2%)	⬆️
lib/private/Installer.php	49.12% <0%> (+2.19%)	87% <0%> (ø)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a369c7...b746e6a. Read the comment docs.

[individual-it]

if this behaviour is expected 👍

[phil-davis]

@tomneedham @patrickjahns @DeepDiver1975 @PVince81 @ anyone else - Tom had a question about if group ids (names) can contain commas - #31578 (comment)

Answer: yes they can, and they at least work in the Provisioning API

Next question: Is it a requirement that group ids (names) can contain a comma?

If yes, then merge this. (It at least demonstrates working behavior in 1 area)

If no, then there would be a whole path to go down to "ban the comma".

[tomneedham]

My concern is that comma's are used as deliminators for groups in some places:

This is what should be fixed probably.

[phil-davis]

lib/private/legacy/util.php - that is unused and code is being deleted #31823

lib/private/Share20/Manager.php - the excluded groups list "should" be json_encoded but for some reason the code here handles the case where it is not, and falls back to making the excluded groups array by splitting on comma. I guess that some time in the past the excluded groups list was stored with comma-separator. The code then puts it back as json_encode - so it is an internal "on the fly migration" of the saved format. That should be fine. Unit tests added for that.

apps/files/lib/Command/Scan.php - fixup the command interface so you say --group=foo --group=bar --group=a,b to scan the groups foo bar and a,b
or the abbreviated form -g=foo -g=bar -g=a,b

and adjust ScanTest.php

[phil-davis]

This gets rid of all those places that try to explode a comma-separated list of group names.
And CI passes.
@tomneedham @PVince81 @DeepDiver1975 please review.

[phil-davis]

@tomneedham @PVince81 @DeepDiver1975 please review.
IMO it would be nice to get this tidied-up for the next stable10 release.

[phil-davis]

@DeepDiver1975 @PVince81 @tomneedham
I rebased this just now, to make sure it still passes CI.
The only question I have is, I have changed the files:scan --groups option to --group
As well as making the caller specify group names 1-by-1

occ files:scan --groups foo,bar

becomes:

occ files:scan --group foo --group bar

The user:add command currently does:

occ user:add user1 --group foo --group bar

This makes things consistent. Whatever I do, there is a backward-compatibility break, because --groups foo,bar was always ambiguous.

Please review.

[mmattel]

@phil-davis I allowed to change the headline to a better text.
The first one looked like to focus on tests and did not tell for which command it was made

[mmattel]

Document relevant, please open a doc Issue/PR

[phil-davis]

Doc PR https://github.com/owncloud/documentation/issues/4215 already exists

[mmattel]

@phil-davis
Idea, in repro password_policy PR 77, the options for using group are the same as here. What is new there, you can use -u for users but in the same way as for groups (elimiating btw commas in user names too). Why not harmonizing it and implement a -u option?
Maybe a separate PR, but just as an idea. You can ping me on talk to discuss if you want 😄

[DeepDiver1975]

👎 on backporting - this will break API. API breakage only on major updates - this goes into oc11/master only

for the rest: 👍

[phil-davis]

Backport in PR #32242 for commits that are adding Provisioning API and Unit Test cases that already pass.

The last 2 commits that change the files:scan command are NOT backported, due to backward-compatibility break.

[phil-davis]

Backporting work is done here, so removing backport-request label.

[mmattel]

@phil-davis will there be a PR that goes into stable10 even on hold for oc11, else this one gets lost completely in any sense and devides master and stable10.

[phil-davis]

@mmattel oc11 will be a branch called stable11 that gets created off master at some time near when oc11 is "about to happen". The backward-compatibility breaks will not end up in oc10, because they break backward-compatibilty. That is the semver definition for versioning.

[phil-davis]

I added the v11 label. That is the current convention to label stuff that can't be released in the current 10.* series, but needs to be easily found for when a "post-10" release is being sorted out.

[phil-davis]

The last 2 commits have now been backported to stable10 and will be in the next release (probably called 10.2)
Removing the v11 label.