Tfa handles plugin instantiation which allows for simple object setup but increases complexity when integrating with Drupal and dealing with context and plugin-specific dependencies. It would be simpler if plugins were instantiated during context setup and injected into the Tfa object.

TFA module will need to manage global configuration and handling fallbacks and can also instantiate default objects if not already done so by an alter hook.

An example of the current difficulties, TFA Basic's SMS plugin stores the mobile number as an account field. To use the number for sending the SMS theplugin can 1) directly query for the account field and its value in the object, 2) call a procedural function in tfa_basic that returns the number, or 3) have the number passed in as plugin context during instantiation. Option 3 is simpler, so TFA module needs to support a TFA Basic context alter doing SMS plugin instantiation.

Comments

coltrane’s picture

Status: Active » Needs work
StatusFileSize
new10.59 KB

Here's a in-progress patch for this. Tests are failing for some unclear reasons. Needs work.

coltrane’s picture

Status: Needs work » Needs review
StatusFileSize
new10.16 KB

Got it working.

coltrane’s picture

Note for those implementing custom plugins with TFA: this change affects Tfa and TfaSetup instantiation. No longer do you pass in a class name, you pass in the object itself.

Before:

$context = array('uid' => '9');
$plugins = array(
  'validate' => 'MyTfaValidatePlugin',
  'fallback' => array(
    'MyTfaFallbackPlugin'
  )
);   

$tfa = new Tfa($plugins, $context);

After:

$context = array('uid' => '9');
$validate = new MyTfaValidatePlugin($context);
$plugins = array(
  'MyTfaFallbackPlugin' => new MyTfaFallbackPlugin($context);
);

$tfa = new Tfa($validate, $plugins, $context);

coltrane’s picture

#2 accomplishes the goal but doesn't completely handle the example use case. Perhaps plugin modules should instantiate objects themselves so that they have control over constructor arguments.

greggles’s picture

I didn't test this, but the changes look logical and seem like they'll make this easier to test.

coltrane’s picture

StatusFileSize
new14.44 KB

This patch changes tfa_get_process() from the approach of #2 to calling new tfa_get_plugin() function that will return a plugin object. tfa_get_plugin() can refer to TFA API implementations for a 'callback' function or the plugin name suffixed with '_create'. This allows modules providing TFA plugins to control the instantiation and thereby control arguments and dependencies.

For example of this see tfa_test_login_create() in tfa_test.module after applying this patch.

  • coltrane committed be120db on 7.x-2.x
    Issue #2327441 by coltrane: Plugin injection
    
coltrane’s picture

Status: Needs review » Fixed

#6 committed.

### Usage and upgrade notes

Users of TFA Basic will see support for this via #2376633: Support new TFA context injection and in the next tagged release.

If you have custom TFA plugins you may not need to make any changes at all depending on your integration with the TFA process.

The only required changes are if you create the Tfa or TfaSetup objects yourself somehow (such as during plugin configuration or custom TFA flows). If so you'll need to change the arguments to these classes to be full plugin objects instead of plugin machine names.

Additionally, if you want to instantiate your plugins yourself (in order to manage dependencies) you can register a 'callback' function in your hook_tfa_api. This function will be called during TFA setup and expect to receive a TFA plugin object back.

Status: Fixed » Closed (fixed)

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