LDAPChangePasswordHandler::doChangePassword() - Code Metrics - Inspection of "SS4 security changes and unit test fixes" - silverstripe/silverstripe-activedirectory - Measure and Improve Code Quality continuously with Scrutinizer

Completed

Pull Request — master (#116)

by Franco

created 2017-09-04 04:37 UTC

LDAPChangePasswordHandler::doChangePassword() D

↳ Parent: LDAPChangePasswordHandler

Complexity

Conditions	13
Paths	54

Size

Total Lines	119
Code Lines	70

Duplication

Lines	25
Ratio	21.01 %

Importance

Changes

Metric	Value
dl	25
loc	119
rs	4.9922
c	0
b	0
f	0
cc	13
eloc	70
nc	54
nop	2

How to fix Long Method Complexity

<?php

namespace SilverStripe\ActiveDirectory\Authenticators;

use Exception;
use SilverStripe\ActiveDirectory\Forms\LDAPChangePasswordForm;
use SilverStripe\ActiveDirectory\Services\LDAPService;
use SilverStripe\Control\Director;
use SilverStripe\Control\HTTP;
use SilverStripe\Control\HTTPResponse;
use SilverStripe\Core\Injector\Injector;
use SilverStripe\ORM\ValidationResult;
use SilverStripe\Security\Member;
use SilverStripe\Security\MemberAuthenticator\ChangePasswordHandler;
use SilverStripe\Security\Security;

class LDAPChangePasswordHandler extends ChangePasswordHandler
{
    /**
     * @var array Allowed Actions
     */
    private static $allowed_actions = [

        'changepassword',
        'changePasswordForm',
    ];

    /**
     * Factory method for the lost password form
     *
     * @return LDAPChangePasswordForm Returns the lost password form
     */
    public function changePasswordForm()
    {
        return LDAPChangePasswordForm::create($this, 'ChangePasswordForm');
    }

    /**
     * Change the password
     *
     * @param array $data The user submitted data
     * @param LDAPChangePasswordForm $form
     * @return HTTPResponse
     */
    public function doChangePassword(array $data, $form)
// 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);
    }
}
    {
        /**
         * @var LDAPService $service
         */
        $service = Injector::inst()->get(LDAPService::class);
        $member = Security::getCurrentUser();
        if ($member) {
            try {
                $userData = $service->getUserByGUID($member->GUID);
            } catch (Exception $e) {
                Injector::inst()->get('Logger')->error($e->getMessage());

                $form->clearMessage();
                $form->sessionMessage(
                    _t(
                        __CLASS__ . '.NOUSER',
                        'Your account hasn\'t been setup properly, please contact an administrator.'
                    ),
                    'bad'
                );
                return $form->getController()->redirect($form->getController()->Link('changepassword'));
            }
            $loginResult = $service->authenticate($userData['samaccountname'], $data['OldPassword']);
            if (!$loginResult['success']) {

                $form->clearMessage();
                $form->sessionMessage(
                    _t(
                        'SilverStripe\\Security\\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 $form->getController()->redirect($form->getController()->Link('changepassword'));
            }
        }

        if (!$member) {
            if ($this->getRequest()->getSession()->get('AutoLoginHash')) {
                $member = Member::member_from_autologinhash($this->getRequest()->getSession()->get('AutoLoginHash'));
            }

            // The user is not logged in and no valid auto login hash is available
            if (!$member) {
                $this->getRequest()->getSession()->clear('AutoLoginHash');
                return $form->getController()->redirect($form->getController()->Link('login'));
            }
        }

        // Check the new password
        if (empty($data['NewPassword1'])) {
            $form->clearMessage();
            $form->sessionMessage(
                _t(
                    'SilverStripe\\Security\\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 $form->getController()->redirect($form->getController()->Link('changepassword'));
        } elseif ($data['NewPassword1'] == $data['NewPassword2']) {
            // Providing OldPassword to perform password _change_ operation. This will respect the
            // password history policy. Unfortunately we cannot support password history policy on password _reset_
            // at the moment, which means it will not be enforced on SilverStripe-driven email password reset.
            $oldPassword = !empty($data['OldPassword']) ? $data['OldPassword']: null;

            /** @var ValidationResult $validationResult */
            $validationResult = $service->setPassword($member, $data['NewPassword1'], $oldPassword);

            // try to catch connection and other errors that the ldap service can through
            if ($validationResult->isValid()) {
                Security::setCurrentUser($member);

                $this->getRequest()->getSession()->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 $form->getController()->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(),

                        $form->getController()->Link('login')
                    );
                    return $form->getController()->redirect($redirectURL);
                }
            } else {
                $form->clearMessage();
                $messages = implode('. ', array_column($validationResult->getMessages(), 'message'));
                $form->sessionMessage($messages, 'bad');
                // redirect back to the form, instead of using redirectBack() which could send the user elsewhere.
                return $form->getController()->redirect($form->getController()->Link('changepassword'));
            }
        } else {

            $form->clearMessage();
            $form->sessionMessage(
                _t(
                    'SilverStripe\\Security\\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 $form->getController()->redirect($form->getController()->Link('changepassword'));
        }
    }
}


1		<?php
2
3		namespace SilverStripe\ActiveDirectory\Authenticators;
4
5		use Exception;
6		use SilverStripe\ActiveDirectory\Forms\LDAPChangePasswordForm;
7		use SilverStripe\ActiveDirectory\Services\LDAPService;
8		use SilverStripe\Control\Director;
9		use SilverStripe\Control\HTTP;
10		use SilverStripe\Control\HTTPResponse;
11		use SilverStripe\Core\Injector\Injector;
12		use SilverStripe\ORM\ValidationResult;
13		use SilverStripe\Security\Member;
14		use SilverStripe\Security\MemberAuthenticator\ChangePasswordHandler;
15		use SilverStripe\Security\Security;
16
17		class LDAPChangePasswordHandler extends ChangePasswordHandler
18		{
19		/**
20		* @var array Allowed Actions
21		*/
22		private static $allowed_actions = [
		0 ignored issues – show Comprehensibility introduced 2017-01-16 21:03 UTC by Report Bug Copy Issue Report Consider using a different property name as you override a private property of the parent class. Loading history... Unused Code introduced 2017-01-16 21:03 UTC by Report Bug Copy Issue Report The property `$allowed_actions` is not used and could be removed. This check marks private properties in classes that are never used. Those properties can be removed. Loading history...
23		'changepassword',
24		'changePasswordForm',
25		];
26
27		/**
28		* Factory method for the lost password form
29		*
30		* @return LDAPChangePasswordForm Returns the lost password form
31		*/
32		public function changePasswordForm()
33		{
34		return LDAPChangePasswordForm::create($this, 'ChangePasswordForm');
35		}
36
37		/**
38		* Change the password
39		*
40		* @param array $data The user submitted data
41		* @param LDAPChangePasswordForm $form
42		* @return HTTPResponse
43		*/
44		public function doChangePassword(array $data, $form)
		0 ignored issues – show Coding Style introduced 2017-09-04 04:34 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...
45		{
46		/**
47		* @var LDAPService $service
48		*/
49		$service = Injector::inst()->get(LDAPService::class);
50		$member = Security::getCurrentUser();
51		if ($member) {
52		try {
53		$userData = $service->getUserByGUID($member->GUID);
54		} catch (Exception $e) {
55		Injector::inst()->get('Logger')->error($e->getMessage());
56
57		$form->clearMessage();
58		$form->sessionMessage(
59		_t(
60		__CLASS__ . '.NOUSER',
61		'Your account hasn\'t been setup properly, please contact an administrator.'
62		),
63		'bad'
64		);
65		return $form->getController()->redirect($form->getController()->Link('changepassword'));
66		}
67		$loginResult = $service->authenticate($userData['samaccountname'], $data['OldPassword']);
68	View Code Duplication	if (!$loginResult['success']) {
		0 ignored issues – show Duplication introduced 2016-03-22 20:04 UTC by Report Bug Copy Issue Report This code seems to be duplicated across your project. Duplicated code is one of the most pungent code smells. If you need to duplicate the same code in three or more different places, we strongly encourage you to look into extracting the code into a single class or operation. You can also find more detailed suggestions in the “Code” section of your repository. Loading history...
69		$form->clearMessage();
70		$form->sessionMessage(
71		_t(
72		'SilverStripe\\Security\\Member.ERRORPASSWORDNOTMATCH',
73		'Your current password does not match, please try again'
74		),
75		'bad'
76		);
77		// redirect back to the form, instead of using redirectBack() which could send the user elsewhere.
78		return $form->getController()->redirect($form->getController()->Link('changepassword'));
79		}
80		}
81
82		if (!$member) {
83		if ($this->getRequest()->getSession()->get('AutoLoginHash')) {
84		$member = Member::member_from_autologinhash($this->getRequest()->getSession()->get('AutoLoginHash'));
85		}
86
87		// The user is not logged in and no valid auto login hash is available
88		if (!$member) {
89		$this->getRequest()->getSession()->clear('AutoLoginHash');
90		return $form->getController()->redirect($form->getController()->Link('login'));
91		}
92		}
93
94		// Check the new password
95		if (empty($data['NewPassword1'])) {
96		$form->clearMessage();
97		$form->sessionMessage(
98		_t(
99		'SilverStripe\\Security\\Member.EMPTYNEWPASSWORD',
100		"The new password can't be empty, please try again"
101		),
102		'bad'
103		);
104
105		// redirect back to the form, instead of using redirectBack() which could send the user elsewhere.
106		return $form->getController()->redirect($form->getController()->Link('changepassword'));
107		} elseif ($data['NewPassword1'] == $data['NewPassword2']) {
108		// Providing OldPassword to perform password _change_ operation. This will respect the
109		// password history policy. Unfortunately we cannot support password history policy on password _reset_
110		// at the moment, which means it will not be enforced on SilverStripe-driven email password reset.
111		$oldPassword = !empty($data['OldPassword']) ? $data['OldPassword']: null;
112
113		/** @var ValidationResult $validationResult */
114		$validationResult = $service->setPassword($member, $data['NewPassword1'], $oldPassword);
115
116		// try to catch connection and other errors that the ldap service can through
117		if ($validationResult->isValid()) {
118		Security::setCurrentUser($member);
119
120		$this->getRequest()->getSession()->clear('AutoLoginHash');
121
122		// Clear locked out status
123		$member->LockedOutUntil = null;
124		$member->FailedLoginCount = null;
125		$member->write();
126
127		if (!empty($_REQUEST['BackURL'])
128		// absolute redirection URLs may cause spoofing
129		&& Director::is_site_url($_REQUEST['BackURL'])
130		) {
131		$url = Director::absoluteURL($_REQUEST['BackURL']);
132		return $form->getController()->redirect($url);
		0 ignored issues – show Security Bug introduced 2017-09-04 04:34 UTC by Report Bug Copy Issue Report It seems like `$url` defined by `\SilverStripe\Control\Di...L($_REQUEST['BackURL'])` on line `131` can also be of type `false`; however, `SilverStripe\Control\RequestHandler::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...
133		} else {
134		// Redirect to default location - the login form saying "You are logged in as..."
135		$redirectURL = HTTP::setGetVar(
136		'BackURL',
137		Director::absoluteBaseURL(),
		0 ignored issues – show Security Bug introduced 2017-01-16 21:03 UTC by Report Bug Copy Issue Report It seems like `\SilverStripe\Control\Director::absoluteBaseURL()` targeting `SilverStripe\Control\Director::absoluteBaseURL()` can also be of type `false`; however, `SilverStripe\Control\HTTP::setGetVar()` does only seem to accept `string`, did you maybe forget to handle an error condition? Loading history...
138		$form->getController()->Link('login')
139		);
140		return $form->getController()->redirect($redirectURL);
141		}
142		} else {
143		$form->clearMessage();
144		$messages = implode('. ', array_column($validationResult->getMessages(), 'message'));
145		$form->sessionMessage($messages, 'bad');
146		// redirect back to the form, instead of using redirectBack() which could send the user elsewhere.
147		return $form->getController()->redirect($form->getController()->Link('changepassword'));
148		}
149	View Code Duplication	} else {
		0 ignored issues – show Duplication introduced 2017-01-16 21:03 UTC by Report Bug Copy Issue Report This code seems to be duplicated across your project. Duplicated code is one of the most pungent code smells. If you need to duplicate the same code in three or more different places, we strongly encourage you to look into extracting the code into a single class or operation. You can also find more detailed suggestions in the “Code” section of your repository. Loading history...
150		$form->clearMessage();
151		$form->sessionMessage(
152		_t(
153		'SilverStripe\\Security\\Member.ERRORNEWPASSWORD',
154		'You have entered your new password differently, try again'
155		),
156		'bad'
157		);
158
159		// redirect back to the form, instead of using redirectBack() which could send the user elsewhere.
160		return $form->getController()->redirect($form->getController()->Link('changepassword'));
161		}
162		}
163		}
164

silverstripe / silverstripe-activedirectory

Pull Request — master (#116)

LDAPChangePasswordHandler::doChangePassword() D

Complexity

Size

Duplication

Importance

How to fix Long Method Complexity

Long Method

Duplication Side-by-Side

Filter issues like