Problem/Motivation

The TfaTrustedBrowser plugin form is showing on the TFA screen even when disabled.

Steps to reproduce

Have empty login_plugins key in config, i.e:

 login_plugins: {  }
 login_plugin_settings:
   tfa_trusted_browser:
     cookie_allow_subdomains: true
     cookie_expiration: 30
     cookie_name: tfa-trusted-browser

Setup TFA for an account, login and see that the trusted browser checkbox is visible.

Proposed resolution

Only apply plugins that are explicitly enabled.

Remaining tasks

Code
Tests
Review
Commit

CommentFileSizeAuthor
#15 Screenshot 2022-12-09 at 4.51.46 PM.png136.97 KBankitv18
#10 interdiff-3319595-8-10.txt5.37 KBacbramley
#10 3319595-10.patch9.21 KBacbramley
#8 interdiff-3319595-5-8.txt544 bytesacbramley
#8 3319595-8.patch6.69 KBacbramley
#5 3319595-5.patch6.69 KBacbramley
#5 3319595-5-failing-test.patch5.84 KBacbramley
#3 3319595-3.patch3.82 KBacbramley
#3 3319595-3-failing-test.patch3 KBacbramley

Issue fork tfa-3319595

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

acbramley created an issue. See original summary.

acbramley’s picture

acbramley commented
Assigned: Unassigned » acbramley

Working on the fix

acbramley’s picture

acbramley commented
Status: Active » Needs review
StatusFileSize
new 3319595-3-failing-test.patch3 KB
new 3319595-3.patch3.82 KB

Here's the first part - skipping forms for plugins that aren't enabled. Second part is skipping the actual check for disabled plugins.

The last submitted patch, 3: 3319595-3-failing-test.patch, failed testing. View results

acbramley’s picture

acbramley commented
StatusFileSize
new 3319595-5-failing-test.patch5.84 KB
new 3319595-5.patch6.69 KB 
+++ b/src/Form/EntryForm.php
@@ -149,7 +149,13 @@ class EntryForm extends FormBase {
+      if ($login_plugins[$plugin_id] ?? NULL !== $plugin_id) {

Slight bug here - needed to wrap left side in brackets, fixed in new patch.

The last submitted patch, 5: 3319595-5-failing-test.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 5: 3319595-5.patch, failed testing. View results

acbramley’s picture

acbramley commented
Status: Needs work » Needs review
StatusFileSize
new 3319595-8.patch6.69 KB
new interdiff-3319595-5-8.txt544 bytes

Subdir fix.

jcnventura’s picture

jcnventura
Primary language English
commented
Status: Needs review » Needs work

It would be cleaner the move the active plugin check into the getLoginDefinitions code, and add a parameter to those functions that allows to get definitions of all plugins or only the acrive ones.

Ideally, the default would be to get only the active plugins, and the patch will need to change the TFA settings form to use the parameter to get also the inactive plugins.

This will prevent a few more bugs like this from showing up in the future.

acbramley’s picture

acbramley commented
Status: Needs work » Needs review
StatusFileSize
new 3319595-10.patch9.21 KB
new interdiff-3319595-8-10.txt5.37 KB

Refactored as per #9 also removed the config check from TfaOverviewForm

jcnventura’s picture

jcnventura
Primary language English
commented
Assigned: acbramley » Unassigned
martijn de wit’s picture

martijn de wit
Primary language Dutch
Location 🇳🇱 The Netherlands
commented

Got same issue where new features in 2.x branch are always enabled.

Patch applies good and works in our situation

+1 RTBC

ankitv18 made their first commit to this issue’s fork.

ankitv18 opened merge request !20

ankitv18’s picture

ankitv18 commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new Screenshot 2022-12-09 at 4.51.46 PM.png136.97 KB

Hi,
Patch#10 is cleanly applied with 2.x-dev branch
I also have opened a MR!20 wrt patch#10, added empty line at the end in tfa.services.yml

+1 RTBC

thatguy’s picture

thatguy commented

The patch installs cleanly but I get the error below when I go to the module configuration page at /admin/config/people/tfa

"ArgumentCountError: Too few arguments to function Drupal\tfa\TfaPluginManager::__construct(), 3 passed in /app/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 262 and exactly 4 expected in Drupal\tfa\TfaPluginManager->__construct() (line 35 of modules/contrib/tfa/src/TfaPluginManager.php). "

jcnventura’s picture

jcnventura
Primary language English
commented

@thatguy, you need to clear caches after applying the patch to not have that error. Drupal is still using the previous definition of the tfa.services.yml file.

thatguy’s picture

thatguy commented

Yep cache clear fixed it, should already maybe know to try it when something doesn't work :D

Patch seems to fix the issue!

  • acbramley authored 2f92b27d on 2.x 
    Issue #3319595 by acbramley, ankitv18, jcnventura, thatguy, Martijn de...
jcnventura’s picture

jcnventura
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

jcnventura closed merge request !20

Status: Fixed » Closed (fixed)

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