-
-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(ShareAPI): Send mails for mail shares by default #48381
base: master
Are you sure you want to change the base?
Conversation
33e46ad
to
6553268
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix makes sense, curious how it broke though.
6553268
to
8103393
Compare
/backport to stable30 |
8103393
to
410854f
Compare
410854f
to
3f7528b
Compare
e2fa7e1
to
9f0eda3
Compare
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. |
9f0eda3
to
aab671f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
aab671f
to
91a3ce1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops
91a3ce1
to
e089e40
Compare
shareType: sharingConfig.isMailShareAllowed ? ShareType.Email : ShareType.Link, | ||
shareType: ShareType.Link, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about this, @skjnldsv what was the idea behind this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@susnux May also be able to help in the absence of Bath.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only take showers 🚿
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. |
@@ -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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File requests, should always be link share? The concept of mail shares is different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
shareType: sharingConfig.isMailShareAllowed ? ShareType.Email : ShareType.Link, | ||
shareType: ShareType.Link, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking, will discuss this in chat for easier comprehension
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. TLDR: |
My fix would be to set the
|
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]>
and what about other things that use the API without specifying what goes to |
e089e40
to
a154393
Compare
/compile |
Signed-off-by: nextcloud-command <[email protected]>
Agree, we can not change default behavior as this would be a breaking change. |
@@ -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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm I thought we do not use empty
for strings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
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