Refactor share permissions handling

[karakayasemi]

Description

This PR moves some of the validation on Share20Controlller to the share manager instead and changes permission calculation on re-shares.

Currently, permission handling different for update and create share operations, also two different classes(Share20OcsController.php and Manager) are responsible from this permission checks. This situation causes issues such as a scenario that works well in one operation does not work in another. (https://github.com/owncloud/enterprise/issues/3299 only affecting update share and the another is only affecting create share).

In a clean approach, Share manager should take care of all sorts of validations and logics instead of controller. I unified these logics under Share Manager and applied this unified checks for both update and create operations.

Also, with this PR re-share permission will be calculated based on share permission of re-share mount point node's.

IMHO, most of the permission exceptions should return 403. However, to keep acceptance test and behavior changes minimal, I did not touch return codes and I kept them as it is.

I removed many unit tests. Some of them were duplicate and redundant. I will add more unit tests after reviews.

Share API, no longer set read permission as default. Clients should include this permission to their requests. We need to test this behavior on clients to ensure there is no regression.

Related Issue

fixes https://github.com/owncloud/enterprise/issues/3299
fixes https://github.com/owncloud/enterprise/issues/3404
fixes #35922

Motivation and Context

To get rid of from bugs. To have a cleaner code. Next time we will know where we should look at on a share permission issue.

How Has This Been Tested?

Acceptance tests, Also it resolves described scenarios in these tickets:

• https://github.com/owncloud/enterprise/issues/3299
• https://github.com/owncloud/enterprise/issues/3404
• Sharing a file with just create and/or delete permission should fail #35922

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Database schema changes (next release will require increase of minor version instead of patch)
• Breaking change (fix or feature that would cause existing functionality to change)
• Technical debt
• Tests only (no source changes)

Checklist:

• Code changes
• Unit tests added
• Acceptance tests added
• Documentation ticket raised:

Open tasks:

• Backport (if applicable set "backport-request" label and remove when the backport was done)

[codecov]

Codecov Report

Merging #35884 into master will decrease coverage by 16.76%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##             master   #35884       +/-   ##
=============================================
- Coverage      65.8%   49.03%   -16.77%     
=============================================
  Files          1229      109     -1120     
  Lines         70856    10535    -60321     
  Branches       1289     1289               
=============================================
- Hits          46626     5166    -41460     
+ Misses        23852     4991    -18861     
  Partials        378      378

Flag	Coverage Δ	Complexity Δ
#javascript	53.7% <ø> (ø)	0 <ø> (ø)	⬇️
#phpunit	38.59% <ø> (-28.6%)	0 <ø> (-18760)

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Files/Storage/DAV.php	58.71% <0%> (-21.37%)	0% <0%> (ø)
apps/updatenotification/templates/admin.php
lib/private/Encryption/Keys/Storage.php
lib/private/App/CodeChecker/NodeVisitor.php
lib/private/RedisFactory.php
apps/dav/lib/Avatars/AvatarNode.php
...s/dav/appinfo/Migrations/Version20170202213905.php
apps/dav/lib/Upload/ChunkLocationProvider.php
apps/files/lib/AppInfo/Application.php
apps/systemtags/list.php
... and 1110 more

---

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 8c402b5...ab8b0af. Read the comment docs.

[codecov]

Codecov Report

Merging #35884 into master will decrease coverage by 0.98%.
The diff coverage is 89.58%.

@@             Coverage Diff              @@
##             master   #35884      +/-   ##
============================================
- Coverage     66.43%   65.44%   -0.99%     
+ Complexity    20183    20172      -11     
============================================
  Files          1233     1296      +63     
  Lines         68965    76359    +7394     
  Branches          0     1301    +1301     
============================================
+ Hits          45814    49972    +4158     
- Misses        23151    26002    +2851     
- Partials          0      385     +385

Flag	Coverage Δ	Complexity Δ
#javascript	53.85% <ø> (?)	0 <ø> (?)
#phpunit	66.68% <89.58%> (+0.25%)	20172 <33> (-11)	⬇️

Impacted Files	Coverage Δ	Complexity Δ
...es_sharing/lib/Controller/Share20OcsController.php	86.75% <100%> (-0.84%)	193 <0> (-20)
lib/private/Share20/Manager.php	97.12% <88.09%> (-0.36%)	244 <33> (+9)
lib/private/Files/Cache/HomePropagator.php	88.88% <0%> (-11.12%)	3% <0%> (ø)
apps/files/lib/Command/Scan.php	71.91% <0%> (-9.17%)	74% <0%> (ø)
core/js/oc-dialogs.js	2.66% <0%> (ø)	0% <0%> (?)
core/js/oc-backbone.js	50% <0%> (ø)	0% <0%> (?)
settings/js/admin-apps.js	6.46% <0%> (ø)	0% <0%> (?)
core/js/config.js	3.33% <0%> (ø)	0% <0%> (?)
apps/files_sharing/js/external.js	77.96% <0%> (ø)	0% <0%> (?)
apps/comments/js/commentmodel.js	100% <0%> (ø)	0% <0%> (?)
... and 73 more

---

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 851bd2e...c89d230. Read the comment docs.

[jvillafanez]

I haven't seen the whole code in depth, but on a general perspective I think it's better to move the permission calculation to a different class, so the share manager can rely on it. This way, we can unittest this new class completely, increase the code coverage, and also simplify the code in the share manager.

Side note, I don't like protected methods unless there is a reason for it.

[karakayasemi]

When I looking to failing unit tests, I discovered that, when a user shares a file with create permission, Share20Controller modifies the share and removes this create permission. If the share had only create permission, this share is created with 0 permission and this should not happen.

In this PR's implementation, this bug is also seems resolved. One of the failing test scenarios was using this wrong behavior. I believe, I corrected the test scenario with this commit: c4f5dc5. @phil-davis , @individual-it please correct me, if I am wrong about the intended behaviour on this acceptance test.

[phil-davis]

Correct, that scenario tried to create a share of a file and give create permission. A file share cannot have create permission, because a single file is already, by definition "created". The back-end subtracts the create permission from the requested permissions when there is a share of a file.

So that scenario should never have worked, because the Given step should not have created the share.

I will make some tests, in another PR, that test attempting to create a share of a file like that. That will confirm the current (probably wrong) behaviour. Then the work in this PR can correct whatever is wrong.

[karakayasemi]

@jvillafanez I used protected-private methods to give descriptive names for chunks of code and shorten method lines. The class has also similar protected validate methods. If we move permission calculations to another class, we should move the rest of the validation methods too. However, I'm not sure this effort worths.

Recently, we faced lots of issues about share permission calculations. Some of them are still waiting to be resolved. As far as I see, some of the features about permissions are working just in luck and they may have undiscovered bugs. The main focus of this PR is unifying share permission checks and fixing these bugs as I described above.

We covered lots of the issues with acceptance tests and the CI is now green. Please, help by reviewing in-depth to do not miss anything.

[phil-davis]

PR #35921 has been merged. That adds more share permissions scenarios.
@karakayasemi you can rebase to latest master, then adjust the scenarios added in that PR so that they demonstrate the expected correct behaviour and then make sure they pass.

[karakayasemi]

I rebased the PR to test against newly added acceptance tests. Looks like some of them failed. I will have a look what's wrong there.

[karakayasemi]

Finally, the CI is green again. @phil-davis your review, especially about acceptance tests, would help us to not miss anything in here. There are some changes related to removing default read permission.

[phil-davis]

Note: the reason for changing this is that the old scenario created a share of a file with only create permission.
That is not a valid thing to do anyway, and so the When step was failing anyway, because the share may not have existed at all.
So using a folder in the scenario is better/valid.

[phil-davis]

Also in apps/files_sharing/tests/ShareTest.php testFileSharePermissions() the comment says:

shared files should never have delete permissions

but now all the dataProviderTestFileSharePermissions() cases are true.

I think that is because $this->share() now does not throw exceptions like it did before. Maybe the requested delete permission gets masked out and the share is created OK, just without delete permission?

And there could be test cases for PERMISSION_CREATE also, because that is a special permission that does not apply to sharing a single file.

[phil-davis]

Congratulations on adjusting all those scenarios, refactoring lots of stuff and also getting green CI! A good example of TDD.

[karakayasemi]

Also in apps/files_sharing/tests/ShareTest.php testFileSharePermissions() the comment says:

shared files should never have delete permissions

but now all the dataProviderTestFileSharePermissions() cases are true.

I think that is because $this->share() now does not throw exceptions like it did before. Maybe the requested delete permission gets masked out and the share is created OK, just without delete permission?

@phil-davis You are right about your prediction. In this test, shares are created correctly with the current desired behavior. Even so, I changed testFileSharePermissions implementation to ensure file shares never have create and delete permissions.

[phil-davis]

No codecov came. I rebased to get a fresh run of CI, and maybe get codecov numbers?
IMO, it would be nice to know if the code+test changes have the same or better coverage.

[karakayasemi]

@phil-davis I guess, we need #35973 for fully green CI. I will rebase this when it merged.

[karakayasemi]

Tried two times Codecov is not willing to come. We saw fully green CI before, I guess we can merge this with the power of one of the repo god.