Summary: Ref T13238. Warn users about these horrible options and encourage them to defuse them.
Test Plan: Hit both warnings, fixed the issues, issues went away.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13238
Differential Revision: https://secure.phabricator.com/D19999
Summary:
Ref T13222. Currently, if a remote address fails a few login attempts (5) in a short period of time (15 minutes) we require a CAPTCHA for each additional attempt.
This relies on:
- Administrators configuring ReCAPTCHA, which they may just not bother with.
- Administrators being comfortable with Google running arbitrary trusted Javascript, which they may not be comfortable with.
- ReCAPTCHA actually being effective, which seems likely true for unsophisticated attackers but perhaps less true for more sophisticated attackers (see <https://github.com/ecthros/uncaptcha2>, for example).
(For unsophisticated attackers and researchers, "Rumola" has been the standard CAPTCHA bypass tool for some time. This is an extension that pays humans to solve CAPTCHAs for you. This is not practical at "brute force a strong password" scale. Google appears to have removed it from the Chrome store. The "submit the captcha back to Google's APIs" trick probably isn't practical at brute-force-scale either, but it's easier to imagine weaponizing that than weaponizing human solvers.)
Add a hard gate behind the CAPTHCA wall so that we fail into a secure state if there's no CAPTCHA or the attacker can defeat CAPTCHAs at a very low cost.
The big downside to this is that an attacker who controls your remote address (e.g., is behind the same NAT device you're behind on corpnet) can lock you out of your account. However:
- That //should// be a lot of access (although maybe this isn't that high of a barrier in many cases, since compromising a "smart fridge" or "smart water glass" or whatever might be good enough).
- You can still do "Forgot password?" and login via email link, although this may not be obvious.
Test Plan:
- Logged in normally.
- Failed many many login attempts, got hard gated.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19997
Summary:
In ~2012, the first of these options was added because someone who hates dogs and works at Asana also hated `[Differential]` in the subject line. The use case there was actually //removing// the text, not changing it, but I made the prefix editable since it seemed like slightly less of a one-off.
These options are among the dumbest and most useless config options we have and very rarely used, see T11760. A very small number of instances have configured one of these options.
Newer applications stopped providing these options and no one has complained.
You can get the same effect with `translation.override`. Although I'm not sure we'll keep that around forever, it's a reasonable replacement today. I'll call out an example in the changelog to help installs that want to preserve this option.
If we did want to provide this, it should just be in {nav Applications > Settings} for each application, but I think it's wildly-low-value and "hack via translations" or "local patch" are entirely reasonable if you really want to change these strings.
Test Plan: Grepped for `subject-prefix`.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19993
Summary:
Ref T13222. If we receive a message and nothing processes it, we normally try to send the user an error message like "hey, nothing handled this, maybe you got the address wrong".
Just skip the "send them an error message" part if any recipient was reserved, so if you "Reply All" to a message that is "From: noreply@phabricator" you don't get a relatively unhelpful error.
This also makes sure that the "void" address doesn't generate bounces if the "From" is a valid user email address (e.g., with `metamta.can-send-as-user`). That is:
- Phabricator needs to send a mail with only "CC" users.
- Phabricator puts the "void" address in "To" as a placeholder.
- The "void" address happens to route back to Phabricator.
We don't want that mail to bounce to anywhere. Normally, it won't:
- From is usually "noreply@phabricator", which isn't a user, so we won't send anything back: we only send mail to verified user email addresses.
- The message will have "X-Phabricator-Sent-This-Message: true" so we won't process it at all.
...but this is another layer of certainty.
Test Plan: Used `bin/mail receive-test` to receive mail to an invalid, unreserved address (bounce/error email) and an invalid, reserved address (no bounce/error email).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19987
Summary:
See PHI1023. Ref T7607. Occasionally, companies need their billing address (or some other custom text) to appear on invoices to satisfy process or compliance requirements.
Allow accounts to have a custom "Billing Name" and a custom "Billing Address" which appear on invoices.
Test Plan: {F6134707}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T7607
Differential Revision: https://secure.phabricator.com/D19979
Summary:
See PHI1025. When you "Import Columns", we test if you're trying to import into a board that already has columns. However, this test is too broad (it incorrectly detects "proxy" columns for milestones as columns) and not user-friendly (it returns 400 instead of a readable error).
Correct these issues, and refine some of the logic around proxy columns.
Test Plan:
- Created a project, A.
- Created a milestone under that project.
- Imported another project's columns to A's workboard.
- Before change: Unhelpful 400.
- After change: import worked fine.
- Also, hit the new error dialogs and read through them.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19978
Summary:
Fixes T13234. Don't let application email addresses be configured with user addresses. This might prevent an unlikely bit of mischief where someone does this intentionally, detailed in T13234.
(Possibly, these tables should just be merged some day, similar to how the "Password" table is now a shared resource that's modular enough for multiple applications to use it.)
Test Plan: {F6132259}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13234
Differential Revision: https://secure.phabricator.com/D19974
Summary:
Fixes T11782. When you "Close as Duplicate", generate a "[Merged]" email by making the merge the first transaction.
(There are other, more-deterministic ways to do this with action strength, but this is much simpler and I believe it suffices.)
Test Plan: Used "Close as Duplicate", got a "[Merged]" email out of it.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11782
Differential Revision: https://secure.phabricator.com/D19972
Summary:
Fixes T12921. Currently, we call `getTitleForHTMLMail()`, but that calls `getTitleForMail()` which forces us into text rendering mode.
Instead, have `getTitleForHTML/TextMail()` force the rendering mode, then call `getTitleForMail()` with the desired rendering mode.
This causes stories like "epriestely added dependent tasks: x, y." to appear as links in email instead of plain text.
Test Plan: Used `bin/mail show-outbound --id ... --dump-html > out.html` to verify HTML mail.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12921
Differential Revision: https://secure.phabricator.com/D19968
Summary: Ref T13222. Ref T920. This is the last of the upstream adapter updates.
Test Plan:
- Sent mail with SES.
- Sent mail with "sendmail". I don't have sendmail actually configured to an upstream MTA so I'm not 100% sure this worked, but the `sendmail` binary didn't complain and almost all of the code is shared with SES, so I'm reasonably confident this actually works.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222, T920
Differential Revision: https://secure.phabricator.com/D19965
Summary:
Ref T920. Ref T12404.
- Update to the new "$message" API.
- Remove "encoding". I believe "base64" is always the best value for this since we stopped seeing issues once we changed the default.
- Remove "mailer". This is a legacy option that makes little sense given how configuration now works.
- Rename to "SMTP". This doesn't affect users anymore since this mailer has been configured as `smtp` for about a year.
- This does NOT add a timeout since the SMTP code is inside PHPMailer (see T12404).
Test Plan: Sent messages with many mail features via GMail SMTP and SendGrid SMTP.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12404, T920
Differential Revision: https://secure.phabricator.com/D19961
Summary:
See T1024. When "CAN_EDIT" became default in T13186, this was missed as an exception.
Watching shouldn't require "CAN_EDIT", so exempt it.
Test Plan:
- Before change: tried to watch a project I could not edit, got a policy error.
- After change: watched/unwatched a project I could not edit.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19977
Summary:
Ref T920. Ref T5969.
- Update to the new "$message" API.
- Update to Sendgrid v3.
- Add a timeout.
- This removes the "api-user" option, which Sendgrid no longer seems to use.
Test Plan: Sent Sendgrid messages with `bin/mail send-test ...` using subject/headers/attachments/html/to/cc.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: jbrownEP
Maniphest Tasks: T5969, T920
Differential Revision: https://secure.phabricator.com/D19960
Summary: Ref T920. Ref T5969. Update the Mailgun adapter for the API changes and add a timeout.
Test Plan: Configured Mailgun as a mailer, sent mail with subject/to/cc/headers/html/attachments using `bin/mail send-test`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T5969, T920
Differential Revision: https://secure.phabricator.com/D19959
Summary: Depends on D19956. Ref T920. Move the TestAdapter to the new API and adjust a couple of tests for the changes.
Test Plan: All tests now pass.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T920
Differential Revision: https://secure.phabricator.com/D19957
Summary:
Depends on D19955. Ref T920. Ref T5969. Update Postmark to accept new Message objects. Also:
- Update the inbound whitelist.
- Add a little support for `media` configuration.
- Add a service call timeout (see T5969).
- Drop the needless word "Implementation" from the Adapter class tree. I could call these "Mailers" instead of "Adapters", but then we get "PhabricatorMailMailer" which feels questionable.
Test Plan: Used `bin/mail send-test` to send mail via Postmark with various options (mulitple recipients, text vs html, attachments).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T5969, T920
Differential Revision: https://secure.phabricator.com/D19956
Summary:
Depends on D19954. Ref T920. This is a step toward a world where "Mailers" are generic and may send messages over a broader array of channels (email, SMS, postal mail).
There are a few major parts here:
- Instead of calling `$mailer->setSubject()`, `$mailer->setFrom()`, etc., build in intermediate `$message` object first, then pass that to the mailer.
- This breaks every mailer! This change on its own does not fix them. I plan to fix them in a series of "update mailer X", "update mailer Y" followups.
- This generally makes the API easier to change in the far future, and in the near future supports mailers accepting different types of `$message` objects with the same API.
- Pull the "build an email" stuff out into a `PhabricatorMailEmailEngine`. `MetaMTAMail` is already a huge object without also doing this translation step. This is just a separation/simplification change, but also tries to fight against `MetaMTAMail` getting 5K lines of email/sms/whatsapp/postal-mail code.
- Try to rewrite the "build an email" stuff to be a bit more straightforward while making it generate objects. Prior to this change, it had this weird flow:
```lang=php
foreach ($properties as $key => $prop) {
switch ($key) {
case 'xyz':
// ...
}
}
```
This is just inherently somewhat hard to puzzle out, and it means that processing order depends on internal property order, which is quite surprising.
Test Plan: This breaks everything on its own; adapters must be updated to use the new API. See followups.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T920
Differential Revision: https://secure.phabricator.com/D19955
Summary:
Depends on D19952. Ref T13222. Never process mail targets if they match:
- The "default" address which we send mail "From".
- The "void" address which we use as a placholder "To" when we only have "CC" addresses.
- Any address from a list of reserved/administrative names.
The first two prevent loops. The third one prevents abuse.
There's a reasonably well-annotated list of reservations and reasons here:
https://webmasters.stackexchange.com/questions/104811/is-there-any-list-of-email-addresses-reserved-because-of-security-concerns-for-a
Stuff like `support@` seems fine; stuff like `ssladmin@` might let you get SSL certs issued for a domain you don't control.
Also, forbid users from creating application emails with these reserved addresses.
Finally, build the default and void addresses somewhat more cleverly.
Test Plan: Added unit tests, tried to configured reserved addresses, hit the default/void cases manually with `bin/mail receive-test`.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: olexiy.myronenko
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19953
Summary:
Fixes T7477. Fixes T13066. Currently, inbound mail is processed by the first receiver that matches any "To:" address. "Cc" addresses are ignored.
**To, CC, and Multiple Receivers**
Some users would like to be able to "Cc" addresses like `bugs@` instead of having to "To" the address, which makes perfect sense. That's the driving use case behind T7477.
Since users can To/Cc multiple "create object" or "update object" addresses, I also wanted to make the behavior more general. For example, if you email `bugs@` and also `paste@`, your mail might reasonably make both a Task and a Paste. Is this useful? I'm not sure. But it seems like it's pretty clearly the best match for user intent, and the least-surprising behavior we can have. There's also no good rule for picking which address "wins" when two or more match -- we ended up with "address order", which is pretty arbitrary since "To" and "Cc" are not really ordered fields.
One part of this change is removing `phabricator.allow-email-users`. In practice, this option only controlled whether users were allowed to send mail to "Application Email" addresses with a configured default author, and it's unlikely that we'll expand it since I think the future of external/grey users is Nuance, not richer interaction with Maniphest/Differential/etc. Since this option only made "Default Author" work and "Default Author" is optional, we can simplify behavior by making the rule work like this:
- If an address specifies a default author, it allows public email.
- If an address does not, it doesn't.
That's basically how it worked already, except that you could intentionally "break" the behavior by not configuring `phabricator.allow-email-users`. This is a backwards compatility change with possible security implications (it might allow email in that was previously blocked by configuration) that I'll call out in the changelog, but I suspect that no installs are really impacted and this new behavior is generally more intuitive.
A somewhat related change here is that each receiver is allowed to react to each individual email address, instead of firing once. This allows you to configure `bugs-a@` and `bugs-b@` and CC them both and get two tasks. Useful? Maybe not, but seems like the best execution of intent.
**Sender vs Author**
Adjacently, T13066 described an improvement to error handling behavior here: we did not distinguish between "sender" (the user matching the email "From" address) and "actor" (the user we're actually acting as in the application). These are different when you're some internet rando and send to `bugs@`, which has a default author. Then the "sender" is `null` and the "author" is `@bugs-robot` or whatever (some user account you've configured).
This refines "Sender" vs "Author". This is mostly a purity/correctness change, but it means that we won't send random email error messages to `@bugs-robot`.
Since receivers are now allowed to process mail with no "sender" if they have some default "actor" they would rather use instead, it's not an error to send from an invalid address unless nothing processes the mail.
**Other**
This removes the "abundant receivers" error since this is no longer an error.
This always sets "external user" mail recipients to be unverified. As far as I can tell, there's no pathway by which we send them email anyway (before or after this change), although it's possible I'm missing something somewhere.
Test Plan:
I did most of this with `bin/mail receive-test`. I rigged the workflow slightly for some of it since it doesn't support multiple addresses or explicit "CC" and adding either would be a bit tricky.
These could also be tested with `scripts/mail/mail_handler.php`, but I don't currently have the MIME parser extension installed locally after a recent upgrade to Mojave and suspect T13232 makes it tricky to install.
- Ran unit tests, which provide significant coverage of this flow.
- Sent mail to multiple Maniphest application emails, got multiple tasks.
- Sent mail to a Maniphest and a Paste application email, got a task and a paste.
- Sent mail to a task.
- Saw original email recorded on tasks. This is a behavior particular to tasks.
- Sent mail to a paste.
- Sent mail to a mock.
- Sent mail to a Phame blog post.
- Sent mail to a Legalpad document.
- Sent mail to a Conpherence thread.
- Sent mail to a poll.
- This isn't every type of supported object but it's enough of them that I'm pretty confident I didn't break the whole flow.
- Sent mail to an object I could not view (got an error).
- As a non-user, sent mail to several "create an object..." addresses.
- Addresses with a default user worked (e.g., created a task).
- Addresses without a default user did not work.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13066, T7477
Differential Revision: https://secure.phabricator.com/D19952
Summary:
Ref T13222. Ref T13231. See PHI912. I'm planning to turn MFA providers into concrete objects, so you can disable and configure them.
Currently, we only support TOTP, which doesn't require any configuration, but other provider types (like Duo or Yubikey OTP) do require some configuration (server URIs, API keys, etc). TOTP //could// also have some configuration, like "bits of entropy" or "allowed window size" or whatever, if we want.
Add concrete objects for this and standard transaction / policy / query support. These objects don't do anything interesting yet and don't actually interact with MFA, this is just skeleton code for now.
Test Plan:
{F6090444}
{F6090445}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13231, T13222
Differential Revision: https://secure.phabricator.com/D19935
Summary:
D19940 removed this file entirely, which has led to at least one user who was unsure how to proceed now that `cluster.mailers` is required for outbound mail: https://discourse.phabricator-community.org/t/invalid-argument-supplied-for-foreach-phabricatormetamtamail-php/2287
This isn't //always// a setup issue for installs that don't care about sending mail, but this at least this gives a sporting chance to users who don't follow the changelogs.
Also, I'm not sure if there's a way to use `pht()` to generate links; right now the phurl is just in plain text.
Test Plan: Removed `cluster.mailers` config; observed expected setup issue.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D19964
Summary: See PHI1017. This is a trivial fix even though these burnups are headed toward a grisly fate.
Test Plan: Moused over some January datapoints, saw "1" instead of "0".
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19967
Summary:
Depends on D19953. Ref T9141. We have a "MetaMTAAttachment" object, rename it to "MailAttachment".
Also add a "Header" object and an "EmailMessage" object. Currently, mail adapters have a large number of methods like `setSubject()`, `addTo()`, etc, that I would like to remove.
I'd like the API to be more like `sendMessage(PhabricatorMailExternalMessage $message)`. This is likely a significant simplification anyway, since the implementations of all these methods are just copy/pasted boilerplate anyway (lots of `$this->subject = $subject;`) and this will let Adapters support other message media (SMS, APNS, Whatsapp, etc.)
That's a larger change, but move toward a world where we can build a concrete `$message` object for "email" or "sms".
The `PhabricatorMailEmailMessage` object is just a dumb, flat object representation of the information an adapter needs to actually send mail. The existing `PhabricatorMetaMTAMail` is a much more complex object and has a lot of rich data (delivery status, related object PHIDs, etc) and is a storage object.
The new flow will be something like:
- `PhabricatorMetaMTAMail` (possibly renamed) is the storage object for any outbound message on this channel. It tracks message content, acceptable delivery media (SMS vs email), delivery status, related objects, has a PHID, and has a daemon worker associated with delivering it.
- It builds a `PhabricatorMailExternalMessage`, which is a simple, flat description of the message it wants to send. The subclass of this object depends on the message medium. For email, this will be an `EmailMessage`. This is just a "bag of strings" sort of object, with appropriate flattened values for the adapter to work with (e.g., Email has email addresses, SMS has phone numbers).
- It passes the `ExternalMessage` (which is a `MailMessage` or `SMSMessage` or whatever) to the adapter.
- The adapter reads the nice flat properties off it and turns them into an API request, SMTP call, etc.
This is sort of how things work today anyway. The major change is that I want to hand off a "bag of strings" object instead of calling `setX()`, `setY()`, `setZ()` with each individual value.
Test Plan: Grepped for `MetaMTAAttachment`. This doesn't change any behavior yet.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T9141
Differential Revision: https://secure.phabricator.com/D19954
Summary:
Ref T7477. The various "create a new X via email" applications (Paste, Differential, Maniphest, etc) all have a bunch of duplicate code.
The inheritance stack here is generally a little weird. Extend these from a shared parent to reduce the number of callsites I need to change when this API is adjusted for T7477.
Test Plan: Ran unit tests. This will get more thorough testing once more pieces are in place.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T7477
Differential Revision: https://secure.phabricator.com/D19950
Summary:
See PHI1014. We may not have Identities if you race the import pipeline, or in some other cases which are more "bug" / "missing migration"-flavored.
Load the commit data so we can fall back to it if we don't have identities.
Test Plan:
- Wiped out all my identities with `UPDATE ... SET authorIdentityPHID = NULL WHERE ...`.
- Before change: blame fataled with `Attempting to access attached data on PhabricatorRepositoryCommit (via getCommitData()), but the data is not actually attached.`.
- After change: blame falls back gracefully.
- Restored identities with `bin/repository rebuild-identities`, checked blame again.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19958
Summary:
Ref T7477. This option was added in D842 in 2011, to support a specific narrow use case at Quora with community moderators using some kind of weird Gmail config.
I don't recall it ever coming up since then, and a survey of a subset of hosted instances (see T11760) reveals that no instances are using this option today. Presumably, even Quora has completed the onboarding discussed in D842, if they still use Phabricator. This option generally does not seem very useful outside of very unusual/narrow cases like the one Quora had.
This would be relatively easy to restore as a local patch if installs //do// need it, but I suspect this has no use cases anywhere.
Test Plan: Grepped for option, blame-delved to figure out why we added it in the first place, surveyed instances for usage.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T7477
Differential Revision: https://secure.phabricator.com/D19949
Summary:
Ref T7477. We have some address normalization code in the reciever stack that is really shared code. I want to introduce some new callsites elsewhere but don't want to put a lot of static calls to other random objects all over the place.
This technically "solves" T7477 (it changes "to" to "to + cc" for finding receivers) but doesn't yet implement proper behavior (with multiple receivers, for example).
Test Plan: Ran unit tests, which cover this pretty well. Additional changes will vet this more thoroughly.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T7477
Differential Revision: https://secure.phabricator.com/D19948
Summary:
Ref T920. Ref T7477. We currently drop empty mail only once it reaches the `ReplyHandler` layer.
I think no plausible receiver can ever do anything useful with this kind of mail, so we can safely drop it earlier and simplify some of the logic. After T7477, we'd end up throwing multiple exceptions if you sent empty mail to several valid receivers.
(I also want to move away from APIs oriented around raw addresses in more specialized layers, and this is one of the few callsites for raw mail address information.)
This requires updating some unit tests to actually have message bodies, since they failed with this error before hitting the other errors otherwise.
Test Plan: Used `bin/mail receive-test` to send empty mail, got appropriate "err:empty" out of it.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T7477, T920
Differential Revision: https://secure.phabricator.com/D19947
Summary:
Ref T12509.
- Remove the "phabricator.csrf-key" configuration option in favor of automatically generating an HMAC key.
- Upgrade two hasher callsites (one in CSRF itself, one in providing a CSRF secret for logged-out users) to SHA256.
- Extract the CSRF logic from `PhabricatorUser` to a standalone engine.
I was originally going to do this as two changes (extract logic, then upgrade hashes) but the logic had a couple of very silly pieces to it that made faithful extraction a little silly.
For example, it computed `time_block = (epoch + (offset * cycle_frequency)) / cycle_frequency` instead of `time_block = (epoch / cycle_frequency) + offset`. These are equivalent but the former was kind of silly.
It also computed `substr(hmac(substr(hmac(secret)).salt))` instead of `substr(hmac(secret.salt))`. These have the same overall effect but the former is, again, kind of silly (and a little bit materially worse, in this case).
This will cause a one-time compatibility break: pages loaded before the upgrade won't be able to submit contained forms after the upgrade, unless they're open for long enough for the Javascript to refresh the CSRF token (an hour, I think?). I'll note this in the changelog.
Test Plan:
- As a logged-in user, submitted forms normally (worked).
- As a logged-in user, submitted forms with a bad CSRF value (error, as expected).
- As a logged-out user, hit the success and error cases.
- Visually inspected tokens for correct format.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12509
Differential Revision: https://secure.phabricator.com/D19946
Summary:
Ref T12509.
- Upgrade an old SHA1 to SHA256.
- Replace an old manually configurable HMAC key with an automatically generated one.
This is generally both simpler (less configuration) and more secure (you now get a unique value automatically).
This causes a one-time compatibility break that invalidates old "Reply-To" addresses. I'll note this in the changelog.
If you leaked a bunch of addresses, you could force a change here by mucking around with `phabricator_auth.auth_hmackey`, but AFAIK no one has ever used this value to react to any sort of security issue.
(I'll note the possibility that we might want to provide/document this "manually force HMAC keys to regenerate" stuff some day in T6994.)
Test Plan: Grepped for removed config. I'll vet this pathway more heavily in upcoming changes.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12509
Differential Revision: https://secure.phabricator.com/D19945
Summary:
Ref T920. This simplifies mail configuration.
The "metamta.domain" option is only used to generate Thread-ID values, and we just need something that looks like a bit like a domain in order to make GMail happy. Just use the install domain. In most cases, this is almost certainly the configured value anyway. In some cases, this may cause a one-time threading break for existing threads; I'll call this out in the changelog.
The "metamta.placeholder-to-recipient" is used to put some null value in "To:" when a mail only has CCs. This is so that if you write a local client mail rule like "when I'm in CC, burn the message in a fire" it works even if all the "to" addresses have elected not to receive the mail. Instead: just send it to an unlikely address at our own domain.
I'll add some additional handling for the possiblity that we may receive this email ourselves in the next change, but it overlaps with T7477.
Test Plan: Grepped for these configuration values.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T920
Differential Revision: https://secure.phabricator.com/D19942
Summary:
Ref T12509. This upgrades a `weakDigest()` callsite to SHA256-HMAC and removes three config options:
- `celerity.resource-hash`: Now hard-coded, since the use case for ever adjusting it was very weak.
- `celerity.enable-deflate`: Intended to make cache inspection easier, but we haven't needed to inspect caches in ~forever.
- `celerity.minify`: Intended to make debugging minification easier, but we haven't needed to debug this in ~forever.
In the latter two cases, the options were purely developer-focused, and it's easy to go add an `&& false` somewhere in the code if we need to disable these features to debug something, but the relevant parts of the code basically work properly and never need debugging. These options were excessively paranoid, based on the static resource enviroment at Facebook being far more perilous.
The first case theoretically had end-user utility for fixing stuck content caches. In modern Phabricator, it's not intuitive that you'd go adjust a Config option to fix this. I don't recall any users ever actually running into problems here, though.
(An earlier version of this change did more magic with `celerity.resource-hash`, but this ended up with a more substantial simplification.)
Test Plan: Grepped for removed configuration options.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12509
Differential Revision: https://secure.phabricator.com/D19941
Summary: Ref T920. We currently embed the Twilio PHP API, but can replace it with about 100 lines of code and get a future-oriented interface as a bonus. Add a Future so we can move toward a simpler calling convention for the API.
Test Plan: Used this future to send SMS messages via the Twilio API.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T920
Differential Revision: https://secure.phabricator.com/D19937
Summary:
Ref T13222. In D19918, I refactored how timelines get "view data". Today, this is always additional data about which images/changesets/diffs are visible on the current revision/commit/mock, so we can tell if inline comments should be linked to a `#anchor` on the same page (if the inline is rendered there somewhere) or to a `/D123?id=1&vs=2` full link on a different page (if it isn't), but in general this could be any sort of state information about the current page that affects how the timeline should render.
Previously, comment previews did not use any specialized object code and always rendered a "generic" timeline story. This was actually a bug, but none of the code we have today cares about this (since it's all inline related, and inlines render separately) so it never impacted anything.
After the `TimelineEngine` change, the preview renders with Differential-specific code. This is more correct, but we were not passing the preview the "view data" so it broke.
This preview doesn't actually need the view data and we could just make it bail out if it isn't present, but pass it through for consistency and so this works like we'd expect if we do something fancier with view data in the future.
Test Plan: Viewed comment and inline comment previews in Differential, saw old behavior restored.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19943
Summary:
Ref T920. About a year ago (in 2018 Week 6, see D19003) we moved from individually configured mailers to `cluster.mailers`, primarily to support fallback across multiple mail providers.
Since this has been stable for quite a while, drop support for the older options.
Test Plan: Grepped for all removed options.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T920
Differential Revision: https://secure.phabricator.com/D19940
Summary:
Ref T920. Over time, mail has become much more complex and I think considering "mail", "sms", "postcards", "whatsapp", etc., to be mostly-the-same is now a more promising avenue than building separate stacks for each one.
Throw away all the standalone SMS code, including the Twilio config options. I have a separate diff that adds Twilio as a mail adapter and functions correctly, but it needs some more work to bring upstream.
This permanently destroys the `sms` table, which no real reachable code ever wrote to. I'll call this out in the changelog.
Test Plan:
- Grepped for `SMS` and `Twilio`.
- Ran storage upgrade.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T920
Differential Revision: https://secure.phabricator.com/D19939
Summary: See T12447 for discussion. It is reasonably intuitive to try and pass Conduit parameters via a JSON-encoded HTTP body, but if you do so, you'll get an unhelpful messsage about how method so-and-so does not accept a parameter named "your_entire_json_body". Instead, detect this mistake and advise developers to use form-encoded parameters.
Test Plan:
Got a better error when attempting to make Conduit calls from React code. Tested the following additional invocations of Conduit and got the expected results without an error:
* From the Conduit UI
* With cURL:
```
~ $ curl http://local.phacility.com:8080/api/conpherence.querythread \
> -d api.token=api-tvv2zb565zrtueab5ddprmpxvrwb \
> -d ids[0]=1
```
* With `arc call-conduit`:
```
~ $ echo '{
> "ids": [
> 1
> ]
> }' | arc call-conduit --conduit-uri http://local.phacility.com:8080/ --conduit-token api-tvv2zb565zrtueab5ddprmpxvrwb conpherence.querythread
```
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D19944
Summary: Ref T920. Bumped into this while looking at SMS support.
Test Plan: Loaded `/mail/`, no more `qsprintf()` warning.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T920
Differential Revision: https://secure.phabricator.com/D19936
Summary:
Depends on D19908. Ref T13222. In D19897, I reordered some transaction code and affected the call order of `willApplyTransactions()`.
It turns out that we use this call for only one thing, and that thing is pretty silly: naming the raw paste data file when editing paste content.
This is only user-visible in the URL when you click "View Raw Paste" and seems exceptionally low-value, so remove the hook and pick a consistent name for the paste datafiles. (We could retain the name behavior in other ways, but it hardly seems worthwhile.)
Test Plan:
- Created and edited a paste.
- Grepped for `willApplyTransactions()`.
Note that `EditEngine` (vs `ApplicationTransacitonEditor`) still has a `willApplyTransactions()`, which has one callsite in Phabricator (in Calendar) and a couple in Instances. That's untouched and still works.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19909
Summary:
Depends on D19906. Ref T13222. This isn't going to win any design awards, but make the "wait" and "answered" elements a little more clear.
Ideally, the icon parts could be animated Google Authenticator-style timers (but I think we'd need to draw them in a `<canvas />` unless there's some clever trick that I don't know) or maybe we could just have the background be like a "water level" that empties out. Not sure I'm going to actually write the JS for either of those, but the UI at least looks a little more intentional.
Test Plan:
{F6070914}
{F6070915}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19908
Summary:
Depends on D19905. Ref T13222. In D19843, I refactored this stuff but `$jump_into_hisec` was dropped.
This is a hint to keep the upgraded session in hisec mode, which we need to do a password reset when using a recovery link. Without it, we double prompt you for MFA: first to upgrade to a full session, then to change your password.
Pass this into the engine properly to avoid the double-prompt.
Test Plan:
- Used `bin/auth recover` to get a partial session with MFA enabled and a password provider.
- Before: double MFA prompt.
- After: session stays upgraded when it becomes full, no second prompt.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19906
Summary:
Depends on D19904. Ref T13226. Ref T13222. Currently, partial sessions (where you've provided a primary auth factor like a password, but not yet provided MFA) work like normal sessions: they're good for 30 days and extend indefinitely under regular use.
This behavior is convenient for full sessions, but normal users don't ever spend 30 minutes answering MFA, so there's no real reason to do it for partial sessions. If we add login alerts in the future, limiting partial sessions to a short lifetime will make them more useful, since an attacker can't get one partial session and keep extending it forever while waiting for an opportunity to get past your MFA.
Test Plan:
- Did a partial login (to the MFA prompt), checked database, saw a ~29 minute partial session.
- Did a full login, saw session extend to ~30 days.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13226, T13222
Differential Revision: https://secure.phabricator.com/D19905
Summary:
Depends on D19903. Ref T13222. This was a Facebook-specific thing from D6202 that I believe no other install ever used, and I'm generally trying to move away from the old "event" system (the more modern modular/engine patterns generally replace it).
Just drop support for this. Since the constant is being removed, anything that's actually using it should break in an obvious way, and I'll note this in the changelog.
There's no explicit replacement but I don't think this hook is useful for anything except "being Facebook in 2013".
Test Plan:
- Grepped for `TYPE_AUTH_WILLLOGIN`.
- Logged in.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19904
Summary:
Depends on D19902. Ref T13222. This is mostly a "while I'm in here..." change since MFA is getting touched so much anyway.
Doing cluster support, I sometimes need to log into user accounts on instances that have MFA. I currently accomplish this by doing `bin/auth recover`, getting a parital session, and then forcing it into a full session in the database. This is inconvenient and somewhat dangerous.
Instead, allow `bin/auth recover` to generate a link that skips the "partial session" stage: adding required MFA, providing MFA, and signing legalpad documents.
Anyone who can run `bin/auth recover` can do this anyway, this just reduces the chance I accidentally bypass MFA on the wrong session when doing support stuff.
Test Plan:
- Logged in with `bin/auth recover`, was prompted for MFA.
- Logged in with `bin/auth recover --force-full-session`, was not prompted for MFA.
- Did a password reset, followed reset link, was prompted for MFA.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19903
Summary:
Depends on D19901. Ref T13222. See PHI873. Currently, the MFA code and the (older, not-really-transactional) token code don't play nicely.
In particular, if the Editor throws we tend to get half an effect applied.
For now, just make this work. Some day it could become more modern so that the transaction actually applies the write.
Test Plan: Awarded and rescinded tokens from an MFA-required object.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19902
Summary:
Depends on D19900. Ref T13222. See PHI873. When an object requires MFA, we currently require MFA for every transaction.
This includes some ambiguous cases like "unsubscribe", but also includes "mention", which seems like clearly bad behavior.
Allow an "MFA" object to be the target of mentions, "edit child tasks", etc.
Test Plan:
- Mentioned an MFA object elsewhere (no MFA prompt).
- Made an MFA object a subtask of a non-MFA object (no MFA prompt).
- Tried to edit an MFA object normally (still got an MFA prompt).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19901
Summary:
Depends on D19899. Ref T13222. When we prompt you for one-shot MFA, we currently give you a lot of misleading text about your session staying in "high security mode".
Differentiate between one-shot and session upgrade MFA, and give the user appropriate cues and explanatory text.
Test Plan:
- Hit one-shot MFA on an "mfa" task in Maniphest.
- Hit session upgrade MFA in Settings > Multi-Factor.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19900
Summary:
Depends on D19898. Ref T13222. See PHI873. Allow objects to opt into an "MFA is required for all edits" mode.
Put tasks in this mode if they're in a status that specifies it is an `mfa` status.
This is still a little rough for now:
- There's no UI hint that you'll have to MFA. I'll likely add some hinting in a followup.
- All edits currently require MFA, even subscribe/unsubscribe. We could maybe relax this if it's an issue.
Test Plan:
- Edited an MFA-required object via comments, edit forms, and most/all of the extensions. These prompted for MFA, then worked correctly.
- Tried to edit via Conduit, failed with a reasonably comprehensible error.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19899
Summary:
Depends on D19897. Ref T13222. See some discussion in D19890.
- Only rate limit users if they're actually answering a challenge, not if they're just clicking "Wait Patiently".
- Reduce the number of allowed attempts per hour from 100 back to 10.
- Reduce the TOTP window from +/- 2 timesteps (allowing ~60 seconds of skew) to +/- 1 timestep (allowing ~30 seconds of skew).
- Change the window where a TOTP response remains valid to a flat 60 seconds instead of a calculation based on windows and timesteps.
Test Plan:
- Hit an MFA prompt.
- Without typing in any codes, mashed "submit" as much as I wanted (>>10 times / hour).
- Answered prompt correctly.
- Mashed "Wait Patiently" as much as I wanted (>>10 times / hour).
- Guessed random numbers, was rate limited after 10 attempts.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19898
Summary:
Depends on D19896. Ref T13222. See PHI873. Add a core "Sign With MFA" transaction type which prompts you for MFA and marks your transactions as MFA'd.
This is a one-shot gate and does not keep you in MFA.
Test Plan:
- Used "Sign with MFA", got prompted for MFA, answered MFA, saw transactions apply with MFA metadata and markers.
- Tried to sign alone, got appropriate errors.
- Tried to sign no-op changes, got appropriate errors.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19897
Summary:
See <https://discourse.phabricator-community.org/t/how-to-get-total-time-spent-on-a-task-in-minutes-or-hours/2241>.
Phrequent has two nearly-identical copies of its rendering code: one for old "property event" objects and one for newer "curtain" objects. In the upstream, both trackable object types (tasks and revisions) use curtains, so throw away the old code since it isn't reachable. Third-party trackable objects can update to the curtain UI, but it's unlikely they exist.
Render the remaining curtain UI with more precision, so we show "Time Spent: 2d, 11h, 49m" instead of "Time Spent: 2d".
Test Plan: {F6074404}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19927
Summary:
Depends on D19928. See <https://discourse.phabricator-community.org/t/firehose-webhook-not-working-with-self-hosted-requestbin-instance/2240/>.
Currently, we report "hook" and "silent", which are raw internal codes.
Instead, report human-readable labels so the user gets a better hint about what's going on ("In Silent Mode").
Also, render a "hey, you're in silent mode so none of this will work" reminder banner in this UI.
Test Plan:
{F6074421}
Note:
- New warning banner.
- Table has more human-readable text ("In Silent Mode").
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19929
Summary: Ref T11351. We only query for images by PHID or by Mock, so the only key we need for now is `<mockPHID>`.
Test Plan: Ran `bin/storage upgrade`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19934
Summary:
Depends on D19924. Ref T11351. Like in D19924, apply these transactions by accepting PHIDs instead of objects so we don't need to juggle the `Image` objects down to PHIDs in `applyInitialEffects`.
(Validation is a little light here for now, but only first-party code can reach this, and you can't violate policies or do anything truly bad even if you could pick values to feed in here.)
Test Plan: Created and edited Mocks; added, removed, and reordered images in a Pholio Mock.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19926
Summary:
Depends on D19923. Ref T11351. Currently, this transaction takes an `Image` as the `newValue` and uses some magic to reduce it into PHIDs by the time we're done.
This creates some problems today where I'd like to get rid of `applyInitialEffects` for MFA code. In the future, it creates a problem becuase there's no way to pass an entire `Image` object over the API.
Instead, create the `Image` first, then just provide the PHID. This is generally simpler, will work well with the API in the future, and stops us from needing any `applyInitialEffects` stuff.
Test Plan: Replaced images in a Pholio mock.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19924
Summary: Depends on D19922. Ref T11351. These query classes have some slightly weird behavior, including `public static function loadImages(...)`. Convert all this stuff into more standard query patterns.
Test Plan: Grepped for callsites, browsed around in Pholio.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19923
Summary: Depends on D19921. Ref T11351. Ref T13065. Update Pholio to use the shared mail infrastructure. See D19670 for a previous change in this vein.
Test Plan: Ran upgrade, spot-checked that everything made it into the new table alive.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13065, T11351
Differential Revision: https://secure.phabricator.com/D19922
Summary:
Depends on D19920. Ref T11351. Currently, "images" and "all images" are attached to Mocks separately, and `getImages()` gets you only some images.
Clean this up slightly:
- One attach method; attach everything.
- Two getters, one for "images" (returns all images); one for "active images" (returns active images).
Test Plan: Browsed around Pholio without any apparent behavioral changes.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19921
Summary:
Depends on D19919. Ref T11351. This method appeared in D8802 (note that "get...Object" was renamed to "get...Transaction" there, so this method was actually "new" even though a method of the same name had existed before).
The goal at the time was to let Harbormaster post build results to Diffs and have them end up on Revisions, but this eventually got a better implementation (see below) where the Harbormaster-specific code can just specify a "publishable object" where build results should go.
The new `get...Object` semantics ultimately broke some stuff, and the actual implementation in Differential was removed in D10911, so this method hasn't really served a purpose since December 2014. I think that broke the Harbormaster thing by accident and we just lived with it for a bit, then Harbormaster got some more work and D17139 introduced "publishable" objects which was a better approach. This was later refined by D19281.
So: the original problem (sending build results to the right place) has a good solution now, this method hasn't done anything for 4 years, and it was probably a bad idea in the first place since it's pretty weird/surprising/fragile.
Note that `Comment` objects still have an unrelated method with the same name. In that case, the method ties the `Comment` storage object to the related `Transaction` storage object.
Test Plan: Grepped for `getApplicationTransactionObject`, verified that all remaining callsites are related to `Comment` objects.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19920
Summary:
Depends on D19918. Ref T11351. In D19918, I removed all calls to this method. Now, remove all implementations.
All of these implementations just `return $timeline`, only the three sites in D19918 did anything interesting.
Test Plan: Used `grep willRenderTimeline` to find callsites, found none.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19919
Summary:
Depends on D19914. Ref T11351. Some of the Phoilo rabbit holes go very deep.
`PhabricatorApplicationTransactionInterface` currently requires you to implement `willRenderTimeline()`. Almost every object just implements this as `return $timeline`; only Pholio, Diffusion, and Differential specialize it. In all cases, they are specializing it mostly to render inline comments.
The actual implementations are a bit of a weird mess and the way the data is threaded through the call stack is weird and not very modern.
Try to clean this up:
- Stop requiring `willRenderTimeline()` to be implemented.
- Stop requiring `getApplicationTransactionViewObject()` to be implemented (only the three above, plus Legalpad, implement this, and Legalpad's implementation is a no-op). These two methods are inherently pretty coupled for almost any reasonable thing you might want to do with the timeline.
- Simplify the handling of "renderdata" and call it "View Data". This is additional information about the current view of the transaction timeline that is required to render it correctly. This is only used in Differential, to decide if we can link an inline comment to an anchor on the same page or should link it to another page. We could perhaps do this on the client instead, but having this data doesn't seem inherently bad to me.
- If objects want to customize timeline rendering, they now implement `PhabricatorTimelineInterface` and provide a `TimelineEngine` which gets a nice formal stack.
This leaves a lot of empty `willRenderTimeline()` implementations hanging around. I'll remove these in the next change, it's just going to be deleting a couple dozen copies of an identical empty method implementation.
Test Plan:
- Viewed audits, revisions, and mocks with inline comments.
- Used "Show Older" to page a revision back in history (this is relevant for "View Data").
- Grepped for symbols: willRenderTimeline, getApplicationTransactionViewObject, Legalpad classes.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19918
Summary:
Ref T11351. In Pholio, we currently use a `mockID`, but a `mockPHID` is generally preferable / more modern / more flexible. In particular, we need PHIDs to load handles and prefer PHIDs when exposing information to the API, and using PHIDs internally makes a bunch of things easier/better/faster and ~nothing harder/worse/slower.
I'll add some inlines about a few things.
Test Plan: Ran migrations, spot-checked database for sanity. Loaded Pholio, saw data unchanged. Created and edited images.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19914
Summary: Depends on D19895. Ref T13222. This is a simple behavioral improvement for the current MFA implementation in Legalpad: don't MFA the user and //then// realize that they forgot to actually check the box.
Test Plan:
- Submitted form without the box checked, got an error saying "check the box" instead of MFA.
- Submitted the form with the box checked, got an MFA prompt.
- Passed the MFA gate, got a signed form.
- Tried to sign another form, hit MFA timed lockout.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19896
Summary:
Depends on D19894. Ref T13222. See PHI873. When you provide a correct response to an MFA challenge, we mark it as "answered".
Currently, we never let you reuse an "answered" token. That's usually fine, but if you have 2+ factors on your account and get one or more (but fewer than all of them) right when you submit the form, you need to answer them all again, possibly after waiting for a lockout period. This is needless.
When you answer a challenge correctly, add a hidden input with a code proving you got it right so you don't need to provide another answer for a little while.
Why not just put your response in a form input, e.g. `<input type="hidden" name="totp-response" value="123456" />`?
- We may allow the "answered" response to be valid for a different amount of time than the actual answer. For TOTP, we currently allow a response to remain valid for 60 seconds, but the actual code you entered might expire sooner.
- In some cases, there's no response we can provide (with push + approve MFA, you don't enter a code, you just tap "yes, allow this" on your phone). Conceivably, we may not be able to re-verify a push+approve code if the remote implements one-shot answers.
- The "responseToken" stuff may end up embedded in normal forms in some cases in the future, and this approach just generally reduces the amount of plaintext MFA we have floating around.
Test Plan:
- Added 2 MFA tokens to my account.
- Hit the MFA prompt.
- Provided one good response and one bad response.
- Submitted the form.
- Old behavior: good response gets locked out for ~120 seconds.
- New behavior: good response is marked "answered", fixing the other response lets me submit the form.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19895
Summary:
Depends on D19893. Ref T13222. See PHI873. A challenge is "answered" if you provide a valid response. A challenge is "completed" if we let you through the MFA check and do whatever actual action the check is protecting.
If you only have one MFA factor, challenges will be "completed" immediately after they are "answered". However, if you have two or more factors, it's possible to "answer" one or more prompts, but fewer than all of the prompts, and end up with "answered" challenges that are not "completed".
In the future, it may also be possible to answer all the challenges but then have an error occur before they are marked "completed" (for example, a unique key collision in the transaction code). For now, nothing interesting happens between "answered" and "completed". This would take the form of the caller explicitly providing flags like "wait to mark the challenges as completed until I do something" and "okay, mark the challenges as completed now".
This change prevents all token reuse, even on the same workflow. Future changes will let the answered challenges "stick" to the client form so you don't have to re-answer challenges for a short period of time if you hit a unique key collision.
Test Plan:
- Used a token to get through an MFA gate.
- Tried to go through another gate, was told to wait for a long time for the next challenge window.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19894
Summary:
Depends on D19890. Ref T13222. See PHI873. Currently, we only validate TOTP responses against the current (realtime) timestep. Instead, also validate them against a specific challenge.
This mostly just moves us toward more specifically preventing responses from being reused, and supporting flows which must look more like this (SMS/push).
One rough edge here is that during the T+3 and T+4 windows (you request a prompt, then wait 60-120 seconds to respond) only past responses actually work (the current code on your device won't). For example:
- At T+0, you request MFA. We issue a T+0 challenge that accepts codes T-2, T-1, T+0, T+1, and T+2. The challenge locks out T+3 and T+4 to prevent the window from overlapping with the next challenge we may issue (see D19890).
- If you wait 60 seconds until T+3 to actually submit a code, the realtime valid responses are T+1, T+2, T+3, T+4, T+5. The challenge valid responses are T-2, T-1, T+0, T+1, and T+2. Only T+1 and T+2 are in the intersection. Your device is showing T+3 if the clock is right, so if you type in what's shown on your device it won't be accepted.
- This //may// get refined in future changes, but, in the worst case, it's probably fine if it doesn't. Beyond 120s you'll get a new challenge and a full [-2, ..., +2] window to respond, so this lockout is temporary even if you manage to hit it.
- If this //doesn't// get refined, I'll change the UI to say "This factor recently issued a challenge which has expired, wait N seconds." to smooth this over a bit.
Test Plan:
- Went through MFA.
- Added a new TOTP factor.
- Hit some error cases on purpose.
- Tried to use an old code a moment after it expired, got rejected.
- Waited 60+ seconds, tried to use the current displayed factor, got rejected (this isn't great, but currently expected).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19893
Summary:
Depends on D19889. Ref T13222. Some of this logic is either not-quite-right or a little more complicated than it needs to be.
Currently, we TTL TOTP challenges after three timesteps -- once the current code could no longer be used. But we actually have to TTL it after five timesteps -- once the most-future acceptable code could no longer be used. Otherwise, you can enter the most-future code now (perhaps the attacker compromises NTP and skews the server clock back by 75 seconds) and then an attacker can re-use it in three timesteps.
Generally, simplify things a bit and trust TTLs more. This also makes the "wait" dialog friendlier since we can give users an exact number of seconds.
The overall behavior here is still a little odd because we don't actually require you to respond to the challenge you were issued (right now, we check that the response is valid whenever you submit it, not that it's a valid response to the challenge we issued), but that will change in a future diff. This is just moving us generally in the right direction, and doesn't yet lock everything down properly.
Test Plan:
- Added a little snippet to the control caption to list all the valid codes to make this easier:
```
$key = new PhutilOpaqueEnvelope($config->getFactorSecret());
$valid = array();
foreach ($this->getAllowedTimesteps() as $step) {
$valid[] = self::getTOTPCode($key, $step);
}
$control->setCaption(
pht(
'Valid Codes: '.implode(', ', $valid)));
```
- Used the most-future code to sign `L3`.
- Verified that `L4` did not unlock until the code for `L3` left the activation window.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19890
Summary:
Fixes https://discourse.phabricator-community.org/t/error-on-project-creation-or-edition-with-php7-3/2236
I didn't actually repro this because I don't have php 7.3 installed. I'm also not sure if the `break; break` was intentional or not, since I'm not sure you could ever reach two consecutive break statements.
Test Plan: Created some projects. Didn't actually try to hit the code that fires if you're making a project both a subproject and a milestone.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D19925
Summary: Suppress an unhelpful Almanac transaction and document the location of the secret clustering management capability. I thought maybe implementing `shouldHide` and checking for `isCreate` would work, but the binding apparently gets created before an interface is bound to it.
Test Plan: Looked at a fresh binding and didn't see "Unknown Object(??)", ran bin/diviner and saw expected output.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D19917
Summary:
Depends on D19912. Ref T11351. Images currently use `getMock()->getPolicy()` stuff to define policies. This causes bugs with object policies like "Subscribers", since the policy engine tries to evaluate the subscribers //for the image// when the intent is to evaluate the subscribers for the mock.
Move this to ExtendedPolicies to fix the behavior, and give Images sensible policy behavior when they aren't attached to a mock (specifically: only the user who created the image can see it).
Test Plan: Applied migrations, created and edited mocks and images without anything blowing up. Set mock visibility to "Subscribers", everything worked great.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19913
Summary: Depends on D19911. Ref T11351. `MarkupInterface` has mostly been replaced with `PHUIRemarkupView`, and isn't really doing anything for us here. Get rid of it to simplify the code.
Test Plan: Viewed various mocks with descriptions and image descriptions, saw remarkup presented properly.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19912
Summary: Depends on D19910. Ref T11351. Minor changes to make this behave in a more modern way.
Test Plan:
- Destroyed a mock.
- Lipsum'd a mock.
- Poked around, edited/viewed mocks.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19911
Summary:
Ref T11351. My end goal is to remove `applyInitialEffects()` from Pholio to clear the way for D19897.
Start with some query modernization.
Test Plan: Browsed Pholio, nothing appeared to have changed.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19910
Summary:
See https://discourse.phabricator-community.org/t/typeahead-returning-only-archived-results/2220. Ref T12538.
If a user has more than 100 disabled projects matching their search term, only disabled projects will be returned in the typeahead search results.
Test Plan: Harcoded hard limit in `PhabricatorTypeaheadModularDatasourceController` to force truncation of search results, observed active project on top of results as expected.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12538
Differential Revision: https://secure.phabricator.com/D19907
Summary:
Depends on D19888. Ref T13222. When we issue an MFA challenge, prevent the user from responding to it in the context of a different workflow: if you ask for MFA to do something minor (award a token) you can't use the same challenge to do something more serious (launch nukes).
This defuses highly-hypothetical attacks where the attacker:
- already controls the user's session (since the challenge is already bound to the session); and
- can observe MFA codes.
One version of this attack is the "spill coffee on the victim when the code is shown on their phone, then grab their phone" attack. This whole vector really strains the bounds of plausibility, but it's easy to lock challenges to a workflow and it's possible that there's some more clever version of the "spill coffee" attack available to more sophisticated social engineers or with future MFA factors which we don't yet support.
The "spill coffee" attack, in detail, is:
- Go over to the victim's desk.
- Ask them to do something safe and nonsuspicious that requires MFA (sign `L123 Best Friendship Agreement`).
- When they unlock their phone, spill coffee all over them.
- Urge them to go to the bathroom to clean up immediately, leaving their phone and computer in your custody.
- Type the MFA code shown on the phone into a dangerous MFA prompt (sign `L345 Eternal Declaration of War`).
- When they return, they may not suspect anything (it would be normal for the MFA token to have expired), or you can spill more coffee on their computer now to destroy it, and blame it on the earlier spill.
Test Plan:
- Triggered signatures for two different documents.
- Got prompted in one, got a "wait" in the other.
- Backed out of the good prompt, returned, still prompted.
- Answered the good prompt.
- Waited for the bad prompt to expire.
- Went through the bad prompt again, got an actual prompt this time.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19889
Summary: Continue clean up of super-old code. I am pretty proud of "defrocked", but would also consider "dethroned", "ousted", "unseated", "unmade", or "disenfranchised". I feel like there's a word for being kicked out of Hogwarts and having your wizarding powers revoked, but it is not leaping to mind.
Test Plan: Promoted/demoted users to/from admin, attempted to demote myself and observed preserved witty text, checked user timelines, checked feed, checked DB for sanity, including `user_logs`. I didn't test exposing this via Conduit to attempt promoting a user without having admin access.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D19891
Summary:
Depends on D19886. Ref T13222. Clean up MFA challenges after they expire.
(There's maybe some argument to keeping these around for a little while for debugging/forensics, but I suspect it would never actually be valuable and figure we can cross that bridge if we come to it.)
Test Plan:
- Ran `bin/garbage collect --collector ...` and saw old MFA challenges collected.
- Triggered a new challenge, GC'd again, saw it survive GC while still active.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19888
Summary:
Ref T13222. See PHI873. Ref T9770.
Currently, we support only TOTP MFA. For some MFA (SMS and "push-to-app"-style MFA) we may need to keep track of MFA details (e.g., the code we SMS'd you). There isn't much support for that yet.
We also currently allow free reuse of TOTP responses across sessions and workflows. This hypothetically enables some "spyglass" attacks where you look at someone's phone and type the code in before they do. T9770 discusses this in more detail, but is focused on an attack window starting when the user submits the form. I claim the attack window opens when the TOTP code is shown on their phone, and the window between the code being shown and being submitted is //much// more interesting than the window after it is submitted.
To address both of these cases, start tracking MFA "Challenges". These are basically a record that we asked you to give us MFA credentials.
For TOTP, the challenge binds a particular timestep to a given session, so an attacker can't look at your phone and type the code into their browser before (or after) you do -- they have a different session. For now, this means that codes are reusable in the same session, but that will be refined in the future.
For SMS / push, the "Challenge" would store the code we sent you so we could validate it.
This is mostly a step on the way toward one-shot MFA, ad-hoc MFA in comment action stacks, and figuring out what's going on with Duo.
Test Plan:
- Passed MFA normally.
- Passed MFA normally, simultaneously, as two different users.
- With two different sessions for the same user:
- Opened MFA in A, opened MFA in B. B got a "wait".
- Submitted MFA in A.
- Clicked "Wait" a bunch in B.
- Submitted MFA in B when prompted.
- Passed MFA normally, then passed MFA normally again with the same code in the same session. (This change does not prevent code reuse.)
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13222, T9770
Differential Revision: https://secure.phabricator.com/D19886
Summary:
Ref T13222. See PHI873. Currently, MFA implementations return this weird sort of ad-hoc dictionary from validation, which is later used to render form/control stuff.
I want to make this more formal to handle token reuse / session binding cases, and let MFA factors share more code around challenges. Formalize this into a proper object instead of an ad-hoc bundle of properties.
Test Plan:
- Answered a TOTP MFA prompt wrong (nothing, bad value).
- Answered a TOTP MFA prompt properly.
- Added new TOTP MFA, survived enrollment.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19885
Summary: I added this recently for debugging test notifications, but goofed up the markup, thought it was just some weird layout issue, and never got back to it.
Test Plan: {F6063455}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19892
Summary: Cleaning up more super-old code from `PhabricatorUserEditor`. Also fix user logging in approve transactions. I'm not sure how it worked at all previously.
Test Plan: Created new users, renamed them, checked DB for sanity. Entered invalid names, duplicate names, and empty names, got appropriate error messages.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D19887
Summary:
Ref T13222. Ref T12509. When you add a new MFA TOTP authenticator, we generate a temporary token to make sure you're actually adding the key we generated and not picking your own key.
That is, if we just put inputs in the form like `key=123, response=456`, users could pick their own keys by changing the value of `key` and then generating the correct `response`. That's probably fine, but maybe attackers could somehow force users to pick known keys in combination with other unknown vulnerabilities that might exist in the future. Instead, we generate a random key and keep track of it to make sure nothing funny is afoot.
As an additional barrier, we do the standard "store the digest, not the real key" sort of thing so you can't force a known value even if you can read the database (although this is mostly pointless since you can just read TOTP secrets directly if you can read the database). But it's pretty standard and doesn't hurt anything.
Update this from SHA1 to SHA256. This will break any TOTP factors which someone was in the middle of adding during a Phabricator upgrade, but that seems reasonable. They'll get a sensible failure mode.
Test Plan: Added a new TOTP factor.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222, T12509
Differential Revision: https://secure.phabricator.com/D19884
Summary:
Ref T13222. Ref T13225. We store a digest of the session key in the session table (not the session key itself) so that users with access to this table can't easily steal sessions by just setting their cookies to values from the table.
Users with access to the database can //probably// do plenty of other bad stuff (e.g., T13134 mentions digesting Conduit tokens) but there's very little cost to storing digests instead of live tokens.
We currently digest session keys with HMAC-SHA1. This is fine, but HMAC-SHA256 is better. Upgrade:
- Always write new digests.
- We still match sessions with either digest.
- When we read a session with an old digest, upgrade it to a new digest.
In a few months we can throw away the old code. When we do, installs that skip upgrades for a long time may suffer a one-time logout, but I'll note this in the changelog.
We could avoid this by storing `hmac256(hmac1(key))` instead and re-hashing in a migration, but I think the cost of a one-time logout for some tiny subset of users is very low, and worth keeping things simpler in the long run.
Test Plan:
- Hit a page with an old session, got a session upgrade.
- Reviewed sessions in Settings.
- Reviewed user logs.
- Logged out.
- Logged in.
- Terminated other sessions individually.
- Terminated all other sessions.
- Spot checked session table for general sanity.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13225, T13222
Differential Revision: https://secure.phabricator.com/D19883
Summary:
Ref T13222. See PHI873. I'm preparing to introduce a new MFA "Challenge" table which stores state about challenges we've issued (to bind challenges to sessions and prevent most challenge reuse).
This table will reference sessions (since each challenge will be bound to a particular session) but sessions currently don't have PHIDs. Give them PHIDs and slightly modernize some related code.
Test Plan:
- Ran migrations.
- Verified table got PHIDs.
- Used `var_dump()` to dump an organic user session.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19881
Summary: Ref T13218. This is the last public-facing API call for `loadRelatives/loadOneRelative`. This just "primed" objects to make the other calls work and had no direct effects.
Test Plan:
- Ran `bin/fact analyze`.
- Used `bin/storage upgrade -f --apply` to apply `20181031.board.01.queryreset.php`, which uses `LiskMigrationIterator`.
- Browsed user list.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim
Maniphest Tasks: T13218
Differential Revision: https://secure.phabricator.com/D19878
Summary: Ref T13218. This is like `loadOneWhere(...)` but with more dark magic. Get rid of it.
Test Plan:
- Forced `20130219.commitsummarymig.php` to hit this code and ran it with `bin/storage upgrade --force --apply ...`.
- Ran `20130409.commitdrev.php` with `bin/storage upgrade --force --apply ...`.
- Called `user.search` to indirectly get primary email information.
- Did not test Releeph at all.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13218
Differential Revision: https://secure.phabricator.com/D19876
Summary: Ref T13218. See that task for some discussion. `loadRelatives()` is like `loadAllWhere(...)` except that it does enormous amounts of weird magic which we've moved away from.
Test Plan: Did not test whatsoever since these changes are in Releeph.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13218
Differential Revision: https://secure.phabricator.com/D19874
Summary: See https://discourse.phabricator-community.org/t/personal-timezone-setting-mismatch-cleared-and-more-specific-cases/1680. The code has always worked correctly, but the resulting timezone mismatch warning messsage wasn't specific enough when the mismatch is by a non-integer number of hours.
Test Plan: Set timezone locally to Asia/Vladivostok and in Phabricator to Australia/Adelaide (which as of today's date are 30 minutes apart) and observed a more precise error message: F6061330
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D19873
Summary:
See <https://discourse.phabricator-community.org/t/tasks-created-via-workboard-column-menu-are-moved-to-wrong-column/2200>. The recent `setIsConduitOnly()` / `setIsFormField()` change (in D19842) disrupted creating tasks directly into a column from the workboard UI.
This field //is// a form field, it just doesn't render a visible control.
Test Plan:
- Created a task directly into a workboard column. Before: column selection ignored. After: appeared in correct column.
- Used "move on workboard" comment action.
- Edited tasks; edited forms for tasks. Didn't observe any collateral damage (weird "Column" fields being present).
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19870
Summary:
Ref T13217. See <https://discourse.phabricator-community.org/t/unsafe-raw-string-warnings-while-importing-git-commits/2191>.
Hunt down and fix two more `qsprintf()` things.
I just converted the "performance optimization" into a normal, safe call since we're dealing with far less SVN stuff nowadays and the actual issue has been lost in the mists of time. If it resurfaces, we can take another look.
Test Plan: Imported some commits, no longer saw these warnings in the daemon logs.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13217
Differential Revision: https://secure.phabricator.com/D19869
Summary: Ref T13222. See PHI996. This is a general correctness improvement, but also allows you to clear test notifications by clicking on them (since their default destination is the recipient's profile page).
Test Plan: Clicked a test notification, got taken to my profile page, saw notification marked as read.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19867
Summary: Depends on D19865. Ref T13222. See PHI996. Provide a `bin/aphlict notify --user ... --message ...` workflow for sending test notifications from the CLI.
Test Plan: {F6058287}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19866
Summary: Depends on D19864. Ref T13222. See PHI996. This is no longer used by anything, so get rid of it.
Test Plan: Grepped; viewed a feed with these stories in it to make sure nothing crashed/exploded.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19865
Summary:
Depends on D19861. Ref T13222. See PHI996. Fixes T10743. Currently, notifications only work if a story also has a feed rendering.
Separate "visible in feed" and "visible in notifications", and make notifications query only notifications and vice versa.
Then, set the test notification stories to be visible in notifications only, not feed.
This could be refined a bit (there's no way to have the two views render different values today, for example) but since the only actual use case we have right now is test notifications I don't want to go //too// crazy future-proofing it. I could imagine doing some more of this kind of stuff in Conpherence eventually, though, perhaps.
Test Plan: Sent myself test notifications, saw them appear on my profile timeline and in the JS popup, and in my notifications menu, but not in feed.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222, T10743
Differential Revision: https://secure.phabricator.com/D19864
Summary:
Depends on D19860. Ref T13222. Ref T10743. See PHI996.
Long ago, there were different types of feed stories. Over time, there was less and less need for this, and nowadays basically everything is a "transaction" feed story. Each story renders differently, but they're fundamentally all about transactions.
The Notification test controller still uses a custom type of feed story to send notifications. Move away from this, and apply a transaction against the user instead. This has the same ultimate effect, but involves less weird custom code from ages long forgotten.
This doesn't fix the actual problem with these things showing up in feed. Currently, stories always use the same rendering for feed and notifications, and there need to be some additional changes to fix this. So no behavioral change yet, just slightly more reasonable code.
Test Plan: Clicked the button and got some test notifications, with Aphlict running.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222, T10743
Differential Revision: https://secure.phabricator.com/D19861
Summary:
Depends on D19858. Ref T13222. See PHI995. In D19635 and related revisions, inline behavior changed to allow you to pre-mark your own inlines as done (as a reviewer) and to pre-mark your inlines for you (as an author).
These actions generate low-value stories in the timeline, like "alice marked 3 comments done." when an author adds some notes to their own revision. These aren't helpful and can be a little misleading.
Instead, just don't count it when someone marks their own inlines as "done". If we throw away all the marks after throwing away the self-marks, hide the whole story.
This happens in three cases:
# You comment on your own revision, and don't uncheck the "Done" checkbox.
# You comment on someone else's revision, and check the "Done" checkbox before submitting.
# You leave a not-"Done" inline on your own revision, then "Done" it later.
Cases (1) and (2) seem unambiguously good/clear. Case (3) is a little more questionable, but I think this still isn't very useful for reviewers.
If there's still a clarity issue around case (3), we could change the story text to "alice marked 3 inline comments by other users as done.", but I think this is probably needlessly verbose and that no one will be confused by the behavior as written here.
(Also note that this story is never shown in feed.)
Test Plan: Created and marked a bunch of inlines as "Done" in Differential and Diffusion, as the author and reviewer/auditor. My own marks didn't generate timeline stories; marking others' comments still does.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19859
Summary:
Ref T13222. See PHI995. Before making a change to inline rendering, consolidate this code for generating the "alice added inlines comments." and "alice marked X inlines as done." transactions.
Both Differential and Diffusion have four very similar chunks of code. Merge them into shared methods and reduce code duplication across the methods.
(In the next change, I plan to hide the "done" story when the mark affects your own inline, since users marking their own inlines as "done" is generally not very interesting or useful.)
Test Plan: As author and reviewer/auditor, added inlines in Differential and Diffusion. As author, marked own and others inlines as done and undone. Got sensible transaction rendering and persistence of "Done".
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19858
Summary:
Ref T13222. Fixes T12588. See PHI683. In several cases, we present the user with a choice between multiple major options: Alamnac service types, Drydock blueprint types, Repository VCS types, Herald rule types, etc.
Today, we generally do this with radio buttons and a "Submit" button. This isn't terrible, but often it means users have to click twice (once on the radio; once on submit) when a single click would be sufficient. The radio click target can also be small.
In other cases, we have a container with a link and we'd like to link the entire container: notifications, the `/drydock/` console, etc. We'd like to just link the entire container, but this causes some problems:
- It's not legal to link block eleements like `<a><div> ... </div></a>` and some browsers actually get upset about it.
- We can `<a><span> ... </span></a>` instead, then turn the `<span>` into a block element with CSS -- and this sometimes works, but also has some drawbacks:
- It's not great to do that for screenreaders, since the readable text in the link isn't necessarily very meaningful.
- We can't have any other links inside the element (e.g., details or documentation).
- We can `<form><button> ... </button></form>` instead, but this has its own set of problems:
- You can't right-click to interact with a button in the same way you can with a link.
- Also not great for screenreaders.
Instead, try adding a `linked-container` behavior which just means "when users click this element, pretend they clicked the first link inside it".
This gives us natural HTML (real, legal HTML with actual `<a>` tags) and good screenreader behavior, but allows the effective link target to be visually larger than just the link.
If no issues crop up with this, I'd plan to eventually use this technique in more places (Repositories, Herald, Almanac, Drydock, Notifications menu, etc).
Test Plan:
{F6053035}
- Left-clicked and command-left-clicked the new JS fanciness, got sensible behaviors.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13222, T12588
Differential Revision: https://secure.phabricator.com/D19855
Summary:
Ref T13222. Ref T12588. See PHI683. After D19853, "Create Subtask" may pop a dialog to let you choose between multiple forms.
Allow users to configure which forms are available by using `maniphest.subtypes` to choose available children for each subtype. Users may either specify particular subtypes or specific forms.
Test Plan: Configured "Quest" tasks to have "Objective" children, got appropriate prompt behavior. Used "subtypes" and "forms" to select forms; used "forms" to reorder forms.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222, T12588
Differential Revision: https://secure.phabricator.com/D19854
Summary:
Ref T13222. Ref T12588. See PHI683. Currently, "Create Subtask" always uses the first edit form that the user has access to for the same task subtype. (For example, if you "Create Subtask" from a "Bug", you get the first edit form for "Bugs".)
I didn't want to go too crazy with the initial subtype implementation, but it seems like we're generally on firm ground and it's working fairly well: user requests are for more flexibility in using the system as implemented, not changes to the system or confusion/difficulty with any of the tradeoffs. Thus, I'm generally comfortable continuing to build it out in the same direction. To improve flexibility, I want to make the options from "Create Subtask" more flexible/configurable.
I plan to let you specify that a given subtype (say, "Quest") prompts you with creation options for a set of other subtypes (say, "Objective"), or prompts you with a particular set of forms.
If we end up with a single option, we just go into the current flow (directly to the edit form). If we end up with more than one option, we prompt the user to choose between them.
This change is a first step toward this:
- When building "Create Subtask", query for multiple forms.
- The default behavior is now "prompt user to choose among create forms of the same subtype". Previously, it was "use the first edit form of the same subtype". This is a behavioral change.
- The next change will make the selected forms configurable.
- (I also plan to make the dialog itself less rough.)
Test Plan: {F6051067}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222, T12588
Differential Revision: https://secure.phabricator.com/D19853
Summary: Without an existing root document, Phriction shows a nice little "fake" document as the landing page, which has its own nice "Edit this document" button. When showing that page, don't also render the standard "New Document" breadcrumb in the top right. That button always prompts first for a slug name, which is silly when the root document doesn't exist (because the slug name is required to be '').
Test Plan: Loaded Phriction with and without a root document.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D19863
Summary: Depends on D19856. Ref T13222. See D19829. Make access to "Track Only" slightly cleaner and more consistent..
Test Plan: Set, edited, and removed "Track Only" settings for a repository. Saw sensible persistence and display behaviors.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19857
Summary:
Ref T13222. See D19829. We're inconsistent about using `getDetail()/setDetail()` to do some ad-hoc reads. Put this stuff in proper accessor methods.
Also a couple of text fixes from D19850.
Test Plan: Set, edited, and removed autoclose branches from a repository. Got sensible persistence and rendering behavior.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19856
Summary: Ref T13222. See PHI992. If you lose an entire cluster, you may want to aggressively demote it out of existence. You currently need to `xargs` your way through this. Allow `--demote <service>`, which demotes all devices in a service.
Test Plan: Demoted with `--demote <device>` and `--demote <service>`. Hit the `--promote service` error.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19850
Summary:
Ref T13222. See PHI992. If you've lost an entire cluster (or have lost a device and are willing to make broad assumptions about the state the device was in) you currently have to `xargs` to thaw everything or do something else creative.
Since this workflow is broadly reasonable, provide an easier way to accomplish the goal.
Test Plan:
- Ran with `--all-repositories`, a list of repositories, both (error) and neither (error).
- Saw a helpful new list of affected repositories.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19849
Summary: Ref T13222. See PHI990. The older `user.query` supports availability information, but it isn't currently available in a modern way. Make it available.
Test Plan: {F6048126}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19851
Summary:
See <https://discourse.phabricator-community.org/t/cannot-accept-commits-in-audit/2166/>.
In D19842, I changed `PhabricatorEditField->shouldGenerateTransactionsFromComment()`.
- Previously, it bailed on `getIsConduitOnly()`.
- After the patch, it bails on a missing `getCommentActionLabel()`.
The old code was actually wrong, and it was previously possible to apply possibly-invalid actions in some cases (or, at least, sneak them through this layer: they would only actually apply if not validated properly).
In practice, it let a different bug through: we sometimes loaded commits without loading their audit authority, so testing whether the viewer could "Accept" the commit or not (or take some other actions like "Raise Concern") would always fail and throw an exception: "Trying to access data not attached to this object..."
Fixing the insufficiently-strict transaction generation code exposed the "authority not attached" bug, which caused some actions to fail to generate transactions.
This appeared in the UI as either an unhelpful error ("You can't post an empty comment") or an action with no effect. The unhelpful error was because we show that error if you aren't taking any //other// actions, and we wouldn't generate an "Accept" action because of the interaction of these bugs, so the code thought you were just posting an empty comment.
Test Plan: Without leaving comments, accepted and rejected commits. No more error messages, and actions took effect.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: stephan.senkbeil, hskiba
Differential Revision: https://secure.phabricator.com/D19845
Summary: Ref T13222. Ref T12588. See PHI683. To make "Create Subtask..." fancier, we need slightly more logic around subtype maps. Upgrade the plain old array into a proper object so it can have relevant methods, notably "get a list of valid child subtypes for some parent subtype".
Test Plan: Created and edited tasks, changed task subtypes. Grepped for affected symbols (`newEditEngineSubtypeMap`, `newSubtypeMap`).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222, T12588
Differential Revision: https://secure.phabricator.com/D19852
Summary:
Ref T13222. See PHI992. If you lose all hosts in a service cluster, you may need to get a list of affected repositories to figure out which backups to pull.
Support doing this via the API.
Test Plan: Queried by service PHID and saw service PHIDs in the call results.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19848
Summary:
Depends on D19831. Ref T13216. See PHI908. Allegedly, a user copied a large repository into itself and then pushed it. Great backup strategy, but it can create headaches for administrators.
Allow a "maximum paths you can touch with one commit" limit to be configured, to make it harder for users to make this push this kind of commit by accident.
If you actually intended to do this, you can work around this by breaking your commit into pieces (or temporarily removing the limit). This isn't a security/policy sort of option, it's just a guard against silly mistakes.
Test Plan: Set limit to 2, tried to push 3 files, got rejected. Raised limit, pushed changes successfully.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19839
Summary: Depends on D19830. Ref T13216. See PHI908. See PHI750. See PHI885. Allow users to configure a filesize limit, and allow them to adjust the clone/fetch timeout.
Test Plan:
{F6021356}
- Configured a filesize limit and pushed, hit it. Made the limit larger and pushed, change went through.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19831
Summary:
Depends on D19829. Ref T13216. See PHI908. The current implementation is kind of a lot to live in `CommitHookEngine` and will likely fail if `git diff-tree` produces more than 2GB of output.
Pull it out and make it slightly more robust against enormous commits. It's probably limited by this, now:
```
implode("\n", $every_path)
```
We could replace that with some `PhutilReverseRopeSource` primitive or something but since we don't have one of those and it seems unlikely that we'll hit this case in practice, I left it here for now with just the easy stuff converted to be stream-oriented.
Test Plan:
Used this script to test the query against various commits, got good results:
```
<?php
require_once 'scripts/init/init-script.php';
$viewer = PhabricatorUser::getOmnipotentUser();
$repository = id(new PhabricatorRepositoryQuery())
->setViewer($viewer)
->withCallsigns(array('P'))
->executeOne();
var_dump(
id(new DiffusionLowLevelFilesizeQuery())
->setRepository($repository)
->withIdentifier($argv[1])
->execute());
```
Used this to find large commits in history and pull filesizes (worked great, although our largest commit only touches a couple thousand paths):
```
for hash in `git log --format=%H`; do echo -n $hash; echo -n ' '; git diff-tree -r --no-commit-id $hash | wc -l | awk '{print $1}'; done | awk '{print $2 " " $1}' | sort -n
```
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19830
Summary: Depends on D19826. Ref T13216. We have a fair number of options here; add some groups so the "Build" stuff can go in a little subcategory and such.
Test Plan: {F6020896}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19827
Summary:
Ref T13216. See PHI985. We currently use a banner to warn you when a revision has no reviewers or only disabled users, but since the changes to track "Resign" more explicilty we'll no longer warn you if everyone has resigned.
(Previously, they'd no longer be reviewers, so you'd end up with the "no reviewers are assigned" warning if everyone resigned.)
This can still interact slightly oddly with some states (e.g., only a package or project reviewer) but I'd like to wait for T731 to tighten those cases up, and they're more advanced/unusual.
Test Plan:
{F6026832}
{F6026833}
{F6026834}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19834
Summary: Ref T13222. See PHI986. See PHI896. Harbormaster build targets don't currently have a modern "*.search" API, but there's no reason not to provide one (even if some of the use cases are a little bit questionable).
Test Plan: {F6032423}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19841
Summary:
Ref T13222. See PHI683. Currently, you can "Change subtype..." via Conduit and the bulk editor, but not via the comment action stack or edit forms.
In PHI683 an install is doing this often enough that they'd like it to become a first-class action. I've generally been cautious about pushing this action to become a first-class action (there are some inevitable rough edges and I don't want to add too much complexity if there isn't a use case for it) but since we have evidence that users would find it useful and nothing has exploded yet, I'm comfortable taking another step forward.
Currently, `EditEngine` has this sort of weird `setIsConduitOnly()` method. This actually means more like "this doesn't show up on forms". Make it better align with that. In particular, a "conduit only" field can already show up in the bulk editor, which is goofy. Change this to `setIsFormField()` and convert/simplify existing callsites.
Test Plan:
There are a lot of ways to reach EditEngine so this probably isn't entirely exhaustive, but I think I got pretty much anything which is likely to break:
- Searched for `setIsConduitOnly()` and `getIsConduitOnly()`, converted all callsites to `setIsFormField()`.
- Searched for `setIsLockable()`, `setIsReorderable()` and `setIsDefaultable()` and aligned these calls to intent where applicable.
- Created an Almanac binding.
- Edited an Almanac binding.
- Created an Almanac service.
- Edited an Almanac service.
- Edited a binding property.
- Deleted a binding property.
- Created and edited a badge.
- Awarded and revoked a badge.
- Created and edited an event.
- Made an event recurring.
- Created and edited a Conpherence thread.
- Edited and updated the diff for a revision.
- Created and edited a repository.
- Created and disabled repository URIs.
- Created and edited a blueprint.
- Created and edited tasks.
- Created a paste, edited/archived a paste.
- Created/edited/archived a package.
- Created/edited a project.
- Made comments.
- Moved tasks on workboards via comment action stack.
- Changed task subtype via comment action stack.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19842
Summary:
Ref T13222. See PHI873. Currently, when applications prompt users to enter MFA, their session upgrades as a side effect.
In some cases (like managing your email addresses) it makes sense to upgrade your session for a little while since it's common to make multiple edits in sequence (add a new address, make it primary, remove an old address). We generally want MFA to stay out of the way and not feel annoying.
In other cases, we don't expect multiple high-security actions in a row. Notably, PHI873 looks at more "one-shot" use cases where a prompt is answering a specific workflow. We already have at least one of these in the upstream: answering an MFA prompt when signing a Legalpad document.
Introduce a "token" workflow (in contrast to the existing "session") workflow that just does a one-shot prompt without upgrading your session statefully. Then, make Legalpad use this new workflow.
Note that this workflow has a significant problem: if the form submission is invalid for some other reason, we re-prompt you on resubmit. In Legalpad, this workflow looks like:
- Forget to check the "I agree" checkbox.
- Submit the form.
- Get prompted for MFA.
- Answer MFA prompt.
- Get dumped back to the form with an error.
- When you fix the error and submit again, you have to do another MFA check.
This isn't a fatal flaw in Legalpad, but would become a problem with wider adoption. I'll work on fixing this (so the MFA token sticks to the form) in the next set of changes.
Roughly, this is headed toward "MFA sticks to the form/workflow" instead of "MFA sticks to the user/session".
Test Plan:
- Signed a legalpad document with MFA enabled.
- Was prompted for MFA.
- Session no longer upgraded (no purple "session in high security" badge).
- Submitted form with error, answered MFA, fixed error, submitted form again.
- Bad behavior: got re-prompted for MFA. In the future, MFA should stick to the form.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19843
Summary:
Ref T13216. I want to add some new management options to repositories (e.g., filesize limit, clone timeouts). Before adding new stuff here, update the UI to a full-width, Phortune-style UI.
This partially reverts D18523. About a year ago, several UIs got converted to fixed-width (repository management, config, settings, instance management in SAAS). I didn't think these were good changes and have never really gotten used to them. The rationale wasn't clear to me and these changes just felt like "be more like GitHub". I think usability is significantly worse, e.g. actions are now hidden inside button menus instead of immediately visible.
Phortune also got converted less dramatically to a full-width-with-menu UI, which I like much better. Adjust repository management to use that UI style instead of the fixed-width style.
Test Plan:
{F6020884}
Viewed every panel, including the Subversion panel.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19826
Summary:
Ref T13216. Ref T13217. Currently, we build this query in a weird way so we end up with `(1, 2, 3)` on both 32-bit and 64-bit systems.
I can't reproduce the string-vs-int MySQL key issue on any system I have access to, so just simplify this and format as `('1', '2', '3')` instead.
The issue this is working around is that MySQL would (I think?) sometimes appear to do something goofy and miss the key if you formatted the query with strings. I never really nailed this down and could have either been mistaken about it or it could be fixed in all modern versions of MySQL. Until we have better evidence to the contrary, assume MySQL is smart enough to handle this sensibly now.
Test Plan: Ran daemons with Feed publish workers, no longer received query warnings.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13217, T13216
Differential Revision: https://secure.phabricator.com/D19837
Summary:
Ref T13216. See PHI985. This config option once controlled adding a Herald transcript link to email. However, this was never implemented in a generic way and was removed from revisions in D8459 and from commits in D10705. No one has noticed or asked for this option for several years, so this is probably a good opportunity to simplify the software and reduce the total amount of configuration.
If we did want to pursue this in the future, I'd generally prefer to make it part of the mail detail page (`/mail/detail/12345/`) anyway.
Test Plan: Grepped for `metamta.herald.show-hints` and `addHeraldSection()`, got no hits for either.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19833
Summary:
Ref T13216. See PHI985. You currently can't commandeer an abandoned revision, but this workflow is perfectly fine.
The caution here is just around weird use cases where, e.g., users want to reopen a revision to add a revert to it. These workflows tend to create problems so we try to guide users away from them.
Test Plan: {F6026841}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19835
Summary: Depends on D19837. Ref T13216. See PHI985. There's an off-by-one error here between how inline comments store "length" and how context rendering treats "length". We need to add 1 to the length, but currently do it a little too early. Do it slightly later so that inlines on the final line of a file render properly.
Test Plan: Left an inline on the final line of a new file, saw it render properly in HTML mail.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19838
Summary:
Ref T13216. Fixes T12920. See PHI911. If you reject a revision and then resign from it, it stays in "Needs Revision".
There's some arguable motivation for this, but it's inconsistent with how "Accept" works (if the last accepting reviewer resigns, we kick you out of "Accepted"). Make it consistent.
Test Plan:
- As the only reviewer: requested changes to a revision, then resigned.
- Before: revision stays in "Needs Revision".
- After: revision moves back to "Needs Review".
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216, T12920
Differential Revision: https://secure.phabricator.com/D19840
Summary:
Depends on D19827. Ref T13221. Ref T13216. To prepare Repositories for a move to ModularTransactions, throw away some very old transaction rendering code.
This will cause these very old transactions (none of which have been written since at least April 2016) to render "epriestley edited this repository." instead of "epriestley changed the SSH login for this repository from X to Y."
These edits were generally obsoleted by repository URIs, Passphrase credentials, and general modernization.
Test Plan: Grepped for all constants, got no hits.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13221, T13216
Differential Revision: https://secure.phabricator.com/D19828
Summary: Ref T13216. See PHI984. The CommitSearchEngine (and, by extension, `diffusion.commit.search`) currently do not support identifier search, but this is a reasonable capability to provide.
Test Plan:
Testing that a commit exists on `master`:
{F6020742}
Same commit is not on `stable`:
{F6020743}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19825
Summary:
Ref T13216. See PHI916. Harbormaster builds may be long-running, particularly if they effectively wrap `ssh ... ./run-huge-build.sh`. If we spend more than a few seconds waiting for futures to resolve, close idle database connections.
The general goal here is to reduce the held connection load for installs with a very large number of test runners.
Test Plan: Added debugging code to `phlog()` closures, saw connections closed while running builds.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19824
Summary:
Depends on D19816. Ref T13216. See PHI908. See PHI750. In a few cases, users have pushed multi-gigabyte files full of various things that probably shouldn't be version controlled. This tends to create various headaches.
Add support for limiting the maximum size of any object. Specifically, we:
- list all the objects each commit touches;
- check their size after the commit applies;
- if it's over the limit, reject the commit.
This change doesn't actually hook the limit up (the limit is always "0", i.e. unlimited), and doesn't have Mercurial or SVN support. The actual parser bit would probably be better in some other `Query/Parser` class eventually, too. But it at least roughly works.
Test Plan:
Changed the hard-coded limit to other values, tried to push stuff, got sensible results:
```
$ echo pew >> magic_missile.txt && git commit -am pew && git push
[master 98d07af] pew
1 file changed, 1 insertion(+)
# Push received by "local.phacility.net", forwarding to cluster host.
# Acquiring write lock for repository "spellbook"...
# Acquired write lock immediately.
# Acquiring read lock for repository "spellbook" on device "local.phacility.net"...
# Acquired read lock immediately.
# Device "local.phacility.net" is already a cluster leader and does not need to be synchronized.
# Ready to receive on cluster host "local.phacility.net".
Counting objects: 49, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (48/48), done.
Writing objects: 100% (49/49), 3.44 KiB | 1.72 MiB/s, done.
Total 49 (delta 30), reused 0 (delta 0)
remote: +---------------------------------------------------------------+
remote: | * * * PUSH REJECTED BY EVIL DRAGON BUREAUCRATS * * * |
remote: +---------------------------------------------------------------+
remote: \
remote: \ ^ /^
remote: \ / \ // \
remote: \ |\___/| / \// .\
remote: \ /V V \__ / // | \ \ *----*
remote: / / \/_/ // | \ \ \ |
remote: @___@` \/_ // | \ \ \/\ \
remote: 0/0/| \/_ // | \ \ \ \
remote: 0/0/0/0/| \/// | \ \ | |
remote: 0/0/0/0/0/_|_ / ( // | \ _\ | /
remote: 0/0/0/0/0/0/`/,_ _ _/ ) ; -. | _ _\.-~ / /
remote: ,-} _ *-.|.-~-. .~ ~
remote: * \__/ `/\ / ~-. _ .-~ /
remote: \____(Oo) *. } { /
remote: ( (..) .----~-.\ \-` .~
remote: //___\\ \ DENIED! ///.----..< \ _ -~
remote: // \\ ///-._ _ _ _ _ _ _{^ - - - - ~
remote:
remote:
remote: OVERSIZED FILE
remote: This repository ("spellbook") is configured with a maximum individual file size limit, but you are pushing a change ("98d07af863e799509e7c3a639404d216f9fc79c7") which causes the size of a file ("magic_missile.txt") to exceed the limit. The commit makes the file 317 bytes long, but the limit for this repository is 1 bytes.
remote:
# Released cluster write lock.
To ssh://local.phacility.com/source/spellbook.git
! [remote rejected] master -> master (pre-receive hook declined)
error: failed to push some refs to 'ssh://epriestley@local.phacility.com/source/spellbook.git'
```
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: joshuaspence
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19817
Summary:
Ref T13216. See PHI943. When you have a large number of cluster bindings for a repository, the UI sorting can be a bit hard to manage.
One install that regularly cycles repository cluster devices had a couple dozen older disabled bindings, with the enabled bindings intermingled.
Sort the UI:
- enabled devices come first;
- in each group, sort by name.
Test Plan: Mixed disabled/enabled bindings, loaded {nav Diffusion > Repository > Storage} page with clustering configured. Before: relatively unhelpful sort order. After: more intuitive sort order.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19813
Summary: Ref T13217. This older query does some manual joins; update it for more modern joins.
Test Plan: Ran `instances/` unit tests and got a clean result, browsed Phortune merchants.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13217
Differential Revision: https://secure.phabricator.com/D19820
Summary:
Ref T13216. See PHI980. Currently, each application in {nav Applications > X > Configure} has a "Can Configure Application" permission which is hard-coded to "Administrators".
There's no technical reason for this, there just hasn't been a great use case for unlocking it. I think when I originally wrote it our protections against locking yourself out of things weren't that great (i.e., it was easier to set the policy to something that prevented you from editing it after the new policy took effect). Our protections are better now.
The major goal here is to let installs open up Custom Forms for given applications (mostly Maniphest) to more users, but the other options mostly go hand-in-hand with that.
Also, in developer mode, include stack traces for policy exceptions. This makes debugging weird stuff (like the indirect Config application errors here) easier.
Test Plan:
- Granted "Can Configure Application" for Maniphest to all users.
- Edited custom forms as a non-administrator.
- Configured Maniphest as a non-administrator.
- Installed/uninstalled Maniphest as a non-administrator.
- Tried to lock myself out (got an error message).
{F6015721}
Reviewers: amckinley, joshuaspence
Reviewed By: joshuaspence
Subscribers: joshuaspence
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19822
Summary:
Depends on D19814. Ref T13216. See PHI885. For various eldritch reasons, `git fetch` can hang. Although we'd probably like to fix this with `git fetch --require-sustained-network-transfer-rate=512KB/5s` or similar, that flag doesn't exist and we don't have a reasonable way to build it.
Short of that, move toward formalizing a repository "copy time limit": the longest amount of time anything may spend trying to make a copy of this repository.
This grows out of the existing intracluster sync limit, which is effectively the same thing. Here, apply it to `git clone` and `git fetch` in Drydock working copy construction, too. A future change may make it configurable.
Test Plan:
- Set the limit to 0.001.
- Tried to build and lease working copies, got sensible timeout errors (see D19815).
```
<Activation Failed> Lease activation failed: [CommandException] Command killed by timeout after running for more than 0.001 seconds.
COMMAND
ssh '-o' 'LogLevel=quiet' '-o' 'StrictHostKeyChecking=no' '-o' 'UserKnownHostsFile=/dev/null' '-o' 'BatchMode=yes' -l '********' -p '2222' -i '********' '127.0.0.1' -- '(cd '\''/var/drydock/workingcopy-163/repo/spellbook/'\'' && git clean -d --force && git fetch && git reset --hard)'
```
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19816
Summary: Depends on D19810. Ref T13217. Ref T13216. I mostly used `grep implode | grep OR` and `grep implode | grep AND` to find these -- not totally exhaustive but should be a big chunk of the callsites that are missing `%LO` / `%LA`.
Test Plan:
These are tricky to test exhaustively, but I made an attempt to hit most of them:
- Browsed Almanac interfaces.
- Created/browsed Calendar events.
- Enabled/disabled/showed the lock log.
- Browsed repositories.
- Loaded Facts UI.
- Poked at Multimeter.
- Used typeahead for users and projects.
- Browsed Phriction.
- Ran various fulltext searches.
Not sure these are reachable:
- All the lint stuff might be dead/unreachable/nonfunctional?
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim
Maniphest Tasks: T13217, T13216
Differential Revision: https://secure.phabricator.com/D19814
Summary:
Ref T6960. Ref T13217. Ref T13216. Depends on D19811. Use the recently-introduced "%P" conversion ("Password/Secret") to load sessions in SessionEngine.
This secret isn't critical to protect (it's the //hash// of the actual secret and not useful to attackers on its own) but it shows up on every page in DarkConsole and is an obvious case where `%P` is a more appropriate conversion.
Test Plan:
Note "*********" in the middle of the output here, instead of a session key hash:
{F6012805}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13217, T13216, T6960
Differential Revision: https://secure.phabricator.com/D19812
Summary:
Ref T13216. See PHI970. Ref T13054. See some discussion in T13216.
When a Harbormaster Buildable object is first created for a Diff, it has no `containerPHID` since the revision has not yet been created.
We later (after creating a revision) send the Buildable a message telling it that we've added a container and it should re-link the container object.
Currently, we send this message in `applyExternalEffects()`, which runs inside the Differential transaction. If Harbormaster races quickly enough, it can read the `Diff` object before the transaction commits, and not see the container update.
Add a `didCommitTransaction()` callback after the transactions commit, then move the message code there instead.
Test Plan:
- See T13216 for substantial evidence that this change is on the right track.
- Before change: added `sleep(15)`, reproduced the issue reliably.
- After change: unable to reproduce issue even with `sleep(15)` (the `containerPHID` always populates correctly).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216, T13054
Differential Revision: https://secure.phabricator.com/D19807
Summary:
See PHI975. Ref T13216. Ref T2543. Previously, see D19204 and PHI433.
When you're acting on a draft revision, we change the button text to "Submit Quietly" as a hint that your actions don't generate notifications yet.
However, this isn't accurate when one of your actions is "Request Review", which causes the revision to publish.
Allow actions to override the submit button text, and make the "Request Review" action change the button text to "Publish Revision".
The alternative change I considered was to remove the word "Quietly" in all cases.
I'm not //thrilled// about how complex this change is to adjust one word, but the various pieces are all fairly clean individually. I'm not sure we'll ever be able to use it for anything else, but I do suspect that the word "Quietly" was the change in D19204 with the largest effect by far (see T10000).
Test Plan:
- Created a draft revision. Saw "Submit Quietly" text.
- Added a "Request Review" action, saw it change to "Publish Revision".
- Reloaded page, saw stack saved and "Publish Revision".
- Removed action, saw "Submit Quietly".
- Repeated on a non-draft revision, button stayed put as "Submit".
- Submitted the various actions, saw them have the desired effects.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216, T2543
Differential Revision: https://secure.phabricator.com/D19810
Summary: Ref T13216. See D19666. It's currently tricky to profile Herald test runs since you have to submit a form and repeating them is a bit of a mess. Provide a simple CLI wrapper so we can use `--xprofile`. This is also maybe nice-to-have if we're ever debugging anything here.
Test Plan: Ran `bin/herald test --object ... --type ...` and got a sensible looking transcript in the UI.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19806
Summary:
Ref T13216. See PHI947. In Herald, Personal rules do not run if their author's account is disabled.
This isn't communicated very clearly in the UI, and the way the SearchEngine/Query are set up isn't great.
Define "active" as "rule will actually run", which specifically means "rule is enabled, and has a valid (non-disabled) author if it needs one".
Change the meaning of the "Active" default filter from "rule is enabled" to "rule is enabled, and has a valid author if it needs one".
Refine the status badge on the view controller to show this "invalid author" state.
Tweak the language for "Disable/Enable" to be more consistent -- we currently call it "disabled" in some cases and "archived" in others.
Test Plan:
- Disabled a user account and saw their personal rules behave properly with the new filters/options/view controller.
- Disabled/enabled a rule, saw consistent text.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19805
Summary: Ref T13216. Update the Herald Rule SearchEngine and Query to use a more modern style.
Test Plan: Ran various rule queries in the UI, got sensible results
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19803
Summary:
Ref T13216. Ref T13217. Depends on D19800. This fixes all of the remaining query warnings that pop up when you run "arc unit --everything".
There's likely still quite a bit of stuff lurking around, but hopefully this covers a big set of the most common queries.
Test Plan: Ran `arc unit --everything`. Before change: lots of query warnings. After change: no query warnings.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13217, T13216
Differential Revision: https://secure.phabricator.com/D19801
Summary: Depends on D19789. Ref T13217. Continue updating things to use the new %Q-flavored conversions instead of smushing a bunch of strings together.
Test Plan: Browsed around, far fewer errors. These changes are largely mechanical in nature.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13217
Differential Revision: https://secure.phabricator.com/D19790
Summary: Depends on D19785. Ref T13217. This converts many of the most common clause construction pathways to the new %Q / %LQ / %LO / %LA / %LJ semantics.
Test Plan: Browsed around a bunch, saw fewer warnings and no obvious behavioral errors. The transformations here are generally mechanical (although I did them by hand).
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: hach-que
Maniphest Tasks: T13217
Differential Revision: https://secure.phabricator.com/D19789
Summary: Depends on D19784. Ref T13217. Reduce uses of unsafe `%Q` in SELECT construction.
Test Plan: This reduces the number of safety warnings when loading Phabricator home from ~900 to ~800.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13217
Differential Revision: https://secure.phabricator.com/D19785
Summary:
See PHI958. Ref T13210. Previously, see PHI720.
The use case for the magic in PHI720 involves multiple patterns, and no parameter can be passed to `branch` that will result in multiple patterns being passed to `git`.
Replace the implicit magic with an explicit `patterns` parameter.
This whole thing is a bit shaky but probably isn't hurting anything.
Test Plan:
- Ran query with no `patterns`.
- Ran query with invalid `patterns`, got readable error.
- Ran query with various valid `patterns` (plain branch name, globs with "?" and "*"), got sensible results.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19771
Summary:
Ref T13217. This method is slightly tricky:
- We can't safely return a string: return an array instead.
- It no longer makes sense to accept glue. All callers use `', '` as glue anyway, so hard-code that.
Then convert all callsites.
Test Plan: Browsed around, saw fewer "unsafe" errors in error log.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13217
Differential Revision: https://secure.phabricator.com/D19784
Summary: Depends on D19798. Ref T13216. This puts at least a basic UI on top of sync logs.
Test Plan:
Viewed logs from the web UI and exported data. Note that these syncs are somewhat simulated since I my local cluster is somewhat-faked (i.e., not actually multiple machines).
{F5995899}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19799
Summary:
Depends on D19797. Ref T13216.
- Put the new `hookWait` in the export and the UI.
- Put the existing waits in the UI, not just the export.
- Make order consistent: host, write, read, hook (this is the order the timers start in).
Test Plan: Pushed some stuff, viewed web UI and saw sensible numbers, exported data and got the same values.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19798
Summary:
Ref T13216. See PHI943. If autoscale lightning strikes all your servers at once and destroys them, the path to recovery can be unclear. You're "supposed" to:
- demote all the devices;
- disable the bindings;
- bind the new servers;
- put whatever working copies you can scrape up back on disk;
- promote one of the new servers.
However, the documentation is a bit misleading (it was sort of written with "you lost one or two devices" in mind, not "you lost every device") and demote-before-disable is unnecessary and slightly risky if servers come back online. There's also a missing guardrail before the promote step which lets you accidentally skip the demotion step and end up in a confusing state. Instead:
- Add a guard rail: when you try to promote a new server, warn if inactive devices still have versions and tell the user to demote them.
- Allow demotion of inactive devices: the order "disable, demote" is safer and more intuitive than "demote, disable" and there's no reason to require the unintuitive order.
- Make the "cluster already has leaders" message more clear.
- Make the documentation more clear.
Test Plan:
- Bound a repository to two devices.
- Wrote to A to make it a leader, then disabled it (simulating a lightning strike).
- Tried to promote B. Got a new, useful error ("demote A first").
- Demoted A (before: error about demoting inactive devices; now: works fine).
- Promoted B. This worked.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19793
Summary: Depends on D19796. Simplify some timing code by using phutil_microseconds_since() instead of duplicate casting and arithmetic.
Test Plan: Grepped for `1000000` to find these. Pulled, pushed, made a conduit call. This isn't exhaustive but it should be hard for these to break in a bad way since they're all just diagnostic.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19797
Summary:
Depends on D19779. Ref T13216. The push logs currently record the "hostWait", which is roughly "locking + subprocess cost". We also record locking separately, so we can figure out "subprocess cost" alone by subtracting the lock costs.
However, the subprocess (normally `git receive-pack`) runs hooks, and we don't have an easy way to figure out how much time was spent doing actual `git` stuff vs spent doing commit hook processing. This would have been useful in diagnosing at least one recent issue.
Track at least a rough hook cost and record it in the push logs.
Test Plan: Pushed to a repository, saw a reasonable hook cost appear in the database table.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19780
Summary:
Depends on D19778. Ref T13216. See PHI943, PHI889, et al.
We currently have a push log and a pull log, but do not separately log intracluster synchronization events. We've encountered several specific cases where having this kind of log would be helpful:
- In PHI943, an install was accidentally aborting locks early. Having timing information in the sync log would let us identify this more quickly.
- In PHI889, an install hit an issue with `MaxStartups` configuration in `sshd`. A log would let us identify when this is an issue.
- In PHI889, I floated a "push the linux kernel + fetch timeout" theory. A sync log would let us see sync/fetch timeouts to confirm this being a problem in practice.
- A sync log will help us assess, develop, test, and monitor intracluster routing sync changes (likely those in T13211) in the future.
Some of these events are present in the pull log already, but only if they make it as far as running a `git upload-pack` subprocess (not the case with `MaxStartups` problems) -- and they can't record end-to-end timing.
No UI yet, I'll add that in a future change.
Test Plan:
- Forced all operations to synchronize by adding `|| true` to the version check.
- Pulled, got a sync log in the database.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19779
Summary:
Ref T13216. When a repository is clustered, we run this cleanup code (to tell the repository to update, and log some timing information) on both nodes. Currently, we do slightly too much work, which is unnecessary and can be a bit confusing to human readers.
The double update message doesn't hurt anything, but there's no reason to write it twice.
Likewise, the second timing information update query doesn't do anything: there's no PushEvent object with the right identifier, so it just updates nothing. We don't need to run it, and it's confusing that we do.
Instead, only do these writes if we're actually the final node with the repository on it.
Test Plan: Added some logging, saw double writes/updates before the change and no doubles afterwards, with no other behavioral changes.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19778
Summary:
See <https://hackerone.com/reports/435648>. We currently use 80-bit TOTP keys. The RFC suggests 128 as a minimum and recommends 160.
The math suggests that doing the hashing for an 80-bit key is hard (slightly beyond the reach of a highly motivated state actor, today) but there's no reason not to use 160 bits instead to put this completely out of reach.
See some additional discussion on the HackerOne report about enormous key sizes, number of required observations, etc.
Test Plan: Added a new 160-bit TOTP factor to Google Authenticator without issue.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19792
Summary:
Ref T13216. We occasionally receive HackerOne reports concerned that you can select your username as a password. I suspect very few users actually do this and that this is mostly a compliance/checklist sort of issue, not a real security issue.
On this install, we have about 41,000 user accounts. Of these, 100 have their username as a password (account or VCS). A substantial subset of these are either explicitly intentional ("demo", "bugmenot") or obvious test accounts ("test" in name, or name is a nonsensical string of gibberish, or looks like "tryphab" or similar) or just a bunch of numbers (?), or clearly a "researcher" doing this on purpose (e.g., name includes "pentest" or "xss" or similar).
So I'm not sure real users are actually very inclined to do this, and we can't really ever stop them from picking awful passwords anyway. But we //can// stop researchers from reporting that this is an issue.
Don't allow users to select passwords which contain words in a blocklist: their username, real name, email addresses, or the install's domain name. These words also aren't allowed to contain the password (that is, neither your password nor your username may be a substring of the other one). We also do a little normalization to try to split apart email addresses, domains, and real names, so I can't have "evan1234" as my password.
Test Plan:
- Added unit tests and made them pass.
- Tried to set my password to a bunch of variations of my username / email / domain name / real name / etc, got rejected.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19776
Summary:
Ref T13216. See PHI948. When you use the remarkup hint button to embed a meme with no text, you get `{meme src=X}`.
If the source is a GIF, we currently split the source apart into frame-by-frame images, process them, and stitch them back together. The end result is the same image we started with, but this process can be slow/expensive, and may timeout for sufficiently large GIFs.
Instead: when there's no text, just return the original image data.
Test Plan:
- Used `{meme src=X}` with no text, got an image faster.
- Used `{meme src=X, above=...}` to add text, got an attempt to add text (which didn't get very far locally since I don't have GD configured).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19777
Summary:
Depends on D19773. See <https://hackerone.com/reports/434116>. You can currently vote for invalid options by submitting, e.g., `vote[]=12345`.
By doing this, you can see the responses, which is sort of theoretically a security problem? This is definitely a bug, regardless.
Instead, only allow users to vote for options which are actually part of the poll.
Test Plan:
- Tried to vote for invalid options by editing the form to `vote[]=12345` (got error).
- Tried to vote for invalid options by editing the radio buttons on a plurality poll into checkboxes, checking multiple boxes, and submitting (got error).
- Voted in approval and plurality polls the right way, from the main web UI and from the embed (`{V...}`) UI.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19774
Summary:
See <https://hackerone.com/reports/434116>. Slowvote has a piece of Javascript that attempts to let you vote on `{V123}` polls inline.
It does not work: nothing ever triggers it (nothing renders a control with a `slowvote-option` sigil).
At least for now, just remove it. It has a completely separate pathway in the controller and both pathways are buggy, so this makes fixing them easier.
Test Plan: Voted in plurality and approval polls via Slowvote and the embedded widget.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19773
Summary:
See <https://discourse.phabricator-community.org/t/diffusionrequest-regex-error/2057/>.
The intent of `[\d-,]` is "digits, hyphen, and comma" but `[x-y]` means "character range x-y".
Specify `[\d,-]` instead to disambiguate the hyphen as "literal hyphen", not a character range marker.
Test Plan: I can't reproduce the original error as reported, but browsed around Diffusion for a bit.
Reviewers: amckinley, avivey
Reviewed By: avivey
Differential Revision: https://secure.phabricator.com/D19770
Summary:
Fixes T13208. See that task for details.
The `clone $query` line is safe if `$query` is a builtin query (like "open").
However, if it's a saved query we clone not only the query parameters but the ID, too. Then when we `save()` the query later, we overwrite the original query.
So this would happen in the database. First, you run a query and save it as the workboard default (query key "abc123"):
| 123 | abc123 | {"...xxx..."} |
Then we `clone` it and change the parameters, and `save()` it. But that causes an `UPDATE ... WHERE id = 123` and the table now looks like this:
| 123 | def456 | {"...yyy..."} |
What we want is to create a new query instead, with an `INSERT ...`:
| 123 | abc123 | {"...xxx..."} |
| 124 | def456 | {"...yyy..."} |
Test Plan:
- Followed reproduction steps from above.
- With just the new `save()` guard, hit the guard error.
- With the `newCopy()`, got a new copy of the query and "View as Query" remained functional without overwriting the original query row.
- Ran migration, saw an affected board get fixed.
Reviewers: amckinley, joshuaspence
Reviewed By: joshuaspence
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13208
Differential Revision: https://secure.phabricator.com/D19768
Summary:
Fixes T12145. Ref T13210. See PHI570. See PHI536.
Currently, when you give Drydock an Almanac host pool with more than one host, it never voluntarily builds a second host resource: there is no way to say "maximum X working copies per host" (only "maximum X global working copies") to make the first host overflow, and the allocator tries to pack resources as tightly as possible.
If you can force it to allocate the 2nd..Nth host, things will work reasonably well from there (it will spread working copies across the hosts randomly), but tricking it is very hard, especially before D19761.
To deal with this, give blueprints a new behavior around "supplemental allocations". The idea here is that a blueprint may decide that it would prefer to allocate a fresh new resource instead of allowing an otherwise valid acquisition to occur.
These supplemental allocations follow all the normal allocation rules (they can't exceed limits or actually replace existing resources), so they can only happen if there's free space in the resource pool. But a blueprint can elect for a supplemental allocation to provide a "grow the pool" hint.
The only useful policies here are probably "true" (immediately use all resources, like Almanac) or "false" (pack resources as efficiently as possible) but some other policies //might// be useful (perhaps "start growing the pool when we're getting a bit full even if we aren't at the limit yet, since our workload is bursty").
Then, give Almanac host resources a "true" policy (always allocate supplemental resources) so they use all hosts once a similar number of concurrent jobs arrive.
One aspect of this approach is that we only do supplemental resources if the normal allocation algorithm already decided that the best resource to acquire was part of the same blueprint. I started with an approach like "look at all the blueprints and see if any of them want to be greedy", but then a not-very-desirable blueprint would end up filling up its whole pool before we skipped the supplemental allocation part and ended up picking a different resource. That felt a bit silly and this feels a little cleaner and more focused.
Test Plan:
- Without changing the Almanac blueprint policy, allocated hosts. Got A, A, A, A, ... (second host never used).
- Changed the Almanac policy.
- Allocated hosts, got A, B, random mix of A and B.
- Destroyed B. Destroyed all leases on A. Allocated. Got A. This tests the "don't build a supplemental resource if there are no leases on the natural resource".
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13210, T12145
Differential Revision: https://secure.phabricator.com/D19762
Summary:
Ref T13210. Ref T12145. The "Almanac Host" blueprint currently hands out new leases on a given host even if the binding has been disabled.
Although there are some more complicated cases here (e.g., involving cleanup of the existing resource and existing leases), this one seems clear cut: if the binding has been disabled, we should stop handing out new leases on it.
Test Plan:
- Created a service with two hosts.
- Requested a lease, got host A.
- Requested more leases, always got host A (we never build a new host when we don't have to, and we currently never have to).
- Disabled the binding to host A.
- Requested a lease.
- Before patch: got host A.
- After patch: got host B.
- Also disabled the other binding to host B, requested a lease, got an indefinite wait for resources (which is expected and reasonable).
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13210, T12145
Differential Revision: https://secure.phabricator.com/D19761
Summary: Ref T13210. See PHI841. This mirrors D19509 for Differential.
Test Plan: Called `transaction.search` on a commit with a bunch of audit activity, got appropriate labels in the results.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19760
Summary:
Ref T13210. The comment action dropdown for audits has a heavy checkmark next to "Accept" and a heavy "X" next to "Raise Concern".
We previously removed similar marks in Differential in D19405 and that seems to have gone fine. For consistency, remove these too.
Test Plan: Viewed the comment action dropdown, no longer saw checkmark and X-mark.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19759
Summary:
Ref T13210. See PHI930. This translation is wrong: the parameter is a comma-separated list as a string, but the USEnglish translation provides alternatives. We can't select among alternatives based on a random string (it isn't a plurality value to let us select "chair" vs "chairs", and isn't a gender value to let us select "his profile" vs "her profile") so we get an error.
But the string itself is also misleading, since "bin/phd log --id A --id B --id C" will say "none of these are valid" if //any// of them are invalid.
Instead, just tell the user explicitly about the first problem.
Test Plan:
- Ran `bin/phd log --id` with good (got logs) and bad IDs (got sensible error).
- Ran `bin/phd log` with any logs (got logs) and (simluated) without any logs (got error).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19755
Summary:
See T13212 for some context and discussion on this being revived.
See T11694 for original context.
Add a query constraint for lease owners and implement the conduit search method for Drydock leases.
Ref T11694. Fixes T13212.
Test Plan:
- Called the API method from conduit and browsed lease queries from the UI.
- Used the new "ownerPHIDs" constraint via API console.
{F5963044}
Reviewers: yelirekim, amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam, epriestley
Maniphest Tasks: T11694, T13212
Differential Revision: https://secure.phabricator.com/D16594
Summary:
Depends on D19753. Ref T13210. This is a small optimization that saves us from waiting up to 15 seconds for a yield.
When there are no Working Copy resources and a new lease comes in, we'll allocate one and yield until it activates.
If activating it (SSH'ing and running `git clone`) takes less than 15 seconds, the resource will activate (say, at T+4) but the lease won't update again for a while (say, until T+15). This leaves us with a pointless wait (in this example, we're sitting around for 9 seconds when we could move forward).
To improve this a little bit, let resources wake up the lease update tasks that triggered allocation after they activate. In the best case, that task runs ~15 seconds sooner. In the worst case, the awaken is just a no-op.
With a more-full queue, this has a smaller effect (it's likely something else will run and be able to use the resource in those 9 seconds).
With already-activated resources, this has no effect (when resources are already activated, we can lease immediately).
Test Plan:
- Cleaned up all working copy resources.
- Requested a new "A" working copy.
- Before patch: got a working copy after 17-18 seconds, most of which was spent yielded.
- After patch: got a working copy after 3-4 seconds.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19754
Summary:
Depends on D19752. Ref T13210. If resources take a long time to reclaim/destroy (normally, more than 15 seconds) a single new lease may update several times during the reclaim/destroy process and end up reclaiming multiple resources.
Instead: after a lease triggers a reclaim, prevent it from triggering another reclaim as long as the resource it is reclaiming hasn't finished its reclaim/destroy cycle. Basically, each lease only gets to destroy one resource at a time.
Test Plan:
- Added a `sleep(120)` to `destroyResource()` to simulate a long reclaim/destroy cycle.
- Allocated A, A, A working copies. Leased a B working copy.
- Before patch: saw "B" lease destroy all three "A" working copies after ~0, ~15, and ~30 seconds, then build a new "B" resource after ~120 seconds (when the first reclaim/destroy finished).
- After patch: saw "B" lease destroy one "A" working copy after ~0 seconds, then wait patiently until it finished up, then build a new "B" resource.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19753
Summary:
Depends on D19751. Ref T13210. When Drydock needs to reclaim an existing unused resource in order to build a new resource to satisfy a lease, the lease which triggered the reclaim currently gets thrown back into the pool with a 15-second yield.
If the queue is pretty empty and the reclaim is quick, this can mean that we spend up to 15 extra seconds just waiting for the lease update task to get another shot at building a resource (the resource reclaim may complete in a second or two, but nothing else happens until the yield ends).
Instead, when a lease triggers a reclaim, have the reclaim reawaken the lease task when it completes. In the best case, this saves us 15 seconds of waiting. In other cases (the task already completed some other way, the resource gets claimed before the lease gets to it), it's harmless.
Test Plan:
- Allocated A, A, A working copies with limit 3. Leased a B working copy.
- Before patch: allocation took ~32 seconds.
- After patch: allocation takes ~17 seconds (i.e., about 15 seconds less).
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19752
Summary:
Depends on D19750. See T13210. The `bin/drydock lease` command makes it easier to request ad-hoc leases, but currently takes lease attributes in the form `--attributes x=y,a=b`.
This was okay for all leases at the time, but doesn't really work for modern WorkingCopy resources since they take a `repositories.map` which has a dictionary as a value. You can't specify that with `repositories.map=...`.
Instead, point `--attributes` at a JSON file or use `--attributes -` to read from stdin.
Test Plan: Used `--attributes` with a file and stdin to allocate working copy leases with repositories.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D19751
Summary:
Ref T13210. We currently "git reset --hard HEAD" during working copy leasing, mostly by convention/familiarity.
However, this command does not work in an empty repository, because there is no HEAD yet.
The command "git reset --hard" appears to have the same meaning and effect in all cases, except that it also works correctly in an empty repository.
The manual suggests that omitting HEAD should be the same as specifying HEAD, too:
> The <tree-ish>/<commit> defaults to HEAD in all forms.
Test Plan: Successfully leased a working copy for an empty repository using Drydock.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19750
Summary:
Ref T13210. See PHI944. When parsing certain large diffs (the case in PHI944 is an 2.5-million line JSON diff), we spend ~66% of runtime and ~80% of memory doing copy detection (the little yellow bar which shows up to give you a hint that code was moved around within a diff).
This is pretty much pointless and copy hints are almost certainly never useful on large changes. Instead, just bail if the change is larger than some arbitrary "probably too big for copy hints to ever be useful" threshold (here, 65535 lines).
Test Plan:
Roughly, ran this against a 2.5 million line JSON diff:
```
$changes = id(new ArcanistDiffParser())->parseDiff($raw_diff);
$diff = DifferentialDiff::newFromRawChanges($viewer, $changes);
```
Before the changes, it took 20s + 2.5GB RAM to parse. After the changes, it took 7s + 500MB RAM to parse.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19748
Summary:
Ref T13210. See PHI937. This function datasource isn't quite implemented correctly: it doesn't resolve `package(project)` properly, since the logic only handles users.
This blames back to D14013, where it looks like `packages(..)` was added mostly as a general nice-to-have as part of a larger modernization change.
Test Plan: Ran a `packages(project)` query in Differential, got accurate results (previously: no results).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19747
Summary:
Depends on D19734. Ref T13202. Ref T13109. Ref T10884. See PHI905. See PHI889. We currently rank cluster nodes in three cases:
# when performing a write, we can go to any node (D19734 should make our ranking good);
# when performing a read, we can go to any node (currently random, but T10884 discusses ideas to improve our ranking);
# when performing an internal synchronization before a read or a write, we must go to an up-to-date node.
Currently, case (3) is not-exactly-deterministic but not random, and we won't spread intracluster traffic acrosss the cluster evenly if, say, half of it is up to date and half of it is still synchronizing. For a given write, I believe all nodes will tend to synchronize from whichever node first received the write today.
Instead, shuffle the list and synchronize from any up-to-date node.
(I think we could improve upon this only by knowing which nodes actually have load and selecting the least-loaded -- doable, but not trivial.)
Test Plan: Poked at it locally, will deploy to `secure`. This is hard to measure/test terribly convincingly.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13202, T13109, T10884
Differential Revision: https://secure.phabricator.com/D19735
Summary:
Ref T13109. Ref T13202. See PHI905. See PHI889. When we receive a write to a repository cluster, we currently send it to a random writable node.
Instead, we can prefer:
- the node currently holding the write lock; or
- any node which is already up to date.
These should simply be better nodes to take writes in all cases. The write lock is global for the repository, so there's no scaling benefit to spreading writes across different nodes, and these particular nodes will be able to accept the write more quickly.
Test Plan:
- This is observable by using `fprintf(STDERR, "%s\n", ...)` in the logic, then running `git push`. I'd like to pull this routing logic out of `PhabricatorRepository` at some point, probably into a dedicated `ClusterURIQuery` sort of class, but that is a larger change.
- Added some `fprintf(...)` stuff around which nodes were being selected.
- Added a `sleep(10)` after grabbing the write lock.
- In one window, pushed. Then pushed in a second window.
- Saw the second window select the lock holder as the write target based on it currently holding the lock.
- Without a concurrent push, saw pushes select up-to-date nodes based on their up-to-date-ness.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: joshuaspence, timhirsh
Maniphest Tasks: T13202, T13109
Differential Revision: https://secure.phabricator.com/D19734
Summary:
See PHI920. Ref T13210. Since the HTML is just:
```
<a>View Inline</a><span>filename.txt</span>
```
..double-clicking "filename.txt" in email selects "Inlinefilename.txt".
Add a space to stop this. At least in Safari, a space between the tags is not sufficient (perhaps because the parent is a `<div>`?). I couldn't find an authoritative-seeming source on what the rules for this actually are and adding a space here fixes the issue without changing the visual rendering, so just put it here.
Test Plan:
- Made an inline.
- Used `bin/mail show-outbound --id ... --dump-html` to dump the HTML.
- Double-clicked the filename.
{F5929186}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19742
Summary:
Depends on D19743. Ref T13210. Since this command can easily dump a bunch of binary data (or just a huge long blob of nonsense) to stdout, default to requiring "--output <file>".
Using `--output -` will print to stdout.
Test Plan: Ran with: no `--output`, `--output file`, `--output -`, `--output - --overwrite`. Got sensible results or errors in all cases.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19744
Summary: Ref T13210. See PHI806. This enables basic export of revisions into flat data formats. This isn't too fancy, but just covers the basics since the driving use case isn't especially concerned about getting all the fields and details.
Test Plan: Exported some revisions into JSON, got sensible output.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19743
Summary: Depends on D19738. Ref T13210. Currently, when you use "--overwrite", we just //append// the new content. Instead, actually overwrite the file.
Test Plan: Used `--overwrite`, saw an actual clean overwrite instead of an append.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19739
Summary:
Ref T13210. Minor usability improvements to "bin/bulk export":
- Allow `--class task` to work (previously, only `--class ManiphestTaskSearchEngine` worked).
- If you run `--query jXIlzQyOYHPU`, don't require `--class`, since the query identifies the class on its own.
- Allow users to call `--query A --query B --query C` and get a union of all results.
Test Plan:
- Ran `--class task`, `--query A --query B`, `--query X` (with no `--class`), got good results.
- Ran various flavors of bad combinations (queries from different engines, invalid engines, query and class differing, ambiguous/invalid `--class` name) and got sensible errors.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19738
Summary:
Ref T13202. See PHI900. Fixes T12814. Pholio currently builds HTML comments in an older way that can dump a bunch of over-escaped HTML into mail bodies.
Update the logic to be more similar to the Differential rendering logic and stop over-escaping things.
The result isn't perfect, but is dramatically less broken.
Test Plan: {F5919860}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13202, T12814
Differential Revision: https://secure.phabricator.com/D19733
Summary:
Ref T13202. See PHI906. This is a reasonable capability which we support in some other applications already.
(The only real reason not to support this is that it creates some clutter in the UI, but I think we're generally in better shape now than we were in the past, and we could make this UI collapse/fold at some point.)
Test Plan: Ran queries with a minimum date, a maximum date, both, and neither. Saw appropriate results in all cases.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13202
Differential Revision: https://secure.phabricator.com/D19732
Summary:
Ref T13202. See <https://discourse.phabricator-community.org/t/2fa-input-box-isnt-hinted-as-a-password-so-browsers-suggest-auto-fills/1959>.
If browsers are autofilling this, I think browser behavior here is bad, but behavior is probably better on the balance if we hint this as `autocomplete="off"` and this is a minor concesssion.
Test Plan:
- I couldn't immediately get any browser to try to autofill this field (perhaps I've disabled autofill, or just not enabled it aggressively?), but this change didn't break anything.
- After the change, answered a TOTP prompt normally.
- After the change, inspected page content and saw `autocomplete="off"` on the `<input />` node.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13202
Differential Revision: https://secure.phabricator.com/D19722
Summary:
Ref T13202. See PHI903 and PHI894. When a bot leaves 100 inline comments on the same file and the revision has a 30-member recipient list, we currently highlight the file 3000 times when building mail.
Instead, engage the parse cache so we highlight it once and reuse the cache 2,999 times.
Test Plan:
- Added debugging code to stop after mail generation and show cache hits/misses.
- Left a bunch of inlines in a file.
- Ran the worker with `bin/worker execute --id ...`.
- Before change: saw 4 comments x 2 recipients = 8 cache misses
- After change: saw 8 cache hits (cache already filled from web UI rendering)
- Removed debugging code, ran worker to completion.
- Used `bin/mail show-outbound --id ... --dump-html` to inspect resulting mail, saw no functional differences.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13202
Differential Revision: https://secure.phabricator.com/D19721
