Create the Send Plugin API with a working test

Hi, I created a patch with the Send Plugin API. I didn't have time to handle the test as well.
Here is my patch

In regards to: * Tease out TfaSendInterface.php. Is begin() the only necessary method?

I'm not sure begin() is the right method even. I see send() as the main method, the reason for this is that it doesn't have to happen at the beginning of the validation process. Most send plugins will use a 'resend' button and having this fire the begin() method all over again seems potentially odd.

Or it might be that a user has two methods enabled, but an authenticator otp code is the default, in this situation you might not want to send unnecessary codes until the user explicitly choose the SMS verification, or some other time of a user's choosing.

On top of this I'm wondering if there aren't things that happen once at the beginning but not when re-sending a code, or vice versa.

Anyway, re-rolled the patch and added some tests to start with.

I'm going to add this comment here although it is broader than just the send plugin. I see a few things holding back the send plugins with the current setup, let me know what you think...

1. There should be a way for setup plugins to validate against the appropriate validate plugin immediately after being configured (perhaps we should consider splitting the setup routes into configuration and validation, or build in multi step support to the setup plugin forms). Currently the ga_login totp plugins seem to import the validation form into the setup form which doesn't work well when you have to configure the setup plugin _before_ it can be validated e.g. adding a phone or email to send the codes to or even adding a user configurable string to generate the totp code.
2. It seems to me that the begin method of a send plugin should be fired the moment the relevant validation plugin is instantiated and the validation form retrieved. There are plenty of instances where a user might enable multiple second factor methods on their account and we would only want the begin methods for each one to be fired when that validation plugin was instantiated e.g. if totp is their preferred method we don't want to send unnecessary text messages or emails to them.

I've attached a little ditty scribble to try and get across these thoughts.

@kalpaitch I have created a patch that can be used to send codes to email but also validate and also remove them incase they haven't been used with-in a specific period of time. I am open to helping out with this initiative.

I just realized patch 'create_the_send_plugin_api-2930532-11.patch' is an empty file. Please find attached create_the_send_plugin_api-2930532-12.patch

Thanks

An updated patch. The last one missed out some new files I had added.

Patch create_the_send_plugin_api-2930532-13.patch is ready for review.

That's awesome, thanks that would be awesome, I'm sure we can pick the better parts of each patch. Before I get started though I think my comments from #2930532-10: Create the Send Plugin API with a working test still stand:

1. Should there not be a unified way for setup plugins to validate against the appropriate validate plugin immediately after being configured?
2. Should the begin method of a send plugin be fired the moment the relevant validation form is retrieved?

IMO these should both to be implemented, but the first point will require a re-work to how existing plugins (ga_login) validate, happy to provide these patches too though. @daggerhart what are your thoughts?

The ValidationSendableInterface is a good idea but I was wondering the usefulness of getSendPlugin() function. My thinking is that it would be applicable to any validation plugin a user has enabled. Also how do we use this with a service such as Authy in which the code is generated by another service?

I think this approach makes a lot more sense. I was going to say that send plugins are tied to a validation plugin in a one to one relationship, and this is probably the best way of representing this.

In terms of only sending the code once per form, I suspect we could either cache the form with the session context or store the hashed code in the session manager.

I agree that the begin() method is best suited to the EntryForm. Not that it bothers me either way but I don't see the need to change the send interface's name, while they might be part of the same plugin they do different things and naming the send interface off the validation interface might misrepresent it's function. But don't think it matters either way.

It's probably worth identifying when the validation plugin should begin again:
if the user closes the browser or logs in again then they should get the code again.
if the user goes to another TFA plugin form and comes back they should not get the code again.
if the user refreshes the page they should not get the code again.
if the user hits the re-send button then they should get the code again.

I think you're right to say that based on this the session is the only appropriate gate keeper for whether the resend the code.

If you don't want to set a variable in the session directly perhaps we could cache the method call with the session context. I briefly messed around with having a lazy loaded element on the validation form that calls begin(), really loose and experimental idea but:

public function begin($session) {
  if ($cache = \Drupal::cache($session->id())) {
    return $cache;
  }
  else {
    $this->send();

    return [
      '#markup' => 'Render ajax button calls send() method',
    ];
  }
}

So long as the PrivateTempStore doesn't persist the session, which I guess it shouldn't for anonymous users (but does for identified users), then this probably is better, worth checking this explicitly I guess.

A second point to make is that currently we're setting the `dont' send marker` in the session when the plugin sends out the code (and then we remove it when the form is submitted. I wonder how this functions when the user doesn't submit the validation form and instead hits the back button and logs in again for example.

Perhaps a more hardy approach would be to set a `to be sent marker` on every plugin whenever the login form is submitted (this appears to be our refresh marker), this way the plugin can remove the marker as appropriate when it issues the code. At this point the only thing that could issue another code would be logging in again.

This might resolve issues like above where the persistence of the cache could be a problem.

Ok so I've made an attempt at updating this with the PrivateTempStore and flagging each ValidationSendPlugin as 'ready to send' upon login.

I'm still unsure as to how best to handle the creation of a re-send button. I feel that this should be consistent and therefore generated by a single method, however, I think it's something that we really should be able to override within a given validation plugin.

The obvious way to do this would be to have the base implementation defined in the a validation plugin base class, however, there doesn't seem to be a specific base class for each plugin which seems strange. Perhaps worth considering why all these plugins share a single base class, and perhaps whether there is another way for each plugin to share common logic like code generation etc.

Oops, I don't know why that last patch ignored some of the test files. Will make sure to get them back in the next patch.

Attached is a bug fix for the previous patch that fixes errors with the PrivateTempStore dependency.

Re-rolled and added some fixes + added back the removed tests.

Fixing private temp store issue among other fixes.

Rerolled patch #37 by lattest module version.

#38 reroll was missing changes to the BasicOverview form

Re-roll against the latest dev.

The #42 patch failed to apply, provided a re-roll for the same.

Let's try to re-roll again.

Now with some bytes in the patch file.

This now seems to be a task for the 2.x branch which will use a single plugin type with different interfaces to distinguish the personalities. And looking at the proposed changes here I'm not even sure if there really should be a 'send' plugin, as it seems to be just a validation plugin with the 'begin' method.