Summary:
Depends on D20111. Ref T6703. Currently, each ExternalAccount row is tied to a provider by `providerType` + `providerDomain`. This effectively prevents multiple providers of the same type, since, e.g., two LDAP providers may be on different ports on the same domain. The `domain` also isn't really a useful idea anyway because you can move which hostname an LDAP server is on, and LDAP actually uses the value `self` in all cases. Yeah, yikes.
Instead, just bind each account to a particular provider. Then we can have an LDAP "alice" on seven different servers on different ports on the same machine and they can all move around and we'll still have a consistent, cohesive view of the world.
(On its own, this creates some issues with the link/unlink/refresh flows. Those will be updated in followups, and doing this change in a way with no intermediate breaks would require fixing them to use IDs to reference providerType/providerDomain, then fixing this, then undoing the first fix most of the way.)
Test Plan: Ran migrations, sanity-checked database. See followup changes for more comprehensive testing.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T6703
Differential Revision: https://secure.phabricator.com/D20112
Summary:
Depends on D20140. Ref T13250. Currently, the top-level exception handler doesn't dump stacks because we might not be in debug mode, and we might double-extra-super fatal if we call `PhabricatorEnv:...` to try to figure out if we're in debug mode or not.
We can get around this by setting a flag on the Sink once we're able to confirm that we're in debug mode. Then it's okay for the top-level error handler to show traces.
There's still some small possibility that showing a trace could make us double-super-fatal since we have to call a little more code, but AphrontStackTraceView is pretty conservative about what it does and 99% of the time this is a huge improvement.
Test Plan: {F6205122}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13250
Differential Revision: https://secure.phabricator.com/D20142
Summary:
Ref T13244. See PHI1059. When you lock a task, users who can edit the task can currently override the lock by using "Edit Task" if they confirm that they want to do this.
Mark these edits with an emblem, similar to the "MFA" and "Silent" emblems, so it's clear that they may have bent the rules.
Also, make the "MFA" and "Silent" emblems more easily visible.
Test Plan:
Edited a locked task, overrode the lock, got marked for it.
{F6195005}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: aeiser
Maniphest Tasks: T13244
Differential Revision: https://secure.phabricator.com/D20131
Summary: Ref T13244. See PHI1052. Our error handling for Stripe errors isn't great right now. We can give users a bit more information, and a less jarring UI.
Test Plan:
Before (this is in developer mode, production doesn't get a stack trace):
{F6197394}
After:
{F6197397}
- Tried all the invalid test codes listed here: https://stripe.com/docs/testing#cards
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13244
Differential Revision: https://secure.phabricator.com/D20132
Summary:
Ref T13244. See PHI1055. (Earlier, see D20091 and PHI1047.) Previously, we expanded the Owners package autoreview rules from "Yes/No" to several "Review (Blocking) If Non-Owner Author Not Subscribed via Package" kinds of rules. The sky didn't fall and this feature didn't turn into "Herald-in-Owners", so I'm comfortable doing something similar to the "Audit" rules.
PHI1055 is a request for a way to configure slightly different audit behavior, and expanding the options seems like a good approach to satisfy the use case.
Prepare to add more options by moving everything into a class that defines all the behavior of different states, and converting the "0/1" boolean column to a text column.
Test Plan:
- Created several packages, some with and some without auditing.
- Inspected database for: package state; and associated transactions.
- Ran the migrations.
- Inspected database to confirm that state and transactions migrated correctly.
- Reviewed transaction logs.
- Created and edited packages and audit state.
- Viewed the "Package List" element in Diffusion.
- Pulled package information with `owners.search`, got sensible results.
- Edited package audit status with `owners.edit`.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13244
Differential Revision: https://secure.phabricator.com/D20124
Summary:
Depends on D20122. Fixes T8029. Adds an "Approve User" action to the "Manage" page.
Users are normally approved from the "Approval Queue", but if you click into a user's profile to check them out in more detail it kind of dead ends you right now. I've occasionally hit this myself, and think this workflow is generally reasonable enough to support upstream.
Test Plan: {F6193742}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T8029
Differential Revision: https://secure.phabricator.com/D20123
Summary:
Depends on D20107. Ref T6703. Legalpad currently inserts "email" records into the external account table, but they're never used for anything and nothing else references them.
They also aren't necessary for anything important to work, and the only effect they have is making the UI say "External Account" instead of "None" under the "Account" column. In particular, the signatures still record the actual email address.
Stop doing this, remove all the references, and destroy all the rows.
(Long ago, Maniphest may also have done this, but no longer does. Nuance/Gatekeeper use a more modern and more suitable "ExternalObject" thing that I initially started adapting here before realizing that Legalpad doesn't actually care about this data.)
Test Plan: Signed documents with an email address, saw signature reflected properly in UI. Grepped for other callsites.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T6703
Differential Revision: https://secure.phabricator.com/D20108
Summary:
Depends on D20106. Ref T6703. Since I plan to change the `ExternalAccount` table, these migrations (which rely on `save()`) will stop working.
They could be rewritten to use raw queries, but I suspect few or no installs are affected. At least for now, just make them safe: if they would affect data, fatal and tell the user to perform a more gradual upgrade.
Also remove an `ALTER IGNORE TABLE` (this syntax was removed at some point) and fix a `%Q` when adjusting certain types of primary keys.
Test Plan: Ran `bin/storage upgrade --no-quickstart --force --namespace test1234` to get a complete migration since the beginning of time.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T6703
Differential Revision: https://secure.phabricator.com/D20107
Summary:
Depends on D20041. See PHI1046. If you do this:
- Create a parent project called "Crab" in Space 1.
- Create a milestone called "Left Claw".
- Shift "Crab" to Space 2.
- Create a milestone called "Right Claw".
...you currently end up with "Left Claw" in the wrong `spacePHID` in the database. At the application level it's in the correct space, but when we `WHERE ... AND spacePHID IN (...)` we can incorrectly filter it out.
Test Plan:
- Did the above setup.
- Saved "Crab", saw the space fix itself.
- Put things back in the broken state.
- Ran the migration script, saw things fix themselves again.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: aeiser, PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D20063
Summary:
Ref T13242. See PHI1039. Maniphest subtypes generally seem to be working well. I designed them as a general capability that might be extended to other `EditEngine` objects later, and PHI1039 describes a situation where extending subtypes to projects would give us some reasonable tools.
(Some installs also already use icons/colors as a sort of lightweight version of subtypes, so I believe this is generally useful capability.)
Some of this is a little bit copy-pasted and could probably be shared, but I'd like to wait a bit longer before merging it. For example, both configs have exactly the same structure right now, but Projects should possibly have some different flags (for example: to disable creating subprojects / milestones).
This implementation is pretty basic for now: notably, subprojects/milestones don't get the nice "choose from among subtype forms" treatment that tasks do. If this ends up being part of a solution to PHI1039, I'd plan to fill that in later on.
Test Plan: Defined multiple subtypes, created subtype forms, created projects with appropriate subtypes. Filtered them by subtype. Saw subtype information on list/detail views.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13242
Differential Revision: https://secure.phabricator.com/D20040
Summary:
Depends on D20026. Ref T13222. Ref T13231. The primary change here is that we'll no longer send you an SMS if you hit an MFA gate without CSRF tokens.
Then there's a lot of support for genralizing into Duo (and other push factors, potentially), I'll annotate things inline.
Test Plan: Implemented Duo, elsewhere.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13231, T13222
Differential Revision: https://secure.phabricator.com/D20028
Summary:
Depends on D20016. Ref T920. This does nothing interesting on its own since the TOTP provider has no guidance/warnings, but landing it separately helps to simplify an upcoming SMS diff.
SMS will have these guidance messages:
- "Administrator: you haven't configured any mailer which can send SMS, like Twilio."
- "Administrator: SMS is weak."
- "User: you haven't configured a contact number."
Test Plan: {F6151283} {F6151284}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T920
Differential Revision: https://secure.phabricator.com/D20017
Summary:
Depends on D20010. Ref T920. Allow users to designate which contact number is "primary": the number we'll actually send stuff to.
Since this interacts in weird ways with "disable", just do a "when any number is touched, put all of the user's rows into the right state" sort of thing.
Test Plan:
- Added numbers, made numbers primary, disabled a primary number, un-disabled a number with no primaries. Got sensible behavior in all cases.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T920
Differential Revision: https://secure.phabricator.com/D20011
Summary:
Ref T920. To send you SMS messages, we need to know your phone number.
This adds bare-bone basics (transactions, storage, editor, etc).
From here:
**Disabling Numbers**: I'll let you disable numbers in an upcoming diff.
**Primary Number**: I think I'm just going to let you pick a number as "primary", similar to how email works. We could imagine a world where you have one "MFA" number and one "notifications" number, but this seems unlikely-ish?
**Publishing Numbers (Profile / API)**: At some point, we could let you say that a number is public / "show on my profile" and provide API access / directory features. Not planning to touch this for now.
**Non-Phone Numbers**: Eventually this could be a list of other similar contact mechanisms (APNS/GCM devices, Whatsapp numbers, ICQ number, twitter handle so MFA can slide into your DM's?). Not planning to touch this for now, but the path should be straightforward when we get there. This is why it's called "Contact Number", not "Phone Number".
**MFA-Required + SMS**: Right now, if the only MFA provider is SMS and MFA is required on the install, you can't actually get into Settings to add a contact number to configure SMS. I'll look at the best way to deal with this in an upcoming diff -- likely, giving you partial access to more of Setings before you get thorugh the MFA gate. Conceptually, it seems reasonable to let you adjust some other settings, like "Language" and "Accessibility", before you set up MFA, so if the "you need to add MFA" portal was more like a partial Settings screen, maybe that's pretty reasonable.
**Verifying Numbers**: We'll probably need to tackle this eventually, but I'm not planning to worry about it for now.
Test Plan: {F6137174}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: avivey, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T920
Differential Revision: https://secure.phabricator.com/D19988
Summary:
Ref T13222. Users configure "Factor Configs", which say "I have an entry on my phone for TOTP secret key XYZ".
Currently, these point at raw implementations -- always "TOTP" in practice.
To support configuring available MFA types (like "no MFA") and adding MFA types that need some options set (like "Duo", which needs API keys), bind "Factor Configs" to a "Factor Provider" instead.
In the future, several "Factors" will be available (TOTP, SMS, Duo, Postal Mail, ...). Administrators configure zero or more "MFA Providers" they want to use (e.g., "Duo" + here's my API key). Then users can add configs for these providers (e.g., "here's my Duo account").
Upshot:
- Factor: a PHP subclass, implements the technical details of a type of MFA factor (TOTP, SMS, Duo, etc).
- FactorProvider: a storage object, owned by administrators, configuration of a Factor that says "this should be available on this install", plus provides API keys, a human-readable name, etc.
- FactorConfig: a storage object, owned by a user, says "I have a factor for provider X on my phone/whatever with secret key Q / my duo account is X / my address is Y".
Couple of things not covered here:
- Statuses for providers ("Disabled", "Deprecated") don't do anything yet, but you can't edit them anyway.
- Some `bin/auth` tools need to be updated.
- When no providers are configured, the MFA panel should probably vanish.
- Documentation.
Test Plan:
- Ran migration with providers, saw configs point at the first provider.
- Ran migration without providers, saw a provider created and configs pointed at it.
- Added/removed factors and providers. Passed MFA gates. Spot-checked database for general sanity.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19975
Summary: Depends on D19992. Ref T13222. If administrators provide a custom login message, show it on the login screen.
Test Plan:
{F6137930}
- Viewed login screen with and without a custom message.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19994
Summary:
Ref T13222. Long ago, we had a Config option (`welcome.html`) to let you dump HTML onto the login screen, but this was relatively hard to use and not good from a security perspective.
In some cases this was obsoleted by Dashboards, but there's at least some remaining set of use cases for actual login instructions on the login screen. For example, WMF has some guidance on //which// SSO mechanism to use based on what types of account you have. On `secure`, users assume they can register by clicking "Log In With GitHub" or whatever, and it might reduce frustration to tell them upfront that registration is closed.
Some other types of auth messaging could also either use customization or defaults (e.g., the invite/welcome/approve mail).
We could do this with a bunch of Config options, but I'd generally like to move to a world where there's less stuff in Config and more configuration is contextual. I think it tends to be easier to use, and we get a lot of fringe benefits (granular permissions, API, normal transaction logs, more abililty to customize workflows and provide contextual help/hints, etc). Here, for example, we can provide a remarkup preview, which would be trickier with Config.
This does not actually do anything yet.
Test Plan: {F6137541}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19992
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:
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: 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:
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 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. 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:
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 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 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 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 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 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:
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. 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 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 T13222. See PHI996. Ref T10743. For context, perhaps see T12171.
Node changed some signatures, behaviors, and error handling here in recent versions. As far as I can tell:
- The `script.runInNewContext(...)` method has never taken a `path` parameter, and passing the path has always been wrong.
- The `script.runInNewContext(...)` method started taking an `[options]` parameter at some point, and validating it, so the bad `path` parameter now throws.
- `vm.createScript(...)` is "soft deprecated" but basically fine, and keeping it looks more compatible.
This seems like the smallest and most compatible correct change.
Test Plan: Under Node 10, started Aphlict. Before: fatal error on bad `options` parameter to `runInNewContext()` (expected dictionary). After: notification server starts.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222, T10743
Differential Revision: https://secure.phabricator.com/D19860
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 T13216. See PHI985. If you disable cookies in Firefox, accessing `window.localStorage` throws an exception. Currently, this pretty much kills all scripts on the page.
Instead, catch and ignore this, as though `window.localStorage` was not defined.
Test Plan:
- Set Firefox to "no cookies".
- Loaded any page while logged out.
- Before: JS fatal early in the stack.
- After: page loads and JS works.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19832
Summary:
Ref T13216. See PHI985. When you click a line number to start an inline comment, we intend to initiate the action only if you used the left mouse button (desktop) or a touch (tablet/device).
We currently have a `not right` condition for doing this, but it only excludes right clicks, not middle clicks (or other nth-button clicks). The `not right` condition was sligthly easier to write, but use an `is left` condition instead of a `not right` condition.
Test Plan:
- In Safari, Firefox and Chrome:
- Used left click to start an inline.
- Used middle click to do nothing (previously: started an inline).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19836
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: See PHI977. Ref T13216. Some text, like long package names, may overflow hovercards. Add overflow CSS behaviors to remedy this.
Test Plan:
Before:
{F6012699}
After:
{F6012700}
(You can use `/search/hovercard/` to render hovercards in a handy standalone way.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19809
Summary:
Fixes T8440. See that task for discussion.
Ref T13216. See PHI976.
Test Plan:
In Chrome, hovered a timestamp and moved the mouse up to the "overlap" area (see T8440). Before: flickered like crazy. After: no flickering.
(I couldn't reproduce the original issue in modern Firefox or Safari.)
Reviewers: amckinley, avivey
Reviewed By: avivey
Maniphest Tasks: T8440, T13216
Differential Revision: https://secure.phabricator.com/D19808
Summary:
Ref T13216. See PHI959. These two recent migrations can be expressed more efficiently:
- When updating commit audit statuses, the field isn't JSON encoded or anything so we can just issue several bulk UPDATEs.
- When inserting mail keys, we can batch them in groups of 100.
Test Plan: Used `bin/storage upgrade -f --apply phabricator:...` to reapply patches. Saw equivalent behavior and faster runtimes.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19802
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 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:
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:
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:
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: Ref T13197. See PHI873. I want to give RepositoryOperation objects access to Drydock logging like leases, resources, and blueprints currently have. This just does the schema/query changes, no actual UI or new logging yet.
Test Plan: Ran storage upgrade, poked around the UI looking for anything broken.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13197
Differential Revision: https://secure.phabricator.com/D19671
