Fix MFA always required for file transfers

[Joerger]

Changelog: Fixed a bug in the WebUI where file transfers would always prompt for MFA, even when not required.

#49794 introduced some hacky useEffect logic that failed to properly share the mfa state between sessions and file transfers within that session. This PR fixes it by having the two straightforwardly share mfa state within the same object.

Fixes #51694

[ravicious]

Why do we need to check the status of the MFA attempt here? I feel like the comment doesn't quite explain this.

[Joerger]

Here's the original piece of code, which I don't fully understand:

  useEffect(() => {
    // when switching tabs or closing tabs, focus on visible terminal
    terminalRef.current?.focus();
  }, [visible, mfa.requested]);

I thought this was meant to focus the page when mfa is requested, but does the useEffect also get called at the start, when the user opens the terminal even if mfa.requested is false?

[Joerger]

This doesn't seem to do anything in the current or previous state. If I Cmd+click to open the terminal in a new tab, shouldn't it autofocus me to that tab?

[ravicious]

I thought this was meant to focus the page when mfa is requested, but does the useEffect also get called at the start, when the user opens the terminal even if mfa.requested is false?

Yes, the effect gets called whenever the deps change. It doesn't matter what their values are, as long as at least for one dep Object.is(oldDep, newDep) returns false.

I think "tabs" here refers not to browser tabs but to a little-known feature that I'm not sure how many people use. :D Probably not much since no one complained about focus not working.

When you connect to an SSH node through the Web UI, there's one in-page "tab" at the top with a plus button next to it. When you click the plus button, you can connect to another SSH node, similar how in Connect you can have multiple tabs.

We should probably bring back the previous behavior where it calls focus whenever visible changes or MFA is requested. Though I'm not sure which mfa prop we should check, mfaRequired? I think the intent might have been that after you go through the MFA check, the focus is automatically on the terminal.

The whole check should first see if visible is truthy, otherwise it'll keep calling focus even on tabs that are not visible. Which might not break anything but why do something that's unnecessary.

[ravicious]

Operations that update React state need to be wrapped in act. https://react.dev/reference/react/act

diff --git a/web/packages/teleport/src/lib/useMfa.test.tsx b/web/packages/teleport/src/lib/useMfa.test.tsx
index e3af0abd90..3c2522edcc 100644
--- a/web/packages/teleport/src/lib/useMfa.test.tsx
+++ b/web/packages/teleport/src/lib/useMfa.test.tsx
@@ -17,6 +17,7 @@
  */
 
 import { renderHook, waitFor } from '@testing-library/react';
+import { act } from 'react';
 
 import { CreateAuthenticateChallengeRequest } from 'teleport/services/auth';
 import auth, { MfaChallengeScope } from 'teleport/services/auth/auth';
@@ -137,23 +138,23 @@ describe('useMfa', () => {
 
     // The mfa required state can be changed directly, in case the requirement
     // need to be updated by the caller.
-    mfa.current.setMfaRequired(true);
-    await waitFor(() => {
-      expect(mfa.current.mfaRequired).toEqual(true);
+    await act(async () => {
+      mfa.current.setMfaRequired(true);
     });
+    expect(mfa.current.mfaRequired).toEqual(true);
 
     // 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);
-    await waitFor(() => {
-      expect(mfa.current.mfaRequired).toEqual(null);
+    await act(async () => {
+      mfa.current.setMfaRequired(null);
     });
+    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();
-    await waitFor(() => {
-      expect(mfa.current.mfaRequired).toEqual(true);
+    await act(async () => {
+      await mfa.current.getChallengeResponse();
     });
+    expect(mfa.current.mfaRequired).toEqual(true);
   });
 
   test('mfa challenge error', async () => {

However, once I do that, for some reason this test times out while waiting for mfa.current.getChallengeResponse() to finish. On top of that, other tests start failing, which suggests that something is wrong with the setup.

What's more, I noticed that the tests in this file mock console.error:

teleport/web/packages/teleport/src/lib/useMfa.test.tsx

Lines 63 to 65 in e7bbfba

	beforeEach(() => {
	jest.spyOn(console, 'error').mockImplementation();
	});

I suspect it was added because of the errors that were logged by React due to not wrapping updates in act. We should not mock console.error unless our code is actually expected to use console.error, otherwise it's going to swallow actually useful errors that might help uncover bugs in the implementation. From what I see, useMfa does not use console.error, so we should remove the mock and fix the tests.

[Joerger]

Got it. Looks like I was using waitFor when act is a much better way to do it (and doesn't result in error logs...).

The only test I couldn't fix is:

useMfa › reset mfa attempt

    User canceled MFA attempt

      133 |   const cancelAttempt = () => {
      134 |     if (mfaResponseRef.current) {
    > 135 |       mfaResponseRef.current.reject(new MfaCanceledError());
          |                                     ^
      136 |     }
      137 |   };
      138 |

      at Object.cancelAttempt (web/packages/teleport/src/lib/useMfa.ts:135:37)
      at cancelAttempt (web/packages/teleport/src/lib/useMfa.test.tsx:243:39)
      at act (node_modules/.pnpm/[email protected]/node_modules/react/cjs/react.development.js:2512:16)
      at Object.<anonymous> (web/packages/teleport/src/lib/useMfa.test.tsx:243:14)

The test is expecting this error - I don't understand why it isn't caught by the await expect(resp).rejects.toThrow(new MfaCanceledError()); line.

     let resp;
    await act(async () => {
      resp = mfa.current.getChallengeResponse();
    });

    await act(async () => mfa.current.cancelAttempt());

    await expect(resp).rejects.toThrow(new MfaCanceledError());

[ravicious]

Ooof I figured it out. I ran into a weird behavior of Jest around promises a few weeks ago. After poking around, I think it boils down to an unhandled promise. jestjs/jest#9210

Browsers give you warnings when you create a promise but don't handle its rejection. https://jakearchibald.com/2023/unhandled-rejections/#unhandled-rejections-are-problematic Jest just fails the test if there's an unhandled promise, but it doesn't tell you that explicitly, instead it depends on Node.js showing you an appropriate warning. This is especially pernicious when running pnpm tdd, because it's easy to miss it.

When you run pnpm test, you can see it more easily:

(node:46684) PromiseRejectionHandledWarning: Promise rejection was handled asynchronously (rejection id: 1)
(Use `node --trace-warnings ...` to show where the warning was created)

The issue was related to the fact that at the moment when we call mfa.current.cancelAttempt, resp gets rejected, but there's nothing to handle it, so Jest fails the test. The solution is to write some kind of code that attaches to the rejection handler of the promise, so that when the promise actually gets rejected, Jest sees that there's something handling the rejection.

[Joerger]

Ah that makes sense, thanks for finding the fix!

[ravicious]

Sorry for not reviewing this earlier, I was a bit swamped with other reviews.

[public-teleport-github-review-bot]

@Joerger See the table below for backport results.

Branch	Result
branch/v17	Create PR