mirror of
https://github.com/silverstripe/silverstripe-framework
synced 2024-10-22 14:05:37 +02:00
BUG Prevent session hijackers from resetting a user password
BUG Member::checkPassword incorrect for default admin
This commit is contained in:
parent
8ebdedf330
commit
4d1ddf0e62
@ -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;
|
||||
}
|
||||
}
|
||||
|
@ -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",
|
||||
|
@ -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
|
||||
|
Loading…
Reference in New Issue
Block a user