Summary:
See PHI307. Currently, when reviews undraft, we retroactively add in older activity to the mail ("alice created this revision...").
However, we don't add that activity to the mail tags, so the relevant tags (like "revision created") are dropped forever.
Instead, use the same set of transactions for both mail body and mail tag construction.
This should be obsoleted in the relatively near future by T10448, but it's a better/more correct behavior in general and we probably can't get rid of tags completely for a while.
Test Plan:
Applied patch, created a revision with builds, saw it auto-undraft after builds finished. Used `bin/mail list-outbound` and `bin/mail show-outbound` to see the mail. Verified that it included retroactive text ("created this revision") AND retroactive tags.
Note that the tag for "A new revision is created" is `DifferentialTransaction::MAILTAG_REVIEW_REQUEST` with literal value `differential-review-request`.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18941
Summary:
Depends on D18939. Ref T13047. Symbol lookup can be activated from a diff (in Differential or Diffusion) or from the static view of a file at a particular commit.
In the latter case, we need to figure out the path a little differently. The character and line number approaches still work as written.
Test Plan:
- Command-clicked symbols in the Diffusion browse view with blame on and off; saw path, line and char populate properly.
- Command-clicked symbols in Differential diff view to check I didn't break anything.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13047
Differential Revision: https://secure.phabricator.com/D18940
Summary: Depends on D18937. Ref T13047. When available, provide character positions so external indexers can return more accurate results.
Test Plan: Clicked symbols in Safari, Firefox and Chrome, got sensible-looking character positions.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13047
Differential Revision: https://secure.phabricator.com/D18939
Summary: See PHI280. We have a similar field for tasks already, this is generally a reasonable sort of thing to support, and the addition of "draft" states means there are some pretty reasonable use cases.
Test Plan:
- Wrote a status-based ("status is needs revision") Herald rule.
- Tested it against a "Needs Revision" revision (passed) and a "Changes Planned" revision (failed).
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18938
Summary:
Depends on D18936. Ref T13047. Third parties can define external symbol sources that let users jump to PHP or Python documentation or query some server.
Give these queries more information so they can try to get better results: the path and line where the symbol appeared, and any known repository scope.
Test Plan: Wrote a fake external source that used this data, command-clicked a symbol in Differential, saw a fake external symbol result.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13047
Differential Revision: https://secure.phabricator.com/D18937
Summary:
Ref T13047. In some reasonable cases, knowing the path and line number where a symbol appears is useful in ranking or filtering the set of matching symbols.
Giving symbol sources more information can't hurt, and it's generally free for us to include this context since we just need to grab it out of the document and pass it along.
We can't always get this data (for example, if a user types `s idx` into global search, we have no clue) but this is similar to other types of context which are only available sometimes (like which repository a symbol appears in).
Test Plan: Command-clicked some symbols in 1-up (unified) and 2-up (side-by-side) diff views with symbol indexes configured. Got accurate path and line information in the URI I was redirected to.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13047
Differential Revision: https://secure.phabricator.com/D18936
Summary:
Depends on D18934. Ref T13046. Add support for the new export flow to a second application.
My goal here is mostly just to make sure that this is general enough to work in more than one place, and exporting user accounts seems plausible as a useful feature, although we do see occasional requests for this feature exactly (like <https://discourse.phabricator-community.org/t/users-export-to-csv/968>).
The exported data may not truly be useful for much (no disabled/admin/verified/MFA flags, no external account data, no email addresses for policy reasons) but we can expand it as use cases arise.
Test Plan: Exported user accounts in several formats.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13046
Differential Revision: https://secure.phabricator.com/D18935
Summary:
Depends on D18918. Ref T13046. Ref T5954. Pull logs can currently be browsed in the web UI, but this isn't very powerful, especially if you have thousands of them.
Allow SearchEngine implementations to define exportable fields so that users can "Use Results > Export Data" on any query. In particular, they can use this workflow to download a file with pull logs.
In the future, this can replace the existing "Export to Excel" feature in Maniphest.
For now, we hard-code JSON as the only supported datatype and don't actually make any effort to format the data properly, but this leaves room to add more exporters (CSV, Excel) and data type awareness (integer casting, date formatting, etc) in the future.
For sufficiently large result sets, this will probably time out. At some point, I'll make this use the job queue (like bulk editing) when the export is "large" (affects more than 1K rows?).
Test Plan: Downloaded pull logs in JSON format.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13046, T5954
Differential Revision: https://secure.phabricator.com/D18919
Summary:
Depends on D18932. Ref T13048. See PHI276. In the cluster, we don't have device keys on `web` nodes. This is generally good, since they don't need them, and it means that we aren't putting more credentials than we need on those hosts.
However, it means that when we pull diff content to test "Commit" rules via the Herald test console, we use the omnipotent user and try to use device credentials, and this fails since we don't have any.
Instead, pass the real viewer in this case so we just sign the request as them, like we do for normal Diffusion requests.
Test Plan:
Wrote and ran a commit content rule locally, no issues.
This isn't completely convincing since my local setup does have device credentials, but I'll double-check in production once this deploys.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13048
Differential Revision: https://secure.phabricator.com/D18933
Summary:
Depends on D18931. Ref T13048. Ref T13041. This field means "the first accepting reviewer, where order is mostly arbitrary". Modern rules should almost certainly use "Accepting Reviewers" instead.
Getting rid of this completely is a pain, but we can at least reduce confusion by marking it as not-the-new-hotness. Add a "Deprecated" group, move it there, and mark it for exile.
Test Plan:
Edited a commit rule, saw it in "Deprecated" group at the bottom of the list:
{F5395001}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13048, T13041
Differential Revision: https://secure.phabricator.com/D18932
Summary: Depends on D18930. Ref T13048. Try to explain what this does and give an example since I think it's probably not very obvious from the name.
Test Plan: Read the text.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13048
Differential Revision: https://secure.phabricator.com/D18931
Summary: Depends on D18927. Ref T13048. This implements a new policy which allows Herald rules to fire on some kinds of state changes.
Test Plan:
Wrote and tested rules with the new policy:
{F5394971}
{F5394972}
Also wrote and tested rules with the old policies:
{F5394973}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13048
Differential Revision: https://secure.phabricator.com/D18930
Summary:
Depends on D18926. Ref T6203. Ref T13048. Herald rule repetition policies are stored as integers but treated as strings in most contexts.
After D18926, the integer stuff is almost totally hidden inside `HeraldRule` and getting rid of it completely isn't too tricky.
Do so now.
Test Plan:
- Created "only the first time" and "every time" rules. Did a SELECT on their rows in the database.
- Ran migrations, got a clean bill of health from `storage adjust`.
- Did another SELECT on the rows, saw a faithful conversion to strings "every" and "first".
- Edited and reviewed rules, swapping them between "every" and "first".
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13048, T6203
Differential Revision: https://secure.phabricator.com/D18927
Summary:
Depends on D18925. Ref T13048. Currently, HeraldRule stores policies as integers (0 or 1) in the database.
The application tries to mostly use strings ("first", "every"), but doesn't do a good job of hiding the fact that the values are integers deeper in the stack. So we end up with a lot of code like this:
```lang=php
$stored_int_value = $rule->getRepetitionPolicy();
$equivalent_string = HeraldRepetitionPolicyConfig::getString($stored_int_value);
$is_first = ($equivalent_string === HeraldRepetitionPolicyConfig::FIRST);
```
This happens in several places and is generally awful. Replace it with:
```lang=php
$is_first = $rule->isRepeatFirst();
```
To do this, merge `HeraldRepetitionPolicyConfig` into `HeraldRule` and hide all the mess inside the methods.
(This may let us just get rid of the integers in a later change, although I'm not sure I want to commit to that.)
Test Plan:
- Grepped for `HeraldRepetitionPolicyConfig`, no more hits.
- Grepped for `setRepetitionPolicy(...)` and `getRepetitionPolicy(...)`. There are no remaining callers outside of `HeraldRule`.
- Browed and edited several rules. I'll vet this more convincingly after adding the new repetition rule.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13048
Differential Revision: https://secure.phabricator.com/D18926
Summary:
Depends on D18924. Ref T13048. Each adapter defines which repetition options ("every time", "only the first time") users may select for rules.
Currently, this is all explicit and hard-coded. However, every adapter really just implements this rule (except for some bugs, see below):
> You can pick "only the first time" if this adapter fires more than once on the same object.
Since we already have a `isSingleEventAdapter()` method which lets us tell if an adapter fires more than once, just write this rule in the base class and delete all the copy/pasting.
This also fixes two bugs because of the copy/pasting: Pholio Mocks and Phriction Documents did not allow you to write "only the first time" rules. There's no reason for this, they just didn't copy/paste enough methods when they were implemented.
This will make a future diff (which introduces an "if the rule did not match last time" policy) cleaner.
Test Plan:
- Checked several different types of rules, saw appropriate options in the dropdown (pre-commit: no options; tasks: first or every).
- Checked mocks and wiki docs, saw that you can now write "only the first time" rules.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13048
Differential Revision: https://secure.phabricator.com/D18925
Summary: Depends on D18928. Ref T13043. Add some automated test coverage for SSH revocation rules.
Test Plan: Ran tests, got a clean bill of health.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18929
Summary:
Ref T13043. In an earlier change I updated this langauge from "Deactivate" to "Revoke", but the behavior doesn't quite match.
This table has a unique key on `<isActive, keyBody>`, which enforces the rule that "a key can only be active for one unique user".
However, we set `isActive` to `null` when we revoke a key, and multiple rows are allowed to have the value `<null, "asdf">` (since a `null` column in a unique key basically means "don't enforce this unique key").
This is intentional, to support this workflow:
- You add key X to bot A.
- Whoops, wrong account.
- You revoke key X from bot A.
- You add key X to bot B.
This isn't necessarily a great workflow -- ideally, you'd throw key X away and go generate a new key after you realize you made a mistake -- but it's the sort of practical workflow that users are likely to expect and want to see work ("I don't want to generate a new key, it's already being used by 5 other services and cycling it is a ton of work and this is just a test install for my dog anyway."), and there's no technical reason we can't support it.
To prevent users from adding keys on the revocation list back to their account, just check explicitly.
(This is probably better in general anyway, because "cert-authority" support from PHI269 may mean that two keys are "equivalent" even if their text differs, and we may not be able to rely on a database test anyway.)
Test Plan:
- Added the key `ssh-rsa asdf` to my account.
- Revoked it.
- Tried to add it again.
- Before patch: worked.
- After patch: error, "this key has been revoked".
- Added it to a different account (the "I put it on the wrong bot" workflow).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18928
Summary:
Ref T13025. See <https://discourse.phabricator-community.org/t/bulk-edit-no-actions-available/1011/1>.
I'm not sure if this is what the user is seeing, but in Chrome, the `<select />` does not automatically get set to the first valid value like it does in Safari.
Set it to the first valid value explicitly.
Test Plan: In Chrome, bulk editor previously hit a JS error when trying to read a bad action off the `<select />`. After patch, bulk edits go through cleanly.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13025
Differential Revision: https://secure.phabricator.com/D18923
Summary:
Fixes T5965.
Fixes two issues:
- Observing an empty repository could write a warning to the log.
- Mirroring an empty repository to a remote could fail.
For observing:
If newly-created with `git init --bare`, `git ls-remote` will
return the empty string. Properly return an empty set of refs, rather
than attempting to parse the single "line" that is produced by
splitting that on newlines:
```
[2018-01-23 18:47:00] ERROR 8: Undefined offset: 1 at [/phab_path/phabricator/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php:405]
arcanist(head=master, ref.master=5634f8410176), phabricator(head=master, ref.master=12551a1055ce), phutil(head=master, ref.master=4755785517cf)
#0 PhabricatorRepositoryPullEngine::loadGitRemoteRefs(PhabricatorRepository) called at [<phabricator>/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php:343]
#1 PhabricatorRepositoryPullEngine::executeGitUpdate() called at [<phabricator>/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php:126]
#2 PhabricatorRepositoryPullEngine::pullRepositoryWithLock() called at [<phabricator>/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php:40]
#3 PhabricatorRepositoryPullEngine::pullRepository() called at [<phabricator>/src/applications/repository/management/PhabricatorRepositoryManagementUpdateWorkflow.php:59]
...
```
For mirroring:
`git` treats `git push --mirror` specially when a repository is empty. Detect this case by seeing if `git for-each-ref --count 1` does anything. If the repository is empty, just bail.
Test Plan:
- Observed an empty and non-empty repository.
- Mirrored an empty and non-empty repository.
Reviewers: alexmv, amckinley
Reviewed By: alexmv
Subscribers: Korvin, epriestley
Maniphest Tasks: T5965
Differential Revision: https://secure.phabricator.com/D18920
Summary: Help for GD SetupChcek was missing the "how to install extension" content.
Test Plan: Uninstalled gd, validated extension installation instructions were present in Setup Issue.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D18922
Summary:
See PHI309. There is a window of time between when all builds pass and when Harbormaster actually publishes a revision out of draft.
If any other user tries to interact with the revision during that window, they'll pick up the undraft transaction as a side effect. However, they won't have permission to apply it and will be stopped by a validation error.
Instead, only automatically publish a revision if the actor is the revision author or some system/application user (essentially always Harbormaster).
Test Plan:
- Added a `echo ...; sleep(30);` to `HarbormasterBuildEngine->updateBuildable()` before the `applyTransactions()` at the bottom.
- Wrote an "Always, run an HTTP request" Herald rule and Harbormaster build plan.
- Ran daemons with `bin/phd debug task`.
- Created a new revision with `arc diff`, as user A.
- Waited for `phd` to enter the race window.
- In a separate browser, as user B, submitted a comment via `differential.revision.edit`.
- Before patch: edits during the race window were rejected with a validation error, "you don't have permission to request review".
- After patch: edits go through cleanly.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18921
Summary: Depends on D18917. Ref T13046. While I'm in here, update this to use more modern construction.
Test Plan: Browed and queried for push logs.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13046
Differential Revision: https://secure.phabricator.com/D18918
Summary:
Depends on D18915. Ref T13046.
- Distinguish between HTTP and HTTPS.
- Use more constants and fewer magical strings.
- For HTTP responses, give them better type information and more helpful UI behaviors.
Test Plan: Pulled over SSH and HTTP. Reviewed resulting logs from the web UI. Hit errors like missing/invalid credentials.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13046
Differential Revision: https://secure.phabricator.com/D18917
Summary: Depends on D18914. Updates this Query to use slightly more modern construction while I'm working in adjacent code.
Test Plan: Viewed push logs in web UI.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18915
Summary:
Depends on D18912. Ref T13046. Add a UI to browse the existing pull log table.
The actual log still has some significant flaws, but get the basics working.
Test Plan: {F5391909}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13046
Differential Revision: https://secure.phabricator.com/D18914
Summary:
See PHI305. Ref T13046.
The SSH workflows currently extend `PhabricatorManagementWorkflow` to benefit from sharing all the standard argument parsing code. Sharing the parsing code is good, but it also means they inherit a `getViewer()` method which returns the ommnipotent viewer.
This is appropriate for everything else which extends `ManagementWorkflow` (like `bin/storage`, `bin/auth`, etc.) but not appropriate for SSH workflows, which have a real user.
This caused a bug with the pull logs where `pullerPHID` was not recorded properly. We used `$this->getViewer()->getPHID()` but the correct code was `$this->getUser()->getPHID()`.
To harden this against future mistakes:
- Don't extend `ManagementWorkflow`. Extend `PhutilArgumentWorkflow` instead. We **only** want the argument parsing code.
- Rename `get/setUser()` to `get/setSSHUser()` to make them explicit.
Then, fix the pull log bug by calling `getSSHUser()` instead of `getViewer()`.
Test Plan:
- Pulled and pushed to a repository over SSH.
- Grepped all the SSH stuff for the altered symbols.
- Saw pulls record a valid `pullerPHID` in the pull log.
- Used `echo {} | ssh ... conduit conduit.ping` to test conduit over SSH.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13046
Differential Revision: https://secure.phabricator.com/D18912
Summary: Depends on D18910. Ref T13043. Provides reasonable user-facing documentation about the general role and utility of this tool.
Test Plan: Read document.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18911
Summary:
Depends on D18908. Ref T13043. Allow users to get information about what revokers do with a new `--list` flag.
You can use `--list --type <key>` to get information about a specfic revoker.
Test Plan: Ran `bin/auth revoke --list`, saw a list of revokers with useful information.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18910
Summary:
Depends on D18907. Ref T13043. Ref T12509. We have some weird old password digest behavior that isn't terribly concerning, but also isn't great.
Specifically, old passwords were digested in weird ways before being hashed. Notably, account passwords were digested with usernames, so your password stops working if your username is chagned. Not the end of the world, but silly.
Mark all existing hashes as "v1", and automatically upgrade then when they're used or changed. Some day, far in the future, we could stop supporting these legacy digests and delete the code and passwords and just issue upgrade advice ("Passwords which haven't been used in more than two years no longer work."). But at least get things on a path toward sane, modern behavior.
Test Plan: Ran migration. Spot-checked that everthing in the database got marked as "v1". Used an existing password to login successfully. Verified that it was upgraded to a `null` (modern) digest. Logged in with it again.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043, T12509
Differential Revision: https://secure.phabricator.com/D18908
Summary:
Depends on D18906. Ref T13043. When SSH keys are edited, we normally include a warning that if you don't recognize the activity you might have problems in the mail body.
Currently, this warning is also shown for revocations with `bin/auth revoke --type ssh`. However, these revocations are safe (revocations are generally not dangerous anyway) and almost certainly legitimate and administrative, so don't warn users about them.
Test Plan:
- Created and revoked a key.
- Creation mail still had warning; revocation mail no longer did.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18907
Summary:
Depends on D18904. Ref T13043. If an attacker compromises a victim's session and bypasses their MFA, they can try to guess the user's current account password by making repeated requests to change it: if they guess the right "Old Password", they get a different error than if they don't.
I don't think this is really a very serious concern (the attacker already got a session and MFA, if configured, somehow; many installs don't use passwords anyway) but we get occasional reports about it from HackerOne. Technically, it's better policy to rate limit it, and this should reduce the reports we receive.
Test Plan: Tried to change password over and over again, eventually got rated limited. Used `bin/auth unlimit` to clear the limit, changed password normally without issues.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18906
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
Summary:
Depends on D18845. See PHI243 for context and more details.
Briefly, some objects need a "type" transaction (or something similar) very early on, and we can't generate their fields until we know the object type. Drydock blueprints are an example: a blueprint's fields depend on the blueprint's type.
In web interfaces, the workflow just forces the user to select a type first. For Conduit workflows, I think the cleanest approach is to proactively extract and apply type information before processing the request. This will make the implementation a little messier, but the API cleaner.
An alternative is to add more fields to the API, like a "type" field. This makes the implementation cleaner, but the API messier. I think we're better off favoring a cleaner API here.
This change just makes it possible for `DrydockBlueprintEditEngine` to look at the incoming transactions and extract a "type"; it doesn't actually change any behavior.
Test Plan: Performed edits via API, but this change doesn't alter any behavior.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18847
Summary: Ref PHI243. This is a followup to D18822, which added an edit-only `drydock.blueprint.edit`. By modularizing transactions (here) and then adding a "type" transaction (next change) I intend to remove the "edit-only" limitation and make this API method fully functional.
Test Plan: Created and edited blueprints via the web UI. Edited blueprints via the API. Disabled/enabled blueprints (currently web UI only).
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D18845
Summary:
Fixes T13031. "Enormous" changes are basically changes which are too large to hold in memory, although the actual definition we use today is "more than 1GB of change text or `git diff` runs for more than 15 minutes".
If an install configures a Herald content rule like "when content matches /XYZ/, do something" and then a user pushes a 30 GB source file, we can't put it into memory to `preg_match()` it. Currently, the way to handle this case is to write a separate Herald rule that rejects enormous changes. However, this isn't obvious and means the default behavior is unsafe.
Make the default behavior safe by rejecting these changes with a message, similar to how we reject "dangerous" changes (which permanently delete or overwrite history) by default.
Also, change a couple of UI strings from "Enormous" to "Very Large" to reduce ambiguity. See <https://discourse.phabricator-community.org/t/herald-enormous-check/822>.
Test Plan: Changed the definition of "enormous" from 1GB to 1 byte. Pushed a change; got rejected. Allowed enormous changes, pushed, got rejected by a Herald rule. Disabled the Herald rule, pushed, got a clean push. Prevented enormous changes again. Grepped for "enormous" elsewhere in the UI.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: joshuaspence
Maniphest Tasks: T13031
Differential Revision: https://secure.phabricator.com/D18850
Summary:
See PHI273. Ref T13020. After D18777, tasks created directly into the default status (which is common) via the web UI no longer write a "status" transaction.
This is consistent with other applications, and consistent with the API/email behavior for tasks since early 2016. It also improves the consistency of //reading// tasks via the API.
However, it impacted the "Burnup Report" which relies on directly reading these rows to detect task creation. Until this is fixed properly (T1562), synthetically generate the "missing" transactions which this page expects by looking at task creation dates instead.
Specifically, we:
- Generate a fake `status: null -> "open"` transaction for every task by looking at the Task table.
- Go through the transaction list and remove all the legacy `status: null -> "any open status"` transactions. These will only exist for older tasks.
- Merge all our new fake transactions into the list of transactions.
- Continue on as though nothing happened, letting the rendering code continue to operate on legacy-looking data.
I think this will slightly miscount tasks which were created directly into a closed status, but this is very rare, and does not significantly impact the accuracy of this report relative to other known issues (notably, merging closed tasks).
This will also get the wrong result if the default status has changed from an "open" status to a "closed" status at any point, but this is exceptionally bizarre/rare.
Ultimately, T1562 will let us delete all this stuff and disavow its existence.
Test Plan:
- Created some tasks, loaded burnup before/after this patch.
- My local chart looks more accurate afterwards, but the data is super weird (I used `bin/lipsum` to create a huge number of tasks a couple months ago). I'll vet this on `secure`, which has more reasonable data.
Here's my local chart:
{F5356499}
That's what it //should// look like, it's just hard to be confident that nothing else is hiding there.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13020
Differential Revision: https://secure.phabricator.com/D18853
Summary:
Depends on D18851. Ref T13035. After D18819, revision creation transactions may be split into two groups (if prototypes are enabled).
This split means we have two workers. The first worker doesn't publish feed stories or mail; the second one does.
Currently, both workers call `shouldPublishFeedStory()` before they queue, and then again after the daemons pull them out of the queue. However, the answer to this question can change.
Specifically, this happens:
- `arc` creates a revision.
- The first transaction group applies, creating the revision as a draft, and returns `false` from `shouldPublishFeedStory()`, and does not generate related PHIDs. It queues a daemon to send mail, expecting it not to publish a feed story.
- The second transaction group applies, promoting the revision to "needs review". Since the revision has promoted, `shouldPublishFeedStory()` now returns true. This editor generates related PHIDs and queues a daemon task, expecting it to send mail / publish feed.
- A few milliseconds pass.
- The first job gets pulled out of the daemon queue and worked on. It does not have any feed metadata because the object wasn't publishable when the job was queued -- but `shouldPublishFeedStory()` now returns true, so it tries to publish a story without any metadata available. Slightly bad stuff happens (see below).
- The second job gets pulled out of the daemon queue and worked on. This one has metadata and works fine.
The "slightly bad stuff" is that we publish an empty feed story with no references to any objects, then try to push it to hooks and other listeners. Since it doesn't have any references, it fails to load during the "push to external listeners" phase.
This is harmless but clutters the log and doesn't help anything.
Instead, cache the state of "are we publishing a feed story for this object?" when we queue the worker so it can't race.
Test Plan:
- Enabled prototypes.
- Disabled all Herald triggers for Harbormaster build plans.
- Ran `bin/phd debug task` in one window.
- Created a revision in a separate window.
- Before patch: saw "unable to load feed story" errors in the daemon log.
- After patch: no more "unable to load feed story" errors.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13035
Differential Revision: https://secure.phabricator.com/D18852
Summary: Ref T13035. See that task for a description of the issue.
Test Plan:
- Enabled prototypes.
- Disabled all Herald rules that trigger Harbormaster builds.
- Created a new revision.
- Before patch: initial review request email was dropped.
- After patch: initial review request email is sent.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13035
Differential Revision: https://secure.phabricator.com/D18851
Summary:
See PHI262. Fixes T12578. Although this is a bit niche and probably better accomplished through advisory/soft measures ("Add blocking reviewers") in most cases, it isn't difficult to implement and doesn't create any technical or product tension.
If installs write a rule that blocks commits, that will probably also naturally lead them to an "add reviewers" rule anyway.
Also, allow packages to be hit with the typeahead. They're valid reviewers but previously you couldn't write rules against them, for no actual reason.
Test Plan: Used test console to run this against commits, got sensible results for the field value.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12578
Differential Revision: https://secure.phabricator.com/D18839
Summary:
See PHI223. Ref T13024. There's a remaining registration/login order issue after the other changes in T13024: we lose track of the current URI when we go through the MFA flow, so we can lose "Set Password" at the end of the flow.
Specifically, the flow goes like this today:
- User clicks the welcome link in email.
- They get redirected to the "set password" settings panel.
- This gets pre-empted by Legalpad (although we'll potentially survive this with the URI intact).
- This also gets pre-empted by the "Set MFA" workflow. If the user completes this flow, they get redirected to a `/auth/multifactor/?id=123` sort of URI to highlight the factor they added. This causes us to lose the `/settings/panel/password/blah/blah?key=xyz` URI.
The ordering on this is also not ideal; it's preferable to start with a password, then do the other steps, so the user can return to the flow more easily if they are interrupted.
Resolve this by separating the "change your password" and "set/reset your password" flows onto two different pages. This copy/pastes a bit of code, but both flows end up simpler so it feels reasonable to me overall.
We don't require a full session for "set/reset password" (so you can do it if you don't have MFA/legalpad yet) and do it first.
This works better and is broadly simpler for users.
Test Plan:
- Required MFA + legalpad, invited a user via email, registered.
- Before: password set flow got lost when setting MFA.
- After: prompted to set password, then sign documents, then set up MFA.
- Reset password (with MFA confgiured, was required to MFA first).
- Tried to reset password without a valid reset key, wasn't successful.
- Changed password using existing flow.
- Hit various (all?) error cases (short password, common password, mismatch, missing password, etc).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13024
Differential Revision: https://secure.phabricator.com/D18840
Summary:
Ref PHI261. This moves Harbormaster build status to work more similarly to other modern status types, like Differential revision status, where we try to specify as much behavior on the server as possible so that the client and server can vary independently.
(I don't have any specific plans to make Harbormaster build status configurable on the server side, but it isn't out of the question.)
Test Plan: Ran `harbormaster.querybuilds` (saw same data as before) and `harbormaster.build.search` (saw same data as before, plus new ANSI color data).
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D18838
Summary: This probably isn't the best solution, however, it conveniently avoids the bug from T13033. It would probably be more user-friendly (but more difficult to implement) if we allowed either Project Details //or// Workboard to be hidden but not both.
Test Plan: Tested locally, indeed this prevents hiding the menu item.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18836
Summary:
Fixes T13033. Currently (prior to D18836) all the default items for projects can be hidden. When this occurs, the main project page fatals.
If we fix the fatal narrowly (don't try to call `null->getBuiltinKey()` when `$default` is `null`), it would 404, which is a little better but not by much.
After D18836 you can't hide "Profile", which is pretty sensible, and effectively fixes this. This change doubles down: let "Manage" be a default, and send the user there if we can't find a different default.
Ideally, the MenuEngine itself will do more rendering eventually (as it does for some of the newer Home stuff) and could handle this defaulting behavior with less special casing, but we'd still end up in a similar situation if a project had only one "Link" item to "coolcats.com" or something: redirecting the user to "coolcats.com" is probably better than fataling, but not by a huge margin, and not likely to be what they expect.
Test Plan:
Before D18836, disabled both "Profile" and "Workboard" items on a project. Visited project page.
Before patch: fatal. After patch: manage page.
After D18836, you can't do this and just get the profile, so this is sort of moot and mostly future-proofing/for-completeness.
Reviewers: 20after4, amckinley
Reviewed By: amckinley
Maniphest Tasks: T13033
Differential Revision: https://secure.phabricator.com/D18843
Summary: Events with no attendees (e.g. fresh instances of recurring events) would trigger an exception when sending notifications, because `$attendee_map` would still get populated.
Test Plan: Declined event, restarted daemons. Did not see exception. Accepted event, restarted daemons. Saw "[Calendar] [Reminder]" email.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D18835
Summary:
We have one production instance with failing database backups since they recently uploaded a 52MB hunk. The production configuration specifies a 64MB "max_allowed_packet" in `[mysqld]`, but this doesn't apply to `mysqldump` (we'd need to specify it in a separate `[mysqldump]` section) and `mysqldump` runs with an effective limit of the default (16MB).
We could change our production config to specify a value in `[mysqldump]`, but just change it unconditionally at execution time since there's no reason for any user to ever want this command to fail because they have too much data.
Test Plan: Dumped locally, will verify production backup goes through cleanly.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18834
Summary: Ref T13030. See PHI254. This behavior could be cleaner than I've made it, but it fixes the "this is totally broken" issue, replacing a fatal/exception with an informative (just not terribly useful) page.
Test Plan:
- Added a submodule to a repository.
- In Diffusion, clicked some other file next to the submodule, then edited the URI to the submodule path instead.
- Before patch: fatal.
- After patch: relatively useful message about this being a submodule.
Note that it's normally hard to hit this URI directly. In the browse view, submodules are marked up as directories and linked to a separate submodule resolution flow.
{F5321524}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13030
Differential Revision: https://secure.phabricator.com/D18831
Summary:
See PHI230. Currently, we denormalize raw line counts onto diffs and revisions, but not added/removed line counts.
I'd like to try a `[---+ ]` sort of size hint element (see D16322 for more) as a general approach to conveying size information at a glance and see how it feels, since I think the raw size number isn't very scannable/useful and it may be a significant improvement to hint about how much of a change is throwing stuff out vs adding new stuff.
This just makes the data available without any subquerying and doesn't actually change the UI.
Test Plan:
Created a revision, saw detailed change information populate in the database.
```
mysql> select * from differential_revision where id = 292\G
*************************** 1. row ***************************
id: 292
title: WIP
originalTitle: WIP
phid: PHID-DREV-ux3cxptibn3l5pxsug3z
status: draft
summary: asdf
testPlan: asdf
authorPHID: PHID-USER-cvfydnwadpdj7vdon36z
lastReviewerPHID: NULL
lineCount: 41
dateCreated: 1513179418
dateModified: 1513179418
attached: []
mailKey: h4mn6perdio47o4beomyvu75zezwvredx3mbrlgz
branchName: NULL
viewPolicy: users
editPolicy: users
repositoryPHID: PHID-REPO-wif5lutk5gn3y6ursk4p
properties: {"lines.added":40,"lines.removed":1}
activeDiffPHID: PHID-DIFF-ixjphpunpkenqgukpmce
1 row in set (0.00 sec)
```
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18832
Summary:
Depends on D18828. Ref T7789. See <https://discourse.phabricator-community.org/t/git-lfs-fails-with-large-images/584>.
Currently, when you upload a large (>4MB) image, we may try to assess the dimensions for the image and for each individual chunk.
At best, this is slow and not useful. At worst, it fatals or consumes a ton of memory and I/O we don't need to be using.
Instead:
- Don't try to assess dimensions for chunked files.
- Don't try to assess dimensions for the chunks themselves.
- Squelch errors for bad data, etc., that `gd` can't actually read, since we recover sensibly.
Test Plan:
- Created a 2048x2048 PNG in Photoshop using the "Random Noise" filter which weighs 8.5MB.
- Uploaded it.
- Before patch: got complaints in log about `imagecreatefromstring()` failing, although the actual upload went OK in my environment.
- After patch: clean log, no attempt to detect the size of a big image.
- Also uploaded a small image, got dimensions detected properly still.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T7789
Differential Revision: https://secure.phabricator.com/D18830
