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

713 commits

Author SHA1 Message Date
epriestley
156b156e77 Give Conduit params/return/errors protected visibility
Summary:
Ref T7803. Ref T5873. I want to drive Conduit through more shared infrastructure, but can't currently add parameters automatically.

Put a `getX()` around the `defineX()` methods so the parent can provide default behaviors.

Also like 60% of methods don't define any special error types; don't require them to implement this method. I want to move away from this in general.

Test Plan:
  - Ran `arc unit --everything`.
  - Called `conduit.query`.
  - Browsed Conduit UI.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: hach-que, epriestley

Maniphest Tasks: T5873, T7803

Differential Revision: https://secure.phabricator.com/D12380
2015-04-13 11:58:35 -07:00
epriestley
6e4f508beb Provide "builtin" high-level result orders
Summary:
Ref T7803. Currently, available high-level orders are spread across Query and SearchEngine classes and implemented separately for each application.

Lift the concept of "builtin" (high-level, user-facing, named) orders (similar to "builtin" queries in ApplicationSearch) into the root Query class, and let it drive the SearchEngine implementation. This allows you to define a new order in one place and have it automatically work across the entire stack.

This will also let Conduit expose this information in a straightforward way.

Test Plan:
  - Used ApplicationSearch in Diffusion.
  - Used all result orderings.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7803

Differential Revision: https://secure.phabricator.com/D12379
2015-04-13 11:58:34 -07:00
epriestley
2794c69db5 Remove getPagingColumn() / getReversePaging()
Summary: Ref T7803. Remove these in favor of more generalized paging and ordering.

Test Plan: Sorted and paged results in various applications.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7803

Differential Revision: https://secure.phabricator.com/D12378
2015-04-13 11:58:32 -07:00
epriestley
4114560844 Modernize more paging/order queries
Summary:
Ref T7803. Removes some getReversePaging().

This also fixes `null` column handling, by adding an explicit `'null'` key with possible values "head" (put NULL before other values) or "tail" (put NULL after other values).

Maniphest has some glitchiness in paging through NULLs right now, but I believe it's all pre-existing and will be resolved when it fully converts. Diffusion is fully converted and pages through NULL correctly.

Test Plan:
  - Failed to identify any reason for ChangesetQuery to reverse paging.
  - Paged thorugh Diffusion.
  - Paged through Maniphest.
    - Maniphest has some issues when paging inside a NULL section, but these issues are preexisting and will be resolved later in this change sequence.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7803

Differential Revision: https://secure.phabricator.com/D12371
2015-04-13 11:58:30 -07:00
epriestley
a4a198342e Modernize ReleephProjectQuery ordering/paging
Summary: Ref T7803. Continue removing implementations of getPagingColumn() and getReversePaging().

Test Plan: Browsed and paged through Releeph projects, Maniphest tasks, Diffusion repositories.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7803

Differential Revision: https://secure.phabricator.com/D12358
2015-04-13 11:58:21 -07:00
epriestley
604d1409f1 Make buildPagingClauseFromMultipleColumns() safer
Summary: Ref T7803. Reduce the amount of code we're trusting to build SQL queries.

Test Plan:
  - Paged through results in Maniphest, Differential and Diffusion.
  - Some of the NULLable groups in Maniphest are a bit funky but this was preexisting.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7803

Differential Revision: https://secure.phabricator.com/D12353
2015-04-13 11:58:15 -07:00
epriestley
9dc114d115 Make formatOrderClause() safer
Summary:
Ref T7803. Instead of trusting subqueries to provide safe values, escape them explicitly.

(We'll probably have a few cases somewhere where this doesn't work, but can make them the exception rather than the rule.)

Test Plan: Issued all "order" queries in Diffusion.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7803

Differential Revision: https://secure.phabricator.com/D12351
2015-04-13 11:58:13 -07:00
epriestley
2b3d3cf7e4 Enforce that global locks have keys shorter than 64 characters
Summary:
Fixes T7484. There's a bunch of spooky mystery here but the current behavior can probably cause problems in at least some situations.

Also moves a couple callsigns to monograms (see T4245).

Test Plan:
  - Faked a short lock length to hit the exception.
  - Updated normally.
  - Grepped for other use sites, none seemed suspicious or likely to overflow the lock length.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7484

Differential Revision: https://secure.phabricator.com/D12263
2015-04-02 13:42:22 -07:00
epriestley
e74c386d56 Fix an issue where RepositoryQuery may fail for viewers who can't use Diffusion
Summary:
See IRC. We don't always reach `loadPage()`, particularly if `canViewerUseQueryApplication()` fails.

Perform initialization steps in `willExecute()` instead.

Trace from IRC:

```
2015/04/02 11:48:32 [error] 26979#0: *48012 FastCGI sent in stderr: "PHP message: [2015-04-02 11:48:32] EXCEPTION: (Exception) You must execute() the query before accessing the identifier map. at [<phabricator>/src/applications/repository/query/PhabricatorRepositoryQuery.php:134]
PHP message:   #0 PhabricatorRepositoryQuery::getIdentifierMap() called at [<phabricator>/src/applications/diffusion/remarkup/DiffusionRepositoryRemarkupRule.php:26]
PHP message:   #1 DiffusionRepositoryRemarkupRule::loadObjects(array) called at [<phabricator>/src/infrastructure/markup/rule/PhabricatorObjectRemarkupRule.php:309]
PHP message:   #2 PhabricatorObjectRemarkupRule::didMarkupText() called at [<phutil>/src/markup/engine/PhutilRemarkupEngine.php:292]
PHP message:   #3 PhutilRemarkupEngine::postprocessText(array) called at [<phabricator>/src/infrastructure/markup/PhabricatorMarkupEngine.php:138]
PHP message:   #4 PhabricatorMarkupEngine::process() called at [<phabricator>/src/infrastructure/markup/PhabricatorMarkupEngine.php:71]
PHP message:   #5 PhabricatorMarkupEngine::renderOneObject(PhabricatorMarkupOneOff, string, PhabricatorUser) called at [<phabricator>/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldRemarkup.php:47]
PHP message:   #6 PhabricatorStandardCustomFieldRemarkup::renderPropertyViewValue(array) called at [<phabricator>/src/infrastructure/customfield/field/PhabricatorCustomField.php:1191]
PHP message:   #7 PhabricatorCustomField::renderPropertyViewValue(array) called at [<phabricator>/src/infrastructure/customfield/field/PhabricatorCustomFieldList.php:176]
PHP message:   #8 PhabricatorCustomFieldList::appendFieldsToPropertyList(PhabricatorProject, PhabricatorUser, PHUIPropertyListView) called at [<phabricator>/src/applications/project/controller/PhabricatorProjectProfileController.php:220]
PHP message:   #9 PhabricatorProjectProfileController::buildPropertyListView(PhabricatorProject, PhabricatorActionListView) called at [<phabricator>/src/applications/project/controller/PhabricatorProjectPr
```

Test Plan:
  - Used `rX` in remarkup.
  - Used `rX` in search.
  - Browed Diffusion, etc.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D12264
2015-04-02 13:41:30 -07:00
epriestley
e9886c4353 Fix an issue where we would try to release an unheld lock
Summary: Fixes T7484. If the lock failed, we'd still try to unlock it, which is incorrect.

Test Plan: Ran two `bin/repository update X` in different windows, got proper LockException instead of indirect symptomatic "not locked by this process" exception.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7484

Differential Revision: https://secure.phabricator.com/D12253
2015-04-01 17:37:46 -07:00
epriestley
bad645f1ec Remove all application-specific reply handler domains
Summary:
Ref T7199. These were a bad idea which got copy-pasted a bunch.

  - There is zero reason to ever set these to different things.
  - Unsurprisingly, I don't know of any install which has them set to different things.

Unless I've completely forgotten about it, this option was not motivated by some obscure business need, it was just a bad decision which didn't catch anyone's attention at the time.

We partially remedied the mistake at some point by introducing `metamta.reply-handler-domain`, which works as a default for all applications, but never cleaned this mess up.

Test Plan: Sent some mail from applications, verified it picked up appropraite reply handler domains.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7199

Differential Revision: https://secure.phabricator.com/D12231
2015-03-31 16:48:40 -07:00
epriestley
030e05aa4c Remove reply handler instructions from email
Summary:
Ref T7199. Although this is useful for discovery, it's un-useful enough that we already have an option to disable it, and most applications do not provide any meaningful instructions.

Throwing it away makes it easier to move forward and lets us get rid of a config option.

This is becoming a more advanced/power-user feature anyway, and the new syntax will be significantly more complex and hard to explain with a one-liner. I'm currently thinking that I'll maybe make the "help" menu a dropdown and give it some options like:

  +---+
  | O |
  +---+---------------------+
  | Maniphest Documentation |
  | Maniphest Email Actions |
  +-------------------------+

Then you click the "Email Actions" thing and get a runtime-derived list of available options. Not sure if I'll actually build that, but I think we can fairly throw the in-mail instructions away even if we don't go in that specific direction.

Test Plan: Grepped for `replyHandlerInstructions`, got no hits.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7199

Differential Revision: https://secure.phabricator.com/D12229
2015-03-31 16:48:17 -07:00
epriestley
d403700e1f Convert all tokenizers to take token/scalar inputs
Summary: Ref T7689. Ref T4100. This advances the goals of removing `loadViewerHandles()` (only 67 callsites remain!) and letting tokenizers some day take token functions like `viewer()` and `members(differential)`.

Test Plan:
- Sent a new message; used "To".
  - I simplified the cancel URI construction slightly because it's moot in all normal cases.
- Edited a thread; used "Add Participants".
- Searched rooms; used "Participants".
- Searched countdowns; used "Authors".
- Created a diff; used "Repository".
- Edited a revision; edited "Projects"; edited "Reveiwers"; edited "Subscribers".
- Searched for revisions; edited "responsible users"; "authors"; "reviwers"; "subscribers"; "repositories".
- Added revision comments; edited "Add Reveiwers"; "Add Subscribers".
- Commented on a commit; edited "Add Auditors"; "Add subscribers".
- Edited a commit; edited "Projects".
- Edited a repository; edited "Projects".
- Searched feed, used "include Users"; "include Proejcts".
- Searched files, used "authors".
- Edited initiative; edited "Projects".
- Searched backers; used "Backers".
- Searched initiatives; used "Owners".
- Edited build plans; edited "Run Command".
- Searched Herald; used "Authors".
- Added signature exemption in Legalpad.
- Searhced legalpad; used "creators"; used "contributors".
- Searched signatures; used "documents"; used "signers".
- Created meme.
- Searched macros; used "Authors".
- Used "Projects" in Maniphest reports.
- Used Maniphest comment actions.
- Edited Maniphest tasks; edited "Assigned To"; edited "CC"; edited "projects".
- Used "parent" in Maniphest task creation workflow.
- Searched for projects; used "assigned to"; "in any projec"; "in all projects"; "not in projects"; "in users' projects"; "authors"; "subscribers".
- Edited Maniphest bug filing domains, used "Default Author".
- Searched for OAuth applications, used "Creators".
- Edited Owners pacakge; edited "Primary Owner"; edited "Owners".
- Searched for Owners packages; used "Owner".
  - OMG this UI is OLD
- Edited a paste; edited "Projects".
- Searched for paste; used "Authors".
- Searched user activity log; used "Actors"; used "Users".
- Edited a mock; edited "Projects"; edited "CC".
- Searched for mocks; used "Authors".
- Edited Phortune account; edited "Members".
- Edited Phortune merchant account; edited "Members".
- Searched Phrequent; used "Users".
- Edited Ponder question; sued "projects".
- Searched Ponder; used "Authors"; used "Answered By".
- Added project members.
- Searched for projects; used "Members".
- Edited a Releeph product; edited "Pushers".
- Searched pull requests; searched "Requestors".
- Edited an arcanist project; used "Uses Symbols From".
- Searhced push logs; used "Repositories"; used "Pushers".
- Searched repositories; used "In nay project".
- Used global search; used Authors/owners/Subscribers/In Any Project.
- Edited a slowvote; used "Projects".
- Searched slovotes; used "Authors".
- Created a custom "Users" field; edited and searched for it.
- Made a whole lot of typos in this list. ^^^^^^

Did not test:

- Lint is nontrivial to test locally, I'll test it in production.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4100, T7689

Differential Revision: https://secure.phabricator.com/D12224
2015-03-31 14:10:55 -07:00
epriestley
c8529787f3 Provide TERM=dumb for Mercurial commit hooks
Summary: Fixes T7119.

Test Plan: Will make someone test.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: cspeckmim, epriestley

Maniphest Tasks: T7119

Differential Revision: https://secure.phabricator.com/D12182
2015-03-30 13:02:43 -07:00
epriestley
a4bfed8415 Censor response bodies from Mercurial error messages
Summary:
Ref T6755. In Git and Subversion, running `git clone http://google.com/` or `svn checkout http://google.com/` does not echo the response body.

In Mercurial, it does. Censor it from the output of `hg pull` and `hg clone`. This prevents an attacker from:

  - Creating a Mercurial remote repository with URI `http://10.0.0.1/secrets/`; and
  - reading the secrets out of the error message after the clone fails.

Test Plan: Set a Mercurial remote URI to a non-Mercurial repository, ran `repository update`, saw censored error message.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T6755

Differential Revision: https://secure.phabricator.com/D12170
2015-03-26 11:13:17 -07:00
epriestley
6eadfe6a6f Allow repositories to be ordered by commit count
Summary: Fixes T7640.

Test Plan: {F346553}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7640

Differential Revision: https://secure.phabricator.com/D12122
2015-03-23 09:10:34 -07:00
epriestley
80b8dc521d Fix Mercurial command injection vulnerability
Summary: See <http://chargen.matasano.com/chargen/2015/3/17/this-new-vulnerability-mercurial-command-injection-cve-2014-9462.html>.

Test Plan: Crafted bad remote URL; got error instead of code execution.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D12112
2015-03-20 09:26:32 -07:00
epriestley
bd2eaad04f Add "phabricator.silent" for stopping all outbound events from an install
Summary:
Ref T7522. This is mostly useful in the cluster, but could be useful for external installs too.

If you want to import an instance into a test/dry-run state (in the cluster, to test an import; in the general case, to do something like test new hardware or configuration), you currently risk spamming users with a lot of duplicate notifications. In particular, if Phabricator tracks remotes, both instances will continue importing commits and sending email about them. Both instances will try to publish to mirrors, too, which could be bad news, and both instances will try to update linked services.

Instead, provide a flag to let an instance run in "silent mode", which disables all outbound messaging and data.

We need to remember to support this flag on any new outbound channels, but we add about one of those per year so I think that's reasonable.

Test Plan:
  - Flipped config.
  - Saw it void email, feed and mirroring.
  - Didn't test SMS since it's not really in use yet and not convenient to test.
  - (Can you think of any publishing I missed?)

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7522

Differential Revision: https://secure.phabricator.com/D12109
2015-03-18 07:09:43 -07:00
Chad Little
386a517c14 Modernize Repositories a bit
Summary: Switch to modern components, crumbs, pht, object boxes.

Test Plan:
Test browsing a list, click edit, click new repository, click delete, verify all work as expected.

{F330864}

Reviewers: btrahan, epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T7427

Differential Revision: https://secure.phabricator.com/D11984
2015-03-05 09:43:13 -08:00
epriestley
027d4ffd8b Set "importing" flag on repositories created via API
Summary: Ref T6516. We incorrectly fail to set this flag on repositories created via Conduit, which activates too many actions on old commits.

Test Plan:
  - Created a new repository via Conduit, verified it was "importing" after creation.
  - Created a new repostiory via web UI, verified it was "importing" after creation.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T6516

Differential Revision: https://secure.phabricator.com/D11964
2015-03-04 10:36:09 -08:00
Joshua Spence
5b104e291a Fix a missing parameter
Summary: Add a missing parameter to a `pht` call

Test Plan: `arc lint`

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D11912
2015-03-02 08:15:02 +11:00
epriestley
8599145b5e Implement more consistent publishing rules for repositories
Summary:
Ref T7298. We are currently inconsistent about when we publish feed, email, notifications, audits and Herald rules.

Specifically, there are two settings which impact these things:

  - The "importing" flag, which is set when we're importing old commits.
  - The "herald-disabled" flag, which was expanded in scope some time ago and now actually means "disable publishing".

Various parts of the pipeline were checking only one of these flags. Instead, all of them should check both.

(For example, we should never email users about importing repositories, nor trigger audits on them.)

Test Plan: See next revision.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7298

Differential Revision: https://secure.phabricator.com/D11826
2015-02-19 10:38:05 -08:00
epriestley
6a60b8cb6f Set "importStatus" as nonmutable on save()
Summary: Fixes T6840. Depends on D11822, which is a little iffy.

Test Plan:
Verified all references to `importStatus` are either:

  - SQL patches creating the column;
  - reads;
  - writes immediately before an insert; or
  - explicit updates of the column.

That is, I identified no cases of `setImportStatus(X)->save()` on a Commit which may already exist. This //would// break that.

In general, almost all writes go through `$commit->writeImportStatusFlag()`, which is an explicit update.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T6840

Differential Revision: https://secure.phabricator.com/D11823
2015-02-19 10:36:36 -08:00
epriestley
35c55f7ddf Improve visibility of repository credential errors
Summary:
Fixes T7310. We have a whole mechanism for surfacing update errors, but only surface actual update errors, not pull errors.

Instead, surface pull errors too.

Then format them a little more nicely.

Test Plan: {F309769}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7310

Differential Revision: https://secure.phabricator.com/D11821
2015-02-19 10:32:25 -08:00
epriestley
dd96967306 Only increment status message cursor if we're going to consume the message
Summary:
Fixes the long uptake we saw on `meta.phacility.com`. I regressed this in D11795.

We make three calls to this method, but only one actually consumes the messages. The other two are just checking to see if there are any messages.

Only move the cursor up if we're actually going to process the messages.

Test Plan: Sort of tricky to test convincingly since it's inherently race-prone, but ran `debug pulllocal` and pushed update messages and saw it pick them up.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D11808
2015-02-18 12:53:37 -08:00
epriestley
02b174c2af Allow a different SSH host to be set in Diffusion
Summary:
Ref T6941. In the cluster (and in other reasonable setups) we've separated SSH load balancers from HTTP load balancers.

In particular, ELBs will not let you load balance port 22, so this is likely a reasonable/common issue in larger clusters in AWS.

Allow users to specify an alternate host for SSH traffic.

Test Plan: Set host to someting different, saw it reflected in UI.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T6941

Differential Revision: https://secure.phabricator.com/D11800
2015-02-18 10:51:14 -08:00
epriestley
6a3824a61d Fix an issue where PullLocal daemon could spin in an error loop
Summary: Fixes T7106. If you have bad credentials AND you've pushed an "update this repository" message into the queue, the loop above this level ends up resetting the timer every time we go through it, so the daemon spins in a loop failing forever.

Test Plan:
  - Created a repo with bad credentials.
  - Clicekd "updated now" to queue an update message.
  - Saw daemon run in a loop.
  - Applied patch, no loop.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7106

Differential Revision: https://secure.phabricator.com/D11795
2015-02-17 15:23:24 -08:00
epriestley
bdd7a35b30 Remove direct calls to LowLevelCommitQuery
Summary: Ref T2783. This cleans up some more of the direct VCS access calls. If the repository is local, this boils down to an in-process call. If not, it uses Conduit to make an intracluster request.

Test Plan: Used `reparse.php --message <commit> --trace` to observe cluster request.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T2783

Differential Revision: https://secure.phabricator.com/D11253
2015-02-10 15:58:51 -08:00
Joshua Spence
aaf8d73ec7 Fix pht method calls
Summary: Ref T7046. This is mainly a proof-of-concept for D11661.

Test Plan: `arc lint`

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T7046

Differential Revision: https://secure.phabricator.com/D11680
2015-02-10 18:57:45 +11:00
Joshua Spence
ddc0041e73 Remove some temporary code
Summary: I think this is safe to remove now.

Test Plan: WIP

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D11717
2015-02-10 08:21:48 +11:00
Chad Little
ae7dc8b9d2 Add getGroup to ConfigOptions
Summary: Adds core and apps grouping to configuration options, makes it somewhat easier to browse config options.

Test Plan: Set each option, review list. Breakdown is nearly 50/50 apps/core.

Reviewers: btrahan, epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D11722
2015-02-09 13:10:56 -08:00
Bob Trahan
5a9df1a225 Policy - filter app engines where the user can't see the application from panel editing
Summary: Fixes T7118. This does the basic "filter the list" thing, though it ends up being a little manual since I guess this hasn't come up before? There is also potential weird behavior if the user was using an app and lost access to it - they will have nothing selected on edit - but I think this is actually correct behavior in this circumstance.

Test Plan:
used a user who couldn't get access to the "quick create" apps and noted that the dropdown list on dashboard panel create was missing the expected engines

ran `arc unit --everything` to verify abstract method implemented everywhere

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T7118

Differential Revision: https://secure.phabricator.com/D11687
2015-02-04 15:47:48 -08:00
Bob Trahan
dd814decbc Policy - remove comment to change policy later as its good as is
Summary: Ref T7094#94295.

Test Plan: noted the absence of the TODO comment in the diff

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T7094

Differential Revision: https://secure.phabricator.com/D11656
2015-02-03 12:28:37 -08:00
Bob Trahan
8c79b3eaed Policy - convert repository worker to query revision with policy query
Summary: Ref T7094. I guess theoretically someone could be making a commit and have just lost access to the revision and thus this could link this commit to that revision, but this all seems far fetched an weird? We also don't necessarily have the commit author's true identity since commit parsing can be a little funky to begin with. Anyhoo, functionally, this makes things no worse, but I am removing the TODO that would make us look at this in a fun way.

Test Plan: `bin/repository reparse --owners rXvalidhash` and it worked

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T7094

Differential Revision: https://secure.phabricator.com/D11654
2015-02-03 12:06:53 -08:00
Bob Trahan
026e379483 Policy - do proper policy queries when updating owners packages in commit workers
Summary: Ref T7094. This makes the underlying class take a $user parameter, and then the worker just hands it an omnipotent user. Said underyling class is the benefactor of a small re-factor, dropping one query per-use, though the single query that now remains is policy-based so maybe its a wash or even worse. Still, gotta love one less query.

Test Plan:
a little tricky to test so some extra thought instead

basic acceptance test with `bin/repository reparse --change rValidHashHere`  -- it worked!

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T7094

Differential Revision: https://secure.phabricator.com/D11653
2015-02-03 11:55:49 -08:00
Bob Trahan
f58dce6819 Policy - remove loadRepository() method from ArcanistProjects
Summary: Ref T7094. This loadRepository() method bypassed policy unnecessarily. kill it.

Test Plan: basically un-tested since arcanist projects are deprecated and the main callsites were in releeph. conduit end point still works though!

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T7094

Differential Revision: https://secure.phabricator.com/D11586
2015-02-02 13:58:33 -08:00
Chad Little
99292c5c6a Use icons with Config Options page
Summary: This sets an icon for each config, makes it easier to scan.

Test Plan:
Reload Config page, see all new icons

{F281089}

Reviewers: btrahan, epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D11619
2015-02-02 10:17:25 -08:00
Chad Little
8b06804394 Remove getIconName from all applications
Summary: Not used anymore

Test Plan: grep for 'getIconName'

Reviewers: btrahan, epriestley

Reviewed By: epriestley

Subscribers: hach-que, Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D11582
2015-01-30 12:11:21 -08:00
epriestley
d8550c114d Promote instance identity to the upstream and pass it to commit hooks
Summary:
Fixes T7019. In a cluster environment, pushes currently fail because the commit hook can't identify the instance.

For web processes, the hostname identifies the instance -- but we don't have a hostname in the hook.

For CLI processes, the environment identifies the instance -- but we don't have an environment in the hook under SVN.

Promote the instance identifier into the upstream and pack/unpack it explicitly for hooks. This is probably not useful for anyone but us, but the amount of special-purpose code we're introducing is very small.

I poked at trying to do this in a more general way, but:

  - We MUST know this BEFORE we run code, so the normal subclassing stuff is useless.
  - I couldn't come up with any other parameter which might ever be useful to pass in.

Test Plan: Used `git push` to push code through proxied HTTP, got a clean push.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7019

Differential Revision: https://secure.phabricator.com/D11495
2015-01-27 14:51:48 -08:00
epriestley
ac41db402a In Diffusion, split service URI resolution from conduit client construction
Summary:
Ref T7019. Ref T7034. In both proxying cases, we want to proxy the request but can not do so over Conduit.

Split the URI resolution apart from Conduit client construction so we can just pull an SSH or HTTP/S URI out of the repository without getting an entire Conduit client.

Test Plan: Browsed around a service-hosted repository. This diff has no behavioral changes.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7034, T7019

Differential Revision: https://secure.phabricator.com/D11491
2015-01-27 14:50:38 -08:00
Chad Little
5d8bb61dde Add FontIcon bridge to AppIcons
Summary: Select a similar or better FontAwesome icon to represent each application

Test Plan: Visual inspection

Reviewers: epriestley, btrahan

Subscribers: hach-que, Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D11489
2015-01-24 23:43:01 -08:00
epriestley
8d087ae738 Remove 'initFromConduit' option from Diffusion
Summary:
Ref T2783. I think this served two purposes:

  - Improving performance in cases where we "know" a repository is local.
  - Preventing loops.

It is now obsolete:

  - After D11476, refs can almost always resolve on a fast path.
  - As T2783 moves forward, we can usually no longer know when a repository is local without actually looking it up -- almost everything is allowed to run anywhere.
  - The cluster behavior in D11475 now prevents loops.

Test Plan: `grep`, browsed around. This didn't really do much of anything anymore.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T2783

Differential Revision: https://secure.phabricator.com/D11477
2015-01-23 13:31:45 -08:00
epriestley
d94d1da610 Proxy Diffusion Conduit API calls
Summary:
Fixes T7020. When an external user makes a Conduit request to Diffusion but the repository isn't hosted locally, we need to proxy it.

This also adds a guard layer to prevent requests from getting infinitely proxied inside the cluster.

In "trivial" configurations (where the repository is a service repository, but the service is on the local device) I'm making us always proxy anyway. This basically makes it reasonable to test this stuff (otherwise you'd have to set up two different installs) and this configuration doesn't make much sense in real life (if you're using multiple machines, making one a dedicating daemons+repo box is almost certainly the most reasonable configuration, even for a cluster size of 2).

Test Plan:
  - With a service-hosted repository, made Diffusion conduit calls and browsed the UI. Verified requests got proxied once, then resovled.
  - With a non-service repository, made Diffusion conduit calls and browsed UI. Verified requests were handled in-process immediately.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7020

Differential Revision: https://secure.phabricator.com/D11475
2015-01-23 13:30:52 -08:00
epriestley
7c2474bef7 Move Conduit client construction logic into Repository
Summary: Ref T7020. I need this elsewhere, and it's relatively internal anyway.

Test Plan: Browsed around my local, cluster-configured install and saw everything working fine.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7020

Differential Revision: https://secure.phabricator.com/D11474
2015-01-23 13:30:00 -08:00
Bob Trahan
a03d16907c Audit - fix issue "showing older" on some commits
Summary: Fixes T7021. When I moved around all the timeline stuff I guess I didn't find this "corner" case, which is wildly common in the post-commit review workflow that we don't use.

Test Plan: pre-patch I could reproduce the issue and post patch I could not. The reproduction case is to have a commit with inline comments and then enough subsequent comments to have a "show older" UI. clicking "show older" now works!

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T7021

Differential Revision: https://secure.phabricator.com/D11479
2015-01-23 11:32:38 -08:00
Joshua Spence
25ee2d4508 Rename DifferentialHunk subclasses for consistency
Summary: Ref T5655.

Test Plan: `arc lint`

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T5655

Differential Revision: https://secure.phabricator.com/D11470
2015-01-23 07:17:04 +11:00
Joshua Spence
daadf95537 Fix visibility of PhutilArgumentWorkflow::didConstruct methods
Summary: Ref T6822.

Test Plan: `grep`. This method is only called from within `PhutilArgumentWorkflow::__construct`.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, epriestley

Maniphest Tasks: T6822

Differential Revision: https://secure.phabricator.com/D11415
2015-01-16 07:42:07 +11:00
Joshua Spence
62dfcd1e55 Fix the visibility of PhutilDaemon::run methods
Summary: Ref T6822. This method is only called from `PhutilDaemon::execute()` and can be made `protected`.

Test Plan: See D11404.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T6822

Differential Revision: https://secure.phabricator.com/D11405
2015-01-16 06:59:29 +11:00
Joshua Spence
94b96ae533 Fix visibility of the PhabricatorWorker::doWork() methods
Summary: Ref T6822. This method is only called from within the `PhabricatorWorker::executeTask()` and `PhabricatorWorker::scheduleTask()` methods.

Test Plan: `grep`ped for `->doWork`.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T6822

Differential Revision: https://secure.phabricator.com/D11406
2015-01-16 06:58:50 +11:00
Joshua Spence
463d094f96 Fix method visibility for PhabricatorPolicyAwareQuery subclasses
Summary: Ref T6822.

Test Plan:
`grep` for the following:

  - `->willFilterPage(`
  - `->loadPage(`
  - `->didFilterPage(`
  - `->getReversePaging(`
  - `->didFilterPage(`
  - `->willExecute(`
  - `->nextPage(`

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: hach-que, Korvin, epriestley

Maniphest Tasks: T6822

Differential Revision: https://secure.phabricator.com/D11367
2015-01-14 07:01:16 +11:00