From a740f35d99d8f4fbd7c3efe7e6f975df2f25e627 Mon Sep 17 00:00:00 2001 From: Johan Kromhout Date: Wed, 29 Jan 2025 16:18:25 +0100 Subject: [PATCH] Feature: Make trusted device cookie toggleable Prior to this change, it was not possible to do a 'warm' rollout of the notification fatique attack prevention feature. This change makes this possible by disabling the enforcement of the check. Fixes https://github.com/OpenConext/Stepup-tiqr/issues/343 --- config/openconext/parameters.yaml.dist | 5 + config/services.yaml | 1 + .../AuthenticationNotificationController.php | 55 ++++++----- src/Service/TrustedDeviceHelper.php | 6 ++ ...thenticationNotificationControllerTest.php | 99 +++++++++++++++++++ 5 files changed, 144 insertions(+), 22 deletions(-) create mode 100644 tests/Unit/Controller/AuthenticationNotificationControllerTest.php diff --git a/config/openconext/parameters.yaml.dist b/config/openconext/parameters.yaml.dist index 850dc62b..33f6f7d2 100644 --- a/config/openconext/parameters.yaml.dist +++ b/config/openconext/parameters.yaml.dist @@ -55,6 +55,11 @@ parameters: # Should be one of the strings in: \Surfnet\Tiqr\Service\TrustedDevice\Http\CookieSameSite: 'none', 'lax', 'strict' trusted_device_cookie_same_site: 'none' + # Enable/Disable enforcement of MFA fatigue prevention + # The trusted device cookie ensures no push notifications are sent when logging in on devices + # To prevent everyone having to scan qr codes all of a sudden, enforcement can be temporarily disabled so trusted devices can build up + trusted_device_enforcement_enabled: true + # The secret key is used for the authenticated encryption (AE) of the trusted-device cookies, it is stored in the # parameters.yaml of Stepup-Tiqr. This secret may only be used for the AE of the trusted device cookies. # The secret key size is fixed, it must be 256 bits (32 bytes (64 hex digits)) diff --git a/config/services.yaml b/config/services.yaml index cbd06994..4d814b49 100644 --- a/config/services.yaml +++ b/config/services.yaml @@ -21,6 +21,7 @@ services: $appSecret: '%app_secret%' $sessionOptions: '%session.storage.options%' $correlationIdSalt: '%correlation_id_salt%' + $trustedDeviceCookieEnforcementEnabled: '%trusted_device_enforcement_enabled%' # makes classes in src/ available to be used as services # this creates a service per class whose id is the fully-qualified class name diff --git a/src/Controller/AuthenticationNotificationController.php b/src/Controller/AuthenticationNotificationController.php index 237be4e3..91fbd77a 100644 --- a/src/Controller/AuthenticationNotificationController.php +++ b/src/Controller/AuthenticationNotificationController.php @@ -27,6 +27,7 @@ use Surfnet\GsspBundle\Service\StateHandlerInterface; use Surfnet\Tiqr\Attribute\RequiresActiveSession; use Surfnet\Tiqr\Service\TrustedDevice\TrustedDeviceService; +use Surfnet\Tiqr\Service\TrustedDeviceHelper; use Surfnet\Tiqr\Tiqr\Exception\UserNotExistsException; use Surfnet\Tiqr\Tiqr\TiqrServiceInterface; use Surfnet\Tiqr\Tiqr\TiqrUserRepositoryInterface; @@ -49,6 +50,7 @@ public function __construct( private readonly TiqrUserRepositoryInterface $userRepository, private readonly LoggerInterface $logger, private readonly TrustedDeviceService $trustedDeviceService, + private readonly TrustedDeviceHelper $trustedDeviceHelper, ) { } @@ -96,31 +98,11 @@ public function __invoke(Request $request): Response return $this->generateNotificationResponse('no-device'); } - $cookie = $this->trustedDeviceService->read($request); - if ($cookie === null) { - $this->logger->notice( - sprintf( - 'No trusted device cookie stored for notification address "%s" and user "%s". No notification was sent', - $notificationAddress, - $nameId - ) - ); + if ($this->trustedDeviceHelper->trustedDeviceCookieEnforcementEnabled() + && !$this->isTrustedDevice($request, $notificationAddress, $nameId)) { return $this->generateNotificationResponse('no-trusted-device'); } - if ($this->trustedDeviceService->isTrustedDevice($cookie, $notificationAddress) === false) { - $this->logger->notice( - sprintf( - 'A trusted device cookie is found for notification address "%s" and user "%s", but has signature mismatch', - $notificationAddress, - $nameId - ) - ); - - return $this->generateNotificationResponse('no-trusted-device'); - } - - $this->logger->notice(sprintf( 'Sending push notification for user "%s" with type "%s" and (untranslated) address "%s"', $nameId, @@ -182,4 +164,33 @@ private function generateNotificationResponse(string $status): JsonResponse { return new JsonResponse($status); } + + private function isTrustedDevice(Request $request, string $notificationAddress, string $nameId): bool + { + $cookie = $this->trustedDeviceService->read($request); + if ($cookie === null) { + $this->logger->notice( + sprintf( + 'No trusted device cookie stored for notification address "%s" and user "%s". No notification was sent', + $notificationAddress, + $nameId + ) + ); + return false; + } + + if ($this->trustedDeviceService->isTrustedDevice($cookie, $notificationAddress) === false) { + $this->logger->notice( + sprintf( + 'A trusted device cookie is found for notification address "%s" and user "%s", but has signature mismatch', + $notificationAddress, + $nameId + ) + ); + + return false; + } + + return true; + } } diff --git a/src/Service/TrustedDeviceHelper.php b/src/Service/TrustedDeviceHelper.php index cad58933..9e5937be 100644 --- a/src/Service/TrustedDeviceHelper.php +++ b/src/Service/TrustedDeviceHelper.php @@ -31,6 +31,7 @@ public function __construct( private TrustedDeviceService $cookieService, private LoggerInterface $logger, + private bool $trustedDeviceCookieEnforcementEnabled, ) { } @@ -51,4 +52,9 @@ public function handleRegisterTrustedDevice( $this->logger->warning('Could not register trusted device on registration', ['exception' => $e]); } } + + public function trustedDeviceCookieEnforcementEnabled(): bool + { + return $this->trustedDeviceCookieEnforcementEnabled === true; + } } diff --git a/tests/Unit/Controller/AuthenticationNotificationControllerTest.php b/tests/Unit/Controller/AuthenticationNotificationControllerTest.php new file mode 100644 index 00000000..90d52605 --- /dev/null +++ b/tests/Unit/Controller/AuthenticationNotificationControllerTest.php @@ -0,0 +1,99 @@ +authService = $this->createMock(AuthenticationService::class); + $this->stateHandler = $this->createMock(StateHandlerInterface::class); + $this->tiqrService = $this->createMock(TiqrServiceInterface::class); + $this->userRepository = $this->createMock(TiqrUserRepositoryInterface::class); + $this->trustedDeviceService = $this->createMock(TrustedDeviceService::class); + + parent::__construct($name, $data, $dataName); + } + + public function provideTrustedDeviceCookieEnforcementEnabledScenarios(): array + { + return [ + [false, '"success"'], + [true, '"no-trusted-device"'], + ]; + } + + /** + * @dataProvider provideTrustedDeviceCookieEnforcementEnabledScenarios + */ + public function testTrustedDeviceCookieEnforcement(bool $trustedDeviceCookieEnforcementEnabled, string $expectedResponse): void + { + $controller = $this->makeController($trustedDeviceCookieEnforcementEnabled); + $this->authService->method('authenticationRequired')->willReturn(true); + + $user = $this->mockUser('ACN', '01011001'); + + $this->userRepository->method('getUser')->willReturn($user); + $this->trustedDeviceService->method('read')->willReturn(null); + + $request = $this->createMock(Request::class); + + $response = $controller->__invoke($request); + + $this->assertSame($expectedResponse, $response->getContent()); + } + + private function makeController(bool $trustedDeviceCookieEnforcementEnabled): AuthenticationNotificationController + { + return new AuthenticationNotificationController( + $this->authService, + $this->stateHandler, + $this->tiqrService, + $this->userRepository, + new NullLogger(), + $this->trustedDeviceService, + new TrustedDeviceHelper($this->trustedDeviceService, new NullLogger(), $trustedDeviceCookieEnforcementEnabled), + ); + } + + private function mockUser(string $notificationType, string $notificationAddress): TiqrUser + { + $user = $this->createMock(TiqrUser::class); + $user->expects($this->once())->method('getNotificationType')->willReturn($notificationType); + $user->method('getNotificationAddress')->willReturn($notificationAddress); + return $user; + } +} \ No newline at end of file