Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Reauthenticate components to handle generic MFA challenges. #49680

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

Joerger
Copy link
Contributor

@Joerger Joerger commented Dec 3, 2024

The overarching change in this PR is the ability of all Reauthenticate flows to handle generic MFA challenges (sso, webauthn, otp) rather than handling each type differently or not at all.

Note: useReauthenticate should now be the goto state handler method for reauthentication flows.

Changes:

  • Refactor useReauthenticate (used in the Reauthenticate components).
    • Return a submitWithMFA method instead of submitWithWebAuthn and submitWithOTP. SSO MFA will use submitWithMFA('sso').
    • Deprecate OnAuthenticated prop
    • Hold MFA challenge in state - prevents the current behavior of retrieving a new MFA challenge every time you open the component or switch between available options. We only need a fresh MFA challenge after the current challenge has been used up.
    • Use update MFA Options from WebUI MFA types refactor #49678 to derive reauth and register options from MFA challenge ^
  • Update change/add device wizards according to useReauthenticate refactors.
  • Use useReauthenticate for change password wizard to get the same changes described above.
  • Update createPrivilegeToken endpoint to accept generic MFA response instead of TOTP or Webauthn, so it will now support SSO MFA.

Note on /e: This PR is dependent on the e PR and vice versa. Therefore I'll need to merge this PR with e PR as the head of /e. Then I'll make a follow up to update the e ref to master once the e PR is merged, shortly after this PR is merged.

TODO (follow ups):

  • (Optional) Instead of getting a privilege token before adding/deleting a device, just use the mfa response directly.
    • This is a piece of work which has been forgotten after being made possible with Let authenticated users issue register challenges #32271
    • Note to self: we will still need to get a privilege token if the user clicks the otp register option since it would cash in the mfa response for the qr code, and as a result the user would need to reauthenticate to switch to webauthn. Also failure/cancel on the last step may need to go back to initial reauth step.

Prerequisite for supporting SSO MFA in device management (add/delete), connection tester (discover), change password, and account recovery flows.

Depends on #49678, #49679, https://github.com/gravitational/teleport.e/pull/5656

@Joerger Joerger force-pushed the joerger/unify-mfa-methods branch from 0d9623a to 518f3ef Compare December 3, 2024 20:59
@Joerger Joerger force-pushed the joerger/refactor-reauthenticate branch from 9d5b285 to b4e63c1 Compare December 3, 2024 20:59
@Joerger Joerger force-pushed the joerger/unify-mfa-methods branch 3 times, most recently from e2afc8c to e53c97c Compare December 3, 2024 23:40
@Joerger Joerger mentioned this pull request Dec 4, 2024
@Joerger Joerger force-pushed the joerger/unify-mfa-methods branch from e53c97c to a36af16 Compare December 4, 2024 20:46
@Joerger Joerger force-pushed the joerger/refactor-reauthenticate branch from b4e63c1 to 5eca740 Compare December 4, 2024 21:09
@Joerger Joerger marked this pull request as ready for review December 4, 2024 21:09
@Joerger Joerger requested a review from avatus December 4, 2024 21:09
@Joerger Joerger force-pushed the joerger/unify-mfa-methods branch from a36af16 to 037f575 Compare December 4, 2024 21:27
@Joerger Joerger force-pushed the joerger/refactor-reauthenticate branch 4 times, most recently from 4abf873 to 42ba430 Compare December 6, 2024 00:11
@Joerger Joerger added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v15 backport/branch/v16 backport/branch/v17 and removed backport/branch/v15 labels Dec 6, 2024
@Joerger Joerger force-pushed the joerger/refactor-reauthenticate branch from ea8e8eb to 40336ed Compare December 9, 2024 22:26
@Joerger Joerger force-pushed the joerger/unify-mfa-methods branch from ad04916 to 831528d Compare December 9, 2024 22:29
Base automatically changed from joerger/unify-mfa-methods to master December 10, 2024 00:51
@Joerger Joerger force-pushed the joerger/refactor-reauthenticate branch 2 times, most recently from af41a4e to bdd69ca Compare December 10, 2024 01:04
@Joerger
Copy link
Contributor Author

Joerger commented Dec 10, 2024

@ryanclark @avatus I added one more commit if you don't mind taking a look e60ec3d

@Joerger Joerger force-pushed the joerger/refactor-reauthenticate branch from e60ec3d to 428e745 Compare December 11, 2024 02:41
@Joerger Joerger force-pushed the joerger/refactor-reauthenticate branch from 428e745 to 8e2fb35 Compare December 14, 2024 00:59
@Joerger Joerger force-pushed the joerger/refactor-reauthenticate branch from 8e2fb35 to 9e8ad10 Compare December 14, 2024 01:18
@Joerger Joerger force-pushed the joerger/refactor-reauthenticate branch from 828bded to 5d0f08d Compare December 14, 2024 01:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v17 no-changelog Indicates that a PR does not require a changelog entry size/sm ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants