Page MenuHomePhabricator

Do not send email notifications to users without view permissions
AbandonedPublic

Authored by fabe on Jan 8 2015, 1:20 PM.

Details

Summary

Ref T4411. Ref T6367. Filter out users in email notifications who do not have view permissions on the object but made it into the subscribed users.

Test Plan

Changed maniphest task permissions, commented on it and received/not-received email notification depending on the permissions set.
Also tried adding projects as subscribers and used custom policies referring to users not actually subscribed but members of a subscribed project.

Diff Detail

Repository
rP Phabricator
Branch
ccrightsnoemail
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 3733
Build 3744: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

fabe updated this revision to Diff 27102.Jan 8 2015, 1:20 PM
fabe retitled this revision from to do not send email notifications to users without view permissions.
fabe updated this object.
fabe edited the test plan for this revision. (Show Details)
fabe added inline comments.Jan 8 2015, 1:21 PM
src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
1936

It seems somewhat duplicate to load the handles and load the Users.
However the policy check needs the real user and the multiplexMail function relies just on object handles...
Is there some way to get an object handle form an existing user object?

I think the fix for this has to be deeper in the stack or occur after some refactoring, because Projects can also be CC'd. I believe this change has incorrect behaviors for projects and mailing lists -- they will either be discarded in all cases, or possibly the policy check will throw (at least right now, it's not meaningful to test if a project can see an object).

If you changed the code to let them survive this check, projects will be evaluated as lists of members later on (in multiplexMail(), I think) and those users will never get checked. So a user who can't see the task will still be able to get mail by being a member of a CC'd project. This should be prevented.

Given the adjacent issues in T6367, I think fixing this is likely to be messy. My initial inclination would be to do T6367 first, although that's also very involved. I'd have to look at things in more detail to come up with a plan of attack.

fabe added a comment.Jan 10 2015, 1:01 PM

Ok yeah you're right. The expansion happens later. it actually happens twice. In the MailReplyHandler and in the MetaMTAMail class again - removing the expansion in the replyhandler seems to have no noticable effect. i still get emails if i'm just a project member.
Sending the email in the right locale would be very messy to resolve right now and i think your idea in T6367 (moving the whole rendering of emails for transactions into a worker) is the right choice. I'll take a look at this in more detail tomorrow.
Fixing the permissions problem for now could go into the ReplyHandler as it already knows about the object it is sending email about. So adding a policy check there should be easy.
I'll attach a diff.

fabe updated this revision to Diff 27175.Jan 10 2015, 1:16 PM
fabe edited edge metadata.

move check into reply handler where phids are already expanded

fabe edited the test plan for this revision. (Show Details)Jan 10 2015, 1:17 PM
joshuaspence retitled this revision from do not send email notifications to users without view permissions to Do not send email notifications to users without view permissions.Feb 26 2015, 1:21 AM
joshuaspence updated this object.
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
fabe abandoned this revision.Jun 5 2015, 9:09 AM