Completed
Push — feature/multiple-second-factor... ( 036b5b )
by
unknown
17:57
created

SecondFactorController::chooseSecondFactorAction()   B

Complexity

Conditions 4
Paths 3

Size

Total Lines 52
Code Lines 33

Duplication

Lines 0
Ratio 0 %

Importance

Changes 0
Metric Value
dl 0
loc 52
c 0
b 0
f 0
rs 8.9408
cc 4
eloc 33
nc 3
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 2014 SURFnet bv
5
 *
6
 * Licensed under the Apache License, Version 2.0 (the "License");
7
 * you may not use this file except in compliance with the License.
8
 * You may obtain a copy of the License at
9
 *
10
 *     http://www.apache.org/licenses/LICENSE-2.0
11
 *
12
 * Unless required by applicable law or agreed to in writing, software
13
 * distributed under the License is distributed on an "AS IS" BASIS,
14
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15
 * See the License for the specific language governing permissions and
16
 * limitations under the License.
17
 */
18
19
namespace Surfnet\StepupGateway\GatewayBundle\Controller;
20
21
use Psr\Log\LoggerInterface;
22
use Sensio\Bundle\FrameworkExtraBundle\Configuration\Template;
23
use Surfnet\StepupBundle\Command\VerifyPossessionOfPhoneCommand;
24
use Surfnet\StepupBundle\Value\PhoneNumber\InternationalPhoneNumber;
25
use Surfnet\StepupGateway\GatewayBundle\Command\ChooseSecondFactorCommand;
26
use Surfnet\StepupGateway\GatewayBundle\Command\SendSmsChallengeCommand;
27
use Surfnet\StepupGateway\GatewayBundle\Command\VerifyYubikeyOtpCommand;
28
use Surfnet\StepupGateway\GatewayBundle\Entity\SecondFactor;
29
use Surfnet\StepupGateway\GatewayBundle\Exception\RuntimeException;
30
use Surfnet\StepupGateway\GatewayBundle\Saml\ResponseContext;
31
use Surfnet\StepupGateway\U2fVerificationBundle\Value\KeyHandle;
32
use Surfnet\StepupU2fBundle\Dto\SignResponse;
33
use Symfony\Bundle\FrameworkBundle\Controller\Controller;
34
use Symfony\Component\Form\FormError;
35
use Symfony\Component\HttpFoundation\RedirectResponse;
36
use Symfony\Component\HttpFoundation\Request;
37
use Symfony\Component\HttpFoundation\Response;
38
use Symfony\Component\HttpFoundation\Session\Attribute\AttributeBagInterface;
39
use Symfony\Component\HttpKernel\Exception\BadRequestHttpException;
40
41
/**
42
 * @SuppressWarnings(PHPMD.CouplingBetweenObjects)
43
 */
44
class SecondFactorController extends Controller
45
{
46
    public function selectSecondFactorForVerificationAction()
47
    {
48
        $context = $this->getResponseContext();
49
        $originalRequestId = $context->getInResponseTo();
50
51
        /** @var \Surfnet\SamlBundle\Monolog\SamlAuthenticationLogger $logger */
52
        $logger = $this->get('surfnet_saml.logger')->forAuthentication($originalRequestId);
53
        $logger->notice('Determining which second factor to use...');
54
55
        $requiredLoa = $this
56
            ->getStepupService()
57
            ->resolveHighestRequiredLoa(
58
                $context->getRequiredLoa(),
59
                $context->getIdentityNameId(),
60
                $context->getServiceProvider()
0 ignored issues
show
Bug introduced by
It seems like $context->getServiceProvider() can be null; however, resolveHighestRequiredLoa() 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...
61
            );
62
63
        if ($requiredLoa === null) {
64
            $logger->notice(
65
                'No valid required Loa can be determined, no authentication is possible, Loa cannot be given'
66
            );
67
68
            return $this->forward('SurfnetStepupGatewayGatewayBundle:Gateway:sendLoaCannotBeGiven');
69
        } else {
70
            $logger->notice(sprintf('Determined that the required Loa is "%s"', $requiredLoa));
71
        }
72
73
        if ($this->getStepupService()->isIntrinsicLoa($requiredLoa)) {
74
            $this->get('gateway.authentication_logger')->logIntrinsicLoaAuthentication($originalRequestId);
75
76
            return $this->forward($context->getResponseAction());
77
        }
78
79
        $secondFactorCollection = $this
80
            ->getStepupService()
81
            ->determineViableSecondFactors(
82
                $context->getIdentityNameId(),
83
                $requiredLoa,
84
                $this->get('gateway.service.whitelist')
85
            );
86
87
        switch (count($secondFactorCollection)) {
88
            case 0:
89
                $logger->notice('No second factors can give the determined Loa');
90
                return $this->forward('SurfnetStepupGatewayGatewayBundle:Gateway:sendLoaCannotBeGiven');
91
                break;
0 ignored issues
show
Unused Code introduced by
break is not strictly necessary here and could be removed.

The break statement is not necessary if it is preceded for example by a return statement:

switch ($x) {
    case 1:
        return 'foo';
        break; // This break is not necessary and can be left off.
}

If you would like to keep this construct to be consistent with other case statements, you can safely mark this issue as a false-positive.

Loading history...
92
93
            case 1:
94
                $secondFactor = $secondFactorCollection->first();
95
                $logger->notice(sprintf(
96
                    'Found "%d" second factors, using second factor of type "%s"',
97
                    count($secondFactorCollection),
98
                    $secondFactor->secondFactorType
99
                ));
100
101
                return $this->selectAndRedirectTo($secondFactor, $context);
102
                break;
0 ignored issues
show
Unused Code introduced by
break is not strictly necessary here and could be removed.

The break statement is not necessary if it is preceded for example by a return statement:

switch ($x) {
    case 1:
        return 'foo';
        break; // This break is not necessary and can be left off.
}

If you would like to keep this construct to be consistent with other case statements, you can safely mark this issue as a false-positive.

Loading history...
103
104
            default:
105
                return $this->forward(
106
                    'SurfnetStepupGatewayGatewayBundle:SecondFactor:chooseSecondFactor',
107
                    ['secondFactors' => $secondFactorCollection]
108
                );
109
                break;
0 ignored issues
show
Unused Code introduced by
break is not strictly necessary here and could be removed.

The break statement is not necessary if it is preceded for example by a return statement:

switch ($x) {
    case 1:
        return 'foo';
        break; // This break is not necessary and can be left off.
}

If you would like to keep this construct to be consistent with other case statements, you can safely mark this issue as a false-positive.

Loading history...
110
        }
111
    }
112
113
    /**
114
     * @Template
115
     * @param Request $request
116
     * @return array|RedirectResponse|Response
117
     */
118
    public function chooseSecondFactorAction(Request $request)
119
    {
120
        $context = $this->getResponseContext();
121
        $originalRequestId = $context->getInResponseTo();
122
123
        $requiredLoa = $this
124
            ->getStepupService()
125
            ->resolveHighestRequiredLoa(
126
                $context->getRequiredLoa(),
127
                $context->getIdentityNameId(),
128
                $context->getServiceProvider()
0 ignored issues
show
Bug introduced by
It seems like $context->getServiceProvider() can be null; however, resolveHighestRequiredLoa() 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...
129
            );
130
131
        $secondFactors = $this
132
            ->getStepupService()
133
            ->determineViableSecondFactors(
134
                $context->getIdentityNameId(),
135
                $requiredLoa,
0 ignored issues
show
Bug introduced by
It seems like $requiredLoa defined by $this->getStepupService(...->getServiceProvider()) on line 123 can be null; however, Surfnet\StepupGateway\Ga...neViableSecondFactors() 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...
136
                $this->get('gateway.service.whitelist')
137
            );
138
139
        $command = new ChooseSecondFactorCommand();
140
        $command->secondFactors = $secondFactors;
0 ignored issues
show
Documentation Bug introduced by
It seems like $secondFactors of type object<Doctrine\Common\Collections\Collection> is incompatible with the declared type array<integer,object<Sur...e\Entity\SecondFactor>> of property $secondFactors.

Our type inference engine has found an assignment to a property that is incompatible with the declared type of that property.

Either this assignment is in error or the assigned type should be added to the documentation/type hint for that property..

Loading history...
141
142
        $logger = $this->get('surfnet_saml.logger')->forAuthentication($originalRequestId);
143
144
        $form = $this
145
            ->createForm(
146
                'gateway_choose_second_factor', $command, [
147
                    'action' => $this->generateUrl('gateway_verify_second_factor_choose_second_factor')
148
                ]
149
            )
150
            ->handleRequest($request);
151
152
        if ($form->get('cancel')->isClicked()) {
0 ignored issues
show
Bug introduced by
It seems like you code against a concrete implementation and not the interface Symfony\Component\Form\FormInterface as the method isClicked() does only exist in the following implementations of said interface: Symfony\Component\Form\SubmitButton.

Let’s take a look at an example:

interface User
{
    /** @return string */
    public function getPassword();
}

class MyUser implements User
{
    public function getPassword()
    {
        // return something
    }

    public function getDisplayName()
    {
        // return some name.
    }
}

class AuthSystem
{
    public function authenticate(User $user)
    {
        $this->logger->info(sprintf('Authenticating %s.', $user->getDisplayName()));
        // do something.
    }
}

In the above example, the authenticate() method works fine as long as you just pass instances of MyUser. However, if you now also want to pass a different implementation of User which does not have a getDisplayName() method, the code will break.

Available Fixes

  1. Change the type-hint for the parameter:

    class AuthSystem
    {
        public function authenticate(MyUser $user) { /* ... */ }
    }
    
  2. Add an additional type-check:

    class AuthSystem
    {
        public function authenticate(User $user)
        {
            if ($user instanceof MyUser) {
                $this->logger->info(/** ... */);
            }
    
            // or alternatively
            if ( ! $user instanceof MyUser) {
                throw new \LogicException(
                    '$user must be an instance of MyUser, '
                   .'other instances are not supported.'
                );
            }
    
        }
    }
    
Note: PHP Analyzer uses reverse abstract interpretation to narrow down the types inside the if block in such a case.
  1. Add the method to the interface:

    interface User
    {
        /** @return string */
        public function getPassword();
    
        /** @return string */
        public function getDisplayName();
    }
    
Loading history...
153
            return $this->forward('SurfnetStepupGatewayGatewayBundle:Gateway:sendAuthenticationCancelledByUser');
154
        }
155
156
        if ($form->isSubmitted() && $form->isValid()) {
157
            $secondFactorIndex = $command->selectedSecondFactor;
158
            $secondFactor = $secondFactors->get($secondFactorIndex);
0 ignored issues
show
Documentation introduced by
$secondFactorIndex is of type object<Surfnet\StepupGat...le\Entity\SecondFactor>, but the function expects a string|integer.

It seems like the type of the argument is not accepted by the function/method which you are calling.

In some cases, in particular if PHP’s automatic type-juggling kicks in this might be fine. In other cases, however this might be a bug.

We suggest to add an explicit type cast like in the following example:

function acceptsInteger($int) { }

$x = '123'; // string "123"

// Instead of
acceptsInteger($x);

// we recommend to use
acceptsInteger((integer) $x);
Loading history...
159
            $logger->notice(sprintf(
160
                'User chose "%s" to use as second factor',
161
                $secondFactor->secondFactorType
162
            ));
163
164
            // Forward to action to verify possession of second factor
165
            return $this->selectAndRedirectTo($secondFactor, $context);
166
        }
167
168
        return ['form' => $form->createView()];
169
    }
170
171
    public function verifyGssfAction()
172
    {
173
        $context = $this->getResponseContext();
174
        $originalRequestId = $context->getInResponseTo();
175
176
        /** @var \Surfnet\SamlBundle\Monolog\SamlAuthenticationLogger $logger */
177
        $logger = $this->get('surfnet_saml.logger')->forAuthentication($originalRequestId);
178
        $logger->info('Received request to verify GSSF');
179
180
        $selectedSecondFactor = $this->getSelectedSecondFactor($context, $logger);
181
182
        $logger->info(sprintf(
183
            'Selected GSSF "%s" for verfication, forwarding to Saml handling',
184
            $selectedSecondFactor
185
        ));
186
187
        /** @var \Surfnet\StepupGateway\GatewayBundle\Service\SecondFactorService $secondFactorService */
188
        $secondFactorService = $this->get('gateway.service.second_factor_service');
189
        /** @var \Surfnet\StepupGateway\GatewayBundle\Entity\SecondFactor $secondFactor */
190
        $secondFactor = $secondFactorService->findByUuid($selectedSecondFactor);
191
        if (!$secondFactor) {
192
            $logger->critical(sprintf(
193
                'Requested verification of GSSF "%s", however that Second Factor no longer exists',
194
                $selectedSecondFactor
195
            ));
196
197
            throw new RuntimeException('Verification of selected second factor that no longer exists');
198
        }
199
200
        return $this->forward(
201
            'SurfnetStepupGatewaySamlStepupProviderBundle:SamlProxy:sendSecondFactorVerificationAuthnRequest',
202
            [
203
                'provider' => $secondFactor->secondFactorType,
204
                'subjectNameId' => $secondFactor->secondFactorIdentifier
205
            ]
206
        );
207
    }
208
209
    public function gssfVerifiedAction()
210
    {
211
        $context = $this->getResponseContext();
212
        $originalRequestId = $context->getInResponseTo();
213
214
        /** @var \Surfnet\SamlBundle\Monolog\SamlAuthenticationLogger $logger */
215
        $logger = $this->get('surfnet_saml.logger')->forAuthentication($originalRequestId);
216
        $logger->info('Attempting to mark GSSF as verified');
217
218
        $selectedSecondFactor = $this->getSelectedSecondFactor($context, $logger);
219
220
        /** @var \Surfnet\StepupGateway\GatewayBundle\Entity\SecondFactor $secondFactor */
221
        $secondFactor = $this->get('gateway.service.second_factor_service')->findByUuid($selectedSecondFactor);
222
        if (!$secondFactor) {
223
            $logger->critical(sprintf(
224
                'Verification of GSSF "%s" succeeded, however that Second Factor no longer exists',
225
                $selectedSecondFactor
226
            ));
227
228
            throw new RuntimeException('Verification of selected second factor that no longer exists');
229
        }
230
231
        $context->markSecondFactorVerified();
232
        $this->getAuthenticationLogger()->logSecondFactorAuthentication($originalRequestId);
233
234
        $logger->info(sprintf(
235
            'Marked GSSF "%s" as verified, forwarding to Gateway controller to respond',
236
            $selectedSecondFactor
237
        ));
238
239
        return $this->forward($context->getResponseAction());
240
    }
241
242
    /**
243
     * @Template
244
     * @param Request $request
245
     * @return array|Response
246
     */
247
    public function verifyYubiKeySecondFactorAction(Request $request)
248
    {
249
        $context = $this->getResponseContext();
250
        $originalRequestId = $context->getInResponseTo();
251
252
        /** @var \Surfnet\SamlBundle\Monolog\SamlAuthenticationLogger $logger */
253
        $logger = $this->get('surfnet_saml.logger')->forAuthentication($originalRequestId);
254
255
        $selectedSecondFactor = $this->getSelectedSecondFactor($context, $logger);
256
257
        $logger->notice('Verifying possession of Yubikey second factor');
258
259
        $command = new VerifyYubikeyOtpCommand();
260
        $command->secondFactorId = $selectedSecondFactor;
261
262
        $form = $this->createForm('gateway_verify_yubikey_otp', $command)->handleRequest($request);
263
264
        if ($form->get('cancel')->isClicked()) {
0 ignored issues
show
Bug introduced by
It seems like you code against a concrete implementation and not the interface Symfony\Component\Form\FormInterface as the method isClicked() does only exist in the following implementations of said interface: Symfony\Component\Form\SubmitButton.

Let’s take a look at an example:

interface User
{
    /** @return string */
    public function getPassword();
}

class MyUser implements User
{
    public function getPassword()
    {
        // return something
    }

    public function getDisplayName()
    {
        // return some name.
    }
}

class AuthSystem
{
    public function authenticate(User $user)
    {
        $this->logger->info(sprintf('Authenticating %s.', $user->getDisplayName()));
        // do something.
    }
}

In the above example, the authenticate() method works fine as long as you just pass instances of MyUser. However, if you now also want to pass a different implementation of User which does not have a getDisplayName() method, the code will break.

Available Fixes

  1. Change the type-hint for the parameter:

    class AuthSystem
    {
        public function authenticate(MyUser $user) { /* ... */ }
    }
    
  2. Add an additional type-check:

    class AuthSystem
    {
        public function authenticate(User $user)
        {
            if ($user instanceof MyUser) {
                $this->logger->info(/** ... */);
            }
    
            // or alternatively
            if ( ! $user instanceof MyUser) {
                throw new \LogicException(
                    '$user must be an instance of MyUser, '
                   .'other instances are not supported.'
                );
            }
    
        }
    }
    
Note: PHP Analyzer uses reverse abstract interpretation to narrow down the types inside the if block in such a case.
  1. Add the method to the interface:

    interface User
    {
        /** @return string */
        public function getPassword();
    
        /** @return string */
        public function getDisplayName();
    }
    
Loading history...
265
            return $this->forward('SurfnetStepupGatewayGatewayBundle:Gateway:sendAuthenticationCancelledByUser');
266
        }
267
268
        if (!$form->isValid()) {
269
            // OTP field is rendered empty in the template.
270
            return ['form' => $form->createView()];
271
        }
272
273
        $result = $this->getStepupService()->verifyYubikeyOtp($command);
274
275
        if ($result->didOtpVerificationFail()) {
276
            $form->addError(new FormError('gateway.form.verify_yubikey.otp_verification_failed'));
277
278
            // OTP field is rendered empty in the template.
279
            return ['form' => $form->createView()];
280
        } elseif (!$result->didPublicIdMatch()) {
281
            $form->addError(new FormError('gateway.form.verify_yubikey.public_id_mismatch'));
282
283
            // OTP field is rendered empty in the template.
284
            return ['form' => $form->createView()];
285
        }
286
287
        $this->getResponseContext()->markSecondFactorVerified();
288
        $this->getAuthenticationLogger()->logSecondFactorAuthentication($originalRequestId);
289
290
        $logger->info(
291
            sprintf(
292
                'Marked Yubikey Second Factor "%s" as verified, forwarding to Saml Proxy to respond',
293
                $selectedSecondFactor
294
            )
295
        );
296
297
        return $this->forward($context->getResponseAction());
298
    }
299
300
    /**
301
     * @Template
302
     * @param Request $request
303
     * @return array|Response
304
     */
305
    public function verifySmsSecondFactorAction(Request $request)
306
    {
307
        $context = $this->getResponseContext();
308
        $originalRequestId = $context->getInResponseTo();
309
310
        /** @var \Surfnet\SamlBundle\Monolog\SamlAuthenticationLogger $logger */
311
        $logger = $this->get('surfnet_saml.logger')->forAuthentication($originalRequestId);
312
313
        $selectedSecondFactor = $this->getSelectedSecondFactor($context, $logger);
314
315
        $logger->notice('Verifying possession of SMS second factor, preparing to send');
316
317
        $command = new SendSmsChallengeCommand();
318
        $command->secondFactorId = $selectedSecondFactor;
319
320
        $form = $this->createForm('gateway_send_sms_challenge', $command)->handleRequest($request);
321
322
        $stepupService = $this->getStepupService();
323
        $phoneNumber = InternationalPhoneNumber::fromStringFormat(
324
            $stepupService->getSecondFactorIdentifier($selectedSecondFactor)
325
        );
326
327
        $otpRequestsRemaining = $stepupService->getSmsOtpRequestsRemainingCount();
328
        $maximumOtpRequests = $stepupService->getSmsMaximumOtpRequestsCount();
329
        $viewVariables = ['otpRequestsRemaining' => $otpRequestsRemaining, 'maximumOtpRequests' => $maximumOtpRequests];
330
331
        if ($form->get('cancel')->isClicked()) {
0 ignored issues
show
Bug introduced by
It seems like you code against a concrete implementation and not the interface Symfony\Component\Form\FormInterface as the method isClicked() does only exist in the following implementations of said interface: Symfony\Component\Form\SubmitButton.

Let’s take a look at an example:

interface User
{
    /** @return string */
    public function getPassword();
}

class MyUser implements User
{
    public function getPassword()
    {
        // return something
    }

    public function getDisplayName()
    {
        // return some name.
    }
}

class AuthSystem
{
    public function authenticate(User $user)
    {
        $this->logger->info(sprintf('Authenticating %s.', $user->getDisplayName()));
        // do something.
    }
}

In the above example, the authenticate() method works fine as long as you just pass instances of MyUser. However, if you now also want to pass a different implementation of User which does not have a getDisplayName() method, the code will break.

Available Fixes

  1. Change the type-hint for the parameter:

    class AuthSystem
    {
        public function authenticate(MyUser $user) { /* ... */ }
    }
    
  2. Add an additional type-check:

    class AuthSystem
    {
        public function authenticate(User $user)
        {
            if ($user instanceof MyUser) {
                $this->logger->info(/** ... */);
            }
    
            // or alternatively
            if ( ! $user instanceof MyUser) {
                throw new \LogicException(
                    '$user must be an instance of MyUser, '
                   .'other instances are not supported.'
                );
            }
    
        }
    }
    
Note: PHP Analyzer uses reverse abstract interpretation to narrow down the types inside the if block in such a case.
  1. Add the method to the interface:

    interface User
    {
        /** @return string */
        public function getPassword();
    
        /** @return string */
        public function getDisplayName();
    }
    
Loading history...
332
            return $this->forward('SurfnetStepupGatewayGatewayBundle:Gateway:sendAuthenticationCancelledByUser');
333
        }
334
335
        if (!$form->isValid()) {
336
            return array_merge($viewVariables, ['phoneNumber' => $phoneNumber, 'form' => $form->createView()]);
337
        }
338
339
        $logger->notice('Verifying possession of SMS second factor, sending challenge per SMS');
340
341
        if (!$stepupService->sendSmsChallenge($command)) {
342
            $form->addError(new FormError('gateway.form.send_sms_challenge.sms_sending_failed'));
343
344
            return array_merge($viewVariables, ['phoneNumber' => $phoneNumber, 'form' => $form->createView()]);
345
        }
346
347
        return $this->redirect($this->generateUrl('gateway_verify_second_factor_sms_verify_challenge'));
348
    }
349
350
    /**
351
     * @Template
352
     * @param Request $request
353
     * @return array|Response
354
     */
355
    public function verifySmsSecondFactorChallengeAction(Request $request)
356
    {
357
        $context = $this->getResponseContext();
358
        $originalRequestId = $context->getInResponseTo();
359
360
        /** @var \Surfnet\SamlBundle\Monolog\SamlAuthenticationLogger $logger */
361
        $logger = $this->get('surfnet_saml.logger')->forAuthentication($originalRequestId);
362
363
        $selectedSecondFactor = $this->getSelectedSecondFactor($context, $logger);
364
365
        $command = new VerifyPossessionOfPhoneCommand();
366
        $form = $this->createForm('gateway_verify_sms_challenge', $command)->handleRequest($request);
367
368
        if ($form->get('cancel')->isClicked()) {
0 ignored issues
show
Bug introduced by
It seems like you code against a concrete implementation and not the interface Symfony\Component\Form\FormInterface as the method isClicked() does only exist in the following implementations of said interface: Symfony\Component\Form\SubmitButton.

Let’s take a look at an example:

interface User
{
    /** @return string */
    public function getPassword();
}

class MyUser implements User
{
    public function getPassword()
    {
        // return something
    }

    public function getDisplayName()
    {
        // return some name.
    }
}

class AuthSystem
{
    public function authenticate(User $user)
    {
        $this->logger->info(sprintf('Authenticating %s.', $user->getDisplayName()));
        // do something.
    }
}

In the above example, the authenticate() method works fine as long as you just pass instances of MyUser. However, if you now also want to pass a different implementation of User which does not have a getDisplayName() method, the code will break.

Available Fixes

  1. Change the type-hint for the parameter:

    class AuthSystem
    {
        public function authenticate(MyUser $user) { /* ... */ }
    }
    
  2. Add an additional type-check:

    class AuthSystem
    {
        public function authenticate(User $user)
        {
            if ($user instanceof MyUser) {
                $this->logger->info(/** ... */);
            }
    
            // or alternatively
            if ( ! $user instanceof MyUser) {
                throw new \LogicException(
                    '$user must be an instance of MyUser, '
                   .'other instances are not supported.'
                );
            }
    
        }
    }
    
Note: PHP Analyzer uses reverse abstract interpretation to narrow down the types inside the if block in such a case.
  1. Add the method to the interface:

    interface User
    {
        /** @return string */
        public function getPassword();
    
        /** @return string */
        public function getDisplayName();
    }
    
Loading history...
369
            return $this->forward('SurfnetStepupGatewayGatewayBundle:Gateway:sendAuthenticationCancelledByUser');
370
        }
371
372
        if (!$form->isValid()) {
373
            return ['form' => $form->createView()];
374
        }
375
376
        $logger->notice('Verifying input SMS challenge matches');
377
378
        $verification = $this->getStepupService()->verifySmsChallenge($command);
379
380
        if ($verification->wasSuccessful()) {
381
            $this->getStepupService()->clearSmsVerificationState();
382
383
            $this->getResponseContext()->markSecondFactorVerified();
384
            $this->getAuthenticationLogger()->logSecondFactorAuthentication($originalRequestId);
385
386
            $logger->info(
387
                sprintf(
388
                    'Marked Sms Second Factor "%s" as verified, forwarding to Saml Proxy to respond',
389
                    $selectedSecondFactor
390
                )
391
            );
392
393
            return $this->forward($context->getResponseAction());
394
        } elseif ($verification->didOtpExpire()) {
395
            $logger->notice('SMS challenge expired');
396
            $form->addError(new FormError('gateway.form.send_sms_challenge.challenge_expired'));
397
        } elseif ($verification->wasAttemptedTooManyTimes()) {
398
            $logger->notice('SMS challenge verification was attempted too many times');
399
            $form->addError(new FormError('gateway.form.send_sms_challenge.too_many_attempts'));
400
        } else {
401
            $logger->notice('SMS challenge did not match');
402
            $form->addError(new FormError('gateway.form.send_sms_challenge.sms_challenge_incorrect'));
403
        }
404
405
        return ['form' => $form->createView()];
406
    }
407
408
    /**
409
     * @Template
410
     */
411
    public function initiateU2fAuthenticationAction()
412
    {
413
        $context = $this->getResponseContext();
414
        $originalRequestId = $context->getInResponseTo();
415
416
        /** @var \Surfnet\SamlBundle\Monolog\SamlAuthenticationLogger $logger */
417
        $logger = $this->get('surfnet_saml.logger')->forAuthentication($originalRequestId);
418
419
        $selectedSecondFactor = $this->getSelectedSecondFactor($context, $logger);
420
        $stepupService = $this->getStepupService();
421
422
        $cancelFormAction = $this->generateUrl('gateway_verify_second_factor_u2f_cancel_authentication');
423
        $cancelForm =
424
            $this->createForm('gateway_cancel_second_factor_verification', null, ['action' => $cancelFormAction]);
425
426
        $logger->notice('Verifying possession of U2F second factor, looking for registration matching key handle');
427
428
        $service = $this->get('surfnet_stepup_u2f_verification.service.u2f_verification');
429
        $keyHandle = new KeyHandle($stepupService->getSecondFactorIdentifier($selectedSecondFactor));
430
        $registration = $service->findRegistrationByKeyHandle($keyHandle);
431
432
        if ($registration === null) {
433
            $logger->critical(
434
                sprintf('No known registration for key handle of second factor "%s"', $selectedSecondFactor)
435
            );
436
            $this->addFlash('error', 'gateway.u2f.alert.unknown_registration');
437
438
            return ['authenticationFailed' => true, 'cancelForm' => $cancelForm->createView()];
439
        }
440
441
        $logger->notice('Creating sign request');
442
443
        $signRequest = $service->createSignRequest($registration);
444
        $signResponse = new SignResponse();
445
446
        /** @var AttributeBagInterface $session */
447
        $session = $this->get('gateway.session.u2f');
448
        $session->set('request', $signRequest);
449
450
        $formAction = $this->generateUrl('gateway_verify_second_factor_u2f_verify_authentication');
451
        $form = $this->createForm(
452
            'surfnet_stepup_u2f_verify_device_authentication',
453
            $signResponse,
454
            ['sign_request' => $signRequest, 'action' => $formAction]
455
        );
456
457
        return ['form' => $form->createView(), 'cancelForm' => $cancelForm->createView()];
458
    }
459
460
    /**
461
     * @Template("SurfnetStepupGatewayGatewayBundle:SecondFactor:initiateU2fAuthentication.html.twig")
462
     *
463
     * @param Request $request
464
     * @return array|Response
465
     */
466
    public function verifyU2fAuthenticationAction(Request $request)
467
    {
468
        $context = $this->getResponseContext();
469
        $originalRequestId = $context->getInResponseTo();
470
471
        /** @var \Surfnet\SamlBundle\Monolog\SamlAuthenticationLogger $logger */
472
        $logger = $this->get('surfnet_saml.logger')->forAuthentication($originalRequestId);
473
474
        $selectedSecondFactor = $this->getSelectedSecondFactor($context, $logger);
475
476
        $logger->notice('Received sign response from device');
477
478
        /** @var AttributeBagInterface $session */
479
        $session = $this->get('gateway.session.u2f');
480
        $signRequest = $session->get('request');
481
        $signResponse = new SignResponse();
482
483
        $formAction = $this->generateUrl('gateway_verify_second_factor_u2f_verify_authentication');
484
        $form = $this
485
            ->createForm(
486
                'surfnet_stepup_u2f_verify_device_authentication',
487
                $signResponse,
488
                ['sign_request' => $signRequest, 'action' => $formAction]
489
            )
490
            ->handleRequest($request);
491
492
        $cancelFormAction = $this->generateUrl('gateway_verify_second_factor_u2f_cancel_authentication');
493
        $cancelForm =
494
            $this->createForm('gateway_cancel_second_factor_verification', null, ['action' => $cancelFormAction]);
495
496
        if (!$form->isValid()) {
497
            $logger->error('U2F authentication verification could not be started because device send illegal data');
498
            $this->addFlash('error', 'gateway.u2f.alert.error');
499
500
            return ['authenticationFailed' => true, 'cancelForm' => $cancelForm->createView()];
501
        }
502
503
        $service = $this->get('surfnet_stepup_u2f_verification.service.u2f_verification');
504
        $result = $service->verifyAuthentication($signRequest, $signResponse);
505
506
        if ($result->wasSuccessful()) {
507
            $context->markSecondFactorVerified();
508
            $this->getAuthenticationLogger()->logSecondFactorAuthentication($originalRequestId);
509
510
            $logger->info(
511
                sprintf(
512
                    'Marked U2F second factor "%s" as verified, forwarding to Saml Proxy to respond',
513
                    $selectedSecondFactor
514
                )
515
            );
516
517
            return $this->forward($context->getResponseAction());
518
        } elseif ($result->didDeviceReportError()) {
519
            $logger->error('U2F device reported error during authentication');
520
            $this->addFlash('error', 'gateway.u2f.alert.device_reported_an_error');
521
        } else {
522
            $logger->error('U2F authentication verification failed');
523
            $this->addFlash('error', 'gateway.u2f.alert.error');
524
        }
525
526
        return ['authenticationFailed' => true, 'cancelForm' => $cancelForm->createView()];
527
    }
528
529
    public function cancelU2fAuthenticationAction()
530
    {
531
        return $this->forward('SurfnetStepupGatewayGatewayBundle:Gateway:sendAuthenticationCancelledByUser');
532
    }
533
534
    /**
535
     * @return \Surfnet\StepupGateway\GatewayBundle\Service\StepupAuthenticationService
536
     */
537
    private function getStepupService()
538
    {
539
        return $this->get('gateway.service.stepup_authentication');
540
    }
541
542
    /**
543
     * @return ResponseContext
544
     */
545
    private function getResponseContext()
546
    {
547
        return $this->get($this->get('gateway.proxy.state_handler')->getResponseContextServiceId());
548
    }
549
550
    /**
551
     * @return \Surfnet\StepupGateway\GatewayBundle\Monolog\Logger\AuthenticationLogger
552
     */
553
    private function getAuthenticationLogger()
554
    {
555
        return $this->get('gateway.authentication_logger');
556
    }
557
558
    /**
559
     * @param ResponseContext $context
560
     * @param LoggerInterface $logger
561
     * @return string
562
     */
563
    private function getSelectedSecondFactor(ResponseContext $context, LoggerInterface $logger)
564
    {
565
        $selectedSecondFactor = $context->getSelectedSecondFactor();
566
567
        if (!$selectedSecondFactor) {
568
            $logger->error('Cannot verify possession of an unknown second factor');
569
570
            throw new BadRequestHttpException('Cannot verify possession of an unknown second factor.');
571
        }
572
573
        return $selectedSecondFactor;
574
    }
575
576
    private function selectAndRedirectTo(SecondFactor $secondFactor, ResponseContext $context)
577
    {
578
        $context->saveSelectedSecondFactor($secondFactor);
579
580
        $this->getStepupService()->clearSmsVerificationState();
581
582
        $route = 'gateway_verify_second_factor_';
583
        if ($secondFactor->isGssf()) {
584
            $route .= 'gssf';
585
        } else {
586
            $route .= strtolower($secondFactor->secondFactorType);
587
        }
588
589
        return $this->redirect($this->generateUrl($route));
590
    }
591
}
592