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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
Summary:
Ref T13216. We occasionally receive HackerOne reports concerned that you can select your username as a password. I suspect very few users actually do this and that this is mostly a compliance/checklist sort of issue, not a real security issue.
On this install, we have about 41,000 user accounts. Of these, 100 have their username as a password (account or VCS). A substantial subset of these are either explicitly intentional ("demo", "bugmenot") or obvious test accounts ("test" in name, or name is a nonsensical string of gibberish, or looks like "tryphab" or similar) or just a bunch of numbers (?), or clearly a "researcher" doing this on purpose (e.g., name includes "pentest" or "xss" or similar).
So I'm not sure real users are actually very inclined to do this, and we can't really ever stop them from picking awful passwords anyway. But we //can// stop researchers from reporting that this is an issue.
Don't allow users to select passwords which contain words in a blocklist: their username, real name, email addresses, or the install's domain name. These words also aren't allowed to contain the password (that is, neither your password nor your username may be a substring of the other one). We also do a little normalization to try to split apart email addresses, domains, and real names, so I can't have "evan1234" as my password.
Test Plan:
- Added unit tests and made them pass.
- Tried to set my password to a bunch of variations of my username / email / domain name / real name / etc, got rejected.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19776
Summary:
Ref T13216. See PHI948. When you use the remarkup hint button to embed a meme with no text, you get `{meme src=X}`.
If the source is a GIF, we currently split the source apart into frame-by-frame images, process them, and stitch them back together. The end result is the same image we started with, but this process can be slow/expensive, and may timeout for sufficiently large GIFs.
Instead: when there's no text, just return the original image data.
Test Plan:
- Used `{meme src=X}` with no text, got an image faster.
- Used `{meme src=X, above=...}` to add text, got an attempt to add text (which didn't get very far locally since I don't have GD configured).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19777
Summary:
Depends on D19773. See <https://hackerone.com/reports/434116>. You can currently vote for invalid options by submitting, e.g., `vote[]=12345`.
By doing this, you can see the responses, which is sort of theoretically a security problem? This is definitely a bug, regardless.
Instead, only allow users to vote for options which are actually part of the poll.
Test Plan:
- Tried to vote for invalid options by editing the form to `vote[]=12345` (got error).
- Tried to vote for invalid options by editing the radio buttons on a plurality poll into checkboxes, checking multiple boxes, and submitting (got error).
- Voted in approval and plurality polls the right way, from the main web UI and from the embed (`{V...}`) UI.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19774
Summary:
See <https://hackerone.com/reports/434116>. Slowvote has a piece of Javascript that attempts to let you vote on `{V123}` polls inline.
It does not work: nothing ever triggers it (nothing renders a control with a `slowvote-option` sigil).
At least for now, just remove it. It has a completely separate pathway in the controller and both pathways are buggy, so this makes fixing them easier.
Test Plan: Voted in plurality and approval polls via Slowvote and the embedded widget.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19773
Summary:
Ref T13210. Ref T11908. Add some basic test coverage for the new "%R" introduced in D19764, then convert LiskDAO to implement the "Database + Table Ref" interface.
To move forward, we need to convert all of these (where `%T` is not a table alias):
```counterexample
qsprintf($conn, '... %T ...', $thing->getTableName());
```
...to these:
```
qsprintf($conn, '... %R ...', $thing);
```
The new code is a little simpler (no `->getTableName()` call) which is sort of nice. But we also have a //lot// of `%T` so this is probably going to take a while.
(I'll hold this until after the release cut.)
Test Plan:
- Ran unit tests.
- Browsed around and edited some objects without issues. This change causes a reasonably large percentage of our total queries to use the new code since the LiskDAO builtin queries are some of the most commonly-constructed queries, although there are still ~700 callsites which need to be examined for possible conversion.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210, T11908
Differential Revision: https://secure.phabricator.com/D19765
Summary:
See <https://discourse.phabricator-community.org/t/diffusionrequest-regex-error/2057/>.
The intent of `[\d-,]` is "digits, hyphen, and comma" but `[x-y]` means "character range x-y".
Specify `[\d,-]` instead to disambiguate the hyphen as "literal hyphen", not a character range marker.
Test Plan: I can't reproduce the original error as reported, but browsed around Diffusion for a bit.
Reviewers: amckinley, avivey
Reviewed By: avivey
Differential Revision: https://secure.phabricator.com/D19770
Summary:
Fixes T13208. See that task for details.
The `clone $query` line is safe if `$query` is a builtin query (like "open").
However, if it's a saved query we clone not only the query parameters but the ID, too. Then when we `save()` the query later, we overwrite the original query.
So this would happen in the database. First, you run a query and save it as the workboard default (query key "abc123"):
| 123 | abc123 | {"...xxx..."} |
Then we `clone` it and change the parameters, and `save()` it. But that causes an `UPDATE ... WHERE id = 123` and the table now looks like this:
| 123 | def456 | {"...yyy..."} |
What we want is to create a new query instead, with an `INSERT ...`:
| 123 | abc123 | {"...xxx..."} |
| 124 | def456 | {"...yyy..."} |
Test Plan:
- Followed reproduction steps from above.
- With just the new `save()` guard, hit the guard error.
- With the `newCopy()`, got a new copy of the query and "View as Query" remained functional without overwriting the original query row.
- Ran migration, saw an affected board get fixed.
Reviewers: amckinley, joshuaspence
Reviewed By: joshuaspence
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13208
Differential Revision: https://secure.phabricator.com/D19768
Summary:
Fixes T12145. Ref T13210. See PHI570. See PHI536.
Currently, when you give Drydock an Almanac host pool with more than one host, it never voluntarily builds a second host resource: there is no way to say "maximum X working copies per host" (only "maximum X global working copies") to make the first host overflow, and the allocator tries to pack resources as tightly as possible.
If you can force it to allocate the 2nd..Nth host, things will work reasonably well from there (it will spread working copies across the hosts randomly), but tricking it is very hard, especially before D19761.
To deal with this, give blueprints a new behavior around "supplemental allocations". The idea here is that a blueprint may decide that it would prefer to allocate a fresh new resource instead of allowing an otherwise valid acquisition to occur.
These supplemental allocations follow all the normal allocation rules (they can't exceed limits or actually replace existing resources), so they can only happen if there's free space in the resource pool. But a blueprint can elect for a supplemental allocation to provide a "grow the pool" hint.
The only useful policies here are probably "true" (immediately use all resources, like Almanac) or "false" (pack resources as efficiently as possible) but some other policies //might// be useful (perhaps "start growing the pool when we're getting a bit full even if we aren't at the limit yet, since our workload is bursty").
Then, give Almanac host resources a "true" policy (always allocate supplemental resources) so they use all hosts once a similar number of concurrent jobs arrive.
One aspect of this approach is that we only do supplemental resources if the normal allocation algorithm already decided that the best resource to acquire was part of the same blueprint. I started with an approach like "look at all the blueprints and see if any of them want to be greedy", but then a not-very-desirable blueprint would end up filling up its whole pool before we skipped the supplemental allocation part and ended up picking a different resource. That felt a bit silly and this feels a little cleaner and more focused.
Test Plan:
- Without changing the Almanac blueprint policy, allocated hosts. Got A, A, A, A, ... (second host never used).
- Changed the Almanac policy.
- Allocated hosts, got A, B, random mix of A and B.
- Destroyed B. Destroyed all leases on A. Allocated. Got A. This tests the "don't build a supplemental resource if there are no leases on the natural resource".
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13210, T12145
Differential Revision: https://secure.phabricator.com/D19762
Summary:
Ref T13210. Ref T12145. The "Almanac Host" blueprint currently hands out new leases on a given host even if the binding has been disabled.
Although there are some more complicated cases here (e.g., involving cleanup of the existing resource and existing leases), this one seems clear cut: if the binding has been disabled, we should stop handing out new leases on it.
Test Plan:
- Created a service with two hosts.
- Requested a lease, got host A.
- Requested more leases, always got host A (we never build a new host when we don't have to, and we currently never have to).
- Disabled the binding to host A.
- Requested a lease.
- Before patch: got host A.
- After patch: got host B.
- Also disabled the other binding to host B, requested a lease, got an indefinite wait for resources (which is expected and reasonable).
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13210, T12145
Differential Revision: https://secure.phabricator.com/D19761
Summary: Ref T13210. See PHI841. This mirrors D19509 for Differential.
Test Plan: Called `transaction.search` on a commit with a bunch of audit activity, got appropriate labels in the results.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19760
Summary:
Ref T13210. The comment action dropdown for audits has a heavy checkmark next to "Accept" and a heavy "X" next to "Raise Concern".
We previously removed similar marks in Differential in D19405 and that seems to have gone fine. For consistency, remove these too.
Test Plan: Viewed the comment action dropdown, no longer saw checkmark and X-mark.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19759
Summary:
Ref T13210. See PHI930. This translation is wrong: the parameter is a comma-separated list as a string, but the USEnglish translation provides alternatives. We can't select among alternatives based on a random string (it isn't a plurality value to let us select "chair" vs "chairs", and isn't a gender value to let us select "his profile" vs "her profile") so we get an error.
But the string itself is also misleading, since "bin/phd log --id A --id B --id C" will say "none of these are valid" if //any// of them are invalid.
Instead, just tell the user explicitly about the first problem.
Test Plan:
- Ran `bin/phd log --id` with good (got logs) and bad IDs (got sensible error).
- Ran `bin/phd log` with any logs (got logs) and (simluated) without any logs (got error).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19755
Summary:
See T13212 for some context and discussion on this being revived.
See T11694 for original context.
Add a query constraint for lease owners and implement the conduit search method for Drydock leases.
Ref T11694. Fixes T13212.
Test Plan:
- Called the API method from conduit and browsed lease queries from the UI.
- Used the new "ownerPHIDs" constraint via API console.
{F5963044}
Reviewers: yelirekim, amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam, epriestley
Maniphest Tasks: T11694, T13212
Differential Revision: https://secure.phabricator.com/D16594
Summary:
Depends on D19753. Ref T13210. This is a small optimization that saves us from waiting up to 15 seconds for a yield.
When there are no Working Copy resources and a new lease comes in, we'll allocate one and yield until it activates.
If activating it (SSH'ing and running `git clone`) takes less than 15 seconds, the resource will activate (say, at T+4) but the lease won't update again for a while (say, until T+15). This leaves us with a pointless wait (in this example, we're sitting around for 9 seconds when we could move forward).
To improve this a little bit, let resources wake up the lease update tasks that triggered allocation after they activate. In the best case, that task runs ~15 seconds sooner. In the worst case, the awaken is just a no-op.
With a more-full queue, this has a smaller effect (it's likely something else will run and be able to use the resource in those 9 seconds).
With already-activated resources, this has no effect (when resources are already activated, we can lease immediately).
Test Plan:
- Cleaned up all working copy resources.
- Requested a new "A" working copy.
- Before patch: got a working copy after 17-18 seconds, most of which was spent yielded.
- After patch: got a working copy after 3-4 seconds.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19754
Summary:
Depends on D19752. Ref T13210. If resources take a long time to reclaim/destroy (normally, more than 15 seconds) a single new lease may update several times during the reclaim/destroy process and end up reclaiming multiple resources.
Instead: after a lease triggers a reclaim, prevent it from triggering another reclaim as long as the resource it is reclaiming hasn't finished its reclaim/destroy cycle. Basically, each lease only gets to destroy one resource at a time.
Test Plan:
- Added a `sleep(120)` to `destroyResource()` to simulate a long reclaim/destroy cycle.
- Allocated A, A, A working copies. Leased a B working copy.
- Before patch: saw "B" lease destroy all three "A" working copies after ~0, ~15, and ~30 seconds, then build a new "B" resource after ~120 seconds (when the first reclaim/destroy finished).
- After patch: saw "B" lease destroy one "A" working copy after ~0 seconds, then wait patiently until it finished up, then build a new "B" resource.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19753
Summary:
Depends on D19751. Ref T13210. When Drydock needs to reclaim an existing unused resource in order to build a new resource to satisfy a lease, the lease which triggered the reclaim currently gets thrown back into the pool with a 15-second yield.
If the queue is pretty empty and the reclaim is quick, this can mean that we spend up to 15 extra seconds just waiting for the lease update task to get another shot at building a resource (the resource reclaim may complete in a second or two, but nothing else happens until the yield ends).
Instead, when a lease triggers a reclaim, have the reclaim reawaken the lease task when it completes. In the best case, this saves us 15 seconds of waiting. In other cases (the task already completed some other way, the resource gets claimed before the lease gets to it), it's harmless.
Test Plan:
- Allocated A, A, A working copies with limit 3. Leased a B working copy.
- Before patch: allocation took ~32 seconds.
- After patch: allocation takes ~17 seconds (i.e., about 15 seconds less).
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19752
Summary:
Depends on D19750. See T13210. The `bin/drydock lease` command makes it easier to request ad-hoc leases, but currently takes lease attributes in the form `--attributes x=y,a=b`.
This was okay for all leases at the time, but doesn't really work for modern WorkingCopy resources since they take a `repositories.map` which has a dictionary as a value. You can't specify that with `repositories.map=...`.
Instead, point `--attributes` at a JSON file or use `--attributes -` to read from stdin.
Test Plan: Used `--attributes` with a file and stdin to allocate working copy leases with repositories.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D19751
Summary:
Ref T13210. We currently "git reset --hard HEAD" during working copy leasing, mostly by convention/familiarity.
However, this command does not work in an empty repository, because there is no HEAD yet.
The command "git reset --hard" appears to have the same meaning and effect in all cases, except that it also works correctly in an empty repository.
The manual suggests that omitting HEAD should be the same as specifying HEAD, too:
> The <tree-ish>/<commit> defaults to HEAD in all forms.
Test Plan: Successfully leased a working copy for an empty repository using Drydock.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19750
Summary:
Ref T13210. See PHI944. When parsing certain large diffs (the case in PHI944 is an 2.5-million line JSON diff), we spend ~66% of runtime and ~80% of memory doing copy detection (the little yellow bar which shows up to give you a hint that code was moved around within a diff).
This is pretty much pointless and copy hints are almost certainly never useful on large changes. Instead, just bail if the change is larger than some arbitrary "probably too big for copy hints to ever be useful" threshold (here, 65535 lines).
Test Plan:
Roughly, ran this against a 2.5 million line JSON diff:
```
$changes = id(new ArcanistDiffParser())->parseDiff($raw_diff);
$diff = DifferentialDiff::newFromRawChanges($viewer, $changes);
```
Before the changes, it took 20s + 2.5GB RAM to parse. After the changes, it took 7s + 500MB RAM to parse.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19748
Summary:
Ref T13210. See PHI937. This function datasource isn't quite implemented correctly: it doesn't resolve `package(project)` properly, since the logic only handles users.
This blames back to D14013, where it looks like `packages(..)` was added mostly as a general nice-to-have as part of a larger modernization change.
Test Plan: Ran a `packages(project)` query in Differential, got accurate results (previously: no results).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19747
Summary:
Depends on D19734. Ref T13202. Ref T13109. Ref T10884. See PHI905. See PHI889. We currently rank cluster nodes in three cases:
# when performing a write, we can go to any node (D19734 should make our ranking good);
# when performing a read, we can go to any node (currently random, but T10884 discusses ideas to improve our ranking);
# when performing an internal synchronization before a read or a write, we must go to an up-to-date node.
Currently, case (3) is not-exactly-deterministic but not random, and we won't spread intracluster traffic acrosss the cluster evenly if, say, half of it is up to date and half of it is still synchronizing. For a given write, I believe all nodes will tend to synchronize from whichever node first received the write today.
Instead, shuffle the list and synchronize from any up-to-date node.
(I think we could improve upon this only by knowing which nodes actually have load and selecting the least-loaded -- doable, but not trivial.)
Test Plan: Poked at it locally, will deploy to `secure`. This is hard to measure/test terribly convincingly.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13202, T13109, T10884
Differential Revision: https://secure.phabricator.com/D19735
Summary:
Ref T13109. Ref T13202. See PHI905. See PHI889. When we receive a write to a repository cluster, we currently send it to a random writable node.
Instead, we can prefer:
- the node currently holding the write lock; or
- any node which is already up to date.
These should simply be better nodes to take writes in all cases. The write lock is global for the repository, so there's no scaling benefit to spreading writes across different nodes, and these particular nodes will be able to accept the write more quickly.
Test Plan:
- This is observable by using `fprintf(STDERR, "%s\n", ...)` in the logic, then running `git push`. I'd like to pull this routing logic out of `PhabricatorRepository` at some point, probably into a dedicated `ClusterURIQuery` sort of class, but that is a larger change.
- Added some `fprintf(...)` stuff around which nodes were being selected.
- Added a `sleep(10)` after grabbing the write lock.
- In one window, pushed. Then pushed in a second window.
- Saw the second window select the lock holder as the write target based on it currently holding the lock.
- Without a concurrent push, saw pushes select up-to-date nodes based on their up-to-date-ness.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: joshuaspence, timhirsh
Maniphest Tasks: T13202, T13109
Differential Revision: https://secure.phabricator.com/D19734
Summary:
See PHI920. Ref T13210. Since the HTML is just:
```
<a>View Inline</a><span>filename.txt</span>
```
..double-clicking "filename.txt" in email selects "Inlinefilename.txt".
Add a space to stop this. At least in Safari, a space between the tags is not sufficient (perhaps because the parent is a `<div>`?). I couldn't find an authoritative-seeming source on what the rules for this actually are and adding a space here fixes the issue without changing the visual rendering, so just put it here.
Test Plan:
- Made an inline.
- Used `bin/mail show-outbound --id ... --dump-html` to dump the HTML.
- Double-clicked the filename.
{F5929186}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19742
Summary:
Depends on D19743. Ref T13210. Since this command can easily dump a bunch of binary data (or just a huge long blob of nonsense) to stdout, default to requiring "--output <file>".
Using `--output -` will print to stdout.
Test Plan: Ran with: no `--output`, `--output file`, `--output -`, `--output - --overwrite`. Got sensible results or errors in all cases.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19744
Summary: Ref T13210. See PHI806. This enables basic export of revisions into flat data formats. This isn't too fancy, but just covers the basics since the driving use case isn't especially concerned about getting all the fields and details.
Test Plan: Exported some revisions into JSON, got sensible output.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19743
Summary: Depends on D19738. Ref T13210. Currently, when you use "--overwrite", we just //append// the new content. Instead, actually overwrite the file.
Test Plan: Used `--overwrite`, saw an actual clean overwrite instead of an append.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19739
Summary:
Ref T13210. Minor usability improvements to "bin/bulk export":
- Allow `--class task` to work (previously, only `--class ManiphestTaskSearchEngine` worked).
- If you run `--query jXIlzQyOYHPU`, don't require `--class`, since the query identifies the class on its own.
- Allow users to call `--query A --query B --query C` and get a union of all results.
Test Plan:
- Ran `--class task`, `--query A --query B`, `--query X` (with no `--class`), got good results.
- Ran various flavors of bad combinations (queries from different engines, invalid engines, query and class differing, ambiguous/invalid `--class` name) and got sensible errors.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19738
Summary:
Ref T13202. See PHI900. Fixes T12814. Pholio currently builds HTML comments in an older way that can dump a bunch of over-escaped HTML into mail bodies.
Update the logic to be more similar to the Differential rendering logic and stop over-escaping things.
The result isn't perfect, but is dramatically less broken.
Test Plan: {F5919860}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13202, T12814
Differential Revision: https://secure.phabricator.com/D19733
Summary:
Ref T13202. See PHI906. This is a reasonable capability which we support in some other applications already.
(The only real reason not to support this is that it creates some clutter in the UI, but I think we're generally in better shape now than we were in the past, and we could make this UI collapse/fold at some point.)
Test Plan: Ran queries with a minimum date, a maximum date, both, and neither. Saw appropriate results in all cases.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13202
Differential Revision: https://secure.phabricator.com/D19732
Summary:
Ref T13202. See <https://discourse.phabricator-community.org/t/2fa-input-box-isnt-hinted-as-a-password-so-browsers-suggest-auto-fills/1959>.
If browsers are autofilling this, I think browser behavior here is bad, but behavior is probably better on the balance if we hint this as `autocomplete="off"` and this is a minor concesssion.
Test Plan:
- I couldn't immediately get any browser to try to autofill this field (perhaps I've disabled autofill, or just not enabled it aggressively?), but this change didn't break anything.
- After the change, answered a TOTP prompt normally.
- After the change, inspected page content and saw `autocomplete="off"` on the `<input />` node.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13202
Differential Revision: https://secure.phabricator.com/D19722
Summary:
Ref T13202. See PHI903 and PHI894. When a bot leaves 100 inline comments on the same file and the revision has a 30-member recipient list, we currently highlight the file 3000 times when building mail.
Instead, engage the parse cache so we highlight it once and reuse the cache 2,999 times.
Test Plan:
- Added debugging code to stop after mail generation and show cache hits/misses.
- Left a bunch of inlines in a file.
- Ran the worker with `bin/worker execute --id ...`.
- Before change: saw 4 comments x 2 recipients = 8 cache misses
- After change: saw 8 cache hits (cache already filled from web UI rendering)
- Removed debugging code, ran worker to completion.
- Used `bin/mail show-outbound --id ... --dump-html` to inspect resulting mail, saw no functional differences.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13202
Differential Revision: https://secure.phabricator.com/D19721
Summary: Ref D18268. This typo cancelled itself out, and I can't find any other callers.
Test Plan: arc unit
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D19718
Summary:
Now on the blame page, identities get `avatar.png` and there are little tooltips that show a few characters of the committer identity string.
Also add a default icon for repo identities.
Test Plan: Loaded some blame pages for files touched by users with and without repo identities attached.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D19587
Summary:
Ref T13202. See PHI889. If the lock log is enabled, we can try to offer more details about lock holders.
When we fail to acquire a lock:
- check for recent acquisitions and suggest that this is a bottleneck issue;
- if there are no recent acquisitions, check for the last acquisition and print details about it (what process, how long ago, whether or not we believe it was released).
Test Plan:
- Enabled the lock log.
- Changed the lock wait time to 1 second.
- Added a `sleep(10)` after grabbing the lock.
- In one window, ran a Conduit call or a `git fetch`.
- In another window, ran another operation.
- Got useful/sensible errors for both ssh and web lock holders, for example:
> PhutilProxyException: Failed to acquire read lock after waiting 1 second(s). You may be able to retry later. (This lock was most recently acquired by a process (pid=12609, host=orbital-3.local, sapi=apache2handler, controller=PhabricatorConduitAPIController, method=diffusion.rawdiffquery) 3 second(s) ago. There is no record of this lock being released.)
> PhutilProxyException: Failed to acquire read lock after waiting 1 second(s). You may be able to retry later. (This lock was most recently acquired by a process (pid=65251, host=orbital-3.local, sapi=cli, argv=/Users/epriestley/dev/core/lib/phabricator/bin/ssh-exec --phabricator-ssh-device local.phacility.net --phabricator-ssh-key 2) 2 second(s) ago. There is no record of this lock being released.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13202
Differential Revision: https://secure.phabricator.com/D19702
Summary:
Ref T13202. See PHI889. Update the read and write locks to the modern parameterized verison, which handles hashing/normalization and can store better logs.
This parameterized mode was added in D19173 and has been used successfully for some time, but not all locks have switched over to it yet.
Test Plan:
- Added an `fprintf(STDERR, $full_name)` to the lock code.
- Pulled a repository.
- Saw sensible lock name on stdout before "acquired read lock...".
- Additional changes in this patch series will vet this more completely.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13202
Differential Revision: https://secure.phabricator.com/D19701
Summary:
Ref T13202. In D19660, I added comments to Phriction and tweaked some CSS.
One of these tweaks was getting rid of an extra border which was rendering under the comment area. However, I took off too much and ended up removing borders from other applications.
I think we don't actually need this `setNoBorder()` stuff after all -- a later change was sufficient to stop the actual border I was trying to get rid of from rendering. So this mostly just reverts part of D19660.
This rendering still isn't perfect, but I'm fine leaving that for another day for now.
Test Plan:
- Viewed comment areas in Phriction. Saw correct number of borders (1).
- Viewed comment areas in Maniphest. Saw correct number of borders (1).
- Grepped for extraneous removed classs, no hits.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13202
Differential Revision: https://secure.phabricator.com/D19684
Summary:
Depends on D19682. Ref T13202. We currently fatal when trying to render a timeline if:
- an install is fresh, so there are no pages yet, and you look at "/w/"; or
- you're looking at a Phriction page which doesn't exist (yet) like "/w/aadsflknadsflnf/".
Rendering a timeline and comment area doesn't make sense in these cases, so don't render them.
Test Plan: Hit both cases described above, got "new/empty page" prompts instead of fatals.
Reviewers: amckinley
Maniphest Tasks: T13202
Differential Revision: https://secure.phabricator.com/D19683
Summary:
Ref T13202. See PHI881. These stories have bad rendering methods, but they didn't previously render into the timeilne (since Phriction documents didn't have a timeline).
Update the rendering to work.
The rendered outcome isn't great (it isn't very clear or explicit about exactly what moved where), but I'll fix that in a followup. This is a net improvement since it doesn't fatal the page, at least.
Test Plan:
- Moved page "X" to "Y".
- Viewed the old page "X".
- Before patch: bad timeline story would fatal rendering.
- After patch: story renders, at least, just not great.
Reviewers: amckinley
Maniphest Tasks: T13202
Differential Revision: https://secure.phabricator.com/D19682
Summary:
Depends on D19672. Ref T13197. See PHI873. This writes a trivial log when we begin acting on a working copy and makes it look reasonable in the UI.
This is mostly just to prove that logging works properly.
Test Plan: {F5887697}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13197
Differential Revision: https://secure.phabricator.com/D19673
Summary:
Depends on D19671. Ref T13197. See PHI873.
Expose logs in the RepositoryOperation UI. Nothing writes the logs yet, so these interfaces are currently always empty.
Test Plan:
{F5887102}
{F5887103}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13197
Differential Revision: https://secure.phabricator.com/D19672
Summary: Ref T13197. See PHI873. I want to give RepositoryOperation objects access to Drydock logging like leases, resources, and blueprints currently have. This just does the schema/query changes, no actual UI or new logging yet.
Test Plan: Ran storage upgrade, poked around the UI looking for anything broken.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13197
Differential Revision: https://secure.phabricator.com/D19671
Summary: Depends on D19668. Ref T13197. See PHI840. This updates the documentation to describe how drafts work in more detail.
Test Plan: Read documentation.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13197
Differential Revision: https://secure.phabricator.com/D19669
Summary:
Ref T13077. Ref T13197. See PHI840.
- In the "History > Diff/Compare" view, the button language wasn't draft-aware.
- Revise language to avoid the word "Revert", since this can be ambiguous.
- "Edit this page, starting with an older version of the text" is now "Edit Older|Current|Draft Version X".
- "Mark this older version of the page as the current published version" is now "Publish Older Version".
- Let the user edit the current published version, too, since this is a reasonable operation if there are drafts.
Test Plan: Navigated the history diff view, saw better button and action text.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13197, T13077
Differential Revision: https://secure.phabricator.com/D19668
Summary: Depends on D19663. Ref T13077. When you edit a Phriction draft, don't publish a feed story. (The eventual "Publish" event gets a story.)
Test Plan: Made draft / non-draft / publish edits, only saw feed stories for non-draft and publish edits.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19664
Summary:
Depends on D19662. Ref T13077. See PHI840.
- If you're looking at the published version of a document, but a draft version exists and you can edit it, add a hint/link.
- Fix an issue where the "draft" transaction would complain when you created a document since the initial content is empty and no "draft" transaction is adding any content.
Test Plan: Created new documents, viewed documents with current published versions and unpublished drafts.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19663
Summary:
Depends on D19661. Ref T13077. See PHI840.
When a user edits a page normally, add a "Save as Draft" button. Much of this change is around making that button render and behave properly: it needs to be an `<input type="submit" ...>` so browsers submit it and we can figure out which button the user clicked.
Then there are a few minor rules:
- If you're editing a page which is already a draft, we only give you "Save as Draft". This makes edits to update/revise a draft more natural.
- Highlight "Publish" if it's a likely action that you might want to take.
Internally, there are two types of edits. Both types create a new version with the new content. However:
- A "content" edit sets the version shown on the live page to the newly-created version.
- A "draft" edit does not update the version shown on the live page.
Test Plan: Edited a published document, edited the draft. Published documents. Reverted documents.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19662
Summary:
Depends on D19660. Ref T5811. Ref T13077.
Long ago, if you started editing a Phriction document but didn't save it, we'd save the draft in the background as part of the preview.
D11169 updated the preview to use shared infrastructure and broke this function, since we never save drafts.
Since this doesn't work right now, I want to add another thing called "draft", and the future of this feature should be more integrated with modern drafts and EditEngine (which fixed some bugs related to versioning), just get rid of this code for the moment.
Test Plan: Edited documents. This code doesn't do anything since D11169, so no behavior changed.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13077, T5811
Differential Revision: https://secure.phabricator.com/D19661
Summary:
Depends on D19659. Fixes T1894. Ref T13077. See PHI840.
- Add an EditEngine, although it currently supports no fields.
- Add (basic, top-level-only) commenting (we already had the table in the database).
This will probably create some issues. I'm most concerned about documents accumulating a ton of old, irrelevant comments over time which are hard to keep track of and no longer relevant. But I think this is probably a step forward in almost all cases, and a good thing on the balance.
This also moves us incrementally toward putting all editing on top of EditEngine.
Test Plan: {F5877347}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13077, T1894
Differential Revision: https://secure.phabricator.com/D19660
Summary:
Ref T13197. See PHI873. Record when a user has MFA'd and add a little icon to the transaction, similar to the exiting "Silent" icon.
For now, this just makes this stuff more auditable. Future changes may add ways to require MFA for certain specific transactions, outside of the ones that already always require MFA (like revealing credentials).
Test Plan: {F5877960}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13197
Differential Revision: https://secure.phabricator.com/D19665
Summary:
Depends on D19657. Ref T13197. See PHI841.
This enriches the results from `diffusion.commit.search` with information similar to the information returned by the "commits" attachment from `differential.diff.search`.
Also include unreachable, imported, message, audit status, and repository PHID.
Test Plan: Called `diffusion.commit.search` and reviewed the results, which looked sensible.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13197
Differential Revision: https://secure.phabricator.com/D19658
Summary:
Depends on D19656. Ref T13197. See PHI851.
- This class is now a real object, so get rid of the "Constants" part of the name.
- Rename it for greater consistency with other modern objects.
- Get rid of the `MODERN_` tag now that the old constants are gone.
Test Plan: Bunch of `grep`, browsed around.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13197
Differential Revision: https://secure.phabricator.com/D19657
Summary: Depends on D19655. Ref T13197. These no longer have callers.
Test Plan: Grepped for these constants.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13197
Differential Revision: https://secure.phabricator.com/D19656
Summary: Depends on D19652. Ref T13197. See PHI851. This migrates the actual `auditStatus` on Commits, and older status transactions.
Test Plan:
- Ran migrations.
- Spot-checked the database for sanity.
- Ran some different queries, got unchanged results from before migration.
- Reviewed historic audit state transactions, and accepted/raised concern on new audits. All state transactions appeared to generate properly.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13197
Differential Revision: https://secure.phabricator.com/D19655
Summary:
Ref T13077. See PHI840. Ref T1894. I'm planning to just let you comment on Phriction documents. I think this will create a few problems (e.g., around popular documents which collect long comment threads that are eventually obsolete) but nothing should be too terribly critical (e.g., we handle it gracefully when objects have very large number of comments/transactions) and for most documents this is likely just a net improvement.
"Just enable comments" is probably not the final iteration on this, but I think it's probably a step forward on the balance, not a step sideways or a slippery slope down into a dark hole or anything.
Test Plan: {F5877316}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13077, T1894
Differential Revision: https://secure.phabricator.com/D19659
Summary: See PHI871. Ref T13197. These sections are only divided visually and don't have textual headers. Add aural headers.
Test Plan: {F5875471}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13197
Differential Revision: https://secure.phabricator.com/D19654
Summary:
Depends on D19650. Ref T13197. Allow `SearchCheckboxesField` to have a "deprecated" map of older aliases, then convert them to modern values.
On the API method page, show all the values.
This technically resolves the issue in PHI841, although I still plan to migrate behind this.
Test Plan:
{F5875363}
- Queried audits, fiddled with `?status=1,audited`, etc.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13197
Differential Revision: https://secure.phabricator.com/D19651
Summary: Ref T13197. We're almost ready to migrate: let the Query accept either older integer values or new string values. Then move some callsites to use strings.
Test Plan: Called `audit.query`, browsed audits, audited commits.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13197
Differential Revision: https://secure.phabricator.com/D19650
Summary: Ref T13195. See PHI851. Continuing down the path toward replacing these legacy numeric constants with more modern string constants.
Test Plan:
- Raised concern, requested verification, verified.
- Looked at commit hovercard with audit status.
- Viewed header on a commit page.
- (Didn't test the Doorkeeper stuff since it requires linking to Asana and seems unlikely to break.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13195
Differential Revision: https://secure.phabricator.com/D19647
Summary:
Ref T13195. If a Phriction page begins with a code block, the `clear: both;` currently makes it clear the action list.
Instead, use table-cell layout on desktops.
Test Plan: Viewed a Phriction page with an initial code block on desktop/tablet/mobile/printable layouts. Now got more sensible layouts in all cases.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: GoogleLegacy
Maniphest Tasks: T13195
Differential Revision: https://secure.phabricator.com/D19649
Summary:
Ref T13195. Fixes T8573. When you're adding inlines to your own stuff, mark them "Done" by default. You can unmark them as "Done" if you're legitimately leaving TODOs for yourself, although I think this is unusual.
(If this turns out to be less unusual than I think, we could consider an alternate rule: mark replies by the author as "Done" by default.)
Test Plan: Added some inlines as an author and a non-author. Saw my author inlines marked as "Done" by default. Submitted them; unmarked and submittted them.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13195, T8573
Differential Revision: https://secure.phabricator.com/D19635
Summary:
Ref T13195. Ref T8573. This allows reviewers to mark their own inline comments as "Done" before they submit them.
If you're leaving a non-actionable comment like "this is good", you can pre-check "Done" to give the author a hint that you don't expect any response.
Test Plan: On revisions and commits, added inlines as the author and a reviewer/auditor. Marked them done/not-done before submitting. As author, marked the not-done ones done after submitting. Checked preivews, toggled done/not done states.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13195, T8573
Differential Revision: https://secure.phabricator.com/D19634
Summary:
Ref T13195. See PHI861. The "+ Draft" flag is not currently exposed over the API, but seems stable enough to expose.
Also expose the "hold as draft" flag, normally `arc diff --draft`.
Today, you can get "+ Draft" with some other state by:
- abandoning a draft revision ("Abandoned + Draft"); or
- using `arc diff --plan-changes` with an older `arc` version ("Changes Planned + Draft").
Test Plan: Called `differential.revision.search` via Conduit and got sensible-looking results for revisions in various states.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13195
Differential Revision: https://secure.phabricator.com/D19648
Summary:
Ref T13195. See PHI851. Add an object, analogous to the `DifferentialRevisionStatus` object, to handle audit status management.
This will primarily make it easier to swap storage over to strings later, but also cleans things up a bit.
Test Plan: Viewed audit/commit lists, saw sensible state icons. Ran `bin/audit synchronize`, got sensible output.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13195
Differential Revision: https://secure.phabricator.com/D19646
Summary:
See PHI859. Ref T13195. The regexp for replacing variables currently does not include underscores.
Include underscores.
I also made a note in T13088: we should (almost certainly?) throw immediately if you try to pass a bogus variable name as a custom parameter, but this is a slightly larger change.
Test Plan:
- Made an "http request" build plan with `?x=${initiator.phid}&y={$some_variable}`.
- Added `some_variable` as a parameter to the parameter collection.
- Before patch: `initiator.phid` was replaced, but `some_variable` was not.
- After patch: both variables are replaced.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13195
Differential Revision: https://secure.phabricator.com/D19645
Summary: See D19632. Agreed that this is more clear.
Test Plan: Read carefully.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19644
Summary:
Ref T13195. See PHI851. Audits currently have older integer status constants. We've moved almost all object types away from this to string constants (which are better in basically every way, and particularly way better for exposing over the API).
Commits/audits are currently accessible over the API and expose these constants via a "statuses" constraint.
Prepare to move toward modern string constants by defining a new, more modern map of status details and defining the existing methods in terms of it.
Test Plan: Browsed audits checking for icons/names/open-ness, saw no changes. This change should have no user-visible effects, as it just reorganizes code.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13195
Differential Revision: https://secure.phabricator.com/D19642
Summary:
Ref T13195. Ref PHI851. Currently, checkbox constraints don't tell you what values are supported on the API page.
You can figure this out with "Inspect Element" or by viewing the source code, but just render a nice table instead.
Test Plan: {F5862969}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13195
Differential Revision: https://secure.phabricator.com/D19641
Summary:
Ref T13195. See PHI851. Start by making some minor improvements here:
- Clarify that the example of what constraints look like is just an example.
- Add descriptions for parameters missing descriptions.
Test Plan: Looked at API method page, saw more helpful/complete instructions.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13195
Differential Revision: https://secure.phabricator.com/D19640
Summary: Ref T13195. See PHI845. For custom OperationTypes, provide access to the Interface and Operation via getters. This is just for convenience, since passing these around everywhere can be a bit of a pain if you have a deep-ish stack of things or love using callbacks or whatever.
Test Plan: Landed a revision via upstream Drydock operations.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13195
Differential Revision: https://secure.phabricator.com/D19636
Summary:
Ref T13195. An install had a user apply a transaction which added about 1,000 inline comments. Rendering the email for this transaction took a very long time because the context section for each comment must be highlighted separately.
We can make the highlighting faster (in this case, by porting the lexer to PHP) but it's also sort of silly to include more than 100 inlines in an email. These emails are likely to be truncated by outbound size rules at some point anyway. Instead, limit inlines rendered directly into email to the first 100 per transaction group.
Test Plan:
Set limit to 2, added 4 comments, viewed text and HTML emails:
{F5859967}
{F5859968}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13195
Differential Revision: https://secure.phabricator.com/D19632
Summary:
Ref T13195. Ref T8573. The inline comment controllers currently use outdated `$user = $this->getRequest()->getUser()` calls.
Instead, use `$viewer = $this->getViewer()`.
This is just a small consistency update with no behavioral changes.
Test Plan: Viewed and added inlines in Differential and Diffusion.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13195, T8573
Differential Revision: https://secure.phabricator.com/D19633
Summary: Ref T13077. This is currently a little too confusing to go out into the world, mostly because there's no way to edit documents without auto-publishing them. Keep it out of the spotlight for this release.
Test Plan: Viewed Phriction, saw publish operation marked as a prototype.
Reviewers: amckinley
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19627
Summary: Ref T13077. Updates the "History" view to be slightly better organized and draft-aware.
Test Plan: Viewed page history in Phriction.
Reviewers: amckinley
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19626
Summary:
Ref T13077. We need to know the maximum version of a document in several cases, so denormalize it onto the Document object.
Then clean up some behaviors where we edit a document with, e.g., 7 versions but version 5 is currently published. For now, we: edit starting with version 7, save as version 8, and immediately publish the new version.
Test Plan:
- Ran migration.
- Edited a draft page without hitting any weird version errors.
- Checked database for sensible `maxVersion` values.
Reviewers: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19625
Summary:
See T13193. See T13077. If we drop a column which is part of a UNIQUE KEY, MariaDB raises an error.
This is probably a bad idea on our side anyway, but in this case it wasn't an obviously bad idea.
To get around this:
- Drop the unique key, if it exists, before dropping the column.
- Explicitly add the new unique key afterward.
Test Plan: Ran `bin/storage upgrade` locally without issue, but I'm on MySQL. Will follow up on T13193.
Reviewers: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D19624
Summary:
See PHI844. Ref T13189.
Add "Revision test plan" as an available field for Herald. This is a little niche -- and a little odd because it sticks around even if you fully disable test plans -- but probably broadly reasonable.
The existing "Revision summary" field counterintuitively included the test plan. Separate this out since it's now a separate field and the behavior was weird historic nonsense. I'll note this in the changelog.
Test Plan: Wrote a rule using both fields, verified they generated the expected values.
Reviewers: amckinley
Maniphest Tasks: T13189
Differential Revision: https://secure.phabricator.com/D19623
Summary: Depends on D19621. Ref T13077. Fixes T4815. This adds previous/current/next/draft buttons and makes navigation between unpublished and published versions of a document more clear.
Test Plan: {F5841997}
Reviewers: amckinley
Maniphest Tasks: T13077, T4815
Differential Revision: https://secure.phabricator.com/D19622
Summary:
Depends on D19620. Ref T13077. This adds a "Publish" operation which points the current version at some historical version of the document -- not necessarily the most recent version. Newer versions become "drafts".
This is still quite rough and missing a lot of hinting in the UI, I'm just making it work so I can start making the UI understand it.
Test Plan: Used the "Publish" action to publish older versions of a document, saw the document revert. Many UI hints are missing and this operation is puzzling and not yet usable for normal users.
Reviewers: amckinley
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19621
Summary: Depends on D19619. Ref T13065. Ref T13077. Migrate Phriction mail keys to the new infrastructure and drop the column.
Test Plan: Ran migrations, spot-checked the database.
Reviewers: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13077, T13065
Differential Revision: https://secure.phabricator.com/D19620
Summary:
Ref T13077. This is mostly just a small cleanup change, even though the actual change is large.
We currently reference content and document objects from one another with `contentID` and `documentID`, but this means that `contentID` must be nullable. Switching to PHIDs allows the column to be non-nullable.
This also supports reorienting some current and future transactions around PHIDs, which is preferable for the API. In particular, I'm adding a "publish version X" transaction soon, and would rather callers pass a PHID than an ID or version number, since this will make the API more consistent and powerful.
Today, `contentID` gets used as a cheaty way to order documents by (content) edit time. Since PHIDs aren't orderable and stuff is going to become actually-revertible soon, replace this with an epoch timestamp.
Test Plan:
- Created, edited, moved, retitled, and deleted Phriction documents.
- Grepped for `documentID` and `contentID`.
- This probably breaks //something// but I'll be in this code for a bit and am likely to catch whatever breaks.
Reviewers: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19619
Summary:
Ref T13077. We currently have these weird policy hints in Phriction that we don't use in other applications. Just remove them for consistency to make the eventual swap to EditEngine a little easier.
Also nuke some unreacahble code.
Test Plan: Loaded edit page, saw more standard UI.
Reviewers: amckinley
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19618
Summary:
Depends on D19616. Ref T13077. Fixes T8172. In the last round of design updates, a lot of actions got stuffed into "Actions" menus.
I never really got used to these and think they're a net usability loss, and broadly agree with the feedback in T8172. I'd generally like to move back toward a state where actions are available on the page, not hidden in a menu.
For now, just put a curtain view on these pages. This could be refined later (e.g., stick this menu to the right hand side of the screen) depending on where other Phriction changes go.
(Broadly, I'm also not satisfied with where we ended up on the fixed-width pages like Diffusion > Manage, Config, and Instances. In contrast, I //do// like where we ended up with Phortune in terms of overall design. I anticipate revisiting some of this stuff eventually.)
Test Plan:
- Looked at Phriction pages on desktop/tablet/mobile/printable -- actions are now available on the page.
- Looked at other DocumentView pages (like Phame blogs) -- no changes for now.
Reviewers: amckinley
Maniphest Tasks: T13077, T8172
Differential Revision: https://secure.phabricator.com/D19617
Summary: Ref T13077. There is no "PHUIDocumentView" so toss the "Pro" suffix from this classname.
Test Plan: Grepped for `PHUIDocumentView` and `PHUIDocumentViewPro`.
Reviewers: amckinley
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19616
Summary: Ref T13189. See PHI710. Ref T13088. Fixes T9951. Allow callers to `harbormaster.sendmessage` to specify that the test details are remarkup so they can use rich formatting and include links, files, etc.
Test Plan: {F5840098}
Reviewers: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13189, T13088, T9951
Differential Revision: https://secure.phabricator.com/D19615
Summary:
Fixes T12251. Ref T13189. See PHI610. The difficulty here is that we don't want to disclose Phabricator account information to Buildkite. We're comfortable disclosing information from `git`, etc.
- For commits, use the Identity to provide authorship information from Git.
- For revisions, use the local commit information on the Diff to provide the Git/Mercurial/etc author of the HEAD commit.
Test Plan:
- Built commits and revisions in Buildkite via Harbormaster.
- I can't actually figure out how to see author information on the Buildkite side, but the values look sane when dumped locally.
Reviewers: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13189, T12251
Differential Revision: https://secure.phabricator.com/D19614
Summary: Ref T13189. See PHI690. When a lease is first acquired or activated, note the time. This supports better visibility into queue lengths. For now, this is only queryable via DB and visible in the UI, but can be more broadly exposed in the future.
Test Plan: Landed a revision, saw the leases get sensible timestamps for acquisition/activation.
Reviewers: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13189
Differential Revision: https://secure.phabricator.com/D19613
Summary: Ref T13189. Summaries do not appear to be meaningfully rendered with Remarkup so just drop the engine. See D19610 for the previous change in this vein.
Test Plan: Viewed config list with option summaries.
Reviewers: amckinley
Maniphest Tasks: T13189
Differential Revision: https://secure.phabricator.com/D19612
Summary:
Depends on D19609. Ref T13189. At some point, we switched from RemarkupEngine to RemarkupView and lost this piece of hack-magic.
Restore the hack-magic. It's still hack-magic instead of a real rule, but things are at least cleaner than they were before.
Test Plan: Viewed `auth.require-approval`, etc. Saw references to other config options linked properly.
Reviewers: amckinley
Maniphest Tasks: T13189
Differential Revision: https://secure.phabricator.com/D19610
Summary: Ref T13189. When there are no setup issues, we currently double-render a weird setup issues box underneath the notice. Get rid of it.
Test Plan: Viewed page with and without setup issues, saw less awkward UI.
Reviewers: amckinley
Maniphest Tasks: T13189
Differential Revision: https://secure.phabricator.com/D19609
Summary:
Depends on D19607. Ref T13189. See PHI642. Ref T13186.
Some transactions can sometimes be applied to objects you can not edit. Currently, using `*.edit` to edit an object always explicitly requires CAN_EDIT.
Now that individual transactions require CAN_EDIT by default and can reduce or replace this requirement, stop requiring CAN_EDIT to reach the editor.
The only expected effect of this change is that low-permission edits (like disabling a user, leaving a project, or leaving a thread) can now work via `*.edit`.
Test Plan:
- Tried to perform a normal edit (changing a task title) against an object with no CAN_EDIT. Still got a permissions error.
- As a non-admin, disabled other users while holding the "Can Disable Users" permission.
- As a non-admin, got a permissions error while trying to disable other users while not holding the "Can Disable Users" permission.
Reviewers: amckinley
Maniphest Tasks: T13189, T13186
Differential Revision: https://secure.phabricator.com/D19608
Summary:
Depends on D19606. Ref T13189. See PHI642.
- Disabling/enabling users no longer requires admin. Now, you just need "Can Disable Users".
- Update the UI to appropriately show the action in black or grey depending on what clicking the button will do.
- For "Approve/Disapprove", fix a couple bugs, then let them go through without respect for "Can Disable Users". This is conceptually a different action, even though it ultimately sets the "Disabled" flag.
Test Plan:
- Disabled/enabled users from the web UI as various users, including a non-administrator with "Can Disable Users".
- Hit permissions errors from the web UI as various users, including an administrator without "Can Disable Users".
- Saw the "Disable/Enable" action activate properly based on whether clicking the button would actually work.
- Disapproved a user without "Can Disable Users" permission, tried to re-disapprove a user.
- Approved a user, tried to reapprove a user.
Reviewers: amckinley
Maniphest Tasks: T13189
Differential Revision: https://secure.phabricator.com/D19607
Summary:
Depends on D19605. Ref T13189. See PHI642. This adds a separate "Can Disable Users" capability, and makes the underlying transaction use it.
This doesn't actually let you weaken the permission, since all pathways need more permissions:
- `user.edit` needs CAN_EDIT.
- `user.disable/enable` need admin.
- Web UI workflow needs admin.
Upcoming changes will update these pathways.
Without additional changes, this does let you //strengthen// the permission.
This also fixes the inability to disable non-bot users via the web UI.
Test Plan:
- Set permission to "No One", tried to disable users. Got a tailored policy error.
- Set permission to "All Users", disabled/enabled a non-bot user.
Reviewers: amckinley
Maniphest Tasks: T13189
Differential Revision: https://secure.phabricator.com/D19606
Summary:
Depends on D19604. Ref T13189. See PHI642. Deprecates these in favor of "user.edit", redefines them in terms of it, and removes the old `disableUser()` method.
I kept the "is admin" permissions check for consistency, since these methods have always said "(admin only)". This check may not be the most tailored check soon, but we can just keep executing it in addition to the real check.
For now, this change stops this method from actually disabling non-bot users (since it implicitly adds a CAN_EDIT requirement, and even administrators don't have that). An upcoming change will fix that.
Test Plan: Enabled and disabled a (bot) user via these methods. Checked API UI, saw them marked as "disabled".
Reviewers: amckinley
Maniphest Tasks: T13189
Differential Revision: https://secure.phabricator.com/D19605
Summary:
Ref T13189. See PHI642. Upgrades the "Disable" action in the web UI to be transaction-based.
This technically breaks things a little (you can't disable non-bot users, since they now require CAN_EDIT and you won't have it) but an upcoming change will fix the permissions issue.
Test Plan: Disabled and enabled a (bot) user from the web UI.
Reviewers: amckinley
Maniphest Tasks: T13189
Differential Revision: https://secure.phabricator.com/D19604
Summary:
Depends on D19585. Ref T13164.
Almost all transactions require CAN_EDIT on the object, but they generally do not enforce this directly today. Instead, this is effectively enforced by Controllers, API methods, and EditEngine doing a `CAN_EDIT` check when loading the object to be edited.
A small number of transactions do not require CAN_EDIT, and instead require only a weaker/lesser permission. These are:
- Joining a project which you have CAN_JOIN on.
- Leaving a project which isn't locked.
- Joining a Conpherence thread you can see (today, no separate CAN_JOIN permission for Conpherence).
- Leaving a Conpherence thread.
- Unsubscribing.
- Using the special `!history` command from email.
Additionally, these require CAN_INTERACT, which is weaker than CAN_EDIT:
- Adding comments.
- Subscribing.
- Awarding tokens.
Soon, I want to add "disabling users" to this list, so that you can disable users if you have "Can Disable User" permission, even if you can not otherwise edit users.
It's possible this list isn't exhaustive, so this change might break something by adding a policy check to a place where we previously didn't have one. If so, we can go weaken that policy check to the appropriate level.
Enforcement of these special cases is currently weird:
- We mostly don't actually enforce CAN_EDIT in the Editor; instead, it's enforced before you get to the editor (in EditEngine/Controllers).
- To apply a weaker requirement (like leaving comments or leaving a project), we let you get through the Controller without CAN_EDIT, then apply the weaker policy check in the Editor.
- Some transactions apply a confusing/redundant explicit CAN_EDIT policy check. These mostly got cleaned up in previous changes.
Instead, the new world order is:
- Every transaction has capability/policy requirements.
- The default is CAN_EDIT, but transactions can weaken this explicitly they want.
- So now we'll get requirements right in the Editor, even if Controllers or API endpoints make a mistake.
- And you don't have to copy/paste a bunch of code to say "yes, every transaction should require CAN_EDIT".
Test Plan:
- Tried to add members to a Conpherence thread I could not edit (permissions error).
- Left a Conpherence thread I could not edit (worked properly).
- Joined a thread I could see but could not edit (worked properly).
- Tried to join a thread I could not see (permissions error).
- Implemented `requireCapabilites()` on ManiphestTransactionEditor and tried to edit a task (upgrade guidance error).
- Mentioned an object I can not edit on another object (works).
- Mentioned another object on an object I can not edit (works).
- Added a `{F...}` reference to an object I can not edit (works).
- Awarded tokens to an object I can not edit (works).
- Subscribed/unsubscribed from an object I can not edit (works).
- Muted/unmuted an object I can not edit (works).
- Tried to do other types of edits to an object I can not edit (correctly results in a permissions error).
- Joined and left a project I can not edit (works).
- Tried to edit and add members to a project I can not edit (permissions error).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19586
Summary:
Ref T13187. See PHI811. If the file tree is enabled and visible, set the default tab to "History".
- This is a bit magic.
- It won't work entirely on mobile (we can't tell that you're on mobile on the server, so we'll pick the "History" tab even though the file tree isn't actually visible on your device).
- There's no corresponding logic in Diffusion. Diffusion doesn't have the same tab layout, but this makes things somewhat inconsistent.
So I don't love this, but we can try it and see if it's confusing or helpful on the balance.
Test Plan: With filetree on and off, reloaded revisions. Saw appropriate tab selected by default.
Reviewers: amckinley
Maniphest Tasks: T13187
Differential Revision: https://secure.phabricator.com/D19601
Summary: Ref T13187. Ref T13176. With drafts, we actually want to suppress this link on "first broadcast" (the first time we send mail), not on "new object" (when the revision is created as a draft).
Test Plan: Poked at this locally, will keep an eye on it in production.
Reviewers: amckinley
Maniphest Tasks: T13187, T13176
Differential Revision: https://secure.phabricator.com/D19598
Summary: Ref T13187. See PHI807. The documentation currently does not make it very clear that this is a local setting, per `phd` process. Make it more clear.
Test Plan: {F5827757}
Reviewers: amckinley
Maniphest Tasks: T13187
Differential Revision: https://secure.phabricator.com/D19597
Summary:
Ref T13187. See PHI834. Mercurial has somewhat-recently (changeset is from Jan 2018) introduced a new "protocaps" command, that appears in Mercurial 4.7 and possibly before then.
We must explicitly enumerate all protocol commands because you can't decode the protocol without knowing how many arguments the command expects, so enumerate it.
(Also fix an issue where the related error message had an extra apostrophe.)
Test Plan:
- Ran `hg clone ...` with client and server on Mercurial 4.7.
- Before: fatal on unknown "protocaps" command.
- Midway: better typography in error message.
- After: clean clone.
Reviewers: amckinley
Maniphest Tasks: T13187
Differential Revision: https://secure.phabricator.com/D19596
Summary:
Depends on D19594. See PHI823. Ref T13164.
- Add a label for the "X" button in comment areas, like "Remove Action: Change Subscribers".
- Add a label for the floating header display options menu in Differential.
- Add `role="button"` to `PHUIButtonView` objects that we render with an `<a ...>` tag.
Test Plan:
Viewed a revision with `?__aural__=true`:
- Saw "Remove Action: ..." label.
- Saw "Display Options" label.
- Used inspector to verify that some `<a class="button" ...>` now have `<a class="button" role="button" ...>`. This isn't exhaustive, but at least improves things. A specific example is the "edit", "reply", etc., actions on inline comments.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19595
