Summary:
Ref T11665. Currently, when a repository hits an error, we retry it after 15s. This is correct if the error was temporary/transient/config-related (e.g., bad network or administrator setting up credentials) but not so great if the error is long-lasting (completely bad authentication, invalid URI, etc), as it can pile up to a meaningful amount of unnecessary load over time.
Instead, record how many times in a row we've hit an error and adjust backoff behavior: first error is 15s, then 30s, 45s, etc.
Additionally, when computing the backoff for an empty repository, use the repository creation time as though it was the most recent commit. This is a good proxy which gives us reasonable backoff behavior.
This required removing the `CODE_WORKING` messages, since they would have reset the error count. We could restore them (as a different type of message), but I think they aren't particularly useful since cloning usually doesn't take too long and there's more status information avilable now than there was when this stuff was written.
Test Plan:
- Ran `bin/phd debug pull`.
- Saw sensible, increasing backoffs selected for repositories with errors.
- Saw sensible backoffs selected for empty repositories.
Reviewers: chad
Maniphest Tasks: T11665
Differential Revision: https://secure.phabricator.com/D16575
Summary:
Minor rebuild / redesign of Conpherence. Most of this is new UX and tossing out things like widgets, device fallbacks. I expect some of the UI to get more polished after next pass, but most everything here is in place.
- Removed "Widgets", now just a single Participants pane
- Added "Topic"
- New header
- Settings, Edit are action icons
- Removed a lot of JS
- Simplified CSS as much as I could
Test Plan:
Desktop, Tablet, Mobile. Adding and removing people. Setting new topics, new rooms.
{F1828662}
{F1828669}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16550
Summary: Fixes T11532. The language selection for pastes is now a typeahead that is backed by `pygments.dropdown-choices`. There is still a bit of weirdness around making "auto-detection" the default state. To actually select a different language, you first need to remove the "auto detect" option that is pre-populated in a new paste. Other than that, it works as intended.
Test Plan:
Create a new paste with a file extension that can be auto-detected.
Created a new paste and manually selected the language
Edited a paste and changed the language.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley, yelirekim
Maniphest Tasks: T11532
Differential Revision: https://secure.phabricator.com/D16463
Summary: Fixes T9117. Adds a migration to remove ponder vote data.
Test Plan: I added a bunch of lines to phabricator_user.edge with type 18 and they were successfully removed by this patch
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, yelirekim
Maniphest Tasks: T9117
Differential Revision: https://secure.phabricator.com/D16452
Summary: Ref T11522. This migrates any "badcommit" data (which probably only exists at Facebook and on 1-2 other installs in the wild) to the new "hint" table.
Test Plan:
- Wrote some bad commit annotations to the badcommit table.
- Viewed them in the web UI and used `bin/repository reparse --change ...` to reparse them. Saw "this is bad" messages.
- Ran migration, verified that valid "badcommit" rows were successfully migrated to become "hint" rows.
- Viewed the new web UI and re-parsed the change, saw "unreadable commit" messages.
- Viewed a good commit; reparsed a good commit.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11522
Differential Revision: https://secure.phabricator.com/D16435
Summary:
Ref T11522. This provides storage for tracking rewritten commits (new feature) and unreadable commits (existing feature, but really hacky).
This doesn't do anything yet, just adds a table and a CLI tool for updating it. I'll document the tool once it works. You just pipe in some JSON, but I need to document the format.
Test Plan:
- Piped JSON for "none", "rewritten" and "unreadable" hints into `bin/repository hint`.
- Examined the database to see that the table was written properly.
- Tried to pipe bad JSON in, invalid hint types, etc. Got reasonable human-readable error messages.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11522
Differential Revision: https://secure.phabricator.com/D16434
Summary: Adds a schema patch that removes conduit_connectionlog. This table hasn't been used in 8ish months so it's probably safe to get rid of.
Test Plan: Apply the patch locally and confirm that the table does indeed get dropped.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D16438
Summary:
Fixes T11453. Currently, commit message summaries are limited to 80 bytes. This may only be 20-40 characters for CJK languages or langauges with Cyrillic script.
Increase storage size to 255, then truncate to the shorter of 255 bytes or 80 glyphs. This preserves the same behavior for latin languages, but is less tight for Russian, etc.
Some minor additional changes:
- Provide a way to ask "how much data fits in this column?" so we don't have to duplicate column lengths across summary checks or UI errors like "title too long".
- Remove the `text80` datatype, since no other columns use it and we have no use cases (or likely use cases) for it.
Test Plan:
- Made a commit with a Cyrillic title, saw reasonable summarization in UI:
{F1757522}
- Added and ran unit tests.
- Grepped for removed `SUMMARY_MAX_LENGTH` constant.
- Grepped for removed `text80` data type.
Reviewers: avivey, chad
Reviewed By: avivey
Subscribers: avivey
Maniphest Tasks: T11453
Differential Revision: https://secure.phabricator.com/D16385
Summary: Ref T8116. Add search-by-name and per-package / per-publisher search to Packages.
Test Plan: Searched publishers, packages, versions by name. Searched packages by publisher. Searched versions by package.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8116
Differential Revision: https://secure.phabricator.com/D16320
Summary:
Ref T8116. A version has:
- a package (like "Arcanist") which it belongs to;
- a name (like "v3.1.5").
The name is immutable and unique, like the package key and publisher key.
Policy stuff:
- Versions have the exact same policies as their packages.
- You must be able to edit a package to create new versions of it.
This is still entirely uninteresting.
Test Plan: {F1731703}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8116
Differential Revision: https://secure.phabricator.com/D16316
Summary:
Ref T8116. A package has:
- a publisher (like "Phacility"), from the previous revision;
- a name (like "Arcanist");
- a package key (like "arcanist").
The package key is immutable, like the publisher key.
This gives a package a full key like "phacility/arcanist".
Policy stuff:
- You must be able to view a publisher to view a package (currently, everyone can always see all publishers).
- You must be able to edit a publisher to create a new package inside it.
- Packages have separate view/edit permissions.
This still does nothing interesting.
Test Plan: {F1731663}
Reviewers: chad
Reviewed By: chad
Subscribers: eadler
Maniphest Tasks: T8116
Differential Revision: https://secure.phabricator.com/D16315
Summary:
Ref T8116. Partially scavenged from D14152. This roughs in a new Packages application for Arcanist extensions and third-party applications, and adds a "Publisher" object.
A "Publisher" represents an individual or entity who is publishing a package, like "Phacility". It's explicitly //not// necessarily the original author -- just the primary entity vouching for the safety of the code.
A publisher just has a name and a unique key for now. For example, Phacility might have "Phacility" and "phacility", respectively.
Unique keys are immutable, e.g., the package "phacility/arcanist" will always be exactly the same package by exactly the same publisher.
Test Plan: {F1731621}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8116
Differential Revision: https://secure.phabricator.com/D16314
Summary:
Fix T11339.
Now, old and new are both simple lists of phids, and the rendering should make sense.
Test Plan: Viewed existing transaction with all 3 states.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T11339
Differential Revision: https://secure.phabricator.com/D16311
Summary:
Ref T11326. Normally, events occur at a specific epoch, independent of the viewer. For example, if we're having a meeting in 35 hours, every user who looks at the event will see that it starts 35 hours from now.
But when an event is "All Day", the start time and end time depend on the //viewer//. A day like "Christmas" does not start at the same time for everyone: it starts sooner if you're in a more-eastern timezone. Baiscally, an event on "July 15th" starts whenever "July 15th" starts for whoever is looking at it.
Previously, we stored these events by using the western-most and eastern-most timezones as the start and end times (the earliest possible start and latest possible end).
This worked OK, but we get into a bunch of trouble with EditEngine, mostly because each field can be updated individually now. We can't easily tell if an event is all-day or not when reading or updating the start time and end time, and making that easier would introduce a huge amount of complexity.
Instead, when we update the start or end time, we write //two// times:
- The epoch timestamp of the time the user entered, which is the start time we will use if the event is a normal event.
- The epoch timestamp of 12:00 AM in UTC on the same date as the //local// date the user entered. This is pretty much like just storing the date the user actually typed. This is what w'ell use if the event is an all-day event.
Then, no matter whether the event is later made all-day or not, we have all the information we need to display it correctly.
Test Plan:
- Created and edited all-day events.
- Migrated existing all-day events, which appeared to survive without problems. (Note that events all-day which were created or edited in the last couple of days `master` won't survive this mutation correctly and will need to be fixed.)
- Created and edited normal, recurring, and recurring all-day events.
- Swapped back to `stable`, created an event, specifically migrated it forward, made sure it survived with times intact.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16305
Summary: Ref T10909. Ref T9224. We label this field "Host" in the UI; make the storage format consistent.
Test Plan:
- Viewed month view, day view, detail view of an event.
- Created a new event, saw myself as the host.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9224, T10909
Differential Revision: https://secure.phabricator.com/D16291
Summary:
Ref T9275. When you create a recurring event which recurs forever, we want to avoid writing an infinite number of rows to the database.
Currently, we write a row to the database right before you edit the event. Until then, we refer to it as `E123/999` or whatever ("instance 999 of event 123").
This creates a big mess with trying to make recurring events work with EditEngine, Subscriptions, Projects, Flags, Tokens, etc -- all of this stuff assumes that whatever you're working with has a PHID.
I poked at letting this stuff work without a PHID a little bit, but that looked like a gigantic mess.
Instead, generate an event "stub" a little sooner (when you look at the event detail page). This is basically just an ID/PHID to refer to the instance.
Then, when you edit the stub, "materialize" it into a real event.
This still has some issues, but I think it's more promising than the other approach was.
Also:
- Removes dead user profile calendar controller.
- Replaces comments with EditEngine comments.
Test Plan:
- Commented on a recurring event.
- Awarded tokens to a recurring event.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9275
Differential Revision: https://secure.phabricator.com/D16248
Summary:
Fixes T11307. Fixes T8124. Currently, builtin files are tracked by using a special transform with an invalid source ID.
Just use a dedicated column instead. The transform thing is too clever/weird/hacky and exposes us to issues with the "file" and "transform" tables getting out of sync (possibly the issue in T11307?) and with race conditions.
Test Plan:
- Loaded profile "edit picture" page, saw builtins.
- Deleted all builtin files, put 3 second sleep in the storage engine write, loaded profile page in two windows.
- Before patch: one of them failed with a race.
- After patch: both of them loaded.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8124, T11307
Differential Revision: https://secure.phabricator.com/D16271
Summary: Ref T9360. These weren't getting set properly, also make them nullable since they're optional.
Test Plan: run upgrade, make a new blog with and without a parent domain. Edit a current blog.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9360
Differential Revision: https://secure.phabricator.com/D16242
Summary:
Ref T9838.
Add a Properties field to Revision, and update a `wasAcceptedBeforeClose` when closing a revision.
Test Plan:
A quick run through the obvious steps (Close with commit/manually, with or w/o accept) and calling `differential.query` shows the `wasAcceptedBeforeClose` property was setup correctly.
Pushing closed + accepted passes the relevant herald, which was my immediate issue; Pushing un-accepted is blocked.
Test the "commit" rule (Different from "pre-commit") by hacking the DB and running the "has accepted revision" rule in a test-console.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T9838
Differential Revision: https://secure.phabricator.com/D15085
Summary: We haven't refreshed this in a while.
Test Plan: Saw unit test times drop about 1.5 seconds locally.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16176
Summary: Ref T9897. This moves "Domain" to "DomainFullURI" to allow setting of https or for some reason, a port. I guess.
Test Plan: Try to break by setting a path, or fake protocol. Set to http, or https, see correct redirects. Verify domain still gets written.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D16173
Summary: Ref T9897. Adds a Parent Site and Parent Domain field to allow external sites to link back to parent.
Test Plan: Set up ```local.blog.phacility.com```, set parent site to "Phacility" and parent domain to "local.www.phacility.com". Get new crumbs at Blog and Post levels.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D16150
Summary: Adds a new header layout for Phame Blog. Subtitles now also.
Test Plan:
With Image, With Subtitle, Without Image, Without Subtitle. Mobile, Tablet, Desktop.
{F1691506}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16147
Summary:
Ref T11153. If you have a build plan like this:
- Lease machine A.
- Lease machine B.
- Run client-tests on machine A.
- Run server-tests on machine B.
...and we get machine A quickly, then finish the tests, we currently do not release machine A until the whole plan finishes.
In the best case, this wastes resources (something else could be using that machine for a while).
In a worse case, this wastes a lot of resources (if machine B is slow to acquire, or the server tests are much slower than the client tests, machine A will get tied up for a really long time).
In the absolute worst case, this might deadlock things.
Instead, release artifacts as soon as no waiting/running steps take them as inputs. In this case, we'd release machine A as soon as we finished running the client tests.
In the case where machines A and B are resources of the same type, this should prevent deadlocks. In all cases, this should improve build throughput at least somewhat.
Test Plan:
I wrote this build plan which runs a "fast" step (10 seconds) and a "slow" step (120 seconds):
{F1691190}
Before the patch, running this build plan held the lease on the "fast" machine for the full 120 seconds, then released both leases at the same time at the very end.
After this patch, I ran this plan and observed the "fast" lease get released after 10 seconds, while the "slow" lease was held for the full 120.
(Also added some `var_dump()` into things to sanity check the logic; it appeared correct.)
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11153
Differential Revision: https://secure.phabricator.com/D16145
Summary: This is the backend half of uploading an image as a header for Phame Blogs. Allows you to upload image, or delete it. Ref T10901
Test Plan:
Go to Manage Blog, visit Edit Header Image, Upload snarky file. See snarky file on Manage page. Edit Header Image, click delete, save, see file goes away.
{F1690966}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10901
Differential Revision: https://secure.phabricator.com/D16140
Summary:
Ref T9028. This allows us to detect when commits are unreachable:
- When a ref (tag, branch, etc) is moved or deleted, store the old thing it pointed at in a list.
- After discovery, go through the list and check if all the stuff on it is still reachable.
- If something isn't, try to follow its ancestors back until we find something that is reachable.
- Then, mark everything we found as unreachable.
- Finally, rebuild the repository summary table to correct the commit count.
Test Plan:
- Deleted a ref, ran `pull` + `refs`, saw oldref in database.
- Ran `discover`, saw it process the oldref, mark the unreachable commit, and update the summary table.
- Visited commit page, saw it properly marked.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9028
Differential Revision: https://secure.phabricator.com/D16133
Summary:
Ref T9789. `Transaction` and `Editor` classes are the last major pieces of infrastructure that haven't been fully modularized.
Some of the specific issues are:
- `Editor` classes rely on a bunch of `instanceof` stuff in the base class to pick up transaction types like "subscribe", "projects", etc. Instead, applications should be adding these, and third-party applications should be able to add them.
- Code is spread across `Transaction` and `Editor` classes somewhat oddly. For example, generating old/new values would probably make more sense at the `Transaction` level, but it currently exists at the `Editor` level.
- Both types of classes have a lot of functions based on `switch()` statements, which require a ton of boilerplate and are just generally kind of hard to work with.
This creates classes for each type of transaction, and moves almost all of the logic to them. These classes are simpler and more focused than the old stuff was, and can organize related code better.
This starts inching toward defining `CoreTransactions` for features shared across applications. It only defines the "Create" transaction so far, but at some point I plan to move all the other shared transactions to Core and let them control which objects they're available for.
Test Plan:
- Created pastes with web UI and API.
- Edited all paste properites.
- Archived/activated.
- Verified files got reasonable names.
- Reviewed timeline and feed.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9789
Differential Revision: https://secure.phabricator.com/D16111
Summary:
Ref T4103. This just adds a single global default setting group, not full profiles.
Primarily, I'm not sure how administrators are supposed to set profiles for users, since most ways user accounts get created don't really support setting roles.. When we figure that out, it should be reasonably easy to extend this. There also isn't much of a need for this now, since pretty much everyone just wants to turn off mail.
Test Plan:
- Edited personal settings.
- Edited global settings.
- Edited a bot's settings.
- Tried to edit some other user's settings.
- Saw defaults change appropriately as I edited global and personal settings.
{F1677266}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16048
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. This isn't completely perfect but should let us move forward without also expanding scope into "too much mail".
I split the existing "Mail Preferences" into two panels: a "Mail Delivery" panel for the EditEngine settings, and a "2000000 dropdowns" panel for the two million dropdowns. This one retains the old code more or less unmodified.
Test Plan:
- Ran unit tests, which cover most of this stuff.
- Grepped for all removed constants.
- Ran migrations, inspected database results.
- Changed settings in both modified panels.
- This covers a lot of ground, but anything I missed will hopefully be fairly obvious.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16038
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. 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 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 T10917. Converts web UI edits to transactions.
This is about 95% "the right way", and then I cheated on the last 5% instead of building a real EditEngine. We don't need it for anything else right now and some of the dialog workflows here are a little weird so I'm just planning to skip it for the moment unless it ends up being easier to do after the next phase (mail notifications) or something like that.
Test Plan: {F1652160}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10917
Differential Revision: https://secure.phabricator.com/D15947
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 T10939. This adds UI, transactions, etc, to adjust dominion rules.
Test Plan:
- Read documentation.
- Changed dominion rules.
- Created packages on `/` ("A") and `/x` ("B") with "Auto Review: Review".
- Touched `/x`.
- Verified that A and B were added with strong dominion.
- Verified that only B was added when A was set to weak dominion.
- Viewed file in Diffusion, saw correct ownership with strong/weak dominion rules.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15936
Summary: Fixes T10975. The "scramble attached file permissions when an object is saved" code is misfiring here too. See T10778 + D15803 for prior work.
Test Plan:
- Ran `bin/storage upgrade -f`.
- Edited the view policy of an OAuth server (prepatch: fatal; postpatch: worked great).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10975
Differential Revision: https://secure.phabricator.com/D15938
Summary:
Ref T10939. Ref T8887. This moves toward letting packages automatically become reviewers or blocking reviewers of owned code.
This change adds an "Auto Review" option to packages. Because adding reviewers/blocking reviewers is a little tricky, it doesn't actually have these options yet -- just a "subscribe" option. I'll do the reviewer work in the next update.
Test Plan:
Created a revision in a package with "Auto Review: Subscribe to Changes". The package got subscribed.
{F1311677}
{F1311678}
{F1311679}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8887, T10939
Differential Revision: https://secure.phabricator.com/D15915
Summary:
Ref T10923. When regenerating the URI index for a repository, index every URI.
- Also, make the index slightly stricter (domain + path instead of just path). Excluding the domain made more sense when we were generating only first-party URIs.
- Make the index smarter about `/diffusion/123/` URIs.
- Show normalized URIs in `diffusion.repository.search` results.
Test Plan:
- Ran migration.
- Verified sensible-looking results in database.
- Searched for a repository URI by first-party clone URI.
- Searched for a repository URI by mirror URI.
- Used `diffusion.repository.search` to get information about repository URIs.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10923
Differential Revision: https://secure.phabricator.com/D15876
Summary:
Ref T10748. This needs more extensive testing and is sure to have some rough edges, but seems to basically work so far.
Throwing this up so I can work through it more deliberately and make notes.
Test Plan:
- Ran migration.
- Used `bin/repository list` to list existing repositories.
- Used `bin/repository update <repository>` to update various repositories.
- Updated a migrated, hosted Git repository.
- Updated a migrated, observed Git repository.
- Converted an observed repository into a hosted repository by toggling the I/O mode of the URI.
- Conveted a hosted repository into an observed repository by toggling it back.
- Created and activated a new empty hosted Git repository.
- Created and activated an observed Git repository.
- Updated a mirrored repository.
- Cloned and pushed over HTTP.
- Tried to HTTP push a read-only repository.
- Cloned and pushed over SSH.
- Tried to SSH push a read-only repository.
- Updated several Mercurial repositories.
- Updated several Subversion repositories.
- Created and edited repositories via the API.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15842
Summary:
Ref T10748. This migrates and swaps mirroring to `PhabricatorRepositoryURI`, obsoleting `PhabricatorRepositoryMirror`.
This prevents you from editing, adding or disabling mirrors unless you know a secret URI (until the UI cuts over fully), but existing mirroring is not affected.
Test Plan:
- Added a mirroring URI to an old repository.
- Verified it worked with `bin/repository mirror`.
- Migrated forward.
- Verified it still worked with `bin/repository mirror`.
- Wow, mirroring: https://github.com/epriestley/locktopia-mirror
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15841
Summary:
Ref T4039. Long ago these were more freely editable and there were some security concerns around creating a repository, then setting its local path to point somewhere it shouldn't.
Local paths are no longer editable so there's no real reason we need to provide a uniqueness guarantee anymore, but you could still make a mistake with `bin/repository move-paths` by accident, and it's a little cleaner to pull them out into their own column with a key.
(We still don't -- and, largely can't -- guarantee that two paths aren't //equivalent// since one might be symlinked to the other, or symlinked only on some hosts, or whatever, but the primary value here is as a sanity check that you aren't goofing things up and pointing a bunch of repositories at the same working copy by mistake.)
Test Plan:
- Ran migrations.
- Grepped for `local-path`.
- Listed and moved paths with `bin/repository`.
- Created a new repository, verified its local path populated correctly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4039
Differential Revision: https://secure.phabricator.com/D15837
Summary:
Ref T10748. Ref T10366. This adds a new EditEngine, EditController, Editor, Query, and Transaction for RepositoryURIs.
None of these really do anything helpful yet, and these URIs are still unused in the actual application.
Test Plan: {F1249794}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10366, T10748
Differential Revision: https://secure.phabricator.com/D15815
Summary:
Fixes T10778. This is a result of T10262: when we save a form configuration and adjust the policy, we try to scramble attached file secrets.
There aren't going to be any attached files, but there's also no edge table, so we fail.
We could skip this code, but we'll likely need an edge table here sooner or later so it's probably simpler in the long run to just add an empty one.
Test Plan:
- Ran `bin/storage upgrade`, got a clean bill of health.
- Saved a form configuration after making a policy edit, no more `edge` exception.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10778
Differential Revision: https://secure.phabricator.com/D15803
Summary:
Ref T10860. This allows us to recover if the connection to the database is lost during a push.
If we lose the connection to the master database during a push, we would previously freeze the repository. This is very safe, but not very operator-friendly since you have to go manually unfreeze it.
We don't need to be quite this aggressive about freezing things. The repository state is still consistent after we've "upgraded" the lock by setting `isWriting = 1`, so we're actually fine even if we lost the global lock.
Instead of just freezing the repository immediately, sit there in a loop waiting for the master to come back up for a few minutes. If it recovers, we can release the lock and everything will be OK again.
Basically, the changes are:
- If we can't release the lock at first, sit in a loop trying really hard to release it for a while.
- Add a unique lock identifier so we can be certain we're only releasing //our// lock no matter what else is going on.
- Do the version reads on the same connection holding the lock, so we can be sure we haven't lost the lock before we do that read.
Test Plan:
- Added a `sleep(10)` after accepting the write but before releasing the lock so I could run `mysqld stop` and force this issue to occur.
- Pushed like this:
```
$ echo D >> record && git commit -am D && git push
[master 707ecc3] D
1 file changed, 1 insertion(+)
# Push received by "local001.phacility.net", forwarding to cluster host.
# Waiting up to 120 second(s) for a cluster write lock...
# Acquired write lock immediately.
# Waiting up to 120 second(s) for a cluster read lock on "local001.phacility.net"...
# Acquired read lock immediately.
# Device "local001.phacility.net" is already a cluster leader and does not need to be synchronized.
# Ready to receive on cluster host "local001.phacility.net".
Counting objects: 3, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (2/2), done.
Writing objects: 100% (3/3), 254 bytes | 0 bytes/s, done.
Total 3 (delta 1), reused 0 (delta 0)
BEGIN SLEEP
```
- Here, I stopped `mysqld` from the CLI in another terminal window.
```
END SLEEP
# CRITICAL. Failed to release cluster write lock!
# The connection to the master database was lost while receiving the write.
# This process will spend 300 more second(s) attempting to recover, then give up.
```
- Here, I started `mysqld` again.
```
# RECOVERED. Link to master database was restored.
# Released cluster write lock.
To ssh://local@localvault.phacility.com/diffusion/26/locktopia.git
2cbf87c..707ecc3 master -> master
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10860
Differential Revision: https://secure.phabricator.com/D15792
Summary: Ref T4292. When we write a push log, also log which node received the request.
Test Plan: {F1230467}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15759
Summary: Ref T4292. This will let the UI and future `bin/repository` tools give administrators more tools to understand problems when reporting or resolving them.
Test Plan:
- Pushed fully clean repository.
- Pushed previously-pushed repository.
- Forced write to abort, inspected useful information in the database.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15748
Summary:
Fixes T10830.
- The return code from `storage adjust` did not propagate correct.
- There was one column issue which I missed the first time around because I had a bunch of unrelated stuff locally.
Test Plan:
- Ran `bin/storage upgrade -f` with failures, used `echo $?` to make sure it exited nonzero.
- Got fully clean `bin/storage adjust` by dropping all my extra local tables.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10830
Differential Revision: https://secure.phabricator.com/D15746
