Summary: Ref T4151. These are a (common) variant of "ssh-dsa" keys (which are somewhat theoretical, but show up on Google).
Test Plan: syntax
Reviewers: btrahan, dctrwatson, phpcodemonkey
Reviewed By: phpcodemonkey
CC: aran
Maniphest Tasks: T4151
Differential Revision: https://secure.phabricator.com/D7704
Summary:
Ref T1715. When the user clicks "Download Raw Diff" in Differential, we try to build a diff of exactly what they're seeing. However:
- This doesn't work if any of the changes have multiple hunks, and fixing it seems hard.
- I suspect this diff is never actually useful anyway? And probably kind of confusing in the best case. You can't really apply it to anyhting, since you'd have to apply another diff first.
Instead, just build the right-side diff, which should align well with user expectation and doesn't suffer from the multi-hunk bug.
Some day, we could maybe add some of the fancy options in T1715.
See: <https://github.com/facebook/phabricator/issues/461>
Test Plan: Downloaded a multi-hunk diff, got the original back and applied it cleanly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1715
Differential Revision: https://secure.phabricator.com/D7694
Summary: This updates DrydockSSHCommandInterface to correctly hold open the private key credentials for the life of the interface so that remote commands will execute correctly with a text-based private key.
Test Plan: Created a text-based private key, created a resource based on it and leased against it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4111
Differential Revision: https://secure.phabricator.com/D7700
Summary: Depends on D7695. This updates preallocated hosts to use Passphrase credentials. Due to the way SSH private key text credentials work (the TempFile disappears before SSH commands can be executed), this only supports file-based private keys at the moment.
Test Plan:
Created a Passphrase credential for a file-based SSH key. Allocated a resource with:
```
bin/drydock create-resource --blueprint 1 --name "My Linux Host" --attributes platform=linux,host=localhost,port=22,path=/var/drydock,credential=2
```
and successfully leased it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4111, T1049
Differential Revision: https://secure.phabricator.com/D7697
Summary: This prevents issues when the user hasn't provided the appropriate attributes for a preallocated host.
Test Plan: Attempted to lease against a resource with omitted attributes, got an exception thrown before any SSH commands occurred.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7695
Summary: Fixes T4198. We don't currently show "(Maniphest) > T123 > Edit" on the edit screen, which is inconsistent. Add the "T123" crumb.
Test Plan: {F87177}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T4198
Differential Revision: https://secure.phabricator.com/D7699
Summary: The link pointed to `create/`, which gives as `404`.
Test Plan: clicked the link. It worked.
Reviewers: epriestley, #blessed_reviewers, chad
Reviewed By: chad
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7698
Summary: Touch up /notifications/ for desktop and mobile
Test Plan: Tested read and unread notifications on mobile and desktop
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7671
Summary:
Ref T4189. This doesn't add any rules yet, but does all the heavy lifting to figure out what's changed and put it in a consuamble (if somewhat ad-hoc) datastructure, which lists all the ref and tag modifications and all the new commits in a consistent way.
From here, it should be fairly straightforward to add top-level rules (e.g., ff pushes only).
Test Plan: Output is huge, see comments.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4189
Differential Revision: https://secure.phabricator.com/D7687
Summary: This adds the ability to float action buttons inside ObjectHeaderView.
Test Plan: Tested a UI Example on desktop and mobile. Will test on Notifications next.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7684
Summary: There's no guarantee that the local path has a trailing "/". We
should probably guarantee that at some point, but just add one
unconditionally for now.
Auditors: btrahan
Summary:
//(this diff used to be about applying policies to blueprints)//
This restructures Drydock so that blueprints are instances in the DB, with an associated implementation class. Thus resources now have a `blueprintPHID` instead of `blueprintClass` and DrydockBlueprint becomes a DAO. The old DrydockBlueprint is renamed to DrydockBlueprintImplementation, and the DrydockBlueprint DAO has a `blueprintClass` column on it.
This now just implements CAN_VIEW and CAN_EDIT policies for blueprints, although they are probably not enforced in all of the places they could be.
Test Plan: Used the `create-resource` and `lease` commands. Closed resources and leases in the UI. Clicked around the new and old lists to make sure everything is still working.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4111, T2015
Differential Revision: https://secure.phabricator.com/D7638
Summary: Ref T4189. Fixes T2066. Mercurial has a //lot// of hooks so I'm not 100% sure this is all we need to install (we may need separate hooks for tags/bookmarks) but it should cover most of what we're after at least.
Test Plan:
- `bin/repository pull`'d a Mercurial repo and got a hook install.
- Pushed to a Mercurial repository over SSH and HTTP, with good/bad hooks. Saw hooks fire.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2066, T4189
Differential Revision: https://secure.phabricator.com/D7685
Summary:
Ref T4189. This adds SVN support, which was a little more messy than I though. Principally, we can not use `PHABRICATOR_USER` for Subversion, because it strips away the entire environment for "security reasons".
Instead, use `--tunnel-user` plus `svnlook author` to figure out the author.
Also fix "ssh://" clone URIs, which needs to be "svn+ssh://".
Test Plan:
- Made SVN commits through the hook.
- Made Git commits, too, to make sure I didn't break anything.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4189
Differential Revision: https://secure.phabricator.com/D7683
Summary:
Ref T4189. T4189 describes most of the intent here:
- When updating hosted repositories, sync a pre-commit hook into them instead of doing a `git fetch`.
- The hook calls into Phabricator. The acting Phabricator user is sent via PHABRICATOR_USER in the environment. The active repository is sent via CLI.
- The hook doesn't do anything useful yet; it just veifies basic parameters, does a little parsing, and exits 0 to allow the commit.
Test Plan:
- Performed Git pushes and pulls over SSH and HTTP.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4189
Differential Revision: https://secure.phabricator.com/D7682
Summary: DrydockResource has been updated to be policy-aware (although there are no policy columns).
Test Plan: Clicked around in Drydock, viewed resources and leases, everything still seemed to work.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3605, T4111
Differential Revision: https://secure.phabricator.com/D7595
Summary: If there is no /query in the URL, the default query would be lost when clicking Next, causing the search form to be shown on the second page. This is not so likely to happen on a standard Phabricator installation because the default query is Assigned, and few people will have 100+ tasks assigned.
Test Plan:
* Go to /maniphest/query/edit/
* Move Open Tasks to the top
* Go to /maniphest/
* Click Next on the bottom right
* See only tasks that are actually open
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7667
Test Plan: Go to a project page, make browser window narrow, click to expand action list.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7666
Summary: `LANG=C` is smooshing UTF-8 in some cases. See IRC.
Test Plan: User confirmed this works.
Reviewers: btrahan, asherkin
Reviewed By: asherkin
CC: aran
Differential Revision: https://secure.phabricator.com/D7659
Summary:
By default in search application document status field is "Open and Closed Documents".
Often searching with this default status I get confused that open and closed items in
search result are not distinguished.
Test Plan: Search and see open/closed issues distinguished.
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: epriestley
CC: epriestley, aran, Korvin
Differential Revision: https://secure.phabricator.com/D7626
Summary: Request from Asana. Adds an option for adding tasks to projects.
Test Plan: Used `bin/feed republish` to create and update Asana tasks with projects configured. Saw them end up in the right projects.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7655
Summary: See D7653. This is exclusively for Asana, who uses Differential for a post-commit, Audit-like workflow but has a small set of requirements for it to be a good fit (just this) and a large set of requirements for Diffusion/Audit to be a good fit.
Test Plan: Set the flag, verified "Accepted" revisions are no longer on the dashboard.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7654
Summary:
Currently, "Closed" and "Abandoned" are treated as "closed". I want to add a flag which treats "Accepted" as "Closed", too, for Asana and other companies who use an Asana-like workflow.
The background here is that their workflow is a bit weird. They basically do audits, but have a lot of things which Diffusion doesn't do well right now. This one change makes Differential fit their workflow fairly well, even though it's an audit workflow.
To prepare for this, normalize the definition of "closed" better. We have a few callsites which explicitly check for "ABANDONED || CLOSED", and normalizing this is cleaner anyway.
Also delete the very old COMMITTED status, which has been obsolete for over a year.
Test Plan: Browsed around most/all of the affected interfaces.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7653
Summary: Fixes T4148. LDAPS works with "ldaps://", it just isn't documented or clear.
Test Plan: {F84893}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4148
Differential Revision: https://secure.phabricator.com/D7652
Summary:
Fixes T4158. Two possible refinements:
- Maybe we should make all of these things respect `ManiphestCapabilityEditAssign::CAPABILITY`, etc. I think it's reasonable either way, and this is probably more intuitive and useful for most cases.
- Maybe we should check that you can see the policies before copying them. Again, this is sort of reasonable either way.
Test Plan: Created a new task from a template, saw that it inherited policies.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T4158
Differential Revision: https://secure.phabricator.com/D7649
Summary: Fixes T3687. Instead of rendering "JIRA Issues" in Differential using plain links, render them using Doorkeeper tags so they get the nice "enhance with object name" effect.
Test Plan: {F84886}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3687
Differential Revision: https://secure.phabricator.com/D7648
Summary: Touched up the layout, css of this page
Test Plan: Viewed linked and linkable accounts. Tested mobile layout
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7644
Summary: Fixes T4155. See discussion in T4155.
Test Plan: @mbishopim3 confirmed this fixes his issue.
Reviewers: btrahan, chad
Reviewed By: chad
CC: mbishopim3, aran
Maniphest Tasks: T4155
Differential Revision: https://secure.phabricator.com/D7646
Summary: This was broken in rP51fb1ca16d7f.
Test Plan: Imported a repository with file:/// location, it worked.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7636
Summary: Fixes T2230. This isn't a total walk in the park to configure, but should work for early adopters now.
Test Plan: Read documentation, browsed UI.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7634
Summary:
Ref T4038. This adds everything except the actual pushing part for mirrors.
This isn't the most beautiful or sophisticated UI, but I want get the authoritative repositories self-hosted and get users beta-ing hosting as soon as possible. We can do transactions, etc., later on.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4038
Differential Revision: https://secure.phabricator.com/D7632
Summary: Fixes T4122. Ref T2230. Instead of storing credentials on each repository, store them in Passphrase. This allows easy creation/management of many repositories which share credentials.
Test Plan:
- Upgraded repositories.
- Created and edited repositories.
- Pulled HTTP and SSH repositories.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2230, T4122
Differential Revision: https://secure.phabricator.com/D7629
Summary: Ref T4122. Add an edge to keep track of where a credential is used, and show it in the UI.
Test Plan:
See "Used By":
{F84099}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4122
Differential Revision: https://secure.phabricator.com/D7628
Summary: Ref T4122. These classes provide typed, checked access to credentials, so you can say "give me this password, and throw if anything is funky".
Test Plan: Used in next revision.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4122
Differential Revision: https://secure.phabricator.com/D7625
Summary:
Ref T4122.
- For Diffusion, we need "allow null" (permits selection of "No Credential") for anonymous HTTP repositories.
- For Diffusion, we can make things a little easier to configure by prefilling the username.
Test Plan: Used UIExample form. These featuers are used in a future revision.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4122
Differential Revision: https://secure.phabricator.com/D7624
Summary:
This adds a Drydock blueprint for preallocated, remote hosts. This will be used by the Harbormaster interface to allow users to specify remote hosts that builds can be run on.
This adds a `canAllocateResource` method to Drydock blueprints; it is used to detect whether a blueprint can allocate a resource for the given type and attributes.
Test Plan:
Ran:
```
bin/drydock lease --type host --attributes remote=true,preallocated=true,host=192.168.56.101,port=22,user=james,keyfile=,path=C:\\Build\\,platform=windows
```
and saw the "C:\Build\<id>" folder appear on the remote Windows machine. Viewed the lease and resource in Drydock as well.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran, jamesr
Maniphest Tasks: T4111
Differential Revision: https://secure.phabricator.com/D7593
Summary: Show SSH user on git-over-ssh repositories and hide both username and password for other repos.
Test Plan: View repository details page in diffusion, Clone URI should appear with a username (taken from repo config) and any http(s) repos should be without usernames.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4147
Differential Revision: https://secure.phabricator.com/D7631
Summary: Simplifies the code a bit and fixes all the wonky previews. Fixes T4053
Test Plan: Test all pages, logged in and logged out.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4053
Differential Revision: https://secure.phabricator.com/D7622
Summary: D7590 made path construction more consistent, but affected this callsite if a subpath is configured. Currently, we end up with double `@@` in the URI.
Test Plan:
- Ran unit tests.
- Ran `bin/repostitory discover`.
Reviewers: staticshock, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7619
Summary:
Ref T4140. We could hit a redirect loop for a user with a verified primary email address but no "is verified" flag on their account. This shouldn't be possible since the migration should have set the flag, but we can deal with it more gracefully when it does happen (maybe because users forgot to run `storage/upgrade`, or because of ghosts).
In the controller, check the same flag we check before forcing the user to the controller.
When verifying, allow the verification if either the email or user flag isn't set.
Test Plan: Hit `/login/mustverify/`; verified an address.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4140
Differential Revision: https://secure.phabricator.com/D7621
Summary: Ref T4140. Allow unapproved users to verify their email addresses. Currently, unapproved blocks email verification, but should not.
Test Plan: Clicked email verification link as an unapproved user, got email verified.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T4140
Differential Revision: https://secure.phabricator.com/D7618
Summary: ...and get the basic edit flow "working" for a new NuanceSourceDefinition - the Phabricator Form. ...and fix a dumb bug in the query class so when you redirect to the view page / try to edit an existing NuanceSource you don't fatal.
Test Plan: played around with the edit form and it worked!
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7585
Summary:
Fixes T4041. We currently detect when "origin" is incorrect, but can do better:
- When "origin" is missing, we can add it. This happens for Git 1.7.1 -- see T4041.
- When "origin" is wrong, we can fix it automatically if we control the repository.
We only need to fail when origin exists, is wrong, and we aren't in charge of the repository.
Test Plan: Ran `bin/repository discover X` on a repository with a good origin, no origin, a bad-but-under-control origin, and a bad-out-of-control origin. Got the right behavior in all cases.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, champo
Maniphest Tasks: T4041
Differential Revision: https://secure.phabricator.com/D7614
Summary:
This cleans up some garbage:
- We were specifying environmental variables with `X=y git ...`, but now have `setEnv()` on both `ExecFuture` and `PhutilExecPassthru`. Use `setEnv()`.
- We were specifying the working directory with `(cd %s && git ...)`, but now have `setCWD()` on both `ExecFuture` and `PhutilExecPassthru`. Use `setCWD()`.
- We were specifying the Git credentials with `ssh-agent -c (ssh-add ... && git ...)`. We can do this more cleanly with `GIT_SSH`. Use `GIT_SSH`.
- Since we have to write a script for `GIT_SSH` anyway, use the same script for Subversion and Mercurial.
This fixes two specific issues:
- Previously, we were not able to set `-o StrictHostKeyChecking=no` on Git commands, so the first time you cloned a git repo the daemons would generally prompt you to add `github.com` or whatever to `known_hosts`. Since this was non-interactive, things would mysteriously hang, in effect. With `GIT_SSH`, we can specify the flag, reducing the number of ways things can go wrong.
- This adds `LANG=C`, which probably (?) forces the language to English for all commands. Apparently you need to install special language packs or something, so I don't know that this actually works, but at least two users with non-English languages have claimed it does (see <https://github.com/facebook/arcanist/pull/114> for a similar issue in Arcanist).
At some point in the future I might want to combine the Arcanist code for command execution with the Phabricator code for command execution (they share some stuff like LANG and HGPLAIN). However, credential management is kind of messy, so I'm adopting a "wait and see" approach for now. I expect to split this at least somewhat in the future, for Drydock/Automerge if nothing else.
Also I'm not sure if we use the passthru stuff at all anymore, I may just be able to delete that. I'll check in a future diff.
Test Plan: Browsed and pulled Git, Subversion and Mercurial repositories.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7600
Summary:
Ref T2230. SVN has some weird rules about path construction. Particularly, if you're missing a "/" in the remote URI right now, the change parsing step doesn't build the right paths.
Instead, build the right paths more intelligently.
Test Plan: Added and executed unit tests. Imported an SVN repo.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, jpeffer
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7590
Summary:
Fixes T4132. If you run "bin/auth recover" before setting the base URI, it throws when trying to generate a production URI.
Instead, just show the path. We can't figure out the domain, and I think this is less confusing than showing "your.phabricator.example.com", etc.
Test Plan: Ran `bin/auth recover <user>` for valid and missing base-uri.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4132
Differential Revision: https://secure.phabricator.com/D7615
Summary:
Ref T4122. Implements a credential management application for the uses described in T4122.
@chad, this needs an icon, HA HA HAHA HA BWW HA HA HA
bwahaha
Test Plan: See screenshots.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T4122
Differential Revision: https://secure.phabricator.com/D7608
Summary:
We've been having trouble with viewing diffs timing out when there's a lot of unit test failures. It was caused by formatting userdata for every single failure. The expensive part of this was actually creating the engine for every result, so moved the construction outside of the loop.
Diffs that timed out (2 min) loading before load in around 6 seconds now.
Test Plan: Loaded diffs that used to time out. Verified that details still looked right when Show Full Unit Test Results Is Clicked.
Reviewers: epriestley, keegancsmith, lifeihuang, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran, andrewjcg
Differential Revision: https://secure.phabricator.com/D7581
Summary:
Ref T4110. This denormalized field used to power "Group By: Assigned" got dropped in the T2217 migration at some point.
Restore its population, and fix all the data in the database.
Test Plan: Ran migration, verified database came out reasonable-looking. Reassigned a task, verified database. Ran a "Group By: assigned" query.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4110
Differential Revision: https://secure.phabricator.com/D7602
Summary:
Fixes T4109. If a revision has a bad `repositoryPHID` (for example, because the repository was deleted), `DifferentialRevisionQuery` calls `didRejectResult()` on it, which raises a policy exception, even if the viewer is omnipotent. This aborts the `MessageParser`, because it does not expect policy exceptions to be raised for an omnipotent viewer.
Fix this in two ways:
# Never raise a policy exception for an omnipotent viewer. I think this is the expected behavior and a reasonable rule.
# In this case, load the revision for an omnipotent viewer.
This feels a little gross, but it's the only place where we do this in the codebase right now. We can clean this up later on once it's more clear what the circumstances of checks like these are.
Test Plan: Set a revision to have an invalid `repositoryPHID`, ran message parser on it, got a clean parse.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4109
Differential Revision: https://secure.phabricator.com/D7603
Summary:
Ref T2230. When fully set up, we have up to three users who all need to write into the repositories:
- The webserver needs to write for HTTP receives.
- The SSH user needs to write for SSH receives.
- The daemons need to write for "git fetch", "git clone", etc.
These three users don't need to be different, but in practice they are often not likely to all be the same user. If for no other reason, making them all the same user requires you to "git clone httpd@host.com", and installs are likely to prefer "git clone git@host.com".
Using three different users also allows better privilege separation. Particularly, the daemon user can be the //only// user with write access to the repositories. The webserver and SSH user can accomplish their writes through `sudo`, with a whitelisted set of commands. This means that even if you compromise the `ssh` user, you need to find a way to escallate from there to the daemon user in order to, e.g., write arbitrary stuff into the repository or bypass commit hooks.
This lays some of the groundwork for a highly-separated configuration where the SSH and HTTP users have the fewest privileges possible and use `sudo` to interact with repositories. Some future work which might make sense:
- Make `bin/phd` respect this (require start as the right user, or as root and drop privileges, if this configuration is set).
- Execute all `git/hg/svn` commands via sudo?
Users aren't expected to configure this yet so I haven't written any documentation.
Test Plan:
Added an SSH user ("dweller") and gave it sudo by adding this to `/etc/sudoers`:
dweller ALL=(epriestley) SETENV: NOPASSWD: /usr/bin/git-upload-pack, /usr/bin/git-receive-pack
Then I ran git pushes and pulls over SSH via "dweller@localhost". They successfully interacted with the repository on disk as the "epriestley" user.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7589
Summary: Although I don't want to end up with 20 of these again, this is a reasonable default to provide, particularly for installs where a large portion of the userbase primarily reports bugs and does not interact with them directly.
Test Plan: Hit `/maniphest/`, saw "Subscribed", clicked it, saw the tasks I'm subscribed to.
Reviewers: jbrown, btrahan
Reviewed By: jbrown
CC: aran
Maniphest Tasks: T4100
Differential Revision: https://secure.phabricator.com/D7586
Summary:
A usable, Land to GitHub flow.
Still to do:
- Refactor all git/hg stratagies to a sane structure.
- Make the dialogs Workflow + explain why it's disabled.
- Show button and request Link Account if GH is enabled, but user is not linked.
- After refreshing token, user ends up in the settings stage.
Hacked something in LandController to be able to show an arbitrary dialog from a strategy.
It's not very nice, but I want to make some more refactoring to the controller/strategy/ies anyway.
Also made PhabricatorRepository::getRemoteURIObject() public, because it was very useful in getting
the domain and path for the repo.
Test Plan:
Went through these flows:
- load revision in hosted, github-backed, non-github backed repos to see button as needed.
- hit land with weak token - sent to refresh it with the extra scope.
- Land to repo I'm not allowed - got proper error message.
- Successfully landed; Failed to apply patch.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T182
Differential Revision: https://secure.phabricator.com/D7555
Summary:
Ref T4039. This fixes an issue where a user with the ability to create repositories could view repositories he is otherwise not permitted to see, by following these steps:
- Suppose you want to see repository "A".
- Create a repository with the same VCS, called "B".
- Edit the local path, changing "/var/repo/B" to "/var/repo/A".
- Now it points at a working copy of a repository you can't see.
- Although you won't be able to make it through discovery (the pull will fail with the wrong credentials), you can read some information out of the repository directly through the Diffusion UI, probably?
I'm not sure this was really practical to execute since there are a bunch of sanity checks along most/all of the major pathways, but lock it down since normal users shouldn't be editing it anyway. In the best case, this would make a mess.
Test Plan: {F81391}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4039
Differential Revision: https://secure.phabricator.com/D7580
Summary:
Ref T4039. This is mostly to deal with that, to prevent the security issues associated with mutable local paths. The next diff will lock them in the web UI.
I also added a confirmation prompt to `bin/repository delete`, which was a little scary without one.
See one comment inline about the `--as` flag. I don't love this, but when I started adding all the stuff we'd need to let this transaction show up as "Administrator" it quickly got pretty big.
Test Plan: Ran `bin/repository edit ...`, saw an edit with a transaction show up on the web UI.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4039
Differential Revision: https://secure.phabricator.com/D7579
Summary:
Fixes T4095. Fixes T3817.
- The batch editor has some funky handle code which misses projects, share that.
- Remove some hacks for T3817 that should be good now.
Test Plan: Looked at batch editor, saw projects. Looked at task list.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, martin.schulz
Maniphest Tasks: T3817, T4095
Differential Revision: https://secure.phabricator.com/D7578
Summary:
Fixes T3741. The flag is respected in terms of actually creating the account, but the UI is a bit unclear.
This can never occur naturally, but installs can register an event which locks it.
Test Plan:
Artificially locked it, verified I got more reasonable UI;
{F81282}
Reviewers: btrahan, datr
Reviewed By: datr
CC: aran
Maniphest Tasks: T3741
Differential Revision: https://secure.phabricator.com/D7577
Summary:
- Add an option for the queue.
- By default, enable it.
- Dump new users into the queue.
- Send admins an email to approve them.
Test Plan:
- Registered new accounts with queue on and off.
- As an admin, approved accounts and disabled the queue from email.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7576
Summary:
- If you're an administrator and there are users waiting for approval, show a count on the home page.
- Sort out the `isUserActivated()` access check.
- Hide all the menu widgets except "Logout" for disabled and unapproved users.
- Add a "Log In" item.
- Add a bunch of unit tests.
Test Plan: Ran unit tests, clicked around as unapproved/approved/logged-in/logged-out users.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, chad
Differential Revision: https://secure.phabricator.com/D7574
Summary:
Nothing fancy here, just:
- UI to show users needing approval.
- "Approve" and "Disable" actions.
- Send "Approved" email on approve.
- "Approve" edit + log operations.
- "Wait for Approval" state for users who need approval.
There's still no natural way for users to end up not-approved -- you have to write directly to the database.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7573
Summary:
Mailbox sometimes (?) changes the case of the email address (?). Be more liberal in what we accept.
Also fix a minor output bug.
Test Plan: Sent mail to `e1+...` instead of `E1+...`, verified it arrived.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7575
Summary:
Small step forward which improves existing stuff or lays groudwork for future stuff:
- Currently, to check for email verification, we have to single-query the email address on every page. Instead, denoramlize it into the user object.
- Migrate all the existing users.
- When the user verifies an email, mark them as `isEmailVerified` if the email is their primary email.
- Just make the checks look at the `isEmailVerified` field.
- Add a new check, `isUserActivated()`, to cover email-verified plus disabled. Currently, a non-verified-but-not-disabled user could theoretically use Conduit over SSH, if anyone deployed it. Tighten that up.
- Add an `isApproved` flag, which is always true for now. In a future diff, I want to add a default-on admin approval queue for new accounts, to prevent configuration mistakes. The way it will work is:
- When the queue is enabled, registering users are created with `isApproved = false`.
- Admins are sent an email, "[Phabricator] New User Approval (alincoln)", telling them that a new user is waiting for approval.
- They go to the web UI and approve the user.
- Manually-created accounts are auto-approved.
- The email will have instructions for disabling the queue.
I think this queue will be helpful for new installs and give them peace of mind, and when you go to disable it we have a better opportunity to warn you about exactly what that means.
Generally, I want to improve the default safety of registration, since if you just blindly coast through the path of least resistance right now your install ends up pretty open, and realistically few installs are on VPNs.
Test Plan:
- Ran migration, verified `isEmailVerified` populated correctly.
- Created a new user, checked DB for verified (not verified).
- Verified, checked DB (now verified).
- Used Conduit, People, Diffusion.
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran
Differential Revision: https://secure.phabricator.com/D7572
Summary:
Ref T3472. Currently, if an install only allows "@mycompany.com" emails and you try to register with an "@personal.com" account, we let you pick an "@mycompany.com" address instead. This is secure: you still have to verify the email. However, it defies user expectation -- it's somewhat confusing that we let you register. Instead, provide a hard roadblock.
(These accounts can still be linked, just not used for registration.)
Test Plan: See screenshot.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3472
Differential Revision: https://secure.phabricator.com/D7571
Summary: See private chatter. Make it explicitly clear when adding a provider that anyone who can browse to Phabricator can register.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7570
Summary: We don't actually support this yet, so hide the configuration.
Test Plan: Edited branches for an hg repo.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7563
Summary:
Ref T2230. As far as I can tell, getting SVN working over HTTP is incredibly complicated. It's all DAV-based and doesn't appear to have any kind of binary we can just execute and pass requests through to. Don't support it for now.
- Disable it in the UI.
- Make sure all the error messages are reasonable.
Test Plan: Tried to HTTP an SVN repo. Tried to clone a Git repo with SVN, got a good error message.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7562
Summary:
Fixes T3034. This is obsoleted by modern policies.
This was written by a Facebook intern and is rarely used -- the Hive install might be the only use in the wild. It has never really worked correctly.
Test Plan: `grep`; browsed Differential.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3034
Differential Revision: https://secure.phabricator.com/D7568
Summary: Fixes T3535. Also, flip flop on that spacing thing and make the spaces purdy
Test Plan: got an arcanist projected phid in the json dict
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3535
Differential Revision: https://secure.phabricator.com/D7565
Summary: adds FIELD_PROJECTS and deploys it to Maniphest Task Herald Adapter. Went with "projects" because it feels like that could go well in other Adapters that want to conditionalize based on project.
Test Plan: made a new herald rule to be cc'd if project foo was on a task. it worked!
Reviewers: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7564
Summary:
Ref T2230. Very rarely, even though we've flushed the connection and sent all the data, we'll close the connection before Git is happy with it and it will flip out with an error like this:
fatal: The remote end hung up unexpectedly
fatal: early EOF
fatal: index-pack failed
This is hard to reproduce because it depends on the order of read/write operations we can't directly control. I only saw it about 2% of the time, by just running `git pull` over and over again.
Waiting for Git to close its side of the connection seems to fix it.
Test Plan: Ran `git clone` a ton of times without seeing the error again. Ran `git push` a ton of times with new commits.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7558
Summary:
Ref T2230. The SVN protocol has a sensible protocol format with a good spec here:
http://svn.apache.org/repos/asf/subversion/trunk/subversion/libsvn_ra_svn/protocol
Particularly, compare this statement to the clown show that is the Mercurial wire protocol:
> It is possible to parse an item without knowing its type in advance.
WHAT A REASONABLE STATEMENT TO BE ABLE TO MAKE ABOUT A WIRE PROTOCOL
Although it makes substantially more sense than Mercurial, it's much heavier-weight than the Git or Mercurial protocols, since it isn't distributed.
It's also not possible to figure out if a request is a write request (or even which repository it is against) without proxying some of the protocol frames. Finally, several protocol commands embed repository URLs, and we need to reach into the protocol and translate them.
Test Plan: Ran various SVN commands over SSH (`svn log`, `svn up`, `svn commit`, etc).
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7556
Summary:
Ref T2230. This is substantially more complicated than Git, but mostly because Mercurial's protocol is a like 50 ad-hoc extensions cobbled together. Because we must decode protocol frames in order to determine if a request is read or write, 90% of this is implementing a stream parser for the protocol.
Mercurial's own parser is simpler, but relies on blocking reads. Since we don't even have methods for blocking reads right now and keeping the whole thing non-blocking is conceptually better, I made the parser nonblocking. It ends up being a lot of stuff. I made an effort to cover it reasonably well with unit tests, and to make sure we fail closed (i.e., reject requests) if there are any parts of the protocol I got wrong.
A lot of the complexity is sharable with the HTTP stuff, so it ends up being not-so-bad, just very hard to verify by inspection as clearly correct.
Test Plan:
- Ran `hg clone` over SSH.
- Ran `hg fetch` over SSH.
- Ran `hg push` over SSH, to a read-only repo (error) and a read-write repo (success).
Reviewers: btrahan, asherkin
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7553
Summary: Ref T2230. Fixes T4079. As it turns out, this is Git being weird. See comments for some detials about what's going on here.
Test Plan: Created shallow and deep Git clones.
Reviewers: hach-que, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4079, T2230
Differential Revision: https://secure.phabricator.com/D7554
Summary:
Ref T2230. In Git, we can determine if a command is read-only or read/write from the command itself, but this isn't the case in Mercurial or SVN.
For Mercurial and SVN, we need to proxy the protocol that's coming over the wire, look at each request from the client, and then check if it's a read or a write. To support this, provide a more flexible version of `passthruIO`.
The way this will work is:
- The SSH IO channel is wrapped in a `ProtocolChannel` which can parse the the incoming stream into message objects.
- The `willWriteCallback` will look at those messages and determine if they're reads or writes.
- If they're writes, it will check for write permission.
- If we're good to go, the message object is converted back into a byte stream and handed to the underlying command.
Test Plan: Executed `git clone`, `git clone --depth 3`, `git push` (against no-write repo, got error), `git push` (against valid repo).
Reviewers: btrahan
Reviewed By: btrahan
CC: hach-que, asherkin, aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7551
Summary: Missing some `break;`, pretty sure this is causing the issue on `secure.phabricator.com`.
Test Plan: Will push.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7559
Summary: This CAN_EDIT capability doesn't exist. `PhabricatorMacroCapabilityManage::CAPABILITY` (checked on line 15) is used instead.
Test Plan: Disabled, then re-enabled a macro.
Reviewers: hach-que, btrahan
Reviewed By: hach-que
CC: aran
Differential Revision: https://secure.phabricator.com/D7550
Summary:
Now that diffs have PHIDs we can create buildables for them.
This also adds `buildable.diff` in the variables list so the diff ID is available, and it also fixes the Cancel button on "Edit Plan" page so it redirects to the right place.
Test Plan: Created a buildable from a diff, ran a build plan against it that had `echo ${buildable.diff}` and got the right ID. Also tested the "Edit Plan" cancel redirect.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7546
