Summary:
Ref T920. Over time, mail has become much more complex and I think considering "mail", "sms", "postcards", "whatsapp", etc., to be mostly-the-same is now a more promising avenue than building separate stacks for each one.
Throw away all the standalone SMS code, including the Twilio config options. I have a separate diff that adds Twilio as a mail adapter and functions correctly, but it needs some more work to bring upstream.
This permanently destroys the `sms` table, which no real reachable code ever wrote to. I'll call this out in the changelog.
Test Plan:
- Grepped for `SMS` and `Twilio`.
- Ran storage upgrade.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T920
Differential Revision: https://secure.phabricator.com/D19939
Summary:
Depends on D19906. Ref T13222. This isn't going to win any design awards, but make the "wait" and "answered" elements a little more clear.
Ideally, the icon parts could be animated Google Authenticator-style timers (but I think we'd need to draw them in a `<canvas />` unless there's some clever trick that I don't know) or maybe we could just have the background be like a "water level" that empties out. Not sure I'm going to actually write the JS for either of those, but the UI at least looks a little more intentional.
Test Plan:
{F6070914}
{F6070915}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19908
Summary:
Depends on D19898. Ref T13222. See PHI873. Allow objects to opt into an "MFA is required for all edits" mode.
Put tasks in this mode if they're in a status that specifies it is an `mfa` status.
This is still a little rough for now:
- There's no UI hint that you'll have to MFA. I'll likely add some hinting in a followup.
- All edits currently require MFA, even subscribe/unsubscribe. We could maybe relax this if it's an issue.
Test Plan:
- Edited an MFA-required object via comments, edit forms, and most/all of the extensions. These prompted for MFA, then worked correctly.
- Tried to edit via Conduit, failed with a reasonably comprehensible error.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19899
Summary:
Depends on D19896. Ref T13222. See PHI873. Add a core "Sign With MFA" transaction type which prompts you for MFA and marks your transactions as MFA'd.
This is a one-shot gate and does not keep you in MFA.
Test Plan:
- Used "Sign with MFA", got prompted for MFA, answered MFA, saw transactions apply with MFA metadata and markers.
- Tried to sign alone, got appropriate errors.
- Tried to sign no-op changes, got appropriate errors.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19897
Summary:
Depends on D19914. Ref T11351. Some of the Phoilo rabbit holes go very deep.
`PhabricatorApplicationTransactionInterface` currently requires you to implement `willRenderTimeline()`. Almost every object just implements this as `return $timeline`; only Pholio, Diffusion, and Differential specialize it. In all cases, they are specializing it mostly to render inline comments.
The actual implementations are a bit of a weird mess and the way the data is threaded through the call stack is weird and not very modern.
Try to clean this up:
- Stop requiring `willRenderTimeline()` to be implemented.
- Stop requiring `getApplicationTransactionViewObject()` to be implemented (only the three above, plus Legalpad, implement this, and Legalpad's implementation is a no-op). These two methods are inherently pretty coupled for almost any reasonable thing you might want to do with the timeline.
- Simplify the handling of "renderdata" and call it "View Data". This is additional information about the current view of the transaction timeline that is required to render it correctly. This is only used in Differential, to decide if we can link an inline comment to an anchor on the same page or should link it to another page. We could perhaps do this on the client instead, but having this data doesn't seem inherently bad to me.
- If objects want to customize timeline rendering, they now implement `PhabricatorTimelineInterface` and provide a `TimelineEngine` which gets a nice formal stack.
This leaves a lot of empty `willRenderTimeline()` implementations hanging around. I'll remove these in the next change, it's just going to be deleting a couple dozen copies of an identical empty method implementation.
Test Plan:
- Viewed audits, revisions, and mocks with inline comments.
- Used "Show Older" to page a revision back in history (this is relevant for "View Data").
- Grepped for symbols: willRenderTimeline, getApplicationTransactionViewObject, Legalpad classes.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19918
Summary:
Depends on D19912. Ref T11351. Images currently use `getMock()->getPolicy()` stuff to define policies. This causes bugs with object policies like "Subscribers", since the policy engine tries to evaluate the subscribers //for the image// when the intent is to evaluate the subscribers for the mock.
Move this to ExtendedPolicies to fix the behavior, and give Images sensible policy behavior when they aren't attached to a mock (specifically: only the user who created the image can see it).
Test Plan: Applied migrations, created and edited mocks and images without anything blowing up. Set mock visibility to "Subscribers", everything worked great.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19913
Summary: Depends on D19911. Ref T11351. `MarkupInterface` has mostly been replaced with `PHUIRemarkupView`, and isn't really doing anything for us here. Get rid of it to simplify the code.
Test Plan: Viewed various mocks with descriptions and image descriptions, saw remarkup presented properly.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19912
Summary: Continue clean up of super-old code. I am pretty proud of "defrocked", but would also consider "dethroned", "ousted", "unseated", "unmade", or "disenfranchised". I feel like there's a word for being kicked out of Hogwarts and having your wizarding powers revoked, but it is not leaping to mind.
Test Plan: Promoted/demoted users to/from admin, attempted to demote myself and observed preserved witty text, checked user timelines, checked feed, checked DB for sanity, including `user_logs`. I didn't test exposing this via Conduit to attempt promoting a user without having admin access.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D19891
Summary:
Depends on D19886. Ref T13222. Clean up MFA challenges after they expire.
(There's maybe some argument to keeping these around for a little while for debugging/forensics, but I suspect it would never actually be valuable and figure we can cross that bridge if we come to it.)
Test Plan:
- Ran `bin/garbage collect --collector ...` and saw old MFA challenges collected.
- Triggered a new challenge, GC'd again, saw it survive GC while still active.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19888
Summary:
Ref T13222. See PHI873. Ref T9770.
Currently, we support only TOTP MFA. For some MFA (SMS and "push-to-app"-style MFA) we may need to keep track of MFA details (e.g., the code we SMS'd you). There isn't much support for that yet.
We also currently allow free reuse of TOTP responses across sessions and workflows. This hypothetically enables some "spyglass" attacks where you look at someone's phone and type the code in before they do. T9770 discusses this in more detail, but is focused on an attack window starting when the user submits the form. I claim the attack window opens when the TOTP code is shown on their phone, and the window between the code being shown and being submitted is //much// more interesting than the window after it is submitted.
To address both of these cases, start tracking MFA "Challenges". These are basically a record that we asked you to give us MFA credentials.
For TOTP, the challenge binds a particular timestep to a given session, so an attacker can't look at your phone and type the code into their browser before (or after) you do -- they have a different session. For now, this means that codes are reusable in the same session, but that will be refined in the future.
For SMS / push, the "Challenge" would store the code we sent you so we could validate it.
This is mostly a step on the way toward one-shot MFA, ad-hoc MFA in comment action stacks, and figuring out what's going on with Duo.
Test Plan:
- Passed MFA normally.
- Passed MFA normally, simultaneously, as two different users.
- With two different sessions for the same user:
- Opened MFA in A, opened MFA in B. B got a "wait".
- Submitted MFA in A.
- Clicked "Wait" a bunch in B.
- Submitted MFA in B when prompted.
- Passed MFA normally, then passed MFA normally again with the same code in the same session. (This change does not prevent code reuse.)
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13222, T9770
Differential Revision: https://secure.phabricator.com/D19886
Summary:
Ref T13222. See PHI873. Currently, MFA implementations return this weird sort of ad-hoc dictionary from validation, which is later used to render form/control stuff.
I want to make this more formal to handle token reuse / session binding cases, and let MFA factors share more code around challenges. Formalize this into a proper object instead of an ad-hoc bundle of properties.
Test Plan:
- Answered a TOTP MFA prompt wrong (nothing, bad value).
- Answered a TOTP MFA prompt properly.
- Added new TOTP MFA, survived enrollment.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19885
Summary: Cleaning up more super-old code from `PhabricatorUserEditor`. Also fix user logging in approve transactions. I'm not sure how it worked at all previously.
Test Plan: Created new users, renamed them, checked DB for sanity. Entered invalid names, duplicate names, and empty names, got appropriate error messages.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D19887
Summary:
Ref T13222. See PHI873. I'm preparing to introduce a new MFA "Challenge" table which stores state about challenges we've issued (to bind challenges to sessions and prevent most challenge reuse).
This table will reference sessions (since each challenge will be bound to a particular session) but sessions currently don't have PHIDs. Give them PHIDs and slightly modernize some related code.
Test Plan:
- Ran migrations.
- Verified table got PHIDs.
- Used `var_dump()` to dump an organic user session.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19881
Summary: Fixes T13218. We have no more callers to any of this and can get rid of it forever.
Test Plan: Grepped for all four API methods, `LiskDAOSet`, and `inSet`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13218
Differential Revision: https://secure.phabricator.com/D19879
Summary: Depends on D19865. Ref T13222. See PHI996. Provide a `bin/aphlict notify --user ... --message ...` workflow for sending test notifications from the CLI.
Test Plan: {F6058287}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19866
Summary: Depends on D19864. Ref T13222. See PHI996. This is no longer used by anything, so get rid of it.
Test Plan: Grepped; viewed a feed with these stories in it to make sure nothing crashed/exploded.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19865
Summary:
Depends on D19860. Ref T13222. Ref T10743. See PHI996.
Long ago, there were different types of feed stories. Over time, there was less and less need for this, and nowadays basically everything is a "transaction" feed story. Each story renders differently, but they're fundamentally all about transactions.
The Notification test controller still uses a custom type of feed story to send notifications. Move away from this, and apply a transaction against the user instead. This has the same ultimate effect, but involves less weird custom code from ages long forgotten.
This doesn't fix the actual problem with these things showing up in feed. Currently, stories always use the same rendering for feed and notifications, and there need to be some additional changes to fix this. So no behavioral change yet, just slightly more reasonable code.
Test Plan: Clicked the button and got some test notifications, with Aphlict running.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222, T10743
Differential Revision: https://secure.phabricator.com/D19861
Summary:
Ref T13222. Ref T12588. See PHI683. Currently, "Create Subtask" always uses the first edit form that the user has access to for the same task subtype. (For example, if you "Create Subtask" from a "Bug", you get the first edit form for "Bugs".)
I didn't want to go too crazy with the initial subtype implementation, but it seems like we're generally on firm ground and it's working fairly well: user requests are for more flexibility in using the system as implemented, not changes to the system or confusion/difficulty with any of the tradeoffs. Thus, I'm generally comfortable continuing to build it out in the same direction. To improve flexibility, I want to make the options from "Create Subtask" more flexible/configurable.
I plan to let you specify that a given subtype (say, "Quest") prompts you with creation options for a set of other subtypes (say, "Objective"), or prompts you with a particular set of forms.
If we end up with a single option, we just go into the current flow (directly to the edit form). If we end up with more than one option, we prompt the user to choose between them.
This change is a first step toward this:
- When building "Create Subtask", query for multiple forms.
- The default behavior is now "prompt user to choose among create forms of the same subtype". Previously, it was "use the first edit form of the same subtype". This is a behavioral change.
- The next change will make the selected forms configurable.
- (I also plan to make the dialog itself less rough.)
Test Plan: {F6051067}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222, T12588
Differential Revision: https://secure.phabricator.com/D19853
Summary: Ref T13222. See PHI990. The older `user.query` supports availability information, but it isn't currently available in a modern way. Make it available.
Test Plan: {F6048126}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19851
Summary: Ref T13222. Ref T12588. See PHI683. To make "Create Subtask..." fancier, we need slightly more logic around subtype maps. Upgrade the plain old array into a proper object so it can have relevant methods, notably "get a list of valid child subtypes for some parent subtype".
Test Plan: Created and edited tasks, changed task subtypes. Grepped for affected symbols (`newEditEngineSubtypeMap`, `newSubtypeMap`).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222, T12588
Differential Revision: https://secure.phabricator.com/D19852
Summary:
Depends on D19831. Ref T13216. See PHI908. Allegedly, a user copied a large repository into itself and then pushed it. Great backup strategy, but it can create headaches for administrators.
Allow a "maximum paths you can touch with one commit" limit to be configured, to make it harder for users to make this push this kind of commit by accident.
If you actually intended to do this, you can work around this by breaking your commit into pieces (or temporarily removing the limit). This isn't a security/policy sort of option, it's just a guard against silly mistakes.
Test Plan: Set limit to 2, tried to push 3 files, got rejected. Raised limit, pushed changes successfully.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19839
Summary: Depends on D19830. Ref T13216. See PHI908. See PHI750. See PHI885. Allow users to configure a filesize limit, and allow them to adjust the clone/fetch timeout.
Test Plan:
{F6021356}
- Configured a filesize limit and pushed, hit it. Made the limit larger and pushed, change went through.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19831
Summary:
Depends on D19829. Ref T13216. See PHI908. The current implementation is kind of a lot to live in `CommitHookEngine` and will likely fail if `git diff-tree` produces more than 2GB of output.
Pull it out and make it slightly more robust against enormous commits. It's probably limited by this, now:
```
implode("\n", $every_path)
```
We could replace that with some `PhutilReverseRopeSource` primitive or something but since we don't have one of those and it seems unlikely that we'll hit this case in practice, I left it here for now with just the easy stuff converted to be stream-oriented.
Test Plan:
Used this script to test the query against various commits, got good results:
```
<?php
require_once 'scripts/init/init-script.php';
$viewer = PhabricatorUser::getOmnipotentUser();
$repository = id(new PhabricatorRepositoryQuery())
->setViewer($viewer)
->withCallsigns(array('P'))
->executeOne();
var_dump(
id(new DiffusionLowLevelFilesizeQuery())
->setRepository($repository)
->withIdentifier($argv[1])
->execute());
```
Used this to find large commits in history and pull filesizes (worked great, although our largest commit only touches a couple thousand paths):
```
for hash in `git log --format=%H`; do echo -n $hash; echo -n ' '; git diff-tree -r --no-commit-id $hash | wc -l | awk '{print $1}'; done | awk '{print $2 " " $1}' | sort -n
```
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19830
Summary: Depends on D19826. Ref T13216. We have a fair number of options here; add some groups so the "Build" stuff can go in a little subcategory and such.
Test Plan: {F6020896}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19827
Summary: Ref T13222. See PHI986. See PHI896. Harbormaster build targets don't currently have a modern "*.search" API, but there's no reason not to provide one (even if some of the use cases are a little bit questionable).
Test Plan: {F6032423}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19841
Summary: Ref T13216. See D19666. It's currently tricky to profile Herald test runs since you have to submit a form and repeating them is a bit of a mess. Provide a simple CLI wrapper so we can use `--xprofile`. This is also maybe nice-to-have if we're ever debugging anything here.
Test Plan: Ran `bin/herald test --object ... --type ...` and got a sensible looking transcript in the UI.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19806
Summary: Depends on D19798. Ref T13216. This puts at least a basic UI on top of sync logs.
Test Plan:
Viewed logs from the web UI and exported data. Note that these syncs are somewhat simulated since I my local cluster is somewhat-faked (i.e., not actually multiple machines).
{F5995899}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19799
Summary:
Depends on D19778. Ref T13216. See PHI943, PHI889, et al.
We currently have a push log and a pull log, but do not separately log intracluster synchronization events. We've encountered several specific cases where having this kind of log would be helpful:
- In PHI943, an install was accidentally aborting locks early. Having timing information in the sync log would let us identify this more quickly.
- In PHI889, an install hit an issue with `MaxStartups` configuration in `sshd`. A log would let us identify when this is an issue.
- In PHI889, I floated a "push the linux kernel + fetch timeout" theory. A sync log would let us see sync/fetch timeouts to confirm this being a problem in practice.
- A sync log will help us assess, develop, test, and monitor intracluster routing sync changes (likely those in T13211) in the future.
Some of these events are present in the pull log already, but only if they make it as far as running a `git upload-pack` subprocess (not the case with `MaxStartups` problems) -- and they can't record end-to-end timing.
No UI yet, I'll add that in a future change.
Test Plan:
- Forced all operations to synchronize by adding `|| true` to the version check.
- Pulled, got a sync log in the database.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19779
Summary:
Ref T13210. Ref T11908. Add some basic test coverage for the new "%R" introduced in D19764, then convert LiskDAO to implement the "Database + Table Ref" interface.
To move forward, we need to convert all of these (where `%T` is not a table alias):
```counterexample
qsprintf($conn, '... %T ...', $thing->getTableName());
```
...to these:
```
qsprintf($conn, '... %R ...', $thing);
```
The new code is a little simpler (no `->getTableName()` call) which is sort of nice. But we also have a //lot// of `%T` so this is probably going to take a while.
(I'll hold this until after the release cut.)
Test Plan:
- Ran unit tests.
- Browsed around and edited some objects without issues. This change causes a reasonably large percentage of our total queries to use the new code since the LiskDAO builtin queries are some of the most commonly-constructed queries, although there are still ~700 callsites which need to be examined for possible conversion.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210, T11908
Differential Revision: https://secure.phabricator.com/D19765
Summary:
See T13212 for some context and discussion on this being revived.
See T11694 for original context.
Add a query constraint for lease owners and implement the conduit search method for Drydock leases.
Ref T11694. Fixes T13212.
Test Plan:
- Called the API method from conduit and browsed lease queries from the UI.
- Used the new "ownerPHIDs" constraint via API console.
{F5963044}
Reviewers: yelirekim, amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam, epriestley
Maniphest Tasks: T11694, T13212
Differential Revision: https://secure.phabricator.com/D16594
Summary:
Depends on D19672. Ref T13197. See PHI873. This writes a trivial log when we begin acting on a working copy and makes it look reasonable in the UI.
This is mostly just to prove that logging works properly.
Test Plan: {F5887697}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13197
Differential Revision: https://secure.phabricator.com/D19673
Summary:
Depends on D19661. Ref T13077. See PHI840.
When a user edits a page normally, add a "Save as Draft" button. Much of this change is around making that button render and behave properly: it needs to be an `<input type="submit" ...>` so browsers submit it and we can figure out which button the user clicked.
Then there are a few minor rules:
- If you're editing a page which is already a draft, we only give you "Save as Draft". This makes edits to update/revise a draft more natural.
- Highlight "Publish" if it's a likely action that you might want to take.
Internally, there are two types of edits. Both types create a new version with the new content. However:
- A "content" edit sets the version shown on the live page to the newly-created version.
- A "draft" edit does not update the version shown on the live page.
Test Plan: Edited a published document, edited the draft. Published documents. Reverted documents.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19662
Summary:
Depends on D19659. Fixes T1894. Ref T13077. See PHI840.
- Add an EditEngine, although it currently supports no fields.
- Add (basic, top-level-only) commenting (we already had the table in the database).
This will probably create some issues. I'm most concerned about documents accumulating a ton of old, irrelevant comments over time which are hard to keep track of and no longer relevant. But I think this is probably a step forward in almost all cases, and a good thing on the balance.
This also moves us incrementally toward putting all editing on top of EditEngine.
Test Plan: {F5877347}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13077, T1894
Differential Revision: https://secure.phabricator.com/D19660
Summary:
Depends on D19656. Ref T13197. See PHI851.
- This class is now a real object, so get rid of the "Constants" part of the name.
- Rename it for greater consistency with other modern objects.
- Get rid of the `MODERN_` tag now that the old constants are gone.
Test Plan: Bunch of `grep`, browsed around.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13197
Differential Revision: https://secure.phabricator.com/D19657
Summary:
Ref T13195. Ref PHI851. Currently, checkbox constraints don't tell you what values are supported on the API page.
You can figure this out with "Inspect Element" or by viewing the source code, but just render a nice table instead.
Test Plan: {F5862969}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13195
Differential Revision: https://secure.phabricator.com/D19641
Summary:
See PHI844. Ref T13189.
Add "Revision test plan" as an available field for Herald. This is a little niche -- and a little odd because it sticks around even if you fully disable test plans -- but probably broadly reasonable.
The existing "Revision summary" field counterintuitively included the test plan. Separate this out since it's now a separate field and the behavior was weird historic nonsense. I'll note this in the changelog.
Test Plan: Wrote a rule using both fields, verified they generated the expected values.
Reviewers: amckinley
Maniphest Tasks: T13189
Differential Revision: https://secure.phabricator.com/D19623
Summary:
Depends on D19620. Ref T13077. This adds a "Publish" operation which points the current version at some historical version of the document -- not necessarily the most recent version. Newer versions become "drafts".
This is still quite rough and missing a lot of hinting in the UI, I'm just making it work so I can start making the UI understand it.
Test Plan: Used the "Publish" action to publish older versions of a document, saw the document revert. Many UI hints are missing and this operation is puzzling and not yet usable for normal users.
Reviewers: amckinley
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19621
Summary:
Ref T13077. This is mostly just a small cleanup change, even though the actual change is large.
We currently reference content and document objects from one another with `contentID` and `documentID`, but this means that `contentID` must be nullable. Switching to PHIDs allows the column to be non-nullable.
This also supports reorienting some current and future transactions around PHIDs, which is preferable for the API. In particular, I'm adding a "publish version X" transaction soon, and would rather callers pass a PHID than an ID or version number, since this will make the API more consistent and powerful.
Today, `contentID` gets used as a cheaty way to order documents by (content) edit time. Since PHIDs aren't orderable and stuff is going to become actually-revertible soon, replace this with an epoch timestamp.
Test Plan:
- Created, edited, moved, retitled, and deleted Phriction documents.
- Grepped for `documentID` and `contentID`.
- This probably breaks //something// but I'll be in this code for a bit and am likely to catch whatever breaks.
Reviewers: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19619
Summary: Ref T13077. There is no "PHUIDocumentView" so toss the "Pro" suffix from this classname.
Test Plan: Grepped for `PHUIDocumentView` and `PHUIDocumentViewPro`.
Reviewers: amckinley
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19616
Summary:
Depends on D19609. Ref T13189. At some point, we switched from RemarkupEngine to RemarkupView and lost this piece of hack-magic.
Restore the hack-magic. It's still hack-magic instead of a real rule, but things are at least cleaner than they were before.
Test Plan: Viewed `auth.require-approval`, etc. Saw references to other config options linked properly.
Reviewers: amckinley
Maniphest Tasks: T13189
Differential Revision: https://secure.phabricator.com/D19610
Summary:
Depends on D19605. Ref T13189. See PHI642. This adds a separate "Can Disable Users" capability, and makes the underlying transaction use it.
This doesn't actually let you weaken the permission, since all pathways need more permissions:
- `user.edit` needs CAN_EDIT.
- `user.disable/enable` need admin.
- Web UI workflow needs admin.
Upcoming changes will update these pathways.
Without additional changes, this does let you //strengthen// the permission.
This also fixes the inability to disable non-bot users via the web UI.
Test Plan:
- Set permission to "No One", tried to disable users. Got a tailored policy error.
- Set permission to "All Users", disabled/enabled a non-bot user.
Reviewers: amckinley
Maniphest Tasks: T13189
Differential Revision: https://secure.phabricator.com/D19606
Summary:
Depends on D19579. Fixes T10003. These have been deprecated with a setup warning about their impending removal for about two and a half years.
Ref T13164. See PHI642. My overall goal here is to simplify how we handle transactions which have special policy behaviors. In particular, I'm hoping to replace `ApplicationTransactionEditor->requireCapabilities()` with a new, more clear policy check.
A problem with `requireCapabilities()` is that it doesn't actually enforce any policies in almost all cases: the default is "nothing", not CAN_EDIT. So it ends up looking like it's the right place to specialize policy checks, but it usually isn't.
For "Disable", I need to be able to weaken the check selectively (you can disable users if you have the permission, even if you can't edit them otherwise). We have a handful of other edits which work like this (notably, leaving and joining projects) but they're very rare.
Test Plan: Grepped for all removed classes. Edited a Maniphest task.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164, T10003
Differential Revision: https://secure.phabricator.com/D19581
Summary:
Depends on D19577. Ref T13164. See PHI642. This adds modern transaction-oriented enable/disable support.
Currently, this also doesn't let you disable normal users even when you're an administrator. I'll refine the policy model later in this change series, since that's also the goal here (let users set "Can Disable Users" to some more broad set of users than "Administrators").
This also leaves us with two different edit pathways: the old UserEditor one and the new UserTransactionEditor one. The next couple diffs will redefine the other pathways in terms of this pathway.
Test Plan:
- Enabled/disabled a bot.
- Tried to disable another non-bot user. This isn't allowed yet, since even as an administrator you don't have CAN_EDIT on them and currently need it: right now, there's no way for a particular set of transactions to say they can move forward with reduced permissions.
- Tried to enable/disable myself. This isn't allowed since you can't enable/disable yourself.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19579
Summary:
Depends on D19576. Ref T13164. See PHI642. This adds an EditEngine for users and a `user.edit` modern API method.
For now, all it supports is editing real name, blurb, title, and icon (same as "Edit Profile" from the UI).
Test Plan:
- Edited my stuff via the new API method.
- Tried to edit another user, got rejected by policies.
- Tried to create a user, got rejected by policies.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19577
