1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-22 13:30:55 +01:00
Commit graph

14773 commits

Author SHA1 Message Date
epriestley
53b25db918 Prevent enormous changes from being pushed to repositoires by default
Summary:
Fixes T13031. "Enormous" changes are basically changes which are too large to hold in memory, although the actual definition we use today is "more than 1GB of change text or `git diff` runs for more than 15 minutes".

If an install configures a Herald content rule like "when content matches /XYZ/, do something" and then a user pushes a 30 GB source file, we can't put it into memory to `preg_match()` it. Currently, the way to handle this case is to write a separate Herald rule that rejects enormous changes. However, this isn't obvious and means the default behavior is unsafe.

Make the default behavior safe by rejecting these changes with a message, similar to how we reject "dangerous" changes (which permanently delete or overwrite history) by default.

Also, change a couple of UI strings from "Enormous" to "Very Large" to reduce ambiguity. See <https://discourse.phabricator-community.org/t/herald-enormous-check/822>.

Test Plan: Changed the definition of "enormous" from 1GB to 1 byte. Pushed a change; got rejected. Allowed enormous changes, pushed, got rejected by a Herald rule. Disabled the Herald rule, pushed, got a clean push. Prevented enormous changes again. Grepped for "enormous" elsewhere in the UI.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: joshuaspence

Maniphest Tasks: T13031

Differential Revision: https://secure.phabricator.com/D18850
2018-01-04 10:02:29 -08:00
epriestley
cb957f8d62 Pile more atrocities onto the Maniphest burnup report
Summary:
See PHI273. Ref T13020. After D18777, tasks created directly into the default status (which is common) via the web UI no longer write a "status" transaction.

This is consistent with other applications, and consistent with the API/email behavior for tasks since early 2016. It also improves the consistency of //reading// tasks via the API.

However, it impacted the "Burnup Report" which relies on directly reading these rows to detect task creation. Until this is fixed properly (T1562), synthetically generate the "missing" transactions which this page expects by looking at task creation dates instead.

Specifically, we:

  - Generate a fake `status: null -> "open"` transaction for every task by looking at the Task table.
  - Go through the transaction list and remove all the legacy `status: null -> "any open status"` transactions. These will only exist for older tasks.
  - Merge all our new fake transactions into the list of transactions.
  - Continue on as though nothing happened, letting the rendering code continue to operate on legacy-looking data.

I think this will slightly miscount tasks which were created directly into a closed status, but this is very rare, and does not significantly impact the accuracy of this report relative to other known issues (notably, merging closed tasks).

This will also get the wrong result if the default status has changed from an "open" status to a "closed" status at any point, but this is exceptionally bizarre/rare.

Ultimately, T1562 will let us delete all this stuff and disavow its existence.

Test Plan:
  - Created some tasks, loaded burnup before/after this patch.
  - My local chart looks more accurate afterwards, but the data is super weird (I used `bin/lipsum` to create a huge number of tasks a couple months ago). I'll vet this on `secure`, which has more reasonable data.

Here's my local chart:

{F5356499}

That's what it //should// look like, it's just hard to be confident that nothing else is hiding there.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13020

Differential Revision: https://secure.phabricator.com/D18853
2018-01-04 10:02:15 -08:00
epriestley
129e3a1208 Fix a minor/harmless race with feed publishers in certain draft states
Summary:
Depends on D18851. Ref T13035. After D18819, revision creation transactions may be split into two groups (if prototypes are enabled).

This split means we have two workers. The first worker doesn't publish feed stories or mail; the second one does.

Currently, both workers call `shouldPublishFeedStory()` before they queue, and then again after the daemons pull them out of the queue. However, the answer to this question can change.

Specifically, this happens:

  - `arc` creates a revision.
  - The first transaction group applies, creating the revision as a draft, and returns `false` from `shouldPublishFeedStory()`, and does not generate related PHIDs. It queues a daemon to send mail, expecting it not to publish a feed story.
  - The second transaction group applies, promoting the revision to "needs review". Since the revision has promoted, `shouldPublishFeedStory()` now returns true. This editor generates related PHIDs and queues a daemon task, expecting it to send mail / publish feed.
  - A few milliseconds pass.
  - The first job gets pulled out of the daemon queue and worked on. It does not have any feed metadata because the object wasn't publishable when the job was queued -- but `shouldPublishFeedStory()` now returns true, so it tries to publish a story without any metadata available. Slightly bad stuff happens (see below).
  - The second job gets pulled out of the daemon queue and worked on. This one has metadata and works fine.

The "slightly bad stuff" is that we publish an empty feed story with no references to any objects, then try to push it to hooks and other listeners. Since it doesn't have any references, it fails to load during the "push to external listeners" phase.

This is harmless but clutters the log and doesn't help anything.

Instead, cache the state of "are we publishing a feed story for this object?" when we queue the worker so it can't race.

Test Plan:
  - Enabled prototypes.
  - Disabled all Herald triggers for Harbormaster build plans.
  - Ran `bin/phd debug task` in one window.
  - Created a revision in a separate window.
  - Before patch: saw "unable to load feed story" errors in the daemon log.
  - After patch: no more "unable to load feed story" errors.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13035

Differential Revision: https://secure.phabricator.com/D18852
2018-01-04 08:14:55 -08:00
epriestley
de6c68b91e Always show "X requested review" in mail to stop some undraft mail from being dropped
Summary: Ref T13035. See that task for a description of the issue.

Test Plan:
  - Enabled prototypes.
  - Disabled all Herald rules that trigger Harbormaster builds.
  - Created a new revision.
  - Before patch: initial review request email was dropped.
  - After patch: initial review request email is sent.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13035

Differential Revision: https://secure.phabricator.com/D18851
2018-01-04 08:14:31 -08:00
epriestley
ead5f4fd9c Add an "Accepting reviewers" Herald field for commits
Summary:
See PHI262. Fixes T12578. Although this is a bit niche and probably better accomplished through advisory/soft measures ("Add blocking reviewers") in most cases, it isn't difficult to implement and doesn't create any technical or product tension.

If installs write a rule that blocks commits, that will probably also naturally lead them to an "add reviewers" rule anyway.

Also, allow packages to be hit with the typeahead. They're valid reviewers but previously you couldn't write rules against them, for no actual reason.

Test Plan: Used test console to run this against commits, got sensible results for the field value.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12578

Differential Revision: https://secure.phabricator.com/D18839
2017-12-26 15:59:36 -08:00
epriestley
ad4db9b2f3 Separate "Set/Reset Password" from "Change Password"
Summary:
See PHI223. Ref T13024. There's a remaining registration/login order issue after the other changes in T13024: we lose track of the current URI when we go through the MFA flow, so we can lose "Set Password" at the end of the flow.

Specifically, the flow goes like this today:

  - User clicks the welcome link in email.
  - They get redirected to the "set password" settings panel.
  - This gets pre-empted by Legalpad (although we'll potentially survive this with the URI intact).
  - This also gets pre-empted by the "Set MFA" workflow. If the user completes this flow, they get redirected to a `/auth/multifactor/?id=123` sort of URI to highlight the factor they added. This causes us to lose the `/settings/panel/password/blah/blah?key=xyz` URI.

The ordering on this is also not ideal; it's preferable to start with a password, then do the other steps, so the user can return to the flow more easily if they are interrupted.

Resolve this by separating the "change your password" and "set/reset your password" flows onto two different pages. This copy/pastes a bit of code, but both flows end up simpler so it feels reasonable to me overall.

We don't require a full session for "set/reset password" (so you can do it if you don't have MFA/legalpad yet) and do it first.

This works better and is broadly simpler for users.

Test Plan:
  - Required MFA + legalpad, invited a user via email, registered.
    - Before: password set flow got lost when setting MFA.
    - After: prompted to set password, then sign documents, then set up MFA.
  - Reset password (with MFA confgiured, was required to MFA first).
  - Tried to reset password without a valid reset key, wasn't successful.
  - Changed password using existing flow.
  - Hit various (all?) error cases (short password, common password, mismatch, missing password, etc).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13024

Differential Revision: https://secure.phabricator.com/D18840
2017-12-26 08:34:14 -08:00
epriestley
66b74073be Provide ANSI color information for Harbormaster build status via API
Summary:
Ref PHI261. This moves Harbormaster build status to work more similarly to other modern status types, like Differential revision status, where we try to specify as much behavior on the server as possible so that the client and server can vary independently.

(I don't have any specific plans to make Harbormaster build status configurable on the server side, but it isn't out of the question.)

Test Plan: Ran `harbormaster.querybuilds` (saw same data as before) and `harbormaster.build.search` (saw same data as before, plus new ANSI color data).

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Differential Revision: https://secure.phabricator.com/D18838
2017-12-23 11:39:05 -08:00
Mukunda Modell
bd5aa0c90f Prevent hiding the PhabricatorProjectDetailsProfileMenuItem
Summary: This probably isn't the best solution, however, it conveniently avoids the bug from T13033. It would probably be more user-friendly (but more difficult to implement) if we allowed either Project Details //or// Workboard to be hidden but not both.

Test Plan: Tested locally, indeed this prevents hiding the menu item.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D18836
2017-12-23 11:38:05 -08:00
epriestley
84cf493879 Allow "Manage" to be the default menu item for projects
Summary:
Fixes T13033. Currently (prior to D18836) all the default items for projects can be hidden. When this occurs, the main project page fatals.

If we fix the fatal narrowly (don't try to call `null->getBuiltinKey()` when `$default` is `null`), it would 404, which is a little better but not by much.

After D18836 you can't hide "Profile", which is pretty sensible, and effectively fixes this. This change doubles down: let "Manage" be a default, and send the user there if we can't find a different default.

Ideally, the MenuEngine itself will do more rendering eventually (as it does for some of the newer Home stuff) and could handle this defaulting behavior with less special casing, but we'd still end up in a similar situation if a project had only one "Link" item to "coolcats.com" or something: redirecting the user to "coolcats.com" is probably better than fataling, but not by a huge margin, and not likely to be what they expect.

Test Plan:
Before D18836, disabled both "Profile" and "Workboard" items on a project. Visited project page.

Before patch: fatal. After patch: manage page.

After D18836, you can't do this and just get the profile, so this is sort of moot and mostly future-proofing/for-completeness.

Reviewers: 20after4, amckinley

Reviewed By: amckinley

Maniphest Tasks: T13033

Differential Revision: https://secure.phabricator.com/D18843
2017-12-23 11:37:12 -08:00
Dmitri Iouchtchenko
393824656f Don't notify without notifiable attendees
Summary: Events with no attendees (e.g. fresh instances of recurring events) would trigger an exception when sending notifications, because `$attendee_map` would still get populated.

Test Plan: Declined event, restarted daemons. Did not see exception. Accepted event, restarted daemons. Saw "[Calendar] [Reminder]" email.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D18835
2017-12-21 12:46:46 -08:00
epriestley
e411d75964 Fix an issue where blame could fatal for unrecognized authors
Summary: See PHI255. See <https://discourse.phabricator-community.org/t/error-generating-blame-data/766>.

Test Plan:
  - Viewed a file contributed to by users with no Phabricator user accounts, in Diffusion.
  - Enabled blame.
  - Before patch: blame failed, fatal in logs.
  - After patch: blame worked.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18833
2017-12-20 11:20:23 -08:00
epriestley
8dccf05c4c Manually set "max_allowed_packet" to 1GB for "mysqldump"
Summary:
We have one production instance with failing database backups since they recently uploaded a 52MB hunk. The production configuration specifies a 64MB "max_allowed_packet" in `[mysqld]`, but this doesn't apply to `mysqldump` (we'd need to specify it in a separate `[mysqldump]` section) and `mysqldump` runs with an effective limit of the default (16MB).

We could change our production config to specify a value in `[mysqldump]`, but just change it unconditionally at execution time since there's no reason for any user to ever want this command to fail because they have too much data.

Test Plan: Dumped locally, will verify production backup goes through cleanly.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18834
2017-12-20 10:29:02 -08:00
epriestley
7cbbe2ccf7 When users browse to a submodule path in Diffusion explicitly, don't fatal
Summary: Ref T13030. See PHI254. This behavior could be cleaner than I've made it, but it fixes the "this is totally broken" issue, replacing a fatal/exception with an informative (just not terribly useful) page.

Test Plan:
  - Added a submodule to a repository.
  - In Diffusion, clicked some other file next to the submodule, then edited the URI to the submodule path instead.
    - Before patch: fatal.
    - After patch: relatively useful message about this being a submodule.

Note that it's normally hard to hit this URI directly. In the browse view, submodules are marked up as directories and linked to a separate submodule resolution flow.

{F5321524}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13030

Differential Revision: https://secure.phabricator.com/D18831
2017-12-18 09:18:22 -08:00
epriestley
a1ad184ddd Denormalize added and removed line counts for the current diff onto revisions
Summary:
See PHI230. Currently, we denormalize raw line counts onto diffs and revisions, but not added/removed line counts.

I'd like to try a `[---+  ]` sort of size hint element (see D16322 for more) as a general approach to conveying size information at a glance and see how it feels, since I think the raw size number isn't very scannable/useful and it may be a significant improvement to hint about how much of a change is throwing stuff out vs adding new stuff.

This just makes the data available without any subquerying and doesn't actually change the UI.

Test Plan:
Created a revision, saw detailed change information populate in the database.

```
mysql> select * from differential_revision where id = 292\G
*************************** 1. row ***************************
              id: 292
           title: WIP
   originalTitle: WIP
            phid: PHID-DREV-ux3cxptibn3l5pxsug3z
          status: draft
         summary: asdf
        testPlan: asdf
      authorPHID: PHID-USER-cvfydnwadpdj7vdon36z
lastReviewerPHID: NULL
       lineCount: 41
     dateCreated: 1513179418
    dateModified: 1513179418
        attached: []
         mailKey: h4mn6perdio47o4beomyvu75zezwvredx3mbrlgz
      branchName: NULL
      viewPolicy: users
      editPolicy: users
  repositoryPHID: PHID-REPO-wif5lutk5gn3y6ursk4p
      properties: {"lines.added":40,"lines.removed":1}
  activeDiffPHID: PHID-DIFF-ixjphpunpkenqgukpmce
1 row in set (0.00 sec)
```

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18832
2017-12-18 09:17:55 -08:00
epriestley
e1d6bad864 Stop trying to assess the image dimensions of large files and file chunks
Summary:
Depends on D18828. Ref T7789. See <https://discourse.phabricator-community.org/t/git-lfs-fails-with-large-images/584>.

Currently, when you upload a large (>4MB) image, we may try to assess the dimensions for the image and for each individual chunk.

At best, this is slow and not useful. At worst, it fatals or consumes a ton of memory and I/O we don't need to be using.

Instead:

  - Don't try to assess dimensions for chunked files.
  - Don't try to assess dimensions for the chunks themselves.
  - Squelch errors for bad data, etc., that `gd` can't actually read, since we recover sensibly.

Test Plan:
  - Created a 2048x2048 PNG in Photoshop using the "Random Noise" filter which weighs 8.5MB.
  - Uploaded it.
  - Before patch: got complaints in log about `imagecreatefromstring()` failing, although the actual upload went OK in my environment.
  - After patch: clean log, no attempt to detect the size of a big image.
  - Also uploaded a small image, got dimensions detected properly still.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T7789

Differential Revision: https://secure.phabricator.com/D18830
2017-12-18 09:17:32 -08:00
epriestley
5295840a4c Restore the "Download from Git LFS" UI button to Diffusion
Summary: Depends on D18827. Ref T7789. See PHI204. See PHI131. This button got accidentally removed in Diffusion refactoring (`$data` is no longer used).

Test Plan: {F5321459}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T7789

Differential Revision: https://secure.phabricator.com/D18828
2017-12-18 09:13:19 -08:00
epriestley
8e416474c0 Add a Herald pre-commit field for detecting LFS usage
Summary: Depends on D18825. Ref T7789. See PHI131. Allows installs to selectively disable LFS by adding Herald rules to block commits that use LFS.

Test Plan:
  - Wrote an LFS rule ("When commit uses git lfs, block commit").
  - Pushed an LFS commit: rejected.
  - Pushed a non-lFS commit: success.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T7789

Differential Revision: https://secure.phabricator.com/D18827
2017-12-18 09:12:52 -08:00
epriestley
e34b4bbd90 Move the Git LFS gate to dedicated (non-prototype) config
Summary: See PHI131. Ref T7789. Although this probably isn't 100% complete, there don't seem to be any actual, known, practical blocking issues remaining (everything is either heresay or not reproducible).

Test Plan: Tried to push LFS locally, got blocked with a helpful message. Enabled setting, tried to push LFS locally, got a successful push.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T7789

Differential Revision: https://secure.phabricator.com/D18825
2017-12-18 09:12:22 -08:00
epriestley
c9a0d68340 Allow Herald rules to add comments
Summary:
See PHI242. All use cases for this that I know of are pretty hacky, but they don't seem perilous, and it's easier than webhooks.

See P1895, T10183, and T9853 for me previously refusing to implement this since all those use cases were also pretty bad.

Test Plan:
  - Wrote a rule to add comments, saw it add comments.
  - Reviewed summary, re-edited rule, reviewed transcript to check that all the strings worked OK.
  - Wrote a new rule for a non-commentable object (a blog) to make sure I wasn't offered the "Add a comment" action.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18823
2017-12-18 09:10:57 -08:00
Tim Hirsh
60e5c0ec1b Add drydock.blueprint.edit Conduit method
Summary:
Ref: https://admin.phacility.com/PHI243

Since our use case primarily focuses on transaction editing, this patch implements the `drydock.blueprint.edit` api method with the understanding that:
a) this is a work in progress
b) object editing is supported, but object creation is not yet implemented

Test Plan:
* updated existing blueprints via Conduit UI
* regression tested `maniphest.edit` by creating new and updating existing tasks

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, yelirekim, jcox

Differential Revision: https://secure.phabricator.com/D18822
2017-12-08 11:55:08 -05:00
epriestley
6d3baa92f9 Execute Herald again after promoting revisions out of the "Draft" state
Summary:
Fixes T13027. Ref T2543. When revisions promote from "Draft" because builds finish or no builds are configured, the status currently switches from "Draft" to "Needs Review" without re-running Herald.

This means that some rules -- notably, "Send me an email" rules -- don't fire as soon as they should.

Instead of applying this promotion in a hacky way inline, queue it and apply it normally in a second edit, after the current group finishes.

Test Plan:
  - Created a revision, reviewed Herald transcripts.
  - Saw three Herald passes:
    - First pass (revision creation) triggered builds and no email.
    - Second pass (builds finished) did not trigger builds (no update) and did not trigger email (revision still a draft).
    - Third pass (after promotion out of 'draft') did not trigger builds (no update) but did trigger email (revision no longer a draft).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13027, T2543

Differential Revision: https://secure.phabricator.com/D18819
2017-12-05 12:13:56 -08:00
epriestley
4f8340c05f Restore the "Log In" menubar action
Summary:
See <https://discourse.phabricator-community.org/t/activation-link-in-welcome-mail-only-works-if-new-user-isnt-semi-logged-in/740/7>.

In T13024, I rewrote the main menu bar to hide potentially sensitive items (like notification and message counts and saved search filters) until users fully log in.

However, the "Log In" item got caught in this too. For clarity, rename `shouldAllowPartialSessions()` to `shouldRequireFullSession()` (since logged-out users don't have any session at all, so it would be a bit misleading to say that "Log In" "allows" a partial session). Then let "Log In" work again for logged-out users.

(In most cases, users are prompted to log in when they take an action which requires them to be logged in -- like creating or editing an object, or adding comments -- so this item doesn't really need to exist. However, it aligns better with user expectations in many cases to have it present, and some reasonable operations like "Check if I have notifications/messages" don't have an obvious thing to click otherwise.)

Test Plan: Viewed site in an incognito window, saw "Log In" button again. Browsed normally, saw normal menu.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18818
2017-12-05 12:13:10 -08:00
epriestley
a989dd181d Fix Mercurial commit history ordering
Summary:
See <https://discourse.phabricator-community.org/t/diffusion-observed-mercurial-repository-history-broken/825>.

In D18769, I rewrote this from using the `--branch` flag (which is unsafe and does not function on branches named `--config=x.y` and such).

However, this rewrite accidentally changed the result order, which impacted Mercurial commit hisotry lists and graphs. Swap the order of the constraints so we get newest-to-oldest again, as expected.

Test Plan: Viewed a Mercurial repository's history graph, saw sensible chronology after the patch.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18817
2017-12-05 12:12:44 -08:00
lkassianik
46d496b8cc Fix error for URL's that could mean several commits
Summary: Ref T13001, URLs that return multiple commits should show a list of those commits. Not sure if the actual list looks very pretty this way, but was wondering if this approach was vaguely correct.

Test Plan:
- Navigate to `install/rPbd3c23`
- User should see a list view providing links to `install/rPbd3c2355e8e2b220ae5e3cbfe4a057c8088c6a38` and `install/rPbd3c239d5aada68a31db5742bbb8ec099074a561`

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Maniphest Tasks: T13001

Differential Revision: https://secure.phabricator.com/D18816
2017-12-05 19:24:57 +00:00
epriestley
c924351a58 Mark sessions as "signed all documents" when Legalpad has been uninstalled
See PHI238. When an install uninstalls "Legalpad", we were incorrectly failing
to mark sessions as "Signed All Required Documents" by bailing early.

Test Plan: Uninstalled Legalpad, logged in.
2017-12-02 16:15:59 -08:00
lkassianik
42034e6739 Property list view on Diffusion commits should show build status but not Subscriptions, Projects, or Tokens
Summary: Ref T13019, adds build status back to Diffusion commits

Test Plan: Open a Diffusion commit that has a build status, property list view should show the build status, but not Subscriptions, Projects, or Tokens.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Maniphest Tasks: T13019

Differential Revision: https://secure.phabricator.com/D18813
2017-12-01 18:16:26 +00:00
epriestley
5240cffd9c Fix an issue where Diffusion could fatal if the default branch was deleted
Summary: See PHI234. In T12931 we improved the behavior of Diffusion when a repository's default branch is set to a branch that does not exist, but in T11823 the way refcursors work changed, and we can now get a cursor (just with no positions) back for a deleted branch. When we did, we didn't handle things gracefully.

Test Plan:
  - Set default branch to a deleted branch, saw nice error instead of fatal.
  - Set default branch to a nonexistent branch which never existed, saw nice error.
  - Set default branch to existing "master", saw repository normally.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18811
2017-11-30 14:06:41 -08:00
epriestley
14cc0abeb3 Fix several safety issues with repository URIs
Summary:
See PHI234. Several issues here:

  - The warning about observing a repository in Read/Write mode checks the raw I/O type, not the effective I/O type. That means we can fail to warn if other URIs are set to "Default", and "Default" is "Read/Write" in practice.
  - There's just an actual typo which prevents the "Observe" version of this error from triggering properly.

Additionally, add more forceful warnings that "Observe" and "Mirror" mean that you want to //replace// a repository with another one, not that we somehow merge branches selectively. It isn't necessarily obvious that "Observe" doesn't mean "merge/union", since the reasons it can't in the general case are somewhat subtle (conflicts between refs with the same names, detecting ref deletion).

Test Plan:
Read documentation. Hit the error locally by trying to "Observe" while in Read/Write mode:

{F5302655}

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18810
2017-11-30 14:06:21 -08:00
epriestley
f786c86a6a Don't require the "gd" extension be installed in order to run unit tests
Summary:
Ref T10405. If `gd` is not installed, a number of unit tests currently fail because they generate synthetic users and try to composite profile pictures for them.

Instead, just fall back to a static default if `gd` is not available.

Test Plan: Faked this pathway, created a new user, saw a default profile picture.

Reviewers: amckinley, lpriestley, avivey

Reviewed By: avivey

Maniphest Tasks: T10405

Differential Revision: https://secure.phabricator.com/D18812
2017-11-30 13:51:31 -08:00
epriestley
0807b70ea1 Add an explicit warning in the Differential transaction log when users skip review
Summary:
Ref T10233. See PHI231. When users ignore the `arc land` prompt about bad revision states, make it explicitly clear in the transaction log that they broke the rules.

You can currently figure this out by noticing that there's no "This revision is accepted and ready to land." message, but it's unrealistic to expect non-expert users to look for the //absence// of a message to indicate something, and this state change is often relevant.

Test Plan: {F5302351}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T10233

Differential Revision: https://secure.phabricator.com/D18808
2017-11-30 11:03:55 -08:00
Aviv Eyal
d8f2630d5c Modernize QuickSearch typeahead
Summary:
Use ClassQuery to find datasources for the quick-search.

Mostly, this allows extensions to add quicksearches.

Test Plan:
using `/typeahead/class/`, tested several search terms that make sense.
Removed the tag interface from a datasource, which removed it from results.

Reviewers: epriestley, amckinley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D18760
2017-11-30 15:07:49 +00:00
epriestley
1b76250f89 Disallow "Accept" on drafts, allow "Resign"
Summary:
Depends on D18801. Ref T2543. See PHI229. I missed "Accept" before, but intended to disallow it (like "Reject") since I don't want drafts to be reviewable.

However, "Resign" seems fine to allow? So let's allow that for now.

Test Plan: Was no longer offered "Accept" on draft revisions.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T2543

Differential Revision: https://secure.phabricator.com/D18802
2017-11-28 13:46:26 -08:00
epriestley
54f131dc4b Make "first broadcast" rules for Differential drafts more general
Summary:
See PHI228. Ref T2543. The current logic gets this slightly wrong: prototypes are off, you create a draft with `--draft`, then promote it with "Request Review". This misses both branches.

Instead, test these conditions a little more broadly. We also need to store broadcast state since `getIsNewObject()` isn't good enough with this workflow.

Test Plan:
  - With prototypes on and autopromotion, got a rich email after builds finished.
  - With prototypes off, got a rich email immediately.
  - With prototypes off and `--draft`, got a rich email after "Request Review".

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T2543

Differential Revision: https://secure.phabricator.com/D18801
2017-11-28 13:42:10 -08:00
epriestley
c7d6fd198c Support "Set X to" as an action in Herald for tokenizer/datasource custom fields
Summary:
See PHI173. Adds custom field support for Herald actions, and implements actions for "Datasource/Tokenizer" fields.

The only action available for now is "set field to...". Other actions ("Add values", "Remove values") might make sense in the future for these fields, but there's currently no use case. For most other field types (text, select, checkbox, etc) only "Set to" makes sense.

Test Plan:
  - Added a "datasource" custom field to the custom field definition in Config.
  - Added a "if field is empty, set field to default value X" rule to Herald.
  - Created a task with a nonempty field: no Herald trigger.
  - Created a task with an empty field: Herald fired.
  - Reviewed rule and transcripts for text strings.

{F5297615}

{F5297616}

{F5297617}

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18784
2017-11-28 13:41:52 -08:00
epriestley
e919233b31 Don't show personalized menu items until users establish a full session
Summary:
Depends on D18792. Fixes T13024. Fixes T89198. Currently, when users are logging in initially (for example, need to enter MFA) we show more menu items than we should.

Notably, we may show some personalized/private account details, like the number of unread notifications (probably not relevant) or a user's saved queries (possibly sensitive). At best these are misleading (they won't work yet) and there's an outside possibility they leak a little bit of private data.

Instead, nuke everything except "Log Out" when users have partial sessions.

Test Plan:
Hit a partial session (MFA required, email verification required) and looked at the menu. Only saw "Log Out".

{F5297713}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13024

Differential Revision: https://secure.phabricator.com/D18793
2017-11-28 10:01:58 -08:00
epriestley
dc62d18b47 Allow MFA enrollment before email verification
Summary: Depends on D18791. Ref T13024. This clears up another initialization order issue, where an unverified address could prevent MFA enrollment.

Test Plan: Configured both verification required and MFA required, clicked "Add Factor", got a dialog for the workflow.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13024

Differential Revision: https://secure.phabricator.com/D18792
2017-11-28 10:01:09 -08:00
epriestley
ab0f61aa32 Tell users to "Wait Patiently" for admin account verification later in the registration process
Summary:
Depends on D18790. Ref T13024. Fixes T8335. Currently, "unapproved" and "disabled" users are bundled together. This prevents users from completing some registration steps (verification, legalpad documents, MFA enrollment) before approval.

Separate approval out and move it to the end so users can do all the required enrollment stuff on their end before we roadblock them.

Test Plan: Required approval, email verification, signatures, and MFA. Registered an account. Verified email, signed documents, enrolled in MFA, and then got prompted to wait for approval.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13024, T8335

Differential Revision: https://secure.phabricator.com/D18791
2017-11-28 10:00:03 -08:00
epriestley
c7718d3a21 Ask users to sign Legalpad documents before requiring they enroll in MFA
Summary:
Depends on D18789. Ref T13024. See PHI223. Currently, if `security.require-multi-factor-auth` and Legalpad "Signature Required" documents are //both// set, it's not possible to survive account registration, since MFA is requiried to sign and signatures are required to add MFA.

Instead, check for signatures before requiring MFA enrollment. This makes logical sense, since it's silly to add MFA if you don't agree to a Terms of Service or whatever.

(Note that if you already have MFA, we prompt for that first, before either of these steps, which also makes sense.)

Test Plan: Configured `security.require-multi-factor-auth`. Added a signature-required document. Loaded a page as a new user. Went through signature workflow, then through the MFA enrollment workflow.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13024

Differential Revision: https://secure.phabricator.com/D18790
2017-11-28 09:59:39 -08:00
epriestley
e850bc6b95 When requiring login signatures, order documents from oldest to newest
Summary: Depends on D18788. Ref T13024. Currently, we prompt users to sign from newest to oldest. This seems less intuitive than oldest to newest.

Test Plan: Dumped document order, saw it swap to oldest-first.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13024

Differential Revision: https://secure.phabricator.com/D18789
2017-11-28 09:59:22 -08:00
epriestley
ba4b9f7184 Refactor on-login Legalpad document signature requirement
Summary: Depends on D18786. Ref T13024. I'm going to change the order this occurs in, but move it to a separate method and clean it up a little first.

Test Plan: Added a new document as required, reloaded, signed it, got logged into a session.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13024

Differential Revision: https://secure.phabricator.com/D18788
2017-11-28 09:58:53 -08:00
epriestley
71406ca93b Lightly modernize LegalpadDocumentSearchEngine
Summary: Depends on D18785. Ref T13024. While I'm in here, update this a bit to use the newer stuff.

Test Plan: Searched for Legalpad documents, saw more modern support (subscribers, order by, viewer()).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13024

Differential Revision: https://secure.phabricator.com/D18786
2017-11-28 09:56:49 -08:00
epriestley
3d742eb50b Lightly modernize LegalpadDocumentQuery
Summary: Ref T13024. Updates LegalpadDocumentQuery to use slightly more modern constructions.

Test Plan: Queried for Legalpad documents, got the same results.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13024

Differential Revision: https://secure.phabricator.com/D18785
2017-11-28 09:56:33 -08:00
epriestley
00baa3c1dd Hide archived projects only on workboards, not hovercards
Summary:
See PHI225. Previously, see D15335 / T10413. On workboards, we hide archived project tags since they aren't terribly useful in that context, at least most of the time. Originally, see T10349#159916 and D15297.

However, hovercards reuse this display logic, and it's inconsistent/confusing to hide them there, since the actual "Tags" elements on task pages show them. Narrow the scope of this rule.

Test Plan:
  - Viewed a hovercard for a task with an archived project tagged, saw archived project.
  - Viewed a workboard for the same task, saw only unarchived projects other than the current board tagged (this behavior is unchanged).

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18783
2017-11-27 14:37:51 -08:00
epriestley
49b57eae7d Revert partial/nonfunctional OpenGraph support
Summary:
Ref T13018. See that task and the Discourse thread for discussion.

This doesn't work as-is and we need to `og:description` everything to make it work. I don't want to sink any more time into this so just back all the changes out for now.

(The `<html>` change is unnecessary anyway.)

Test Plan: Strict revert.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13018

Differential Revision: https://secure.phabricator.com/D18782
2017-11-22 15:21:10 -08:00
epriestley
c3d6c4b0ee Include OpenGraph prefix material in <html> tag if OpenGraph is enabled
Summary: Ref T13018. Discourse doesn't seem to be picking this up yet (see <https://discourse.phabricator-community.org/t/opengraph-test-topic/762>) so maybe it really needs this meta-meta-XML stuff?

Test Plan: Will push.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13018

Differential Revision: https://secure.phabricator.com/D18781
2017-11-22 11:37:08 -08:00
epriestley
2c72c2b924 Add basic support for OpenGraph header tags for public installs
Summary: Ref T13018. This is easy to get working roughly, at least, and seems reasonable.

Test Plan: Viewed page source, saw tags. Custom header logo still worked. Pretty hard to debug against a local install since Disqus / debugger tools can't hit it, but I'll see what it looks like in production and tweak it if I got anything horribly wrong.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13018

Differential Revision: https://secure.phabricator.com/D18780
2017-11-22 11:16:56 -08:00
epriestley
2d4a158356 Fix a bad link target in Diffusion content search results
Summary: See <https://discourse.phabricator-community.org/t/broken-links-in-diffusion-pattern-search-results-page/757>. This links to the display path, which is incorrect.

Test Plan:
  - In any repository, browsed into a directory.
  - Used pattern search to search for something that hits results.
  - Clicked the title (filename/path) of a result table.
    - Before patch: URL omits path context, 404 or wrong result.
    - After patch: taken to proper page.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18779
2017-11-22 11:16:14 -08:00
epriestley
1cb0d41367 Fix inbound mail handling for messages with no HTML body part
Summary:
See D18776. See <https://discourse.phabricator-community.org/t/cant-create-maniphest-tasks-by-email/754/2>.

The change in D18776 to improve handling of non-utf8 HTML parts broke handling of mail with //no// HTML parts. Partly, this is because MimeMailParser has a "traditional" PHP-style API where the return type is an exciting surprise.

Test Plan:
  - Sent a text-only message in `Mail.app`.
  - Used "Show Raw" to copy it to `mail.txt`, verifying that the raw message contains ONLY a text body.
  - Ran `cat mail.txt | ./scripts/mail/mail_handler.php --trace --process-duplicates`.
    - Before patch: error about bad `idx()` on a non-array.
    - After patch: clean mail processing.
  - Did the same with a message with both HTML and text bodies to make sure I didn't break anything.

Ideally we'd probably get test coverage on this, but it's been touched roughly once a year since 2013 so it'll probably hold.

Reviewers: amckinley, alexmv

Reviewed By: amckinley, alexmv

Differential Revision: https://secure.phabricator.com/D18778
2017-11-22 11:14:24 -08:00
epriestley
d321cc810a Freeze "maniphest.gettasktransactions" and make status/priority transactions more consistent
Summary:
Ref T13020. See PHI221.

Freeze legacy method `maniphest.gettasktransactions` in favor of modern method `transaction.search`.

Remove legacy "null on create" behavior from Maniphest status and priority transactions. This behavior is obsolete with EditEngine, and leads to inconsistent transaction sets in the transaction record.

The desired behavior is that transactions which don't do anything (e.g., default value was not changed) don't appear in the transaction log.

Test Plan:
  - Viewed API UI and saw `maniphest.gettasktransactions` marked as "Frozen".
  - Created a new task via web UI (without changing status/priority), queried transactions with `maniphest.gettasktransacitons`/`transaction.search`, no longer saw "null on create" no-op transactions in record.
    - Web UI is unchanged, since these transactions were hidden before and now do not exist.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13020

Differential Revision: https://secure.phabricator.com/D18777
2017-11-22 11:13:53 -08:00
Alex Vandiver
d2cff6a2cf Transcode the HTML part of incoming email into UTF-8 as well
Summary:
D1093 did this for just the text/plain part of incoming
email.  Most text/html parts choose to either use entity encoding
//or// are already UTF-8, thus obviating the need to transcode the
HTML part.  However, this is not always the case, and leads to dropped
messages, by way of:

```
EXCEPTION: (Exception) Failed to JSON encode value (#5: Malformed UTF-8 characters, possibly incorrectly encoded): Dictionary value at key "html" is not valid UTF8, and cannot be JSON encoded: [snip HTML part of message content]```

Generalize the charset transcoding to not apply to just the text/plain part, but
both text/plain and text/html parts.

Test Plan:
Fed in a Windows-1252-encoded text/html part with 0x92
bytes in it; verified that $content only contained valid UTF-8 after
this change.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D18776
2017-11-16 10:08:39 -08:00