Completed
Push — master ( 197387...875417 )
by Damian
11s
created

LDAPChangePasswordForm::doChangePassword()   D

Complexity

Conditions 13
Paths 54

Size

Total Lines 110
Code Lines 64

Duplication

Lines 29
Ratio 26.36 %

Importance

Changes 0
Metric Value
dl 29
loc 110
rs 4.9922
c 0
b 0
f 0
cc 13
eloc 64
nc 54
nop 1

How to fix   Long Method    Complexity   

Long Method

Small methods make your code easier to understand, in particular if combined with a good name. Besides, if your method is small, finding a good name is usually much easier.

For example, if you find yourself adding comments to a method's body, this is usually a good sign to extract the commented part to a new method, and use the comment as a starting point when coming up with a good name for this new method.

Commonly applied refactorings include:

1
<?php
2
3
namespace SilverStripe\ActiveDirectory\Forms;
4
5
use Exception;
6
use SilverStripe\Control\Director;
7
use SilverStripe\Control\HTTP;
8
use SilverStripe\Control\Session;
9
use SilverStripe\Core\Config\Config;
10
use SilverStripe\Core\Injector\Injector;
11
use SilverStripe\Forms\TextField;
12
use SilverStripe\Security\ChangePasswordForm;
13
use SilverStripe\Security\Member;
14
15
/**
16
 * @package activedirectory
17
 */
18
class LDAPChangePasswordForm extends ChangePasswordForm
19
{
20
    /**
21
     * The sole purpose for overriding the constructor is surfacing the username to the user.
22
     */
23
    public function __construct($controller, $name, $fields = null, $actions = null)
24
    {
25
        parent::__construct($controller, $name, $fields, $actions);
26
27
        // Obtain the Member object. If the user got this far, they must have already been synced.
28
        $member = Member::currentUser();
29 View Code Duplication
        if (!$member) {
0 ignored issues
show
Duplication introduced by
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...
30
            if (Session::get('AutoLoginHash')) {
31
                $member = Member::member_from_autologinhash(Session::get('AutoLoginHash'));
32
            }
33
34
            // The user is not logged in and no valid auto login hash is available
35
            if (!$member) {
36
                Session::clear('AutoLoginHash');
37
                return $this->controller->redirect($this->controller->Link('login'));
0 ignored issues
show
Bug introduced by
Constructors do not have meaningful return values, anything that is returned from here is discarded. Are you sure this is correct?
Loading history...
38
            }
39
        }
40
41
        $data = Injector::inst()
42
            ->get('SilverStripe\\ActiveDirectory\\Services\\LDAPService')
43
            ->getUserByGUID($member->GUID, ['samaccountname']);
44
45
        $emailField = null;
46
        $usernameField = null;
47
        if (Config::inst()->get('SilverStripe\\ActiveDirectory\\Authenticators\\LDAPAuthenticator', 'allow_email_login') === 'yes'
48
            && !empty($member->Email)
49
        ) {
50
            $emailField = TextField::create(
51
                'Email',
52
                _t('LDAPLoginForm.USERNAMEOREMAIL', 'Email'),
53
                $member->Email,
54
                null,
55
                $this
56
            );
57
        }
58
        if (!empty($data['samaccountname'])) {
59
            $usernameField = TextField::create(
60
                'Username',
61
                _t('LDAPLoginForm.USERNAME', 'Username'),
62
                $data['samaccountname'],
63
                null,
64
                $this
65
            );
66
        }
67
68
        if ($emailField) {
69
            $emailFieldReadonly = $emailField->performDisabledTransformation();
70
            $this->Fields()->unshift($emailFieldReadonly);
71
        }
72
        if ($usernameField) {
73
            $usernameFieldReadonly = $usernameField->performDisabledTransformation();
74
            $this->Fields()->unshift($usernameFieldReadonly);
75
        }
76
    }
77
78
    /**
79
     * Change the password
80
     *
81
     * @param array $data The user submitted data
82
     * @return HTTPResponse
83
     */
84
    public function doChangePassword(array $data)
0 ignored issues
show
Coding Style introduced by
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...
85
    {
86
        /**
87
         * @var LDAPService $service
88
         */
89
        $service = Injector::inst()->get('SilverStripe\\ActiveDirectory\\Services\\LDAPService');
90
        $member = Member::currentUser();
91
        if ($member) {
92
            try {
93
                $userData = $service->getUserByGUID($member->GUID);
94
            } catch (Exception $e) {
95
                Injector::inst()->get('Logger')->error($e->getMessage());
96
97
                $this->clearMessage();
98
                $this->sessionMessage(
99
                    _t(
100
                        'LDAPAuthenticator.NOUSER',
101
                        'Your account hasn\'t been setup properly, please contact an administrator.'
102
                    ),
103
                    'bad'
104
                );
105
                return $this->controller->redirect($this->controller->Link('changepassword'));
106
            }
107
            $loginResult = $service->authenticate($userData['samaccountname'], $data['OldPassword']);
108 View Code Duplication
            if (!$loginResult['success']) {
0 ignored issues
show
Duplication introduced by
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...
109
                $this->clearMessage();
110
                $this->sessionMessage(
111
                    _t('Member.ERRORPASSWORDNOTMATCH', 'Your current password does not match, please try again'),
112
                    'bad'
113
                );
114
                // redirect back to the form, instead of using redirectBack() which could send the user elsewhere.
115
                return $this->controller->redirect($this->controller->Link('changepassword'));
116
            }
117
        }
118
119 View Code Duplication
        if (!$member) {
0 ignored issues
show
Duplication introduced by
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...
120
            if (Session::get('AutoLoginHash')) {
121
                $member = Member::member_from_autologinhash(Session::get('AutoLoginHash'));
122
            }
123
124
            // The user is not logged in and no valid auto login hash is available
125
            if (!$member) {
126
                Session::clear('AutoLoginHash');
127
                return $this->controller->redirect($this->controller->Link('login'));
128
            }
129
        }
130
131
        // Check the new password
132
        if (empty($data['NewPassword1'])) {
133
            $this->clearMessage();
134
            $this->sessionMessage(
135
                _t('Member.EMPTYNEWPASSWORD', "The new password can't be empty, please try again"),
136
                'bad'
137
            );
138
139
            // redirect back to the form, instead of using redirectBack() which could send the user elsewhere.
140
            return $this->controller->redirect($this->controller->Link('changepassword'));
141
        } elseif ($data['NewPassword1'] == $data['NewPassword2']) {
142
            // Providing OldPassword to perform password _change_ operation. This will respect the
143
            // password history policy. Unfortunately we cannot support password history policy on password _reset_
144
            // at the moment, which means it will not be enforced on SilverStripe-driven email password reset.
145
            $oldPassword = !empty($data['OldPassword']) ? $data['OldPassword']: null;
146
147
            /** @var ValidationResult $validationResult */
148
            $validationResult = $service->setPassword($member, $data['NewPassword1'], $oldPassword);
149
150
            // try to catch connection and other errors that the ldap service can through
151
            if ($validationResult->isValid()) {
152
                $member->logIn();
153
154
                Session::clear('AutoLoginHash');
155
156
                // Clear locked out status
157
                $member->LockedOutUntil = null;
158
                $member->FailedLoginCount = null;
159
                $member->write();
160
161
                if (!empty($_REQUEST['BackURL'])
162
                    // absolute redirection URLs may cause spoofing
163
                    && Director::is_site_url($_REQUEST['BackURL'])
164
                ) {
165
                    $url = Director::absoluteURL($_REQUEST['BackURL']);
166
                    return $this->controller->redirect($url);
0 ignored issues
show
Security Bug introduced by
It seems like $url defined by \SilverStripe\Control\Di...L($_REQUEST['BackURL']) on line 165 can also be of type false; however, SilverStripe\Control\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...
167
                } else {
168
                    // Redirect to default location - the login form saying "You are logged in as..."
169
                    $redirectURL = HTTP::setGetVar(
170
                        'BackURL',
171
                        Director::absoluteBaseURL(),
0 ignored issues
show
Security Bug introduced by
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...
172
                        $this->controller->Link('login')
173
                    );
174
                    return $this->controller->redirect($redirectURL);
175
                }
176
            } else {
177
                $this->clearMessage();
178
                $messages = implode('. ', array_column($validationResult->getMessages(), 'message'));
179
                $this->sessionMessage($messages, 'bad');
180
                // redirect back to the form, instead of using redirectBack() which could send the user elsewhere.
181
                return $this->controller->redirect($this->controller->Link('changepassword'));
182
            }
183 View Code Duplication
        } else {
0 ignored issues
show
Duplication introduced by
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...
184
            $this->clearMessage();
185
            $this->sessionMessage(
186
                _t('Member.ERRORNEWPASSWORD', 'You have entered your new password differently, try again'),
187
                'bad'
188
            );
189
190
            // redirect back to the form, instead of using redirectBack() which could send the user elsewhere.
191
            return $this->controller->redirect($this->controller->Link('changepassword'));
192
        }
193
    }
194
}
195