Remove fallback plugins, make recovery code concept into validation & setup plugins

This looks good @therealssj.

Not sure why, but I couldn't get #2 to apply cleanly against 8.x-1.x branch. It was having trouble with the EntryForm.php changes. So I applied your patch and fixed EntryForm by hand. Attached is a re-roll of your patch. Results should be the same.

Is this ready for review?

Yes please. I didn’t have a chance to test thoroughly, just looked over the patch and tried applying it.

Not sure why #2 wouldn’t apply, could you try it first in case there’s something off about my local repo?

Note on testing this patch. The recovery code form is currently broken. See: #2924970: Recovery code form broken- checking for code value in non-existent field

I see that #2924970: Recovery code form broken- checking for code value in non-existent field has been fixed, so that should simplify testing.

Please update the issue description to explain further what the goal is. I think the idea is to entirely remove the idea of a "fallback plugin". Currently, recovery codes are implemented as a fallback plugin, and the goal is to make them more like any other TFA plugin. That seems like a good idea, but it seems that there is just as much code in EntryForm.php. I would expect more savings. Is that because this is just an intermediate step? Maybe it would be simpler to postpone this issue until #2923786: Allow users to select from multiple 2nd-Factor approaches is fixed.

For now, here is a partial code review. The first two items are picky (but easy to fix). The third is more important.

1. Comments should be full sentences, starting with a capital letter:

+      // show secondary plugins only if a validation plugin is activated.
...
+          // recovery codes setup.

2. Do we need two blank lines?

@@ -410,9 +381,12 @@ class SettingsForm extends ConfigFormBase {
    */
   public function submitForm(array &$form, FormStateInterface $form_state) {
     $validation_plugin = $form_state->getValue('tfa_validate');
-    $fallback_plugins = $form_state->getValue('tfa_fallback');
     $validation_plugin_settings = $form_state->getValue('validation_plugin_settings');

+    $recovery_codes_enabled = $form_state->getValue('recovery_codes_enabled');
+    $recovery_codes_settings = $form_state->getValue('recovery_codes')['settings'];
+
+

3. It looks as though this patch removes one setting and adds another. We should add an update function to remove the obsolete setting. If we can create the new setting at the same time, that will help users upgrading from the alpha1 release. If that is not possible, then we should at least add upgrade instructions to the README.

-    $settings = \Drupal::config('tfa.settings')->get('fallback_plugins');
-    $this->codeLimit = (isset($settings[$validation_plugin]['tfa_recovery_code']['settings']['recovery_codes_amount'])) ? $settings[$validation_plugin]['tfa_recovery_code']['settings']['recovery_codes_amount'] : 9;
+    $settings = \Drupal::config('tfa.settings')->get('recovery_codes_settings');
+    $this->codeLimit = (isset($settings['recovery_codes_amount'])) ? $settings['recovery_codes_amount'] : 9;

Update: I just noticed that #2923786: Allow users to select from multiple 2nd-Factor approaches is listed as a child issue of this one. If you think it makes more sense to handle this one first, that is OK with me. I expect we will get savings in EntryForm.php once the two issues are done, in whichever order.

I was about to review the patch and re-roll it with some adjustments but I realized that the patch will need to be re-rolled after #2928728: Remove Totp and Hotp validation plugins after they are moved to ga_login due to some changes that were necessary to get tests working. I'm going to hold off for the moment.

Also, we might need to discuss the approach a little more on this issue. I don't see the value in making a new setting for "Enable Recovery Codes" and treating it like a special validation plugin. As this is currently written, we're removing the concept of a fallback plugin in name only because we continue to treat recovery codes as a fallback plugin by making it a special case.

I think instead, recovery codes should just be another Validation plugins listed with the others. Then when we deal with #2923786: Allow users to select from multiple 2nd-Factor approaches, it will appear within the list of other Validation plugins and can be enabled just like everything else. We can recommend in the UI that site owners enable the recovery code validation plugin without treating it as a special case.

Thoughts?

@benjifisher - thanks for these notes

> We should add an update function to remove the obsolete setting.

We definitely need to consider this within this issue or the related #2923786. Data structures will be changing.

> I expect we will get savings in EntryForm.php once the two issues are done, in whichever order.

Same here, I'd hope we can simplify much by removing an entire concept. I'm not really sure this issues has to be before #2923786. Now that I'm thinking about it more, it might be easier to do #2923786 first, make the data structure adjustments as needed. Then in this issue we remove the fallback concept and enable recovery_codes validation plugin in an update hook if the site had them enabled in the old structure.

Also, I'm going to change #2923786 to be related to this issue rather than a child of it.

Thoughts?

I think we agree. First priority is #2928728: Remove Totp and Hotp validation plugins after they are moved to ga_login. Then let's do #2923786: Allow users to select from multiple 2nd-Factor approaches before returning to this issue.

Circling back around to this a bit today. This is a work in progress patch, but you can see where I'm going with it.

I've:

• Removed all references to fallbacks
• Modified the recovery code plugin to be a normal validation plugin
• Written a setup plugin for recovery codes
• Create a TfaRandomTrait for generating cryptographically secure random strings.
• Test for setting up recovery codes (not passing yet)

And maybe a little more...

Still needs work around the tests as well as manual testing of various other aspects of TFA.

Updated patch, now with passing tests.

Also did a lot of manual testing around the settings page, setting up the plugin, and validation using the plugin.

This change would make the tfa module no longer dependent on the "christian-riesen/otp": "2.*" library.

Updated patch that removes this dependency. Note, doing this means GA_Login module will need to include the dependency on its own. I'll make an issue in that module queue for it.

Patch no longer applies. Needs reroll.

Rerolled the patch.

Updated patch, now with passing tests.

Added the required modules in the composer.json file so the patch will pass the tests.

Made changes to the composer.json file so the patch will pass the tests.

Patch fails to apply in a lot of ways (that's what I get for waiting 1 year).

Attempted re-roll. Getting a number of errors during tests relate to deprecated methods, so this may be a WIP patch.

Went ahead and replaced drupal_set_message() calls with new messenger service to mitigate some of the errors.

Found one of the errors. Let's see how this goes.

Reroll because I commited another patch that made one of the changes that this patch was making.

The patch #27 works correctly on my local environment. I did an initial pass on the patch, but I will try to take a more in-depth look at the new classes later.

1. +++ b/composer.json
   @@ -1,17 +1,17 @@
   -    "name": "drupal/tfa",
   

   The indentation change here is incorrect: composer json/lock files have a different code style. See https://git.drupalcode.org/project/drupal/blob/8.8.x/.editorconfig

2. +++ b/config/schema/tfa.schema.yml
   @@ -76,32 +71,14 @@ tfa.settings:
   +  label: 'Tfa Recovery Code settings'
   

   Other labels in the schema file use 'TFA' instead of 'Tfa'. Let's keep this TFA for consistency

3. +++ b/src/Form/BasicDisable.php
   @@ -155,7 +155,7 @@ class BasicDisable extends FormBase {
   +      $this->messenger()->addStatus($this->t('TFA disable canceled.'));
   
   @@ -180,7 +180,7 @@ class BasicDisable extends FormBase {
   +    $this->messenger()->addWarning($this->t('TFA Disable cancelled.'));
   

   These interface texts are inconsistent. Drupal uses American English (cancelled)

4. +++ b/src/Plugin/TfaSetup/TfaRecoveryCodeSetup.php
   @@ -0,0 +1,196 @@
   +        '#value' => $this->t('Recovery Codes'),
   ...
   +      '#title' => $this->t('Recovery codes'),
   

   These interface texts are inconsistent. Let's stick with 'Recovery codes'

Thanks for the review!

Here is an updated patch that addresses the items you listed. There are more inconsistencies throughout the module along the same lines as the items you mention, but I didn't address them here. Would be nice to standardize a lot of that in another issue.

Did some more local testing and the patch works great!

A few nitpicks as a final review:

1. +++ b/src/Plugin/TfaSetup/TfaRecoveryCodeSetup.php
   @@ -0,0 +1,196 @@
   +  use StringTranslationTrait;
   

   This trait is already added in \Drupal\tfa\Plugin\TfaValidation\TfaRecoveryCode

2. +++ b/src/Plugin/TfaSetup/TfaRecoveryCodeSetup.php
   @@ -0,0 +1,196 @@
   +   *    True or False based on the checks performed.
   

   Comment indentation must be 3 spaces

3. +++ b/src/Plugin/TfaSetup/TfaRecoveryCodeSetup.php
   @@ -0,0 +1,196 @@
   +  public function getOverview($params) {
   

   Type hint "array" missing for $params

4. +++ b/src/Plugin/TfaSetup/TfaRecoveryCodeSetup.php
   @@ -0,0 +1,196 @@
   +              'reset' => '1',
   

   $reset is expected to be an integer (a boolean would make more sense though)

5. +++ b/src/Plugin/TfaValidation/TfaRecoveryCode.php
   @@ -33,27 +33,19 @@ class TfaRecoveryCode extends TfaBasePlugin implements TfaValidationInterface {
   +    $codes_amount = \Drupal::config('tfa.settings')
   +      ->get('validation_plugin_settings.tfa_recovery_code.recovery_codes_amount');
   

   The \Drupal::config() call should be replaced with dependency injection (config.factory service)

6. +++ b/src/Plugin/TfaValidation/TfaRecoveryCode.php
   @@ -137,6 +140,19 @@ class TfaRecoveryCode extends TfaBasePlugin implements TfaValidationInterface {
   +   * Generate an array of secure recovery codes.
   

   Missing @return statement

7. +++ b/tests/src/Functional/TfaRecoveryCodePluginTest.php
   @@ -0,0 +1,190 @@
   + * @group Tfa
   
   +++ b/tests/src/Unit/Plugin/TfaValidation/TfaRecoveryCodeTest.php
   @@ -17,7 +17,7 @@ use Drupal\user\UserDataInterface;
   + * @group Tfa
   
   +++ b/tests/src/Unit/TfaContextTest.php
   @@ -16,7 +16,7 @@ use Symfony\Component\HttpFoundation\Request;
   + * @group Tfa
   

   These changes are incorrect: The capitalized group names are referring to core component names.
   See https://www.drupal.org/project/drupal/issues/2301481

Thanks!

I have most of these resolved in this patch, but skipped number 3. Details:

1. ✅ Extra trait, removed
2. ✅ Fixed comment indent
3. Can't easily do this one. TfaSetupInterface doesn't type hint the array there, so adding a type hint in TfaRecoveryCodeSetup::getOverview() causes a fatal error. We could add the type hint to TfaSetupInterface, but then all setup plugins that don't have the array type hint will break. Would be nice to get this type hint into place eventually, but I think it's too much to put into this patch.
4. ✅ Changes '1' to 1. I don't think it really matters here because we're building a URL, but I like consistency so why not!
5. ✅ Good catch. Our ::create method wasn't doing anything at all here until I implemented ContainerFactoryPluginInterface.
6. ✅ Added return statement
7. ✅ Updated 'Tfa' to 'TFA' for lines already involved in this class.

Looking good, nice work!

Committed. Thanks for everyone for the help!