fix(files_sharing): skip expiration notify for invalid share record

[luka-nextcloud]

• Resolves: Skip expiration notify for invalid share record #50541

Summary

When the share is expired and the file is no longer present, the expiration notification should be skipped.

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• Tests (unit, integration, api and/or acceptance) are included
• Screenshots before/after for front-end changes
• Documentation (manuals or wiki) has been updated or is not required
• Backports requested where applicable (ex: critical bugfixes)

[juliusknorr]

This only seems to happen if you manually call this command. I think this only addresses one part of the original issue.

There seems to be also an activity message at the actual expiration that may then be sent by mail: https://github.com/nextcloud/activity/blob/b2d16c9b996b5c70488c932b25ddd7d83246ec6f/lib/FilesHooks.php#L859

However I'm unsure which notification the original report is referring to

[juliusknorr]

Change makes sense nevertheless 👍

[luka-nextcloud]

This only seems to happen if you manually call this command. I think this only addresses one part of the original issue.

There seems to be also an activity message at the actual expiration that may then be sent by mail: https://github.com/nextcloud/activity/blob/b2d16c9b996b5c70488c932b25ddd7d83246ec6f/lib/FilesHooks.php#L859

However I'm unsure which notification the original report is referring to

@juliusknorr @nickvergessen After checking the flow, I would suggest to update function Manager::deleteShare() to delete invalid shares silently (https://github.com/nextcloud/server/blob/master/lib/private/Share20/Manager.php#L1125).
This would be a better approach since we might have some more apps listening to event BeforeShareDeletedEvent. What do you think?

[juliusknorr]

As long as we not generally skip emitting the event. It might still be useful even for invalid shares to be emitted on removal (e.g. for the audit log). Not entirely sure what is best there. Any further insight @nickvergessen or @artonge for sharing in general?

[nickvergessen]

At least completely skipping the hooks is not a good/valid idea.
audit.log also listens to it and should log that a share was removed, etc.

I had raised the question what the actual problem we are trying to fix is, but we don't have a screenshot/video so it's a bit of blind guessing.
I assume, it's an activity entry:

server/apps/files_sharing/lib/Activity/Providers/PublicLinks.php

Lines 29 to 38 in 1e04619

	} elseif ($event->getSubject() === self::SUBJECT_UNSHARED_LINK_SELF) {
	$subject = $this->l->t('Removed public link');
	} elseif ($event->getSubject() === self::SUBJECT_LINK_EXPIRED) {
	$subject = $this->l->t('Public link expired');
	} elseif ($event->getSubject() === self::SUBJECT_RESHARED_LINK_BY) {
	$subject = $this->l->t('{actor} shared as public link');
	} elseif ($event->getSubject() === self::SUBJECT_UNSHARED_LINK_BY) {
	$subject = $this->l->t('{actor} removed public link');
	} elseif ($event->getSubject() === self::SUBJECT_LINK_BY_EXPIRED) {
	$subject = $this->l->t('Public link of {actor} expired');

which is expected.

It could be adjusted in https://github.com/nextcloud/activity/blob/af19269515991ea4e9863606b2b9211ccda9176d/lib/FilesHooks.php#L967-L981
But I think it can really make sense to receive a notification that your share expired. So I'd assume the request is just from a different usecase that does not match to our overall usecase.

When discussing this a bit, @luka-nextcloud said

I think it only makes sense if the file/folder still exists

The problem is, the file might still exist, just not for you. And maybe it was not intentional, so it would be good to inform you that something you shared is no longer working :/

[luka-nextcloud]

@nickvergessen @juliusknorr I just confirmed with the customer that the notification email originates from the Activity app.
Given this, I believe it would make sense to adjust the Activity app's behavior. Specifically, we could modify it to skip sending expiration notifications if the shared node has been trashed or deleted.

[juliusknorr]

Sounds good, lets do it like this then.

[luka-nextcloud]

PR on activity app: nextcloud/activity#1915

[luka-nextcloud]

/backport to stable29

[luka-nextcloud]

/backport to stable30

[luka-nextcloud]

/backport to stable31