From 7112e932a281250cf3d27311c470a32bd516b98a Mon Sep 17 00:00:00 2001 From: Kevin Papst Date: Mon, 1 May 2023 08:28:35 +0200 Subject: [PATCH] Allow 2FA for SAML and LDAP users (#4000) * inline totp image as data uri to prevent caching issues * allow 2fa for ldap and saml users * allow 2FA access for super admin to all profiles --- config/packages/scheb_2fa.yaml | 8 +-- src/Controller/ProfileController.php | 49 +++++++----------- src/Security/TwoFactorCondition.php | 5 -- src/Voter/UserVoter.php | 5 +- templates/user/2fa.html.twig | 2 +- tests/Controller/ProfileControllerTest.php | 59 +++++++++------------- 6 files changed, 51 insertions(+), 77 deletions(-) diff --git a/config/packages/scheb_2fa.yaml b/config/packages/scheb_2fa.yaml index 355dd78e..d1de5164 100644 --- a/config/packages/scheb_2fa.yaml +++ b/config/packages/scheb_2fa.yaml @@ -3,12 +3,14 @@ scheb_two_factor: security_tokens: - Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken - Symfony\Component\Security\Http\Authenticator\Token\PostAuthenticationToken + - App\Saml\SamlToken + totp: enabled: true template: security/2fa.html.twig # Overwritten template window: 1 # How many codes before/after the current one would be accepted as valid -# server_name: Server Name # Server name used in QR code issuer: Kimai # Issuer name used in QR code -# parameters: # Additional parameters added in the QR code -# image: 'https://my-service/img/logo.png' + two_factor_condition: App\Security\TwoFactorCondition + + # FIXME add backup codes - https://symfony.com/bundles/SchebTwoFactorBundle/current/backup_codes.html diff --git a/src/Controller/ProfileController.php b/src/Controller/ProfileController.php index f970d66d..a72d061c 100644 --- a/src/Controller/ProfileController.php +++ b/src/Controller/ProfileController.php @@ -367,23 +367,34 @@ final class ProfileController extends AbstractController return $this->redirectToRoute('user_profile_2fa', ['username' => $profile->getUserIdentifier()]); } + $qrCodeContent = $totpAuthenticator->getQRContent($profile); + + $result = Builder::create() + ->writer(new PngWriter()) + ->writerOptions([]) + ->data($qrCodeContent) + ->encoding(new Encoding('UTF-8')) + ->errorCorrectionLevel(new ErrorCorrectionLevelHigh()) + ->size(200) + ->margin(0) + ->roundBlockSizeMode(new RoundBlockSizeModeMargin()) + ->build(); + return $this->render('user/2fa.html.twig', [ 'tab' => '2fa', 'user' => $profile, 'form' => $form->createView(), 'deactivate' => $this->getTwoFactorDeactivationForm($profile)->createView(), + 'qr_code' => $result, ]); } private function getTwoFactorDeactivationForm(User $user): FormInterface { - return $this->createFormBuilder( - [], - [ - 'action' => $this->generateUrl('user_profile_2fa_deactivate', ['username' => $user->getUserIdentifier()]), - 'method' => 'POST' - ] - )->getForm(); + return $this->createFormBuilder([], [ + 'action' => $this->generateUrl('user_profile_2fa_deactivate', ['username' => $user->getUserIdentifier()]), + 'method' => 'POST' + ])->getForm(); } #[Route(path: '/{username}/2fa_deactivate', name: 'user_profile_2fa_deactivate', methods: ['POST'])] @@ -405,28 +416,4 @@ final class ProfileController extends AbstractController return $this->redirectToRoute('user_profile_2fa', ['username' => $profile->getUserIdentifier()]); } - - #[Route(path: '/{username}/totp-qr-code', name: 'user_profile_2fa_image', methods: ['GET'])] - #[IsGranted('2fa', 'profile')] - public function displayTotpQrCode(User $profile, TotpAuthenticatorInterface $totpAuthenticator): Response - { - if (!$profile->hasTotpSecret()) { - throw $this->createNotFoundException('User has no TOTP secret.'); - } - - $qrCodeContent = $totpAuthenticator->getQRContent($profile); - - $result = Builder::create() - ->writer(new PngWriter()) - ->writerOptions([]) - ->data($qrCodeContent) - ->encoding(new Encoding('UTF-8')) - ->errorCorrectionLevel(new ErrorCorrectionLevelHigh()) - ->size(200) - ->margin(0) - ->roundBlockSizeMode(new RoundBlockSizeModeMargin()) - ->build(); - - return new Response($result->getString(), 200, ['Content-Type' => 'image/png']); - } } diff --git a/src/Security/TwoFactorCondition.php b/src/Security/TwoFactorCondition.php index 20c2443a..c9f6c5a3 100644 --- a/src/Security/TwoFactorCondition.php +++ b/src/Security/TwoFactorCondition.php @@ -25,11 +25,6 @@ final class TwoFactorCondition implements TwoFactorConditionInterface /** @var User $user */ $user = $context->getUser(); - // only internal users support 2FA currently - if (!$user->isInternalUser()) { - return false; - } - // never require 2FA on API calls if (str_starts_with($context->getRequest()->getRequestUri(), '/api/')) { return false; diff --git a/src/Voter/UserVoter.php b/src/Voter/UserVoter.php index ba311920..46c46a12 100644 --- a/src/Voter/UserVoter.php +++ b/src/Voter/UserVoter.php @@ -83,9 +83,8 @@ final class UserVoter extends Voter } if ($attribute === '2fa') { - // two factor only works for internal users and - // can only be activated by the logged-in user for himself - return $subject->isInternalUser() && $subject->getId() === $user->getId(); + // can only be activated by the logged-in user for himself or by a super-admin + return $subject->getId() === $user->getId() || $user->isSuperAdmin(); } $permission = $attribute; diff --git a/templates/user/2fa.html.twig b/templates/user/2fa.html.twig index c4158e71..ddec3f0a 100644 --- a/templates/user/2fa.html.twig +++ b/templates/user/2fa.html.twig @@ -8,7 +8,7 @@ {{ 'profile.2fa_intro'|trans }}

- TOTP QR Code + TOTP QR Code

{% if user.totpAuthenticationEnabled %}
diff --git a/tests/Controller/ProfileControllerTest.php b/tests/Controller/ProfileControllerTest.php index c359ff52..1d4d86c7 100644 --- a/tests/Controller/ProfileControllerTest.php +++ b/tests/Controller/ProfileControllerTest.php @@ -474,17 +474,36 @@ class ProfileControllerTest extends ControllerBaseTest self::assertFalse($user->hasTotpSecret()); $this->request($client, '/profile/' . UserFixtures::USERNAME_USER . '/2fa'); + $this->assertTrue($client->getResponse()->isSuccessful()); $user = $this->getUserByName(UserFixtures::USERNAME_USER); self::assertTrue($user->hasTotpSecret()); - $content = $client->getResponse()->getContent(); - self::assertNotFalse($content); - - $imgUrl = $this->createUrl('/profile/' . UserFixtures::USERNAME_USER . '/totp-qr-code'); - $this->assertStringContainsString('TOTP QR Code', $content); - $formUrl = $this->createUrl('/profile/' . UserFixtures::USERNAME_USER . '/2fa'); + $content = $client->getResponse()->getContent(); + $this->assertNotFalse($content); + + $this->assertStringContainsString('TOTP QR Code', $content); + } + + public function testTwoFactorAsAdmin(): void + { + $this->assertUrlIsSecuredForRole(User::ROLE_ADMIN, '/profile/' . UserFixtures::USERNAME_USER . '/2fa'); + } + + public function testTwoFactorAsSuperAdmin(): void + { + $client = $this->getClientForAuthenticatedUser(User::ROLE_SUPER_ADMIN); + + $this->request($client, '/profile/' . UserFixtures::USERNAME_USER . '/2fa'); + $this->assertTrue($client->getResponse()->isSuccessful()); + + $content = $client->getResponse()->getContent(); + $this->assertNotFalse($content); + $formUrl = $this->createUrl('/profile/' . UserFixtures::USERNAME_USER . '/2fa'); + + $this->assertStringContainsString('TOTP QR Code', $content); } @@ -522,32 +541,4 @@ class ProfileControllerTest extends ControllerBaseTest { $this->assertUrlIsSecured('/profile/' . UserFixtures::USERNAME_USER . '/2fa_deactivate', 'POST'); } - - public function testIsTwoFactorImageSecure(): void - { - $this->assertUrlIsSecured('/profile/' . UserFixtures::USERNAME_USER . '/totp-qr-code'); - } - - public function testTwoFactorImageFailsOnMissingSecret(): void - { - $client = $this->getClientForAuthenticatedUser(User::ROLE_USER); - - $this->request($client, '/profile/' . UserFixtures::USERNAME_USER . '/totp-qr-code'); - $this->assertRouteNotFound($client); - } - - public function testTwoFactorImage(): void - { - $client = $this->getClientForAuthenticatedUser(User::ROLE_USER); - - $user = $this->getUserByName(UserFixtures::USERNAME_USER); - self::assertFalse($user->hasTotpSecret()); - - // this is required, so the totp secret is stored in the user entity - $this->request($client, '/profile/' . UserFixtures::USERNAME_USER . '/2fa'); - - $this->request($client, '/profile/' . UserFixtures::USERNAME_USER . '/totp-qr-code'); - self::assertTrue($client->getResponse()->isSuccessful()); - self::assertEquals('image/png', $client->getResponse()->headers->get('Content-Type')); - } }