Summary:
Depends on D18907. Ref T13043. Ref T12509. We have some weird old password digest behavior that isn't terribly concerning, but also isn't great.
Specifically, old passwords were digested in weird ways before being hashed. Notably, account passwords were digested with usernames, so your password stops working if your username is chagned. Not the end of the world, but silly.
Mark all existing hashes as "v1", and automatically upgrade then when they're used or changed. Some day, far in the future, we could stop supporting these legacy digests and delete the code and passwords and just issue upgrade advice ("Passwords which haven't been used in more than two years no longer work."). But at least get things on a path toward sane, modern behavior.
Test Plan: Ran migration. Spot-checked that everthing in the database got marked as "v1". Used an existing password to login successfully. Verified that it was upgraded to a `null` (modern) digest. Logged in with it again.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043, T12509
Differential Revision: https://secure.phabricator.com/D18908
Summary:
Ref T13043. After D18903, this data has migrated to shared infrastructure and has no remaining readers or writers.
Just delete it now, since the cost of a mistake here is very small (users need to "Forgot Password?" and pick a new password).
Test Plan: Grepped for `passwordHash`, `passwordSalt`, and variations.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18904
Summary:
Ref T13043. This moves user account passwords to the new shared infrastructure.
There's a lot of code changes here, but essentially all of it is the same as the VCS password logic in D18898.
Test Plan:
- Ran migration.
- Spot checked table for general sanity.
- Logged in with an existing password.
- Hit all error conditions on "change password", "set password", "register new account" flows.
- Verified that changing password logs out other sessions.
- Verified that revoked passwords of a different type can't be selected.
- Changed passwords a bunch.
- Verified that salt regenerates properly after password change.
- Tried to login with the wrong password, which didn't work.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18903
Summary:
Ref T13043. In D18898 I moved VCS passwords to the new shared infrastructure.
Before account passwords can move, we need to make two changes:
- For legacy reasons, VCS passwords and Account passwords have different "digest" algorithms. Both are more complicated than they should be, but we can't easily fix it without breaking existing passwords. Add a `PasswordHashInterface` so that objects which can have passwords hashes can implement custom digest logic for each password type.
- Account passwords have a dedicated external salt (`PhabricatorUser->passwordSalt`). This is a generally reasonable thing to support (since not all hashers are self-salting) and we need to keep it around so existing passwords still work. Add salt support to `AuthPassword` and make it generate/regenerate when passwords are updated.
Then add a nice story about password digestion.
Test Plan: Ran migrations. Used an existing VCS password; changed VCS password. Tried to use a revoked password. Unit tests still pass. Grepped for callers to legacy `PhabricatorHash::digestPassword()`, found none.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18900
Summary:
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: 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 PHI79. When you edit another user's SSH keys (normally, for a bot account) we currently redirect you to an older URI.
Test Plan:
- Viewed a bot's profile page.
- Clicked "Edit Settings" on the Manage page.
- Went to "SSH Keys".
- Uploaded an SSH key.
- Before: redirected to a 404 after finishing the workflow.
- After: redirected to the same page after the workflow.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18633
Summary:
See brief discussion in D18554. All the index tables are the same for every application (and, at this point, seem unlikely to change) and we never actually pass these objects around (they're only used internally).
In some other cases (like Transactions) not every application has the same tables (for example, Differential has extra field for inline comments), and/or we pass the objects around (lots of stuff uses `$xactions` directly).
However, in this case, and in Edges, we don't interact with any representation of the database state directly in much of the code, and it doesn't change from application to application.
Just automatically define document, field, and ngram tables for anything which implements `FerretInterface`. This makes the query and index logic a tiny bit messier but lets us delete a ton of boilerplate classes.
Test Plan: Indexed objects, searched for objects. Same results as before with much less code. Ran `bin/storage upgrade`, got a clean bill of health.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18559
Summary:
Ref T12819. Adds support for indexing user accounts so they appear in global fulltext results.
Also, always rank users ahead of other results.
Test Plan: Indexed users. Searched for a user, got that user.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18552
Summary:
Ref T11816. Two minor issues:
- We used `$event`, not `$next_event`, as the event providing the PHID for "Busy at <event name>". This rendered "Busy at <most future event>" on the profile instead of "Busy at <next upcoming event".
- The TTL computation used the event start, not the event end, so we could end up rebuilding the cache too often for users busy at an event.
Test Plan:
- Attended an event in the near future and one later on.
- Saw profile now say "busy at <near future event>" correctly.
- In DarkConsole "Services" tab, no longer saw unnecessary cache refills while attending an event.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11816
Differential Revision: https://secure.phabricator.com/D17643
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 T12270. Builds out a BadgeCache for PhabricatorUser, primarily for Timeline, potentially feed? This should still work if we later let people pick which two, just switch query in BadgeCache.
Test Plan: Give out badges, test timeline for displaying badges from handles and without queries. Revoke a badge, see cache change.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12270
Differential Revision: https://secure.phabricator.com/D17503
Summary: Fixes T10319. This looks for custom profile image, then falls back to a generated profile image.
Test Plan: Create a new user, log in, and see new profile image. Note this seems to break `bin/lipsum generate user`
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10319
Differential Revision: https://secure.phabricator.com/D17467
Summary: Ref T10319. Adds in database columns for upcoming default generated avatar support.
Test Plan: Ran storage upgrade, log into local site to verify it didn't blow up.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10319
Differential Revision: https://secure.phabricator.com/D17459
Summary:
Fixes T12172. Fixes T12060. This allows runtime code building CSS for mail to read CSS variables, then makes all the code do that.
It reverts the non-colorblind red/green to the colors in use before T12060, which seem better for non-colorblind users since no one really complained?
Test Plan:
- Viewed code diffs in Web UI.
- Viewed prose diffs in Web UI.
- Viewed code diffs in email.
- Viewed prose diffs in email.
All modes respected the accessibility color scheme.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12172, T12060
Differential Revision: https://secure.phabricator.com/D17269
Summary:
Ref T12268. Ref T12157. When you mention or interact with a user who is unlikely to be able to respond (for example, because their account is disabled), we try to show a colored dot to provide a hint about this.
Recently, we no longer send any normal mail to unverified addresses. However, the rules for showing a dot haven't been updated yet, so they only care about this if `auth.require-verification` is set. This can be misleading, because if you say `Hey @alice, what do you think about this?` and she hasn't verified her email, you may not get a response.
Update the rule so users with unverified email addresses get a grey dot in all cases. The hint is basically "you shouldn't expect a response from this user".
Make the meaning of this hint more clear on the hovercard and profile.
Also:
- Allow the non-ajax version of the hovercard page (which is basically only useful for testing hovercards) accept `?names=...` so you can just plug usernames, hashtags, etc., in there.
- Fix a bug where the user's join date was based on their profile creation date instead of account creation date on the hovercard. Users may not have a profile creation date (if they never changed any account details), and it may be different from their account creation date.
Test Plan: {F2998517}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12268, T12157
Differential Revision: https://secure.phabricator.com/D17374
Summary:
Ref T11954. In cluster configurations, we get repository information by making HTTP calls over Conduit.
These are slower than local calls, so clustering imposes a performance penalty. However, we can use futures and parallelize them so that clustering actually improves overall performance.
When not running in clustered mode, this just makes us run stuff inline.
Test Plan:
- Browsed Git, Mercurial and Subversion repositories.
- Locally, saw a 700ms wall time page drop to 200ms.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11954
Differential Revision: https://secure.phabricator.com/D17009
Summary:
Ref T11954. This reduces how much work we need to do to load settings, particularly for Conduit (which currently can not benefit directly from the user cache, because it loads the user indirectly via a token).
Specifically:
- Cache builtin defaults in the runtime cache. This means Phabricator may need to be restarted if you change a global setting default, but this is exceptionally rare.
- Cache global defaults in the mutable cache. This means we do less work to load them.
- Avoid loading settings classes if we don't have to.
- If we missed the user cache for settings, try to read it from the cache table before we actually go regenerate it (we miss on Conduit pathways).
Test Plan: Used `ab -n100 ...` to observe a ~6-10ms performance improvement for `user.whoami`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11954
Differential Revision: https://secure.phabricator.com/D16998
Summary:
Ref T11939. IPv4 addresses can normally only be written in one way, but IPv6 addresses have several formats.
For example, the addresses "FFF::", "FfF::", "fff::", "0ffF::", "0fFf:0::", and "0FfF:0:0:0:0:0:0:0" are all the same address.
Normalize all addresses before writing them to logs, etc, so we store the most-preferred form ("fff::", above).
Test Plan:
Ran an SSH clone over IPv6:
```
$ git fetch ssh://local@::1/diffusion/26/locktopia.git
```
It worked; verified that address read out of `SSH_CLIENT` sensibly.
Faked my remote address as a non-preferred-form IPv6 address using `preamble.php`.
Failed to login, verified that the preferred-form version of the address appeared in the user activity log.
Made IPv6 requests over HTTP:
```
$ curl -H "Host: local.phacility.com" "http://[::1]/"
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11939
Differential Revision: https://secure.phabricator.com/D16987
Summary: Fixes T11917. Give logged-out / omnipotent users the global settings, not the default settings.
Test Plan: Changed applications and language, logged out, saw changes as a public user.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11917
Differential Revision: https://secure.phabricator.com/D16936
Summary:
Ref T5267. When extrating data from `pht()` calls, also extract the argument types and export them into the map so they can be used by consumers.
We recognize plurals (`phutil_count()`, `new PhutilNumber`) and genders (`phutil_person()`). We'll need to annotate the codebase for those, since they're currently runtime-only.
Test Plan:
Rebuilt extraction maps, got data like this (note "number" type annotation).
```
"Scaling pool \"%s\" up to %s daemon(s).": {
"uses": [
{
"file": "/daemon/PhutilDaemonOverseer.php",
"line": 378
}
],
"types": [
null,
"number"
]
},
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5267
Differential Revision: https://secure.phabricator.com/D16823
Summary:
Ref T11816.
- Now that we can do something meaningful with them, bring back the yellow dots for "busy".
- Default to "busy" when attending events (we could make this "busy" for short events and "away" for long events or something).
- Let users pick how to display their attending status on the event page.
- Also show which event the user is attending since I had to mess with the cache code anyway. We can get rid of this again if it doesn't feel good.
Test Plan:
{F1904179}
{F1904180}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11816
Differential Revision: https://secure.phabricator.com/D16802
Summary:
Ref T11809. We show a red dot next to a username to indicate that the user is away (on vacation, in a meeting, etc).
It's not very obvious what this means unless you know that's what it is: when you click the username or view a hovercard, there's no visual hint about what the red dot means. It does say "Away", but there is a lot of information and it doesn't visually connect the two.
Connect the two visually by putting a red dot next to the "Away" bit, too.
Test Plan:
Here's my version of it, this feels OK to me but could maybe be more designed:
{F1893916}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11809
Differential Revision: https://secure.phabricator.com/D16791
Summary: Fixes T8850. Previously, if a user's preamble script mangled `$_SERVER['REMOTE_ADDR']` or somehow set it to `null`, the user would get errors when performing certain actions. Now those errors shouldn't occur, and instead the user will be warned that there is a setup issue related to their preamble script.
Test Plan: Create a preamble script that contains `$_SERVER['REMOTE_ADDR'] = null;` then navigate to /config/issue/. There should be a warning there about `REMOTE_ADDR` not being available.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, yelirekim, epriestley
Maniphest Tasks: T8850
Differential Revision: https://secure.phabricator.com/D16450
Summary:
Fixes T10633. When generating email about a transaction which adjusts a date, render the offset explicitly (like "UTC-7").
This makes it more clear in cases like this:
- mail is being sent to multiple users, and not necessarily using the viewer's settings;
- you get some mail while travelling and aren't sure which timezone setting it generated under.
Test Plan: Rendered in text mode, saw UTC offset.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10633
Differential Revision: https://secure.phabricator.com/D16287
Summary: Fixes T11198. These are confusing or premature if you aren't an activated user: disabled or unapproved accounts won't be able to act on them.
Test Plan: Changed timezone, went through flow to correct it
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11198
Differential Revision: https://secure.phabricator.com/D16167
Summary:
Ref T4103. Two small improvements:
- Don't work as hard to validate translations. We just need to know if a translation exists, we don't need to count how many strings it has and build the entire menu.
- Allow `getUserSetting()` to work on any setting without doing all the application/visibility checks. It's OK for code to look at, say, your "Conpherence Notifications" setting even if that application is not installed for you.
Test Plan: Used XHProf and saw 404 page drop from ~60ms to ~40ms locally.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16046
Summary: Ref T4103. Ref T10078. We currently have separate "usable" and "raw" values, but can simplify this by making `newValueForUsers()` return the raw value.
Test Plan: Ran unit tests; browsed around; dropped caches and browsed around.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103, T10078
Differential Revision: https://secure.phabricator.com/D16043
Summary:
Ref T4103. Ref T10078. This puts a user cache in front of notification and message counts.
This reduces the number of queries issued on every page by 4 (2x building the menu, 2x building Quicksand data).
Also fixes some minor issues:
- Daemons could choke on sending mail in the user's translation.
- No-op object updates could fail in the daemons.
- Questionable data access pattern in the file query coming out of the profile file cache.
Test Plan:
- Sent myself notifications. Saw count go up.
- Cleared them by visiting objects and clearing all notifications. Saw count go down.
- Sent myself messages. Saw count go up.
- Cleared them by visiting threads. Saw count go down.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103, T10078
Differential Revision: https://secure.phabricator.com/D16041
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. This method has no more callers.
Test Plan: `grep`
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16039
Summary: Ref T4103. Also get rid of the weird cache clear that nothing else uses and which we don't actually need.
Test Plan:
- Resolved timezone conflict by ignoring it.
- Resolved timezone conflict by picking a valid timezone.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16037
Summary:
Ref T4103. These settings long-predate proper settings and are based on hard-coded user properties. Turn them into real settings.
(I didn't try to migrate the value since they're trivial to restore and only useful to developers.)
Test Plan:
- Toggled console on/off.
- Swapped tabs.
- Reloaded page, everything stayed sticky.
Reviewers: chad
Reviewed By: chad
Subscribers: eadler
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16029
Summary:
Ref T4103. Convert this into a proper internal setting and use transactions to mutate it.
Also remove some no-longer-used old non-modular settings constants.
Test Plan:
- Used policy dropdown, saw recently-used projects.
- Selected some new projects, saw them appear.
- Grepped for all removed constants.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16027
Summary:
Ref T4103. Conpherence is doing some weird stuff and has its own redudnant settings object.
- Get rid of `ConpherenceSettings`.
- Use `getUserSetting()` instead of `loadPreferences()`.
- When applying transactions, add a new mechanism to efficiently prefill caches (this will still work anyway, but it's slower if we don't bulk-fetch).
Test Plan:
- Changed global Conpherence setting.
- Created a new Conpherence, saw setting set to global default.
- Changed local room setting.
- Submitted messages.
- Saw cache prefill for all particpiants in database.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16025
Summary:
Ref T4103. Some settings (like the collapsed/expanded state of the diff filetree) are currently ad-hoc. They weren't being read correctly.
Also, simplify the caching code a little bit.
Test Plan: Toggled filetree, reloaded page, got sticky behavior.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16021
Summary:
Ref T4103. This is just incremental cleanup:
- Add "internal" settings, which aren't editable via the UI. They can still do validation and run through the normal pathway. Move a couple settings to use this.
- Remove `getPreference()` on `PhabricatorUser`, which was a sort of prototype version of `getUserSetting()`.
- Make `getUserSetting()` validate setting values before returning them, to improve robustness if we change allowable values later.
- Add a user setting cache, since reading user settings was getting fairly expensive on Calendar.
- Improve performance of setting validation for timezone setting (don't require building/computing all timezone offsets).
- Since we have the cache anyway, make the timezone override a little more general in its approach.
- Move editor stuff to use `getUserSetting()`.
Test Plan:
- Changed search scopes.
- Reconciled local and server timezone settings by ignoring and changing timezones.
- Changed date/time settings, browsed Calendar, queried date ranges.
- Verified editor links generate properly in Diffusion.
- Browsed around with time/date settings looking at timestamps.
- Grepped for `getPreference()`, nuked all the ones coming off `$user` or `$viewer` that I could find.
- Changed accessiblity to high-contrast colors.
- Ran all unit tests.
- Grepped for removed constants.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16015
Summary:
Ref T4103. These are currently stored on the user, for historic/performance reasons.
Since I want administrators to be able to set defaults for translations and timezones at a minimum and there's no longer a meaningful performance penalty for moving them off the user record, turn them into real preferences and then nuke the columns.
Test Plan:
- Set settings to unusual values.
- Ran migrations.
- Verified my unusual settings survived.
- Created a new user.
- Edited all settings with old and new UIs.
- Reconciled client/server timezone disagreement.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16005
Summary:
Ref T4103. This doesn't get everything, but takes care of most of the easy stuff.
The tricky-ish bit here is that I need to move timezones, pronouns and translations to proper settings. I expect to pursue that next.
Test Plan:
- Grepped for `loadPreferences` to identify callsites.
- Changed start-of-week setting, loaded Calendar, saw correct start.
- Visited welcome page, read "Adjust Settings" point.
- Loaded Conpherence -- I changed behavior here slightly (switching threads drops the title glyph) but it wasn't consistent to start with and this seems like a good thing to push to the next version of Conpherence.
- Enabled Filetree, toggled in Differential.
- Disabled Filetree, no longer visible in Differential.
- Changed "Unified Diffs" preference to "Small Screens" vs "Always".
- Toggled filetree in Diffusion.
- Edited a task, saw sensible projects in policy dropdown.
- Viewed user profile, uncollapsed/collapsed side nav, reloaded page, sticky'd.
- Toggled "monospaced textareas", used a comment box, got appropriate fonts.
- Toggled durable column.
- Disabled title glyphs.
- Changed monospaced font to 18px/36px impact.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16004
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 T4103. This starts breaking out settings in a modern way to prepare for global defaults.
Test Plan:
- Edited diff settings.
- Saw them take effect in primary settings pane.
- Set stuff to new automatic defaults.
- Tried to edit another user's settings.
- Edited a bot's settings as an administrator.
{F1669077}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D15995
Summary:
Ref T4103. This give preferences a PHID, policy/transaction interfaces, a transaction table, and a Query class.
This doesn't actually change how they're edited, yet.
Test Plan:
- Ran migrations.
- Inspected database for date created, date modified, PHIDs.
- Changed some of my preferences.
- Deleted a user's preferences, verified they reset properly.
- Set some preferences as a new user, got a new row.
- Destroyed a user, verified their preferences were destroyed.
- Sent Conpherence messages.
- Send mail.
- Tried to edit another user's settings.
- Tried to edit a bot's settings as a non-admin.
- Edited a bot's settings as an admin (technically, none of the editable settings are actually stored in the settings table, currently).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D15991
Summary:
Ref T10769. The user availability cache write shouldn't happen in read-only mode, nor should the Differential parse cache write.
(We might want to turn off the availbility feature completely since it's potentially expensive if we can't cache it, but I think we're OK for now.)
Test Plan:
In read-only mode:
- Browsed as a user with an out-of-date availability cache.
- Loaded an older revision without cached parse data.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10769
Differential Revision: https://secure.phabricator.com/D15988
Summary: Ref T10512. This is fairly bare-bones but appears to work.
Test Plan: Queried all users, queried some stuff by constraints.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10512
Differential Revision: https://secure.phabricator.com/D15959
Summary: Ref T3025. This adds a check for different client/server timezone offsets and gives users an option to fix them or ignore them.
Test Plan:
- Fiddled with timezone in Settings and System Preferences.
- Got appropriate prompts and behavior after simulating various trips to and from exotic locales.
In particular, this slightly tricky case seems to work correctly:
- Travel to NY.
- Ignore discrepancy (you're only there for a couple hours for an important meeting, and returning to SF on a later flight).
- Return to SF for a few days.
- Travel back to NY.
- You should be prompted again, since you left the timezone after you ignored the discrepancy.
{F1654528}
{F1654529}
{F1654530}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T3025
Differential Revision: https://secure.phabricator.com/D15961
Summary:
Ref T10917. This cheats fairly heavily to generate SSH key mail:
- Generate normal transaction mail.
- Force it to go to the user.
- Use `setForceDelivery()` to force it to actually be delivered.
- Add some warning language to the mail body.
This doesn't move us much closer to Glorious Infrastructure for this whole class of events, but should do what it needs to for now and doesn't really require anything sketchy.
Test Plan: Created and edited SSH keys, got security notice mail.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10917
Differential Revision: https://secure.phabricator.com/D15948
Summary:
Ref T10917. Currently, when you delete an SSH key, we really truly delete it forever.
This isn't very consistent with other applications, but we built this stuff a long time ago before we were as rigorous about retaining data and making it auditable.
In partiular, destroying data isn't good for auditing after security issues, since it means we can't show you logs of any changes an attacker might have made to your keys.
To prepare to improve this, stop destoying data. This will allow later changes to become transaction-oriented and show normal transaction logs.
The tricky part here is that we have a `UNIQUE KEY` on the public key part of the key.
Instead, I changed this to `UNIQUE (key, isActive)`, where `isActive` is a nullable boolean column. This works because MySQL does not enforce "unique" if part of the key is `NULL`.
So you can't have two rows with `("A", 1)`, but you can have as many rows as you want with `("A", null)`. This lets us keep the "each key may only be active for one user/object" rule without requiring us to delete any data.
Test Plan:
- Ran schema changes.
- Viewed public keys.
- Tried to add a duplicate key, got rejected (already associated with another object).
- Deleted SSH key.
- Verified that the key was no longer actually deleted from the database, just marked inactive (in future changes, I'll update the UI to be more clear about this).
- Uploaded a new copy of the same public key, worked fine (no duplicate key rejection).
- Tried to upload yet another copy, got rejected.
- Generated a new keypair.
- Tried to upload a duplicate to an Almanac device, got rejected.
- Generated a new pair for a device.
- Trusted a device key.
- Untrusted a device key.
- "Deleted" a device key.
- Tried to trust a deleted device key, got "inactive" message.
- Ran `bin/ssh-auth`, got good output with unique keys.
- Ran `cat ~/.ssh/id_rsa.pub | ./bin/ssh-auth-key`, got good output with one key.
- Used `auth.querypublickeys` Conduit method to query keys, got good active keys.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10917
Differential Revision: https://secure.phabricator.com/D15943
Summary:
Ref T10784. On `secure`, logged-out users currently can't browse repositories when cluster/service mode is enabled because they aren't permitted to make intracluster requests.
We don't allow totally public external requests (they're hard to rate limit and users might write bots that polled `feed.query` or whatever which we'd have no way to easily disable) but it's fine to allow intracluster public requests.
Test Plan: Browsed a clustered repository while logged out locally.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10784
Differential Revision: https://secure.phabricator.com/D15695
Summary: Ref T7673. This is really just so I can force admin.phacility.com logout when you log out of an instance, but there are a few other things we could move here eventually, like the WILLREGISTERUSER event.
Test Plan: Logged out of an instance, got logged out of parent (see next change).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7673
Differential Revision: https://secure.phabricator.com/D15629
