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

15721 commits

Author SHA1 Message Date
epriestley
c6fc05ee09 Pull Git filesize logic into a separate LowLevel query and use more Iterators
Summary:
Depends on D19829. Ref T13216. See PHI908. The current implementation is kind of a lot to live in `CommitHookEngine` and will likely fail if `git diff-tree` produces more than 2GB of output.

Pull it out and make it slightly more robust against enormous commits. It's probably limited by this, now:

```
implode("\n", $every_path)
```

We could replace that with some `PhutilReverseRopeSource` primitive or something but since we don't have one of those and it seems unlikely that we'll hit this case in practice, I left it here for now with just the easy stuff converted to be stream-oriented.

Test Plan:
Used this script to test the query against various commits, got good results:

```
<?php

require_once 'scripts/init/init-script.php';

$viewer = PhabricatorUser::getOmnipotentUser();

$repository = id(new PhabricatorRepositoryQuery())
  ->setViewer($viewer)
  ->withCallsigns(array('P'))
  ->executeOne();

var_dump(
  id(new DiffusionLowLevelFilesizeQuery())
    ->setRepository($repository)
    ->withIdentifier($argv[1])
    ->execute());
```

Used this to find large commits in history and pull filesizes (worked great, although our largest commit only touches a couple thousand paths):

```
for hash in `git log --format=%H`; do echo -n $hash; echo -n ' '; git diff-tree -r --no-commit-id $hash | wc -l | awk '{print $1}'; done | awk '{print $2 " " $1}' | sort -n
```

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19830
2018-11-28 14:32:59 -08:00
epriestley
fd12b37d16 Modularize Repository transactions
Summary: Depends on D19828. Ref T13216. Before adding new transactions to repositories (filesize limit, copy time limit, etc) modularize the existing transactions.

Test Plan:

- Created repository.
- Edited callsign (invalid, valid, duplicate, add, remove).
- Edited short name (invaild, valid, duplicate, add, remove).
- Edited description (add, remove).
- Edited encoding (invalid, valid, remove).
- Allowed/denied dangerous changes.
- Allowed/denied enormous chagnes.
- Activated, deactivated, reactivated.
- Changed tags.
- Changed push policy.
- Changed default branch (add, remove).
- Changed track only: add, remove, invalid function, invalid regex.
- Changed autoclose only: add, remove, invalid function, invalid regex.
- Changed publish/notify.
- Changed autoclose.
- Changed staging area (add, remove, invalid).
- Changed blueprints (add, remove).
- Changed symbols (add, remove).
- Grepped for `PhabricatorRepositoryTransaction::TYPE_`.
- Reviewed transaction history:

{F6021036}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19829
2018-11-28 14:29:18 -08:00
epriestley
c25d2a399d Separate the repository management UI into sections
Summary: Depends on D19826. Ref T13216. We have a fair number of options here; add some groups so the "Build" stuff can go in a little subcategory and such.

Test Plan: {F6020896}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19827
2018-11-28 13:53:30 -08:00
epriestley
c457d23a1d Tailor the "no reviewers on this revision" warnings to handle the case where all reviewers have resigned
Summary:
Ref T13216. See PHI985. We currently use a banner to warn you when a revision has no reviewers or only disabled users, but since the changes to track "Resign" more explicilty we'll no longer warn you if everyone has resigned.

(Previously, they'd no longer be reviewers, so you'd end up with the "no reviewers are assigned" warning if everyone resigned.)

This can still interact slightly oddly with some states (e.g., only a package or project reviewer) but I'd like to wait for T731 to tighten those cases up, and they're more advanced/unusual.

Test Plan:
{F6026832}

{F6026833}

{F6026834}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19834
2018-11-28 13:50:29 -08:00
epriestley
01c7be059d Add support for "harbormaster.target.search"
Summary: Ref T13222. See PHI986. See PHI896. Harbormaster build targets don't currently have a modern "*.search" API, but there's no reason not to provide one (even if some of the use cases are a little bit questionable).

Test Plan: {F6032423}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D19841
2018-11-28 13:49:27 -08:00
epriestley
2f11001f6e Allow "Change Subtype" to be selected from the comment action stack
Summary:
Ref T13222. See PHI683. Currently, you can "Change subtype..." via Conduit and the bulk editor, but not via the comment action stack or edit forms.

In PHI683 an install is doing this often enough that they'd like it to become a first-class action. I've generally been cautious about pushing this action to become a first-class action (there are some inevitable rough edges and I don't want to add too much complexity if there isn't a use case for it) but since we have evidence that users would find it useful and nothing has exploded yet, I'm comfortable taking another step forward.

Currently, `EditEngine` has this sort of weird `setIsConduitOnly()` method. This actually means more like "this doesn't show up on forms". Make it better align with that. In particular, a "conduit only" field can already show up in the bulk editor, which is goofy. Change this to `setIsFormField()` and convert/simplify existing callsites.

Test Plan:
There are a lot of ways to reach EditEngine so this probably isn't entirely exhaustive, but I think I got pretty much anything which is likely to break:

- Searched for `setIsConduitOnly()` and `getIsConduitOnly()`, converted all callsites to `setIsFormField()`.
- Searched for `setIsLockable()`, `setIsReorderable()` and `setIsDefaultable()` and aligned these calls to intent where applicable.
- Created an Almanac binding.
- Edited an Almanac binding.
- Created an Almanac service.
- Edited an Almanac service.
- Edited a binding property.
- Deleted a binding property.
- Created and edited a badge.
- Awarded and revoked a badge.
- Created and edited an event.
- Made an event recurring.
- Created and edited a Conpherence thread.
- Edited and updated the diff for a revision.
- Created and edited a repository.
- Created and disabled repository URIs.
- Created and edited a blueprint.
- Created and edited tasks.
- Created a paste, edited/archived a paste.
- Created/edited/archived a package.
- Created/edited a project.
- Made comments.
- Moved tasks on workboards via comment action stack.
- Changed task subtype via comment action stack.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D19842
2018-11-28 13:40:40 -08:00
epriestley
1d0b99e1f8 Allow applications to require a High Security token without doing a session upgrade
Summary:
Ref T13222. See PHI873. Currently, when applications prompt users to enter MFA, their session upgrades as a side effect.

In some cases (like managing your email addresses) it makes sense to upgrade your session for a little while since it's common to make multiple edits in sequence (add a new address, make it primary, remove an old address). We generally want MFA to stay out of the way and not feel annoying.

In other cases, we don't expect multiple high-security actions in a row. Notably, PHI873 looks at more "one-shot" use cases where a prompt is answering a specific workflow. We already have at least one of these in the upstream: answering an MFA prompt when signing a Legalpad document.

Introduce a "token" workflow (in contrast to the existing "session") workflow that just does a one-shot prompt without upgrading your session statefully. Then, make Legalpad use this new workflow.

Note that this workflow has a significant problem: if the form submission is invalid for some other reason, we re-prompt you on resubmit. In Legalpad, this workflow looks like:

  - Forget to check the "I agree" checkbox.
  - Submit the form.
  - Get prompted for MFA.
  - Answer MFA prompt.
  - Get dumped back to the form with an error.
  - When you fix the error and submit again, you have to do another MFA check.

This isn't a fatal flaw in Legalpad, but would become a problem with wider adoption. I'll work on fixing this (so the MFA token sticks to the form) in the next set of changes.

Roughly, this is headed toward "MFA sticks to the form/workflow" instead of "MFA sticks to the user/session".

Test Plan:
  - Signed a legalpad document with MFA enabled.
  - Was prompted for MFA.
  - Session no longer upgraded (no purple "session in high security" badge).
  - Submitted form with error, answered MFA, fixed error, submitted form again.
    - Bad behavior: got re-prompted for MFA. In the future, MFA should stick to the form.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D19843
2018-11-28 13:39:59 -08:00
epriestley
bb369c7b71 Convert the "Repository Management" UI to a full-width, Phortune-style UI
Summary:
Ref T13216. I want to add some new management options to repositories (e.g., filesize limit, clone timeouts). Before adding new stuff here, update the UI to a full-width, Phortune-style UI.

This partially reverts D18523. About a year ago, several UIs got converted to fixed-width (repository management, config, settings, instance management in SAAS). I didn't think these were good changes and have never really gotten used to them. The rationale wasn't clear to me and these changes just felt like "be more like GitHub". I think usability is significantly worse, e.g. actions are now hidden inside button menus instead of immediately visible.

Phortune also got converted less dramatically to a full-width-with-menu UI, which I like much better. Adjust repository management to use that UI style instead of the fixed-width style.

Test Plan:
{F6020884}

Viewed every panel, including the Subversion panel.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19826
2018-11-27 05:06:07 -08:00
epriestley
2e8a5e843f Recover when cookies are disabled in Firefox and accessing localStorage throws
Summary:
Ref T13216. See PHI985. If you disable cookies in Firefox, accessing `window.localStorage` throws an exception. Currently, this pretty much kills all scripts on the page.

Instead, catch and ignore this, as though `window.localStorage` was not defined.

Test Plan:
  - Set Firefox to "no cookies".
  - Loaded any page while logged out.
  - Before: JS fatal early in the stack.
  - After: page loads and JS works.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19832
2018-11-26 16:30:42 -08:00
epriestley
88189f723f Make a Feed query construction less clever/sneaky for new qsprintf() semantics
Summary:
Ref T13216. Ref T13217. Currently, we build this query in a weird way so we end up with `(1, 2, 3)` on both 32-bit and 64-bit systems.

I can't reproduce the string-vs-int MySQL key issue on any system I have access to, so just simplify this and format as `('1', '2', '3')` instead.

The issue this is working around is that MySQL would (I think?) sometimes appear to do something goofy and miss the key if you formatted the query with strings. I never really nailed this down and could have either been mistaken about it or it could be fixed in all modern versions of MySQL. Until we have better evidence to the contrary, assume MySQL is smart enough to handle this sensibly now.

Test Plan: Ran daemons with Feed publish workers, no longer received query warnings.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13217, T13216

Differential Revision: https://secure.phabricator.com/D19837
2018-11-26 10:47:19 -08:00
epriestley
8b550ce2cd Don't allow the middle mouse button to start an inline comment
Summary:
Ref T13216. See PHI985. When you click a line number to start an inline comment, we intend to initiate the action only if you used the left mouse button (desktop) or a touch (tablet/device).

We currently have a `not right` condition for doing this, but it only excludes right clicks, not middle clicks (or other nth-button clicks). The `not right` condition was sligthly easier to write, but use an `is left` condition instead of a `not right` condition.

Test Plan:
  - In Safari, Firefox and Chrome:
    - Used left click to start an inline.
    - Used middle click to do nothing (previously: started an inline).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19836
2018-11-26 10:14:48 -08:00
epriestley
5343b1f898 Remove defunct "metamta.herald.show-hints" Config option
Summary:
Ref T13216. See PHI985. This config option once controlled adding a Herald transcript link to email. However, this was never implemented in a generic way and was removed from revisions in D8459 and from commits in D10705. No one has noticed or asked for this option for several years, so this is probably a good opportunity to simplify the software and reduce the total amount of configuration.

If we did want to pursue this in the future, I'd generally prefer to make it part of the mail detail page (`/mail/detail/12345/`) anyway.

Test Plan: Grepped for `metamta.herald.show-hints` and `addHeraldSection()`, got no hits for either.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19833
2018-11-26 10:14:25 -08:00
epriestley
9473f60a36 Allow "Abandoned" revisions to be commandeered
Summary:
Ref T13216. See PHI985. You currently can't commandeer an abandoned revision, but this workflow is perfectly fine.

The caution here is just around weird use cases where, e.g., users want to reopen a revision to add a revert to it. These workflows tend to create problems so we try to guide users away from them.

Test Plan: {F6026841}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19835
2018-11-26 10:13:52 -08:00
epriestley
bcc90d8c6b Fix an off-by-one error affecting mail rendering of inlines on the final line of a file
Summary: Depends on D19837. Ref T13216. See PHI985. There's an off-by-one error here between how inline comments store "length" and how context rendering treats "length". We need to add 1 to the length, but currently do it a little too early. Do it slightly later so that inlines on the final line of a file render properly.

Test Plan: Left an inline on the final line of a new file, saw it render properly in HTML mail.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19838
2018-11-26 10:12:09 -08:00
epriestley
97e7ef0f01 When the last rejecting reviewer resigns from a revision, return it to "Needs Review"
Summary:
Ref T13216. Fixes T12920. See PHI911. If you reject a revision and then resign from it, it stays in "Needs Revision".

There's some arguable motivation for this, but it's inconsistent with how "Accept" works (if the last accepting reviewer resigns, we kick you out of "Accepted"). Make it consistent.

Test Plan:
  - As the only reviewer: requested changes to a revision, then resigned.
  - Before: revision stays in "Needs Revision".
  - After: revision moves back to "Needs Review".

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13216, T12920

Differential Revision: https://secure.phabricator.com/D19840
2018-11-26 10:11:41 -08:00
John Linahan
433a7321ff Add harbormaster.buildable.search API Method
Summary:
This revision adds a Conduit search method for buildables. It exposes:
  * `objectPHID`
  * `containerPHID`
  * `buildableStatus`
  * `isManual`

Test Plan:
Use the API Console to run searches. Example:
```
{
  "data": [
    {
      "id": 2,
      "type": "HMBB",
      "phid": "PHID-HMBB-m4k5lodx6naq22576a7d",
      "fields": {
        "objectPHID": "PHID-DIFF-vzvgqqcyscpd7ta4osy2",
        "containerPHID": "PHID-DREV-vsivs5276c7vtgpmssn2",
        "buildableStatus": {
          "value": "passed"
        },
        "isManual": true,
        "dateCreated": 1542407155,
        "dateModified": 1542407156,
        "policy": {
          "view": "users",
          "edit": "users"
        }
      },
      "attachments": {}
    },
    {
      "id": 1,
      "type": "HMBB",
      "phid": "PHID-HMBB-opxfl4auoz3ey5klplrx",
      "fields": {
        "objectPHID": "PHID-DIFF-vzvgqqcyscpd7ta4osy2",
        "containerPHID": null,
        "buildableStatus": {
          "value": "passed"
        },
        "isManual": false,
        "dateCreated": 1542406968,
        "dateModified": 1542406968,
        "policy": {
          "view": "users",
          "edit": "users"
        }
      },
      "attachments": {}
    }
  ],
  "maps": {},
  "query": {
    "queryKey": null
  },
  "cursor": {
    "limit": 100,
    "after": null,
    "before": null,
    "order": null
  }
}
```

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, O14 ATC Monitoring

Differential Revision: https://secure.phabricator.com/D19818
2018-11-26 14:16:57 +00:00
epriestley
03f249baf3 Remove rendering support for very old Repository transactions
Summary:
Depends on D19827. Ref T13221. Ref T13216. To prepare Repositories for a move to ModularTransactions, throw away some very old transaction rendering code.

This will cause these very old transactions (none of which have been written since at least April 2016) to render "epriestley edited this repository." instead of "epriestley changed the SSH login for this repository from X to Y."

These edits were generally obsoleted by repository URIs, Passphrase credentials, and general modernization.

Test Plan: Grepped for all constants, got no hits.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13221, T13216

Differential Revision: https://secure.phabricator.com/D19828
2018-11-24 07:21:20 -08:00
epriestley
45e93c8f1d Expose "identifiers" as a query constraint for Commit search
Summary: Ref T13216. See PHI984. The CommitSearchEngine (and, by extension, `diffusion.commit.search`) currently do not support identifier search, but this is a reasonable capability to provide.

Test Plan:
Testing that a commit exists on `master`:

{F6020742}

Same commit is not on `stable`:

{F6020743}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19825
2018-11-24 07:20:01 -08:00
epriestley
43cf4edfb1 When waiting for long-running Harbormaster futures to resolve, close idle database connections
Summary:
Ref T13216. See PHI916. Harbormaster builds may be long-running, particularly if they effectively wrap `ssh ... ./run-huge-build.sh`. If we spend more than a few seconds waiting for futures to resolve, close idle database connections.

The general goal here is to reduce the held connection load for installs with a very large number of test runners.

Test Plan: Added debugging code to `phlog()` closures, saw connections closed while running builds.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19824
2018-11-21 07:53:40 -08:00
epriestley
a0d4b6da4b Support (but do not actually enable) a maximum file size limit for Git repositories
Summary:
Depends on D19816. Ref T13216. See PHI908. See PHI750. In a few cases, users have pushed multi-gigabyte files full of various things that probably shouldn't be version controlled. This tends to create various headaches.

Add support for limiting the maximum size of any object. Specifically, we:

  - list all the objects each commit touches;
  - check their size after the commit applies;
  - if it's over the limit, reject the commit.

This change doesn't actually hook the limit up (the limit is always "0", i.e. unlimited), and doesn't have Mercurial or SVN support. The actual parser bit would probably be better in some other `Query/Parser` class eventually, too. But it at least roughly works.

Test Plan:
Changed the hard-coded limit to other values, tried to push stuff, got sensible results:

```
$ echo pew >> magic_missile.txt && git commit -am pew && git push
[master 98d07af] pew
 1 file changed, 1 insertion(+)
# Push received by "local.phacility.net", forwarding to cluster host.
# Acquiring write lock for repository "spellbook"...
# Acquired write lock immediately.
# Acquiring read lock for repository "spellbook" on device "local.phacility.net"...
# Acquired read lock immediately.
# Device "local.phacility.net" is already a cluster leader and does not need to be synchronized.
# Ready to receive on cluster host "local.phacility.net".
Counting objects: 49, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (48/48), done.
Writing objects: 100% (49/49), 3.44 KiB | 1.72 MiB/s, done.
Total 49 (delta 30), reused 0 (delta 0)
remote: +---------------------------------------------------------------+
remote: |      * * * PUSH REJECTED BY EVIL DRAGON BUREAUCRATS * * *     |
remote: +---------------------------------------------------------------+
remote:              \
remote:               \                    ^    /^
remote:                \                  / \  // \
remote:                 \   |\___/|      /   \//  .\
remote:                  \  /V  V  \__  /    //  | \ \           *----*
remote:                    /     /  \/_/    //   |  \  \          \   |
remote:                    @___@`    \/_   //    |   \   \         \/\ \
remote:                   0/0/|       \/_ //     |    \    \         \  \
remote:               0/0/0/0/|        \///      |     \     \       |  |
remote:            0/0/0/0/0/_|_ /   (  //       |      \     _\     |  /
remote:         0/0/0/0/0/0/`/,_ _ _/  ) ; -.    |    _ _\.-~       /   /
remote:                     ,-}        _      *-.|.-~-.           .~    ~
remote:   *     \__/         `/\      /                 ~-. _ .-~      /
remote:    \____(Oo)            *.   }            {                   /
remote:    (    (..)           .----~-.\        \-`                 .~
remote:    //___\\  \ DENIED!  ///.----..<        \             _ -~
remote:   //     \\                ///-._ _ _ _ _ _ _{^ - - - - ~
remote:
remote:
remote: OVERSIZED FILE
remote: This repository ("spellbook") is configured with a maximum individual file size limit, but you are pushing a change ("98d07af863e799509e7c3a639404d216f9fc79c7") which causes the size of a file ("magic_missile.txt") to exceed the limit. The commit makes the file 317 bytes long, but the limit for this repository is 1 bytes.
remote:
# Released cluster write lock.
To ssh://local.phacility.com/source/spellbook.git
 ! [remote rejected] master -> master (pre-receive hook declined)
error: failed to push some refs to 'ssh://epriestley@local.phacility.com/source/spellbook.git'
```

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: joshuaspence

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19817
2018-11-20 08:04:17 -08:00
epriestley
ab14f49ef8 On the Diffusion cluster status page, improve device sort order
Summary:
Ref T13216. See PHI943. When you have a large number of cluster bindings for a repository, the UI sorting can be a bit hard to manage.

One install that regularly cycles repository cluster devices had a couple dozen older disabled bindings, with the enabled bindings intermingled.

Sort the UI:

  - enabled devices come first;
  - in each group, sort by name.

Test Plan: Mixed disabled/enabled bindings, loaded {nav Diffusion > Repository > Storage} page with clustering configured. Before: relatively unhelpful sort order. After: more intuitive sort order.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19813
2018-11-20 08:03:31 -08:00
epriestley
4967cd6ab9 Fix some "%Q" behavior in PhortuneMerchantQuery
Summary: Ref T13217. This older query does some manual joins; update it for more modern joins.

Test Plan: Ran `instances/` unit tests and got a clean result, browsed Phortune merchants.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13217

Differential Revision: https://secure.phabricator.com/D19820
2018-11-20 07:59:57 -08:00
epriestley
9481b9eff1 Allow "Can Configure Application" permissions to be configured
Summary:
Ref T13216. See PHI980. Currently, each application in {nav Applications > X > Configure} has a "Can Configure Application" permission which is hard-coded to "Administrators".

There's no technical reason for this, there just hasn't been a great use case for unlocking it. I think when I originally wrote it our protections against locking yourself out of things weren't that great (i.e., it was easier to set the policy to something that prevented you from editing it after the new policy took effect). Our protections are better now.

The major goal here is to let installs open up Custom Forms for given applications (mostly Maniphest) to more users, but the other options mostly go hand-in-hand with that.

Also, in developer mode, include stack traces for policy exceptions. This makes debugging weird stuff (like the indirect Config application errors here) easier.

Test Plan:
  - Granted "Can Configure Application" for Maniphest to all users.
  - Edited custom forms as a non-administrator.
  - Configured Maniphest as a non-administrator.
  - Installed/uninstalled Maniphest as a non-administrator.
  - Tried to lock myself out (got an error message).

{F6015721}

Reviewers: amckinley, joshuaspence

Reviewed By: joshuaspence

Subscribers: joshuaspence

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19822
2018-11-19 07:25:41 -08:00
epriestley
cb033673b6 Unify intracluster sync and Drydock working copy construction timeouts as a repository "copy time limit"
Summary:
Depends on D19814. Ref T13216. See PHI885. For various eldritch reasons, `git fetch` can hang. Although we'd probably like to fix this with `git fetch --require-sustained-network-transfer-rate=512KB/5s` or similar, that flag doesn't exist and we don't have a reasonable way to build it.

Short of that, move toward formalizing a repository "copy time limit": the longest amount of time anything may spend trying to make a copy of this repository.

This grows out of the existing intracluster sync limit, which is effectively the same thing. Here, apply it to `git clone` and `git fetch` in Drydock working copy construction, too. A future change may make it configurable.

Test Plan:
  - Set the limit to 0.001.
  - Tried to build and lease working copies, got sensible timeout errors (see D19815).

```
<Activation Failed> Lease activation failed: [CommandException] Command killed by timeout after running for more than 0.001 seconds.
COMMAND
ssh '-o' 'LogLevel=quiet' '-o' 'StrictHostKeyChecking=no' '-o' 'UserKnownHostsFile=/dev/null' '-o' 'BatchMode=yes' -l '********' -p '2222' -i '********' '127.0.0.1' -- '(cd '\''/var/drydock/workingcopy-163/repo/spellbook/'\'' && git clean -d --force && git fetch && git reset --hard)'
```

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19816
2018-11-16 13:08:12 -08:00
epriestley
933462b487 Continue cleaning up queries in the wake of changes to "%Q"
Summary: Depends on D19810. Ref T13217. Ref T13216. I mostly used `grep implode | grep OR` and `grep implode | grep AND` to find these -- not totally exhaustive but should be a big chunk of the callsites that are missing `%LO` / `%LA`.

Test Plan:
These are tricky to test exhaustively, but I made an attempt to hit most of them:

- Browsed Almanac interfaces.
- Created/browsed Calendar events.
- Enabled/disabled/showed the lock log.
- Browsed repositories.
- Loaded Facts UI.
- Poked at Multimeter.
- Used typeahead for users and projects.
- Browsed Phriction.
- Ran various fulltext searches.

Not sure these are reachable:

- All the lint stuff might be dead/unreachable/nonfunctional?

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13217, T13216

Differential Revision: https://secure.phabricator.com/D19814
2018-11-16 12:49:44 -08:00
epriestley
49483bdb48 Use "%P" to protect session key hashes in SessionEngine queries from DarkConsole
Summary:
Ref T6960. Ref T13217. Ref T13216. Depends on D19811. Use the recently-introduced "%P" conversion ("Password/Secret") to load sessions in SessionEngine.

This secret isn't critical to protect (it's the //hash// of the actual secret and not useful to attackers on its own) but it shows up on every page in DarkConsole and is an obvious case where `%P` is a more appropriate conversion.

Test Plan:
Note "*********" in the middle of the output here, instead of a session key hash:

{F6012805}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13217, T13216, T6960

Differential Revision: https://secure.phabricator.com/D19812
2018-11-16 12:36:35 -08:00
epriestley
b2e91d2205 Move the "container updated" message for Buildables that build Diffs outside of the transaction
Summary:
Ref T13216. See PHI970. Ref T13054. See some discussion in T13216.

When a Harbormaster Buildable object is first created for a Diff, it has no `containerPHID` since the revision has not yet been created.

We later (after creating a revision) send the Buildable a message telling it that we've added a container and it should re-link the container object.

Currently, we send this message in `applyExternalEffects()`, which runs inside the Differential transaction. If Harbormaster races quickly enough, it can read the `Diff` object before the transaction commits, and not see the container update.

Add a `didCommitTransaction()` callback after the transactions commit, then move the message code there instead.

Test Plan:
  - See T13216 for substantial evidence that this change is on the right track.
  - Before change: added `sleep(15)`, reproduced the issue reliably.
  - After change: unable to reproduce issue even with `sleep(15)` (the `containerPHID` always populates correctly).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13216, T13054

Differential Revision: https://secure.phabricator.com/D19807
2018-11-16 12:34:06 -08:00
epriestley
44c32839a6 When you "Request Review" of a draft revision, change the button text from "Submit Quietly" to "Publish Revision"
Summary:
See PHI975. Ref T13216. Ref T2543. Previously, see D19204 and PHI433.

When you're acting on a draft revision, we change the button text to "Submit Quietly" as a hint that your actions don't generate notifications yet.

However, this isn't accurate when one of your actions is "Request Review", which causes the revision to publish.

Allow actions to override the submit button text, and make the "Request Review" action change the button text to "Publish Revision".

The alternative change I considered was to remove the word "Quietly" in all cases.

I'm not //thrilled// about how complex this change is to adjust one word, but the various pieces are all fairly clean individually. I'm not sure we'll ever be able to use it for anything else, but I do suspect that the word "Quietly" was the change in D19204 with the largest effect by far (see T10000).

Test Plan:
  - Created a draft revision. Saw "Submit Quietly" text.
  - Added a "Request Review" action, saw it change to "Publish Revision".
  - Reloaded page, saw stack saved and "Publish Revision".
  - Removed action, saw "Submit Quietly".
  - Repeated on a non-draft revision, button stayed put as "Submit".
  - Submitted the various actions, saw them have the desired effects.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13216, T2543

Differential Revision: https://secure.phabricator.com/D19810
2018-11-15 20:50:21 -08:00
epriestley
ec452e548a Improve text overflow behavior for hovercards with (for example) long package names
Summary: See PHI977. Ref T13216. Some text, like long package names, may overflow hovercards. Add overflow CSS behaviors to remedy this.

Test Plan:
Before:

{F6012699}

After:

{F6012700}

(You can use `/search/hovercard/` to render hovercards in a handy standalone way.)

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19809
2018-11-15 20:43:10 -08:00
epriestley
533e4e13b3 Add a bin/herald test ... for doing test runs via the CLI
Summary: Ref T13216. See D19666. It's currently tricky to profile Herald test runs since you have to submit a form and repeating them is a bit of a mess. Provide a simple CLI wrapper so we can use `--xprofile`. This is also maybe nice-to-have if we're ever debugging anything here.

Test Plan: Ran `bin/herald test --object ... --type ...` and got a sensible looking transcript in the UI.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19806
2018-11-15 15:48:52 -08:00
epriestley
8a8123c9db Replace the primary "Disabled/Enabled" Herald Rule filter with "Active/Inactive", considering author status
Summary:
Ref T13216. See PHI947. In Herald, Personal rules do not run if their author's account is disabled.

This isn't communicated very clearly in the UI, and the way the SearchEngine/Query are set up isn't great.

Define "active" as "rule will actually run", which specifically means "rule is enabled, and has a valid (non-disabled) author if it needs one".

Change the meaning of the "Active" default filter from "rule is enabled" to "rule is enabled, and has a valid author if it needs one".

Refine the status badge on the view controller to show this "invalid author" state.

Tweak the language for "Disable/Enable" to be more consistent -- we currently call it "disabled" in some cases and "archived" in others.

Test Plan:
  - Disabled a user account and saw their personal rules behave properly with the new filters/options/view controller.
  - Disabled/enabled a rule, saw consistent text.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19805
2018-11-15 15:47:35 -08:00
epriestley
bbd292b9b3 Modernize the Herald rule search engine
Summary: Ref T13216. Update the Herald Rule SearchEngine and Query to use a more modern style.

Test Plan: Ran various rule queries in the UI, got sensible results

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19803
2018-11-15 15:37:00 -08:00
epriestley
ea6d2afa86 Fix flickering tooltips in Chrome when the tip container overlaps the triggering element
Summary:
Fixes T8440. See that task for discussion.

Ref T13216. See PHI976.

Test Plan:
In Chrome, hovered a timestamp and moved the mouse up to the "overlap" area (see T8440). Before: flickered like crazy. After: no flickering.

(I couldn't reproduce the original issue in modern Firefox or Safari.)

Reviewers: amckinley, avivey

Reviewed By: avivey

Maniphest Tasks: T8440, T13216

Differential Revision: https://secure.phabricator.com/D19808
2018-11-15 10:43:55 -08:00
epriestley
e57bfbf421 Pull some debugging code back out of "master"
See D19778. This is a workaround for T13179 that landed by mistake.
2018-11-15 08:19:29 -08:00
epriestley
96f9b0917e Improve performance of two recent commit migrations
Summary:
Ref T13216. See PHI959. These two recent migrations can be expressed more efficiently:

  - When updating commit audit statuses, the field isn't JSON encoded or anything so we can just issue several bulk UPDATEs.
  - When inserting mail keys, we can batch them in groups of 100.

Test Plan: Used `bin/storage upgrade -f --apply phabricator:...` to reapply patches. Saw equivalent behavior and faster runtimes.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19802
2018-11-15 03:52:06 -08:00
epriestley
86fd204148 Fix all query warnings in "arc unit --everything"
Summary:
Ref T13216. Ref T13217. Depends on D19800. This fixes all of the remaining query warnings that pop up when you run "arc unit --everything".

There's likely still quite a bit of stuff lurking around, but hopefully this covers a big set of the most common queries.

Test Plan: Ran `arc unit --everything`. Before change: lots of query warnings. After change: no query warnings.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13217, T13216

Differential Revision: https://secure.phabricator.com/D19801
2018-11-15 03:51:25 -08:00
epriestley
2f10d4adeb Continue making application fixes to Phabricator for changes to %Q semantics
Summary: Depends on D19789. Ref T13217. Continue updating things to use the new %Q-flavored conversions instead of smushing a bunch of strings together.

Test Plan: Browsed around, far fewer errors. These changes are largely mechanical in nature.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13217

Differential Revision: https://secure.phabricator.com/D19790
2018-11-15 03:50:02 -08:00
epriestley
98690ee326 Update many Phabricator queries for new %Q query semantics
Summary: Depends on D19785. Ref T13217. This converts many of the most common clause construction pathways to the new %Q / %LQ / %LO / %LA / %LJ semantics.

Test Plan: Browsed around a bunch, saw fewer warnings and no obvious behavioral errors. The transformations here are generally mechanical (although I did them by hand).

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: hach-que

Maniphest Tasks: T13217

Differential Revision: https://secure.phabricator.com/D19789
2018-11-15 03:48:10 -08:00
epriestley
64b52b9952 Make SELECT construction in PolicyAwareQuery safer
Summary: Depends on D19784. Ref T13217. Reduce uses of unsafe `%Q` in SELECT construction.

Test Plan: This reduces the number of safety warnings when loading Phabricator home from ~900 to ~800.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13217

Differential Revision: https://secure.phabricator.com/D19785
2018-11-14 15:32:09 -08:00
epriestley
e26c4bddab Replace magical "branch" behavior in "diffusion.branchquery" with an explicit "patterns"
Summary:
See PHI958. Ref T13210. Previously, see PHI720.

The use case for the magic in PHI720 involves multiple patterns, and no parameter can be passed to `branch` that will result in multiple patterns being passed to `git`.

Replace the implicit magic with an explicit `patterns` parameter.

This whole thing is a bit shaky but probably isn't hurting anything.

Test Plan:
  - Ran query with no `patterns`.
  - Ran query with invalid `patterns`, got readable error.
  - Ran query with various valid `patterns` (plain branch name, globs with "?" and "*"), got sensible results.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13210

Differential Revision: https://secure.phabricator.com/D19771
2018-11-14 14:50:53 -08:00
epriestley
da40f80741 Update PhabricatorLiskDAO::chunkSQL() for new %Q semantics
Summary:
Ref T13217. This method is slightly tricky:

  - We can't safely return a string: return an array instead.
  - It no longer makes sense to accept glue. All callers use `', '` as glue anyway, so hard-code that.

Then convert all callsites.

Test Plan: Browsed around, saw fewer "unsafe" errors in error log.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13217

Differential Revision: https://secure.phabricator.com/D19784
2018-11-13 08:59:18 -08:00
epriestley
315d857a8a Add a basic web UI for intracluster sync logs
Summary: Depends on D19798. Ref T13216. This puts at least a basic UI on top of sync logs.

Test Plan:
Viewed logs from the web UI and exported data. Note that these syncs are somewhat simulated since I my local cluster is somewhat-faked (i.e., not actually multiple machines).

{F5995899}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19799
2018-11-10 04:58:36 -08:00
epriestley
1d7c960531 Put push log "hookWait" to data export and add all wait values to UI
Summary:
Depends on D19797. Ref T13216.

  - Put the new `hookWait` in the export and the UI.
  - Put the existing waits in the UI, not just the export.
  - Make order consistent: host, write, read, hook (this is the order the timers start in).

Test Plan: Pushed some stuff, viewed web UI and saw sensible numbers, exported data and got the same values.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19798
2018-11-10 04:47:38 -08:00
epriestley
2a7ac8e388 Make "bin/repository thaw" workflow more clear when devices are disabled
Summary:
Ref T13216. See PHI943. If autoscale lightning strikes all your servers at once and destroys them, the path to recovery can be unclear. You're "supposed" to:

  - demote all the devices;
  - disable the bindings;
  - bind the new servers;
  - put whatever working copies you can scrape up back on disk;
  - promote one of the new servers.

However, the documentation is a bit misleading (it was sort of written with "you lost one or two devices" in mind, not "you lost every device") and demote-before-disable is unnecessary and slightly risky if servers come back online. There's also a missing guardrail before the promote step which lets you accidentally skip the demotion step and end up in a confusing state. Instead:

  - Add a guard rail: when you try to promote a new server, warn if inactive devices still have versions and tell the user to demote them.
  - Allow demotion of inactive devices: the order "disable, demote" is safer and more intuitive than "demote, disable" and there's no reason to require the unintuitive order.
  - Make the "cluster already has leaders" message more clear.
  - Make the documentation more clear.

Test Plan:
  - Bound a repository to two devices.
  - Wrote to A to make it a leader, then disabled it (simulating a lightning strike).
  - Tried to promote B. Got a new, useful error ("demote A first").
  - Demoted A (before: error about demoting inactive devices; now: works fine).
  - Promoted B. This worked.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19793
2018-11-10 04:46:46 -08:00
epriestley
c32fa06266 Use phutil_microseconds_since(...) to simplify some timing arithmetic
Summary: Depends on D19796. Simplify some timing code by using phutil_microseconds_since() instead of duplicate casting and arithmetic.

Test Plan: Grepped for `1000000` to find these. Pulled, pushed, made a conduit call. This isn't exhaustive but it should be hard for these to break in a bad way since they're all just diagnostic.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19797
2018-11-08 16:46:32 -08:00
epriestley
b12e92e6e2 Add timing information for commit hooks to push logs
Summary:
Depends on D19779. Ref T13216. The push logs currently record the "hostWait", which is roughly "locking + subprocess cost". We also record locking separately, so we can figure out "subprocess cost" alone by subtracting the lock costs.

However, the subprocess (normally `git receive-pack`) runs hooks, and we don't have an easy way to figure out how much time was spent doing actual `git` stuff vs spent doing commit hook processing. This would have been useful in diagnosing at least one recent issue.

Track at least a rough hook cost and record it in the push logs.

Test Plan: Pushed to a repository, saw a reasonable hook cost appear in the database table.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19780
2018-11-08 06:00:26 -08:00
epriestley
966db4d38e Add an intracluster synchronization log for cluster repositories
Summary:
Depends on D19778. Ref T13216. See PHI943, PHI889, et al.

We currently have a push log and a pull log, but do not separately log intracluster synchronization events. We've encountered several specific cases where having this kind of log would be helpful:

  - In PHI943, an install was accidentally aborting locks early. Having timing information in the sync log would let us identify this more quickly.
  - In PHI889, an install hit an issue with `MaxStartups` configuration in `sshd`. A log would let us identify when this is an issue.
  - In PHI889, I floated a "push the linux kernel + fetch timeout" theory. A sync log would let us see sync/fetch timeouts to confirm this being a problem in practice.
  - A sync log will help us assess, develop, test, and monitor intracluster routing sync changes (likely those in T13211) in the future.

Some of these events are present in the pull log already, but only if they make it as far as running a `git upload-pack` subprocess (not the case with `MaxStartups` problems) -- and they can't record end-to-end timing.

No UI yet, I'll add that in a future change.

Test Plan:
  - Forced all operations to synchronize by adding `|| true` to the version check.
  - Pulled, got a sync log in the database.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19779
2018-11-07 18:24:20 -08:00
epriestley
e09d29fb1a Clean up the workflow for some post-push logging code
Summary:
Ref T13216. When a repository is clustered, we run this cleanup code (to tell the repository to update, and log some timing information) on both nodes. Currently, we do slightly too much work, which is unnecessary and can be a bit confusing to human readers.

The double update message doesn't hurt anything, but there's no reason to write it twice.

Likewise, the second timing information update query doesn't do anything: there's no PushEvent object with the right identifier, so it just updates nothing. We don't need to run it, and it's confusing that we do.

Instead, only do these writes if we're actually the final node with the repository on it.

Test Plan: Added some logging, saw double writes/updates before the change and no doubles afterwards, with no other behavioral changes.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19778
2018-11-07 17:46:50 -08:00
epriestley
b645af981b Correct a missing parameter in "Outbound Mail" documentation
Summary: See <https://discourse.phabricator-community.org/t/documentation-error/2079>. This command is missing the configuration key.

Test Plan: Ran `bin/config set --stdin` with no other arguments, got an error about missing key. Ran with `... cluster.mailers`, got read from stdin.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19791
2018-11-07 17:43:01 -08:00
epriestley
8a4bf38655 Use 160-bit TOTP keys rather than 80-bit TOTP keys
Summary:
See <https://hackerone.com/reports/435648>. We currently use 80-bit TOTP keys. The RFC suggests 128 as a minimum and recommends 160.

The math suggests that doing the hashing for an 80-bit key is hard (slightly beyond the reach of a highly motivated state actor, today) but there's no reason not to use 160 bits instead to put this completely out of reach.

See some additional discussion on the HackerOne report about enormous key sizes, number of required observations, etc.

Test Plan: Added a new 160-bit TOTP factor to Google Authenticator without issue.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19792
2018-11-07 15:44:02 -08:00