AzureAD / microsoft-authentication-library-for-js

Microsoft Authentication Library (MSAL) for JS
http://aka.ms/aadv2
MIT License
3.65k stars 2.65k forks source link

Cannot update a component (`MsalProvider`) #5796

Closed timburgess closed 1 year ago

timburgess commented 1 year ago

Core Library

MSAL.js v2 (@azure/msal-browser)

Core Library Version

2.34.0

Wrapper Library

MSAL React (@azure/msal-react)

Wrapper Library Version

1.5.4

Public or Confidential Client?

Public

Description

We use a GraphQL API to provide a React SPA with access to various data across our systems. The SPA GraphQL client we're using is Formidable's urql - https://formidable.com/open-source/urql/

We're trying to introduce msal as a valid authentication method and we can successfully acquire tokens silently and use those for access to the backend. The urql graphQL operation handling can be extended by providing callback functions for specific purposes. We're providing a callback that requests the token and then adds it to the urql operation.

However using the urql useQuery hook in the same pattern we have used elsewhere, we get a Cannot update a component (MsalProvider) while rendering a different component error - this occurs regardless of whether the token is existing in cache or newly acquired.

I've provided a stripped down index.tsx to demonstrate the issue.

Error Message

image

Msal Logs

image

MSAL Configuration

export const msalConfig: Configuration = {
  auth: {
    clientId: `${process.env.REACT_APP_WEB_CLIENT_ID}`,
    authority: `https://login.microsoftonline.com/${
      process.env.REACT_APP_TENANT_ID
    }`,
    redirectUri: `${process.env.REACT_APP_REDIRECT_URI ?? "http://localhost:3000"}`,
  },
  cache: {
    cacheLocation: "sessionStorage", // This configures where your cache will be stored
    storeAuthStateInCookie: false, // Set this to "true" if you are having issues on IE11 or Edge
  },
}

Relevant Code Snippets

This is a single index.tsx incorporating the MSAL components used, the urql Provider and a simple query:

import { useEffect } from "react"
import ReactDOM from "react-dom/client"
import { MsalAuthenticationTemplate, MsalProvider } from "@azure/msal-react"
import { InteractionType, PublicClientApplication } from "@azure/msal-browser"
import { useQuery, Operation, makeOperation } from "urql"

// self-host fonts
import "@fontsource/inter"
import "./index.css"
import { GRAPHQL_URL, APP_MODE } from "./conf/config"
import { msalConfig } from "./conf/ssoConfig"
import { createClient, Provider, fetchExchange } from "urql"
import { contextExchange } from "./exchanges/contextExchange"
import { FetchingLabel } from "./common/FetchingLabel"
import { UhOh } from "./common/UhOh"

console.log(`App mode is ${APP_MODE}`)
console.log(`Using graphql at ${GRAPHQL_URL}`)

const SITES = `
query utags {
  sites {
    id
    name
    description
  }
}
`
const msalInstance = new PublicClientApplication(msalConfig)

// get the msal access token from cache or acquire a new one
const acquireAccessToken = async (): Promise<string> => {
  const activeAccount = msalInstance.getActiveAccount() // This will only return a non-null value if you have logic somewhere else that calls the setActiveAccount API
  const accounts = msalInstance.getAllAccounts()

  if (!activeAccount && accounts.length === 0) {
    console.error("No accounts found. Please login.")
    /*
     * User is not signed in. Throw error or wait for user to login.
     * Do not attempt to log a user in outside of the context of MsalProvider
     */
  }
  const request = {
    scopes: ["User.Read"],
    account: activeAccount || accounts[0],
  }

  const authResult = await msalInstance.acquireTokenSilent(request)
  return authResult.accessToken
}

// add the access token to the exchange operation context
const addTokenToContext = async (operation: Operation) => {
  // if operation is not a query or mutation, return the operation
  if (operation.kind !== "query" && operation.kind !== "mutation") {
    return operation.context
  }

  const token = await acquireAccessToken()

  const fetchOptions =
    typeof operation.context.fetchOptions === "function"
      ? operation.context.fetchOptions()
      : operation.context.fetchOptions || {}

  return makeOperation(operation.kind, operation, {
    ...operation.context,
    fetchOptions: {
      ...fetchOptions,
      headers: {
        ...fetchOptions.headers,
        Authorization: `Bearer ${token}`,
      },
    },
  }).context
}

const root = ReactDOM.createRoot(document.getElementById("root") as HTMLElement)

const exchanges = [
  contextExchange({
    getContext: addTokenToContext,
  }),
  fetchExchange,
]

// setup urql client for use with msal
const client = createClient({
  url: GRAPHQL_URL,
  exchanges,
})

root.render(
  <MsalProvider instance={msalInstance}>
    <MsalAuthenticationTemplate interactionType={InteractionType.Redirect}>
      <Provider value={client}>
        <SimpleQuery />
      </Provider>
    </MsalAuthenticationTemplate>
  </MsalProvider>,
)

export function SimpleQuery() {
  const [result] = useQuery({
    query: SITES,
  })
  const { data, fetching, error } = result

  useEffect(() => {
    if (data?.sites) {
      console.log(data.sites)
    }
  }, [data])

  if (fetching) {
    return <FetchingLabel>Fetching universal tags</FetchingLabel>
  }
  if (error) {
    return <UhOh error={error}>We were unable to find any Sites</UhOh>
  }

  return <div>Simple Query</div>
}


### Reproduction Steps

1. `npm start` with a React CRA setup

### Expected Behavior

We would expect the MSALProvider not to generate a React error on silent token request

### Identity Provider

Azure AD / MSA

### Browsers Affected (Select all that apply)

Chrome

### Regression

_No response_

### Source

External (Customer)
sameerag commented 1 year ago

@timburgess So token acquisition works but only the component update fails?

timburgess commented 1 year ago

Yes, the addTokenToContext acquires a token and we can use that successfully with the API

sameerag commented 1 year ago

@timburgess Is it possible this can be an issue with urql? We found some similar issues closed on their end, one example here. cc @tnorling

ghost commented 1 year ago

@timburgess This issue has been automatically marked as stale because it is marked as requiring author feedback but has not had any activity for 5 days. If your issue has been resolved please let us know by closing the issue. If your issue has not been resolved please leave a comment to keep this open. It will be closed automatically in 7 days if it remains stale.

timburgess commented 1 year ago

See urql #3088

ghost commented 1 year ago

This issue requires attention from the MSAL.js team and has not seen activity in 5 days. @sameerag please follow up.

sameerag commented 1 year ago

@timburgess Closing this and please check the above comments for the context. Please let us know here if something else is needed fro. MSAL JS team.

kawazoe commented 1 year ago

Hello,

Me and my team have been trying to integrate MSAL in a React + Relay application and are getting a similar behaviour. I've looked at the issue in urql mentioned before in this thread, but they never actually fixed the problem. They simply stated that the problem was with MSAL, and they added a patch on their side to silence the error.

In your case, it's likely caused by some kind of synchronisation that happens between msalInstance.acquireTokenSilent and MsalProvider, but it's hard to tell.

Hence, the PR I've linked (https://github.com/urql-graphql/urql/pull/3095) silences these warnings. It doesn't address the root cause that often causes them because that's out of our control...

Per our investigation, this error happens every time acquireTokenSilent falls into a scenario were it triggers an event which is handled by the MsalProvider.

In a Relay application, the call is done in the fetch function provided to the Network.create factory in the RelayEnvironement module.

const fetchFn: FetchFunction = async (request, variables) => {
  const accessToken = await getAccessToken();

  const resp = await fetch(`${config.API_URL}/graphql`, {
    method: 'POST',
    headers: {
      Accept: 'application/json',
      'Content-Type': 'application/json',
      ...(accessToken != null ? { Authorization: `bearer ${accessToken}` } : {}),
    },
    body: JSON.stringify({
      query: request.text, // <-- The GraphQL document composed by Relay
      variables,
    }),
  });

  return await resp.json();
};

async function getAccessToken(): Promise<string | null> {
  // The same instance given to the MsalProvider.
  const accounts = msalInstance.getAllAccounts();

  if (accounts) {
    const account = accounts[0];
    const tokenRequest = { ...apiRequest, account };
    const { accessToken } = (await msalInstance.acquireTokenSilent(tokenRequest)) || {};
    return accessToken;
  }
  return null;
}

While the issue was previously closed as being on the urql side, seeing this in Relay as well makes me think there might be an architectural issue in Msal that enables this issue.

@sameerag Care to give your opinion on this?

eberridge commented 1 year ago

For anyone coming late like me, I had the same issue with React + MSAL + Relay. After a lot of trial and error I realized the simplest way to fix this was to simply make sure none of the Relay queries happen in the render cycle by moving them into useEffect. Here's a custom hook that illustrates one of the changes involved.

export const usePreloadedQueryOnce = <T extends OperationType>(
  query: GraphQLTaggedNode,
  initialVariables?: VariablesOf<T>,
  options?: UseQueryLoaderLoadQueryOptions,
): PreloadedQuery<T> | null | undefined => {
  const [queryRef, loadQuery] = useQueryLoader<T>(query);
  const [isLoaded, setIsLoaded] = useState(false);

  useEffect(() => {
    if (!isLoaded) {
      loadQuery(initialVariables || {}, options);
      setIsLoaded(true);
    }
  }, [isLoaded, loadQuery, initialVariables, options]);

  return queryRef;
};
timburgess commented 1 year ago

Interesting. FWIW we migrated away from urql to the Apollo client and all the MsalProvider issues went away.

kawazoe commented 1 year ago

@eberridge This isn't really a solution. Since you cannot use useLazyLoadQuery anymore, and can't load content on first render, you'll have a hard time using fetchOptions that provides data immediately. You'll always flicker with your loading state for at least a frame.

tnorling commented 1 year ago

Closing as it's not clear this is an issue with MSAL. If anyone disagrees please feel free to open a new issue thanks!

kawazoe commented 1 year ago

After some investigations, I have found a better workaround for relay applications like mine and @eberridge.

The fix is the ensure that the call to acquireTokenSilent never happen during the react render phase. Here's a version of the Relay project setup sample modified with the fix.

const fetchFn: FetchFunction = async (request, variables) => {
  await Promise.resolve(); //< This call make sure we're now running in a different MicroTask.
  const accessToken = // It is safe to call acquireTokenSilent() here.

  const resp = await fetch(`${config.API_URL}/graphql`, {
    method: 'POST',
    headers: {
      Accept: 'application/json',
      'Accept-Language': resolvedLanguage(i18n),
      'Content-Type': 'application/json',
      ...(accessToken != null ? { Authorization: `bearer ${accessToken}` } : {})
    },
    body: JSON.stringify({
      query: request.text, // <-- The GraphQL document composed by Relay
      variables,
    }),
  });

  return await resp.json();
};
LarsKemmann commented 8 months ago

Oh wow @kawazoe -- I can't tell you how many hours I've spent fighting this exact same issue on different projects where I was using msal-react! Thank you!! Adding await Promise.resolve(); is exactly what's needed. I'm guessing the underlying issue is that acquireTokenSilent() completes synchronously for the typical case, at which point the MSAL instance (which we're of course referencing in the component tree via <MsalProvider />) is being updated while still in the same MicroTask as a React rendering step.

In my case I'm using Jotai and trying to initialize an atom asynchronously on first use with an API call (that requires getting a token from MSAL), but I've also seen this issue pop up with Recoil.js. No amount of useEffect(...)-foo was ever really helpful, and in one case I ended up tearing out msal-react altogether in frustration and managing authentication entirely outside of the React tree.

@tnorling Could you add some guidance for this type of scenario in the msal-react tutorial step/documentation for getting an access token?

ashelopukho commented 6 months ago

We faced the same problem when using the method useSuspenseQuery from the library (Tanstack Query). @kawazoe fix works well.