Provide drush command to reset a user's TFA data

See attached patch for 8.x-1.x version of this module.

As this is a new feature, lets target it to 2.x first and than backport to 1.x. As a new feature this will need tests as well.

1. +++ b/src/Commands/TfaCommands.php
   @@ -56,4 +83,53 @@ class TfaCommands extends DrushCommands implements SanitizePluginInterface {
   +     */
   ...
   +      if (!Drush::bootstrapManager()->doBootstrap(DRUSH_BOOTSTRAP_DRUPAL_FULL)) {
   ...
   +      }
   

   This constant is deprecated.

   Per https://docs.drush.org/en/9.x/bootstrap/
   "Commands supplied by Drupal modules are always @bootstrap full"
   Has that changed in newer Drush versions?

2. +++ b/src/Commands/TfaCommands.php
   @@ -56,4 +83,53 @@ class TfaCommands extends DrushCommands implements SanitizePluginInterface {
   +      if (empty($account)) {
   +        $account = User::load(1);
   +      }
   +
   

   Given this command can cause TFA to be disabled for a user I would prefer we not default to user 1 and instead error out.

3. +++ b/src/Commands/TfaCommands.php
   @@ -56,4 +83,53 @@ class TfaCommands extends DrushCommands implements SanitizePluginInterface {
   +      $answer = $this->io()->confirm('Are you sure you want to reset ' . $account->getAccountName() . ' (UID: ' . $account->id() . ")'s data?", $do_run_if_no_input);
   

   Should this use dt() and placeholders as well?

4. +++ b/src/Commands/TfaCommands.php
   @@ -56,4 +83,53 @@ class TfaCommands extends DrushCommands implements SanitizePluginInterface {
   +
   +      $this->logger('tfa')->notice('TFA deleted and reset for user ' . $account->getAccountName() . ' (UID ' . $account->id() . ')');
   

   This should use the context parameter and placeholders instead of concatenating.

5. +++ b/src/Commands/TfaCommands.php
   @@ -56,4 +83,53 @@ class TfaCommands extends DrushCommands implements SanitizePluginInterface {
   +      $this->mailManager->mail('tfa', 'tfa_disabled_configuration', $account->getEmail(), $account->getPreferredLangcode(), $params);
   

   $account->getEmail() can return NULL which could cause an error as mail() expects a string for $to.

6. Instead of using User::load(), user_load_by_name(), and user_load_by_mail() it will probably make writing tests easier if we inject the EntityTypeManager so we can mock through injection instead of the global container.

I have updated patch from #2 and addressed review feedback from #4 .

I think it is ready to Merge. Any additional changes required?

Thanks all for adjusting the patch and writing tests, I don't time/know how to do all these things. 🙂

Just wanted to allow a bit of time for everyone who had worked on it to comment after I made the last minor commits changes based on MR feedback before committing. Sounds like everyone is good with it.

Automatic rebase on latest head, and committed to Dev. Thanks all who worked on this so far.

I see no reason not to consider this for 8.x-1.x as well, however it does not cleanly cherry-pick, setting PTBP for followup.

I have backported patch from mr59 to 8.x branch in mr62
Please review

NW for code errors.

PHPUnit stage shows a declaration failure along with the code quality report shows methods being called with 3 parameters when 4 are required.

I am just wondering why we are getting error on 8.x branch and not on 2.x branch, when the changes are same.

I am just wondering why we are getting error on 8.x branch and not on 2.x branch, when the changes are same.

Difference in the codebase, there has been a lot of refactoring since 8.x-1x

PHP Fatal error: Drupal\tfa\Commands\TfaTokenManagement and Drupal\tfa\TfaUserDataTrait define the same property ($userData) in the composition of Drupal\tfa\Commands\TfaTokenManagement. However, the definition differs and is considered incompatible. Class was composed in /builds/issue/tfa-3381701/web/modules/custom/tfa-3381701/src/Commands/TfaTokenManagement.php on line 20

That is likely caused by the property promotion of the constructor clashing with the trait. We actually can’t use property promotion in 8.x-1.x as it’s a PHP8 only feature so we probably need to refactor that out anyways to use the older $this->property = $passed_in_service; method.

Major - Method Drupal\tfa\Commands\TfaTokenManagement::deleteUserData() invoked with 3 parameters, 4 required.

In 8.x-1.x the User Data service needs to be provided to the method as the fourth option, in 2.x it pulls from the property.

I have backported patch to 8.x branch. But seems there is issue with logger channel argument passing in TfaTokenManagement Class.

I am getting below error when running drush command

drush cc drush

In LegacyServiceInstantiator.php line 282:

  You have requested a non-existent parameter "logger.channel.tfa".

If I try to add logger channel in tfa.services.yml files still getting above error.

  logger.channel.tfa:
    parent: logger.channel_base
    arguments: ['tfa']

If I add same code in drush.services.yml I am getting below error.

drush tfa:reset-user


  There are no commands defined in the "tfa" namespace.

It should definitely be in the tfa.services.yml not the drush.services.yml for the logger service.

Just clearing the drush bin would probably not be enough as tfa.services.yml would be in the Drupal container not the Drush container.

I have updated the service.yml file and the drush command is working as expected. Please re-review.

@cmlara I have made the required changes. Except few PHPStan errors which can be added to ignore list I think the MR #62 is ready.

Thanks for all the work,

Merged the backport to 8.x-1.x.

Automatically closed - issue fixed for 2 weeks with no activity.