-
Notifications
You must be signed in to change notification settings - Fork 225
fix: Clear persisted MSAL token cache on Disconnect-MgGraph #3649
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -430,6 +430,18 @@ public static async Task<IAuthContext> LogoutAsync() | |
| { | ||
| var authContext = GraphSession.Instance.AuthContext; | ||
| GraphSession.Instance.InMemoryTokenCache?.ClearCache(); | ||
| if (authContext?.ContextScope == ContextScope.CurrentUser) | ||
| { | ||
| try | ||
| { | ||
| await TokenCacheUtilities.ClearPersistedTokenCacheAsync(Constants.CacheName).ConfigureAwait(false); | ||
| } | ||
| catch (Exception) | ||
| { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any actual failure here would be silent, could you add a log here so it's diagnosable? |
||
| // Non-fatal: persisted cache clearing may fail on some platforms. | ||
| // The auth record and in-memory state are still cleared below. | ||
| } | ||
| } | ||
| GraphSession.Instance.AuthContext = null; | ||
| GraphSession.Instance.GraphHttpClient = null; | ||
| await DeleteAuthRecordAsync().ConfigureAwait(false); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| // ------------------------------------------------------------------------------ | ||
| // Copyright (c) Microsoft Corporation. All Rights Reserved. Licensed under the MIT License. See License in the project root for license information. | ||
| // ------------------------------------------------------------------------------ | ||
|
|
||
| using Microsoft.Identity.Client.Extensions.Msal; | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.IO; | ||
| using System.Threading.Tasks; | ||
|
|
||
| namespace Microsoft.Graph.PowerShell.Authentication.Core.Utilities | ||
| { | ||
| /// <summary> | ||
| /// Utilities for managing the MSAL token cache persisted to disk by Azure.Identity. | ||
| /// </summary> | ||
| internal static class TokenCacheUtilities | ||
| { | ||
| // Azure.Identity internal constants for cache storage configuration. | ||
| // See: Azure/azure-sdk-for-net - sdk/core/Azure.Core/src/Identity/Constants.cs | ||
| private const string DefaultCacheKeychainService = "Microsoft.Developer.IdentityService"; | ||
| private const string DefaultCacheKeyringSchema = "msal.cache"; | ||
| private const string DefaultCacheKeyringCollection = "default"; | ||
| private static readonly KeyValuePair<string, string> DefaultCacheKeyringAttribute1 = | ||
| new KeyValuePair<string, string>("MsalClientID", "Microsoft.Developer.IdentityService"); | ||
| private static readonly KeyValuePair<string, string> DefaultCacheKeyringAttribute2 = | ||
| new KeyValuePair<string, string>("Microsoft.Developer.IdentityService", "1.0.0.0"); | ||
|
|
||
| // Azure.Identity appends CAE suffixes to the cache name internally. | ||
| private const string CaeEnabledSuffix = ".cae"; | ||
| private const string CaeDisabledSuffix = ".nocae"; | ||
|
|
||
| private static readonly string DefaultCacheDirectory = | ||
| Path.Combine( | ||
| Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData), | ||
| ".IdentityService"); | ||
|
|
||
| /// <summary> | ||
| /// Clears the persisted MSAL token cache files created by Azure.Identity | ||
| /// for the given cache name. Clears both CAE-enabled and CAE-disabled variants. | ||
| /// </summary> | ||
| /// <param name="cacheName">The cache name (e.g., "mg.msal.cache").</param> | ||
| public static async Task ClearPersistedTokenCacheAsync(string cacheName) | ||
| { | ||
| // Azure.Identity creates separate caches for CAE-enabled and CAE-disabled tokens. | ||
| await ClearCacheAsync(cacheName + CaeEnabledSuffix).ConfigureAwait(false); | ||
| await ClearCacheAsync(cacheName + CaeDisabledSuffix).ConfigureAwait(false); | ||
| } | ||
|
|
||
| private static async Task ClearCacheAsync(string cacheFileName) | ||
| { | ||
| var storageProperties = new StorageCreationPropertiesBuilder(cacheFileName, DefaultCacheDirectory) | ||
| .WithMacKeyChain(DefaultCacheKeychainService, cacheFileName) | ||
| .WithLinuxKeyring( | ||
| DefaultCacheKeyringSchema, | ||
| DefaultCacheKeyringCollection, | ||
| cacheFileName, | ||
| DefaultCacheKeyringAttribute1, | ||
| DefaultCacheKeyringAttribute2) | ||
| .Build(); | ||
|
|
||
| var cacheHelper = await MsalCacheHelper.CreateAsync(storageProperties).ConfigureAwait(false); | ||
| #pragma warning disable CS0618 // MsalCacheHelper.Clear is obsolete but is the correct approach for full cache wipe on disconnect | ||
| cacheHelper.Clear(); | ||
|
Comment on lines
+62
to
+63
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My thinking here is that it would be roughly equivalent to the approach of iterating over the results from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. My comment was more about removing only the account being disconnected rather than all accounts, mainly from a UX perspective. That said, if there aren't any issues with clearing all accounts, then this approach works for me.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The CLI does not offer an account parameter, so any user account selection is done during the browser flow, which requires Disconnect-MgGraph to trigger the account selection again, so this feel like the right option. I suppose that there could be the case where a user has closed a terminal and is running Connect-MgGraph again triggering account selection. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Msal cache scope is per window user, so .clear() will only scope to disconnecting user, I believe. |
||
| #pragma warning restore CS0618 | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| using Microsoft.Graph.PowerShell.Authentication; | ||
| using Microsoft.Graph.PowerShell.Authentication.Core.TokenCache; | ||
| using Microsoft.Graph.PowerShell.Authentication.Core.Utilities; | ||
| using System; | ||
| using System.IO; | ||
| using System.Text; | ||
| using System.Threading.Tasks; | ||
| using Xunit; | ||
|
|
||
| namespace Microsoft.Graph.Authentication.Test.TokenCache | ||
| { | ||
| public class TokenCacheUtilitiesTests : IDisposable | ||
| { | ||
| public TokenCacheUtilitiesTests() | ||
| { | ||
| GraphSession.Initialize(() => new GraphSession()); | ||
| GraphSession.Instance.InMemoryTokenCache = new InMemoryTokenCache(); | ||
| } | ||
|
|
||
| public void Dispose() | ||
| { | ||
| GraphSession.Reset(); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task LogoutAsyncShouldClearInMemoryCacheForProcessScope() | ||
| { | ||
| // Arrange | ||
| GraphSession.Instance.InMemoryTokenCache = new InMemoryTokenCache( | ||
| Encoding.UTF8.GetBytes("mockTokenData")); | ||
| GraphSession.Instance.AuthContext = new AuthContext | ||
| { | ||
| AuthType = AuthenticationType.UserProvidedAccessToken, | ||
| ContextScope = ContextScope.Process | ||
| }; | ||
|
|
||
| // Act | ||
| var result = await AuthenticationHelpers.LogoutAsync(); | ||
|
|
||
| // Assert | ||
| Assert.NotNull(result); | ||
| Assert.Equal(AuthenticationType.UserProvidedAccessToken, result.AuthType); | ||
| Assert.Null(GraphSession.Instance.AuthContext); | ||
| Assert.Null(GraphSession.Instance.GraphHttpClient); | ||
| Assert.Empty(GraphSession.Instance.InMemoryTokenCache.ReadTokenData()); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task LogoutAsyncShouldNotThrowWhenAuthContextIsNull() | ||
| { | ||
| // Arrange | ||
| GraphSession.Instance.AuthContext = null; | ||
|
|
||
| // Act - should not throw even though there's no auth context | ||
| var result = await AuthenticationHelpers.LogoutAsync(); | ||
|
|
||
| // Assert | ||
| Assert.Null(result); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task LogoutAsyncShouldAttemptCacheClearForCurrentUserScope() | ||
| { | ||
| // Arrange | ||
| GraphSession.Instance.AuthContext = new AuthContext | ||
| { | ||
| AuthType = AuthenticationType.UserProvidedAccessToken, | ||
| ContextScope = ContextScope.CurrentUser | ||
| }; | ||
|
|
||
| // Act - should not throw even if no persisted cache exists on disk | ||
| var result = await AuthenticationHelpers.LogoutAsync(); | ||
|
|
||
| // Assert | ||
| Assert.NotNull(result); | ||
| Assert.Equal(ContextScope.CurrentUser, result.ContextScope); | ||
| Assert.Null(GraphSession.Instance.AuthContext); | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| Use Disconnect-MgGraph to sign out. | ||
| Use Disconnect-MgGraph to sign out. This clears the persisted MSAL token cache from disk when using CurrentUser context scope, as well as removing the in-memory token cache and authentication record. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on Windows, WAM is on by default for interactive sign-in, and WAM keeps the account + refresh token in its own cache. So cleaning this file won't actually sign the user out of WAM.
For the broker we would need to ask WAM to forget the account. Is the Windows Broker in scope for this fix?
Copilot suggested something like:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JonathanCrd I think we probably need to clear the WAM token cache if possible to ensure a more complete fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, would using this approach avoid the need to have the TokenCacheUtilities class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so, the broker only covers Windows, and there are non-broker auth flows on windows too.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JonathanCrd Sorry, I wasn't very clear here I was more meaning the general approach of using a PCA instance to clear the cached tokens not that specific implementation. Maybe something like this to provide broad cover and eliminate the additional code that knows about the caching specifics
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TokenCacheUtilities -> ClearPersistedTokenCacheAsync still needs to be retained as it deletes the file based caches. PCA code above is needed on top to clear the WAM caches.
Just heads up
WAM is shared broker storage across Windows Azure tools. Removing accounts would also sign out:
Visual Studio
Azure CLI
Azure PowerShell
VS Code Azure extensions
Any app using Azure.Identity with broker enabled
This can impact UX for other logged in flows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@g2vinay does the above code impact all those other apps given that the PCABuilder is specifically targeting the ClientId for this Module?