API CHANGE Security::setDefaultAdmin() no longer writes credentials to any Member database records (created through Security::findAnAdministrator(). This prevents outdated credentials when setDefaultAdmin() code changes after creating the database record (see #4271)

API CHANGE Security::findAnAdministrator() no longer sets 'Email' and 'Password' properties on newly created members. Removed the $username and $password argments from the method.
ENHANCEMENT Member->requireDefaultRecords() no longer creates a default administrator based on $_REQUEST data. Moved functionality into Installer->install()
MINOR Security::findAnAdministrator() names any default administrators 'Default Admin' instead of 'Admin' (from r97478)

git-svn-id: svn://svn.silverstripe.com/silverstripe/open/modules/sapphire/trunk@102493 467b73ca-7a2a-4603-9d3b-597d59a354a9

security/Member.php

@@ -1033,15 +1033,6 @@ class Member extends DataObject {
 		}
 		return $labels;
 	}
-
-	function requireDefaultRecords() {
-		parent::requireDefaultRecords();
-		
-		if(!DB::query("SELECT * FROM \"Member\"")->value() && isset($_REQUEST['username']) && isset($_REQUEST['password'])) {
-			Security::findAnAdministrator($_REQUEST['username'], $_REQUEST['password']);
-			DB::alteration_message("Added admin account","created");
-		}
-	}
 	
 	/**
 	 * Users can view their own record.

security/Security.php

@@ -609,12 +609,19 @@ class Security extends Controller {
 
 
 	/**
-	 * Return a member with administrator privileges
+	 * Return an existing member with administrator privileges, or create one of necessary.
-	 *
+	 * 
-	 * @return Member Returns a member object that has administrator
+	 * Will create a default 'Administrators' group if no group is found
-	 *                privileges.
+	 * with an ADMIN permission. Will create a new 'Admin' member with administrative permissions
+	 * if no existing Member with these permissions is found. 
+	 * 
+	 * Important: Any newly created administrator accounts will NOT have valid
+	 * login credentials (Email/Password properties), which means they can't be used for login
+	 * purposes outside of any default credentials set through {@link Security::setDefaultAdmin()}.
+	 * 
+	 * @return Member 
 	 */
-	static function findAnAdministrator($username = 'admin', $password = 'password') {
+	static function findAnAdministrator() {
 		// coupling to subsites module
 		$subsiteCheck = class_exists('GroupSubsites') ?  ' AND "Group"."SubsiteID" = 0' : '';
 		
@@ -641,10 +648,10 @@ class Security extends Controller {
 		}
 		
 		if(!isset($member)) {
+			// Leave 'Email' and 'Password' are not set to avoid creating
+			// persistent logins in the database. See Security::setDefaultAdmin().
 			$member = Object::create('Member');
-			$member->FirstName = $member->Surname = 'Admin';
+			$member->FirstName = 'Default Admin';
-			$member->Email = $username;
-			$member->Password = $password;
 			$member->write();
 			$member->Groups()->add($adminGroup);
 		}
@@ -656,13 +663,13 @@ class Security extends Controller {
 	/**
 	 * Set a default admin in dev-mode
 	 * 
-	 * This will set a static default-admin (e.g. "td") which is not existing
+	 * This will set a static default-admin which is not existing
 	 * as a database-record. By this workaround we can test pages in dev-mode
 	 * with a unified login. Submitted login-credentials are first checked
-	 * against this static information in {@authenticate()}.
+	 * against this static information in {@link Security::authenticate()}.
 	 *
 	 * @param string $username The user name
-	 * @param string $password The password in cleartext
+	 * @param string $password The password (in cleartext)
 	 */
 	public static function setDefaultAdmin($username, $password) {
 		// don't overwrite if already set

tests/security/SecurityDefaultAdminTest.php

@@ -0,0 +1,48 @@
+<?php
+class SecurityDefaultAdminTest extends SapphireTest {
+	
+	function setUp() {
+		// TODO Workaround to force database clearing with no fixture present, 
+		// and avoid sideeffects from other tests
+		self::empty_temp_db();
+		
+		parent::setUp();
+	}
+	
+	function testCheckDefaultAdmin() {
+		// TODO There's currently no way to inspect default admin state,
+		//  hence we don't override existing settings
+		if(Security::has_default_admin()) return;
+		
+		Security::setDefaultAdmin('admin', 'password');
+		
+		$this->assertTrue(Security::has_default_admin());
+		$this->assertTrue(
+			Security::check_default_admin('admin', 'password'),
+			'Succeeds with correct username and password'
+		);
+		$this->assertFalse(
+			Security::check_default_admin('wronguser', 'password'),
+			'Fails with incorrect username'
+		);
+		$this->assertFalse(
+			Security::check_default_admin('admin', 'wrongpassword'),
+			'Fails with incorrect password'
+		);
+		
+		Security::setDefaultAdmin(null, null);
+	}
+	
+	function testFindAnAdministratorCreatesNewUser() {
+		$adminMembers = Permission::get_members_by_permission('ADMIN');
+		$this->assertFalse($adminMembers);
+		
+		$admin = Security::findAnAdministrator();
+		
+		$this->assertType('Member', $admin);
+		$this->assertTrue(Permission::checkMember($admin, 'ADMIN'));
+		$this->assertNull($admin->Email);
+		$this->assertNull($admin->Password);
+	}
+	
+}