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

15625 commits

Author SHA1 Message Date
epriestley
cab2bba6f2 Remove "passwordHash" and "passwordSalt" from User objects
Summary:
Ref T13043. After D18903, this data has migrated to shared infrastructure and has no remaining readers or writers.

Just delete it now, since the cost of a mistake here is very small (users need to "Forgot Password?" and pick a new password).

Test Plan: Grepped for `passwordHash`, `passwordSalt`, and variations.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13043

Differential Revision: https://secure.phabricator.com/D18904
2018-01-23 13:44:26 -08:00
epriestley
abc030fa00 Move account passwords to shared infrastructure
Summary:
Ref T13043. This moves user account passwords to the new shared infrastructure.

There's a lot of code changes here, but essentially all of it is the same as the VCS password logic in D18898.

Test Plan:
- Ran migration.
- Spot checked table for general sanity.
- Logged in with an existing password.
- Hit all error conditions on "change password", "set password", "register new account" flows.
- Verified that changing password logs out other sessions.
- Verified that revoked passwords of a different type can't be selected.
- Changed passwords a bunch.
- Verified that salt regenerates properly after password change.
- Tried to login with the wrong password, which didn't work.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13043

Differential Revision: https://secure.phabricator.com/D18903
2018-01-23 13:43:07 -08:00
epriestley
6b99aac49d Digest changeset anchors into purely alphanumeric strings
Summary:
Ref T13045. See that task for discussion.

This replaces `digestForIndex()` with a "clever" algorithm in `digestForAnchor()`. The new digest is the same as `digestForIndex()` except when the original output was "." or "_". In those cases, a replacement character is selected based on entropy accumulated by the digest function as it iterates through the string.

Test Plan: Added unit tests.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13045

Differential Revision: https://secure.phabricator.com/D18909
2018-01-23 13:42:08 -08:00
epriestley
b8a515cb29 Bring new password validation into AuthPasswordEngine
Summary:
Ref T13043. We have ~4 copies of this logic (registration, lost password recovery, set password, set VCS password).

Currently it varies a bit from case to case, but since it's all going to be basically identical once account passwords swap to the new infrastructure, bring it into the Engine so it can live in one place.

This also fixes VCS passwords not being affected by `account.minimum-password-length`.

Test Plan: Hit all errors in "VCS Password" panel. Successfully changed password.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13043

Differential Revision: https://secure.phabricator.com/D18902
2018-01-23 10:58:37 -08:00
epriestley
aa3b582c7b Remove "set password" from bin/accountadmin and let bin/auth recover recover anyone
Summary:
Ref T13043. This cleans some things up to prepare for moving account passwords to shared infrastructure.

Currently, the (very old, fairly unusual) `bin/accountadmin` tool can set account passwords. This is a bit weird, generally not great, and makes upgrading to shared infrastructure more difficult. Just get rid of this to simplify things. Many installs don't have passwords and this is pointless and unhelpful in those cases.

Instead, let `bin/auth recover` recover any account, not just administrator accounts. This was a guardrail against administrative abuse, but it has always seemed especially flimsy (since anyone who can run the tool can easily comment out the checks) and I use this tool in cluster support with some frequency, occasionally just commenting out the checks. This is generally a better solution than actually setting a password on accounts anyway. Just get rid of the check and give users enough rope to shoot themselves in the foot with if they truly desire.

Test Plan:
  - Ran `bin/accountadmin`, didn't get prompted to swap passwords anymore.
  - Ran `bin/auth recover` to recover a non-admin account.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13043

Differential Revision: https://secure.phabricator.com/D18901
2018-01-23 10:58:11 -08:00
epriestley
5a8a56f414 Prepare the new AuthPassword infrastructure for storing account passwords
Summary:
Ref T13043. In D18898 I moved VCS passwords to the new shared infrastructure.

Before account passwords can move, we need to make two changes:

  - For legacy reasons, VCS passwords and Account passwords have different "digest" algorithms. Both are more complicated than they should be, but we can't easily fix it without breaking existing passwords. Add a `PasswordHashInterface` so that objects which can have passwords hashes can implement custom digest logic for each password type.
  - Account passwords have a dedicated external salt (`PhabricatorUser->passwordSalt`). This is a generally reasonable thing to support (since not all hashers are self-salting) and we need to keep it around so existing passwords still work. Add salt support to `AuthPassword` and make it generate/regenerate when passwords are updated.

Then add a nice story about password digestion.

Test Plan: Ran migrations. Used an existing VCS password; changed VCS password. Tried to use a revoked password. Unit tests still pass. Grepped for callers to legacy `PhabricatorHash::digestPassword()`, found none.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13043

Differential Revision: https://secure.phabricator.com/D18900
2018-01-23 10:57:40 -08:00
epriestley
753c4c5ff1 Remove the "PhabricatorRepositoryVCSPassword" class and table
Summary:
Ref T13043. After D18898, this has been migrated to new, more modern storage and no longer has any readers or writers.

One migration from long ago (early 2014) is affected. Since this is ancient and the cost of dropping this is small (see inline), I just dropped it.

I'll note this in the changelog.

Test Plan: Ran migrations, got a clean bill of health from `storage status`. Grepped for removed symbol.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13043

Differential Revision: https://secure.phabricator.com/D18899
2018-01-23 10:56:37 -08:00
epriestley
dd8f588ac5 Migrate VCS passwords to new shared password infrastructure
Summary:
Ref T13043. Migrate VCS passwords away from their dedicated table to new the new shared infrastructure.

Future changes will migrate account passwords and remove the old table.

Test Plan:
- Ran migrations.
  - Cloned with the same password that was configured before the migrations (worked).
  - Cloned with a different, invalid password (failed).
- Changed password.
  - Cloned with old password (failed).
  - Cloned with new password (worked).
- Deleted password in web UI.
  - Cloned with old password (failed).
- Set password to the same password as it currently is set to (worked, no "unique" collision).
- Set password to account password. !!This (incorrectly) works for now until account passwords migrate, since the uniqueness check can't see them yet.!!
- Set password to a new unique password.
  - Cloned (worked).
  - Revoked the password with `bin/auth revoke`.
  - Verified web UI shows "no password set".
  - Verified that pull no longer works.
  - Verified that I can no longer select the revoked password.
- Verified that accounts do not interact:
  - Tried to set account B to account A's password (worked).
  - Tried to set account B to a password revoked on account A (worked).
- Spot checked the `password` and `passwordtransaction` tables for saniity.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13043

Differential Revision: https://secure.phabricator.com/D18898
2018-01-23 10:56:13 -08:00
epriestley
bb12f4bab7 Add test coverage to the PasswordEngine upgrade workflow and fix a few bugs
Summary:
Ref T13043. When we verify a password and a better hasher is available, we automatically upgrade the stored hash to the stronger hasher.

Add test coverage for this workflow and fix a few bugs and issues, mostly related to shuffling the old hasher name into the transaction.

This doesn't touch anything user-visible yet.

Test Plan: Ran unit tests.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13043

Differential Revision: https://secure.phabricator.com/D18897
2018-01-23 10:55:35 -08:00
epriestley
c280c85772 Consolidate password verification/revocation logic in a new PhabricatorAuthPasswordEngine
Summary:
Ref T13043. This provides a new piece of shared infrastructure that VCS passwords and account passwords can use to validate passwords that users enter.

This isn't reachable by anything yet.

The test coverage of the "upgrade" flow (where we rehash a password to use a stronger hasher) isn't great in this diff, I'll expand that in the next change and then start migrating things.

Test Plan: Added a bunch of unit tests.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13043

Differential Revision: https://secure.phabricator.com/D18896
2018-01-23 10:54:49 -08:00
epriestley
42e2cd9af0 Add a "--force" flag to bin/auth revoke
Summary: Ref T13043. I'd like to replace the manual credential revocation in the Phacility export workflow with shared code in `bin/auth revoke`, but we need it to run non-interactively. Add a `--force` flag purely to make our lives easier.

Test Plan: Ran `bin/auth revoke --everywhere ...` with and without `--force`. Got prompted without, got total annihilation with.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13043

Differential Revision: https://secure.phabricator.com/D18895
2018-01-22 20:12:36 -08:00
epriestley
9c00a43784 Add a more modern object for storing password hashes
Summary:
Ref T13043. Currently:

  - Passwords are stored separately in the "VCS Passwords" and "User" tables and don't share as much code as they could.
  - Because User objects are all over the place in the code, password hashes are all over the place too (i.e., often somewhere in process memory). This is a very low-severity, theoretical sort of issue, but it could make leaving a stray `var_dump()` in the code somewhere a lot more dangerous than it otherwise is. Even if we never do this, third-party developers might. So it "feels nice" to imagine separating this data into a different table that we rarely load.
  - Passwords can not be //revoked//. They can be //deleted//, but users can set the same password again. If you believe or suspect that a password may have been compromised, you might reasonably prefer to revoke it and force the user to select a //different// password.

This change prepares to remedy these issues by adding a new, more modern dedicated password storage table which supports storing multiple password types (account vs VCS), gives passwords real PHIDs and transactions, supports DestructionEngine, supports revocation, and supports `bin/auth revoke`.

It doesn't actually make anything use this new table yet. Future changes will migrate VCS passwords and account passwords to this table.

(This also gives third party applications a reasonable place to store password hashes in a consistent way if they have some need for it.)

Test Plan: Added some basic unit tests to cover general behavior. This is just skeleton code for now and will get more thorough testing when applications move.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13043

Differential Revision: https://secure.phabricator.com/D18894
2018-01-22 15:35:28 -08:00
epriestley
fa1ecb7f66 Add a bin/auth revoke revoker for SSH keys
Summary: Ref T13043. Adds CLI support for revoking SSH keys. Also retargets UI language from "Deactivate" to "Revoke" to make it more clear that this is a one-way operation. This operation is already correctly implemented as a "Revoke" operation.

Test Plan: Used `bin/auth revoke --type ssh` to revoke keys, verified they became revoked (with proper transactions) in the UI. Revoked keys from the web UI flow.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13043

Differential Revision: https://secure.phabricator.com/D18893
2018-01-22 15:35:07 -08:00
epriestley
39c3b10a2f Add a bin/auth revoke revoker for sessions
Summary: Ref T13043. Allows CLI revocation of login sessions.

Test Plan: Used `bin/auth revoke --type session` with `--from` and `--everywhere` to revoke sessions. Saw accounts get logged out in web UI.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13043

Differential Revision: https://secure.phabricator.com/D18892
2018-01-22 12:01:14 -08:00
epriestley
7970cf0585 Add a bin/auth revoke revoker for temporary tokens
Summary: Ref T13043. Allows CLI revocation of temporary ("forgot password", "one-time login") tokens.

Test Plan: Used "Forgot Password?" to generate tokens, used `bin/auth revoke --type temporary` with `--from` and `--everywhere` to revoke them.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13043

Differential Revision: https://secure.phabricator.com/D18891
2018-01-22 12:00:33 -08:00
epriestley
a9d7b4f0ff Support bulk edit of "points" for Maniphest tasks
Summary: Ref T13025. Fixes T10973. Fairly straightforward. The "points" type is just an alias for "text" today.

Test Plan: Bulk edited points.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13025, T10973

Differential Revision: https://secure.phabricator.com/D18889
2018-01-22 11:59:52 -08:00
epriestley
d9b6513a21 Respect tokenizer limits in the bulk editor
Summary: Ref T13025. This makes limits (for fields like "Assign To") work in the bulk editor, so you can't type "Assign to: x, y, z" anymore.

Test Plan: Hit limit for "Assign to" and a custom project field. No limit for "Add subscribers".

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13025

Differential Revision: https://secure.phabricator.com/D18888
2018-01-22 11:55:55 -08:00
epriestley
fbfcc37531 Respect token limits for "Assign to" and custom datasource fields in Herald
Summary:
See PHI173. Currently, Herald has an "Assign to" action for tasks, and you can specify custom fields with datasource values (like users or projects) that have a limit (like 1 "Owner", or 12 "Jury Members").

Herald doesn't support these limits right now, so you can write `[ Assign to ][ X, Y, Z ]`. This just means "Assign to X", but make it more clear by actually enforcing the limit in the UI.

Test Plan:
  - Created a "projects" custom field with limit 1.
  - Tried to create actions that 'assign to' or 'set custom field to' more than one thing, got helpfully rebuffed by the UI.
  - Created an "add subscribers" action with more than one value.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18887
2018-01-22 11:54:12 -08:00
epriestley
6a62797056 Fix some issues with Diffusion file data limits
Summary:
See <https://discourse.phabricator-community.org/t/files-created-from-repository-contents-slightly-over-one-chunk-in-size-are-truncated-to-exactly-one-chunk-in-size/988/1>. Three issues here:

  - When we finish reading `git cat-file ...` or whatever, we can end up with more than one chunk worth of bytes left in the internal buffer if the read is fast. Use `while` instead of `if` to make sure we write the whole buffer.
  - Limiting output with `setStdoutSizeLimit()` isn't really a reliable way to limit the size if we're also reading from the buffer. It's also pretty indirect and confusing. Instead, just let the `FileUploadSource` explicitly implement a byte limit in a straightforward way.
  - We weren't setting the time limit correctly on the main path.

Overall, this could cause >4MB files to "write" as 4MB files, with the rest of the file left in the UploadSource buffer. Since these files were technically under the limit, they could return as valid. This was intermittent.

Test Plan:
  - Pushed a ~4.2MB file.
  - Reloaded Diffusion a bunch, sometimes saw the `while/if` buffer race and produce a 4MB file with a prompt to download it. (Other times, the buffer worked right and the page just says "this file is too big, sorry").
  - Applied patches.
  - Reloaded Diffusion a bunch, no longer saw bad behavior or truncated files.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18885
2018-01-22 11:52:37 -08:00
epriestley
c637680445 Show related objects on daemon worker task queue detail pages
Summary:
See PHI287. Currently, you can't get very much information about a task in the worker queue from the web UI.

This is largely intentional (e.g., it's bad if we let you inspect the content of a "send an administrator's password reset email" task), and a lot of the data is task-class specific so it would be a lot of work to expose it, but we can add one useful piece of information pretty easily: for tasks with an `objectPHID` that points at a valid object that's visible to the user, tell the user which object the task is associated with.

Test Plan: {F5386562}

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18890
2018-01-20 06:52:15 -08:00
epriestley
3b7547e726 Fix an issue where certain configurations could fail to publish revision feed stories
Summary: See PHI292. This is just a generalization of D18851: feed stories have the same issue as mail. Don't hide "requested a review" in either mail or feed.

Test Plan:
  - Enable prototypes.
  - No harbormaster builds.
  - Create a revision.
    - Pre-patch: no feed story.
    - Post-patch: feed story.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18886
2018-01-19 15:39:31 -08:00
epriestley
86a0c7daa2 Fix a fatal in blame when the viewer can't see a revision because of a permission issue
Summary:
Fixes T13040. To reproduce:

  - View a file with blame enabled, where some line has an associated revision (say, `D123`).
  - Edit `D123` so it exists and is a valid revision, but the viewer can't see it.
  - Reload the page.

Instead, only add revisions to the map if we actually managed to load them.

Test Plan: Page no longer fatals.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13040

Differential Revision: https://secure.phabricator.com/D18884
2018-01-19 14:19:06 -08:00
epriestley
3038d564a6 Allow bulk edits to be made silently if you have CLI access
Summary:
Fixes T13042. This hooks up the new "silent" mode from D18882 and makes it actually work.

The UI (where we tell you to go run some command and then reload the page) is pretty clumsy, but should solve some problems for now and can be cleaned up eventually. The actual mechanics (timeline aggregation, Herald interaction,  etc.) are on firmer ground.

Test Plan:
  - Made a normal bulk edit, got mail and feed stories.
  - Made a silent bulk edit, no mail and no feed.
  - Saw "Silent Edit" marker in timeline for silent edits:

{F5386245}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13042

Differential Revision: https://secure.phabricator.com/D18883
2018-01-19 13:24:54 -08:00
epriestley
8b12fa6d6e Prepare TransactionEditor for silent transactions via bulk edit
Summary:
Ref T13042. This adds a "silent" edit mechanism which suppresses feed stories, email, and notifications.

The other behaviors here are:

  - The transactions are marked as "silent" so we can render a hint in the UI in the future to make it clear to users that they aren't missing email.
  - If the editor uses Herald, mail rules are suppressed so they don't fire incorrectly (this mostly affects "the first time this rule matches, send me an email" rules: without this, they'd match "the first time" on the bulk edit, not send email, then never match again since they already matched).
  - If the edit queues additional edits, those are applied silently too.

This doesn't (or, at least, shouldn't) actually change any behavior since you can't apply silent edits yet.

Test Plan:
Somewhat theoretical, since this isn't reachable yet. Should get meaningful testing in an upcoming change.

Did a bit of var_dump() / debug poking to attempt to verify that nothing too crazy is happening.

Viewed and edited objects, no changes in behavior.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13042

Differential Revision: https://secure.phabricator.com/D18882
2018-01-19 13:23:38 -08:00
epriestley
8a5def8e0a Remove legacy transaction editor "getDisableEmail()" method
Summary:
Ref T13042. This is a very, very old policy-violating option from yesteryear which supported build systems publishing updates by adding comments to revisions, without sending email about it.

Harbormaster has served this role for a long time and this is policy-violating in the general case (it allows attackers to act in secret).

Test Plan: Grepped for affected symbols.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13042

Differential Revision: https://secure.phabricator.com/D18881
2018-01-19 13:23:06 -08:00
epriestley
7a43181337 Organize bulk edit actions into nice groups
Summary: Ref T13025. We're getting kind of a lot of actions, so put them in nice groups so they're easier to work with.

Test Plan: {F5386038}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13025

Differential Revision: https://secure.phabricator.com/D18880
2018-01-19 13:22:25 -08:00
epriestley
ae1b07bcfb Support "<select />" custom fields in bulk editor
Summary: Ref T13025. Fixes T5689. A straightforward change!

Test Plan: Used the bulk editor to modify a custom "select" field like the one in T5689.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13025, T5689

Differential Revision: https://secure.phabricator.com/D18879
2018-01-19 13:18:02 -08:00
epriestley
f8113aecdc Make bulk edit <select /> fields a little more natrual and set options for subtype transactions
Summary:
Ref T13025. This is some minor technical stuff: make the "select" bulk edit type a little more consistent with other types by passing data down instead of having it reach up the stack. This simplifies the implementation of a custom field "select" in a future change.

Also, provide an option list to the "select" edit field for object subtypes. This is only accessible via Conduit so it currently never actually renders anything in the UI, but with the bulk edit stuff we get some initialization order issues if we don't set anything. This will also make any future changes which expose subtypes more broadly more straightforward.

Test Plan:
  - Bulk edited "select" fields, like "Status" and "Priority".
  - No more fatal when trying to `getOptions()` internally on the subtype field.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13025

Differential Revision: https://secure.phabricator.com/D18878
2018-01-19 13:17:28 -08:00
epriestley
b6737554e1 Support tokenizer custom fields in bulk editor
Summary:
Ref T13025. This allows custom tokenizer fields, like a "Owning Group" field, to be edited with the bulk editor.

See PHI173 for some context.

Test Plan: Edited a custom "Owner" field (a project tokenizer) with the bulk editor.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13025

Differential Revision: https://secure.phabricator.com/D18877
2018-01-19 13:16:46 -08:00
epriestley
a26cf20dd1 Fix a bug with setting custom PHID list field values via Conduit and prepare for bulk edits
Summary:
Ref T13025. Custom field transactions work somewhat unusually: the values sometimes need to be encoded. We currently do not apply this encoding correctly via Conduit.

For example, setting some custom PHID field to `["PHID-X-Y"]` fails with a bunch of JSON errors.

Add an extra hook callback so that EditTypes can apply processing to transaction values, then apply the correct CustomField processing.

This only affects Conduit. In a future diff, this also allows bulk edit of custom fields to work correctly.

Test Plan: Added a custom field to Maniphest with a list of projects. Used Conduit to bulk edit it (which now works, but did not before). Used the web UI to bulk edit it.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13025

Differential Revision: https://secure.phabricator.com/D18876
2018-01-19 12:51:35 -08:00
epriestley
1e51f1d0dc Reuse more transaction construction code in bulk editor
Summary:
Ref T13025. Currently, the bulk editor takes an HTTP request and emits a list of "raw" transactions (simple dictionaries). This goes into the job queue, and the background job builds a real transaction.

However, the logic to turn an HTTP request into a raw transaction is ending up with some duplication, since we generally already have logic to turn an HTTP request into a full object.

Instead: build real objects first, then serialize them to dictionaries. Send those to the job queue, rebuild them into objects again, and we end up in the same spot with a little less code duplication.

Finally, delete the mostly-copied code.

Test Plan: Used bulk editor to add comments, projects, and rename tasks.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13025

Differential Revision: https://secure.phabricator.com/D18875
2018-01-19 12:49:17 -08:00
epriestley
91a78db99b Support "Assign To" in Maniphest bulk editor
Summary:
Ref T13025. See PHI173. This supports the "Assign to" field in the new editor.

This is very slightly funky: to unassign tasks, you need to leave the field blank. I have half a diff to fix this, but the way the `none()` token works in the default datasource is odd so it needs a separate datasource. I'm punting on this for now since it works, at least, and isn't //completely// unreasonable.

This also simplifies some EditEngine stuff a little. Notably:

  - I reorganized EditType construction slightly so subclasses can copy/paste a little bit less.
  - EditType had `field` and `editField` properties which had the same values. I canonicalized on `editField` and made this value set a little more automatically.

Test Plan: Used bulk editor to reassign some tasks. By leaving the field blank, unassigned tasks.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13025

Differential Revision: https://secure.phabricator.com/D18874
2018-01-19 12:48:41 -08:00
epriestley
0cad6021b6 Restore "Tags" and "Subscribers" edit capabilities to Maniphest bulk editor
Summary: Depends on D18867. Ref T13025. Fixes T8740. Rebuilds the tag/subscriber actions (add, remove, set) into the bulk editor.

Test Plan: Added, removed and set these values via bulk edit.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13025, T8740

Differential Revision: https://secure.phabricator.com/D18868
2018-01-19 12:47:10 -08:00
epriestley
687fada5af Restore bulk edit support for remarkup fields (description, add comment)
Summary:
Depends on D18866. Ref T13025. Fixes T12415. This makes the old "Add Comment" action work, and adds support for a new "Set description to" action (possibly, I could imagine "append description" being useful some day, maybe).

The implementation is just a `<textarea />`, not a whole fancy remarkup box with `[Bold] [Italic] ...` buttons, preview, typeaheads, etc. It would be nice to enrich this eventually but doing the rendering in pure JS is currently very involved.

This requires a little bit of gymnastics to get the transaction populated properly, and adds some extra validation since we need some code there anyway.

Test Plan:
  - Changed the description of a task via bulk editor.
  - Added a comment to a task via bulk editor.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13025, T12415

Differential Revision: https://secure.phabricator.com/D18867
2018-01-19 12:45:34 -08:00
epriestley
bf1ac701c3 Support "select" types in bulk editor (status, priority)
Summary: Depends on D18864. Ref T13025. Adds bulk edit support back for "status" and "priority" using `<select />` controls.

Test Plan:
Used bulk editor to change status and priority for tasks.

{F5374436}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13025

Differential Revision: https://secure.phabricator.com/D18866
2018-01-19 12:44:48 -08:00
epriestley
a251db4618 Remove the Maniphest-specific bulk job type
Summary: Depends on D18863. Ref PHI173. Ref T13025. After D18863, this job type is no longer used: the workflow uses a genric worker instead which can apply transactions to any object.

Test Plan: Grepped for callsites, found none.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13025

Differential Revision: https://secure.phabricator.com/D18864
2018-01-19 12:44:16 -08:00
epriestley
09e71a4082 Define bulk edits in terms of EditEngine, not hard-coded ad-hoc definitions
Summary:
Depends on D18862. See PHI173. Ref T13025. Fixes T10005. This redefines bulk edits in terms of EditEngine fields, rather than hard-coding the whole thing.

Only text fields -- and, specifically, only the "Title" field -- are supported after this change. Followup changes will add more bulk edit parameter types and broader field support.

However, the title field now works without any Maniphest-specific code, outside of the small amount of binding code in the `ManiphestBulkEditor` subclass.

Test Plan: Used the bulk edit workflow to change the titles of tasks.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13025, T10005

Differential Revision: https://secure.phabricator.com/D18863
2018-01-19 12:43:47 -08:00
epriestley
6ef45d8245 Provide a generic transaction-oriented bulk job worker
Summary:
Depends on D18806. Ref T13025. See PHI173. Currently, Maniphest bulk edits are processed by a Maniphest-specific worker. I want to replace this with a generic worker which can apply transactional edits to any object.

This implements a generic worker, although it has no callers yet. Future changes give it callers, and later remove the Maniphest-specific worker.

Test Plan: See next changes.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13025

Differential Revision: https://secure.phabricator.com/D18862
2018-01-19 12:41:56 -08:00
epriestley
7f91c8c4ac Rebuild the bulk editor on SearchEngine
Summary:
Depends on D18805. Ref T13025. Fixes T10268.

Instead of using a list of IDs for the bulk editor, power it with SearchEngine queries. This gives us the full power of SearchEngine and lets us use a query key instead of a list of 20,000 IDs to avoid issues with URL lengths.

Also, split it into a base `BulkEngine` and per-application subclasses. This moves us toward T10005 and universal support for bulk operations.

Also:

  - Renames most of "batch" to "bulk": we're curently inconsitent about this, I like "bulk" better since I think it's more clear if you don't regularly interact with `.bat` files, and newer stuff mostly uses "bulk".
  - When objects in the result set can't be edited because you don't have permission, show the status more clearly.

This probably breaks some stuff a bit since I refactored so heavily, but it seems mostly OK from poking around. I'll clean up anything I missed in followups to deal with remaining items on T13025.

Test Plan:
{F5302300}

  - Bulk edited from Maniphest.
  - Bulk edited from a workboard (no more giant `?ids=....` in the URL).
  - Hit most of the error conditions, I think?
  - Clicked the "Cancel" button.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13025, T10268

Differential Revision: https://secure.phabricator.com/D18806
2018-01-19 12:40:08 -08:00
epriestley
ad659627b3 Make bulk editor working set editable and more homogenous
Summary:
Ref T13025. See PHI50. Fixes T11286. Ref T10005. Begin modernizing the bulk editor.

For T10005 ("move the bulk editor to modern infrastructure"), rewrite the rendering of the editable set so that it is application-agnostic and can work with any kind of object.

For T11286 ("let users de-select items in the working set"), make the working set editable.

Test Plan:
{F5302158}

  - Deselected some objects, applied an edit, saw the edit apply to only selected objects.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13025, T11286, T10005

Differential Revision: https://secure.phabricator.com/D18805
2018-01-19 12:39:27 -08:00
epriestley
3e983b583d Fix a transposed feed story in Badges
Summary: See <https://discourse.phabricator-community.org/t/badge-quality-from-and-to-interchanged-in-activity-log/987>. These are swapped.

Test Plan: Changed a badge quality, viewed feed story, saw it position the strings correctly.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18870
2018-01-16 13:57:01 -08:00
epriestley
82bfb98179 Fix a copy/paste error on the burnup chart
Summary: See PHI286, maybe. See PHI273. Strongly considering just deleting this.

Test Plan: ~.~

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18865
2018-01-11 07:07:46 -08:00
Alex Vandiver
2140741e25 Fix typo in new setting description
Summary:
Noticed by @amckinley in
https://secure.phabricator.com/D18850#inline-57246 but not fixed
before landing.

Test Plan: ispell

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, amckinley, epriestley

Differential Revision: https://secure.phabricator.com/D18861
2018-01-06 07:25:27 -08:00
epriestley
adbd7d4fd8 Make the new synthetic burnup chart data respect the "Project" filter
Summary: See PHI273. Third time's the charm? This page has a "Project" filter which lets you view data for only one project, but the synthetic data currently ignores it.

Test Plan: Filtered burnup chart by various projects, saw sensible-looking data.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18860
2018-01-05 10:34:45 -08:00
epriestley
5543592034 Add a couple of clarifying comments to the Mercurial protocol parser
Summary: See D18857. Ref T13036. See PHI275. Explain what's going on here a little better since it isn't entirely obvious and debugging these stream parsers is a gigantic pain.

Test Plan: Read text.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13036

Differential Revision: https://secure.phabricator.com/D18859
2018-01-04 14:23:28 -08:00
epriestley
94db95a165 Sort burnup data chronologically after merging synthetic and "real" data
Summary:
Ref T13020. See PHI273. See D18853. On `secure`, the chart looks less promising than it did locally, and is full of discontinuities:

{F5356544}

I think this is a sorting issue. But if I can't fake my way through this soon I'll maybe get the Fact engine running and use it to provide the data here, as a sort of half-step toward T1562?

Test Plan: Chart looks the same locally, will push and see if `secure` improves?

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13020

Differential Revision: https://secure.phabricator.com/D18854
2018-01-04 14:08:03 -08:00
epriestley
13c8963dab Fix a Mercurial wire protocol parser issue when we receive a length frame before any data
Summary:
Depends on D18856. Ref T13036. See PHI275. When we receive a length frame but the buffer doesn't have any data yet, we currently emit a pointless 0-length data frame on the channel.

For normal chatter this is harmless/valid, but it causes problems when a channel has transitioned into bundle2 mode (probably it indicates "end of stream")?

In any case, it's never helpful, so if we're about to read a data block and don't have any data, just bail out until we see some more data.

Note that we can't end up here //expecting// a 0-length data block: both the `data-length` and `data-bytes` states already handle that properly.

Test Plan: Pushed 4MB changes to a Mercurial repository with Mercurial 4.1.1, was no longer able to hit channel errors.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13036

Differential Revision: https://secure.phabricator.com/D18857
2018-01-04 14:06:52 -08:00
epriestley
3a4e14431f Remove an obsolete comment about Mercurial SSH error behavior
Summary:
Depends on D18855. Ref T13036. This comment no longer seems to be accurate: anything we send over `stderr` is faithfully shown to the user with recent clients.

From [[ https://www.mercurial-scm.org/repo/hg/file/default/mercurial/help/internals/wireprotocol.txt | this document ]], the missing sauce may have been:

```
A generic error response type is also supported. It consists of a an error
message written to ``stderr`` followed by ``\n-\n``. In addition, ``\n`` is
written to ``stdout``.
```

That is, writing "\n" to stdout in addition to writing the error to stderr. However, this no longer appears to be necessary.

I think the modern client behavior is generally sensible (and consistent with the behavior of Git and Subversion) so this //probably// isn't a bug or me making a mistake.

Test Plan: With a modern client, threw some arbitrary exception during execution. Observed a helpful message on the client with no additional steps.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13036

Differential Revision: https://secure.phabricator.com/D18856
2018-01-04 14:05:44 -08:00
epriestley
0f02d79ffa Remove nonfunctional Mercurial "bundle2" capability filtering from SSH pathway
Summary:
Ref T13036. This code attempts to filter the "capabilities" message to remove "bundle2", but I think this has never worked.

Specifically, the //write// pathway is hooked, and "write" here means "client is writing a message to the server". However, the "capabilities" frame is part of the response, not part of the request. Thus, this code never fires, at least on recent versions of Mercurial.

Since I plan to support bundle2 and don't want to decode response frames, just get rid of this, assuming we'll achieve those goals.

I think this was just overlooked in D14241, which probably focused on the HTTP version. This code does (at least, potentially) do something for HTTP.

I'm leaving the actual "strip stuff" code in place for now since I think it's still used on the HTTP pathway.

Test Plan:
  - Added debug logging, saw this code never hit even though `hg push --debug` shows the client believing bundle2 is supported.
  - Logged both halves of the wire protocol and saw this come from the server, not the client.
  - Ran the failing `hg push` of a 4MB file under hg 4.4.1, got the same error as before.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: cspeckmim

Maniphest Tasks: T13036

Differential Revision: https://secure.phabricator.com/D18855
2018-01-04 14:05:13 -08:00
epriestley
f3f1f9dc57 Allow "drydock.blueprint.edit" to create blueprints
Summary:
Depends on D18848. Ref PHI243. This puts a bit of logic up front to figure out the blueprint type before we actually start editing it.

This implementation is a little messy but it keeps the API clean. Eventually, the implementation could probably go in the TransactionTypes so more code is shared, but I'd like to wait for a couple more of these first.

This capability probably isn't too useful, but just pays down a bit of technical debt from the caveat introduced in D18822.

Test Plan:
  - Created a new blueprint with the API.
  - Tried to create a blueprint without a "type" (got a helpful error).
  - Created and edited blueprints via the web UI.
  - Tried to change the "type" of an existing blueprint (got a helpful error).

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Differential Revision: https://secure.phabricator.com/D18849
2018-01-04 10:08:07 -08:00