Currently when defining a Validation plugin the TFA module requires you to also write a Setup plugin. Additionally, the setup plugin must have a specific ID as implied by the TFA source.

For example in BasicOverview.php, the module attempts to load a Setup plugin for every Validation plugin with the following approach: $plugin . '_setup'. This pattern of a Validation's plugin ID concatenated with '_setup' is repeated a number of times to load the setup plugin.

One problem with this approach is that it forces every Validation plugin to implement a Setup plugin with the very specific ID. I believe a more flexible approach would be to be to make the Setup plugin ID an optional part of the Validation plugin's annotation.

For example:

/**
 * TOTP validation class for performing TOTP validation.
 *
 * @TfaValidation(
 *   id = "tfa_totp",
 *   label = @Translation("TFA Time-based OTP(TOTP)"),
 *   description = @Translation("TFA Totp Validation Plugin"),
 *   setupPluginId = "tfa_totp_setup",
 * )
 */

And I believe this annotation should be optional because I can imagine cases where a plugin does not require additional setup by the end user. I'm not saying I have a great example in mind, just that I can imagine someone else smarter than me coming up with valid reasons not to have a user-setup form for a plugin.

CommentFileSizeAuthor
#18 2925066-18.patch17.94 KBjcnventura
#16 2925066-16-interdiff.txt2.85 KBjonas139
#16 2925066-16.patch18.82 KBjonas139
#15 interdiff.txt13.23 KBdaggerhart
#15 2925066-15.patch18.12 KBdaggerhart
#14 2925066-14.patch10.9 KBdaggerhart
#13 interdiff-2925066-11-13.txt656 bytesscreon
#13 2925066-13.patch10.94 KBscreon
#11 interdiff-2925066-9-11.txt603 bytesjeroent
#11 2925066-11.patch10.48 KBjeroent
#9 2925066-9.patch9.71 KBjeroent
#7 2925066-8.patch9.71 KBjeroent
#4 2925066-4-tfa-setup-plugin-id.patch11.14 KBdaggerhart

Comments

daggerhart created an issue. See original summary.

daggerhart’s picture

daggerhart commented
Title: Refactor implicitly named plugin type expectations to an annotation property » Refactor implicitly named plugin id expectations to an annotation property
daggerhart’s picture

daggerhart commented
Title: Refactor implicitly named plugin id expectations to an annotation property » Refactor implicitly named setup plugin id expectations to an annotation property
Issue summary: View changes
daggerhart’s picture

daggerhart commented
Related issues: +#2925282: Use annotation property to define a Validation plugin's Setup Plugin Id
StatusFileSize
new 2925066-4-tfa-setup-plugin-id.patch11.14 KB

Attach patch defines the new annotation property for Login, Send, and Validation plugins. It also refactors all previous usage of $plugin_id . '_setup' to checking for and using this new annotation property.

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
Status: Active » Needs review
jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
Assigned: Unassigned » jeroent
Status: Needs review » Needs work

Patch no longer applies.

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
Assigned: jeroent » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 2925066-8.patch9.71 KB

Created reroll.

Status: Needs review » Needs work

The last submitted patch, 7: 2925066-8.patch, failed testing. View results

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
Status: Needs work » Needs review
StatusFileSize
new 2925066-9.patch9.71 KB

Status: Needs review » Needs work

The last submitted patch, 9: 2925066-9.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
Status: Needs work » Needs review
StatusFileSize
new 2925066-11.patch10.48 KB
new interdiff-2925066-9-11.txt603 bytes

Fixed failing test. Added SetupPluginId in TfaTestValidationPlugin.

screon’s picture

screon commented
Status: Needs review » Needs work

Something breaks after applying the patch:

@@ -160,15 +202,15 @@ class BasicOverview extends FormBase {
    * @return array
    *   Render array
    */
-  protected function tfaPluginSetupFormOverview($plugin, $account, $enabled) {
+  protected function tfaPluginSetupFormOverview(array $plugin, $account, $enabled) {
     $params = [
       'enabled' => $enabled,
       'account' => $account,
-      'plugin_id' => $plugin,
+      'plugin_id' => $plugin['id'],
     ];
     $output = $this->tfaSetup
-                ->createInstance($plugin . '_setup', ['uid' => $account->id()])
-                ->getOverview($params);
+      ->createInstance($plugin['setupPluginId'], ['uid' => $account->id()])
+      ->getOverview($params);
     return $output;
   }

You are expecting an array but in the function calls in buildForm() the $plugin argument is still a string (coming from $configuration).

This results in the following error when browsing to /user/UID/security/tfa:
TypeError: Argument 1 passed to Drupal\tfa\Form\BasicOverview::tfaPluginSetupFormOverview() must be of the type array, string given, called in /var/www/bricks/modules/custom/tfa/src/Form/BasicOverview.php on line 149 in Drupal\tfa\Form\BasicOverview->tfaPluginSetupFormOverview() (regel 205 van /var/www/bricks/modules/custom/tfa/src/Form/BasicOverview.php)

screon’s picture

screon commented
Status: Needs work » Needs review
StatusFileSize
new 2925066-13.patch10.94 KB
new interdiff-2925066-11-13.txt656 bytes

Fixed the above mentioned issue.

daggerhart’s picture

daggerhart commented
StatusFileSize
new 2925066-14.patch10.9 KB

Patch seems to still apply, but with some fudging. Re-rolling for some additional review.

daggerhart’s picture

daggerhart commented
StatusFileSize
new 2925066-15.patch18.12 KB
new interdiff.txt13.23 KB

Here is a patch where I've added some backwards compatibility methods to our plugin managers. These methods basically just add the old implicit setupPluginId to any plugin that does not have one explicitly set. This should remove our need to coordinate this change with every existing contrib module that implement TFA plugins.

I've reviewed changes a bit and did a lot of manual testing. If someone else could give this a glance, I think we can get this committed without much risk.

jonas139’s picture

jonas139 commented
StatusFileSize
new 2925066-16.patch18.82 KB
new 2925066-16-interdiff.txt2.85 KB

I reviewed the patch and all seems correct.
I only made some small esthetic changes (typos and such) to the patch so for me it's good to go.

jcnventura’s picture

jcnventura
Primary language English
commented
Related issues: +#2938085: Define setup plugin id annotation as the TFA module api changes
jcnventura’s picture

jcnventura
Primary language English
commented
StatusFileSize
new 2925066-18.patch17.94 KB

Re-roll of #16. No interdiff as the patch no longer applies.

  • jcnventura authored 2fd6997 on 8.x-1.x 
    Issue #2925066 by daggerhart, JeroenT, screon, jonas139, jcnventura:...
jcnventura’s picture

jcnventura
Primary language English
commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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