Merge pull request #5533 from open-sausages/pulls/3/require-existing-password

BUG Prevent session hijackers from resetting a user password

forms/ConfirmedPasswordField.php

@@ -54,13 +54,28 @@ class ConfirmedPasswordField extends FormField {
 	 */
 	protected $showOnClick = false;
 
+	/**
+	 * Check if the existing password should be entered first
+	 *
+	 * @var bool
+	 */
+	protected $requireExistingPassword = false;
+
 
 	/**
-	 * A place to temporarly store the confirm password value
+	 * A place to temporarily store the confirm password value
+	 *
 	 * @var string
 	 */
 	protected $confirmValue;
 
+	/**
+	 * Store value of "Current Password" field
+	 *
+	 * @var string
+	 */
+	protected $currentPasswordValue;
+
 	/**
 	 * Title for the link that triggers the visibility of password fields.
 	 *
@@ -105,6 +120,7 @@ class ConfirmedPasswordField extends FormField {
 
 		// disable auto complete
 		foreach($this->children as $child) {
+			/** @var FormField $child */
 			$child->setAttribute('autocomplete', 'off');
 		}
 
@@ -113,7 +129,7 @@ class ConfirmedPasswordField extends FormField {
 		// we have labels for the subfields
 		$title = false;
 
-		parent::__construct($name, $title, null, $form);
+		parent::__construct($name, $title);
 		$this->setValue($value);
 	}
 
@@ -147,6 +163,7 @@ class ConfirmedPasswordField extends FormField {
 		}
 
 		foreach($this->children as $field) {
+			/** @var FormField $field */
 			$field->setDisabled($this->isDisabled());
 			$field->setReadonly($this->isReadonly());
 
@@ -220,6 +237,7 @@ class ConfirmedPasswordField extends FormField {
 	 */
 	public function setRightTitle($title) {
 		foreach($this->children as $field) {
+			/** @var FormField $field */
 			$field->setRightTitle($title);
 		}
 
@@ -227,15 +245,20 @@ class ConfirmedPasswordField extends FormField {
 	}
 
 	/**
-	 * @param array $titles 2 entry array with the customized title for each
-	 *						of the 2 children.
+	 * Set child field titles. Titles in order should be:
+	 *  - "Current Password" (if getRequireExistingPassword() is set)
+	 *  - "Password"
+	 *  - "Confirm Password"
 	 *
-	 * @return ConfirmedPasswordField
+	 * @param array $titles List of child titles
+	 * @return $this
 	 */
 	public function setChildrenTitles($titles) {
-		if(is_array($titles) && count($titles) == 2) {
+		$expectedChildren = $this->getRequireExistingPassword() ? 3 : 2;
+		if(is_array($titles) && count($titles) == $expectedChildren) {
 			foreach($this->children as $field) {
 				if(isset($titles[0])) {
+					/** @var FormField $field */
 					$field->setTitle($titles[0]);
 
 					array_shift($titles);
@@ -251,8 +274,8 @@ class ConfirmedPasswordField extends FormField {
 	 * to handle both cases.
 	 *
 	 * @param mixed $value
-	 *
-	 * @return ConfirmedPasswordField
+	 * @param mixed $data
+	 * @return $this
 	 */
 	public function setValue($value, $data = null) {
 		// If $data is a DataObject, don't use the value, since it's a hashed value
@@ -264,6 +287,9 @@ class ConfirmedPasswordField extends FormField {
 		if(is_array($value)) {
 			$this->value = $value['_Password'];
 			$this->confirmValue = $value['_ConfirmPassword'];
+			$this->currentPasswordValue = ($this->getRequireExistingPassword() && isset($value['_CurrentPassword']))
+				? $value['_CurrentPassword']
+				: null;
 
 			if($this->showOnClick && isset($value['_PasswordFieldVisible'])) {
 				$this->children->fieldByName($this->getName() . '[_PasswordFieldVisible]')
@@ -292,6 +318,7 @@ class ConfirmedPasswordField extends FormField {
 	 * Update the names of the child fields when updating name of field.
 	 *
 	 * @param string $name new name to give to the field.
+	 * @return $this
 	 */
 	public function setName($name) {
 		$this->children->fieldByName($this->getName() . '[_Password]')
@@ -340,8 +367,7 @@ class ConfirmedPasswordField extends FormField {
 			$validator->validationError(
 				$name,
 				_t('Form.VALIDATIONPASSWORDSDONTMATCH',"Passwords don't match"),
-				"validation",
-				false
+				"validation"
 			);
 
 			return false;
@@ -353,8 +379,7 @@ class ConfirmedPasswordField extends FormField {
 				$validator->validationError(
 					$name,
 					_t('Form.VALIDATIONPASSWORDSNOTEMPTY', "Passwords can't be empty"),
-					"validation",
-					false
+					"validation"
 				);
 
 				return false;
@@ -363,6 +388,8 @@ class ConfirmedPasswordField extends FormField {
 
 		// lengths
 		if(($this->minLength || $this->maxLength)) {
+			$errorMsg = null;
+			$limit = null;
 			if($this->minLength && $this->maxLength) {
 				$limit = "{{$this->minLength},{$this->maxLength}}";
 				$errorMsg = _t(
@@ -390,8 +417,7 @@ class ConfirmedPasswordField extends FormField {
 				$validator->validationError(
 					$name,
 					$errorMsg,
-					"validation",
-					false
+					"validation"
 				);
 			}
 		}
@@ -402,14 +428,56 @@ class ConfirmedPasswordField extends FormField {
 					$name,
 					_t('Form.VALIDATIONSTRONGPASSWORD',
 						"Passwords must have at least one digit and one alphanumeric character"),
-					"validation",
-					false
+					"validation"
 				);
 
 				return false;
 			}
 		}
 
+		// Check if current password is valid
+		if(!empty($value) && $this->getRequireExistingPassword()) {
+			if(!$this->currentPasswordValue) {
+				$validator->validationError(
+					$name,
+					_t(
+						'ConfirmedPasswordField.CURRENT_PASSWORD_MISSING',
+						"You must enter your current password."
+					),
+					"validation"
+				);
+				return false;
+			}
+
+			// Check this password is valid for the current user
+			$member = Member::currentUser();
+			if(!$member) {
+				$validator->validationError(
+					$name,
+					_t(
+						'ConfirmedPasswordField.LOGGED_IN_ERROR',
+						"You must be logged in to change your password."
+					),
+					"validation"
+				);
+				return false;
+			}
+
+			// With a valid user and password, check the password is correct
+			$checkResult = $member->checkPassword($this->currentPasswordValue);
+			if(!$checkResult->valid()) {
+				$validator->validationError(
+					$name,
+					_t(
+						'ConfirmedPasswordField.CURRENT_PASSWORD_ERROR',
+						"The current password you have entered is not correct."
+					),
+					"validation"
+				);
+				return false;
+			}
+		}
+
 		return true;
 	}
 
@@ -442,4 +510,36 @@ class ConfirmedPasswordField extends FormField {
 
 		return $field;
 	}
+
+	/**
+	 * Check if existing password is required
+	 *
+	 * @return bool
+	 */
+	public function getRequireExistingPassword() {
+		return $this->requireExistingPassword;
+	}
+
+	/**
+	 * Set if the existing password should be required
+	 *
+	 * @param bool $show Flag to show or hide this field
+	 * @return $this
+	 */
+	public function setRequireExistingPassword($show) {
+		// Don't modify if already added / removed
+		if((bool)$show === $this->requireExistingPassword) {
+			return $this;
+		}
+		$this->requireExistingPassword = $show;
+		$name = $this->getName();
+		$currentName = "{$name}[_CurrentPassword]";
+		if ($show) {
+			$confirmField = PasswordField::create($currentName, _t('Member.CURRENT_PASSWORD', 'Current Password'));
+			$this->children->unshift($confirmField);
+		} else {
+			$this->children->removeByName($currentName, true);
+		}
+		return $this;
+	}
 }

security/Member.php

@@ -312,15 +312,23 @@ class Member extends DataObject implements TemplateGlobalProvider {
 	/**
 	 * Check if the passed password matches the stored one (if the member is not locked out).
 	 *
-	 * @param  string $password
+	 * @param string $password
 	 * @return ValidationResult
 	 */
 	public function checkPassword($password) {
 		$result = $this->canLogIn();
 
 		// Short-circuit the result upon failure, no further checks needed.
-		if (!$result->valid()) return $result;
+		if (!$result->valid()) {
+			return $result;
+		}
 
+		// Allow default admin to login as self
+		if($this->isDefaultAdmin() && Security::check_default_admin($this->Email, $password)) {
+			return $result;
+		}
+
+		// Check a password is set on this member
 		if(empty($this->Password) && $this->exists()) {
 			$result->error(_t('Member.NoPassword','There is no password on this member.'));
 			return $result;
@@ -337,6 +345,16 @@ class Member extends DataObject implements TemplateGlobalProvider {
 		return $result;
 	}
 
+	/**
+	 * Check if this user is the currently configured default admin
+	 *
+	 * @return bool
+	 */
+	public function isDefaultAdmin() {
+		return Security::has_default_admin()
+			&& $this->Email === Security::default_admin_username();
+	}
+
 	/**
 	 * Returns a valid {@link ValidationResult} if this member can currently log in, or an invalid
 	 * one with error messages to display if the member is locked out.
@@ -730,14 +748,7 @@ class Member extends DataObject implements TemplateGlobalProvider {
 	public function getMemberFormFields() {
 		$fields = parent::getFrontendFields();
 
-		$fields->replaceField('Password', $password = new ConfirmedPasswordField (
-			'Password',
-			$this->fieldLabel('Password'),
-			null,
-			null,
-			(bool) $this->ID
-		));
-		$password->setCanBeEmpty(true);
+		$fields->replaceField('Password', $this->getMemberPasswordField());
 
 		$fields->replaceField('Locale', new DropdownField (
 			'Locale',
@@ -754,6 +765,36 @@ class Member extends DataObject implements TemplateGlobalProvider {
 		return $fields;
 	}
 
+	/**
+	 * Builds "Change / Create Password" field for this member
+	 *
+	 * @return ConfirmedPasswordField
+	 */
+	public function getMemberPasswordField() {
+		$editingPassword = $this->isInDB();
+		$label = $editingPassword
+			? _t('Member.EDIT_PASSWORD', 'New Password')
+			: $this->fieldLabel('Password');
+		/** @var ConfirmedPasswordField $password */
+		$password = ConfirmedPasswordField::create(
+			'Password',
+			$label,
+			null,
+			null,
+			$editingPassword
+		);
+
+		// If editing own password, require confirmation of existing
+		if($editingPassword && $this->ID == Member::currentUserID()) {
+			$password->setRequireExistingPassword(true);
+		}
+
+		$password->setCanBeEmpty(true);
+		$this->extend('updateMemberPasswordField', $password);
+		return $password;
+	}
+
+
 	/**
 	 * Returns the {@link RequiredFields} instance for the Member object. This
 	 * Validator is used when saving a {@link CMSProfileController} or added to
@@ -1341,19 +1382,12 @@ class Member extends DataObject implements TemplateGlobalProvider {
 		require_once 'Zend/Date.php';
 
 		$self = $this;
-		$this->beforeUpdateCMSFields(function($fields) use ($self) {
-			$mainFields = $fields->fieldByName("Root")->fieldByName("Main")->Children;
+		$this->beforeUpdateCMSFields(function(FieldList $fields) use ($self) {
+			/** @var FieldList $mainFields */
+			$mainFields = $fields->fieldByName("Root")->fieldByName("Main")->getChildren();
 
-			$password = new ConfirmedPasswordField(
-				'Password',
-				null,
-				null,
-				null,
-				true // showOnClick
-			);
-			$password->setCanBeEmpty(true);
-			if( ! $self->ID) $password->showOnClick = false;
-			$mainFields->replaceField('Password', $password);
+			// Build change password field
+			$mainFields->replaceField('Password', $self->getMemberPasswordField());
 
 			$mainFields->replaceField('Locale', new DropdownField(
 				"Locale",

tests/behat/features/profile.feature

@@ -19,9 +19,18 @@ Feature: Manage my own settings
 		Then I should see "Jack"
 		And I should see "Johnson"
 
+	Scenario: I can't reset the password without the original
+		Given I follow "Change Password"
+		And I fill in "Current Password" with "idontknow"
+		And I fill in "New Password" with "newsecret"
+		And I fill in "Confirm Password" with "newsecret"
+		And I press the "Save" button
+		Then I should see "The current password you have entered is not correct."
+
 	Scenario: I can change my password
 		Given I follow "Change Password"
-		And I fill in "Password" with "newsecret"
+		And I fill in "Current Password" with "secret"
+		And I fill in "New Password" with "newsecret"
 		And I fill in "Confirm Password" with "newsecret"
 		And I press the "Save" button
 		And I am not logged in
@@ -35,4 +44,4 @@ Feature: Manage my own settings
 		Then I should see "Sprache"
 
   # TODO Date/time format - Difficult because its not exposed anywhere in the CMS?
-  # TODO Group modification as ADMIN user
+  # TODO Group modification as ADMIN user