fix(ShareAPI): Send mails for mail shares by default #48381

Fenn-CS · 2024-09-26T11:08:04Z

Summary

It looks like, the frontend it needs to provide the sendMail param for the backend to decide whether mails would be sent.

Our UI does not have that at the moment so it should default to sending emails always for mail shares.

Not exactly sure how this was handled earlier but this is a good starting point.

Resolves : #48012

Checklist

Backports requested where applicable (ex: critical bugfixes)

apps/files_sharing/lib/Controller/ShareAPIController.php

provokateurin

Fix makes sense, curious how it broke though.

apps/files_sharing/lib/Controller/ShareAPIController.php

Fenn-CS · 2024-09-26T13:08:21Z

/backport to stable30

provokateurin · 2024-09-30T11:13:43Z

Why rebase it so often?

Fenn-CS · 2024-09-30T12:35:43Z

Why rebase it so often?

Some tests that seem unrelated (cypress especially are failing consistently)...

Re-basing triggers a rerun, but also adds the latest updates on master in case something that broke the tests has been resolved. (Because a bunch of other PRs are failing tests too)...

Now taking a closer to see if something is indeed broken.

Fenn-CS

This breaks "File Requests"

server/apps/files_sharing/lib/Controller/ShareAPIController.php

Lines 721 to 727 in 363ba6b

    
           if (is_string($shareWith) && $shareType === IShare::TYPE_EMAIL) { 
        
           	// If sending a mail have been requested, validate the mail address 
        
           	if ($share->getMailSend() && !$this->mailer->validateMailAddress($shareWith)) { 
        
           		throw new OCSNotFoundException($this->l->t('Please specify a valid email address')); 
        
           	} 
        
           	$share->setSharedWith($shareWith); 
        
           }

Why?

File requests are sent with an empty string for shareWith which the backend uses to check if a shareWith value is available (poorly so)
The file requests shareType is 4, which is mail share and I am not sure why that is.

apps/files_sharing/lib/Controller/ShareAPIController.php

provokateurin

Oops

provokateurin · 2024-10-07T08:15:42Z

apps/files_sharing/src/components/NewFileRequestDialog.vue

-					shareType: sharingConfig.isMailShareAllowed ? ShareType.Email : ShareType.Link,
+					shareType: ShareType.Link,


Not sure about this, @skjnldsv what was the idea behind this?

Yes, I checked on talk and seems he is on holiday... However as far as I know "file drop" which is what was converted to "File request" was only possible on public link shares.

Leaving all this context as I won't be around as from tomorrow, however, this is an important fix. Mails not being sent out for mails shares is quite critical.

@susnux May also be able to help in the absence of Bath.

I only take showers 🚿

Fenn-CS · 2024-10-07T08:56:40Z

Add some more context on why the tests are failing and some related stuff with a plausible fix:

Share type setting can be changed (current solution), this seems to prevent the sendMail function for file requests as at now but since the links can be copied and shared, it's way better than breaking email shares on prod.

Fenn-CS · 2024-10-07T08:13:09Z

apps/files_sharing/src/components/NewFileRequestDialog.vue

@@ -284,7 +284,7 @@ export default defineComponent({
 				const request = await axios.post<OCSResponse>(shareUrl, {
 					// Always create a file request, but without mail share
 					// permissions, only a share link will be created.
-					shareType: sharingConfig.isMailShareAllowed ? ShareType.Email : ShareType.Link,
+					shareType: ShareType.Link,


File requests, should always be link share? The concept of mail shares is different.

No, a mail share is a link share.
They both behave the same way. A link share does NOT send emails, if we start doing this, then we would break the default sharing rule that admin allows to send emails or not.

If admin allowed email shares, then a file request allow users to add emails and send the notifications
If admin does NOT, then users can only create a share link file request, and have to manually give the link to the recipients :)

I don't think this is the right place to check for that.

A file request is (correct me if I am wrong) : Simply a link that you can send to someone to "requests files"...

The means of sending the link can either be:

Manually copying the link and sending

Automatic via mail (after you must have already created the link share)

Why should the means for sending the "File request" (link) modify/change what a file request is? Does the mail not serve the purpose of simply propagating the link ?

My thoughts are, if the admin setting must be enforced, it must be enforced at the step where the user is asked to add emails in file request.

The thing already breaks with errors concerning email without even knowing whether the user would end up wanting to send the file request link via email or not. In fact creating a file request that's failing with invalid email errors when the user has not even reached the email step in the UI, definitely not okay.

If admin allowed email shares, then a file request allow users to add emails and send the notifications
If admin does NOT, then users can only create a share link file request, and have to manually give the link to the recipients :)

Also this seems pretty mixed up, why should the setting for "allow email shares" affect whether a file request link is sent to provided email addresses or not?

It is a convenience thing and even if it was not, "File drop" (now "File requests") really had nothing to do with email shares? and by convenience I mean literally helping the user send the file request link directly without extra steps (copy and share) necessarily.

In other words, if we have something that is actually called "e/mail shares" the admin setting that controls or disables that should not control two things (e/mail shares and whether file requests links can be sent via mail).

In my opinion a "File request" is not a mail share and cannot be. (Because the fact that all mail shares are links shares sent via mail does not imply link shares are equally mail shares even if the link is sent via mail)

Fenn-CS · 2024-10-07T09:02:48Z

apps/files_sharing/src/components/NewFileRequestDialog.vue

-					shareType: sharingConfig.isMailShareAllowed ? ShareType.Email : ShareType.Link,
+					shareType: ShareType.Link,


Yes, I checked on talk and seems he is on holiday... However as far as I know "file drop" which is what was converted to "File request" was only possible on public link shares.

Leaving all this context as I won't be around as from tomorrow, however, this is an important fix. Mails not being sent out for mails shares is quite critical.

skjnldsv

Blocking, will discuss this in chat for easier comprehension

skjnldsv · 2024-10-08T08:56:17Z

This breaks "File Requests"

File requests are sent with an empty string for shareWith which the backend uses to check if a shareWith value is available (poorly so)

The file requests shareType is 4, which is mail share and I am not sure why that is.

File requests allow to add emails in attributes instead of shareWith. ShareWith does NOT support multiple recipients, and that was the only way to /cleanly/ do it without rewriting the API.
That being said, we allow shareWith to be empty (as for link shares), only if you specify some mails as attributes. This also allow to create a share wthout sending the mails straight away and specify the recipients as a later step.
It's a cleaner UX as you can enter emails OR manually copy the link with token, which we would not have without creating the share.

TLDR:
create share > get token > copy link or edit share to set recipients

skjnldsv · 2024-10-08T08:58:00Z

My fix would be to set the sendMail param to true by default, as file requests set it to false anyway 🤔

server/apps/files_sharing/lib/Controller/ShareAPIController.php

Line 568 in e089e40

?string $sendMail = null,

It looks like, the frontend it needs to provide the `sendMail` param for the backend to decide wether mails would be sent. Our UI does not have that at the moment so it should default to sending emails always for mail shares. Not exactly sure how this was handled earlier but this is a good starting point. Resolves : #48012 Signed-off-by: fenn-cs <[email protected]>

"File request" is the new "File drop"... File drop was only possible on link shares. The mails sent at the end of creating a file request is "sending the link to various emails" it does should not turn the share type to email. Signed-off-by: fenn-cs <[email protected]>

Fenn-CS · 2024-10-23T21:05:46Z

My fix would be to set the sendMail param to true by default, as file requests set it to false anyway 🤔

and what about other things that use the API without specifying what goes to sendMail???

Fenn-CS · 2024-10-23T21:07:33Z

/compile

Signed-off-by: nextcloud-command <[email protected]>

susnux · 2024-10-24T09:58:12Z

and what about other things that use the API without specifying what goes to sendMail???

Agree, we can not change default behavior as this would be a breaking change.

susnux · 2024-10-24T09:59:09Z

apps/files_sharing/lib/Controller/ShareAPIController.php

@@ -719,7 +728,7 @@ public function createShare(
 			}

 			// Only share by mail have a recipient
-			if (is_string($shareWith) && $shareType === IShare::TYPE_EMAIL) {
+			if (!empty($shareWith) && $shareType === IShare::TYPE_EMAIL) {


hm I thought we do not use empty for strings?

It can be used with its own dangers for example empty would return true for "0" which okay in this case but we can also use strlen($shareWith) !== 0 here for developer happiness and perhaps more robustness :)

Fenn-CS added 3. to review Waiting for reviews feature: sharing backport-request 30-feedback labels Sep 26, 2024

Fenn-CS requested review from artonge, skjnldsv, sorbaugh and come-nc September 26, 2024 11:08

Fenn-CS force-pushed the fix/48012/fix-share-email-send-mail-share branch from 33e46ad to 6553268 Compare September 26, 2024 11:10

github-advanced-security bot found potential problems Sep 26, 2024

View reviewed changes

apps/files_sharing/lib/Controller/ShareAPIController.php Fixed Show fixed Hide fixed

provokateurin approved these changes Sep 26, 2024

View reviewed changes

apps/files_sharing/lib/Controller/ShareAPIController.php Outdated Show resolved Hide resolved

Fenn-CS force-pushed the fix/48012/fix-share-email-send-mail-share branch from 6553268 to 8103393 Compare September 26, 2024 12:31

Fenn-CS force-pushed the fix/48012/fix-share-email-send-mail-share branch from 8103393 to 410854f Compare September 26, 2024 13:08

Fenn-CS enabled auto-merge September 26, 2024 13:08

come-nc approved these changes Sep 26, 2024

View reviewed changes

Fenn-CS force-pushed the fix/48012/fix-share-email-send-mail-share branch from 410854f to 3f7528b Compare September 26, 2024 13:52

Fenn-CS mentioned this pull request Sep 26, 2024

Add "Send mail" toggle in sharing details ui for new shares #48397

Open

Fenn-CS force-pushed the fix/48012/fix-share-email-send-mail-share branch 3 times, most recently from e2fa7e1 to 9f0eda3 Compare September 30, 2024 11:01

Fenn-CS force-pushed the fix/48012/fix-share-email-send-mail-share branch from 9f0eda3 to aab671f Compare October 7, 2024 06:57

Fenn-CS commented Oct 7, 2024

View reviewed changes

Fenn-CS force-pushed the fix/48012/fix-share-email-send-mail-share branch from aab671f to 91a3ce1 Compare October 7, 2024 07:57

provokateurin approved these changes Oct 7, 2024

View reviewed changes

apps/files_sharing/lib/Controller/ShareAPIController.php Outdated Show resolved Hide resolved

provokateurin requested changes Oct 7, 2024

View reviewed changes

Fenn-CS force-pushed the fix/48012/fix-share-email-send-mail-share branch from 91a3ce1 to e089e40 Compare October 7, 2024 08:09

Fenn-CS disabled auto-merge October 7, 2024 08:11

provokateurin reviewed Oct 7, 2024

View reviewed changes

Fenn-CS commented Oct 7, 2024

View reviewed changes

thbemme mentioned this pull request Oct 7, 2024

[Bug]: 30.0.0 share by email - no email is sent #48012

Open

8 tasks

skjnldsv requested changes Oct 8, 2024

View reviewed changes

Fenn-CS added 2 commits October 23, 2024 21:21

Fenn-CS force-pushed the fix/48012/fix-share-email-send-mail-share branch from e089e40 to a154393 Compare October 23, 2024 21:07

chore(assets): Recompile assets

3dd0f4b

Signed-off-by: nextcloud-command <[email protected]>

susnux reviewed Oct 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ShareAPI): Send mails for mail shares by default #48381

fix(ShareAPI): Send mails for mail shares by default #48381

Fenn-CS commented Sep 26, 2024

provokateurin left a comment

Fenn-CS commented Sep 26, 2024

provokateurin commented Sep 30, 2024

Fenn-CS commented Sep 30, 2024 •

edited

Loading

Fenn-CS left a comment

provokateurin left a comment

provokateurin Oct 7, 2024

Fenn-CS Oct 7, 2024

Fenn-CS Oct 7, 2024

skjnldsv Oct 8, 2024

Fenn-CS commented Oct 7, 2024

Fenn-CS Oct 7, 2024

skjnldsv Oct 8, 2024

Fenn-CS Oct 23, 2024 •

edited

Loading

Fenn-CS Oct 23, 2024 •

edited

Loading

Fenn-CS Oct 7, 2024

skjnldsv left a comment

skjnldsv commented Oct 8, 2024

skjnldsv commented Oct 8, 2024

Fenn-CS commented Oct 23, 2024

Fenn-CS commented Oct 23, 2024

susnux commented Oct 24, 2024

susnux Oct 24, 2024

Fenn-CS Oct 24, 2024

	if (is_string($shareWith) && $shareType === IShare::TYPE_EMAIL) {
	// If sending a mail have been requested, validate the mail address
	if ($share->getMailSend() && !$this->mailer->validateMailAddress($shareWith)) {
	throw new OCSNotFoundException($this->l->t('Please specify a valid email address'));
	}
	$share->setSharedWith($shareWith);
	}

		shareType: sharingConfig.isMailShareAllowed ? ShareType.Email : ShareType.Link,
		shareType: ShareType.Link,

fix(ShareAPI): Send mails for mail shares by default #48381

Are you sure you want to change the base?

fix(ShareAPI): Send mails for mail shares by default #48381

Conversation

Fenn-CS commented Sep 26, 2024

Summary

Checklist

provokateurin left a comment

Choose a reason for hiding this comment

Fenn-CS commented Sep 26, 2024

provokateurin commented Sep 30, 2024

Fenn-CS commented Sep 30, 2024 • edited Loading

Fenn-CS left a comment

Choose a reason for hiding this comment

provokateurin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fenn-CS commented Oct 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fenn-CS Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Fenn-CS Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skjnldsv left a comment

Choose a reason for hiding this comment

skjnldsv commented Oct 8, 2024

skjnldsv commented Oct 8, 2024

Fenn-CS commented Oct 23, 2024

Fenn-CS commented Oct 23, 2024

susnux commented Oct 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fenn-CS commented Sep 30, 2024 •

edited

Loading

Fenn-CS Oct 23, 2024 •

edited

Loading

Fenn-CS Oct 23, 2024 •

edited

Loading