tonyair/silverstripe-framework
1
0
Fork 0
mirror of https://github.com/silverstripe/silverstripe-framework synced 2024-10-22 14:05:37 +02:00
Code Issues Projects Releases Wiki Activity

API CHANGE Removed "auto-merging" of member records from Member->onBeforeWrite() due to security reasons - please use DataObject->merge() explicitly if this is desired behaviour

Browse Source 
git-svn-id: svn://svn.silverstripe.com/silverstripe/open/modules/sapphire/trunk@100705 467b73ca-7a2a-4603-9d3b-597d59a354a9
This commit is contained in:
Ingo Schommer 2010-03-09 04:10:38 +00:00
parent f4e284a3c1
commit c604341a1d
2 changed files with 64 additions and 19 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

44
security/Member.php
View File

@ -554,27 +554,33 @@ class Member extends DataObject {
*/ */
function onBeforeWrite() { function onBeforeWrite() {
if($this->SetPassword) $this->Password = $this->SetPassword; if($this->SetPassword) $this->Password = $this->SetPassword;
// If a member with the same "unique identifier" already exists with a different ID, don't allow merging.
// Note: This does not a full replacement for safeguards in the controller layer (e.g. in a registration form),
// but rather a last line of defense against data inconsistencies.
$identifierField = self::$unique_identifier_field; $identifierField = self::$unique_identifier_field;
if($this->$identifierField) { if($this->$identifierField) {
$idClause = ($this->ID) ? " AND \"Member\".\"ID\" <> $this->ID" : ''; // Note: Same logic as Member_Validator class
$SQL_identifierField = Convert::raw2sql($this->$identifierField); $idClause = ($this->ID) ? sprintf(" AND \"Member\".\"ID\" <> %d", (int)$this->ID) : '';
$existingRecord = DataObject::get_one(
$existingRecord = DataObject::get_one('Member', "\"$identifierField\" = '{$SQL_identifierField}'{$idClause}"); 'Member',
sprintf(
"\"%s\" = '%s' %s",
$identifierField,
Convert::raw2sql($this->$identifierField),
$idClause
)
);
if($existingRecord) { if($existingRecord) {
$newID = $existingRecord->ID; throw new ValidationException(sprintf(
if($this->ID) { 'Can\'t overwrite existing member #%d with identical $unique_identifier_field (%s = %s))',
DB::query("UPDATE \"Group_Members\" SET \"MemberID\" = $newID WHERE \"MemberID\" = $this->ID"); $existingRecord->ID,
} $identifierField,
$this->ID = $newID; $this->$identifierField
// Merge existing data into the local record ));
foreach($existingRecord->getAllFields() as $k => $v) {
if(!$this->isChanged($k)) $this->record[$k] = $v;
}
$existingRecord->destroy();
} }
} }
// We don't send emails out on dev/tests sites to prevent accidentally spamming users. // We don't send emails out on dev/tests sites to prevent accidentally spamming users.
// However, if TestMailer is in use this isn't a risk. // However, if TestMailer is in use this isn't a risk.
if( if(
@ -585,7 +591,7 @@ class Member extends DataObject {
) { ) {
$this->sendInfo('changePassword'); $this->sendInfo('changePassword');
} }
// The test on $this->ID is used for when records are initially created. // The test on $this->ID is used for when records are initially created.
// Note that this only works with cleartext passwords, as we can't rehash // Note that this only works with cleartext passwords, as we can't rehash
// existing passwords. // existing passwords.
@ -601,7 +607,7 @@ class Member extends DataObject {
$this->Password = $encryption_details['password']; $this->Password = $encryption_details['password'];
$this->Salt = $encryption_details['salt']; $this->Salt = $encryption_details['salt'];
$this->PasswordEncryption = $encryption_details['algorithm']; $this->PasswordEncryption = $encryption_details['algorithm'];
// If we haven't manually set a password expiry // If we haven't manually set a password expiry
if(!$this->isChanged('PasswordExpiry')) { if(!$this->isChanged('PasswordExpiry')) {
// then set it for us // then set it for us
@ -612,7 +618,7 @@ class Member extends DataObject {
} }
} }
} }
// save locale // save locale
if(!$this->Locale) { if(!$this->Locale) {
$this->Locale = i18n::get_locale(); $this->Locale = i18n::get_locale();

39
tests/security/MemberTest.php
View File

@ -6,12 +6,51 @@
class MemberTest extends FunctionalTest { class MemberTest extends FunctionalTest {
static $fixture_file = 'sapphire/tests/security/MemberTest.yml'; static $fixture_file = 'sapphire/tests/security/MemberTest.yml';
protected $orig = array();
function setUp() { function setUp() {
parent::setUp(); parent::setUp();
$this->orig['Member_unique_identifier_field'] = Member::get_unique_identifier_field();
Member::set_unique_identifier_field('Email');
Member::set_password_validator(null); Member::set_password_validator(null);
} }
function tearDown() {
Member::set_unique_identifier_field($this->orig['Member_unique_identifier_field']);
parent::tearDown();
}
/**
* @expectedException ValidationException
*/
function testWriteDoesntMergeNewRecordWithExistingMember() {
$m1 = new Member();
$m1->Email = 'member@test.com';
$m1->write();
$m2 = new Member();
$m2->Email = 'member@test.com';
$m2->write();
}
/**
* @expectedException ValidationException
*/
function testWriteDoesntMergeExistingMemberOnIdentifierChange() {
$m1 = new Member();
$m1->Email = 'member@test.com';
$m1->write();
$m2 = new Member();
$m2->Email = 'member_new@test.com';
$m2->write();
$m2->Email = 'member@test.com';
$m2->write();
}
function testDefaultPasswordEncryptionOnMember() { function testDefaultPasswordEncryptionOnMember() {
$member = new Member(); $member = new Member();
$member->Password = 'mypassword'; $member->Password = 'mypassword';