Complex classes like SecondFactorController often do a lot of different things. To break such a class down, we need to identify a cohesive component within that class. A common approach to find such a component is to look for fields/methods that share the same prefixes, or suffixes. You can also have a look at the cohesion graph to spot any un-connected, or weakly-connected components.
Once you have determined the fields that belong together, you can apply the Extract Class refactoring. If the component makes sense as a sub-class, Extract Subclass is also a candidate, and is often faster.
While breaking up the class, it is a good idea to analyze how other classes use SecondFactorController, and based on these observations, apply Extract Interface, too.
| 1 | <?php |
||
| 44 | class SecondFactorController extends Controller |
||
| 45 | { |
||
| 46 | public function selectSecondFactorForVerificationAction() |
||
| 112 | |||
| 113 | /** |
||
| 114 | * @Template |
||
| 115 | * @param Request $request |
||
| 116 | * @return array|RedirectResponse|Response |
||
| 117 | */ |
||
| 118 | public function chooseSecondFactorAction(Request $request) |
||
| 119 | { |
||
| 120 | $context = $this->getResponseContext(); |
||
| 121 | $originalRequestId = $context->getInResponseTo(); |
||
| 122 | |||
| 123 | $requiredLoa = $this |
||
| 124 | ->getStepupService() |
||
| 125 | ->resolveHighestRequiredLoa( |
||
| 126 | $context->getRequiredLoa(), |
||
| 127 | $context->getIdentityNameId(), |
||
| 128 | $context->getServiceProvider() |
||
| 129 | ); |
||
| 130 | |||
| 131 | $secondFactors = $this |
||
| 132 | ->getStepupService() |
||
| 133 | ->determineViableSecondFactors( |
||
| 134 | $context->getIdentityNameId(), |
||
| 135 | $requiredLoa, |
||
| 136 | $this->get('gateway.service.whitelist') |
||
| 137 | ); |
||
| 138 | |||
| 139 | $command = new ChooseSecondFactorCommand(); |
||
| 140 | $command->secondFactors = $secondFactors; |
||
| 141 | |||
| 142 | $logger = $this->get('surfnet_saml.logger')->forAuthentication($originalRequestId); |
||
| 143 | |||
| 144 | $form = $this |
||
| 145 | ->createForm( |
||
| 146 | 'gateway_choose_second_factor', |
||
| 147 | $command, |
||
| 148 | ['action' => $this->generateUrl('gateway_verify_second_factor_choose_second_factor')] |
||
| 149 | ) |
||
| 150 | ->handleRequest($request); |
||
| 151 | |||
| 152 | if ($form->get('cancel')->isClicked()) { |
||
| 153 | return $this->forward('SurfnetStepupGatewayGatewayBundle:Gateway:sendAuthenticationCancelledByUser'); |
||
| 154 | } |
||
| 155 | |||
| 156 | if ($form->isSubmitted() && $form->isValid()) { |
||
| 157 | $secondFactorIndex = $command->selectedSecondFactor; |
||
| 158 | $secondFactor = $secondFactors->get($secondFactorIndex); |
||
| 159 | $logger->notice(sprintf( |
||
| 160 | 'User chose "%s" to use as second factor', |
||
| 161 | $secondFactor->secondFactorType |
||
| 162 | )); |
||
| 163 | |||
| 164 | // Forward to action to verify possession of second factor |
||
| 165 | return $this->selectAndRedirectTo($secondFactor, $context); |
||
| 166 | } |
||
| 167 | |||
| 168 | return ['form' => $form->createView()]; |
||
| 169 | } |
||
| 170 | |||
| 171 | public function verifyGssfAction() |
||
| 208 | |||
| 209 | public function gssfVerifiedAction() |
||
| 241 | |||
| 242 | /** |
||
| 243 | * @Template |
||
| 244 | * @param Request $request |
||
| 245 | * @return array|Response |
||
| 246 | */ |
||
| 247 | public function verifyYubiKeySecondFactorAction(Request $request) |
||
| 248 | { |
||
| 249 | $context = $this->getResponseContext(); |
||
| 250 | $originalRequestId = $context->getInResponseTo(); |
||
| 251 | |||
| 252 | /** @var \Surfnet\SamlBundle\Monolog\SamlAuthenticationLogger $logger */ |
||
| 253 | $logger = $this->get('surfnet_saml.logger')->forAuthentication($originalRequestId); |
||
| 254 | |||
| 255 | $selectedSecondFactor = $this->getSelectedSecondFactor($context, $logger); |
||
| 256 | |||
| 257 | $logger->notice('Verifying possession of Yubikey second factor'); |
||
| 258 | |||
| 259 | $command = new VerifyYubikeyOtpCommand(); |
||
| 260 | $command->secondFactorId = $selectedSecondFactor; |
||
| 261 | |||
| 262 | $form = $this->createForm('gateway_verify_yubikey_otp', $command)->handleRequest($request); |
||
| 263 | |||
| 264 | if ($form->get('cancel')->isClicked()) { |
||
| 265 | return $this->forward('SurfnetStepupGatewayGatewayBundle:Gateway:sendAuthenticationCancelledByUser'); |
||
| 266 | } |
||
| 267 | |||
| 268 | if (!$form->isValid()) { |
||
| 269 | // OTP field is rendered empty in the template. |
||
| 270 | return ['form' => $form->createView()]; |
||
| 271 | } |
||
| 272 | |||
| 273 | $result = $this->getStepupService()->verifyYubikeyOtp($command); |
||
| 274 | |||
| 275 | if ($result->didOtpVerificationFail()) { |
||
| 276 | $form->addError(new FormError('gateway.form.verify_yubikey.otp_verification_failed')); |
||
| 277 | |||
| 278 | // OTP field is rendered empty in the template. |
||
| 279 | return ['form' => $form->createView()]; |
||
| 280 | } elseif (!$result->didPublicIdMatch()) { |
||
| 281 | $form->addError(new FormError('gateway.form.verify_yubikey.public_id_mismatch')); |
||
| 282 | |||
| 283 | // OTP field is rendered empty in the template. |
||
| 284 | return ['form' => $form->createView()]; |
||
| 285 | } |
||
| 286 | |||
| 287 | $this->getResponseContext()->markSecondFactorVerified(); |
||
| 288 | $this->getAuthenticationLogger()->logSecondFactorAuthentication($originalRequestId); |
||
| 289 | |||
| 290 | $logger->info( |
||
| 291 | sprintf( |
||
| 292 | 'Marked Yubikey Second Factor "%s" as verified, forwarding to Saml Proxy to respond', |
||
| 293 | $selectedSecondFactor |
||
| 294 | ) |
||
| 295 | ); |
||
| 296 | |||
| 297 | return $this->forward($context->getResponseAction()); |
||
| 298 | } |
||
| 299 | |||
| 300 | /** |
||
| 301 | * @Template |
||
| 302 | * @param Request $request |
||
| 303 | * @return array|Response |
||
| 304 | */ |
||
| 305 | public function verifySmsSecondFactorAction(Request $request) |
||
| 306 | { |
||
| 307 | $context = $this->getResponseContext(); |
||
| 308 | $originalRequestId = $context->getInResponseTo(); |
||
| 309 | |||
| 310 | /** @var \Surfnet\SamlBundle\Monolog\SamlAuthenticationLogger $logger */ |
||
| 311 | $logger = $this->get('surfnet_saml.logger')->forAuthentication($originalRequestId); |
||
| 312 | |||
| 313 | $selectedSecondFactor = $this->getSelectedSecondFactor($context, $logger); |
||
| 314 | |||
| 315 | $logger->notice('Verifying possession of SMS second factor, preparing to send'); |
||
| 316 | |||
| 317 | $command = new SendSmsChallengeCommand(); |
||
| 318 | $command->secondFactorId = $selectedSecondFactor; |
||
| 319 | |||
| 320 | $form = $this->createForm('gateway_send_sms_challenge', $command)->handleRequest($request); |
||
| 321 | |||
| 322 | $stepupService = $this->getStepupService(); |
||
| 323 | $phoneNumber = InternationalPhoneNumber::fromStringFormat( |
||
| 324 | $stepupService->getSecondFactorIdentifier($selectedSecondFactor) |
||
| 325 | ); |
||
| 326 | |||
| 327 | $otpRequestsRemaining = $stepupService->getSmsOtpRequestsRemainingCount(); |
||
| 328 | $maximumOtpRequests = $stepupService->getSmsMaximumOtpRequestsCount(); |
||
| 329 | $viewVariables = ['otpRequestsRemaining' => $otpRequestsRemaining, 'maximumOtpRequests' => $maximumOtpRequests]; |
||
| 330 | |||
| 331 | if ($form->get('cancel')->isClicked()) { |
||
| 332 | return $this->forward('SurfnetStepupGatewayGatewayBundle:Gateway:sendAuthenticationCancelledByUser'); |
||
| 333 | } |
||
| 334 | |||
| 335 | if (!$form->isValid()) { |
||
| 336 | return array_merge($viewVariables, ['phoneNumber' => $phoneNumber, 'form' => $form->createView()]); |
||
| 337 | } |
||
| 338 | |||
| 339 | $logger->notice('Verifying possession of SMS second factor, sending challenge per SMS'); |
||
| 340 | |||
| 341 | if (!$stepupService->sendSmsChallenge($command)) { |
||
| 342 | $form->addError(new FormError('gateway.form.send_sms_challenge.sms_sending_failed')); |
||
| 343 | |||
| 344 | return array_merge($viewVariables, ['phoneNumber' => $phoneNumber, 'form' => $form->createView()]); |
||
| 345 | } |
||
| 346 | |||
| 347 | return $this->redirect($this->generateUrl('gateway_verify_second_factor_sms_verify_challenge')); |
||
| 348 | } |
||
| 349 | |||
| 350 | /** |
||
| 351 | * @Template |
||
| 352 | * @param Request $request |
||
| 353 | * @return array|Response |
||
| 354 | */ |
||
| 355 | public function verifySmsSecondFactorChallengeAction(Request $request) |
||
| 356 | { |
||
| 357 | $context = $this->getResponseContext(); |
||
| 358 | $originalRequestId = $context->getInResponseTo(); |
||
| 359 | |||
| 360 | /** @var \Surfnet\SamlBundle\Monolog\SamlAuthenticationLogger $logger */ |
||
| 361 | $logger = $this->get('surfnet_saml.logger')->forAuthentication($originalRequestId); |
||
| 362 | |||
| 363 | $selectedSecondFactor = $this->getSelectedSecondFactor($context, $logger); |
||
| 364 | |||
| 365 | $command = new VerifyPossessionOfPhoneCommand(); |
||
| 366 | $form = $this->createForm('gateway_verify_sms_challenge', $command)->handleRequest($request); |
||
| 367 | |||
| 368 | if ($form->get('cancel')->isClicked()) { |
||
| 369 | return $this->forward('SurfnetStepupGatewayGatewayBundle:Gateway:sendAuthenticationCancelledByUser'); |
||
| 370 | } |
||
| 371 | |||
| 372 | if (!$form->isValid()) { |
||
| 373 | return ['form' => $form->createView()]; |
||
| 374 | } |
||
| 375 | |||
| 376 | $logger->notice('Verifying input SMS challenge matches'); |
||
| 377 | |||
| 378 | $verification = $this->getStepupService()->verifySmsChallenge($command); |
||
| 379 | |||
| 380 | if ($verification->wasSuccessful()) { |
||
| 381 | $this->getStepupService()->clearSmsVerificationState(); |
||
| 382 | |||
| 383 | $this->getResponseContext()->markSecondFactorVerified(); |
||
| 384 | $this->getAuthenticationLogger()->logSecondFactorAuthentication($originalRequestId); |
||
| 385 | |||
| 386 | $logger->info( |
||
| 387 | sprintf( |
||
| 388 | 'Marked Sms Second Factor "%s" as verified, forwarding to Saml Proxy to respond', |
||
| 389 | $selectedSecondFactor |
||
| 390 | ) |
||
| 391 | ); |
||
| 392 | |||
| 393 | return $this->forward($context->getResponseAction()); |
||
| 394 | } elseif ($verification->didOtpExpire()) { |
||
| 395 | $logger->notice('SMS challenge expired'); |
||
| 396 | $form->addError(new FormError('gateway.form.send_sms_challenge.challenge_expired')); |
||
| 397 | } elseif ($verification->wasAttemptedTooManyTimes()) { |
||
| 398 | $logger->notice('SMS challenge verification was attempted too many times'); |
||
| 399 | $form->addError(new FormError('gateway.form.send_sms_challenge.too_many_attempts')); |
||
| 400 | } else { |
||
| 401 | $logger->notice('SMS challenge did not match'); |
||
| 402 | $form->addError(new FormError('gateway.form.send_sms_challenge.sms_challenge_incorrect')); |
||
| 403 | } |
||
| 404 | |||
| 405 | return ['form' => $form->createView()]; |
||
| 406 | } |
||
| 407 | |||
| 408 | /** |
||
| 409 | * @Template |
||
| 410 | */ |
||
| 411 | public function initiateU2fAuthenticationAction() |
||
| 459 | |||
| 460 | /** |
||
| 461 | * @Template("SurfnetStepupGatewayGatewayBundle:SecondFactor:initiateU2fAuthentication.html.twig") |
||
| 462 | * |
||
| 463 | * @param Request $request |
||
| 464 | * @return array|Response |
||
| 465 | */ |
||
| 466 | public function verifyU2fAuthenticationAction(Request $request) |
||
| 528 | |||
| 529 | public function cancelU2fAuthenticationAction() |
||
| 533 | |||
| 534 | /** |
||
| 535 | * @return \Surfnet\StepupGateway\GatewayBundle\Service\StepupAuthenticationService |
||
| 536 | */ |
||
| 537 | private function getStepupService() |
||
| 541 | |||
| 542 | /** |
||
| 543 | * @return ResponseContext |
||
| 544 | */ |
||
| 545 | private function getResponseContext() |
||
| 549 | |||
| 550 | /** |
||
| 551 | * @return \Surfnet\StepupGateway\GatewayBundle\Monolog\Logger\AuthenticationLogger |
||
| 552 | */ |
||
| 553 | private function getAuthenticationLogger() |
||
| 557 | |||
| 558 | /** |
||
| 559 | * @param ResponseContext $context |
||
| 560 | * @param LoggerInterface $logger |
||
| 561 | * @return string |
||
| 562 | */ |
||
| 563 | private function getSelectedSecondFactor(ResponseContext $context, LoggerInterface $logger) |
||
| 564 | { |
||
| 565 | $selectedSecondFactor = $context->getSelectedSecondFactor(); |
||
| 566 | |||
| 567 | if (!$selectedSecondFactor) { |
||
| 568 | $logger->error('Cannot verify possession of an unknown second factor'); |
||
| 569 | |||
| 570 | throw new BadRequestHttpException('Cannot verify possession of an unknown second factor.'); |
||
| 571 | } |
||
| 572 | |||
| 573 | return $selectedSecondFactor; |
||
| 574 | } |
||
| 575 | |||
| 576 | private function selectAndRedirectTo(SecondFactor $secondFactor, ResponseContext $context) |
||
| 591 | } |
||
| 592 |
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: