Force user to setup TFA when required and there are no remaining skips

Hopefully fixed the patch not applying + interdiff of Simeon's changes in #7.

On second thought, it doesn't make sense to bypass the one-time-login.
Related: #2930355: Password reset bypasses TFA

Moving all open issues to the 2.x branch.

Rerolled the latest patch against 2.x-dev:

• Removed support for 'remaining skips' when forced 2FA is selected for now, Makes it a lot easier to implement and communicate.
• Rewritten the EventSubscriber and functional tests, reused the available TfaLoginContextTrait.
• Excluded all 2FA and user login/logout/password paths from forced 2FA redirect.

Maybe it's a good idea to move extra functionalities like configurable whitelist or remaining skips integration (needs new messages) to new tickets and focus on the basics first.

Fixed patch

Excluded user edit routes for password reset support

Trying to applying latest patch not applying. Added screenshot for reference.

@Madhu Kumar M E

Patch should work, tests also passed. Please make sure you are applying the patch correctly: through composer-patches or manually at the tfa module root using the 2.x-dev branch. Looking at your screenshot the patch was not applied to the tfa module directory.

@Maico de Jong

I made a mistake, i didn't checked my directory properly.
Verified the patch #16 and tested it on Drupal version 10.1.x. The patch works fine also and I have added the screenshots for reference.

Thanks for reminding me.

I was just testing this and noticed that when "Force TFA setup" is selected the text box for "Skip Validation" is hidden. Is this as designed?

1. +++ b/src/EventSubscriber/ForceTfaSetup.php
   @@ -0,0 +1,134 @@
   +    $user = $this->userStorage->load($this->currentUser->id());
   +    $this->setUser($user);
   +
   +    if ($this->isReady() || !$this->forceTFASetup()) {
   +      return;
   +    }
   +
   

   This can be VERY heavy latency. While the built in plugins can get this with just a few database queries, on a test(non performance tuend) laptop redirect() this ends up being ~10% of the request time (22ms) for the front page to load.

   With other authentication plugins isReady() could trigger a network query which will add even more latency and network dependency, see https://git.drupalcode.org/project/tfa_vault_totp/-/blob/0.x/src/Plugin/... for an example.

   I've been experimenting with using the request_session and parameter bags, perhaps similar may be a good fit here?.

2. +++ b/src/EventSubscriber/ForceTfaSetup.php
   @@ -0,0 +1,134 @@
   +    $ignored_route_names = [
   

   I'm debating if we should we include system.403 and system.404 and system.4xx in here as well? I hit #3339277: Inform admin that role does not have permission to setup own tfa and ended up in an endless redirect loop. Ideally this shouldn't ever happen when redirecting to the TFA setup page, and fixing that issue would solve this concern here, however I can't help wondering if there might be other hidden problems and that exempting these paths might be wise.

#3108099: Redirect to validation setup after login without tfa would appear to be a related request.

Rerolling #16 as patch is not applying clean anymore.

Missed the EventSubscriber and functional test on #23.

Adding patch again.

Added patch for version 8.x-1.x

Re-rolling #25 as failing to applied on 8.x-1.2

Re-rolling patch for version 8.x-1.x

Missed the service and the unit test on last patch. Re-rolling patch for version 8.x-1.x

Following up on my comments from #22

In #3339277: Inform admin that role does not have permission to setup own tfa we did not actually prevent an access denied error as the action to require TFA is different from being permitted to setup TFA, as such at the system.403 should be exempted (at least for the TFA setup page) to prevent a loop.

The patch prevents generating CSS and JS aggregation in Drupal 10.1.x. fixed by excluding the CSS and JS asset routes from the list.

@yovince I used your patch its fine working Drupal 10.1.x

Linking a dupe #2962542: Implement a configurable setting that allows site admins to force users to setup TFA

Rerolled patch for latest 2.x verison

Patch is not getting applied for 8.x-1.5 tfa module version along with 10.2.2 drupal version.

Rerolled patch for latest 8.x-1.x verison, working fine with Drupal 10.2.2

Hello all,

Apologies Narendra :) I've spotted four issues with the patch in #35:

- Hunk in tfa.schema.yml doesn't apply because indentation of two subsequent lines is missing a space

- Indentation is 4 spaces instead of two

- Hiding the Allowed Validation plugins field when "force redirection" is enabled seems like a bad idea to me; when hidden, the administrator can't change which validation plugins are selected, and because this field is below the "force" checkbox, users working from top to bottom down the form might never see the allowed validation plugins field.

- A typo; "FTA"

Attaching a patch which:

- Fixes broken indentation - bad:

@@ -11,6 +11,9 @@ tfa.settings:
       sequence:
         type: string
         label: 'Role'
+    forced:
+      type: integer
+      label: 'Force required roles to setup when on last validation skip'
     send_plugins:
      type: sequence
      label: 'Enabled send plugins'

Good (i.e. the last two lines):

@@ -11,6 +11,9 @@ tfa.settings:
       sequence:
         type: string
         label: 'Role'
+    forced:
+      type: integer
+      label: 'Force required roles to setup when on last validation skip'
     send_plugins:
       type: sequence
       label: 'Enabled send plugins'

- Fixes indents - bad:

+    public function __construct(RouteMatchInterface $route_match, MessengerInterface $messenger, AccountProxyInterface $current_user, ConfigFactoryInterface $config_factory,
+                                EntityTypeManagerInterface $entity_type_manager, UserDataInterface $user_data, TfaValidationPluginManager $tfa_validation_manager, TfaLoginPluginManager $tfa_plugin_manager) {
+        $this->messenger = $messenger;
+        $this->routeMatch = $route_match;
+        $this->currentUser = $current_user;
+        $this->userStorage = $entity_type_manager->getStorage('user');
+        $this->tfaSettings = $config_factory->get('tfa.settings');
+        $this->userData = $user_data;
+        $this->tfaValidationManager = $tfa_validation_manager;
+        $this->tfaLoginManager = $tfa_plugin_manager;
+    }
+
+    /**
+     * Redirect users to TFA overview when no remaining skips.
+     *
+     * @param \Symfony\Component\HttpKernel\Event\RequestEvent $event
+     *   The request event.
+     */
+    public function redirect(RequestEvent $event): void {
+        /** @var \Drupal\user\UserInterface $user */
+        $user = $this->userStorage->load($this->currentUser->id());
+        $this->setUser($user);
+
+        if ($this->isReady() || !$this->forceTFASetup()) {
+            return;
+        }
+
+        $this->messenger->addWarning(t('You need to enable Two Factor Authentication.'));
+
+        // Don't redirect the user if on password/profile edit page,
+        // as it is possible the user used one-time login URL
+        // and need to change the password.
+        $ignored_route_names = [
+            'user.login',
+            'user.logout',
+            'user.pass',
+            'user.edit',
+            'entity.user.edit_form',
+            'user.reset.login',
+            'user.reset',
+            'user.reset.form',
+            'user.well-known.change_password',
+            'tfa.entry',
+            'tfa.login',
+            'tfa.overview',
+            'tfa.validation.setup',
+            'tfa.disable',
+            'tfa.plugin.reset',
+        ];
+        if (in_array($this->routeMatch->getRouteName(), $ignored_route_names)) {
+            return;
+        }
+
+        $tfa_overview_url = Url::fromRoute('tfa.overview', ['user' => $this->user->id()]);
+        $event->setResponse(new RedirectResponse($tfa_overview_url->toString()));
+    }
+
+    /**
+     * {@inheritdoc}
+     */
+    public static function getSubscribedEvents() {
+        $events[KernelEvents::REQUEST][] = ['redirect', 32];
+        return $events;
+    }
+
+}

Good:

+    public function __construct(RouteMatchInterface $route_match, MessengerInterface $messenger, AccountProxyInterface $current_user, ConfigFactoryInterface $config_factory,
+                                EntityTypeManagerInterface $entity_type_manager, UserDataInterface $user_data, TfaValidationPluginManager $tfa_validation_manager, TfaLoginPluginManager $tfa_plugin_manager) {
+      $this->messenger = $messenger;
+      $this->routeMatch = $route_match;
+      $this->currentUser = $current_user;
+      $this->userStorage = $entity_type_manager->getStorage('user');
+      $this->tfaSettings = $config_factory->get('tfa.settings');
+      $this->userData = $user_data;
+      $this->tfaValidationManager = $tfa_validation_manager;
+      $this->tfaLoginManager = $tfa_plugin_manager;
+    }
+
+    /**
+     * Redirect users to TFA overview when no remaining skips.
+     *
+     * @param \Symfony\Component\HttpKernel\Event\RequestEvent $event
+     *   The request event.
+     */
+    public function redirect(RequestEvent $event): void {
+      /** @var \Drupal\user\UserInterface $user */
+      $user = $this->userStorage->load($this->currentUser->id());
+      $this->setUser($user);
+
+      if ($this->isReady() || !$this->forceTFASetup()) {
+          return;
+      }
+
+      $this->messenger->addWarning(t('You need to enable Two Factor Authentication.'));
+
+      // Don't redirect the user if on password/profile edit page,
+      // as it is possible the user used one-time login URL
+      // and need to change the password.
+      $ignored_route_names = [
+        'user.login',
+        'user.logout',
+        'user.pass',
+        'user.edit',
+        'entity.user.edit_form',
+        'user.reset.login',
+        'user.reset',
+        'user.reset.form',
+        'user.well-known.change_password',
+        'tfa.entry',
+        'tfa.login',
+        'tfa.overview',
+        'tfa.validation.setup',
+        'tfa.disable',
+        'tfa.plugin.reset',
+      ];
+      if (in_array($this->routeMatch->getRouteName(), $ignored_route_names)) {
+          return;
+      }
+
+      $tfa_overview_url = Url::fromRoute('tfa.overview', ['user' => $this->user->id()]);
+      $event->setResponse(new RedirectResponse($tfa_overview_url->toString()));
+    }
+
+    /**
+     * {@inheritdoc}
+     */
+    public static function getSubscribedEvents() {
+      $events[KernelEvents::REQUEST][] = ['redirect', 32];
+      return $events;
+    }
+
+}

- Removes the behaviour to hide allowed validation plugins - i.e. this chunk:

@@ -186,7 +194,14 @@ class SettingsForm extends ConfigFormBase {
       '#default_value' => $config->get('allowed_validation_plugins') ?: ['tfa_totp'],
       '#description' => $this->t('Plugins that can be setup by users for various TFA processes.'),
       // Show only when TFA is enabled.
-      '#states' => $enabled_state,
+      '#states' => [
+        'visible' => [
+          [
+            ':input[name="tfa_enabled"]' => ['checked' => TRUE],
+            ':input[name="tfa_forced"]' => ['checked' => FALSE],
+          ],
+        ],
+      ],
       '#required' => TRUE,
     ];
     $form['tfa_validate'] = [

- Fixes the typo "FTA":

+      '#description' => $this->t('Force TFA setup on login, redirect user to FTA overview page.'),

... to:

+      '#description' => $this->t('Force TFA setup on login, redirect user to TFA overview page.'),

Found another incorrect indent - updated patch attached.

Playing whack-a-mole with indentation... [facepalm.gif]

Generally development should occur on the latest branch first (2.x) and be backported to older branches once complete. This helps prevent a feature or bugfix being lost in newer versions. Addtionaly it reduces overall effort of maintaining multiple branches at once.

Leaving issue as needs-work per #22 and #29 comments.

Note: As part of the DrupalCi deprecation process the TFA project has already been migrated to only use GitLabCI for testing of the D8+ development branches. Due to this change we need contributors to utilize the MR workflow going forward as patches are no longer testable in the TFA project.

Contributors will likely encounter this with other projects as the patch workflow as DrupalCi has already been significantly limited as of Febuary 2024 and will be fully disabled for all D.O. hosted projects on July 1st. See https://www.drupal.org/about/core/blog/drupalci-and-all-patch-testing-wi... for more information.

Hi @cmlara, apologies - I provided the patch in #38 in good faith to fix the broken patch in #35. I'm not working with the 2.x branch due to SA-CONTRIB-2024-003, so wasn't able to provide a patch against that branch.

Hopefully the patch at #38 will help those still on 8.x-1.x ¯\_(ツ)_/¯

/A

Hi all!

I tried to update the patch from #30 the the new rerolled patch added in #38. I noticed a major difference (2 lines hehe), and thats in the $ignored_routes array. As both css and JS are not ignored, your assest will not be loaded in. Basicly #30 comment got reversed. Please reroll the patch of #38, but with the systems routes added in :)

Hi MarcellinoStroosnijder,

> Please reroll the patch of #38, but with the systems routes added in :)

I'm away from that project now so won't be able to do this, but feel free to do the re-roll yourself :)

/Alex

Dear friends,

I have created a merge request 89 (MR) incorporating the various patches. You can review the MR and apply it as a patch. It is based on the 2.x branch.

This is the initial commit, so I would appreciate your feedback on whether it makes sense and if everything looks correct.

Please let us know if any parts are missing or if there are additional tasks that need to be addressed.

Thank you.

Provided feedback in the MR. Leaving as NW for concerns.

Can you please read through the logic, let me know if I missed something:

Adding event_subscriber which listens to KernelEvents::REQUEST, make some checks, and if needed - redirect the user to TFA overview page - preventing navigating to other pages in the site.

The checks are:

• Is TFA is enabled?
• Is the current route applicable for redirect? (we should allow logging-out, running-cron, css/js/images assets, and setting up the TFA)
• Is user logged in? (anonymous users are irrelevant)
• Is TFA setting force user to setup TFA?
• Does TFA has at least one validation plugin enabled?

At this point we should set a warning in messenger: "You must setup TFA".

Further checks:

• Is this route applicable for a Redirect?
• If not, should we block the request altogether?
• Are there situations that we would allow bypassing this behavior?

At this point we should Issue the redirect for TFA overview page.

The order of the checks are not final, and should sorted by their complexity and latency impact.