Fix WebUI Admin Action infinite retry with no MFA devices (#51134)

* Fix admin action retry method resulting in an infinite loop. * Return an empty challenge and challenge response instead of undefined. * Add api.getAdminActionMfaResponse and add new custom error message. * Resolve comments. * Don't swallow non-admin-action-required errors. * Remove unnecessary awaits. * Fix try/catch.
gravitational · Jan 25, 2025 · 6b6b0cd · 6b6b0cd
1 parent 738b152
commit 6b6b0cd
Show file tree

Hide file tree

Showing 5 changed files with 83 additions and 77 deletions.
diff --git a/web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.tsx b/web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.tsx
@@ -64,7 +64,7 @@ export function ChangePasswordWizard({
   const reauthState = useReAuthenticate({
     challengeScope: MfaChallengeScope.CHANGE_PASSWORD,
     onMfaResponse: async mfaResponse =>
-      setWebauthnResponse(mfaResponse?.webauthn_response),
+      setWebauthnResponse(mfaResponse.webauthn_response),
   });
 
   const [reauthMethod, setReauthMethod] = useState<ReauthenticationMethod>();

diff --git a/web/packages/teleport/src/lib/term/tty.ts b/web/packages/teleport/src/lib/term/tty.ts
@@ -80,16 +80,16 @@ class Tty extends EventEmitterMfaSender {
     this.socket.send(bytearray.buffer);
   }
 
-  sendChallengeResponse(resp: MfaChallengeResponse) {
+  sendChallengeResponse(data: MfaChallengeResponse) {
     // we want to have the backend listen on a single message type
     // for any responses. so our data will look like data.webauthn, data.sso, etc
     // 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.0.0
     const backwardCompatibleData = {
-      ...resp?.webauthn_response,
-      ...resp,
+      ...data.webauthn_response,
+      ...data,
     };
     const encoded = this._proto.encodeChallengeResponse(
       JSON.stringify(backwardCompatibleData)

diff --git a/web/packages/teleport/src/services/api/api.test.ts b/web/packages/teleport/src/services/api/api.test.ts
@@ -16,6 +16,7 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+import { MfaChallengeResponse } from '../mfa';
 import api, {
   defaultRequestOptions,
   getAuthHeaders,
@@ -24,9 +25,18 @@ import api, {
 } from './api';
 
 describe('api.fetch', () => {
-  const mockedFetch = jest.spyOn(global, 'fetch').mockResolvedValue({} as any); // we don't care about response
+  let mockedFetch: jest.SpiedFunction<typeof fetch>;
+  beforeEach(() => {
+    mockedFetch = jest
+      .spyOn(global, 'fetch')
+      .mockResolvedValue({ json: async () => ({}), ok: true } as Response); // we don't care about response
+  });
+
+  afterEach(() => {
+    jest.resetAllMocks();
+  });
 
-  const mfaResp = {
+  const mfaResp: MfaChallengeResponse = {
     webauthn_response: {
       id: 'some-id',
       type: 'some-type',
@@ -43,18 +53,14 @@ describe('api.fetch', () => {
     },
   };
 
-  const customOpts = {
+  const customOpts: RequestInit = {
     method: 'POST',
     // Override the default header from `defaultRequestOptions`.
     headers: {
       Accept: 'application/json',
     },
   };
 
-  afterEach(() => {
-    jest.resetAllMocks();
-  });
-
   test('default (no optional params provided)', async () => {
     await api.fetch('/something');
     expect(mockedFetch).toHaveBeenCalledTimes(1);
@@ -156,7 +162,7 @@ describe('api.fetch', () => {
   });
 });
 
-// The code below should guard us from changes to api.fetchJson which would cause it to lose type
+// The code below should guard us from changes to api.fetchJsonWithMfaAuthnRetry which would cause it to lose type
 // information, for example by returning `any`.
 
 const fooService = {
@@ -171,13 +177,13 @@ const makeFoo = (): { foo: string } => {
 
 // This is a bogus test to satisfy Jest. We don't even need to execute the code that's in the async
 // function, we're interested only in the type system checking the code.
-test('fetchJson does not return any', () => {
+test('fetchJsonWithMfaAuthnRetry does not return any', () => {
   const bogusFunction = async () => {
     const result = await fooService.doSomething();
     // Reading foo is correct. We add a bogus expect to satisfy Jest.
     JSON.stringify(result.foo);
 
-    // @ts-expect-error If there's no error here, it means that api.fetchJson returns any, which it
+    // @ts-expect-error If there's no error here, it means that api.fetchJsonWithMfaAuthnRetry returns any, which it
     // shouldn't.
     JSON.stringify(result.bar);
   };

diff --git a/web/packages/teleport/src/services/api/api.ts b/web/packages/teleport/src/services/api/api.ts
@@ -150,66 +150,31 @@ const api = {
     customOptions: RequestInit,
     mfaResponse?: MfaChallengeResponse
   ): Promise<any> {
-    const response = await api.fetch(url, customOptions, mfaResponse);
-
-    let json;
     try {
-      json = await response.json();
+      return await api.fetch(url, customOptions, mfaResponse);
     } catch (err) {
-      // error reading JSON
-      const message = response.ok
-        ? err.message
-        : `${response.status} - ${response.url}`;
-      throw new ApiError({ message, response, opts: { cause: err } });
-    }
-
-    if (response.ok) {
-      return json;
-    }
-
-    /** This error can occur in the edge case where a role in the user's certificate was deleted during their session. */
-    const isRoleNotFoundErr = isRoleNotFoundError(parseError(json));
-    if (isRoleNotFoundErr) {
-      websession.logoutWithoutSlo({
-        /* Don't remember location after login, since they may no longer have access to the page they were on. */
-        rememberLocation: false,
-        /* Show "access changed" notice on login page. */
-        withAccessChangedMessage: true,
-      });
-      return;
+      // Retry with MFA if we get an admin action MFA error.
+      if (!mfaResponse && isAdminActionRequiresMfaError(err)) {
+        mfaResponse = await api.getAdminActionMfaResponse();
+        return api.fetch(url, customOptions, mfaResponse);
+      } else {
+        throw err;
+      }
     }
+  },
 
-    // Retry with MFA if we get an admin action missing MFA error.
-    const isAdminActionMfaError = isAdminActionRequiresMfaError(
-      parseError(json)
-    );
-    const shouldRetry = isAdminActionMfaError && !mfaResponse;
-    if (!shouldRetry) {
-      throw new ApiError({
-        message: parseError(json),
-        response,
-        proxyVersion: parseProxyVersion(json),
-        messages: json.messages,
-      });
-    }
+  async getAdminActionMfaResponse() {
+    const challenge = await auth.getMfaChallenge({
+      scope: MfaChallengeScope.ADMIN_ACTION,
+    });
 
-    let mfaResponseForRetry;
-    try {
-      const challenge = await auth.getMfaChallenge({
-        scope: MfaChallengeScope.ADMIN_ACTION,
-      });
-      mfaResponseForRetry = await auth.getMfaChallengeResponse(challenge);
-    } catch {
+    if (!challenge) {
       throw new Error(
-        'Failed to fetch MFA challenge. Please connect a registered hardware key and try again. If you do not have a hardware key registered, you can add one from your account settings page.'
+        '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.'
       );
     }
 
-    return api.fetchJsonWithMfaAuthnRetry(
-      url,
-      customOptions,
-      mfaResponseForRetry
-    );
+    return auth.getMfaChallengeResponse(challenge);
   },
 
   /**
@@ -254,7 +219,7 @@ const api = {
    * @param mfaResponse if defined (eg: `fetchJsonWithMfaAuthnRetry`)
    * will add a custom MFA header field that will hold the mfaResponse.
    */
-  fetch(
+  async fetch(
     url: string,
     customOptions: RequestInit = {},
     mfaResponse?: MfaChallengeResponse
@@ -280,7 +245,41 @@ const api = {
     }
 
     // native call
-    return fetch(url, options);
+    const response = await fetch(url, options);
+
+    let json;
+    try {
+      json = await response.json();
+    } catch (err) {
+      // error reading JSON
+      const message = response.ok
+        ? err.message
+        : `${response.status} - ${response.url}`;
+      throw new ApiError({ message, response, opts: { cause: err } });
+    }
+
+    if (response.ok) {
+      return json;
+    }
+
+    /** This error can occur in the edge case where a role in the user's certificate was deleted during their session. */
+    const isRoleNotFoundErr = isRoleNotFoundError(parseError(json));
+    if (isRoleNotFoundErr) {
+      websession.logoutWithoutSlo({
+        /* Don't remember location after login, since they may no longer have access to the page they were on. */
+        rememberLocation: false,
+        /* Show "access changed" notice on login page. */
+        withAccessChangedMessage: true,
+      });
+      return;
+    }
+
+    throw new ApiError({
+      message: parseError(json),
+      response,
+      proxyVersion: parseProxyVersion(json),
+      messages: json.messages,
+    });
   },
 };
 
@@ -326,8 +325,8 @@ export function getHostName() {
   return location.hostname + (location.port ? ':' + location.port : '');
 }
 
-function isAdminActionRequiresMfaError(errMessage) {
-  return errMessage.includes(
+function isAdminActionRequiresMfaError(err: Error) {
+  return err.message.includes(
     'admin-level API request requires MFA verification'
   );
 }

diff --git a/web/packages/teleport/src/services/auth/auth.ts b/web/packages/teleport/src/services/auth/auth.ts
@@ -238,7 +238,7 @@ const auth = {
       .then(res => {
         const request = {
           action: 'accept',
-          webauthnAssertionResponse: res?.webauthn_response,
+          webauthnAssertionResponse: res.webauthn_response,
         };
 
         return api.put(cfg.getHeadlessSsoPath(transactionId), request);
@@ -254,11 +254,11 @@ const auth = {
   },
 
   // getChallenge gets an MFA challenge for the provided parameters. If is_mfa_required_req
-  // is provided and it is found that MFA is not required, returns null instead.
+  // is provided and it is found that MFA is not required, returns undefined instead.
   async getMfaChallenge(
     req: CreateAuthenticateChallengeRequest,
     abortSignal?: AbortSignal
-  ) {
+  ): Promise<MfaAuthenticateChallenge | undefined> {
     return api
       .post(
         cfg.api.mfaAuthnChallengePath,
@@ -274,13 +274,14 @@ const auth = {
   },
 
   // getChallengeResponse gets an MFA challenge response for the provided parameters.
-  // If is_mfa_required_req is provided and it is found that MFA is not required, returns null instead.
+  // If challenge is undefined or has no viable challenge options, returns empty response.
   async getMfaChallengeResponse(
     challenge: MfaAuthenticateChallenge,
     mfaType?: DeviceType,
     totpCode?: string
-  ): Promise<MfaChallengeResponse | undefined> {
-    if (!challenge) return;
+  ): Promise<MfaChallengeResponse> {
+    // No challenge, return empty response.
+    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
@@ -310,7 +311,7 @@ const auth = {
     }
 
     // No viable challenge, return empty response.
-    return;
+    return {};
   },
 
   async getWebAuthnChallengeResponse(
@@ -439,7 +440,7 @@ const auth = {
     return auth
       .getMfaChallenge({ scope, allowReuse, isMfaRequiredRequest }, abortSignal)
       .then(challenge => auth.getMfaChallengeResponse(challenge, 'webauthn'))
-      .then(res => res?.webauthn_response);
+      .then(res => res.webauthn_response);
   },
 
   getMfaChallengeResponseForAdminAction(allowReuse?: boolean) {