One setup step remaining, two QR Code Scans required

Updated issue with reproduction steps.

It appears this only occurs when setting up a plugin that is not the 'default' plugin type.

Need to review code to determine what the original intended logic was before evaluating resolutions.

I came across this issue while using multiple plugin types. I can confirm that this only occurs for the plugin types that are not the default plugin type.
The user does not get redirected to the overview but instead also has a "Skip and finish" button. When the user clicks on that button, the status message: "Setup of TFA Email one-time password (EOTP) canceled." appears, which suggests that the setup did not happen but it did.

I removed most of the tfaNextSetupStep() function in a patch, which solved this problem for me. I could not find any further issues without the removed code.

Hi!

I found out that the form plugin was not updating when the form was rebuilding.
I've created a little patch that updates the plugin before the form renders again.

This bug occurred for me while setting up the TFA TOTP validation plugin which I have set as the default plugin. I have no other validation plugins enabled but I do have the TFA Trusted Browser login plugin enabled.

I can confirm the patch provided in #4 🐛 One setup step remaining, two QR Code Scans required Active fixes the issue. Thanks for the fix @tmiguelv.

D.O. has migrated away from patch files.

In order for tests to run we require all submissions to be in the form of a Merge Request.

More details may be found at: https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr... →

Quick glancing:
I suspect there will be a PHPCS warning on the comment in the patch file,
I haven’t analyzed the logic flow.

Same here as for tame4tex. Drupal 10.3.13 and tfa module 1.10.0. I had the TOTP method set as default, with a second method enabled. When I tried to set up the TOTP for a user, the code scan was requested twice. This stopped with the patch.

I created an issue fork and committed the patch from #4 by tmiguelv. I fixed the comment to adhere to Drupal guidelines. For me, this works. but I also didn't analyze the logic flow in detail.

I see there are some functional tests already. Why aren't they picking up this behavior? With the patch applied, the second step of the form was setting up the trusted browsers. `TfaTotpSetupPluginTest` probably doesn't use this feature, so it is not a multi-step form. I think, `testPluginSetup` should be copied to a `testPluginSetupMultiStep`. Or should we add a new test for the trusted browser feature?

Quick glance, it appears the tests run with just a single plugin, they would not hit this condition. which is reasonable, the tests as written are plugin specific and should not be testing the rest of the general usage.

Ideally yes this would be a dedicated test that performs a mulit-step setup to validate the multi step form. Ideally built off the test plugins and not the production plugins.

Can also confirm this issue, with TFA 1.10

Validation Plugin: TFA TOTP

1. Add new user
2. Login with new user
3. Prompted to setup TFA
4. Enter password
5. Given QR code, scan the code (Google Auth app)
6. Add application verified code
7. Verify and Save
8. Form reloads, with a new app code, (assumedly new QR code)
9. Scan the QR code again (Google Auth app)
10. Add application verified code
11. Save

Seems to work properly the second time.

This also only seems to happen for a brand new user.... if I 'Reset application' I don't need to do the second QR code scan,

Patch from #4 does seem to fix this (thanks) - I believe it also fixes some other issues I was seeing, IE, the 'Skip and finish' element (I believe) was displaying on the wrong step. It now sits on the 'Trust browser' step which feels proper.

Exact same situation as comment 5 - can confirm also that patch in comment 4 worked for us. Thank you.

It's a bit confusing to see a both the 'One setup step remaining' and the 'TFA setup complete' message, but at least this avoids the 'loop' of presenting users with the QR step again! Perhaps the 'One setup step remaining' could be clearer, explicitly saying what the remaining step is? Could it even mention that it's optional (at least for the trusted browsers plugin, since that provides the 'Skip and finish' button) ? For example, to come out something like this (with 'otherwise' added to the last line too):

✅ Application code verified. One setup step remaining, which can be skipped: TFA Trusted Browser.

TFA setup otherwise complete.

I've opened an actual merge request from the existing issue fork from comment #9 that had the patch from #4 plus its minor comment fix. I've then added some changes to implement my suggested message tweak; what do we think of this?

I've also merged in the latest 8.x-1.x branch code to resolve the existing linting warnings.

Leaving as 'needs work' though, for a dedicated test that performs a mulit-step setup to validate the multi step form, as requested in comment #10.

(Here's a screenshot of how the messages could now look, with these changes.)

Just a thought; does this really need blocking on additional tests for multistep forms? The multistep forms themselves have existed before this ticket, so although tests are certainly needed for them, could that be left out of scope of this particular tweak to them?

Linking 🐛 Full Setup not working on 8.x-1.7 Active as it proposed similar for the core portion of the patch.

does this really need blocking on additional tests for multistep forms?

I will note I would not be expecting a test to test everything, only that we end up up without being redirected back to the same plugin, essentially only needing to explicitly test this particular (that it doesn't redirect back to the same setup page).

I will give it a a bit more thought before giving a final decision on this, there is part of me that does want to cleanup bugs, and wishes to avoid excessive burden, there is also a part of me that sees this as one of very few faults that can arise from multi-step forms making it relevant as a 'first start to a test'

🐛 Redirect correctly after first time plugin setup Needs review seems relevant, once the code hits

 $this->messenger()->addStatus($this->t('TFA setup complete.'));

we should not end up on the setup page for another module. We may not need the wording change, although that may indicate there is still something else at play here that we should not be reaching that far into the code execution.

Looking at code and operations and descriptions, this is actually a duplicate of 🐛 Full Setup not working on 8.x-1.7 Active .

I apparently did not realize this back in October in order to consolidate them at that time. My apologies for any time that was spent discovering the same solutions that were previously discovered.

Looking at the patch in 🐛 Full Setup not working on 8.x-1.7 Active I believe it is slightly cleaner. It places the plugin exists validation in a single location while the proposal here does not currently check the plugin (I can see a possible edge case if the plugin is removed between the first step and the second step that the current proposed patch would possible cause an exception to rise to the user). I would suggest that version be adopted.

The wording changes I believe will indeed be fixed by 🐛 Redirect correctly after first time plugin setup Needs review and can be de-scoped from the issue.

Regarding tests:
As I look at this issue deeper and realize how much is indeed at play in the multi-step forms processing I agree that would be an undue burden to place on this issue and am willing to deffer the tests to a followup issue.

We can solve this in either issue as such leaving both open for now and will close whichever is not used and migrate any credit over to the solution issue.