From 8dccf05c4c1af535ca1310e9e893bd2e4d316db3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 20 Dec 2017 09:47:40 -0800 Subject: [PATCH 1/7] Manually set "max_allowed_packet" to 1GB for "mysqldump" Summary: We have one production instance with failing database backups since they recently uploaded a 52MB hunk. The production configuration specifies a 64MB "max_allowed_packet" in `[mysqld]`, but this doesn't apply to `mysqldump` (we'd need to specify it in a separate `[mysqldump]` section) and `mysqldump` runs with an effective limit of the default (16MB). We could change our production config to specify a value in `[mysqldump]`, but just change it unconditionally at execution time since there's no reason for any user to ever want this command to fail because they have too much data. Test Plan: Dumped locally, will verify production backup goes through cleanly. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D18834 --- .../PhabricatorStorageManagementDumpWorkflow.php | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementDumpWorkflow.php b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementDumpWorkflow.php index 88bd80a9a8..d5f1da9ecf 100644 --- a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementDumpWorkflow.php +++ b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementDumpWorkflow.php @@ -187,6 +187,22 @@ final class PhabricatorStorageManagementDumpWorkflow $argv[] = '-h'; $argv[] = $host; + // MySQL's default "max_allowed_packet" setting is fairly conservative + // (16MB). If we try to dump a row which is larger than this limit, the + // dump will fail. + + // We encourage users to increase this limit during setup, but modifying + // the "[mysqld]" section of the configuration file (instead of + // "[mysqldump]" section) won't apply to "mysqldump" and we can not easily + // detect what the "mysqldump" setting is. + + // Since no user would ever reasonably want a dump to fail because a row + // was too large, just manually force this setting to the largest supported + // value. + + $argv[] = '--max-allowed-packet'; + $argv[] = '1G'; + if ($port) { $argv[] = '--port'; $argv[] = $port; From e411d75964e51686ce95d9a9cc245193160f9f70 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 18 Dec 2017 06:26:38 -0800 Subject: [PATCH 2/7] Fix an issue where blame could fatal for unrecognized authors Summary: See PHI255. See . Test Plan: - Viewed a file contributed to by users with no Phabricator user accounts, in Diffusion. - Enabled blame. - Before patch: blame failed, fatal in logs. - After patch: blame worked. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D18833 --- .../controller/DiffusionBrowseController.php | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/applications/diffusion/controller/DiffusionBrowseController.php b/src/applications/diffusion/controller/DiffusionBrowseController.php index 77ec8e46ca..92181ff551 100644 --- a/src/applications/diffusion/controller/DiffusionBrowseController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseController.php @@ -1804,10 +1804,17 @@ final class DiffusionBrowseController extends DiffusionController { // revision. We just render a blank for alignment. $style = null; $href = null; + $sigil = null; + $meta = null; } else { $src = $handles[$phid]->getImageURI(); $style = 'background-image: url('.$src.');'; $href = $handles[$phid]->getURI(); + $sigil = 'has-tooltip'; + $meta = array( + 'tip' => $handles[$phid]->getName(), + 'align' => 'E', + ); } $links[$phid] = javelin_tag( @@ -1816,11 +1823,8 @@ final class DiffusionBrowseController extends DiffusionController { 'class' => 'diffusion-author-link', 'style' => $style, 'href' => $href, - 'sigil' => 'has-tooltip', - 'meta' => array( - 'tip' => $handles[$phid]->getName(), - 'align' => 'E', - ), + 'sigil' => $sigil, + 'meta' => $meta, )); } From 393824656fcc222b8e0e9a87614ca189794d8208 Mon Sep 17 00:00:00 2001 From: Dmitri Iouchtchenko Date: Thu, 21 Dec 2017 12:46:45 -0800 Subject: [PATCH 3/7] Don't notify without notifiable attendees Summary: Events with no attendees (e.g. fresh instances of recurring events) would trigger an exception when sending notifications, because `$attendee_map` would still get populated. Test Plan: Declined event, restarted daemons. Did not see exception. Accepted event, restarted daemons. Saw "[Calendar] [Reminder]" email. Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: Korvin, epriestley Differential Revision: https://secure.phabricator.com/D18835 --- .../notifications/PhabricatorCalendarNotificationEngine.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/applications/calendar/notifications/PhabricatorCalendarNotificationEngine.php b/src/applications/calendar/notifications/PhabricatorCalendarNotificationEngine.php index 4ae007fc0c..59d8476c87 100644 --- a/src/applications/calendar/notifications/PhabricatorCalendarNotificationEngine.php +++ b/src/applications/calendar/notifications/PhabricatorCalendarNotificationEngine.php @@ -100,10 +100,11 @@ final class PhabricatorCalendarNotificationEngine } $notifiable_phids[] = $invitee->getInviteePHID(); } - if (!$notifiable_phids) { + if ($notifiable_phids) { + $attendee_map[$key] = array_fuse($notifiable_phids); + } else { unset($events[$key]); } - $attendee_map[$key] = array_fuse($notifiable_phids); } if (!$attendee_map) { // None of the events have any notifiable attendees, so there is no From 84cf4938798473a4e0c370702fae56d24116052f Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 23 Dec 2017 08:36:58 -0800 Subject: [PATCH 4/7] Allow "Manage" to be the default menu item for projects Summary: Fixes T13033. Currently (prior to D18836) all the default items for projects can be hidden. When this occurs, the main project page fatals. If we fix the fatal narrowly (don't try to call `null->getBuiltinKey()` when `$default` is `null`), it would 404, which is a little better but not by much. After D18836 you can't hide "Profile", which is pretty sensible, and effectively fixes this. This change doubles down: let "Manage" be a default, and send the user there if we can't find a different default. Ideally, the MenuEngine itself will do more rendering eventually (as it does for some of the newer Home stuff) and could handle this defaulting behavior with less special casing, but we'd still end up in a similar situation if a project had only one "Link" item to "coolcats.com" or something: redirecting the user to "coolcats.com" is probably better than fataling, but not by a huge margin, and not likely to be what they expect. Test Plan: Before D18836, disabled both "Profile" and "Workboard" items on a project. Visited project page. Before patch: fatal. After patch: manage page. After D18836, you can't do this and just get the profile, so this is sort of moot and mostly future-proofing/for-completeness. Reviewers: 20after4, amckinley Reviewed By: amckinley Maniphest Tasks: T13033 Differential Revision: https://secure.phabricator.com/D18843 --- .../controller/PhabricatorProjectViewController.php | 11 +++++++++++ .../PhabricatorProjectManageProfileMenuItem.php | 5 +++++ 2 files changed, 16 insertions(+) diff --git a/src/applications/project/controller/PhabricatorProjectViewController.php b/src/applications/project/controller/PhabricatorProjectViewController.php index 7d5dc37e0b..2e53fe7276 100644 --- a/src/applications/project/controller/PhabricatorProjectViewController.php +++ b/src/applications/project/controller/PhabricatorProjectViewController.php @@ -20,6 +20,14 @@ final class PhabricatorProjectViewController $engine = $this->getProfileMenuEngine(); $default = $engine->getDefaultItem(); + // If defaults are broken somehow, serve the manage page. See T13033 for + // discussion. + if ($default) { + $default_key = $default->getBuiltinKey(); + } else { + $default_key = PhabricatorProject::ITEM_MANAGE; + } + switch ($default->getBuiltinKey()) { case PhabricatorProject::ITEM_WORKBOARD: $controller_object = new PhabricatorProjectBoardViewController(); @@ -27,6 +35,9 @@ final class PhabricatorProjectViewController case PhabricatorProject::ITEM_PROFILE: $controller_object = new PhabricatorProjectProfileController(); break; + case PhabricatorProject::ITEM_MANAGE: + $controller_object = new PhabricatorProjectManageController(); + break; default: return $engine->buildResponse(); } diff --git a/src/applications/project/menuitem/PhabricatorProjectManageProfileMenuItem.php b/src/applications/project/menuitem/PhabricatorProjectManageProfileMenuItem.php index ddb59ec095..1bd7e796dc 100644 --- a/src/applications/project/menuitem/PhabricatorProjectManageProfileMenuItem.php +++ b/src/applications/project/menuitem/PhabricatorProjectManageProfileMenuItem.php @@ -18,6 +18,11 @@ final class PhabricatorProjectManageProfileMenuItem return false; } + public function canMakeDefault( + PhabricatorProfileMenuItemConfiguration $config) { + return true; + } + public function getDisplayName( PhabricatorProfileMenuItemConfiguration $config) { $name = $config->getMenuItemProperty('name'); From bd5aa0c90fd1816830948a9e118a9e514145f813 Mon Sep 17 00:00:00 2001 From: Mukunda Modell Date: Sat, 23 Dec 2017 11:38:05 -0800 Subject: [PATCH 5/7] Prevent hiding the PhabricatorProjectDetailsProfileMenuItem Summary: This probably isn't the best solution, however, it conveniently avoids the bug from T13033. It would probably be more user-friendly (but more difficult to implement) if we allowed either Project Details //or// Workboard to be hidden but not both. Test Plan: Tested locally, indeed this prevents hiding the menu item. Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D18836 --- .../menuitem/PhabricatorProjectDetailsProfileMenuItem.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/applications/project/menuitem/PhabricatorProjectDetailsProfileMenuItem.php b/src/applications/project/menuitem/PhabricatorProjectDetailsProfileMenuItem.php index 96619b8ec5..b779f4be90 100644 --- a/src/applications/project/menuitem/PhabricatorProjectDetailsProfileMenuItem.php +++ b/src/applications/project/menuitem/PhabricatorProjectDetailsProfileMenuItem.php @@ -13,6 +13,11 @@ final class PhabricatorProjectDetailsProfileMenuItem return pht('Project Details'); } + public function canHideMenuItem( + PhabricatorProfileMenuItemConfiguration $config) { + return false; + } + public function canMakeDefault( PhabricatorProfileMenuItemConfiguration $config) { return true; From 66b74073be68007ae918e4e4115df8b9b8c68fe8 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 22 Dec 2017 09:59:43 -0800 Subject: [PATCH 6/7] Provide ANSI color information for Harbormaster build status via API Summary: Ref PHI261. This moves Harbormaster build status to work more similarly to other modern status types, like Differential revision status, where we try to specify as much behavior on the server as possible so that the client and server can vary independently. (I don't have any specific plans to make Harbormaster build status configurable on the server side, but it isn't out of the question.) Test Plan: Ran `harbormaster.querybuilds` (saw same data as before) and `harbormaster.build.search` (saw same data as before, plus new ANSI color data). Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Differential Revision: https://secure.phabricator.com/D18838 --- ...arbormasterQueryBuildsConduitAPIMethod.php | 10 ++ .../constants/HarbormasterBuildStatus.php | 128 +++++++++++------- .../storage/build/HarbormasterBuild.php | 2 + 3 files changed, 88 insertions(+), 52 deletions(-) diff --git a/src/applications/harbormaster/conduit/HarbormasterQueryBuildsConduitAPIMethod.php b/src/applications/harbormaster/conduit/HarbormasterQueryBuildsConduitAPIMethod.php index a76f53a362..c4622a902c 100644 --- a/src/applications/harbormaster/conduit/HarbormasterQueryBuildsConduitAPIMethod.php +++ b/src/applications/harbormaster/conduit/HarbormasterQueryBuildsConduitAPIMethod.php @@ -65,6 +65,16 @@ final class HarbormasterQueryBuildsConduitAPIMethod $fields = idx($build_data, 'fields', array()); unset($build_data['fields']); unset($build_data['attachments']); + + // To retain backward compatibility, remove newer keys from the + // result array. + $fields['buildStatus'] = array_select_keys( + $fields['buildStatus'], + array( + 'value', + 'name', + )); + $data[] = array_mergev(array($build_data, $querybuilds, $fields)); } diff --git a/src/applications/harbormaster/constants/HarbormasterBuildStatus.php b/src/applications/harbormaster/constants/HarbormasterBuildStatus.php index a2cdba5b63..b0e3105d78 100644 --- a/src/applications/harbormaster/constants/HarbormasterBuildStatus.php +++ b/src/applications/harbormaster/constants/HarbormasterBuildStatus.php @@ -55,67 +55,28 @@ final class HarbormasterBuildStatus extends Phobject { * @return string Human-readable name. */ public static function getBuildStatusName($status) { - $map = self::getBuildStatusMap(); - return idx($map, $status, pht('Unknown ("%s")', $status)); + $spec = self::getBuildStatusSpec($status); + return idx($spec, 'name', pht('Unknown ("%s")', $status)); } public static function getBuildStatusMap() { - return array( - self::STATUS_INACTIVE => pht('Inactive'), - self::STATUS_PENDING => pht('Pending'), - self::STATUS_BUILDING => pht('Building'), - self::STATUS_PASSED => pht('Passed'), - self::STATUS_FAILED => pht('Failed'), - self::STATUS_ABORTED => pht('Aborted'), - self::STATUS_ERROR => pht('Unexpected Error'), - self::STATUS_PAUSED => pht('Paused'), - self::STATUS_DEADLOCKED => pht('Deadlocked'), - ); + $specs = self::getBuildStatusSpecMap(); + return ipull($specs, 'name'); } public static function getBuildStatusIcon($status) { - switch ($status) { - case self::STATUS_INACTIVE: - case self::STATUS_PENDING: - return PHUIStatusItemView::ICON_OPEN; - case self::STATUS_BUILDING: - return PHUIStatusItemView::ICON_RIGHT; - case self::STATUS_PASSED: - return PHUIStatusItemView::ICON_ACCEPT; - case self::STATUS_FAILED: - return PHUIStatusItemView::ICON_REJECT; - case self::STATUS_ABORTED: - return PHUIStatusItemView::ICON_MINUS; - case self::STATUS_ERROR: - return PHUIStatusItemView::ICON_MINUS; - case self::STATUS_PAUSED: - return PHUIStatusItemView::ICON_MINUS; - case self::STATUS_DEADLOCKED: - return PHUIStatusItemView::ICON_WARNING; - default: - return PHUIStatusItemView::ICON_QUESTION; - } + $spec = self::getBuildStatusSpec($status); + return idx($spec, 'icon', 'fa-question-circle'); } public static function getBuildStatusColor($status) { - switch ($status) { - case self::STATUS_INACTIVE: - return 'dark'; - case self::STATUS_PENDING: - case self::STATUS_BUILDING: - return 'blue'; - case self::STATUS_PASSED: - return 'green'; - case self::STATUS_FAILED: - case self::STATUS_ABORTED: - case self::STATUS_ERROR: - case self::STATUS_DEADLOCKED: - return 'red'; - case self::STATUS_PAUSED: - return 'dark'; - default: - return 'bluegrey'; - } + $spec = self::getBuildStatusSpec($status); + return idx($spec, 'color', 'bluegrey'); + } + + public static function getBuildStatusANSIColor($status) { + $spec = self::getBuildStatusSpec($status); + return idx($spec, 'color.ansi', 'magenta'); } public static function getWaitingStatusConstants() { @@ -142,4 +103,67 @@ final class HarbormasterBuildStatus extends Phobject { ); } + private static function getBuildStatusSpecMap() { + return array( + self::STATUS_INACTIVE => array( + 'name' => pht('Inactive'), + 'icon' => 'fa-circle-o', + 'color' => 'dark', + 'color.ansi' => 'yellow', + ), + self::STATUS_PENDING => array( + 'name' => pht('Pending'), + 'icon' => 'fa-circle-o', + 'color' => 'blue', + 'color.ansi' => 'yellow', + ), + self::STATUS_BUILDING => array( + 'name' => pht('Building'), + 'icon' => 'fa-chevron-circle-right', + 'color' => 'blue', + 'color.ansi' => 'yellow', + ), + self::STATUS_PASSED => array( + 'name' => pht('Passed'), + 'icon' => 'fa-check-circle', + 'color' => 'green', + 'color.ansi' => 'green', + ), + self::STATUS_FAILED => array( + 'name' => pht('Failed'), + 'icon' => 'fa-times-circle', + 'color' => 'red', + 'color.ansi' => 'red', + ), + self::STATUS_ABORTED => array( + 'name' => pht('Aborted'), + 'icon' => 'fa-minus-circle', + 'color' => 'red', + 'color.ansi' => 'red', + ), + self::STATUS_ERROR => array( + 'name' => pht('Unexpected Error'), + 'icon' => 'fa-minus-circle', + 'color' => 'red', + 'color.ansi' => 'red', + ), + self::STATUS_PAUSED => array( + 'name' => pht('Paused'), + 'icon' => 'fa-minus-circle', + 'color' => 'dark', + 'color.ansi' => 'yellow', + ), + self::STATUS_DEADLOCKED => array( + 'name' => pht('Deadlocked'), + 'icon' => 'fa-exclamation-circle', + 'color' => 'red', + 'color.ansi' => 'red', + ), + ); + } + + private static function getBuildStatusSpec($status) { + return idx(self::getBuildStatusSpecMap(), $status, array()); + } + } diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuild.php b/src/applications/harbormaster/storage/build/HarbormasterBuild.php index 958eaa1f2b..92d4293913 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuild.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuild.php @@ -435,6 +435,8 @@ final class HarbormasterBuild extends HarbormasterDAO 'buildStatus' => array( 'value' => $status, 'name' => HarbormasterBuildStatus::getBuildStatusName($status), + 'color.ansi' => + HarbormasterBuildStatus::getBuildStatusANSIColor($status), ), 'initiatorPHID' => nonempty($this->getInitiatorPHID(), null), 'name' => $this->getName(), From ad4db9b2f3f83ab2506de9897d159917454cad5b Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 22 Dec 2017 11:55:39 -0800 Subject: [PATCH 7/7] Separate "Set/Reset Password" from "Change Password" Summary: See PHI223. Ref T13024. There's a remaining registration/login order issue after the other changes in T13024: we lose track of the current URI when we go through the MFA flow, so we can lose "Set Password" at the end of the flow. Specifically, the flow goes like this today: - User clicks the welcome link in email. - They get redirected to the "set password" settings panel. - This gets pre-empted by Legalpad (although we'll potentially survive this with the URI intact). - This also gets pre-empted by the "Set MFA" workflow. If the user completes this flow, they get redirected to a `/auth/multifactor/?id=123` sort of URI to highlight the factor they added. This causes us to lose the `/settings/panel/password/blah/blah?key=xyz` URI. The ordering on this is also not ideal; it's preferable to start with a password, then do the other steps, so the user can return to the flow more easily if they are interrupted. Resolve this by separating the "change your password" and "set/reset your password" flows onto two different pages. This copy/pastes a bit of code, but both flows end up simpler so it feels reasonable to me overall. We don't require a full session for "set/reset password" (so you can do it if you don't have MFA/legalpad yet) and do it first. This works better and is broadly simpler for users. Test Plan: - Required MFA + legalpad, invited a user via email, registered. - Before: password set flow got lost when setting MFA. - After: prompted to set password, then sign documents, then set up MFA. - Reset password (with MFA confgiured, was required to MFA first). - Tried to reset password without a valid reset key, wasn't successful. - Changed password using existing flow. - Hit various (all?) error cases (short password, common password, mismatch, missing password, etc). Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13024 Differential Revision: https://secure.phabricator.com/D18840 --- src/__phutil_library_map__.php | 2 + .../PhabricatorAuthApplication.php | 1 + .../PhabricatorAuthOneTimeLoginController.php | 3 +- .../PhabricatorAuthSetPasswordController.php | 155 ++++++++++++++++++ .../people/storage/PhabricatorUser.php | 4 + .../PhabricatorPasswordSettingsPanel.php | 79 +++------ 6 files changed, 185 insertions(+), 59 deletions(-) create mode 100644 src/applications/auth/controller/PhabricatorAuthSetPasswordController.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index f86b3a1d2d..f031f7deca 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2112,6 +2112,7 @@ phutil_register_library_map(array( 'PhabricatorAuthSessionGarbageCollector' => 'applications/auth/garbagecollector/PhabricatorAuthSessionGarbageCollector.php', 'PhabricatorAuthSessionInfo' => 'applications/auth/data/PhabricatorAuthSessionInfo.php', 'PhabricatorAuthSessionQuery' => 'applications/auth/query/PhabricatorAuthSessionQuery.php', + 'PhabricatorAuthSetPasswordController' => 'applications/auth/controller/PhabricatorAuthSetPasswordController.php', 'PhabricatorAuthSetupCheck' => 'applications/config/check/PhabricatorAuthSetupCheck.php', 'PhabricatorAuthStartController' => 'applications/auth/controller/PhabricatorAuthStartController.php', 'PhabricatorAuthTOTPKeyTemporaryTokenType' => 'applications/auth/factor/PhabricatorAuthTOTPKeyTemporaryTokenType.php', @@ -7377,6 +7378,7 @@ phutil_register_library_map(array( 'PhabricatorAuthSessionGarbageCollector' => 'PhabricatorGarbageCollector', 'PhabricatorAuthSessionInfo' => 'Phobject', 'PhabricatorAuthSessionQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', + 'PhabricatorAuthSetPasswordController' => 'PhabricatorAuthController', 'PhabricatorAuthSetupCheck' => 'PhabricatorSetupCheck', 'PhabricatorAuthStartController' => 'PhabricatorAuthController', 'PhabricatorAuthTOTPKeyTemporaryTokenType' => 'PhabricatorAuthTemporaryTokenType', diff --git a/src/applications/auth/application/PhabricatorAuthApplication.php b/src/applications/auth/application/PhabricatorAuthApplication.php index dfc855f315..a6127394fd 100644 --- a/src/applications/auth/application/PhabricatorAuthApplication.php +++ b/src/applications/auth/application/PhabricatorAuthApplication.php @@ -84,6 +84,7 @@ final class PhabricatorAuthApplication extends PhabricatorApplication { => 'PhabricatorAuthSSHKeyDeactivateController', 'view/(?P\d+)/' => 'PhabricatorAuthSSHKeyViewController', ), + 'password/' => 'PhabricatorAuthSetPasswordController', ), '/oauth/(?P\w+)/login/' diff --git a/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php b/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php index 534bda3f35..9f74d50765 100644 --- a/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php +++ b/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php @@ -139,8 +139,7 @@ final class PhabricatorAuthOneTimeLoginController ->save(); unset($unguarded); - $username = $target_user->getUsername(); - $panel_uri = "/settings/user/{$username}/page/password/"; + $panel_uri = '/auth/password/'; $next = (string)id(new PhutilURI($panel_uri)) ->setQueryParams( diff --git a/src/applications/auth/controller/PhabricatorAuthSetPasswordController.php b/src/applications/auth/controller/PhabricatorAuthSetPasswordController.php new file mode 100644 index 0000000000..0db23e52a7 --- /dev/null +++ b/src/applications/auth/controller/PhabricatorAuthSetPasswordController.php @@ -0,0 +1,155 @@ +getViewer(); + + if (!PhabricatorPasswordAuthProvider::getPasswordProvider()) { + return new Aphront404Response(); + } + + $token = id(new PhabricatorAuthSessionEngine())->requireHighSecuritySession( + $viewer, + $request, + '/'); + + $key = $request->getStr('key'); + $password_type = PhabricatorAuthPasswordResetTemporaryTokenType::TOKENTYPE; + if (!$key) { + return new Aphront404Response(); + } + + $auth_token = id(new PhabricatorAuthTemporaryTokenQuery()) + ->setViewer($viewer) + ->withTokenResources(array($viewer->getPHID())) + ->withTokenTypes(array($password_type)) + ->withTokenCodes(array(PhabricatorHash::weakDigest($key))) + ->withExpired(false) + ->executeOne(); + if (!$auth_token) { + return new Aphront404Response(); + } + + $min_len = PhabricatorEnv::getEnvConfig('account.minimum-password-length'); + $min_len = (int)$min_len; + + $e_password = true; + $e_confirm = true; + $errors = array(); + if ($request->isFormPost()) { + $password = $request->getStr('password'); + $confirm = $request->getStr('confirm'); + + $e_password = null; + $e_confirm = null; + + if (!strlen($password)) { + $errors[] = pht('You must choose a password or skip this step.'); + $e_password = pht('Required'); + } else if (strlen($password) < $min_len) { + $errors[] = pht( + 'The selected password is too short. Passwords must be a minimum '. + 'of %s characters.', + new PhutilNumber($min_len)); + $e_password = pht('Too Short'); + } else if (!strlen($confirm)) { + $errors[] = pht('You must confirm the selecetd password.'); + $e_confirm = pht('Required'); + } else if ($password !== $confirm) { + $errors[] = pht('The password and confirmation do not match.'); + $e_password = pht('Invalid'); + $e_confirm = pht('Invalid'); + } else if (PhabricatorCommonPasswords::isCommonPassword($password)) { + $e_password = pht('Very Weak'); + $errors[] = pht( + 'The selected password is very weak: it is one of the most common '. + 'passwords in use. Choose a stronger password.'); + } + + if (!$errors) { + $envelope = new PhutilOpaqueEnvelope($password); + + // 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(); + + id(new PhabricatorUserEditor()) + ->setActor($viewer) + ->changePassword($viewer, $envelope); + + unset($unguarded); + + // Destroy the token. + $auth_token->delete(); + + return id(new AphrontRedirectResponse())->setURI('/'); + } + } + + $len_caption = null; + if ($min_len) { + $len_caption = pht('Minimum password length: %d characters.', $min_len); + } + + if ($viewer->hasPassword()) { + $title = pht('Reset Password'); + $crumb = pht('Reset Password'); + $submit = pht('Reset Password'); + } else { + $title = pht('Set Password'); + $crumb = pht('Set Password'); + $submit = pht('Set Account Password'); + } + + $form = id(new AphrontFormView()) + ->setViewer($viewer) + ->addHiddenInput('key', $key) + ->appendChild( + id(new AphrontFormPasswordControl()) + ->setDisableAutocomplete(true) + ->setLabel(pht('New Password')) + ->setError($e_password) + ->setName('password')) + ->appendChild( + id(new AphrontFormPasswordControl()) + ->setDisableAutocomplete(true) + ->setLabel(pht('Confirm Password')) + ->setCaption($len_caption) + ->setError($e_confirm) + ->setName('confirm')) + ->appendChild( + id(new AphrontFormSubmitControl()) + ->addCancelButton('/', pht('Skip This Step')) + ->setValue($submit)); + + $form_box = id(new PHUIObjectBoxView()) + ->setHeaderText($title) + ->setFormErrors($errors) + ->setBackground(PHUIObjectBoxView::WHITE_CONFIG) + ->setForm($form); + + $main_view = id(new PHUITwoColumnView()) + ->setFooter($form_box); + + $crumbs = $this->buildApplicationCrumbs() + ->addTextCrumb($crumb) + ->setBorder(true); + + return $this->newPage() + ->setTitle($title) + ->setCrumbs($crumbs) + ->appendChild($main_view); + } +} diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php index 1745154826..30aa3d81ef 100644 --- a/src/applications/people/storage/PhabricatorUser.php +++ b/src/applications/people/storage/PhabricatorUser.php @@ -262,6 +262,10 @@ final class PhabricatorUser PhabricatorPeopleUserPHIDType::TYPECONST); } + public function hasPassword() { + return (bool)strlen($this->passwordHash); + } + public function setPassword(PhutilOpaqueEnvelope $envelope) { if (!$this->getPHID()) { throw new Exception( diff --git a/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php b/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php index c1250fca23..3807139fe3 100644 --- a/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php +++ b/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php @@ -35,23 +35,10 @@ final class PhabricatorPasswordSettingsPanel extends PhabricatorSettingsPanel { $min_len = PhabricatorEnv::getEnvConfig('account.minimum-password-length'); $min_len = (int)$min_len; - // NOTE: To change your password, you need to prove you own the account, - // either by providing the old password or by carrying a token to - // the workflow from a password reset email. - - $key = $request->getStr('key'); - $password_type = PhabricatorAuthPasswordResetTemporaryTokenType::TOKENTYPE; - - $token = null; - if ($key) { - $token = id(new PhabricatorAuthTemporaryTokenQuery()) - ->setViewer($user) - ->withTokenResources(array($user->getPHID())) - ->withTokenTypes(array($password_type)) - ->withTokenCodes(array(PhabricatorHash::weakDigest($key))) - ->withExpired(false) - ->executeOne(); - } + // NOTE: Users can also change passwords through the separate "set/reset" + // interface which is reached by logging in with a one-time token after + // registration or password reset. If this flow changes, that flow may + // also need to change. $e_old = true; $e_new = true; @@ -59,12 +46,10 @@ final class PhabricatorPasswordSettingsPanel extends PhabricatorSettingsPanel { $errors = array(); if ($request->isFormPost()) { - if (!$token) { - $envelope = new PhutilOpaqueEnvelope($request->getStr('old_pw')); - if (!$user->comparePassword($envelope)) { - $errors[] = pht('The old password you entered is incorrect.'); - $e_old = pht('Invalid'); - } + $envelope = new PhutilOpaqueEnvelope($request->getStr('old_pw')); + if (!$user->comparePassword($envelope)) { + $errors[] = pht('The old password you entered is incorrect.'); + $e_old = pht('Invalid'); } $pass = $request->getStr('new_pw'); @@ -98,16 +83,7 @@ final class PhabricatorPasswordSettingsPanel extends PhabricatorSettingsPanel { unset($unguarded); - if ($token) { - // Destroy the token. - $token->delete(); - - // If this is a password set/reset, kick the user to the home page - // after we update their account. - $next = '/'; - } else { - $next = $this->getPanelURI('?saved=true'); - } + $next = $this->getPanelURI('?saved=true'); id(new PhabricatorAuthSessionEngine())->terminateLoginSessions( $user, @@ -125,19 +101,15 @@ final class PhabricatorPasswordSettingsPanel extends PhabricatorSettingsPanel { } catch (PhabricatorPasswordHasherUnavailableException $ex) { $can_upgrade = false; - // Only show this stuff if we aren't on the reset workflow. We can - // do resets regardless of the old hasher's availability. - if (!$token) { - $errors[] = pht( - 'Your password is currently hashed using an algorithm which is '. - 'no longer available on this install.'); - $errors[] = pht( - 'Because the algorithm implementation is missing, your password '. - 'can not be used or updated.'); - $errors[] = pht( - 'To set a new password, request a password reset link from the '. - 'login screen and then follow the instructions.'); - } + $errors[] = pht( + 'Your password is currently hashed using an algorithm which is '. + 'no longer available on this install.'); + $errors[] = pht( + 'Because the algorithm implementation is missing, your password '. + 'can not be used or updated.'); + $errors[] = pht( + 'To set a new password, request a password reset link from the '. + 'login screen and then follow the instructions.'); } if ($can_upgrade) { @@ -153,20 +125,13 @@ final class PhabricatorPasswordSettingsPanel extends PhabricatorSettingsPanel { $len_caption = pht('Minimum password length: %d characters.', $min_len); } - $form = new AphrontFormView(); - $form - ->setUser($user) - ->addHiddenInput('key', $key); - - if (!$token) { - $form->appendChild( + $form = id(new AphrontFormView()) + ->setViewer($user) + ->appendChild( id(new AphrontFormPasswordControl()) ->setLabel(pht('Old Password')) ->setError($e_old) - ->setName('old_pw')); - } - - $form + ->setName('old_pw')) ->appendChild( id(new AphrontFormPasswordControl()) ->setDisableAutocomplete(true)