-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Add SSO option during PerSessionMFA in the web #47876
Conversation
This pull request is automatically being deployed by Amplify Hosting (learn more). |
This pull request is automatically being deployed by Amplify Hosting (learn more). |
...data.webauthn_response, | ||
...data, | ||
}; | ||
console.log({ data }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops. Will remove
<DialogHeader style={{ flexDirection: 'column' }}> | ||
<DialogTitle textAlign="center"> | ||
Multi-factor authentication | ||
</DialogTitle> | ||
</DialogHeader> | ||
<DialogContent mb={6}> | ||
{mfa.errorText && ( | ||
<Danger mt={2} width="100%"> | ||
<Danger data-testid="danger-alert" mt={2} width="100%"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, as a side note (not to be addressed in this particular PR): perhaps we should use an alert
ARIA role for warning and danger boxes. This would both help making these more accessible, and would help in writing tests. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that'd probably be more helpful
<ButtonSecondary onClick={onCancel}>Cancel</ButtonSecondary> | ||
<Flex textAlign="center" width="100%" flexDirection="column" gap={3}> | ||
{mfa.ssoChallenge && ( | ||
<ButtonSecondary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't we just reuse the entire ButtonSso
component here? If it's about size or different icon gap, I'd encourage unifying it instead of creating a slightly customized version everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forget what issue I had using the entire button SSO component but I'll revisit and try again, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh thats right, because the rest of the buttons around this dialog aren't sso. There is only 1 SSO challenge being retreives, and/or a webauthn challenge.
So its either use ButtonSso for the single sso option and also for two non-sso buttons (webauthn and totp. totp isnt in yet tho) or just remake a button in here for this dialog to match. And I'd rather these 3 buttons in this dialog all use the same button instead of ButtonSso for one and a "copy" of its styles for the non sso buttons
</ButtonSecondary> | ||
)} | ||
{mfa.webauthnPublicKey && ( | ||
<ButtonSecondary onClick={mfa.onWebauthnAuthenticate} autoFocus block> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we have auto-focus here, why don't we make it a primary button?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can remove auto focus. We don't want it primary because there can be multiple options that exist with no real preference
import { | ||
MfaChallengeResponse, | ||
WebauthnAssertionResponse, | ||
} from 'teleport/services/auth'; | ||
|
||
class EventEmitterMfaSender extends EventEmitter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this class extend EventEmitter
if it doesn't emit anything? I'm a bit puzzled here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class gets extended by tty (and desktop client), which emits many things. This prior work seems to be more for typing (perhaps virtual/abstract methods instead but I'll check out the impact of that)
channel.removeEventListener('message', handleMessage); | ||
channel.close(); | ||
}; | ||
}, [state, emitterSender, state.ssoChallenge]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you depend on state
, every state change will also execute the effect, and depending on state.ssoChallenge
will be moot. Is this what you intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, we don't need state here, only the challenge and emitter
const channel = new BroadcastChannel(state.ssoChallenge.channelId); | ||
|
||
function handleMessage(e: MessageEvent<{ mfaToken: string }>) { | ||
if (!state.ssoChallenge) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if there's a different challenge? Also, I don't think this conditional is necessary at all; the ssoChallenge
change would trigger cancelling this effect, which leads to removing the listener and closing the channel. So perhaps either cache the ssoChallenge
locally and compare the state
field with the local copy (in the spirit of defensive programming), or just remove the conditional altogether, as we close the channel when the effect is cancelled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This flow is specific to the SSO challenge as it's awaiting a response from a separate window. The webauthn challenge/flow remain unchanged
} | ||
|
||
// open a broadcast channel if sso challenge exists so it can listen | ||
// for a confirmation response token |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm gonna need more details on how it's going to work. Do you have an RFD? I'm interested in who talks to that broadcast channel and where the ssoChallenge
(and channelId
) come from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The window pop up we use during the SSO ceremony is what creates and broadcasts the token from a broadcast channel from its confirmation page. Check out the e PR linked in this PRs description
@@ -146,6 +141,11 @@ export function makeWebauthnAssertionResponse(res): WebauthnAssertionResponse { | |||
}; | |||
} | |||
|
|||
export type SsoChallengeResponse = { | |||
requestId: string; | |||
token: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which party generates this token?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We generate it in the backend when asking for challenges, returned in the same response that a webauthn challenge from us would come from.
// but to be backward compatible, we need to still spread the existing webauthn only fields | ||
// as "top level" fields so old proxies can still respond to webauthn challenges. | ||
// in 19, we can just pass "data" without this extra step | ||
// TODO (avatus): DELETE IN 19 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming this makes it into v17, we can delete this in v18 instead. Same above and below.
} | ||
}, [emitterSender, onChallenge]); | ||
let ssoChallengeAbortController: AbortController | undefined; | ||
const chanllengeHandler = (challengeJson: string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const chanllengeHandler = (challengeJson: string) => { | |
const challengeHandler = (challengeJson: string) => { |
if (ssoChallenge) { | ||
ssoChallengeAbortController?.abort(); | ||
ssoChallengeAbortController = new AbortController(); | ||
void waitForSsoChallengeResponse( | ||
ssoChallenge, | ||
ssoChallengeAbortController.signal | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: It seems to me that onSsoAuthenticate
is a better place for this logic. We don't really need to setup this listener until the user actually clicks SSO and wants to go through with it. Could we move all this abortController
logic and waiting there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need the abortController logic here because we need a way to abort if the component unmounts (in the cleanup function of this useEffect).
We could do a separate useEffect but it'd be doing the same thing outside of onSsoAuthenticate so might as well keep it local to the single useEffect
} | ||
}; | ||
|
||
emitterSender?.on(TermEvent.WEBAUTHN_CHALLENGE, chanllengeHandler); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: We should rename this const to MFA
as I did on the backend.
emitterSender?.on(TermEvent.WEBAUTHN_CHALLENGE, chanllengeHandler); | |
emitterSender?.on(TermEvent.MFA_CHALLENGE, chanllengeHandler); |
This PR adds SSO as a valid option for Per Session MFA dialogs.
The dialog design is not final but is a stepping stone until the dialog is entirely redesigned. The SSO buttons are generated the same way as Login, so the icon and display name match the connector sending the challenge (here, mine is Okta).
Depends on https://github.com/gravitational/teleport.e/pull/5229 and #47832