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
Summary:
Fixes T10830. Ref T10366. I wasn't writing to this table yet so I didn't build it, but the fact that `bin/storage adjust` would complain slipped my mind.
- Add the table.
- Make the tests run `adjust`. This is a little slow (a few extra seconds) but we could eventually move some steps like this to run server-side only.
Test Plan: Ran `bin/storage upgrade -f`, got a clean `adjust`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10366, T10830
Differential Revision: https://secure.phabricator.com/D15744
Summary:
Ref T4292. This mostly implements the locking/versioning logic for multi-master repositories. It is only active on Git SSH pathways, and doesn't actually do anything useful yet: it just does bookkeeping so far.
When we read (e.g., `git fetch`) the logic goes like this:
- Get the read lock (unique to device + repository).
- Read all the versions of the repository on every other device.
- If any node has a newer version:
- Fetch the newer version.
- Increment our version to be the same as the version we fetched.
- Release the read lock.
- Actually do the fetch.
This makes sure that any time you do a read, you always read the most recently acknowledged write. You may have to wait for an internal fetch to happen (this isn't actually implemented yet) but the operation will always work like you expect it to.
When we write (e.g., `git push`) the logic goes like this:
- Get the write lock (unique to the repository).
- Do all the read steps so we're up to date.
- Mark a write pending.
- Do the actual write.
- Bump our version and mark our write finished.
- Release the write lock.
This allows you to write to any replica. Again, you might have to wait for a fetch first, but everything will work like you expect.
There's one notable failure mode here: if the network connection between the repository node and the database fails during the write, the write lock might be released even though a write is ongoing.
The "isWriting" column protects against that, by staying locked if we lose our connection to the database. This will currently "freeze" the repository (prevent any new writes) until an administrator can sort things out, since it'd dangerous to continue doing writes (we may lose data).
(Since we won't actually acknowledge the write, I think, we could probably smooth this out a bit and make it self-healing //most// of the time: basically, have the broken node rewind itself by updating from another good node. But that's a little more complex.)
Test Plan:
- Pushed changes to a cluster-mode repository.
- Viewed web interface, saw "writing" flag and version changes.
- Pulled changes.
- Faked various failures, got sensible states.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15688
Summary: Closes T10690
Test Plan: Open Badges application, go to Advanced Search, search for a badge by its name and see result.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T10690
Differential Revision: https://secure.phabricator.com/D15656
Summary: Ref T6027. This converts the old transaction records to the new format so we don't have to keep legacy code around.
Test Plan: Migrated tasks, browsed around, looked at transaction records, didn't see any issues.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6027
Differential Revision: https://secure.phabricator.com/D15637
Summary: Ref T7303. This interaction is very oldschool; modernize it to enable/disable instead of "nuke from orbit".
Test Plan:
- Enabled applications.
- Disabled applications.
- Viewed applications in list view.
- Generated new tokens.
- Tried to use a token from a disabled application (got rebuffed).
- Tried to use a token from an enabled application (worked fine).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7303
Differential Revision: https://secure.phabricator.com/D15620
Summary: Ref T7303. This application is currently stone-age tech (no transactions, hard "delete" action). Bring it up to modern specs.
Test Plan:
- Created and edited an OAuth application.
- Viewed transaction record.
- Tried to create something with no name, invalid redirect URI, etc. Was gently rebuffed with detailed explanatory errors.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7303
Differential Revision: https://secure.phabricator.com/D15609
Summary: Adds basic commenting to Fund Initiatives.
Test Plan: Leave a comment, see comment.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15554
Summary:
Ref T10537. Currently, Nuance has a `NuanceRequestor` object, intended to represent the external user who created content (e.g., a GitHub account or a Twitter account or whatever).
This object is currently almost unused, and its design predates Doorkeeper. In D15541, I chose to use doorkeeper objects instead of NuanceRequestor objects to represent requestors.
I don't currently anticipate a need for such an object, given that we have Doorkeeper. If we do need it in the future for some reason, it would be fairly easy to restore it, create a requestor type which wraps a Doorkeeper object, and then migrate. Not super thrilling to do that, but not a huge mess.
`NuanceItem` still has a `requestorPHID`, but this is now a less formal object PHID instead of a more formal Requestor-object PHID, and holds a doorkeeper exeternal object PHID for GitHub events.
Test Plan:
- Grepped for `nuancerequestor`.
- Ran `bin/storage upgrade -f`.
- Grepped for `requestor`, remaining uses of this term seem reasonable/correct.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15546
Summary: Ref T8996, Convert badge recipients from Edges to actual BadgeAward objects
Test Plan: Create badge, award it to recipient. Make sure adding/removing recipients works. (Still need to migrate exisiting recipients to new table and need to create activity feed blurbs)
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: chad, Korvin
Maniphest Tasks: T8996
Differential Revision: https://secure.phabricator.com/D15014
Summary:
Ref T10537. For Nuance, I want to introduce new sources (like "GitHub" or "GitHub via Nuance" or something) but this needs to modularize eventually.
Split ContentSource apart so applications can add new content sources.
Test Plan:
This change has huge surface area, so I'll hold it until post-release. I think it's fairly safe (and if it does break anything, the breaks should be fatals, not anything subtle or difficult to fix), there's just no reason not to hold it for a few hours.
- Viewed new module page.
- Grepped for all removed functions/constants.
- Viewed some transactions.
- Hovered over timestamps to get content source details.
- Added a comment via Conduit.
- Added a comment via web.
- Ran `bin/storage upgrade --namespace XXXXX --no-quickstart -f` to re-run all historic migrations.
- Generated some objects with `bin/lipsum`.
- Ran a bulk job on some tasks.
- Ran unit tests.
{F1190182}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15521
Summary:
Ref T10537. Generally, when users interact with Nuance items we'll dump a command into a queue and apply it in the background. This avoids race conditions with multiple users interacting with an item, which Nuance is more subject to than other applications because it has an import/external component.
The "sync" command doesn't actually do anything yet.
Test Plan: {F1186365}
Reviewers: chad
Reviewed By: chad
Subscribers: Luke081515.2
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15506
Summary:
Ref T10537. These are objects which are bound to some external object, like a Maniphest task which is a representation of a GitHub issue.
This doesn't do much yet and may change, but my thinking is:
- I'm putting these on-object instead of on edges because I think we want to actively change the UI for them (e.g., clearly call out that the object is bridged) but don't want every page to need to do extra queries in the common case where zero bridged objects exist anywhere in the system.
- I'm making these one-to-one, more or less: an issue can't be bridged to a bunch of tasks, nor can a bunch of tasks be bridged to a single issue. Pretty sure this makes sense? I can't come up with any reasonable, realistic cases where you want a single GitHub issue to publish to multiple different tasks in Maniphest.
- Technically, one type of each bridgable object could be bridged, but I expect this to never actually occur. Hopefully.
Test Plan: Ran storage upgrade, loaded some pages.
Reviewers: chad
Reviewed By: chad
Subscribers: Luke081515.2
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15502
Summary:
Ref T7789. This implements:
- A new table to store the `<objectHash, filePHID>` relationship between Git LFS files and Phabricator file objects.
- A basic response to `batch` commands, which return actions for a list of files.
Test Plan:
Ran `git lfs push origin master`, got a little further than previously:
```
epriestley@orbital ~/dev/scratch/poemslocal $ git lfs push origin master
Git LFS: (2 of 1 files) 174.24 KB / 87.12 KB
Git LFS operation "upload/b7e0aeb82a03d627c6aa5fc1bbfd454b6789d9d9affc8607d40168fa18cf6c69" is not supported by this server.
Git LFS operation "upload/b7e0aeb82a03d627c6aa5fc1bbfd454b6789d9d9affc8607d40168fa18cf6c69" is not supported by this server.
```
With `GIT_TRACE=1`, this shows the batch part of the API going through.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7789
Differential Revision: https://secure.phabricator.com/D15489
Summary:
Ref T10603. This makes minor updates to temporary tokens:
- Rename `objectPHID` (which is sometimes used to store some other kind of identifier instead of a PHID) to `tokenResource` (i.e., which resource does this token permit access to?).
- Add a `userPHID` column. For LFS tokens and some other types of tokens, I want to bind the token to both a resource (like a repository) and a user.
- Add a `properties` column. This makes tokens more flexible and supports custom behavior (like scoping LFS tokens even more tightly).
Test Plan:
- Ran `bin/storage upgrade -f`, got a clean upgrade.
- Viewed one-time tokens.
- Revoked one token.
- Revoked all tokens.
- Performed a one-time login.
- Performed a password reset.
- Added an MFA token.
- Removed an MFA token.
- Used a file token to view a file.
- Verified file token was removed after viewing file.
- Linked my account to an OAuth1 account (Twitter).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10603
Differential Revision: https://secure.phabricator.com/D15478
Summary:
Ref T10537. Ref T10538. This polls the GitHub events API and creates Nuance items from the raw data.
It does nothing useful with them.
Test Plan:
- Polled GitHub.
- Saw some items get created.
- X-Poll-Interval seemed to work.
- ETag seemed to work.
- Recognizing when we hit items we've already seen seemed to work.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537, T10538
Differential Revision: https://secure.phabricator.com/D15440
Summary:
Ref T10537. More infrastructure:
- Put a `bin/nuance` in place with `bin/nuance import`. This has no useful behavior yet.
- Allow sources to be searched by substring. This supports `bin/nuance import --source whatever` so you don't have to dig up PHIDs.
Test Plan:
- Applied migrations.
- Ran `bin/nuance import --source ...` (no meaningful effect, but works fine).
- Searched for sources by substring in the UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15436
Summary:
Ref T10537. Some sources (like the future "GitHub Repository" source) need to poll remotes.
- Provide a mechanism for sources to emit import cursors.
- Hook them into the trigger daemon so they'll fire periodically.
- Provide some storage.
This diff does nothing useful or interesting, and is pure infrastructure.
Test Plan:
- Ran `bin/storage upgrade -f`, no adjustment issues.
- Poked around Nuance.
- Ran the trigger daemon, verified it didn't crash and checked for Nuance stuff to do.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15435
Summary:
Ref T10457.
- Let blueprints be tagged so you can search and annotate them a little more easily.
- Give each blueprint type an optional icon to make things a little easier to parse visually.
Test Plan:
- Tagged blueprints.
- Searched by tags.
- Looked at nice little icons.
{F1139712}
Reviewers: chad
Reviewed By: chad
Subscribers: yelirekim
Maniphest Tasks: T10457
Differential Revision: https://secure.phabricator.com/D15392
Summary:
Ref T10457. The ngram indexing seems to be working well; extend it into Drydock.
Also clean up the list controller a little bit.
Test Plan:
- Ran migrations.
- Searched for blueprints by name.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10457
Differential Revision: https://secure.phabricator.com/D15389
Summary: Ref T10457. Allow build plans to be queried by name.
Test Plan:
- Searched for plans by name.
- Renamed a plan, searched for new name.
{F1133085}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10457
Differential Revision: https://secure.phabricator.com/D15359
Summary:
Ref T10449. Currently, we store classes (like "AlmanacClusterRepositoryServiceType") in the database.
Instead, store types (like "cluster.repository").
This is a small change, but types are a little more flexible (they let us freely reanme classes), a little cleaner (fewer magic strings in the codebase), and a little better for API usage (they're more human readable).
Make this minor usability change now, before we unprototype.
Also make services searchable by type.
Also remove old Almanac API endpoints.
Test Plan:
- Ran migration, verified all data migrated properly.
- Created, edited, rebound, and changed properties of services.
- Searched for services by service type.
- Reviewed available Conduit methods.
Reviewers: chad
Reviewed By: chad
Subscribers: yelirekim
Maniphest Tasks: T10449
Differential Revision: https://secure.phabricator.com/D15346
Summary:
Fixes T9762. Ref T10246.
**Disabling Bindings**: Previously, there was no formal way to disable bindings. The internal callers sometimes check some informal property on the binding, but this is a common need and deserves first-class support in the UI. Allow bindings to be disabled.
**Deleting Interfaces**: Previously, you could not delete interfaces. Now, you can delete unused interfaces.
Also some minor cleanup and slightly less mysterious documentation.
Test Plan: Disabled bindings and deleted interfaces.
Reviewers: chad
Reviewed By: chad
Subscribers: yelirekim
Maniphest Tasks: T9762, T10246
Differential Revision: https://secure.phabricator.com/D15345
Summary:
Fixes T6741. Ref T10246. Broadly, we want to protect Almanac cluster services:
- Today, against users in the Phacility cluster accidentally breaking their own instances.
- In the future, against attackers compromising administrative accounts and adding a new "cluster database" which points at hardware they control.
The way this works right now is really complicated: there's a global "can create cluster services" setting, and then separate per-service and per-device locks.
Instead, change "Can Create Cluster Services" into "Can Manage Cluster Services". Require this permission (in addition to normal permissions) to edit or create any cluster service.
This permission can be locked to "No One" via config (as we do in the Phacility cluster) so we only need this one simple setting.
There's also zero reason to individually lock //some// of the cluster services.
Also improve extended policy errors.
The UI here is still a little heavy-handed, but should be good enough for the moment.
Test Plan:
- Ran migrations.
- Verified that cluster services and bindings reported that they belonged to the cluster.
- Edited a cluster binding.
- Verified that the bound device was marked as a cluster device
- Moved a cluster binding, verified the old device was unmarked as a cluster device.
- Tried to edit a cluster device as an unprivileged user, got a sensible error.
{F1126552}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6741, T10246
Differential Revision: https://secure.phabricator.com/D15339
Summary:
I'm having trouble figuring out exactly what the timeframe on this was, but for a while in November we were not writing edges between pastes and their attached files correctly.
An example of this on this install is here:
https://secure.phabricator.com/P1893
That will start working once the migration runs, but until it does it shows this:
{F1126605}
This got fixed so recent stuff works fine, but it looks like WMF updated while the bug was active so they have more affected pastes than we do (we only have about 10).
Test Plan:
Ran this query to find pastes with missing edges:
```
select id, FROM_UNIXTIME(p.dateCreated) from pastebin_paste p LEFT JOIN edge ON edge.src = p.phid AND edge.type = 25 WHERE edge.dst IS NULL order by id;
```
Ran the migration.
Verified the edges were fixed.
Viewed one of the affected pastes, things now worked properly.
Reviewers: chad
Reviewed By: chad
Subscribers: 20after4
Differential Revision: https://secure.phabricator.com/D15340
Summary:
Fixes T10410. Immediate impact of this is that you can now actually delete properties from Almanac services, devices and bindings.
The meat of the change is switching from CustomField to EditEngine for most of the actual editing logic. CustomField creates a lot of problems with using EditEngine for everything else (D15326), and weird, hard-to-resolve bugs like this one (not being able to delete stuff).
Using EditEngine to do this stuff instead seems like it works out much better -- I did this in ProfilePanel first and am happy with how it looks.
This also makes the internal storage for properties JSON instead of raw text.
Test Plan:
- Created, edited and deleted properties on services, devices and bindings.
- Edited and reset builtin properties on repository services.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10410
Differential Revision: https://secure.phabricator.com/D15327
