Summary: Ref T13043. Adds CLI support for revoking SSH keys. Also retargets UI language from "Deactivate" to "Revoke" to make it more clear that this is a one-way operation. This operation is already correctly implemented as a "Revoke" operation.
Test Plan: Used `bin/auth revoke --type ssh` to revoke keys, verified they became revoked (with proper transactions) in the UI. Revoked keys from the web UI flow.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18893
Summary: Ref T13043. Allows CLI revocation of login sessions.
Test Plan: Used `bin/auth revoke --type session` with `--from` and `--everywhere` to revoke sessions. Saw accounts get logged out in web UI.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18892
Summary:
See PHI223. Ref T13024. There's a remaining registration/login order issue after the other changes in T13024: we lose track of the current URI when we go through the MFA flow, so we can lose "Set Password" at the end of the flow.
Specifically, the flow goes like this today:
- User clicks the welcome link in email.
- They get redirected to the "set password" settings panel.
- This gets pre-empted by Legalpad (although we'll potentially survive this with the URI intact).
- This also gets pre-empted by the "Set MFA" workflow. If the user completes this flow, they get redirected to a `/auth/multifactor/?id=123` sort of URI to highlight the factor they added. This causes us to lose the `/settings/panel/password/blah/blah?key=xyz` URI.
The ordering on this is also not ideal; it's preferable to start with a password, then do the other steps, so the user can return to the flow more easily if they are interrupted.
Resolve this by separating the "change your password" and "set/reset your password" flows onto two different pages. This copy/pastes a bit of code, but both flows end up simpler so it feels reasonable to me overall.
We don't require a full session for "set/reset password" (so you can do it if you don't have MFA/legalpad yet) and do it first.
This works better and is broadly simpler for users.
Test Plan:
- Required MFA + legalpad, invited a user via email, registered.
- Before: password set flow got lost when setting MFA.
- After: prompted to set password, then sign documents, then set up MFA.
- Reset password (with MFA confgiured, was required to MFA first).
- Tried to reset password without a valid reset key, wasn't successful.
- Changed password using existing flow.
- Hit various (all?) error cases (short password, common password, mismatch, missing password, etc).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13024
Differential Revision: https://secure.phabricator.com/D18840
Summary:
See <https://discourse.phabricator-community.org/t/activation-link-in-welcome-mail-only-works-if-new-user-isnt-semi-logged-in/740/7>.
In T13024, I rewrote the main menu bar to hide potentially sensitive items (like notification and message counts and saved search filters) until users fully log in.
However, the "Log In" item got caught in this too. For clarity, rename `shouldAllowPartialSessions()` to `shouldRequireFullSession()` (since logged-out users don't have any session at all, so it would be a bit misleading to say that "Log In" "allows" a partial session). Then let "Log In" work again for logged-out users.
(In most cases, users are prompted to log in when they take an action which requires them to be logged in -- like creating or editing an object, or adding comments -- so this item doesn't really need to exist. However, it aligns better with user expectations in many cases to have it present, and some reasonable operations like "Check if I have notifications/messages" don't have an obvious thing to click otherwise.)
Test Plan: Viewed site in an incognito window, saw "Log In" button again. Browsed normally, saw normal menu.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18818
Summary: Depends on D18791. Ref T13024. This clears up another initialization order issue, where an unverified address could prevent MFA enrollment.
Test Plan: Configured both verification required and MFA required, clicked "Add Factor", got a dialog for the workflow.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13024
Differential Revision: https://secure.phabricator.com/D18792
Summary:
Depends on D18790. Ref T13024. Fixes T8335. Currently, "unapproved" and "disabled" users are bundled together. This prevents users from completing some registration steps (verification, legalpad documents, MFA enrollment) before approval.
Separate approval out and move it to the end so users can do all the required enrollment stuff on their end before we roadblock them.
Test Plan: Required approval, email verification, signatures, and MFA. Registered an account. Verified email, signed documents, enrolled in MFA, and then got prompted to wait for approval.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13024, T8335
Differential Revision: https://secure.phabricator.com/D18791
Summary: Noticed a couple of typos in the docs, and then things got out of hand.
Test Plan:
- Stared at the words until my eyes watered and the letters began to swim on the screen.
- Consulted a dictionary.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D18693
Summary:
See PHI78. The user was getting this message and (reasonably) interpreted it to mean "reset mail can never be sent to unverified addresses".
Reword it to be more clear, albeit an entire paragraph long. I don't really have a good solution in these cases where we'd need a whole page to explain what's happening (this, plus "we can't tell you which address you should use because an attacker could get information if we did" and "this rule defuses the risk that an opportunistic attacker may try to compromise your account after you add an email you don't own by mistake"). We could write it up separately and link to it, but I feel like that stuff tends to get out of date.
Just land somewhere in the middle.
Test Plan: {F5189105}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18630
Summary: Ref T12964. This feels like a cheat, but works well. Just redirect the user back to the form they came from instead of to the key page.
Test Plan: Add a key to a user profile, add a key to an Alamanac device. Grep for PhabricatorAuthSSHKeyTableView and check all locations.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12964
Differential Revision: https://secure.phabricator.com/D18445
Summary: Cursory research indicates that "login" is a noun, referring to a form, and "log in" is a verb, referring to the action of logging in. I went though every instances of 'login' I could find and tried to clarify all this language. Also, we have "Phabricator" on the registration for like 4-5 times, which is a bit verbose, so I tried to simplify that language as well.
Test Plan: Tested logging in and logging out. Pages feel simpler.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18322
Summary:
Ref M1476. Currently, `setColor('simple')` is meaningful. Instead, `setButtonType('simple')`.
Depends on D18047.
Test Plan: Looked at UI examples, Phame, Auth. Notifications mooted by D18047.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18048
Summary:
Fixes T12554. The SSH key cache contains usernames, but is not currently dirtied on username changes.
An alternative solution would be to use user PHIDs instead of usernames in the file, which would make this unnecessary, but that would make debugging a bit harder. For now, I think this small added complexity is worth the easier debugging, but we could look at this again if cache management gets harder in the future.
Test Plan:
- Added a key as `ducksey`, ran `bin/ssh-auth`, saw key immediately.
- Renamed `ducksey` to `ducker`, ran `bin/ssh-auth`, saw username change immediately.
- Added another key as `ducker`, ran `bin/ssh-auth`, saw key immediately.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12554
Differential Revision: https://secure.phabricator.com/D17687
Summary: Ref T12509. This encourages code to move away from HMAC+SHA1 by making the method name more obviously undesirable.
Test Plan: `grep`, browsed around.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12509
Differential Revision: https://secure.phabricator.com/D17632
Summary:
Ref T12509. This adds support for HMAC+SHA256 (instead of HMAC+SHA1). Although HMAC+SHA1 is not currently broken in any sense, SHA1 has a well-known collision and it's good to look at moving away from HMAC+SHA1.
The new mechanism also automatically generates and stores HMAC keys.
Currently, HMAC keys largely use a per-install constant defined in `security.hmac-key`. In theory this can be changed, but in practice essentially no install changes it.
We generally (in fact, always, I think?) don't use HMAC digests in a way where it matters that this key is well-known, but it's slightly better if this key is unique per class of use cases. Principally, if use cases have unique HMAC keys they are generally less vulnerable to precomputation attacks where an attacker might generate a large number of HMAC hashes of well-known values and use them in a nefarious way. The actual threat here is probably close to nonexistent, but we can harden against it without much extra effort.
Beyond that, this isn't something users should really have to think about or bother configuring.
Test Plan:
- Added unit tests.
- Used `bin/files integrity` to verify, strip, and recompute hashes.
- Tampered with a generated HMAC key, verified it invalidated hashes.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12509
Differential Revision: https://secure.phabricator.com/D17630
Summary:
Ref T12464. This is a very old method which can return an existing file instead of creating a new one, if there's some existing file with the same content.
In the best case this is a bad idea. This being somewhat reasonable predates policies, temporary files, etc. Modern methods like `newFromFileData()` do this right: they share underlying data in storage, but not the actual `File` records.
Specifically, this is the case where we get into trouble:
- I upload a private file with content "X".
- You somehow generate a file with the same content by, say, viewing a raw diff in Differential.
- If the diff had the same content, you get my file, but you don't have permission to see it or whatever so everything breaks and is terrible.
Just get rid of this.
Test Plan:
- Generated an SSH key.
- Viewed a raw diff in Differential.
- (Did not test Phragment.)
Reviewers: chad
Reviewed By: chad
Subscribers: hach-que
Maniphest Tasks: T12464
Differential Revision: https://secure.phabricator.com/D17617
Summary:
Ref T11357. When creating a file, callers can currently specify a `ttl`. However, it isn't unambiguous what you're supposed to pass, and some callers get it wrong.
For example, to mean "this file expires in 60 minutes", you might pass either of these:
- `time() + phutil_units('60 minutes in seconds')`
- `phutil_units('60 minutes in seconds')`
The former means "60 minutes from now". The latter means "1 AM, January 1, 1970". In practice, because the GC normally runs only once every four hours (at least, until recently), and all the bad TTLs are cases where files are normally accessed immediately, these 1970 TTLs didn't cause any real problems.
Split `ttl` into `ttl.relative` and `ttl.absolute`, and make sure the values are sane. Then correct all callers, and simplify out the `time()` calls where possible to make switching to `PhabricatorTime` easier.
Test Plan:
- Generated an SSH keypair.
- Viewed a changeset.
- Viewed a raw diff.
- Viewed a commit's file data.
- Viewed a temporary file's details, saw expiration date and relative time.
- Ran unit tests.
- (Didn't really test Phragment.)
Reviewers: chad
Reviewed By: chad
Subscribers: hach-que
Maniphest Tasks: T11357
Differential Revision: https://secure.phabricator.com/D17616
Summary:
Ref T12313. This puts a UI on revoking credentials after a widespread compromise like Cloudbleed or a local one like copy/pasting a token into public chat.
For now, I'm only providing a revoker for conduit tokens since that's the immediate use case.
Test Plan:
- Revoked in user + type, everything + user, everywhere + type, and everything + everywhere modes.
- Verified that conduit tokens were destroyed in all cases.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12313
Differential Revision: https://secure.phabricator.com/D17458
Summary: Ref T10390. Simplifies dropdown by rolling out canUseInPanel in useless panels
Test Plan: Add a query panel, see less options.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10390
Differential Revision: https://secure.phabricator.com/D17341
Test Plan: attempted to create a new auth provider; observed that "enabled" ui element does not render. viewed existing auth provider and observed that "enabled" ui element still renders
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T12245
Differential Revision: https://secure.phabricator.com/D17337
Summary:
Ref T12140. The major effect of this change is that uninstalling "Home" (as we do on admin.phacility.com) no longer uninstalls the user menu (which is required to access settings or log out).
This also simplifies the code a bit, by consolidating how menus are built into MenuBarExtensions instead of some in Applications and some in Extensions.
Test Plan:
- While logged in and logged out, saw main menus in the correct order.
- Uninstalled Favorites, saw the menu vanish.
- Uninstalled Home, still had a user menu.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12140
Differential Revision: https://secure.phabricator.com/D17239
Summary:
Still lots to fix here, punting up since I'm running into a few roadblocks.
TODO:
[] Sort Personal/Global correctly
[] Quicksand in Help Items correctly on page changes
Test Plan: Verify new menus work on desktop, tablet, mobile. Test logged in menus, logged out menus. Logging out via a menu, verify each link works as expected. Help menus get build when using an app like Maniphest, Differential. Check that search works, preferences still save.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12107
Differential Revision: https://secure.phabricator.com/D17209
Summary: Fixes T11982. If an install is not public, the registering user may not be able to see the inviting user.
Test Plan: {F2097656}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11982
Differential Revision: https://secure.phabricator.com/D17015
Summary:
Persona is going to be decommed November 30th, 2016.
It is highly unlikely that anyone is currently using persona as a real
login method at this point.
Test Plan: tried locally to add auth adapter.
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D16371
Summary: This supports doing a bunch of sales funnel tracking on Phacility.
Test Plan: See next diff.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16890
Summary:
Ref T9304. This adds a "GuidanceEngine" which can generate "Guidance".
In practice, this lets third-party code (rSERVICES) remove and replace instructions in the UI, which is basically only usefulf or us to tell users to go read the documentation in the Phacility cluster.
The next diff tailors the help on the "Auth Providers" and "Create New User" pages to say "PHACILITY PHACILITY PHACILITY PHACILITY".
Test Plan: Browed to "Auth Providers" and "Create New User" on instanced and non-instanced installs, saw appropriate guidance.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9304
Differential Revision: https://secure.phabricator.com/D16861
Summary:
This has been replaced by `PolicyCodex` after D16830. Also:
- Rebuild Celerity map to fix grumpy unit test.
- Fix one issue on the policy exception workflow to accommodate the new code.
Test Plan:
- `arc unit --everything`
- Viewed policy explanations.
- Viewed policy errors.
Reviewers: chad
Reviewed By: chad
Subscribers: hach-que, PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D16831
Summary:
Ref T11469. This isn't directly related, but has been on my radar for a while: building SSH keyfiles (particular for installs with a lot of keys, like ours) can be fairly slow.
At least one cluster instance is making multiple clone requests per second. While that should probably be rate limited separately, caching this should mitigate the impact of these requests.
This is pretty straightforward to cache since it's exactly the same every time, and only changes when users modify SSH keys (which is rare).
Test Plan:
- Ran `bin/auth-ssh`, saw authfile generate.
- Ran it again, saw it read from cache.
- Changed an SSH key.
- Ran it again, saw it regenerate.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11469
Differential Revision: https://secure.phabricator.com/D16744
Summary:
Fixes T11586. First pass at a class for displaying invisible characters. Still need to:
- Write a couple unit tests
- Add some styling to the .invisible-special spans
- Actually start using the class when displaying form errors to users
Currently this makes the string `"\nab\x00c\x01d\te\nf"` look like:
{F1812711}
Test Plan:
Unit tests all pass and run in <1ms:
{F1812998}
Reviewers: epriestley, #blessed_reviewers, chad
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley, yelirekim
Maniphest Tasks: T11586
Differential Revision: https://secure.phabricator.com/D16541
Summary: For phabricator. Adds a Slack auth adapater and icon.
Test Plan:
Create a new Slack Application for login, generate id and secret. Activate login and registration for Slack. Create a new account with Slack credentials. Log out. Log in with Slack credentials. Set my avatar with Slack. Slack. Slack.
{F1802649}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16496
Summary: Ref T11132, significantly cleans up the Config app, new layout, icons, spacing, etc. Some minor todos around re-designing "issues", mobile support, and maybe another pass at actual Group pages.
Test Plan: Visit and test every page in the config app, set new items, resolve setup issues, etc.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam, Korvin
Maniphest Tasks: T11132
Differential Revision: https://secure.phabricator.com/D16468
Summary: Switches over to new property UI boxes, splits core and apps into separate pages. Move Versions into "All Settings". I think there is some docs I likely need to update here as well.
Test Plan: Click on each item in the sidebar, see new headers.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16429
Summary: I don't think we use footicons, removing that CSS. States were added but only used in Auth, convert them to statusIcon instead.
Test Plan: Visit Auth, UIExamples, grep for `setState`
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16418
Summary:
Fixes T11480. This cleans up the error logs a little by quieting three common errors which are really malformed requests:
- The CSRF error happens when bots hit anything which does write checks.
- The "wrong cookie domain" errors happen when bots try to use the `security.alternate-file-domain` to browse stuff like `/auth/start/`.
- The "no phcid" errors happen when bots try to go through the login flow.
All of these are clearly communicated to human users, commonly encountered by bots, and not useful to log.
I collapsed the `CSRFException` type into a standard malformed request exception, since nothing catches it and I can't really come up with a reason why anything would ever care.
Test Plan:
Hit each error through some level of `curl -H ...` and/or fakery. Verified that they showed to users before/after, but no longer log.
Hit some other real errors, verified that they log.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11480
Differential Revision: https://secure.phabricator.com/D16402
Summary:
Finishes fixing T11365. rP28199bcb48 added the new numeric entry
control and used it for TOTP setup, but missed the case of entering
a factor when TOTP was already set up.
Test Plan:
Observe behaviour of TOTP setup and subsequent factor entry
in iOS browser, make sure they're consistent.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Maniphest Tasks: T11365
Differential Revision: https://secure.phabricator.com/D16325
Summary:
Fixes T11365. I tested these variants:
- `<input type="number" />`
- `<input type="text" pattern="\d*" />`
Of these, this one (using `pattern`) appears to have the best behavior: it shows the correct keyboard on iOS mobile and does nothing on desktops.
Using `type="number"` causes unwanted sub-controls to appear in desktop Safari, and a numbers + symbols keyboard to appear on iOS (presumably so users can type "." and "-" and maybe ",").
Test Plan: Tested variants in desktop browsers and iOS simulator, see here and T11365 for discussion.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11365
Differential Revision: https://secure.phabricator.com/D16323
Summary: Fixes T11223. I missed a few of these; most of them kept working anyway because we have redirects in place, but make them a bit more modern/not-hard-coded.
Test Plan:
- Generated and revoked API tokens for myself.
- Generated and revoked API tokens for bots.
- Revoked temporary tokens for myself.
- Clicked the link to the API tokens panel from the Conduit console.
- Clicked all the cancel buttons in all the dialogs, too.
In all cases, everything now points at the correct URIs. Previously, some things pointed at the wrong URIs (mostly dealing with stuff for bots).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11223
Differential Revision: https://secure.phabricator.com/D16185
Summary:
Ref T11179. This splits "Edit Blocking Tasks" into two options now that we have more room ("Edit Parent Tasks", "Edit Subtasks").
This also renames "Blocking" tasks to "Subtasks", and "Blocked" tasks to "Parent" tasks. My goals here are:
- Make the relationship direction more clear: it's more clear which way is up with "parent" and "subtask" at a glance than with "blocking" and "blocked" or "dependent" and "dependency".
- Align language with "Create Subtask".
- To some small degree, use more flexible/general-purpose language, although I haven't seen any real confusion here.
Fixes T6815. I think I narrowed this down to two issues:
- Just throwing a bare exeception (we now return a dialog explicitly).
- Not killing open transactions when the cyclec check fails (we now kill them).
Test Plan:
- Edited parent tasks.
- Edited subtasks.
- Tried to introduce graph cycles, got a nice error dialog.
{F1697087}
{F1697088}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6815, T11179
Differential Revision: https://secure.phabricator.com/D16166
Summary: Fixes T11156. These were never correct, but also never actually used until I made timelines load object handles unconditionally in D16111.
Test Plan: Viewed an auth provider with transactions, no more fatal.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11156
Differential Revision: https://secure.phabricator.com/D16128
Summary:
Via HackerOne. This page fatals if accessed directly while logged out.
The "shouldRequireLogin()" check is wrong; this is a logged-in page.
Test Plan:
Viewed the page while logged out, no more fatal.
Faked my way through the actual verification flow.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16077
Summary: Fixes T11107. The URI change here meant we were dropping the "key" parameter, which allows you to set a new password without knowing your old one.
Test Plan: Reset password, didn't need to provide old one anymore.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11107
Differential Revision: https://secure.phabricator.com/D16075
Summary:
Ref T11098. This primarily fixes Conduit calls to `*.edit` methods failing when trying to access user preferences.
(The actual access is a little weird, since it seems like we're building some UI stuff inside a policy query, but that's an issue for another time.)
To fix this, consolidate the "we're about to run some kind of request with this user" code and run it consistently for web, conduit, and SSH sessions.
Additionally, make sure we swap things to the user's translation.
Test Plan:
- Ran `maniphest.edit` via `arc call-conduit`, no more settings exception.
- Set translation to ALL CAPS, got all caps output from `ssh` and Conduit.
Reviewers: avivey, chad
Reviewed By: chad
Maniphest Tasks: T11098
Differential Revision: https://secure.phabricator.com/D16066
Summary:
Ref T10785. Around the time we launched Phacility SAAS we implemented this weird autologin hack. It works fine, so clean it up, get rid of the `instanceof` stuff, and support it for any OAuth2 provider.
(We could conceivably support OAuth1 as well, but no one has expressed an interest in it and I don't think I have any OAuth1 providers configured correctly locally so it would take a little bit to set up and test.)
Test Plan:
- Configured OAuth2 adapters (Facebook) for auto-login.
- Saw no config option on other adapters (LDAP).
- Nuked all options but one, did autologin with Facebook and Phabricator.
- Logged out, got logout screen.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10785
Differential Revision: https://secure.phabricator.com/D16060
Summary:
Ref T4103. Ref T10078. This moves profile image caches to new usercache infrastructure.
These dirty automatically based on configuration and User properties, so add some stuff to make that happen.
This reduces the number of queries issued on every page by 1.
Test Plan: Browsed around, changed profile image, viewed as self, viewed as another user, verified no more query to pull this information on every page
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103, T10078
Differential Revision: https://secure.phabricator.com/D16040
Summary:
Ref T4103. Ref T10078. Currently, when a user misses a cache we just build it for them.
This is the behavior we want for the the viewer (so we don't have to build every cache up front if we don't actually need them), but not the right behavior for other users (since it allows performance problems to go undetected).
Make inline cache generation strict by default, then make sure all the things that rely on cache data request the correct data (well, all of the things identified by unit tests, at least: there might be some more stuff I haven't hit yet).
This fixes test failures in D16040, and backports a piece of that change.
Test Plan: Identified and then fixed failures with `arc unit --everything`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103, T10078
Differential Revision: https://secure.phabricator.com/D16042
Summary:
Ref T4103. Currently, we issue a `SELECT * FROM user_preferences ... WHERE userPHID = ...` on every page to load the viewer's settings.
There are several other questionable data accesses on every page too, most of which could benefit from improved caching strategies (see T4103#178122).
This query will soon get more expensive, since it may need to load several objects (e.g., the user's settings and their "role profile" settings). Although we could put that data on the User and do both in one query, it's nicer to put it on the Preferences object ("This inherits from profile X") which means we need to do several queries.
Rather than paying a greater price, we can cheat this stuff into the existing query where we load the user's session by providing a user cache table and doing some JOIN magic. This lets us issue one query and try to get cache hits on a bunch of caches cheaply (well, we'll be in trouble at the MySQL JOIN limit of 61 tables, but have some headroom).
For now, just get it working:
- Add the table.
- Try to get user settings "for free" when we load the session.
- If we miss, fill user settings into the cache on-demand.
- We only use this in one place (DarkConsole) for now. I'll use it more widely in the next diff.
Test Plan:
- Loaded page as logged-in user.
- Loaded page as logged-out user.
- Examined session query to see cache joins.
- Changed settings, saw database cache fill.
- Toggled DarkConsole on and off.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16001
Summary: Ref T10917. This is getting added as a link right now, which causes it to get `<a href>`'d in HTML mail. Add it as text instead.
Test Plan: Edited a key, examined HTML mail body carefully.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10917
Differential Revision: https://secure.phabricator.com/D15952
