From 83f1140785ab19f33635a9df419174254d28dee7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 1 Sep 2011 09:35:35 -0700 Subject: [PATCH 01/17] Use text, not icons, to indicate content sources Summary: oh god everyone hates this revert revert https://www.facebook.com/photo.php?fbid=787360256660&set=p.787360256660&type=1&theater (I left the icons themselves since I have some plans to do other things with them.) Test Plan: I am not good at designer Reviewers: ola, elynde, bh, ashwin, jungejason, kdelong, zrait, tomo, aran Reviewed By: aran CC: aran, epriestley, tomo Differential Revision: 885 --- src/__celerity_resource_map__.php | 2 +- .../view/PhabricatorContentSourceView.php | 17 ++++++------- .../contentsource/content-source-view.css | 25 ------------------- 3 files changed, 8 insertions(+), 36 deletions(-) diff --git a/src/__celerity_resource_map__.php b/src/__celerity_resource_map__.php index 7ecff1e3cd..c61d7fab4b 100644 --- a/src/__celerity_resource_map__.php +++ b/src/__celerity_resource_map__.php @@ -1180,7 +1180,7 @@ celerity_register_resource_map(array( ), 'phabricator-content-source-view-css' => array( - 'uri' => '/res/7147f14c/rsrc/css/application/contentsource/content-source-view.css', + 'uri' => '/res/361cae20/rsrc/css/application/contentsource/content-source-view.css', 'type' => 'css', 'requires' => array( diff --git a/src/applications/metamta/contentsource/view/PhabricatorContentSourceView.php b/src/applications/metamta/contentsource/view/PhabricatorContentSourceView.php index 02b83c7d68..5018b27eb8 100644 --- a/src/applications/metamta/contentsource/view/PhabricatorContentSourceView.php +++ b/src/applications/metamta/contentsource/view/PhabricatorContentSourceView.php @@ -35,13 +35,12 @@ final class PhabricatorContentSourceView extends AphrontView { public function render() { require_celerity_resource('phabricator-content-source-view-css'); - $type = null; $map = array( - PhabricatorContentSource::SOURCE_WEB => 'web', - PhabricatorContentSource::SOURCE_CONDUIT => 'conduit', - PhabricatorContentSource::SOURCE_EMAIL => 'email', - PhabricatorContentSource::SOURCE_MOBILE => 'mobile', - PhabricatorContentSource::SOURCE_TABLET => 'tablet', + PhabricatorContentSource::SOURCE_WEB => 'Web', + PhabricatorContentSource::SOURCE_CONDUIT => 'Conduit', + PhabricatorContentSource::SOURCE_EMAIL => 'Email', + PhabricatorContentSource::SOURCE_MOBILE => 'Mobile', + PhabricatorContentSource::SOURCE_TABLET => 'Tablet', ); $source = $this->contentSource->getSource(); @@ -51,14 +50,12 @@ final class PhabricatorContentSourceView extends AphrontView { return; } - $type_class = 'phabricator-content-source-'.$type; - return phutil_render_tag( 'span', array( - 'class' => "phabricator-content-source-view {$type_class}", + 'class' => "phabricator-content-source-view", ), - 'Via'); + "Via {$type}"); } } diff --git a/webroot/rsrc/css/application/contentsource/content-source-view.css b/webroot/rsrc/css/application/contentsource/content-source-view.css index 9ab0da3c07..09ee5aac3d 100644 --- a/webroot/rsrc/css/application/contentsource/content-source-view.css +++ b/webroot/rsrc/css/application/contentsource/content-source-view.css @@ -3,30 +3,5 @@ */ .phabricator-content-source-view { - padding: 2px 20px 2px 0; - font-size: 11px; color: #888888; - font-weight: normal; - background: no-repeat right center; - position: relative; -} - -.phabricator-content-source-web { - background-image: url(/rsrc/image/icon/fatcow/source/web.png); -} - -.phabricator-content-source-email { - background-image: url(/rsrc/image/icon/fatcow/source/email.png); -} - -.phabricator-content-source-conduit { - background-image: url(/rsrc/image/icon/fatcow/source/conduit.png); -} - -.phabricator-content-source-mobile { - background-image: url(/rsrc/image/icon/fatcow/source/mobile.png); -} - -.phabricator-content-source-tablet { - background-image: url(/rsrc/image/icon/fatcow/source/tablet.png); } From c2fef51b3da6bc10547ce4b79b6f7fc045045358 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 1 Sep 2011 09:29:33 -0700 Subject: [PATCH 02/17] Refine error messages for CSRF exceptions Summary: See T489. Provide slightly more detail so we can figure out if there's a real issue here. Test Plan: Hit URIs like: /differential/comment/preview/29/ /differential/comment/preview/29/?__ajax__=1 /differential/comment/preview/29/?__csrf__=1 ..and got appropriate error messages. Reviewers: jungejason Reviewed By: jungejason CC: aran, jungejason Differential Revision: 884 --- src/aphront/request/AphrontRequest.php | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/src/aphront/request/AphrontRequest.php b/src/aphront/request/AphrontRequest.php index d00efc6b60..e2a6313839 100644 --- a/src/aphront/request/AphrontRequest.php +++ b/src/aphront/request/AphrontRequest.php @@ -145,6 +145,22 @@ class AphrontRequest { $valid = $this->getUser()->validateCSRFToken($token); if (!$valid) { + + // Add some diagnostic details so we can figure out if some CSRF issues + // are JS problems or people accessing Ajax URIs directly with their + // browsers. + if ($token) { + $token_info = "with an invalid CSRF token"; + } else { + $token_info = "without a CSRF token"; + } + + if ($this->isAjax()) { + $more_info = "(This was an Ajax request, {$token_info}.)"; + } else { + $more_info = "(This was a web request, {$token_info}.)"; + } + // This should only be able to happen if you load a form, pull your // internet for 6 hours, and then reconnect and immediately submit, // but give the user some indication of what happened since the workflow @@ -155,7 +171,8 @@ class AphrontRequest { "certain type of login hijacking attack. However, the token can ". "become invalid if you leave a page open for more than six hours ". "without a connection to the internet. To fix this problem: reload ". - "the page, and then resubmit it."); + "the page, and then resubmit it.\n\n". + $more_info); } return true; From f4c8525a9a926ff26c4b07fcaa037d3b6baba6cb Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 1 Sep 2011 12:22:43 -0700 Subject: [PATCH 03/17] Add "Fax" content source Summary: Can't believe I missed this. Test Plan: !!! Reviewers: isaac, ola, g, jungejason Reviewed By: ola CC: aran, ola Differential Revision: 886 --- .../source/PhabricatorContentSource.php | 1 + .../view/PhabricatorContentSourceView.php | 1 + webroot/rsrc/image/icon/fatcow/README | 3 ++- webroot/rsrc/image/icon/fatcow/source/fax.png | Bin 0 -> 600 bytes 4 files changed, 4 insertions(+), 1 deletion(-) create mode 100644 webroot/rsrc/image/icon/fatcow/source/fax.png diff --git a/src/applications/metamta/contentsource/source/PhabricatorContentSource.php b/src/applications/metamta/contentsource/source/PhabricatorContentSource.php index e058dfdb1d..c190df5e8e 100644 --- a/src/applications/metamta/contentsource/source/PhabricatorContentSource.php +++ b/src/applications/metamta/contentsource/source/PhabricatorContentSource.php @@ -24,6 +24,7 @@ final class PhabricatorContentSource { const SOURCE_CONDUIT = 'conduit'; const SOURCE_MOBILE = 'mobile'; const SOURCE_TABLET = 'tablet'; + const SOURCE_FAX = 'fax'; private $source; private $params = array(); diff --git a/src/applications/metamta/contentsource/view/PhabricatorContentSourceView.php b/src/applications/metamta/contentsource/view/PhabricatorContentSourceView.php index 5018b27eb8..5ad1f0d297 100644 --- a/src/applications/metamta/contentsource/view/PhabricatorContentSourceView.php +++ b/src/applications/metamta/contentsource/view/PhabricatorContentSourceView.php @@ -41,6 +41,7 @@ final class PhabricatorContentSourceView extends AphrontView { PhabricatorContentSource::SOURCE_EMAIL => 'Email', PhabricatorContentSource::SOURCE_MOBILE => 'Mobile', PhabricatorContentSource::SOURCE_TABLET => 'Tablet', + PhabricatorContentSource::SOURCE_FAX => 'Fax', ); $source = $this->contentSource->getSource(); diff --git a/webroot/rsrc/image/icon/fatcow/README b/webroot/rsrc/image/icon/fatcow/README index 5d3ea5ed31..c294834741 100644 --- a/webroot/rsrc/image/icon/fatcow/README +++ b/webroot/rsrc/image/icon/fatcow/README @@ -13,4 +13,5 @@ Some icons have been adapted from the FatCow set for use in Phabricator: source/email.png (from email.png) source/conduit.png (from satellite_dish.png) source/mobile.png (from phone.png) - source/tablet.png (from tablet.png) \ No newline at end of file + source/tablet.png (from tablet.png) + source/fax.png (from fax.png) \ No newline at end of file diff --git a/webroot/rsrc/image/icon/fatcow/source/fax.png b/webroot/rsrc/image/icon/fatcow/source/fax.png new file mode 100644 index 0000000000000000000000000000000000000000..399c895aaac65dcdc42c64e2437af9c2a9b22d0c GIT binary patch literal 600 zcmV-e0;m0nP)6WanULmLH zTyP8kbl`wC!2bT12Qs6UN?}5$XE5mF@Jto`9}|eBN%FL+=gPQRp1QTk6pVoi6fAi* zj(`7QsB+U;G)?V)%`CLmPL0fD1uAeS%pES)YN%Pm_hk-XZ*95-rx}Cq-{xafO66UK z!%^Z#BlLPb)a!Nh_V#d%{O8?Ww3^NVUMhR9M2{=m_f@_c0u^-@wnK z3GS}nLs5n4xa}MeIt3yJ6bX3w>lTL39-+Llf|sve<5K?}#?K#9GvzKCE09d$EffmU zc?O3ePeh=!`P6kfPx%`p--t~*XsR+A1-b|PpNOJp;(6ZLq*!Dx6zt;QMRv_|k Date: Fri, 2 Sep 2011 13:06:23 -0700 Subject: [PATCH 04/17] Tweak width of differential-panel to match aphront-panels on differential Summary: The differential panels at the top of the differential revision view page were 2px smaller than the divs on the bottom of the page (everything below the table of contents). This diff makes differential-panel 2px wider so it matches. Test Plan: viewed a differential revision and checked that the divs lined up Reviewers: epriestley Reviewed By: epriestley CC: aran, epriestley Differential Revision: 887 --- src/__celerity_resource_map__.php | 44 +++++++++---------- .../css/application/differential/core.css | 2 +- 2 files changed, 22 insertions(+), 24 deletions(-) diff --git a/src/__celerity_resource_map__.php b/src/__celerity_resource_map__.php index c61d7fab4b..21038f4976 100644 --- a/src/__celerity_resource_map__.php +++ b/src/__celerity_resource_map__.php @@ -172,7 +172,7 @@ celerity_register_resource_map(array( ), 'differential-core-view-css' => array( - 'uri' => '/res/438fe316/rsrc/css/application/differential/core.css', + 'uri' => '/res/584d40e8/rsrc/css/application/differential/core.css', 'type' => 'css', 'requires' => array( @@ -652,17 +652,6 @@ celerity_register_resource_map(array( ), 'disk' => '/rsrc/js/application/maniphest/behavior-transaction-preview.js', ), - 0 => - array( - 'uri' => '/res/1da00bfe/rsrc/js/javelin/lib/__tests__/URI.js', - 'type' => 'js', - 'requires' => - array( - 0 => 'javelin-uri', - 1 => 'javelin-php-serializer', - ), - 'disk' => '/rsrc/js/javelin/lib/__tests__/URI.js', - ), 'javelin-behavior-owners-path-editor' => array( 'uri' => '/res/9cf78ffc/rsrc/js/application/owners/owners-path-editor.js', @@ -1180,7 +1169,7 @@ celerity_register_resource_map(array( ), 'phabricator-content-source-view-css' => array( - 'uri' => '/res/361cae20/rsrc/css/application/contentsource/content-source-view.css', + 'uri' => '/res/8c738a93/rsrc/css/application/contentsource/content-source-view.css', 'type' => 'css', 'requires' => array( @@ -1294,6 +1283,15 @@ celerity_register_resource_map(array( ), 'disk' => '/rsrc/js/application/core/Prefab.js', ), + 0 => + array( + 'uri' => '/res/936e8e81/rsrc/js/javelin/docs/onload.js', + 'type' => 'js', + 'requires' => + array( + ), + 'disk' => '/rsrc/js/javelin/docs/onload.js', + ), 'phabricator-profile-css' => array( 'uri' => '/res/ebe1ac2f/rsrc/css/application/profile/profile-view.css', @@ -1419,7 +1417,7 @@ celerity_register_resource_map(array( 'uri' => '/res/pkg/3dbf4083/javelin.pkg.js', 'type' => 'js', ), - '3f2092d7' => + '7bf96a66' => array( 'name' => 'differential.pkg.css', 'symbols' => @@ -1433,7 +1431,7 @@ celerity_register_resource_map(array( 6 => 'differential-revision-add-comment-css', 7 => 'differential-revision-comment-list-css', ), - 'uri' => '/res/pkg/3f2092d7/differential.pkg.css', + 'uri' => '/res/pkg/7bf96a66/differential.pkg.css', 'type' => 'css', ), '95c67dcd' => @@ -1518,14 +1516,14 @@ celerity_register_resource_map(array( 'aphront-table-view-css' => '70966590', 'aphront-tokenizer-control-css' => '70966590', 'aphront-typeahead-control-css' => '70966590', - 'differential-changeset-view-css' => '3f2092d7', - 'differential-core-view-css' => '3f2092d7', - 'differential-revision-add-comment-css' => '3f2092d7', - 'differential-revision-comment-css' => '3f2092d7', - 'differential-revision-comment-list-css' => '3f2092d7', - 'differential-revision-detail-css' => '3f2092d7', - 'differential-revision-history-css' => '3f2092d7', - 'differential-table-of-contents-css' => '3f2092d7', + 'differential-changeset-view-css' => '7bf96a66', + 'differential-core-view-css' => '7bf96a66', + 'differential-revision-add-comment-css' => '7bf96a66', + 'differential-revision-comment-css' => '7bf96a66', + 'differential-revision-comment-list-css' => '7bf96a66', + 'differential-revision-detail-css' => '7bf96a66', + 'differential-revision-history-css' => '7bf96a66', + 'differential-table-of-contents-css' => '7bf96a66', 'diffusion-commit-view-css' => '03ef179e', 'javelin-behavior' => '3dbf4083', 'javelin-behavior-aphront-basic-tokenizer' => 'ac869011', diff --git a/webroot/rsrc/css/application/differential/core.css b/webroot/rsrc/css/application/differential/core.css index 643991eaa9..12c8b24a79 100644 --- a/webroot/rsrc/css/application/differential/core.css +++ b/webroot/rsrc/css/application/differential/core.css @@ -15,7 +15,7 @@ .differential-panel { margin: 25px 0; - max-width: 1118px; + max-width: 1120px; border: 1px solid #666622; background: #efefdf; padding: 15px 20px; From ed508247ba1ae8d4512f718ba35dd1a06223680a Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 4 Sep 2011 10:11:12 -0700 Subject: [PATCH 05/17] Fix a bug in the SVN parser which causes it to find commit refs in other SVN repositories Summary: This query isn't scoped correctly to the repository ID, so we may identify commits from other repositories. This causes a somewhat subtle issue since we only use it to manage file copies/moves, so you end up with a file "copied from" the same revision in another repository. I think the UI probably even renders correctly. Once I finish T325 and better understand what's going on here, I'll see how much work is involved in writing an SQL patch to fix this. Test Plan: Parsed the test repo from T325 with the expected error. Reviewers: jungejason, nh, tuomaspelkonen, aran Reviewed By: jungejason CC: aran, jungejason Differential Revision: 891 --- .../svn/PhabricatorRepositorySvnCommitChangeParserWorker.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/applications/repository/worker/commitchangeparser/svn/PhabricatorRepositorySvnCommitChangeParserWorker.php b/src/applications/repository/worker/commitchangeparser/svn/PhabricatorRepositorySvnCommitChangeParserWorker.php index f365552475..bb92e0e9c0 100644 --- a/src/applications/repository/worker/commitchangeparser/svn/PhabricatorRepositorySvnCommitChangeParserWorker.php +++ b/src/applications/repository/worker/commitchangeparser/svn/PhabricatorRepositorySvnCommitChangeParserWorker.php @@ -496,8 +496,10 @@ class PhabricatorRepositorySvnCommitChangeParserWorker $commit_table = new PhabricatorRepositoryCommit(); $commit_data = queryfx_all( $commit_table->establishConnection('w'), - 'SELECT id, commitIdentifier FROM %T WHERE commitIdentifier in (%Ld)', + 'SELECT id, commitIdentifier FROM %T + WHERE repositoryID = %d AND commitIdentifier in (%Ld)', $commit_table->getTableName(), + $repository->getID(), $commits); return ipull($commit_data, 'id', 'commitIdentifier'); From ae045a9cf2668094bda56ae8e7b1afd981257ee8 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 4 Sep 2011 10:34:53 -0700 Subject: [PATCH 06/17] When doing partial subdirectory parses in Subversion, stub out foreign commit references Summary: See T325. We tentatively support doing partial subdirectory parses in Phabricator for Subversion, so you can elect to import only "trunk/local/" or similar. We do this by importing only some of the commits (those commits which affected that directory). In Subversion, you can also "svn cp svn+ssh://example.com/svnroot/trunk/foreign/example.c@13 local.c". This means that commits which reference "trunk/local/" may themselves reference foreign commits. Currently, we break in this case and can't find the commit reference. Instead, generate a foreign commit stub so we can at least point at some reasonable object. Test Plan: Successfully imported trunk/a/ of the test repo in T325 without errors. Verified commit 3 in that repo is imported as a foreign stub. Reviewers: jungejason, nh, tuomaspelkonen, aran Reviewed By: jungejason CC: aran, jungejason, epriestley Differential Revision: 892 --- ...rRepositorySvnCommitChangeParserWorker.php | 48 ++++++++++++++++++- .../commitchangeparser/svn/__init__.php | 1 + 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/src/applications/repository/worker/commitchangeparser/svn/PhabricatorRepositorySvnCommitChangeParserWorker.php b/src/applications/repository/worker/commitchangeparser/svn/PhabricatorRepositorySvnCommitChangeParserWorker.php index bb92e0e9c0..0965aaef9d 100644 --- a/src/applications/repository/worker/commitchangeparser/svn/PhabricatorRepositorySvnCommitChangeParserWorker.php +++ b/src/applications/repository/worker/commitchangeparser/svn/PhabricatorRepositorySvnCommitChangeParserWorker.php @@ -502,7 +502,53 @@ class PhabricatorRepositorySvnCommitChangeParserWorker $repository->getID(), $commits); - return ipull($commit_data, 'id', 'commitIdentifier'); + $commit_map = ipull($commit_data, 'id', 'commitIdentifier'); + + $need = array(); + foreach ($commits as $commit) { + if (empty($commit_map[$commit])) { + $need[] = $commit; + } + } + + // If we are parsing a Subversion repository and have been configured to + // import only some subdirectory of it, we may find commits which reference + // other foreign commits outside of the directory (for instance, because of + // a move or copy). Rather than trying to execute full parses on them, just + // create stub commits and identify the stubs as foreign commits. + if ($need) { + $subpath = $repository->getDetail('svn-subpath'); + if (!$subpath) { + $commits = implode(', ', $need); + throw new Exception( + "Missing commits ({$need}) in a SVN repository which is not ". + "configured for subdirectory-only parsing!"); + } + foreach ($need as $foreign_commit) { + $commit = new PhabricatorRepositoryCommit(); + $commit->setRepositoryID($repository->getID()); + $commit->setCommitIdentifier($foreign_commit); + $commit->setEpoch(0); + $commit->save(); + + $data = new PhabricatorRepositoryCommitData(); + $data->setCommitID($commit->getID()); + $data->setAuthorName(''); + $data->setCommitMessage(''); + $data->setCommitDetails( + array( + 'foreign-svn-stub' => true, + // Denormalize this to make it easier to debug cases where someone + // did half a parse and then changed the subdirectory or something + // like that. + 'svn-subpath' => $subpath, + )); + $data->save(); + $commit_map[$foreign_commit] = $commit->getID(); + } + } + + return $commit_map; } private function lookupPathFileType( diff --git a/src/applications/repository/worker/commitchangeparser/svn/__init__.php b/src/applications/repository/worker/commitchangeparser/svn/__init__.php index 27bd1250dc..88eb272294 100644 --- a/src/applications/repository/worker/commitchangeparser/svn/__init__.php +++ b/src/applications/repository/worker/commitchangeparser/svn/__init__.php @@ -8,6 +8,7 @@ phutil_require_module('phabricator', 'applications/differential/constants/changetype'); phutil_require_module('phabricator', 'applications/repository/storage/commit'); +phutil_require_module('phabricator', 'applications/repository/storage/commitdata'); phutil_require_module('phabricator', 'applications/repository/storage/repository'); phutil_require_module('phabricator', 'applications/repository/worker/commitchangeparser/base'); phutil_require_module('phabricator', 'storage/qsprintf'); From 2db912e859e86036221805ca08337ef857bbf896 Mon Sep 17 00:00:00 2001 From: Nick Harper Date: Sun, 4 Sep 2011 01:53:58 -0700 Subject: [PATCH 07/17] Add change password settings panel Summary: In password-based auth environments, there is now a user settings panel to allow them to change their password. Test Plan: Click settings, choose password from the left: * enter current password, new password (twice), log out, and log in with new password * enter current password, non-matching passwords, and get error * enter invalid old password, and get error * use firebug to change csrf token and verify that it does not save with and invalid token Changed config to disable password auth, loaded settings panel and saw that password was no longer visible. Tried loading the panel anyway and got redirected. Reviewers: epriestley Reviewed By: epriestley CC: aran, epriestley Differential Revision: 890 --- src/__phutil_library_map__.php | 2 + .../PhabricatorUserSettingsController.php | 12 +- .../people/controller/settings/__init__.php | 2 + ...torUserPasswordSettingsPanelController.php | 111 ++++++++++++++++++ .../settings/panels/password/__init__.php | 24 ++++ 5 files changed, 149 insertions(+), 2 deletions(-) create mode 100644 src/applications/people/controller/settings/panels/password/PhabricatorUserPasswordSettingsPanelController.php create mode 100644 src/applications/people/controller/settings/panels/password/__init__.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index b487f2a6aa..b78249d51f 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -642,6 +642,7 @@ phutil_register_library_map(array( 'PhabricatorUserLog' => 'applications/people/storage/log', 'PhabricatorUserOAuthInfo' => 'applications/people/storage/useroauthinfo', 'PhabricatorUserOAuthSettingsPanelController' => 'applications/people/controller/settings/panels/oauth', + 'PhabricatorUserPasswordSettingsPanelController' => 'applications/people/controller/settings/panels/password', 'PhabricatorUserPreferenceSettingsPanelController' => 'applications/people/controller/settings/panels/preferences', 'PhabricatorUserPreferences' => 'applications/people/storage/preferences', 'PhabricatorUserProfile' => 'applications/people/storage/profile', @@ -1222,6 +1223,7 @@ phutil_register_library_map(array( 'PhabricatorUserLog' => 'PhabricatorUserDAO', 'PhabricatorUserOAuthInfo' => 'PhabricatorUserDAO', 'PhabricatorUserOAuthSettingsPanelController' => 'PhabricatorUserSettingsPanelController', + 'PhabricatorUserPasswordSettingsPanelController' => 'PhabricatorUserSettingsPanelController', 'PhabricatorUserPreferenceSettingsPanelController' => 'PhabricatorUserSettingsPanelController', 'PhabricatorUserPreferences' => 'PhabricatorUserDAO', 'PhabricatorUserProfile' => 'PhabricatorUserDAO', diff --git a/src/applications/people/controller/settings/PhabricatorUserSettingsController.php b/src/applications/people/controller/settings/PhabricatorUserSettingsController.php index acc107b519..120e2d3f2e 100644 --- a/src/applications/people/controller/settings/PhabricatorUserSettingsController.php +++ b/src/applications/people/controller/settings/PhabricatorUserSettingsController.php @@ -29,16 +29,20 @@ class PhabricatorUserSettingsController extends PhabricatorPeopleController { $request = $this->getRequest(); - // TODO: Implement a password panel. - $this->pages = array( 'account' => 'Account', 'profile' => 'Profile', 'email' => 'Email', + 'password' => 'Password', 'preferences' => 'Preferences', 'conduit' => 'Conduit Certificate', ); + if (!PhabricatorEnv::getEnvConfig('account.editable') || + !PhabricatorEnv::getEnvConfig('auth.password-auth-enabled')) { + unset($this->pages['password']); + } + if (PhabricatorUserSSHKeysSettingsPanelController::isEnabled()) { $this->pages['sshkeys'] = 'SSH Public Keys'; } @@ -67,6 +71,10 @@ class PhabricatorUserSettingsController extends PhabricatorPeopleController { case 'email': $delegate = new PhabricatorUserEmailSettingsPanelController($request); break; + case 'password': + $delegate = new PhabricatorUserPasswordSettingsPanelController( + $request); + break; case 'conduit': $delegate = new PhabricatorUserConduitSettingsPanelController($request); break; diff --git a/src/applications/people/controller/settings/__init__.php b/src/applications/people/controller/settings/__init__.php index ec95b76f5a..08c21ebe1e 100644 --- a/src/applications/people/controller/settings/__init__.php +++ b/src/applications/people/controller/settings/__init__.php @@ -13,9 +13,11 @@ phutil_require_module('phabricator', 'applications/people/controller/settings/pa phutil_require_module('phabricator', 'applications/people/controller/settings/panels/conduit'); phutil_require_module('phabricator', 'applications/people/controller/settings/panels/email'); phutil_require_module('phabricator', 'applications/people/controller/settings/panels/oauth'); +phutil_require_module('phabricator', 'applications/people/controller/settings/panels/password'); phutil_require_module('phabricator', 'applications/people/controller/settings/panels/preferences'); phutil_require_module('phabricator', 'applications/people/controller/settings/panels/profile'); phutil_require_module('phabricator', 'applications/people/controller/settings/panels/sshkeys'); +phutil_require_module('phabricator', 'infrastructure/env'); phutil_require_module('phabricator', 'view/layout/sidenav'); phutil_require_module('phutil', 'markup'); diff --git a/src/applications/people/controller/settings/panels/password/PhabricatorUserPasswordSettingsPanelController.php b/src/applications/people/controller/settings/panels/password/PhabricatorUserPasswordSettingsPanelController.php new file mode 100644 index 0000000000..06dae9d2ff --- /dev/null +++ b/src/applications/people/controller/settings/panels/password/PhabricatorUserPasswordSettingsPanelController.php @@ -0,0 +1,111 @@ +getRequest(); + $user = $request->getUser(); + $editable = $this->getAccountEditable(); + + // There's no sense in showing a change password panel if the user + // can't change their password + if (!$editable || + !PhabricatorEnv::getEnvConfig('auth.password-auth-enabled')) { + return new Aphront400Response(); + } + + $errors = array(); + if ($request->isFormPost()) { + if ($user->comparePassword($request->getStr('old_pw'))) { + $pass = $request->getStr('new_pw'); + $conf = $request->getStr('conf_pw'); + if ($pass === $conf) { + if (strlen($pass)) { + $user->setPassword($pass); + // This write is unguarded because the CSRF token has already + // been checked in the call to $request->isFormPost() and + // the CSRF token depends on the password hash, so when it + // is changed here the CSRF token check will fail. + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + $user->save(); + unset($unguarded); + return id(new AphrontRedirectResponse()) + ->setURI('/settings/page/password/?saved=true'); + } else { + $errors[] = 'Your new password is too short.'; + } + } else { + $errors[] = 'New password and confirmation do not match.'; + } + } else { + $errors[] = 'The old password you entered is incorrect.'; + } + } + + $notice = null; + if (!$errors) { + if ($request->getStr('saved')) { + $notice = new AphrontErrorView(); + $notice->setSeverity(AphrontErrorView::SEVERITY_NOTICE); + $notice->setTitle('Changes Saved'); + $notice->appendChild('

Your password has been updated.

'); + } + } else { + $notice = new AphrontErrorView(); + $notice->setTitle('Error Changing Password'); + $notice->setErrors($errors); + } + + $form = new AphrontFormView(); + $form + ->setUser($user) + ->appendChild( + id(new AphrontFormPasswordControl()) + ->setLabel('Old Password') + ->setName('old_pw')); + $form + ->appendChild( + id(new AphrontFormPasswordControl()) + ->setLabel('New Password') + ->setName('new_pw')); + $form + ->appendChild( + id(new AphrontFormPasswordControl()) + ->setLabel('Confirm Password') + ->setName('conf_pw')); + $form + ->appendChild( + id(new AphrontFormSubmitControl()) + ->setValue('Save')); + + $panel = new AphrontPanelView(); + $panel->setHeader('Change Password'); + $panel->setWidth(AphrontPanelView::WIDTH_FORM); + $panel->appendChild($form); + + return id(new AphrontNullView()) + ->appendChild( + array( + $notice, + $panel, + )); + } +} diff --git a/src/applications/people/controller/settings/panels/password/__init__.php b/src/applications/people/controller/settings/panels/password/__init__.php new file mode 100644 index 0000000000..7a375a6fd8 --- /dev/null +++ b/src/applications/people/controller/settings/panels/password/__init__.php @@ -0,0 +1,24 @@ + Date: Sun, 4 Sep 2011 14:57:33 -0700 Subject: [PATCH 08/17] Don't flag "EXPLAIN" as a write Summary: These queries are safe to run without a CSRF token, and we need them for the query analyzer in DarkConsole. Test Plan: "Analyze Query Plans" works again. Reviewers: jungejason, nh, tuomaspelkonen, aran Reviewed By: nh CC: aran, epriestley, nh Differential Revision: 895 --- src/storage/connection/mysql/AphrontMySQLDatabaseConnection.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/storage/connection/mysql/AphrontMySQLDatabaseConnection.php b/src/storage/connection/mysql/AphrontMySQLDatabaseConnection.php index 4cce4ed84e..6a9b7639c9 100644 --- a/src/storage/connection/mysql/AphrontMySQLDatabaseConnection.php +++ b/src/storage/connection/mysql/AphrontMySQLDatabaseConnection.php @@ -209,7 +209,7 @@ class AphrontMySQLDatabaseConnection extends AphrontDatabaseConnection { $this->requireConnection(); // TODO: Do we need to include transactional statements here? - $is_write = !preg_match('/^(SELECT|SHOW)\s/', $raw_query); + $is_write = !preg_match('/^(SELECT|SHOW|EXPLAIN)\s/', $raw_query); if ($is_write) { AphrontWriteGuard::willWrite(); } From 8f3b342287bccbac1ce5b65b66f23792a0d2251c Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 4 Sep 2011 14:39:52 -0700 Subject: [PATCH 09/17] Improve several Diffusion UI error states Summary: Give users better errors and UI: - For subpath SVN repositories, default the path to the subdirectory, not to "/". This makes the home screen useful and things generally less confusing. - For unparsed commits, show a more descriptive error message without the "blah blah" silliness. - For paths outside of the subpath parse tree, short circuit into an appropriate error message. - For foreign SVN stub commits (see D892), show an explicit message. Test Plan: Looked at unparsed commits, subpath repositories, foreign stub commits, and paths outside of the subpath parse tree. Received sensible error messages. Reviewers: jungejason, nh, tuomaspelkonen, aran Reviewed By: jungejason CC: aran, jungejason Differential Revision: 894 --- .../PhabricatorWorkerTaskDetailController.php | 42 ++++- .../controller/workertaskdetail/__init__.php | 4 + .../browse/DiffusionBrowseController.php | 10 ++ .../diffusion/controller/browse/__init__.php | 1 + .../commit/DiffusionCommitController.php | 167 ++++++++++-------- .../browse/base/DiffusionBrowseQuery.php | 11 +- .../browse/svn/DiffusionSvnBrowseQuery.php | 9 + .../request/svn/DiffusionSvnRequest.php | 8 + 8 files changed, 174 insertions(+), 78 deletions(-) diff --git a/src/applications/daemon/controller/workertaskdetail/PhabricatorWorkerTaskDetailController.php b/src/applications/daemon/controller/workertaskdetail/PhabricatorWorkerTaskDetailController.php index 03cbfdcadc..d4acf74840 100644 --- a/src/applications/daemon/controller/workertaskdetail/PhabricatorWorkerTaskDetailController.php +++ b/src/applications/daemon/controller/workertaskdetail/PhabricatorWorkerTaskDetailController.php @@ -46,6 +46,35 @@ class PhabricatorWorkerTaskDetailController $data = id(new PhabricatorWorkerTaskData())->loadOneWhere( 'id = %d', $task->getDataID()); + + $extra = null; + switch ($task->getTaskClass()) { + case 'PhabricatorRepositorySvnCommitChangeParserWorker': + case 'PhabricatorRepositoryGitCommitChangeParserWorker': + $commit_id = idx($data->getData(), 'commitID'); + if ($commit_id) { + $commit = id(new PhabricatorRepositoryCommit())->load($commit_id); + if ($commit) { + $repository = id(new PhabricatorRepository())->load( + $commit->getRepositoryID()); + if ($repository) { + $extra = + "NOTE: ". + "You can manually retry this task by running this script:". + "
".
+                  "phabricator/\$ ./scripts/repository/parse_one_commit.php ".
+                  "r".
+                  phutil_escape_html($repository->getCallsign()).
+                  phutil_escape_html($commit->getCommitIdentifier()).
+                "
"; + } + } + } + break; + default: + break; + } + if ($data) { $data = json_encode($data->getData()); } @@ -75,14 +104,23 @@ class PhabricatorWorkerTaskDetailController ->appendChild( id(new AphrontFormTextAreaControl()) ->setLabel('Data') - ->setValue($data)) + ->setValue($data)); + + if ($extra) { + $form->appendChild( + id(new AphrontFormMarkupControl()) + ->setLabel('More') + ->setValue($extra)); + } + + $form ->appendChild( id(new AphrontFormSubmitControl()) ->addCancelButton('/daemon/')); $panel = new AphrontPanelView(); $panel->setHeader('Task Detail'); - $panel->setWidth(AphrontPanelView::WIDTH_FORM); + $panel->setWidth(AphrontPanelView::WIDTH_WIDE); $panel->appendChild($form); return $this->buildStandardPageResponse( diff --git a/src/applications/daemon/controller/workertaskdetail/__init__.php b/src/applications/daemon/controller/workertaskdetail/__init__.php index 13e9017739..66839c4cf7 100644 --- a/src/applications/daemon/controller/workertaskdetail/__init__.php +++ b/src/applications/daemon/controller/workertaskdetail/__init__.php @@ -7,15 +7,19 @@ phutil_require_module('phabricator', 'applications/daemon/controller/base'); +phutil_require_module('phabricator', 'applications/repository/storage/commit'); +phutil_require_module('phabricator', 'applications/repository/storage/repository'); phutil_require_module('phabricator', 'infrastructure/daemon/workers/storage/task'); phutil_require_module('phabricator', 'infrastructure/daemon/workers/storage/taskdata'); phutil_require_module('phabricator', 'view/form/base'); +phutil_require_module('phabricator', 'view/form/control/markup'); phutil_require_module('phabricator', 'view/form/control/static'); phutil_require_module('phabricator', 'view/form/control/submit'); phutil_require_module('phabricator', 'view/form/control/textarea'); phutil_require_module('phabricator', 'view/form/error'); phutil_require_module('phabricator', 'view/layout/panel'); +phutil_require_module('phutil', 'markup'); phutil_require_module('phutil', 'utils'); diff --git a/src/applications/diffusion/controller/browse/DiffusionBrowseController.php b/src/applications/diffusion/controller/browse/DiffusionBrowseController.php index f69d18da9e..f67b9cfaf2 100644 --- a/src/applications/diffusion/controller/browse/DiffusionBrowseController.php +++ b/src/applications/diffusion/controller/browse/DiffusionBrowseController.php @@ -75,6 +75,16 @@ class DiffusionBrowseController extends DiffusionController { $controller->setDiffusionRequest($drequest); return $this->delegateToController($controller); break; + case DiffusionBrowseQuery::REASON_IS_UNTRACKED_PARENT: + $subdir = $drequest->getRepository()->getDetail('svn-subpath'); + $title = 'Directory Not Tracked'; + $body = + "This repository is configured to track only one subdirectory ". + "of the entire repository ('".phutil_escape_html($subdir)."'), ". + "but you aren't looking at something in that subdirectory, so no ". + "information is available."; + $severity = AphrontErrorView::SEVERITY_WARNING; + break; default: throw new Exception("Unknown failure reason!"); } diff --git a/src/applications/diffusion/controller/browse/__init__.php b/src/applications/diffusion/controller/browse/__init__.php index 20a7e93855..2c07414577 100644 --- a/src/applications/diffusion/controller/browse/__init__.php +++ b/src/applications/diffusion/controller/browse/__init__.php @@ -14,6 +14,7 @@ phutil_require_module('phabricator', 'applications/phid/handle/data'); phutil_require_module('phabricator', 'view/form/error'); phutil_require_module('phabricator', 'view/layout/panel'); +phutil_require_module('phutil', 'markup'); phutil_require_module('phutil', 'utils'); diff --git a/src/applications/diffusion/controller/commit/DiffusionCommitController.php b/src/applications/diffusion/controller/commit/DiffusionCommitController.php index 4b7e80178c..1d8566656d 100644 --- a/src/applications/diffusion/controller/commit/DiffusionCommitController.php +++ b/src/applications/diffusion/controller/commit/DiffusionCommitController.php @@ -44,31 +44,47 @@ class DiffusionCommitController extends DiffusionController { $commit_data = $drequest->loadCommitData(); - $engine = PhabricatorMarkupEngine::newDifferentialMarkupEngine(); + $is_foreign = $commit_data->getCommitDetail('foreign-svn-stub'); + if ($is_foreign) { + $subpath = $commit_data->getCommitDetail('svn-subpath'); - require_celerity_resource('diffusion-commit-view-css'); - require_celerity_resource('phabricator-remarkup-css'); + $error_panel = new AphrontErrorView(); + $error_panel->setWidth(AphrontErrorView::WIDTH_WIDE); + $error_panel->setTitle('Commit Not Tracked'); + $error_panel->setSeverity(AphrontErrorView::SEVERITY_WARNING); + $error_panel->appendChild( + "This Diffusion repository is configured to track only one ". + "subdirectory of the entire Subversion repository, and this commit ". + "didn't affect the tracked subdirectory ('". + phutil_escape_html($subpath)."'), so no information is available."); + $content[] = $error_panel; + } else { + $engine = PhabricatorMarkupEngine::newDifferentialMarkupEngine(); - $property_table = $this->renderPropertyTable($commit, $commit_data); + require_celerity_resource('diffusion-commit-view-css'); + require_celerity_resource('phabricator-remarkup-css'); - $detail_panel->appendChild( - '
'. - '
'. - 'r'.$callsign.$commit->getCommitIdentifier(). - ' · '. - phabricator_datetime($commit->getEpoch(), $user). - '
'. - '

Revision Detail

'. - '
'. - $property_table. - '
'. - '
'. - $engine->markupText($commit_data->getCommitMessage()). + $property_table = $this->renderPropertyTable($commit, $commit_data); + + $detail_panel->appendChild( + '
'. + '
'. + 'r'.$callsign.$commit->getCommitIdentifier(). + ' · '. + phabricator_datetime($commit->getEpoch(), $user). '
'. - '
'. - '
'); + '

Revision Detail

'. + '
'. + $property_table. + '
'. + '
'. + $engine->markupText($commit_data->getCommitMessage()). + '
'. + '
'. + '
'); - $content[] = $detail_panel; + $content[] = $detail_panel; + } $change_query = DiffusionPathChangeQuery::newFromDiffusionRequest( $drequest); @@ -103,6 +119,23 @@ class DiffusionCommitController extends DiffusionController { phutil_escape_html($bad_commit['description'])); $content[] = $error_panel; + } else if ($is_foreign) { + // Don't render anything else. + } else if (!count($changes)) { + $no_changes = new AphrontErrorView(); + $no_changes->setWidth(AphrontErrorView::WIDTH_WIDE); + $no_changes->setSeverity(AphrontErrorView::SEVERITY_WARNING); + $no_changes->setTitle('Not Yet Parsed'); + // TODO: This can also happen with weird SVN changes that don't do + // anything (or only alter properties?), although the real no-changes case + // is extremely rare and might be impossible to produce organically. We + // should probably write some kind of "Nothing Happened!" change into the + // DB once we parse these changes so we can distinguish between + // "not parsed yet" and "no changes". + $no_changes->appendChild( + "This commit hasn't been fully parsed yet (or doesn't affect any ". + "paths)."); + $content[] = $no_changes; } else { $change_panel = new AphrontPanelView(); $change_panel->setHeader("Changes (".number_format($count).")"); @@ -130,60 +163,52 @@ class DiffusionCommitController extends DiffusionController { $content[] = $change_panel; - if ($changes) { - $changesets = DiffusionPathChange::convertToDifferentialChangesets( - $changes); + $changesets = DiffusionPathChange::convertToDifferentialChangesets( + $changes); - $vcs = $repository->getVersionControlSystem(); - switch ($vcs) { - case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN: - $vcs_supports_directory_changes = true; - break; - case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT: - $vcs_supports_directory_changes = false; - break; - default: - throw new Exception("Unknown VCS."); - } - - $references = array(); - foreach ($changesets as $key => $changeset) { - $file_type = $changeset->getFileType(); - if ($file_type == DifferentialChangeType::FILE_DIRECTORY) { - if (!$vcs_supports_directory_changes) { - unset($changesets[$key]); - continue; - } - } - - $branch = $drequest->getBranchURIComponent( - $drequest->getBranch()); - $filename = $changeset->getFilename(); - $commit = $drequest->getCommit(); - $reference = "{$branch}{$filename};{$commit}"; - $references[$key] = $reference; - } - - $change_list = new DifferentialChangesetListView(); - $change_list->setChangesets($changesets); - $change_list->setRenderingReferences($references); - $change_list->setRenderURI('/diffusion/'.$callsign.'/diff/'); - - // TODO: This is pretty awkward, unify the CSS between Diffusion and - // Differential better. - require_celerity_resource('differential-core-view-css'); - $change_list = - '
'. - $change_list->render(). - '
'; - } else { - $change_list = - '
'. - '(no changes blah blah)'. - '
'; + $vcs = $repository->getVersionControlSystem(); + switch ($vcs) { + case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN: + $vcs_supports_directory_changes = true; + break; + case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT: + $vcs_supports_directory_changes = false; + break; + default: + throw new Exception("Unknown VCS."); } + $references = array(); + foreach ($changesets as $key => $changeset) { + $file_type = $changeset->getFileType(); + if ($file_type == DifferentialChangeType::FILE_DIRECTORY) { + if (!$vcs_supports_directory_changes) { + unset($changesets[$key]); + continue; + } + } + + $branch = $drequest->getBranchURIComponent( + $drequest->getBranch()); + $filename = $changeset->getFilename(); + $commit = $drequest->getCommit(); + $reference = "{$branch}{$filename};{$commit}"; + $references[$key] = $reference; + } + + $change_list = new DifferentialChangesetListView(); + $change_list->setChangesets($changesets); + $change_list->setRenderingReferences($references); + $change_list->setRenderURI('/diffusion/'.$callsign.'/diff/'); + + // TODO: This is pretty awkward, unify the CSS between Diffusion and + // Differential better. + require_celerity_resource('differential-core-view-css'); + $change_list = + '
'. + $change_list->render(). + '
'; + $content[] = $change_list; } diff --git a/src/applications/diffusion/query/browse/base/DiffusionBrowseQuery.php b/src/applications/diffusion/query/browse/base/DiffusionBrowseQuery.php index 02c3283a87..cba77ec2b5 100644 --- a/src/applications/diffusion/query/browse/base/DiffusionBrowseQuery.php +++ b/src/applications/diffusion/query/browse/base/DiffusionBrowseQuery.php @@ -25,11 +25,12 @@ abstract class DiffusionBrowseQuery { protected $deletedAtCommit; protected $validityOnly; - const REASON_IS_FILE = 'is-file'; - const REASON_IS_DELETED = 'is-deleted'; - const REASON_IS_NONEXISTENT = 'nonexistent'; - const REASON_BAD_COMMIT = 'bad-commit'; - const REASON_IS_EMPTY = 'empty'; + const REASON_IS_FILE = 'is-file'; + const REASON_IS_DELETED = 'is-deleted'; + const REASON_IS_NONEXISTENT = 'nonexistent'; + const REASON_BAD_COMMIT = 'bad-commit'; + const REASON_IS_EMPTY = 'empty'; + const REASON_IS_UNTRACKED_PARENT = 'untracked-parent'; final private function __construct() { // diff --git a/src/applications/diffusion/query/browse/svn/DiffusionSvnBrowseQuery.php b/src/applications/diffusion/query/browse/svn/DiffusionSvnBrowseQuery.php index c47952118a..b513b50763 100644 --- a/src/applications/diffusion/query/browse/svn/DiffusionSvnBrowseQuery.php +++ b/src/applications/diffusion/query/browse/svn/DiffusionSvnBrowseQuery.php @@ -25,6 +25,15 @@ final class DiffusionSvnBrowseQuery extends DiffusionBrowseQuery { $path = $drequest->getPath(); $commit = $drequest->getCommit(); + $subpath = $repository->getDetail('svn-subpath'); + if ($subpath && strncmp($subpath, $path, strlen($subpath))) { + // If we have a subpath and the path isn't a child of it, it (almost + // certainly) won't exist since we don't track commits which affect + // it. (Even if it exists, return a consistent result.) + $this->reason = self::REASON_IS_UNTRACKED_PARENT; + return array(); + } + $conn_r = $repository->establishConnection('r'); $parent_path = dirname($path); diff --git a/src/applications/diffusion/request/svn/DiffusionSvnRequest.php b/src/applications/diffusion/request/svn/DiffusionSvnRequest.php index 577e36c64e..81c9c68bd1 100644 --- a/src/applications/diffusion/request/svn/DiffusionSvnRequest.php +++ b/src/applications/diffusion/request/svn/DiffusionSvnRequest.php @@ -20,6 +20,14 @@ class DiffusionSvnRequest extends DiffusionRequest { protected function initializeFromAphrontRequestDictionary(array $data) { parent::initializeFromAphrontRequestDictionary($data); + + if ($this->path === null) { + $subpath = $this->repository->getDetail('svn-subpath'); + if ($subpath) { + $this->path = $subpath; + } + } + if (!strncmp($this->path, ':', 1)) { $this->path = substr($this->path, 1); $this->path = ltrim($this->path, '/'); From e875c81f6d2afd2d88485036794f0c92fd36530f Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 18 Aug 2011 12:45:59 -0700 Subject: [PATCH 10/17] Remove blameRevision and revertPlan from the DifferentialRevision schema Summary: These fields use auxiliary storage now. Migrate the data and get rid of the columns in the main table. - This might take a little while to run, although there are <500k rows so probably not too long. - Maybe grab a backup of the table first, if I screwed something up this will delete the data in these fields. Test Plan: - Ran migration locally. - Browsed Differential. - Grepped for "revertPlan" and "blameRevision". Reviewers: jungejason, tuomaspelkonen, aran Reviewed By: jungejason CC: aran, jungejason, epriestley Differential Revision: 832 --- resources/sql/patches/072.blamerevert.sql | 20 +++++++++++++++++++ ...uitAPI_differential_getrevision_Method.php | 2 -- .../revision/DifferentialRevisionEditor.php | 6 ------ .../storage/revision/DifferentialRevision.php | 2 -- 4 files changed, 20 insertions(+), 10 deletions(-) create mode 100644 resources/sql/patches/072.blamerevert.sql diff --git a/resources/sql/patches/072.blamerevert.sql b/resources/sql/patches/072.blamerevert.sql new file mode 100644 index 0000000000..76b4c087e6 --- /dev/null +++ b/resources/sql/patches/072.blamerevert.sql @@ -0,0 +1,20 @@ +INSERT INTO phabricator_differential.differential_auxiliaryfield + (revisionPHID, name, value, dateCreated, dateModified) +SELECT phid, 'phabricator:blame-revision', blameRevision, + dateCreated, dateModified + FROM phabricator_differential.differential_revision + WHERE blameRevision != ''; + +ALTER TABLE phabricator_differential.differential_revision + DROP blameRevision; + + +INSERT INTO phabricator_differential.differential_auxiliaryfield + (revisionPHID, name, value, dateCreated, dateModified) +SELECT phid, 'phabricator:revert-plan', revertPlan, + dateCreated, dateModified + FROM phabricator_differential.differential_revision + WHERE revertPlan != ''; + +ALTER TABLE phabricator_differential.differential_revision + DROP revertPlan; diff --git a/src/applications/conduit/method/differential/getrevision/ConduitAPI_differential_getrevision_Method.php b/src/applications/conduit/method/differential/getrevision/ConduitAPI_differential_getrevision_Method.php index de5b3bef06..30ccd480ae 100644 --- a/src/applications/conduit/method/differential/getrevision/ConduitAPI_differential_getrevision_Method.php +++ b/src/applications/conduit/method/differential/getrevision/ConduitAPI_differential_getrevision_Method.php @@ -91,8 +91,6 @@ class ConduitAPI_differential_getrevision_Method extends ConduitAPIMethod { $revision->getStatus()), 'summary' => $revision->getSummary(), 'testPlan' => $revision->getTestPlan(), - 'revertPlan' => $revision->getRevertPlan(), - 'blameRevision' => $revision->getBlameRevision(), 'lineCount' => $revision->getLineCount(), 'reviewerPHIDs' => $reviewer_phids, 'diffs' => $diff_dicts, diff --git a/src/applications/differential/editor/revision/DifferentialRevisionEditor.php b/src/applications/differential/editor/revision/DifferentialRevisionEditor.php index 20a3fab1f9..ee7cd58eff 100644 --- a/src/applications/differential/editor/revision/DifferentialRevisionEditor.php +++ b/src/applications/differential/editor/revision/DifferentialRevisionEditor.php @@ -175,12 +175,6 @@ class DifferentialRevisionEditor { if ($revision->getAuthorPHID() === null) { $revision->setAuthorPHID($this->getActorPHID()); } - if ($revision->getRevertPlan() === null) { - $revision->setRevertPlan(''); - } - if ($revision->getBlameRevision() === null) { - $revision->setBlameRevision(''); - } if ($revision->getSummary() === null) { $revision->setSummary(''); } diff --git a/src/applications/differential/storage/revision/DifferentialRevision.php b/src/applications/differential/storage/revision/DifferentialRevision.php index fd432fce1f..2e055ef361 100644 --- a/src/applications/differential/storage/revision/DifferentialRevision.php +++ b/src/applications/differential/storage/revision/DifferentialRevision.php @@ -23,8 +23,6 @@ class DifferentialRevision extends DifferentialDAO { protected $summary; protected $testPlan; - protected $revertPlan; - protected $blameRevision; protected $phid; protected $authorPHID; From 1df7d4039e20c18e9460d37ee29399dcaffeac45 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 2 Sep 2011 17:20:14 -0700 Subject: [PATCH 11/17] Store repository credentials with repositories Summary: Move toward storing credentials in configuration so it's easier to get the daemons working. This should eventually solve all the key juggling junk you have to do right now. This only gets us part of the way to actually using these credentials in the daemons since I have to go swap everything for $repository->execBlah(). I tried to write a web "Test Connection" button but it was too much of a mess to get git to work since git doesn't give you access to its SSH command and SSH has a bunch of interactive prompts which you can't really do anything about without it or a bunch of ~/.ssh/config editing. This is what Git recommends: https://git.wiki.kernel.org/index.php/GitFaq#How_do_I_specify_what_ssh_key_git_should_use.3F ..but it's not a great match for this use case. Test Plan: - Only partial. - Ran "test_connection.php" on a Git repo with and without SSH, and with and without valid credentials. This part works properly. - Ran "test_connection.php" on a public SVN repo, but I don't have private or WEBDAV repos set up at the moment. - Mercurial doesn't work yet. - Daemons haven't been converted yet. Reviewers: jungejason, tuomaspelkonen, aran Reviewed By: jungejason CC: aran, abdul, nmalcolm, epriestley, jungejason Differential Revision: 888 --- scripts/repository/test_connection.php | 90 +++++++++ src/__celerity_resource_map__.php | 20 +- .../PhabricatorRepositoryEditController.php | 77 ++++++-- .../repository/controller/edit/__init__.php | 1 + .../repository/PhabricatorRepository.php | 181 ++++++++++++++++++ .../storage/repository/__init__.php | 6 + webroot/index.php | 2 +- 7 files changed, 355 insertions(+), 22 deletions(-) create mode 100755 scripts/repository/test_connection.php diff --git a/scripts/repository/test_connection.php b/scripts/repository/test_connection.php new file mode 100755 index 0000000000..3f511d40c5 --- /dev/null +++ b/scripts/repository/test_connection.php @@ -0,0 +1,90 @@ +#!/usr/bin/env php +\n"; + exit(1); +} + +echo phutil_console_wrap( + phutil_console_format( + 'This script will test that you have configured valid credentials for '. + 'access to a repository, so the Phabricator daemons can pull from it. '. + 'You should run this as the **same user you will run the daemons as**, '. + 'from the **same machine they will run from**. Doing this will help '. + 'detect various problems with your configuration, such as SSH issues.')); + +list($whoami) = execx('whoami'); +$whoami = trim($whoami); + +$ok = phutil_console_confirm("Do you want to continue as '{$whoami}'?"); +if (!$ok) { + die(1); +} + +$callsign = $argv[1]; +echo "Loading '{$callsign}' repository...\n"; +$repository = id(new PhabricatorRepository())->loadOneWhere( + 'callsign = %s', + $argv[1]); +if (!$repository) { + throw new Exception("No such repository exists!"); +} + +$vcs = $repository->getVersionControlSystem(); + +PhutilServiceProfiler::installEchoListener(); + +echo "Trying to connect to the remote...\n"; +switch ($vcs) { + case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN: + $err = $repository->passthruRemoteCommand( + '--limit 1 log %s', + $repository->getRemoteURI()); + break; + case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT: + // Do an ls-remote on a nonexistent ref, which we expect to just return + // nothing. + $err = $repository->passthruRemoteCommand( + 'ls-remote %s %s', + $repository->getRemoteURI(), + 'just-testing'); + break; + default: + throw new Exception("Unsupported repository type."); +} + +if ($err) { + echo phutil_console_format( + "** FAIL ** Connection failed. The credentials for this ". + "repository appear to be incorrectly configured.\n"); + exit(1); +} else { + echo phutil_console_format( + "** OKAY ** Connection successful. The credentials for ". + "this repository appear to be correctly configured.\n"); +} + diff --git a/src/__celerity_resource_map__.php b/src/__celerity_resource_map__.php index 21038f4976..83d7e1f9cc 100644 --- a/src/__celerity_resource_map__.php +++ b/src/__celerity_resource_map__.php @@ -652,6 +652,17 @@ celerity_register_resource_map(array( ), 'disk' => '/rsrc/js/application/maniphest/behavior-transaction-preview.js', ), + 0 => + array( + 'uri' => '/res/1da00bfe/rsrc/js/javelin/lib/__tests__/URI.js', + 'type' => 'js', + 'requires' => + array( + 0 => 'javelin-uri', + 1 => 'javelin-php-serializer', + ), + 'disk' => '/rsrc/js/javelin/lib/__tests__/URI.js', + ), 'javelin-behavior-owners-path-editor' => array( 'uri' => '/res/9cf78ffc/rsrc/js/application/owners/owners-path-editor.js', @@ -1283,15 +1294,6 @@ celerity_register_resource_map(array( ), 'disk' => '/rsrc/js/application/core/Prefab.js', ), - 0 => - array( - 'uri' => '/res/936e8e81/rsrc/js/javelin/docs/onload.js', - 'type' => 'js', - 'requires' => - array( - ), - 'disk' => '/rsrc/js/javelin/docs/onload.js', - ), 'phabricator-profile-css' => array( 'uri' => '/res/ebe1ac2f/rsrc/css/application/profile/profile-view.css', diff --git a/src/applications/repository/controller/edit/PhabricatorRepositoryEditController.php b/src/applications/repository/controller/edit/PhabricatorRepositoryEditController.php index 159cc45c0c..91d3715d2d 100644 --- a/src/applications/repository/controller/edit/PhabricatorRepositoryEditController.php +++ b/src/applications/repository/controller/edit/PhabricatorRepositoryEditController.php @@ -239,6 +239,9 @@ class PhabricatorRepositoryEditController 'default-owners-path', '/')); + $repository->setDetail('ssh-login', $request->getStr('ssh-login')); + $repository->setDetail('ssh-key', $request->getStr('ssh-key')); + $repository->setDetail( 'herald-disabled', $request->getInt('herald-disabled', 0)); @@ -329,10 +332,14 @@ class PhabricatorRepositoryEditController 'Differential, Diffusion, Herald, and other services. To enable '. 'tracking for a repository, configure it here and then start (or '. 'restart) the daemons. More information is available in the '. - ''.$user_guide_link.'.

') + ''.$user_guide_link.'.

'); + + $form + ->appendChild( + '

Basics

') ->appendChild( id(new AphrontFormStaticControl()) - ->setLabel('Repository') + ->setLabel('Repository Name') ->setValue($repository->getName())) ->appendChild( id(new AphrontFormSelectControl()) @@ -345,13 +352,19 @@ class PhabricatorRepositoryEditController ->setValue( $repository->getDetail('tracking-enabled') ? 'enabled' - : 'disabled')); + : 'disabled')) + ->appendChild('
'); + + $form->appendChild( + '

Remote URI

'. + '
'); $uri_label = 'Repository URI'; if ($is_git) { $instructions = - 'NOTE: The user the tracking daemon runs as must have permission to '. - 'git clone from this URI.'; + 'Enter the URI to clone this repository from. It should look like '. + 'git@github.com:example/example.git or '. + 'ssh://user@host.com/git/example.git'; $form->appendChild( '

'.$instructions.'

'); } else if ($is_svn) { @@ -360,10 +373,7 @@ class PhabricatorRepositoryEditController 'You can figure this out by running svn info and looking at '. 'the value in the Repository Root field. It should be a URI '. 'and look like http://svn.example.org/svn/ or '. - 'svn+ssh://svn.example.com/svnroot/.'. - '

'. - 'NOTE: The user the daemons run as must be able to execute '. - 'svn log against this URI.'; + 'svn+ssh://svn.example.com/svnroot/'; $form->appendChild( '

'.$instructions.'

'); $uri_label = 'Repository Root'; @@ -374,9 +384,44 @@ class PhabricatorRepositoryEditController id(new AphrontFormTextControl()) ->setName('uri') ->setLabel($uri_label) + ->setID('remote-uri') ->setValue($repository->getDetail('remote-uri')) ->setError($e_uri)); + $form->appendChild( + '
'. + 'If you want to connect to this repository over SSH, enter the '. + 'username and private key to use. You can leave these fields blank if '. + 'the repository does not use SSH. NOTE: This feature is not '. + 'yet fully supported.'. + '
'); + + $form + ->appendChild( + id(new AphrontFormTextControl()) + ->setName('ssh-login') + ->setLabel('SSH User') + ->setValue($repository->getDetail('ssh-login'))) + ->appendChild( + id(new AphrontFormTextAreaControl()) + ->setName('ssh-key') + ->setLabel('SSH Private Key') + ->setHeight(AphrontFormTextAreaControl::HEIGHT_VERY_SHORT) + ->setValue($repository->getDetail('ssh-key'))) + ->appendChild( + id(new AphrontFormMarkupControl()) + ->setLabel('Test Connection') + ->setValue( + 'To test these credentials, save this form and '. + 'then run: phabricator/scripts/repository/test_connection'. + '.php '.phutil_escape_html($repository->getCallsign()).'')); + + $form->appendChild('
'); + + $form->appendChild( + '

Importing Repository Information

'. + '
'); + if ($is_git) { $form->appendChild( '

Select a path on local disk '. @@ -415,6 +460,12 @@ class PhabricatorRepositoryEditController 'Number of seconds daemon should sleep between requests. Larger '. 'numbers reduce load but also decrease responsiveness.')); + $form->appendChild('

'); + + $form->appendChild( + '

Application Configuration

'. + '
'); + if ($is_git) { $form ->appendChild( @@ -452,8 +503,8 @@ class PhabricatorRepositoryEditController 1 => 'Disabled - Do Not Send Email', )) ->setCaption( - 'You can temporarily disable Herald notifications when reparsing '. - 'a repository or importing a new repository.')); + 'You can temporarily disable Herald commit notifications when '. + 'reparsing a repository or importing a new repository.')); $parsers = id(new PhutilSymbolLoader()) ->setAncestorClass('PhabricatorRepositoryCommitMessageDetailParser') @@ -487,10 +538,12 @@ class PhabricatorRepositoryEditController ->setCaption('Repository UUID from svn info.')); } + $form->appendChild('
'); + $form ->appendChild( id(new AphrontFormSubmitControl()) - ->setValue('Save')); + ->setValue('Save Configuration')); $panel = new AphrontPanelView(); $panel->setHeader('Repository Tracking'); diff --git a/src/applications/repository/controller/edit/__init__.php b/src/applications/repository/controller/edit/__init__.php index 7da55db338..4f61cde518 100644 --- a/src/applications/repository/controller/edit/__init__.php +++ b/src/applications/repository/controller/edit/__init__.php @@ -17,6 +17,7 @@ phutil_require_module('phabricator', 'applications/repository/storage/repository phutil_require_module('phabricator', 'infrastructure/env'); phutil_require_module('phabricator', 'view/control/table'); phutil_require_module('phabricator', 'view/form/base'); +phutil_require_module('phabricator', 'view/form/control/markup'); phutil_require_module('phabricator', 'view/form/control/select'); phutil_require_module('phabricator', 'view/form/control/static'); phutil_require_module('phabricator', 'view/form/control/submit'); diff --git a/src/applications/repository/storage/repository/PhabricatorRepository.php b/src/applications/repository/storage/repository/PhabricatorRepository.php index 0235e1a435..bb1181ceea 100644 --- a/src/applications/repository/storage/repository/PhabricatorRepository.php +++ b/src/applications/repository/storage/repository/PhabricatorRepository.php @@ -32,6 +32,8 @@ class PhabricatorRepository extends PhabricatorRepositoryDAO { protected $versionControlSystem; protected $details = array(); + private $sshKeyfile; + public function getConfiguration() { return array( self::CONFIG_AUX_PHID => true, @@ -55,4 +57,183 @@ class PhabricatorRepository extends PhabricatorRepositoryDAO { return $this; } + public function getRemoteURI() { + $raw_uri = $this->getDetail('remote-uri'); + + $vcs = $this->getVersionControlSystem(); + $is_git = ($vcs == PhabricatorRepositoryType::REPOSITORY_TYPE_GIT); + + // If there's no protocol (git implicit SSH) reformat the URI to be a + // normal URI. These git URIs look like "user@domain.com:path" instead of + // "ssh://user@domain/path". + $uri = new PhutilURI($raw_uri); + if ($is_git && !$uri->getProtocol()) { + list($domain, $path) = explode(':', $raw_uri, 2); + $uri = new PhutilURI('ssh://'.$domain.'/'.$path); + } + + if ($this->isSSHProtocol($uri->getProtocol())) { + if ($this->getSSHLogin()) { + $uri->setUser($this->getSSHLogin()); + } + } + + return (string)$uri; + } + + public function getLocalPath() { + return $this->getDetail('local-path'); + } + + public function execRemoteCommand($pattern /*, $arg, ... */) { + $args = func_get_args(); + $args = $this->formatRemoteCommand($args); + return call_user_func_array('exec_manual', $args); + } + + public function execxRemoteCommand($pattern /*, $arg, ... */) { + $args = func_get_args(); + $args = $this->formatRemoteCommand($args); + return call_user_func_array('execx', $args); + } + + public function passthruRemoteCommand($pattern /*, $arg, ... */) { + $args = func_get_args(); + $args = $this->formatRemoteCommand($args); + return call_user_func_array('phutil_passthru', $args); + } + + public function execLocalCommand($pattern /*, $arg, ... */) { + $args = func_get_args(); + $args = $this->formatLocalCommand($args); + return call_user_func_array('exec_manual', $args); + } + + public function execxLocalCommand($pattern /*, $arg, ... */) { + $args = func_get_args(); + $args = $this->formatLocalCommand($args); + return call_user_func_array('execx', $args); + } + + public function passthruLocalCommand($pattern /*, $arg, ... */) { + $args = func_get_args(); + $args = $this->formatLocalCommand($args); + return call_user_func_array('phutil_passthru', $args); + } + + private function formatRemoteCommand(array $args) { + $pattern = $args[0]; + $args = array_slice($args, 1); + + if ($this->shouldUseSSH()) { + switch ($this->getVersionControlSystem()) { + case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN: + $pattern = "SVN_SSH=%s svn {$pattern}"; + array_unshift( + $args, + csprintf( + 'ssh -l %s -i %s', + $this->getSSHLogin(), + $this->getSSHKeyfile())); + break; + case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT: + $command = call_user_func_array( + 'csprintf', + array_merge( + array( + "(ssh-add %s && git {$pattern})", + $this->getSSHKeyfile(), + ), + $args)); + $pattern = "ssh-agent sh -c %s"; + $args = array($command); + break; + case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL: + $pattern = "hg --config ui.ssh=%s {$pattern}"; + array_unshift( + $args, + csprintf( + 'ssh -l %s -i %s', + $this->getSSHLogin(), + $this->getSSHKeyfile())); + break; + default: + throw new Exception("Unrecognized version control system."); + } + } else { + switch ($this->getVersionControlSystem()) { + case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN: + $pattern = "svn {$pattern}"; + break; + case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT: + $pattern = "git {$pattern}"; + break; + case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL: + $pattern = "hg {$pattern}"; + break; + default: + throw new Exception("Unrecognized version control system."); + } + } + + array_unshift($args, $pattern); + + return $args; + } + + private function formatLocalCommand(array $args) { + $pattern = $args[0]; + $args = array_slice($args, 1); + + switch ($this->getVersionControlSystem()) { + case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN: + $pattern = "(cd %s && svn {$pattern})"; + array_unshift($args, $this->getLocalPath()); + break; + case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT: + $pattern = "(cd %s && git {$pattern})"; + array_unshift($args, $this->getLocalPath()); + break; + case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL: + $pattern = "(cd %s && hg {$pattern})"; + array_unshift($args, $this->getLocalPath()); + break; + default: + throw new Exception("Unrecognized version control system."); + } + + array_unshift($args, $pattern); + + return $args; + } + + private function getSSHLogin() { + return $this->getDetail('ssh-login'); + } + + private function getSSHKeyfile() { + if (!$this->sshKeyfile) { + $keyfile = new TempFile('phabricator-repository-ssh-key'); + chmod($keyfile, 0600); + Filesystem::writeFile($keyfile, $this->getDetail('ssh-key')); + $this->sshKeyfile = $keyfile; + } + + return (string)$this->sshKeyfile; + } + + public function shouldUseSSH() { + $uri = new PhutilURI($this->getRemoteURI()); + $protocol = $uri->getProtocol(); + if ($this->isSSHProtocol($protocol)) { + return (bool)$this->getDetail('ssh-key'); + } else { + return false; + } + } + + private function isSSHProtocol($protocol) { + return ($protocol == 'ssh' || $protocol == 'svn+ssh'); + } + } diff --git a/src/applications/repository/storage/repository/__init__.php b/src/applications/repository/storage/repository/__init__.php index 6480f38dac..e08a467030 100644 --- a/src/applications/repository/storage/repository/__init__.php +++ b/src/applications/repository/storage/repository/__init__.php @@ -8,9 +8,15 @@ phutil_require_module('phabricator', 'applications/phid/constants'); phutil_require_module('phabricator', 'applications/phid/storage/phid'); +phutil_require_module('phabricator', 'applications/repository/constants/repositorytype'); phutil_require_module('phabricator', 'applications/repository/storage/base'); +phutil_require_module('phutil', 'filesystem'); +phutil_require_module('phutil', 'filesystem/tempfile'); +phutil_require_module('phutil', 'future/exec'); +phutil_require_module('phutil', 'parser/uri'); phutil_require_module('phutil', 'utils'); +phutil_require_module('phutil', 'xsprintf/csprintf'); phutil_require_source('PhabricatorRepository.php'); diff --git a/webroot/index.php b/webroot/index.php index 44159da28b..2c2154fadb 100644 --- a/webroot/index.php +++ b/webroot/index.php @@ -250,7 +250,7 @@ function phabricator_shutdown() { return; } - if ($event['type'] != E_ERROR) { + if ($event['type'] != E_ERROR && $event['type'] != E_PARSE) { return; } From cd7ba81d83ef1f1ccfd0e16a060cd88301ca4e0e Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 6 Sep 2011 15:15:01 -0700 Subject: [PATCH 12/17] Use "Best" URI when linking to files from Maniphest file previews Summary: Previously, this code accidentally did not use the best URI. Instead, use the best URI. It's the best, obviously. Test Plan: Uploaded a binary file and then clicked the preview. Reviewers: hunterbridges, jungejason, nh, tuomaspelkonen, aran Reviewed By: tuomaspelkonen CC: aran, tuomaspelkonen Differential Revision: 905 --- src/view/layout/filepreview/AphrontFilePreviewView.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/view/layout/filepreview/AphrontFilePreviewView.php b/src/view/layout/filepreview/AphrontFilePreviewView.php index 274d640e77..d1f017e4dd 100644 --- a/src/view/layout/filepreview/AphrontFilePreviewView.php +++ b/src/view/layout/filepreview/AphrontFilePreviewView.php @@ -41,7 +41,7 @@ final class AphrontFilePreviewView extends AphrontView { $link = phutil_render_tag( 'a', array( - 'href' => $file->getViewURI(), + 'href' => $file->getBestURI(), 'target' => '_blank', ), $img); From cd6eb836f6ca73c7aa17ddfd65beb062c82836fe Mon Sep 17 00:00:00 2001 From: Hua Wang Date: Wed, 31 Aug 2011 23:18:38 -0700 Subject: [PATCH 13/17] Enable comments for image Summary: Added line number 1 for each image and added code to display the comments for each image. Test Plan: Adding an image in my local directory and create a revision for it. Click line number 1, and the comment window prompts. Adding and save the comment. The comment shows in the differential comment list and in the inline comment. Submit the comment. Create more comments for the image and the "Previous" and "Next" buttons all work well. Reviewers: epriestley, jungejason CC: Differential Revision: 901 --- .../changeset/DifferentialChangesetParser.php | 50 +++++++++++++++++-- 1 file changed, 47 insertions(+), 3 deletions(-) diff --git a/src/applications/differential/parser/changeset/DifferentialChangesetParser.php b/src/applications/differential/parser/changeset/DifferentialChangesetParser.php index fe0878b8c9..b8ace11c4d 100644 --- a/src/applications/differential/parser/changeset/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/changeset/DifferentialChangesetParser.php @@ -899,22 +899,66 @@ class DifferentialChangesetParser { } } + $this->comments = msort($this->comments, 'getID'); + $old_comments = array(); + $new_comments = array(); + foreach ($this->comments as $comment) { + if ($this->isCommentOnRightSideWhenDisplayed($comment)) { + $new_comments[] = $comment; + } else { + $old_comments[] = $comment; + } + } + + $html_old = array(); + $html_new = array(); + foreach ($old_comments as $comment) { + $xhp = $this->renderInlineComment($comment); + $html_old[] = + ''. + $xhp. + ''; + } + foreach ($new_comments as $comment) { + $xhp = $this->renderInlineComment($comment); + $html_new[] = + ''. + $xhp. + ''; + } + + $changset_id = $this->changeset->getID(); + if (!$old) { + $th_old = ''; + } + else { + $th_old = '1'; + } + if (!$cur) { + $th_new = ''; + } + else { + $th_new = '1'; + } + $output = $this->renderChangesetTable( $this->changeset, ''. - ''. + $th_old. ''. '
'. $old. '
'. ''. - ''. + $th_new. ''. '
'. $cur. '
'. ''. - ''); + ''. + implode('', $html_old). + implode('', $html_new)); return $output; case DifferentialChangeType::FILE_DIRECTORY: From 40c14501298ece87104e958d668e27781422e609 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 31 Aug 2011 10:50:40 -0700 Subject: [PATCH 14/17] Add an explicit test for the availablility of 'php' from the command line during setup Summary: See T481. We'll fail the pcntl test if we don't have this, in a potentially confusing way. Test and detect missing 'php' explicitly before we try the pcntl test, so we can give the user a better error message. Test Plan: In setup mode, did a good run and then faked it to execute 'phpx' instead to get a failure. Reviewers: johnduhart, jungejason, tuomaspelkonen, aran Reviewed By: tuomaspelkonen CC: aran, epriestley, tuomaspelkonen Differential Revision: 878 --- src/infrastructure/setup/PhabricatorSetup.php | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/infrastructure/setup/PhabricatorSetup.php b/src/infrastructure/setup/PhabricatorSetup.php index 5a27eb4e50..59648608f6 100644 --- a/src/infrastructure/setup/PhabricatorSetup.php +++ b/src/infrastructure/setup/PhabricatorSetup.php @@ -145,6 +145,21 @@ class PhabricatorSetup { } } + list($err, $stdout, $stderr) = exec_manual( + '/usr/bin/env php -r %s', + 'exit;'); + if ($err) { + self::writeFailure(); + self::write("Unable to execute 'php' on the command line from the web ". + "server. Verify that 'php' is in the webserver's PATH.\n". + " err: {$err}\n". + "stdout: {$stdout}\n". + "stderr: {$stderr}\n"); + return; + } else { + self::write(" okay PHP is available from the command line.\n"); + } + $root = dirname(phutil_get_library_root('phabricator')); // On RHEL6, doing a distro install of pcntl makes it available from the From e3a9d73fe140ca53d010c2582df861f31d33e538 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 6 Sep 2011 09:37:35 -0700 Subject: [PATCH 15/17] Add keyfile and HTTP Basic auth support to repositories Summary: I still need to go through all the daemon and Diffusion code and change the bare execx() calls to $repository->execxXXX() to actually make this work, but we're getting close. Test Plan: Configured repositories with various HTTP / SVN setups and ran the test_connection.php script to verify keys were located and added and username/password information was supplied. Reviewers: jungejason, nh, tuomaspelkonen, aran Reviewed By: nh CC: aran, nh, jungejason Differential Revision: 902 --- .../PhabricatorRepositoryEditController.php | 75 ++++++++++++++++--- .../repository/controller/edit/__init__.php | 1 - .../repository/PhabricatorRepository.php | 62 ++++++++++++--- webroot/rsrc/css/aphront/form-view.css | 12 +++ 4 files changed, 131 insertions(+), 19 deletions(-) diff --git a/src/applications/repository/controller/edit/PhabricatorRepositoryEditController.php b/src/applications/repository/controller/edit/PhabricatorRepositoryEditController.php index 91d3715d2d..414f8bd94e 100644 --- a/src/applications/repository/controller/edit/PhabricatorRepositoryEditController.php +++ b/src/applications/repository/controller/edit/PhabricatorRepositoryEditController.php @@ -205,6 +205,9 @@ class PhabricatorRepositoryEditController $is_git = false; $is_svn = false; + $e_ssh_key = null; + $e_ssh_keyfile = null; + switch ($repository->getVersionControlSystem()) { case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT: $is_git = true; @@ -241,6 +244,19 @@ class PhabricatorRepositoryEditController $repository->setDetail('ssh-login', $request->getStr('ssh-login')); $repository->setDetail('ssh-key', $request->getStr('ssh-key')); + $repository->setDetail('ssh-keyfile', $request->getStr('ssh-keyfile')); + + $repository->setDetail('http-login', $request->getStr('http-login')); + $repository->setDetail('http-pass', $request->getStr('http-pass')); + + if ($repository->getDetail('ssh-key') && + $repository->getDetail('ssh-keyfile')) { + $errors[] = + "Specify only one of 'SSH Private Key' and 'SSH Private Key File', ". + "not both."; + $e_ssh_key = 'Choose Only One'; + $e_ssh_keyfile = 'Choose Only One'; + } $repository->setDetail( 'herald-disabled', @@ -392,8 +408,8 @@ class PhabricatorRepositoryEditController '
'. 'If you want to connect to this repository over SSH, enter the '. 'username and private key to use. You can leave these fields blank if '. - 'the repository does not use SSH. NOTE: This feature is not '. - 'yet fully supported.'. + 'the repository does not use SSH.'. + ' NOTE: This feature is not yet fully supported.'. '
'); $form @@ -407,14 +423,55 @@ class PhabricatorRepositoryEditController ->setName('ssh-key') ->setLabel('SSH Private Key') ->setHeight(AphrontFormTextAreaControl::HEIGHT_VERY_SHORT) - ->setValue($repository->getDetail('ssh-key'))) + ->setValue($repository->getDetail('ssh-key')) + ->setError($e_ssh_key) + ->setCaption('Specify the entire private key, or...')) ->appendChild( - id(new AphrontFormMarkupControl()) - ->setLabel('Test Connection') - ->setValue( - 'To test these credentials, save this form and '. - 'then run: phabricator/scripts/repository/test_connection'. - '.php '.phutil_escape_html($repository->getCallsign()).'')); + id(new AphrontFormTextControl()) + ->setName('ssh-keyfile') + ->setLabel('SSH Private Key File') + ->setValue($repository->getDetail('ssh-keyfile')) + ->setError($e_ssh_keyfile) + ->setCaption( + '...specify a path on disk where the daemon should '. + 'look for a private key.')); + + $supports_http = $is_svn; + + if ($supports_http) { + $form + ->appendChild( + '
'. + 'If you want to connect to this repository over HTTP Basic Auth, '. + 'enter the username and password to use. You can leave these '. + 'fields blank if the repository does not use HTTP Basic Auth.'. + ' NOTE: This feature is not yet fully supported.'. + '
') + ->appendChild( + id(new AphrontFormTextControl()) + ->setName('http-login') + ->setLabel('HTTP Basic Login') + ->setValue($repository->getDetail('http-login'))) + ->appendChild( + id(new AphrontFormTextControl()) + ->setName('http-pass') + ->setLabel('HTTP Basic Password') + ->setValue($repository->getDetail('http-pass'))); + } + + $form + ->appendChild( + '
'. + 'To test your authentication configuration, save this '. + 'form and then run this script:'. + ''. + 'phabricator/ $ ./scripts/repository/test_connection.php '. + phutil_escape_html($repository->getCallsign()). + ''. + 'This will verify that your configuration is correct and the '. + 'daemons can connect to the remote repository and pull changes '. + 'from it.'. + '
'); $form->appendChild('
'); diff --git a/src/applications/repository/controller/edit/__init__.php b/src/applications/repository/controller/edit/__init__.php index 4f61cde518..7da55db338 100644 --- a/src/applications/repository/controller/edit/__init__.php +++ b/src/applications/repository/controller/edit/__init__.php @@ -17,7 +17,6 @@ phutil_require_module('phabricator', 'applications/repository/storage/repository phutil_require_module('phabricator', 'infrastructure/env'); phutil_require_module('phabricator', 'view/control/table'); phutil_require_module('phabricator', 'view/form/base'); -phutil_require_module('phabricator', 'view/form/control/markup'); phutil_require_module('phabricator', 'view/form/control/select'); phutil_require_module('phabricator', 'view/form/control/static'); phutil_require_module('phabricator', 'view/form/control/submit'); diff --git a/src/applications/repository/storage/repository/PhabricatorRepository.php b/src/applications/repository/storage/repository/PhabricatorRepository.php index bb1181ceea..d0e5e29808 100644 --- a/src/applications/repository/storage/repository/PhabricatorRepository.php +++ b/src/applications/repository/storage/repository/PhabricatorRepository.php @@ -128,7 +128,7 @@ class PhabricatorRepository extends PhabricatorRepositoryDAO { if ($this->shouldUseSSH()) { switch ($this->getVersionControlSystem()) { case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN: - $pattern = "SVN_SSH=%s svn {$pattern}"; + $pattern = "SVN_SSH=%s svn --non-interactive {$pattern}"; array_unshift( $args, csprintf( @@ -160,10 +160,30 @@ class PhabricatorRepository extends PhabricatorRepositoryDAO { default: throw new Exception("Unrecognized version control system."); } + } else if ($this->shouldUseHTTP()) { + switch ($this->getVersionControlSystem()) { + case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN: + $pattern = + "svn ". + "--non-interactive ". + "--no-auth-cache ". + "--trust-server-cert ". + "--username %s ". + "--password %s ". + $pattern; + array_unshift( + $args, + $this->getDetail('http-login'), + $this->getDetail('http-pass')); + break; + default: + throw new Exception( + "No support for HTTP Basic Auth in this version control system."); + } } else { switch ($this->getVersionControlSystem()) { case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN: - $pattern = "svn {$pattern}"; + $pattern = "svn --non-interactive {$pattern}"; break; case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT: $pattern = "git {$pattern}"; @@ -187,7 +207,7 @@ class PhabricatorRepository extends PhabricatorRepositoryDAO { switch ($this->getVersionControlSystem()) { case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN: - $pattern = "(cd %s && svn {$pattern})"; + $pattern = "(cd %s && svn --non-interactive {$pattern})"; array_unshift($args, $this->getLocalPath()); break; case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT: @@ -212,11 +232,21 @@ class PhabricatorRepository extends PhabricatorRepositoryDAO { } private function getSSHKeyfile() { - if (!$this->sshKeyfile) { - $keyfile = new TempFile('phabricator-repository-ssh-key'); - chmod($keyfile, 0600); - Filesystem::writeFile($keyfile, $this->getDetail('ssh-key')); - $this->sshKeyfile = $keyfile; + if ($this->sshKeyfile === null) { + $key = $this->getDetail('ssh-key'); + $keyfile = $this->getDetail('ssh-keyfile'); + if ($keyfile) { + // Make sure we can read the file, that it exists, etc. + Filesystem::readFile($keyfile); + $this->sshKeyfile = $keyfile; + } else if ($key) { + $keyfile = new TempFile('phabricator-repository-ssh-key'); + chmod($keyfile, 0600); + Filesystem::writeFile($keyfile, $key); + $this->sshKeyfile = $keyfile; + } else { + $this->sshKeyfile = ''; + } } return (string)$this->sshKeyfile; @@ -226,7 +256,17 @@ class PhabricatorRepository extends PhabricatorRepositoryDAO { $uri = new PhutilURI($this->getRemoteURI()); $protocol = $uri->getProtocol(); if ($this->isSSHProtocol($protocol)) { - return (bool)$this->getDetail('ssh-key'); + return (bool)$this->getSSHKeyfile(); + } else { + return false; + } + } + + public function shouldUseHTTP() { + $uri = new PhutilURI($this->getRemoteURI()); + $protocol = $uri->getProtocol(); + if ($this->isHTTPProtocol($protocol)) { + return (bool)$this->getDetail('http-login'); } else { return false; } @@ -236,4 +276,8 @@ class PhabricatorRepository extends PhabricatorRepositoryDAO { return ($protocol == 'ssh' || $protocol == 'svn+ssh'); } + private function isHTTPProtocol($protocol) { + return ($protocol == 'http' || $protocol == 'https'); + } + } diff --git a/webroot/rsrc/css/aphront/form-view.css b/webroot/rsrc/css/aphront/form-view.css index 6c228cc51a..f05e957d3f 100644 --- a/webroot/rsrc/css/aphront/form-view.css +++ b/webroot/rsrc/css/aphront/form-view.css @@ -87,6 +87,18 @@ margin: 0.75em 3% 1.25em; } +.aphront-form-important { + margin: .5em 0; + background: #ffffdd; + padding: .5em 1em; +} +.aphront-form-important code { + display: block; + padding: .25em; + margin: .5em 2em; +} + + .aphront-form-control-static .aphront-form-input { padding-top: 4px; font-size: 13px; From 6355b291ed4478e5312b3e9de07a4b1bdf154005 Mon Sep 17 00:00:00 2001 From: Abdul Qabiz Date: Fri, 9 Sep 2011 01:16:48 +0530 Subject: [PATCH 16/17] - Added getRemoteCommandFuture(..) and getLocalCommand Future(..) methods to PhabricatorRepository - Removed irrelevant csprintf(..) - Updated code to use $repository->getRemoteURI() - Updated code to use getRemoteCommandFuture(..) in Diffusion code - Updated code to use $repository->getRemoteURI() --- .../query/diff/svn/DiffusionSvnDiffQuery.php | 6 +++--- .../diffusion/query/diff/svn/__init__.php | 1 - .../svn/DiffusionSvnFileContentQuery.php | 6 +++--- .../diffusion/query/filecontent/svn/__init__.php | 1 - ...habricatorRepositorySvnCommitDiscoveryDaemon.php | 11 ++++++----- .../daemon/commitdiscovery/svn/__init__.php | 1 - .../storage/repository/PhabricatorRepository.php | 13 +++++++++++++ .../PhabricatorRepositoryCommitParserWorker.php | 4 ++-- ...ricatorRepositorySvnCommitChangeParserWorker.php | 11 +++++------ 9 files changed, 32 insertions(+), 22 deletions(-) diff --git a/src/applications/diffusion/query/diff/svn/DiffusionSvnDiffQuery.php b/src/applications/diffusion/query/diff/svn/DiffusionSvnDiffQuery.php index 80f3348f76..cf8543002b 100644 --- a/src/applications/diffusion/query/diff/svn/DiffusionSvnDiffQuery.php +++ b/src/applications/diffusion/query/diff/svn/DiffusionSvnDiffQuery.php @@ -140,9 +140,9 @@ final class DiffusionSvnDiffQuery extends DiffusionDiffQuery { $repository = $drequest->getRepository(); list($ref, $rev) = $spec; - return new ExecFuture( - 'svn --non-interactive cat %s%s@%d', - $repository->getDetail('remote-uri'), + return $repository->getRemoteCommandFuture( + 'cat %s%s@%d', + $repository->getRemoteURI(), $ref, $rev); } diff --git a/src/applications/diffusion/query/diff/svn/__init__.php b/src/applications/diffusion/query/diff/svn/__init__.php index a87b1cbdfe..c7ae1d6b3a 100644 --- a/src/applications/diffusion/query/diff/svn/__init__.php +++ b/src/applications/diffusion/query/diff/svn/__init__.php @@ -16,7 +16,6 @@ phutil_require_module('phabricator', 'applications/diffusion/query/pathchange/ba phutil_require_module('phabricator', 'infrastructure/diff/engine'); phutil_require_module('phutil', 'future'); -phutil_require_module('phutil', 'future/exec'); phutil_require_module('phutil', 'utils'); diff --git a/src/applications/diffusion/query/filecontent/svn/DiffusionSvnFileContentQuery.php b/src/applications/diffusion/query/filecontent/svn/DiffusionSvnFileContentQuery.php index 3371c3027b..2104823b64 100644 --- a/src/applications/diffusion/query/filecontent/svn/DiffusionSvnFileContentQuery.php +++ b/src/applications/diffusion/query/filecontent/svn/DiffusionSvnFileContentQuery.php @@ -33,11 +33,11 @@ final class DiffusionSvnFileContentQuery extends DiffusionFileContentQuery { $path = $drequest->getPath(); $commit = $drequest->getCommit(); - $remote_uri = $repository->getDetail('remote-uri'); + $remote_uri = $repository->getRemoteURI(); try { - list($corpus) = execx( - 'svn --non-interactive %s %s%s@%s', + list($corpus) = $repository->execxRemoteCommand( + '%s %s%s@%s', $this->getNeedsBlame() ? 'blame' : 'cat', $remote_uri, $path, diff --git a/src/applications/diffusion/query/filecontent/svn/__init__.php b/src/applications/diffusion/query/filecontent/svn/__init__.php index c10e912fa8..57fe48ee4c 100644 --- a/src/applications/diffusion/query/filecontent/svn/__init__.php +++ b/src/applications/diffusion/query/filecontent/svn/__init__.php @@ -9,7 +9,6 @@ phutil_require_module('phabricator', 'applications/diffusion/data/filecontent'); phutil_require_module('phabricator', 'applications/diffusion/query/filecontent/base'); -phutil_require_module('phutil', 'future/exec'); phutil_require_module('phutil', 'utils'); diff --git a/src/applications/repository/daemon/commitdiscovery/svn/PhabricatorRepositorySvnCommitDiscoveryDaemon.php b/src/applications/repository/daemon/commitdiscovery/svn/PhabricatorRepositorySvnCommitDiscoveryDaemon.php index 1492caa547..fc68dad836 100644 --- a/src/applications/repository/daemon/commitdiscovery/svn/PhabricatorRepositorySvnCommitDiscoveryDaemon.php +++ b/src/applications/repository/daemon/commitdiscovery/svn/PhabricatorRepositorySvnCommitDiscoveryDaemon.php @@ -28,9 +28,9 @@ class PhabricatorRepositorySvnCommitDiscoveryDaemon } $uri = $this->getBaseSVNLogURI(); - list($xml) = execx( - 'svn log --xml --non-interactive --quiet --limit 1 %s@HEAD', - $uri); + list($xml) = $repository->execxRemoteCommand( + ' log --xml --quiet --limit 1 %s@HEAD', + $uri); $results = $this->parseSVNLogXML($xml); $commit = key($results); @@ -47,6 +47,7 @@ class PhabricatorRepositorySvnCommitDiscoveryDaemon private function discoverCommit($commit, $epoch) { $uri = $this->getBaseSVNLogURI(); + $repository = $this->getRepository(); $discover = array( $commit => $epoch, @@ -58,8 +59,8 @@ class PhabricatorRepositorySvnCommitDiscoveryDaemon // Find all the unknown commits on this path. Note that we permit // importing an SVN subdirectory rather than the entire repository, so // commits may be nonsequential. - list($err, $xml, $stderr) = exec_manual( - 'svn log --xml --non-interactive --quiet --limit %d %s@%d', + list($err, $xml, $stderr) = $repository->execRemoteCommand( + ' log --xml --quiet --limit %d %s@%d', $limit, $uri, $upper_bound - 1); diff --git a/src/applications/repository/daemon/commitdiscovery/svn/__init__.php b/src/applications/repository/daemon/commitdiscovery/svn/__init__.php index df2258faf8..e8b0dd6f29 100644 --- a/src/applications/repository/daemon/commitdiscovery/svn/__init__.php +++ b/src/applications/repository/daemon/commitdiscovery/svn/__init__.php @@ -9,7 +9,6 @@ phutil_require_module('phabricator', 'applications/repository/constants/repositorytype'); phutil_require_module('phabricator', 'applications/repository/daemon/commitdiscovery/base'); -phutil_require_module('phutil', 'future/exec'); phutil_require_module('phutil', 'utils'); diff --git a/src/applications/repository/storage/repository/PhabricatorRepository.php b/src/applications/repository/storage/repository/PhabricatorRepository.php index d0e5e29808..1775f7ba0f 100644 --- a/src/applications/repository/storage/repository/PhabricatorRepository.php +++ b/src/applications/repository/storage/repository/PhabricatorRepository.php @@ -97,6 +97,12 @@ class PhabricatorRepository extends PhabricatorRepositoryDAO { return call_user_func_array('execx', $args); } + public function getRemoteCommandFuture($pattern /*, $arg, ... */) { + $args = func_get_args(); + $args = $this->formatRemoteCommand($args); + return newv('ExecFuture', $args); + } + public function passthruRemoteCommand($pattern /*, $arg, ... */) { $args = func_get_args(); $args = $this->formatRemoteCommand($args); @@ -115,12 +121,19 @@ class PhabricatorRepository extends PhabricatorRepositoryDAO { return call_user_func_array('execx', $args); } + public function getLocalCommandFuture($pattern /*, $arg, ... */) { + $args = func_get_args(); + $args = $this->formatLocalCommand($args); + return newv('ExecFuture', $args); + + } public function passthruLocalCommand($pattern /*, $arg, ... */) { $args = func_get_args(); $args = $this->formatLocalCommand($args); return call_user_func_array('phutil_passthru', $args); } + private function formatRemoteCommand(array $args) { $pattern = $args[0]; $args = array_slice($args, 1); diff --git a/src/applications/repository/worker/base/PhabricatorRepositoryCommitParserWorker.php b/src/applications/repository/worker/base/PhabricatorRepositoryCommitParserWorker.php index 7596b9cd23..938c9eb199 100644 --- a/src/applications/repository/worker/base/PhabricatorRepositoryCommitParserWorker.php +++ b/src/applications/repository/worker/base/PhabricatorRepositoryCommitParserWorker.php @@ -68,8 +68,8 @@ abstract class PhabricatorRepositoryCommitParserWorker } try { - list($xml) = execx( - "svn log --xml {$verbose} --limit 1 --non-interactive %s@%d", + list($xml) = $this->repository->execxRemoteCommand( + "log --xml {$verbose} --limit 1 %s@%d", $uri, $revision); } catch (CommandException $ex) { diff --git a/src/applications/repository/worker/commitchangeparser/svn/PhabricatorRepositorySvnCommitChangeParserWorker.php b/src/applications/repository/worker/commitchangeparser/svn/PhabricatorRepositorySvnCommitChangeParserWorker.php index 0965aaef9d..266dff941a 100644 --- a/src/applications/repository/worker/commitchangeparser/svn/PhabricatorRepositorySvnCommitChangeParserWorker.php +++ b/src/applications/repository/worker/commitchangeparser/svn/PhabricatorRepositorySvnCommitChangeParserWorker.php @@ -593,10 +593,9 @@ class PhabricatorRepositorySvnCommitChangeParserWorker // position in the document. $all_paths = array_reverse(array_keys($parents)); foreach (array_chunk($all_paths, 64) as $path_chunk) { - list($raw_xml) = execx( - 'svn --non-interactive --xml ls %C', - implode(' ', $path_chunk)); - + list($raw_xml) = $repository->execxRemoteCommand( + '--xml ls %C', + implode(' ', $path_chunk)); $xml = new SimpleXMLElement($raw_xml); foreach ($xml->list as $list) { $list_path = (string)$list['path']; @@ -669,8 +668,8 @@ class PhabricatorRepositorySvnCommitChangeParserWorker $cache_loc = sys_get_temp_dir().'/diffusion.'.$hashkey.'.svnls'; if (!Filesystem::pathExists($cache_loc)) { $tmp = new TempFile(); - execx( - 'svn --non-interactive --xml ls -R %s%s@%d > %s', + $repository->execxRemoteCommand( + '--xml ls -R %s%s@%d > %s', $repository->getDetail('remote-uri'), $path, $rev, From 87309734cc5c8ab3be532e21279199450264019d Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 8 Sep 2011 14:16:59 -0700 Subject: [PATCH 17/17] Nuke sessions from the database when users logout Summary: @tomo ran into an issue where he had some non-SSL-only cookie or whatever, so "Logout" had no apparent effect. Make sure "Logout" really works by destroying the session. I originally kept the sessions around to be able to debug session stuff, but we have a fairly good session log now and no reprorted session bugs except for all the cookie stuff. It's also slightly more secure to actually destroy sessions, since it means "logout" breaks any cookies that attackers somehow stole (e.g., by reading your requests off a public wifi network). Test Plan: Commented out the cookie clear and logged out. I was logged out and given a useful error message about clearing my cookies. Reviewers: jungejason, nh, tuomaspelkonen, aran Reviewed By: aran CC: tomo, aran, epriestley Differential Revision: 911 --- .../controller/logout/PhabricatorLogoutController.php | 8 ++++++++ .../people/storage/user/PhabricatorUser.php | 10 ++++++++++ 2 files changed, 18 insertions(+) diff --git a/src/applications/auth/controller/logout/PhabricatorLogoutController.php b/src/applications/auth/controller/logout/PhabricatorLogoutController.php index ddf31ad76b..3db12be594 100644 --- a/src/applications/auth/controller/logout/PhabricatorLogoutController.php +++ b/src/applications/auth/controller/logout/PhabricatorLogoutController.php @@ -39,7 +39,15 @@ class PhabricatorLogoutController extends PhabricatorAuthController { PhabricatorUserLog::ACTION_LOGOUT); $log->save(); + // Destroy the user's session in the database so logout works even if + // their cookies have some issues. We'll detect cookie issues when they + // try to login again and tell them to clear any junk. + $phsid = $request->getCookie('phsid'); + if ($phsid) { + $user->destroySession($phsid); + } $request->clearCookie('phsid'); + return id(new AphrontRedirectResponse()) ->setURI('/login/'); } diff --git a/src/applications/people/storage/user/PhabricatorUser.php b/src/applications/people/storage/user/PhabricatorUser.php index 043ee96e2f..7a21d30594 100644 --- a/src/applications/people/storage/user/PhabricatorUser.php +++ b/src/applications/people/storage/user/PhabricatorUser.php @@ -284,6 +284,16 @@ class PhabricatorUser extends PhabricatorUserDAO { return $session_key; } + public function destroySession($session_key) { + $conn_w = $this->establishConnection('w'); + queryfx( + $conn_w, + 'DELETE FROM %T WHERE userPHID = %s AND sessionKey = %s', + self::SESSION_TABLE, + $this->getPHID(), + $session_key); + } + private function generateEmailToken($offset = 0) { return $this->generateToken( time() + ($offset * self::EMAIL_CYCLE_FREQUENCY),