Details
- Reviewers
- None
- Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T4411: Adding a CC to a Maniphest Task should give View rights for that user
T6367: Send email with recipient's language and access levels, not sender's
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 Passed - Unit
No Test Coverage - Build Status
Buildable 3688 Build 3698: [Placeholder Plan] Wait for 30 Seconds
Event Timeline
| src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php | ||
|---|---|---|
| 1960 | It seems somewhat duplicate to load the handles and load the Users. | |
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.
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.