From c604341a1d3457b0cee77dd2e2d5699d525d4d2d Mon Sep 17 00:00:00 2001
From: Ingo Schommer <ingo@silverstripe.com>
Date: Tue, 9 Mar 2010 04:10:38 +0000
Subject: [PATCH] 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

git-svn-id: svn://svn.silverstripe.com/silverstripe/open/modules/sapphire/trunk@100705 467b73ca-7a2a-4603-9d3b-597d59a354a9
---
 security/Member.php           | 44 ++++++++++++++++++++---------------
 tests/security/MemberTest.php | 39 +++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+), 19 deletions(-)

diff --git a/security/Member.php b/security/Member.php
index 30f6b45d5..21d08387c 100755
--- a/security/Member.php
+++ b/security/Member.php
@@ -554,27 +554,33 @@ class Member extends DataObject {
 	 */
 	function onBeforeWrite() {
 		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;
 		if($this->$identifierField) {
-			$idClause = ($this->ID) ? " AND \"Member\".\"ID\" <> $this->ID" : '';
-			$SQL_identifierField = Convert::raw2sql($this->$identifierField);
-			
-			$existingRecord = DataObject::get_one('Member', "\"$identifierField\" = '{$SQL_identifierField}'{$idClause}");
+			// Note: Same logic as Member_Validator class
+			$idClause = ($this->ID) ? sprintf(" AND \"Member\".\"ID\" <> %d", (int)$this->ID) : '';
+			$existingRecord = DataObject::get_one(
+				'Member', 
+				sprintf(
+					"\"%s\" = '%s' %s",
+					$identifierField,
+					Convert::raw2sql($this->$identifierField),
+					$idClause
+				)
+			);
 			if($existingRecord) {
-				$newID = $existingRecord->ID;
-				if($this->ID) {
-					DB::query("UPDATE \"Group_Members\" SET \"MemberID\" = $newID WHERE \"MemberID\" = $this->ID");
-				}
-				$this->ID = $newID;
-				// Merge existing data into the local record
-
-				foreach($existingRecord->getAllFields() as $k => $v) {
-					if(!$this->isChanged($k)) $this->record[$k] = $v;
-				}
-				$existingRecord->destroy();
+				throw new ValidationException(sprintf(
+					'Can\'t overwrite existing member #%d with identical $unique_identifier_field (%s = %s))',
+					$existingRecord->ID,
+					$identifierField,
+					$this->$identifierField
+				));
 			}
 		}
-		
+
 		// 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.
 		if(
@@ -585,7 +591,7 @@ class Member extends DataObject {
 		) {
 			$this->sendInfo('changePassword');
 		}
-		
+
 		// 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
 		// existing passwords.
@@ -601,7 +607,7 @@ class Member extends DataObject {
 			$this->Password = $encryption_details['password'];
 			$this->Salt = $encryption_details['salt'];
 			$this->PasswordEncryption = $encryption_details['algorithm'];
-			
+
 			// If we haven't manually set a password expiry
 			if(!$this->isChanged('PasswordExpiry')) {
 				// then set it for us
@@ -612,7 +618,7 @@ class Member extends DataObject {
 				}
 			}
 		}
-		
+
 		// save locale
 		if(!$this->Locale) {
 			$this->Locale = i18n::get_locale();
diff --git a/tests/security/MemberTest.php b/tests/security/MemberTest.php
index 190f66b9a..218f29441 100644
--- a/tests/security/MemberTest.php
+++ b/tests/security/MemberTest.php
@@ -6,12 +6,51 @@
 class MemberTest extends FunctionalTest {
 	static $fixture_file = 'sapphire/tests/security/MemberTest.yml';
 	
+	protected $orig = array();
+	
 	function 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);
 	}
 	
+	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() {
 		$member = new Member();
 		$member->Password = 'mypassword';