useIsAuthenticated hook potentially may return false even if user is actually authenticated

[bohdanzhylavskyi]

Core Library

MSAL.js (@azure/msal-browser)

Wrapper Library

MSAL React (@azure/msal-react)

Public or Confidential Client?

Public

Description

Here is the current implementation of the useIsAuthenticated hook (https://github.com/AzureAD/microsoft-authentication-library-for-js/blob/msal-react-v2.0.12/lib/msal-react/src/hooks/useIsAuthenticated.ts#L32):

export function useIsAuthenticated(matchAccount?: AccountIdentifiers): boolean {
    const { accounts: allAccounts, inProgress } = useMsal();

    const [hasAuthenticated, setHasAuthenticated] = useState<boolean>(() => {
        if (inProgress === InteractionStatus.Startup) {
            return false;
        }
        return isAuthenticated(allAccounts, matchAccount);
    });

    useEffect(() => {
        setHasAuthenticated(isAuthenticated(allAccounts, matchAccount));
    }, [allAccounts, matchAccount]);

    return hasAuthenticated;
}

If you look carefully at the implementation, you may notice that there is a chance that on subsequent renders (all renders except the initial) that hook may return false even if user is authenticated and there are actually presented accounts in msal instance (isAuthenticated(allAccounts, matchAccount) === true). It happens because on each render we return the value (hasAuthenticated) calculated at the previous render (to be precise calculated right after the previous render, in useEffect hook).

That behavior may cause problems, for example, lately, I had a scenario where UnauthenticatedTemplate component (which uses useIsAuthenticated under the hood) was rendering its content (children props) even when the user was actually authenticated (so the account was presented in msal instance).

Below you can see pseudo-code to demonstrate that scenario:

<MsalProvider instance={msal}>
    <AuthenticatedTemplate>
        <App />
    </AuthenticatedTemplate>
    <UnauthenticatedTemplate>
        <DoLoginRedirect msal={msal} /> // that component will just call msal.redirectLogin() on initial render
    </UnauthenticatedTemplate>
</MsalProvider>

In my case, DoLoginRedirect was rendered for a short time after completing authentication (after redirecting from authentication server to be precise) and as a result, one more redundant login redirect step/loop happened.
After some time of debugging, I suspect that the implementation of useIsAuthenticated is the culprit of my problems. To be precise, I think that on render, the value returned by useIsAuthenticated hook is stale cause it was calculated before the actual render (it was calculated after the previous render in the useEffect hook).

Taking into account everything mentioned above, I would suggest to change implementation of useIsAuthenticated hook to something like that:

export function useIsAuthenticated(matchAccount?: AccountIdentifiers): boolean {
    const { accounts: allAccounts, inProgress } = useMsal();
    const isInitialRenderRef = useRef(true);

    const hasAuthenticated = useMemo(() => {
        if (isInitialRenderRef.current && inProgress === InteractionStatus.Startup) {
            return false;
        }

        return isAuthenticated(allAccounts, matchAccount);
    }, [inProgress, allAccounts, matchAccount]);

    if (isInitialRenderRef.current) {
        isInitialRenderRef.current = false;
    }

    return hasAuthenticated;
}

I know that the suggested implementation does not look very elegant and may be improved but the main point is that new implementation solves an issue with returned stale value and it also prevents redundant re-render (because we don't update any state (useState) anymore).

Please let me know your opinion on that matter and correct me if something is wrong with my suggestions and considerations. Thank you in advance

Source

External (Customer)

[edsolanas]

I am getting the opposite isAuthenticated when there are no accounts so my pages are blowing up even though they are wrapped under AuthenticatedTemplate

[sameerag]

Can you please raise a PR so we can review and apply the suggested change it if approved? The guidelines to contribute are here

[bohdanzhylavskyi]

@sameerag ok, I will create PR

[hatched-kade]

I believe I just ran into this issue. It surfaced after upgrading to react 18. Overall flow is that this issue happens for freshly created accounts that get redirected back to my app.

My code looks like:

  const { accounts, inProgress } = useMsal();
  const isAuthenticated = useIsAuthenticated(); 

Render 1:
{isAuthenticated: false, inProgress: 'handleRedirect', accounts: Array(0)}
Render 2:
{isAuthenticated: false, inProgress: 'none', accounts: Array(1)}
Render 3:
{isAuthenticated: true, inProgress: 'none', accounts: Array(1)}

Render 2 is the problem, where the hook is returning that the user is not authenticated even though there is an account. If I revert to React 17 that doesn't happen.

[hatched-kade]

@sameerag Did this get completed? I'm not seeing any associated release notes or pull request