fix permission bits when enforcing rw links

[jnweiger]

Description

The bits READ UPDATE CREATE DELETE are used to differentiate the differnrt types of shares.
passwordMustBeEnforced() maps these bits to the read-only, read-write, read-write-delete, write-only settings.
This code is used for both, files and foldes. with files, the logic was wrong, in such a way, that no setting would enforce passwords on a Download/View/Edit type file share.

Related Issue

• Fixes [QA] Public link on a file with Download/View/Edit ignores password enforcement #40699

Motivation and Context

How Has This Been Tested?

• Tested wit no enforcement: all file and folder link types can be set without a password.
• Tested with exactly one type enforced, (all four after another): the correct folder link type is enforced; the two file link types correspond to the first two enforcement settings.
• Tested with all types enforced: No file or folder link can be created without a password.

• test environment:
  keyboard + hands + eyes

Screenshots (if appropriate):

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:
• Changelog item, see TEMPLATE

[update-docs]

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

[ownclouders]

💥 Acceptance tests pipeline webUISettingsMenu-chrome-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/38131/133

[phil-davis]

I restarted CI. At least one of the pipelines had a problem fetching its docker images. It should run soon, after the various nightly pipelines are finished and there are docker agents available.

[phil-davis]

https://drone.owncloud.com/owncloud/core/38138/9/7

There was 1 failure:

1) Test\Share20\ManagerTest::testPasswordMustBeEnforcedForReadWrite
Failed asserting that false is true.

/drone/src/tests/lib/Share20/ManagerTest.php:667
phpvfscomposer:///drone/src/lib/composer/phpunit/phpunit/phpunit:97

[pako81]

This will probably also need changelog.

[mrow4a]

@jnweiger let me also review this, just to make sure it wont introduce some more regressions that are not covered with enough tests.

[pako81]

@mrow4a CI is now happy. Review it?

[mrow4a]

@jnweiger @pako81 ok, so the logic must match both server side and client side:
https://github.com/owncloud/core/blob/master/core/js/sharedialoglinkshareview.js#L121-L130
https://github.com/owncloud/core/blob/master/lib/private/Share20/Manager.php#L221-L232

Also we need to make sure all cases are covered (both folder and file). I will add commits to this change.

[jnweiger]

@mrow4a you can see in the initial post, 'How Has This Been Tested' what I tested.
Now when looking at sharedialoglinkshareview.js#L121-L130 I am surprised that all this actually worked correctly... hmm.

[mrow4a]

it works, because this checks what should NOT require password. I am also investigation whether something else should be considered.

[mrow4a]

@jnweiger @pako81 so the logic that was there was too simple, also too little tests. It was somehow missed when adding more "public link roles". Now I list all cases possible and do not leave out "default". The same with tests

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

100.0% Coverage
0.0% Duplication

[jnweiger]

@mrow4a Thank you! That is much cleaner now: Better and consistent naming, js and php "obviously" do the same logic now. All cases explicit (that always puzzled me).
From reading code and comments, I think I really understand now, what is going on.
Now if I cannot break it by manual chaos monkey testing, then I am convinced it is correct.

[jnweiger]

Tested all my combinations from the initial comment, and a few more.

We now have:
Password for a file public link with Download View Edit is enforced by the setting [x] Enforce password protection for read + write + delete links

I'd argue that controlling this case with the setting [x] Enforce password protection for read + write links
is more logical: A public file link even when writable cannot do a delete at all. So, when I want to enforce passwords for a writable file link, I would not try anything that has 'delete' in it.

Everything else works as expected.

[mrow4a]

@jnweiger so one detail, for the shares on a file, delete permission is always there. As you can delete a share which is basically unmount. I understand this is confusing, but it is as is.

[jnweiger]

Ah, understand. Deleting the share is a permission. Well, that is not exactly deleting the file. But makes sense from an end user perspective.

[pako81]

@jnweiger @mrow4a can we have a changelog for this? Thx!