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