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.

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

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

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.

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

This will probably also need changelog.

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

@mrow4a CI is now happy. Review it?

@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.

@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.

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

@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

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

100.0% Coverage
0.0% Duplication

@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.

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.

@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.

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

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