Handle second factor test response #120

nicwortel · 2017-02-27T17:24:03Z

Part of https://www.pivotaltracker.com/story/show/79436876

…ions.sh

DRvanR

Before reviewing in depth these issues should be resolved.

DRvanR · 2017-02-28T07:45:46Z

src/Surfnet/StepupSelfService/SelfServiceBundle/Controller/SamlController.php

+            $loaResolutionService->getLoaByLevel($secondFactorType->getLevel())
+        );
+
+        $this->get('session')->set('second_factor_test_mode', true);


A bit more should be stored (and tracked) in the session; e.g. the request ID of the AuthnRequest.

DRvanR · 2017-02-28T07:48:25Z

src/Surfnet/StepupSelfService/SelfServiceBundle/Controller/SamlController.php

+        $translator = $this->get('translator');
+
+        try {
+            $postBinding->processResponse(


it must be verified that the received response is in response to the sent request; for reference see here

Should the second factor test flow also use an implementation of the SamlAuthenticationStateHandler? Or is it more appropriate to use the session directly?

Ideally we would want to keep the test implementation as close to the actual implementation. However, the intent and flow is different. Therefore, one would ideally create a wrapper for this specific case. For now, I'd use the session and register the improvement as technical debt.

Tech debt ticket created: https://www.pivotaltracker.com/story/show/141126081

DRvanR · 2017-02-28T07:54:52Z

src/Surfnet/StepupSelfService/SelfServiceBundle/Controller/SamlController.php

+
+        $this->get('session')->set('second_factor_test_mode', true);
+
+        $this->get('logger')->notice(


The moment we have an AuthnRequest and thus a RequestID, we should use the SamlAuthenticationLogger (same when processing the response). For an example as how to use that, see here

Is it correct that this logger can only be used when the request ID is set? In other words, the SamlAuthenticationLogger is invalid without a request ID and the default logger service should be used before we know the request ID?

Also, do you prefer to keep the request ID in the log message (as an argument for the sprintf call), or is it enough to have it to the logging context?

The SamlAuthenticationLogger can only be used when the requestId is set.
It is enough to have it in the logging context.

…request instance

nicwortel · 2017-03-03T11:51:01Z

src/Surfnet/StepupSelfService/SelfServiceBundle/Controller/SamlController.php

+
+            $session->getFlashBag()->add(
+                'success',
+                $translator->trans('ss.test_second_factor.verification_successful')


I noticed from the logs that this string is being translated twice: once here in the controller, and once more in the session_flash macro that is provided by the MopaBootstrapBundle. This triggers a warning, because the already translated string cannot be translated again.

However, when I remove the translation here, the string is no longer recognized as a translation key by the JMSTranslationBundle, so it will be removed from the result of the extract-translations script.

I have found a translations.twig file in the SelfServiceBundle which (I presume) exists to work around this problem. Is that correct? So then I could remove the translator here and add the translation keys to that file?

Nice find. I think that should work. It is consistent with how other flash messages are done, see SmsController.php (82) and translations.twig (31)

The flash messages are being translated again in the session_flash macro provided by the MopaBootstrapBundle. Trying to translate an already translated string results in a warning from the translation component.

The currently installed version of JMSTranslationBundle requires an old version of the nikic/php-parser package, which doesn't support the ::class keyword introduced in PHP 5.5. This should be fixed by upgrading JMSTranslationBundle to a more recent version. See nikic/PHP-Parser#200 for more information.

The SamlAuthenticationLogger already adds the ID to the context.

nicwortel · 2017-03-06T15:44:30Z

Note that I could not reproduce a situation where testing the second factor fails because the request ID differs from the one that is stored in the session (for instance by firing two requests in parallel), because the gateway also fails in such a scenario.

arothuis

All in all, this looks good to me -- bar a few nits.

arothuis · 2017-03-06T15:35:28Z

src/Surfnet/StepupSelfService/SelfServiceBundle/Controller/SamlController.php

     */
+    public function testSecondFactorAction($secondFactorId)
+    {
+        $this->get('logger')->notice(


You could extract $this->get('logger') to a variable as it is reused later on.
Also, this could fit on a single line.

The latter $logger could be renamed to $samlLogger or something else.

arothuis · 2017-03-06T15:39:32Z

src/Surfnet/StepupSelfService/SelfServiceBundle/Controller/SamlController.php

+                )
+            );
+
+            throw $this->createNotFoundException();


Why not throw the exception itself?

This is just a Symfony convenience method that creates a NotFoundHttpException with a default message, I guess it is a matter of personal preference.

arothuis · 2017-03-06T15:40:14Z

src/Surfnet/StepupSelfService/SelfServiceBundle/Controller/SamlController.php

+                'Received an authentication response for testing a second factor, but no second factor test response was expected'
+            );
+
+            throw $this->createAccessDeniedException('Did not expect an authentication response');


Same here; why not throw the exception itself?

arothuis · 2017-03-06T15:40:52Z

src/Surfnet/StepupSelfService/SelfServiceBundle/Controller/SamlController.php

+
+        $initiatedRequestId = $session->get('second_factor_test_request_id');
+
+        $logger = $this->get('surfnet_saml.logger')->forAuthentication($initiatedRequestId);


You are reassigning the $logger here, you would probably want to rename this to prevent confusion.

arothuis · 2017-03-06T15:50:40Z

src/Surfnet/StepupSelfService/SelfServiceBundle/Controller/SamlController.php

+            }
+
+            $session->getFlashBag()->add('success', 'ss.test_second_factor.verification_successful');
+        } catch (Exception $exception) {


With regards to your comment above; this would also catch an Exception if anything goes wrong in Gateway or client bundles (which is possible when sessions are somehow tainted during testing), which should also result in an error message. I think this suffices for now.

…tions

Changes have been implemented

nicwortel · 2017-03-07T13:37:31Z

Includes changes from #118 and #119.

nicwortel added 9 commits February 23, 2017 22:08

Add a test button to the second factor overview

61cab63

Document the process of extracting and translating translation keys

0ff931a

Replace translation:extract console command with bin/extract-translat…

afd9568

…ions.sh

Only show the test button for vetted second factors

207f82f

Create a controller action that sends an authentication request

e79a58b

Make the IDP to test second factors with configurable

3dbd5cf

Break long line

921d0b8

Add logging

ca788f5

Handle the authentication response

76baa54

nicwortel requested a review from DRvanR February 27, 2017 17:24

DRvanR previously requested changes Feb 28, 2017

View reviewed changes

nicwortel added 2 commits March 3, 2017 11:36

Store the request ID of the authentication request in the session

595a2ca

Use the SamlAuthenticationLogger once when we have an authentication …

cbbf842

…request instance

nicwortel commented Mar 3, 2017

View reviewed changes

nicwortel added 2 commits March 3, 2017 14:47

Remove flash message translation from SamlController

db20bc4

The flash messages are being translated again in the session_flash macro provided by the MopaBootstrapBundle. Trying to translate an already translated string results in a warning from the translation component.

nicwortel force-pushed the handle-second-factor-test-response branch from cd16c94 to eb56708 Compare March 3, 2017 14:00

nicwortel added 2 commits March 6, 2017 12:17

Remove request ID from the log message

3d75be9

The SamlAuthenticationLogger already adds the ID to the context.

Validate that the request ID matches the one stored in the session

eeb49cc

arothuis approved these changes Mar 6, 2017

View reviewed changes

nicwortel added 2 commits March 6, 2017 16:56

Extract $logger variable

a0fa39f

Rename references to the SamlAuthenticationLogger to $samlLogger

56f90c1

nicwortel force-pushed the handle-second-factor-test-response branch from 062cb4a to 56f90c1 Compare March 6, 2017 16:08

Replace Symfony convenience methods with self-instantiated HTTP excep…

87418d5

…tions

nicwortel merged commit 7de4511 into develop Mar 7, 2017

nicwortel deleted the handle-second-factor-test-response branch March 7, 2017 13:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle second factor test response #120

Handle second factor test response #120

nicwortel commented Feb 27, 2017

DRvanR left a comment

DRvanR Feb 28, 2017

DRvanR Feb 28, 2017

nicwortel Mar 3, 2017

arothuis Mar 6, 2017 •

edited

Loading

nicwortel Mar 6, 2017

DRvanR Feb 28, 2017

nicwortel Mar 3, 2017

nicwortel Mar 3, 2017 •

edited

Loading

arothuis Mar 6, 2017

nicwortel Mar 3, 2017

arothuis Mar 6, 2017

nicwortel commented Mar 6, 2017

arothuis left a comment •

edited

Loading

arothuis Mar 6, 2017 •

edited

Loading

arothuis Mar 6, 2017

nicwortel Mar 6, 2017

arothuis Mar 6, 2017

arothuis Mar 6, 2017

arothuis Mar 6, 2017 •

edited

Loading

nicwortel commented Mar 7, 2017


		$this->get('session')->set('second_factor_test_mode', true);

		$this->get('logger')->notice(


		$initiatedRequestId = $session->get('second_factor_test_request_id');

		$logger = $this->get('surfnet_saml.logger')->forAuthentication($initiatedRequestId);

Handle second factor test response #120

Handle second factor test response #120

Conversation

nicwortel commented Feb 27, 2017

DRvanR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arothuis Mar 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicwortel Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicwortel commented Mar 6, 2017

arothuis left a comment • edited Loading

Choose a reason for hiding this comment

arothuis Mar 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arothuis Mar 6, 2017 • edited Loading

Choose a reason for hiding this comment

nicwortel commented Mar 7, 2017

arothuis Mar 6, 2017 •

edited

Loading

nicwortel Mar 3, 2017 •

edited

Loading

arothuis left a comment •

edited

Loading

arothuis Mar 6, 2017 •

edited

Loading

arothuis Mar 6, 2017 •

edited

Loading