Page MenuHomePhabricator

Implemented hosted Git repositories in Phabricator over Smart HTTP.
AbandonedPublic

Authored by epriestley on Oct 24 2013, 11:14 AM.
Tags
None
Referenced Files
F14041993: D7391.id16701.diff
Mon, Nov 11, 11:30 PM
F14033159: D7391.diff
Sat, Nov 9, 4:58 PM
F14031918: D7391.id16657.diff
Sat, Nov 9, 12:15 PM
F14030291: D7391.id16657.diff
Sat, Nov 9, 1:58 AM
F14009810: D7391.id16659.diff
Wed, Oct 30, 11:52 PM
F13997810: D7391.id16647.diff
Thu, Oct 24, 6:17 AM
F13994276: D7391.id16690.diff
Wed, Oct 23, 5:02 AM
F13992327: D7391.diff
Tue, Oct 22, 3:48 PM

Details

Reviewers
hach-que
btrahan
Group Reviewers
Blessed Reviewers
Maniphest Tasks
Restricted Maniphest Task
Summary

A massive extension of D7387.

This basically allows Phabricator to support hosting Git repositories. In this model, when repositories are edited there's now a new sidebar entry called "Hosting" (in addition to "Tracking") in the repository application. The general workflow is as such:

  • Administrator creates a repository with a particular version control type.
  • Administrator goes to the "Hosting" tab.
  • Administrator sets the path, enables hosting and saves changes.
  • Administrator can then set the view (pull) and edit (push) policies for the self-hosted repository.

This diff implements:

  • Serving Git repositories over HTTP, using git-http-backend. I believe the pcntl extension is required due to the use of proc_open, but this is required to feed the posted data into git-http-backend's input stream.
  • A configuration UI under the Repositories application to allow Administrators to create hosted Git repositories.
  • VCS passwords for users. Since Phabricator users don't necessarily login with a username / password combination (because they might be authing with other services such as Google), it was necessary to add support for a "VCS password". A VCS password acts almost identical to the way a Conduit certificate does, except that it's much shorter. When a user goes to pull or push from a Git repository (and the policy is not Public), basic HTTP authorization will be used, and the user is expected to type in their username and VCS password to authorize themselves.
  • Push and pull policies. This allows Administrators to restrict who can pull changes and who can push changes from a hosted repository. I've tested it with all various kinds of policies (including custom and project policies) and they are all enforced correctly.
  • Initialisation of repository. Pretty simple, but basically if the Administrator enters a directory that does not already exist, the code executes git init --bare for them so they don't need to set anything up. If the directory already exists, it's assumed it's already a valid Git repository.

Although this implementation adds support for Git, there's no reason that Mercurial and Subversion support can't be added using the same CGI-based backend. The hosting already infers the version control type from the repository it's attached to, so to add support for these would only require the implementation of MercurialHTTPServer, SubversionHTTPServer and a few switch statements around the Diffusion serving controller.

Also because screenshots are nice:

{F74368}

{F74370}

{F74376}

{F74378}

{F74380}

Test Plan
  • Went to my account settings and generated a VCS password.
  • Added a repository and configured hosting for it.
  • Ensured the policies allowed me push and pull access.
  • Did git clone http://phabricator.local/diffusion/git/PROJECT which worked.
  • Changed the policies to deny push access.
  • Verified I could pull but not push.
  • Tested variations of policies, etc.

Diff Detail

Branch
server
Lint
Lint Errors
SeverityLocationCodeMessage
Errorsrc/applications/auth/provider/PhabricatorAuthProviderPersona.php:19PHL1Unknown Symbol
Errorsrc/applications/policy/rule/PhabricatorPolicyRuleLunarPhase.php:16PHL1Unknown Symbol
Advicesrc/applications/diffusion/controller/DiffusionGitServeController.php:101XHP16TODO Comment
Unit
Test Failures

Event Timeline

hach-que updated this revision to Unknown Object (????).Oct 24 2013, 11:31 AM

Remove irrelevant TODO and remove unused table column.

In broad strokes:

  • I think having only one Repository object is cleaner -- the policies would map like this:
    • View: Can view repository from web UI in Diffusion, and clone/read/pull it.
    • Edit: Can manage repository details from web UI (e.g., edit policies, edit local paths, enable/disable/etc).
    • Push (new): Can push to the repository. At some point we should probably add additional checks (e.g., policies for each branch) but I think a single policy that you must satisfy to be able to push at all is probably a good starting point. I can sketch out the CAN_PUSH capability.
  • Generally, want to get rid of the "Repository" UI and put everything in Diffusion instead. There's a new (not-connected-to-anything) create workflow here: https://secure.phabricator.com/diffusion/create/ and new (connected and functional) edit UIs here: https://secure.phabricator.com/diffusion/P/edit/ -- we should ideally put new stuff in as sections in that.
  • I want to get local directory management on slightly firmer ground before we allow writes to it. Everything this is doing is generally fine, but my plan is to have a default directory (like /var/phabricator/repo/ or something) where we just put everything by default, and move the prompting to be more like "Everything will be done for you automatically! Or you can customize it if you insist. [Laborious Customization] [Sweet Automatic Correctness]". The "local path" is a pain for users to get right and a lot of them make mistakes right now. I want to move this customization to being an advanced option.
  • The VCS password stuff seems like a pretty reasonable solution, but man is it gross. I don't immediately see a cleaner way to do it. One thought is arc push, which would go lookup a password and then just run git push, but that's almost worse.
  • For the URIs, I wonder if we could just detect that a request is a git/svn/hg request by examining it, and then use /diffusion/X/ for everything. This is sort of sketchy, but it strikes me as desirable to have a single URI just work correctly for everything. Git looks like it has request parameters that we could use; it also has a git user-agent, although that seems highly suspect to rely on.

For moving forward, let's do this:

  • Unify Repository/HostedRepository objects.
  • Move "HTTP Access: off, read-only, read/write" option to a new "hosting" panel in repository/X/edit. For now, provide only "off" and "read-only". Default to "off". Mark "read-only" as "(BETA!!!~~)".
  • The read stuff basically all looks good to me and can just go in as-is.
  • Let's stick with /git/ for now, although I'd like to take a shot at maybe getting rid of that. It's not very straightforward, though.
  • Let's split the push/init stuff into a separate diff for later. I want to stabilize things more first and support those in a more first-class way.

Sound reasonableish?

src/applications/diffusion/controller/DiffusionGitServeController.php
36–37

This should use PhabricatorRepositoryQuery so it hits policies.

src/infrastructure/git/GitHTTPServer.php
25–27

It's impossible to get this far without proc_open, I'm pretty sure.

39–63

Add setEnv() to ExecFuture, or use A=b git ..., then:

list($err, $stdout) = id(new ExecFuture('%s', $this->path))
  ->setCWD($pwd)
  ->setEnv(...)
  ->write(PhabricatorStartup::getRawInput())
  ->resolve();
54

Use:

/**
 * @phutil-external-symbol class PhabricatorStartup
 */

...above the method to silence this warning.

I'd like to keep pushing / read-write in scope; at work we're going to be moving to Git within the next few weeks and I'd like to be able to be hosting repositories in Phabricator directly (there's strong pushback against having "yet another thing" for doing the actual hosting).

For now, I don't think "local path" being specified absolutely is too much of a big deal; later on down the track the option to automatically pick a location doesn't impact an administrator's ability to provide a full path if they wish anyway, so this can really be viewed as just "not having the automatic option" for now.

I'll move the hosting options into the new repository edit UI (with the provision that the local hosting path won't be shown unless you can edit it). I'll put the push policy in the policies section, with the column name pushPolicy.

In future I don't think removing /git/ and changing it to /serve/ will be as problematic as we think. We already know the repository type (svn/hg/git), so we can fire off different *HTTPServer implementations based on the repository type, and then we don't even have to care about what requests the client is making. It wouldn't make sense if someone was trying to use Mercurial to clone a Git repository anyway, so I think it's practically better if we just let the client software deal with the fact that it isn't a repository it understands.

src/applications/diffusion/controller/DiffusionGitServeController.php
36–37

I don't believe PhabricatorRepositoryQuery will work here because at this point in time, we don't know the user. We can't evaluate policies until we know the user.

However, we also don't want to prompt the user via Basic Auth unless we know they need it; e.g. if a repository is set to public we don't want them to have to login. This causes a problem because we don't know what the view policy is until we actually have the repository.

Further on in the code it actually does the validation. All of this is a little bit special because this code is technically performing custom authorisation that isn't seen elsewhere in the system.

src/infrastructure/git/GitHTTPServer.php
25–27

I'll remove that check.

39–63

I'll give this a shot, but I'm wary of the fact that I had to get these commands in a very specific order before it would work.

hach-que updated this revision to Unknown Object (????).Oct 25 2013, 9:01 AM
  • Merge hosted repository into repository table.
hach-que updated this revision to Unknown Object (????).Oct 25 2013, 9:26 AM
  • Add phutil-external-symbol for PhabricatorStartup
hach-que updated this revision to Unknown Object (????).Oct 25 2013, 11:03 AM
  • Migrate configuration to new Diffusion editor.
hach-que updated this revision to Unknown Object (????).Oct 25 2013, 11:17 AM
  • Remove hosting configuration from repository application.

Okay, summary of changes:

  • All configuration is now done through the Diffusion editor.
  • The call to git init --bare no longer happens. It's left up to the user to create the directory and initialise the repository on disk for Phabricator to serve it up.
  • I added the CAN_PUSH capability. I don't particularly like the Pushable By text, but I couldn't think of any better phrasing.
  • I haven't yet modified GitHTTPServer to use ExecFuture. Unless this is essential, I think it's better if I just send in some future diffs; one to add setEnv and the other to modify the GitHTTPServer implementation.
hach-que updated this revision to Unknown Object (????).Oct 25 2013, 11:56 AM
  • Put the commas back.
hach-que updated this revision to Unknown Object (????).Oct 25 2013, 10:02 PM

Migrate GitHTTPServer to use ExecFuture. Tested both git clone and git push and they both still work after this change.

hach-que updated this revision to Unknown Object (????).Oct 25 2013, 11:40 PM

Change how the local path and protocol configuration works.

hach-que updated this revision to Unknown Object (????).Oct 25 2013, 11:49 PM

Change push policy transaction constant. Leave migration of code from global to app-specific for later.

epriestley edited reviewers, added: hach-que; removed: epriestley.

I think all of this is in HEAD in some form or other now. Thanks for this, it was a huge help in getting this feature built.