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
Summary:
This uses an event listener to render the status of builds on their buildables. The revision and commit view now renders out the status of each of the builds.
Currently the revision controller has the results for the latest diff rendered out. We might want to show the status of previous diffs in the future, but for now I think the latest diff should do fine.
There's also a number of bug fixes in this diff, including a particularly nasty one where builds would have a build plan PHID generated for them, which resulted in handle lookups always returning invalid objects.
Test Plan: Ran builds against diffs and commits, saw them appear on the revision and commit view controllers.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7544
Summary:
Ref T1049. This is very minimal, but does what it says.
I merged the variable replacement code so Remote + HTTP can share more stuff.
Test Plan:
Ran "HTTP" and "Remote" build plans.
{F79886}
{F79887}
Reviewers: hach-que, btrahan
Reviewed By: hach-que
CC: zeeg, aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7541
Summary: This prevents a crash in applying build plans when more than one buildable exists for the same object. It also adds a check into the "New Manual Build" page to ensure that users can't create a buildable for an object that already has one.
Test Plan: Tried to create a buildable for an object that already has one and a nice friendly error appeared. Applied a build plan to a buildable whose object has two buildables and didn't get a crash any more.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7543
Summary: This adds a `build.id` variable, cleans up the naming convention of other variables and also fixes an issue in the remote command to read the buffers after the command finishes.
Test Plan: Ran a build with `/bin/echo ${build.id}` and saw the build ID come through.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7540
Summary: This puts back the stronger variable replacement that was missed the last update to D7519.
Test Plan: Re-ran a remote build that had variables in the command and everything worked as expected.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7539
Summary: This fixes an issue where content would be discarded when the content to append is larger than the chunk size limit.
Test Plan: Tested running a remote command that does `I=0; while true; do echo "$I"; I=$[$I+1]; done` and all of the outputted numbers matched the line numbers in the logs.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7537
Summary: This adds a build step implementation for running a command on a remote machine over SSH. It supports merging in various variables about the build (such as the commit hash / revision ID, repository call sign, version control type and clone URI).
Test Plan: Configured a build plan to run `/bin/true` on localhost and the build passed. Configured a build plan to run `/bin/false` on localhost and the build failed.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7519
Summary:
Depends on D7519.
This implements support for build logs in Harbormaster. This includes support for appending to a log from the "Run Remote Command" build step.
It also adds the ability to cancel builds.
Currently the build view page doesn't update the logs live; I'm sure this can be achieved with Javelin, but I don't have enough experience with Javelin to actually make it poll from updates to content in the background.
{F79151}
{F79153}
{F79150}
{F79152}
Test Plan:
Tested this by setting up SSH on a Windows machine and using a Remote Command configured with:
```
C:\Windows\system32\cmd.exe /C cd C:\Build && mkdir Build_${timestamp} && cd Build_${timestamp} && git clone --recursive https://github.com/hach-que/Tychaia.git && cd Tychaia && Protobuild.exe && C:\Windows\Microsoft.NET\Framework\v4.0.30319\MSBuild.exe Tychaia.Windows.sln
```
and observed the output of the build stream from the Windows machine into Phabricator.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7521
Summary: these transactions should //never// merge since they are always created for a 1:1 replacement. (ie any merging would be implicitly erroneous). Fixes T4081
Test Plan: made a mock with three images and replaced all three successfully. replaced image A with image B, did not save, replaced image B with image C, then saved and verified transaction correctly showed image A replaced with image C.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4081
Differential Revision: https://secure.phabricator.com/D7536
Summary:
Depends on D7500.
This seemed like a pretty good idea once I thought of it. Instead of having some custom triggering logic instead Harbormaster, I figured it best to leverage all of Herald's power so that users can create rules to apply builds to commits and differential revisions. This gives the added advantage that they can trigger off builds for particular types of revisions and commits, which seems like it could be really useful (e.g. run extra tests against revisions that touch sensitive areas of the code).
Test Plan: Ran the usual daemons + the Harbormaster daemon. Pushed a commit to the repository and saw both the buildable and build get created when the commit worked picked it up. Submitted a diff and saw both the buildable and build get created when the Herald rules were evaluated for the diff.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran, hwinkel
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7501
Summary: another little piece here that basically just adds some permissions to source editing. serving it up before I do anything too complicated to make sure it seems kosher. in terms of what comes next this form needs to be dynamic based on source type so there'll be some fun there. That said, I plan to implement a more simple "phabricator form" only version to start here and flesh out a few other things like queues with that.
Test Plan: set permission to no one for source edit and got a nice error page.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7535
Summary: I've kept this as close as possible to the Git version for ease of review and later refactoring of them both together. At minimum, the functions to get the working dir should probably be cleaned up one day.
Test Plan: Landed a revision.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7534
Summary:
See <https://github.com/facebook/phabricator/issues/433>. We were missing a "^" here.
This should be moved over to transactions soon and then we can get rid of the duplication. :/
Test Plan: Tried to create a repository with callsign "9X", got a helpful error about "ALL UPPERCASE LETTERS".
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7531
Summary:
After upgrading to PHP 5.5, the conduit list was not fully visible because
INF was being treated as "0" for some reason. Fixed by making it a PHP_MAX_INT
Test Plan: Checked on PHP 5.5
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7530
Summary:
Fixes T4067. The way `DiffusionCommitQuery` works prevents it from loading SVN identifiers in some cases without additional constraints, since "12345" might be an SVN revision 12345, or it might be the first 5 characters of a Git commit hash.
Introduce `withRepository()` as a shorthand for `withDefaultRepository()` + `withRepositoryIDs()`. This tells the query to:
- Only look in the given repository; and
- use the more liberal identifier resolution rules while doing so.
The practical impact this has is that blame tooltips in SVN work again. The other queries which are fixed here were never run in SVN (which doesn't have first-class branches or tags); I've cleaned them up only for completeness.
Test Plan:
- Viewed blame in SVN, saw information again instead of empty tooltip.
- Viewed brnaches/tags in Mercurial and Git.
{F79226}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4067
Differential Revision: https://secure.phabricator.com/D7523
Summary:
we were just checking if projects/ was in the URI before barfing. Use some more fun utility functions such that we only complain if there is no project.
Fixes T4071.
Test Plan: made a subpage under a project - success! tried to make a project wiki page where there was no project - successful failure! tried to make a project wiki sub page where there was no project - successful failure!
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4071
Differential Revision: https://secure.phabricator.com/D7527
Summary:
See discussion in <https://github.com/facebook/phabricator/issues/430>.
(If we end up with more than like 5 of these we should probably make this a warning or something instead, the only goal is to prevent user error.)
Test Plan:
{F79196}
{F79197}
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran, enko
Differential Revision: https://secure.phabricator.com/D7522
Summary: The "Reviewers" condition in Differential Revision rules has the wrong typeahead and can't select projects, but should be able to.
Test Plan: {F79273}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7526
Summary: Ref T2230. This is easily the worst thing I've had to write in a while. I'll leave some notes inline.
Test Plan: Ran `hg clone http://...` on a hosted repo. Ran `hg push` on the same. Changed sync'd both ways.
Reviewers: asherkin, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7520
Summary: This is starting to get a bit sizable and it turns out Mercurial is sort of a beast, so split the VCS serve stuff into a separate controller.
Test Plan: Pushed and pulled an authenticated Git repository.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, hach-que
Differential Revision: https://secure.phabricator.com/D7494
Summary: Ref T4068. Partly, this moves discovery to the more unit-testable PhabricatorRepositoryDiscoveryEngine. It also fixes some issues, see inlines.
Test Plan: In a Mercurial repository, ran `bin/repository discover --repair`, verified commits came out topographically sorted. Ran without `--repair` and in various other contexts, like with no commits to discover and some-but-not-all commits to discover.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4068
Differential Revision: https://secure.phabricator.com/D7518
Summary:
I updated the wiki too - https://secure.phabricator.com/w/projects/pebkac/ - with what I am thinking right now. Rough plan here is
- next diff:
- implement editors and transactions
- implement "web type" for contact source
- /pebkac/item/new/ will be the entry point for this
- implement "actions" on a contact
- probably some "polish" on the scaffolding laid out here; like "create" permissions maybs
- diffs after that:
- implement "twitter" type for source
- implement email reply handler stuff for item and source
Probs a great time to blast huge holes in all this stuff. :D
Test Plan: these pages load and arc lint doesn't complain
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran, chad
Differential Revision: https://secure.phabricator.com/D7465
