Exclude users from sharing if they are a member of any excluded group - util.php

[phil-davis]

Description

Apply changes from PR #31490 to the "copy" of the logic that (unfortunately) is in lib/private/legacy/util.php isSharingDisabledForUser()

Related Issue

https://github.com/owncloud/enterprise/issues/2508

Motivation and Context

Fix all copies of this code.
Discovered while doing the code for issue #31817 PR #31816

How Has This Been Tested?

Unit test change.

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)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• 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.

[PVince81]

👍

[PVince81]

I wonder from where this is called, we should find this for the acceptance tests to trigger it

[PVince81]

Ouch, lots of them.

apps/files_sharing/appinfo/app.php|77| if (\OCP\Util::isSharingDisabledForUser() === false) {
apps/files_sharing/lib/External/Storage.php|297| if (\OCP\Util::isSharingDisabledForUser() || !\OC\Share\Share::isResharingAllowed()) {
apps/files_sharing/lib/SharedStorage.php|169| if (\OCP\Util::isSharingDisabledForUser()) {
apps/files_sharing/lib/SharedStorage.php|200| if (\OCP\Util::isSharingDisabledForUser() || !\OC\Share\Share::isResharingAllowed()) {
lib/public/Util.php|179| public static function isSharingDisabledForUser() {
lib/private/Files/FileInfo.php|210| if (\OCP\Util::isSharingDisabledForUser() || ($this->isShared() && !\OC\Share\Share::isResharingAllowed())) {
lib/private/Files/View.php|1474| $sharingDisabled = Util::isSharingDisabledForUser();
lib/private/Files/View.php|1552| if (Util::isSharingDisabledForUser()) {
lib/private/Share/Share.php|671| if (!\OC\Files\Filesystem::isSharable($path) || \OCP\Util::isSharingDisabledForUser()) {
lib/private/Share/Share.php|1928| if (isset($row['permissions']) && (!self::isResharingAllowed() | \OCP\Util::isSharingDisabledForUser())) {
core/js/config.php|172| 'sharingDisabledForUser' => \OCP\Util::isSharingDisabledForUser(),

As I can see it might be reflected on "oc:permissions" for a received share: the user wouldn't be able to reshare. The share permission would be missing on that entry.

[codecov]

Codecov Report

Merging #31819 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master   #31819      +/-   ##
============================================
- Coverage     63.24%   63.24%   -0.01%     
+ Complexity    18455    18454       -1     
============================================
  Files          1158     1158              
  Lines         69305    69304       -1     
  Branches       1261     1261              
============================================
- Hits          43830    43829       -1     
  Misses        25105    25105              
  Partials        370      370

Flag	Coverage Δ	Complexity Δ
#javascript	52.47% <ø> (ø)	0 <ø> (ø)	⬇️
#phpunit	64.47% <100%> (-0.01%)	18454 <0> (-1)

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Share20/Manager.php	97.21% <ø> (ø)	226 <0> (ø)	⬇️
lib/private/legacy/util.php	70.5% <100%> (-0.05%)	245 <0> (-1)

---

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 a75f9be...0b59275. Read the comment docs.

[PVince81]

Okay, I looked deeper and found out that the code in question is dead: this PR adjusts OC_Util::isSharingDisabledForUser(), the private namespace one.

The public namespace one OCP\Util::isSharingDisabledForUser() has apparently been rewired to use the share manager instead, which contains the correct logic.

I quickly grepped through apps I had checked out locally (ex: gallery) and none seem to be using this.

We could remove it in the next release. To play it safe, let's merge this then but I don't see any way to test it.

[PVince81]

backport conflict... investigating...

[phil-davis]

Backport stable10 #31822

[phil-davis]

The conflict is like always nowadays - many lines of code between master and stable10 are different because of the code-style changes in master. The backport PR has not been merged yet.

[phil-davis]

Yes, I agree - I could not see anything that currently uses it, but was not sure if there might be some other app out there that does use it still. I just "happened" to use it in #31816 and thus noticed the missing logic. I will amend that PR to use a not-deprecated method.

I guess we could tag this old method as deprecated? @PVince81 feel free to push an extra commit here if you want to do that now. (it's merged already)

[PVince81]

@phil-davis 🔥 #31823

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.