From cf3adcb758c3a9497a3f7f92baf35011777d9891 Mon Sep 17 00:00:00 2001 From: Dillon-Brown Date: Fri, 18 Jan 2019 11:57:35 +0000 Subject: [PATCH 1/7] Fixed #5414 - Password is not saving --- modules/Users/User.php | 53 ++++++++++++++++++++++++++---------------- 1 file changed, 33 insertions(+), 20 deletions(-) diff --git a/modules/Users/User.php b/modules/Users/User.php index e8a0fe01d74..511fd01c014 100755 --- a/modules/Users/User.php +++ b/modules/Users/User.php @@ -5,7 +5,7 @@ * SugarCRM, Inc. Copyright (C) 2004-2013 SugarCRM Inc. * * SuiteCRM is an extension to SugarCRM Community Edition developed by SalesAgility Ltd. - * Copyright (C) 2011 - 2018 SalesAgility Ltd. + * Copyright (C) 2011 - 2019 SalesAgility Ltd. * * This program is free software; you can redistribute it and/or modify it under * the terms of the GNU Affero General Public License version 3 as published by the @@ -587,26 +587,24 @@ public static function getLicensedUsersWhere() } /** - * Normally a bean returns ID from save() method if it was + * Normally a bean returns ID from save() method if it was * success and false (or maybe null) is something went wrong. - * BUT (for some reason) if User bean saved properly except - * the email addresses of it, this User::save() method also + * BUT (for some reason) if User bean saved properly except + * the email addresses of it, this User::save() method also * return a false. - * It's a confusing ambiguous return value for caller method. - * - * To handle this issue when save method can not save email + * It's a confusing ambiguous return value for caller method. + * + * To handle this issue when save method can not save email * addresses and return false it also set the variable called * User::$lastSaveErrorIsEmailAddressSaveError to true. - * - * @global User $current_user - * @global array $sugar_config - * @global array $mod_strings + * * @param bool $check_notify - * @return boolean + * @return bool|string + * @throws SuiteException */ public function save($check_notify = false) { - global $current_user, $sugar_config, $mod_strings; + global $current_user, $mod_strings; $msg = ''; @@ -643,8 +641,6 @@ public function save($check_notify = false) } if ($this->factor_auth && $isUpdate && is_admin($current_user)) { - $tmpUser = BeanFactory::getBean('Users', $this->id); - $factorAuthFactory = new FactorAuthFactory(); $factorAuth = $factorAuthFactory->getFactorAuth($this); @@ -653,10 +649,6 @@ public function save($check_notify = false) } } - - $query = "SELECT count(id) as total from users WHERE " . self::getLicensedUsersWhere(); - - // is_group & portal should be set to 0 by default if (!isset($this->is_group)) { $this->is_group = 0; @@ -678,12 +670,33 @@ public function save($check_notify = false) // set some default preferences when creating a new user $setNewUserPreferences = empty($this->id) || !empty($this->new_with_id); + if (!$this->verify_data()) { + SugarApplication::appendErrorMessage($this->error_string); + header('Location: index.php?action=Error&module=Users'); + exit; + } + + if ((isset($_POST['old_password']) || $this->portal_only) && + (isset($_POST['new_password']) && !empty($_POST['new_password'])) && + (isset($_POST['password_change']) && $_POST['password_change'] === 'true') && + (!$this->change_password($_POST['old_password'], $_POST['new_password']))) { + if (isset($_POST['page']) && $_POST['page'] === 'EditView') { + SugarApplication::appendErrorMessage($this->error_string); + header("Location: index.php?action=EditView&module=Users&record=" . $_POST['record']); + exit; + } + if (isset($_POST['page']) && $_POST['page'] === 'Change') { + SugarApplication::appendErrorMessage($this->error_string); + header("Location: index.php?action=ChangePassword&module=Users&record=" . $_POST['record']); + exit; + } + } $retId = parent::save($check_notify); if (!$retId) { LoggerManager::getLogger()->fatal('save error: User is not saved, Person ID is not returned.'); } - if ($retId != $this->id) { + if ($retId !== $this->id) { LoggerManager::getLogger()->fatal('save error: User is not saved properly, returned Person ID does not match to User ID.'); } // set some default preferences when creating a new user From a9b6e0d99e45fee23c83dd782cd7354df03ee0f8 Mon Sep 17 00:00:00 2001 From: Dillon-Brown Date: Fri, 18 Jan 2019 12:49:32 +0000 Subject: [PATCH 2/7] Fix user save --- modules/Users/User.php | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/modules/Users/User.php b/modules/Users/User.php index 511fd01c014..4c914125f75 100755 --- a/modules/Users/User.php +++ b/modules/Users/User.php @@ -678,17 +678,18 @@ public function save($check_notify = false) if ((isset($_POST['old_password']) || $this->portal_only) && (isset($_POST['new_password']) && !empty($_POST['new_password'])) && - (isset($_POST['password_change']) && $_POST['password_change'] === 'true') && - (!$this->change_password($_POST['old_password'], $_POST['new_password']))) { - if (isset($_POST['page']) && $_POST['page'] === 'EditView') { - SugarApplication::appendErrorMessage($this->error_string); - header("Location: index.php?action=EditView&module=Users&record=" . $_POST['record']); - exit; - } - if (isset($_POST['page']) && $_POST['page'] === 'Change') { - SugarApplication::appendErrorMessage($this->error_string); - header("Location: index.php?action=ChangePassword&module=Users&record=" . $_POST['record']); - exit; + (isset($_POST['password_change']) && $_POST['password_change'] === 'true')) { + if (!$this->change_password($_POST['old_password'], $_POST['new_password'])) { + if (isset($_POST['page']) && $_POST['page'] === 'EditView') { + SugarApplication::appendErrorMessage($this->error_string); + header("Location: index.php?action=EditView&module=Users&record=" . $_POST['record']); + exit; + } + if (isset($_POST['page']) && $_POST['page'] === 'Change') { + SugarApplication::appendErrorMessage($this->error_string); + header("Location: index.php?action=ChangePassword&module=Users&record=" . $_POST['record']); + exit; + } } } From c9e3173c1cdd87bc4f70143d4dc676c652dfc7d5 Mon Sep 17 00:00:00 2001 From: Dillon-Brown Date: Fri, 18 Jan 2019 13:56:18 +0000 Subject: [PATCH 3/7] Fix user save --- modules/Users/User.php | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/modules/Users/User.php b/modules/Users/User.php index 4c914125f75..ef134917f54 100755 --- a/modules/Users/User.php +++ b/modules/Users/User.php @@ -676,22 +676,6 @@ public function save($check_notify = false) exit; } - if ((isset($_POST['old_password']) || $this->portal_only) && - (isset($_POST['new_password']) && !empty($_POST['new_password'])) && - (isset($_POST['password_change']) && $_POST['password_change'] === 'true')) { - if (!$this->change_password($_POST['old_password'], $_POST['new_password'])) { - if (isset($_POST['page']) && $_POST['page'] === 'EditView') { - SugarApplication::appendErrorMessage($this->error_string); - header("Location: index.php?action=EditView&module=Users&record=" . $_POST['record']); - exit; - } - if (isset($_POST['page']) && $_POST['page'] === 'Change') { - SugarApplication::appendErrorMessage($this->error_string); - header("Location: index.php?action=ChangePassword&module=Users&record=" . $_POST['record']); - exit; - } - } - } $retId = parent::save($check_notify); if (!$retId) { @@ -711,6 +695,23 @@ public function save($check_notify = false) $this->savePreferencesToDB(); + if ((isset($_POST['old_password']) || $this->portal_only) && + (isset($_POST['new_password']) && !empty($_POST['new_password'])) && + (isset($_POST['password_change']) && $_POST['password_change'] === 'true')) { + if (!$this->change_password($_POST['old_password'], $_POST['new_password'])) { + if (isset($_POST['page']) && $_POST['page'] === 'EditView') { + SugarApplication::appendErrorMessage($this->error_string); + header("Location: index.php?action=EditView&module=Users&record=" . $_POST['record']); + exit; + } + if (isset($_POST['page']) && $_POST['page'] === 'Change') { + SugarApplication::appendErrorMessage($this->error_string); + header("Location: index.php?action=ChangePassword&module=Users&record=" . $_POST['record']); + exit; + } + } + } + // User Profile specific save for Email addresses $this->lastSaveErrorIsEmailAddressSaveError = false; if (!$this->emailAddress->saveAtUserProfile($_REQUEST)) { From 712c34be3cf423b5181e9ba5e1b002973883bddc Mon Sep 17 00:00:00 2001 From: Dillon-Brown Date: Mon, 21 Jan 2019 11:06:34 +0000 Subject: [PATCH 4/7] User creation acceptance test --- .../_support/Step/Acceptance/UsersTester.php | 19 +++++++ tests/acceptance/modules/Users/UsersCest.php | 55 +++++++++++++++++++ 2 files changed, 74 insertions(+) diff --git a/tests/_support/Step/Acceptance/UsersTester.php b/tests/_support/Step/Acceptance/UsersTester.php index 12d2b1904ca..0ed1b82ec68 100644 --- a/tests/_support/Step/Acceptance/UsersTester.php +++ b/tests/_support/Step/Acceptance/UsersTester.php @@ -12,4 +12,23 @@ public function gotoProfile() $I = new NavigationBarTester($this->getScenario()); $I->clickUserMenuItem('Profile'); } + + /** + * Navigate to users module + */ + public function gotoUsers() + { + $I = new NavigationBarTester($this->getScenario()); + $I->clickUserMenuItem('Admin'); + $I->see('ADMINISTRATION'); + } + + /** + * Logout a user + */ + public function logoutUser() + { + $I = new NavigationBarTester($this->getScenario()); + $I->clickUserMenuItem('#logout_link'); + } } diff --git a/tests/acceptance/modules/Users/UsersCest.php b/tests/acceptance/modules/Users/UsersCest.php index 8d52d3f9597..625656e750b 100644 --- a/tests/acceptance/modules/Users/UsersCest.php +++ b/tests/acceptance/modules/Users/UsersCest.php @@ -33,6 +33,61 @@ public function _before(AcceptanceTester $I) $this->fakeDataSeed = rand(0, 2048); $this->fakeData->seed($this->fakeDataSeed); } + + + /** + * @param \AcceptanceTester $I + * @param \Step\Acceptance\ListView $listView + * @param \Step\Acceptance\EditView $editView + * @param \Step\Acceptance\UsersTester $users + * @param \Step\Acceptance\SideBar $sidebar + * @param \Helper\WebDriverHelper $webDriverHelper + * + * As an administrator I want to create a user. + */ + public function testScenarioCreateUser( + \AcceptanceTester $I, + \Step\Acceptance\ListView $listView, + \Step\Acceptance\EditView $editView, + \Step\Acceptance\UsersTester $users, + \Step\Acceptance\SideBar $sidebar, + \Helper\WebDriverHelper $webDriverHelper + ) { + $I->wantTo('Create a user'); + + $I->amOnUrl( + $webDriverHelper->getInstanceURL() + ); + + $this->fakeData->seed($this->fakeDataSeed); + $userName = 'Test_'. $this->fakeData->name; + + // Navigate to users list-view + $I->loginAsAdmin(); + $users->gotoUsers(); + $I->click('User Management'); + $listView->waitForListViewVisible(); + + // Create user + $sidebar->clickSideBarAction('Create'); + $editView->waitForEditViewVisible(); + $I->fillField('user_name', $userName); + $I->fillField('last_name', $this->fakeData->name); + $I->scrollTo('#Users0emailAddress0'); + $I->fillField('#Users0emailAddress0', $this->fakeData->email); + $I->executeJS('window.scrollTo(0,0); return true;'); + $I->click('Password'); + $I->fillField('#new_password', 'Test'); + $I->fillField('#confirm_pwd', 'Test'); + $editView->clickSaveButton(); + + // Logout and Login + $users->logoutUser(); + $I->fillField('#user_name', $userName); + $I->fillField('#username_password', 'Test'); + $I->click('Log In'); + $I->waitForElementNotVisible('#loginform', 120); + } public function testEmailSettingsMailAccountAdd(AcceptanceTester $I, UsersTester $Users, WebDriverHelper $webDriverHelper) { From 8e42562afe049f4435662efe3cdb40a207bb3330 Mon Sep 17 00:00:00 2001 From: Dillon-Brown Date: Mon, 21 Jan 2019 12:39:08 +0000 Subject: [PATCH 5/7] Logout user after test --- tests/acceptance/modules/Users/UsersCest.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/acceptance/modules/Users/UsersCest.php b/tests/acceptance/modules/Users/UsersCest.php index 625656e750b..dc51e2933cf 100644 --- a/tests/acceptance/modules/Users/UsersCest.php +++ b/tests/acceptance/modules/Users/UsersCest.php @@ -87,6 +87,12 @@ public function testScenarioCreateUser( $I->fillField('#username_password', 'Test'); $I->click('Log In'); $I->waitForElementNotVisible('#loginform', 120); + + // Logout + $I->amOnUrl( + $webDriverHelper->getInstanceURL() + ); + $users->logoutUser(); } public function testEmailSettingsMailAccountAdd(AcceptanceTester $I, UsersTester $Users, WebDriverHelper $webDriverHelper) From f823a6f4e16b7ddc1e085d192d51ea11a0587bab Mon Sep 17 00:00:00 2001 From: Dillon-Brown Date: Fri, 25 Jan 2019 13:34:37 +0000 Subject: [PATCH 6/7] Login --- tests/acceptance/modules/Users/UsersCest.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/acceptance/modules/Users/UsersCest.php b/tests/acceptance/modules/Users/UsersCest.php index dc51e2933cf..8b51e05dde2 100644 --- a/tests/acceptance/modules/Users/UsersCest.php +++ b/tests/acceptance/modules/Users/UsersCest.php @@ -88,11 +88,15 @@ public function testScenarioCreateUser( $I->click('Log In'); $I->waitForElementNotVisible('#loginform', 120); - // Logout $I->amOnUrl( $webDriverHelper->getInstanceURL() ); $users->logoutUser(); + $I->waitForElementVisible('#loginform'); + $I->fillField('#user_name', $I->getAdminUser()); + $I->fillField('#username_password', $I->getAdminPassword()); + $I->click('Log In'); + $I->waitForElementNotVisible('#loginform', 120); } public function testEmailSettingsMailAccountAdd(AcceptanceTester $I, UsersTester $Users, WebDriverHelper $webDriverHelper) From 496b20b2e643105cd49380fc2a4c564aac0fffdd Mon Sep 17 00:00:00 2001 From: Dillon Brown <26431166+Dillon-Brown@users.noreply.github.com> Date: Fri, 25 Jan 2019 15:52:55 +0000 Subject: [PATCH 7/7] Update UsersCest.php --- tests/acceptance/modules/Users/UsersCest.php | 64 -------------------- 1 file changed, 64 deletions(-) diff --git a/tests/acceptance/modules/Users/UsersCest.php b/tests/acceptance/modules/Users/UsersCest.php index a91b39ee279..cf5210ebef5 100644 --- a/tests/acceptance/modules/Users/UsersCest.php +++ b/tests/acceptance/modules/Users/UsersCest.php @@ -33,70 +33,6 @@ public function _before(AcceptanceTester $I) $this->fakeDataSeed = rand(0, 2048); $this->fakeData->seed($this->fakeDataSeed); } - - /** - * @param \AcceptanceTester $I - * @param \Step\Acceptance\ListView $listView - * @param \Step\Acceptance\EditView $editView - * @param \Step\Acceptance\UsersTester $users - * @param \Step\Acceptance\SideBar $sidebar - * @param \Helper\WebDriverHelper $webDriverHelper - * - * As an administrator I want to create a user. - */ - public function testScenarioCreateUser( - \AcceptanceTester $I, - \Step\Acceptance\ListView $listView, - \Step\Acceptance\EditView $editView, - \Step\Acceptance\UsersTester $users, - \Step\Acceptance\SideBar $sidebar, - \Helper\WebDriverHelper $webDriverHelper - ) { - $I->wantTo('Create a user'); - - $I->amOnUrl( - $webDriverHelper->getInstanceURL() - ); - - $this->fakeData->seed($this->fakeDataSeed); - $userName = 'Test_'. $this->fakeData->name; - - // Navigate to users list-view - $I->loginAsAdmin(); - $users->gotoUsers(); - $I->click('User Management'); - $listView->waitForListViewVisible(); - - // Create user - $sidebar->clickSideBarAction('Create'); - $editView->waitForEditViewVisible(); - $I->fillField('user_name', $userName); - $I->fillField('last_name', $this->fakeData->name); - $I->scrollTo('#Users0emailAddress0'); - $I->fillField('#Users0emailAddress0', $this->fakeData->email); - $I->executeJS('window.scrollTo(0,0); return true;'); - $I->click('Password'); - $I->fillField('#new_password', 'Test'); - $I->fillField('#confirm_pwd', 'Test'); - $editView->clickSaveButton(); - - // Logout and Login - $users->logoutUser(); - $I->fillField('#user_name', $userName); - $I->fillField('#username_password', 'Test'); - $I->click('Log In'); - $I->waitForElementNotVisible('#loginform', 120); - - $I->amOnUrl( - $webDriverHelper->getInstanceURL() - ); - $users->logoutUser(); - $I->waitForElementVisible('#loginform'); - $I->fillField('#user_name', $I->getAdminUser()); - $I->fillField('#username_password', $I->getAdminPassword()); - $I->click('Log In'); - $I->waitForElementNotVisible('#loginform', 120); - } public function testEmailSettingsMailAccountAdd(AcceptanceTester $I, UsersTester $Users, WebDriverHelper $webDriverHelper) {