1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-26 00:32:42 +01:00
Commit graph

15484 commits

Author SHA1 Message Date
epriestley
876638e428 Add a UI element for navigating between versions of a Phriction document
Summary: Depends on D19621. Ref T13077. Fixes T4815. This adds previous/current/next/draft buttons and makes navigation between unpublished and published versions of a document more clear.

Test Plan: {F5841997}

Reviewers: amckinley

Maniphest Tasks: T13077, T4815

Differential Revision: https://secure.phabricator.com/D19622
2018-08-29 13:49:15 -07:00
epriestley
349686319e Allow the published version of a Phriction document to differ from the most recent version
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
2018-08-29 13:47:36 -07:00
epriestley
50f4adef64 Remove on-object mailkeys from Phriction
Summary: Depends on D19619. Ref T13065. Ref T13077. Migrate Phriction mail keys to the new infrastructure and drop the column.

Test Plan: Ran migrations, spot-checked the database.

Reviewers: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13077, T13065

Differential Revision: https://secure.phabricator.com/D19620
2018-08-29 13:43:13 -07:00
epriestley
64cee4a902 Move Phriction internal document/content references from IDs to PHIDs
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
2018-08-29 13:41:24 -07:00
epriestley
04f8270a74 Remove some unusual UI policy hints in Phriction
Summary:
Ref T13077. We currently have these weird policy hints in Phriction that we don't use in other applications. Just remove them for consistency to make the eventual swap to EditEngine a little easier.

Also nuke some unreacahble code.

Test Plan: Loaded edit page, saw more standard UI.

Reviewers: amckinley

Maniphest Tasks: T13077

Differential Revision: https://secure.phabricator.com/D19618
2018-08-29 07:32:45 -07:00
epriestley
4afb6446d9 Allow DocumentView to render with a curtain, and make Phriction use a curtain
Summary:
Depends on D19616. Ref T13077. Fixes T8172. In the last round of design updates, a lot of actions got stuffed into "Actions" menus.

I never really got used to these and think they're a net usability loss, and broadly agree with the feedback in T8172. I'd generally like to move back toward a state where actions are available on the page, not hidden in a menu.

For now, just put a curtain view on these pages. This could be refined later (e.g., stick this menu to the right hand side of the screen) depending on where other Phriction changes go.

(Broadly, I'm also not satisfied with where we ended up on the fixed-width pages like Diffusion > Manage, Config, and Instances. In contrast, I //do// like where we ended up with Phortune in terms of overall design. I anticipate revisiting some of this stuff eventually.)

Test Plan:
  - Looked at Phriction pages on desktop/tablet/mobile/printable -- actions are now available on the page.
  - Looked at other DocumentView pages (like Phame blogs) -- no changes for now.

Reviewers: amckinley

Maniphest Tasks: T13077, T8172

Differential Revision: https://secure.phabricator.com/D19617
2018-08-28 14:58:05 -07:00
epriestley
fd0da4c41f Rename "PHUIDocumentViewPro" to "PHUIDocumentView"
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
2018-08-28 14:53:07 -07:00
epriestley
614f9ba1fb Allow unit test results to specify that their details are formatted with remarkup when reporting to "harbormaster.sendmessage"
Summary: Ref T13189. See PHI710. Ref T13088. Fixes T9951. Allow callers to `harbormaster.sendmessage` to specify that the test details are remarkup so they can use rich formatting and include links, files, etc.

Test Plan: {F5840098}

Reviewers: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13189, T13088, T9951

Differential Revision: https://secure.phabricator.com/D19615
2018-08-28 13:26:11 -07:00
epriestley
632cafec88 Pass commit authorship information to Buildkite
Summary:
Fixes T12251. Ref T13189. See PHI610. The difficulty here is that we don't want to disclose Phabricator account information to Buildkite. We're comfortable disclosing information from `git`, etc.

  - For commits, use the Identity to provide authorship information from Git.
  - For revisions, use the local commit information on the Diff to provide the Git/Mercurial/etc author of the HEAD commit.

Test Plan:
  - Built commits and revisions in Buildkite via Harbormaster.
  - I can't actually figure out how to see author information on the Buildkite side, but the values look sane when dumped locally.

Reviewers: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13189, T12251

Differential Revision: https://secure.phabricator.com/D19614
2018-08-27 12:52:11 -07:00
epriestley
2f5c6541fc Add an "Activated Epoch" and an "Acquired Epoch" to Drydock Leases
Summary: Ref T13189. See PHI690. When a lease is first acquired or activated, note the time. This supports better visibility into queue lengths. For now, this is only queryable via DB and visible in the UI, but can be more broadly exposed in the future.

Test Plan: Landed a revision, saw the leases get sensible timestamps for acquisition/activation.

Reviewers: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13189

Differential Revision: https://secure.phabricator.com/D19613
2018-08-27 11:27:45 -07:00
epriestley
ee823982a4 Remove another old remarkup engine callsite in Config
Summary: Ref T13189. Summaries do not appear to be meaningfully rendered with Remarkup so just drop the engine. See D19610 for the previous change in this vein.

Test Plan: Viewed config list with option summaries.

Reviewers: amckinley

Maniphest Tasks: T13189

Differential Revision: https://secure.phabricator.com/D19612
2018-08-27 11:10:14 -07:00
epriestley
415de4ce37 Repaint filetree more consistently for mobile/device views
Summary:
Ref T13189. See <https://discourse.phabricator-community.org/t/diffusion-differential-mobile-layout-broken-when-enabling-file-tree/1751>.

We currently call a nonexistent `resetdrag()` which does nothing. Some sequences of interactions can result in a blank left column in mobile/device widths.

Repaint the filetree away more consistently on device change.

Test Plan: Viewed a revision, toggled filetree off + on, resized to narrow width. Before: bad left margin, JS console error. After: proper repaint at device breakpoint, no JS console error.

Reviewers: amckinley

Maniphest Tasks: T13189

Differential Revision: https://secure.phabricator.com/D19611
2018-08-27 10:32:47 -07:00
epriestley
b87a809b0b Make some remarkup handling in Config cleaner, fixing {{other.option} links
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
2018-08-27 09:54:19 -07:00
epriestley
0295a00229 When there are no setup issues, don't show a weird empty box
Summary: Ref T13189. When there are no setup issues, we currently double-render a weird setup issues box underneath the notice. Get rid of it.

Test Plan: Viewed page with and without setup issues, saw less awkward UI.

Reviewers: amckinley

Maniphest Tasks: T13189

Differential Revision: https://secure.phabricator.com/D19609
2018-08-27 09:53:52 -07:00
epriestley
cd8b5b82c8 Stop requiring CAN_EDIT to reach the TransactionEditor via "*.edit" in EditEngine
Summary:
Depends on D19607. Ref T13189. See PHI642. Ref T13186.

Some transactions can sometimes be applied to objects you can not edit. Currently, using `*.edit` to edit an object always explicitly requires CAN_EDIT.

Now that individual transactions require CAN_EDIT by default and can reduce or replace this requirement, stop requiring CAN_EDIT to reach the editor.

The only expected effect of this change is that low-permission edits (like disabling a user, leaving a project, or leaving a thread) can now work via `*.edit`.

Test Plan:
  - Tried to perform a normal edit (changing a task title) against an object with no CAN_EDIT. Still got a permissions error.
  - As a non-admin, disabled other users while holding the "Can Disable Users" permission.
  - As a non-admin, got a permissions error while trying to disable other users while not holding the "Can Disable Users" permission.

Reviewers: amckinley

Maniphest Tasks: T13189, T13186

Differential Revision: https://secure.phabricator.com/D19608
2018-08-27 08:10:08 -07:00
epriestley
f9192d07f2 Align web UI "Disable" and "Approve/Disapprove" flows with new "Can Disable Users" permission
Summary:
Depends on D19606. Ref T13189. See PHI642.

  - Disabling/enabling users no longer requires admin. Now, you just need "Can Disable Users".
  - Update the UI to appropriately show the action in black or grey depending on what clicking the button will do.
  - For "Approve/Disapprove", fix a couple bugs, then let them go through without respect for "Can Disable Users". This is conceptually a different action, even though it ultimately sets the "Disabled" flag.

Test Plan:
  - Disabled/enabled users from the web UI as various users, including a non-administrator with "Can Disable Users".
  - Hit permissions errors from the web UI as various users, including an administrator without "Can Disable Users".
  - Saw the "Disable/Enable" action activate properly based on whether clicking the button would actually work.
  - Disapproved a user without "Can Disable Users" permission, tried to re-disapprove a user.
  - Approved a user, tried to reapprove a user.

Reviewers: amckinley

Maniphest Tasks: T13189

Differential Revision: https://secure.phabricator.com/D19607
2018-08-27 08:09:42 -07:00
epriestley
058952e72e Add a "Can Disable Users" capability to the "People" application
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
2018-08-27 08:01:27 -07:00
epriestley
8cf56913d8 Deprecate "user.enable" and "user.disable" API methods, redefine them in terms of "user.edit"
Summary:
Depends on D19604. Ref T13189. See PHI642. Deprecates these in favor of "user.edit", redefines them in terms of it, and removes the old `disableUser()` method.

I kept the "is admin" permissions check for consistency, since these methods have always said "(admin only)". This check may not be the most tailored check soon, but we can just keep executing it in addition to the real check.

For now, this change stops this method from actually disabling non-bot users (since it implicitly adds a CAN_EDIT requirement, and even administrators don't have that). An upcoming change will fix that.

Test Plan: Enabled and disabled a (bot) user via these methods. Checked API UI, saw them marked as "disabled".

Reviewers: amckinley

Maniphest Tasks: T13189

Differential Revision: https://secure.phabricator.com/D19605
2018-08-27 08:00:48 -07:00
epriestley
2f7b10c023 Replace "Disable User" web UI flow with transactions
Summary:
Ref T13189. See PHI642. Upgrades the "Disable" action in the web UI to be transaction-based.

This technically breaks things a little (you can't disable non-bot users, since they now require CAN_EDIT and you won't have it) but an upcoming change will fix the permissions issue.

Test Plan: Disabled and enabled a (bot) user from the web UI.

Reviewers: amckinley

Maniphest Tasks: T13189

Differential Revision: https://secure.phabricator.com/D19604
2018-08-27 08:00:21 -07:00
epriestley
4d89afcc61 Remove requireCapabilities() from ApplicationTransactionEditor and require CAN_EDIT by default
Summary:
Depends on D19585. Ref T13164.

Almost all transactions require CAN_EDIT on the object, but they generally do not enforce this directly today. Instead, this is effectively enforced by Controllers, API methods, and EditEngine doing a `CAN_EDIT` check when loading the object to be edited.

A small number of transactions do not require CAN_EDIT, and instead require only a weaker/lesser permission. These are:

  - Joining a project which you have CAN_JOIN on.
  - Leaving a project which isn't locked.
  - Joining a Conpherence thread you can see (today, no separate CAN_JOIN permission for Conpherence).
  - Leaving a Conpherence thread.
  - Unsubscribing.
  - Using the special `!history` command from email.

Additionally, these require CAN_INTERACT, which is weaker than CAN_EDIT:

  - Adding comments.
  - Subscribing.
  - Awarding tokens.

Soon, I want to add "disabling users" to this list, so that you can disable users if you have "Can Disable User" permission, even if you can not otherwise edit users.

It's possible this list isn't exhaustive, so this change might break something by adding a policy check to a place where we previously didn't have one. If so, we can go weaken that policy check to the appropriate level.

Enforcement of these special cases is currently weird:

  - We mostly don't actually enforce CAN_EDIT in the Editor; instead, it's enforced before you get to the editor (in EditEngine/Controllers).
  - To apply a weaker requirement (like leaving comments or leaving a project), we let you get through the Controller without CAN_EDIT, then apply the weaker policy check in the Editor.
  - Some transactions apply a confusing/redundant explicit CAN_EDIT policy check. These mostly got cleaned up in previous changes.

Instead, the new world order is:

  - Every transaction has capability/policy requirements.
  - The default is CAN_EDIT, but transactions can weaken this explicitly they want.
  - So now we'll get requirements right in the Editor, even if Controllers or API endpoints make a mistake.
  - And you don't have to copy/paste a bunch of code to say "yes, every transaction should require CAN_EDIT".

Test Plan:
- Tried to add members to a Conpherence thread I could not edit (permissions error).
- Left a Conpherence thread I could not edit (worked properly).
- Joined a thread I could see but could not edit (worked properly).
- Tried to join a thread I could not see (permissions error).
- Implemented `requireCapabilites()` on ManiphestTransactionEditor and tried to edit a task (upgrade guidance error).
- Mentioned an object I can not edit on another object (works).
- Mentioned another object on an object I can not edit (works).
- Added a `{F...}` reference to an object I can not edit (works).
- Awarded tokens to an object I can not edit (works).
- Subscribed/unsubscribed from an object I can not edit (works).
- Muted/unmuted an object I can not edit (works).
- Tried to do other types of edits to an object I can not edit (correctly results in a permissions error).
- Joined and left a project I can not edit (works).
- Tried to edit and add members to a project I can not edit (permissions error).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19586
2018-08-24 17:45:56 -07:00
epriestley
b584834b19 In Differential: when the file tree is enabled, default to the "History" tab instead of "Files"
Summary:
Ref T13187. See PHI811. If the file tree is enabled and visible, set the default tab to "History".

  - This is a bit magic.
  - It won't work entirely on mobile (we can't tell that you're on mobile on the server, so we'll pick the "History" tab even though the file tree isn't actually visible on your device).
  - There's no corresponding logic in Diffusion. Diffusion doesn't have the same tab layout, but this makes things somewhat inconsistent.

So I don't love this, but we can try it and see if it's confusing or helpful on the balance.

Test Plan: With filetree on and off, reloaded revisions. Saw appropriate tab selected by default.

Reviewers: amckinley

Maniphest Tasks: T13187

Differential Revision: https://secure.phabricator.com/D19601
2018-08-24 10:29:35 -07:00
epriestley
8a6d767843 Fix a minor text alignment issue for static text comment actions like "Accept Revision"
Summary:
Ref T13187. See PHI836. The "action" comment actions in Differential (Accept, Reject, etc) render a single line of descriptive text. This is currently slightly misaligned.

Give it similar sizing information to the label element to the left, so it lines up properly.

Test Plan:
Note that "Request Review" and "This revision will be..." are now aligned:

{F5828077}

Reviewers: amckinley

Maniphest Tasks: T13187

Differential Revision: https://secure.phabricator.com/D19600
2018-08-24 10:12:06 -07:00
epriestley
b6fa009cf0 Enrich "priority" transactions in Maniphest for "transaction.search"
Summary:
Ref T13187. See <https://discourse.phabricator-community.org/t/task-priority-change-info-missing-in-firehose-webhook/1832/2>. We can reasonably enrich these transactions.

Since priorities don't have unique authorative string identifiers, I've mostly mimicked the `maniphest.search` structure.

Test Plan: Called `transaction.search` on tasks which were: created normally, created with a priority change, saw a priority change after creation. All the output looked useful and sensible.

Reviewers: amckinley

Maniphest Tasks: T13187

Differential Revision: https://secure.phabricator.com/D19599
2018-08-24 10:05:05 -07:00
epriestley
5e4d9dfa92 Condition "Changes Since Last Action" Differential link on "first broadcast", not "new object"
Summary: Ref T13187. Ref T13176. With drafts, we actually want to suppress this link on "first broadcast" (the first time we send mail), not on "new object" (when the revision is created as a draft).

Test Plan: Poked at this locally, will keep an eye on it in production.

Reviewers: amckinley

Maniphest Tasks: T13187, T13176

Differential Revision: https://secure.phabricator.com/D19598
2018-08-24 10:03:55 -07:00
epriestley
ca618a8679 Document that phd.taskmasters is a local setting, per daemon
Summary: Ref T13187. See PHI807. The documentation currently does not make it very clear that this is a local setting, per `phd` process. Make it more clear.

Test Plan: {F5827757}

Reviewers: amckinley

Maniphest Tasks: T13187

Differential Revision: https://secure.phabricator.com/D19597
2018-08-24 08:08:19 -07:00
epriestley
7ef2bb1b56 Support Mercurial "protocaps" wire command
Summary:
Ref T13187. See PHI834. Mercurial has somewhat-recently (changeset is from Jan 2018) introduced a new "protocaps" command, that appears in Mercurial 4.7 and possibly before then.

We must explicitly enumerate all protocol commands because you can't decode the protocol without knowing how many arguments the command expects, so enumerate it.

(Also fix an issue where the related error message had an extra apostrophe.)

Test Plan:
  - Ran `hg clone ...` with client and server on Mercurial 4.7.
  - Before: fatal on unknown "protocaps" command.
  - Midway: better typography in error message.
  - After: clean clone.

Reviewers: amckinley

Maniphest Tasks: T13187

Differential Revision: https://secure.phabricator.com/D19596
2018-08-23 15:06:25 -07:00
epriestley
75a5dd8d8c Add more accessibility labels for screen readers
Summary:
Depends on D19594. See PHI823. Ref T13164.

  - Add a label for the "X" button in comment areas, like "Remove Action: Change Subscribers".
  - Add a label for the floating header display options menu in Differential.
  - Add `role="button"` to `PHUIButtonView` objects that we render with an `<a ...>` tag.

Test Plan:
Viewed a revision with `?__aural__=true`:

  - Saw "Remove Action: ..." label.
  - Saw "Display Options" label.
  - Used inspector to verify that some `<a class="button" ...>` now have `<a class="button" role="button" ...>`. This isn't exhaustive, but at least improves things. A specific example is the "edit", "reply", etc., actions on inline comments.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19595
2018-08-17 13:31:51 -07:00
Austin McKinley
5c4c593af3 Update DiffusionLastModifiedController to use identities
Summary: Ref T12164. Updates another controller to use identities.

Test Plan:
Pretty ad-hoc, but loaded the main pages of several different repos with and without repo identities. I'm not totally convinced the `author` from this data structure is actually being used:
```
$return = array(
  'commit'    => $modified,
  'date'      => $date,
  'author'    => $author,
  'details'   => $details,
);
```

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12164

Differential Revision: https://secure.phabricator.com/D19580
2018-08-17 12:24:21 -07:00
epriestley
438edde031 Add some missing aural button labels for accessibility
Summary:
Ref T13164. See PHI823. (See that issue for some more details and discussion.)

Add aural labels to various buttons which were missing reasonable aural labels.

The "Search" button (magnifying glass in the global search input) had an entire menu thing inside it. I moved that one level up and it doesn't look like it broke anything (?). All the other changes are pretty straightforward.

Test Plan:
{F5806497}

{F5806498}

  - Will follow up on the issue to make sure things are in better shape for the reporting user.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19594
2018-08-17 11:00:29 -07:00
epriestley
a48e6897a4 Remove obsolete setup check call to Maniphest "Can Edit <X>" field checks
Summary: Ref T13164. Missed this in D19581.

Test Plan:
  - Forced setup checks to re-run by visiting {nav Config > Setup Issues} explicitly.
  - Before patch: fatal on call to nonexistent method.
  - After patch: setup issues.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19593
2018-08-16 13:56:20 -07:00
epriestley
0ccf1410e0 Give PhabricatorAuthPassword a formal CAN_EDIT policy
Summary:
Depends on D19585. Ref T13164. This is a precursor for D19586, which causes Editors to start doing more explicit CAN_EDIT checks.

Passwords have an Editor, but don't actually define a CAN_EDIT capability. Define one (you can edit a password if you can edit the object the password is associated with).

(Today, this object is always a User -- this table just unified VCS passwords and Account passwords so they can be handled more consistently.)

Test Plan:
  - With D19586, ran unit tests and got a pass.
  - Edited my own password.
  - Tried to edit another user's password and wasn't permitted to.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19592
2018-08-16 11:53:24 -07:00
epriestley
7e29ec2e2a Move the "Can Lock Projects" check from requireCapabilities() to transaction validation
Summary: Depends on D19584. Ref T13164. This check is an //extra// check: you need EDIT //and// this capability. Thus, we can do it in validation without issues.

Test Plan:
  - This code isn't reachable today: all methods of applying this transaction do a separate check for "Can Lock" upfront.
  - Commented out the "Can Lock" check in the LockController, tried to lock as a user without permission. Was rejected with a policy exception.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19585
2018-08-16 10:56:00 -07:00
epriestley
3b92da22f4 Move the hierarchical edit policy check in Phriction from requireCapabilities() to validateTransactions()
Summary:
Depends on D19583. Ref T13164. This continues the work of getting rid of `requireCapabilities()`.

This check is valid, but can be a `validateTransactions()` check instead. This is generally more consistent with how other applications work (e.g., creating subprojects).

The UI for this isn't terribly great: you get a policy error //after// you try to create the object. But that's how it worked before, so this isn't any worse than it was. The actual policy exception is (very) slightly more clear now (raised against the right object).

Test Plan:
  - Created a child as a user with permission to do so to make sure I didn't break that.
  - Set edit permission on `a/` to just me, tried to create `a/b/` as another user, got a policy exception since they can't edit the parent.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19584
2018-08-16 10:55:11 -07:00
epriestley
24d4445845 Remove pointless requireCapabilities() method from PhabricatorRepositoryEditor
Summary: Depends on D19582. Ref T13164. It's not possible to reach the editor without passing through a CAN_EDIT check, and it shouldn't be necessarily to manually specify that edits require CAN_EDIT by default.

Test Plan: Grepped for `RepositoryEditor`, verified that all callsites pass through a CAN_EDIT check.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19583
2018-08-16 10:53:42 -07:00
epriestley
a39852ae1b Remove pointless requireCapabilities() method from PhabricatorProjectColumnTransactionEditor
Summary:
Depends on D19581. Ref T13164. This method has no effect:

  - You must always have CAN_EDIT to reach an Editor in the first place.
  - Per previous change, I'm going to restructure this so transactions explicitly check CAN_EDIT by default anyway.

Test Plan: Tried to edit and hide a project column as a user without permission, hit global permission checks long before reaching this method.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19582
2018-08-16 10:51:57 -07:00
epriestley
296bf046a8 Remove deprecated Maniphest "Can Edit <Specific Property>" capabilities
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
2018-08-16 10:51:06 -07:00
epriestley
f9673a72a8 Allow "user.edit" to enable or disable users
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
2018-08-16 10:49:35 -07:00
epriestley
65904d7c51 Add a modern "user.edit" API method for users
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
2018-08-16 10:48:38 -07:00
epriestley
39d415e90e Move users to modular transactions
Summary:
Ref T13164. See PHI642. I'd like to provide a third-generation `user.edit` API endpoint and make `user.enable` and `user.disable` obsolete before meddling with policy details, even if it isn't full-fledged yet.

Users do already have a transactions table and a Transaction-based editor, but it's only used for editing title, real name, etc. All of these are custom fields, so their support comes in automatically through CustomField extension code.

Realign it for modular transactions so new code will be fully modern. There are no actual standalone transaction types yet so this diff is pretty thin.

Test Plan:
  - Grepped for `UserProfileEditor`.
  - Edited a user's title/real name/icon.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19576
2018-08-16 10:47:47 -07:00
Joshua Spence
ba25586016 Improve symbol generation scripts
Summary: Currently the symbol generation scripts fail if passed a list containing no files because `explode("\n", $input)` returns `array("")` rather than `array()`. This means that a generic Harbormaster Build Plan with a step which executes `find . -type f -name '*.php' | ./scripts/generate_php_symbols.php` won't work because it fails in repositories that don't contain any PHP code.

Test Plan: Ran `echo | generate_php_symbols` and saw no output instead of an exception.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D19588
2018-08-16 06:40:13 +10:00
Austin McKinley
cc1def6cea Remove some array typehints for passing around
Summary: See discussion at https://secure.phabricator.com/D19492#241996

Test Plan: Refreshed a few Diffusion tabs; nothing broke.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D19578
2018-08-13 16:07:56 -07:00
Austin McKinley
3b05e920e0 Start changing DiffusionCommitController to use identities
Summary: Depends on D19491.

Test Plan: Viewed some commits where the identity was mapped to a user and another that wasn't; saw the header render either a link to the user or the identity object.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D19492
2018-08-13 15:23:31 -07:00
epriestley
9a15129b40 Remove 750ms timeout on owners path validation
Summary:
Ref T13164. See PHI748. Path validation has a 750ms timeout which blames to rP5038ab850c, in 2011.

Production path validation is sometimes taking more than 750ms, particularly on the initial page load where we may validate many paths simultaneously.

I have no idea why we have this timeout, and it isn't consistent with how we perform other AJAX requests. Just remove it.

Test Plan:
  - Reproduced issue in production, saw all validation calls failing at 750ms. Actual underlying calls succeed, they just take more than 750ms to resolve.
  - Loaded path validator locally, got green checkmark.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19575
2018-08-13 13:52:26 -07:00
epriestley
92a29f72c1 Make the Drydock repository operation page slightly richer
Summary:
Ref T13164. See PHI788. The issue requests a "created" timestamp.

Also add filtering for repository, state, and author.

Test Plan:
Used all filters.

{F5795085}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19574
2018-08-13 11:42:10 -07:00
epriestley
fb3ae72e36 When cancelling addition of an Almanac interface, return to the Device page
Summary:
Fixes T13184. In Almanac, interfaces are always added to devices. However, if you "Add New Interface" and then "Cancel", you go to the nonexistent `/interface/` page.

Instead, return to the device page.

Test Plan: From a device page, clicked "Add Interface" and then "Cancel". Ended up back where I was.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13184

Differential Revision: https://secure.phabricator.com/D19573
2018-08-13 11:39:37 -07:00
epriestley
b86dae6214 Fix an issue with error handling when no mailers are available
Summary:
Ref T13164. See PHI785. See D19546. I think I didn't test the updated error messaging here entirely properly, since I have some tasks in queue which error out here ("Missing argument 1 to newMailers(...)").

This is an error condition already, but we want to get through this call so we can raise a tailored message.

Test Plan: Tasks which errored out here now succeed. This condition is only reachable if you misconfigure things in the first place.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19572
2018-08-13 11:39:13 -07:00
epriestley
e5906f4e12 In Differential standalone views, disable some keyboard shortcuts which don't work
Summary:
Ref T13164. See PHI693. In Differential, you can {nav View Options > View Standalone} to get a standalone view of a single changeset. You can also arrive here via the big changeset list for revisions affecting a huge number of files.

We currently suggest that all the keyboard shortcuts work, but some do not. In particular, the "Next File" and "Previous File" keyboard shortcuts (and some similar shortcuts) do not work. In the main view, the next/previous files are on the same page. In the standalone view, we'd need to actually change the URI.

Ideally, we should do this (and, e.g., put prev/next links on the page). As a first step toward that, hide the nonfunctional shortcuts to stop users from being misled.

Test Plan:
  - Viewed a revision in normal and standalone views.
  - No changes in normal view, and all keys still work ("N", "P", etc).
  - In standalone view, "?" no longer shows nonfunctional key commands.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19571
2018-08-13 08:59:05 -07:00
Austin McKinley
a6951a0a5a Add migration to encourage rebuilding repository identities
Summary: Ref T12164. Defines a new manual activity that suggests rebuilding repository identities before Phabricator begins to rely on them.

Test Plan:
- Ran migration, observed expected setup issue: {F5788217}
- Ran `bin/config done identities` and observed setup issue get marked as done.
- Ran `/bin/storage upgrade --apply phabricator:20170912.ferret.01.activity.php` to make sure I didn't break the reindex migration; observed reindex setup issue appear as expected.
- Ran `./bin/config done reindex` and observed reindex issue cleared as expected.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T12164

Differential Revision: https://secure.phabricator.com/D19497
2018-08-10 13:47:03 -07:00
Austin McKinley
2951694c27 Correctly spell 'committer'
Summary: It's a funny word. h/t @joshuaspence

Test Plan: Inspection of correct spelling.

Reviewers: epriestley, joshuaspence

Reviewed By: joshuaspence

Subscribers: Korvin, joshuaspence

Differential Revision: https://secure.phabricator.com/D19570
2018-08-09 17:52:43 -07:00
epriestley
6df278bea8 In "bin/ssh-auth", cache a structure instead of a flat file because paths may change at runtime
Summary:
Fixes T12397. Ref T13164. See PHI801.

Several installs have hit various use cases where the path on disk where Phabricator lives changes at runtime. Currently, `bin/ssh-auth` caches a flat file which includes the path to `bin/ssh-exec`, so this may fall out of date if `phabricator/` moves.

These use cases have varying strengths of legitimacy, but "we're migrating to a new set of hosts and the pool is half old machines and half new machines" seems reasonably compelling and not a problem entirely of one's own making.

Test Plan:
  - Compared output on `master` to output after change, found them byte-for-byte identical.
  - Moved `phabricator/` to `phabricator2/`, ran `bin/ssh-auth`, got updated output.
  - Added a new SSH key, saw it appear in the output.
  - Grepped for `AUTHFILE_CACHEKEY` (no hits).
  - Dropped the cache, verified that the file regenerates cleanly.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164, T12397

Differential Revision: https://secure.phabricator.com/D19568
2018-08-09 13:33:23 -07:00