1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-14 02:42:40 +01:00
Commit graph

28 commits

Author SHA1 Message Date
epriestley
3d816e94df Rename "PhabricatorHash::digest()" to "weakDigest()"
Summary: Ref T12509. This encourages code to move away from HMAC+SHA1 by making the method name more obviously undesirable.

Test Plan: `grep`, browsed around.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12509

Differential Revision: https://secure.phabricator.com/D17632
2017-04-06 15:43:33 -07:00
epriestley
4997b6bd02 Never send normal mail to unverified addresses
Summary:
Ref T12237. This tightens our delivery rules, which previously sent normal mail to unverified addresses:

  - We sent general mail to unverified addresses so that you wouldn't miss anything between the time you sign up (or have an account created) and the time you verify your address. This was imagined as a slight convenience for users.
  - We sent automatic reply mail to unverified addresses if they sent mail to us first, saying "we don't recognize that address". This was imagined as a convenience for users who accidentally send mail "From" the wrong address (personal vs work, for example).

I think both behaviors are probably a little better for users on the balance, but not having mail providers randomly shut us off without warning is better for me, personally -- so stop doing this stuff.

This creates a problem which we likely need to solve before the release is cut:

  - On installs which do not require mail verification, mail to you will now mostly-silently be dropped if you never bothered to verify your address.

I'd like to solve this by adding some kind of per-user alert that says "We recently tried to send you some mail but you haven't verified your address.", and giving them links to verify the address and review the mail. I'll pursue this after restoring mail service to `secure.phabricator.com`.

Test Plan:
  - Added a unit test.
  - Unverified my address, sent mail, saw it get dropped.
  - Reverified my address, sent mail, saw it go through.
  - Verified that important mail (password reset, invite, confirm-this-address) either uses "Force Delivery" (skips this check) or "Raw To Addresses" (also skips this check).
    - Verified that Phacility instance stuff is also covered: it uses the same invite flow.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12237

Differential Revision: https://secure.phabricator.com/D17329
2017-02-09 10:20:57 -08:00
epriestley
19b2eb57a9 Improve lipsum generation of projects
Summary:
Fixes T9156.

  - Fix hashtag generation.
  - Fix various badnesses.
  - Improve project name generator.

Test Plan:
```
$ ./bin/lipsum generate projects
 GENERATORS  Selected generators: Projects.
 WARNING  This command generates synthetic test data, including user accounts. It is intended for use in development environments so you can test features more easily. There is no easy way to delete this data or undo the effects of this command. If you run it in a production environment, it will pollute your data with large amounts of meaningless garbage that you can not get rid of.

    Are you sure you want to generate piles of garbage? [y/N] y

 LIPSUM  Generating synthetic test objects forever. Use ^C to stop when satisfied.
Generated "Project": Self-Flying Data Center Swag Performance
Generated "Project": Optimize Cars
Generated "Project": Triaging Culture Optimization
Generated "Project": Automating Experience
Generated "Project": Accelerating NUX Performance
Generated "Project": Optimizing Culture Optimization
Generated "Project": Optimize Hardware
```

{F1042949}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9156

Differential Revision: https://secure.phabricator.com/D14874
2015-12-24 09:06:56 -08:00
epriestley
b3d3130b71 Fix a potentially lax hash comparison
Summary: Via HackerOne. See D14025. I missed this comparison when making the original change.

Test Plan:
  - Used `cat mail.txt | scripts/mail/mail_handler.php --process-duplicates` to pipe mail in a whole lot of times.
  - Tried bad hashes, saw rejections.
  - Tried good hash, saw mail accepted.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D14455
2015-11-10 10:22:25 -08:00
epriestley
de01f3e2e0 Add Maniphest Task email creator to CCs
Summary: Fixes T9369.

Test Plan:
  - Sent a mail with Mail.app to `bugs@local.phacility.com`.
  - Used "View Raw Mail", copy-pasted it into `mail.txt` on disk.
  - Ran `cat mail.txt | ./scripts/mail/manage_mail.php --process-duplicates`.
  - Saw task get created and me get added as CC.
  - Changed "To" to include another user, ran command again, saw task get created and other user get added as CC.

Reviewers: chad

Reviewed By: chad

Subscribers: Korvin

Maniphest Tasks: T9369

Differential Revision: https://secure.phabricator.com/D14086
2015-09-09 14:07:07 -07:00
Joshua Spence
b6d745b666 Extend from Phobject
Summary: All classes should extend from some other class. See D13275 for some explanation.

Test Plan: `arc unit`

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D13283
2015-06-15 18:02:27 +10:00
Joshua Spence
f47e69c015 Mark some strings for translation
Summary: Add some more `pht`izations.

Test Plan: Eyeball it.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: hach-que, Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D13200
2015-06-09 23:06:52 +10:00
Joshua Spence
36e2d02d6e phtize all the things
Summary: `pht`ize a whole bunch of strings in rP.

Test Plan: Intense eyeballing.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: hach-que, Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D12797
2015-05-22 21:16:39 +10:00
epriestley
eb81fd1562 Expose all application mail receivers
Summary:
Fixes T7199. This still isn't a shining example of perfect code, but the raw amount of copy/paste is much lower than it used to be.

  - Reduce code duplication between existing receivers.
  - Expose receiving objects in help menus where appropriate.
  - Connect some "TODO" receivers.

Test Plan:
  - Sent mail to every supported object type.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7199

Differential Revision: https://secure.phabricator.com/D12249
2015-04-01 11:52:02 -07:00
Bob Trahan
c89dc19976 Application emails - move over paste and files
Summary: Fixes T3404 (post D11565), fixes T5952. This infrastructure has been getting deployed against Maniphest and its time to get these other two applications going on it.

Test Plan: created an email address for paste and used `./bin/mail receive-test` ; a paste was successfully created

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T5952, T3404

Differential Revision: https://secure.phabricator.com/D11570
2015-01-29 14:47:32 -08:00
Bob Trahan
799dada3ad MetaMTA - add support for "Default Author"
Summary: Ref T5952. This adds support for a "default author" and deploys it on Maniphest.

Test Plan: used augmented (by this diff) bin/mail receive-test to test creation via an application email with a default author configured and no author specified. a task was created with the author as the default author i configured.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T5952

Differential Revision: https://secure.phabricator.com/D11446
2015-01-28 11:13:29 -08:00
Bob Trahan
53b06408f4 MetaMTA - add (basic) application emails and deploy to Maniphest
Summary: Ref T5952, T3404. This lays the basic plumbing for how this will work, all the way to deploying on Maniphest. Aside from what is mentioned on T5952, I think page(s) on editing application emails could use a little more helpful text about what's going on, similar to how the config page that's getting deprecated works.

Test Plan: ran migration and noted my create email address migrated successfully. used bin/mail to make a task. added another email and used bin/mail to make a task. deleted an email. edited an email. invoked various error states and they all looked good.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T3404, T5952

Differential Revision: https://secure.phabricator.com/D11418
2015-01-19 16:07:26 -08:00
epriestley
e46826ad36 Introduce CAN_EDIT for ExternalAccount, and make CAN_VIEW more liberal
Summary:
Fixes T3732. Ref T1205. Ref T3116.

External accounts (like emails used as identities, Facebook accounts, LDAP accounts, etc.) are stored in "ExternalAccount" objects.

Currently, we have a very restrictive `CAN_VIEW` policy for ExternalAccounts, to add an extra layer of protection to make sure users can't use them in unintended ways. For example, it would be bad if a user could link their Phabricator account to a Facebook account without proper authentication. All of the controllers which do sensitive things have checks anyway, but a restrictive CAN_VIEW provided an extra layer of protection. Se T3116 for some discussion.

However, this means that when grey/external users take actions (via email, or via applications like Legalpad) other users can't load the account handles and can't see anything about the actor (they just see "Restricted External Account" or similar).

Balancing these concerns is mostly about not making a huge mess while doing it. This seems like a reasonable approach:

  - Add `CAN_EDIT` on these objects.
  - Make that very restricted, but open up `CAN_VIEW`.
  - Require `CAN_EDIT` any time we're going to do something authentication/identity related.

This is slightly easier to get wrong (forget CAN_EDIT) than other approaches, but pretty simple, and we always have extra checks in place anyway -- this is just a safety net.

I'm not quite sure how we should identify external accounts, so for now we're just rendering "Email User" or similar -- clearly not a bug, but not identifying. We can figure out what to render in the long term elsewhere.

Test Plan:
  - Viewed external accounts.
  - Linked an external account.
  - Refreshed an external account.
  - Edited profile picture.
  - Viewed sessions panel.
  - Published a bunch of stuff to Asana/JIRA.
  - Legalpad signature page now shows external accounts.

{F171595}

Reviewers: chad, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T3732, T1205, T3116

Differential Revision: https://secure.phabricator.com/D9767
2014-07-10 10:18:10 -07:00
epriestley
d9cdbdb9fa When we fail to process mail, tell the user about it
Summary:
Ref T4371. Ref T4699. Fixes T3994.

Currently, we're very conservative about sending errors back to users. A concern I had about this was that mistakes could lead to email loops, massive amounts of email spam, etc. Because of this, I was pretty hesitant about replying to email with more email when I wrote this stuff.

However, this was a long time ago. We now have Message-ID deduplication, "X-Phabricator-Sent-This-Mail", generally better mail infrastructure, and rate limiting. Together, these mechanisms should reasonably prevent anything crazy (primarily, infinite email loops) from happening.

Thus:

  - When we hit any processing error after receiving a mail, try to send the author a reply with details about what went wrong. These are limited to 6 per hour per address.
  - Rewrite most of the errors to be more detailed and informative.
  - Rewrite most of the errors in a user-facing voice ("You sent this mail..." instead of "This mail was sent..").
  - Remove the redundant, less sophisticated code which does something similar in Differential.

Test Plan:
  - Using `scripts/mail/mail_receiver.php`, artificially received a pile of mail.
  - Hit a bunch of different errors.
  - Saw reasonable error mail get sent to me.
  - Saw other reasonable error mail get rate limited.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T3994, T4371, T4699

Differential Revision: https://secure.phabricator.com/D8692
2014-04-03 18:43:18 -07:00
Joshua Spence
e11adc4ad7 Added some additional assertion methods.
Summary:
There are quite a few tests in Arcanist, libphutil and Phabricator that do something similar to `$this->assertEqual(false, ...)` or `$this->assertEqual(true, ...)`.

This is unnecessarily verbose and it would be cleaner if we had `assertFalse` and `assertTrue` methods.

Test Plan: I contemplated adding a unit test for the `getCallerInfo` method but wasn't sure if it was required / where it should live.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8460
2014-03-08 19:16:21 -08:00
Bob Trahan
41d2a09536 Legalpad - make it work for not logged in users
Summary: Adds "verified" and "secretKey" to Legalpad document signatures. For logged in users using an email address they own, things are verified right away. Otherwise, the email is sent a verification letter. When the user clicks the link the signature is marked verified.

Test Plan: signed the document with a bogus email address not logged in. verified the email that would be sent looked good from command line. followed link and successfully verified bogus email address

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran, asherkin

Maniphest Tasks: T4283

Differential Revision: https://secure.phabricator.com/D7930
2014-01-14 17:17:18 -08:00
epriestley
a3c811f281 Accept case-insensitive mail replies
Summary:
Mailbox sometimes (?) changes the case of the email address (?). Be more liberal in what we accept.

Also fix a minor output bug.

Test Plan: Sent mail to `e1+...` instead of `E1+...`, verified it arrived.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D7575
2013-11-12 21:23:23 -08:00
epriestley
7f11e8d740 Improve handling of email verification and "activated" accounts
Summary:
Small step forward which improves existing stuff or lays groudwork for future stuff:

  - Currently, to check for email verification, we have to single-query the email address on every page. Instead, denoramlize it into the user object.
    - Migrate all the existing users.
    - When the user verifies an email, mark them as `isEmailVerified` if the email is their primary email.
    - Just make the checks look at the `isEmailVerified` field.
  - Add a new check, `isUserActivated()`, to cover email-verified plus disabled. Currently, a non-verified-but-not-disabled user could theoretically use Conduit over SSH, if anyone deployed it. Tighten that up.
  - Add an `isApproved` flag, which is always true for now. In a future diff, I want to add a default-on admin approval queue for new accounts, to prevent configuration mistakes. The way it will work is:
    - When the queue is enabled, registering users are created with `isApproved = false`.
    - Admins are sent an email, "[Phabricator] New User Approval (alincoln)", telling them that a new user is waiting for approval.
    - They go to the web UI and approve the user.
    - Manually-created accounts are auto-approved.
    - The email will have instructions for disabling the queue.

I think this queue will be helpful for new installs and give them peace of mind, and when you go to disable it we have a better opportunity to warn you about exactly what that means.

Generally, I want to improve the default safety of registration, since if you just blindly coast through the path of least resistance right now your install ends up pretty open, and realistically few installs are on VPNs.

Test Plan:
  - Ran migration, verified `isEmailVerified` populated correctly.
  - Created a new user, checked DB for verified (not verified).
  - Verified, checked DB (now verified).
  - Used Conduit, People, Diffusion.

Reviewers: btrahan

Reviewed By: btrahan

CC: chad, aran

Differential Revision: https://secure.phabricator.com/D7572
2013-11-12 14:37:04 -08:00
epriestley
8886416e30 Expand the "PhabricatorExternalAccount" table for new registration
Summary:
Ref T1536. This is the schema code for `PhabricatorExternalAccount` which was previously in D4647. I'm splitting it out so I can put it earlier in the sequence and because it's simple and standalone.

Expands `PhabricatorExternalAccount` to have everything we need for the rest of registration.

Test Plan: Implemented the remainder of new registration on top of this.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1536

Differential Revision: https://secure.phabricator.com/D6169
2013-06-14 06:55:18 -07:00
Jakub Vrana
8b95669e82 Fix typo in typehint 2013-06-05 11:52:12 -07:00
epriestley
5cd13c3c65 Remove the last hardcoding from PhabricatorMetaMTAReceivedMail
Summary:
Moves all remaining mail handling into ReplyHandlers.

Farewell, `getPhabricatorToInformation()`! You were a bad method and no one liked you.

Ref T1205.

Test Plan:
  - Used test console to send mail to Revisions, Tasks, Conpherences and Commits (these all actually work).
  - Used test console to send mail to Requests, Macros, Questions and Mocks (these accept the mail but don't do anything with it, but didn't do anything before either).

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1205

Differential Revision: https://secure.phabricator.com/D5953
2013-05-17 10:00:49 -07:00
epriestley
c967141f92 Make "new task" and "new conpherence" not-so-awful
Summary: Ref T1205. Moves the handling logic for these email types to reply handlers.

Test Plan: Used test form to send conpherence and maniphest mail.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1205

Differential Revision: https://secure.phabricator.com/D5945
2013-05-17 03:51:57 -07:00
epriestley
a548773209 Make "Receive Test" mail form use MailReceivers
Summary: Currently this is fairly hard-coded. Instead, make it use available receivers. Ref T1205.

Test Plan: Used mail form to send mail to various objects (Dnn, Tnn, Cnn, etc.). Only some of these work right now because the receiver thing still hard-codes a bunch of junk.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1205

Differential Revision: https://secure.phabricator.com/D5944
2013-05-17 03:51:33 -07:00
epriestley
2676e91dd8 Move sender validation into MailReceiver classes
Summary:
Ref T1205. Finally able to delete a big chunk of this nastiness.

Make MailReceivers responsible for validating senders. For object creation receivers (bugs, conpherences) this just means that users must not be disabled. For other receivers the senders must be able to see the objects, have the right hashes, etc., according to policy.

Test Plan: Added a bunch of test cases (everything except policy). Verified behavior via the Receive test console.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1205

Differential Revision: https://secure.phabricator.com/D5943
2013-05-17 03:49:29 -07:00
epriestley
5243b0d653 Move computeMailHash() to PhabricatorObjectMailReceiver
Summary: Kick this out of here. Ref T1205.

Test Plan: Grep.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1205

Differential Revision: https://secure.phabricator.com/D5942
2013-05-17 03:49:00 -07:00
epriestley
ff4073c2f4 Copy mail sender selection logic into MailReceivers
Summary: Copies sender identification logic into MailReceivers and makes it basically sane. The mess we run into after this try/catch is terrifying so I'm avoiding actually getting rid of any of it quite yet. Ref T1205.

Test Plan: Added a bit of test coverage. Used Receiver test console to verify some additional behaviors.

Reviewers: btrahan

Reviewed By: btrahan

CC: Afaque_Hussain, aran

Maniphest Tasks: T1205

Differential Revision: https://secure.phabricator.com/D5931
2013-05-15 08:44:54 -07:00
epriestley
b10b1fc65f Add unit tests to cover PhabricatorMailReceiver::matchAddresses()
Summary: Ref T3183. We should accept addresses like `"New Bug" <bugs@example.com>` to match `bugs@example.com`.

Test Plan: Ran unit tests.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3183

Differential Revision: https://secure.phabricator.com/D5923
2013-05-14 15:04:17 -07:00
epriestley
341079c3cf Move some received mail responsibility to applications
Summary:
Ref T1205. Continuation of D5915.

Currently, `PhabricatorMetaMTAReceivedMail` has //all// the logic for routing mail. In particular:

  - New mail receivers in applications must edit it.
  - Mail receivers don't drop out when applications are uninstalled.

Applications have some logic in subclasses of `PhabricatorMailReplyHandler`, but this class is a bit of a mess. It is also heavily based on the assumption that mail receivers are objects (like revisions), but this is not true in at least two cases today (creating new tasks with `bugs@`, creating a new Conpherence thread) and likely other cases in the future (e.g., revision-by-mail).

Move this logic into a new `PhabricatorMailReceiver` classtree. This is similar to `PhabricatorMailReplyHandler` but a bit cleaner and more general. I plan to heavily reduce the responsibilities of `PhabricatorMailReplyHandler` or possibly eliminate it entirely.

For now, the new classtree doesn't do much of interest. The only behavioral change this diff causes is that Phabricator will now reject mail to an application when that application is uninstalled.

I also moved all the `ReplyHandler` classes into `mail/` directories in their respective applications.

Test Plan: Unit tests, used receive test to route mail to various objects.

Reviewers: btrahan

Reviewed By: btrahan

CC: Afaque_Hussain, edward, aran

Maniphest Tasks: T1205

Differential Revision: https://secure.phabricator.com/D5922
2013-05-14 10:57:41 -07:00