- Issue created by @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
9 months ago 5:27am 22 March 2024 - Status changed to Needs work
9 months ago 6:48pm 22 March 2024 - ๐บ๐ธ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.
- ๐ฎ๐ณ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