AbstractLogoutController::actionIndex()   B
last analyzed

Complexity

Conditions 8
Paths 7

Size

Total Lines 60

Duplication

Lines 0
Ratio 0 %

Importance

Changes 0
Metric Value
dl 0
loc 60
rs 7.6282
c 0
b 0
f 0
cc 8
nc 7
nop 1

How to fix   Long Method   

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
/**
4
 * @copyright  Copyright (c) Flipbox Digital Limited
5
 */
6
7
namespace flipbox\saml\core\controllers\messages;
8
9
use craft\db\Table;
10
use flipbox\saml\core\helpers\MessageHelper;
11
use flipbox\saml\core\models\AbstractSettings;
12
use flipbox\saml\core\records\AbstractProvider;
13
use flipbox\saml\core\records\AbstractProviderIdentity;
14
use flipbox\saml\core\records\ProviderInterface;
15
use flipbox\saml\core\services\bindings\Factory;
16
use SAML2\LogoutRequest;
17
use SAML2\LogoutResponse;
18
use yii\web\HttpException;
19
20
/**
21
 * Class AbstractLogoutController
22
 * @package flipbox\saml\core\controllers\messages
23
 */
24
abstract class AbstractLogoutController extends AbstractController implements \flipbox\saml\core\EnsureSAMLPlugin
25
{
26
27
    protected $allowAnonymous = [
28
        'actionIndex',
29
        'actionRequest',
30
    ];
31
32
    public $enableCsrfValidation = false;
33
34
    /**
35
     * @return ProviderInterface
36
     */
37
    abstract protected function getRemoteProvider($uid = null);
38
39
    /**
40
     * @param \yii\base\Action $action
41
     * @return bool
42
     */
43
    public function beforeAction($action)
44
    {
45
        if (in_array(
46
            $action->actionMethod,
47
            [
48
                'actionIndex',
49
                'actionRequest',
50
            ]
51
        )) {
52
            return true;
53
        }
54
        return parent::beforeAction($action);
55
    }
56
57
58
    /**
59
     * @return \yii\web\Response
60
     * @throws HttpException
61
     * @throws \flipbox\saml\core\exceptions\InvalidMetadata
62
     * @throws \yii\base\ExitException
63
     * @throws \yii\base\InvalidConfigException
64
     */
65
    public function actionIndex($uid = null)
66
    {
67
        $message = Factory::receive();
68
69
        $isRequest = $message instanceof LogoutRequest;
70
        $isResponse = $message instanceof LogoutResponse;
71
72
        if ((! $isRequest && ! $isResponse) ||
73
            $isResponse && $this->getPlugin()->getSession()->getRequestId() !== $message->getInResponseTo()) {
0 ignored issues
show
Documentation Bug introduced by
The method getSession does not exist on object<flipbox\saml\core\AbstractPlugin>? Since you implemented __call, maybe consider adding a @method annotation.

If you implement __call and you know which methods are available, you can improve IDE auto-completion and static analysis by adding a @method annotation to the class.

This is often the case, when __call is implemented by a parent class and only the child class knows which methods exist:

class ParentClass {
    private $data = array();

    public function __call($method, array $args) {
        if (0 === strpos($method, 'get')) {
            return $this->data[strtolower(substr($method, 3))];
        }

        throw new \LogicException(sprintf('Unsupported method: %s', $method));
    }
}

/**
 * If this class knows which fields exist, you can specify the methods here:
 *
 * @method string getName()
 */
class SomeClass extends ParentClass { }
Loading history...
74
            throw new HttpException(400, "Invalid request");
75
        }
76
77
        $settings = $this->getPlugin()->getSettings();
78
79
        /** @var AbstractProvider $theirProvider */
80
        $theirProvider = $this->getPlugin()->getProvider()->findByEntityId(
81
            MessageHelper::getIssuer($message->getIssuer())
82
        )->one();
83
        $condition = [
84
            'enabled' => 1
85
        ];
86
87
        if ($uid) {
88
            $condition['uid'] = $uid;
89
        } else {
90
            $condition['entityId'] = $settings->getEntityId();
91
        }
92
        /** @var AbstractProvider $ourProvider */
93
        $ourProvider = $this->getPlugin()->getProvider()->find($condition)->one();
94
95
        if ($isRequest) {
96
            if (\Craft::$app->getUser()->isGuest) {
0 ignored issues
show
Bug introduced by
The method getUser does only exist in yii\web\Application, but not in yii\console\Application.

It seems like the method you are trying to call exists only in some of the possible types.

Let’s take a look at an example:

class A
{
    public function foo() { }
}

class B extends A
{
    public function bar() { }
}

/**
 * @param A|B $x
 */
function someFunction($x)
{
    $x->foo(); // This call is fine as the method exists in A and B.
    $x->bar(); // This method only exists in B and might cause an error.
}

Available Fixes

  1. Add an additional type-check:

    /**
     * @param A|B $x
     */
    function someFunction($x)
    {
        $x->foo();
    
        if ($x instanceof B) {
            $x->bar();
        }
    }
    
  2. Only allow a single type to be passed if the variable comes from a parameter:

    function someFunction(B $x) { /** ... */ }
    
Loading history...
97
                $this->destroySpecifiedSession(
98
                    $message,
99
                    $theirProvider,
100
                    $this->getPlugin()->getSettings()
0 ignored issues
show
Bug introduced by
It seems like $this->getPlugin()->getSettings() can be null; however, destroySpecifiedSession() does not accept null, maybe add an additional type check?

Unless you are absolutely sure that the expression can never be null because of other conditions, we strongly recommend to add an additional type check to your code:

/** @return stdClass|null */
function mayReturnNull() { }

function doesNotAcceptNull(stdClass $x) { }

// With potential error.
function withoutCheck() {
    $x = mayReturnNull();
    doesNotAcceptNull($x); // Potential error here.
}

// Safe - Alternative 1
function withCheck1() {
    $x = mayReturnNull();
    if ( ! $x instanceof stdClass) {
        throw new \LogicException('$x must be defined.');
    }
    doesNotAcceptNull($x);
}

// Safe - Alternative 2
function withCheck2() {
    $x = mayReturnNull();
    if ($x instanceof stdClass) {
        doesNotAcceptNull($x);
    }
}
Loading history...
101
                );
102
            }
103
104
            /** @var LogoutResponse $response */
105
            $response = $this->getPlugin()->getLogoutResponse()->create(
106
                $message,
107
                $theirProvider,
108
                $ourProvider
109
            );
110
111
            /**
112
             * Add the request id to the the response.
113
             */
114
            $response->setInResponseTo($message->getId());
115
116
            \Craft::$app->user->logout(true);
117
            Factory::send($response, $theirProvider);
118
            \Craft::$app->end();
119
        }
120
121
        return $this->redirect(
122
            \Craft::$app->config->general->logoutPath
123
        );
124
    }
125
126
    /**
127
     * @param LogoutRequest $message
128
     * @param AbstractProvider $theirProvider
129
     * @throws \yii\db\Exception
130
     */
131
    protected function destroySpecifiedSession(
132
        LogoutRequest $message,
133
        AbstractProvider $theirProvider,
134
        AbstractSettings $settings
135
    ) {
136
        if (! $settings->sloDestroySpecifiedSessions) {
137
            return;
138
        }
139
140
        /** @var AbstractProviderIdentity $user */
141
        $user = $this->getPlugin()->getProviderIdentity()->findByNameId(
142
            $message->getNameId()->getValue(),
143
            $theirProvider
144
        )->one();
145
146
        if ($user) {
147
            \Craft::$app->getDb()->createCommand()
148
                ->delete(Table::SESSIONS, [
149
                    'userId' => $user->userId,
150
                ])
151
                ->execute();
152
        }
153
    }
154
155
156
    /**
157
     * @param null $uid
158
     * @throws \flipbox\saml\core\exceptions\InvalidMetadata
159
     * @throws \yii\base\ExitException
160
     * @throws \yii\base\InvalidConfigException
161
     */
162
    public function actionRequest($uid = null)
163
    {
164
        // Backwards compatibility with 1.0
165
        // The request shouldn't get here with a SAMLResponse
166
        if (\Craft::$app->request->getBodyParam('SAMLResponse')) {
167
            return $this->actionIndex();
168
        }
169
170
        /** @var AbstractProvider $theirProvider */
171
        $theirProvider = $this->getRemoteProvider($uid);
172
173
        /** @var AbstractProvider $ourProvider */
174
        $ourProvider = $this->getPlugin()->getProvider()->findOwn();
175
176
        $user = \Craft::$app->user->getIdentity();
177
178
        if (! $user || (
179
                $user &&
180
                ! $identity = $this->getPlugin()->getProviderIdentity()->findByUserAndProvider($user, $theirProvider)
181
            )
182
        ) {
183
            $saml = $this->getPlugin();
184
            $saml::warning('Logout not available. User is not logged in.');
185
            // Logout locally only
186
            return $this->redirect(
187
                \Craft::$app->config->general->logoutPath
188
            );
189
        }
190
191
        $logoutRequest = $this->getPlugin()->getLogoutRequest()->create(
192
            $theirProvider,
193
            $ourProvider,
194
            $identity,
0 ignored issues
show
Bug introduced by
The variable $identity does not seem to be defined for all execution paths leading up to this point.

If you define a variable conditionally, it can happen that it is not defined for all execution paths.

Let’s take a look at an example:

function myFunction($a) {
    switch ($a) {
        case 'foo':
            $x = 1;
            break;

        case 'bar':
            $x = 2;
            break;
    }

    // $x is potentially undefined here.
    echo $x;
}

In the above example, the variable $x is defined if you pass “foo” or “bar” as argument for $a. However, since the switch statement has no default case statement, if you pass any other value, the variable $x would be undefined.

Available Fixes

  1. Check for existence of the variable explicitly:

    function myFunction($a) {
        switch ($a) {
            case 'foo':
                $x = 1;
                break;
    
            case 'bar':
                $x = 2;
                break;
        }
    
        if (isset($x)) { // Make sure it's always set.
            echo $x;
        }
    }
    
  2. Define a default value for the variable:

    function myFunction($a) {
        $x = ''; // Set a default which gets overridden for certain paths.
        switch ($a) {
            case 'foo':
                $x = 1;
                break;
    
            case 'bar':
                $x = 2;
                break;
        }
    
        echo $x;
    }
    
  3. Add a value for the missing path:

    function myFunction($a) {
        switch ($a) {
            case 'foo':
                $x = 1;
                break;
    
            case 'bar':
                $x = 2;
                break;
    
            // We add support for the missing case.
            default:
                $x = '';
                break;
        }
    
        echo $x;
    }
    
Loading history...
195
            \Craft::$app->config->general->getPostLogoutRedirect()
196
        );
197
198
        /**
199
         * Save id to session so we can validate the response.
200
         */
201
        $this->getPlugin()->getSession()->setRequestId($logoutRequest->getId());
0 ignored issues
show
Documentation Bug introduced by
The method getSession does not exist on object<flipbox\saml\core\AbstractPlugin>? Since you implemented __call, maybe consider adding a @method annotation.

If you implement __call and you know which methods are available, you can improve IDE auto-completion and static analysis by adding a @method annotation to the class.

This is often the case, when __call is implemented by a parent class and only the child class knows which methods exist:

class ParentClass {
    private $data = array();

    public function __call($method, array $args) {
        if (0 === strpos($method, 'get')) {
            return $this->data[strtolower(substr($method, 3))];
        }

        throw new \LogicException(sprintf('Unsupported method: %s', $method));
    }
}

/**
 * If this class knows which fields exist, you can specify the methods here:
 *
 * @method string getName()
 */
class SomeClass extends ParentClass { }
Loading history...
202
203
        Factory::send($logoutRequest, $theirProvider);
204
        \Craft::$app->end();
205
    }
206
}
207