Skip to content

Commit

Permalink
Initialize mfaRequired as false when determining the requirement thro…
Browse files Browse the repository at this point in the history
…ugh the event emitter.
  • Loading branch information
Joerger committed Feb 2, 2025
1 parent bed9f18 commit ce976bb
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 14 deletions.
3 changes: 3 additions & 0 deletions web/packages/teleport/src/Console/DocumentSsh/DocumentSsh.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,9 @@ function DocumentSsh({ doc, visible }: PropTypes) {
const [showSearch, setShowSearch] = useState(false);

const mfa = useMfaEmitter(tty, {
// The MFA requirement will be determined by whether we do/don't get
// an mfa challenge over the event emitter at session start.
isMfaRequired: false,
req: {
scope: MfaChallengeScope.USER_SESSION,
},
Expand Down
23 changes: 13 additions & 10 deletions web/packages/teleport/src/lib/useMfa.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -121,30 +121,33 @@ describe('useMfa', () => {
await waitFor(() => expect(mfa.current.mfaRequired).toEqual(false));
});

test('adaptable mfa requirement state', async () => {
test('adaptable mfa requirement', async () => {
jest.spyOn(auth, 'getMfaChallenge').mockResolvedValueOnce(mockChallenge);
jest
.spyOn(auth, 'getMfaChallengeResponse')
.mockResolvedValueOnce(mockResponse);

const { result: mfa } = renderHook(() =>
useMfa({
isMfaRequired: false,
req: mockChallengeReq,
})
);

// The mfa required state can be changed directly, in case the requirement
// is predetermined by other means (e.g. session mfa event emitter).
mfa.current.setMfaRequired(false);
// The mfa requirement can be initialized to a non-undefined value to skip
// the mfa check when it isn't needed, e.g. the requirement was predetermined.
expect(mfa.current.mfaRequired).toEqual(false);

// The mfa required state can be changed directly, in case the requirement
// need to be updated by the caller.
mfa.current.setMfaRequired(true);
expect(mfa.current.mfaRequired).toEqual(false);
await waitFor(() => {
expect(mfa.current.mfaRequired).toEqual(true);
});

// The mfa required state can be changed back to null (default) to force
// The mfa required state can be changed back to undefined (default) or null to force
// the next mfa attempt to re-check mfa required / attempt to get a challenge.
mfa.current.setMfaRequired(null);
expect(mfa.current.mfaRequired).toEqual(null);
await waitFor(() => {
expect(mfa.current.mfaRequired).toEqual(null);
});

// mfa required will be updated to true as usual once the server returns an mfa challenge.
mfa.current.getChallengeResponse();
Expand Down
12 changes: 8 additions & 4 deletions web/packages/teleport/src/lib/useMfa.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ import {

export type MfaProps = {
req?: CreateAuthenticateChallengeRequest;
// isMfaRequired is an initial state for isMfaRequired. Useful in cases
// where the mfa requirement is discovered and set indirectly by the caller.
isMfaRequired?: boolean;
};

type mfaResponsePromiseWithResolvers = {
Expand All @@ -51,8 +54,8 @@ type mfaResponsePromiseWithResolvers = {
* be used to display options to the user and prompt for them to complete
* the MFA check.
*/
export function useMfa({ req }: MfaProps): MfaState {
const [mfaRequired, setMfaRequired] = useState<boolean>();
export function useMfa({ req, isMfaRequired }: MfaProps): MfaState {
const [mfaRequired, setMfaRequired] = useState<boolean>(isMfaRequired);
const [options, setMfaOptions] = useState<MfaOption[]>();
const [challenge, setMfaChallenge] = useState<MfaAuthenticateChallenge>();

Expand All @@ -68,8 +71,9 @@ export function useMfa({ req }: MfaProps): MfaState {
const [attempt, getResponse, setMfaAttempt] = useAsync(
useCallback(
async (challenge?: MfaAuthenticateChallenge) => {
// If a previous call determined that MFA is not required, this is a noop.
if (mfaRequired === false) return;
// If a challenge wasn't provided and we previously determined MFA is not
// required, this is a noop.
if (mfaRequired === false && !challenge) return;

challenge = challenge ? challenge : await auth.getMfaChallenge(req);
if (!challenge) {
Expand Down

0 comments on commit ce976bb

Please sign in to comment.