The "tfa_trusted_browser" plugin does not exist.

Created on 21 March 2024, over 1 year ago

Problem/Motivation

When I edit a user (as root), and go to "TFA", I get the following message:

Drupal\Component\Plugin\Exception\PluginNotFoundException: The "tfa_trusted_browser" plugin does not exist. Valid plugin IDs for Drupal\tfa\TfaValidationPluginManager are: tfa_recovery_code, tfa_hotp, tfa_totp in Drupal\tfa\TfaValidationPluginManager->doGetDefinition() (line 53 of core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php).

Steps to reproduce

  • Go to a user with tfa enabled
  • Click on the tfa-tab
๐Ÿ› Bug report
Status

Active

Version

1.5

Component

Code

Created by

๐Ÿ‡ณ๐Ÿ‡ฑNetherlands joshahubbers

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @joshahubbers
  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands joshahubbers
  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands joshahubbers
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

    A couple quick notes, not near a lab at the moment to check this:
    We probably want to validate that itโ€™s a validation plug-in class rather than excluding by plug-in name.

    Iโ€™m curious why the method is being set to a login plugin in the first place. Do we have a lower root fault that this would be masking?

    As an aside:
    The TFA project has moved fully to GitlabCi for the D8+ release branches. Due to this change we can only work with MRโ€™s as patch files can no longer be tested. This will become more common among contrib projects as DrupalCi is fast approaching a full shutdown. MRโ€™s are now recommended to be used whenever possible across the entire D.O. ecosystem.

  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands joshahubbers

    I totally agree with you that this was not the way to go. I attach a new patch and will open a MR for it too (we only use patches in our CI because the diff-contents of a mr can change without notice).

    Also we should use dependency injection for the plugin manager, but that is beyond the scope of this bug.

  • Status changed to Needs review over 1 year ago
  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands joshahubbers
  • Status changed to Needs work over 1 year ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

    I have only been able to reproduce this by Navigating to a user with TFA(at least the trusted browser plugin), clicking on the TFA tab, and than clicking on "Configure trusted browsers"

    Is that true for your scenario? If so the question regarding method being set from #4 would make sense, otherwise the method should not be present on the TFA page itself.

    It appears this stems from implementing #3075304: Users' recovery codes exposed to admin users โ†’ which implies we will need to ensure we evaluate the security implications of how we solve this.

  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands joshahubbers

    Hm, I get the error as soon as I click on the "TFA"-tab of a user with TFA enabled, and setup (TOTP). We do have patch #34 of this issue โœจ Force user to setup TFA when required and there are no remaining skips Needs work applied.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

    I get the error as soon as I click on the "TFA"-tab of a user with TFA enabled, and setup (TOTP). We do have patch #34 of this issue #3223327: Force user to setup TFA when required and there are no remaining skips applied.

    That should be the 'tfa.overview' page, which does not have the method named parameter (it will return NULL when called) see
    https://git.drupalcode.org/project/tfa/-/blob/b1ab6d112fff674db2d1b660c3....

    Can you try without the patch to confirm if the issue still exists or not, and validate if your routing file has any changes?

  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands joshahubbers

    Hmm, I replayed the scenario in a clean d10 install, and I cannot reproduce it there. So we have to dig deeper in our setup and other authentication-related modules to find out what is causing this error.

  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands joshahubbers

    When debugging the accessSelfOrAdmin function when accessing /user/x/security/tfa (tfa.overview), i notice that the error occurs on the route tfa.validation.setup. This is function also is the access check for this route, so it will be called when rendering the link to /user/x/security/tfa/tfa_trusted_browser.

    Now I come to your scenario mentioned above:

    I have only been able to reproduce this by Navigating to a user with TFA(at least the trusted browser plugin), clicking on the TFA tab, and than clicking on "Configure trusted browsers".

    Apperantly on a clean install the access-check for tfa.validation.setup is not called when rendering the link. But in our setup the access check is also called when rendering the link.

    So after this I think we can narrow the root problem down to the access check for the route tfa.validation.setup for login-plugins when $is_self is NOT true.

    So I think this patch is valid because this breaks:

    • access the tfa.validation.setup path
    • for another user, not yourself
    • for a login-plugin (currently only trusted_browser)

    The question I cannot answer is: why is there no access check on rendering the links in the clean d10 setup? Because this is the cause of also breaking the tfa.overview route in our setup.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

    Apperantly on a clean install the access-check for tfa.validation.setup is not called when rendering the link. But in our setup the access check is also called when rendering the link, not only at accessing the link.

    OK, at least we have a matching proof at this point.

    So I think this patch is valid because this breaks:

    This is going to get into the architectural side now. While I do believe this was a bug from #3075304: Users' recovery codes exposed to admin users โ†’ I'm not sure if we should allow Login plugins to be maintained by admins in 8.x-1.x or not. This has acted as an unofficial prohibition against admins viewing/editing login plugins since at least the 8.x-1.0 stable release. We may need to make 1.x always deny for login plugins.

    Opened ๐Ÿ“Œ Evaluate if allowUserSetupAccess should be on API or removed Active to deal with the 2.x issue of how that method should be treated going forward.

  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands joshahubbers
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia bhanu951

    I too am also able to reproduce this by Navigating to a user with TFA (at least the trusted browser plugin), clicking on the TFA tab, and than clicking on "Configure trusted browsers" as mentioned in #7 ๐Ÿ› The "tfa_trusted_browser" plugin does not exist. Needs work

  • @pcambra opened merge request.
  • ๐Ÿ‡ช๐Ÿ‡ธSpain pcambra Asturies

    I can confirm this bug on 8.x-1.12 on user/{id}/security/tfa/tfa_trusted_browser

    Drupal\Component\Plugin\Exception\PluginNotFoundException: The "tfa_trusted_browser" plugin does not exist. Valid plugin IDs for Drupal\tfa\TfaValidationPluginManager are: tfa_hotp, tfa_recovery_code, tfa_totp in Drupal\tfa\TfaValidationPluginManager->doGetDefinition() (line 53 of core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php). 
    

    I'm seeing that the other plugins have unit tests for them but tfa_trusted_browser seems to be missing.

    1. tfa_hotp: TfaHotpTest.php
    2. tfa_recovery_code: TfaRecoveryCodeTest.php
    3. tfa_totp: TfaTotpTest.php

    I've created the MR from the available branch and I can confirm it fixes or at least works around the issue.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

    See #12
    There is still concern for the 8.x-1.x branch that the solution should not allow admins access due to the possibility that we may grant access to an area that should be prohibited. A strong argument needs to be provided to refute this if a change allowing access to login plugins is to be accepted for 1.x, otherwise it needs to be done only in 2.x where plugin API rules can change (which necessitates the need to answer the related questions of API changes).

    As specific example, based on visual review, it appears this change may allow an admin to register their browser as trusted to a user account creating a method for them to silently bypass TFA.

    For now if we want a quick 1.x fix we should likely look at always denying access to login plugins setup unless it is for the current user.

    SA-CONTRIB-2025-085 is somewhat related, as it involved flaws in the expectations of allowUserSetupAccess() causing security concerns.

Production build 0.71.5 2024