share manager should emit share.link auth events to catch all accesses

[karakayasemi]

Description

Currently, WebDAV access for the password-protected public links is not emitting any link share auth events. These events are only emitted when ShareController used.

This PR moves link share auth events emitting logic to share manager to cover all access cases.

Related Issue

• Discovered in here: Public share protect (4) brute_force_protection#123 (comment)
• Partly related https://github.com/owncloud/admin_audit/issues/229

How Has This Been Tested?

• The problem described in here fixed with this pr: Public share protect (4) brute_force_protection#123 (comment)

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
• Changelog item

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

[phil-davis]

No codecov result had come. The branch was off a quite-old version of master.
I rebased and force-pushed just now. The codecov reporting problem was fixed recently in master, so this should get a codecov result from CI this time.

[jvillafanez]

Question: how about triggering a new event? I'm concerned this PR might break things because the events are triggered in a different place, so it could happen that the event was triggered before an error happened, but this might not happen any longer.
In addition, having an error code of 403, probably referencing a http error code, doesn't make sense in the share manager. I know it's to keep compatibility, but this one problem of moving events after they're been established.

[codecov]

Codecov Report

Merging #37430 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             master   #37430   +/-   ##
=========================================
  Coverage     64.68%   64.68%           
  Complexity    19331    19331           
=========================================
  Files          1277     1277           
  Lines         75506    75522   +16     
  Branches       1331     1331           
=========================================
+ Hits          48838    48854   +16     
  Misses        26276    26276           
  Partials        392      392           

Flag	Coverage Δ	Complexity Δ
#javascript	54.14% <ø> (ø)	0.00 <ø> (ø)
#phpunit	65.85% <100.00%> (+<0.01%)	19331.00 <0.00> (ø)

Impacted Files	Coverage Δ	Complexity Δ
.../files_sharing/lib/Controllers/ShareController.php	53.52% <ø> (-1.07%)	54.00 <0.00> (ø)
lib/private/Share20/Manager.php	96.26% <100.00%> (+0.10%)	273.00 <0.00> (ø)

---

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 1d6094c...59dfc8e. Read the comment docs.

[karakayasemi]

Question: how about triggering a new event? I'm concerned this PR might break things because the events are triggered in a different place, so it could happen that the event was triggered before an error happened, but this might not happen any longer.

@jvillafanez before and after share link auth events was already newly added with this pr #34158, for the purpose of using it in brute force protection app. So, by assuming there is no other usage of it in the organization-wide (not even brute force protection app yet), I can say there is no need to afraid to move them to a different class.

In addition, having an error code of 403, probably referencing a http error code, doesn't make sense in the share manager. I know it's to keep compatibility, but this one problem of moving events after they're been established.

I assume this event is intended to emit on every access attempt of a share. However, currently, WebDAV accesses miss these events. I identify this situation as a bug. I agree with you on about the error code issue but could not find a better solution. I kept the event format the same for compatibility reasons as you said.

Since ShareController also using the Share Manager, the previously emitted events will still be emitted as identical with before in this PR's solution, so, I expect It should not break any current usage. In addition, it will fix the bug by emitting event signals in DAV accesses also.

Another solution would be adding the same events to dav app, but I think, unifying emitting logic in the share manager is a better solution.

[jvillafanez]

I still think it's a better idea to deprecate the old events (leaving them for a while) and add new events to fix this, then the app can switch to the new events once they're in place. This way we can adjust the event name to be more intuitive (using this the "checkPassword" function doesn't mean that a link is being accessed, but a link, in this particular case, has its password checked), and adjust what the event is sending to be more meaningful.

[karakayasemi]

I will open a new pr without touching existing events. Closing this for now.