LDAPChangePasswordForm

Complexity

Total Complexity

22

Size/Duplication

Total Lines	151
Duplicated Lines	26.49 %

Coupling/Cohesion

Components	1
Dependencies	12

Importance

Changes	6
Bugs	0	Features	1

2 Methods

Rating	Name	Duplication	Size	Complexity
D	__construct()	11	38	9
D	doChangePassword()	29	100	13

<?php

class LDAPChangePasswordForm extends ChangePasswordForm

PSR1 recommends that each class must be in a namespace of at least one level to avoid collisions.

{
    /**
     * The sole purpose for overriding the constructor is surfacing the username to the user.
     */
    public function __construct($controller, $name, $fields = null, $actions = null)
    {
        parent::__construct($controller, $name, $fields, $actions);

        // Obtain the Member object. If the user got this far, they must have already been synced.
        $member = Member::currentUser();
        if (!$member) {

This code seems to be duplicated across your project.

            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'));

The call to Controller::Link() has too many arguments starting with 'login'.

Constructors do not have meaningful return values, anything that is returned from here is discarded. Are you sure this is correct?

            }
        }

        $data = Injector::inst()->get('LDAPService')->getUserByGUID($member->GUID, array('samaccountname'));

        $emailField = null;
        $usernameField = null;
        if (Config::inst()->get('LDAPAuthenticator', 'allow_email_login')==='yes' && !empty($member->Email)) {
            $emailField = new TextField('Email', _t('LDAPLoginForm.USERNAMEOREMAIL', 'Email'), $member->Email, null, $this);
        }
        if (!empty($data['samaccountname'])) {
            $usernameField = new TextField('Username', _t('LDAPLoginForm.USERNAME', 'Username'), $data['samaccountname'], null, $this);
        }

        if ($emailField) {
            $emailFieldReadonly = $emailField->performDisabledTransformation();
            $this->Fields()->unshift($emailFieldReadonly);
        }
        if ($usernameField) {
            $usernameFieldReadonly = $usernameField->performDisabledTransformation();
            $this->Fields()->unshift($usernameFieldReadonly);
        }
    }

    /**
     * Change the password
     *
     * @param array $data The user submitted data
     * @return SS_HTTPResponse
     */
    public function doChangePassword(array $data)

doChangePassword uses the super-global variable $_REQUEST which is generally not recommended.

    {
        /**
         * @var LDAPService $service
         */
        $service = Injector::inst()->get('LDAPService');
        $member = Member::currentUser();
        if ($member) {
            try {
                $userData = $service->getUserByGUID($member->GUID);
            } catch (Exception $e) {
                SS_Log::log($e->getMessage(), SS_Log::ERR);
                $this->clearMessage();
                $this->sessionMessage(
                    _t('LDAPAuthenticator.NOUSER', 'Your account hasn\'t been setup properly, please contact an administrator.'),
                    'bad'
                );
                return $this->controller->redirect($this->controller->Link('changepassword'));

The call to Controller::Link() has too many arguments starting with 'changepassword'.

            }
            $loginResult = $service->authenticate($userData['samaccountname'], $data['OldPassword']);
            if (!$loginResult['success']) {

This code seems to be duplicated across your project.

                $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'));

The call to Controller::Link() has too many arguments starting with 'changepassword'.

            }
        }

        if (!$member) {

This code seems to be duplicated across your project.

            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'));

The call to Controller::Link() has too many arguments starting with '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'));

The call to Controller::Link() has too many arguments starting with '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;
            $isValid = $service->setPassword($member, $data['NewPassword1'], $oldPassword);

$member of type object<DataObject> is not a sub-type of object<Member>. It seems like you assume a child class of the class DataObject to be always present.

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

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

The call to Controller::Link() has too many arguments starting with 'login'.

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?

                    );
                    return $this->controller->redirect($redirectURL);
                }
            } else {
                $this->clearMessage();
                $this->sessionMessage($isValid->message(), "bad");
                // redirect back to the form, instead of using redirectBack() which could send the user elsewhere.
                return $this->controller->redirect($this->controller->Link('changepassword'));

The call to Controller::Link() has too many arguments starting with 'changepassword'.

            }
        } else {

This code seems to be duplicated across your project.

            $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'));

The call to Controller::Link() has too many arguments starting with 'changepassword'.

        }
    }
}