Skip to content

LG 8622 Removes stand alone error page for F/T Unlock (only WebAuthn Method)#8716

Merged
kevinsmaster5 merged 9 commits intomainfrom
kmas-lg-8622-f-t-unlock-do-not-prompt-again
Jul 7, 2023
Merged

LG 8622 Removes stand alone error page for F/T Unlock (only WebAuthn Method)#8716
kevinsmaster5 merged 9 commits intomainfrom
kmas-lg-8622-f-t-unlock-do-not-prompt-again

Conversation

@kevinsmaster5
Copy link
Contributor

🎫 Ticket

Link to the relevant ticket.
LG_8622

🛠 Summary of changes

Removes the control statement checking if multiple factors are enabled in handle_invalid_webauthn so in any case it will redirect to login_two_factor_webauthn_url rather than the stand alone error page.
Revised the Spec to account for that change.
Deleted the stand alone error page erb file.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

  • Create an account using a device that is able to support Face or Touch Unlock as MFA
  • Set up Face or Touch Unlock as the only MFA method
  • When the account is complete attempt to log in using a device that does not support Face or Touch Unlock
  • An error screen should appear with a button to Use face or touch unlock, a link to choose another authentication method, and a link to Cancel
  • It should not show a screen that says "We can't identify your device"

👀 Screenshots

Details FT Unlock can't identify your device Previous UI
Details Two MFA please try again Revised UI

@kevinsmaster5 kevinsmaster5 force-pushed the kmas-lg-8622-f-t-unlock-do-not-prompt-again branch from 99d4efc to bc4c13e Compare July 3, 2023 20:22
@kevinsmaster5 kevinsmaster5 marked this pull request as draft July 3, 2023 21:00
@kevinsmaster5 kevinsmaster5 force-pushed the kmas-lg-8622-f-t-unlock-do-not-prompt-again branch from 8ba4476 to 113bdfd Compare July 5, 2023 15:33
@kevinsmaster5 kevinsmaster5 marked this pull request as ready for review July 5, 2023 18:12
@aduth aduth requested a review from a team July 5, 2023 18:53
@jmdembe
Copy link
Contributor

jmdembe commented Jul 5, 2023

When I set up Face/Touch unlock as my only method on Chrome and then try to authenticate on the same machine, but on Firefox, I am getting a 404. Is this an expected behavior?

@kevinsmaster5
Copy link
Contributor Author

When I set up Face/Touch unlock as my only method on Chrome and then try to authenticate on the same machine, but on Firefox, I am getting a 404. Is this an expected behavior?

No it shouldn't go to a not found page. What path is showing when it 404s?

@jmdembe
Copy link
Contributor

jmdembe commented Jul 5, 2023

When I set up Face/Touch unlock as my only method on Chrome and then try to authenticate on the same machine, but on Firefox, I am getting a 404. Is this an expected behavior?

No it shouldn't go to a not found page. What path is showing when it 404s?

It appears on the webauthn_error path
404 error page on the webauthn error path

@kevinsmaster5
Copy link
Contributor Author

Thank you @jmdembe I'll try to see why it's trying to get that path in Firefox.

@kevinsmaster5 kevinsmaster5 marked this pull request as draft July 5, 2023 21:14
@aduth
Copy link
Contributor

aduth commented Jul 6, 2023

I think that error page might be related to what I'm working on in LG-10177 / #8723, where we redirect the user to the error page if they're signing in and a platform authenticator is not available on the device (specifically, this code).

We probably want to keep that error page around as long as it's used, which will be much less between this ticket and mine. I expect it'd continue to be used for a scenario where someone tries logging in from a device which doesn't support WebAuthn at all (although maybe this falls outside our browser support commitments).

@kevinsmaster5 kevinsmaster5 force-pushed the kmas-lg-8622-f-t-unlock-do-not-prompt-again branch from 952eebe to f0c772b Compare July 6, 2023 19:37
@kevinsmaster5
Copy link
Contributor Author

@jmdembe and @aduth please take a look in particular how I solved the webauthn verification controller on line 30. I'm mostly concerned about inadvertently short circuiting legitimate errors.

Firefox no longer sends a 404 or put you on that dead end page anymore. It does tell you to Connect your security key which is confusing and not exactly what I would expect. Safari still works as before and Chrome where I have created the Touch authentication continues to work.

@kevinsmaster5 kevinsmaster5 marked this pull request as ready for review July 6, 2023 20:11
Comment on lines 31 to 38
flash[:error] = t(
'two_factor_authentication.webauthn_error.multiple_methods',
link: view_context.link_to(
t('two_factor_authentication.webauthn_error.additional_methods_link'),
login_two_factor_options_path,
),
)
redirect_to login_two_factor_webauthn_url(platform: params[:platform])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious, how do the code changes here relate to the ticket. I'm not sure I follow why we'd need them, and the tests pass without them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original code that was there

Suggested change
flash[:error] = t(
'two_factor_authentication.webauthn_error.multiple_methods',
link: view_context.link_to(
t('two_factor_authentication.webauthn_error.additional_methods_link'),
login_two_factor_options_path,
),
)
redirect_to login_two_factor_webauthn_url(platform: params[:platform])
def error; end

is what was redirecting Firefox to the dead end page.
For whatever reason Safari was working without the code change. I may be missing something elsewhere that would make the solution work without this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think keeping the error view to avoid the 404 with your changes in 8a44fb6 is enough for this ticket, and we can address the Firefox behavior as part of my work in #8723.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have taken out that change. Thank you.

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@kevinsmaster5 kevinsmaster5 merged commit 7d85822 into main Jul 7, 2023
@kevinsmaster5 kevinsmaster5 deleted the kmas-lg-8622-f-t-unlock-do-not-prompt-again branch July 7, 2023 13:57
This was referenced Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants