Azure.Identity adding SharedTokenCacheCredential #7546
Conversation
schaabs
requested review from
AlexGhiondea,
chidozieononiwu,
danieljurek,
mitchdenny and
weshaggard
as code owners
| { | ||
| internal class MsalCacheReader | ||
| { | ||
| private string _cachePath; |
| // Note: this only works on windows if we extend to work on unix systems we need to set FileShare.None | ||
| fileStream = new FileStream(lockfilePath, FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.Read, 4096, FileOptions.DeleteOnClose); | ||
|
|
||
| using (var writer = new StreamWriter(fileStream, Encoding.UTF8, 4096, leaveOpen: true)) |
| { | ||
| // We are using the file locking to synchronize the store, do not allow multiple writers for the file. | ||
| // Note: this only works on windows if we extend to work on unix systems we need to set FileShare.None | ||
| fileStream = new FileStream(lockfilePath, FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.Read, 4096, FileOptions.DeleteOnClose); |
There was a problem hiding this comment.
Shouldn't it be FileMode.Truncate?
There was a problem hiding this comment.
I used the same FileMode as was used in the MSAL extensions library which VS is using.
| /// <summary> | ||
| /// Path to the persisted token cache | ||
| /// </summary> | ||
| public string CacheFilePath { get; set; } = Path.Combine(DefaultCacheDirectory, "msal.cache"); |
There was a problem hiding this comment.
Does it make sense for customers to change this?
| /// <summary> | ||
| /// Millisecond delay between attempts to read the persisted token cache | ||
| /// </summary> | ||
| public TimeSpan CacheAccessRetryDelay { get; set; } = TimeSpan.FromMilliseconds(60000 / 100); |
There was a problem hiding this comment.
TimeSpan.FromMilliseconds(600)?
| } | ||
| break; | ||
| } | ||
| catch (IOException ex) |
There was a problem hiding this comment.
You can do this using the exception filter catch (Exception ex) when (ex is IOException || ex is UnauthorizedAccessException )
| byte[] cacheBytesFromDisk = await ReadCacheFromProtectedStorageAsync().ConfigureAwait(false); | ||
|
|
||
| // update the last read time before deserialization so if deserialization fails we won't continually read the invalid file | ||
| _lastReadTime = DateTimeOffset.UtcNow; |
There was a problem hiding this comment.
If we are comparing to LastWriteTimeUtc in initial check shouldn't we setting _lastReadTime to File.GetLastWriteTimeUtc? Otherwise, we might miss a change that happened in between our check an setting _lastReadTime = DateTimeOffset.UtcNow;
|
Logs and tests would've been nice. Otherwise LGTM |
| @@ -25,7 +25,7 @@ | |||
| <PackageReference Include="System.Text.Json" /> | |||
| <PackageReference Include="System.Threading.Tasks.Extensions" /> | |||
| <PackageReference Include="Microsoft.Identity.Client" /> | |||
|
|
|||
| <PackageReference Include="System.Security.Cryptography.ProtectedData" /> | |||
There was a problem hiding this comment.
Just so you are aware this only works on windows (https://github.com/dotnet/corefx/issues/22510) so if we need it for cross-platform then we will need another solution.
There was a problem hiding this comment.
Yes I'm aware of this. We're only supporting SSO on windows since the only participating App is VS on windows so far. They are possibly going to use a different approach on different platforms so until that's decided the code assumes that we're executing on windows.
| if (!string.IsNullOrEmpty(AzureUsername)) | ||
| // if only the username is specified via the enviornment (not password as well) and we're running on windows add the SharedTokenCacheCredential to the | ||
| // default credential to enable SSO | ||
| if (!string.IsNullOrEmpty(AzureUsername) && string.IsNullOrEmpty(AzurePassword) && (Environment.OSVersion.Platform == PlatformID.Win32NT)) |
There was a problem hiding this comment.
Should SharedTokenCacheCredential also throw on non-windows?
|
/azp run net - client - ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
* SharedTokenCacheCredential initial implementation * minor code formatting / refactoring * adding SharedTokenCacheCredential to the DefaultAzureCredential chain * Addressing PR feedback * only add SharedTokenCacheCredential to DefaultAzureCredential on when running on windows * updating Azure.Identity release notes
No description provided.