Summary:
- Original command is in SSH_ORIGINAL_COMMAND, not normal argv.
- Use PhutilShellLexer to parse it.
- Fix a protocol encoding issue with ConduitSSHWorkflow. I think I'm going to make this protocol accept multiple commands anyway because SSH pipes are crazy expensive to build (even locally, they're ~300ms).
Test Plan: With other changes, successfully executed "arc list --conduit-uri=ssh://localhost:2222".
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T550
Differential Revision: https://secure.phabricator.com/D4232
Summary:
- Build "sshd-auth" (for authentication) and "sshd-exec" (for command execution) binaries. These are callable by "sshd-vcs", located [[https://github.com/epriestley/sshd-vcs | in my account on GitHub]]. They are based on precursors [[https://github.com/epriestley/sshd-vcs-glue | here on GitHub]] which I deployed for TenXer about a year ago, so I have some confidence they at least basically work.
- The problem this solves is that normally every user would need an account on a machine to connect to it, and/or their public keys would all need to be listed in `~/.authorized_keys`. This is a big pain in most installs. Software like Gitosis/Gitolite solve this problem by giving you an easy way to add public keys to `~/.authorized_keys`, but this is pretty gross.
- Roughly, instead of looking in `~/.authorized_keys` when a user connects, the patched sshd instead runs `echo <public key> | sshd-auth`. The `sshd-auth` script looks up the public key and authorizes the matching user, if they exist. It also forces sshd to run `sshd-exec` instead of a normal shell.
- `sshd-exec` receives the authenticated user and any command which was passed to ssh (like `git receive-pack`) and can route them appropriately.
- Overall, this permits a single account to be set up on a server which all Phabricator users can connect to without any extra work, and which can safely execute commands and apply appropriate permissions, and disable users when they are disabled in Phabricator and all that stuff.
- Build out "sshd-exec" to do more thorough checks and setup, and delegate command execution to Workflows (they now exist, and did not when I originally built this stuff).
- Convert @btrahan's conduit API script into a workflow and slightly simplify it (ConduitCall did not exist at the time it was written).
The next steps here on the Repository side are to implement Workflows for Git, SVN and HG wire protocols. These will mostly just proxy the protocols, but also need to enforce permissions. So the approach will basically be:
- Implement workflows for stuff like `git receive-pack`.
- These workflows will implement enough of the underlying protocol to determine what resource the user is trying to access, and whether they want to read or write it.
- They'll then do a permissons check, and kick the user out if they don't have permission to do whatever they are trying to do.
- If the user does have permission, we just proxy the rest of the transaction.
Next steps on the Conduit side are more simple:
- Make ConduitClient understand "ssh://" URLs.
Test Plan: Ran `sshd-exec --phabricator-ssh-user epriestley conduit differential.query`, etc. This will get a more comprehensive test once I set up sshd-vcs.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603, T550
Differential Revision: https://secure.phabricator.com/D4229
Test Plan: Called it on a diff with postponed linters and no messages.
Reviewers: mgummelt, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4223
Summary: arc incorrectly passes a "user" parameter to differential.createrevision (long ago, we respected it, I think). After D4191 this fatals. Provide a stub call until the next version bump.
Test Plan: inspection
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4220
Summary: I plan to use this in Arcanist.
Test Plan:
$ echo '{}' | arc call-conduit x
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4192
Summary:
Suhosin has about 50 options for filtering input variables, doucmented here:
http://www.hardened-php.net/suhosin/configuration.html
The default behavior of Suhosin is to drop the variable entirely if it violates any of the rules, then continue with the request. It doesn't affect 'php://input' and doesn't drop other variables, so it evades existing detection, and we can't figure out that it's happened at runtime. We could add blanket checks (Suhosin enabled + suhosin.filter.action set to nothing means this may happen, and will be undetectable if it does happen) but can't tailor a check or recovery to this specific problem.
Instead, raise a better error in the specific case where we encounter this, which is Conduit calls of "arc diff" of files over 1MB (the default POST limit). In these cases, Suhosin drops the variable entirely. If there is no 'params', scream. We never encounter this case normall (`arc`, including `arc call-conduit`, always sends this parameter) although other clients might omit it. The only exception is the web console with `conduit.ping`, which submits nothing; make it submit something so it keeps working.
See also https://github.com/facebook/phabricator/issues/233#issuecomment-11186074
Test Plan: Brought up a Debian + Suhosin box, verified the behavior of Suhosin, made requests with and without 'params'.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4144
Summary: This is to reduce number of calls from Arcanist.
Test Plan: Called it from web interface.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4146
Summary:
Issues here:
- Need an application-sized "eye", or a "home" icon for "Phabricator Home".
- Some of the "apps_lb_2x" sliced images are the "_dark_" versions, not the light versions.
- If you slice an application-sized "logout" (power off) icon and application-sized "help" (questionmark in circle) icon I can replace the current menu icons and nearly get rid of "autosprite".
- To replace the icons on /applications/, the non-retina size is "4x", so we'd need "8x" for retina. Alternatively I can reduce the icon sizes by 50%.
- The "Help", "Settings" and "Logout" items currently have a "glowing" hover state, which needs a variant (or we can drop it).
- The /applications/ icons have a white hover state (or we can drop it).
- The 1x application (14x14) icons aren't used anywhere right now, should they be? Maybe in the feed in the future, etc?
- The "apps-2x" and "apps-large" sheets are the same image, but getting them to actually use the same file is a bit tricky, so I just left them separate for now.
Test Plan:
{F26698}
{F26699}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1960
Differential Revision: https://secure.phabricator.com/D4108
Summary: 'cuz we need it in arcanist for T479 to commit as author
Test Plan: verified the return value was correct in conduit
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T479
Differential Revision: https://secure.phabricator.com/D3917
Summary:
This commit doesn't change license of any file. It just makes the license implicit (inherited from LICENSE file in the root directory).
We are removing the headers for these reasons:
- It wastes space in editors, less code is visible in editor upon opening a file.
- It brings noise to diff of the first change of any file every year.
- It confuses Git file copy detection when creating small files.
- We don't have an explicit license header in other files (JS, CSS, images, documentation).
- Using license header in every file is not obligatory: http://www.apache.org/dev/apply-license.html#new.
This change is approved by Alma Chao (Lead Open Source and IP Counsel at Facebook).
Test Plan: Verified that the license survived only in LICENSE file and that it didn't modify externals.
Reviewers: epriestley, davidrecordon
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2035
Differential Revision: https://secure.phabricator.com/D3886
Summary: you can now add, edit, and delete status events. also added a "description" to status events and surface it in the big calendar view on mouse hover. some refactoring changes as well to make validation logic centralized within the storage class.
Test Plan: added, edited, deleted. yay.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T407
Differential Revision: https://secure.phabricator.com/D3810
Summary: When we receive an email, figure out if any of the other tos and ccs are users. If they are, pass their phids through the stach as "exclude phids" and exclude them from getting the email.
Test Plan: used the various applications (audit, differential, maniphest) and noted emails were sent as expected.
Reviewers: epriestley, vrana
Reviewed By: vrana
CC: aran, Korvin, vrana
Maniphest Tasks: T1676
Differential Revision: https://secure.phabricator.com/D3645
Summary: After D3630, make the API more clear: withAllProjects() vs withAnyProjects()
Test Plan: Loaded project page, maniphest task query, reports, filtered by project and "noproject". Grep.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1610
Differential Revision: https://secure.phabricator.com/D3631
Summary: instance-wide this setting be
Test Plan: made a new task and noted the default priority honored what was in btrahan.conf
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1842
Differential Revision: https://secure.phabricator.com/D3626
Summary:
This got refactored at some point and lost access to $method. Also make the error a little more helpful.
See https://groups.google.com/forum/?fromgroups=#!topic/phabricator-dev/05voYIPV7uU
Test Plan:
$ arc list --conduit-uri=http://local.aphront.com:8080/
Exception
ERR-CONDUIT-CORE: Invalid parameter information was passed to method 'conduit.connect', could not decode JSON serialization. Data: xxx{"client":"arc","clientVersion":5,"clientDescription":"orbital:\/INSECURE\/devtools\/arcanist\/bin\/..\/scripts\/arcanist.php list --conduit-uri=http:\/\/local.aphront.com:8080\/","user":"epriestley","host":"http:\/\/local.aphront.com:8080\/api\/","authToken":1349367823,"authSignature":"54bc136589c076ea06f8e5fb77c76ea7d57aec5b"}
(Run with --trace for a full exception trace.)
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3622
Summary: So they're maybe a little easier to deal with? I'm going to take this formally to "plz @chad plz help" land.
Test Plan: {F20329}
Reviewers: btrahan, vrana, chad
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3609
Summary: A bunch of recently-created applications have help available; link to it.
Test Plan: Clicked each app, clicked help link in menu bar, ended up in relevant documentation.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3602
Test Plan: Tested with various unit test states and noted that the
worst unit test result was always the state used for the entire diff.
Reviewers: nh, epriestley
Reviewed By: nh
Differential Revision https://secure.phabricator.com/D3465
Summary:
If the actual commit message has a duplicate field and we shouldAutoclose it then the commit message parser fails.
Put the error in `$errors` instead.
Test Plan:
Reparsed commit with duplicate field in message.
Tried to `arc diff` message with duplicate field.
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3470
Summary:
I want to implement a `{P123}` rule to embed pastes, but we need viewers everywhere before it will work with privacy.
This is not exhaustive; many Remarkup callsites haven't been converted to `PhabricatorMarkupInterface` yet.
Test Plan: Looked at Maniphest, Differential, Diffusion, Phriction; added markup, made edits and hit previews.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3428
Summary:
We have lots of info about unit tests.
This allows linking them from Unit field.
Test Plan: Monkey patched `$test['link']`, clicked on it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3434
Summary:
This does a few things:
- Allows you to flag pastes. This is straightforward.
- Allows Applications to register event listeners.
- Makes object action lists emit a 'didrenderactions' event, so other applications can add more actions. The Flags application injects its action in this way. This should generally make it much easier to add actions to objects when we add new applications, with less code duplication and better modularity. We have a really hacky version of this in Differential that I want to get rid of in lieu of this more general approach. I'm going to make object lists do the same thing, so any application can jump in and add stuff.
Test Plan: Flagged and unflagged pastes. Viewed home page, differential, flags list.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3377
Summary: This has been deprecated for quite a while and I'm pretty sure there are no callsites in the wild since this tool doesn't get much use outside of Facebook.
Test Plan: grep
Reviewers: vrana, btrahan, meitros
Reviewed By: vrana
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3195
Summary: See T1677. I think wanting bots to be able to post comments without sending email is a pretty reasonable use case. Eventually we should probably support this more broadly and maybe protect it with permissions (normal users maybe shouldn't be able to do this?) but we can wait for use cases.
Test Plan: Made comments with and without "silent". Verified that the non-silent comment sent email, and the silent comment did not.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1677
Differential Revision: https://secure.phabricator.com/D3341
Summary: See D3291.
Test Plan: Ran `phpast.getast` via API.
Reviewers: alanh
Reviewed By: alanh
CC: aran
Differential Revision: https://secure.phabricator.com/D3292
Summary:
Create `phpast.{version,getast}` methods for calling xhpast
with `--version` and with source code as input, respectively.
Test Plan:
Run `arc call-conduit` a bunch of times; delete xhpast; run
it a bunch more times.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1534
Differential Revision: https://secure.phabricator.com/D3289
Summary:
- Add getHelpURI() to PhabricatorApplication for application user guides.
- Add a new "help" icon menu item and skeletal Diviner application.
- Move help tabs to Applications where they exist, document the other ones that don't exist yet.
- Grep for all tab-related stuff and delete it.
Test Plan: Clicked "help" for some apps. Clicked around randomly in a bunch of other apps.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3267
Summary: 'cuz we don't need it and it's lame complexity for API clients of all kinds. Rip the band-aid off now.
Test Plan: used conduit console and verified no more shield. also did some JS stuff around the suite to verify I didn't kill JS
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T891
Differential Revision: https://secure.phabricator.com/D3265
Summary: We managed to move enough Owners stuff aside to make this reasonable; make projects implement the policy interface and projectquery use cursor-based paging.
Test Plan:
- Grepped for ProjectQuery callsites.
- Created an audit comment.
- Used `project.query` to query projects.
- Loaded homepage.
- Viewed Maniphest task list, grouped by project.
- Viewed project list.
- Created / edited project.
- Browsed Owners.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3200
Summary:
I want to:
- move the membership storage to edges
- remove the concepts of "roles" (which are decorative text only) and "owners" (which will be replaced with policy-based controls)
This moves us a step closer to that by reducing the use of ProjectAffiliation outside of the class.
Test Plan: Loaded project profile. Called `project.query`. Joined and left a project.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3182
Summary: This is clearer and more consistent with other Query classes.
Test Plan: Used home page, conduit api, project list, other interfaces.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3179
Summary:
- Put the code to generate informational dicts about flags into the
base class.
- Update flag.delete to accept an object PHID in order to delete the
flag on that object, since currently the model is that each object
may have at most one flag, and each flag has exactly one object,
although the former is not enforced.
- Add flag.edit, which creates or updates a flag, optionally with the
given color and note.
Test Plan:
Spend endless hours repeatedly running arc call-conduit and
arc flag.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3141
Summary: as title
Test Plan: tested without params. Tested with single known path
Reviewers: epriestley, vrana, nh
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3139
Summary:
To make it easier to monitor daemons, let's store their current state
(running, died, exited, or unknown) to the db. The purpose of this is to
provide more information on the daemon console about the status of daemons,
especially when they are running on multiple machines. This is mostly backend
work, with only a few frontend changes. (It is also dependent on a change
to libphutil.)
These changes will make dead or stuck daemons more obvious, and will allow
more work on the frontend to hide daemons (and logs) that have exited cleanly,
i.e. ones we don't care about any more.
Test Plan:
- run db migration, check in db that all daemons were marked as exited
- start up a daemon, check in db that it is marked as running
- open web interface, check that daemon is listed as running
- after daemon has been running for a little bit, check in db that dateModified
is being updated (indicating daemon is properly sending heartbeat)
- kill -9 daemon (but don't run bin/phd yet), and check that db still shows it
as running
- edit daemon db entry to show it as being on a different host, and backdate
dateModified field by 3 minutes, and check the web ui to show that the status
is unknown.
- change db entry to have proper host, check in web ui that daemon status is
displayed as dead. Check db to see that the status was saved.
- run bin/phd stop, and see that the formerly dead daemon is now exited.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3126
Summary: ...also swapped "status" and "order" so "status" is first, as in my testing it was sub-optimal to specifiy status (more of "what i want") after order ("how I want it")
Test Plan: ran various queries on my test instance via conduit console and the results all seem correct
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1381
Differential Revision: https://secure.phabricator.com/D3028
