-
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
Use Web MFA dialog for admin actions #50373
base: joerger/fix-useMfa-error-state
Are you sure you want to change the base?
Changes from all commits
8cf3345
f7a93e3
ae9b0de
821b778
900d875
d35da73
5c1e086
73fd28a
3dfc7bc
6b2d294
4d48924
656f425
aded158
b5581df
1024b38
9db6d2b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
import { createContext, PropsWithChildren, useCallback, useState } from 'react'; | ||
|
||
import AuthnDialog from 'teleport/components/AuthnDialog'; | ||
import { useMfa } from 'teleport/lib/useMfa'; | ||
import api from 'teleport/services/api/api'; | ||
import { CreateAuthenticateChallengeRequest } from 'teleport/services/auth'; | ||
import auth from 'teleport/services/auth/auth'; | ||
import { MfaChallengeResponse } from 'teleport/services/mfa'; | ||
|
||
export interface MfaContextValue { | ||
getMfaChallengeResponse( | ||
req: CreateAuthenticateChallengeRequest | ||
): Promise<MfaChallengeResponse>; | ||
} | ||
|
||
export const MfaContext = createContext<MfaContextValue>(null); | ||
|
||
/** | ||
* Provides a global MFA context to handle MFA prompts for methods outside | ||
* of the React scope, such as admin action API calls in auth.ts or api.ts. | ||
* This is intended as a workaround for such cases, and should not be used | ||
* for methods with access to the React scope. Use useMfa directly instead. | ||
*/ | ||
export const MfaContextProvider = ({ children }: PropsWithChildren) => { | ||
const adminMfa = useMfa({}); | ||
|
||
const getMfaChallengeResponse = useCallback( | ||
async (req: CreateAuthenticateChallengeRequest) => { | ||
const chal = await auth.getMfaChallenge(req); | ||
|
||
const res = await adminMfa.getChallengeResponse(chal); | ||
if (!res) { | ||
return {}; // return an empty challenge to prevent mfa retry. | ||
} | ||
|
||
return res; | ||
}, | ||
[adminMfa] | ||
); | ||
|
||
const [mfaCtx] = useState<MfaContextValue>(() => { | ||
const mfaCtx = { getMfaChallengeResponse }; | ||
auth.setMfaContext(mfaCtx); | ||
api.setMfaContext(mfaCtx); | ||
return mfaCtx; | ||
}); | ||
|
||
return ( | ||
<MfaContext.Provider value={mfaCtx}> | ||
<AuthnDialog mfaState={adminMfa}></AuthnDialog> | ||
{children} | ||
</MfaContext.Provider> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ | |
*/ | ||
|
||
import cfg from 'teleport/config'; | ||
import { MfaContextValue } from 'teleport/MFAContext/MFAContext'; | ||
import api from 'teleport/services/api'; | ||
import { | ||
DeviceType, | ||
|
@@ -44,7 +45,13 @@ import { | |
UserCredentials, | ||
} from './types'; | ||
|
||
let mfaContext: MfaContextValue; | ||
|
||
const auth = { | ||
setMfaContext(mfa: MfaContextValue) { | ||
mfaContext = mfa; | ||
}, | ||
|
||
checkWebauthnSupport() { | ||
if (window.PublicKeyCredential) { | ||
return Promise.resolve(); | ||
|
@@ -277,24 +284,9 @@ const auth = { | |
// If is_mfa_required_req is provided and it is found that MFA is not required, returns null instead. | ||
async getMfaChallengeResponse( | ||
challenge: MfaAuthenticateChallenge, | ||
mfaType?: DeviceType, | ||
mfaType: DeviceType, | ||
totpCode?: string | ||
): Promise<MfaChallengeResponse | undefined> { | ||
if (!challenge) return; | ||
|
||
// TODO(Joerger): If mfaType is not provided by a parent component, use some global context | ||
// to display a component, similar to the one used in useMfa. For now we just default to | ||
// whichever method we can succeed with first. | ||
if (!mfaType) { | ||
if (totpCode) { | ||
mfaType = 'totp'; | ||
} else if (challenge.webauthnPublicKey) { | ||
mfaType = 'webauthn'; | ||
} else if (challenge.ssoChallenge) { | ||
mfaType = 'sso'; | ||
} | ||
} | ||
|
||
if (mfaType === 'webauthn') { | ||
return auth.getWebAuthnChallengeResponse(challenge.webauthnPublicKey); | ||
} | ||
|
@@ -389,7 +381,7 @@ const auth = { | |
createPrivilegeTokenWithWebauthn() { | ||
return auth | ||
.getMfaChallenge({ scope: MfaChallengeScope.MANAGE_DEVICES }) | ||
.then(auth.getMfaChallengeResponse) | ||
.then(chal => auth.getMfaChallengeResponse(chal, 'webauthn')) | ||
.then(mfaResp => auth.createPrivilegeToken(mfaResp)); | ||
}, | ||
|
||
|
@@ -442,27 +434,36 @@ const auth = { | |
.then(res => res?.webauthn_response); | ||
}, | ||
|
||
getMfaChallengeResponseForAdminAction(allowReuse?: boolean) { | ||
// If the client is checking if MFA is required for an admin action, | ||
// but we know admin action MFA is not enforced, return early. | ||
if (!cfg.isAdminActionMfaEnforced()) { | ||
return; | ||
getAdminActionMfaResponse(allowReuse?: boolean) { | ||
// mfaContext is set once the react-scoped MFA Context Provider is initialized. | ||
// Since this is a global object outside of the react scope, there is a marginal | ||
// chance for a race condition here (the react scope should generally be initialized | ||
// before this has a chance of being called). This conditional is not expected to | ||
// be hit, but will catch any major issues that could arise from this solution. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm. This is quite a hack. The fact that auth service started to depend on an MFA context that is injected on the top level by context provider makes me think that it should be turned into a class (just like the MFA service) that is instantiated by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does sound like a big refactor to me, but I'm not really in a position with my level of typescript/React experience to judge or carry out this type of refactor. I also have some other important work to move onto. Do you think it's something we can live with today and fix tomorrow, or do we need to scrap this approach for now? Note that without this change, we still support SSO MFA for admin actions, we just don't provide the user with the choice between SSO MFA and Webauthn when applicable. Instead we automatically open the Webauthn or SSO MFA pop up without a dialog, which may be jarring, but not the end of the world. In the long term we should find a solution. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Joerger I think that given above, it would be best to continue with the PR as is, and treat this part as a technical debt that we take to improve UX. Can you please create a tracking issue for this refactoring? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
if (!mfaContext) { | ||
throw new Error( | ||
'Failed to set up MFA prompt for admin action. Please try refreshing the page to try again. If the issue persists, contact support as this is likely a bug.' | ||
); | ||
} | ||
|
||
return auth | ||
.getMfaChallenge({ | ||
try { | ||
return mfaContext.getMfaChallengeResponse({ | ||
scope: MfaChallengeScope.ADMIN_ACTION, | ||
allowReuse: allowReuse, | ||
allowReuse, | ||
isMfaRequiredRequest: { | ||
admin_action: {}, | ||
}, | ||
}) | ||
.then(auth.getMfaChallengeResponse); | ||
}); | ||
} catch { | ||
throw new Error( | ||
'This is an admin-level API request and requires MFA verification. Please try again with a registered MFA device. If you do not have an MFA device registered, you can add one in the account settings page.' | ||
); | ||
} | ||
}, | ||
|
||
// TODO(Joerger): Delete in favor of getMfaChallengeResponseForAdminAction once /e is updated. | ||
// TODO(Joerger): Delete in favor of getAdminActionMfaResponse once /e is updated. | ||
getWebauthnResponseForAdminAction(allowReuse?: boolean) { | ||
return auth.getMfaChallengeResponseForAdminAction(allowReuse); | ||
return auth.getAdminActionMfaResponse(allowReuse); | ||
}, | ||
}; | ||
|
||
|
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.
Should this unset the mfaContext in auth/api on unmount? (I'm not sure if that matters)
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 not sure how unmounting in React works, how would I go about this?
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.
@Joerger To execute code on unmount, you can add an
useEffect
that will only execute once and return a cleanup function. The callback will be executed when component is mounted, and then the returned cleanup function will be executed when it's being unmounted. Example:The cleanup function is executed by React every time when the
useEffect
dependencies change, just before the next effect callback is called or when component is unmounted. This is typically used to cancel network requests, making sure that no excess bandwidth is consumed, but most of all to protect out-of-order responses to force the component into an unexpected state. But here, since there are no dependencies, the cleanup function will only be executed once.(Also note that the cleanup function is the very reason why you can't pass an async function directly as an
useEffect
callback, and you'd have to wrap it into a synchronous one. React would treat the returned promise object as a function and try to call it, with rather predictable effect.)