Completed
Pull Request — develop (#129)
by
unknown
04:20 queued 02:13
created

SecondFactorController::verifyGssfAction()   B

Complexity

Conditions 2
Paths 2

Size

Total Lines 37
Code Lines 20

Duplication

Lines 0
Ratio 0 %

Importance

Changes 0
Metric Value
dl 0
loc 37
rs 8.8571
c 0
b 0
f 0
cc 2
eloc 20
nc 2
nop 0
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\LoaCannotBeGivenException;
30
use Surfnet\StepupGateway\GatewayBundle\Exception\RuntimeException;
31
use Surfnet\StepupGateway\GatewayBundle\Saml\ResponseContext;
32
use Surfnet\StepupGateway\U2fVerificationBundle\Value\KeyHandle;
33
use Surfnet\StepupU2fBundle\Dto\SignResponse;
34
use Symfony\Bundle\FrameworkBundle\Controller\Controller;
35
use Symfony\Component\Form\FormError;
36
use Symfony\Component\HttpFoundation\RedirectResponse;
37
use Symfony\Component\HttpFoundation\Request;
38
use Symfony\Component\HttpFoundation\Response;
39
use Symfony\Component\HttpFoundation\Session\Attribute\AttributeBagInterface;
40
use Symfony\Component\HttpKernel\Exception\BadRequestHttpException;
41
42
/**
43
 * @SuppressWarnings(PHPMD.CouplingBetweenObjects)
44
 */
45
class SecondFactorController extends Controller
46
{
47
    public function selectSecondFactorForVerificationAction()
48
    {
49
        $context = $this->getResponseContext();
50
        $originalRequestId = $context->getInResponseTo();
51
52
        /** @var \Surfnet\SamlBundle\Monolog\SamlAuthenticationLogger $logger */
53
        $logger = $this->get('surfnet_saml.logger')->forAuthentication($originalRequestId);
54
        $logger->notice('Determining which second factor to use...');
55
56
        try {
57
            $requiredLoa = $this
58
                ->getStepupService()
59
                ->resolveHighestRequiredLoa(
60
                    $context->getRequiredLoa(),
61
                    $context->getIdentityNameId(),
62
                    $context->getSchacHomeOrganization(),
63
                    $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...
64
                );
65
        } catch (LoaCannotBeGivenException $e) {
66
            // Log the message of the domain exception, this contains a meaningful message.
67
            $logger->notice($e->getMessage());
68
            return $this->forward('SurfnetStepupGatewayGatewayBundle:Gateway:sendLoaCannotBeGiven');
69
        }
70
        if ($requiredLoa === null) {
71
            $logger->notice(
72
                'No valid required Loa can be determined, no authentication is possible, Loa cannot be given'
73
            );
74
75
            return $this->forward('SurfnetStepupGatewayGatewayBundle:Gateway:sendLoaCannotBeGiven');
76
        } else {
77
            $logger->notice(sprintf('Determined that the required Loa is "%s"', $requiredLoa));
78
        }
79
80
        if ($this->getStepupService()->isIntrinsicLoa($requiredLoa)) {
81
            $this->get('gateway.authentication_logger')->logIntrinsicLoaAuthentication($originalRequestId);
82
83
            return $this->forward($context->getResponseAction());
84
        }
85
86
        $secondFactorCollection = $this
87
            ->getStepupService()
88
            ->determineViableSecondFactors(
89
                $context->getIdentityNameId(),
90
                $requiredLoa,
91
                $this->get('gateway.service.whitelist')
92
            );
93
94
        switch (count($secondFactorCollection)) {
95
            case 0:
96
                $logger->notice('No second factors can give the determined Loa');
97
                return $this->forward('SurfnetStepupGatewayGatewayBundle:Gateway:sendLoaCannotBeGiven');
98
                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...
99
100
            case 1:
101
                $secondFactor = $secondFactorCollection->first();
102
                $logger->notice(sprintf(
103
                    'Found "%d" second factors, using second factor of type "%s"',
104
                    count($secondFactorCollection),
105
                    $secondFactor->secondFactorType
106
                ));
107
108
                return $this->selectAndRedirectTo($secondFactor, $context);
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
            default:
112
                return $this->forward(
113
                    'SurfnetStepupGatewayGatewayBundle:SecondFactor:chooseSecondFactor',
114
                    ['secondFactors' => $secondFactorCollection]
115
                );
116
                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...
117
        }
118
    }
119
120
    /**
121
     * @Template
122
     * @param Request $request
123
     * @return array|RedirectResponse|Response
124
     */
125
    public function chooseSecondFactorAction(Request $request)
126
    {
127
        $context = $this->getResponseContext();
128
        $originalRequestId = $context->getInResponseTo();
129
130
        $requiredLoa = $this
131
            ->getStepupService()
132
            ->resolveHighestRequiredLoa(
133
                $context->getRequiredLoa(),
134
                $context->getIdentityNameId(),
135
                $context->getSchacHomeOrganization(),
136
                $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...
137
            );
138
139
        $secondFactors = $this
140
            ->getStepupService()
141
            ->determineViableSecondFactors(
142
                $context->getIdentityNameId(),
143
                $requiredLoa,
0 ignored issues
show
Bug introduced by
It seems like $requiredLoa defined by $this->getStepupService(...->getServiceProvider()) on line 130 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...
144
                $this->get('gateway.service.whitelist')
145
            );
146
147
        $command = new ChooseSecondFactorCommand();
148
        $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...
149
150
        $logger = $this->get('surfnet_saml.logger')->forAuthentication($originalRequestId);
151
152
        $form = $this
153
            ->createForm(
154
                'gateway_choose_second_factor',
155
                $command,
156
                ['action' => $this->generateUrl('gateway_verify_second_factor_choose_second_factor')]
157
            )
158
            ->handleRequest($request);
159
160
        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...
161
            return $this->forward('SurfnetStepupGatewayGatewayBundle:Gateway:sendAuthenticationCancelledByUser');
162
        }
163
164
        if ($form->isSubmitted() && $form->isValid()) {
165
            $secondFactorIndex = $command->selectedSecondFactor;
166
            $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...
167
            $logger->notice(sprintf(
168
                'User chose "%s" to use as second factor',
169
                $secondFactor->secondFactorType
170
            ));
171
172
            // Forward to action to verify possession of second factor
173
            return $this->selectAndRedirectTo($secondFactor, $context);
174
        }
175
176
        return ['form' => $form->createView()];
177
    }
178
179
    public function verifyGssfAction()
180
    {
181
        $context = $this->getResponseContext();
182
        $originalRequestId = $context->getInResponseTo();
183
184
        /** @var \Surfnet\SamlBundle\Monolog\SamlAuthenticationLogger $logger */
185
        $logger = $this->get('surfnet_saml.logger')->forAuthentication($originalRequestId);
186
        $logger->info('Received request to verify GSSF');
187
188
        $selectedSecondFactor = $this->getSelectedSecondFactor($context, $logger);
189
190
        $logger->info(sprintf(
191
            'Selected GSSF "%s" for verfication, forwarding to Saml handling',
192
            $selectedSecondFactor
193
        ));
194
195
        /** @var \Surfnet\StepupGateway\GatewayBundle\Service\SecondFactorService $secondFactorService */
196
        $secondFactorService = $this->get('gateway.service.second_factor_service');
197
        /** @var \Surfnet\StepupGateway\GatewayBundle\Entity\SecondFactor $secondFactor */
198
        $secondFactor = $secondFactorService->findByUuid($selectedSecondFactor);
199
        if (!$secondFactor) {
200
            $logger->critical(sprintf(
201
                'Requested verification of GSSF "%s", however that Second Factor no longer exists',
202
                $selectedSecondFactor
203
            ));
204
205
            throw new RuntimeException('Verification of selected second factor that no longer exists');
206
        }
207
208
        return $this->forward(
209
            'SurfnetStepupGatewaySamlStepupProviderBundle:SamlProxy:sendSecondFactorVerificationAuthnRequest',
210
            [
211
                'provider' => $secondFactor->secondFactorType,
212
                'subjectNameId' => $secondFactor->secondFactorIdentifier
213
            ]
214
        );
215
    }
216
217
    public function gssfVerifiedAction()
218
    {
219
        $context = $this->getResponseContext();
220
        $originalRequestId = $context->getInResponseTo();
221
222
        /** @var \Surfnet\SamlBundle\Monolog\SamlAuthenticationLogger $logger */
223
        $logger = $this->get('surfnet_saml.logger')->forAuthentication($originalRequestId);
224
        $logger->info('Attempting to mark GSSF as verified');
225
226
        $selectedSecondFactor = $this->getSelectedSecondFactor($context, $logger);
227
228
        /** @var \Surfnet\StepupGateway\GatewayBundle\Entity\SecondFactor $secondFactor */
229
        $secondFactor = $this->get('gateway.service.second_factor_service')->findByUuid($selectedSecondFactor);
230
        if (!$secondFactor) {
231
            $logger->critical(sprintf(
232
                'Verification of GSSF "%s" succeeded, however that Second Factor no longer exists',
233
                $selectedSecondFactor
234
            ));
235
236
            throw new RuntimeException('Verification of selected second factor that no longer exists');
237
        }
238
239
        $context->markSecondFactorVerified();
240
        $this->getAuthenticationLogger()->logSecondFactorAuthentication($originalRequestId);
241
242
        $logger->info(sprintf(
243
            'Marked GSSF "%s" as verified, forwarding to Gateway controller to respond',
244
            $selectedSecondFactor
245
        ));
246
247
        return $this->forward($context->getResponseAction());
248
    }
249
250
    /**
251
     * @Template
252
     * @param Request $request
253
     * @return array|Response
254
     */
255
    public function verifyYubiKeySecondFactorAction(Request $request)
256
    {
257
        $context = $this->getResponseContext();
258
        $originalRequestId = $context->getInResponseTo();
259
260
        /** @var \Surfnet\SamlBundle\Monolog\SamlAuthenticationLogger $logger */
261
        $logger = $this->get('surfnet_saml.logger')->forAuthentication($originalRequestId);
262
263
        $selectedSecondFactor = $this->getSelectedSecondFactor($context, $logger);
264
265
        $logger->notice('Verifying possession of Yubikey second factor');
266
267
        $command = new VerifyYubikeyOtpCommand();
268
        $command->secondFactorId = $selectedSecondFactor;
269
270
        $form = $this->createForm('gateway_verify_yubikey_otp', $command)->handleRequest($request);
271
272
        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...
273
            return $this->forward('SurfnetStepupGatewayGatewayBundle:Gateway:sendAuthenticationCancelledByUser');
274
        }
275
276
        if (!$form->isValid()) {
277
            // OTP field is rendered empty in the template.
278
            return ['form' => $form->createView()];
279
        }
280
281
        $result = $this->getStepupService()->verifyYubikeyOtp($command);
282
283
        if ($result->didOtpVerificationFail()) {
284
            $form->addError(new FormError('gateway.form.verify_yubikey.otp_verification_failed'));
285
286
            // OTP field is rendered empty in the template.
287
            return ['form' => $form->createView()];
288
        } elseif (!$result->didPublicIdMatch()) {
289
            $form->addError(new FormError('gateway.form.verify_yubikey.public_id_mismatch'));
290
291
            // OTP field is rendered empty in the template.
292
            return ['form' => $form->createView()];
293
        }
294
295
        $this->getResponseContext()->markSecondFactorVerified();
296
        $this->getAuthenticationLogger()->logSecondFactorAuthentication($originalRequestId);
297
298
        $logger->info(
299
            sprintf(
300
                'Marked Yubikey Second Factor "%s" as verified, forwarding to Saml Proxy to respond',
301
                $selectedSecondFactor
302
            )
303
        );
304
305
        return $this->forward($context->getResponseAction());
306
    }
307
308
    /**
309
     * @Template
310
     * @param Request $request
311
     * @return array|Response
312
     */
313
    public function verifySmsSecondFactorAction(Request $request)
314
    {
315
        $context = $this->getResponseContext();
316
        $originalRequestId = $context->getInResponseTo();
317
318
        /** @var \Surfnet\SamlBundle\Monolog\SamlAuthenticationLogger $logger */
319
        $logger = $this->get('surfnet_saml.logger')->forAuthentication($originalRequestId);
320
321
        $selectedSecondFactor = $this->getSelectedSecondFactor($context, $logger);
322
323
        $logger->notice('Verifying possession of SMS second factor, preparing to send');
324
325
        $command = new SendSmsChallengeCommand();
326
        $command->secondFactorId = $selectedSecondFactor;
327
328
        $form = $this->createForm('gateway_send_sms_challenge', $command)->handleRequest($request);
329
330
        $stepupService = $this->getStepupService();
331
        $phoneNumber = InternationalPhoneNumber::fromStringFormat(
332
            $stepupService->getSecondFactorIdentifier($selectedSecondFactor)
333
        );
334
335
        $otpRequestsRemaining = $stepupService->getSmsOtpRequestsRemainingCount();
336
        $maximumOtpRequests = $stepupService->getSmsMaximumOtpRequestsCount();
337
        $viewVariables = ['otpRequestsRemaining' => $otpRequestsRemaining, 'maximumOtpRequests' => $maximumOtpRequests];
338
339
        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...
340
            return $this->forward('SurfnetStepupGatewayGatewayBundle:Gateway:sendAuthenticationCancelledByUser');
341
        }
342
343
        if (!$form->isValid()) {
344
            return array_merge($viewVariables, ['phoneNumber' => $phoneNumber, 'form' => $form->createView()]);
345
        }
346
347
        $logger->notice('Verifying possession of SMS second factor, sending challenge per SMS');
348
349
        if (!$stepupService->sendSmsChallenge($command)) {
350
            $form->addError(new FormError('gateway.form.send_sms_challenge.sms_sending_failed'));
351
352
            return array_merge($viewVariables, ['phoneNumber' => $phoneNumber, 'form' => $form->createView()]);
353
        }
354
355
        return $this->redirect($this->generateUrl('gateway_verify_second_factor_sms_verify_challenge'));
356
    }
357
358
    /**
359
     * @Template
360
     * @param Request $request
361
     * @return array|Response
362
     */
363
    public function verifySmsSecondFactorChallengeAction(Request $request)
364
    {
365
        $context = $this->getResponseContext();
366
        $originalRequestId = $context->getInResponseTo();
367
368
        /** @var \Surfnet\SamlBundle\Monolog\SamlAuthenticationLogger $logger */
369
        $logger = $this->get('surfnet_saml.logger')->forAuthentication($originalRequestId);
370
371
        $selectedSecondFactor = $this->getSelectedSecondFactor($context, $logger);
372
373
        $command = new VerifyPossessionOfPhoneCommand();
374
        $form = $this->createForm('gateway_verify_sms_challenge', $command)->handleRequest($request);
375
376
        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...
377
            return $this->forward('SurfnetStepupGatewayGatewayBundle:Gateway:sendAuthenticationCancelledByUser');
378
        }
379
380
        if (!$form->isValid()) {
381
            return ['form' => $form->createView()];
382
        }
383
384
        $logger->notice('Verifying input SMS challenge matches');
385
386
        $verification = $this->getStepupService()->verifySmsChallenge($command);
387
388
        if ($verification->wasSuccessful()) {
389
            $this->getStepupService()->clearSmsVerificationState();
390
391
            $this->getResponseContext()->markSecondFactorVerified();
392
            $this->getAuthenticationLogger()->logSecondFactorAuthentication($originalRequestId);
393
394
            $logger->info(
395
                sprintf(
396
                    'Marked Sms Second Factor "%s" as verified, forwarding to Saml Proxy to respond',
397
                    $selectedSecondFactor
398
                )
399
            );
400
401
            return $this->forward($context->getResponseAction());
402
        } elseif ($verification->didOtpExpire()) {
403
            $logger->notice('SMS challenge expired');
404
            $form->addError(new FormError('gateway.form.send_sms_challenge.challenge_expired'));
405
        } elseif ($verification->wasAttemptedTooManyTimes()) {
406
            $logger->notice('SMS challenge verification was attempted too many times');
407
            $form->addError(new FormError('gateway.form.send_sms_challenge.too_many_attempts'));
408
        } else {
409
            $logger->notice('SMS challenge did not match');
410
            $form->addError(new FormError('gateway.form.send_sms_challenge.sms_challenge_incorrect'));
411
        }
412
413
        return ['form' => $form->createView()];
414
    }
415
416
    /**
417
     * @Template
418
     */
419
    public function initiateU2fAuthenticationAction()
420
    {
421
        $context = $this->getResponseContext();
422
        $originalRequestId = $context->getInResponseTo();
423
424
        /** @var \Surfnet\SamlBundle\Monolog\SamlAuthenticationLogger $logger */
425
        $logger = $this->get('surfnet_saml.logger')->forAuthentication($originalRequestId);
426
427
        $selectedSecondFactor = $this->getSelectedSecondFactor($context, $logger);
428
        $stepupService = $this->getStepupService();
429
430
        $cancelFormAction = $this->generateUrl('gateway_verify_second_factor_u2f_cancel_authentication');
431
        $cancelForm =
432
            $this->createForm('gateway_cancel_second_factor_verification', null, ['action' => $cancelFormAction]);
433
434
        $logger->notice('Verifying possession of U2F second factor, looking for registration matching key handle');
435
436
        $service = $this->get('surfnet_stepup_u2f_verification.service.u2f_verification');
437
        $keyHandle = new KeyHandle($stepupService->getSecondFactorIdentifier($selectedSecondFactor));
438
        $registration = $service->findRegistrationByKeyHandle($keyHandle);
439
440
        if ($registration === null) {
441
            $logger->critical(
442
                sprintf('No known registration for key handle of second factor "%s"', $selectedSecondFactor)
443
            );
444
            $this->addFlash('error', 'gateway.u2f.alert.unknown_registration');
445
446
            return ['authenticationFailed' => true, 'cancelForm' => $cancelForm->createView()];
447
        }
448
449
        $logger->notice('Creating sign request');
450
451
        $signRequest = $service->createSignRequest($registration);
452
        $signResponse = new SignResponse();
453
454
        /** @var AttributeBagInterface $session */
455
        $session = $this->get('gateway.session.u2f');
456
        $session->set('request', $signRequest);
457
458
        $formAction = $this->generateUrl('gateway_verify_second_factor_u2f_verify_authentication');
459
        $form = $this->createForm(
460
            'surfnet_stepup_u2f_verify_device_authentication',
461
            $signResponse,
462
            ['sign_request' => $signRequest, 'action' => $formAction]
463
        );
464
465
        return ['form' => $form->createView(), 'cancelForm' => $cancelForm->createView()];
466
    }
467
468
    /**
469
     * @Template("SurfnetStepupGatewayGatewayBundle:SecondFactor:initiateU2fAuthentication.html.twig")
470
     *
471
     * @param Request $request
472
     * @return array|Response
473
     */
474
    public function verifyU2fAuthenticationAction(Request $request)
475
    {
476
        $context = $this->getResponseContext();
477
        $originalRequestId = $context->getInResponseTo();
478
479
        /** @var \Surfnet\SamlBundle\Monolog\SamlAuthenticationLogger $logger */
480
        $logger = $this->get('surfnet_saml.logger')->forAuthentication($originalRequestId);
481
482
        $selectedSecondFactor = $this->getSelectedSecondFactor($context, $logger);
483
484
        $logger->notice('Received sign response from device');
485
486
        /** @var AttributeBagInterface $session */
487
        $session = $this->get('gateway.session.u2f');
488
        $signRequest = $session->get('request');
489
        $signResponse = new SignResponse();
490
491
        $formAction = $this->generateUrl('gateway_verify_second_factor_u2f_verify_authentication');
492
        $form = $this
493
            ->createForm(
494
                'surfnet_stepup_u2f_verify_device_authentication',
495
                $signResponse,
496
                ['sign_request' => $signRequest, 'action' => $formAction]
497
            )
498
            ->handleRequest($request);
499
500
        $cancelFormAction = $this->generateUrl('gateway_verify_second_factor_u2f_cancel_authentication');
501
        $cancelForm =
502
            $this->createForm('gateway_cancel_second_factor_verification', null, ['action' => $cancelFormAction]);
503
504
        if (!$form->isValid()) {
505
            $logger->error('U2F authentication verification could not be started because device send illegal data');
506
            $this->addFlash('error', 'gateway.u2f.alert.error');
507
508
            return ['authenticationFailed' => true, 'cancelForm' => $cancelForm->createView()];
509
        }
510
511
        $service = $this->get('surfnet_stepup_u2f_verification.service.u2f_verification');
512
        $result = $service->verifyAuthentication($signRequest, $signResponse);
513
514
        if ($result->wasSuccessful()) {
515
            $context->markSecondFactorVerified();
516
            $this->getAuthenticationLogger()->logSecondFactorAuthentication($originalRequestId);
517
518
            $logger->info(
519
                sprintf(
520
                    'Marked U2F second factor "%s" as verified, forwarding to Saml Proxy to respond',
521
                    $selectedSecondFactor
522
                )
523
            );
524
525
            return $this->forward($context->getResponseAction());
526
        } elseif ($result->didDeviceReportError()) {
527
            $logger->error('U2F device reported error during authentication');
528
            $this->addFlash('error', 'gateway.u2f.alert.device_reported_an_error');
529
        } else {
530
            $logger->error('U2F authentication verification failed');
531
            $this->addFlash('error', 'gateway.u2f.alert.error');
532
        }
533
534
        return ['authenticationFailed' => true, 'cancelForm' => $cancelForm->createView()];
535
    }
536
537
    public function cancelU2fAuthenticationAction()
538
    {
539
        return $this->forward('SurfnetStepupGatewayGatewayBundle:Gateway:sendAuthenticationCancelledByUser');
540
    }
541
542
    /**
543
     * @return \Surfnet\StepupGateway\GatewayBundle\Service\StepupAuthenticationService
544
     */
545
    private function getStepupService()
546
    {
547
        return $this->get('gateway.service.stepup_authentication');
548
    }
549
550
    /**
551
     * @return ResponseContext
552
     */
553
    private function getResponseContext()
554
    {
555
        return $this->get($this->get('gateway.proxy.state_handler')->getResponseContextServiceId());
556
    }
557
558
    /**
559
     * @return \Surfnet\StepupGateway\GatewayBundle\Monolog\Logger\AuthenticationLogger
560
     */
561
    private function getAuthenticationLogger()
562
    {
563
        return $this->get('gateway.authentication_logger');
564
    }
565
566
    /**
567
     * @param ResponseContext $context
568
     * @param LoggerInterface $logger
569
     * @return string
570
     */
571
    private function getSelectedSecondFactor(ResponseContext $context, LoggerInterface $logger)
572
    {
573
        $selectedSecondFactor = $context->getSelectedSecondFactor();
574
575
        if (!$selectedSecondFactor) {
576
            $logger->error('Cannot verify possession of an unknown second factor');
577
578
            throw new BadRequestHttpException('Cannot verify possession of an unknown second factor.');
579
        }
580
581
        return $selectedSecondFactor;
582
    }
583
584
    private function selectAndRedirectTo(SecondFactor $secondFactor, ResponseContext $context)
585
    {
586
        $context->saveSelectedSecondFactor($secondFactor);
587
588
        $this->getStepupService()->clearSmsVerificationState();
589
590
        $route = 'gateway_verify_second_factor_';
591
        if ($secondFactor->isGssf()) {
592
            $route .= 'gssf';
593
        } else {
594
            $route .= strtolower($secondFactor->secondFactorType);
595
        }
596
597
        return $this->redirect($this->generateUrl($route));
598
    }
599
}
600