ChangePasswordForm::doChangePassword() - Code Metrics - Inspection of "API Apply SilverStripe\Security namespace" - silverstripe/silverstripe-framework - Measure and Improve Code Quality continuously with Scrutinizer

Completed

Pull Request — master (#5741)

by Damian

created 2016-07-06 23:33 UTC

ChangePasswordForm::doChangePassword() C

↳ Parent: ChangePasswordForm

Complexity

Conditions	12
Paths	35

Size

Total Lines	90
Code Lines	53

Duplication

Lines	0
Ratio	0 %

Importance

Changes	1
Bugs	0	Features	0

Metric	Value
cc	12
eloc	53
c	1
b	0
f	0
nc	35
nop	1
dl	0
loc	90
rs	5.034

How to fix Long Method Complexity

<?php

namespace SilverStripe\Security;

use Form;
use Session;
use FieldList;
use PasswordField;
use FormAction;
use HiddenField;
use Director;
use HTTP;
use Convert;

/**
 * Standard Change Password Form
 * @package framework
 * @subpackage security
 */
class ChangePasswordForm extends Form {

	/**
	 * Constructor
	 *
	 * @param Controller $controller The parent controller, necessary to
	 *                               create the appropriate form action tag.
	 * @param string $name The method on the controller that will return this
	 *                     form object.
	 * @param FieldList|FormField $fields All of the fields in the form - a
	 *                                   {@link FieldList} of {@link FormField}
	 *                                   objects.
	 * @param FieldList|FormAction $actions All of the action buttons in the
	 *                                     form - a {@link FieldList} of
	 */
	public function __construct($controller, $name, $fields = null, $actions = null) {
// Bad
class Router
{
    public function generate($path)
    {
        return $_SERVER['HOST'].$path;
    }
}

// Better
class Router
{
    private $host;

    public function __construct($host)
    {
        $this->host = $host;
    }

    public function generate($path)
    {
        return $this->host.$path;
    }
}

class Controller
{
    public function myAction(Request $request)
    {
        // Instead of
        $page = isset($_GET['page']) ? intval($_GET['page']) : 1;

        // Better (assuming you use the Symfony2 request)
        $page = $request->query->get('page', 1);
    }
}
		if(isset($_REQUEST['BackURL'])) {
			$backURL = $_REQUEST['BackURL'];
		} else {
			$backURL = Session::get('BackURL');
		}

		if(!$fields) {
			$fields = new FieldList();

			// Security/changepassword?h=XXX redirects to Security/changepassword
			// without GET parameter to avoid potential HTTP referer leakage.
			// In this case, a user is not logged in, and no 'old password' should be necessary.
			if(Member::currentUser()) {
				$fields->push(new PasswordField("OldPassword",_t('Member.YOUROLDPASSWORD', "Your old password")));
			}

			$fields->push(new PasswordField("NewPassword1", _t('Member.NEWPASSWORD', "New Password")));
			$fields->push(new PasswordField("NewPassword2", _t('Member.CONFIRMNEWPASSWORD', "Confirm New Password")));
		}
		if(!$actions) {
			$actions = new FieldList(
				new FormAction("doChangePassword", _t('Member.BUTTONCHANGEPASSWORD', "Change Password"))
			);
		}

		if(isset($backURL)) {
			$fields->push(new HiddenField('BackURL', 'BackURL', $backURL));
		}

		parent::__construct($controller, $name, $fields, $actions);

	}


	/**
	 * Change the password
	 *
	 * @param array $data The user submitted data
	 * @return SS_HTTPResponse
	 */
	public function doChangePassword(array $data) {
// Bad
class Router
{
    public function generate($path)
    {
        return $_SERVER['HOST'].$path;
    }
}

// Better
class Router
{
    private $host;

    public function __construct($host)
    {
        $this->host = $host;
    }

    public function generate($path)
    {
        return $this->host.$path;
    }
}

class Controller
{
    public function myAction(Request $request)
    {
        // Instead of
        $page = isset($_GET['page']) ? intval($_GET['page']) : 1;

        // Better (assuming you use the Symfony2 request)
        $page = $request->query->get('page', 1);
    }
}
		if($member = Member::currentUser()) {
			// The user was logged in, check the current password
			if(empty($data['OldPassword']) || !$member->checkPassword($data['OldPassword'])->valid()) {
				$this->clearMessage();
				$this->sessionMessage(
					_t('Member.ERRORPASSWORDNOTMATCH', "Your current password does not match, please try again"),
					"bad"
				);
				// redirect back to the form, instead of using redirectBack() which could send the user elsewhere.
				return $this->controller->redirect($this->controller->Link('changepassword'));

			}
		}

		if(!$member) {
			if(Session::get('AutoLoginHash')) {
				$member = Member::member_from_autologinhash(Session::get('AutoLoginHash'));
			}

			// The user is not logged in and no valid auto login hash is available
			if(!$member) {
				Session::clear('AutoLoginHash');
				return $this->controller->redirect($this->controller->Link('login'));

			}
		}

		// Check the new password
		if(empty($data['NewPassword1'])) {
			$this->clearMessage();
			$this->sessionMessage(
				_t('Member.EMPTYNEWPASSWORD', "The new password can't be empty, please try again"),
				"bad");

			// redirect back to the form, instead of using redirectBack() which could send the user elsewhere.
			return $this->controller->redirect($this->controller->Link('changepassword'));

		}
		else if($data['NewPassword1'] == $data['NewPassword2']) {
			$isValid = $member->changePassword($data['NewPassword1']);
			if($isValid->valid()) {
				$member->logIn();

				// TODO Add confirmation message to login redirect
				Session::clear('AutoLoginHash');

				// Clear locked out status
				$member->LockedOutUntil = null;
				$member->FailedLoginCount = null;
				$member->write();

				if (!empty($_REQUEST['BackURL'])
					// absolute redirection URLs may cause spoofing
					&& Director::is_site_url($_REQUEST['BackURL'])
				) {
					$url = Director::absoluteURL($_REQUEST['BackURL']);
					return $this->controller->redirect($url);
<?php

function getDate($date)
{
    if ($date !== null) {
        return new DateTime($date);
    }

    return false;
}
				}
				else {
					// Redirect to default location - the login form saying "You are logged in as..."
					$redirectURL = HTTP::setGetVar(
						'BackURL',
						Director::absoluteBaseURL(), $this->controller->Link('login')

					);
					return $this->controller->redirect($redirectURL);
				}
			} else {
				$this->clearMessage();
				$this->sessionMessage(
					_t(
						'Member.INVALIDNEWPASSWORD',
						"We couldn't accept that password: {password}",
						array('password' => nl2br("\n".Convert::raw2xml($isValid->starredList())))
					),
					"bad",
					false
				);

				// redirect back to the form, instead of using redirectBack() which could send the user elsewhere.
				return $this->controller->redirect($this->controller->Link('changepassword'));

			}

		} else {
			$this->clearMessage();
			$this->sessionMessage(
				_t('Member.ERRORNEWPASSWORD', "You have entered your new password differently, try again"),
				"bad");

			// redirect back to the form, instead of using redirectBack() which could send the user elsewhere.
			return $this->controller->redirect($this->controller->Link('changepassword'));

		}
	}

}



1			<?php
2
3			namespace SilverStripe\Security;
4
5			use Form;
6			use Session;
7			use FieldList;
8			use PasswordField;
9			use FormAction;
10			use HiddenField;
11			use Director;
12			use HTTP;
13			use Convert;
14
15			/**
16			* Standard Change Password Form
17			* @package framework
18			* @subpackage security
19			*/
20			class ChangePasswordForm extends Form {
21
22			/**
23			* Constructor
24			*
25			* @param Controller $controller The parent controller, necessary to
26			* create the appropriate form action tag.
27			* @param string $name The method on the controller that will return this
28			* form object.
29			* @param FieldList\|FormField $fields All of the fields in the form - a
30			* {@link FieldList} of {@link FormField}
31			* objects.
32			* @param FieldList\|FormAction $actions All of the action buttons in the
33			* form - a {@link FieldList} of
34			*/
35			public function __construct($controller, $name, $fields = null, $actions = null) {
			0 ignored issues – show Coding Style introduced 2016-06-29 00:25 UTC by Report Bug Copy Issue Report `__construct` uses the super-global variable `$_REQUEST` which is generally not recommended. Instead of super-globals, we recommend to explicitly inject the dependencies of your class. This makes your code less dependent on global state and it becomes generally more testable: // Bad class Router { public function generate($path) { return $_SERVER['HOST'].$path; } } // Better class Router { private $host; public function __construct($host) { $this->host = $host; } public function generate($path) { return $this->host.$path; } } class Controller { public function myAction(Request $request) { // Instead of $page = isset($_GET['page']) ? intval($_GET['page']) : 1; // Better (assuming you use the Symfony2 request) $page = $request->query->get('page', 1); } } Loading history...
36			if(isset($_REQUEST['BackURL'])) {
37			$backURL = $_REQUEST['BackURL'];
38			} else {
39			$backURL = Session::get('BackURL');
40			}
41
42			if(!$fields) {
43			$fields = new FieldList();
44
45			// Security/changepassword?h=XXX redirects to Security/changepassword
46			// without GET parameter to avoid potential HTTP referer leakage.
47			// In this case, a user is not logged in, and no 'old password' should be necessary.
48			if(Member::currentUser()) {
49			$fields->push(new PasswordField("OldPassword",_t('Member.YOUROLDPASSWORD', "Your old password")));
50			}
51
52			$fields->push(new PasswordField("NewPassword1", _t('Member.NEWPASSWORD', "New Password")));
53			$fields->push(new PasswordField("NewPassword2", _t('Member.CONFIRMNEWPASSWORD', "Confirm New Password")));
54			}
55			if(!$actions) {
56			$actions = new FieldList(
57			new FormAction("doChangePassword", _t('Member.BUTTONCHANGEPASSWORD', "Change Password"))
58			);
59			}
60
61			if(isset($backURL)) {
62			$fields->push(new HiddenField('BackURL', 'BackURL', $backURL));
63			}
64
65			parent::__construct($controller, $name, $fields, $actions);
			0 ignored issues – show Bug introduced 2016-06-29 00:25 UTC by Report Bug Copy Issue Report It seems like `$actions` defined by parameter `$actions` on line 35 can also be of type `object<FormAction>`; however, `Form::__construct()` does only seem to accept `object<FieldList>`, maybe add an additional type check? This check looks at variables that have been passed in as parameters and are passed out again to other methods. If the outgoing method call has stricter type requirements than the method itself, an issue is raised. An additional type check may prevent trouble. Loading history...
66			}
67
68
69			/**
70			* Change the password
71			*
72			* @param array $data The user submitted data
73			* @return SS_HTTPResponse
74			*/
75			public function doChangePassword(array $data) {
			0 ignored issues – show Coding Style introduced 2016-06-29 00:25 UTC by Report Bug Copy Issue Report `doChangePassword` uses the super-global variable `$_REQUEST` which is generally not recommended. Instead of super-globals, we recommend to explicitly inject the dependencies of your class. This makes your code less dependent on global state and it becomes generally more testable: // Bad class Router { public function generate($path) { return $_SERVER['HOST'].$path; } } // Better class Router { private $host; public function __construct($host) { $this->host = $host; } public function generate($path) { return $this->host.$path; } } class Controller { public function myAction(Request $request) { // Instead of $page = isset($_GET['page']) ? intval($_GET['page']) : 1; // Better (assuming you use the Symfony2 request) $page = $request->query->get('page', 1); } } Loading history...
76			if($member = Member::currentUser()) {
77			// The user was logged in, check the current password
78			if(empty($data['OldPassword']) \|\| !$member->checkPassword($data['OldPassword'])->valid()) {
79			$this->clearMessage();
80			$this->sessionMessage(
81			_t('Member.ERRORPASSWORDNOTMATCH', "Your current password does not match, please try again"),
82			"bad"
83			);
84			// redirect back to the form, instead of using redirectBack() which could send the user elsewhere.
85			return $this->controller->redirect($this->controller->Link('changepassword'));
			0 ignored issues – show Unused Code introduced 2016-06-29 00:25 UTC by Report Bug Copy Issue Report The call to `Controller::Link()` has too many arguments starting with `'changepassword'`. This check compares calls to functions or methods with their respective definitions. If the call has more arguments than are defined, it raises an issue. If a function is defined several times with a different number of parameters, the check may pick up the wrong definition and report false positives. One codebase where this has been known to happen is Wordpress. In this case you can add the `@ignore` PhpDoc annotation to the duplicate definition and it will be ignored. Loading history...
86			}
87			}
88
89			if(!$member) {
90			if(Session::get('AutoLoginHash')) {
91			$member = Member::member_from_autologinhash(Session::get('AutoLoginHash'));
92			}
93
94			// The user is not logged in and no valid auto login hash is available
95			if(!$member) {
96			Session::clear('AutoLoginHash');
97			return $this->controller->redirect($this->controller->Link('login'));
			0 ignored issues – show Unused Code introduced 2016-06-29 00:25 UTC by Report Bug Copy Issue Report The call to `Controller::Link()` has too many arguments starting with `'login'`. This check compares calls to functions or methods with their respective definitions. If the call has more arguments than are defined, it raises an issue. If a function is defined several times with a different number of parameters, the check may pick up the wrong definition and report false positives. One codebase where this has been known to happen is Wordpress. In this case you can add the `@ignore` PhpDoc annotation to the duplicate definition and it will be ignored. Loading history...
98			}
99			}
100
101			// Check the new password
102			if(empty($data['NewPassword1'])) {
103			$this->clearMessage();
104			$this->sessionMessage(
105			_t('Member.EMPTYNEWPASSWORD', "The new password can't be empty, please try again"),
106			"bad");
107
108			// redirect back to the form, instead of using redirectBack() which could send the user elsewhere.
109			return $this->controller->redirect($this->controller->Link('changepassword'));
			0 ignored issues – show Unused Code introduced 2016-06-29 00:25 UTC by Report Bug Copy Issue Report The call to `Controller::Link()` has too many arguments starting with `'changepassword'`. This check compares calls to functions or methods with their respective definitions. If the call has more arguments than are defined, it raises an issue. If a function is defined several times with a different number of parameters, the check may pick up the wrong definition and report false positives. One codebase where this has been known to happen is Wordpress. In this case you can add the `@ignore` PhpDoc annotation to the duplicate definition and it will be ignored. Loading history...
110			}
111			else if($data['NewPassword1'] == $data['NewPassword2']) {
112			$isValid = $member->changePassword($data['NewPassword1']);
113			if($isValid->valid()) {
114			$member->logIn();
115
116			// TODO Add confirmation message to login redirect
117			Session::clear('AutoLoginHash');
118
119			// Clear locked out status
120			$member->LockedOutUntil = null;
121			$member->FailedLoginCount = null;
122			$member->write();
123
124			if (!empty($_REQUEST['BackURL'])
125			// absolute redirection URLs may cause spoofing
126			&& Director::is_site_url($_REQUEST['BackURL'])
127			) {
128			$url = Director::absoluteURL($_REQUEST['BackURL']);
129			return $this->controller->redirect($url);
			0 ignored issues – show Security Bug introduced 2016-06-29 00:25 UTC by Report Bug Copy Issue Report It seems like `$url` defined by `\Director::absoluteURL($_REQUEST['BackURL'])` on line `128` can also be of type `false`; however, `Controller::redirect()` does only seem to accept `string`, did you maybe forget to handle an error condition? This check looks for type mismatches where the missing type is `false`. This is usually indicative of an error condtion. Consider the follow example <?php function getDate($date) { if ($date !== null) { return new DateTime($date); } return false; } This function either returns a new `DateTime` object or false, if there was an error. This is a typical pattern in PHP programming to show that an error has occurred without raising an exception. The calling code should check for this returned `false` before passing on the value to another function or method that may not be able to handle a `false`. Loading history...
130			}
131			else {
132			// Redirect to default location - the login form saying "You are logged in as..."
133			$redirectURL = HTTP::setGetVar(
134			'BackURL',
135			Director::absoluteBaseURL(), $this->controller->Link('login')
			0 ignored issues – show Unused Code introduced 2016-06-29 00:25 UTC by Report Bug Copy Issue Report The call to `Controller::Link()` has too many arguments starting with `'login'`. This check compares calls to functions or methods with their respective definitions. If the call has more arguments than are defined, it raises an issue. If a function is defined several times with a different number of parameters, the check may pick up the wrong definition and report false positives. One codebase where this has been known to happen is Wordpress. In this case you can add the `@ignore` PhpDoc annotation to the duplicate definition and it will be ignored. Loading history... Security Bug introduced 2016-06-29 00:25 UTC by Report Bug Copy Issue Report It seems like `\Director::absoluteBaseURL()` targeting `Director::absoluteBaseURL()` can also be of type `false`; however, `HTTP::setGetVar()` does only seem to accept `string`, did you maybe forget to handle an error condition? Loading history...
136			);
137			return $this->controller->redirect($redirectURL);
138			}
139			} else {
140			$this->clearMessage();
141			$this->sessionMessage(
142			_t(
143			'Member.INVALIDNEWPASSWORD',
144			"We couldn't accept that password: {password}",
145			array('password' => nl2br("\n".Convert::raw2xml($isValid->starredList())))
146			),
147			"bad",
148			false
149			);
150
151			// redirect back to the form, instead of using redirectBack() which could send the user elsewhere.
152			return $this->controller->redirect($this->controller->Link('changepassword'));
			0 ignored issues – show Unused Code introduced 2016-06-29 00:25 UTC by Report Bug Copy Issue Report The call to `Controller::Link()` has too many arguments starting with `'changepassword'`. This check compares calls to functions or methods with their respective definitions. If the call has more arguments than are defined, it raises an issue. If a function is defined several times with a different number of parameters, the check may pick up the wrong definition and report false positives. One codebase where this has been known to happen is Wordpress. In this case you can add the `@ignore` PhpDoc annotation to the duplicate definition and it will be ignored. Loading history...
153			}
154
155			} else {
156			$this->clearMessage();
157			$this->sessionMessage(
158			_t('Member.ERRORNEWPASSWORD', "You have entered your new password differently, try again"),
159			"bad");
160
161			// redirect back to the form, instead of using redirectBack() which could send the user elsewhere.
162			return $this->controller->redirect($this->controller->Link('changepassword'));
			0 ignored issues – show Unused Code introduced 2016-06-29 00:25 UTC by Report Bug Copy Issue Report The call to `Controller::Link()` has too many arguments starting with `'changepassword'`. This check compares calls to functions or methods with their respective definitions. If the call has more arguments than are defined, it raises an issue. If a function is defined several times with a different number of parameters, the check may pick up the wrong definition and report false positives. One codebase where this has been known to happen is Wordpress. In this case you can add the `@ignore` PhpDoc annotation to the duplicate definition and it will be ignored. Loading history...
163			}
164			}
165
166			}
167
168

silverstripe / silverstripe-framework

Pull Request — master (#5741)

ChangePasswordForm::doChangePassword() C

Complexity

Size

Duplication

Importance

How to fix Long Method Complexity

Long Method

Duplication Side-by-Side

Filter issues like