Commit 3c0307a0 authored by João Ventura's avatar João Ventura Committed by Joao Ventura
Browse files

Issue #3316499 by jcnventura: Further code refinements to TfaLoginContextTrait

parent 27789f9b

src/Form/TfaLoginForm.php

+4 −18
Original line number Diff line number Diff line
@@ -4,7 +4,6 @@ namespace Drupal\tfa\Form; 


use Drupal\Core\Form\FormStateInterface;

use Drupal\Core\Url;

use Drupal\tfa\Plugin\TfaSendInterface;

use Drupal\tfa\TfaLoginContextTrait;

use Drupal\tfa\TfaLoginTrait;

use Drupal\user\Form\UserLoginForm;
@@ -75,7 +74,9 @@ class TfaLoginForm extends UserLoginForm { 
    }



    // Similar to tfa_user_login() but not required to force user logout.

    $this->setUser($uid);

    /** @var \Drupal\user\UserInterface $user */

    $user = $this->userStorage->load($uid);

    $this->setUser($user);



    /* Uncomment when things go wrong and you get logged out.

    user_login_finalize($user);
@@ -84,7 +85,7 @@ class TfaLoginForm extends UserLoginForm { 
     */



    // Stop processing if Tfa is not enabled.

    if (!$this->isModuleSetup() || !$this->isTfaRequired()) {

    if ($this->isTfaDisabled()) {

      parent::submitForm($form, $form_state);

    }

    else {
@@ -116,8 +117,6 @@ class TfaLoginForm extends UserLoginForm { 
    }

    else {

      // Begin TFA and set process context.

      // @todo This is used in send plugins which has not been implemented yet.

      // $this->begin($tfaValidationPlugin);

      if (!empty($this->getRequest()->query->get('destination'))) {

        $parameters = $this->destination->getAsArray();

        $this->getRequest()->query->remove('destination');
@@ -189,17 +188,4 @@ class TfaLoginForm extends UserLoginForm { 
    }

  }



  /**

   * Begin the TFA process.

   *

   * @param \Drupal\tfa\Plugin\TfaSendInterface $tfaSendPlugin

   *   The send plugin instance.

   */

  protected function begin(TfaSendInterface $tfaSendPlugin) {

    // Invoke begin method on send validation plugins.

    if (method_exists($tfaSendPlugin, 'begin')) {

      $tfaSendPlugin->begin();

    }

  }



}

src/TfaLoginContextTrait.php

+59 −70
Original line number Diff line number Diff line
@@ -2,13 +2,19 @@ 


namespace Drupal\tfa;



use Drupal\Component\Plugin\Exception\PluginException;

use Drupal\user\UserInterface;



/**

 * Provide context for the current login attempt.

 *

 * This class collects data needed to decide whether TFA is required and, if so,

 * This trait collects data needed to decide whether TFA is required and, if so,

 * whether it is successful. This includes configuration of the module, the

 * current request, and the user that is attempting to log in.

 *

 * The methods defined in this trait require that the user property is defined,

 * so make sure to call the setUser method before using any other method here.

 *

 * @internal

 */

trait TfaLoginContextTrait {
@@ -21,13 +27,6 @@ trait TfaLoginContextTrait { 
   */

  protected $tfaPluginManager;



  /**

   * The tfaValidation plugin.

   *

   * @var \Drupal\tfa\Plugin\TfaValidationInterface|null

   */

  protected $tfaValidationPlugin;



  /**

   * Tfa settings config object.

   *
@@ -35,13 +34,6 @@ trait TfaLoginContextTrait { 
   */

  protected $tfaSettings;



  /**

   * The user storage.

   *

   * @var \Drupal\user\UserStorageInterface

   */

  protected $userStorage;



  /**

   * Entity for the user that is attempting to login.

   *
@@ -49,35 +41,14 @@ trait TfaLoginContextTrait { 
   */

  protected $user;



  /**

   * Array of login plugins.

   *

   * @var \Drupal\tfa\Plugin\TfaLoginInterface[]

   */

  protected $tfaLoginPlugins;



  /**

   * Set the user object.

   *

   * @param \Drupal\user\UserInterface $user

   *   The entity object of the user attempting to log in.

   */

  public function setUser($uid) {

    $this->user = $this->userStorage->load($uid);



    $this->tfaLoginPlugins = [];

    $login_definitions = $this->tfaPluginManager->getLoginDefinitions();

    if (!empty($login_definitions)) {

      foreach ($login_definitions as $plugin_id => $definition) {

        $this->tfaLoginPlugins[] = $this->tfaPluginManager->createInstance($plugin_id, ['uid' => $uid]);

      }

    }

    // If possible, set up an instance of tfaValidationPlugin and the user's

    // list of plugins.

    $validation_plugin = $this->tfaSettings->get('default_validation_plugin');

    if (!empty($validation_plugin)) {

      $this->tfaValidationPlugin = $this->tfaPluginManager->createInstance($validation_plugin, ['uid' => $uid]);

    }

    else {

      $this->tfaValidationPlugin = NULL;

    }

  public function setUser(UserInterface $user) {

    $this->user = $user;

  }



  /**
@@ -94,29 +65,23 @@ trait TfaLoginContextTrait { 
   * Is TFA enabled and configured?

   *

   * @return bool

   *   Whether or not the TFA module is configured for use.

   *   TRUE if TFA is disabled.

   */

  public function isModuleSetup() {

    return intval($this->tfaSettings->get('enabled')) && !empty($this->tfaSettings->get('default_validation_plugin'));

  public function isTfaDisabled() {

    // Global TFA settings take precedence.

    if (!($this->tfaSettings->get('enabled')) || empty($this->tfaSettings->get('default_validation_plugin'))) {

      return TRUE;

    }



  /**

   * Check whether $this->getUser() is required to use TFA.

   *

   * @return bool

   *   TRUE if $this->getUser() is required to use TFA.

   */

  public function isTfaRequired() {

    // If TFA has been set up for the user, then it is required.

    $user_tfa_data = $this->tfaGetTfaData($this->getUser()->id());

    // Check if the user has enabled TFA.

    $user_tfa_data = $this->tfaGetTfaData($this->user->id());

    if (!empty($user_tfa_data['status']) && !empty($user_tfa_data['data']['plugins'])) {

      return TRUE;

      return FALSE;

    }



    // If the user has a role that is required to use TFA, then return TRUE.

    // TFA is not necessary if the user doesn't have one of the required roles.

    $required_roles = array_filter($this->tfaSettings->get('required_roles'));

    $user_roles = $this->getUser()->getRoles();

    return (bool) array_intersect($required_roles, $user_roles);

    return empty(array_intersect($required_roles, $this->user->getRoles()));

  }



  /**
@@ -126,7 +91,23 @@ trait TfaLoginContextTrait { 
   *   TRUE if Validation Plugin exists and is ready for use.

   */

  public function isReady() {

    return isset($this->tfaValidationPlugin) && $this->tfaValidationPlugin->ready();

    // If possible, set up an instance of tfaValidationPlugin and the user's

    // list of plugins.

    $default_validation_plugin = $this->tfaSettings->get('default_validation_plugin');

    if (!empty($default_validation_plugin)) {

      /** @var \Drupal\tfa\Plugin\TfaValidationInterface $validation_plugin */

      try {

        $validation_plugin = $this->tfaPluginManager->createInstance($default_validation_plugin, ['uid' => $this->user->id()]);

        if (isset($validation_plugin) && $validation_plugin->ready()) {

          return TRUE;

        }

      }

      catch (PluginException $e) {

        return FALSE;

      }

    }



    return FALSE;

  }



  /**
@@ -134,8 +115,7 @@ trait TfaLoginContextTrait { 
   *

   * @return int|false

   *   FALSE if users are never allowed to log in without setting up TFA.

   *   The remaining number of times $this->getUser() may log in without setting

   *   up TFA.

   *   The remaining number of times user may log in without setting up TFA.

   */

  public function remainingSkips() {

    $allowed_skips = intval($this->tfaSettings->get('validation_skip'));
@@ -144,19 +124,19 @@ trait TfaLoginContextTrait { 
      return FALSE;

    }



    $user_tfa_data = $this->tfaGetTfaData($this->getUser()->id());

    $user_tfa_data = $this->tfaGetTfaData($this->user->id());

    $validation_skipped = $user_tfa_data['validation_skipped'] ?? 0;

    return max(0, $allowed_skips - $validation_skipped);

  }



  /**

   * Increment the count of $this->getUser() logins without setting up TFA.

   * Increment the count of user logins without setting up TFA.

   */

  public function hasSkipped() {

    $user_tfa_data = $this->tfaGetTfaData($this->getUser()->id());

    $user_tfa_data = $this->tfaGetTfaData($this->user->id());

    $validation_skipped = $user_tfa_data['validation_skipped'] ?? 0;

    $user_tfa_data['validation_skipped'] = $validation_skipped + 1;

    $this->tfaSaveTfaData($this->getUser()->id(), $user_tfa_data);

    $this->tfaSaveTfaData($this->user->id(), $user_tfa_data);

  }



  /**
@@ -168,13 +148,22 @@ trait TfaLoginContextTrait { 
   *   TRUE if login allowed otherwise FALSE.

   */

  public function pluginAllowsLogin() {

    if (!empty($this->tfaLoginPlugins)) {

      foreach ($this->tfaLoginPlugins as $plugin) {

        if ($plugin->loginAllowed()) {

    $login_definitions = $this->tfaPluginManager->getLoginDefinitions();

    if (!empty($login_definitions)) {

      foreach ($login_definitions as $plugin_id => $definition) {

        /** @var \Drupal\tfa\Plugin\TfaLoginInterface $login_plugin */

        try {

          $login_plugin = $this->tfaPluginManager->createInstance($plugin_id, ['uid' => $this->user->id()]);

          if (isset($login_plugin) && $login_plugin->loginAllowed()) {

            return TRUE;

          }

        }

        catch (PluginException $e) {

          continue;

        }

      }

    }



    return FALSE;

  }
@@ -183,7 +172,7 @@ trait TfaLoginContextTrait { 
   */

  public function doUserLogin() {

    // @todo Set a hash mark to indicate TFA authorization has passed.

    user_login_finalize($this->getUser());

    user_login_finalize($this->user);

  }



}

tests/src/Unit/TfaContextTest.php

+19 −29
Original line number Diff line number Diff line
@@ -103,7 +103,7 @@ class TfaContextTest extends UnitTestCase { 
   */

  protected function getFixture() {

    // Use simple anonymous class to add the TfaLoginContextTrait.

    return new class($this->tfaPluginManager, $this->configFactory, $this->user, $this->userData, $this->userStorage) {

    return new class($this->tfaPluginManager, $this->configFactory, $this->userData, $this->userStorage) {

      use TfaLoginContextTrait;



      /**
@@ -113,20 +113,19 @@ class TfaContextTest extends UnitTestCase { 
       *   The plugin manager for TFA plugins.

       * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory

       *   The configuration service.

       * @param \Drupal\user\UserInterface $user

       *   The user currently attempting to log in.

       * @param \Drupal\user\UserDataInterface $user_data

       *   The user data service.

       * @param \Drupal\user\UserStorageInterface $user_storage

       *   The user storage.

       */

      public function __construct(TfaPluginManager $tfa_plugin_manager, ConfigFactoryInterface $config_factory, UserInterface $user, UserDataInterface $user_data, UserStorageInterface $user_storage) {

      public function __construct(TfaPluginManager $tfa_plugin_manager, ConfigFactoryInterface $config_factory, UserDataInterface $user_data, UserStorageInterface $user_storage) {

        $this->tfaPluginManager = $tfa_plugin_manager;

        $this->tfaSettings = $config_factory->get('tfa.settings');

        $this->userData = $user_data;

        $this->userStorage = $user_storage;



        $this->setUser(3);

        /** @var \Drupal\user\UserInterface $user */

        $user = $user_storage->load(3);

        $this->setUser($user);

      }



    };
@@ -141,28 +140,13 @@ class TfaContextTest extends UnitTestCase { 
  }



  /**

   * @covers ::isModuleSetup

   * @covers ::isTfaDisabled

   */

  public function testIsModuleSetup() {

    // Defaults to false with empty mocked services.

  public function testIsTfaDisabled() {

    // Defaults to true with empty mocked services.

    $fixture = $this->getFixture();

    $this->assertFalse($fixture->isModuleSetup());

    $this->assertTrue($fixture->isTfaDisabled());



    // Enable.

    $settings = $this->prophesize(ImmutableConfig::class);

    $settings->get('enabled')->willReturn(TRUE);

    $settings->get('default_validation_plugin')->willReturn('foo');

    $config_factory = $this->prophesize(ConfigFactoryInterface::class);

    $config_factory->get('tfa.settings')->willReturn($settings->reveal());

    $this->configFactory = $config_factory->reveal();

    $fixture = $this->getFixture();

    $this->assertTrue($fixture->isModuleSetup());

  }



  /**

   * @covers ::isTfaRequired

   */

  public function testIsTfaRequired() {

    // User has setup TFA.

    $user_data = $this->prophesize(UserDataInterface::class);

    $user_data->get('tfa', 3, 'tfa_user_settings')->willReturn([
@@ -172,8 +156,14 @@ class TfaContextTest extends UnitTestCase { 
      'validation_skipped' => 1,

    ]);

    $this->userData = $user_data->reveal();

    $settings = $this->prophesize(ImmutableConfig::class);

    $settings->get('enabled')->willReturn(TRUE);

    $settings->get('default_validation_plugin')->willReturn('foo');

    $config_factory = $this->prophesize(ConfigFactoryInterface::class);

    $config_factory->get('tfa.settings')->willReturn($settings->reveal());

    $this->configFactory = $config_factory->reveal();

    $fixture = $this->getFixture();

    $this->assertTrue($fixture->isTfaRequired());

    $this->assertFalse($fixture->isTfaDisabled());



    // Not setup, no required roles matching the user.

    $user_data->get('tfa', 3, 'tfa_user_settings')->willReturn([
@@ -184,6 +174,7 @@ class TfaContextTest extends UnitTestCase { 
    ]);

    $this->userData = $user_data->reveal();

    $settings = $this->prophesize(ImmutableConfig::class);

    $settings->get('enabled')->willReturn(TRUE);

    $settings->get('default_validation_plugin')->willReturn('foo');

    $settings->get('required_roles')->willReturn(['foo' => 'foo']);

    $config_factory = $this->prophesize(ConfigFactoryInterface::class);
@@ -196,9 +187,8 @@ class TfaContextTest extends UnitTestCase { 
    $user_storage = $this->prophesize(UserStorageInterface::class);

    $user_storage->load(3)->willReturn($this->user);

    $this->userStorage = $user_storage->reveal();



    $fixture = $this->getFixture();

    $this->assertFalse($fixture->isTfaRequired());

    $this->assertTrue($fixture->isTfaDisabled());



    // Setup, matching roles.

    $user_data->get('tfa', 3, 'tfa_user_settings')->willReturn([
@@ -213,7 +203,7 @@ class TfaContextTest extends UnitTestCase { 
    $user->getRoles()->willReturn(['foo' => 'foo', 'bar' => 'bar']);

    $this->user = $user->reveal();

    $fixture = $this->getFixture();

    $this->assertTrue($fixture->isTfaRequired());

    $this->assertFalse($fixture->isTfaDisabled());

  }



  /**