Fix Per-session MFA for desktops #50793
Conversation
| @@ -172,7 +172,7 @@ export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { | |||
| }; | |||
| } | |||
|
|
|||
| export function useMfaTty(emitterSender: EventEmitterMfaSender): MfaState { | |||
| export function useMfaEmitter(emitterSender: EventEmitterMfaSender): MfaState { | |||
There was a problem hiding this comment.
Something still feels off about this.
Are you sure desktop code should be calling this? AFAIK, the TermEvent enum is a bunch of events that our web SSH console depends on. I wouldn't expect the desktop player to depend on any term events since it is not a terminal.
If your intent is some refactoring so that SSH and desktop MFA share more code I think that's a fine goal, but I think the abstraction is probably not quite right. I don't think desktop code should depend on TermEvent.
There was a problem hiding this comment.
I didn't make any significant refactor to the Desktop session MFA code, it was already using useMfa, which took Tty or EventEmitterMfaSender as an argument (tdpClient implements EventEmitterMfaSender). I renamed it to useMfaTty or useMfaEmitter so that I could make useMfa more generally usable.
EventEmitterMfaSender just represents the websocket used in WebUI sessions to send MFA challenges from the proxy and retrieve a challenge response from the client. Tty extends that for Terminal purposes. I can take a stab at renaming to make this more clear.
There was a problem hiding this comment.
@avatus Can you help provide any more clarification? It is a dense section of the code base.
There was a problem hiding this comment.
I think this is a result of poor naming/abstractions from the past. desktop and ssh docs both implemented the old EventWebAuthnEmitter or whatever it was called, which had a single non-implemented sendWebauthn method that tty and tdpClient had to implement themselves. And we've since updated it from useWebauthn to useMfa but that weird abstraction layer should probably just go imo, we don't need it. Outside of this PR probably
Joerger
added
the
no-changelog
|
@zmb3 @gzdunek @ryanclark Friendly reminder to review, this should go into v17.2 |
public-teleport-github-review-bot
bot
removed the request for review
from gzdunek
* Add sendChallengeResponse implementation for desktop sessions. * Rename useMfaTty to useMfaEmitter.
* Add sendChallengeResponse implementation for desktop sessions. * Rename useMfaTty to useMfaEmitter.
* Add sendChallengeResponse implementation for desktop sessions. * Rename useMfaTty to useMfaEmitter.
* Add sendChallengeResponse implementation for desktop sessions. * Rename useMfaTty to useMfaEmitter.
* Add SSO MFA prompt for WebUI MFA flows (#49794) * Include sso channel ID in web mfa challenges. * Handle SSO MFA challenges. * Handle sso response in backend. * Handle non-webauthn mfa response for file transfer, admin actions, and app session. * Simplify useMfa with new helpers. * Fix lint. * Use AuthnDialog for file transfers; Fix json backend logic for file transfers. * Make useMfa and AuthnDialog more reusable and error proof. * Use AuthnDialog for App sessions. * Resolve comments. * Fix broken app launcher; improve mfaRequired logic in useMfa. * Fix AuthnDialog test. * Fix merge conflict with Db web access. * fix stories. * Refactor mfa required logic. * Address bl-nero's comments. * Address Ryan's comments. * Add useMfa unit test. * Fix story lint. * Replace Promise.withResolvers for compatiblity with older browers; Fix bug where MFA couldn't be retried after a failed attempt; Add extra tests. * Fix Per-session MFA for desktops (#50793) * Add sendChallengeResponse implementation for desktop sessions. * Rename useMfaTty to useMfaEmitter. * Remove unused code. * Fix `useMfa` error handling (#50844) * Fix useMfa cancel logic to avoid duplicate error messages across dialog layers. * Add MfaCanceledError and use promise.reject for mfa cancel. * Address comments. * Fix test. * Remove outdated comment. * Add comment; Simplify who dialog logic. --------- Co-authored-by: Roman Tkachenko <[email protected]>
Restore
EventEmitterMfaSenderimplementation for desktop access mistakenly removed in #49794.This will be backported in #50529
Closes #50557