CommunityToolkit / Graph-Controls

Set of Helpers and Controls for Windows development using the Microsoft Graph.
https://docs.microsoft.com/en-us/windows/communitytoolkit/graph/overview
Other
155 stars 39 forks source link

MsalProvider not handling token expiration properly #189

Open matheus-inacio opened 2 years ago

matheus-inacio commented 2 years ago

Describe the bug

My app periodically check for files in OneDrive and i've notices that if the app is kept opened long enough, the token will expire and the user will be prompt to select an account again.

I've manage to work around this issue by creating my own MsalProvider and changing the GetTokenWithScopesAsync as follows:

protected async Task<string> GetTokenWithScopesAsync(string[] scopes, bool silentOnly = false)
{
    await SemaphoreSlim.WaitAsync();

    try
    {
        AuthenticationResult authResult = null;
        try
        {
            var account = Account ?? (await Client.GetAccountsAsync()).FirstOrDefault();
            if (account != null)
            {
                authResult = await Client.AcquireTokenSilent(scopes, account).ExecuteAsync();
            }
        }
        catch (MsalUiRequiredException)
        {
           // CODE ADDED TO WORK AROUND THE EXPIRATION TOKEN ISSUE
            if (Account != null)
            {
                Account = null;
                SemaphoreSlim.Release();
                return await GetTokenWithScopesAsync(scopes, silentOnly);
            }
        }
        catch
        {
            // Unexpected exception
            // TODO: Send exception to a logger.
        }

        if (authResult == null && !silentOnly)
        {
            try
            {
                var paramBuilder = Client.AcquireTokenInteractive(scopes);

                if (Account != null)
                {
                    paramBuilder = paramBuilder.WithAccount(Account);
                }
                paramBuilder = paramBuilder.WithPrompt(Microsoft.Identity.Client.Prompt.NoPrompt);

                authResult = await paramBuilder.ExecuteAsync();
            }
            catch
            {
                // Unexpected exception
                // TODO: Send exception to a logger.
            }
        }

        Account = authResult?.Account;

        return authResult?.AccessToken;
    }
    finally
    {
        SemaphoreSlim.Release();
    }
}

I'm not submitting this as a PR cause i think that this is not the best way to handle this issue.

Steps to Reproduce

Steps to reproduce the behavior:

  1. Open an app and make a request so that a token is generated
  2. Keep the app open until the token expires
  3. Try to make another request
  4. The app will prompt you to select an account again.

Expected behavior

I believe that once the user has authenticated the account selection window should not be prompt again.

Environment

Package Version(s): 7.1.1

Windows 11 Build Number:
- [x] Windows 11 (22000)

App min and target version:
- [ ] Fall Creators Update (16299)
- [ ] April 2018 Update (17134)
- [ ] October 2018 Update (17763)
- [x] May 2019 Update (18362)
- [x] Windows 11 (Build 22000)

Device form factor:
- [x] Desktop
- [ ] Xbox
- [ ] Surface Hub
- [ ] IoT

Visual Studio 
- [ ] 2017 (version: )
- [ ] 2019 (version: ) 
- [ ] 2019 Preview (version: )
- [x] 2022
ghost commented 2 years ago

Hello matheus-inacio, thank you for opening an issue with us!

I have automatically added a "needs triage" label to help get things started. Our team will analyze and investigate the issue, and escalate it to the relevant team if possible. Other community members may also look into the issue and provide feedback 🙌

michael-hawker commented 2 years ago

I'm wondering if silentOnly is set to true here, so we're not getting the prompt to do the interactive re-login maybe?

https://github.com/CommunityToolkit/Graph-Controls/blob/15513b1861d6fa016165e61867c08ec05c0a70a0/CommunityToolkit.Authentication.Msal/MsalProvider.cs#L248

In the docs about providers here it does show falling back to interactive if the exception is hit.

So, maybe we just need to remove the check for !silentOnly and always do that if we don't have an account after trying to acquire silently.

Does that sound right @shweaver-MSFT?

shweaver-MSFT commented 2 years ago

I'm not sure. GetTokenWithScopesAsync is called during AuthenticateRequestAsync, but it does not specify silentOnly, so I'm expecting the prompt to show under most/all circumstances. The only time the function is called with the silent flag is during TrySilentSignInAsync.

Thinking of one scenario, I imagine that when the token is expired and using the LoginButton, the sign in will fail when the token is expired. LoginButton will attempt to silently login on load, but otherwise the button does not attempt any interactive login without being invoked by the user first.

In this situation, perhaps calling SignInAsync or GetTokenAsync might resolve the issue because it would prompt an interactive login.

But why is MSAL not auto-refreshing the token? Why do we need to interactively login again? That I cannot answer. A question for the MSAL team perhaps.