From 476d74bf97bef58e032a6ce6c1027d92ccba6d1b Mon Sep 17 00:00:00 2001 From: Joris Steyn Date: Mon, 13 Nov 2017 11:58:07 +0100 Subject: [PATCH 01/36] Show expiration error early in the vetting process Instead of showing the expiration error when the vetting process is almost completed, show it right after looking up the registration code. This saves time of the user and the RA. See: https://www.pivotaltracker.com/story/show/133928873 --- .../RaBundle/Controller/VettingController.php | 22 +++++ .../RaBundle/Service/VettingService.php | 17 ++++ .../Tests/Service/VettingServiceTest.php | 81 +++++++++++++++++++ 3 files changed, 120 insertions(+) create mode 100644 src/Surfnet/StepupRa/RaBundle/Tests/Service/VettingServiceTest.php diff --git a/src/Surfnet/StepupRa/RaBundle/Controller/VettingController.php b/src/Surfnet/StepupRa/RaBundle/Controller/VettingController.php index 6efea3f9..ef2b4f1d 100644 --- a/src/Surfnet/StepupRa/RaBundle/Controller/VettingController.php +++ b/src/Surfnet/StepupRa/RaBundle/Controller/VettingController.php @@ -37,6 +37,7 @@ /** * @SuppressWarnings(PHPMD.CouplingBetweenObjects) + * @SuppressWarnings(PHPMD.ExcessiveMethodLength) */ class VettingController extends Controller { @@ -108,6 +109,27 @@ public function startProcedureAction(Request $request) $command->authorityLoa = $token->getLoa(); $command->secondFactor = $secondFactor; + if ($this->getVettingService()->isExpiredRegistrationCode($command)) { + $form->addError( + new FormError( + $this->getTranslator() + ->trans( + 'ra.verify_identity.registration_code_expired', + [ + '%self_service_url%' => $this->getParameter('surfnet_stepup_ra.self_service_url'), + ] + ) + ) + ); + + $logger->notice( + 'Second factor registration code is expired', + ['registration_requested_at' => $secondFactor->registrationRequestedAt->format('Y-m-d')] + ); + + return ['form' => $form->createView()]; + } + if (!$this->getVettingService()->isLoaSufficientToStartProcedure($command)) { $form->addError(new FormError('ra.form.start_vetting_procedure.loa_insufficient')); diff --git a/src/Surfnet/StepupRa/RaBundle/Service/VettingService.php b/src/Surfnet/StepupRa/RaBundle/Service/VettingService.php index 6c57f203..b6af4c34 100644 --- a/src/Surfnet/StepupRa/RaBundle/Service/VettingService.php +++ b/src/Surfnet/StepupRa/RaBundle/Service/VettingService.php @@ -35,11 +35,13 @@ use Surfnet\StepupRa\RaBundle\Exception\DomainException; use Surfnet\StepupRa\RaBundle\Exception\InvalidArgumentException; use Surfnet\StepupRa\RaBundle\Exception\LoaTooLowException; +use Surfnet\StepupRa\RaBundle\Exception\RegistrationCodeExpiredException; use Surfnet\StepupRa\RaBundle\Exception\UnknownVettingProcedureException; use Surfnet\StepupRa\RaBundle\Repository\VettingProcedureRepository; use Surfnet\StepupRa\RaBundle\Service\Gssf\VerificationResult as GssfVerificationResult; use Surfnet\StepupRa\RaBundle\Service\U2f\AuthenticationVerificationResult; use Surfnet\StepupRa\RaBundle\Service\U2f\SignRequestCreationResult; +use Surfnet\StepupRa\RaBundle\Value\DateTime; use Surfnet\StepupRa\RaBundle\VettingProcedure; use Surfnet\StepupU2fBundle\Dto\SignRequest; use Surfnet\StepupU2fBundle\Dto\SignResponse; @@ -132,6 +134,21 @@ public function isLoaSufficientToStartProcedure(StartVettingProcedureCommand $co return $this->secondFactorTypeService->isSatisfiedBy($secondFactorType, $command->authorityLoa); } + /** + * @param StartVettingProcedureCommand $command + * @return bool + */ + public function isExpiredRegistrationCode(StartVettingProcedureCommand $command) + { + return DateTime::now()->comesAfter( + new DateTime( + $command->secondFactor->registrationRequestedAt + ->add(new \DateInterval('P14D')) + ->setTime(23, 59, 59) + ) + ); + } + /** * @param StartVettingProcedureCommand $command * @return string The procedure ID. diff --git a/src/Surfnet/StepupRa/RaBundle/Tests/Service/VettingServiceTest.php b/src/Surfnet/StepupRa/RaBundle/Tests/Service/VettingServiceTest.php new file mode 100644 index 00000000..9a2454b6 --- /dev/null +++ b/src/Surfnet/StepupRa/RaBundle/Tests/Service/VettingServiceTest.php @@ -0,0 +1,81 @@ +secondFactor = new VerifiedSecondFactor(); + $command->secondFactor->registrationRequestedAt = $registrationRequestedAt; + + $service = Mockery::mock(VettingService::class)->makePartial(); + + $this->assertFalse( + $service->isExpiredRegistrationCode($command) + ); + } + + public function validRegistrationDatesProvider() { + return [ + [date_create('- 1 week')], + [date_create('- 2 weeks')], + [date_create('- 2 weeks')->setTime(0, 0, 0)], + [date_create('- 2 weeks')->setTime(23, 59, 59)], + ]; + } + + /** + * @test + * @group vetting + * @dataProvider expiredRegistrationDatesProvider + */ + public function registration_code_is_invalid_two_weeks_after_verification($registrationRequestedAt) + { + $command = new StartVettingProcedureCommand(); + $command->secondFactor = new VerifiedSecondFactor(); + $command->secondFactor->registrationRequestedAt = $registrationRequestedAt; + + $service = Mockery::mock(VettingService::class)->makePartial(); + + $this->assertTrue( + $service->isExpiredRegistrationCode($command) + ); + } + + public function expiredRegistrationDatesProvider() { + return [ + [date_create('- 3 weeks')], + [date_create('- 15 days')->setTime(0, 0, 0)], + [date_create('- 15 days')->setTime(23, 59, 59)], + ]; + } +} From 7513d45a923218f782fbd42a289ec91b3220fab0 Mon Sep 17 00:00:00 2001 From: Joris Steyn Date: Thu, 16 Nov 2017 09:30:59 +0100 Subject: [PATCH 02/36] Update middleware client bundle to version 2.1 --- composer.lock | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/composer.lock b/composer.lock index 963c5264..2eac90cc 100644 --- a/composer.lock +++ b/composer.lock @@ -2149,16 +2149,16 @@ }, { "name": "surfnet/stepup-middleware-client-bundle", - "version": "2.0.0", + "version": "2.1.0", "source": { "type": "git", "url": "https://github.com/OpenConext/Stepup-Middleware-clientbundle.git", - "reference": "2cd5c2532cb99c370cfc7aea992450a83d14a9c9" + "reference": "c0d6721efa82ad9b52235c5a2b8e7947e56e3fe2" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/OpenConext/Stepup-Middleware-clientbundle/zipball/2cd5c2532cb99c370cfc7aea992450a83d14a9c9", - "reference": "2cd5c2532cb99c370cfc7aea992450a83d14a9c9", + "url": "https://api.github.com/repos/OpenConext/Stepup-Middleware-clientbundle/zipball/c0d6721efa82ad9b52235c5a2b8e7947e56e3fe2", + "reference": "c0d6721efa82ad9b52235c5a2b8e7947e56e3fe2", "shasum": "" }, "require": { @@ -2198,7 +2198,7 @@ "Apache-2.0" ], "description": "Symfony2 bundle for consuming the Step-up Middleware API.", - "time": "2017-03-07T14:10:57+00:00" + "time": "2017-11-16T08:28:13+00:00" }, { "name": "surfnet/stepup-saml-bundle", @@ -2288,7 +2288,7 @@ "Apache-2.0" ], "description": "The SURFnet Step-up U2F bundle contains server-side device verification, and the necessary forms and resources to enable client-side U2F interaction with Step-up Identities", - "time": "2015-09-17 15:02:04" + "time": "2015-09-17T15:02:04+00:00" }, { "name": "symfony/assetic-bundle", From fc337b18cca174968384fc2082732aff1abd74e4 Mon Sep 17 00:00:00 2001 From: Joris Steyn Date: Thu, 16 Nov 2017 12:21:18 +0100 Subject: [PATCH 03/36] Fix form buttons inaccessible on mobile device --- .../Resources/views/Vetting/verifyIdentity.html.twig | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Surfnet/StepupRa/RaBundle/Resources/views/Vetting/verifyIdentity.html.twig b/src/Surfnet/StepupRa/RaBundle/Resources/views/Vetting/verifyIdentity.html.twig index 4a5f811d..e23d25c9 100644 --- a/src/Surfnet/StepupRa/RaBundle/Resources/views/Vetting/verifyIdentity.html.twig +++ b/src/Surfnet/StepupRa/RaBundle/Resources/views/Vetting/verifyIdentity.html.twig @@ -46,11 +46,11 @@
-
-
+ +
{{ form_row(form.verifyIdentity) }}
-
+