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
Summary: I think I like this better -- but maybe right-aligned?
Test Plan:
{F1180295}
{F1180296}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15495
Summary: Ref T10054. This primarily improves aesthetics and consistency for member/wathcher lists in projects.
Test Plan:
{F1068873}
{F1068874}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10054
Differential Revision: https://secure.phabricator.com/D15103
Summary: Ref T4245. Pass the whole repository in so it can do something else in a future change.
Test Plan: Loaded changesets in Diffusion.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14931
Summary: Ref T9979. This simplifies/standardizes the code a bit, but mostly gives us more consistent class names and structure.
Test Plan:
- Used `bin/search index --type ...` to index documents of every indexable type.
- Searched for documents by unique text, found them.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9979
Differential Revision: https://secure.phabricator.com/D14842
Summary:
Ref T9890. Ref T9979. Several adjacent goals:
- The `SearchEngine` vs `ApplicationSearchEngine` thing is really confusing. There are also a bunch of confusing class names and class relationships within the fulltext indexing. I want to rename these classes to be more standard (`IndexEngine`, `IndexEngineExtension`, etc). Rename `SearchIndexer` to `IndexEngine`. A future change will rename `SearchEngine`.
- Add the index locks described in T9890.
- Structure things a little more normally so future diffs can do the "EngineExtension" thing more cleanly.
Test Plan:
Indexing:
- Renamed a task to have a unique word in the title.
- Ran `bin/search index Txxx`.
- Searched for unique word.
- Found task.
Locking:
- Added a `sleep(10)` after the `lock()` call.
- Ran `bin/search index Txxx` in two windows.
- Saw first one lock, sleep 10 seconds, index.
- Saw second one give up temporarily after failing to grab the lock.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9890, T9979
Differential Revision: https://secure.phabricator.com/D14834
Summary:
Fixes T9446. We allow administrators to send "Welcome" mail to bots and mailing lists.
This is harmless (these links do not function), but confusing.
Instead, disable this option in the UI and explain why it is disabled when it is clicked. Also prevent generation of this mail lower in the stack.
Test Plan:
- Viewed a bot page, saw action disabled, clicked it, got explanation.
- Viewed a normal user page, saw action enabled, clicked it, sent welcome email.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9446
Differential Revision: https://secure.phabricator.com/D14134
Summary: See D14025. In all cases where we compare hashes, use strict, constant-time comparisons.
Test Plan: Logged in, logged out, added TOTP, ran Conduit, terminated sessions, submitted forms, changed password. Tweaked CSRF token, got rejected.
Reviewers: chad
Reviewed By: chad
Subscribers: chenxiruanhai
Differential Revision: https://secure.phabricator.com/D14026
Summary: Shows badges on profile if you have them. Check if app is installed, show badges.
Test Plan:
Gave myself a liberal selection of badge. Gave notchad one badge. Gave chadtwo absolutely nothing.
{F651069}
Reviewers: btrahan, lpriestley, epriestley
Reviewed By: epriestley
Subscribers: johnny-bit, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13692
Summary: Ref T8888, Makes People Flaggable (and makes me wonder if we should rename Flags->Bookmarks).
Test Plan: Flag myself. Get excited.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: joshuaspence, epriestley, Korvin
Maniphest Tasks: T8888
Differential Revision: https://secure.phabricator.com/D13654
Summary:
Ref T8631. The query plan for feed stories is really bad right now, because we miss caches we should be hitting:
- The workspace cache is stored at each query, so adjacent queries can't benefit from the cache (only subqueries). Feed has primarily sibling queries.
- There is no technical reason to do this. Store the workspace cache on the root query, so sibling queries can hit it.
- In `ObjectQuery`, we check the workspace once, then load all the PHIDs. When the PHIDs are a mixture of transactions and objects, we always miss the workspace and load the objects twice.
- Instead, check the workspace after loading each type of object.
- `HandleQuery` does not set itself as the parent query for `ObjectQuery`, so handles never hit the workspace cache.
- Pass it, so they can hit the workspace cache.
- Feed's weird `PhabricatorFeedStory::loadAllFromRows()` method does not specify a parent query on its object/handle queries.
- Just declare the object query to be the "root" query until this eventually gets cleaned up.
Test Plan: Saw queries for each object drop from 4-6x to 1x in `/feed/`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8631
Differential Revision: https://secure.phabricator.com/D13479
Summary:
Fixes T8601. To reproduce the problem:
- Set your time preference to `""` (the empty string). This isn't possible from the modern UI, but can be done with "Right Click > Inspect Element", or users may have carried it forward from an older setting (this is the case with me and @hach-que on this install).
- Load Calendar with some events.
- This parses an epoch, which sets `valueTime` to `""` (since there are no format characters in the preference) and then `getEpoch()` fails because `strlen($time)` is 0.
- Since `getEpoch()` failed, `getDateTime()` also fails.
To fix this:
- Only permit the date and time preferences to have valid values.
Test Plan:
- Loaded page before patch, saw fatal.
- Applied patch.
- No more fatal.
- Viewed tooltips, dates/times, dates/times in other apps.
- Changed my preferences, saw them respected.
Reviewers: lpriestley
Reviewed By: lpriestley
Subscribers: epriestley, hach-que
Maniphest Tasks: T8601
Differential Revision: https://secure.phabricator.com/D13346
Summary:
Ref T8377. This adds a standard disable/enable feature to Spaces, with a couple of twists:
- You can't create new stuff in an archived space, and you can't move stuff into an archived space.
- We don't show results from an archived space by default in ApplicationSearch queries. You can still find these objects if you explicitly search for "Spaces: <the archived space>".
So this is a "put it in a box in the attic" sort of operation, but that seems fairly nice/reasonable.
Test Plan:
- Archived and activated spaces.
- Used ApplicationSearch, which omitted archived objects by default but allowed searches for them, specifically, to succeed.
- Tried to create objects into an archived space (this is not allowed).
- Edited objects in an archived space (this is OK).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8377
Differential Revision: https://secure.phabricator.com/D13238
Summary:
Ref T8449. Try out some more subtle behaviors:
- Make the "Space" control part of the policy control, so the UI shows "Visible To: [Space][Policy]". I think this helps make the role of spaces more clear. It also makes them easier to implement.
- Don't show the default space in headers: instead, show nothing.
- If the user has access to only one space, pretend spaces don't exist (no edit controls, no header stuff).
This might be confusing, but I think most of the time it will all align fairly well with user expectation.
Test Plan:
- Viewed a list of pastes (saw Space with non-default space, no space with default space, no space with user in only one space).
- Viewed a paste (saw Space with non-default space, saw no space with default space, saw no space with user in only one space).
- Edited spaces on objects (control as privileged user, no control as locked user).
- Created a new paste in a space (got space select as privileged user, no select as locked user).
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8449
Differential Revision: https://secure.phabricator.com/D13229
Summary: Ref T8496. In D13123, the condition for establishing a web session was made too strict: we need to let non-activated users establish web sessions in order to see "you are a bad disabled person" or "your account needs approval" messages. The previous behavior let them in, the new behavior incorrectly locks them out.
Test Plan: Enabled login approvals and registered a new account with username/password auth.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T8496
Differential Revision: https://secure.phabricator.com/D13239
Summary:
We can end up here with a stack trace like this, while rendering an embedded Slowvote trying to publish a Feed story:
```
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] [2015-06-08 22:49:57] EXCEPTION: (PhutilProxyException) Error while executing Task ID 830591. {>} (PhabricatorDataNotAttachedException) Attempting to access attached data on PhabricatorUser (via getAlternateCSRFString()), but the data is not actually attached. Before accessing attachable data on an object, you must load and attach it.
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] Data is normally attached by calling the corresponding needX() method on the Query class when the object is loaded. You can also call the corresponding attachX() method explicitly. at [<phabricator>/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php:166]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] arcanist(head=master, ref.master=7d15b85a1bc0), phabricator(head=master, ref.master=929f5f22acef), phutil(head=master, ref.master=92882eb9404d)
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #0 <#2> PhabricatorLiskDAO::assertAttached(string) called at [<phabricator>/src/applications/people/storage/PhabricatorUser.php:556]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #1 <#2> PhabricatorUser::getAlternateCSRFString() called at [<phabricator>/src/applications/people/storage/PhabricatorUser.php:432]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #2 <#2> PhabricatorUser::generateToken(integer, integer, string, integer) called at [<phabricator>/src/applications/people/storage/PhabricatorUser.php:344]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #3 <#2> PhabricatorUser::getRawCSRFToken() called at [<phabricator>/src/applications/people/storage/PhabricatorUser.php:357]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #4 <#2> PhabricatorUser::getCSRFToken() called at [<phabricator>/src/infrastructure/javelin/markup.php:91]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #5 <#2> phabricator_form(PhabricatorUser, array, array) called at [<phabricator>/src/applications/slowvote/view/SlowvoteEmbedView.php:169]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #6 <#2> SlowvoteEmbedView::render() called at [<phabricator>/src/view/AphrontView.php:175]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #7 <#2> AphrontView::producePhutilSafeHTML() called at [<phutil>/src/markup/render.php:133]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #8 <#2> phutil_escape_html(SlowvoteEmbedView)
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #9 <#2> array_map(string, array) called at [<phutil>/src/markup/engine/remarkup/PhutilRemarkupBlockStorage.php:56]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #10 <#2> PhutilRemarkupBlockStorage::restore(PhutilSafeHTML, integer) called at [<phutil>/src/markup/engine/PhutilRemarkupEngine.php:299]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #11 <#2> PhutilRemarkupEngine::restoreText(PhutilSafeHTML, integer) called at [<phutil>/src/markup/engine/PhutilRemarkupEngine.php:295]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #12 <#2> PhutilRemarkupEngine::postprocessText(array) called at [<phabricator>/src/infrastructure/markup/PhabricatorMarkupEngine.php:138]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #13 <#2> PhabricatorMarkupEngine::process() called at [<phabricator>/src/applications/feed/story/PhabricatorFeedStory.php:167]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #14 <#2> PhabricatorFeedStory::loadAllFromRows(array, PhabricatorUser) called at [<phabricator>/src/applications/feed/query/PhabricatorFeedQuery.php:37]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #15 <#2> PhabricatorFeedQuery::willFilterPage(array) called at [<phabricator>/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php:237]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #16 <#2> PhabricatorPolicyAwareQuery::execute() called at [<phabricator>/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php:168]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #17 <#2> PhabricatorPolicyAwareQuery::executeOne() called at [<phabricator>/src/applications/feed/worker/FeedPushWorker.php:12]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #18 <#2> FeedPushWorker::loadFeedStory() called at [<phabricator>/src/applications/feed/worker/FeedPublisherWorker.php:6]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #19 <#2> FeedPublisherWorker::doWork() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorWorker.php:91]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #20 <#2> PhabricatorWorker::executeTask() called at [<phabricator>/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php:162]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #21 <#2> PhabricatorWorkerActiveTask::executeTask() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorTaskmasterDaemon.php:22]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #22 PhabricatorTaskmasterDaemon::run() called at [<phutil>/src/daemon/PhutilDaemon.php:185]
Daemon 43450 STDE [Mon, 08 Jun 2015 22:49:57 +0000] #23 PhutilDaemon::execute() called at [<phutil>/scripts/daemon/exec/exec_daemon.php:125]
```
Just return `null`.
Test Plan: Will check that tasks clear in production.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D13218
Summary: Right now, "Publish" workers for user profile edits (title / blub) can get gummed up in the daemons. Implement the interfaces and provide a Query so they can go through.
Test Plan:
- Made a profile "Title" edit.
- Used `bin/worker execute --id <id>` to see task fail.
- Applied patch.
- Saw task work.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D13213
Summary:
Ref T8424. This adds crude integration with Paste's edit/view workflows: you can change the space a Paste appears in, see transactions, and get a policy callout.
Lots of rough edges and non-obviousness but it pretty much works.
Test Plan:
- Created and updated Pastes.
- Moved them between spaces, saw policy effects.
- Read transactions.
- Looked at feed.
- Faked query to return no spaces, saw control and other stuff vanish.
- Faked query to return no spaces, created pastes.
- Tried to submit bad values and got errors.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8424
Differential Revision: https://secure.phabricator.com/D13159
Test Plan: Submitted a form - saw nothing out of ordinary.
Reviewers: #blessed_reviewers
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D13167
Summary:
Fixes T8326. This removes calls to PhabricatorStartup from places that daemons may access.
This salt doesn't need to be global; it's embedded in the token we return. It's fine if we use a different salt every time. In practice, we always use the same viewer, so this change causes little or no behavioral change.
Ref T8424. For Spaces, I need a per-request cache for all spaces, because they have unusual access patterns and require repeated access, in some cases by multiple viewers.
We don't currently have a per-request in-process cache that we, e.g., clear in the daemons.
We do have a weak/theoretical/forward-looking attempt at this in `PhabricatorStartup::getGlobal()` but I'm going to throw that away (it's kind of junky, partly because of T8326) and replace it with a more formal mechanism.
Test Plan:
- Submitted some forms.
- Grepped for `csrf.salt`.
- Viewed page source, saw nice CSRF tokens with salt.
- All the salts are still the same on every page I checked, but it doesn't matter if this isn't true everywhere.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8326, T8424
Differential Revision: https://secure.phabricator.com/D13151
Summary:
Ref T8387. Adds new mailing list users.
This doesn't migrate anything yet. I also need to update the "Email Addresses" panel to let administrators change the list address.
Test Plan:
- Created and edited a mailing list user.
- Viewed profile.
- Viewed People list.
- Searched for lists / nonlists.
- Grepped for all uses of `getIsDisabled()` / `getIsSystemAgent()` and added relevant corresponding behaviors.
- Hit the web/api/ssh session blocks.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: eadler, tycho.tatitscheff, epriestley
Maniphest Tasks: T8387
Differential Revision: https://secure.phabricator.com/D13123
Summary: Fixes T8242. D12833 removed the title as well as the blurb from people hovercards. When re-adding the title don't bother throwing things through pht since that seems like not something you translate exactly and also lose the flavor text which most users end up having since title is rarely set (at least on this install).
Test Plan: viewed hovercards and saw title and blurb again as appropos relative to the data being set
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8242
Differential Revision: https://secure.phabricator.com/D12915
Summary: Fix some method signatures so that arguments with default values are at the end of the argument list (see D12418).
Test Plan: Eyeballed the callsites.
Reviewers: epriestley, #blessed_reviewers, hach-que
Reviewed By: epriestley, #blessed_reviewers, hach-que
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12782
Summary: Ref T7707. Caches availability on users to reduce the cost of loading handles. This cache is very slightly tricky to dirty properly.
Test Plan:
- Use DarkConsole to examine queries; saw cache hits, miss+fill, dirty.
- Saw availability change correctly after canceling, joining, declining events.
- Saw no queries to Calendar for pages with only availability data.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7707
Differential Revision: https://secure.phabricator.com/D12838
Summary:
Ref T7707. Ref T8183.
- Currently, user status is derived by looking at events they //created//. Instead, look at non-cancelled invites they are attending.
- Prepare for on-user caching.
- Mostly remove "Sporradic" as a status, although I left room for adding more information later.
Test Plan:
- Called user.query.
- Viewed profile.
- Viewed hovercard.
- Used mentions.
- Saw status immediately update when attending/leaving/cancelling a current event.
- Created an event ending at 6 PM and an event from 6:10PM - 7PM, saw "Away until 7PM".
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8183, T7707
Differential Revision: https://secure.phabricator.com/D12833
Summary:
Ref T7707. The general form of this can probably be refined somewhat over time as we have more use cases.
I put this cache on the user object itself because we essentially always need this data and it's trivial to invalidate the cache (we can do it implicilty during reads).
Also fix an issue with short, wide images not thumbnailing properly after recent changes.
Test Plan:
- Loaded some pages; saw caches write; saw good pictures.
- Reloaded; saw cache reads; saw good pictures.
- Changed profile picture; saw immediate update.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7707
Differential Revision: https://secure.phabricator.com/D12826
Summary:
Ref T8021.
- When "All Day" events are loaded, convert them into the viewer's time.
- When "All Day" events are saved, convert them into a +24 hour range.
Test Plan:
- Created and updated "All Day" events.
- Created and updated normal events.
- Changed timezones, edited and viewed "All Day" events and normal events.
- In all cases, "All Day" events appeared to be 12:00AM - 11:59:59PM to the viewer, on the correct day.
- Normal events shifted around properly according to timezones.
Reviewers: lpriestley
Reviewed By: lpriestley
Subscribers: epriestley
Maniphest Tasks: T8021
Differential Revision: https://secure.phabricator.com/D12765
Summary:
Ref T7689, which discusses some of the motivation here. Briefly, these methods are awkward:
- Controller->loadHandles()
- Controller->loadViewerHandles()
- Controller->renderHandlesForPHIDs()
This moves us toward better semantics, less awkwardness, and a more reasonable attack on T7688 which won't double-fetch a bunch of data.
Test Plan:
- Added unit tests.
- Converted one controller to the new stuff.
- Viewed countdown lists, saw handles render.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7689
Differential Revision: https://secure.phabricator.com/D12202
Summary:
Currently, PhortuneAccounts have a very open default policy to allow merchants to see and interact with them.
This has the undesirable side effect of leaking their names in too many places, because all users are allowed to load the handles for the accounts. Although this information is not super sensitive, we shouldn't expose it.
I went through about 5 really messy diffs trying to fix this. It's very complicated because there are a lot of objects and many of them are related to PhortuneAccounts, but PhortuneAccounts are not bound to a specific merchant. This lead to a lot of threading viewers and merchants all over the place through the call stack and some really sketchy diffs with OmnipotentUsers that weren't going anywhere good.
This is the cleanest approach I came up with, by far:
- Introduce the concept of an "Authority", which gives a user more powers as a viewer. For now, since we only have one use case, this is pretty open-ended.
- When a viewer is acting as a merchant, grant them authority through the merchant.
- Have Accounts check if the viewer is acting with merchant authority. This lets us easily implement the rule "merchants can see this stuff" without being too broad.
Then update the Subscription view to respect Merchant Authority.
I partially updated the Cart views to respect it. I'll finish this up in a separate diff, but this seemed like a good checkpoint that introduced the concept without too much extra baggage.
This feels pretty good/clean to me, overall, even ignoring the series of horrible messes I made on my way here.
Test Plan:
- Verified I can see everything I need to as a merchant (modulo un-updated Cart UIs).
- Verified I can see nothing when acting as a normal user.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11945
Summary: Fixes T7159.
Test Plan:
Created a legalpad document that needed a signature and I was required to sign it no matter what page I hit. Signed it and things worked! Added a new legalpad document and I had to sign again!
Ran unit tests and they passed!
Logged out as a user who was roadblocked into signing a bunch of stuff and it worked!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7159
Differential Revision: https://secure.phabricator.com/D11759
Summary:
Ref T7152. Ref T1139. This updates Phabricator so third-party libraries can translate their own stuff. Also:
- Hide "All Caps" when not in development mode, since some users have found this a little confusing.
- With other changes, adds a "Raw Strings" mode (development mode only).
- Add an example silly translation to make sure the serious business flag works.
- Add a basic British English translation.
- Simplify handling of translation overrides.
Test Plan:
- Flipped serious business / development on and off and saw silly/development translations drop off.
- Switched to "All Caps" and saw all caps.
- Switched to Very English, Wow!
- Switched to British english and saw "colour".
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7152, T1139
Differential Revision: https://secure.phabricator.com/D11747
Summary:
Ref T7152. This builds the core of email invites and implements all the hard logic for them, covering it with a pile of tests.
There's no UI to create these yet, so users can't actually get invites (and administrators can't send them).
This stuff is a complicated mess because there are so many interactions between accounts, email addresses, email verification, email primary-ness, and user verification. However, I think I got it right and got test coverage everwhere.
The degree to which this is exception-driven is a little icky, but I think it's a reasonable way to get the testability we want while still making it hard for callers to get the flow wrong. In particular, I expect there to be at least two callers (one invite flow in the upstream, and one derived invite flow in Instances) so I believe there is merit in burying as much of this logic inside the Engine as is reasonably possible.
Test Plan: Unit tests only.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7152
Differential Revision: https://secure.phabricator.com/D11723
Summary: Ref T7094. We already had and were mostly using "needProfileImage" on the people query class. Only real trick in this diff is deleting a conduit end point that has been marked deprecated for the better part of 3 years.
Test Plan: clicked around the people action and profiles and calendars loaded nicely.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11630
Summary:
Ref T2783. Ref T6706.
- Add `cluster.addresses`. This is a whitelist of CIDR blocks which define cluster hosts.
- When we recieve a request that has a cluster-based authentication token, require the cluster to be configured and require the remote address to be a cluster member before we accept it.
- This provides a general layer of security for these mechanisms.
- In particular, it means they do not work by default on unconfigured hosts.
- When cluster addresses are configured, and we receive a request //to// an address not on the list, reject it.
- This provides a general layer of security for getting the Ops side of cluster configuration correct.
- If cluster nodes have public IPs and are listening on them, we'll reject requests.
- Basically, this means that any requests which bypass the LB get rejected.
Test Plan:
- With addresses not configured, tried to make requests; rejected for using a cluster auth mechanism.
- With addresses configred wrong, tried to make requests; rejected for sending from (or to) an address outside of the cluster.
- With addresses configured correctly, made valid requests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6706, T2783
Differential Revision: https://secure.phabricator.com/D11159
Summary:
Ref T6240. Some discussion in that task. In instance/cluster environments, daemons need to make Conduit calls that bypass policy checks.
We can't just let anyone add SSH keys with this capability to the web directly, because then an adminstrator could just add a key they own and start signing requests with it, bypassing policy checks.
Add a `bin/almanac trust-key --id <x>` workflow for trusting keys. Only trusted keys can sign requests.
Test Plan:
- Generated a user key.
- Generated a device key.
- Trusted a device key.
- Untrusted a device key.
- Hit the various errors on trust/untrust.
- Tried to edit a trusted key.
{F236010}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6240
Differential Revision: https://secure.phabricator.com/D10878
Summary:
Ref T5833. I want to add SSH keys to Almanac devices, but the edit workflows for them are currently bound tightly to users.
Instead, decouple key management from users and the settings panel.
Test Plan:
- Uploaded, generated, edited and deleted SSH keys.
- Hit missing name, missing key, bad key format, duplicate key errors.
- Edited/generated/deleted/etc keys for a bot user as an administrator.
- Got HiSec'd on everything.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10824
Summary:
Ref T5833. This fixes a few weird things with this table:
- A bunch of columns were nullable for no reason.
- We stored an MD5 hash of the key (unusual) but never used it and callers were responsible for manually populating it.
- We didn't perform known-key-text lookups by using an index.
Test Plan:
- Ran migrations.
- Faked duplicate keys, saw them clean up correctly.
- Added new keys.
- Generated new keys.
- Used `bin/auth-ssh` and `bin/auth-ssh-key`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10805
Summary: Ref T1191. Same deal as D10786. These were previously case-insensitive, but changed to a case-sensitive column type.
Test Plan: Ran `bin/storage adjust` and got and adjustment.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: webframp, epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10806
Summary: Ref T5833. Since these will no longer be bound specifically to users, bring them to a more central location.
Test Plan:
- Edited SSH keys.
- Ran `bin/ssh-auth` and `bin/ssh-auth-key`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10791
Summary: Ref T1191. After adjustment, usernames currently end up case-sensitive, which means `alincoln` and `Alincoln` are different users. Make them case-sensitive so these names collie.
Test Plan: Ran `bin/storage adjust`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10786
Summary:
Ref T1191. Notable stuff:
- Adds `--disable-utf8mb4` to `bin/storage` to make it easier to test what things will (approximately) do on old MySQL. This isn't 100% perfect but should catch all the major stuff. It basically makes us pretend the server is an old server.
- Require utf8mb4 to dump a quickstart.
- Fix some issues with quickstart generation, notably special casing the FULLTEXT handling.
- Add an `--unsafe` flag to `bin/storage adjust` to let it truncate data to fix schemata.
- Fix some old patches which don't work if the default table charset is utf8mb4.
Test Plan:
- Dumped a quickstart.
- Loaded the quickstart with utf8mb4.
- Loaded the quickstart with `--disable-utf8mb4` (verified that we get binary columns, etc).
- Adjusted schema with `--disable-utf8mb4` (got a long adjustment with binary columns, some truncation stuff with weird edge case test data).
- Adjusted schema with `--disable-utf8mb4 --unsafe` (got truncations and clean adjust).
- Adjusted schema back without `--disable-utf8mb4` (got a long adjustment with utf8mb4 columns, some invalid data on truncated utf8).
- Adjusted schema without `--disable-utf8mb4`, but with `--unsafe` (got truncations on the invalid data).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10757
