Completed
Pull Request — develop (#127)
by
unknown
28:06 queued 23:19
created

SecondFactorController::getAuthenticationLogger()   A

Complexity

Conditions 1
Paths 1

Size

Total Lines 4
Code Lines 2

Duplication

Lines 0
Ratio 0 %

Importance

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