Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
BUGFIX Keep Member.PasswordEncryption setting on empty passwords
This will prevent empty passwords to set the encryption to 'none',
which in turn will store any subsequent password changes in cleartext.
Reproduceable e.g. with ConfirmedPasswordField and setCanBeEmpty(true).
  • Loading branch information
chillu committed Feb 17, 2013
1 parent 3e27d27 commit eecd348
Show file tree
Hide file tree
Showing 2 changed files with 72 additions and 64 deletions.
119 changes: 55 additions & 64 deletions security/Security.php
Expand Up @@ -5,7 +5,7 @@
* @subpackage security
*/
class Security extends Controller {

/**
* Default user name. Only used in dev-mode by {@link setDefaultAdmin()}
*
Expand Down Expand Up @@ -82,7 +82,7 @@ class Security extends Controller {
* @var array|string
*/
protected static $default_message_set = '';

/**
* Get location of word list file
*/
Expand Down Expand Up @@ -130,22 +130,22 @@ static function set_default_message_set($messageSet) {
* If you don't provide a messageSet, a default will be used.
*
* @param Controller $controller The controller that you were on to cause the permission
* failure.
* failure.
* @param string|array $messageSet The message to show to the user. This
* can be a string, or a map of different
* messages for different contexts.
* If you pass an array, you can use the
* following keys:
* - default: The default message
* - logInAgain: The message to show
* if the user has just
* logged out and the
* - alreadyLoggedIn: The message to
* show if the user
* is already logged
* in and lacks the
* permission to
* access the item.
* can be a string, or a map of different
* messages for different contexts.
* If you pass an array, you can use the
* following keys:
* - default: The default message
* - logInAgain: The message to show
* if the user has just
* logged out and the
* - alreadyLoggedIn: The message to
* show if the user
* is already logged
* in and lacks the
* permission to
* access the item.
*
* The alreadyLoggedIn value can contain a '%s' placeholder that will be replaced with a link
* to log in.
Expand Down Expand Up @@ -233,7 +233,7 @@ static function permissionFailure($controller = null, $messageSet = null) {
}


/**
/**
* Get the login form to process according to the submitted data
*/
protected function LoginForm() {
Expand All @@ -255,7 +255,7 @@ protected function LoginForm() {
}


/**
/**
* Get the login forms for all available authentication methods
*
* @return array Returns an array of available login forms (array of Form
Expand All @@ -269,8 +269,8 @@ protected function GetLoginForms()

$authenticators = Authenticator::get_authenticators();
foreach($authenticators as $authenticator) {
array_push($forms,
call_user_func(array($authenticator, 'get_login_form'),
array_push($forms,
call_user_func(array($authenticator, 'get_login_form'),
$this));
}

Expand All @@ -293,9 +293,9 @@ public static function Link($action = null) {
* Log the currently logged in user out
*
* @param bool $redirect Redirect the user back to where they came.
* - If it's false, the code calling logout() is
* responsible for sending the user where-ever
* they should go.
* - If it's false, the code calling logout() is
* responsible for sending the user where-ever
* they should go.
*/
public function logout($redirect = true) {
$member = Member::currentUser();
Expand Down Expand Up @@ -328,15 +328,15 @@ public function login() {
Requirements::css($customCSS);
}

$tmpPage = new Page();
$tmpPage->Title = _t('Security.LOGIN', 'Log in');
$tmpPage->URLSegment = "Security";
// Disable ID-based caching of the log-in page by making it a random number
$tmpPage->ID = -1 * rand(1,10000000);
$tmpPage = new Page();
$tmpPage->Title = _t('Security.LOGIN', 'Log in');
$tmpPage->URLSegment = "Security";
// Disable ID-based caching of the log-in page by making it a random number
$tmpPage->ID = -1 * rand(1,10000000);

$controller = new Page_Controller($tmpPage);
$controller->init();
//Controller::$currentController = $controller;
$controller->init();
//Controller::$currentController = $controller;

$content = '';
$forms = $this->GetLoginForms();
Expand Down Expand Up @@ -418,12 +418,12 @@ public function lostpassword() {
Requirements::javascript(SAPPHIRE_DIR . '/javascript/prototype_improvements.js');
Requirements::javascript(THIRDPARTY_DIR . '/scriptaculous/effects.js');

$tmpPage = new Page();
$tmpPage->Title = _t('Security.LOSTPASSWORDHEADER', 'Lost Password');
$tmpPage->URLSegment = 'Security';
$tmpPage->ID = -1; // Set the page ID to -1 so we dont get the top level pages as its children
$tmpPage = new Page();
$tmpPage->Title = _t('Security.LOSTPASSWORDHEADER', 'Lost Password');
$tmpPage->URLSegment = 'Security';
$tmpPage->ID = -1; // Set the page ID to -1 so we dont get the top level pages as its children
$controller = new Page_Controller($tmpPage);
$controller->init();
$controller->init();

$customisedController = $controller->customise(array(
'Content' =>
Expand Down Expand Up @@ -478,15 +478,15 @@ public function passwordsent($request) {
Requirements::javascript(SAPPHIRE_DIR . '/javascript/prototype_improvements.js');
Requirements::javascript(THIRDPARTY_DIR . '/scriptaculous/effects.js');

$tmpPage = new Page();
$tmpPage->Title = _t('Security.LOSTPASSWORDHEADER');
$tmpPage->URLSegment = 'Security';
$tmpPage->ID = -1; // Set the page ID to -1 so we dont get the top level pages as its children
$tmpPage = new Page();
$tmpPage->Title = _t('Security.LOSTPASSWORDHEADER');
$tmpPage->URLSegment = 'Security';
$tmpPage->ID = -1; // Set the page ID to -1 so we dont get the top level pages as its children
$controller = new Page_Controller($tmpPage);
$controller->init();
$controller->init();

$email = Convert::raw2xml($request->param('ID') . '.' . $request->getExtension());

$customisedController = $controller->customise(array(
'Title' => sprintf(_t('Security.PASSWORDSENTHEADER', "Password reset link sent to '%s'"), $email),
'Content' =>
Expand Down Expand Up @@ -529,12 +529,12 @@ public static function getPasswordResetLink($member, $autologinToken) {
* @return string Returns the "change password" page as HTML code.
*/
public function changepassword() {
$tmpPage = new Page();
$tmpPage->Title = _t('Security.CHANGEPASSWORDHEADER', 'Change your password');
$tmpPage->URLSegment = 'Security';
$tmpPage->ID = -1; // Set the page ID to -1 so we dont get the top level pages as its children
$tmpPage = new Page();
$tmpPage->Title = _t('Security.CHANGEPASSWORDHEADER', 'Change your password');
$tmpPage->URLSegment = 'Security';
$tmpPage->ID = -1; // Set the page ID to -1 so we dont get the top level pages as its children
$controller = new Page_Controller($tmpPage);
$controller->init();
$controller->init();

// Extract the member from the URL.
$member = null;
Expand Down Expand Up @@ -673,8 +673,8 @@ static function findAnAdministrator() {
$adminGroup = $adminGroup->First();

if($adminGroup->Members()->First()) {
$member = $adminGroup->Members()->First();
}
$member = $adminGroup->Members()->First();
}
}

if(!$adminGroup) {
Expand Down Expand Up @@ -835,18 +835,9 @@ public static function get_password_encryption_algorithm() {
* @see encrypt_passwords()
* @see set_password_encryption_algorithm()
*/
static function encrypt_password($password, $salt = null, $algorithm = null, $member = null) {
if(
// if the password is empty, don't encrypt
strlen(trim($password)) == 0
// if no algorithm is provided and no default is set, don't encrypt
|| (!$algorithm && self::$encryptPasswords == false)
) {
$algorithm = 'none';
} else {
// Fall back to the default encryption algorithm
if(!$algorithm) $algorithm = self::$encryptionAlgorithm;
}
public static function encrypt_password($password, $salt = null, $algorithm = null, $member = null) {
// Fall back to the default encryption algorithm
if(!$algorithm) $algorithm = self::$encryptionAlgorithm;

$e = PasswordEncryptor::create_for_algorithm($algorithm);

Expand All @@ -870,15 +861,15 @@ static function encrypt_password($password, $salt = null, $algorithm = null, $me
public static function database_is_ready() {
// Used for unit tests
if(self::$force_database_is_ready !== NULL) return self::$force_database_is_ready;

$requiredTables = ClassInfo::dataClassesFor('Member');
$requiredTables[] = 'Group';
$requiredTables[] = 'Permission';

foreach($requiredTables as $table) {
// if any of the tables aren't created in the database
if(!ClassInfo::hasTable($table)) return false;

// if any of the tables don't have all fields mapped as table columns
$dbFields = DB::fieldList($table);
if(!$dbFields) return false;
Expand Down Expand Up @@ -929,5 +920,5 @@ public static function default_login_dest() {
return self::$default_login_dest;
}

}
}
?>
17 changes: 17 additions & 0 deletions tests/security/MemberTest.php
Expand Up @@ -114,6 +114,23 @@ function testDefaultPasswordEncryptionDoesntChangeExistingMembers() {

Security::set_password_encryption_algorithm($origAlgo);
}

public function testKeepsEncryptionOnEmptyPasswords() {
$member = new Member();
$member->Password = 'mypassword';
$member->PasswordEncryption = 'sha1_v2.4';
$member->write();

$member->Password = '';
$member->write();

$this->assertEquals(
$member->PasswordEncryption,
'sha1_v2.4'
);
$result = $member->checkPassword('');
$this->assertTrue($result->valid());
}

function testSetPassword() {
$member = $this->objFromFixture('Member', 'test');
Expand Down

0 comments on commit eecd348

Please sign in to comment.