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

Azure Active Directory refresh access token #1675

Merged
merged 2 commits into from
Jan 28, 2021
Merged

Azure Active Directory refresh access token #1675

merged 2 commits into from
Jan 28, 2021

Conversation

jeliasson
Copy link
Contributor

@jeliasson jeliasson commented Jan 27, 2021

Addressing a fix for #1672 using acquireTokenSilent to obtain access token and handle the renewal.

The pattern for acquiring tokens for APIs with MSAL.js is to first attempt a silent token request by using the acquireTokenSilent method. When this method is called, the library first checks the cache in browser storage to see if a valid token exists and returns it. When no valid token is in the cache, it sends a silent token request to Azure Active Directory (Azure AD) from a hidden iframe. This method also allows the library to renew tokens.

https://docs.microsoft.com/en-us/azure/active-directory/develop/scenario-spa-acquire-token?tabs=javascript

@github-actions
Copy link

github-actions bot commented Jan 27, 2021

@jeliasson jeliasson marked this pull request as ready for review January 28, 2021 11:19
@jeliasson
Copy link
Contributor Author

Initial tests indicates that access token is being successfully renewed. @dthyresson, please make a review and merge at your convenience.

/cc @dac09 @AndrewLamYW

@dac09
Copy link
Collaborator

dac09 commented Jan 28, 2021

@jeliasson won't this refresh the token every time getToken is called? Is there a way to determine if the token has expired first then do the fetch?

@dthyresson
Copy link
Contributor

won't this refresh the token every time getToken is called?

@dac09 I was wondering the same so I began reading

https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/wiki/AcquireTokenSilentAsync-using-a-cached-token

Next time the application wants a token, it should first call AcquireTokenSilent, to verify if an acceptable token is in the cache, can be refreshed, or can get derived.

So, it checks if it is the cache first, and if not will request it.

The recommended call pattern is to first try to call AcquireTokenSilent, and if it fails with a MsalUiRequiredException, call AcquireTokenXYZ

And that's what @jeliasson is doing --

try {
        const response = await client.acquireTokenSilent(renewIdTokenRequest)
        return response?.idToken?.rawIdToken || null
      } catch (error) {
        if (error instanceof InteractionRequiredAuthError) {
          client.acquireTokenRedirect(renewIdTokenRequest)
        } else {
          console.error(`azureActiveDirectory: Uncaught exception`, error)
        }
      }

So, while I cannot test, this seems to be implemented according to the docs.

In fact, this is the same as Auth0's getTokenSilently which the Auth0 provider uses:

https://github.com/redwoodjs/redwood/blob/main/packages/auth/src/authClients/auth0.ts#L36

https://auth0.github.io/auth0-spa-js/classes/auth0client.html#gettokensilently

@jeliasson
Copy link
Contributor Author

jeliasson commented Jan 28, 2021

won't this refresh the token every time getToken is called? Is there a way to determine if the token has expired first then do the fetch?

@dac09 From my initial tests it will grab the cached token (msal.idtoken) if available, or perform a renewal approximately 5 minutes prior token expiration, or perform a renewal if cache is not available or is token is invalid. If it can't silently acquire a new token, it will redirect the user for re-authentication.

So, while I cannot test, this seems to be implemented according to the docs.

@dthyresson If you'd like, I can bring in this PR build to a new PR in playground-auth for testing. Let me know.

@dthyresson
Copy link
Contributor

If you'd like, I can bring in this PR build to a new PR in playground-auth for testing. Let me know.

@jeliasson I say we merge, I am actually updating playground-auth for Supabase OAuth and magic link (and oh by the way Supabase is supporting Azure as an OAuth provider, they just have not configured the admin UI yet) and am waiting for a solid canary or 0.24 to do that.

So, this will get in when that happens (hopefully in the next day or so).

@dthyresson dthyresson merged commit cab9e5f into redwoodjs:main Jan 28, 2021
@jeliasson jeliasson deleted the fix/azure-active-directory-token-refresh branch January 28, 2021 12:37
@thedavidprice thedavidprice added this to the next release milestone Jan 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants