Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Token refresh issues #4

Closed
dnk8n opened this issue Apr 25, 2023 · 6 comments · Fixed by #7
Closed

Token refresh issues #4

dnk8n opened this issue Apr 25, 2023 · 6 comments · Fixed by #7
Labels
bug Something isn't working

Comments

@dnk8n
Copy link

dnk8n commented Apr 25, 2023

To reproduce:

  1. log into the system using your Azure AD credentials
  2. Check your JWT at https://jwt.ms
  3. See the "exp" key, and copy its value to https://www.epochconverter.com to see when the current JWT expires
  4. Navigate your application just before the expiry time comes around, you will be logged out and asked which account you want to be logged out of, and you are forced to redo the login.
  5. Side effect: Bearer tokens from re-login is not attached to the data provider requests, thus returning a 401 and kicking you out of the system again due to authentication error.

What should happen, is that the frontend should refresh the token in time in the background, so that the ID or Access token sent to APIs does not expire.

@dnk8n
Copy link
Author

dnk8n commented Apr 25, 2023

It is likely my API is being called with stale JWT before the frontend has called its acquireTokenSilent function. This means that by default the system performs a logout in response to the API's 401 error.

i.e. FE might be doing what it needs to, but too late. I am investigating a way for the 401 error to be caught and retried. See below code (which is unfortunately not yet working, but explains what I am tying):

Edit: Retracted code, because it is repeated in the following comment in better format.

@dnk8n
Copy link
Author

dnk8n commented Apr 25, 2023

On further inspection, it seems the problem is only on the boundary of the expiry time (an edge case). Often the token refresh is working as expected. The rough debugging code that helped me work out what is going on is here in case anyone feels like digging deeper... but this is working for me.

In the rare case that the API is called before the token is refreshed, it seems to be covered (I am pretty fresh to javascript/react, so feedback welcome):

export const tokenRequest: SilentRequest = {
  scopes: ["User.Read"],
  forceRefresh: false,
};

export const myMSALObj = new PublicClientApplication(msalConfig);

const baseAuthProvider = msalAuthProvider({
  msalInstance: myMSALObj,
  loginRequest,
  tokenRequest,
  redirectOnCheckAuth: false
});

export const authProvider = {
  ...baseAuthProvider,
  checkError: async (error: { status: number; message: { body: string; }; }) => {
    if (error.status === 403) {
      return Promise.resolve();
    } else if (
      error.status === 401 &&
      error instanceof HttpError &&
      error.message === "JWT expired"
    ) {
      return Promise.resolve();
    }
    return baseAuthProvider.checkError(error);
  }
};

So basically in the above, we ensure that the frontend is OK with 403s and a 401 in the case that JWT is expired. The dataprovider needs to catch that error and retry so that the failed request brings the expected data the second time round... e.g

const acquireToken = async ({ msalInstance, tokenRequest }: MsalHttpClientParams) => {
  const authResult = await msalInstance.acquireTokenSilent({
    account: msalInstance.getActiveAccount() as AccountInfo,
    ...tokenRequest,
    scopes: tokenRequest!.scopes !== undefined ? tokenRequest!.scopes : [],
  });
  const id_token = authResult?.idToken;
  return { authenticated: !!id_token, token: `Bearer ${id_token}` };
};

const postgrestHttpClient = ({ msalInstance, tokenRequest }: MsalHttpClientParams) => async (url: string, options: Options = {}) => {
  // ...
  const user = await acquireToken({ msalInstance, tokenRequest });
  let onboardCalled = false;
  try {
    return await fetchUtils.fetchJson(url, { ...options, user });
  } catch (error) {
    if (
      error instanceof HttpError &&
      error.body &&
      error.body.message &&
      error.body.message.includes("JWT expired")
    ) {
      const user = await acquireToken(
        {
          msalInstance,
          tokenRequest: { ...tokenRequest, forceRefresh: true } as SilentRequest
        }
      );
      return await fetchUtils.fetchJson(url, { ...options, user });
    } else if {
    // ...
    }
    else {
      throw error;
    }
  }
};

@dnk8n dnk8n changed the title Token is not being refreshed automatically Token refresh issues Apr 26, 2023
@slax57 slax57 added the bug Something isn't working label Apr 27, 2023
@slax57
Copy link
Collaborator

slax57 commented Apr 27, 2023

Thanks for the detailed report!
We'll look into it.

@dnk8n
Copy link
Author

dnk8n commented Apr 27, 2023

Note: Edited the code that worked for me once last time as I realized it wasn't entering the if condition I expected it to, and forceRefresh is required to be true in this edge case.

@dnk8n
Copy link
Author

dnk8n commented Apr 29, 2023

The solution implemented above, successfully handles an edge case identified (the window of time before token has expired according to the front-end and backend) by a try/catch, but one should probably allow a configurable tolerance, defaulting to ~10 seconds). I.e. refresh the token a few seconds before it is about to expire, so that the next API call is sure to receive a valid token. This doesn't necessarily have to be the front end's responsibility though, although easier to configure (so allow tolerance to be disabled too).

The following might need to be logged as a new issue, but is somewhat related:

In Azure AD, there is the following option:

Front-channel logout URL
This is where we send a request to have the application clear the user's session data. This is required for single sign-out to work correctly.

I don't think your tutorial mentions this. Maybe not so necessary by default because React-admin defaults to redirecting to /login. But probably good to set (edit, I found I didn't need to set it... but the information to clear users session data was helpful).

Apparently, it is the responsibility of the app to "clear the user's session data". Without doing this, in the case where you are automatically logged out (by edge case above!) or when you manually log out yourself, you won't be allowed to log in again unless you do something like (note the line with localStorage.removeItem):

export const CustomLoginPage = () => {
  const login = useLogin();
  const [loading, setLoading] = useSafeSetState(false);
  const notify = useNotify();
  const submit = () => {
    setLoading(true);
    for (const key in localStorage) {
      if (key.includes(import.meta.env.VITE_MSAL_CLIENT_ID)) {
        localStorage.removeItem(key);
      }
    }
    login({})
      .then(() => {
        setLoading(false);
      })
      .catch((error) => {
        setLoading(false);
        const errorMsg =
          typeof error === "string"
            ? error
            : error && error.message
              ? error.message
              : undefined;
        notify(errorMsg, {
          type: "error",
          messageArgs: {
            _: errorMsg,
          },
        });
      });
  };

I delete from localstorage because of my configuration:

const msalConfig: Configuration = {
  auth: {
    clientId: import.meta.env.VITE_MSAL_CLIENT_ID,
    authority: import.meta.env.VITE_MSAL_AUTHORITY,
    redirectUri: `${import.meta.env.VITE_APP_BASE_URI}/auth-callback`,
    navigateToLoginRequestUrl: false
  },
  cache: {
    cacheLocation: "localStorage"
  },
};

Note: navigateToLoginRequestUrl being false was needed, else odd behavior.

@slax57
Copy link
Collaborator

slax57 commented May 2, 2023

Again, thank you so much for this helpful additional info 🙂 .

Indeed this would deserve a new issue IMO.

I have a question though. If we set the logout URL to /logout (which internally will call msalInstance.logoutRedirect({ account })), does it not automatically remove the localStorage keys used for the cache for you?
IMO this should be the responsibility of the MSAL, RA should only call the lib's logout mehod at logout.
Did you give it a try?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants