Summary: Depends on D19429. Depends on D19423. Ref T12164. This creates new columns `authorIdentityPHID` and `committerIdentityPHID` on commit objects and starts populating them. Also adds the ability to explicitly set an Identity's assignee to "unassigned()" to null out an incorrect auto-assign. Adds more search functionality to identities. Also creates a daemon task for handling users adding new email address and attempts to associate unclaimed identities.
Test Plan: Imported some repos, watched new columns get populated. Added a new email address for a previous commit, saw daemon job run and assign the identity to the new user. Searched for identities in various and sundry ways.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T12164
Differential Revision: https://secure.phabricator.com/D19443
Summary: Depends on D19423. Ref T12164. Adds controllers capable of listing and editing `PhabricatorRepositoryIdentity` objects. Starts creating those objects when commits are parsed.
Test Plan: Reparsed some revisions, observed objects getting created in the database. Altered some `Identity` objects using the controllers and observed effects in the database. No attempts made to validate behavior under "challenging" author/committer strings.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T12164
Differential Revision: https://secure.phabricator.com/D19429
Summary: Ref T12164. Start building initial objects for managing `RepositoryIdentity` objects. This won't land until much more of the infrastructure is in place.
Test Plan: Ran `bin/storage upgrade` and observed expected table.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T12164
Differential Revision: https://secure.phabricator.com/D19423
Summary:
Depends on D19427. Ref T13130. See PHI251. Support configuring owners packages so they ignore generated paths.
This is still a little rough. A couple limitations:
- It's hard to figure out how to use this control if you don't know what it's for, but we don't currently have a "CheckboxesEditField". I may add that soon.
- The attribute ignore list doesn't apply to Diffusion, only Differential, which isn't obvious. I'll either try to make it work in Diffusion or note this somewhere.
- No documentation yet (which could mitigate the other two issues a bit).
But the actual behavior seems to work fine.
Test Plan:
- Set a package to ignore paths with the "generated" attribute. Saw the package stop matching generated paths in Differential.
- Removed the attribute from the ignore list.
- Tried to set invalid attributes, got sensible errors.
- Queried a package with Conduit, got the ignored attribute list.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13130
Differential Revision: https://secure.phabricator.com/D19428
Summary:
Depends on D19426. Ref T13130. Ref T13065. While I'm making changes to Owners for "Ignore generated paths", clean up the "mailKey" column.
We recently (D19399) added code to automatically generate and manage mail keys so we don't need a ton of `mailKey` properties in the future. Migrate existing mail keys and blow away the explicit column on packages.
Test Plan: Ran migration, manually looked at the database and saw sensible data. Edited a package to send some mail, which looked good.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13130, T13065
Differential Revision: https://secure.phabricator.com/D19427
Summary:
Ref T13065. `mailKey`s are a private secret for each object. In some mail configurations, they help us ensure that inbound mail is authentic: when we send you mail, the "Reply-To" is "T123+456+abcdef".
- The `T123` is the object you're actually replying to.
- The `456` is your user ID.
- The `abcdef` is a hash of your user account with the `mailKey`.
Knowing this hash effectively proves that Phabricator has sent you mail about the object before, i.e. that you legitimately control the account you're sending from. Without this, anyone could send mail to any object "From" someone else, and have comments post under their username.
To generate this hash, we need a stable secret per object. (We can't use properties like the PHID because the secret has to be legitimately secret.)
Today, we store these in `mailKey` properties on the actual objects, and manually generate them. This results in tons and tons and tons of copies of this same ~10 lines of code.
Instead, just store them in the Mail application and generate them on demand. This change also anticipates possibly adding flags like "must encrypt" and "original subject", which are other "durable metadata about mail transmission" properties we may have use cases for eventually.
Test Plan:
- See next change for additional testing and context.
- Sent mail about Herald rules (next change); saw mail keys generate cleanly.
- Destroyed a Herald rule with a mail key, saw the mail properties get nuked.
- Grepped for `getMailKey()` and converted all callsites I could which aren't the copy/pasted boilerplate present in 50 places.
- Used `bin/mail receive-test --to T123` to test normal mail receipt of older-style objects and make sure that wasn't broken.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13065
Differential Revision: https://secure.phabricator.com/D19399
Summary: See discussion in D19379. The 4-tuple of (device, network, address, port) should be unique.
Test Plan: Created lots of duplicate interfaces, bound those interfaces to various services, observed migration script clean things up correctly.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D19388
Summary: Fixes T13129. This at least makes the existing UI work again before we banish Phlux to the shadow realm.
Test Plan: Edited the visibility for a Phlux variable, didn't get an error. Nothing showed up in the edge tables when I made those changes, but at least it doesn't error out anymore.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13129
Differential Revision: https://secure.phabricator.com/D19387
Summary:
The name of networks should be unique.
Also adds support for exact-name queries for AlamanacNetworks.
Test Plan: Applied migration with existing duplicates, saw networks renamed, attempted to add duplicates, got a nice error message.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D19379
Summary:
Depends on D19322. Ref T13120. Ref T12414.
Currently, `AlmanacDevice` has a bit of a beast of a `TYPE_INTERFACE` transaction that fully creates a complex Interface object. This isn't very flexible or consistent, and Interfaces are complex enough to reasonably have their own object behaviors (for example, they have their own PHIDs).
The complexity of this transaction makes modularizing `AlmanacDevice` transactions tricky. To simplify this, move Interface toward having its own set of normal transactions.
This change just adds some reasonable-looking transactions; it doesn't actually hook them up in the UI or make them reachable. I'll test that they actually work as I swap the UI over.
We may also have some code using the `TYPE_INTERFACE` transaction in Phacility support stuff, so that may need to wait a week to actually phase out.
Test Plan: Ran `bin/storage upgrade` and `arc liberate`. This code isn't reachable yet.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13120, T12414
Differential Revision: https://secure.phabricator.com/D19323
Summary:
Depends on D19284. Ref T13110. It's now possible to get a revision into a "Abandoned + But, Never Promoted From Draft" state. Show this in the header and provide the draft hint above the comment area.
Also, remove `shouldBroadcast()`. The method `getShouldBroadcast()` now has the same meaning.
Finally, migrate existing drafts to `shouldBroadcast = false` and default `shouldBroadcast` to `true`. If we don't do this, every older revision becomes a non-broadcasting revision because this flag was not explicitly set on revision creation before, only on promotion out of draft.
Test Plan: Ran migration; abandoned draft revisions and ended up in a draft + abandoned state.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19285
Summary:
Depends on D19249. Ref T13109. Add timing information to the `PushEvent`:
- `writeWait`: Time spent waiting for a write lock.
- `readWait`: Time spent waiting for a read lock.
- `hostWait`: Roughly, total time spent on the leaf node.
The primary goal here is to see if `readWait` is meaningful in the wild. If it is, that motivates smarter routing, and the value of smarter routing can be demonstrated by looking for a reduction in read wait times.
Test Plan: Pushed some stuff, saw reasonable timing values in the table. Saw timing information in "Export Data".
Maniphest Tasks: T13109
Differential Revision: https://secure.phabricator.com/D19250
Summary:
Depends on D19247. Ref T13109. When we receive an SSH request, generate a random unique ID for the request. Then thread it down through the process tree.
The immediate goal is to let the `ssh-exec` process coordinate with `commit-hook` process and log information about read and write lock wait times. Today, there's no way for `ssh-exec` to interact with the `PushEvent`, but this is the most helpful place to store this data for users.
Test Plan: Made pushes, saw the `PushEvent` table populate with a random request ID. Exported data and saw the ID preserved in the export.
Maniphest Tasks: T13109
Differential Revision: https://secure.phabricator.com/D19249
Summary:
See PHI431. Ref T13102. An install is interested in a custom "non-sticky" accept action, roughly.
On the one hand, this is a pretty hacky patch. However, I suspect it inches us closer to T731, and I'm generally comfortable with exploring the realms of "Accept Next Update", "Unblock Without Accepting", etc., as long as most of it doesn't end up enabled by default in the upstream.
Test Plan:
- Accepted and updated revisions normally, saw accepts respect global stickiness.
- Modified the "Accept" action to explicitly be unsticky, saw nonsticky accept behavior after update.
Maniphest Tasks: T13102
Differential Revision: https://secure.phabricator.com/D19211
Summary:
See PHI439. This fills in additional information about Owners packages.
Also removes dead `primaryOwnerPHID`.
Test Plan: Called `owners.search` and reviewed the results. Grepped for `primaryOwnerPHID`.
Differential Revision: https://secure.phabricator.com/D19207
Summary:
Depends on D19182. Ref T11015. This changes `path` from `text255` to `longtext` because paths may be arbitrarily long.
It adds `pathDisplay` to prepare for display paths and storage paths having different values. For now, `pathDisplay` is copied from `path` and always has the same value.
Test Plan:
- Ran migration, checked database for sanity (all `pathDisplay` and `path` values identical).
- Added new paths, saw `pathDisplay` and `path` get the same values.
- Added an unreasonably enormous path with far more than 255 characters.
Maniphest Tasks: T11015
Differential Revision: https://secure.phabricator.com/D19183
Summary:
Depends on D19181. Ref T11015. This nukes duplicates from the table if they exist, then adds a unique key.
(Duplicates should not exist and can not be added with any recent version of the web UI.)
Test Plan:
- Tried to add duplicates with web UI, didn't have any luck.
- Explicitly added duplicates with manual `INSERT`s.
- Viewed packages in web UI and saw duplicates.
- Ran migrations, got a clean purge and a nice unique key.
- There's still no way to actually hit a duplicate key error in the UI (unless you can collide hashes, I suppose), this is purely a correctness/robustness change.
Maniphest Tasks: T11015
Differential Revision: https://secure.phabricator.com/D19182
Summary: Ref T11015. This supports making path names arbitrarily long and putting a proper unique key on the table.
Test Plan:
- Migrated, checked database, saw nice digested indexes.
- Edited a package, saw new rows update with digested indexes.
Maniphest Tasks: T11015
Differential Revision: https://secure.phabricator.com/D19181
Summary: Ref T13099. Move most of the "Update" logic to modular transactions
Test Plan: Created and updated revisions. Flushed the task queue. Grepped for `TYPE_UPDATE`. Reviewed update transactions in the timeline and feed.
Maniphest Tasks: T13099
Differential Revision: https://secure.phabricator.com/D19175
Summary: Depends on D19173. Ref T13096. Adds an optional, disabled-by-default lock log to make it easier to figure out what is acquiring and holding locks.
Test Plan: Ran `bin/lock log --enable`, `--disable`, `--name`, etc. Saw sensible-looking output with log enabled and daemons restarted. Saw no additional output with log disabled and daemons restarted.
Maniphest Tasks: T13096
Differential Revision: https://secure.phabricator.com/D19174
Summary: Depends on D19148. Ref T13088. The new rendering always executes range requests for data it needs, and we can satisfy these requests by loading the smallest number of chunks which span that range.
Test Plan: Piped 50,000 lines of Apache log into Harbormaster, viewed it in the new UI, got sensible rendering times and a reasonable amount of data actually going over the wire.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19149
Summary:
Depends on D19138. Ref T13088. When we want to read the last part of a logfile //and show accurate line numbers//, we need to be able to get from byte offsets to line numbers somehow.
Our fundamental unit must remain byte offsets, because a test can emit an arbitrarily long line, and we should accommodate it cleanly if a test emits 2GB of the letter "A".
To support going from byte offsets to line numbers, compute a map with periodic line markers throughout the offsets of the file. From here, we can figure out the line numbers for arbitrary positions in the file with only a constant amount of work.
Test Plan: Added unit tests; ran unit tests.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19139
Summary: Depends on D19137. Ref T13088. This allows `rebuild-log` to skip work if the chunks are already compressed. It also prepares for a future GC which is looking for "text" or "gzip" chunks to throw away in favor of archival into Files; such a GC can use this column to find collectable logs and then write "file" to it, meaning "chunks are gone, this data is only available in Files".
Test Plan: Ran migration, saw logs populate as "text". Ran `rebuild-log`, saw logs rebuild as "gzip".
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19138
Summary: Depends on D19135. Ref T13088. Denormalize the total log size onto the log itself. This makes reasoning about the log at display time easier, and we don't need to fish around in the database as much to figure out what we're dealing with.
Test Plan: Ran `bin/harbormaster rebuild-log`, saw an existing log populate. Ran `bin/harbormaster write-log`, saw new log write with proper length information.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19136
Summary: Depends on D19131. Ref T13088. During log finalization, stream the log into Files to support "Download Log", archive to Files, and API access.
Test Plan: Ran `write-log` and `rebuild-log`, saw Files objects generate with log content and appropriate permissions.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19132
Summary:
Ref T13083. Facts has a fair amount of weird hardcoding and duplication of responsibilities. Reduce this somewhat: no more hard-coded fact aggregates, no more database-driven list of available facts, etc. Generally, derive all objective truth from FactEngines. This is more similar to how most other modern applications work.
For clarity, hopefully: rename "FactSpec" to "Fact". Rename "RawFact" to "Datapoint".
Split the fairly optimistic "RawFact" table into an "IntDatapoint" table with less stuff in it, then dimension tables for the object PHIDs and key names. This is primarily aimed at reducing the row size of each datapoint. At the time I originally wrote this code we hadn't experimented much with storing similar data in multiple tables, but this is now more common and has worked well elsewhere (CustomFields, Edges, Ferret) so I don't anticipate this causing issues. If we need more complex or multidimension/multivalue tables later we can accommodate them. The queries a single table supports (like "all facts of all kinds in some time window") don't make any sense as far as I can tell and could likely be UNION ALL'd anyway.
Remove all the aggregation stuff for now, it's not really clear to me what this should look like.
Test Plan: Ran `bin/fact analyze` and viewed web UI. Nothing exploded too violently.
Subscribers: yelirekim
Maniphest Tasks: T13083
Differential Revision: https://secure.phabricator.com/D19119
Summary:
Depends on D19099. Ref T13077. Updates Phriction documents to string constants to make API interactions cleaner and statuses more practical to extend.
This does not seem to require any transaction migrations because none of the Phriction transactions actually store status values: status is always a side effect of other edits.
Test Plan: Created, edited, deleted, moved documents. Saw appropriate UI cues. Browsed and filtered documents by status in the index.
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19100
Summary:
Depends on D19095. Ref T6203. Ref T13077. This column is nullable in an inconsistent way. Make it non-nullable.
Also clean up one more content query on the history view.
Test Plan: Ran migration, then created and edited documents without providing a descriptino or hitting `NULL` exceptions.
Maniphest Tasks: T13077, T6203
Differential Revision: https://secure.phabricator.com/D19096
Summary: Ref T13077. Prepares for modern API access to document history using standard "v3" APIs.
Test Plan: Ran migration, verified PHIDs appeared in the database. Created/edited a document, got even more PHIDs in the database.
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19092
Summary:
Ref T13072. See PHI361. The bug in T10746 where aborting builds didn't propagate properly to the buildable was fixed, but existing builds are still stuck "Building".
Since it doesn't look like anything will moot this before these changes promote to `stable`, just migrate these builds into "failed".
Test Plan: Ran migration, saw it affect only relevant builds and correctly fail them.
Maniphest Tasks: T13072
Differential Revision: https://secure.phabricator.com/D19091
Summary: Ref T13054. Companion storage change for D19062.
Test Plan: Applied migration and adjustments. Viewed messages in Harbormaster; created them with `harbormaster.sendmessage`; processed them with `bin/phd debug task`.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13054
Differential Revision: https://secure.phabricator.com/D19063
Summary: Depends on D19056. Fixes T8475. Ref T13054. Merges "ModernHunk" back into "Hunk".
Test Plan: Grepped for `modernhunk`. Reviewed revisions. Created a new revision. Used `bin/differential migrate-hunk` to migrate hunks between storage formats and back.
Maniphest Tasks: T13054, T8475
Differential Revision: https://secure.phabricator.com/D19057
Summary: Ref T13054. Ref T8475. This table has had no readers or writers for more than a year after it was migrated to the modern table.
Test Plan: Ran migration, verified that all the data was still around.
Maniphest Tasks: T13054, T8475
Differential Revision: https://secure.phabricator.com/D19056
Summary: These transaction constants are flipped, which can produce the wrong result in some cases.
Test Plan: `./bin/storage upgrade -f --apply phabricator:20180208.maniphest.02.populate.php`
Differential Revision: https://secure.phabricator.com/D19054
Summary: Ref T11330. Adds general support for webhooks. This is still rough and missing a lot of pieces -- and not yet useful for anything -- but can make HTTP requests.
Test Plan: Used `bin/webhook call ...` to complete requests to a test endpoint.
Maniphest Tasks: T11330
Differential Revision: https://secure.phabricator.com/D19045
Summary:
Ref T4434. Although some of the use cases for this data are better fits for Facts, this data is reasonable to track separately.
I have an approximate view of it already ("closed, ordered by date modified") that's useful to review things that were fixed recently. This lets us make that view more effective.
This just adds (and populates) the storage. Followups will add Conduit, Export, Search, and UI support.
This is slightly tricky because merges work oddly (see T13020).
Test Plan:
- Ran migration, checked database for sensible results.
- Created a task in open/closed status, got the right database values.
- Modified a task to close/open it, got the right values.
- Merged an open task, got updates.
Maniphest Tasks: T4434
Differential Revision: https://secure.phabricator.com/D19037
Summary:
Depends on D19012. Ref T13053. In D19012, I've changed "Thread-Topic" to always use PHIDs.
This change drops the selective on-object storage we have to track the original, human-readable title for objects.
Even if we end up backing out the "Thread-Topic" change, we'd be better off storing this in a table in the Mail app which just has `<objectPHID, first subject we used when sending mail for that object>`, since then we get the right behavior without needing every object to have this separate field.
Test Plan: Grepped for `original`, `originalName`, `originalTitle`, etc.
Reviewers: amckinley
Maniphest Tasks: T13053
Differential Revision: https://secure.phabricator.com/D19013
When we change a nullable column to a non-nullable column, we can get a
data truncation error if any value was "NULL".
This is exceptionally unusual, but our two very oldest Herald rules have
a "NULL" policy on `secure`.
Summary:
Depends on D18926. Ref T6203. Ref T13048. Herald rule repetition policies are stored as integers but treated as strings in most contexts.
After D18926, the integer stuff is almost totally hidden inside `HeraldRule` and getting rid of it completely isn't too tricky.
Do so now.
Test Plan:
- Created "only the first time" and "every time" rules. Did a SELECT on their rows in the database.
- Ran migrations, got a clean bill of health from `storage adjust`.
- Did another SELECT on the rows, saw a faithful conversion to strings "every" and "first".
- Edited and reviewed rules, swapping them between "every" and "first".
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13048, T6203
Differential Revision: https://secure.phabricator.com/D18927
Summary:
Ref T13048. This migration is from January 2012 and probably only impacted Facebook.
It references `HeraldRepetitionPolicyConfig`, which I'd like to change significantly. I initially just replaced the constant with a literal `0`, but I don't think there's any actual value in retaining this migration nowadays.
The cost of removing this migration is: if you installed Phabricator before January 2012 and haven't upgraded since then, you'll have a few more rows in the `APPLIED` table than necessary. Herald will still work correctly.
Test Plan: Reading.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13048
Differential Revision: https://secure.phabricator.com/D18924
Summary:
Depends on D18907. Ref T13043. Ref T12509. We have some weird old password digest behavior that isn't terribly concerning, but also isn't great.
Specifically, old passwords were digested in weird ways before being hashed. Notably, account passwords were digested with usernames, so your password stops working if your username is chagned. Not the end of the world, but silly.
Mark all existing hashes as "v1", and automatically upgrade then when they're used or changed. Some day, far in the future, we could stop supporting these legacy digests and delete the code and passwords and just issue upgrade advice ("Passwords which haven't been used in more than two years no longer work."). But at least get things on a path toward sane, modern behavior.
Test Plan: Ran migration. Spot-checked that everthing in the database got marked as "v1". Used an existing password to login successfully. Verified that it was upgraded to a `null` (modern) digest. Logged in with it again.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043, T12509
Differential Revision: https://secure.phabricator.com/D18908
Summary:
Ref T13043. After D18903, this data has migrated to shared infrastructure and has no remaining readers or writers.
Just delete it now, since the cost of a mistake here is very small (users need to "Forgot Password?" and pick a new password).
Test Plan: Grepped for `passwordHash`, `passwordSalt`, and variations.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18904
Summary:
Ref T13043. This moves user account passwords to the new shared infrastructure.
There's a lot of code changes here, but essentially all of it is the same as the VCS password logic in D18898.
Test Plan:
- Ran migration.
- Spot checked table for general sanity.
- Logged in with an existing password.
- Hit all error conditions on "change password", "set password", "register new account" flows.
- Verified that changing password logs out other sessions.
- Verified that revoked passwords of a different type can't be selected.
- Changed passwords a bunch.
- Verified that salt regenerates properly after password change.
- Tried to login with the wrong password, which didn't work.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18903
Summary:
Ref T13043. In D18898 I moved VCS passwords to the new shared infrastructure.
Before account passwords can move, we need to make two changes:
- For legacy reasons, VCS passwords and Account passwords have different "digest" algorithms. Both are more complicated than they should be, but we can't easily fix it without breaking existing passwords. Add a `PasswordHashInterface` so that objects which can have passwords hashes can implement custom digest logic for each password type.
- Account passwords have a dedicated external salt (`PhabricatorUser->passwordSalt`). This is a generally reasonable thing to support (since not all hashers are self-salting) and we need to keep it around so existing passwords still work. Add salt support to `AuthPassword` and make it generate/regenerate when passwords are updated.
Then add a nice story about password digestion.
Test Plan: Ran migrations. Used an existing VCS password; changed VCS password. Tried to use a revoked password. Unit tests still pass. Grepped for callers to legacy `PhabricatorHash::digestPassword()`, found none.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18900
Summary:
Ref T13043. After D18898, this has been migrated to new, more modern storage and no longer has any readers or writers.
One migration from long ago (early 2014) is affected. Since this is ancient and the cost of dropping this is small (see inline), I just dropped it.
I'll note this in the changelog.
Test Plan: Ran migrations, got a clean bill of health from `storage status`. Grepped for removed symbol.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18899
Summary:
Ref T13043. Migrate VCS passwords away from their dedicated table to new the new shared infrastructure.
Future changes will migrate account passwords and remove the old table.
Test Plan:
- Ran migrations.
- Cloned with the same password that was configured before the migrations (worked).
- Cloned with a different, invalid password (failed).
- Changed password.
- Cloned with old password (failed).
- Cloned with new password (worked).
- Deleted password in web UI.
- Cloned with old password (failed).
- Set password to the same password as it currently is set to (worked, no "unique" collision).
- Set password to account password. !!This (incorrectly) works for now until account passwords migrate, since the uniqueness check can't see them yet.!!
- Set password to a new unique password.
- Cloned (worked).
- Revoked the password with `bin/auth revoke`.
- Verified web UI shows "no password set".
- Verified that pull no longer works.
- Verified that I can no longer select the revoked password.
- Verified that accounts do not interact:
- Tried to set account B to account A's password (worked).
- Tried to set account B to a password revoked on account A (worked).
- Spot checked the `password` and `passwordtransaction` tables for saniity.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18898
Summary:
Ref T13043. Currently:
- Passwords are stored separately in the "VCS Passwords" and "User" tables and don't share as much code as they could.
- Because User objects are all over the place in the code, password hashes are all over the place too (i.e., often somewhere in process memory). This is a very low-severity, theoretical sort of issue, but it could make leaving a stray `var_dump()` in the code somewhere a lot more dangerous than it otherwise is. Even if we never do this, third-party developers might. So it "feels nice" to imagine separating this data into a different table that we rarely load.
- Passwords can not be //revoked//. They can be //deleted//, but users can set the same password again. If you believe or suspect that a password may have been compromised, you might reasonably prefer to revoke it and force the user to select a //different// password.
This change prepares to remedy these issues by adding a new, more modern dedicated password storage table which supports storing multiple password types (account vs VCS), gives passwords real PHIDs and transactions, supports DestructionEngine, supports revocation, and supports `bin/auth revoke`.
It doesn't actually make anything use this new table yet. Future changes will migrate VCS passwords and account passwords to this table.
(This also gives third party applications a reasonable place to store password hashes in a consistent way if they have some need for it.)
Test Plan: Added some basic unit tests to cover general behavior. This is just skeleton code for now and will get more thorough testing when applications move.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18894
Summary:
Fixes T13042. This hooks up the new "silent" mode from D18882 and makes it actually work.
The UI (where we tell you to go run some command and then reload the page) is pretty clumsy, but should solve some problems for now and can be cleaned up eventually. The actual mechanics (timeline aggregation, Herald interaction, etc.) are on firmer ground.
Test Plan:
- Made a normal bulk edit, got mail and feed stories.
- Made a silent bulk edit, no mail and no feed.
- Saw "Silent Edit" marker in timeline for silent edits:
{F5386245}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13042
Differential Revision: https://secure.phabricator.com/D18883
